From 0d2fd6d9865cf3e5d9f5dd295ec4e47272263700 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 17:12:16 -0400 Subject: [PATCH] Keep a surviving journal on the identity the manifest implies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sealing fallback was reaching a journal that seals nothing. A recovery keeping an empty tail gave it the fallback's generation, but no TailRange recorded the move — nothing was sealed — so the next open derived the identity the manifest still implied while the session in between had appended frames under the moved one and sealed a run over them. The guard searched published runs for the derived generation, found none, and let the store open with the run unresolvable. Review confirmed it: empty tail plus an orphan at 1, reopen taking 2, frames and a run at 2, an orphan at 2, then an open at 3 with `object_source(0, 2) == None`. `RecoveryGenerations` carries a third number. `tail` is what a surviving journal keeps and is always the tail's own identity: segment-name occupancy is a fact about `segments/`, and a journal that seals nothing does not go there. `logical` remains the sealing identity and still falls back under the guard. That restores the invariant the split rests on — the active tail's identity is always derivable from the manifest — so the guard and the frames are always naming the same generation. A tail may keep a generation an orphan occupies; the collision is only real when frames are sealed under that name, and it is refused then, with the run in hand. The regression follows the reported sequence and reads the run's generation out of the run rather than assuming it, since which identity the frames ended up with is the thing under test. Mutation-checked in both places it can fail: the run drifts to generation 2, and with the mid-assertion relaxed the open succeeds with None pinned there, reproducing the report exactly. The shared helper now reports the run's own generation, so all three refusal paths fail legibly rather than against a hard-coded number. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XKzM69CHmBuDcA3qN1jFdh --- crates/levcs-store/src/engine.rs | 83 +++++++++++++++++++++++-- crates/levcs-store/src/recovery.rs | 57 +++++++++++++---- doc/instance-throughput-rewrite-plan.md | 32 ++++++++-- doc/phase1-storage-spine-scope.md | 7 +++ 4 files changed, 159 insertions(+), 20 deletions(-) diff --git a/crates/levcs-store/src/engine.rs b/crates/levcs-store/src/engine.rs index e0452e8..1b9f33e 100644 --- a/crates/levcs-store/src/engine.rs +++ b/crates/levcs-store/src/engine.rs @@ -6551,8 +6551,8 @@ mod index_maintenance_tests { /// 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 runs = manifest_runs(root, 0, root_uuid_of(root)); - assert_eq!(runs.len(), 1, "one published run is the whole premise"); + let (run, named) = the_published_runs_generation(root); + let runs = [run]; match StoreEngine::open(options) { Err(StoreError::Corruption(message)) => assert!( @@ -6565,16 +6565,36 @@ mod index_maintenance_tests { // the refusal reports what it costs rather than a bare expectation. Ok(opened) => { let committed = opened.committed_root(); - let pinned = committed.object_source(0, 1).expect("resolve generation 1"); + let pinned = committed.object_source(0, named).expect("resolve"); panic!( - "the open succeeded with a published run naming generation 1, and the \ + "the open succeeded; published run {} names generation {named} and the \ reopened root pins {pinned:?} there — every lookup reaching the run \ - rather than the replay delta above it reads nothing" + rather than the replay delta above it reads nothing", + runs[0] ); } } } + /// The generation a published run's entries name — discovered, not assumed. + /// + /// 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) { + 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) + } + /// Contract review 2026-07-30-B: the two states an occupied identity leaves. /// /// The tail's identity is generation 1 and a published run holds locations @@ -6644,6 +6664,59 @@ mod index_maintenance_tests { assert_the_open_refuses_naming_the_run(configure(), temporary.path()); } + /// The identity a surviving journal is given must be one the next open can + /// derive, or the guard searches for a generation nothing carries. + /// + /// Review's sequence. An empty journal seals nothing, so a fallback applied + /// to it moved an identity that no `TailRange` recorded — the session went + /// on to write frames and seal a run under the moved number while the next + /// open recomputed the old one. The guard then looked for the wrong + /// generation, found no run naming it, and let the store open with the run + /// unresolvable. + /// + /// 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. + #[test] + fn a_surviving_journal_keeps_an_identity_the_next_open_can_derive() { + let serial = writer_serial(); + let temporary = tempfile::tempdir().expect("tempdir"); + let namespace = NamespaceId([0x4E; 32]); + let configure = || { + let mut options = sealing_options(&serial, temporary.path(), 4_000_000); + options.max_index_runs = 3; + options.max_open_index_runs = 3; + options + }; + + // An empty tail, and an orphan on the identity it carries. + drop(StoreEngine::open(configure()).expect("open a fresh root")); + orphan_segment_at(temporary.path(), 0, 1); + + // 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. + { + let engine = StoreEngine::open(configure()).expect("a surviving empty tail opens"); + block_on(engine.submit(create_transaction(namespace, 2))).expect("create"); + push_groups(&engine, namespace, 0x60, 2); + block_on(engine.submit(push_transaction(namespace, 0x68, 0x68, None))) + .expect("the group after the seal commits"); + assert_eq!(engine.index_maintenance().sealed_runs, 1, "the seal ran"); + } + + let (_, named) = the_published_runs_generation(temporary.path()); + assert_eq!( + named, 1, + "a journal that seals nothing keeps the identity the manifest implies; \ + a run naming anything else is an identity the next open cannot derive" + ); + + // 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()); + } + /// Resumption itself is not the hazard, and a guard that treated it as one /// would refuse every interrupted recovery on a root that has ever sealed. /// diff --git a/crates/levcs-store/src/recovery.rs b/crates/levcs-store/src/recovery.rs index c04b60c..9954365 100644 --- a/crates/levcs-store/src/recovery.rs +++ b/crates/levcs-store/src/recovery.rs @@ -1938,6 +1938,7 @@ fn recover_shard_under_lock( ActiveJournalDisposition::Replay => { let RecoveryGenerations { logical: logical_generation, + tail: tail_generation, manifest: manifest_generation, } = recovery_generations_for_journal( &paths, @@ -2114,17 +2115,28 @@ fn recover_shard_under_lock( complete_interrupted_seal(&path, &paths, &counters)?; must_create_fresh = true; } else { + // This journal survives; nothing is sealed and nothing is + // renamed, so it keeps the identity the manifest implies — + // `tail_generation`, not the sealing fallback beside it. + // + // Taking `logical_generation` here was a real defect and a + // quiet one: a fallback would move the tail's identity while + // no `TailRange` recorded the move, so the next open derived + // the old number and the guard above searched published runs + // for a generation the frames no longer carried. An + // identity nothing persists is not an identity (contract + // review 2026-07-30-B). let shared_file = Arc::new(file); let retained_tail = PinnedFile::from_shared(path.clone(), Arc::clone(&shared_file)); recovered_tail = Some(RecoveredTail { - logical_generation, + logical_generation: tail_generation, journal_id: header.journal_id, path, validated_through: JOURNAL_HEADER_LEN as u64, file: shared_file, }); - retained_tails.push(RetainedTail::new(logical_generation, retained_tail)); + retained_tails.push(RetainedTail::new(tail_generation, retained_tail)); } } } @@ -2579,21 +2591,35 @@ fn apply_recovered_refs( Ok(()) } -/// The two generations a recovery needs, which are not the same number. +/// The generations a recovery needs, which are not the same number. /// -/// Contract review 2026-07-30-A. They were one, and the conflation was invisible -/// for as long as nothing outlived a session holding an `IndexLocation`. +/// Contract review 2026-07-30-A separated the first two; 2026-07-30-B separated +/// `tail` from `logical` after review found a fallback leaking onto a journal +/// that seals nothing. pub(crate) struct RecoveryGenerations { - /// The **logical** generation of the frames: the identity an `IndexLocation` - /// names, inherited from the active tail so that sealing it into a segment - /// moves the bytes without changing what they are called. + /// The **logical** generation of frames this recovery seals into a segment: + /// the identity an `IndexLocation` names, inherited from the active tail so + /// that sealing moves the bytes without changing what they are called. It + /// is the tail's identity unless a `.seg` already occupies that name, in + /// which case the frames are renamed and the guard decides whether that is + /// affordable. logical: u64, + /// The identity a **surviving** journal keeps, always the tail's own. + /// + /// Always, because segment-name occupancy is a fact about `segments/` and a + /// journal that seals nothing does not go there. Letting the fallback reach + /// this number is how an unrecorded identity appeared: nothing sealed, so + /// no `TailRange` recorded the moved value, and the next open recomputed + /// the identity the manifest still implied — while the frames this session + /// went on to append, and the run that sealed them, carried the moved one. + /// The guard then searched for the wrong generation and found nothing. + tail: u64, /// The generation of the manifest this recovery installs. A counter over /// manifests, which an index run's manifest also advances. manifest: u64, } -/// Choose both. +/// Choose all three. /// /// # Why they had to be separated /// @@ -2831,13 +2857,18 @@ fn recovery_generations_for_journal( // no-op on it. if let Some(logical) = resumable.into_iter().next() { refuse_if_displacement_strands_a_run(logical)?; - return Ok(RecoveryGenerations { logical, manifest }); + return Ok(RecoveryGenerations { + logical, + tail: preferred, + manifest, + }); } // Otherwise the frames keep the name they already have. if !occupied_segments.contains(&preferred) { return Ok(RecoveryGenerations { logical: preferred, + tail: preferred, manifest, }); } @@ -2859,7 +2890,11 @@ fn recovery_generations_for_journal( StoreError::Corruption("no logical generation remains for a recovery segment".into()) })?; refuse_if_displacement_strands_a_run(logical)?; - Ok(RecoveryGenerations { logical, manifest }) + Ok(RecoveryGenerations { + logical, + tail: preferred, + manifest, + }) } /// The logical generation of the active tail above a manifest's committed prefix. diff --git a/doc/instance-throughput-rewrite-plan.md b/doc/instance-throughput-rewrite-plan.md index 73068df..1f1169c 100644 --- a/doc/instance-throughput-rewrite-plan.md +++ b/doc/instance-throughput-rewrite-plan.md @@ -1582,12 +1582,36 @@ that keyed on "a resumable artifact exists" would fail rather than quietly refus recovery on a root that has ever sealed. The lesson generalizes past this fix: the check belongs on the *outcome* — the frames are being renamed — not on the branch that produced it. +**Second amendment: the fallback was reaching a journal that seals nothing.** Review found the guard +searching for the wrong generation entirely. A recovery that keeps an *empty* journal was giving it +the sealing fallback's identity — but nothing was sealed, so no `TailRange` recorded the move, and +the next open derived the identity the manifest still implied. The session in between had appended +frames under the moved number and sealed a run over them. The guard looked for a run naming the +derived generation, found none, and opened a store whose run resolved to nothing. Confirmed by +review: empty tail plus an orphan at 1, reopen taking 2, frames and a run at 2, an orphan at 2, and +the next open choosing 3 with `object_source(0, 2) == None`. + +`RecoveryGenerations` now carries **three** numbers, and the third is the point: `tail` is the +identity a surviving journal keeps and is always the tail's own. Segment-name occupancy is a fact +about `segments/`, and a journal that seals nothing does not go there — the fallback exists to avoid +a name collision that path never risks. `logical` remains the sealing identity and still falls back +under the guard. The invariant this restores is the one 2026-07-30-A was built on and did not fully +hold: **the active tail's identity is always derivable from the manifest**, so the guard and the +frames are always talking about the same number. + +What the tail keeps can be a generation an orphan already occupies, and that is correct: the +collision is only real when frames are sealed under that name, and it is refused then, by the guard, +with the run in hand. An identity nothing persists is not an identity. + **Evidence.** `an_orphan_holding_a_published_runs_identity_refuses_the_open`, +`a_surviving_journal_keeps_an_identity_the_next_open_can_derive`, `a_resumed_fallback_refuses_on_the_identity_it_resumes`, and -`an_orphan_holding_no_published_identity_still_opens` are the states, and both refusing tests share -one assertion helper so the two paths cannot drift in the tests either. It asserts the damage rather -than an expectation: with either guard call disabled the open succeeds and the reopened root pins -`None` at the generation the run names. The exactness test is +`an_orphan_holding_no_published_identity_still_opens` are the states, and every refusing test shares +one assertion helper so the paths cannot drift in the tests either. It asserts the damage rather than +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 `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 diff --git a/doc/phase1-storage-spine-scope.md b/doc/phase1-storage-spine-scope.md index a886770..7902762 100644 --- a/doc/phase1-storage-spine-scope.md +++ b/doc/phase1-storage-spine-scope.md @@ -1713,6 +1713,13 @@ The test is what the recovery *names its frames*, not how it got there: an inter resuming a fallback an earlier session chose strands the same run, and takes the same refusal. A resumed seal at the identity the frames already carry displaces nothing and opens normally. +The fallback applies only to frames being **sealed**. A journal that survives recovery keeps the +identity the manifest implies even when a `.seg` occupies that name, because nothing is being written +to `segments/` and there is no collision yet. This is what keeps the active tail's identity derivable +from the manifest across every open — the property the split rests on. Giving a surviving journal a +fallback identity moved a number nothing recorded, and the next open, deriving the old one, searched +published runs for a generation the frames no longer carried. + The closure is for recovery to **discard an index run whose covered identity was not preserved**, which turns the refusal into successful reclamation. It is a change to what recovery reclaims and belongs with the checkpointing work that will exercise it. Until then a root carrying both an orphan