From 53b32010242e17d44cc9eb14262b51228b17ad79 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:07:20 +0000 Subject: [PATCH] fix(fleet): let a deliberate re-claim outrank an older signed eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unclaiming a device and re-claiming it left it permanently unreachable. The device looked healthy — owner, fleet key and attachment all intact locally — while every peer refused it with `evicted`, so it served its own LAN UI and was invisible to the fleet. ensure_fleet_network prunes any locally listed device the signed governance has removed, so one owner's admit loop can't resurrect a device another owner evicted. It decided on set membership alone, which cannot tell that case apart from a device this owner has deliberately re-claimed SINCE the eviction. The prune runs before the admit loop, so the re-claimed device was dropped first, every time, and the superseding RoleGrant was never authored. Membership itself converges last-writer-wins on the entry stamp and myownmesh-core already honours exactly this (`re_admitting_an_evicted_member_supersedes_the_tombstone`) — the grant just never got written, which made unclaim → re-claim a one-way door. So compare stamps. OwnedMember records claimed_at when the owner first adds it — first add only, since a later upsert is a label refresh driven by roster gossip and would otherwise push a device past every tombstone just by being talked about. signed_evicted returns pubkey → removal stamp instead of a bare set, dating each removal from the member log the daemon already returns alongside the verdict: `evicted` stays the authoritative verified answer to WHETHER a device is removed, and the log is read only for WHEN, so nothing here re-decides membership or trusts an unverified entry. Both unknowns fail toward the old behaviour rather than toward resurrection: a removal the log can't date sorts at u64::MAX, and a record written before claimed_at existed sorts as older than any eviction. A device stranded by this bug therefore recovers by being re-claimed once, which stamps it. The member-log JSON shape is load-bearing and pinned by test: myownmesh-core serialises Transition with the variant NESTED, not flattened. If that changed, eviction_stamps would silently date nothing, every removal would sort at u64::MAX and the prune would quietly go back to being unconditional. cargo test (200 node + protocol) and clippy -D warnings pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012vfwNJWeimvaQ9FGHqjM7i --- crates/allmystuff-protocol/src/app.rs | 21 +++ node/src/mesh.rs | 187 +++++++++++++++++++++++--- node/src/ownership.rs | 40 ++++++ 3 files changed, 232 insertions(+), 16 deletions(-) diff --git a/crates/allmystuff-protocol/src/app.rs b/crates/allmystuff-protocol/src/app.rs index 2320110..3c691ad 100644 --- a/crates/allmystuff-protocol/src/app.rs +++ b/crates/allmystuff-protocol/src/app.rs @@ -391,6 +391,24 @@ pub struct OwnedMember { /// label a gossip carries wins). #[serde(default)] pub label: String, + /// Unix seconds at which this owner first added the member, or `None` for a + /// record written before this field existed. + /// + /// Load-bearing for exactly one decision: whether a signed eviction still + /// applies. `ensure_fleet_network` prunes a locally listed device the signed + /// governance has removed, so one owner's admit loop can't resurrect a + /// device another owner evicted. Membership converges last-writer-wins on + /// the entry stamp, so an eviction only outranks a claim that came *before* + /// it — and without this the prune could not tell the two apart, which made + /// a deliberate re-claim of a previously evicted device impossible: the + /// prune ran before the admit loop every time, so the superseding grant the + /// governance layer would have honoured was never authored. + /// + /// `None` is treated as older than any eviction, so a legacy record keeps + /// the previous (always-prune) behaviour rather than being resurrected by + /// an upgrade. Re-claiming such a device stamps it and recovers it. + #[serde(default)] + pub claimed_at: Option, } /// A snapshot of an owner's fleet for the front-end: the shared key, the @@ -1670,10 +1688,13 @@ mod tests { OwnedMember { device: "my-laptop".into(), label: "My laptop".into(), + claimed_at: Some(1_700_000_000), }, OwnedMember { device: "spare-nuc".into(), label: "Spare NUC".into(), + // Unstamped: a member whose record predates the field. + claimed_at: None, }, ], }; diff --git a/node/src/mesh.rs b/node/src/mesh.rs index 739f9fc..da81b1d 100644 --- a/node/src/mesh.rs +++ b/node/src/mesh.rs @@ -6408,6 +6408,11 @@ impl Mesh { members.push(OwnedMember { device: NodeId::from(pubkey_part(id)), label, + // A projection of the signed roster for the + // GUI, not this owner's local record — the + // claim stamp lives on the latter and is + // read only by the eviction prune. + claimed_at: None, }); } } @@ -6451,6 +6456,7 @@ impl Mesh { members.push(OwnedMember { device: NodeId::from(canon.as_str()), label: m.label.clone(), + claimed_at: m.claimed_at, }); } member_roles @@ -6473,6 +6479,7 @@ impl Mesh { members.push(OwnedMember { device: NodeId::from(canon.as_str()), label, + claimed_at: None, }); } // Best-effort role for this device (it isn't in its own roster): @@ -6506,6 +6513,7 @@ impl Mesh { members.push(OwnedMember { device: NodeId::from(canon.as_str()), label: String::new(), + claimed_at: None, }); } member_roles @@ -7306,15 +7314,36 @@ impl Mesh { let signed_evicted = self.signed_evicted(&network).await; if !signed_evicted.is_empty() { let mut pruned = false; - for member in self.ownership.fleet_member_ids() { - if signed_evicted.contains(pubkey_part(&member)) { + for m in self.ownership.fleet_members() { + let member = m.device.to_string(); + let Some(&evicted_at) = signed_evicted.get(pubkey_part(&member)) else { + continue; + }; + // Compare stamps rather than mere set membership. Membership + // converges last-writer-wins, so an eviction only outranks a + // claim that came before it. A device this owner deliberately + // re-claimed AFTER the eviction is the later intent, and the + // admit loop below authors the superseding grant the governance + // layer already honours (`re_admitting_an_evicted_member_ + // supersedes_the_tombstone`). Pruning on set membership alone + // dropped it here first, every time, so that grant was never + // written and unclaim → re-claim was a one-way door. + // + // An unstamped (pre-upgrade) record sorts as older, so it keeps + // the previous always-prune behaviour; re-claiming stamps it. + if m.claimed_at.is_some_and(|at| at > evicted_at) { tracing::info!( - "pruning {} from the local fleet list — the signed governance evicted it", + "keeping {} — re-claimed after the signed eviction; re-admitting it", short_id(&member) ); - let _ = self.ownership.kick_member(&member); - pruned = true; + continue; } + tracing::info!( + "pruning {} from the local fleet list — the signed governance evicted it", + short_id(&member) + ); + let _ = self.ownership.kick_member(&member); + pruned = true; } if pruned { // Reflect the removal now: the authorised-controller cache and the @@ -7654,7 +7683,10 @@ impl Mesh { /// daemon that doesn't report the field), so a transient read failure or a /// version skew never prunes a live member — it just falls back to the old /// (re-asserting) behaviour. - async fn signed_evicted(self: &Arc, network: &str) -> std::collections::HashSet { + async fn signed_evicted( + self: &Arc, + network: &str, + ) -> std::collections::HashMap { let data = match self .client .request(&Request::GovernanceState { @@ -7663,17 +7695,9 @@ impl Mesh { .await { Ok(r) if r.ok => r.data.unwrap_or(Value::Null), - _ => return std::collections::HashSet::new(), + _ => return std::collections::HashMap::new(), }; - data.get("evicted") - .and_then(|v| v.as_array()) - .map(|arr| { - arr.iter() - .filter_map(|v| v.as_str()) - .map(|k| pubkey_part(k).to_string()) - .collect() - }) - .unwrap_or_default() + eviction_stamps(&data) } /// Refresh the authorised-controller cache ([`Mesh::fleet_authorized`]) @@ -12529,6 +12553,71 @@ fn unique_path(dir: &std::path::Path, name: &str) -> std::path::PathBuf { /// so a device id in display form (`pubkey-SUFFIX`, what `IdentityShow` and /// presence use) and bare form (`pubkey`, what the daemon delivers as a /// channel `from`) compare equal. +/// Date each device the fleet's signed governance has removed: canonical pubkey +/// → the unix second its removal was authored. +/// +/// Takes the raw `GovernanceState` reply. `evicted` is the authoritative, +/// already-verified answer to *whether* a device is removed; `state.member_log` +/// is read only for **when**, so nothing here re-decides membership or trusts an +/// unverified entry. The latest entry targeting a removed device IS its removal +/// — that is what made the verdict come out removed under the member log's +/// last-writer-wins fold — so the max stamp over that device's entries is the +/// tombstone's stamp without having to match variant kinds. +/// +/// A removed device the log can't date is recorded at `u64::MAX`, which outranks +/// every claim: an undatable eviction keeps the old always-prune behaviour +/// rather than being quietly waived. +fn eviction_stamps(data: &Value) -> std::collections::HashMap { + let removed: std::collections::HashSet = data + .get("evicted") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|v| v.as_str()) + .map(|k| pubkey_part(k).to_string()) + .collect() + }) + .unwrap_or_default(); + if removed.is_empty() { + return std::collections::HashMap::new(); + } + + let mut at: std::collections::HashMap = std::collections::HashMap::new(); + if let Some(log) = data + .get("state") + .and_then(|s| s.get("member_log")) + .and_then(|v| v.as_array()) + { + for entry in log { + let Some(stamp) = entry.get("at").and_then(|v| v.as_u64()) else { + continue; + }; + let Some(target) = entry + .get("variant") + .and_then(|v| v.get("target")) + .and_then(|v| v.as_str()) + else { + continue; + }; + let target = pubkey_part(target).to_string(); + if !removed.contains(&target) { + continue; + } + at.entry(target) + .and_modify(|s| *s = (*s).max(stamp)) + .or_insert(stamp); + } + } + + removed + .into_iter() + .map(|k| { + let stamp = at.get(&k).copied().unwrap_or(u64::MAX); + (k, stamp) + }) + .collect() +} + fn pubkey_part(id: &str) -> &str { if let Some((body, suffix)) = id.rsplit_once('-') { if suffix.len() == 5 && suffix.chars().all(|c| c.is_ascii_alphanumeric()) { @@ -12830,6 +12919,72 @@ fn parse_media(s: &str) -> MediaKind { #[cfg(test)] mod tests { + // ---- eviction stamps ------------------------------------------------- + // + // The JSON below matches what `GovernanceState` actually emits — verified + // against myownmesh-core's `Transition`, which serialises the variant as a + // NESTED object ({"at":..,"variant":{"kind":"evict","target":".."},..}), not + // flattened. If that ever changes, `eviction_stamps` silently dates nothing, + // every eviction sorts as u64::MAX and the prune goes back to being + // unconditional — so these pin the shape as much as the logic. + + #[test] + fn eviction_stamps_dates_each_removal_from_the_member_log() { + let data = serde_json::json!({ + "evicted": ["kvmkey"], + "state": {"member_log": [ + {"at": 100, "variant": {"kind": "role_grant", "target": "kvmkey", "role": "member"}, + "signers": ["owner"], "signatures": ["s"]}, + {"at": 200, "variant": {"kind": "evict", "target": "kvmkey"}, + "signers": ["owner"], "signatures": ["s"]}, + // A different device's entries must not leak into the stamp. + {"at": 900, "variant": {"kind": "evict", "target": "otherkey"}, + "signers": ["owner"], "signatures": ["s"]}, + ]} + }); + + let stamps = super::eviction_stamps(&data); + + assert_eq!(stamps.len(), 1, "only devices in `evicted` are dated"); + assert_eq!(stamps.get("kvmkey"), Some(&200)); + } + + #[test] + fn an_undatable_eviction_outranks_every_claim() { + // No member log to read (an older daemon, a read that lost the field). + // The device is still removed, so it must stay pruned rather than being + // waived by a stamp we couldn't find. + let data = serde_json::json!({"evicted": ["kvmkey"]}); + + let stamps = super::eviction_stamps(&data); + + assert_eq!(stamps.get("kvmkey"), Some(&u64::MAX)); + } + + #[test] + fn no_evictions_dates_nothing() { + let data = serde_json::json!({"evicted": [], "state": {"member_log": []}}); + assert!(super::eviction_stamps(&data).is_empty()); + } + + #[test] + fn eviction_stamps_canonicalises_the_display_suffix() { + // The log and the `evicted` list may carry either the bare pubkey or a + // display id; the local member list is canonical, so both sides must + // collapse or the lookup misses and the prune runs anyway. + let data = serde_json::json!({ + "evicted": ["kvmkey-AB12C"], + "state": {"member_log": [ + {"at": 500, "variant": {"kind": "evict", "target": "kvmkey"}, + "signers": ["owner"], "signatures": ["s"]}, + ]} + }); + + let stamps = super::eviction_stamps(&data); + + assert_eq!(stamps.get("kvmkey"), Some(&500)); + } + use super::*; #[test] diff --git a/node/src/ownership.rs b/node/src/ownership.rs index 45595ac..f8f2ef6 100644 --- a/node/src/ownership.rs +++ b/node/src/ownership.rs @@ -529,6 +529,15 @@ fn upsert_member_into(members: &mut Vec, device: &str, label: &str) members.push(OwnedMember { device: NodeId::from(canon), label: label.to_string(), + // Stamped on first add only. A later upsert is a label refresh, + // not a fresh claim, so it must not move this — the stamp is + // what tells `ensure_fleet_network` whether a signed eviction + // predates the claim, and a gossip-driven label touch would + // otherwise keep pushing it past every tombstone. + // An unset clock stamps this low, which reads as older than any + // eviction — the conservative direction (prune), not the one + // that resurrects a device. + claimed_at: Some(crate::cec::now_secs()), }); true } @@ -873,6 +882,36 @@ mod tests { assert!(!resolved_claimed); } + #[test] + fn upsert_stamps_the_first_add_and_a_label_refresh_never_moves_it() { + // The stamp dates this owner's CLAIM of the device, and the eviction + // prune in `ensure_fleet_network` compares it against the tombstone. A + // later upsert is a label refresh driven by roster gossip, not a fresh + // claim — if it moved the stamp, a device would drift past every + // eviction just by being talked about. + let mut members = Vec::new(); + assert!(upsert_member_into(&mut members, "k1", "Laptop")); + let stamped = members[0].claimed_at.expect("first add must be stamped"); + + assert!(upsert_member_into(&mut members, "k1", "My laptop")); + assert_eq!( + members[0].claimed_at, + Some(stamped), + "a label refresh moved the claim stamp" + ); + } + + #[test] + fn a_record_without_a_claim_stamp_still_loads() { + // Records written before the field existed must keep loading: this is + // the owner's fleet list, and a parse failure would drop real members. + // `None` reads as older than any eviction, so such a device keeps the + // previous always-prune behaviour rather than being resurrected by an + // upgrade. + let m: OwnedMember = serde_json::from_str(r#"{"device":"k1","label":"Laptop"}"#).unwrap(); + assert_eq!(m.claimed_at, None); + } + #[test] fn upsert_dedups_by_canonical_pubkey_and_refreshes_labels() { let mut members = Vec::new(); @@ -922,6 +961,7 @@ mod tests { dev.inner.lock().fleet_members.push(OwnedMember { device: "old-owner".into(), label: "Old".into(), + claimed_at: None, }); assert!(dev.try_accept_claim("new-owner"));