---
phase: 01-federation-mesh-hardening
plan: 01
subsystem: federation-storage
tags: [federation, concurrency, tokio-mutex, atomic-write, FED-01]
status: complete
dependency-graph:
  requires: []
  provides:
    - FEDERATION_STORE_LOCK
    - atomic nodes.json write (temp+rename)
  affects:
    - core/archipelago/src/federation/sync.rs (caller, unchanged signatures)
    - core/archipelago/src/api/rpc/federation/handlers.rs (caller, unchanged signatures)
tech-stack:
  added:
    - tokio::sync::Mutex (module-level static, const_new)
  patterns:
    - "Async static lock for a racy on-disk resource (mirrors update.rs's UPDATE_OP_LOCK, but .lock().await not try_lock, since federation writes must queue not reject)"
    - "Public fn = thin lock wrapper; private *_inner fn = lock-free body, so multi-step critical sections (tombstone + node-list save) don't self-deadlock on a non-reentrant Mutex"
key-files:
  created: []
  modified:
    - core/archipelago/src/federation/storage.rs
decisions:
  - "set_trust_level pulled forward into Task 1's commit (2f99db5e) rather than Task 2, because test_concurrent_writes_do_not_lose_updates (a Task 1 required-green test) races add_node against set_trust_level and needs it locked to pass"
  - "record_peer_transport and update_node route through *_inner under a single held guard rather than calling the public load_nodes/save_nodes, closing the same unlocked-two-call race the whole plan exists to fix"
  - "Tombstone-write failure test (test_remove_errors_when_tombstone_write_fails) forces failure by pre-creating removed-nodes.json as a directory, not by mocking I/O — no existing I/O mocking harness in this module, and this is the simplest deterministic failure induction available"
metrics:
  duration: "resumed/completed in this session; Task 1 was previously committed 2026-07-30"
  completed: 2026-07-31
---

# Phase 01 Plan 01: Serialize the federation node store and make removal stick (FED-01) Summary

Closed the concurrency race that let a removed federation node reappear: every load-mutate-save
cycle in `federation/storage.rs` is now serialized behind one module-level `FEDERATION_STORE_LOCK`,
the node-list write is atomic (temp file + same-directory rename), and a tombstone-write failure
now surfaces as an `Err` instead of a silent no-op.

## What Was Built

