A long connect screen cost a game its listing, for ever

migrations/0008_bounded_field_value_index.sql

@@ -0,0 +1,37 @@
+-- The (field, value) index could not hold a connect screen, and a game whose screen was long enough
+-- failed to ingest at all.
+--
+-- Observed on the first crawl large enough to find it: three of four hundred games died with
+-- "54000: index row size 3048 exceeds btree version 4 maximum 2704 for index
+-- game_field_field_value_idx". PostgreSQL's btree cannot index a row wider than about 2704 bytes,
+-- and a connect screen is routinely thousands of characters — the longest in this catalogue is 9,376.
+-- The failure is not partial: the INSERT is refused, so the whole probe's ingestion is lost for that
+-- game, and it is lost again on every future probe, for ever. A game with a generous piece of ASCII
+-- art was permanently unlistable.
+--
+-- The index's stated purpose (0002) is §9's faceted search: which games have CODEBASE = PennMUSH, or
+-- capability.gmcp.measured = true. Every value that purpose looks up is short. NOTHING HAS EVER
+-- SEARCHED BY CONNECT SCREEN and nothing ever will — it is a display asset and a fingerprint, and
+-- the fingerprint has its own column. So the index covers a bounded prefix, which serves the lookups
+-- it was built for and cannot overflow: 256 characters is under the limit even at four bytes each.
+--
+-- The stored value is untouched. Truncating what a game said in order to fit our own index would be
+-- exactly the kind of quiet lossiness this schema refuses everywhere else; it is the *index* that is
+-- bounded, not the fact.
+DROP INDEX IF EXISTS game_field_field_value_idx;
+
+CREATE INDEX game_field_field_value_idx ON game_field (field, left(value, 256));
+
+-- The same flaw, one index over: §7.3's identity lookup folds case and whitespace on both columns,
+-- and folding does not shorten a connect screen. This one is partial rather than prefixed, because
+-- its reader asks an equality question and a prefix would silently turn that into a
+-- starts-with — over-matching where the raw index merely refused. Every identity signal §7.3 names
+-- is short: a name, a year, a hostname, a hash, a token. A value longer than this is not one of
+-- them, so excluding it from the lookup changes no correct answer.
+--
+-- CatalogueDirectories carries the same predicate, or the planner cannot use a partial index.
+DROP INDEX IF EXISTS game_field_folded_value_idx;
+
+CREATE INDEX game_field_folded_value_idx
+    ON game_field (lower(btrim(field)), lower(btrim(value)))
+ WHERE length(value) <= 256;

src/MUI.Crawler/Persistence/CatalogueDirectories.cs

@@ -101,6 +101,14 @@ SELECT DISTINCT game_id
               FROM game_field
              WHERE lower(btrim(field)) = lower(btrim(@field))
                AND lower(btrim(value)) = lower(btrim(@value))
