Fixed web searches failing when several pages were read at once (#972)

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;
 
     /// <summary>
-    /// The HTML to Markdown converter, built once from a fixed configuration.
+    /// The fixed configuration every HTML to Markdown conversion runs with.
     /// </summary>
     /// <remarks>
-    /// Shared rather than built per call: the configuration never changes, and one web search
+    /// This one is shared, because it is only ever read: a configuration holds no counters and no
-    /// converts a page per result.
+    /// collections which get written to. The converters reading it are not shared, see the pool
+    /// below.
     /// </remarks>
-    private static readonly Converter MARKDOWN_CONVERTER = new(new Config
+    private static readonly Config MARKDOWN_CONFIG = new()
     {
         UnknownTags = Config.UnknownTagsOption.Bypass,
         RemoveComments = true,
         SmartHrefHandling = true,
-    });
+    };
+
+    /// <summary>
+    /// The converters not currently in use, kept so that the reflection in their constructor does
+    /// not run for every page.
+    /// </summary>
+    /// <remarks>
+    /// 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.<br/><br/>
+    /// 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.
+    /// <br/><br/>
+    /// The pool holds no more converters than are ever converting at once, which is a handful.
+    /// </remarks>
+    private static readonly ConcurrentBag<Converter> CONVERTER_POOL = [];
 
     /// <summary>
     /// Loads a web page.
@@ -238,5 +260,21 @@ public sealed class HTMLParser
     /// </summary>
     /// <param name="html">The HTML content to parse.</param>
     /// <returns>The converted Markdown content.</returns>
-    public static string ParseToMarkdown(string html) => MARKDOWN_CONVERTER.Convert(html);
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    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;
+    }
 }

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<WebSearchResultRetrievalService> LOGGER = Program.LOGGER_FACTORY.CreateLogger<WebSearchResultRetrievalService>();
+
     private const int MAX_PARALLEL_RETRIEVALS = 4;
 
     /// <summary>
@@ -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

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
         };
     }
 
+    /// <summary>
+    /// Converts the readable part of the page to Markdown.
+    /// </summary>
+    /// <remarks>
+    /// 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.<br/><br/>
+    /// 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.
+    /// </remarks>
+    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;

app/Tests/Tools/HTMLParserConcurrencyTests.cs

@@ -0,0 +1,136 @@
+using System.Collections.Concurrent;
+using AIStudio.Tools;
+
+namespace AIStudio.Tests.Tools;
+
+/// <summary>
+/// Checks that converting several pages to Markdown at the same time keeps them apart.
+/// </summary>
+/// <remarks>
+/// 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.<br/><br/>
+/// 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.
+/// </remarks>
+[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<string>();
+        var failures = new ConcurrentBag<Exception>();
+
+        //
+        // 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>(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.");
+        });
+    }
+
+    /// <summary>
+    /// Converts the same page over and over, once every thread has arrived at the barrier.
+    /// </summary>
+    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);
+            }
+        }
+    }
+
+    /// <summary>
+    /// Builds a page out of the elements the reported stack traces named.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    private static string BuildPageHtml()
+    {
+        const string BLOCK =
+            """
+            <div>
+              <p>An introduction to the topic at hand.</p>
+              <ol>
+                <li>First item
+                  <ul>
+                    <li>Nested item
+                      <ol>
+                        <li>Deeply nested item</li>
+                        <li>Another one
+                          <ul><li>And one level deeper still</li></ul>
+                        </li>
+                      </ol>
+                    </li>
+                  </ul>
+                </li>
+                <li>Second item</li>
+              </ol>
+              <table>
+                <tr><th>Column A</th><th>Column B</th></tr>
+                <tr>
+                  <td><div><p>A cell holding a paragraph.</p></div></td>
+                  <td><ul><li>A cell holding a list</li><li>with two entries</li></ul></td>
+                </tr>
+              </table>
+              <p>A closing paragraph with <strong>bold</strong> and <em>emphasized</em> text.</p>
+            </div>
+            """;
+
+        return string.Concat(Enumerable.Repeat(BLOCK, 20));
+    }
+
+    /// <summary>
+    /// Everything one thread of this test needs, so that it is passed rather than captured.
+    /// </summary>
+    private sealed record ConversionRun(Barrier StartSignal, string Html, ConcurrentBag<string> Results, ConcurrentBag<Exception> Failures);
+}