diff --git a/migrations/0008_bounded_field_value_index.sql b/migrations/0008_bounded_field_value_index.sql new file mode 100644 index 0000000..a12d538 --- /dev/null +++ b/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; diff --git a/migrations/0010_submitted_games.sql b/migrations/0010_submitted_games.sql new file mode 100644 index 0000000..7dfed18 --- /dev/null +++ b/migrations/0010_submitted_games.sql @@ -0,0 +1,48 @@ +-- spec §8 — a signed-in operator may add their own game, and it stays out of the listing until they +-- have proved they run it. +-- +-- ONE NULLABLE COLUMN, AND IT IS NOT A LIFECYCLE STATE. `game.state` is derived from availability +-- history and never set by hand (0001), so a submission cannot live there: it is a different axis +-- entirely — how a game reached us, rather than how it has been behaving. Two axes in one column +-- would mean a submitted game that goes dark has nowhere to say both things. +-- +-- The listing rule is one sentence and reads off this column alone: +-- +-- a game is public if nobody submitted it, or if it has been claimed. +-- +-- Which keeps §7.1's auto-listing exactly as it was — anything the crawler found for itself is +-- listed immediately as discovered-and-unclaimed — while a stranger's assertion that some address is +-- a game waits for that stranger to prove they run it. +-- +-- WHY THE ASYMMETRY IS NOT ARBITRARY. Discovery is something we did: a referral we walked and a +-- resolved-address gate we applied (§7.2). A submission is somebody else pointing us at a host. If a +-- submitted game listed on sight, the form would be a way to put any address on a public page under +-- any description, and the answer to "who says this is a game?" would be "a stranger". Requiring the +-- claim makes the form mean *add your own game*, not *add somebody's*. +-- +-- THIS IS NOT THE MODERATION QUEUE §3 CONDEMNS, AND THE DIFFERENCE MUST BE KEPT. The incumbents' +-- queues waited on a human at their end, which is why listings sat unapproved for a year. This waits +-- on the submitter, is settled by one probe, and has nobody in the middle. If a screen is ever added +-- where staff approve submissions, that distinction is gone and so is the argument for the feature. +ALTER TABLE game + ADD COLUMN submitted_by uuid REFERENCES app_user (id); + +-- The listing filters on this on every read, and the overwhelming majority of rows are NULL, so it +-- is worth an index only over the rows that are not. +CREATE INDEX game_submitted_by_idx ON game (submitted_by) WHERE submitted_by IS NOT NULL; + +-- How many submissions one account may make in a day (§8). Recorded as a table rather than a +-- column so the bound is auditable: a burst of submissions from one account is a thing somebody +-- will want to look at, and a counter that only counts would not say what happened. +CREATE TABLE game_submission ( + id uuid PRIMARY KEY, + user_id uuid NOT NULL REFERENCES app_user (id), + game_id uuid NOT NULL REFERENCES game (id), + host text NOT NULL, + port integer NOT NULL, + submitted_at timestamptz NOT NULL, + + CONSTRAINT game_submission_port_is_a_port CHECK (port >= 1 AND port <= 65535) +); + +CREATE INDEX game_submission_user_idx ON game_submission (user_id, submitted_at DESC); diff --git a/src/MUI.Catalog/Persistence/NpgsqlGameQueries.cs b/src/MUI.Catalog/Persistence/NpgsqlGameQueries.cs index 59a1ab9..5ce420f 100644 --- a/src/MUI.Catalog/Persistence/NpgsqlGameQueries.cs +++ b/src/MUI.Catalog/Persistence/NpgsqlGameQueries.cs @@ -31,6 +31,29 @@ namespace MUI.Catalog.Persistence; public sealed class NpgsqlGameQueries(NpgsqlDataSource source, IFieldRegistry? registry = null) : IGameQueries { + /// + /// The rule that keeps an unclaimed submission off every public surface (spec §8, migration 0010). + /// + /// + /// + /// A game is public if nobody submitted it, or if it has been claimed. Anything the + /// crawler found for itself is listed on sight exactly as §7.1 says; a stranger's assertion that + /// some address is a game waits until that stranger proves they run it. + /// + /// + /// It is one constant because it has to hold on every read — the listing, the search, + /// the three feeds, the ecosystem shares, the rankings, and both lookups. A predicate written + /// out per query is a predicate that will be forgotten on the next query somebody adds, and the + /// failure mode is a game on a public page that nobody vouched for. The test that enumerates + /// IGameQueries and asserts an unclaimed submission appears in none of it is the real + /// guard; this constant is what makes passing it easy. + /// + /// + private const string Public = "(submitted_by IS NULL OR is_claimed)"; + + /// The same rule where the table is aliased. + private const string PublicG = "(g.submitted_by IS NULL OR g.is_claimed)"; + /// The heatmap's window (spec §5.2). public static readonly TimeSpan ActivityWindow = TimeSpan.FromDays(56); @@ -119,11 +142,12 @@ public async Task SearchAsync( var includeArchived = filter.IncludeArchived || filter.Band is ActivityBand.Archived; var rows = (await connection.QueryAsync(new CommandDefinition( - """ + $""" SELECT g.id AS Id, g.slug AS Slug, g.name AS Name, g.tagline AS Tagline, g.state AS State, g.is_claimed AS IsClaimed, g.last_reachable_at AS LastReachableAt FROM game g WHERE (@includeArchived OR g.state <> 'archived') + AND {PublicG} ORDER BY g.name """, new { includeArchived }, @@ -229,11 +253,11 @@ FROM game_endpoint await using var connection = await source.OpenConnectionAsync(cancellationToken); var row = await connection.QuerySingleOrDefaultAsync(new CommandDefinition( - """ + $""" SELECT id AS Id, slug AS Slug, name AS Name, tagline AS Tagline, state AS State, is_claimed AS IsClaimed, last_reachable_at AS LastReachableAt FROM game - WHERE id = @id + WHERE id = @id AND {Public} """, new { id }, cancellationToken: cancellationToken)); @@ -268,11 +292,11 @@ FROM game await using var connection = await source.OpenConnectionAsync(cancellationToken); var row = await connection.QuerySingleOrDefaultAsync(new CommandDefinition( - """ + $""" SELECT id AS Id, slug AS Slug, name AS Name, tagline AS Tagline, state AS State, is_claimed AS IsClaimed, last_reachable_at AS LastReachableAt FROM game - WHERE slug = @slug + WHERE slug = @slug AND {Public} """, new { slug }, cancellationToken: cancellationToken)); @@ -333,10 +357,10 @@ public async Task FeedsAsync(CancellationToken cancellationToken // §9's three liveness feeds — the differentiator no incumbent can publish, because none of // them measured continuously enough to know when a game came back. var discovered = await connection.QueryAsync(new CommandDefinition( - """ + $""" SELECT slug AS Slug, name AS Name, first_seen_at AS At, NULL AS Cause FROM game - WHERE first_seen_at >= @since + WHERE first_seen_at >= @since AND {Public} ORDER BY first_seen_at DESC LIMIT @limit """, @@ -411,9 +435,9 @@ public async Task EcosystemAsync(CancellationToken cancellat await using var connection = await source.OpenConnectionAsync(cancellationToken); var totals = await connection.QuerySingleAsync(new CommandDefinition( - """ + $""" SELECT - (SELECT count(*)::int FROM game WHERE state <> 'archived') AS Listed, + (SELECT count(*)::int FROM game WHERE state <> 'archived' AND {Public}) AS Listed, -- A completed session, which is what a measured capability is a capability of. (SELECT count(DISTINCT a.game_id)::int @@ -507,7 +531,7 @@ public async Task RankingsAsync(CancellationToken cancellationToken = await using var connection = await source.OpenConnectionAsync(cancellationToken); var listed = await connection.ExecuteScalarAsync(new CommandDefinition( - "SELECT count(*)::int FROM game WHERE state <> 'archived'", + $"SELECT count(*)::int FROM game WHERE state <> 'archived' AND {Public}", cancellationToken: cancellationToken)); var busiest = (await connection.QueryAsync(new CommandDefinition( diff --git a/src/MUI.Crawler/Persistence/CatalogueDirectories.cs b/src/MUI.Crawler/Persistence/CatalogueDirectories.cs index d668463..aaa69c5 100644 --- a/src/MUI.Crawler/Persistence/CatalogueDirectories.cs +++ b/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)); diff --git a/tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs b/tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs new file mode 100644 index 0000000..d0fb497 --- /dev/null +++ b/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; + +/// +/// A field value too large for an index must not cost a game its listing. +/// +/// +/// Found on the first crawl big enough to find it: three games of four hundred died with +/// 54000: index row size … exceeds btree version 4 maximum 2704, because a connect screen is +/// routinely thousands of characters and the (field, value) 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. +/// +public class OversizedFieldValueTests +{ + private static readonly DateTimeOffset Now = Seed.Now; + + /// The real shape: a connect screen far past the btree limit, stored whole. + [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); + } + + /// + /// Two long values that differ only past the indexed prefix are still two distinct rows. + /// + /// + /// 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. + /// + [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"); + } + + /// + /// Both indexes over this table are bounded, because both could refuse a connect screen. + /// + /// + /// 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 game_field 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. + /// + [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( + "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}"); + } + } + + /// The index still exists and still leads on the field, which is what it is for. + [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( + "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(); + } +} diff --git a/tests/MUI.Catalog.Tests/Persistence/SubmittedGameTests.cs b/tests/MUI.Catalog.Tests/Persistence/SubmittedGameTests.cs new file mode 100644 index 0000000..5d6b1dc --- /dev/null +++ b/tests/MUI.Catalog.Tests/Persistence/SubmittedGameTests.cs @@ -0,0 +1,155 @@ +using Dapper; + +using MUI.Catalog.Persistence; +using MUI.Catalog.Tests.Persistence.Support; + +namespace MUI.Catalog.Tests.Persistence; + +/// +/// A submitted game stays off every public surface until it is claimed (spec §8, migration 0010). +/// +/// +/// +/// The rule is one sentence — a game is public if nobody submitted it, or if it has been +/// claimed — and the danger is not the rule, it is the number of places it has to hold. The +/// listing, the faceted search, the three liveness feeds, the ecosystem shares, the rankings, the +/// by-slug lookup and the by-id lookup are seven surfaces, and the failure mode of forgetting one is +/// a game on a public page that nobody vouched for. +/// +/// +/// So the test does not check the surfaces somebody remembered. It walks +/// by reflection and requires every member to be covered here — a method added later fails this +/// until it is either filtered or explicitly declared to need no filtering. +/// +/// +public class SubmittedGameTests +{ + private static readonly DateTimeOffset Now = Seed.Now; + + [Test] + public async Task AnUnclaimedSubmissionIsOnNoPublicSurface() + { + await using var db = await PostgresFixture.MigratedAsync(); + var queries = new NpgsqlGameQueries(db.DataSource); + + var found = await Seed.GameAsync(db, slug: "found", name: "Found By Us"); + var submitted = await SubmittedAsync(db, slug: "submitted", name: "Somebody Said So"); + + var listed = await queries.ListAsync(new GameFilter { IncludeArchived = true }); + var searched = await queries.SearchAsync(new GameFilter { IncludeArchived = true }); + var feeds = await queries.FeedsAsync(); + + await Assert.That(listed.Select(g => g.Id)).Contains(found); + await Assert.That(listed.Select(g => g.Id)).DoesNotContain(submitted); + await Assert.That(searched.Games.Select(g => g.Id)).DoesNotContain(submitted); + + // A submission is not "newly discovered" — nothing discovered it. + await Assert.That(feeds.NewlyDiscovered.Select(e => e.Slug)).DoesNotContain("submitted"); + + // Not reachable by guessing its address either, on either lookup. + await Assert.That(await queries.FindAsync("submitted")).IsNull(); + await Assert.That(await queries.FindByIdAsync(submitted)).IsNull(); + + // And not in the denominator of any published share, which would let its existence be + // inferred from arithmetic even while its page is hidden. + var ecosystem = await queries.EcosystemAsync(); + var everything = await queries.ListAsync(new GameFilter { IncludeArchived = true }); + + await Assert.That(ecosystem.ListedGames).IsEqualTo(everything.Count); + } + + /// Claiming it lists it, by the same rule and with nothing else changed. + [Test] + public async Task ClaimingASubmissionListsIt() + { + await using var db = await PostgresFixture.MigratedAsync(); + var queries = new NpgsqlGameQueries(db.DataSource); + var submitted = await SubmittedAsync(db, slug: "submitted", name: "Somebody Said So"); + + await Assert.That(await queries.FindAsync("submitted")).IsNull(); + + await new NpgsqlGameStore(db.DataSource).SetClaimedAsync(submitted, true); + + await Assert.That(await queries.FindAsync("submitted")).IsNotNull(); + await Assert.That((await queries.ListAsync(new GameFilter())).Select(g => g.Id)) + .Contains(submitted); + } + + /// + /// A game the crawler found for itself is listed on sight, claimed or not. + /// + /// + /// §7.1's auto-listing is the feature this must not break. If the new column had been read as + /// "unclaimed games are hidden" rather than "unclaimed submissions are hidden", the + /// whole catalogue would have vanished — 409 of 409 games here are unclaimed. + /// + [Test] + public async Task AnUnclaimedGameTheCrawlerFoundIsStillListed() + { + await using var db = await PostgresFixture.MigratedAsync(); + var queries = new NpgsqlGameQueries(db.DataSource); + var found = await Seed.GameAsync(db, slug: "found", name: "Found By Us"); + + await Assert.That((await queries.ListAsync(new GameFilter())).Select(g => g.Id)) + .Contains(found); + await Assert.That(await queries.FindAsync("found")).IsNotNull(); + } + + /// + /// Every read on is covered above, by name. + /// + /// + /// The list is written out so that adding a query fails this test rather than silently shipping + /// an eighth surface nobody filtered. If a new member genuinely needs no filtering, add it here + /// with the reason — the point is that the decision is made, not that the list is long. + /// + [Test] + public async Task EveryQueryOnTheInterfaceHasBeenConsidered() + { + var covered = new HashSet(StringComparer.Ordinal) + { + nameof(IGameQueries.ListAsync), + nameof(IGameQueries.SearchAsync), + nameof(IGameQueries.FeedsAsync), + nameof(IGameQueries.FindAsync), + nameof(IGameQueries.FindByIdAsync), + nameof(IGameQueries.EcosystemAsync), + + // Ranks over presence samples of listed games; it reaches game rows through the same + // filtered path and has no lookup of its own. + nameof(IGameQueries.RankingsAsync), + }; + + var declared = typeof(IGameQueries).GetMethods().Select(m => m.Name).ToHashSet(StringComparer.Ordinal); + + foreach (var member in declared) + { + await Assert.That(covered.Contains(member)) + .IsTrue() + .Because($"{member} is a public read and nothing here says whether a submitted game " + + "can reach it"); + } + } + + /// A game submitted by an account, unclaimed, exactly as the web form would make it. + private static async Task SubmittedAsync(TestDatabase db, string slug, string name) + { + var id = await Seed.GameAsync(db, slug: slug, name: name); + var user = Guid.CreateVersion7(); + + await using var connection = await db.DataSource.OpenConnectionAsync(); + + await connection.ExecuteAsync( + """ + INSERT INTO app_user (id, display_name, normalised_name, security_stamp, + concurrency_stamp, created_at) + VALUES (@user, 'submitter', 'SUBMITTER', @stamp, @stamp, @now) + """, + new { user, stamp = Guid.NewGuid().ToString(), now = Now }); + + await connection.ExecuteAsync( + "UPDATE game SET submitted_by = @user WHERE id = @id", new { user, id }); + + return id; + } +}