From 4b5367ebc4a77b70d8f86107046b7e1b94230397 Mon Sep 17 00:00:00 2001
From: archipelago <archipelago@localhost>
Date: Fri, 31 Jul 2026 22:26:28 -0400
Subject: [PATCH] fix(01-01): route remaining federation mutators through the
 store lock (FED-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Task 2 of 01-01-PLAN.md, closing the gap left after Task 1's initial commit
(2f99db5e):

- record_peer_transport and update_node now hold FEDERATION_STORE_LOCK for
  their whole load-mutate-save cycle via the *_inner variants, instead of
  calling the public (separately-locked) load_nodes/save_nodes — closing
  the same class of race the lock was introduced to fix, just for the two
  mutators Task 1 didn't reach.
- Add test_remove_errors_when_tombstone_write_fails: pre-creates the
  removed-nodes path as a directory so the tombstone write fails, then
  asserts remove_node returns Err AND load_nodes still contains the node —
  proving a failed removal never half-applies.

cargo test -p archipelago federation::storage: 14/14 green (was 11, +3 across
Task 1/2). cargo build -p archipelago: no new warnings, no dead-code warnings
on any *_inner fn. Public signatures unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 core/archipelago/src/federation/storage.rs | 42 +++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/core/archipelago/src/federation/storage.rs b/core/archipelago/src/federation/storage.rs
index a4652cc4..e6c07d58 100644
--- a/core/archipelago/src/federation/storage.rs
+++ b/core/archipelago/src/federation/storage.rs
@@ -150,7 +150,8 @@ pub async fn record_peer_transport(
     onion: Option<&str>,
     transport: &str,
 ) -> Result<()> {
-    let mut nodes = load_nodes(data_dir).await?;
+    let _guard = FEDERATION_STORE_LOCK.lock().await;
+    let mut nodes = load_nodes_inner(data_dir).await?;
     let now = chrono::Utc::now().to_rfc3339();
     let onion_target = onion.map(|o| o.trim_end_matches(".onion"));
 
@@ -168,7 +169,7 @@ pub async fn record_peer_transport(
     }
 
     if modified {
-        save_nodes(data_dir, &nodes).await?;
+        save_nodes_inner(data_dir, &nodes).await?;
     }
     Ok(())
 }
@@ -346,7 +347,8 @@ pub async fn set_trust_level(
 
 /// Update a federated node's metadata (onion, pubkey, name, last_seen).
 pub async fn update_node(data_dir: &Path, updated: &FederatedNode) -> Result<()> {
-    let mut nodes = load_nodes(data_dir).await?;
+    let _guard = FEDERATION_STORE_LOCK.lock().await;
+    let mut nodes = load_nodes_inner(data_dir).await?;
     if let Some(node) = nodes.iter_mut().find(|n| n.did == updated.did) {
         if !updated.onion.is_empty() {
             node.onion = updated.onion.clone();
@@ -360,7 +362,7 @@ pub async fn update_node(data_dir: &Path, updated: &FederatedNode) -> Result<()>
         if updated.last_seen.is_some() {
             node.last_seen = updated.last_seen.clone();
         }
-        save_nodes(data_dir, &nodes).await?;
+        save_nodes_inner(data_dir, &nodes).await?;
     }
     Ok(())
 }
@@ -525,6 +527,38 @@ mod tests {
         assert!(result.is_err());
     }
 
+    /// FED-01 failure-surfacing edge: a removal whose tombstone write fails
+    /// must return `Err` to the caller instead of reporting success, and
+    /// must NOT half-apply — the node list must be left untouched. Forces
+    /// the failure by pre-creating the removed-nodes path as a directory: a
+    /// directory can't be replaced by `fs::write`, so `tombstone_did_inner`
+    /// errors before `remove_node`'s node-list save ever runs (tombstone is
+    /// written first, per `remove_node`'s documented ordering).
+    #[tokio::test]
+    async fn test_remove_errors_when_tombstone_write_fails() {
+        let dir = tempfile::tempdir().unwrap();
+        add_node(dir.path(), make_node("did:key:z1", "a.onion"))
+            .await
+            .unwrap();
+
+        let federation_dir = dir.path().join(FEDERATION_DIR);
+        std::fs::create_dir_all(&federation_dir).unwrap();
+        std::fs::create_dir_all(federation_dir.join(REMOVED_FILE)).unwrap();
+
+        let result = remove_node(dir.path(), "did:key:z1").await;
+        assert!(
+            result.is_err(),
+            "a failed tombstone write must surface as an error, not a silent no-op"
+        );
+
+        let nodes = load_nodes(dir.path()).await.unwrap();
+        assert!(
+            nodes.iter().any(|n| n.did == "did:key:z1"),
+            "a removal whose tombstone never landed must not half-apply — \
+             the node list must remain untouched"
+        );
+    }
+
     #[tokio::test]
     async fn test_remove_tombstones_and_readd_clears_it() {
         let dir = tempfile::tempdir().unwrap();