diff --git a/app/MindWork AI Studio/Tools/HTMLParser.cs b/app/MindWork AI Studio/Tools/HTMLParser.cs index a5095830..9e2ba1ad 100644 --- a/app/MindWork AI Studio/Tools/HTMLParser.cs +++ b/app/MindWork AI Studio/Tools/HTMLParser.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Net; using System.Net.Http.Headers; using System.Net.Sockets; @@ -14,18 +15,39 @@ public sealed class HTMLParser private const int DEFAULT_MAX_RESPONSE_BYTES = 5 * 1024 * 1024; /// - /// The HTML to Markdown converter, built once from a fixed configuration. + /// The fixed configuration every HTML to Markdown conversion runs with. /// /// - /// Shared rather than built per call: the configuration never changes, and one web search - /// converts a page per result. + /// This one is shared, because it is only ever read: a configuration holds no counters and no + /// collections which get written to. The converters reading it are not shared, see the pool + /// below. /// - private static readonly Converter MARKDOWN_CONVERTER = new(new Config + private static readonly Config MARKDOWN_CONFIG = new() { UnknownTags = Config.UnknownTagsOption.Bypass, RemoveComments = true, SmartHrefHandling = true, - }); + }; + + /// + /// The converters not currently in use, kept so that the reflection in their constructor does + /// not run for every page. + /// + /// + /// One converter per conversion rather than one for all of them: a converter tracks the + /// ancestors of the node it is at in state of its own, updates that state at every single node, + /// and does so without any synchronization. A web search converts up to four pages at the same + /// time, which let those conversions tear each other's ancestor lists apart — sometimes loudly, + /// as an index outside the bounds of an array, and sometimes quietly, as a list indented by the + /// depth another page happened to be at.

+ /// Which converter gets which page does not matter, so the pool needs no key: that ancestor + /// state is entered and left in pairs around every node, which leaves it empty once a + /// conversion returns. Nothing of a page outlives its own conversion. A key would, in fact, do + /// harm — two conversions of the same page at the same time would share one converter again. + ///

