diff --git a/CHANGELOG.md b/CHANGELOG.md index eafe18d6..635f675f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,14 @@ breaks something, or changes the observable behavior of existing surfaces broadly; **patch** for fixes, and for additive opt-in capability — a new key, flag, or page that no existing user can encounter without opting into it. +## [Unreleased] + +### Fixed + +- Recovering a stale hosted session no longer kills a remembered process ID + without checking the process's identity. If that identity changed or cannot + be verified, the run request reports the refusal and leaves the process alone. + ## [0.68.0] - 2026-09-08 Web serving. Files, directories, adapter documentation and the changelog are diff --git a/crates/spt-daemon/src/broker.rs b/crates/spt-daemon/src/broker.rs index 5c7d94ec..41cae3ca 100644 --- a/crates/spt-daemon/src/broker.rs +++ b/crates/spt-daemon/src/broker.rs @@ -424,6 +424,33 @@ fn zombie_verdict( } } +/// Kill only the root whose birth was captured before its exit waiter started. +/// A dead root permits record cleanup WITHOUT signaling its remembered number. +/// An error forbids both kill and forced de-tabling: dropping session resources +/// is teardown too, not a safe fallback for an identity we could not establish. +// [impl->REQ-BROKER-ZOMBIE-IDENTITY] +fn reap_zombie_root( + pid: Option, + born: Option, + observe: impl FnOnce(u32) -> spt_store::proc::ProcIdentity, + kill_tree: impl FnOnce(u32), +) -> Result<(), &'static str> { + use spt_store::proc::ProcIdentity; + let pid = pid.filter(|p| *p != 0).ok_or("pid-unknown")?; + match observe(pid) { + ProcIdentity::Absent => Ok(()), + ProcIdentity::Present(now) => match born { + Some(expected) if expected == now => { + kill_tree(pid); + Ok(()) + } + Some(_) => Err("identity-mismatch"), + None => Err("identity-unproven"), + }, + ProcIdentity::Unproven => Err("identity-unproven"), + } +} + /// The impure probe feeding [`zombie_verdict`] for a claimed live session — /// runs OFF every broker lock (a process-table walk is real work; KH 7.12). /// `spawned_ms_ago` comes from the session's `spawned_at`; callers on the @@ -4116,6 +4143,10 @@ struct HostedSession { /// yet and must never read as a dead tree during bringup. // [impl->REQ-ENDPOINT-CYCLE-HONEST] spawned_at: Instant, + /// OS birth stamp captured while the newly spawned child is still owned + /// and BEFORE any waiter can reap it. Never refreshed from a remembered pid. + // [impl->REQ-BROKER-ZOMBIE-IDENTITY] + process_started_at: Option, } /// One record queued to a session's dedicated PTY input-writer thread — the raw @@ -8007,6 +8038,7 @@ impl Broker { AlreadyLive { sid: u64, spid: Option, + process_started_at: Option, adapter_labeled: bool, spawned_ms_ago: u64, }, @@ -8020,6 +8052,7 @@ impl Broker { ( *sid, h.session.process_id(), + h.process_started_at, !h.adapter.is_empty(), h.spawned_at.elapsed().as_millis() as u64, ) @@ -8034,9 +8067,11 @@ impl Broker { .is_some_and(|c| c.phase_at.elapsed() >= phase_wedge_ceiling(&c.phase)); match wake_gate_decision(live.is_some(), held.is_some(), holder_stalled) { WakeGate::AlreadyLive => { - let (sid, spid, adapter_labeled, spawned_ms_ago) = + let (sid, spid, process_started_at, adapter_labeled, spawned_ms_ago) = live.expect("live is Some on AlreadyLive"); - Gate::AlreadyLive { sid, spid, adapter_labeled, spawned_ms_ago } + Gate::AlreadyLive { + sid, spid, process_started_at, adapter_labeled, spawned_ms_ago, + } } WakeGate::Racing => Gate::Racing, WakeGate::Claim => { @@ -8081,7 +8116,9 @@ impl Broker { spawn_phase("gate_claimed", ""); break Some(guard); } - Gate::AlreadyLive { sid, spid, adapter_labeled, spawned_ms_ago } => { + Gate::AlreadyLive { + sid, spid, process_started_at, adapter_labeled, spawned_ms_ago, + } => { // ONE liveness authority (ADR-0041 decision 6, // REQ-ENDPOINT-CYCLE-HONEST): before refusing/deduping by // citing the claimed session, PROBE its client tree — off @@ -8093,14 +8130,43 @@ impl Broker { // refusal citing a corpse. // [impl->REQ-ENDPOINT-CYCLE-HONEST] if session_is_zombie(spid, adapter_labeled, spawned_ms_ago) { + // Re-observe immediately before any kill, off-lock. + // The existing owned-process death oracle preserves + // snapshot-less Unix cleanup and Linux zombie demotion. + // [impl->REQ-BROKER-ZOMBIE-IDENTITY] + if let Err(reason) = reap_zombie_root( + spid, + process_started_at, + |pid| { + use spt_store::proc::{self, ProcIdentity}; + if proc::provably_gone(pid) { + ProcIdentity::Absent + } else { + proc::process_started_at(pid) + .map(ProcIdentity::Present) + .unwrap_or(ProcIdentity::Unproven) + } + }, + spt_store::proc::kill_pid_tree, + ) { + spt_proto::emit_line_err!( + "SPAWN_ZOMBIE_REAP_DECLINE:{}: session {sid} pid={spid:?} \ + reason={reason} — no kill or forced de-table", + req.endpoint + ); + // Return BEFORE the cleanup loop. In particular, + // do not drop HostedSession/PTY resources or call + // dispatch_teardown as a fallback after refusal. + return Err(format!( + "SPAWN_ZOMBIE_REAP_DECLINE:{}: session {sid}: {reason}", + req.endpoint + )); + } spt_proto::emit_line_err!( "SPAWN_ZOMBIE_REAP:{}: claimed session {sid} has a dead client \ tree — reaping before honest respawn [REQ-ENDPOINT-CYCLE-HONEST]", req.endpoint ); - if let Some(pid) = spid { - spt_store::proc::kill_pid_tree(pid); - } // Bounded wait for the exit-waiter to reap the table row; // belt: a wedged/missed waiter (root already dead) gets a // direct de-table + stamp clear so the gate can converge. @@ -8212,6 +8278,11 @@ impl Broker { let session = Arc::new(session); let id = self.next_id.fetch_add(1, Ordering::Relaxed); let pid = session.process_id(); + // Birth identity is trustworthy here: Windows retains the child handle; + // Unix cannot recycle our child's pid until it is reaped, and no waiter + // has been started yet. Exec may change its image, not its birth stamp. + // [impl->REQ-BROKER-ZOMBIE-IDENTITY] + let process_started_at = pid.and_then(spt_store::proc::process_started_at); // The ConPTY-create span releases#199 candidate C names, closed. spawn_phase("pty_created", &format!("pid={pid:?}")); @@ -8514,6 +8585,7 @@ impl Broker { adapter: req.adapter.clone(), install_dir: req.install_dir.clone().map(std::path::PathBuf::from), spawned_at: Instant::now(), + process_started_at, }, ); // The row exists, so the in-flight ledger has nothing left to say about @@ -10071,6 +10143,44 @@ mod tests { assert!(!zombie_verdict(None, false, false, true)); } + // [unit->REQ-BROKER-ZOMBIE-IDENTITY] + #[test] + fn zombie_reap_declines_recycled_pid() { + use spt_store::proc::ProcIdentity; + // A record still remembers the old wrapper's birth, but the same pid + // now denotes a different process. It passes the old zombie predicate. + let pid = 42; + let born = 100; + assert!(zombie_verdict(Some(true), true, false, true)); + let mut killed = None; + let result = reap_zombie_root( + Some(pid), + Some(born), + |_| ProcIdentity::Present(born + 1), + |target| killed = Some(target), + ); + assert_eq!(result, Err("identity-mismatch")); + assert_eq!(killed, None, "the replacement and its descendants must survive"); + } + + // [unit->REQ-BROKER-ZOMBIE-IDENTITY] + #[test] + fn zombie_reap_requires_positive_identity_but_dead_root_needs_no_kill() { + use spt_store::proc::ProcIdentity; + for (pid, born, observed, expected, killed_pid) in [ + (Some(42), Some(100), ProcIdentity::Present(100), Ok(()), Some(42)), + (Some(42), None, ProcIdentity::Present(100), Err("identity-unproven"), None), + (Some(42), Some(100), ProcIdentity::Unproven, Err("identity-unproven"), None), + (None, Some(100), ProcIdentity::Present(100), Err("pid-unknown"), None), + (Some(42), None, ProcIdentity::Absent, Ok(()), None), + ] { + let mut killed = None; + let result = reap_zombie_root(pid, born, |_| observed, |target| killed = Some(target)); + assert_eq!(result, expected, "{pid:?} / {born:?} / {observed:?}"); + assert_eq!(killed, killed_pid, "{pid:?} / {born:?} / {observed:?}"); + } + } + /// [unit->REQ-LIVENESS-ORACLE-SOUND] the NO-SNAPSHOT arm, pinned WITHOUT a /// platform that lacks a process table — the one claim neither the /// manufactured corpse nor the int can make, since both run where a table diff --git a/docs/KNOWN-HAZARDS.md b/docs/KNOWN-HAZARDS.md index 8ab726b7..6f4ad578 100644 --- a/docs/KNOWN-HAZARDS.md +++ b/docs/KNOWN-HAZARDS.md @@ -845,6 +845,8 @@ These were absent from the sister-project harvest; codex surfaced them as load-b - **Failure (paid-for, hertz v0.39.4 field RCA 2026-07-22, doyle code-verified same day):** `resume.pid` custody is a bare PID consumed as `read_resume_pid(..).is_some_and(is_process_alive)` at BOTH the livehost restart gate and the liveness-reconcile DEFER. A dead wake-resume spawn's pid, recycled by the OS onto an unrelated process (field proof: `resume.pid=29456` resolved to a random `cmd.exe`), reads as "a resume is in flight" indefinitely: reconcile defers every tick, the row stays online-authoritative, and the endpoint is FALSE-ONLINE with no self-repair — every reader faithfully resurrects the lie. The ABA problem, in process custody. - **Invariant:** a custody record that gates lifecycle decisions stores an **identity pair (pid + process creation time)**, written atomically at spawn-mint; consumers test the PAIR, and a mismatch means NOT OURS — the discovering reader DELETES the record and proceeds as if no custody existed (self-heal, not error). Bind and reap clear custody atomically with their own outcome. Creation time comes from the process snapshot, never a retained handle (7.50); no snapshot ⇒ "unproven" ⇒ defer ONE tick, never a manufactured verdict. - **spt-core mapping:** `spt-daemon/src/livehost.rs` (restart gate + reconcile DEFER custody reads); `spt-store` resume-custody record + `proc.rs` snapshot identity; regression = the recycled-pid rig (custody pair mismatching a live impostor pid → record deleted, reconcile proceeds, row goes honest). + +- **Zombie-reap kill boundary (releases#285):** `broker.rs` captures the PTY root's creation time before starting its exit waiter; a later zombie verdict alone never licenses killing that remembered number. Fresh death permits cleanup without a signal; otherwise the live start time must match. `SPAWN_ZOMBIE_REAP_DECLINE` means neither tree-kill nor forced de-table runs, including any resource-drop fallback. The regression injects a different birth at the same PID and observes refusal with no kill. This is not atomic with the numeric tree-kill; the check-to-signal race and Linux same-tick identity collision remain (ADR-0041 decision 6). - **Source:** DAEMON-LIFECYCLE W1 (ADR-0047 decision 1; hertz field RCA bug 1). ### 7.52 An operator stop outranks every implicit ensure — no convenience path resurrects what the operator just killed `[REQ-HAZARD-STOP-RESPAWN-CONVOY]` diff --git a/docs/adr/0041-endpoint-lifecycle-truth.md b/docs/adr/0041-endpoint-lifecycle-truth.md index 4e83f020..ca94dfa2 100644 --- a/docs/adr/0041-endpoint-lifecycle-truth.md +++ b/docs/adr/0041-endpoint-lifecycle-truth.md @@ -80,6 +80,33 @@ authority ever verifies or normalizes.** honestly); the shutdown state machine consults the same source, so "is it live" has one answer (no ALREADY_LIVE/OFFLINE/NO_EDGE three-way contradiction). + + **Zombie-kill identity guard (releases#285).** Liveness is not permission to + kill a remembered PID. The broker records the root's OS creation time just + after PTY spawn, before starting its exit waiter. The child is still owned: + on Windows its handle is retained; on Unix it has not been reaped, so its + number cannot yet be recycled. The expected identity is never refreshed at + reap time. Start time is preferred over image because a launcher can `exec` + another image without becoming a different process. + + At the zombie kill boundary, a fresh owned-process death probe permits + stale-row cleanup for a dead root **without any PID/tree kill**. Otherwise a + fresh start-time query must match the captured stamp before tree-kill. + Unknown PID, missing/unreadable identity, or a changed stamp emits + `SPAWN_ZOMBIE_REAP_DECLINE` and fails the spawn request **before** the forced + de-table loop. There is no fallback `session.kill`, teardown dispatch, or + session-resource drop on refusal; the old row remains for its normal exit + waiter. This deliberately trades automatic recovery of an unverifiable + live-number row for safety, rather than using cleanup as an indirect kill. + Fresh-dead cleanup retains the existing snapshot-less Unix liveness fallback. + + **Limits:** this is a checked numeric tree-kill, not an atomic handle/pidfd + tree termination. A process can exit and its PID can be reused after the + identity query and before a signal; descendant enumeration/kill retains the + same race. Linux start times have clock-tick resolution, so same-tick reuse + is indistinguishable; Windows uses creation FILETIME. This scoped guard does + not change generic `kill_pid_tree` or other kill sites. + ## Consequences - A dead endpoint converges to offline+suspended+uncontrolled within one reconcile, from diff --git a/traceable-reqs.toml b/traceable-reqs.toml index 21917ea9..b518a80c 100644 --- a/traceable-reqs.toml +++ b/traceable-reqs.toml @@ -2840,6 +2840,11 @@ id = "REQ-ENDPOINT-CYCLE-HONEST" title = "REGISTRY-LIFECYCLE W3 (ADR-0041 decision 6, operator deployah stop/run wedge): cycle verbs share ONE liveness authority — the ALREADY_LIVE dup-guard liveness-probes the claimed session client tree before refusing (dead tree means reap + respawn honestly, never a refusal citing a zombie); the shutdown state machine consults the same source so is-it-live has one answer (no ALREADY_LIVE / list-OFFLINE / shutdown-NO_EDGE three-way contradiction on the same endpoint). Gate: impl — probing dup-guard + unified authority; unit — dead-tree claim probes and reaps, live claim still refuses; int — controlled zombie (killed client tree, surviving hosted record) leads to endpoint run succeeding honestly end-to-end; doc — ADR-0041." required_stages = ["doc","impl","unit","int"] # W3 ACTIVATED at build start (todlando 2026-07-17). +[[requirements]] +id = "REQ-BROKER-ZOMBIE-IDENTITY" +title = "Broker zombie reaping checks the session root's spawn-time process identity before tree-kill; unknown or changed identity declines without teardown, while a provably dead root still permits stale-session cleanup without signaling its remembered PID" +required_stages = ["doc", "impl", "unit"] # releases#285: deterministic injected identity/kill boundary; existing cycle tests cover dead-root convergence. + [[requirements]] id = "REQ-GOSSIP-PROJECT-DERIVE-ONCE" title = "REGISTRY-LIFECYCLE W4 fast-follow (hertz v0.37.0 field-verify CPU-gate FAIL RCA 2026-07-17, doyle seam-verified same day; timing-pinned ~16s burst clusters per 30s Registry pump round): recent-project gossip derives a project id AT MOST ONCE PER DISTINCT CWD per advertisement round — recent_projects_for dedups the RAW cwd string BEFORE project_id_for_dir (today seen.insert(pid) dedups AFTER derivation at registryhost.rs:1001, so every duplicate-cwd sessions.log row pays a git remote/rev-parse spawn: ~200 derivations/round on a 12-perch box, the post-self-hash-fix CPU floor). The pid-level dedup stays as the output-uniqueness second layer (distinct cwd spellings may collapse to one project); ordering semantics unchanged (first-occurrence newest-first); bounded ledger read unchanged. Gate: impl — cwd-dedup before derivation; unit — derivation-count observable == distinct cwds not ledger rows (duplicate-cwd ledger derives once), output/order parity vs the pre-fix shape on a mixed ledger. SEEDED SEPARATELY (not this REQ): gossip leg consults the ADR-0037 materialized index (kills git entirely + cross-round rederivation; staleness-semantics design change) + epoch-mint batching (26 atomic writes/round; durable-monotonic contract change)." @@ -4475,6 +4480,7 @@ name = "daemon-process-lifecycle" requirements = [ "REQ-BROKER-STOP-DENY-NAMES-BLAST", "REQ-BROKER-STOP-ENDPOINT-DENY", + "REQ-BROKER-ZOMBIE-IDENTITY", "REQ-DAEMON-1", "REQ-DAEMON-3", "REQ-DAEMON-4", [raw output: artifact://115]