---
phase: 01-federation-mesh-hardening
plan: 01
type: execute
wave: 1
depends_on: []
files_modified:
- core/archipelago/src/federation/storage.rs
autonomous: true
requirements: [FED-01]
must_haves:
truths:
- "A federation removal issued while an auto-sync pass is in flight leaves the peer removed — a sync's pre-removal node-list snapshot can no longer re-save the removed peer (FED-01 adjacency edge)"
- "Two concurrent federation node writes both persist — neither silently loses the other's update (the lost-update class behind the reported 'removed nodes reappear' symptom)"
- "Removing the last remaining federated node succeeds, leaves an empty node list, and returns Ok with an empty Vec (FED-01 empty edge)"
- "A removal whose tombstone write fails returns Err to the caller instead of reporting success (FED-01 failure-surfacing)"
- "The tombstone is durably written before the filtered node list is saved, so an interruption between the two never resurrects the removed peer (FED-01 ordering edge)"
- "A partially-written federation node list can never be observed by a concurrent reader — the list is written to a sibling temp file and renamed into place"
prohibitions:
- statement: "Removing a federation node MUST NOT delete or destroy that peer's local data — no app data directory under /var/lib/archipelago, no mesh message history, no credential store is erased by unfederating; removal revokes trust, it never destroys operator data"
category: safety
artifacts:
- path: core/archipelago/src/federation/storage.rs
provides: "Serialized, crash-safe federation node store"
contains: "FEDERATION_STORE_LOCK"
key_links:
- from: core/archipelago/src/federation/storage.rs
to: core/archipelago/src/federation/sync.rs
via: "update_node_state acquires FEDERATION_STORE_LOCK for its whole load-mutate-save cycle, so a sync pass cannot interleave with remove_node"
pattern: "FEDERATION_STORE_LOCK"
---
Close the concurrency race that lets a removed federation node come back: serialize every
read-modify-write against `federation/nodes.json` behind one async lock, and make the node-list
write atomic.
Purpose: FED-01 — "removing a federation node sticks" is the reason this phase exists. RESEARCH.md
identifies an unlocked read-modify-write on `federation/nodes.json` as the primary suspect: the 90s
auto-sync loop, the 1800s auto-sync loop, `federation.sync-state`, and `federation.remove-node` all
load → mutate → save the same file with zero coordination, so a sync task holding a pre-removal
snapshot silently re-saves the peer the operator just removed — with no error logged anywhere.
Output: `federation/storage.rs` with a module-level `FEDERATION_STORE_LOCK`, inner/outer function
split to avoid re-entrancy deadlock, an atomic temp-file+rename node-list write, and three new
regression tests that fail without the lock.
@$HOME/.claude/gsd-core/workflows/execute-plan.md
@$HOME/.claude/gsd-core/templates/summary.md
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/01-federation-mesh-hardening/01-RESEARCH.md
@.planning/phases/01-federation-mesh-hardening/01-PATTERNS.md
@core/archipelago/src/federation/storage.rs
@core/archipelago/src/update.rs
## Artifacts this phase produces
Created or changed by **this plan**:
| Symbol | Kind | File |
|---|---|---|
| `FEDERATION_STORE_LOCK` | `static tokio::sync::Mutex<()>` | `core/archipelago/src/federation/storage.rs` |
| `save_nodes_inner` | private async fn (no lock; atomic temp+rename write) | same |
| `load_nodes_inner` | private async fn (no lock) | same |
| `tombstone_did_inner` / `untombstone_did_inner` | private async fns (no lock) | same |
| `test_concurrent_writes_do_not_lose_updates` | `#[tokio::test]` | same (`mod tests`) |
| `test_remove_survives_concurrent_state_sync` | `#[tokio::test]` | same (`mod tests`) |
| `test_remove_last_node_leaves_empty_list` | `#[tokio::test]` | same (`mod tests`) |
| `test_remove_errors_when_tombstone_write_fails` | `#[tokio::test]` | same (`mod tests`) |
Public function signatures of `load_nodes`, `save_nodes`, `add_node`, `remove_node`,
`set_trust_level`, `update_node`, `update_node_state`, `record_peer_transport`, `tombstone_did`,
`untombstone_did`, `load_removed_dids` are **unchanged** — callers in `sync.rs`, `handlers.rs`,
`server.rs`, and `mesh/mod.rs` compile untouched.
Task 1: End-to-end — a removal survives an in-flight sync (lock + atomic write)
A module-private lock and a temp+rename write are internal
to storage.rs behind unchanged public signatures; reverting is a single-file change with no
on-disk format change.
core/archipelago/src/federation/storage.rs
- `core/archipelago/src/federation/storage.rs` — the whole file (532 lines). Note in particular:
`load_nodes` (L51), `record_peer_transport` (L120), `save_nodes` (L149), `add_node` (L161,
calls `untombstone_did`), `remove_node` (L180, calls `tombstone_did`), `tombstone_did` (L214),
`untombstone_did` (L237), `set_trust_level` (L256), `update_node` (L272), `update_node_state`
(L292), and the existing `#[cfg(test)] mod tests` (L341) with its `make_node(did, onion)`
helper and `tempfile::tempdir()` convention.
- `core/archipelago/src/update.rs` lines 25-40 — `UPDATE_OP_LOCK`, the in-repo precedent for
"two async call sites race on one on-disk resource". Copy its doc-comment style (name the
concrete historical incident, then state the acquisition policy).
- `.planning/phases/01-federation-mesh-hardening/01-PATTERNS.md` — section "Async static lock
for a racy on-disk resource" and "core/archipelago/src/federation/storage.rs (locking fix)".
- `test_concurrent_writes_do_not_lose_updates`: under `#[tokio::test(flavor = "multi_thread", worker_threads = 4)]`,
seed one node, then `tokio::join!` an `add_node(B)` with a `set_trust_level(A, Observer)`;
afterwards `load_nodes` returns 2 nodes AND node A's trust level is Observer. Without the
lock one of the two writes is lost.
- `test_remove_survives_concurrent_state_sync`: under the multi-thread flavor, loop 50 times:
fresh tempdir, seed nodes A and B, `tokio::join!(remove_node(A), update_node_state(A, snapshot))`,
then assert `load_nodes` contains no entry whose `did` is A and `load_removed_dids` contains A.
Without the lock this reliably fails within 50 iterations.
- `test_remove_last_node_leaves_empty_list`: seed exactly one node, remove it, assert the
returned Vec is empty, `load_nodes` returns an empty Vec (not an error), and the DID is
tombstoned.
Write the three tests FIRST in the existing `mod tests` block and confirm they fail (run the
verify command and capture the failure) before writing the fix.
Then add at module scope, immediately after the existing `use` block:
`static FEDERATION_STORE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());`
with a doc comment that names the failure it prevents — a `federation.remove-node` RPC racing
the 90s auto-sync pass, whose pre-removal `load_nodes` snapshot re-saves the removed peer with
no error logged — and states the acquisition policy: acquire with `.lock().await`, never
`try_lock`. Reject-on-contention is wrong here: a rejected federation write reproduces the very
lost-write symptom the lock exists to stop, unlike `update.rs` where rejecting a second
concurrent download is the desired UX.
Restructure so no public function can deadlock on itself. For each function that both takes the
lock and calls another locking function, extract the body into a private `*_inner` fn that does
NOT acquire, and make the public fn a thin wrapper: acquire the guard, call the inner(s), drop.
Required inner fns for this task: `load_nodes_inner`, `save_nodes_inner`, `tombstone_did_inner`,
`untombstone_did_inner`. `remove_node` (which calls `tombstone_did`) and `add_node` (which calls
`untombstone_did`) must call the `*_inner` variants under a single held guard so tombstone +
node-list save are one critical section.
Convert the node-list write in `save_nodes_inner` to atomic replace: serialize to a sibling path
formed by appending a `.tmp` suffix to the resolved nodes file path in the same directory, write
it with `tokio::fs::write`, then `tokio::fs::rename` it onto the real path. Keep the existing
`.context(...)` error strings so callers' messages are unchanged. Same-directory rename is
required — a cross-filesystem rename is not atomic.
In THIS task route `load_nodes`, `save_nodes`, `remove_node`, `tombstone_did`,
`untombstone_did`, and `update_node_state` through the lock. The remaining mutators are Task 2.
Preserve `remove_node`'s existing ordering exactly: the retain/`bail!`-on-not-found check, then
the tombstone write with its failure propagated via `.context("persist removal tombstone")?`,
then the node-list save. Do not weaken that ordering or its error propagation.
Build gotcha from CLAUDE.md: if the build hits `rust-lld: undefined hidden symbol`, that is
incremental-cache corruption — re-run with `CARGO_INCREMENTAL=0`.
cd core && cargo test -p archipelago federation::storage
- `cd core && cargo test -p archipelago federation::storage` exits 0.
- `grep -c 'FEDERATION_STORE_LOCK' core/archipelago/src/federation/storage.rs` is at least 7.
- `grep -c 'tokio::sync::Mutex::const_new' core/archipelago/src/federation/storage.rs` equals 1.
- `grep -c 'fs::rename' core/archipelago/src/federation/storage.rs` is at least 1.
- `grep -Eq 'async fn test_remove_survives_concurrent_state_sync' core/archipelago/src/federation/storage.rs` succeeds.
- `grep -Eq 'async fn test_concurrent_writes_do_not_lose_updates' core/archipelago/src/federation/storage.rs` succeeds.
- `grep -Eq 'async fn test_remove_last_node_leaves_empty_list' core/archipelago/src/federation/storage.rs` succeeds.
- `grep -Eq 'flavor = "multi_thread"' core/archipelago/src/federation/storage.rs` succeeds (the
race tests are useless on the single-threaded default runtime).
- The SUMMARY records the captured pre-fix failure output for at least one of the three tests.
The removal-vs-sync race is closed at the storage layer and proven by a test that fails without the lock; the node list is written atomically.
Task 2: Bring every remaining federation mutator under the lock + surface tombstone-write failure
core/archipelago/src/federation/storage.rs
- `core/archipelago/src/federation/storage.rs` as left by Task 1 — specifically the four
mutators not yet routed through the lock: `record_peer_transport` (L120 pre-change),
`add_node`, `set_trust_level`, `update_node`.
- The existing test `test_remove_nonexistent_node_errors` (L445 pre-change) — mirror its
assertion style for the new failure test.
Route `add_node`, `set_trust_level`, `update_node`, and `record_peer_transport` through
`FEDERATION_STORE_LOCK` using the same wrapper + `*_inner` split established in Task 1. Every
public function in this module that performs a load → mutate → save cycle must hold the guard
for the whole cycle; none may call another lock-acquiring public function while holding it.
Add `test_remove_errors_when_tombstone_write_fails`: seed a node, then make the tombstone write
fail by pre-creating the removed-nodes path as a directory (a directory cannot be replaced by a
file write), call `remove_node`, and assert the result is `Err` AND that `load_nodes` still
contains the node — a removal whose tombstone never landed must not have half-applied. This is
the FED-01 "a failed removal surfaces an error instead of silently no-opping" criterion at the
storage layer.
Do not change any public signature and do not touch `load_invites`/`save_invites` (a separate
file with no cross-writer).
cd core && cargo test -p archipelago federation
- `cd core && cargo test -p archipelago federation` exits 0.
- `grep -Eq 'async fn test_remove_errors_when_tombstone_write_fails' core/archipelago/src/federation/storage.rs` succeeds.
- `grep -c 'FEDERATION_STORE_LOCK.lock().await' core/archipelago/src/federation/storage.rs` is at least 9.
- `cd core && cargo build -p archipelago` exits 0 with no new warnings in `federation::storage`
(dead-code warnings on unused `*_inner` fns mean a mutator was missed).
- `cd core && cargo test -p archipelago` exits 0 — no caller in `sync.rs`, `handlers.rs`,
`server.rs`, or `mesh/mod.rs` was broken by the refactor.
Every federation node-store mutator is serialized; a tombstone-write failure is proven to surface as an error with no half-applied removal.
## Trust Boundaries
| Boundary | Description |
|----------|-------------|
| federated peer → `federation::sync` → node store | A remote peer's state snapshot crosses into local persisted trust state |
| operator RPC (`federation.remove-node`) → node store | An authenticated local operator action mutates trust membership |
| process → `federation/nodes.json` on disk | Multiple concurrent async tasks write one file; a crash can leave it partial |
## STRIDE Threat Register
| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan |
|-----------|----------|-----------|----------|-------------|-----------------|
| T-01-01 | Tampering | `federation::storage` concurrent read-modify-write | high | mitigate | `FEDERATION_STORE_LOCK` held across the whole load-mutate-save cycle in every mutator (Tasks 1-2); regression test proves a removal survives a concurrent sync |
| T-01-02 | Elevation of Privilege | a removed (untrusted) peer regaining federation membership via the race | high | mitigate | Same lock; plus the pre-existing tombstone check in `merge_transitive_peers` and `handle_federation_peer_joined` is left intact and re-verified by `cargo test -p archipelago federation` |
| T-01-03 | Denial of Service | a partial `nodes.json` write on crash making the node list unreadable | medium | mitigate | Atomic temp-file + same-directory `fs::rename` in `save_nodes_inner` (Task 1) |
| T-01-04 | Denial of Service | lock contention stalling the federation RPC surface | low | accept | Federation writes are infrequent (90s loop + operator actions); `.lock().await` queues rather than rejects, and every critical section is a bounded file read+write |
- `cd core && cargo test -p archipelago federation` — green.
- `cd core && cargo test -p archipelago` — green (no caller regressions).
- The pre-fix failure of `test_remove_survives_concurrent_state_sync` is recorded in the SUMMARY as
evidence the test is fail-first and not vacuous.
- Every read-modify-write in `federation/storage.rs` is serialized behind one module-level async mutex with no re-entrancy path.
- The node list is written atomically (temp file + same-directory rename).
- Four new tests exist and pass; at least one is demonstrated to fail without the lock.
- Public signatures unchanged; the full `archipelago` test suite is green.