+ /// The pool holds no more converters than are ever converting at once, which is a handful. + /// + private static readonly ConcurrentBag CONVERTER_POOL = []; /// /// Loads a web page. @@ -238,5 +260,21 @@ public sealed class HTMLParser /// /// The HTML content to parse. /// The converted Markdown content. - public static string ParseToMarkdown(string html) => MARKDOWN_CONVERTER.Convert(html); + /// + /// The converter returns to the pool only after it converted without throwing, and that is + /// deliberately not done in a finally block: a conversion which throws leaves the ancestors it + /// entered behind, because the library does not unwind them itself. Such a converter would + /// count those ancestors into every page it is handed afterwards, so it is left to the garbage + /// collector rather than passed on. + /// + public static string ParseToMarkdown(string html) + { + if (!CONVERTER_POOL.TryTake(out var converter)) + converter = new Converter(MARKDOWN_CONFIG); + + var markdown = converter.Convert(html); + + CONVERTER_POOL.Add(converter); + return markdown; + } } \ No newline at end of file diff --git a/app/MindWork AI Studio/Tools/ToolCallingSystem/ToolCallingImplementations/WebSearch/WebSearchResultRetrievalService.cs b/app/MindWork AI Studio/Tools/ToolCallingSystem/ToolCallingImplementations/WebSearch/WebSearchResultRetrievalService.cs index c3a9f0ed..a8aab398 100644 --- a/app/MindWork AI Studio/Tools/ToolCallingSystem/ToolCallingImplementations/WebSearch/WebSearchResultRetrievalService.cs +++ b/app/MindWork AI Studio/Tools/ToolCallingSystem/ToolCallingImplementations/WebSearch/WebSearchResultRetrievalService.cs @@ -4,6 +4,8 @@ namespace AIStudio.Tools.ToolCallingSystem.ToolCallingImplementations.WebSearch; internal sealed class WebSearchResultRetrievalService(WebPageRetrievalService webPageRetrievalService) { + private static readonly ILogger LOGGER = Program.LOGGER_FACTORY.CreateLogger(); + private const int MAX_PARALLEL_RETRIEVALS = 4; /// @@ -110,9 +112,16 @@ internal sealed class WebSearchResultRetrievalService(WebPageRetrievalService we Interlocked.Increment(ref counters.PageTimedOut); return new(candidate, null, WebSearchPageRetrievalOutcome.PAGE_TIMED_OUT); } - catch (InvalidOperationException) + catch (InvalidOperationException exception) { + // + // The only outcome here which is not an expected one: a page was blocked on purpose, + // and a timeout is a limit the user set, but this is something going wrong. It is + // logged rather than only counted, because a search which quietly returns one result + // fewer is a search nobody can tell was incomplete. + // Interlocked.Increment(ref counters.Failed); + LOGGER.LogError(exception, "Reading a search result page failed. Url={Url}", candidate.RetrievalUrl); return new(candidate, null, WebSearchPageRetrievalOutcome.FAILED); } finally diff --git a/app/MindWork AI Studio/Tools/Web/WebPageContentExtractor.cs b/app/MindWork AI Studio/Tools/Web/WebPageContentExtractor.cs index 2576f11c..c4ff6342 100644 --- a/app/MindWork AI Studio/Tools/Web/WebPageContentExtractor.cs +++ b/app/MindWork AI Studio/Tools/Web/WebPageContentExtractor.cs @@ -76,7 +76,7 @@ internal static class WebPageContentExtractor .Select(x => LimitLength(x, MAX_OUTLINE_ITEM_CHARACTERS)) .Distinct(StringComparer.Ordinal) .ToList(); - var markdown = HTMLParser.ParseToMarkdown(contentRoot.InnerHtml) + var markdown = ConvertToMarkdown(contentRoot.InnerHtml, finalUrl) .Replace("\r\n", "\n", StringComparison.Ordinal) .Replace('\r', '\n') .Trim(); @@ -141,6 +141,30 @@ internal static class WebPageContentExtractor }; } + /// + /// Converts the readable part of the page to Markdown. + /// + /// + /// Only the call into the Markdown library is wrapped, not the extraction around it: a fault of + /// our own has to keep surfacing as what it is, instead of being filed away as an unreadable + /// page.

+ /// What the library throws depends on the HTML it was handed, and it says nothing beyond "this + /// page could not be converted". Reported as an InvalidOperationException, the retrieval treats + /// it like any other page it could not read, which costs this one page rather than the whole + /// search it belongs to. + /// + private static string ConvertToMarkdown(string html, Uri finalUrl) + { + try + { + return HTMLParser.ParseToMarkdown(html); + } + catch (Exception exception) when (exception is not OperationCanceledException) + { + throw new InvalidOperationException($"Converting the HTML of '{finalUrl}' to Markdown failed: {exception.Message}", exception); + } + } + private static JsonLdMetadata ExtractJsonLdMetadata(HtmlDocument document, Uri finalUrl) { JsonLdCandidate? bestCandidate = null; diff --git a/app/Tests/Tools/HTMLParserConcurrencyTests.cs b/app/Tests/Tools/HTMLParserConcurrencyTests.cs new file mode 100644 index 00000000..86600910 --- /dev/null +++ b/app/Tests/Tools/HTMLParserConcurrencyTests.cs @@ -0,0 +1,136 @@ +using System.Collections.Concurrent; +using AIStudio.Tools; + +namespace AIStudio.Tests.Tools; + +/// +/// Checks that converting several pages to Markdown at the same time keeps them apart. +/// +/// +/// A web search reads up to four result pages in parallel, and every one of them is converted +/// through the same entry point. The converter doing that work tracks the ancestors of the node it +/// is at, and it does so without any synchronization, so sharing one converter between those +/// conversions let them write into each other's ancestor lists.