- `FEDERATION_STORE_LOCK: tokio::sync::Mutex<()>` (module-level static, `const_new`), documented
  with the concrete failure it prevents (a `federation.remove-node` RPC racing the 90s auto-sync
  loop's stale pre-removal snapshot).
- `load_nodes_inner` / `save_nodes_inner` / `tombstone_did_inner` / `untombstone_did_inner`:
  lock-free bodies so `remove_node` (tombstone + save) and `add_node` (untombstone + save) can
  each hold the guard across their whole multi-step critical section without self-deadlocking
  (`tokio::sync::Mutex` is not re-entrant).
- Every public mutator now routes through the lock: `load_nodes`, `save_nodes`, `add_node`,
  `remove_node`, `tombstone_did`, `untombstone_did`, `set_trust_level`, `update_node`,
  `update_node_state`, `record_peer_transport` — 10 `.lock().await` call sites total.
- `save_nodes_inner` writes atomically: serialize to `nodes.json.tmp` in the same directory,
  then `fs::rename` onto `nodes.json` — a crash mid-write can never leave a partial file for a
  concurrent reader.
- Four new regression tests in `federation::storage::tests`:
  - `test_concurrent_writes_do_not_lose_updates` — races `add_node` against `set_trust_level`
    via real `tokio::spawn` tasks, 40 iterations; asserts both writes persist.
  - `test_remove_survives_concurrent_state_sync` — races `remove_node` against a 12-task burst
    of `update_node_state` calls, 30 iterations; asserts the removed DID stays removed and
    tombstoned.
  - `test_remove_last_node_leaves_empty_list` — removing the sole federated node returns/loads
    an empty `Vec`, not an error.
  - `test_remove_errors_when_tombstone_write_fails` — pre-creates `removed-nodes.json` as a
    directory so the tombstone write fails; asserts `remove_node` returns `Err` AND the node
    list is untouched (no half-applied removal).

## Task Execution Note (continuation)

Task 1 (the tracer: lock + atomic write + first three tests) was already committed in a prior
session (`2f99db5e`, 2026-07-30) and pushed. This session picked up as a continuation: verified
Task 1's commit and tests were real and green, then completed Task 2 — routing
`record_peer_transport` and `update_node` through the lock (they still called the public,
separately-locked `load_nodes`/`save_nodes` instead of the `*_inner` pair under one guard) and
adding the tombstone-failure test. No SUMMARY/STATE/ROADMAP update had been done for this plan
before this session; that gap is closed by this document.

## Pre-fix Failure Evidence (Task 1, historical)

Task 1's commit message (`2f99db5e`) records that both `test_concurrent_writes_do_not_lose_updates`
and `test_remove_survives_concurrent_state_sync` were proven fail-first before the lock existed:
using real `tokio::spawn` tasks (not just `tokio::join!`, since `remove_node`'s extra tombstone
I/O hop structurally biases a simple 2-task race toward the safe ordering) reliably reproduced
both the lost concurrent write and the removed-node-reappears bug pre-fix. This session did not
re-run the pre-fix reproduction (the fix and lock already exist on disk); the historical evidence
is carried forward from the Task 1 commit message since no separate SUMMARY captured it at the
time.

## Verification

- `cd core && cargo test -p archipelago federation::storage` — **14/14 passed, 0 failed**
  (11 pre-existing + 3 new from Task 1's earlier commit + this session's
  `test_remove_errors_when_tombstone_write_fails`).
- `cd core && cargo build -p archipelago` — succeeds, no new warnings in `federation::storage`
  (no dead-code warnings on any `*_inner` fn — confirms every mutator is wired through).
- Acceptance-criteria greps: `FEDERATION_STORE_LOCK` count 16, `.lock().await` count 10 (≥9
  required), `tokio::sync::Mutex::const_new` count 1, `fs::rename` count 1, all four new test
  function names present, `flavor = "multi_thread"` present.
- **`cd core && cargo test -p archipelago` (full suite) — NOT clean this session.** The
  workspace test binary fails to *compile*, but the failure is in
  `core/archipelago/src/api/rpc/package/install.rs:592` (a tuple-pattern-vs-`Result` mismatch on
  a `.await?` line marked "Not Committed Yet" by `git blame` at the time of this run) — a file
  this plan never touches, mid-edit by a different, concurrent agent session in this shared
  checkout (`git status` at commit time showed `install.rs`, `config.rs`, `dependencies.rs`,
  `secrets.rs`, and several `neode-ui` files dirty, none authored by this plan). This is the
  exact shared-tree hazard the task's hard constraints warn about, not a regression from this
  plan's change. `federation::storage`'s own test binary (scoped `cargo test -p archipelago
  federation::storage`) compiles and passes clean, and `cargo build -p archipelago` (non-test)
  also succeeds — the compile error is specific to the test-cfg path in `install.rs`, unrelated
  to `storage.rs`. Recorded honestly per the task's own instruction rather than declared green;
  re-run `cargo test -p archipelago` once the other in-flight session's `install.rs` edit lands
  or is reverted.

## Deviations from Plan

### Auto-fixed Issues

**1. [Rule 3 - blocking issue, scope-bounded] Full-suite verification blocked by a concurrent
agent's uncommitted edit in an unrelated file**
- **Found during:** final verification step (`cargo test -p archipelago`)
- **Issue:** `install.rs:592` fails to compile (tuple destructure not wrapped in `Ok(...)`
  against a function that now returns `Result`), per `git blame` an uncommitted, in-progress
  edit by a different session.
- **Fix:** None applied — out of this plan's scope per the SCOPE BOUNDARY rule (issue not caused
  by this plan's changes, and touching a file another agent is actively editing risks clobbering
  their work). Logged here and left for that session to resolve.
- **Files modified:** none (no fix applied)

No other deviations — the rest of this plan (routing `record_peer_transport`/`update_node`
through the lock, adding the failure test) executed exactly as written in Task 2's `<action>`.

## Known Stubs

None.

## Self-Check: PASSED

- `core/archipelago/src/federation/storage.rs` — FOUND (modified, contains `FEDERATION_STORE_LOCK`,
  `record_peer_transport`, `update_node`, `test_remove_errors_when_tombstone_write_fails`).
- Commit `4b5367eb` — FOUND in `git log --oneline`.
- Commit `2f99db5e` (Task 1, prior session) — FOUND in `git log --oneline`.
- Push to `gitea-ai main` — confirmed (`bc9a210c..4b5367eb main -> main`).