A long connect screen cost a game its listing, for ever - #19
Merged
Conversation
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>
WalkthroughThe migration bounds ChangesBounded game-field indexes
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs`:
- Around line 87-104: Update NoIndexOnThisTableCanRefuseALongValue to explicitly
locate the game_field_folded_value_idx definition, assert that it exists, and
verify its predicate limits value length to 256. Keep the existing table-wide
checks and TheFacetLookupIsStillIndexed coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0da4e8ff-7e75-41da-9a6d-f14af008da5b
📒 Files selected for processing (3)
migrations/0008_bounded_field_value_index.sqlsrc/MUI.Crawler/Persistence/CatalogueDirectories.cstests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs
Comment on lines
+87
to
+104
| 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}"); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target test ---'
sed -n '1,150p' tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs
printf '%s\n' '--- index references ---'
rg -n -C 3 'game_field_folded_value_idx|TheFacetLookupIsStillIndexed|folded_value|game_field' tests src . --glob '!**/bin/**' --glob '!**/obj/**' | head -n 300
printf '%s\n' '--- schema and migrations ---'
rg -n -C 4 'CREATE (UNIQUE )?INDEX|folded_value|length\\(|game_field' . --glob '*.sql' --glob '*.cs' --glob '*.fs' --glob '*.fsx' | head -n 400Repository: SharpMUSH/MUIndex
Length of output: 37649
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,150p' tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs
rg -n -C 3 'game_field_folded_value_idx|TheFacetLookupIsStillIndexed|folded_value|game_field' . --glob '!**/bin/**' --glob '!**/obj/**' | head -n 300Repository: SharpMUSH/MUIndex
Length of output: 39137
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
migration = Path("migrations/0008_bounded_field_value_index.sql").read_text()
test = Path("tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs").read_text()
folded = re.search(
r"CREATE INDEX\s+game_field_folded_value_idx\b(?P<body>.*?);",
migration,
re.I | re.S,
)
assert folded, "game_field_folded_value_idx is not created by migration 0008"
body = folded.group("body")
print("folded_index_definition:", " ".join(body.split()))
print("folded_index_has_length_bound:", bool(re.search(r"WHERE\s+length\s*\(\s*value\s*\)\s*<=\s*256", body, re.I)))
facet = re.search(
r"indexname\s*=\s*'game_field_field_value_idx'.*?;",
test,
re.I | re.S,
)
print("named_assertion_targets_field_value_idx:", bool(facet))
print("named_assertion_targets_folded_value_idx:", "game_field_folded_value_idx" in (facet.group(0) if facet else ""))
loop = re.search(
r"SELECT indexdef FROM pg_indexes WHERE tablename = 'game_field'",
test,
re.I,
)
print("loop_checks_table_indexes:", bool(loop))
print("loop_requires_folded_index_name:", False)
PYRepository: SharpMUSH/MUIndex
Length of output: 465
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
migration = Path("migrations/0008_bounded_field_value_index.sql").read_text()
test = Path("tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs").read_text()
assert "game_field_folded_value_idx" in migration
assert re.search(r"WHERE\s+length\s*\(\s*value\s*\)\s*<=\s*256", migration, re.I)
assert "indexname = 'game_field_field_value_idx'" in test
assert "indexname = 'game_field_folded_value_idx'" not in test
print("migration defines the bounded folded-value index")
print("test names only game_field_field_value_idx")
print("test has no named assertion for game_field_folded_value_idx")
PYRepository: SharpMUSH/MUIndex
Length of output: 307
Assert game_field_folded_value_idx explicitly.
The table-wide loop passes when this index is absent, and TheFacetLookupIsStillIndexed checks only game_field_field_value_idx. Add a named assertion that game_field_folded_value_idx exists and has WHERE length(value) <= 256.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/MUI.Catalog.Tests/Persistence/OversizedFieldValueTests.cs` around lines
87 - 104, Update NoIndexOnThisTableCanRefuseALongValue to explicitly locate the
game_field_folded_value_idx definition, assert that it exists, and verify its
predicate limits value length to 256. Keep the existing table-wide checks and
TheFacetLookupIsStillIndexed coverage unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by the first crawl large enough to find it — seeding 701 addresses from the directories, three games of four hundred died with:
PostgreSQL cannot index a btree row past ~2704 bytes, and a connect screen is routinely thousands of characters (the longest here is 9,376). The failure is total, not partial: the
INSERTis refused, so the whole probe's ingestion is lost for that game — and 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 the fifty-one rows 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 why looking at the longest stored value would not have predicted which games were affected.
Both indexes had it, and they are bounded differently because they are read differently
The first pass caught one — and the very next probe failed on the other.
game_field_field_value_idxgame_field_folded_value_idxWHERE length(value) <= 256). That reader asks an equality question, and a prefix would silently turn it into a starts-with — over-matching where the raw index merely refused.CatalogueDirectoriescarries 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 of them, so excluding it changes no correct answer.
The stored value is untouched. Truncating what a game sent in order to fit our own index would be exactly the quiet lossiness this schema refuses everywhere else. It is the index that is bounded, never the fact.
The test
It asserts the property over every index on
game_field, not over the one that was noticed — which is precisely the mistake the first pass made. Plus: an oversized connect screen is stored whole, and two values sharing their first 256 characters stay distinct (a prefix is an index, not a key — collapsing them would have traded a loud failure for a silent one).Verified against the live catalogue
The affected games re-probed clean, and the one that raised the original error now holds its 7,535-character connect screen. Five suites green, 601 tests, zero warnings.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NrGKmKcRCGktyhRTFbQDMk
Summary by CodeRabbit