+/// That went wrong in two ways, and this test covers both. Loudly, as a torn list throwing an index +/// out of range — which is what showed up in the logs. And quietly, as a list indented by the depth +/// a different page happened to be at, which nothing reports and which only a comparison against a +/// known-good conversion catches. +/// +[TestFixture] +public sealed class HTMLParserConcurrencyTests +{ + private const int THREAD_COUNT = 8; + private const int CONVERSIONS_PER_THREAD = 40; + + [Test] + public void ParallelConversionsDoNotInterfereWithEachOther() + { + var html = BuildPageHtml(); + + // Converted alone, with nothing else running, this is what the page has to come back as: + var expected = HTMLParser.ParseToMarkdown(html); + + var results = new ConcurrentBag(); + var failures = new ConcurrentBag(); + + // + // Real threads released by a barrier rather than a parallel loop: the conversions have to + // overlap for this test to mean anything, and only starting them together makes that + // certain. + // + // What a thread works with is handed over when it starts rather than captured. The barrier + // is disposed at the end of this method, and while the joins below make sure no thread is + // still at it by then, that is nothing one can see from inside a lambda. + // + using var startSignal = new Barrier(THREAD_COUNT); + var threads = new List(THREAD_COUNT); + for (var threadIndex = 0; threadIndex < THREAD_COUNT; threadIndex++) + { + var thread = new Thread(ConvertRepeatedly); + thread.Start(new ConversionRun(startSignal, html, results, failures)); + threads.Add(thread); + } + + foreach (var thread in threads) + thread.Join(); + + var failureKinds = string.Join(", ", failures.Select(x => x.GetType().Name).Distinct(StringComparer.Ordinal)); + var deviatingCount = results.Count(x => !string.Equals(x, expected, StringComparison.Ordinal)); + + Assert.Multiple(() => + { + Assert.That(failures, Is.Empty, $"Converting in parallel threw {failures.Count} times ({failureKinds}). A conversion must not depend on what another thread is converting."); + Assert.That(deviatingCount, Is.Zero, $"{deviatingCount} of {results.Count} conversions came back different from the same page converted on its own. Their indentation was counted from ancestors belonging to another conversion."); + }); + } + + /// + /// Converts the same page over and over, once every thread has arrived at the barrier. + /// + private static void ConvertRepeatedly(object? state) + { + var run = (ConversionRun)state!; + run.StartSignal.SignalAndWait(); + + for (var conversion = 0; conversion < CONVERSIONS_PER_THREAD; conversion++) + { + try + { + run.Results.Add(HTMLParser.ParseToMarkdown(run.Html)); + } + catch (Exception exception) + { + run.Failures.Add(exception); + } + } + } + + /// + /// Builds a page out of the elements the reported stack traces named. + /// + /// + /// The nested lists are what makes this sharp: their indentation is computed from the ancestors + /// the converter is tracking, so a conversion which picked up somebody else's ancestors comes + /// back indented differently rather than failing outright. The block is repeated so that the + /// conversions take long enough to actually overlap. + /// + private static string BuildPageHtml() + { + const string BLOCK = + """ +
+

An introduction to the topic at hand.

+
    +
  1. First item +
      +
    • Nested item +
        +
      1. Deeply nested item
        2. +
      2. Another one +
        • And one level deeper still
        +
        3. +
      +
      • +
    +
    2. +
  2. Second item
    3. +
+ + + + + + +
Column AColumn B

A cell holding a paragraph.

  • A cell holding a list
  • with two entries
+

A closing paragraph with bold and emphasized text.

+
+ """; + + return string.Concat(Enumerable.Repeat(BLOCK, 20)); + } + + /// + /// Everything one thread of this test needs, so that it is passed rather than captured. + /// + private sealed record ConversionRun(Barrier StartSignal, string Html, ConcurrentBag Results, ConcurrentBag Failures); +} \ No newline at end of file