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