From dccc1859b027e785ad6c9f52fc6f807ff30a9804 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 18:34:36 -0400 Subject: [PATCH] Read the run's identity from an entry, not from a guessed range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test oracle probed generations 0..16 to discover what a published run names, which reproduced the defect it was written to catch: it could not report an identity outside the range it guessed. Review drifted the identity to 101 and the test died on "a run with entries names some generation" before reaching either the identity assertion or the damage assertion. It would also have picked the lowest of several once a run spans generations. The helper now takes an IndexKey the test knows is covered and reads `segment_generation` off `IndexRun::get`, which answers exactly and has no range to outgrow. It fails loudly if the key is absent, since a missing entry means the test is measuring nothing. The surviving-journal regression keeps a second orphan at generation 100, so the fallback it must not take is 101 — a number that cannot be read as an off-by-one. Verified by reverting the tail fix under it: the assertion now reports left: 101, right: 1 instead of failing to find a generation at all. No production change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XKzM69CHmBuDcA3qN1jFdh --- crates/levcs-store/src/engine.rs | 69 +++++++++++++++++++------ doc/instance-throughput-rewrite-plan.md | 9 +++- 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/crates/levcs-store/src/engine.rs b/crates/levcs-store/src/engine.rs index 1b9f33e..f5e32b3 100644 --- a/crates/levcs-store/src/engine.rs +++ b/crates/levcs-store/src/engine.rs @@ -6550,8 +6550,12 @@ mod index_maintenance_tests { /// Every path that names the frames something other than the identity they /// carry owes the same refusal, so the tests assert it through one function /// rather than through copies that can drift apart the way the code did. - fn assert_the_open_refuses_naming_the_run(options: StoreOptions, root: &Path) { - let (run, named) = the_published_runs_generation(root); + fn assert_the_open_refuses_naming_the_run( + options: StoreOptions, + root: &Path, + covered: IndexKey, + ) { + let (run, named) = the_published_runs_generation(root, covered); let runs = [run]; match StoreEngine::open(options) { @@ -6576,23 +6580,34 @@ mod index_maintenance_tests { } } - /// The generation a published run's entries name — discovered, not assumed. + /// The generation the published run names for `covered` — read out of the + /// entry rather than assumed, and rather than searched for. /// /// Which identity the frames ended up with is the thing under test, so a - /// test that hard-codes it reports the number it expected rather than the - /// one the store chose. That is precisely how the fallback leaking onto a - /// surviving journal stayed invisible. - fn the_published_runs_generation(root: &Path) -> (String, u64) { + /// test that hard-codes it reports the number it expected instead of the one + /// the store chose. That is how the fallback leaking onto a surviving + /// journal stayed invisible. + /// + /// Probing generations in a range was the first attempt and had the same + /// defect one level up: it could not report an identity outside the range it + /// guessed, so a drift past it failed as "no generation found" before any + /// assertion about the identity ran. It would also pick the lowest of + /// several once a run spans generations. A named entry answers exactly. + fn the_published_runs_generation(root: &Path, covered: IndexKey) -> (String, u64) { let uuid = root_uuid_of(root); let runs = manifest_runs(root, 0, uuid); assert_eq!(runs.len(), 1, "one published run is the whole premise"); let run = crate::index::IndexRun::open(&shard_paths(root, 0).indexes().join(&runs[0]), &uuid) .expect("open the published run"); - let named = (0..16u64) - .find(|generation| run.references_segment_generation(*generation)) - .expect("a run with entries names some generation"); - (runs[0].clone(), named) + let location = run.get(&covered).unwrap_or_else(|| { + panic!( + "{} is not in the published run, so this test is not measuring the identity \ + of anything", + covered.object.to_hex() + ) + }); + (runs[0].clone(), location.segment_generation) } /// Contract review 2026-07-30-B: the two states an occupied identity leaves. @@ -6626,7 +6641,11 @@ mod index_maintenance_tests { orphan_segment_at(temporary.path(), 0, 1); } - assert_the_open_refuses_naming_the_run(configure(), temporary.path()); + assert_the_open_refuses_naming_the_run( + configure(), + temporary.path(), + IndexKey::new(namespace, ObjectId([0x60; 32])), + ); } /// The same displacement, one crash later, which is the case the first @@ -6661,7 +6680,11 @@ mod index_maintenance_tests { resumable_prefix_at(temporary.path(), 0, 2); } - assert_the_open_refuses_naming_the_run(configure(), temporary.path()); + assert_the_open_refuses_naming_the_run( + configure(), + temporary.path(), + IndexKey::new(namespace, ObjectId([0x60; 32])), + ); } /// The identity a surviving journal is given must be one the next open can @@ -6676,7 +6699,10 @@ mod index_maintenance_tests { /// /// The assertion is deliberately about what the *store* names things: the /// run's generation is read out of the run rather than assumed, so this - /// fails the same way whether the identity drifts by one or by ten. + /// fails the same way whether the identity drifts by one or by a hundred. + /// The distant orphan is there to make the difference unmistakable — a + /// fallback reaching the surviving journal yields 101, which cannot be read + /// as an off-by-one. #[test] fn a_surviving_journal_keeps_an_identity_the_next_open_can_derive() { let serial = writer_serial(); @@ -6689,9 +6715,11 @@ mod index_maintenance_tests { options }; - // An empty tail, and an orphan on the identity it carries. + // An empty tail, an orphan on the identity it carries, and a second one + // far away so the fallback this must not take is a distinctive number. drop(StoreEngine::open(configure()).expect("open a fresh root")); orphan_segment_at(temporary.path(), 0, 1); + orphan_segment_at(temporary.path(), 0, 100); // The session that keeps that journal writes frames into it and seals a // run over them. Whatever identity it kept, the run now names it. @@ -6704,7 +6732,10 @@ mod index_maintenance_tests { assert_eq!(engine.index_maintenance().sealed_runs, 1, "the seal ran"); } - let (_, named) = the_published_runs_generation(temporary.path()); + let (_, named) = the_published_runs_generation( + temporary.path(), + IndexKey::new(namespace, ObjectId([0x60; 32])), + ); assert_eq!( named, 1, "a journal that seals nothing keeps the identity the manifest implies; \ @@ -6714,7 +6745,11 @@ mod index_maintenance_tests { // Now occupy the identity the frames carry, so the next open must // displace them — and owes the refusal for the run that names them. orphan_segment_at(temporary.path(), 0, 2); - assert_the_open_refuses_naming_the_run(configure(), temporary.path()); + assert_the_open_refuses_naming_the_run( + configure(), + temporary.path(), + IndexKey::new(namespace, ObjectId([0x60; 32])), + ); } /// Resumption itself is not the hazard, and a guard that treated it as one diff --git a/doc/instance-throughput-rewrite-plan.md b/doc/instance-throughput-rewrite-plan.md index 1f1169c..b60005a 100644 --- a/doc/instance-throughput-rewrite-plan.md +++ b/doc/instance-throughput-rewrite-plan.md @@ -1611,7 +1611,14 @@ one assertion helper so the paths cannot drift in the tests either. It asserts t an expectation, and it **reads the generation out of the published run** rather than assuming one — hard-coding it is how a drifting identity would report the number the test expected instead of the number the store chose. With any of the three changes reverted the open succeeds and the helper -reports the run's own generation with `None` pinned at it. The exactness test is +reports the run's own generation with `None` pinned at it. + +The helper reads that generation from a **named entry**, `IndexRun::get` on a key the test knows is +covered. Its first version probed generations `0..16` and had the original defect one level up: it +could not report an identity outside the range it guessed, so review's drift to generation 101 failed +as "no generation found" before any assertion about the identity ran, and a run spanning generations +would have reported the lowest. The regression now keeps a distant orphan precisely so the fallback +it must not take is 101 rather than a number that reads as an off-by-one. The exactness test is `a_run_reports_only_the_segment_generations_its_entries_actually_name`, whose negative cases include a generation inside a section's packed span that no entry uses. A first draft of the refusing test passed for the wrong reason — its workload re-pushed the genesis object id as a blob, so the reopen