fix(01-01): route remaining federation mutators through the store lock (FED-01)

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>
This commit is contained in:
archipelago 2026-07-31 22:26:28 -04:00
parent bc9a210c75
commit 4b5367ebc4

View File

@ -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();