Skip to content

Commit 67ddedb

Browse files
dirvineclaude
andcommitted
fix(home): a stored canonical pointer to a retired Home must stop governing (#449)
Independent review r3 of 413028e: the remaining P2 was only half-fixed. Filtering withdrawn groups in `DaemonView::home_pointer` stops FUTURE publication, but the prior `(HomePointer, "home")` record stays in `OwnerSyncStore`. Reconciliation does nothing when the view returns `None`, tombstone retention never clears owner-sync state, and `canonical_home` returns the stored value without consulting the locally known tombstone. So the real lifecycle — advertise, withdraw, then reconcile/restart — left the device yielding to a dead pointer forever: `provision_home` hit the suppression branch and returned instead of creating a usable replacement, and `GET /home` reported `elsewhere` for a group that no longer exists. Permanently, across restarts. The previous test could not detect this: it inserted an already-withdrawn group into an EMPTY store, so no stored pointer ever governed. Fixed on BOTH sides, keyed on the same proof. Proof of retirement is LOCAL: the group is in our own roster AND carries the terminal `withdrawn` flag. Inability to see the group is deliberately NOT proof — a canonical Home on an unreachable device is unknown, not retired, and treating unknown as retired would let any partitioned device mint over the owner's real Home and refork the space this issue exists to unify. - Consumer: new `effective_canonical_home()` backs BOTH `resolve_home` and `provision_home`, so a provably retired pointer stops governing. - Producer: `home_pointer_mint_decision` gained a `stored_is_retired` override, fed by the new `SyncDaemonView::canonical_pointer_is_retired`. Without this the fix is still broken: the ordering rule refuses EVERY replacement, because a replacement Home is always NEWER than the dead one it replaces, so the register would keep naming a tombstone forever. `canonical_pointer_is_retired` fails safe on a contended roster lock (answers `false`), so a busy daemon never mints over a live canonical Home. Regressions: - `a_retired_advertised_home_does_not_suppress_its_replacement` — the real lifecycle: advertise, withdraw, reconcile/restart, assert a NEW usable Home is provisioned. Verified to FAIL against pre-fix behaviour before being kept ("a pointer to a Home we hold and know to be retired must stop governing"). - `an_unreachable_remote_home_is_never_treated_as_retired` — a Home we merely cannot see keeps governing, and we still yield rather than fork. - `a_provably_retired_pointer_can_be_replaced_by_a_newer_home` — and, without proof, the ordering rule still stands. Gates on this exact tree, separate commands, actual exits: cargo fmt --all -> 0 cargo clippy --all-features --all-targets -- -D warnings -> 0 cargo clippy --all-features --lib --bins -- -D warnings -D clippy::panic -D clippy::unwrap_used -D clippy::expect_used -> 0 cargo check --workspace --all-targets -> 0 RUSTDOCFLAGS="-D warnings" cargo doc --all-features --no-deps -> 0 cargo nextest run --workspace --all-features --no-fail-fast -> 0 (3322 passed, 0 failed, 295 skipped) One earlier run of the same tree failed `inbound_redial_of_suppressed_peer_emits_no_peer_connected` at `.expect("alice connects bob")` — a QUIC dial under full-suite load, the #316 family. Neither agent in that test is built `with_user_key`, so `owner_sync` is never constructed and every line changed here sits behind an owner-keypair guard; it also passes in isolation (2.3s) and the re-run above is green. Not attributed to this diff and not claimed resolved. #449 remains partial and open: no adoption, no retirement, enrolled devices only. Refs #449 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ovKUXBeg5GZ51YECS9p51
1 parent 413028e commit 67ddedb

3 files changed

Lines changed: 212 additions & 20 deletions

File tree

src/owner_sync.rs

Lines changed: 70 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1899,6 +1899,7 @@ fn home_pointer_mint_decision(
18991899
desired: &SyncValue,
19001900
stored: Option<&SyncValue>,
19011901
local_agent_hex: &str,
1902+
stored_is_retired: bool,
19021903
) -> bool {
19031904
let SyncValue::HomePointer {
19041905
group_id: desired_id,
@@ -1922,6 +1923,14 @@ fn home_pointer_mint_decision(
19221923
return true; // defensively: a foreign kind is not a Home winner
19231924
};
19241925

1926+
// r3 P2: the slot is held by a Home we can PROVE is retired. Take it —
1927+
// the ordering rule below would otherwise refuse, because a replacement
1928+
// Home is always newer than the dead one, leaving every device yielding
1929+
// to a tombstone forever.
1930+
if stored_is_retired && desired_id != stored_id {
1931+
return true;
1932+
}
1933+
19251934
if desired_id == stored_id {
19261935
// Single-writer refresh: only the Home's designated primary agent,
19271936
// and only when the value actually changed. `mint` would no-op on an
@@ -1977,6 +1986,14 @@ pub trait SyncDaemonView: Send + Sync + 'static {
19771986
display_name: Option<String>,
19781987
machine_name: Option<String>,
19791988
);
1989+
/// Whether `group_id` is a Home this device can PROVE is retired (r3 P2).
1990+
///
1991+
/// Proof is local: the group is in our roster and carries the terminal
1992+
/// `withdrawn` flag. Inability to see the group is NOT proof — a Home on
1993+
/// an unreachable device is unknown, and treating unknown as retired
1994+
/// would let a partitioned device mint over the owner's real Home.
1995+
/// Implementations MUST be non-blocking and answer `false` when unsure.
1996+
fn canonical_pointer_is_retired(&self, group_id: &str) -> bool;
19801997
}
19811998

19821999
/// Current daemon self-profile names, best-effort snapshot.
@@ -2399,10 +2416,21 @@ impl OwnerSyncService {
23992416
.store
24002417
.stored_value(SyncKind::HomePointer, HOME_POINTER_KEY)
24012418
.await;
2419+
// r3 P2: a stored pointer naming a Home we hold and know to be
2420+
// retired must not keep the slot. Without this the ordering rule
2421+
// would refuse every replacement, since a new Home is always NEWER
2422+
// than the dead one it replaces.
2423+
let stored_is_retired = match (&stored, self.view()) {
2424+
(Some(SyncValue::HomePointer { group_id, .. }), Some(view)) => {
2425+
view.canonical_pointer_is_retired(group_id)
2426+
}
2427+
_ => false,
2428+
};
24022429
home_pointer_mint_decision(
24032430
desired,
24042431
stored.as_ref(),
24052432
&hex::encode(self.agent.agent_id().as_bytes()),
2433+
stored_is_retired,
24062434
)
24072435
}
24082436

@@ -3908,7 +3936,7 @@ mod home_pointer_election_tests {
39083936
for _ in 0..10 {
39093937
let mut mints = 0;
39103938
for (agent_hex, desired) in &devices {
3911-
if home_pointer_mint_decision(desired, register.as_ref(), agent_hex) {
3939+
if home_pointer_mint_decision(desired, register.as_ref(), agent_hex, false) {
39123940
register = Some(desired.clone());
39133941
mints += 1;
39143942
}
@@ -3940,9 +3968,19 @@ mod home_pointer_election_tests {
39403968
let newer = home_ptr("g-aaa", "agent-b", 2_000, vec![]);
39413969

39423970
// Newer already in the slot: the older device takes it.
3943-
assert!(home_pointer_mint_decision(&older, Some(&newer), "agent-a"));
3971+
assert!(home_pointer_mint_decision(
3972+
&older,
3973+
Some(&newer),
3974+
"agent-a",
3975+
false
3976+
));
39443977
// Older already in the slot: the newer device yields.
3945-
assert!(!home_pointer_mint_decision(&newer, Some(&older), "agent-b"));
3978+
assert!(!home_pointer_mint_decision(
3979+
&newer,
3980+
Some(&older),
3981+
"agent-b",
3982+
false
3983+
));
39463984
}
39473985

39483986
/// Equal `provisioned_at_ms` — a genuine simultaneous genesis, or just
@@ -3952,8 +3990,31 @@ mod home_pointer_election_tests {
39523990
fn home_pointer_election_breaks_timestamp_ties_on_group_id() {
39533991
let a = home_ptr("g-aaa", "agent-a", 5_000, vec![]);
39543992
let b = home_ptr("g-bbb", "agent-b", 5_000, vec![]);
3955-
assert!(home_pointer_mint_decision(&a, Some(&b), "agent-a"));
3956-
assert!(!home_pointer_mint_decision(&b, Some(&a), "agent-b"));
3993+
assert!(home_pointer_mint_decision(&a, Some(&b), "agent-a", false));
3994+
assert!(!home_pointer_mint_decision(&b, Some(&a), "agent-b", false));
3995+
}
3996+
3997+
/// A slot held by a PROVABLY retired Home must be takeable (r3 P2).
3998+
///
3999+
/// The ordering rule alone refuses every replacement here, because a new
4000+
/// Home is always NEWER than the dead one it replaces — so without this
4001+
/// override the register keeps naming a tombstone forever and every
4002+
/// device yields to it. The proof is local (`withdrawn` in our own
4003+
/// roster); an unreachable remote Home is unknown, not retired, and must
4004+
/// NOT be overridden.
4005+
#[test]
4006+
fn a_provably_retired_pointer_can_be_replaced_by_a_newer_home() {
4007+
let retired = home_ptr("g-old", "agent-a", 1_000, vec![]);
4008+
let replacement = home_ptr("g-new", "agent-a", 9_999, vec![]);
4009+
4010+
assert!(
4011+
!home_pointer_mint_decision(&replacement, Some(&retired), "agent-a", false),
4012+
"without proof of retirement the ordering rule stands: a newer Home does not win"
4013+
);
4014+
assert!(
4015+
home_pointer_mint_decision(&replacement, Some(&retired), "agent-a", true),
4016+
"a provably retired pointer must be replaceable despite being older"
4017+
);
39574018
}
39584019

39594020
/// The owner's first device must publish: an empty register is not a
@@ -3963,7 +4024,8 @@ mod home_pointer_election_tests {
39634024
assert!(home_pointer_mint_decision(
39644025
&home_ptr("g-aaa", "agent-a", 1, vec![]),
39654026
None,
3966-
"agent-a"
4027+
"agent-a",
4028+
false
39674029
));
39684030
}
39694031

@@ -3985,11 +4047,11 @@ mod home_pointer_election_tests {
39854047
}],
39864048
);
39874049
assert!(
3988-
home_pointer_mint_decision(&refreshed, Some(&stored), "agent-a"),
4050+
home_pointer_mint_decision(&refreshed, Some(&stored), "agent-a", false),
39894051
"the primary agent must be able to refresh its own Home pointer"
39904052
);
39914053
assert!(
3992-
!home_pointer_mint_decision(&refreshed, Some(&stored), "agent-b"),
4054+
!home_pointer_mint_decision(&refreshed, Some(&stored), "agent-b", false),
39934055
"a co-member must not refresh the primary's Home pointer"
39944056
);
39954057
}

src/server/routes/home.rs

Lines changed: 131 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -101,16 +101,50 @@ pub(in crate::server) enum HomeResolution {
101101
/// The canonical Home is the Tier-1 `("home")` register winner. Absence of a
102102
/// register value means "no owner device has advertised one yet" — NOT "none
103103
/// exists" — so an un-synced device with its own Home still reports `Local`.
104+
/// Whether `group_id` is a Home this device can PROVE is retired (r3 P2).
105+
///
106+
/// Proof means: the group is in our own roster and carries the terminal
107+
/// `withdrawn` flag. Not being able to see the group is deliberately NOT
108+
/// proof — a canonical Home living on an unreachable device is simply
109+
/// unknown, and treating unknown as retired would let any partitioned device
110+
/// mint over the owner's real Home.
111+
async fn is_provably_retired(state: &Arc<AppState>, group_id: &str) -> bool {
112+
state.named_groups.read().await.iter().any(|(id, info)| {
113+
(id.as_str() == group_id || info.stable_group_id() == group_id) && info.withdrawn
114+
})
115+
}
116+
117+
/// The canonical Home pointer that should actually govern this device (r3 P2).
118+
///
119+
/// The stored `("home")` record survives the withdrawal of the Home it names —
120+
/// tombstone retention never touches owner-sync state — so a device that
121+
/// retires its own advertised Home would otherwise keep yielding to the dead
122+
/// pointer forever and never build a replacement. Filtering the PUBLISHER was
123+
/// only half the fix; the stored record has to stop governing too.
124+
pub(in crate::server) async fn effective_canonical_home(state: &Arc<AppState>) -> Option<String> {
125+
let canonical = state
126+
.owner_sync
127+
.as_ref()?
128+
.canonical_home()
129+
.await
130+
.map(|home| home.group_id)?;
131+
if is_provably_retired(state, &canonical).await {
132+
tracing::info!(
133+
group_id = %canonical,
134+
"canonical Home pointer names a group we hold and know to be retired; ignoring it (#449)"
135+
);
136+
return None;
137+
}
138+
Some(canonical)
139+
}
140+
104141
pub(in crate::server) async fn resolve_home(state: &Arc<AppState>) -> HomeResolution {
105142
let Some(user_kp) = state.agent.identity().user_keypair() else {
106143
return HomeResolution::Unknown;
107144
};
108145
let owner = user_kp.user_id();
109146
let local = find_home(state.as_ref(), &owner).await;
110-
let canonical = match state.owner_sync.as_ref() {
111-
Some(sync) => sync.canonical_home().await.map(|home| home.group_id),
112-
None => None,
113-
};
147+
let canonical = effective_canonical_home(state).await;
114148
// Prefer the CANONICAL Home whenever we are already seated in it. After
115149
// adoption a device is briefly seated in both its old duplicate and the
116150
// canonical Home; without this, `find_home`'s smallest-id rule could
@@ -625,14 +659,15 @@ pub(in crate::server) async fn provision_home(state: &Arc<AppState>) {
625659
// Absence of a register value means "nobody has advertised one yet",
626660
// NOT "none exists", so an un-synced or first device still provisions
627661
// optimistically — that is what keeps an offline install usable.
628-
if let Some(sync) = state.owner_sync.as_ref() {
629-
if let Some(canonical) = sync.canonical_home().await {
630-
tracing::info!(
631-
canonical_group_id = %canonical.group_id,
632-
"owner's Home is advertised by another device; not provisioning a duplicate (#449)"
633-
);
634-
return;
635-
}
662+
// A pointer naming a Home we hold and know to be RETIRED does not
663+
// count (r3 P2): the stored record outlives the group it names, so
664+
// yielding to it would leave this device permanently without a Home.
665+
if let Some(canonical) = effective_canonical_home(state).await {
666+
tracing::info!(
667+
canonical_group_id = %canonical,
668+
"owner's Home is advertised by another device; not provisioning a duplicate (#449)"
669+
);
670+
return;
636671
}
637672

638673
// 4) Fresh provisioning through the full creation path.
@@ -1211,6 +1246,90 @@ pub(in crate::server::routes) mod tests {
12111246
Ok(())
12121247
}
12131248

1249+
/// WHY (r3 P2): the REAL lifecycle — advertise, then withdraw, then
1250+
/// reconcile/restart. This is the case the earlier test could not reach,
1251+
/// because it only inserted an already-withdrawn group into an EMPTY
1252+
/// store.
1253+
///
1254+
/// Tombstone retention never clears owner-sync state, so the stored
1255+
/// `("home")` record outlives the Home it names. Filtering the publisher
1256+
/// stops future advertisements but leaves the old record governing: the
1257+
/// device yields to a dead pointer, provisions no replacement, and
1258+
/// `GET /home` reports `elsewhere` for a group that no longer exists —
1259+
/// permanently, across restarts.
1260+
#[tokio::test]
1261+
async fn a_retired_advertised_home_does_not_suppress_its_replacement() -> anyhow::Result<()> {
1262+
let dir = tempfile::tempdir()?;
1263+
let state = owned_state(dir.path(), [0x5D; 32]).await?;
1264+
let owner = owner_of(&state);
1265+
1266+
// 1. Advertise our real Home, exactly as a live device would.
1267+
provision_home(&state).await;
1268+
let (advertised, _) = find_home(&state, &owner).await.expect("Home provisioned");
1269+
advertise_canonical_home(&state, &advertised).await;
1270+
1271+
// 2. Retire it. The stored pointer still names it.
1272+
state
1273+
.named_groups
1274+
.write()
1275+
.await
1276+
.get_mut(&advertised)
1277+
.expect("advertised Home")
1278+
.withdrawn = true;
1279+
assert_eq!(
1280+
state
1281+
.owner_sync
1282+
.as_ref()
1283+
.expect("sync")
1284+
.canonical_home()
1285+
.await
1286+
.map(|home| home.group_id)
1287+
.as_deref(),
1288+
Some(advertised.as_str()),
1289+
"precondition: the retired Home is still the stored canonical pointer"
1290+
);
1291+
1292+
// 3. Reconcile / restart.
1293+
assert!(
1294+
effective_canonical_home(&state).await.is_none(),
1295+
"a pointer to a Home we hold and know to be retired must stop governing"
1296+
);
1297+
provision_home(&state).await;
1298+
1299+
let (replacement, info) = find_home(&state, &owner)
1300+
.await
1301+
.expect("a usable replacement Home must be provisioned");
1302+
assert_ne!(replacement, advertised, "the replacement is a NEW Home");
1303+
assert!(!info.withdrawn);
1304+
Ok(())
1305+
}
1306+
1307+
/// WHY (r3 P2): proof of retirement is LOCAL, and absence of knowledge is
1308+
/// not proof. A canonical Home living on a device we cannot currently
1309+
/// reach is unknown, not retired — clearing it would let any partitioned
1310+
/// device mint over the owner's real Home and refork the space this whole
1311+
/// issue exists to unify.
1312+
#[tokio::test]
1313+
async fn an_unreachable_remote_home_is_never_treated_as_retired() -> anyhow::Result<()> {
1314+
let dir = tempfile::tempdir()?;
1315+
let state = owned_state(dir.path(), [0x5E; 32]).await?;
1316+
let remote = "ac".repeat(16);
1317+
advertise_canonical_home(&state, &remote).await;
1318+
1319+
assert_eq!(
1320+
effective_canonical_home(&state).await.as_deref(),
1321+
Some(remote.as_str()),
1322+
"a Home we simply cannot see must keep governing — unknown is not retired"
1323+
);
1324+
1325+
provision_home(&state).await;
1326+
assert!(
1327+
find_home(&state, &owner_of(&state)).await.is_none(),
1328+
"we must still yield to an unreachable canonical Home, not fork a new one"
1329+
);
1330+
Ok(())
1331+
}
1332+
12141333
/// WHY (#449 D5): withdrawal keeps `members_v2` and `home` populated, so
12151334
/// without an explicit filter a RETIRED Home still resolves here — which
12161335
/// would make `GET /home` serve a tombstone and `provision_home` return

src/server/routes/sync.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,17 @@ impl SyncDaemonView for DaemonView {
136136
})
137137
}
138138

139+
fn canonical_pointer_is_retired(&self, group_id: &str) -> bool {
140+
// Fail SAFE when the roster lock is contended: "unsure" must answer
141+
// false, so a busy daemon never mints over a live canonical Home.
142+
let Ok(groups) = self.state.named_groups.try_read() else {
143+
return false;
144+
};
145+
groups.iter().any(|(id, info)| {
146+
(id.as_str() == group_id || info.stable_group_id() == group_id) && info.withdrawn
147+
})
148+
}
149+
139150
fn apply_names(
140151
&self,
141152
human_name: Option<String>,

0 commit comments

Comments
 (0)