feat(01-05): surface federation sync failures to the operator (FED-02)
A failed federation sync existed only as a `debug!` line on the node, so a peer that had not synced in days looked identical in the UI to one that synced a minute ago. Now the failure is persisted per peer and rendered. - `FederatedNode.last_sync_error` / `.last_sync_error_at` — the failure-side mirror of the existing `last_transport` / `last_transport_at` pair. - `federation::record_sync_result(data_dir, did, outcome)` — records the message on `Err`, CLEARS both fields on `Ok` so the badge disappears when the peer recovers. Runs under FEDERATION_STORE_LOCK via the `*_inner` load/save convention established by plan 01-01. An unknown DID is a silent Ok that writes nothing, so a peer removed mid-pass is never resurrected by an in-flight sync's error write. Skips the save entirely when nothing changed, keeping the steady state read-only rather than rewriting nodes.json (and contending for the lock) every 90s. - Message truncated to MAX_SYNC_ERROR_CHARS (256), counted in chars not bytes so truncation cannot split a UTF-8 sequence (T-01-18). - The 90s auto-sync loop calls it on both arms; the existing `debug!` line is kept — persisting is additive, not a replacement for logs. - `federation.list-nodes` emits both fields when set, omits them when unset. - NodeList renders a red SYNC badge beside the transport badge on both the trusted-node and peer rows, message + age in the `title` so the row stays single-line. Tests (written first, confirmed failing — 16 compile errors, E0425 on `record_sync_result` and E0609 on `last_sync_error`): - persists_error / success_clears_error / missing_did_is_noop / on_empty_store_is_noop / truncates_long_error - NodeList: badge present when set, ABSENT when unset (the guard against a badge that always renders), and present on an observer peer row. cargo test -p archipelago federation — 42 passed, 0 failed. vitest NodeList.test.ts — 4 passed. npm run build — green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
archipelago
co-authored by
Claude Opus 5
Claude Opus 5
parent
262998747e
commit
454388226c
@@ -199,6 +199,8 @@ pub async fn accept_invite(
|
||||
fips_npub: fips_npub.clone(),
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
};
|
||||
|
||||
add_node(data_dir, node.clone()).await?;
|
||||
|
||||
@@ -21,7 +21,7 @@ pub(crate) use storage::load_invites;
|
||||
#[allow(unused_imports)]
|
||||
pub use storage::{
|
||||
add_node, fips_npub_for_onion, load_nodes, load_removed_dids, record_peer_transport,
|
||||
remove_node, save_nodes, set_trust_level, update_node,
|
||||
record_sync_result, remove_node, save_nodes, set_trust_level, update_node,
|
||||
};
|
||||
pub use sync::{build_local_state, deploy_to_peer, sync_with_peer, sync_with_peer_by_did};
|
||||
pub use types::{AppStatus, FederatedNode, NodeStateSnapshot, TrustLevel};
|
||||
|
||||
@@ -174,6 +174,69 @@ pub async fn record_peer_transport(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Upper bound on a persisted `last_sync_error` message, in characters.
|
||||
///
|
||||
/// T-01-18: the periodic loop records an outcome for every peer on every
|
||||
/// pass, so an unbounded error string (a peer echoing a huge body, a deep
|
||||
/// `anyhow` chain) would be rewritten into `nodes.json` every 90 seconds.
|
||||
/// Counted in `char`s, not bytes, so truncation can never split a UTF-8
|
||||
/// sequence and produce a file that fails to deserialize.
|
||||
pub(crate) const MAX_SYNC_ERROR_CHARS: usize = 256;
|
||||
|
||||
/// Record the outcome of the most recent federation sync attempt with a peer.
|
||||
///
|
||||
/// `Err(msg)` stores the message (truncated to `MAX_SYNC_ERROR_CHARS`) plus
|
||||
/// the current time; `Ok(())` clears both fields so a recovered peer stops
|
||||
/// showing a stale error badge. Only the named DID is touched.
|
||||
///
|
||||
/// Why this exists: both periodic sync loops previously logged failures at
|
||||
/// `debug!` and nothing else, so a peer that had not synced in days looked
|
||||
/// identical in the UI to one that synced a minute ago (FED-02).
|
||||
///
|
||||
/// A DID that isn't in the node list is a silent `Ok` and writes nothing —
|
||||
/// a peer the operator removed while a sync was in flight must not be
|
||||
/// resurrected by that sync's error write. This function never creates a
|
||||
/// node entry.
|
||||
pub async fn record_sync_result(
|
||||
data_dir: &Path,
|
||||
did: &str,
|
||||
outcome: Result<(), String>,
|
||||
) -> Result<()> {
|
||||
let _guard = FEDERATION_STORE_LOCK.lock().await;
|
||||
let mut nodes = load_nodes_inner(data_dir).await?;
|
||||
|
||||
let Some(node) = nodes.iter_mut().find(|n| n.did == did) else {
|
||||
// Unknown/removed peer: nothing to record against. Not an error.
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let changed = match outcome {
|
||||
Err(msg) => {
|
||||
let truncated: String = msg.chars().take(MAX_SYNC_ERROR_CHARS).collect();
|
||||
node.last_sync_error = Some(truncated);
|
||||
node.last_sync_error_at = Some(chrono::Utc::now().to_rfc3339());
|
||||
true
|
||||
}
|
||||
Ok(()) => {
|
||||
// Peer recovered — drop the badge rather than leaving a stale one.
|
||||
let had_error = node.last_sync_error.is_some() || node.last_sync_error_at.is_some();
|
||||
node.last_sync_error = None;
|
||||
node.last_sync_error_at = None;
|
||||
had_error
|
||||
}
|
||||
};
|
||||
|
||||
// A healthy peer stays healthy on most passes, and the surviving loop
|
||||
// calls this for every peer every 90s. Skipping the write when nothing
|
||||
// actually changed keeps the steady state read-only, so this failure
|
||||
// surfacing doesn't add a rewrite of nodes.json (and lock contention
|
||||
// with `federation.remove-node`) every single pass.
|
||||
if changed {
|
||||
save_nodes_inner(data_dir, &nodes).await?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub async fn save_nodes(data_dir: &Path, nodes: &[FederatedNode]) -> Result<()> {
|
||||
let _guard = FEDERATION_STORE_LOCK.lock().await;
|
||||
save_nodes_inner(data_dir, nodes).await
|
||||
@@ -435,6 +498,8 @@ mod tests {
|
||||
fips_npub: None,
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -750,6 +815,122 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// FED-02: a failed sync must leave a durable, per-peer record instead of
|
||||
/// only a `debug!` line, so the operator can tell a peer that hasn't
|
||||
/// synced in days from one that synced a minute ago.
|
||||
#[tokio::test]
|
||||
async fn test_record_sync_result_persists_error() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
add_node(dir.path(), make_node("did:key:z1", "a.onion"))
|
||||
.await
|
||||
.unwrap();
|
||||
add_node(dir.path(), make_node("did:key:z2", "b.onion"))
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
record_sync_result(dir.path(), "did:key:z1", Err("peer unreachable".to_string()))
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let nodes = load_nodes(dir.path()).await.unwrap();
|
||||
let n1 = nodes.iter().find(|n| n.did == "did:key:z1").unwrap();
|
||||
assert_eq!(n1.last_sync_error.as_deref(), Some("peer unreachable"));
|
||||
assert!(
|
||||
n1.last_sync_error_at.is_some(),
|
||||
"an error must carry the time it happened"
|
||||
);
|
||||
|
||||
let n2 = nodes.iter().find(|n| n.did == "did:key:z2").unwrap();
|
||||
assert!(
|
||||
n2.last_sync_error.is_none(),
|
||||
"only the failing peer may be marked"
|
||||
);
|
||||
}
|
||||
|
||||
/// FED-02 adjacency edge: the badge must not outlive the failure. A
|
||||
/// successful sync clears the previously recorded error for that peer.
|
||||
#[tokio::test]
|
||||
async fn test_record_sync_result_success_clears_error() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
add_node(dir.path(), make_node("did:key:z1", "a.onion"))
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
record_sync_result(dir.path(), "did:key:z1", Err("timed out".to_string()))
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(load_nodes(dir.path()).await.unwrap()[0]
|
||||
.last_sync_error
|
||||
.is_some());
|
||||
|
||||
record_sync_result(dir.path(), "did:key:z1", Ok(())).await.unwrap();
|
||||
|
||||
let nodes = load_nodes(dir.path()).await.unwrap();
|
||||
assert!(
|
||||
nodes[0].last_sync_error.is_none(),
|
||||
"a recovered peer must not keep its stale error badge"
|
||||
);
|
||||
assert!(
|
||||
nodes[0].last_sync_error_at.is_none(),
|
||||
"the error timestamp must clear with the error"
|
||||
);
|
||||
}
|
||||
|
||||
/// FED-02: a peer removed mid-pass must not be resurrected by the
|
||||
/// in-flight sync attempt's error write. Missing DID is a silent Ok.
|
||||
#[tokio::test]
|
||||
async fn test_record_sync_result_missing_did_is_noop() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
add_node(dir.path(), make_node("did:key:z1", "a.onion"))
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
record_sync_result(dir.path(), "did:key:zGONE", Err("unreachable".to_string()))
|
||||
.await
|
||||
.expect("recording against an unknown DID must be a silent Ok, not an error");
|
||||
|
||||
let nodes = load_nodes(dir.path()).await.unwrap();
|
||||
assert_eq!(nodes.len(), 1, "a removed peer must not be resurrected");
|
||||
assert_eq!(nodes[0].did, "did:key:z1");
|
||||
assert!(nodes[0].last_sync_error.is_none());
|
||||
}
|
||||
|
||||
/// FED-02 empty edge: recording against an empty node store writes
|
||||
/// nothing and errors on nobody (the zero-federated-node sync pass).
|
||||
#[tokio::test]
|
||||
async fn test_record_sync_result_on_empty_store_is_noop() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
record_sync_result(dir.path(), "did:key:zAny", Err("boom".to_string()))
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(load_nodes(dir.path()).await.unwrap().is_empty());
|
||||
assert!(
|
||||
!dir.path().join(FEDERATION_DIR).join(NODES_FILE).exists(),
|
||||
"a no-op must not create the node file"
|
||||
);
|
||||
}
|
||||
|
||||
/// T-01-18: an unbounded error string must not bloat nodes.json on every
|
||||
/// failed pass. The recorded message is truncated before persistence.
|
||||
#[tokio::test]
|
||||
async fn test_record_sync_result_truncates_long_error() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
add_node(dir.path(), make_node("did:key:z1", "a.onion"))
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let huge = "x".repeat(5000);
|
||||
record_sync_result(dir.path(), "did:key:z1", Err(huge)).await.unwrap();
|
||||
|
||||
let nodes = load_nodes(dir.path()).await.unwrap();
|
||||
let msg = nodes[0].last_sync_error.as_deref().unwrap();
|
||||
assert!(
|
||||
msg.chars().count() <= MAX_SYNC_ERROR_CHARS,
|
||||
"recorded error must be truncated to {MAX_SYNC_ERROR_CHARS} chars, got {}",
|
||||
msg.chars().count()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_update_node_state() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
|
||||
@@ -187,6 +187,8 @@ async fn merge_transitive_peers(
|
||||
fips_npub: hint.fips_npub.clone(),
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
});
|
||||
added += 1;
|
||||
}
|
||||
@@ -375,6 +377,8 @@ mod tests {
|
||||
fips_npub: Some("npub1a".into()),
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
},
|
||||
FederatedNode {
|
||||
did: "did:key:zObserver".into(),
|
||||
@@ -388,6 +392,8 @@ mod tests {
|
||||
fips_npub: Some("npub1b".into()),
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
},
|
||||
FederatedNode {
|
||||
did: "did:key:zUntrusted".into(),
|
||||
@@ -401,6 +407,8 @@ mod tests {
|
||||
fips_npub: None,
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
},
|
||||
];
|
||||
let state = build_local_state(
|
||||
@@ -443,6 +451,8 @@ mod tests {
|
||||
fips_npub: None,
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
}],
|
||||
)
|
||||
.await
|
||||
|
||||
@@ -77,6 +77,23 @@ pub struct FederatedNode {
|
||||
/// RFC 3339 timestamp of the last_transport value.
|
||||
#[serde(default)]
|
||||
pub last_transport_at: Option<String>,
|
||||
/// Error from the most recent federation sync attempt with this peer,
|
||||
/// `None` when that attempt succeeded. Written back after every attempt
|
||||
/// (same shape as `last_transport`, for the failure side) so the
|
||||
/// operator can tell a peer that hasn't synced in days from one that
|
||||
/// synced a minute ago — previously a failed sync existed only as a
|
||||
/// `debug!` log line, making the two indistinguishable in the UI.
|
||||
///
|
||||
/// Truncated to `storage::MAX_SYNC_ERROR_CHARS` before it is persisted
|
||||
/// so a pathological error can't bloat `nodes.json` on every pass, and
|
||||
/// carries only the error's own display string — never credential
|
||||
/// material (see FED-02's privacy prohibition).
|
||||
#[serde(default)]
|
||||
pub last_sync_error: Option<String>,
|
||||
/// RFC 3339 timestamp of the last_sync_error value. Cleared together
|
||||
/// with `last_sync_error` when the peer recovers.
|
||||
#[serde(default)]
|
||||
pub last_sync_error_at: Option<String>,
|
||||
}
|
||||
|
||||
/// State snapshot received from a federated peer during sync.
|
||||
@@ -204,6 +221,8 @@ mod tests {
|
||||
fips_npub: None,
|
||||
last_transport: None,
|
||||
last_transport_at: None,
|
||||
last_sync_error: None,
|
||||
last_sync_error_at: None,
|
||||
};
|
||||
let json = serde_json::to_string(&node).unwrap();
|
||||
let parsed: FederatedNode = serde_json::from_str(&json).unwrap();
|
||||
|
||||
Reference in New Issue
Block a user