A long connect screen cost a game its listing, for ever (#19)

Found by the first crawl large enough to find it: seeding 701 addresses from the
directories, three games of four hundred died with

  54000: index row size 3048 exceeds btree version 4 maximum 2704
         for index "game_field_field_value_idx"

PostgreSQL cannot index a btree row past about 2704 bytes, and a connect screen
is routinely thousands of characters -- the longest in this catalogue is 9,376.
The failure is total rather than 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. A game with a generous piece of ASCII art was permanently unlistable and
nothing said so.

WHY THREE AND NOT FIFTY-ONE, WHICH IS HOW MANY ROWS ARE NOW OVER THAT SIZE.
Index tuples cannot be stored out of line but they can be compressed, so whether
a game was listable depended on how well its ASCII art compressed. That is why
it presented as three unrelated failures rather than as a rule, and it is why
looking at the length of the longest value would not have predicted it.

Both indexes over game_field had the flaw, and the first fix caught only one --
the very next probe failed on the other. They are bounded differently because
they are read differently:

  - game_field_field_value_idx serves §9's faceted search and is indexed on a
    256-character prefix. Nothing has ever searched by connect screen and nothing
    will; it is a display asset, and its fingerprint has its own column.

  - game_field_folded_value_idx serves §7.3's identity lookup, which asks an
    equality question. A prefix there would silently turn that into a
    starts-with -- over-matching where the raw index merely refused -- so it is
    partial instead, and CatalogueDirectories carries the same predicate or the
    planner cannot use it. Every identity signal §7.3 names is short: a name, a
    year, a hostname, a hash, a token. A value longer than that is not one.

THE STORED VALUE IS UNTOUCHED. Truncating what a game sent in order to fit our
own index would be the quiet lossiness this schema refuses everywhere else. It is
the index that is bounded, never the fact.

The test asserts the property over EVERY index on game_field rather than over the
one that was noticed, which is the mistake the first pass made.

Verified against the live catalogue: the games that had been failing re-probed
clean, and the one that raised the original error now holds its 7,535-character
connect screen.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: HarryCordewener

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();
+    }
+}