fix(storage): stop the migration denying a promise a node already made, and report what the neighbours say - #223
Conversation
89fb6b9 to
e57ddf3
Compare
a0f904a to
b2df569
Compare
b2df569 to
0e07a90
Compare
80dbed4 to
3cd8356
Compare
8df37a9 to
5191e45
Compare
4ea582a to
529a72c
Compare
d7d969c to
e2c2aa3
Compare
…e, and report what the neighbours say A patch on top of the release that replaced the LMDB chunk store, which has already merged and is running on nodes part-way through their migration. It changes no on-disk format and writes no migration state: a node mid-flight re-reads its marker, its first-start time, its shed count and its remaining keys and carries on. A test asserts that rather than the commit claiming it. Two things, and only two. **A node can no longer be made to deny a promise it already made.** Narrowing a promise is fine and is untouched: a node that sheds rotates to a commitment over fewer keys, and `rotate` keeps the old roots as retained slots, so a peer that pinned one can still challenge it and get an answer until it ages out by its gossip TTL. That is the staged narrowing this migration was designed around. The rotation also had a branch that fired when a node had NO responsible keys at all, not a smaller set. It dropped every retained root outright when the node "looked empty", decided from a key count, and that count was wrong in the same direction every time it was fixed: a node whose disk filled before it could copy anything, then one whose file had been dropped from the index by a failed read while its legacy copy was being restored, then a files-only node that had published bytes to disk but not yet indexed them, because a file is published before it is indexed. Each fix closed one window and left another. So the branch is gone, and with it the question. The asymmetry settles it: clearing repudiates a root a peer is still pinning, which answers UnknownCommitment and is graded a confirmed failure on the commitment-bound lane, enforced in every release and not the lane the migration holds off; retiring stops advertising and stays answerable until the gossip TTL lapses. Both set has_current = false. A genuinely empty node cannot answer either way, so retiring costs it nothing it had. **Every node says whether it still has an old chunk store, and reads what its neighbours say.** The state rides the user agent saorsa-core already sends with every signed message and keeps for each peer, so it costs no new message, no new field and no protocol version, and the `node/` prefix that gates DHT membership is preserved. Three states, never folded into two: a directory that could not be read is not one that is not there. It counts what the peers a node is connected to announced, each as of that peer own last start: saorsa-core copies the user agent when it builds the transport, so a node that finishes migrating keeps announcing legacy until it restarts. Two consequences, running in opposite directions, so the tally bounds nothing. A peer announcing legacy may have finished since, so the count can be too high. A node that is offline, or simply not connected to, is absent from it, so the count can be too low. An all-zero tally proves nothing on its own either, because a node connected to nobody produces one; the number of peers seen is what tells that apart. So this can surface nodes that have not finished. It cannot establish that none remain, and no amount of it adds up to that. A peer still on the previous release announces no token and counts as unreported, never as finished. The reporter holds a weak handle to the node. A reporter must not be the reason the thing it observes stays alive: a strong one would keep a dropped node's transport, and its bound port, for as long as the task ran. The signal module is crate-private; neither goal needs new public API. The terraform worker unit passes --enable-logging, without which a node on that path emits nothing at all and none of this is readable.
e2c2aa3 to
264014a
Compare
dirvine
left a comment
There was a problem hiding this comment.
Review — APPROVE (at 5191e45)
Reviewed the migration patch against base 31fcbae, with independent review-panel consensus across storage/audit, network/user-agent & lifecycle, and tests/deployment. No material blockers found; no dissent.
Fix 1 — audit slashing: removing the storage_empty → clear_all branch and replacing it with retire_current() keeps pinned roots answerable for the gossip TTL (16-slot hard bound, commitment_state.rs:317-327/510-518/917-935), eliminating the UnknownCommitment → trust-penalty window without continuing to advertise stale commitments. Semantics pinned by test (commitment_state.rs:948-995).
Fix 2 — neighbour migration awareness: user-agent node/{version} migration/{token} preserves the node/ prefix that saorsa-core gates DHT membership on (core network.rs:113-115), rides connection setup, and is stored per-peer (transport_handle.rs:330/343). Conservative tally (outstanding = legacy + unknown + unreported), weak-handle + shutdown-token lifecycle, no lock held across await. No wire incompatibility.
Verification: cargo fmt clean; 1,102 lib tests pass; focused migration disk-reclaim test passes locally and on Linux (ext4/xfs/btrfs).
CI note: Windows migration_reclaims_disk is red, but that test is byte-identical to base and exercises zero changed code; its store-scoped assertions passed while only a volume-wide free-space endpoint assertion failed — runner/harness noise, not a regression. Recommend a rerun to confirm; no code change needed for it.
Non-blocking follow-ups (separate from this merge):
- Harden the Windows end-cost assertion in migration_reclaims_disk (re-baseline free_at_start after seeding / scope to directory walk).
- De-duplicate migration constants (LEGACY_ENV_DIR/RETIRED_* / MAX_TOMBSTONES) between migration_signal.rs and chunk_store.rs.
- Add
--enable-loggingto deploy/scripts/spawn-nodes.sh for fleet visibility parity with worker.yml.
A patch on top of #216, which has already merged and is running on nodes part-way through their
migration. It does two things and nothing else.
It changes no on-disk format and writes no migration state. A node mid-flight re-reads its
marker, its first-start time, its shed count and its remaining keys and carries on. That is
asserted by
a_store_left_midway_by_the_previous_release_keeps_its_place, not just claimed.1. The migration can no longer make a node deny a promise it already made
Narrowing a commitment is fine and is untouched. A node that sheds rotates to a new
commitment over fewer keys, and
rotatekeeps the old roots as retained slots, so a peer thatpinned one can still challenge it and get an answer until it ages out by its gossip TTL. That is
the staged narrowing this migration was designed around, and nothing here changes it.
The bug is in a different path, and it does not narrow. It erases.
The rotation had a branch that fired when a node had no responsible keys at all, not a
smaller set. It dropped every retained root outright when the node "looked empty", decided from
a key count. That count asked whether there was anything left to commit
to, not whether there were any bytes, and it was wrong in the same direction every time it was
fixed:
old store;
being restored;
is published before it is indexed.
Each fix closed one window and left another. So the branch is gone, and with it the question.
The asymmetry settles it. Clearing repudiates a root a peer is still pinning, which answers
UnknownCommitment— a confirmed failure on the commitment-bound lane, which isdeliberately enforced in every release, including the one that withholds the penalty for
merely not holding a close-group chunk, because the migration depends on a reduced commitment
still meaning something (
replication/mod.rs:9107). So the node slashed itself for data itstill had, and the migration's suspension could not stop it.
Retiring instead stops advertising and stays answerable until the gossip TTL lapses. Both set
has_current = false; they differ only in whether the node goes on being answerable meanwhile.A genuinely empty node cannot answer either way, so retiring costs it nothing it had.
This bug is live in
v0.19.0-rc.1, and it fires on exactly the nodes the migration is hardeston.
2. A node reports what its neighbours say
Each node says whether it still has an old chunk store, and reads what its neighbours say. This does not establish that the fleet has finished, and cannot: a node sees only the peers it is connected to, and each of those answers as of its own last start, so the most it can show is that some peer reported an old store when it last started. It can never show that no node has one. What it gives is the only view we get of the nodes we do not run. The
state rides the user agent
saorsa-corealready sends with every signed message and keeps perpeer, so it costs no new message, no new field and no protocol version. The
node/prefixthat gates DHT membership is preserved.
Three states, never folded into two: a directory that could not be read is not one that is not
there.
What a peer's answer means, in the fewest words that are all true. It counts what the peers
a node is connected to announced, each as of that peer's own last start:
saorsa-corecopiesthe user agent when it builds the transport, so a node that finishes migrating keeps announcing
legacyuntil it restarts.Two consequences, running in opposite directions, so the tally bounds nothing. A peer
announcing
legacymay have finished since, so the count can be too high. A node that isoffline, or simply not connected to, is absent from it, so the count can be too low. An all-zero
tally proves nothing on its own either, because a node connected to nobody produces one; the
number of peers seen is what tells that apart.
So this can surface nodes that have not finished. It cannot establish that none remain, and no
amount of it adds up to that.
outstandingcountslegacy,unknownandunreportedtogether, because a peer whose disk could not be read and a peer on a build from before this
existed are both as far from finished as
legacyis.Linear issue
Closes V2-1200
Risk tier
Compatibility
migration/<state>token after the existingnode/<version>prefix, which is preserved. Nodes on feat(storage): replace the LMDB chunk store with one file per chunk, and migrate onto it #216 do not read it; this build reads a peer without one
as unreported. No new message, field or protocol version.
part-way through by feat(storage): replace the LMDB chunk store with one file per chunk, and migrate onto it #216 keeps its phase, first-start time, shed count and remaining keys.
storage::migration_signal.Semver impact
Test evidence
Local, on
31fcbaeplus this branch:cargo test --lib1102 passed, 0 failed.cargo clippy --all-targets --all-features -- -D warningsclean (the exact CI command), pluscargo fmt,RUSTFLAGS="-D warnings" cargo build --release --no-default-features, andscripts/adr-governance.py.Regressions that matter here:
a_store_left_midway_by_the_previous_release_keeps_its_placea_peer_on_the_previous_release_is_not_counted_as_finishedan_entry_that_cannot_be_read_is_never_read_as_finishedthe_reporter_lets_go_of_a_node_that_was_droppedNot covered here: a dev testnet run showing the per-peer lines aggregating into a fleet
count.
New dependency
none
ADR
https://github.com/grumbach/ant-node/blob/264014a14c8a96d3159ab6b7720c97fbd2b0750f/docs/adr/ADR-0014-file-based-chunk-store-and-lmdb-retirement.md
Mitigation / rollback
Revert the commit. The user agent loses its token and peers read the node as unreported, which
is where every #216 node already sits. The commitment predicate goes back to the shipped one.
Nothing on disk differs either way.