+               -- The bound is here as well as on the index, and both are deliberate. PostgreSQL's
+               -- btree cannot hold a row past ~2704 bytes, and a connect screen is thousands of
+               -- characters, so an unbounded index refused the INSERT and cost the game its whole
+               -- ingestion. The index is now partial; repeating its predicate here is what lets the
+               -- planner use it rather than sequentially scanning game_field once per identity
+               -- signal per probe. It changes no answer: every §7.3 signal — a name, a year, a
+               -- hostname, a hash, a token — is short, and a value longer than this is not one.
+               AND length(value) <= 256
             """,
             new { field, value },
             cancellationToken: ct));

tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs

@@ -0,0 +1,127 @@
+using Dapper;
+
+using MUI.Catalog.Persistence;
+using MUI.Catalog.Tests.Persistence.Support;
+
+namespace MUI.Catalog.Tests.Persistence;
+
+/// <summary>
+/// A field value too large for an index must not cost a game its listing.
+/// </summary>
+/// <remarks>
+/// Found on the first crawl big enough to find it: three games of four hundred died with
+/// <c>54000: index row size … exceeds btree version 4 maximum 2704</c>, because a connect screen is
+/// routinely thousands of characters and the <c>(field, value)</c> index tried to hold one. The
+/// failure is total rather than partial — the insert is refused, the whole probe's ingestion is lost,
+/// and it is lost again on every future probe. A game with a generous piece of ASCII art was
+/// permanently unlistable, and nothing said so.
+/// </remarks>
+public class OversizedFieldValueTests
+{
+    private static readonly DateTimeOffset Now = Seed.Now;
+
+    /// <summary>The real shape: a connect screen far past the btree limit, stored whole.</summary>
+    [Test]
+    public async Task AConnectScreenTooLargeToIndexIsStillStored()
+    {
+        await using var db = await PostgresFixture.MigratedAsync();
+        var game = await Seed.GameAsync(db);
+        var store = new NpgsqlGameFieldStore(db.DataSource);
+
+        // Longer than the longest observed in the wild (9,376 characters) and several times the
+        // index limit, so this fails against the old index and passes against the bounded one.
+        var screen = string.Join('\n', Enumerable.Repeat(new string('=', 78), 160));
+
+        await store.UpsertAsync(new GameField(
+            game, InternalFields.ConnectScreen, FieldSource.Banner, screen, Now, Now));
+
+        var stored = (await store.ForGameAsync(game))
+            .Single(f => f.Field == InternalFields.ConnectScreen);
+
+        // Stored whole. It is the index that is bounded, never the fact — truncating what a game
+        // sent in order to fit our own index is the kind of quiet lossiness this schema refuses.
+        await Assert.That(stored.Value).IsEqualTo(screen);
+        await Assert.That(stored.Value.Length).IsGreaterThan(2704);
+    }
+
+    /// <summary>
+    /// Two long values that differ only past the indexed prefix are still two distinct rows.
+    /// </summary>
+    /// <remarks>
+    /// The prefix is an index, not a key. If bounding it had collapsed rows that share their first
+    /// 256 characters — which two connect screens from one codebase easily do — the fix would have
+    /// traded a loud failure for a silent one.
+    /// </remarks>
+    [Test]
+    public async Task TwoValuesSharingTheirFirstBytesRemainDistinct()
+    {
+        await using var db = await PostgresFixture.MigratedAsync();
+        var one = await Seed.GameAsync(db, slug: "one", name: "One");
+        var two = await Seed.GameAsync(db, slug: "two", name: "Two");
+        var store = new NpgsqlGameFieldStore(db.DataSource);
+
+        var shared = new string('#', 4000);
+
+        await store.UpsertAsync(new GameField(
+            one, InternalFields.ConnectScreen, FieldSource.Banner, shared + "ONE", Now, Now));
+        await store.UpsertAsync(new GameField(
+            two, InternalFields.ConnectScreen, FieldSource.Banner, shared + "TWO", Now, Now));
+
+        var first = (await store.ForGameAsync(one)).Single(f => f.Field == InternalFields.ConnectScreen);
+        var second = (await store.ForGameAsync(two)).Single(f => f.Field == InternalFields.ConnectScreen);
+
+        await Assert.That(first.Value).EndsWith("ONE");
+        await Assert.That(second.Value).EndsWith("TWO");
+    }
+
+    /// <summary>
+    /// Both indexes over this table are bounded, because both could refuse a connect screen.
+    /// </summary>
+    /// <remarks>
+    /// The first fix caught only one of them and the very next probe failed on the other — so this
+    /// asserts the property over every index on <c>game_field</c> rather than over the one that was
+    /// noticed. An index on a raw or merely case-folded value is the shape of the bug: folding does
+    /// not shorten anything.
+    /// </remarks>
+    [Test]
+    public async Task NoIndexOnThisTableCanRefuseALongValue()
+    {
+        await using var db = await PostgresFixture.MigratedAsync();
+
+        await using var connection = await db.DataSource.OpenConnectionAsync();
+
+        var definitions = (await connection.QueryAsync<string>(
+            "SELECT indexdef FROM pg_indexes WHERE tablename = 'game_field'")).ToList();
+
+        foreach (var definition in definitions.Where(d => d.Contains("value", StringComparison.Ordinal)))
+        {
+            // Either the indexed expression is bounded, or the index only covers rows short enough.
+            var bounded = definition.Contains("256", StringComparison.Ordinal);
+
+            await Assert.That(bounded)
+                .IsTrue()
+                .Because($"an unbounded index over `value` refuses a connect screen: {definition}");
+        }
+    }
+
+    /// <summary>The index still exists and still leads on the field, which is what it is for.</summary>
+    [Test]
+    public async Task TheFacetLookupIsStillIndexed()
+    {
+        await using var db = await PostgresFixture.MigratedAsync();
+
+        await using var connection = await db.DataSource.OpenConnectionAsync();
+
+        var definition = await connection.ExecuteScalarAsync<string>(
+            "SELECT indexdef FROM pg_indexes WHERE indexname = 'game_field_field_value_idx'");
+
+        await Assert.That(definition).IsNotNull();
+        await Assert.That(definition!).Contains("field");
+
+        // Asserted on the bound rather than the spelling: PostgreSQL reports the expression back as
+        // "left"(value, 256), quoted, and a test that matched the source text would break on a
+        // formatting difference while saying nothing about whether the index can overflow.
+        await Assert.That(definition!).Contains("256");
+        await Assert.That(definition!.Contains("(field, value)", StringComparison.Ordinal)).IsFalse();
+    }
+}