test(database): pin the #1881 cross-table secondary-index read contract, including wrong-row substitution

[kriszyp]

Human-Review-Need: 4 @ 5f72a5b
Promotes two exploratory-QA specs into permanent regression anchors for #1881 (secondary-index reads returning partial or empty results for the second table accessed in a single request), closed 2026-07-23.

The root cause was in @harperfast/rocksdb-js: TransactionHandle::get honoured the caller's column-family override on its synchronous block-cache attempt but dropped it in the async worker, so a warm key read correctly and a cold one resolved against the transaction's own CF. All tables in a database share one read transaction, so every table after the first was read through a foreign column family. That asymmetry is why it presented as an MVCC/snapshot bug.

Test-only. No product code changes.

Spec	What it pins
crosstable-index-scan-completeness	The core contract — table order within one request must not change an indexed result
crosstable-index-scan-blast-radius	Spread across four tables, reach into a plain REST GET, heal-on-touch behaviour

These are measured anchors, not just passing tests

Each was verified to go RED without the fix. With @harperfast/rocksdb-js pinned to 2.4.0 (last pre-fix release; #1881 shipped in 2.5.0): 5 tests fail. On 2.5.0: 12/12 green.

That check is why this PR is two specs and not four. Two further candidates — boundaries and residency — were green on main, passed every static check, and stayed 100% green against 2.4.0. They could never have caught the regression they were written for. They are dropped, and demoted back to QA candidate with the measurement recorded. An anchor that cannot fail is worse than no test: it reports coverage that does not exist.

The severe mode now reproduces

#1881's worse outcome was never just missing rows. Tables sharing a key format can hit in the foreign CF and return another table's row — silent wrong data. That had been reasoned about but, as far as I can tell, never reproduced.

These fixtures now key rows table-independently (r7-n3, identical across siblings — which is what production had; the four config tables behind #1881 all keyed on hostname|pageGroup|version) and carry ownership in an owner field, so it cannot be inferred from the key. Pre-fix, all five red tests now fail on ownership rather than on a count:

read TableB but got rows owned by ["TableA"] — a foreign column family answered this read
read GenB  but got rows owned by ["GenA"]
read GenA  but got rows owned by ["GenB"]

The check lives in the shared drain() helper, not per test, so no call site can opt out of it.

Where to look

The arming, and what is actually asserted about it. The defect only appears once a table's rows for the queried slot span more than one on-disk sorted run, and default WriteBufferManager sizing never flushes at these volumes. Arming uses an explicit flush() via a fixture resource rather than a storage.rocks.writeBufferManagerSize cap — the cap wedges a ~40MB/table single-transaction seed past undici's 300s timeout on current main.

Only the filesystem .sst count is asserted. The per-column-family levelstats test is a diagnostic, deliberately: RocksDB background compaction can legitimately merge a table's L0 to a single bottom-level file before the check runs, so a >1 sorted run per CF gate would fail on correct behaviour. An earlier revision of this PR claimed per-CF arming in its description; that was wrong, and the code now says what it does. The fails-on-base result above is the real evidence these suites can detect anything.

Readiness is a direct poll of the probe route requiring a 200 (a non-404 check accepted 500/503 during boot), and it now asserts it actually saw one rather than running the deadline out and proceeding into confusing errors. Deliberately not restartHttpWorkers() — fire-and-forget against a pre-installed fixture, races the worker respawn, turned #1886 red.

Open concern I did not resolve

CI cost. The two suites add roughly 30,000 serial puts, ~120 MiB of logical payload and ~120 forced flushes to every full integration run (~25s wall clock locally). That volume is what arms the oracle, so I have deliberately not tuned it down: shrinking it risks de-arming the precondition, which is the exact failure the dropped specs demonstrate. If you want it cheaper, the principled path is to reduce the volume and re-run the 2.4.0 check to confirm it still goes red — happy to do that, but it seemed like your call rather than mine.

What these do and do not prove

They prove the read contract — cardinality and row identity — holds on current main with the precondition armed, on RocksDB, and that both suites detect the pre-fix defect. They do not cover LMDB (the defect is RocksDB-only; the engine is now forced so an inherited LMDB run cannot fail them for the wrong reason), and they do not cover replication or multi-node paths.

Review coverage

Three passes of prepush-review.mjs (Codex + Gemini + Harper-domain). The grok leg did not run — it failed a version gate (pins 0.2.112, this host has 0.2.114) — so this is two outside lenses, not three.

The reviews earned their place. They caught that my first ownership assertion was vacuous (key-derived, with non-colliding keys, so it could never fire), the symmetry miss where only one of the two suites got the unreadable-root fix, and the missing engine force. They were also wrong once, in the other direction: Codex argued blast-radius could not fail because a full-scan control pre-warms the tables. It goes red anyway — a full scan warms the primary CF, and the defect lives in the index-CF read path.

Generated by Claude Opus 5.

[kriszyp]

Guided tour

Reading order, and the two places the risk actually lives.

1. Start with the headers — crosstable-index-scan-completeness.test.ts:1-45. The FAILS-ON-BASE block is the claim this PR rests on. Everything else is mechanics.

2. Then drain() — crosstable-index-scan-completeness.test.ts:171 and crosstable-index-scan-blast-radius.test.ts:209. This is the whole oracle. It asserts two things per read: the row count, and that every returned row's owner field matches the table it was read from. The second is what catches #1881's severe mode, and it is asserted in the helper rather than per test so a future spec author cannot add a drain() call that silently skips it.

3. Then the seeders — */resources.js:52. One line carries the design decision worth questioning:

await table.put({ id: `r${r}-n${n}`, repositoryId: `repo-${r}`, n, owner: tableName, body: payload });

Keys are deliberately identical across sibling tables. That looks wrong at a glance — normally you'd prefix to keep fixtures distinguishable. It's the point: with prefixed keys a foreign-CF read looks up a key that isn't there, finds nothing, and comes back short. The wrong-row outcome is unreachable, and an ownership check can never fire. Colliding keys are also what production had — the four config tables behind #1881 all keyed on hostname|pageGroup|version.

4. Then the arming — */resources.js SeedWave + Flush, and the oracle tests at completeness.test.ts:188.

Where to look hardest

Whether the diagnostic/gate split is right. completeness.test.ts has two oracle tests and only one is a gate. The filesystem .sst count is asserted; the per-column-family levelstats test asserts nothing but HTTP 200 and is labelled a diagnostic. That is deliberate — background compaction can merge a table's L0 to one bottom-level file before the check runs, so a >1 sorted run per CF assertion would fail on correct behaviour (observed: GenA at l0Files=0, one file at L6). But it means the per-CF precondition is unverified at read time, and if you think there's a formulation that gates it without being flaky, that's the most useful thing you could push back on.

An earlier revision of the PR description claimed per-CF arming was asserted. It wasn't. Fixed, and worth knowing the claim was made.

What the tests prove and don't

Prove: the read contract — cardinality and row identity — holds on current main with the precondition armed, on RocksDB; and both suites go red against pre-fix rocksdb-js 2.4.0 (5 tests), all five on the ownership assertion.

Don't: LMDB (engine now forced, so an inherited LMDB run can't fail them for the wrong reason); replication or multi-node; and they don't prove the per-CF sorted-run state at the moment of the read, per above.

Unresolved, your call: these add ~30,000 serial puts / ~120 MiB / ~120 forced flushes to every full integration run. That volume is what arms the oracle, so I haven't tuned it — shrinking it risks de-arming the precondition, which is precisely how the two dropped specs failed. If you want it cheaper the principled path is reduce, then re-run the 2.4.0 check.

Coverage, honestly

Three prepush-review.mjs passes: Codex + Gemini + Harper-domain. The grok leg never ran — version gate, pins 0.2.112 against 0.2.114 on this host. So two outside lenses, not three.

They caught real things: my first ownership assertion was vacuous (key-derived against non-colliding keys — it could never fire), only one of the two suites got the unreadable-root fix, and completeness wasn't forcing the engine. They were also wrong once in the other direction — Codex argued blast-radius couldn't fail because a full-scan control pre-warms the tables. It goes red anyway: a full scan warms the primary CF, and the defect lives in the index-CF read path. Worth knowing that both a model and I read that code and drew the same wrong conclusion; only the 2.4.0 run settled it.

_{Reviewed and narrated by Claude Opus 5.}

[gemini-code-assist]

Code Review

This pull request introduces integration tests and supporting resources to characterize and regression-test a cross-table secondary-index scan defect (GH #1881 / F-158) under RocksDB. It adds two test suites (crosstable-index-scan-blast-radius and crosstable-index-scan-completeness) that utilize explicit memtable flushing to reliably trigger multiple sorted runs without causing write stalls. The review feedback focuses on improving the robustness of both the test assertions and the custom resource endpoints. Key recommendations include checking that query.get and store.getDBProperty are functions before invocation, adding defensive guards for database and table retrieval, and utilizing optional chaining when parsing nested JSON response properties to prevent unhandled TypeError crashes.

[claude]

Reviewed; no blockers found.

[kriszyp]

Thanks — addressed in 8058ead. Four accepted, two declined with a different fix.

Accepted (fixture robustness): databases[db]?.[table], typeof query.get === 'function' (a query param literally named get would otherwise be invoked as one), typeof store.getDBProperty === 'function', and !t?.primaryStore. Each turns a latent TypeError into the fixture's own descriptive error. Same reasoning as the countSstFiles unreadable-root fix already in this branch: a confusing failure sends the reader after the wrong problem.

Declined as suggested — body?.steps ?? [] and Object.entries(body?.owners ?? {}).

These two are assertion loops, and defaulting their input to empty would make them iterate nothing on a malformed response and pass silently. That is precisely the vacuity this PR removed two commits ago: the first version of the ownership check derived ownership from the primary key, and because the fixtures used non-colliding keys it could never fire. Re-introducing a silent-skip path is the one change I don't want here.

Did the inverse instead — assert the shape is present, so a malformed response fails the check loudly:

assert.ok(Array.isArray(body.steps), `Drain(${steps}) returned no steps array — cannot verify row ownership`);
ok(body.owners && typeof body.owners === 'object', `Drain(${tablesCsv}) returned no owners map — cannot verify row ownership`);

That covers the crash you were guarding against and keeps the check non-vacuous.

Re-verified both properties after the change: 12/12 green on rocksdb-js 2.5.0, 5 red on 2.4.0, all five still failing on the ownership assertion.

_{Claude Opus 5}

[DavidCockerill]

Approving. Two things worth stating up front.

The red CI is not this PR — it's pre-existing main breakage, and I can show the ancestry. Three jobs fail on txnlog-purge-stale-read-blast.test.ts:683 (purge job should COMPLETE, got ERROR: Table-level transaction log deletion is not supported for RocksDB tables) and the fourth on six redeploy-runtime-equivalence tests under Bun. Neither file is in this PR, both fail identically on main, and both are already fixed on main — after the snapshot this run used. The run checked out Merge 8058eadb into 1201afb01, which contains the break (4bd78178, Aug 3) but not f85fc0fb (Aug 4 23:07Z) or dc70e300/d10b79c8 (23:24Z/23:39Z). The odd shard pattern is the confirmation rather than a puzzle: the runner shards by sorted-file index, so inserting two files shifts everything after them by +2 — main's 5/6 becomes 1/6 (5+2≡1 mod 6), main's Bun 1/6 becomes 3/6. Your earlier run at 12c61ec4, before the break landed, was fully green. Merge main and re-run. Both new suites ran and passed in the failing run (✔ QA-772 (9146ms) on Node 24 1/6 and uWS 1/6; ✔ QA-631 F-158 blast-radius [rocksdb] (13532ms)), skipped on Windows by design.

The tests are load-bearing, not shape-checking — which is the part that actually matters. The oracle is owner, a field written into each row rather than derived from the key, and primary keys deliberately collide across sibling tables. That's the only fixture shape in which #1881's wrong-row-substitution mode is reachable at all: with prefixed keys a foreign-CF read finds nothing and comes back short, and an ownership check can never fire. The precondition is demonstrably armed in CI (56 .sst files under metrics-repro, 102 under qa631-blast, index-CF L0=4 on all four tables), and the read shape matches all four conditions #1881 requires — same database, one request transaction, for await async iteration, 4KB values. The fails-on-base measurement is the right kind of proof: rocksdb-js 2.4.0 → 5 red, all five on the ownership assertion; 2.5.0 → 12/12 green. Reintroducing the bug fails GenB,GenA (index) in completeness and Q1 SPREAD positions 2/3/4 plus Q3 positions 2/3 in blast-radius.

One thing I'd take before merge (thread on blast-radius.test.ts), plus two small notes.

Also worth checking on cost, since it came up: the two suites run ~9s and ~13.5s in two different shards of ~140s each, and files run concurrently — cheaper than feared.

Reviewed by Claude Opus 5 for @DavidCockerill.