From 3b2b7093ae806a9ecc77b07b93e83eb860ff8621 Mon Sep 17 00:00:00 2001 From: Chris O'Neil Date: Tue, 15 Sep 2026 00:11:06 +0100 Subject: [PATCH] fix: reopen the bridge for chunks a committed node never agreed to shed Once a node reaches `Committed`, the copier moves only the keys the rank check refuses to shed. A chunk that turns up in the legacy environment after that, and that this node never agreed to give up, is on no such list: nothing copies it, the shed gate correctly refuses to prove a single copy exists elsewhere, and the startup reconciliation only returns a node to `Bridging` when the file store holds less than it recorded keeping, which the file store never does. The node is terminal in that state. Free disk does not change it and neither does a restart. Seen on one node of 38 in the 2026-09-08 beta cohort: it committed with "nothing has to be shed", then pre-retirement verification found 317 chunks in the legacy environment that were in neither view and re-queued them. It has refused retirement every 30 seconds since. Once #218 removes the LMDB reader those 317 stop being servable, and on the evidence they are the network's only copy. The fix is a third arm in the reconciliation block, next to the two of the same shape: when the marker says `Committed` and there are more legacy-only keys than the node ever approved for shedding, go back to `Bridging`, clear `committed_at_unix` and reset `rebuilds_since_commit`. `open_legacy` already computes the legacy-only set before the marker is read, so this is a comparison against a number that is already in memory. No new I/O, no marker format change. The count, not emptiness, is the test. A node that legitimately shed keeps exactly the keys it is giving up in the legacy environment until they stop being answerable, so a non-empty legacy-only set is its normal state; bouncing it to `Bridging` on every restart would reset its retention clock each time and a regularly restarted node would never retire. The arm fires only on `legacy_only > shed_key_count`. It logs `migration_event = "back_to_bridging"` with both numbers so the case is visible in the beta watch. The upgrade's own restart is enough to take an affected node out of the loop. Tests: `cargo test --lib` 1104 passed, 0 failed; fmt clean; clippy with `-D clippy::panic -D clippy::unwrap_used -D clippy::expect_used` clean on lib and tests. Two tests added, and both were mutation-checked: with the discriminator replaced by `legacy_only > 0`, the new regression test and the existing `the_migration_marker_survives_a_restart` both fail; with the arm disabled, the fire-case test fails. Closes V2-1232 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UrjwPBwPqMi3F2sT1yVYEa --- src/storage/chunk_store.rs | 125 +++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/src/storage/chunk_store.rs b/src/storage/chunk_store.rs index 700b05b4..a6c59470 100644 --- a/src/storage/chunk_store.rs +++ b/src/storage/chunk_store.rs @@ -292,6 +292,7 @@ impl ChunkStore { MigrationPhase::FilesOnly }; let mut state = MigrationState::load_or_create(&config.root_dir, phase); + let legacy_only = Self::legacy_only_count(legacy.as_ref()); // The filesystem is the authority on whether a legacy environment exists; the // marker only records decisions. Reconcile rather than trust. @@ -322,6 +323,32 @@ impl ChunkStore { if let Err(e) = state.save(&config.root_dir) { warn!("Could not persist the migration marker: {e}"); } + } else if state.phase == MigrationPhase::Committed && legacy_only > state.shed_key_count { + // Once committed, the copier only moves keys the rank check refuses to shed. + // A chunk that turns up in the legacy environment after that, and that this + // node never agreed to give up, is never copied and never proven to be held + // elsewhere, so nothing frees it from the environment and the environment is + // never retired. Free disk does not change that; only the phase does. + // + // The count, not emptiness, is the test. A node that legitimately shed keeps + // exactly the keys it is giving up in the legacy environment until they stop + // being answerable, and sending it back to the bridge on every restart would + // reset its retention clock each time. + warn!( + migration_event = "back_to_bridging", + legacy_only, + shed = state.shed_key_count, + "The migration marker says this node committed after agreeing to shed {} \ + chunk(s), but {legacy_only} are still only in the legacy environment. \ + Restarting the migration from the copying stage so they are copied.", + state.shed_key_count + ); + state.phase = MigrationPhase::Bridging; + state.committed_at_unix = None; + state.rebuilds_since_commit = 0; + if let Err(e) = state.save(&config.root_dir) { + warn!("Could not persist the migration marker: {e}"); + } } else if legacy.is_some() && state.phase == MigrationPhase::FilesOnly { warn!( "The migration marker says this node is done but {} is still on disk. \ @@ -416,6 +443,11 @@ impl ChunkStore { .collect() } + /// How many keys only the legacy environment holds; zero once it is gone. + fn legacy_only_count(legacy: Option<&Legacy>) -> u64 { + legacy.map_or(0, |l| l.only.read().len().try_into().unwrap_or(u64::MAX)) + } + /// Take the critical section for one key. async fn key_lock(&self, address: &XorName) -> Option> { let lane = address.last().copied().unwrap_or(0) as usize; @@ -4668,6 +4700,99 @@ mod tests { assert_eq!(store.legacy_only_keys().len(), 3); } + /// V2-1232. Once committed, the copier moves only the keys the rank check refuses to + /// shed, so a chunk that is found in the legacy environment after the commitment, + /// and that this node never agreed to give up, is never copied, never provable + /// elsewhere, and keeps the environment from ever being retired. Free disk does not + /// help and neither does a restart. Reopening has to notice and reopen the bridge. + #[tokio::test] + async fn a_node_that_committed_with_nothing_to_shed_resumes_the_copy_for_chunks_only_it_holds() + { + let dir = TempDir::new().expect("temp dir"); + let keys = seed_legacy(&dir, &["c1", "c2"]).await; + let store = open(&dir).await; + store + .copy_batch(&keys, 0, 0, &never_cancelled()) + .await + .expect("copy"); + store.commit_to_files().expect("commit"); + store.note_commitment_rebuilt(); + let state = store.migration_state(); + assert_eq!(state.shed_key_count, 0, "nothing had to be shed"); + assert!(state.committed_at_unix.is_some()); + assert_eq!(state.rebuilds_since_commit, 1); + store.wait_idle().await; + drop(store); + + // A chunk in the legacy environment that the file store has no record of, found + // after the node committed. The file store is untouched, so the kept-count rule + // has nothing to say about this. + let late = seed_legacy(&dir, &["c3"]).await; + + let store = open(&dir).await; + let state = store.migration_state(); + assert_eq!( + state.phase, + MigrationPhase::Bridging, + "a chunk the node never agreed to shed must reopen the bridge" + ); + assert!( + state.committed_at_unix.is_none(), + "the retirement clock must not carry over from the abandoned commitment" + ); + assert_eq!(state.rebuilds_since_commit, 0); + assert_eq!(store.legacy_only_keys(), late); + + // And the copier, which the phase change lets run again, moves it. + store + .copy_batch(&late, 0, 0, &never_cancelled()) + .await + .expect("copy"); + assert!(store.legacy_only_keys().is_empty()); + assert_eq!( + store + .get(late.first().expect("a key")) + .await + .expect("get") + .expect("present"), + addressed("c3").1 + ); + } + + /// The counterpart of the test above, and it matters as much. A node that shed keeps + /// exactly the keys it is giving up in the legacy environment until they stop being + /// answerable, so a non-empty legacy-only set is its normal state. Sending it back + /// to the bridge on every restart would reset its retention clock each time, and a + /// node restarted regularly would never retire. + #[tokio::test] + async fn a_node_that_shed_stays_committed_across_a_restart() { + let dir = TempDir::new().expect("temp dir"); + let keys = seed_legacy(&dir, &["k1", "k2"]).await; + let store = open(&dir).await; + store + .copy_batch(&keys[..1], 0, 0, &never_cancelled()) + .await + .expect("copy"); + store.commit_to_files().expect("commit"); + store.note_commitment_rebuilt(); + let before = store.migration_state(); + assert_eq!(before.shed_key_count, 1); + assert_eq!(store.legacy_only_keys().len(), 1); + store.wait_idle().await; + drop(store); + + let store = open(&dir).await; + let after = store.migration_state(); + assert_eq!( + after.phase, + MigrationPhase::Committed, + "the keys it is giving up are not a reason to reopen the bridge" + ); + assert_eq!(after.committed_at_unix, before.committed_at_unix); + assert_eq!(after.rebuilds_since_commit, before.rebuilds_since_commit); + assert_eq!(store.legacy_only_keys().len(), 1); + } + #[tokio::test] async fn a_file_that_vanished_mid_verification_is_requeued_not_republished() { let dir = TempDir::new().expect("temp dir");