From bcdfe44c683b02a18fc87f689226c3c480934a6c Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:30:56 -0700 Subject: [PATCH 01/24] docs: add implementation plan for opencode-daemon-death-recovery --- ...26-09-21-opencode-daemon-death-recovery.md | 779 ++++++++++++++++++ 1 file changed, 779 insertions(+) create mode 100644 docs/plans/2026-09-21-opencode-daemon-death-recovery.md diff --git a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md new file mode 100644 index 000000000..2acb6bc79 --- /dev/null +++ b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md @@ -0,0 +1,779 @@ +# OpenCode Daemon Death Recovery Implementation Plan + +> **For agentic workers:** Execute this plan task by task with a fresh +> implementer and a specification-plus-quality review after every task. Track +> progress with the checkbox steps below. + +## User Request + +### Requested result +Fix the freshopencode shared-daemon death incident class in the Freshell repo (Rust server + React client): (1) an opencode compact request timeout must no longer kill the shared `opencode serve` sidecar; (2) a daemon discard must emit a structured log; (3) daemon loss must self-heal at the freshopencode runtime level with a client-visible status edge and a backoff-guarded respawn, mirroring the freshcodex onExit self-heal; (4) the client must not dead-end on the fresh-agent snapshot 409 RESTORE_UNAVAILABLE — it must drive the documented generation-fenced attach recovery and refetch. + +### Explicit constraints +- The user explicitly requested the-usual workflow (plan, load-bearing validation, independent fresh-eyes reviews, TDD execution, recap). +- Work in a dedicated worktree under `.worktrees/`; branch from `origin/main`; PR only after explicit user approval; never push behavior changes to `main` directly. +- Red/Green/Refactor TDD; ensure unit and e2e coverage; never reduce test coverage to get tests passing. +- Never restart the self-hosted production Freshell server on port 3001 without the user's explicit "APPROVED". +- TypeScript NodeNext relative imports require `.js` extensions. +- Follow repo test-coordination rules (coordinated broad runs, base-gate for green-base checks). + +### Accepted tradeoffs and residuals +- The user approved the proposed fix set "presumably step 1-4, unless analysis reveals otherwise": planning analysis may adjust the exact fix set if evidence shows a different cut is more idiomatic, but the four identified defects are the baseline scope. + +**Goal:** A freshopencode pane survives — and automatically recovers from — the loss of the shared `opencode serve` daemon, and no single slow request can kill that daemon for every session again. + +**Architecture:** Four layers, each mirroring an established in-repo precedent. (1) The compact request lane adopts the b8ke FR2 captured-base + `DiscardOnTimeout::No` pattern already used by `get_session_at`/`list_messages_at`/`abort_at`, so a timed-out summarize POST returns `RequestTimeout` without touching the shared daemon. (2) `discard_running` logs a structured WARN with its reason (the parameter already exists, unused). (3) The serve manager gains a daemon-level exit watcher + loss signal channel + backoff-guarded automatic re-warm (mirroring freshcodex `spawn_exit_watcher`), and the freshopencode runtime subscribes one listener task that fans a typed `freshAgent.error{OPENCODE_DAEMON_LOST}` edge to every materialized session and, after a successful respawn, restarts dead serve bridges and pushes idle snapshot edges. (4) The client's `handleSnapshotError` gains a 409 `RESTORE_UNAVAILABLE` arm that drives the documented recovery — one generation-fenced `freshAgent.attach` per pane identity plus a snapshot refetch — instead of dead-ending at a dismiss-only banner. + +**Tech Stack:** Rust (tokio, axum, tracing; crates `freshell-opencode`, `freshell-freshagent`), TypeScript/React (Redux Toolkit, Zod, Vitest + Testing Library, Playwright). + +## Global Constraints + +- All work happens in the worktree `.worktrees/opencode-daemon-death-recovery` on branch `the-usual/opencode-daemon-death-recovery` (base `855dae72a`). Never commit on `main`. +- The production self-hosted server on port 3001 must not be restarted without explicit user "APPROVED". All verification runs against locally spawned test servers or in-process harnesses only. +- The snapshot threads-route 409 envelope is a frozen contract: `status:"error"`, `code:"RESTORE_UNAVAILABLE"`, message `"Session is still running on the server."` are load-bearing (pinned by `snapshot.rs:1151` `opencode_cold_get_owned_or_transitioning_answers_the_typed_409`; client regexes depend on the text). Additive fields are allowed; changing/removing pinned fields is not. +- The snapshot GET stays side-effect-free: never spawn, kill, or discard from `get_opencode_snapshot` (pinned by `snapshot.rs:1070` and `lib.rs:7474`/`lib.rs:7529`). +- `ServeError::RequestTimeout` must stay OUTSIDE `never_dispatched()` (a timed-out POST may have reached the daemon — the compact redo-destroy stands; serve.rs:539-551 pins this forever). +- Structured logging: `tracing` macros, dotted event name as the message, structured fields (schema: `freshell-server/src/logging.rs`). New event names follow the `freshagent.opencode.*` family (existing: `freshagent.opencode.compact_failed`, `freshagent.opencode.handoff_stop_abort_undelivered`). +- The shared daemon is NEVER a per-session kill target (`freshAgent.kill` stays session-scoped; lib.rs:2318-2325 "the shared opencode serve daemon is NOT the per-session writer and must NEVER be killed" — that invariant refers to the ownership watchdog; the manager's own discard/re-warm lifecycle is the exception this plan carefully rebuilds). +- Client a11y: no new interactive elements without labels/roles; the recovery reuses existing banner/card components, so no new a11y surface should be introduced. +- Rust: `cargo fmt --all --check` and `cargo clippy --workspace --exclude freshell-tauri --all-targets -- -D warnings` must stay clean (pre-push gate). +- TypeScript: `npm run typecheck` clean; relative imports in NodeNext contexts need `.js` extensions (the client uses `@/` aliases). +- Focused test commands (delegated, non-coordinated — safe for TDD loops): + - `cargo test -p freshell-opencode` + - `cargo test -p freshell-freshagent opencode_ws::tests` + - `cargo test -p freshell-freshagent lib::tests` + - `npm run test:vitest -- run test/unit/client/components/fresh-agent/FreshAgentView.test.tsx` + - `npm run test:e2e:local -- --project=chromium test/e2e-browser/specs/.ts` +- Broad/coordinated runs (`npm test`, `test:server` zero-arg, `test:integration` zero-arg) go through the shared coordinator; wait for a free gate, never kill a foreign holder. + +### Deliberate residuals (documented, out of scope) + +- `prompt_async` (the send-turn POST) and the thin `json_request` wrappers (`get_session`, `list_messages`, `get_session_status_map`, `abort`, `fork`, `revert`, `unrevert`) KEEP `DiscardOnTimeout::Yes`. Rationale: they are the deliberate wedged-daemon recycler for writes (FR2 kept Yes for writes on purpose), and the incident class was compact-specific (an LLM-scale budget routinely exceeded by a healthy-but-busy daemon). A wedged-alive daemon is also caught by the new exit-watcher only if it exits; a hung-but-alive daemon remains the send-lane's recycle responsibility. Revisit only with a dedicated wedged-detection design. +- The frozen 409 message text stays (even when the Live owner's daemon is dead, the text says "still running on the server" — clients' muscle memory depends on it). The new `OPENCODE_DAEMON_LOST` runtime edge is what tells the user the truth. +- E2E daemon-death/respawn coverage against a real spawned daemon is the cloud-skipped provider-lifecycle class (see `CLOUD_SKIP_SPECS`: `freshopencode-restart-recovery` et al.). This run adds cloud-legal e2e for the client recovery lane (Task 6) and covers the server lanes with the Rust unit tests (the same coverage strategy the freshcodex self-heal uses). + +--- + +### Task 1: Compact (and its pre-flight config read) no longer kill the shared daemon on timeout + +**Files:** +- Modify: `crates/freshell-opencode/src/serve.rs` (`compact()` at ~:1193-1232, `get_config()` at ~:1169, `json_request_maybe_witnessed` at ~:864-888 stays untouched) +- Test: `crates/freshell-opencode/src/serve.rs` `#[cfg(test)] mod tests` (beside `compact_uses_the_dedicated_compact_timeout_not_the_generic_request_bound`, ~:2083) + +**Interfaces:** +- Consumes: `json_request_over_base(method, path, body, not_found_value, base: String, discard_on_timeout: DiscardOnTimeout, dispatch_witnesses, timeout_override)` (serve.rs:890), `require_base()` (serve.rs:845), `DiscardOnTimeout` (serve.rs:345-355). +- Produces: unchanged public signatures for `compact()` / `get_config()` — the change is internal lane behavior only. Later tasks rely on: "a compact timeout does not clear the manager's running entry". + +**Behavior:** `compact` becomes the FR2 shape for writes: resolve the base once (`require_base()` — spawn-on-demand is preserved), then POST `/session/{id}/summarize` through `json_request_over_base` with `DiscardOnTimeout::No` and the dedicated `compact_timeout`. `get_config` (a read, and the compact drive's pre-flight model-pair resolution at opencode_ws.rs:3675-3696) gets the same treatment — a slow GET must never kill the shared daemon (the FR2 doc rule for reads; today it violates it). All other lanes keep their current discard policy (see residuals). + +- [ ] **Step 1: Write the failing behavioral test** + +Add to the `#[cfg(test)] mod tests` module in serve.rs, reusing the existing fakes (`started_recording_manager_with_config`, `NeverExitsProcess` with its `killed: Arc` counter, and a recording HTTP fake scripted so health answers 200 and `/summarize` never resolves — the wedged shape from `tests/serve_health_bounded.rs:78` (`std::future::pending()`), exposed through a per-URL scripting seam like `RecordingHttp` at serve.rs:1781-1871): + +```rust +// 2026-09-20 incident: a compact timeout (600 s budget) ran the +// DiscardOnTimeout::Yes arm and KILLED the one shared `opencode serve` +// daemon for every freshopencode session. The compact lane must degrade +// like the FR2 snapshot lane: the POST times out, the daemon survives. +#[tokio::test] +async fn compact_timeout_does_not_kill_the_shared_daemon() { + let killed = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let mut config = ServeConfig::default(); + config.compact_timeout = std::time::Duration::from_millis(50); + let (manager, http) = started_recording_manager_with_config( + /* summarize always times out: script `/summarize` responses to hang */ + RecordingHttpScript::SummarizePending, + config, + killed.clone(), + ); + let err = manager + .compact("ses_timeout", "anthropic", "claude-sonnet-4-5", &route_for("/w"), None, None) + .await + .expect_err("the summarize POST must time out"); + assert!(matches!(err, ServeError::RequestTimeout { .. }), "got {err:?}"); + assert_eq!( + killed.load(std::sync::atomic::Ordering::SeqCst), + 0, + "a compact timeout must NEVER kill the shared daemon" + ); + assert!( + manager.base_url().await.is_some(), + "the running entry must survive a compact timeout" + ); + // The compact-timeout POST must still carry the dedicated budget. + let summarize_index = http.index_of("POST", "/session/ses_timeout/summarize"); + assert_eq!(http.recorded_timeout(summarize_index), Some(std::time::Duration::from_millis(50))); +} + +// The compact drive's pre-flight model-pair resolution reads /config; a slow +// config GET is the same defect class (a read must never kill the daemon). +#[tokio::test] +async fn get_config_timeout_does_not_kill_the_shared_daemon() { + let killed = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let mut config = ServeConfig::default(); + config.request_timeout = std::time::Duration::from_millis(50); + let (manager, _http) = started_recording_manager_with_config( + RecordingHttpScript::ConfigPending, + config, + killed.clone(), + ); + let err = manager.get_config().await.expect_err("config GET must time out"); + assert!(matches!(err, ServeError::RequestTimeout { .. })); + assert_eq!(killed.load(std::sync::atomic::Ordering::SeqCst), 0, + "a config read timeout must NEVER kill the shared daemon"); + assert!(manager.base_url().await.is_some()); +} +``` + +(Draft: adapt the fake construction to the actual `RecordingHttp`/`started_recording_manager*` signatures — the fakes already record per-request timeouts at serve.rs:1808-1817; extend their URL scripting to include a never-resolving `/summarize` and `/config` arm if not already scriptable. The two assertions that matter are `killed == 0` and `base_url().is_some()` after `Err(RequestTimeout)`.) + +- [ ] **Step 2: Run the test and verify the intended failure** + +Run: `cargo test -p freshell-opencode compact_timeout_does_not_kill get_config_timeout_does_not` + +Expected: FAIL — `compact_timeout_does_not_kill_the_shared_daemon` fails with `killed: 1` (the discard-on-timeout arm killed the process) and/or `base_url()` is `None`; `get_config_timeout_does_not_kill_the_shared_daemon` fails the same way. + +- [ ] **Step 3: Add the minimal production implementation** + +In `crates/freshell-opencode/src/serve.rs`: + +```rust +/// `getConfig` for the compact drive's model-pair resolution (and the model +/// catalog lanes). A slow config read must never kill the shared daemon — +/// the FR2 read rule (b8ke): capture the base once (spawn-on-demand is fine), +/// then transport over the captured base with `DiscardOnTimeout::No`. +pub async fn get_config(&self) -> Result { + let base = self.require_base().await?; + self.json_request_over_base( + HttpMethod::Get, "/config", None, None, + base, + DiscardOnTimeout::No, + &[], + None, + ).await +} +``` + +and inside `compact()` (serve.rs:1193-1232), replace the `json_request_maybe_witnessed(...)` call with the captured-base form (keep the exact path/body/witness/timeout logic): + +```rust +// 2026-09-20 incident: the summarize POST used the discard-on-timeout lane, +// so a 600 s budget exceeded on a healthy-but-busy daemon KILLED the one +// shared daemon for every freshopencode session. Mirror the FR2 captured-base +// transport (`get_session_at`, serve.rs:1003): a timed-out compact answers +// `RequestTimeout` and NEVER kills the shared daemon. The redo-destroy +// classification is unchanged — `RequestTimeout` stays outside +// `never_dispatched()` (a timed-out POST may have reached the daemon). +let base = self.require_base().await?; +self.json_request_over_base( + HttpMethod::Post, &path, Some(body), None, + base, + DiscardOnTimeout::No, + &witnesses, + Some(self.config().compact_timeout), +).await?; +Ok(()) +``` + +- [ ] **Step 4: Run the focused test** + +Run: `cargo test -p freshell-opencode compact_timeout_does_not_kill get_config_timeout_does_not` + +Expected: PASS + +- [ ] **Step 5: Refactor while green** + +None needed beyond the above (the change is already the FR2 mirror; `json_request_maybe_witnessed` remains for the lanes that intentionally keep `Yes`). + +- [ ] **Step 6: Run impacted-test verification** + +Impacted set: all `freshell-opencode` tests (compact family, config, timeout plumbing) plus the `freshell-freshagent` compact-drive tests (the redo-destroy family pins `never_dispatched` classification and the `compact_failed` WARN — unchanged by this fix, but they exercise the compact lane end-to-end). + +Run: `cargo test -p freshell-opencode && cargo test -p freshell-freshagent compact` + +Expected: PASS (a pre-existing-failure comparison against the baseline ledger is not needed; baseline is green). + +- [ ] **Step 7: Commit the task** + +```bash +git add crates/freshell-opencode/src/serve.rs +git commit -m "fix(opencode): compact and config timeouts never kill the shared serve daemon" +``` + +--- + +### Task 2: Daemon discards emit a structured log + +**Files:** +- Modify: `crates/freshell-opencode/src/serve.rs` (`discard_running` at ~:1348-1354) +- Test: `crates/freshell-opencode/src/serve.rs` `#[cfg(test)]` (beside the `config_capture` tracing-capture module, ~:2451-2502) + +**Interfaces:** +- Consumes: the `config_capture` thread-local tracing capture idiom (serve.rs:2451-2502, the DIAG-01 pattern), `prompt_async` (serve.rs:1091 — a lane that intentionally keeps `DiscardOnTimeout::Yes`). +- Produces: `discard_running(reason: &str)` logs `tracing::warn!(reason = ..., "freshagent.opencode.daemon_discarded")` before the kill. Task 3 builds on this exact site. + +- [ ] **Step 1: Write the failing behavioral test** + +```rust +// 2026-09-20 incident: the daemon discard that killed the shared serve left +// ZERO log trace (discard_running's reason parameter is unused). The discard +// must be observable in the structured JSONL log. +#[tokio::test] +async fn discard_running_emits_a_structured_warn_with_its_reason() { + let capture = config_capture::capture(); + let mut config = ServeConfig::default(); + config.request_timeout = std::time::Duration::from_millis(50); + let (manager, _http) = started_recording_manager_with_config( + RecordingHttpScript::PromptPending, // a Yes-lane request that times out + config, + Default::default(), + ); + let err = manager.prompt_async(/* minimal args as in existing prompt tests */).await + .expect_err("prompt POST must time out"); + assert!(matches!(err, ServeError::RequestTimeout { .. })); + let events = capture.finish(); + let discard = events.iter().find(|e| e.get("message") + .map(|m| m.as_str() == Some("freshagent.opencode.daemon_discarded")).unwrap_or(false)) + .expect("a daemon discard must emit freshagent.opencode.daemon_discarded"); + assert_eq!(discard.get("reason").and_then(|r| r.as_str()), Some("request_timeout")); +} +``` + +(Draft: adapt to the actual `config_capture` helper API and `prompt_async` minimal-args shape used by `run_turn_arms_the_accepted_witness_at_the_dispatch_boundary` at serve.rs:1982. If `config_capture` needs the event on the test's own thread, note `#[tokio::test]` runs current-thread — `discard_running` executes inline on it, so the capture sees it.) + +- [ ] **Step 2: Run the test and verify the intended failure** + +Run: `cargo test -p freshell-opencode discard_running_emits` + +Expected: FAIL — no `freshagent.opencode.daemon_discarded` event is captured (discard_running is tracing-silent today). + +- [ ] **Step 3: Add the minimal production implementation** + +```rust +async fn discard_running(&self, reason: &str) { + let taken = self.inner.running.lock().await.take(); + if let Some(running) = taken { + tracing::warn!( + reason = reason, + "freshagent.opencode.daemon_discarded" + ); + running.process.kill(); + } + self.emit_lost_for_all(); +} +``` + +- [ ] **Step 4: Run the focused test** + +Run: `cargo test -p freshell-opencode discard_running_emits` + +Expected: PASS + +- [ ] **Step 5: Refactor while green** + +None (single-site change; keep the underscore removal as the whole diff). + +- [ ] **Step 6: Run impacted-test verification** + +Impacted set: the whole `freshell-opencode` unit suite (tracing capture tests assert event sets; adding an event could affect any test asserting exact event streams — none do outside `config_capture`). + +Run: `cargo test -p freshell-opencode` + +Expected: PASS + +- [ ] **Step 7: Commit the task** + +```bash +git add crates/freshell-opencode/src/serve.rs +git commit -m "feat(opencode): structured log for shared-daemon discards" +``` + +--- + +### Task 3: Manager-level daemon-loss machinery — exit watcher, loss signal, backoff re-warm + +**Files:** +- Modify: `crates/freshell-opencode/src/serve.rs` (Inner at ~:643-655, `RunningServe` at ~:634-641, `ensure_started` at ~:690-770, `discard_running` at ~:1348, `ServeConfig` at ~:557-603, `shutdown` at ~:1510-1517) +- Test: `crates/freshell-opencode/src/serve.rs` `#[cfg(test)]` + a new integration file `crates/freshell-opencode/tests/serve_daemon_selfheal.rs` (follows `serve_idle_edge.rs` / `serve_health_bounded.rs` conventions) + +**Interfaces:** +- Consumes: `ServeProcess::exited()` (serve.rs:404-411), `emit_lost_for_all` (serve.rs:1332), Task 2's log site. +- Produces (used by Task 4 and later tests): + - `pub enum DaemonSignal { Lost { reason: &'static str }, Started }` (crate-root re-export) + - `OpencodeServeManager::subscribe_daemon_signals(&self) -> tokio::sync::broadcast::Receiver` (capacity 16) + - `ServeConfig` gains: `daemon_watch_interval: Duration` (default 1000 ms), `re_warm_backoff_initial_ms: u64` (default 2000), `re_warm_backoff_max_ms: u64` (default 60_000). + - Semantics: `Started` is broadcast on every successful cold start (not on fast-path returns of an already-running daemon); `Lost{reason}` on discard (`"request_timeout"` today) and on unrequested process exit (`"process_exit"`). + +**Behavior:** +1. `ensure_started` spawns a daemon exit-watcher task after a successful health check (store its abort handle on `RunningServe` as `_exit_watch`). The watcher polls `process.exited()` every `daemon_watch_interval`; on `Some(exit)` it verifies the running entry is still ITS daemon (compare the captured `ownership_id`), then runs the manager's loss path. +2. The loss path (shared by watcher-exit and, minus the abort, by `discard_running`): WARN `freshagent.opencode.daemon_crash_detected` (watcher arm; fields `reason="process_exit"`, `base_url`) or the Task-2 discard WARN; take the running entry (killing it in the watcher arm is unnecessary — the process already exited; still call `process.kill()` for the /proc ownership reaper parity); `emit_lost_for_all()`; broadcast `DaemonSignal::Lost{reason}`; schedule a backoff-guarded re-warm. +3. Re-warm: a spawned task sleeps `min(re_warm_backoff_initial_ms * 2^(attempts-1), re_warm_backoff_max_ms)`, then calls `ensure_started()` (shutdown-flag checked inside; the task also checks it before sleeping). `attempts` is an `AtomicUsize` on `Inner`, incremented per scheduled re-warm, never reset (a crash-looping daemon retries at the max interval forever — self-heals when e.g. disk frees). Log `tracing::info!(outcome=..., attempt=..., "freshagent.opencode.daemon_re_warm")` on success and `tracing::warn!` on failure. +4. `discard_running` aborts the watcher FIRST (requested kill — no crash event), then the existing kill+lost, then `Lost` signal + re-warm schedule. +5. `shutdown`'s inline duplicate (serve.rs:1510-1517) also aborts the watcher; it must NOT schedule a re-warm (shutdown flag blocks it) and need not signal (server is going down) — keep it minimal: abort watcher + existing behavior. + +- [ ] **Step 1: Write the failing behavioral tests** + +New integration file `crates/freshell-opencode/tests/serve_daemon_selfheal.rs` (drafts; adapt fakes from `serve_health_bounded.rs:41-127` — add an `ExitingProcess` fake whose `exited()` flips to `Some(0)` after the test sets a shared flag, plus a kill counter): + +```rust +// A daemon that dies on its own must not leave a poisoned running entry +// forever (the 2026-09-20 incident's silent half): the watcher clears the +// entry, emits Lost for in-flight turns, signals daemon loss, and schedules +// a backoff-guarded respawn. +#[tokio::test] +async fn unrequested_daemon_exit_clears_running_emits_lost_and_signals() { + let exiting = Arc::new(AtomicBool::new(false)); + let killed = Arc::new(AtomicUsize::new(0)); + let spawner = FakeSpawner::with_process(ExitingProcess::new(exiting.clone(), killed.clone())); + let mut config = ServeConfig::default(); + config.daemon_watch_interval = Duration::from_millis(10); + config.re_warm_backoff_initial_ms = 5; + let manager = started_manager_with(spawner, config); // health 200 fake + let mut signals = manager.subscribe_daemon_signals(); + let mut idle = manager.subscribe("ses_a").expect("subscribable"); + exiting.store(true, Ordering::SeqCst); // the daemon "exits" + let signal = tokio::time::timeout(Duration::from_secs(2), signals.recv()) + .await.expect("loss signal within budget").expect("channel alive"); + assert!(matches!(signal, DaemonSignal::Lost { reason: "process_exit" })); + assert!(manager.base_url().await.is_none(), "running entry must be cleared"); + assert!(matches!(idle.try_recv(), Ok(SessionSignal::Lost)), "in-flight subscribers must see Lost"); + // ...assert the WARN freshagent.opencode.daemon_crash_detected via the + // tracing capture if the integration file can host it (else assert in unit tests). +} + +// Crash-loop guard: the automatic re-warm must back off exponentially, not +// spawn-storm. +#[tokio::test] +async fn daemon_loss_re_warm_backs_off_exponentially() { + // Process that "dies" immediately after every successful start: + let spawns = Arc::new(AtomicUsize::new(0)); + let spawner = FakeSpawner::with_process(ExitAfterHealthProcess::new(spawns.clone())); + let mut config = ServeConfig::default(); + config.daemon_watch_interval = Duration::from_millis(5); + config.re_warm_backoff_initial_ms = 50; + config.re_warm_backoff_max_ms = 400; + let manager = started_manager_with(spawner, config); + manager.ensure_started().await.expect("first start"); + tokio::time::sleep(Duration::from_millis(700)).await; + let observed = spawns.load(Ordering::SeqCst); + // With 50ms initial doubling to 400ms cap: expected spawns within 700ms + // of the first loss are ~3-4. Un-backed-off would be dozens. Assert a + // conservative bound: + assert!(observed <= 6, "re-warm must back off (observed {observed} spawns)"); +} + +// A discard (the intentional kill path) must ALSO signal daemon loss so the +// runtime self-heal (Task 4) observes it. +#[tokio::test] +async fn discard_running_signals_daemon_loss_and_schedules_re_warm() { + // prompt-timeout discard (Yes-lane), then: + // - DaemonSignal::Lost { reason: "request_timeout" } arrives + // - after the backoff, a respawn happened (spawner count grew) +} +``` + +- [ ] **Step 2: Run the tests and verify the intended failure** + +Run: `cargo test -p freshell-opencode --test serve_daemon_selfheal` + +Expected: FAIL — `subscribe_daemon_signals` does not exist (compile error is the intended missing behavior; write the enum + stub method returning a channel that never signals if needed to make it a runtime red instead — prefer the compile-first red, then a minimal stub for a runtime red on the watcher semantics). + +- [ ] **Step 3: Add the minimal production implementation** + +In serve.rs (sketch — the implementer adapts to the actual Inner/ensure_started structure): + +```rust +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum DaemonSignal { Lost { reason: &'static str }, Started } + +// Inner gains: +// daemon_signals: tokio::sync::broadcast::Sender, (capacity 16, on construction) +// re_warm_attempts: std::sync::atomic::AtomicUsize, +// RunningServe gains: +// _exit_watch: Option, + +// ensure_started, after storing RunningServe (the cold-start success path): +let watch = self.spawn_exit_watch(base_url.clone(), process_handle_for_watch, ownership_id.clone()); +running._exit_watch = watch; +let _ = self.inner.daemon_signals.send(DaemonSignal::Started); + +fn spawn_exit_watch(self: &Arc, base_url: String, process: Box, ownership_id: String) -> Option { + let manager = self.manager_clone(); // however the crate threads weak self (mirror make_dispatch_sink's weak-Inner pattern, serve.rs:836-843) + let interval = ...config.daemon_watch_interval...; + let handle = tokio::spawn(async move { + loop { + tokio::time::sleep(interval).await; + if let Some(_code) = process.exited() { + manager.handle_unrequested_exit(&base_url, &ownership_id).await; + return; + } + } + }); + Some(handle.abort_handle()) +} + +// handle_unrequested_exit: guard stale watchers (compare ownership_id against +// the current running entry), WARN freshagent.opencode.daemon_crash_detected, +// take the entry (kill for reaper parity), emit_lost_for_all, send +// DaemonSignal::Lost{reason:"process_exit"}, schedule_re_warm(). + +// discard_running: abort the watcher (running._exit_watch), Task-2 WARN, kill, +// emit_lost_for_all, send Lost{reason}, schedule_re_warm(). + +fn schedule_re_warm(&self) { + if self.inner.shutdown.load(Ordering::SeqCst) { return; } + let attempts = self.inner.re_warm_attempts.fetch_add(1, Ordering::SeqCst) + 1; + let delay_ms = (self.config().re_warm_backoff_initial_ms.saturating_mul(1 << (attempts-1).min(16))) + .min(self.config().re_warm_backoff_max_ms); + let manager = self.clone(); + tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(delay_ms)).await; + if manager.inner.shutdown.load(Ordering::SeqCst) { return; } + match manager.ensure_started().await { + Ok(_) => tracing::info!(attempt = attempts, "freshagent.opencode.daemon_re_warm"), + Err(err) => tracing::warn!(attempt = attempts, error = %err, "freshagent.opencode.daemon_re_warm"), + } + }); +} + +pub fn subscribe_daemon_signals(&self) -> tokio::sync::broadcast::Receiver { + self.inner.daemon_signals.subscribe() +} +``` + +- [ ] **Step 4: Run the focused tests** + +Run: `cargo test -p freshell-opencode --test serve_daemon_selfheal && cargo test -p freshell-opencode` + +Expected: PASS (including all pre-existing tests — especially `rejects_on_sidecar_lost`, `settles_within_deadline_when_health_never_resolves` (its kill path now aborts the watcher too), and the FR2 trio). + +- [ ] **Step 5: Refactor while green** + +Fold the shared loss path (take-entry + lost + signal + schedule) into one private helper used by both the watcher arm and `discard_running` (the only difference: the WARN text/reason and the pre-abort). + +- [ ] **Step 6: Run impacted-test verification** + +Impacted set: all of `freshell-opencode` (manager core), plus `freshell-freshagent opencode_ws::tests` (the runtime drives the manager — its fakes seed via `set_manager_for_test`; new fields/behavior must not break the 119 existing tests) and `freshell-freshagent lib::tests` (FR2 pins). + +Run: `cargo test -p freshell-opencode && cargo test -p freshell-freshagent opencode_ws::tests && cargo test -p freshell-freshagent lib::tests` + +Expected: PASS + +- [ ] **Step 7: Commit the task** + +```bash +git add crates/freshell-opencode/src/serve.rs crates/freshell-opencode/tests/serve_daemon_selfheal.rs +git commit -m "feat(opencode): daemon exit watcher, loss signal, and backoff re-warm" +``` + +--- + +### Task 4: Runtime-level self-heal — daemon-loss fan-out, bridge restart, and pane revival + +**Files:** +- Modify: `crates/freshell-freshagent/src/opencode_ws.rs` (state struct ~:92-162, `handle_attach` ~:5166, `handle_send` ~:1428, `handle_compact` ~:3544, `spawn_serve_bridge` ~:5971) +- Modify: `crates/freshell-freshagent/src/lib.rs` (`ensure_manager` ~:2803 — expose the manager cell clone helper if needed) +- Modify: `AGENTS.md` (architecture prose, "Agent Status Indicators" freshopencode sentence) and `crates/freshell-server/src/logging.rs` canonical event-name list (~:48-54) if it enumerates event names +- Test: `crates/freshell-freshagent/src/opencode_ws.rs` `#[cfg(test)] mod tests` (mirror the codex self-heal test at codex.rs:15846) + +**Interfaces:** +- Consumes: Task 3's `subscribe_daemon_signals()` / `DaemonSignal`; `event_frame`/`emit_fresh_agent_error` (opencode_ws.rs:6068/686), `spawn_serve_bridge` (opencode_ws.rs:5971), `FreshAgentState.broadcast_tx` (lib.rs:1917), `set_manager_for_test` (lib.rs:2837). +- Produces: a per-materialized-session typed edge on daemon loss: `freshAgent.event{provider:"opencode", sessionType:"freshopencode", event:{type:"freshAgent.error", code:"OPENCODE_DAEMON_LOST", message:"The opencode serve daemon was lost unexpectedly - it is restarting automatically."}}` — folds client-side through the EXISTING generic `sessionError` path (fresh-agent-ws.ts:514-520), showing the dismissible "Agent error:" banner and clearing busy. After a successful respawn: dead serve bridges restart and each materialized session gets `freshAgent.session.snapshot{status:"idle"}` (which the client treats as snapshot-invalidating → transcript refetch). No chime: the recovery must never emit `freshAgent.turn.complete`. + +- [ ] **Step 1: Write the failing behavioral test** + +In `opencode_ws.rs` tests (draft; mirror `onexit_self_heal_emits_exited_status_with_no_chime_and_keeps_session_mapped` at codex.rs:15846-15896 and the `state_with_bus` harness at codex.rs:9983-9991 — the opencode tests already have the bus pattern + `set_manager_for_test`): + +```rust +// 2026-09-20 incident: the shared daemon died and NOTHING told the panes — +// no status edge, no respawn, no bridge revival; panes dead-ended on the +// snapshot 409. The runtime self-heal must make daemon loss observable and +// recoverable per session. +#[tokio::test] +async fn daemon_loss_fans_out_a_typed_edge_then_revives_bridges_after_respawn() { + let exiting = Arc::new(AtomicBool::new(false)); + let (state, rx) = opencode_state_with_bus(); // FreshAgentState::new(auth, tx) + FreshOpencodeState, per existing helpers + state.fresh_agent.set_manager_for_test(fake_manager_exiting_after( // health 200, prompt/summarize ok, ExitingProcess(exiting) + exiting.clone(), /* tiny watch + backoff config */)); + let session = materialized_opencode_session(&state, "ses_recover").await; // via handle_send against the fake http, as existing send tests do + exiting.store(true, Ordering::SeqCst); // the daemon dies + let frame = next_fresh_agent_frame(&rx).await; + assert_eq!(frame["event"]["type"], "freshAgent.error"); + assert_eq!(frame["event"]["code"], "OPENCODE_DAEMON_LOST"); + assert_eq!(frame["sessionId"], "ses_recover"); + // NO chime ever accompanies a daemon loss: + assert_no_turn_complete(&rx).await; + exiting.store(false, Ordering::SeqCst); // the re-warm's respawn now succeeds + let frame = next_fresh_agent_frame(&rx).await; + assert_eq!(frame["event"]["type"], "freshAgent.session.snapshot"); + assert_eq!(frame["event"]["status"], "idle"); + assert!(session_serve_bridge_alive(&state, "ses_recover").await, "bridge restarted after respawn"); +} +``` + +- [ ] **Step 2: Run the test and verify the intended failure** + +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` + +Expected: FAIL — no `OPENCODE_DAEMON_LOST` frame is broadcast (today `SessionSignal::Lost` is a no-op at opencode_ws.rs:6018; no listener exists). + +- [ ] **Step 3: Add the minimal production implementation** + +In `opencode_ws.rs` (sketch): + +```rust +// FreshOpencodeState gains: daemon_loss_watcher: Arc> (or AtomicBool) +// and this idempotent starter, called at the top of handle_send, handle_attach, +// and handle_compact: +fn ensure_daemon_loss_watcher(&self) { + if self.daemon_loss_watcher.set(()).is_err() { return; } // already running + let Ok(manager) = ... self.fresh_agent.peek_or_ensure_manager() ... else { reset the guard and return; }; + let sessions = Arc::clone(&self.sessions); + let broadcast_tx = Arc::clone(&self.fresh_agent.broadcast_tx); + let mut signals = manager.subscribe_daemon_signals(); + tokio::spawn(async move { + loop { + let Ok(signal) = signals.recv().await else { return }; + match signal { + DaemonSignal::Lost { reason } => { + tracing::warn!(reason = reason, "freshagent.opencode.daemon_loss_observed"); + let materialized: Vec = sessions.lock().await.values() + .filter_map(|s| s.lock().await.real_session_id.clone()).collect(); + for id in materialized { + let _ = broadcast_tx.send(event_frame_json(&id, json!({ + "type": "freshAgent.error", "sessionId": id, + "code": "OPENCODE_DAEMON_LOST", + "message": "The opencode serve daemon was lost unexpectedly - it is restarting automatically.", + }))); + } + } + DaemonSignal::Started => { + // Restart dead bridges for materialized sessions and push + // an idle snapshot edge (client refetches the transcript). + ...for each materialized session: if bridge handle is_finished/absent -> spawn_serve_bridge(...); send snapshot_event(id, "idle")... + } + } + } + }); +} +``` + +(Adapt to the actual locking model of `sessions` (a `TokioMutex>>>`) and the real `spawn_serve_bridge` signature — it is an async method on `FreshOpencodeState`; if it cannot be called from a free task, factor the bridge-restart body (the same logic as handle_attach's restart arm at opencode_ws.rs:5460-5473) into a standalone async helper both call. Track "Started after Lost" via a local `saw_loss` bool so normal cold starts emit nothing.) + +- [ ] **Step 4: Run the focused test** + +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` + +Expected: PASS + +- [ ] **Step 5: Refactor while green** + +Ensure the fan-out + bridge-revival helper is shared (not duplicated) with `handle_attach`'s restart arm; keep AGENTS.md's "Agent Status Indicators" paragraph truthful — update the freshopencode sentence to document the daemon-loss self-heal (edge shape, no chime, backoff respawn, bridge revival, and the fix-4 client recovery pointer). + +- [ ] **Step 6: Run impacted-test verification** + +Impacted set: all `opencode_ws::tests` (119 tests), `lib::tests` snapshot pins, plus the whole `freshell-freshagent` unit suite. + +Run: `cargo test -p freshell-freshagent` + +Expected: PASS + +- [ ] **Step 7: Commit the task** + +```bash +git add crates/freshell-freshagent/src/opencode_ws.rs crates/freshell-freshagent/src/lib.rs AGENTS.md crates/freshell-server/src/logging.rs +git commit -m "feat(freshopencode): daemon-loss self-heal edge, respawn revival, and bridge restarts" +``` + +--- + +### Task 5: Client drives fenced attach + refetch on the snapshot 409 RESTORE_UNAVAILABLE + +**Files:** +- Modify: `src/components/fresh-agent/FreshAgentView.tsx` (predicate beside `isLostFreshOpencodeThreadError` at ~:455-463; new arm in `handleSnapshotError` at ~:2770-2827; a one-shot guard ref; the pane-refresh attach-decision lane at ~:1718-1761 is the reuse pattern) +- Test: `test/unit/client/components/fresh-agent/FreshAgentView.test.tsx` (beside the 404 test at ~:1887-1930) + +**Interfaces:** +- Consumes: `ApiError.details` (the full 409 body: `code`, `ownerKind`, `ownerGeneration`), `captureAttachmentAttempt` (FreshAgentView.tsx:403-412), `sendFencedFreshAgentAttach` (:1262-1275), `requestSnapshotRefresh` / `requestRevealRefresh`, `selectPaneOwnerFence`. +- Produces: on a 409 `RESTORE_UNAVAILABLE` snapshot error for a freshopencode pane: ONE generation-fenced `freshAgent.attach` (via a fresh attachment decision — bump `attachDecisionSerialRef`, capture, send) followed by a snapshot refetch. Bounded: once per pane identity (`createRequestId` + `snapshotThreadId`); a second 409 falls through to the existing error surfaces (loadError banner / reveal error), no loop. The pane identity is NOT reset (unlike the 404 lost-thread path). + +- [ ] **Step 1: Write the failing behavioral test** + +In FreshAgentView.test.tsx (draft — template is the 404 test at :1887-1930; reuse its harness: `apiMock.getFreshAgentThreadSnapshot.mockRejectedValueOnce`, `StoreBackedFreshAgentView`, `sentFreshAgentMessages`): + +```ts +// 2026-09-20 incident: the daemon died, the reveal GET answered the typed +// 409 RESTORE_UNAVAILABLE, and the pane dead-ended on a dismiss-only banner +// forever ("Session ... is still running on the server."). The documented +// recovery is the generation-fenced attach + refetch — drive it once. +it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and a refetch', async () => { + apiMock.getFreshAgentThreadSnapshot + .mockRejectedValueOnce({ + status: 409, + message: 'Session ses_live is still running on the server.', + details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'terminal', ownerGeneration: 2 }, + }) + .mockResolvedValue(freshopencodeSnapshot({ sessionId: 'ses_live', status: 'idle' })) // the refetch succeeds + renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) + await waitFor(() => { + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(1) + }) + await waitFor(() => { + expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(2) // the refetch + }) + // The pane kept its identity (the 409 is NOT the 404 lost-thread reset): + expect(getFreshAgentPaneContent(store).sessionId).toBe('ses_live') + // And no dead-end banner for the recovered pane: + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()) +}) + +it('does not loop attach attempts on repeated 409s', async () => { + apiMock.getFreshAgentThreadSnapshot.mockRejectedValue({ + status: 409, message: 'Session ses_live is still running on the server.', + details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'terminal', ownerGeneration: 2 }, + }) + renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) + await waitFor(() => expect(screen.findByText(/still running on the server/i)).toBeTruthy()) + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(1) // once, not per fetch +}) +``` + +- [ ] **Step 2: Run the test and verify the intended failure** + +Run: `npm run test:vitest -- run test/unit/client/components/fresh-agent/FreshAgentView.test.tsx -t '409'` + +Expected: FAIL — no attach is sent; the pane shows the dismiss-only alert banner (the current dead-end). + +- [ ] **Step 3: Add the minimal production implementation** + +In FreshAgentView.tsx (sketch): + +```ts +function isRestoreUnavailableSnapshotError(error: unknown): boolean { + if (!error || typeof error !== 'object') return false + const status = 'status' in error ? (error as { status?: unknown }).status : undefined + const details = 'details' in error ? (error as { details?: unknown }).details : undefined + const code = details && typeof details === 'object' && 'code' in details + ? (details as { code?: unknown }).code + : undefined + return status === 409 && code === 'RESTORE_UNAVAILABLE' +} +``` + +In `handleSnapshotError`, after the opencode lost-404 arm and BEFORE the reveal arm — provider-gated to `opencode`: + +```ts +// 2026-09-20 incident: with the daemon dead, daemon-absent snapshot GETs +// answer the typed 409 RESTORE_UNAVAILABLE for as long as the session key +// stays Live. The documented recovery is the generation-fenced attach +// (b8ke Task-5: cold resume flows only through the explicit lifecycle +// commands) — drive it ONCE per pane identity, then refetch. Repeated 409s +// fall through to the honest error surfaces below; never reset the pane. +if (paneContent.provider === 'opencode' && isRestoreUnavailableSnapshotError(error)) { + const fresh = paneContentRef.current + const recoveryKey = `${fresh.createRequestId}:${sessionId}` + if (restoreUnavailableRecoveryRef.current !== recoveryKey) { + restoreUnavailableRecoveryRef.current = recoveryKey + attachDecisionSerialRef.current += 1 + const attempt = captureAttachmentAttempt('restore-unavailable-recovery') + sendFencedFreshAgentAttach(attempt) + } + if (trigger === 'reveal' && snapshotDirtyRef.current) { + revealRefreshStartedAtRef.current = null + setSnapshotRevealError(null) // recovery in flight; don't dead-end the reveal lane + } + setLoadError(null) + requestSnapshotRefresh('manual') // refetch once the attach lands + return +} +``` + +(Adapt: `captureAttachmentAttempt`'s real signature at :403-412; the ref `restoreUnavailableRecoveryRef = useRef(null)` beside the other reveal refs ~:800-804. The `trigger === 'reveal'` branch must still let the reveal-dirty state clear on the NEXT successful fetch — verify against the reveal-refresh state machine at :2601-2624; if clearing `snapshotRevealError` alone is insufficient, keep the reveal arm's bookkeeping and skip its error assignment while recovery is in flight.) + +- [ ] **Step 4: Run the focused test** + +Run: `npm run test:vitest -- run test/unit/client/components/fresh-agent/FreshAgentView.test.tsx -t '409'` + +Expected: PASS + +- [ ] **Step 5: Refactor while green** + +If the 409 arm and the 404 arm now share reset-vs-recover structure, extract only what is genuinely shared (they intentionally differ: reset vs recover) — otherwise leave as-is. + +- [ ] **Step 6: Run impacted-test verification** + +Impacted set: the full FreshAgentView suite (the snapshot error paths, reveal lanes, attach lanes, and scheduler tests all touch `handleSnapshotError`) plus the fresh-agent-ws fold tests. + +Run: `npm run test:vitest -- run test/unit/client/components/fresh-agent/ test/unit/client/lib/fresh-agent-ws.test.ts test/unit/client/lib/fresh-agent-turn-complete.test.ts` + +Expected: PASS + +- [ ] **Step 7: Commit the task** + +```bash +git add src/components/fresh-agent/FreshAgentView.tsx test/unit/client/components/fresh-agent/FreshAgentView.test.tsx +git commit -m "fix(fresh-agent): recover freshopencode panes from snapshot 409 via fenced attach and refetch" +``` + +--- + +### Task 6: Cloud-legal e2e — the 409 recovery story end to end + +**Files:** +- Create: `test/e2e-browser/specs/freshopencode-snapshot-409-recovery.spec.ts` +- Test: itself (local chromium run; must NOT be added to `CLOUD_SKIP_SPECS` in `test/e2e-browser/playwright.cloud.config.ts`) + +**Interfaces:** +- Consumes: the model-picker "sidecar suppressed + routed fetch" pattern (the explicitly-cloud-legal pattern cited at playwright.cloud.config.ts:36-38; follow `test/e2e-browser/specs/freshopencode-model-picker.spec.ts`), the `TestHarness` (`test/e2e-browser/helpers/test-harness.js`), `RustServer` helper. +- Produces: an e2e proof of the Task-5 user story: a freshopencode pane whose snapshot fetch first 409s (`RESTORE_UNAVAILABLE` + `ownerKind`/`ownerGeneration` body) then 200s, recovers by sending `freshAgent.attach` and rendering the transcript — with no dismiss-only dead-end. + +- [ ] **Step 1: Write the failing-passing spec (verification task; Task 5 already turned the behavior green)** + +Draft structure (follow the model-picker spec's routing/suppression mechanics exactly): + +```ts +test('freshopencode pane recovers from a snapshot 409 via fenced attach', async ({ page }) => { + // 1. Suppress the opencode sidecar (model-picker pattern). + // 2. Route /api/fresh-agent/threads/freshopencode/opencode/:id: + // first call -> fulfill(409, { status:'error', code:'RESTORE_UNAVAILABLE', + // message:'Session is still running on the server.', + // ownerKind:'terminal', ownerGeneration:2 }) + // subsequent -> fulfill(200, ) + // 3. Seed a freshopencode pane with a durable ses_* session (harness). + // 4. Assert: a freshAgent.attach frame is sent (harness ws capture), + // the transcript renders from the 200 snapshot, and no dismiss-only + // dead-end alert remains. +}) +``` + +- [ ] **Step 2: Run it locally** + +Run: `npm run test:e2e:local -- --project=chromium test/e2e-browser/specs/freshopencode-snapshot-409-recovery.spec.ts` + +Expected: PASS. (Sanity-check the red history: `git stash` the Task-5 commit is NOT needed — Task 5's unit red already proves the pre-fix dead-end; record that linkage in the commit message.) + +- [ ] **Step 3: Verify cloud inclusion** + +Run: `FRESHELL_E2E_BACKEND=cloud npm run test:e2e` in the coordinated lane (or the narrow cloud invocation the repo sanctions for one spec) and confirm the spec is selected — it must not appear in `CLOUD_SKIP_SPECS`, `LOCAL_ONLY_SPECS`, or match any `CLOUD_SKIP_TITLES` pattern. + +Expected: the spec runs (and passes) on the cloud backend; per AGENTS.md, "a spec sitting in CLOUD_SKIP_SPECS is not coverage". + +- [ ] **Step 4: Commit the task** + +```bash +git add test/e2e-browser/specs/freshopencode-snapshot-409-recovery.spec.ts +git commit -m "test(e2e): freshopencode snapshot-409 recovery runs cloud-legal end to end" +``` + +--- + +## Plan self-review (completed before commit) + +1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + backoff re-warm + runtime fan-out edge + bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 5+6 (client fenced-attach recovery + refetch, cloud-legal e2e). The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config; Task 4 `OPENCODE_DAEMON_LOST` edge). +2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; cloud-skipped real-daemon-lifecycle e2e class) are stated in Global Constraints, each with its reason and precedent. +3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports (`serve-lane-mechanics.md`, `runtime-compact-events.md`, `codex-selfheal-precedent.md`, `client-409-recovery.md`, `server-snapshot-409.md`, `testing-conventions.md`) at base 855dae72a. +4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template). +5. **Placeholder scan:** drafts reference real helpers; where a fake needs a small extension (ExitingProcess, summarize/config hang scripting), the extension is named and its model (existing fakes) is cited — no TBDs. +6. **Operational completeness:** new structured event names are logged (Task 3/4) and registered in the logging docs if enumerated; AGENTS.md architecture prose updated (Task 4); no migrations; rollback = revert the commits (no persisted-state changes). + +UNRESOLVED COVERAGE GAP: none known. The one soft spot — whether `captureAttachmentAttempt`'s real signature supports the Task-5 recovery arm without a wrapper — is a load-bearing assumption carried to Stage 2 for validation before execution. From e5024b65bb722db1504384df4e8cec6b6e75592b Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:07:55 -0700 Subject: [PATCH 02/24] docs: apply load-bearing corrections to the daemon-death recovery plan --- ...26-09-21-opencode-daemon-death-recovery.md | 278 +++++++++++++----- 1 file changed, 200 insertions(+), 78 deletions(-) diff --git a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md index 2acb6bc79..27cee2650 100644 --- a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md +++ b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md @@ -50,6 +50,10 @@ Fix the freshopencode shared-daemon death incident class in the Freshell repo (R - `prompt_async` (the send-turn POST) and the thin `json_request` wrappers (`get_session`, `list_messages`, `get_session_status_map`, `abort`, `fork`, `revert`, `unrevert`) KEEP `DiscardOnTimeout::Yes`. Rationale: they are the deliberate wedged-daemon recycler for writes (FR2 kept Yes for writes on purpose), and the incident class was compact-specific (an LLM-scale budget routinely exceeded by a healthy-but-busy daemon). A wedged-alive daemon is also caught by the new exit-watcher only if it exits; a hung-but-alive daemon remains the send-lane's recycle responsibility. Revisit only with a dedicated wedged-detection design. - The frozen 409 message text stays (even when the Live owner's daemon is dead, the text says "still running on the server" — clients' muscle memory depends on it). The new `OPENCODE_DAEMON_LOST` runtime edge is what tells the user the truth. +- **Terminal-owner 409s are a different scenario from the incident** (log-validated in the load-bearing stage: the incident-time key was `Live{FreshAgent, gen 1}` — the pane's own stale claim; the later-observed `ownerKind:"terminal", ownerGeneration:2` body was the post-salvage handoff state, minted ~2h17m after the daemon died). For a genuine terminal owner the fenced attach is refused by design, and the existing recovery doors are the session-directory handoff (the door the user actually used to salvage the session) or the owning terminal's exit. The client 409 recovery (Task 5) is scoped to `ownerKind:"fresh-agent"` — the stale-own-claim class the incident actually was. +- **The validated core gap for the incident state** (LB-05, falsified): a map-hit fenced attach re-subscribes the serve bridge but never respawns the daemon — `spawn_serve_bridge` never calls `ensure_started`, and `ensure_manager` returns the discarded manager. The attach was exercised 3× in the incident and recovered nothing. Task 4 therefore adds `ensure_started` to the attach tail's bridge-restart arm (mirroring what `resume_durable_session` already does for map-misses), turning the fenced attach into a real recovery verb for the map-hit daemon-dead state. +- After Task 1, a timed-out compact leaves the summarize turn running daemon-side until its own ~600 s budget expires or an interrupt arrives; the pane's busy state settles via the existing await-idle/turn-settle machinery. +- Runtime watcher arming (Task 4): armed from the WS materializing handlers (handle_send/handle_attach/handle_compact) plus an immediate level pass at arming (revive dead bridges if the daemon is already running). Residual: a REST-only, never-viewed pane misses the `OPENCODE_DAEMON_LOST` banner until its first WS interaction — accepted, because the pane renders nothing until viewed, and revival is level-triggered. - E2E daemon-death/respawn coverage against a real spawned daemon is the cloud-skipped provider-lifecycle class (see `CLOUD_SKIP_SPECS`: `freshopencode-restart-recovery` et al.). This run adds cloud-legal e2e for the client recovery lane (Task 6) and covers the server lanes with the Rust unit tests (the same coverage strategy the freshcodex self-heal uses). --- @@ -300,13 +304,14 @@ git commit -m "feat(opencode): structured log for shared-daemon discards" - Consumes: `ServeProcess::exited()` (serve.rs:404-411), `emit_lost_for_all` (serve.rs:1332), Task 2's log site. - Produces (used by Task 4 and later tests): - `pub enum DaemonSignal { Lost { reason: &'static str }, Started }` (crate-root re-export) - - `OpencodeServeManager::subscribe_daemon_signals(&self) -> tokio::sync::broadcast::Receiver` (capacity 16) + - `OpencodeServeManager::subscribe_daemon_signals(&self) -> tokio::sync::broadcast::Receiver` (capacity 16; NOTE: tokio broadcast does NOT replay history to late subscribers — late `subscribe()` starts at the tail, so Task 4's watcher design is level-triggered, not event-history-dependent) - `ServeConfig` gains: `daemon_watch_interval: Duration` (default 1000 ms), `re_warm_backoff_initial_ms: u64` (default 2000), `re_warm_backoff_max_ms: u64` (default 60_000). - - Semantics: `Started` is broadcast on every successful cold start (not on fast-path returns of an already-running daemon); `Lost{reason}` on discard (`"request_timeout"` today) and on unrequested process exit (`"process_exit"`). + - `RunningServe` gains an additive `ownership_id: String` field (currently only a local in `ensure_started`), and `process` becomes `Arc` (LB-06: the watcher needs a handle that outlives the entry; share the Arc, never move the Box). + - Semantics: `Started` is broadcast on every successful cold start (not on fast-path returns of an already-running daemon); `Lost{reason}` on discard (`"request_timeout"` today) and on unrequested process exit (`"process_exit"`). The shared loss path is exactly-once (LB-07): only the arm whose running-entry take yields `Some` logs/signals/schedules; a `None` take is a silent no-op (the watcher-vs-discard race resolves via the take). **Behavior:** 1. `ensure_started` spawns a daemon exit-watcher task after a successful health check (store its abort handle on `RunningServe` as `_exit_watch`). The watcher polls `process.exited()` every `daemon_watch_interval`; on `Some(exit)` it verifies the running entry is still ITS daemon (compare the captured `ownership_id`), then runs the manager's loss path. -2. The loss path (shared by watcher-exit and, minus the abort, by `discard_running`): WARN `freshagent.opencode.daemon_crash_detected` (watcher arm; fields `reason="process_exit"`, `base_url`) or the Task-2 discard WARN; take the running entry (killing it in the watcher arm is unnecessary — the process already exited; still call `process.kill()` for the /proc ownership reaper parity); `emit_lost_for_all()`; broadcast `DaemonSignal::Lost{reason}`; schedule a backoff-guarded re-warm. +2. The loss path (shared by watcher-exit and, minus the abort, by `discard_running`): WARN `freshagent.opencode.daemon_crash_detected` (watcher arm; fields `reason="process_exit"`, `base_url`) or the Task-2 discard WARN; take the running entry (killing it in the watcher arm is unnecessary — the process already exited; still call `process.kill()` for the /proc ownership reaper parity); `emit_lost_for_all()`; broadcast `DaemonSignal::Lost{reason}`; schedule a backoff-guarded re-warm. **Exactly-once (LB-07):** the take is the race arbiter — if it yields `None` (the other arm already ran), the whole path is a silent no-op: no log, no Lost, no re-warm. 3. Re-warm: a spawned task sleeps `min(re_warm_backoff_initial_ms * 2^(attempts-1), re_warm_backoff_max_ms)`, then calls `ensure_started()` (shutdown-flag checked inside; the task also checks it before sleeping). `attempts` is an `AtomicUsize` on `Inner`, incremented per scheduled re-warm, never reset (a crash-looping daemon retries at the max interval forever — self-heals when e.g. disk frees). Log `tracing::info!(outcome=..., attempt=..., "freshagent.opencode.daemon_re_warm")` on success and `tracing::warn!` on failure. 4. `discard_running` aborts the watcher FIRST (requested kill — no crash event), then the existing kill+lost, then `Lost` signal + re-warm schedule. 5. `shutdown`'s inline duplicate (serve.rs:1510-1517) also aborts the watcher; it must NOT schedule a re-warm (shutdown flag blocks it) and need not signal (server is going down) — keep it minimal: abort watcher + existing behavior. @@ -380,7 +385,7 @@ Expected: FAIL — `subscribe_daemon_signals` does not exist (compile error is t - [ ] **Step 3: Add the minimal production implementation** -In serve.rs (sketch — the implementer adapts to the actual Inner/ensure_started structure): +In serve.rs (sketch — the implementer adapts to the actual Inner/ensure_started structure; LB-06/LB-07 corrections applied: the watcher holds an `Arc` clone of the process + the ownership id, never a moved Box): ```rust #[derive(Clone, Debug, PartialEq, Eq)] @@ -390,20 +395,24 @@ pub enum DaemonSignal { Lost { reason: &'static str }, Started } // daemon_signals: tokio::sync::broadcast::Sender, (capacity 16, on construction) // re_warm_attempts: std::sync::atomic::AtomicUsize, // RunningServe gains: +// ownership_id: String, +// process: Arc, // was Box — LB-06 // _exit_watch: Option, // ensure_started, after storing RunningServe (the cold-start success path): -let watch = self.spawn_exit_watch(base_url.clone(), process_handle_for_watch, ownership_id.clone()); +let watch = self.spawn_exit_watch(base_url.clone(), Arc::clone(&process), ownership_id.clone()); running._exit_watch = watch; let _ = self.inner.daemon_signals.send(DaemonSignal::Started); -fn spawn_exit_watch(self: &Arc, base_url: String, process: Box, ownership_id: String) -> Option { - let manager = self.manager_clone(); // however the crate threads weak self (mirror make_dispatch_sink's weak-Inner pattern, serve.rs:836-843) +// The watcher polls the SHARED process Arc (the running entry keeps its own +// Arc; nothing is moved out of RunningServe): +fn spawn_exit_watch(&self, base_url: String, process: Arc, ownership_id: String) -> Option { + let manager = self.clone(); // OpencodeServeManager is Arc-backed-cheap let interval = ...config.daemon_watch_interval...; let handle = tokio::spawn(async move { loop { tokio::time::sleep(interval).await; - if let Some(_code) = process.exited() { + if process.exited().is_some() { manager.handle_unrequested_exit(&base_url, &ownership_id).await; return; } @@ -412,13 +421,28 @@ fn spawn_exit_watch(self: &Arc, base_url: String, process: Box running.take(), // ours, still current + _ => return, // stale watcher (a newer daemon owns the entry) — no-op + } + }; + let Some(running) = taken else { return }; + tracing::warn!(reason = "process_exit", base_url = %base_url, "freshagent.opencode.daemon_crash_detected"); + running.process.kill(); // reaper parity only — the process already exited + self.emit_lost_for_all(); + let _ = self.inner.daemon_signals.send(DaemonSignal::Lost { reason: "process_exit" }); + self.schedule_re_warm(); +} -// discard_running: abort the watcher (running._exit_watch), Task-2 WARN, kill, -// emit_lost_for_all, send Lost{reason}, schedule_re_warm(). +// discard_running: abort the watcher (running._exit_watch) FIRST, Task-2 WARN, +// kill, emit_lost_for_all, send Lost{reason}, schedule_re_warm(). Same +// exactly-once discipline: the take yields None only if the watcher lost the +// race, in which case the abort already ran and the watcher returned. fn schedule_re_warm(&self) { if self.inner.shutdown.load(Ordering::SeqCst) { return; } @@ -477,18 +501,22 @@ git commit -m "feat(opencode): daemon exit watcher, loss signal, and backoff re- - Test: `crates/freshell-freshagent/src/opencode_ws.rs` `#[cfg(test)] mod tests` (mirror the codex self-heal test at codex.rs:15846) **Interfaces:** -- Consumes: Task 3's `subscribe_daemon_signals()` / `DaemonSignal`; `event_frame`/`emit_fresh_agent_error` (opencode_ws.rs:6068/686), `spawn_serve_bridge` (opencode_ws.rs:5971), `FreshAgentState.broadcast_tx` (lib.rs:1917), `set_manager_for_test` (lib.rs:2837). -- Produces: a per-materialized-session typed edge on daemon loss: `freshAgent.event{provider:"opencode", sessionType:"freshopencode", event:{type:"freshAgent.error", code:"OPENCODE_DAEMON_LOST", message:"The opencode serve daemon was lost unexpectedly - it is restarting automatically."}}` — folds client-side through the EXISTING generic `sessionError` path (fresh-agent-ws.ts:514-520), showing the dismissible "Agent error:" banner and clearing busy. After a successful respawn: dead serve bridges restart and each materialized session gets `freshAgent.session.snapshot{status:"idle"}` (which the client treats as snapshot-invalidating → transcript refetch). No chime: the recovery must never emit `freshAgent.turn.complete`. +- Consumes: Task 3's `subscribe_daemon_signals()` / `DaemonSignal`; `event_frame`/`emit_fresh_agent_error` (opencode_ws.rs:6068/686), `spawn_serve_bridge` (opencode_ws.rs:5971), `FreshAgentState.broadcast_tx` (lib.rs:1917), `set_manager_for_test` (lib.rs:2837), `ensure_manager` (lib.rs:2803 — the real seam; there is no `peek_or_ensure_manager`). +- Produces: + - A per-materialized-session typed edge on daemon loss: `freshAgent.event{provider:"opencode", sessionType:"freshopencode", event:{type:"freshAgent.error", code:"OPENCODE_DAEMON_LOST", message:"The opencode serve daemon was lost unexpectedly - it is restarting automatically."}}` — folds client-side through the EXISTING generic `sessionError` path (fresh-agent-ws.ts:514-520), showing the dismissible "Agent error:" banner and clearing busy. + - Level-triggered bridge revival: on arming, and again on every `DaemonSignal::Started`, restart bridges that are dead/absent for materialized sessions and push `freshAgent.session.snapshot{status:"idle"}` ONLY to sessions whose bridge was actually restarted (which the client treats as snapshot-invalidating → transcript refetch). No `saw_loss` heuristic — tokio broadcast does not replay history, so revival must not depend on having seen the `Lost` edge (LB-02). + - **The fenced attach becomes a real recovery verb (LB-05 redesign):** `handle_attach`'s dead-bridge restart arm (opencode_ws.rs:5460-5473) gains `manager.ensure_started().await` before `spawn_serve_bridge` — a map-hit fenced attach against a daemon-absent manager now respawns the shared daemon and re-bridges, exactly as `resume_durable_session` already does for map-misses. No chime: the recovery must never emit `freshAgent.turn.complete`. -- [ ] **Step 1: Write the failing behavioral test** +- [ ] **Step 1: Write the failing behavioral tests** -In `opencode_ws.rs` tests (draft; mirror `onexit_self_heal_emits_exited_status_with_no_chime_and_keeps_session_mapped` at codex.rs:15846-15896 and the `state_with_bus` harness at codex.rs:9983-9991 — the opencode tests already have the bus pattern + `set_manager_for_test`): +In `opencode_ws.rs` tests (drafts; mirror `onexit_self_heal_emits_exited_status_with_no_chime_and_keeps_session_mapped` at codex.rs:15846-15896 and the `state_with_bus` harness at codex.rs:9983-9991 — the opencode tests already have the bus pattern + `set_manager_for_test`): ```rust // 2026-09-20 incident: the shared daemon died and NOTHING told the panes — // no status edge, no respawn, no bridge revival; panes dead-ended on the // snapshot 409. The runtime self-heal must make daemon loss observable and -// recoverable per session. +// recoverable per session. (LB-02: revival is level-triggered — it runs on +// arming and on Started, never dependent on having observed Lost.) #[tokio::test] async fn daemon_loss_fans_out_a_typed_edge_then_revives_bridges_after_respawn() { let exiting = Arc::new(AtomicBool::new(false)); @@ -504,61 +532,123 @@ async fn daemon_loss_fans_out_a_typed_edge_then_revives_bridges_after_respawn() // NO chime ever accompanies a daemon loss: assert_no_turn_complete(&rx).await; exiting.store(false, Ordering::SeqCst); // the re-warm's respawn now succeeds - let frame = next_fresh_agent_frame(&rx).await; + let frame = next_fresh_agent_frame(&rx).await; // DaemonSignal::Started → level-triggered revival assert_eq!(frame["event"]["type"], "freshAgent.session.snapshot"); assert_eq!(frame["event"]["status"], "idle"); assert!(session_serve_bridge_alive(&state, "ses_recover").await, "bridge restarted after respawn"); } + +// LB-05 (falsified → redesign): in the incident, the pane's fenced attach was +// exercised 3× against the dead shared daemon and recovered NOTHING, because +// the attach tail only re-subscribed the bridge — nothing respawns the +// daemon for a map-hit. The attach tail must ensure the daemon exists. +#[tokio::test] +async fn map_hit_fenced_attach_respawns_the_daemon_and_rebridges() { + let spawns = Arc::new(AtomicUsize::new(0)); + let (state, rx) = opencode_state_with_bus(); + state.fresh_agent.set_manager_for_test(fake_manager_with( // health 200; daemon DISCARDED before the attach + FakeSpawner::counting(spawns.clone()))); + let session = materialized_opencode_session(&state, "ses_attach_recover").await; + discard_manager_running_entry(&state).await; // the shared daemon is dead; the session row persists + let fence = observed_fence_for(&state, "ses_attach_recover").await; // the runtime-owner pair + state.handle_attach(attach_msg("ses_attach_recover", fence)).await; + assert!(spawns.load(Ordering::SeqCst) >= 1, "a map-hit attach against a daemon-absent manager must respawn the daemon"); + assert!(session_serve_bridge_alive(&state, "ses_attach_recover").await, "the bridge must be restarted"); + let frame = next_fresh_agent_frame(&rx).await; // the attach tail's snapshot push + assert_eq!(frame["event"]["type"], "freshAgent.session.snapshot"); +} ``` +(Drafts: adapt to the actual harness helpers — `opencode_state_with_bus`, session materialization via `handle_send` against the seeded fake http, and the manager's running-entry discard via the fake's own seams or `discard_running`. Lock discipline per LB-01: any test helper that walks the sessions map must clone the `Arc` session handles under a short map lock and drop the map guard before locking a session — the map guard is NEVER held across a per-session lock acquisition, per the documented contract at opencode_ws.rs:100-115.) + - [ ] **Step 2: Run the test and verify the intended failure** -Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out map_hit_fenced_attach` -Expected: FAIL — no `OPENCODE_DAEMON_LOST` frame is broadcast (today `SessionSignal::Lost` is a no-op at opencode_ws.rs:6018; no listener exists). +Expected: FAIL — `daemon_loss_fans_out...` fails with no `OPENCODE_DAEMON_LOST` frame (today `SessionSignal::Lost` is a no-op at opencode_ws.rs:6018; no listener exists); `map_hit_fenced_attach...` fails because the attach tail never spawns the daemon (spawns == 0, the LB-05-validated gap). - [ ] **Step 3: Add the minimal production implementation** -In `opencode_ws.rs` (sketch): +Two server-side changes (LB-01/LB-02/LB-08/LB-10/N-3 corrections applied): + +**(a) The attach tail's bridge-restart arm (opencode_ws.rs:5460-5473) ensures the daemon exists before re-bridging — the LB-05 redesign that turns the fenced attach into a real recovery verb:** + +```rust +// LB-05 (falsified → redesign): in the incident the fenced attach was +// exercised 3× against the dead daemon and recovered nothing — the tail only +// re-subscribed the bridge. A map-hit attach must respawn the shared daemon +// (mirroring resume_durable_session's map-miss behavior). ensure_started is +// single-flighted, so concurrent attach/send/compact callers cannot spawn a +// second daemon. +let manager = self.fresh_agent.ensure_manager().await; +if let Err(err) = manager.ensure_started().await { + // Bounded health failure: answer the attach with the typed error path + // (the existing emit_fresh_agent_error machinery) instead of a silent + // half-attached state. + ... +} +// ...existing dead-bridge restart + spawn_serve_bridge tail... +``` + +**(b) The daemon-loss watcher on `FreshOpencodeState` (idempotent; armed from handle_send/handle_attach/handle_compact; LB-10: the task holds a full state clone so `spawn_serve_bridge(&self, ...)` is callable directly):** ```rust -// FreshOpencodeState gains: daemon_loss_watcher: Arc> (or AtomicBool) -// and this idempotent starter, called at the top of handle_send, handle_attach, -// and handle_compact: +// FreshOpencodeState gains: daemon_loss_watcher: Arc>. fn ensure_daemon_loss_watcher(&self) { - if self.daemon_loss_watcher.set(()).is_err() { return; } // already running - let Ok(manager) = ... self.fresh_agent.peek_or_ensure_manager() ... else { reset the guard and return; }; - let sessions = Arc::clone(&self.sessions); - let broadcast_tx = Arc::clone(&self.fresh_agent.broadcast_tx); + if self.daemon_loss_watcher.set(()).is_err() { return; } // already armed + let manager = self.fresh_agent.ensure_manager().await; // lib.rs:2803 (N-3: the real seam) + let state = self.clone(); let mut signals = manager.subscribe_daemon_signals(); tokio::spawn(async move { + // Arming-time level pass (LB-02: broadcast does NOT replay history — + // if the daemon already re-warmed before we subscribed, revive now). + state.revive_dead_bridges_if_daemon_running().await; loop { - let Ok(signal) = signals.recv().await else { return }; - match signal { - DaemonSignal::Lost { reason } => { + match signals.recv().await { + Ok(DaemonSignal::Lost { reason }) => { tracing::warn!(reason = reason, "freshagent.opencode.daemon_loss_observed"); - let materialized: Vec = sessions.lock().await.values() - .filter_map(|s| s.lock().await.real_session_id.clone()).collect(); + // LB-01: NEVER hold the sessions-map guard across a + // per-session lock (contract at opencode_ws.rs:100-115 — + // the reverse edge deadlocked production). Clone the + // (id, Arc) pairs under ONE short map lock, + // drop the guard, then read each session outside it. + let materialized: Vec = { + let map = state.sessions.lock().await; + let handles: Vec>> = map.values().cloned().collect(); + drop(map); + handles.into_iter().filter_map(|s| s.lock().await.real_session_id.clone()).collect() + }; for id in materialized { - let _ = broadcast_tx.send(event_frame_json(&id, json!({ + let _ = state.fresh_agent.broadcast_tx.send(event_frame_json(&id, json!({ "type": "freshAgent.error", "sessionId": id, "code": "OPENCODE_DAEMON_LOST", "message": "The opencode serve daemon was lost unexpectedly - it is restarting automatically.", }))); } } - DaemonSignal::Started => { - // Restart dead bridges for materialized sessions and push - // an idle snapshot edge (client refetches the transcript). - ...for each materialized session: if bridge handle is_finished/absent -> spawn_serve_bridge(...); send snapshot_event(id, "idle")... + Ok(DaemonSignal::Started) => { + // Level-triggered revival (LB-02): revive whatever is + // dead; push the idle snapshot ONLY to sessions whose + // bridge was actually restarted. No `saw_loss` heuristic. + state.revive_dead_bridges_if_daemon_running().await; } + Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => continue, // LB-08: never disarm on Lagged + Err(tokio::sync::broadcast::error::RecvError::Closed) => return, } } }); } -``` -(Adapt to the actual locking model of `sessions` (a `TokioMutex>>>`) and the real `spawn_serve_bridge` signature — it is an async method on `FreshOpencodeState`; if it cannot be called from a free task, factor the bridge-restart body (the same logic as handle_attach's restart arm at opencode_ws.rs:5460-5473) into a standalone async helper both call. Track "Started after Lost" via a local `saw_loss` bool so normal cold starts emit nothing.) +// The revival pass (also called at arming), respecting LB-01's lock order: +// 1. manager.base_url().await is None → return (daemon absent — nothing to +// revive into; the next Started signal or attach drives revival). +// 2. Snapshot the map: clone (session_id, Arc>) pairs +// under ONE short map lock, dropping the guard immediately. +// 3. For each pair (OUTSIDE the map guard): lock the session; if +// real_session_id is Some AND the serve-bridge handle is_finished/absent +// → spawn_serve_bridge(...) and broadcast snapshot_event(real_id, "idle"). +// Push the snapshot ONLY to sessions whose bridge was actually restarted. +``` - [ ] **Step 4: Run the focused test** @@ -594,32 +684,38 @@ git commit -m "feat(freshopencode): daemon-loss self-heal edge, respawn revival, - Test: `test/unit/client/components/fresh-agent/FreshAgentView.test.tsx` (beside the 404 test at ~:1887-1930) **Interfaces:** -- Consumes: `ApiError.details` (the full 409 body: `code`, `ownerKind`, `ownerGeneration`), `captureAttachmentAttempt` (FreshAgentView.tsx:403-412), `sendFencedFreshAgentAttach` (:1262-1275), `requestSnapshotRefresh` / `requestRevealRefresh`, `selectPaneOwnerFence`. -- Produces: on a 409 `RESTORE_UNAVAILABLE` snapshot error for a freshopencode pane: ONE generation-fenced `freshAgent.attach` (via a fresh attachment decision — bump `attachDecisionSerialRef`, capture, send) followed by a snapshot refetch. Bounded: once per pane identity (`createRequestId` + `snapshotThreadId`); a second 409 falls through to the existing error surfaces (loadError banner / reveal error), no loop. The pane identity is NOT reset (unlike the 404 lost-thread path). +- Consumes: `ApiError.details` (the full 409 body: `code`, `ownerKind`, `ownerGeneration`), `captureFreshAgentAttachmentAttempt` (the real wrapper at FreshAgentView.tsx — R-1: the pane-refresh reaction pattern at :1718-1761 is the exact reuse: bump `attachDecisionSerialRef`, capture, `sendFencedFreshAgentAttach(attempt)`), `sendFencedFreshAgentAttach` (:1262-1275), `requestSnapshotRefresh` / `requestRevealRefresh`, `selectPaneOwnerFence`. +- Produces: on a 409 `RESTORE_UNAVAILABLE` snapshot error for a freshopencode pane whose refusal names a `fresh-agent` owner (the incident class — the pane's own stale claim; LB-05 scoped terminal owners out to the session-directory handoff door): ONE generation-fenced `freshAgent.attach` followed by a snapshot refetch. Bounded (LB-03): the ENTIRE recovery — attach + refetch — runs once per pane identity (`createRequestId` + `snapshotThreadId`); a second 409 falls through to the existing error surfaces (loadError banner / reveal error), never re-triggering fetches. When the 409 arrived on the reveal lane with `snapshotDirty` set, the recovery drives the reveal-refresh path (`requestRevealRefresh(true)`) so the success-path reveal-dirty clear can run and the "Refreshing conversation" overlay lifts (LB-04). The pane identity is NOT reset (unlike the 404 lost-thread path). - [ ] **Step 1: Write the failing behavioral test** In FreshAgentView.test.tsx (draft — template is the 404 test at :1887-1930; reuse its harness: `apiMock.getFreshAgentThreadSnapshot.mockRejectedValueOnce`, `StoreBackedFreshAgentView`, `sentFreshAgentMessages`): ```ts -// 2026-09-20 incident: the daemon died, the reveal GET answered the typed -// 409 RESTORE_UNAVAILABLE, and the pane dead-ended on a dismiss-only banner -// forever ("Session ... is still running on the server."). The documented -// recovery is the generation-fenced attach + refetch — drive it once. +// 2026-09-20 incident (log-validated): the daemon died, the reveal GET +// answered the typed 409 RESTORE_UNAVAILABLE for the pane's OWN stale +// Live{FreshAgent, gen 1} claim, and the pane dead-ended on a dismiss-only +// banner forever. The documented recovery is the generation-fenced attach + +// refetch — drive it once. +// LB-09: the mount attach already sends ONE freshAgent.attach on mount, so a +// bare length assertion is vacuous — snapshot the sent-frame log after the +// mount settles and assert the POST-409 delta. it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and a refetch', async () => { apiMock.getFreshAgentThreadSnapshot .mockRejectedValueOnce({ status: 409, message: 'Session ses_live is still running on the server.', - details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'terminal', ownerGeneration: 2 }, + details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 1 }, }) .mockResolvedValue(freshopencodeSnapshot({ sessionId: 'ses_live', status: 'idle' })) // the refetch succeeds renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) + await waitFor(() => expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(1)) + const attachCountBeforeRecovery = sentFreshAgentMessages('freshAgent.attach').length // the mount attach await waitFor(() => { - expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(1) + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(attachCountBeforeRecovery + 1) // the RECOVERY attach (LB-09) }) await waitFor(() => { - expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(2) // the refetch + expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(2) // exactly one recovery refetch }) // The pane kept its identity (the 409 is NOT the 404 lost-thread reset): expect(getFreshAgentPaneContent(store).sessionId).toBe('ses_live') @@ -627,14 +723,18 @@ it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()) }) -it('does not loop attach attempts on repeated 409s', async () => { +it('does not loop recovery fetches on repeated 409s', async () => { apiMock.getFreshAgentThreadSnapshot.mockRejectedValue({ status: 409, message: 'Session ses_live is still running on the server.', - details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'terminal', ownerGeneration: 2 }, + details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 1 }, }) renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) await waitFor(() => expect(screen.findByText(/still running on the server/i)).toBeTruthy()) - expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(1) // once, not per fetch + const baseline = sentFreshAgentMessages('freshAgent.attach').length + // Let any would-be refetch loop run (fake timers or a short flush): + await act(async () => { await vi.advanceTimersByTimeAsync(2_000) }) + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(baseline) // one recovery total, not per fetch (LB-03) + expect(apiMock.getFreshAgentThreadSnapshot.mock.calls.length).toBeLessThanOrEqual(3) // mount + recovery only — no loop }) ``` @@ -656,39 +756,58 @@ function isRestoreUnavailableSnapshotError(error: unknown): boolean { const code = details && typeof details === 'object' && 'code' in details ? (details as { code?: unknown }).code : undefined - return status === 409 && code === 'RESTORE_UNAVAILABLE' + const ownerKind = details && typeof details === 'object' && 'ownerKind' in details + ? (details as { ownerKind?: unknown }).ownerKind + : undefined + // LB-05 scoping: fresh-agent owners are the pane's own stale-claim class + // (the incident state) — the fenced attach proceeds for them. Terminal + // owners are a different scenario (a genuinely terminal-owned session); + // their recovery door is the session-directory handoff, so the client does + // not attempt the (refused) attach for them. + return status === 409 && code === 'RESTORE_UNAVAILABLE' && ownerKind === 'fresh-agent' } ``` -In `handleSnapshotError`, after the opencode lost-404 arm and BEFORE the reveal arm — provider-gated to `opencode`: +In `handleSnapshotError`, after the opencode lost-404 arm and BEFORE the reveal arm — provider-gated to `opencode` (LB-03: the ENTIRE recovery — attach + refetch — sits inside the once-per-identity guard; a second 409 falls through to the honest error surfaces below, never looping): ```ts // 2026-09-20 incident: with the daemon dead, daemon-absent snapshot GETs // answer the typed 409 RESTORE_UNAVAILABLE for as long as the session key // stays Live. The documented recovery is the generation-fenced attach // (b8ke Task-5: cold resume flows only through the explicit lifecycle -// commands) — drive it ONCE per pane identity, then refetch. Repeated 409s -// fall through to the honest error surfaces below; never reset the pane. +// commands) — drive it ONCE per pane identity, then refetch via the +// reveal-refresh path when reveal-dirty (LB-04) so the overlay can clear. +// Repeated 409s fall through to the honest error surfaces below; never +// reset the pane. if (paneContent.provider === 'opencode' && isRestoreUnavailableSnapshotError(error)) { const fresh = paneContentRef.current const recoveryKey = `${fresh.createRequestId}:${sessionId}` if (restoreUnavailableRecoveryRef.current !== recoveryKey) { restoreUnavailableRecoveryRef.current = recoveryKey attachDecisionSerialRef.current += 1 - const attempt = captureAttachmentAttempt('restore-unavailable-recovery') + const attempt = captureFreshAgentAttachmentAttempt(fresh) // R-1: the real wrapper's call shape sendFencedFreshAgentAttach(attempt) + // LB-04: a reveal-lane 409 with snapshotDirty set must refetch through + // the reveal path ('reveal' trigger), or the success-path reveal-dirty + // clear never runs and the pane hides behind the "Refreshing + // conversation" overlay forever. Otherwise refetch via 'manual'. + if (trigger === 'reveal' && snapshotDirtyRef.current) { + revealRefreshStartedAtRef.current = null + setSnapshotRevealError(null) + requestRevealRefresh(true) + } else { + setLoadError(null) + requestSnapshotRefresh('manual') + } + return // recovery fired for this error — the honest error surfaces below are for SUBSEQUENT 409s only } - if (trigger === 'reveal' && snapshotDirtyRef.current) { - revealRefreshStartedAtRef.current = null - setSnapshotRevealError(null) // recovery in flight; don't dead-end the reveal lane - } - setLoadError(null) - requestSnapshotRefresh('manual') // refetch once the attach lands - return + // Recovery already attempted for this identity: do NOT clear errors and do + // NOT refetch again — fall through to the reveal error arm / setLoadError + // below so the user sees the honest state. } ``` -(Adapt: `captureAttachmentAttempt`'s real signature at :403-412; the ref `restoreUnavailableRecoveryRef = useRef(null)` beside the other reveal refs ~:800-804. The `trigger === 'reveal'` branch must still let the reveal-dirty state clear on the NEXT successful fetch — verify against the reveal-refresh state machine at :2601-2624; if clearing `snapshotRevealError` alone is insufficient, keep the reveal arm's bookkeeping and skip its error assignment while recovery is in flight.) +(Adapt: the ref `restoreUnavailableRecoveryRef = useRef(null)` beside the other reveal refs ~:800-804; `captureFreshAgentAttachmentAttempt`'s real signature follows the pane-refresh reaction lane at :1735-1739. Verify the reveal-refresh request helper's exact name/behavior (`requestRevealRefresh(true)` forces a reveal-tagged refresh) against the state machine at :2601-2624 and the arming sites ~:1233.) - [ ] **Step 4: Run the focused test** @@ -733,16 +852,19 @@ Draft structure (follow the model-picker spec's routing/suppression mechanics ex ```ts test('freshopencode pane recovers from a snapshot 409 via fenced attach', async ({ page }) => { - // 1. Suppress the opencode sidecar (model-picker pattern). + // 1. Suppress the opencode sidecar (model-picker pattern: + // setSuppressAllFreshAgentNetworkEffects(true) routes freshAgent.* WS + // frames to the harness spy — getSentWsMessages() captures them). // 2. Route /api/fresh-agent/threads/freshopencode/opencode/:id: // first call -> fulfill(409, { status:'error', code:'RESTORE_UNAVAILABLE', // message:'Session is still running on the server.', - // ownerKind:'terminal', ownerGeneration:2 }) + // ownerKind:'fresh-agent', ownerGeneration:1 }) // the incident class (LB-05) // subsequent -> fulfill(200, ) // 3. Seed a freshopencode pane with a durable ses_* session (harness). - // 4. Assert: a freshAgent.attach frame is sent (harness ws capture), - // the transcript renders from the 200 snapshot, and no dismiss-only - // dead-end alert remains. + // 4. Assert: a POST-409 recovery freshAgent.attach frame is sent (R-2: the + // mount attach also appears in the spy log — count the delta after the + // 409 lands, not the total), the transcript renders from the 200 + // snapshot, and no dismiss-only dead-end alert remains. }) ``` @@ -767,13 +889,13 @@ git commit -m "test(e2e): freshopencode snapshot-409 recovery runs cloud-legal e --- -## Plan self-review (completed before commit) +## Plan self-review (re-run after the Stage-2 load-bearing corrections) -1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + backoff re-warm + runtime fan-out edge + bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 5+6 (client fenced-attach recovery + refetch, cloud-legal e2e). The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config; Task 4 `OPENCODE_DAEMON_LOST` edge). -2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; cloud-skipped real-daemon-lifecycle e2e class) are stated in Global Constraints, each with its reason and precedent. -3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports (`serve-lane-mechanics.md`, `runtime-compact-events.md`, `codex-selfheal-precedent.md`, `client-409-recovery.md`, `server-snapshot-409.md`, `testing-conventions.md`) at base 855dae72a. -4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template). +1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + backoff re-warm + runtime fan-out edge + level-triggered bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 4a+5+6 (the fenced attach made a real recovery verb by respawning the daemon on map-hits, the client 409 arm driving it once, cloud-legal e2e). The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config; Task 4 `OPENCODE_DAEMON_LOST` edge). +2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; terminal-owner 409s recover via the session-directory handoff door; cloud-skipped real-daemon-lifecycle e2e class; REST-only never-viewed panes miss the banner until first WS interaction) are stated in Global Constraints, each with its reason and precedent. +3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger: LB-01 (map lock order), LB-02 (no broadcast replay → level-triggered revival + arming pass), LB-03 (bounded recovery), LB-04 (reveal-trigger refetch), LB-05 (falsified: attach must respawn the daemon — Task 4a added; recovery scoped to fresh-agent owners), LB-06 (Arc process + ownership_id), LB-07 (exactly-once take), LB-08 (Lagged tolerance), LB-09 (attach-count delta), LB-10 (state clone), R-1 (`captureFreshAgentAttachmentAttempt`), N-3 (`ensure_manager` seam). +4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing spawn, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template with delta-based attach counting). 5. **Placeholder scan:** drafts reference real helpers; where a fake needs a small extension (ExitingProcess, summarize/config hang scripting), the extension is named and its model (existing fakes) is cited — no TBDs. 6. **Operational completeness:** new structured event names are logged (Task 3/4) and registered in the logging docs if enumerated; AGENTS.md architecture prose updated (Task 4); no migrations; rollback = revert the commits (no persisted-state changes). -UNRESOLVED COVERAGE GAP: none known. The one soft spot — whether `captureAttachmentAttempt`'s real signature supports the Task-5 recovery arm without a wrapper — is a load-bearing assumption carried to Stage 2 for validation before execution. +UNRESOLVED COVERAGE GAP: none. The original soft spot was resolved in Stage 2 (R-1: `captureFreshAgentAttachmentAttempt` supports the mid-flight re-decision), and the one falsified assumption (LB-05) reshaped Tasks 4+5 — the fenced attach now respawns the daemon (map-hit), and the client recovery is scoped to the fresh-agent-owner class the incident actually was. From 7bf6f2821437f8a7613ea88eb7296e4e656ddff1 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:40:43 -0700 Subject: [PATCH 03/24] docs: apply plan-review round 1 fixes to the daemon-death recovery plan --- ...26-09-21-opencode-daemon-death-recovery.md | 204 ++++++++++++++---- 1 file changed, 164 insertions(+), 40 deletions(-) diff --git a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md index 27cee2650..963028b50 100644 --- a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md +++ b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md @@ -41,7 +41,7 @@ Fix the freshopencode shared-daemon death incident class in the Freshell repo (R - Focused test commands (delegated, non-coordinated — safe for TDD loops): - `cargo test -p freshell-opencode` - `cargo test -p freshell-freshagent opencode_ws::tests` - - `cargo test -p freshell-freshagent lib::tests` + - `cargo test -p freshell-freshagent get_opencode_snapshot` (the lib.rs FR2 pins; crate-root tests are named `tests::...`, so filter by test-name substring — `lib::tests` matches nothing) - `npm run test:vitest -- run test/unit/client/components/fresh-agent/FreshAgentView.test.tsx` - `npm run test:e2e:local -- --project=chromium test/e2e-browser/specs/.ts` - Broad/coordinated runs (`npm test`, `test:server` zero-arg, `test:integration` zero-arg) go through the shared coordinator; wait for a free gate, never kill a foreign holder. @@ -54,7 +54,7 @@ Fix the freshopencode shared-daemon death incident class in the Freshell repo (R - **The validated core gap for the incident state** (LB-05, falsified): a map-hit fenced attach re-subscribes the serve bridge but never respawns the daemon — `spawn_serve_bridge` never calls `ensure_started`, and `ensure_manager` returns the discarded manager. The attach was exercised 3× in the incident and recovered nothing. Task 4 therefore adds `ensure_started` to the attach tail's bridge-restart arm (mirroring what `resume_durable_session` already does for map-misses), turning the fenced attach into a real recovery verb for the map-hit daemon-dead state. - After Task 1, a timed-out compact leaves the summarize turn running daemon-side until its own ~600 s budget expires or an interrupt arrives; the pane's busy state settles via the existing await-idle/turn-settle machinery. - Runtime watcher arming (Task 4): armed from the WS materializing handlers (handle_send/handle_attach/handle_compact) plus an immediate level pass at arming (revive dead bridges if the daemon is already running). Residual: a REST-only, never-viewed pane misses the `OPENCODE_DAEMON_LOST` banner until its first WS interaction — accepted, because the pane renders nothing until viewed, and revival is level-triggered. -- E2E daemon-death/respawn coverage against a real spawned daemon is the cloud-skipped provider-lifecycle class (see `CLOUD_SKIP_SPECS`: `freshopencode-restart-recovery` et al.). This run adds cloud-legal e2e for the client recovery lane (Task 6) and covers the server lanes with the Rust unit tests (the same coverage strategy the freshcodex self-heal uses). +- E2E daemon-death/respawn coverage runs in TWO lanes (plan-review round 1): Task 6 adds the cloud-legal client-recovery spec (routed-fetch pattern — it satisfies the configured-cloud-backend PR gate), and Task 7 adds the real-daemon self-heal spec on the LOCAL lane, modeled on the existing `freshopencode-restart-recovery.spec.ts` harness (real RustServer + fake-opencode on PATH). The real-daemon spec lands in `CLOUD_SKIP_SPECS` (same provider-lifecycle-timing class as its model), so the cloud gate's e2e coverage is carried by Task 6 while the end-to-end server lanes (process-exit detection, backoff respawn, bridge revival, status edge) are proven by Task 7 locally plus the Rust unit tests (the same coverage strategy the freshcodex self-heal uses). --- @@ -133,23 +133,26 @@ async fn get_config_timeout_does_not_kill_the_shared_daemon() { - [ ] **Step 2: Run the test and verify the intended failure** -Run: `cargo test -p freshell-opencode compact_timeout_does_not_kill get_config_timeout_does_not` +Run: `cargo test -p freshell-opencode does_not_kill` -Expected: FAIL — `compact_timeout_does_not_kill_the_shared_daemon` fails with `killed: 1` (the discard-on-timeout arm killed the process) and/or `base_url()` is `None`; `get_config_timeout_does_not_kill_the_shared_daemon` fails the same way. +Expected: FAIL — `compact_timeout_does_not_kill_the_shared_daemon` fails with `killed: 1` (the discard-on-timeout arm killed the process) and/or `base_url()` is `None`; `get_config_timeout_does_not_kill_the_shared_daemon` fails the same way. (Single positional filter: cargo test accepts ONE TESTNAME — `does_not_kill` matches both new tests.) - [ ] **Step 3: Add the minimal production implementation** In `crates/freshell-opencode/src/serve.rs`: ```rust -/// `getConfig` for the compact drive's model-pair resolution (and the model -/// catalog lanes). A slow config read must never kill the shared daemon — -/// the FR2 read rule (b8ke): capture the base once (spawn-on-demand is fine), -/// then transport over the captured base with `DiscardOnTimeout::No`. -pub async fn get_config(&self) -> Result { +/// `getConfig` for the compact drive's model-pair resolution (and any other +/// routed config read). Signature UNCHANGED (`route: &Route` stays — the +/// runtime calls `manager.get_config(&route)` for project-scoped config, and +/// the path must be `with_route("/config", route)`). A slow config read must +/// never kill the shared daemon — the FR2 read rule (b8ke): capture the base +/// once (spawn-on-demand is fine), then transport over the captured base +/// with `DiscardOnTimeout::No`. +pub async fn get_config(&self, route: &Route) -> Result { let base = self.require_base().await?; self.json_request_over_base( - HttpMethod::Get, "/config", None, None, + HttpMethod::Get, &with_route("/config", route), None, None, base, DiscardOnTimeout::No, &[], @@ -181,7 +184,7 @@ Ok(()) - [ ] **Step 4: Run the focused test** -Run: `cargo test -p freshell-opencode compact_timeout_does_not_kill get_config_timeout_does_not` +Run: `cargo test -p freshell-opencode does_not_kill` Expected: PASS @@ -367,6 +370,35 @@ async fn daemon_loss_re_warm_backs_off_exponentially() { assert!(observed <= 6, "re-warm must back off (observed {observed} spawns)"); } +// Plan-review round 1: a failed re-warm attempt must RETRY (the loop), not +// give up — a transient spawn/health failure (e.g. disk pressure) must not +// leave the daemon permanently absent until unrelated user activity. +#[tokio::test] +async fn a_failed_re_warm_retries_until_the_daemon_starts() { + // Scripted spawner/health: the first two cold starts FAIL health, the + // third succeeds (a spawner whose fake process exits before health, then + // a healthy one — or an http fake scripted 500/500/200 on /global/health). + let spawns = Arc::new(AtomicUsize::new(0)); + let (manager, _http) = started_manager_with_failing_then_healthy( + /* fail_twice_then_healthy */ spawns.clone(), + ServeConfig { + daemon_watch_interval: Duration::from_millis(5), + re_warm_backoff_initial_ms: 10, + re_warm_backoff_max_ms: 50, + ..ServeConfig::default() + }); + manager.ensure_started().await.expect_err("first start fails (scripted)"); + // Drive the FIRST loss (watcher fires on the dead fake process), then the + // retry loop must keep trying until the scripted success: + tokio::time::timeout(Duration::from_secs(2), async { + loop { + if manager.base_url().await.is_some() { break; } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }).await.expect("the re-warm loop must eventually succeed"); + assert!(spawns.load(Ordering::SeqCst) >= 3, "failed attempts must be retried (observed {})", spawns.load(Ordering::SeqCst)); +} + // A discard (the intentional kill path) must ALSO signal daemon loss so the // runtime self-heal (Task 4) observes it. #[tokio::test] @@ -446,16 +478,29 @@ async fn handle_unrequested_exit(&self, base_url: &str, ownership_id: &str) { fn schedule_re_warm(&self) { if self.inner.shutdown.load(Ordering::SeqCst) { return; } - let attempts = self.inner.re_warm_attempts.fetch_add(1, Ordering::SeqCst) + 1; - let delay_ms = (self.config().re_warm_backoff_initial_ms.saturating_mul(1 << (attempts-1).min(16))) - .min(self.config().re_warm_backoff_max_ms); let manager = self.clone(); tokio::spawn(async move { - tokio::time::sleep(Duration::from_millis(delay_ms)).await; - if manager.inner.shutdown.load(Ordering::SeqCst) { return; } - match manager.ensure_started().await { - Ok(_) => tracing::info!(attempt = attempts, "freshagent.opencode.daemon_re_warm"), - Err(err) => tracing::warn!(attempt = attempts, error = %err, "freshagent.opencode.daemon_re_warm"), + // RETRY LOOP (plan-review round 1): a failed attempt must schedule + // the NEXT attempt — a single-shot spawn that only WARNs leaves the + // daemon permanently absent (the disk-pressure case). Retry forever at + // the capped interval; the shutdown flag is checked each iteration. + loop { + let attempts = manager.inner.re_warm_attempts.fetch_add(1, Ordering::SeqCst) + 1; + let delay_ms = (manager.config().re_warm_backoff_initial_ms + .saturating_mul(1u64 << (attempts - 1).min(16))) + .min(manager.config().re_warm_backoff_max_ms); + tokio::time::sleep(Duration::from_millis(delay_ms)).await; + if manager.inner.shutdown.load(Ordering::SeqCst) { return; } + match manager.ensure_started().await { + Ok(_) => { + tracing::info!(attempt = attempts, "freshagent.opencode.daemon_re_warm"); + return; // started — the loop ends; the exit watcher arms again for this daemon + } + Err(err) => { + // Log and RETRY (backoff escalates via the attempts counter). + tracing::warn!(attempt = attempts, error = %err, "freshagent.opencode.daemon_re_warm"); + } + } } }); } @@ -479,7 +524,7 @@ Fold the shared loss path (take-entry + lost + signal + schedule) into one priva Impacted set: all of `freshell-opencode` (manager core), plus `freshell-freshagent opencode_ws::tests` (the runtime drives the manager — its fakes seed via `set_manager_for_test`; new fields/behavior must not break the 119 existing tests) and `freshell-freshagent lib::tests` (FR2 pins). -Run: `cargo test -p freshell-opencode && cargo test -p freshell-freshagent opencode_ws::tests && cargo test -p freshell-freshagent lib::tests` +Run: `cargo test -p freshell-opencode && cargo test -p freshell-freshagent opencode_ws::tests && cargo test -p freshell-freshagent get_opencode_snapshot` Expected: PASS @@ -531,6 +576,10 @@ async fn daemon_loss_fans_out_a_typed_edge_then_revives_bridges_after_respawn() assert_eq!(frame["sessionId"], "ses_recover"); // NO chime ever accompanies a daemon loss: assert_no_turn_complete(&rx).await; + // Plan-review round 1 (Minor): the session is dual-keyed (placeholder + + // ses_*) in the map — exactly ONE OPENCODE_DAEMON_LOST edge per + // materialized session must be emitted (drain the bus and count). + assert_exactly_one_daemon_lost_edge(&rx, "ses_recover").await; exiting.store(false, Ordering::SeqCst); // the re-warm's respawn now succeeds let frame = next_fresh_agent_frame(&rx).await; // DaemonSignal::Started → level-triggered revival assert_eq!(frame["event"]["type"], "freshAgent.session.snapshot"); @@ -563,7 +612,7 @@ async fn map_hit_fenced_attach_respawns_the_daemon_and_rebridges() { - [ ] **Step 2: Run the test and verify the intended failure** -Run: `cargo test -p freshell-freshagent daemon_loss_fans_out map_hit_fenced_attach` +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` && `cargo test -p freshell-freshagent map_hit_fenced_attach` (two commands — cargo test accepts ONE positional TESTNAME) Expected: FAIL — `daemon_loss_fans_out...` fails with no `OPENCODE_DAEMON_LOST` frame (today `SessionSignal::Lost` is a no-op at opencode_ws.rs:6018; no listener exists); `map_hit_fenced_attach...` fails because the attach tail never spawns the daemon (spawns == 0, the LB-05-validated gap). @@ -612,7 +661,11 @@ fn ensure_daemon_loss_watcher(&self) { // the reverse edge deadlocked production). Clone the // (id, Arc) pairs under ONE short map lock, // drop the guard, then read each session outside it. - let materialized: Vec = { + // Plan-review round 1 (Minor): the map is keyed by BOTH + // the placeholder and the durable id pointing at the SAME + // session — dedupe by real_session_id (BTreeSet) so each + // materialized session gets exactly ONE edge. + let materialized: std::collections::BTreeSet = { let map = state.sessions.lock().await; let handles: Vec>> = map.values().cloned().collect(); drop(map); @@ -652,7 +705,7 @@ fn ensure_daemon_loss_watcher(&self) { - [ ] **Step 4: Run the focused test** -Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` && `cargo test -p freshell-freshagent map_hit_fenced_attach` Expected: PASS @@ -662,7 +715,7 @@ Ensure the fan-out + bridge-revival helper is shared (not duplicated) with `hand - [ ] **Step 6: Run impacted-test verification** -Impacted set: all `opencode_ws::tests` (119 tests), `lib::tests` snapshot pins, plus the whole `freshell-freshagent` unit suite. +Impacted set: all `opencode_ws::tests` (119 tests) plus the lib.rs snapshot pins (`get_opencode_snapshot_*` — crate-root `mod tests` tests are named `tests::...`, so the filter is the test-name substring, not `lib::tests`). Run: `cargo test -p freshell-freshagent` @@ -698,19 +751,26 @@ In FreshAgentView.test.tsx (draft — template is the 404 test at :1887-1930; re // banner forever. The documented recovery is the generation-fenced attach + // refetch — drive it once. // LB-09: the mount attach already sends ONE freshAgent.attach on mount, so a -// bare length assertion is vacuous — snapshot the sent-frame log after the -// mount settles and assert the POST-409 delta. +// bare length assertion is vacuous — read the baseline AFTER the mount +// settles and assert the POST-409 delta. +// Plan-review round 1: DEFER the first rejection until after the baseline is +// read — an immediately-rejected mock races the mount fetch (the recovery +// attach may land before the test snapshots the count). it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and a refetch', async () => { + let rejectFirstSnapshot!: (error: unknown) => void apiMock.getFreshAgentThreadSnapshot - .mockRejectedValueOnce({ + .mockImplementationOnce(() => new Promise((_, reject) => { rejectFirstSnapshot = reject })) + .mockResolvedValue(freshopencodeSnapshot({ sessionId: 'ses_live', status: 'idle' })) // the refetch succeeds + renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) + await waitFor(() => expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(1)) + const attachCountBeforeRecovery = sentFreshAgentMessages('freshAgent.attach').length // the mount attach, settled + await act(async () => { + rejectFirstSnapshot({ status: 409, message: 'Session ses_live is still running on the server.', details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 1 }, }) - .mockResolvedValue(freshopencodeSnapshot({ sessionId: 'ses_live', status: 'idle' })) // the refetch succeeds - renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) - await waitFor(() => expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(1)) - const attachCountBeforeRecovery = sentFreshAgentMessages('freshAgent.attach').length // the mount attach + }) await waitFor(() => { expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(attachCountBeforeRecovery + 1) // the RECOVERY attach (LB-09) }) @@ -729,7 +789,9 @@ it('does not loop recovery fetches on repeated 409s', async () => { details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 1 }, }) renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) - await waitFor(() => expect(screen.findByText(/still running on the server/i)).toBeTruthy()) + // Plan-review round 1: AWAIT the banner — findByText returns a promise; + // asserting its truthiness is always true and leaves the baseline unordered. + await screen.findByText(/still running on the server/i) const baseline = sentFreshAgentMessages('freshAgent.attach').length // Let any would-be refetch loop run (fake timers or a short flush): await act(async () => { await vi.advanceTimersByTimeAsync(2_000) }) @@ -889,13 +951,75 @@ git commit -m "test(e2e): freshopencode snapshot-409 recovery runs cloud-legal e --- -## Plan self-review (re-run after the Stage-2 load-bearing corrections) +### Task 7: Local-lane e2e — the real daemon-death self-heal path end to end + +**Files:** +- Create: `test/e2e-browser/specs/freshopencode-daemon-death-selfheal.spec.ts` +- Modify: `test/e2e-browser/playwright.cloud.config.ts` (add the new spec to `CLOUD_SKIP_SPECS`, same provider-lifecycle-timing reason as `freshopencode-restart-recovery`) +- Test: itself (local chromium run) + +**Interfaces:** +- Consumes: the `freshopencode-restart-recovery.spec.ts` harness pattern — `installFakeOpencode` (`fixtures/fake-opencode.cjs` on the spawned server's PATH), the `RustServer` + `TestHarness` helpers, the fake's `FAKE_OPENCODE_AUDIT_LOG` JSONL for spawn/event assertions, and a fixture capability to make the fake daemon process DIE on demand (reuse the model spec's daemon restart/death mechanics if present; otherwise add a minimal scripted-exit verb to the fixture — e.g. an env-armed exit or a served `/__kill` endpoint the spec hits). +- Produces: an e2e proof of the SERVER-side self-heal chain with a real spawned server and a fake daemon process: (1) the pane is materialized and live; (2) the daemon dies an UNREQUESTED death; (3) the pane shows the `OPENCODE_DAEMON_LOST` "Agent error:" banner; (4) the daemon respawns automatically within a bounded wait (audit log shows a second serve spawn); (5) the pane recovers (the idle snapshot push refetches the transcript; the banner is dismissible and no dead-end remains); (6) NO `freshAgent.turn.complete` chime during the window. + +- [ ] **Step 1: Write the spec** (verification task; Tasks 3+4 turned the chain green — their Rust unit tests carry the TDD red history for this behavior) + +Follow the restart-recovery spec's structure (fake CLI on PATH, harness-seeded freshopencode pane with a durable `ses_*` id, deterministic waits on harness state — never wall-clock-sensitive provider-boot timing). + +- [ ] **Step 2: Run it locally** + +Run: `npm run test:e2e:local -- --project=chromium test/e2e-browser/specs/freshopencode-daemon-death-selfheal.spec.ts` + +Expected: PASS + +- [ ] **Step 3: Register the cloud-skip honestly** + +Add the filename to `CLOUD_SKIP_SPECS` (the spec is the same provider-lifecycle class as its model — 2-CPU/2-worker cloud contention cannot guarantee daemon-death timing). Cloud-backend PR coverage is carried by Task 6's cloud-legal spec; this spec is the local-lane end-to-end proof. + +- [ ] **Step 4: Commit the task** + +```bash +git add test/e2e-browser/specs/freshopencode-daemon-death-selfheal.spec.ts test/e2e-browser/playwright.cloud.config.ts +git commit -m "test(e2e): real-daemon death self-heal recovery runs end to end (local lane)" +``` + +--- + +### Task 8: Whole-branch verification gates + +**Files:** none (verification only — the plan declares these gates, so the plan must run them; the-usual's Stage-5 exit additionally runs the coordinated full suite once at the final HEAD after the review loop closes) + +- [ ] **Step 1: Rust formatting and lints** + +Run: `cargo fmt --all --check && cargo clippy --workspace --exclude freshell-tauri --all-targets -- -D warnings` + +Expected: PASS (clean) + +- [ ] **Step 2: Client typecheck and lints** + +Run: `npm run typecheck && npm run lint` + +Expected: PASS (clean; eslint includes the jsx-a11y rules) + +- [ ] **Step 3: Focused suite confirmation** + +Run: `cargo test -p freshell-opencode && cargo test -p freshell-freshagent && npm run test:vitest -- run test/unit/client/components/fresh-agent/ test/unit/client/lib/fresh-agent-ws.test.ts` + +Expected: PASS (all tasks' focused suites green together on the final HEAD) + +- [ ] **Step 4: Record** + +No commit (verification only). Record the gate results in the run state; the coordinated full-suite gate at final HEAD runs per the-usual Stage 5 after the delta review loop ends. + +--- + +## Plan self-review (re-run after Stage-2 load-bearing corrections AND plan-review round 1) -1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + backoff re-warm + runtime fan-out edge + level-triggered bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 4a+5+6 (the fenced attach made a real recovery verb by respawning the daemon on map-hits, the client 409 arm driving it once, cloud-legal e2e). The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config; Task 4 `OPENCODE_DAEMON_LOST` edge). -2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; terminal-owner 409s recover via the session-directory handoff door; cloud-skipped real-daemon-lifecycle e2e class; REST-only never-viewed panes miss the banner until first WS interaction) are stated in Global Constraints, each with its reason and precedent. -3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger: LB-01 (map lock order), LB-02 (no broadcast replay → level-triggered revival + arming pass), LB-03 (bounded recovery), LB-04 (reveal-trigger refetch), LB-05 (falsified: attach must respawn the daemon — Task 4a added; recovery scoped to fresh-agent owners), LB-06 (Arc process + ownership_id), LB-07 (exactly-once take), LB-08 (Lagged tolerance), LB-09 (attach-count delta), LB-10 (state clone), R-1 (`captureFreshAgentAttachmentAttempt`), N-3 (`ensure_manager` seam). -4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing spawn, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template with delta-based attach counting). -5. **Placeholder scan:** drafts reference real helpers; where a fake needs a small extension (ExitingProcess, summarize/config hang scripting), the extension is named and its model (existing fakes) is cited — no TBDs. -6. **Operational completeness:** new structured event names are logged (Task 3/4) and registered in the logging docs if enumerated; AGENTS.md architecture prose updated (Task 4); no migrations; rollback = revert the commits (no persisted-state changes). +1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + retrying backoff re-warm + runtime fan-out edge + level-triggered bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 4a+5+6 (the fenced attach made a real recovery verb by respawning the daemon on map-hits, the client 409 arm driving it once, cloud-legal e2e). E2E coverage: Task 6 (cloud-legal client recovery) + Task 7 (local-lane real-daemon self-heal chain) + Rust unit tests (server lanes). Gates: Task 8 runs fmt/clippy/typecheck/lint + the focused suites on the final HEAD; the coordinated full suite runs at the-usual Stage-5 exit. The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config + retry loop; Task 4 `OPENCODE_DAEMON_LOST` edge). +2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; terminal-owner 409s recover via the session-directory handoff door; REST-only never-viewed panes miss the banner until first WS interaction) are stated in Global Constraints, each with its reason and precedent. The real-daemon e2e is local-lane with an honest CLOUD_SKIP_SPECS entry (cloud PR coverage carried by Task 6). +3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger (LB-01 map lock order, LB-02 no-replay → level-triggered revival + arming pass, LB-03 bounded recovery, LB-04 reveal-trigger refetch, LB-05 attach-respawn redesign + fresh-agent-owner scoping, LB-06 Arc process + ownership_id, LB-07 exactly-once take, LB-08 Lagged tolerance, LB-09 attach-count delta, LB-10 state clone, R-1 `captureFreshAgentAttachmentAttempt`, N-3 `ensure_manager` seam) AND plan-review round 1 (routed `get_config(route)`, retrying re-warm with a fail-then-succeed test, single-filter cargo commands, deferred-rejection + awaited-banner client tests, dual-key dedupe, Tasks 7/8). +4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing spawn, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template with delta-based attach counting and an explicit synchronization point). Every cargo invocation uses a single positional TESTNAME filter that matches the named tests. +5. **Placeholder scan:** drafts reference real helpers; where a fake needs a small extension (ExitingProcess, summarize/config hang scripting, fail-twice-then-healthy scripting, the fake-opencode death verb), the extension is named and its model (existing fakes) is cited — no TBDs. +6. **Operational completeness:** new structured event names are logged (Task 3/4) and registered in the logging docs if enumerated; AGENTS.md architecture prose updated (Task 4); no migrations; rollback = revert the commits (no persisted-state changes); verification gates explicit (Task 8 + the Stage-5 full suite). -UNRESOLVED COVERAGE GAP: none. The original soft spot was resolved in Stage 2 (R-1: `captureFreshAgentAttachmentAttempt` supports the mid-flight re-decision), and the one falsified assumption (LB-05) reshaped Tasks 4+5 — the fenced attach now respawns the daemon (map-hit), and the client recovery is scoped to the fresh-agent-owner class the incident actually was. +UNRESOLVED COVERAGE GAP: none. The original soft spot was resolved in Stage 2 (R-1), and the one falsified assumption (LB-05) reshaped Tasks 4+5 — the fenced attach now respawns the daemon (map-hit), and the client recovery is scoped to the fresh-agent-owner class the incident actually was. From 385da616ab3dcf520a4fbeee2f34bd2e8c245df7 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:01:32 -0700 Subject: [PATCH 04/24] docs: apply plan-review round 2 fixes (fence binding, re-warm test shape, ApiError tests) --- ...26-09-21-opencode-daemon-death-recovery.md | 131 ++++++++++++------ 1 file changed, 86 insertions(+), 45 deletions(-) diff --git a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md index 963028b50..283a041ba 100644 --- a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md +++ b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md @@ -373,30 +373,34 @@ async fn daemon_loss_re_warm_backs_off_exponentially() { // Plan-review round 1: a failed re-warm attempt must RETRY (the loop), not // give up — a transient spawn/health failure (e.g. disk pressure) must not // leave the daemon permanently absent until unrelated user activity. +// Plan-review round 2: the exit watcher arms only after a SUCCESSFUL health +// check — so the test must first start a HEALTHY daemon, make THAT daemon +// exit (the watcher's loss path schedules the re-warm), and only then script +// the re-warm attempts to fail-fail-succeed. #[tokio::test] async fn a_failed_re_warm_retries_until_the_daemon_starts() { - // Scripted spawner/health: the first two cold starts FAIL health, the - // third succeeds (a spawner whose fake process exits before health, then - // a healthy one — or an http fake scripted 500/500/200 on /global/health). let spawns = Arc::new(AtomicUsize::new(0)); - let (manager, _http) = started_manager_with_failing_then_healthy( - /* fail_twice_then_healthy */ spawns.clone(), + // Scripted topology: spawn #1 healthy (watcher armed) and exits on demand; + // re-warm spawns #2 and #3 fail health; spawn #4 is healthy. + let (manager, http) = started_manager_with( + HealthyThenDyingThenFailFailThenHealthy::new(spawns.clone()), ServeConfig { daemon_watch_interval: Duration::from_millis(5), re_warm_backoff_initial_ms: 10, re_warm_backoff_max_ms: 50, ..ServeConfig::default() }); - manager.ensure_started().await.expect_err("first start fails (scripted)"); - // Drive the FIRST loss (watcher fires on the dead fake process), then the - // retry loop must keep trying until the scripted success: + manager.ensure_started().await.expect("first start is healthy"); + make_fake_daemon_exit(&manager).await; // the unrequested exit the watcher detects tokio::time::timeout(Duration::from_secs(2), async { loop { if manager.base_url().await.is_some() { break; } tokio::time::sleep(Duration::from_millis(10)).await; } - }).await.expect("the re-warm loop must eventually succeed"); - assert!(spawns.load(Ordering::SeqCst) >= 3, "failed attempts must be retried (observed {})", spawns.load(Ordering::SeqCst)); + }).await.expect("the re-warm loop must eventually succeed (fail, fail, then healthy)"); + assert!(spawns.load(Ordering::SeqCst) >= 4, + "the failed re-warm attempts must be retried (observed {} spawns: initial + 2 failures + success)", + spawns.load(Ordering::SeqCst)); } // A discard (the intentional kill path) must ALSO signal daemon loss so the @@ -493,6 +497,10 @@ fn schedule_re_warm(&self) { if manager.inner.shutdown.load(Ordering::SeqCst) { return; } match manager.ensure_started().await { Ok(_) => { + // Plan-review round 2 (Minor): reset the attempts counter + // on success — historical incidents must not accumulate + // into a permanent 60 s first delay for the NEXT loss. + manager.inner.re_warm_attempts.store(0, Ordering::SeqCst); tracing::info!(attempt = attempts, "freshagent.opencode.daemon_re_warm"); return; // started — the loop ends; the exit watcher arms again for this daemon } @@ -547,7 +555,7 @@ git commit -m "feat(opencode): daemon exit watcher, loss signal, and backoff re- **Interfaces:** - Consumes: Task 3's `subscribe_daemon_signals()` / `DaemonSignal`; `event_frame`/`emit_fresh_agent_error` (opencode_ws.rs:6068/686), `spawn_serve_bridge` (opencode_ws.rs:5971), `FreshAgentState.broadcast_tx` (lib.rs:1917), `set_manager_for_test` (lib.rs:2837), `ensure_manager` (lib.rs:2803 — the real seam; there is no `peek_or_ensure_manager`). -- Produces: +- Produces: - A per-materialized-session typed edge on daemon loss: `freshAgent.event{provider:"opencode", sessionType:"freshopencode", event:{type:"freshAgent.error", code:"OPENCODE_DAEMON_LOST", message:"The opencode serve daemon was lost unexpectedly - it is restarting automatically."}}` — folds client-side through the EXISTING generic `sessionError` path (fresh-agent-ws.ts:514-520), showing the dismissible "Agent error:" banner and clearing busy. - Level-triggered bridge revival: on arming, and again on every `DaemonSignal::Started`, restart bridges that are dead/absent for materialized sessions and push `freshAgent.session.snapshot{status:"idle"}` ONLY to sessions whose bridge was actually restarted (which the client treats as snapshot-invalidating → transcript refetch). No `saw_loss` heuristic — tokio broadcast does not replay history, so revival must not depend on having seen the `Lost` edge (LB-02). - **The fenced attach becomes a real recovery verb (LB-05 redesign):** `handle_attach`'s dead-bridge restart arm (opencode_ws.rs:5460-5473) gains `manager.ensure_started().await` before `spawn_serve_bridge` — a map-hit fenced attach against a daemon-absent manager now respawns the shared daemon and re-bridges, exactly as `resume_durable_session` already does for map-misses. No chime: the recovery must never emit `freshAgent.turn.complete`. @@ -734,16 +742,18 @@ git commit -m "feat(freshopencode): daemon-loss self-heal edge, respawn revival, **Files:** - Modify: `src/components/fresh-agent/FreshAgentView.tsx` (predicate beside `isLostFreshOpencodeThreadError` at ~:455-463; new arm in `handleSnapshotError` at ~:2770-2827; a one-shot guard ref; the pane-refresh attach-decision lane at ~:1718-1761 is the reuse pattern) -- Test: `test/unit/client/components/fresh-agent/FreshAgentView.test.tsx` (beside the 404 test at ~:1887-1930) +- Modify: the runtime-owner store fold (the slice/action behind `src/store/selectors/runtimeOwner.ts` + the WS refusal fold at `src/lib/fresh-agent-ws.ts:224` — reuse the existing "refresh the observed fence from the refusal" action if one exists; otherwise add a minimal reducer action mirroring that fold, e.g. `applyRefusalFence({ sessionId, ownerKind, ownerGeneration })` that advances the record's generation to the refusal's while preserving its epoch) +- Test: `test/unit/client/components/fresh-agent/FreshAgentView.test.tsx` (beside the 404 test at ~:1887-1930; owner-record seeding follows the patterns in `test/unit/client/store/selectors-runtime-owner.test.ts`) **Interfaces:** - Consumes: `ApiError.details` (the full 409 body: `code`, `ownerKind`, `ownerGeneration`), `captureFreshAgentAttachmentAttempt` (the real wrapper at FreshAgentView.tsx — R-1: the pane-refresh reaction pattern at :1718-1761 is the exact reuse: bump `attachDecisionSerialRef`, capture, `sendFencedFreshAgentAttach(attempt)`), `sendFencedFreshAgentAttach` (:1262-1275), `requestSnapshotRefresh` / `requestRevealRefresh`, `selectPaneOwnerFence`. -- Produces: on a 409 `RESTORE_UNAVAILABLE` snapshot error for a freshopencode pane whose refusal names a `fresh-agent` owner (the incident class — the pane's own stale claim; LB-05 scoped terminal owners out to the session-directory handoff door): ONE generation-fenced `freshAgent.attach` followed by a snapshot refetch. Bounded (LB-03): the ENTIRE recovery — attach + refetch — runs once per pane identity (`createRequestId` + `snapshotThreadId`); a second 409 falls through to the existing error surfaces (loadError banner / reveal error), never re-triggering fetches. When the 409 arrived on the reveal lane with `snapshotDirty` set, the recovery drives the reveal-refresh path (`requestRevealRefresh(true)`) so the success-path reveal-dirty clear can run and the "Refreshing conversation" overlay lifts (LB-04). The pane identity is NOT reset (unlike the 404 lost-thread path). +- Produces: on a 409 `RESTORE_UNAVAILABLE` snapshot error for a freshopencode pane whose refusal names a `fresh-agent` owner (the incident class — the pane's own stale claim; LB-05 scoped terminal owners out to the session-directory handoff door): ONE generation-fenced `freshAgent.attach` whose `observedGeneration` is refreshed from the 409's own `ownerGeneration` (plan-review round 2: the fence binds to the refusal, not the possibly-stale owner record — an unfenced or stale attach is refused `FENCE_REQUIRED` and preserves the dead-end), followed by a snapshot refetch. Bounded (LB-03): the ENTIRE recovery — attach + refetch — runs once per pane identity (`createRequestId` + `snapshotThreadId`); a suppressed attach (`sendFencedFreshAgentAttach` returning false) does NOT consume the one-shot guard and does NOT refetch; a second 409 falls through to the existing error surfaces (loadError banner / reveal error), never re-triggering fetches. When the 409 arrived on the reveal lane with `snapshotDirty` set, the recovery drives the reveal-refresh path (`requestRevealRefresh(true)`) so the success-path reveal-dirty clear can run and the "Refreshing conversation" overlay lifts (LB-04). The pane identity is NOT reset (unlike the 404 lost-thread path). - [ ] **Step 1: Write the failing behavioral test** In FreshAgentView.test.tsx (draft — template is the 404 test at :1887-1930; reuse its harness: `apiMock.getFreshAgentThreadSnapshot.mockRejectedValueOnce`, `StoreBackedFreshAgentView`, `sentFreshAgentMessages`): +```ts ```ts // 2026-09-20 incident (log-validated): the daemon died, the reveal GET // answered the typed 409 RESTORE_UNAVAILABLE for the pane's OWN stale @@ -756,7 +766,13 @@ In FreshAgentView.test.tsx (draft — template is the 404 test at :1887-1930; re // Plan-review round 1: DEFER the first rejection until after the baseline is // read — an immediately-rejected mock races the mount fetch (the recovery // attach may land before the test snapshots the count). +// Plan-review round 2: seed the runtime-owner record and make the 409 name a +// NEWER generation — the recovery attach MUST carry the 409's generation +// (fence bound to the refusal), or the wired server refuses it with +// FENCE_REQUIRED and the dead-end persists. Also: reject with a real ApiError +// instance so the banner text is the 409's own message. it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and a refetch', async () => { + seedRuntimeOwnerRecord(store, { sessionId: 'ses_live', ownerKind: 'fresh-agent', epoch: 1, generation: 1 }) // follow the selectors-runtime-owner test fold patterns let rejectFirstSnapshot!: (error: unknown) => void apiMock.getFreshAgentThreadSnapshot .mockImplementationOnce(() => new Promise((_, reject) => { rejectFirstSnapshot = reject })) @@ -765,14 +781,15 @@ it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and await waitFor(() => expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(1)) const attachCountBeforeRecovery = sentFreshAgentMessages('freshAgent.attach').length // the mount attach, settled await act(async () => { - rejectFirstSnapshot({ - status: 409, - message: 'Session ses_live is still running on the server.', - details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 1 }, - }) + rejectFirstSnapshot(new ApiError(409, 'Session ses_live is still running on the server.', { + code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 2, + })) }) await waitFor(() => { expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(attachCountBeforeRecovery + 1) // the RECOVERY attach (LB-09) + const recoveryAttach = sentFreshAgentMessages('freshAgent.attach').at(-1) + expect(recoveryAttach?.observedEpoch).toBe(1) // the record's epoch + expect(recoveryAttach?.observedGeneration).toBe(2) // the 409's CURRENT generation, not the stale record's 1 }) await waitFor(() => { expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(2) // exactly one recovery refetch @@ -784,19 +801,24 @@ it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and }) it('does not loop recovery fetches on repeated 409s', async () => { - apiMock.getFreshAgentThreadSnapshot.mockRejectedValue({ - status: 409, message: 'Session ses_live is still running on the server.', - details: { code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 1 }, - }) - renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) - // Plan-review round 1: AWAIT the banner — findByText returns a promise; - // asserting its truthiness is always true and leaves the baseline unordered. - await screen.findByText(/still running on the server/i) - const baseline = sentFreshAgentMessages('freshAgent.attach').length - // Let any would-be refetch loop run (fake timers or a short flush): - await act(async () => { await vi.advanceTimersByTimeAsync(2_000) }) - expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(baseline) // one recovery total, not per fetch (LB-03) - expect(apiMock.getFreshAgentThreadSnapshot.mock.calls.length).toBeLessThanOrEqual(3) // mount + recovery only — no loop + vi.useFakeTimers() // plan-review round 2: timers must be ENABLED before advancing + try { + apiMock.getFreshAgentThreadSnapshot.mockRejectedValue(new ApiError(409, 'Session ses_live is still running on the server.', { + code: 'RESTORE_UNAVAILABLE', ownerKind: 'fresh-agent', ownerGeneration: 2, + })) + renderFreshAgentPane({ provider: 'opencode', sessionId: 'ses_live', status: 'connected' }) + // Plan-review round 1: AWAIT the banner — findByText returns a promise. + // Plan-review round 2: reject with a real ApiError (an Error instance) so + // handleSnapshotError preserves the 409's message — plain objects render + // 'Failed to load session' instead. + await screen.findByText(/still running on the server/i) + const baseline = sentFreshAgentMessages('freshAgent.attach').length + await act(async () => { await vi.advanceTimersByTimeAsync(2_000) }) + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(baseline) // one recovery total, not per fetch (LB-03) + expect(apiMock.getFreshAgentThreadSnapshot.mock.calls.length).toBeLessThanOrEqual(3) // mount + recovery only — no loop + } finally { + vi.useRealTimers() + } }) ``` @@ -844,24 +866,43 @@ In `handleSnapshotError`, after the opencode lost-404 arm and BEFORE the reveal if (paneContent.provider === 'opencode' && isRestoreUnavailableSnapshotError(error)) { const fresh = paneContentRef.current const recoveryKey = `${fresh.createRequestId}:${sessionId}` + const refusal = (error as ApiError).details as { ownerGeneration: number } if (restoreUnavailableRecoveryRef.current !== recoveryKey) { + const previousRecoveryKey = restoreUnavailableRecoveryRef.current restoreUnavailableRecoveryRef.current = recoveryKey + // Plan-review round 2: bind the fence to the 409's CURRENT generation — + // refresh the observed owner fence from the refusal itself (the same + // fold the WS create.failed lane uses for its ownerKind/ownerGeneration/ + // ownerEpoch fields, fresh-agent-ws.ts:224). Without this, a stale or + // absent owner record sends an unfenced or stale-generation attach the + // wired server refuses with FENCE_REQUIRED — preserving the dead-end. + dispatch(refreshObservedFenceFromRefusal({ + sessionId, ownerKind: 'fresh-agent', ownerGeneration: refusal.ownerGeneration, + })) attachDecisionSerialRef.current += 1 const attempt = captureFreshAgentAttachmentAttempt(fresh) // R-1: the real wrapper's call shape - sendFencedFreshAgentAttach(attempt) - // LB-04: a reveal-lane 409 with snapshotDirty set must refetch through - // the reveal path ('reveal' trigger), or the success-path reveal-dirty - // clear never runs and the pane hides behind the "Refreshing - // conversation" overlay forever. Otherwise refetch via 'manual'. - if (trigger === 'reveal' && snapshotDirtyRef.current) { - revealRefreshStartedAtRef.current = null - setSnapshotRevealError(null) - requestRevealRefresh(true) + const sent = sendFencedFreshAgentAttach(attempt) + if (!sent) { + // Plan-review round 2: the attach was suppressed (lifecycle superseded + // or attempt-key mismatch). Do NOT consume the one-shot recovery and do + // NOT refetch — restore the guard and fall through to the honest error + // surfaces below. + restoreUnavailableRecoveryRef.current = previousRecoveryKey } else { - setLoadError(null) - requestSnapshotRefresh('manual') + // LB-04: a reveal-lane 409 with snapshotDirty set must refetch through + // the reveal path ('reveal' trigger), or the success-path reveal-dirty + // clear never runs and the pane hides behind the "Refreshing + // conversation" overlay forever. Otherwise refetch via 'manual'. + if (trigger === 'reveal' && snapshotDirtyRef.current) { + revealRefreshStartedAtRef.current = null + setSnapshotRevealError(null) + requestRevealRefresh(true) + } else { + setLoadError(null) + requestSnapshotRefresh('manual') + } + return // recovery fired for this error — the honest error surfaces below are for SUBSEQUENT 409s only } - return // recovery fired for this error — the honest error surfaces below are for SUBSEQUENT 409s only } // Recovery already attempted for this identity: do NOT clear errors and do // NOT refetch again — fall through to the reveal error arm / setLoadError @@ -869,7 +910,7 @@ if (paneContent.provider === 'opencode' && isRestoreUnavailableSnapshotError(err } ``` -(Adapt: the ref `restoreUnavailableRecoveryRef = useRef(null)` beside the other reveal refs ~:800-804; `captureFreshAgentAttachmentAttempt`'s real signature follows the pane-refresh reaction lane at :1735-1739. Verify the reveal-refresh request helper's exact name/behavior (`requestRevealRefresh(true)` forces a reveal-tagged refresh) against the state machine at :2601-2624 and the arming sites ~:1233.) +(Adapt: the ref `restoreUnavailableRecoveryRef = useRef(null)` beside the other reveal refs ~:800-804; `captureFreshAgentAttachmentAttempt`'s real signature follows the pane-refresh reaction lane at :1735-1739; `refreshObservedFenceFromRefusal` is the reuse-or-add-mirror of the WS refusal fold at fresh-agent-ws.ts:224 — if the exact action differs in the slice, reuse it; the TEST pins the observable contract: the recovery attach carries `observedGeneration === `. Verify the reveal-refresh request helper's exact name/behavior (`requestRevealRefresh(true)` forces a reveal-tagged refresh) against the state machine at :2601-2624 and the arming sites ~:1233.) - [ ] **Step 4: Run the focused test** @@ -1017,7 +1058,7 @@ No commit (verification only). Record the gate results in the run state; the coo 1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + retrying backoff re-warm + runtime fan-out edge + level-triggered bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 4a+5+6 (the fenced attach made a real recovery verb by respawning the daemon on map-hits, the client 409 arm driving it once, cloud-legal e2e). E2E coverage: Task 6 (cloud-legal client recovery) + Task 7 (local-lane real-daemon self-heal chain) + Rust unit tests (server lanes). Gates: Task 8 runs fmt/clippy/typecheck/lint + the focused suites on the final HEAD; the coordinated full suite runs at the-usual Stage-5 exit. The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config + retry loop; Task 4 `OPENCODE_DAEMON_LOST` edge). 2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; terminal-owner 409s recover via the session-directory handoff door; REST-only never-viewed panes miss the banner until first WS interaction) are stated in Global Constraints, each with its reason and precedent. The real-daemon e2e is local-lane with an honest CLOUD_SKIP_SPECS entry (cloud PR coverage carried by Task 6). -3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger (LB-01 map lock order, LB-02 no-replay → level-triggered revival + arming pass, LB-03 bounded recovery, LB-04 reveal-trigger refetch, LB-05 attach-respawn redesign + fresh-agent-owner scoping, LB-06 Arc process + ownership_id, LB-07 exactly-once take, LB-08 Lagged tolerance, LB-09 attach-count delta, LB-10 state clone, R-1 `captureFreshAgentAttachmentAttempt`, N-3 `ensure_manager` seam) AND plan-review round 1 (routed `get_config(route)`, retrying re-warm with a fail-then-succeed test, single-filter cargo commands, deferred-rejection + awaited-banner client tests, dual-key dedupe, Tasks 7/8). +3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger (LB-01 map lock order, LB-02 no-replay → level-triggered revival + arming pass, LB-03 bounded recovery, LB-04 reveal-trigger refetch, LB-05 attach-respawn redesign + fresh-agent-owner scoping, LB-06 Arc process + ownership_id, LB-07 exactly-once take, LB-08 Lagged tolerance, LB-09 attach-count delta, LB-10 state clone, R-1 `captureFreshAgentAttachmentAttempt`, N-3 `ensure_manager` seam), plan-review round 1 (routed `get_config(route)`, retrying re-warm with a fail-then-succeed test, single-filter cargo commands, deferred-rejection + awaited-banner client tests, dual-key dedupe, Tasks 7/8), AND plan-review round 2 (the re-warm retry test now starts healthy → kills the watched daemon → scripts failing re-warms; the recovery fence binds to the 409's `ownerGeneration` via the refusal-fold, suppressed attaches do not consume the one-shot guard; ApiError instances + fake timers in the client tests; the re-warm attempts counter resets on success; no trailing whitespace). 4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing spawn, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template with delta-based attach counting and an explicit synchronization point). Every cargo invocation uses a single positional TESTNAME filter that matches the named tests. 5. **Placeholder scan:** drafts reference real helpers; where a fake needs a small extension (ExitingProcess, summarize/config hang scripting, fail-twice-then-healthy scripting, the fake-opencode death verb), the extension is named and its model (existing fakes) is cited — no TBDs. 6. **Operational completeness:** new structured event names are logged (Task 3/4) and registered in the logging docs if enumerated; AGENTS.md architecture prose updated (Task 4); no migrations; rollback = revert the commits (no persisted-state changes); verification gates explicit (Task 8 + the Stage-5 full suite). From d07e01610fef35ecb3f2c3a71f1a931148d5357a Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:25:11 -0700 Subject: [PATCH 05/24] docs: apply plan-review round 3 fixes (ownership-gated revival, committed interfaces, self-exit e2e) --- ...26-09-21-opencode-daemon-death-recovery.md | 76 +++++++++++++++---- 1 file changed, 60 insertions(+), 16 deletions(-) diff --git a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md index 283a041ba..fd671e507 100644 --- a/docs/plans/2026-09-21-opencode-daemon-death-recovery.md +++ b/docs/plans/2026-09-21-opencode-daemon-death-recovery.md @@ -301,6 +301,7 @@ git commit -m "feat(opencode): structured log for shared-daemon discards" **Files:** - Modify: `crates/freshell-opencode/src/serve.rs` (Inner at ~:643-655, `RunningServe` at ~:634-641, `ensure_started` at ~:690-770, `discard_running` at ~:1348, `ServeConfig` at ~:557-603, `shutdown` at ~:1510-1517) +- Modify: `crates/freshell-opencode/src/lib.rs` (plan-review round 3: the crate uses an explicit `pub use serve::{...}` re-export list — `DaemonSignal` must be added there or Task 4's root-level import cannot compile) - Test: `crates/freshell-opencode/src/serve.rs` `#[cfg(test)]` + a new integration file `crates/freshell-opencode/tests/serve_daemon_selfheal.rs` (follows `serve_idle_edge.rs` / `serve_health_bounded.rs` conventions) **Interfaces:** @@ -315,7 +316,7 @@ git commit -m "feat(opencode): structured log for shared-daemon discards" **Behavior:** 1. `ensure_started` spawns a daemon exit-watcher task after a successful health check (store its abort handle on `RunningServe` as `_exit_watch`). The watcher polls `process.exited()` every `daemon_watch_interval`; on `Some(exit)` it verifies the running entry is still ITS daemon (compare the captured `ownership_id`), then runs the manager's loss path. 2. The loss path (shared by watcher-exit and, minus the abort, by `discard_running`): WARN `freshagent.opencode.daemon_crash_detected` (watcher arm; fields `reason="process_exit"`, `base_url`) or the Task-2 discard WARN; take the running entry (killing it in the watcher arm is unnecessary — the process already exited; still call `process.kill()` for the /proc ownership reaper parity); `emit_lost_for_all()`; broadcast `DaemonSignal::Lost{reason}`; schedule a backoff-guarded re-warm. **Exactly-once (LB-07):** the take is the race arbiter — if it yields `None` (the other arm already ran), the whole path is a silent no-op: no log, no Lost, no re-warm. -3. Re-warm: a spawned task sleeps `min(re_warm_backoff_initial_ms * 2^(attempts-1), re_warm_backoff_max_ms)`, then calls `ensure_started()` (shutdown-flag checked inside; the task also checks it before sleeping). `attempts` is an `AtomicUsize` on `Inner`, incremented per scheduled re-warm, never reset (a crash-looping daemon retries at the max interval forever — self-heals when e.g. disk frees). Log `tracing::info!(outcome=..., attempt=..., "freshagent.opencode.daemon_re_warm")` on success and `tracing::warn!` on failure. +3. Re-warm: a spawned retry loop sleeps `min(re_warm_backoff_initial_ms * 2^(attempts-1), re_warm_backoff_max_ms)`, then calls `ensure_started()`; a FAILED attempt schedules the next (escalating backoff), a SUCCESSFUL attempt resets `attempts` to 0 and exits the loop. `attempts` is an `AtomicUsize` on `Inner`, incremented per attempt and reset on success — so each new incident starts at the initial delay while a crash-looping daemon still escalates to and retries at the max interval forever (self-heals when e.g. disk frees). Shutdown-flag checked per iteration. Log `tracing::info!(attempt = ..., "freshagent.opencode.daemon_re_warm")` on success and `tracing::warn!(..., error = ...)` on failure. 4. `discard_running` aborts the watcher FIRST (requested kill — no crash event), then the existing kill+lost, then `Lost` signal + re-warm schedule. 5. `shutdown`'s inline duplicate (serve.rs:1510-1517) also aborts the watcher; it must NOT schedule a re-warm (shutdown flag blocks it) and need not signal (server is going down) — keep it minimal: abort watcher + existing behavior. @@ -539,7 +540,7 @@ Expected: PASS - [ ] **Step 7: Commit the task** ```bash -git add crates/freshell-opencode/src/serve.rs crates/freshell-opencode/tests/serve_daemon_selfheal.rs +git add crates/freshell-opencode/src/serve.rs crates/freshell-opencode/src/lib.rs crates/freshell-opencode/tests/serve_daemon_selfheal.rs git commit -m "feat(opencode): daemon exit watcher, loss signal, and backoff re-warm" ``` @@ -614,15 +615,36 @@ async fn map_hit_fenced_attach_respawns_the_daemon_and_rebridges() { let frame = next_fresh_agent_frame(&rx).await; // the attach tail's snapshot push assert_eq!(frame["event"]["type"], "freshAgent.session.snapshot"); } + +// Plan-review round 3: the revival pass must respect the ownership +// coordinator — a session killed/retired or handed to a terminal owner +// between the loss and the respawn must NOT be revived. +#[tokio::test] +async fn revival_skips_sessions_handed_off_or_removed_after_the_loss() { + let (state, rx) = opencode_state_with_bus(); + state.fresh_agent.set_manager_for_test(fake_manager_healthy()); + let _kept = materialized_opencode_session(&state, "ses_keeps").await; + let _gone = materialized_opencode_session(&state, "ses_gone").await; + // The concurrent-handoff shape: while the daemon is down, session B is + // retired from the map and its key transitions (Stopping/handoff → + // terminal owner): + retire_session_from_map(&state, "ses_gone").await; + mark_session_transition_or_terminal_owner(&state, "ses_gone").await; + drive_daemon_respawn(&state).await; // DaemonSignal::Started arrives + assert!(session_serve_bridge_alive(&state, "ses_keeps").await, "healthy fresh-agent sessions revive"); + assert!(!session_serve_bridge_alive(&state, "ses_gone").await, + "removed/transition/terminal-owned sessions must NOT be revived"); + assert_no_snapshot_push_for(&rx, "ses_gone").await; +} ``` (Drafts: adapt to the actual harness helpers — `opencode_state_with_bus`, session materialization via `handle_send` against the seeded fake http, and the manager's running-entry discard via the fake's own seams or `discard_running`. Lock discipline per LB-01: any test helper that walks the sessions map must clone the `Arc` session handles under a short map lock and drop the map guard before locking a session — the map guard is NEVER held across a per-session lock acquisition, per the documented contract at opencode_ws.rs:100-115.) - [ ] **Step 2: Run the test and verify the intended failure** -Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` && `cargo test -p freshell-freshagent map_hit_fenced_attach` (two commands — cargo test accepts ONE positional TESTNAME) +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` && `cargo test -p freshell-freshagent map_hit_fenced_attach` && `cargo test -p freshell-freshagent revival_skips` (three commands — cargo test accepts ONE positional TESTNAME) -Expected: FAIL — `daemon_loss_fans_out...` fails with no `OPENCODE_DAEMON_LOST` frame (today `SessionSignal::Lost` is a no-op at opencode_ws.rs:6018; no listener exists); `map_hit_fenced_attach...` fails because the attach tail never spawns the daemon (spawns == 0, the LB-05-validated gap). +Expected: FAIL — `daemon_loss_fans_out...` fails with no `OPENCODE_DAEMON_LOST` frame (today `SessionSignal::Lost` is a no-op at opencode_ws.rs:6018; no listener exists); `map_hit_fenced_attach...` fails because the attach tail never spawns the daemon (spawns == 0, the LB-05-validated gap); `revival_skips...` fails because no revival machinery exists yet. - [ ] **Step 3: Add the minimal production implementation** @@ -700,20 +722,39 @@ fn ensure_daemon_loss_watcher(&self) { }); } -// The revival pass (also called at arming), respecting LB-01's lock order: +// The revival pass (also called at arming), respecting LB-01's lock order AND +// the ownership coordinator (plan-review round 3: a revival that checks only +// real_session_id + bridge state can race a concurrent handoff that kills the +// session, removes it from the map, and commits a terminal owner — the stale +// Arc would then spawn a fresh-agent bridge broadcasting beside the terminal +// owner. The attach path guards exactly this; revival must too.): // 1. manager.base_url().await is None → return (daemon absent — nothing to // revive into; the next Started signal or attach drives revival). -// 2. Snapshot the map: clone (session_id, Arc>) pairs -// under ONE short map lock, dropping the guard immediately. -// 3. For each pair (OUTSIDE the map guard): lock the session; if -// real_session_id is Some AND the serve-bridge handle is_finished/absent -// → spawn_serve_bridge(...) and broadcast snapshot_event(real_id, "idle"). -// Push the snapshot ONLY to sessions whose bridge was actually restarted. +// 2. Snapshot the map: clone the (session_id, Arc>) +// pairs under ONE short map lock, dropping the guard immediately. +// 3. For each pair (OUTSIDE the map guard), per candidate: +// a. RE-LOOKUP the id in the sessions map at revival time — if the key is +// gone (killed/handoff removed it), skip. Never act on the retained Arc +// alone. +// b. Observe the canonical ownership state fresh (the runtime's +// canonical_ownership_snapshot): skip on ANY transition +// (Handoff/Starting/Stopping/Fenced — something else owns the session +// right now) and skip on Live{Terminal} (a terminal owner holds it). +// Revive only Live{FreshAgent} (this runtime's own sessions). +// c. Arm the SAME adopt-guard machinery handle_attach's restart tail uses +// (ownership_lane::arm_adopt_guard, held across the bridge restart) — +// factor handle_attach's guard-held restart tail into a shared helper +// (e.g. restart_session_bridge_guarded) that BOTH handle_attach and the +// revival pass call, so the coordinator's atomicity rides along instead +// of being re-implemented. +// d. Only then: spawn_serve_bridge(...) and broadcast +// snapshot_event(real_id, "idle") — the snapshot push goes ONLY to +// sessions whose bridge was actually restarted. ``` - [ ] **Step 4: Run the focused test** -Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` && `cargo test -p freshell-freshagent map_hit_fenced_attach` +Run: `cargo test -p freshell-freshagent daemon_loss_fans_out` && `cargo test -p freshell-freshagent map_hit_fenced_attach` && `cargo test -p freshell-freshagent revival_skips` Expected: PASS @@ -933,7 +974,10 @@ Expected: PASS - [ ] **Step 7: Commit the task** ```bash -git add src/components/fresh-agent/FreshAgentView.tsx test/unit/client/components/fresh-agent/FreshAgentView.test.tsx +git add src/components/fresh-agent/FreshAgentView.tsx src/store/freshAgentSlice.ts test/unit/client/components/fresh-agent/FreshAgentView.test.tsx +# Plan-review round 3: stage EVERY file the fold/refusal-fence change lands in +# (freshAgentSlice.ts, the runtimeOwner selector's store module, or wherever +# the reuse-or-mirror action lives — match the actual touched files). git commit -m "fix(fresh-agent): recover freshopencode panes from snapshot 409 via fenced attach and refetch" ``` @@ -1000,8 +1044,8 @@ git commit -m "test(e2e): freshopencode snapshot-409 recovery runs cloud-legal e - Test: itself (local chromium run) **Interfaces:** -- Consumes: the `freshopencode-restart-recovery.spec.ts` harness pattern — `installFakeOpencode` (`fixtures/fake-opencode.cjs` on the spawned server's PATH), the `RustServer` + `TestHarness` helpers, the fake's `FAKE_OPENCODE_AUDIT_LOG` JSONL for spawn/event assertions, and a fixture capability to make the fake daemon process DIE on demand (reuse the model spec's daemon restart/death mechanics if present; otherwise add a minimal scripted-exit verb to the fixture — e.g. an env-armed exit or a served `/__kill` endpoint the spec hits). -- Produces: an e2e proof of the SERVER-side self-heal chain with a real spawned server and a fake daemon process: (1) the pane is materialized and live; (2) the daemon dies an UNREQUESTED death; (3) the pane shows the `OPENCODE_DAEMON_LOST` "Agent error:" banner; (4) the daemon respawns automatically within a bounded wait (audit log shows a second serve spawn); (5) the pane recovers (the idle snapshot push refetches the transcript; the banner is dismissible and no dead-end remains); (6) NO `freshAgent.turn.complete` chime during the window. +- Consumes: the `freshopencode-restart-recovery.spec.ts` harness pattern — `installFakeOpencode` (`fixtures/fake-opencode.cjs` on the spawned server's PATH), the `RustServer` + `TestHarness` helpers, the fake's `FAKE_OPENCODE_AUDIT_LOG` JSONL for spawn/event assertions, and a fixture capability for an UNREQUESTED daemon death that is a scripted SELF-exit — the fake child exits on its own schedule/trigger (plan-review round 3: the spec must not kill any process — `AGENTS.md`'s destructive-test sandbox rule requires process-kill suites to run in `scripts/sandbox-test.sh`; a fixture child exiting itself is the test-sandbox doc's explicitly host-legal fake-child-lifecycle class, the same precedent as the codex onExit self-heal test spawning `true`. E.g. the fixture env-arms an exit-after-N-secs or polls a marker file and exits when it appears — no foreign PID is ever killed. If during implementation the spec cannot avoid killing something, run the spec via `npm run test:sandbox -- "..."` instead of host Playwright). +- Produces: an e2e proof of the SERVER-side self-heal chain with a real spawned server and a fake daemon process: (1) the pane is materialized and live; (2) the daemon dies an UNREQUESTED death (self-exit); (3) the pane shows the `OPENCODE_DAEMON_LOST` "Agent error:" banner; (4) the daemon respawns automatically within a bounded wait (audit log shows a second serve spawn); (5) the pane recovers (the idle snapshot push refetches the transcript; the banner is dismissible and no dead-end remains); (6) NO `freshAgent.turn.complete` chime during the window. - [ ] **Step 1: Write the spec** (verification task; Tasks 3+4 turned the chain green — their Rust unit tests carry the TDD red history for this behavior) @@ -1058,7 +1102,7 @@ No commit (verification only). Record the gate results in the run state; the coo 1. **Spec coverage:** defect 1 → Tasks 1 (compact/config no-kill, FR2 mirror); defect 2 → Task 2 (structured discard log); defect 3 → Tasks 3+4 (exit watcher + loss signal + retrying backoff re-warm + runtime fan-out edge + level-triggered bridge revival — the freshcodex-onExit mirror adapted to shared-daemon topology); defect 4 → Tasks 4a+5+6 (the fenced attach made a real recovery verb by respawning the daemon on map-hits, the client 409 arm driving it once, cloud-legal e2e). E2E coverage: Task 6 (cloud-legal client recovery) + Task 7 (local-lane real-daemon self-heal chain) + Rust unit tests (server lanes). Gates: Task 8 runs fmt/clippy/typecheck/lint + the focused suites on the final HEAD; the coordinated full suite runs at the-usual Stage-5 exit. The "backoff-guarded respawn" and "client-visible status edge" elements of defect 3 are both explicit (Task 3 re-warm config + retry loop; Task 4 `OPENCODE_DAEMON_LOST` edge). 2. **No silent deferrals:** the deliberate residuals (prompt_async and thin wrappers keep `DiscardOnTimeout::Yes`; frozen 409 text; terminal-owner 409s recover via the session-directory handoff door; REST-only never-viewed panes miss the banner until first WS interaction) are stated in Global Constraints, each with its reason and precedent. The real-daemon e2e is local-lane with an honest CLOUD_SKIP_SPECS entry (cloud PR coverage carried by Task 6). -3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger (LB-01 map lock order, LB-02 no-replay → level-triggered revival + arming pass, LB-03 bounded recovery, LB-04 reveal-trigger refetch, LB-05 attach-respawn redesign + fresh-agent-owner scoping, LB-06 Arc process + ownership_id, LB-07 exactly-once take, LB-08 Lagged tolerance, LB-09 attach-count delta, LB-10 state clone, R-1 `captureFreshAgentAttachmentAttempt`, N-3 `ensure_manager` seam), plan-review round 1 (routed `get_config(route)`, retrying re-warm with a fail-then-succeed test, single-filter cargo commands, deferred-rejection + awaited-banner client tests, dual-key dedupe, Tasks 7/8), AND plan-review round 2 (the re-warm retry test now starts healthy → kills the watched daemon → scripts failing re-warms; the recovery fence binds to the 409's `ownerGeneration` via the refusal-fold, suppressed attaches do not consume the one-shot guard; ApiError instances + fake timers in the client tests; the re-warm attempts counter resets on success; no trailing whitespace). +3. **File and interface consistency:** all paths/signatures cross-checked against the six exploration reports at base 855dae72a, then corrected against the load-bearing ledger (LB-01 map lock order, LB-02 no-replay → level-triggered revival + arming pass, LB-03 bounded recovery, LB-04 reveal-trigger refetch, LB-05 attach-respawn redesign + fresh-agent-owner scoping, LB-06 Arc process + ownership_id, LB-07 exactly-once take, LB-08 Lagged tolerance, LB-09 attach-count delta, LB-10 state clone, R-1 `captureFreshAgentAttachmentAttempt`, N-3 `ensure_manager` seam), plan-review round 1 (routed `get_config(route)`, retrying re-warm with a fail-then-succeed test, single-filter cargo commands, deferred-rejection + awaited-banner client tests, dual-key dedupe, Tasks 7/8), AND plan-review round 2 (the re-warm retry test now starts healthy → kills the watched daemon → scripts failing re-warms; the recovery fence binds to the 409's `ownerGeneration` via the refusal-fold, suppressed attaches do not consume the one-shot guard; ApiError instances + fake timers in the client tests; the re-warm attempts counter resets on success; no trailing whitespace), AND plan-review round 3 (the revival pass re-looks-up each id, observes canonical ownership, skips transitions/terminal owners, and rides the same adopt-guard-protected restart helper as handle_attach; `DaemonSignal` re-exported from the crate root via lib.rs; Task 5's commit stages the store fold files; Task 7's daemon death is a scripted self-exit, never a foreign process kill). 4. **Executable tests:** each red test names its exact lane failure (killed counter, missing frame, missing spawn, missing attach) and reuses pinned fake/harness idioms (NeverExitsProcess kill counters, config_capture tracing capture, state_with_bus + set_manager_for_test, the 404 ApiError-mock template with delta-based attach counting and an explicit synchronization point). Every cargo invocation uses a single positional TESTNAME filter that matches the named tests. 5. **Placeholder scan:** drafts reference real helpers; where a fake needs a small extension (ExitingProcess, summarize/config hang scripting, fail-twice-then-healthy scripting, the fake-opencode death verb), the extension is named and its model (existing fakes) is cited — no TBDs. 6. **Operational completeness:** new structured event names are logged (Task 3/4) and registered in the logging docs if enumerated; AGENTS.md architecture prose updated (Task 4); no migrations; rollback = revert the commits (no persisted-state changes); verification gates explicit (Task 8 + the Stage-5 full suite). From 8ba3accc5f0e147b9d7b7a41060c6f0d4f2688b7 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:59:28 -0700 Subject: [PATCH 06/24] fix(opencode): compact and config timeouts never kill the shared serve daemon --- crates/freshell-opencode/src/serve.rs | 188 +++++++++++++++++++++++++- 1 file changed, 183 insertions(+), 5 deletions(-) diff --git a/crates/freshell-opencode/src/serve.rs b/crates/freshell-opencode/src/serve.rs index 9a86e8860..b10628366 100644 --- a/crates/freshell-opencode/src/serve.rs +++ b/crates/freshell-opencode/src/serve.rs @@ -1164,9 +1164,24 @@ impl OpencodeServeManager { /// compact path consumes only its `model` key (probed on 1.18.18: present, /// string-or-null) as the model-pair fallback when a session carries no splittable /// model of its own. + /// + /// A slow config read must never kill the shared daemon — the FR2 read rule + /// (b8ke): capture the base once (spawn-on-demand is preserved), then + /// transport over the captured base with `DiscardOnTimeout::No`. pub async fn get_config(&self, route: &Route) -> Result { + let base = self.require_base().await?; let path = with_route("/config", route); - self.json_request(HttpMethod::Get, &path, None, None).await + self.json_request_over_base( + HttpMethod::Get, + &path, + None, + None, + base, + DiscardOnTimeout::No, + &[], + None, + ) + .await } /// `POST /session/:id/summarize` — the compact RPC. VALIDATED opencode 1.18.18 @@ -1217,11 +1232,22 @@ impl OpencodeServeManager { if let Some(w) = accepted_witness { witnesses.push(w); } - self.json_request_maybe_witnessed( + // 2026-09-20 incident: the summarize POST used the discard-on-timeout + // lane, so a 600 s budget exceeded on a healthy-but-busy daemon KILLED + // the one shared daemon for every freshopencode session. Mirror the + // FR2 captured-base transport (`get_session_at`): a timed-out compact + // answers `RequestTimeout` and NEVER kills the shared daemon. The + // redo-destroy classification is unchanged — `RequestTimeout` stays + // outside `never_dispatched()` (a timed-out POST may have reached the + // daemon). + let base = self.require_base().await?; + self.json_request_over_base( HttpMethod::Post, &path, Some(json!({ "providerID": provider_id, "modelID": model_id })), None, + base, + DiscardOnTimeout::No, &witnesses, // The summarize handler runs the whole LLM turn before answering; // use its dedicated timeout rather than the generic request bound. @@ -1617,7 +1643,7 @@ fn encode_path_segment(segment: &str) -> String { #[cfg(test)] mod tests { use super::*; - use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; // ── display_error_chain (transport diagnostics preservation) ───────────── @@ -1774,17 +1800,21 @@ mod tests { // ── compact (POST /session/:id/summarize) + get_config (GET /config) ──────── /// A `ServeHttp` fake that records every request (`METHOD url body?`) and scripts - /// responses: healthy probes, summarize per `summarize_status`, fork per - /// `fork_status`/`fork_body`, `/config` per `config_body`, everything else a + /// responses: healthy probes, summarize per `summarize_status` (or a NEVER-resolving + /// response when `summarize_pending` — the wedged shape from + /// `tests/serve_health_bounded.rs`), fork per `fork_status`/`fork_body`, `/config` + /// per `config_body` (or never-resolving when `config_pending`), everything else a /// benign 200 `{}`. Per-request timeouts land in the index-aligned /// [`RecordingHttp::timeouts`] vec (`requests[i]`'s timeout is `timeouts[i]`). struct RecordingHttp { requests: Mutex)>>, timeouts: Mutex>>, summarize_status: u16, + summarize_pending: bool, fork_status: u16, fork_body: Vec, config_body: Vec, + config_pending: bool, revert_status: u16, } @@ -1794,9 +1824,11 @@ mod tests { requests: Mutex::new(Vec::new()), timeouts: Mutex::new(Vec::new()), summarize_status: 200, + summarize_pending: false, fork_status: 200, fork_body: br#"{"id":"ses_child","directory":"/tmp/x"}"#.to_vec(), config_body: br#"{"model":null}"#.to_vec(), + config_pending: false, revert_status: 200, } } @@ -1839,6 +1871,14 @@ mod tests { return Box::pin(async { Ok(ServeHttpResponse::new(200, b"{}".to_vec())) }); } if req.url.contains("/summarize") { + if self.summarize_pending { + // A genuine wedge: the response NEVER resolves — only the + // caller's per-request bound can settle it. + return Box::pin(async { + std::future::pending::<()>().await; + unreachable!() + }); + } let status = self.summarize_status; let body = if status == 200 { // VALIDATED 1.18.18 contract: the summarize success body is a boolean. @@ -1863,6 +1903,14 @@ mod tests { return Box::pin(async move { Ok(ServeHttpResponse::new(status, body)) }); } if req.url.contains("/config") { + if self.config_pending { + // A genuine wedge: the response NEVER resolves — only the + // caller's per-request bound can settle it. + return Box::pin(async { + std::future::pending::<()>().await; + unreachable!() + }); + } let body = self.config_body.clone(); return Box::pin(async move { Ok(ServeHttpResponse::new(200, body)) }); } @@ -1898,6 +1946,35 @@ mod tests { } } + /// A never-exiting serve process whose `kill()` calls are COUNTED — the + /// discard-on-timeout assertion seam (the `tests/serve_health_bounded.rs` + /// `NeverExitsProcess` pattern). + struct KillCountingProcess { + killed: Arc, + } + impl ServeProcess for KillCountingProcess { + fn exited(&self) -> Option { + None + } + fn take_fatal_startup_error(&self) -> Option { + None + } + fn kill(&self) { + self.killed.fetch_add(1, Ordering::SeqCst); + } + } + + struct KillCountingSpawner { + killed: Arc, + } + impl ProcessSpawner for KillCountingSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + Ok(Box::new(KillCountingProcess { + killed: self.killed.clone(), + })) + } + } + struct NoopHandle; impl EventStreamHandle for NoopHandle {} struct NoopEventSource; @@ -1928,6 +2005,26 @@ mod tests { mgr } + /// [`started_recording_manager_with_config`] with a kill-counting spawner, + /// for the lanes that must NEVER kill the shared daemon. + async fn started_recording_manager_counting_kills( + http: Arc, + config: ServeConfig, + killed: Arc, + ) -> OpencodeServeManager { + let deps = ServeDeps { + spawner: Arc::new(KillCountingSpawner { killed }), + http, + ports: Arc::new(FakeAllocator), + events: Arc::new(NoopEventSource), + }; + let mgr = OpencodeServeManager::new(deps, config); + mgr.ensure_started() + .await + .expect("healthy fake serve starts"); + mgr + } + // ── b8ke focused round-2 review R2-4: the dispatch-boundary witness ───────── /// A `ServeHttp` fake whose `prompt_async` handler parks the response @@ -2135,6 +2232,53 @@ mod tests { } } + // 2026-09-20 incident: a compact timeout (600 s budget) ran the + // DiscardOnTimeout::Yes arm and KILLED the one shared `opencode serve` + // daemon for every freshopencode session. The compact lane must degrade + // like the FR2 snapshot lane: the POST times out, the daemon survives. + #[tokio::test] + async fn compact_timeout_does_not_kill_the_shared_daemon() { + let killed = Arc::new(AtomicUsize::new(0)); + let http = Arc::new(RecordingHttp { + summarize_pending: true, + ..RecordingHttp::new() + }); + let config = ServeConfig { + compact_timeout: Duration::from_millis(50), + ..ServeConfig::default() + }; + let mgr = + started_recording_manager_counting_kills(http.clone(), config, killed.clone()).await; + + let err = mgr + .compact("ses_timeout", "prov-a", "mdl-x", &None, None, None) + .await + .expect_err("the summarize POST must time out"); + assert!( + matches!(err, ServeError::RequestTimeout { .. }), + "got {err:?}" + ); + assert_eq!( + killed.load(Ordering::SeqCst), + 0, + "a compact timeout must NEVER kill the shared daemon" + ); + assert!( + mgr.base_url().await.is_some(), + "the running entry must survive a compact timeout" + ); + // The compact-timeout POST must still carry the dedicated budget. + let requests = http.recorded(); + let summarize_index = requests + .iter() + .position(|(method, url, _)| method == "POST" && url.contains("/summarize")) + .expect("a summarize POST was recorded"); + assert_eq!( + http.recorded_timeout(summarize_index), + Some(Duration::from_millis(50)) + ); + } + #[tokio::test] async fn get_config_returns_the_raw_config_body() { let http = Arc::new(RecordingHttp { @@ -2155,6 +2299,40 @@ mod tests { assert!(body.is_none(), "GET /config carries no body"); } + // The compact drive's pre-flight model-pair resolution reads /config; a slow + // config GET is the same defect class (a read must never kill the daemon). + #[tokio::test] + async fn get_config_timeout_does_not_kill_the_shared_daemon() { + let killed = Arc::new(AtomicUsize::new(0)); + let http = Arc::new(RecordingHttp { + config_pending: true, + ..RecordingHttp::new() + }); + let config = ServeConfig { + request_timeout: Duration::from_millis(50), + ..ServeConfig::default() + }; + let mgr = started_recording_manager_counting_kills(http, config, killed.clone()).await; + + let err = mgr + .get_config(&None) + .await + .expect_err("config GET must time out"); + assert!( + matches!(err, ServeError::RequestTimeout { .. }), + "got {err:?}" + ); + assert_eq!( + killed.load(Ordering::SeqCst), + 0, + "a config read timeout must NEVER kill the shared daemon" + ); + assert!( + mgr.base_url().await.is_some(), + "the running entry must survive a config read timeout" + ); + } + // ── fork (POST /session/:id/fork) ──────────────────────────────────────── /// The recorded `POST /session/:id/fork` request, if any. From 4fed3f255f0028028fc7ead4d4cb49707e4001d2 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:28:46 -0700 Subject: [PATCH 07/24] feat(opencode): structured log for shared-daemon discards --- crates/freshell-opencode/src/serve.rs | 71 ++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/crates/freshell-opencode/src/serve.rs b/crates/freshell-opencode/src/serve.rs index b10628366..fa8bb79d3 100644 --- a/crates/freshell-opencode/src/serve.rs +++ b/crates/freshell-opencode/src/serve.rs @@ -1371,9 +1371,10 @@ impl OpencodeServeManager { } } - async fn discard_running(&self, _reason: &str) { + async fn discard_running(&self, reason: &str) { let taken = self.inner.running.lock().await.take(); if let Some(running) = taken { + tracing::warn!(reason = reason, "freshagent.opencode.daemon_discarded"); running.process.kill(); } self.emit_lost_for_all(); @@ -1803,7 +1804,8 @@ mod tests { /// responses: healthy probes, summarize per `summarize_status` (or a NEVER-resolving /// response when `summarize_pending` — the wedged shape from /// `tests/serve_health_bounded.rs`), fork per `fork_status`/`fork_body`, `/config` - /// per `config_body` (or never-resolving when `config_pending`), everything else a + /// per `config_body` (or never-resolving when `config_pending`), `/prompt_async` + /// never-resolving when `prompt_pending`, everything else a /// benign 200 `{}`. Per-request timeouts land in the index-aligned /// [`RecordingHttp::timeouts`] vec (`requests[i]`'s timeout is `timeouts[i]`). struct RecordingHttp { @@ -1816,6 +1818,7 @@ mod tests { config_body: Vec, config_pending: bool, revert_status: u16, + prompt_pending: bool, } impl RecordingHttp { @@ -1830,6 +1833,7 @@ mod tests { config_body: br#"{"model":null}"#.to_vec(), config_pending: false, revert_status: 200, + prompt_pending: false, } } @@ -1914,6 +1918,14 @@ mod tests { let body = self.config_body.clone(); return Box::pin(async move { Ok(ServeHttpResponse::new(200, body)) }); } + if req.url.contains("/prompt_async") && self.prompt_pending { + // A genuine wedge: the response NEVER resolves — only the + // caller's per-request bound can settle it. + return Box::pin(async { + std::future::pending::<()>().await; + unreachable!() + }); + } Box::pin(async move { Ok(ServeHttpResponse::new(200, b"{}".to_vec())) }) } } @@ -2932,6 +2944,61 @@ mod tests { ); } + /// 2026-09-20 incident: the daemon discard that killed the shared serve left + /// ZERO log trace (its reason parameter went unused), so the shared-daemon + /// death was undiagnosable from the structured JSONL log. The discard must + /// be observable: a WARN `freshagent.opencode.daemon_discarded` naming its + /// reason. Driven through `prompt_async` — a deliberate + /// `DiscardOnTimeout::Yes` lane — so a pending prompt POST times out and + /// takes the discard path. + #[tokio::test] + async fn discard_running_emits_a_structured_warn_with_its_reason() { + let killed = Arc::new(AtomicUsize::new(0)); + let http = Arc::new(RecordingHttp { + prompt_pending: true, + ..RecordingHttp::new() + }); + let config = ServeConfig { + request_timeout: Duration::from_millis(50), + ..ServeConfig::default() + }; + let (events, _guard) = config_capture::capture(); + let mgr = started_recording_manager_counting_kills(http, config, killed.clone()).await; + + let err = mgr + .prompt_async( + "ses_discard", + build_prompt_body("hi", None, None), + &None, + None, + ) + .await + .expect_err("the prompt POST must time out"); + assert!( + matches!(err, ServeError::RequestTimeout { .. }), + "got {err:?}" + ); + // The discard itself ran: the Yes-lane timeout took the daemon down. + assert_eq!( + killed.load(Ordering::SeqCst), + 1, + "the discard must actually kill the running daemon here" + ); + let events = events.lock().expect("capture lock"); + let discard = events + .iter() + .find(|fields| { + fields.get("message").map(String::as_str) + == Some("freshagent.opencode.daemon_discarded") + }) + .expect("a daemon discard must emit freshagent.opencode.daemon_discarded"); + assert_eq!( + discard.get("reason").map(String::as_str), + Some("request_timeout"), + "the discard warn carries its reason: {discard:?}" + ); + } + /// Spawn-level: a config-supplied inline document is MERGED into the launch /// (sibling keys survive, snapshot pinned), never replaced — and the spawn env /// carries EXACTLY ONE occurrence (the merged value). From 900742d1a059d9cd53b3f96a60f6d85e54058d0b Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:13:33 -0700 Subject: [PATCH 08/24] feat(opencode): daemon exit watcher, loss signal, and backoff re-warm --- crates/freshell-opencode/src/lib.rs | 4 +- crates/freshell-opencode/src/serve.rs | 410 ++++++++++++++- .../tests/serve_daemon_selfheal.rs | 496 ++++++++++++++++++ 3 files changed, 896 insertions(+), 14 deletions(-) create mode 100644 crates/freshell-opencode/tests/serve_daemon_selfheal.rs diff --git a/crates/freshell-opencode/src/lib.rs b/crates/freshell-opencode/src/lib.rs index 0145dc8e9..0de7946a4 100644 --- a/crates/freshell-opencode/src/lib.rs +++ b/crates/freshell-opencode/src/lib.rs @@ -50,8 +50,8 @@ pub use model::{ FRESHOPENCODE_DEFAULT_EFFORT, }; pub use serve::{ - build_prompt_body, display_error_chain, is_healthy_response, CreatedSession, Endpoint, - EventSource, EventStreamHandle, ForkedSession, OpencodeServeManager, PortAllocator, + build_prompt_body, display_error_chain, is_healthy_response, CreatedSession, DaemonSignal, + Endpoint, EventSource, EventStreamHandle, ForkedSession, OpencodeServeManager, PortAllocator, ProcessSpawner, Route, ServeConfig, ServeDeps, ServeError, ServeHttp, ServeHttpError, ServeHttpRequest, ServeHttpResponse, ServeProcess, SessionSignal, SpawnRequest, OPENCODE_SIDECAR_OWNERSHIP_ENV, diff --git a/crates/freshell-opencode/src/serve.rs b/crates/freshell-opencode/src/serve.rs index fa8bb79d3..6806ba293 100644 --- a/crates/freshell-opencode/src/serve.rs +++ b/crates/freshell-opencode/src/serve.rs @@ -23,7 +23,7 @@ use std::collections::HashMap; use std::future::Future; use std::pin::Pin; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; @@ -581,6 +581,17 @@ pub struct ServeConfig { /// 600 s opencode turn budget that also bounds the compact's await-idle /// tail (`opencode_ws.rs`'s `DEFAULT_TURN_TIMEOUT`). pub compact_timeout: Duration, + /// Daemon exit-watcher poll cadence (`Task 3`): how often the running + /// daemon's `exited()` is consulted between request traffic. + pub daemon_watch_interval: Duration, + /// Re-warm backoff: the initial delay before the first respawn attempt + /// after a daemon loss, doubling per failed attempt, capped at + /// [`ServeConfig::re_warm_backoff_max_ms`]. + pub re_warm_backoff_initial_ms: u64, + /// Re-warm backoff ceiling: the escalation stops here (a crash-looping + /// daemon retries at this interval forever — it self-heals when e.g. + /// disk frees). + pub re_warm_backoff_max_ms: u64, } impl Default for ServeConfig { @@ -598,6 +609,9 @@ impl Default for ServeConfig { required_idle_status_polls: 2, request_timeout: Duration::from_millis(30_000), compact_timeout: Duration::from_millis(600_000), + daemon_watch_interval: Duration::from_millis(1_000), + re_warm_backoff_initial_ms: 2_000, + re_warm_backoff_max_ms: 60_000, } } } @@ -640,10 +654,54 @@ pub enum SessionSignal { const SESSION_CHANNEL_CAPACITY: usize = 256; +/// The daemon-level channel capacity for [`DaemonSignal`] broadcasts. +const DAEMON_CHANNEL_CAPACITY: usize = 16; + +/// A daemon-lifecycle edge broadcast by the manager (the client-facing +/// runtime's Task-4 revival design consumes this): `Lost` when the shared +/// daemon is gone (a requested discard with its reason, or an unrequested +/// process exit), `Started` on every successful COLD start (not on the +/// fast-path return of an already-running daemon). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum DaemonSignal { + /// The running daemon is gone. `reason` is the loss class: `"process_exit"` + /// for an unrequested exit, the discard's reason (e.g. `"request_timeout"`) + /// for a requested kill. + Lost { reason: &'static str }, + /// A previously-lost (or never-started) daemon completed a cold start and + /// is healthy again. + Started, +} + +/// Which loss arm is running the shared exactly-once path +/// ([`OpencodeServeManager::lose_daemon`]). +enum LossArm<'a> { + /// The exit watcher observed an unrequested process exit. `ownership_id` + /// gates staleness (a watcher for a superseded daemon must not run the + /// loss path on its successor); `base_url` rides the crash WARN. + Watcher { + base_url: &'a str, + ownership_id: &'a str, + }, + /// A requested discard (kill). The watcher is aborted first (a requested + /// kill never raises the crash event); `reason` names the discard cause + /// and rides both the WARN and the `Lost` signal. + Discard { reason: &'static str }, +} + struct RunningServe { base_url: String, - process: Box, + /// The shared daemon handle. `Arc` (LB-06): the exit watcher keeps a clone + /// that outlives this entry — the watcher polls ITS Arc, the entry keeps + /// its own, nothing is moved out. + process: Arc, + /// THIS daemon's spawn identity: the stale-watcher gate (a watcher for a + /// superseded daemon must not run the loss path on its successor). + ownership_id: String, _event_handle: Box, + /// The exit watcher's abort handle — aborted on the requested-loss paths + /// (discard/shutdown) so a killed daemon never raises the crash event. + _exit_watch: Option, } struct Inner { @@ -652,6 +710,16 @@ struct Inner { shutdown: AtomicBool, running: tokio::sync::Mutex>>, session_emitters: Mutex>>, + daemon_signals: broadcast::Sender, + /// The re-warm backoff's attempt counter: incremented per re-warm attempt + /// and reset when a loss is a FRESH incident (see + /// [`OpencodeServeManager::schedule_re_warm`]) — each new incident starts + /// at the initial delay while a crash-looping daemon still escalates to + /// and retries at the capped interval. + re_warm_attempts: AtomicUsize, + /// When the running daemon completed its (healthy) cold start — the + /// fresh-incident clock for the re-warm backoff. + last_cold_start_at: Mutex>, } /// The opencode serve sidecar client. Cheap to clone (`Arc`-backed). @@ -669,6 +737,9 @@ impl OpencodeServeManager { shutdown: AtomicBool::new(false), running: tokio::sync::Mutex::new(None), session_emitters: Mutex::new(HashMap::new()), + daemon_signals: broadcast::Sender::new(DAEMON_CHANNEL_CAPACITY), + re_warm_attempts: AtomicUsize::new(0), + last_cold_start_at: Mutex::new(None), }), } } @@ -734,7 +805,7 @@ impl OpencodeServeManager { OPENCODE_CONFIG_CONTENT_ENV.to_string(), merged_opencode_config_content(inherited.as_deref()), )); - let process = self + let process: Arc = self .inner .deps .spawner @@ -742,18 +813,26 @@ impl OpencodeServeManager { command: self.config().command.clone(), hostname: endpoint.hostname.clone(), port: endpoint.port, - ownership_id, + ownership_id: ownership_id.clone(), env, pure: false, cwd: None, }) - .map_err(ServeError::Spawn)?; + .map_err(ServeError::Spawn)? + .into(); if let Err(e) = self.wait_for_health(&base_url, process.as_ref()).await { process.kill(); return Err(e); } + // Arm the exit watcher BEFORE storing the entry — the spawn is + // synchronous (the guard is never held across an await here) and the + // watcher's first action is a sleep, so by the time it first consults + // `exited()` the entry is stored; its loss path still re-verifies + // ownership, so a store-visibility race can only no-op, never mis-fire. + let watch = + self.spawn_exit_watch(base_url.clone(), Arc::clone(&process), ownership_id.clone()); let sink = self.make_dispatch_sink(); let handle = self .inner @@ -764,8 +843,20 @@ impl OpencodeServeManager { *guard = Some(Arc::new(RunningServe { base_url: base_url.clone(), process, + ownership_id, _event_handle: handle, + _exit_watch: Some(watch), })); + // The fresh-incident clock for the re-warm backoff: this daemon's + // healthy-service lifetime starts now. + *self + .inner + .last_cold_start_at + .lock() + .expect("cold-start clock mutex") = Some(Instant::now()); + // COLD-start edge only — the fast path above (already running) never + // re-broadcasts this. + let _ = self.inner.daemon_signals.send(DaemonSignal::Started); Ok(base_url) } @@ -842,6 +933,176 @@ impl OpencodeServeManager { }) } + /// Spawn the daemon exit watcher for one cold-started daemon (Task 3, + /// the shared-daemon adaptation of the freshcodex onExit self-heal): poll + /// the SHARED process Arc's `exited()` every `daemon_watch_interval`; on + /// `Some` run the staleness-gated loss path and end. The manager clone is + /// cheap (`Arc`-backed); the process Arc and ownership id move in with + /// the task — the running entry keeps its own Arc (LB-06: share, never + /// move the daemon out of the entry). + fn spawn_exit_watch( + &self, + base_url: String, + process: Arc, + ownership_id: String, + ) -> tokio::task::AbortHandle { + let manager = self.clone(); + let interval = self.config().daemon_watch_interval; + let handle = tokio::spawn(async move { + loop { + tokio::time::sleep(interval).await; + if process.exited().is_some() { + manager + .lose_daemon(LossArm::Watcher { + base_url: &base_url, + ownership_id: &ownership_id, + }) + .await; + return; + } + } + }); + handle.abort_handle() + } + + /// The shared exactly-once daemon-loss path: a daemon that died on its own + /// (the watcher arm) or was discarded (the requested-kill arm) must not + /// leave a poisoned running entry (the 2026-09-20 incident's silent + /// half). **Exactly-once (LB-07):** the running-entry take is the race + /// arbiter — a `None` take means the other arm already handled this loss, + /// and the whole path is a silent no-op: no log, no Lost, no re-warm. The + /// arm selects the pre-take gate and the structured WARN (a requested kill + /// never raises the crash event; a watcher's WARN names the dead daemon). + async fn lose_daemon(&self, arm: LossArm<'_>) { + let taken = { + let mut running = self.inner.running.lock().await; + match arm { + LossArm::Watcher { + base_url: _, + ownership_id, + } => match running.as_ref() { + // Still OUR daemon: take it (the loss is ours to handle). + Some(r) if r.ownership_id == ownership_id => running.take(), + // Stale watcher — a newer daemon owns the entry: no-op. + _ => return, + }, + LossArm::Discard { reason: _ } => { + // Abort the watcher FIRST (inside the lock, before the + // take and the WARN/kill sequence) — the requested kill + // must never raise the crash event. After the take the + // watcher can never win its own take; if it already won, + // our take below is the silent no-op. + if let Some(r) = running.as_ref() { + if let Some(watch) = &r._exit_watch { + watch.abort(); + } + } + running.take() + } + } + }; + let Some(running) = taken else { + return; + }; + let reason = match arm { + LossArm::Watcher { base_url, .. } => { + tracing::warn!( + reason = "process_exit", + base_url = %base_url, + "freshagent.opencode.daemon_crash_detected" + ); + "process_exit" + } + LossArm::Discard { reason } => { + tracing::warn!(reason = reason, "freshagent.opencode.daemon_discarded"); + reason + } + }; + // The watcher arm's kill is reaper parity only (the process already + // exited); the discard arm's kill is the requested kill. Either way + // kill() reaps the /proc-scoped ownership tree. + running.process.kill(); + self.emit_lost_for_all(); + let _ = self + .inner + .daemon_signals + .send(DaemonSignal::Lost { reason }); + self.schedule_re_warm(); + } + + /// Schedule the backoff-guarded respawn after a daemon loss: a RETRY loop + /// that sleeps `re_warm_backoff_initial_ms * 2^(attempts-1)` (capped at + /// `re_warm_backoff_max_ms`), then calls `ensure_started`. A FAILED + /// attempt logs and schedules the next (escalating) attempt — a transient + /// spawn/health failure (e.g. disk pressure) must not strand the daemon + /// permanently absent. A SUCCESSFUL attempt ends the loop; the fresh + /// daemon's own exit watcher is armed by `ensure_started`. Never spawns + /// (nor retries) once shutdown is set. + /// + /// **Fresh-incident gate** (the round-2 "no permanent 60 s first delay" + /// finding, reconciled with the behavior list's crash-loop escalation): + /// a daemon that OUTLIVED the whole backoff ladder makes this loss a NEW + /// incident — the attempt counter resets so its re-warm starts at the + /// initial delay. A daemon that died faster KEEPS the accumulated + /// escalation: a daemon dying immediately after every successful start + /// must climb the ladder (50→100→200→400 ms…), never respawn at the + /// floor every cycle. The counter therefore persists across re-warm + /// successes and resets only here, at the next loss, when the lost + /// daemon's healthy lifetime reached the ladder's cap. + fn schedule_re_warm(&self) { + if self.inner.shutdown.load(Ordering::SeqCst) { + return; + } + let fresh_incident = { + let last_cold_start = *self + .inner + .last_cold_start_at + .lock() + .expect("cold-start clock mutex"); + last_cold_start + .map(|started_at| { + started_at.elapsed() + >= Duration::from_millis(self.config().re_warm_backoff_max_ms) + }) + .unwrap_or(true) + }; + if fresh_incident { + self.inner.re_warm_attempts.store(0, Ordering::SeqCst); + } + let manager = self.clone(); + tokio::spawn(async move { + loop { + let attempts = manager + .inner + .re_warm_attempts + .fetch_add(1, Ordering::SeqCst) + + 1; + let delay_ms = (manager + .config() + .re_warm_backoff_initial_ms + .saturating_mul(1u64 << (attempts - 1).min(16))) + .min(manager.config().re_warm_backoff_max_ms); + tokio::time::sleep(Duration::from_millis(delay_ms)).await; + if manager.inner.shutdown.load(Ordering::SeqCst) { + return; + } + match manager.ensure_started().await { + Ok(_) => { + tracing::info!(attempt = attempts, "freshagent.opencode.daemon_re_warm"); + return; + } + Err(err) => { + tracing::warn!( + attempt = attempts, + error = %err, + "freshagent.opencode.daemon_re_warm" + ); + } + } + } + }); + } + async fn require_base(&self) -> Result { self.ensure_started().await } @@ -1347,6 +1608,18 @@ impl OpencodeServeManager { self.emitter_for(session_id).subscribe() } + /// Subscribe to the daemon-level lifecycle stream ([`DaemonSignal`]). + /// + /// NOTE (LB-02a, source-verified at the locked tokio version): tokio + /// broadcast does NOT replay history to late subscribers — a receiver + /// created here starts at the channel's current TAIL and observes only + /// signals sent AFTER this call. Consumers (Task 4's bridge revival) must + /// therefore be LEVEL-TRIGGERED (query daemon state on receipt), never + /// event-history-dependent. + pub fn subscribe_daemon_signals(&self) -> broadcast::Receiver { + self.inner.daemon_signals.subscribe() + } + /// Feed one parsed SSE event into the per-session fan-out. This is the ingestion /// point the [`EventSource`] sink calls (`dispatchEvent`, `serve-manager.ts:429-432`). pub fn dispatch_event(&self, event: ParsedServeEvent) { @@ -1371,13 +1644,13 @@ impl OpencodeServeManager { } } - async fn discard_running(&self, reason: &str) { - let taken = self.inner.running.lock().await.take(); - if let Some(running) = taken { - tracing::warn!(reason = reason, "freshagent.opencode.daemon_discarded"); - running.process.kill(); - } - self.emit_lost_for_all(); + /// The requested-loss arm: discard the running daemon (a Yes-lane request + /// timeout, a shutdown-adjacent lane, …), WARN the Task-2 structured + /// event, then run the shared exactly-once loss path (Lost signal + + /// backoff re-warm). `reason` is `&'static` because it rides the + /// [`DaemonSignal::Lost`] broadcast to daemon-signal subscribers. + async fn discard_running(&self, reason: &'static str) { + self.lose_daemon(LossArm::Discard { reason }).await; } // ── the IDLE edge (once_idle / await_idle, serve-manager.ts:440-520) ───────── @@ -1538,6 +1811,13 @@ impl OpencodeServeManager { self.inner.shutdown.store(true, Ordering::SeqCst); let taken = self.inner.running.lock().await.take(); if let Some(running) = taken { + // The requested-loss discipline: abort the watcher so the shutdown + // kill never raises the crash event. No `Lost` signal, no re-warm — + // the shutdown flag set above blocks `schedule_re_warm`, and the + // server is going down. + if let Some(watch) = &running._exit_watch { + watch.abort(); + } running.process.kill(); } self.emit_lost_for_all(); @@ -2999,6 +3279,112 @@ mod tests { ); } + // ── Task 3: the daemon exit watcher's loss path (unit side) ────────────── + + /// A serve whose "exit" is test-controlled: `exited()` reports `Some(0)` + /// once the shared flag is set (the `tests/serve_daemon_selfheal.rs` + /// `FlagExitProcess` shape, unit-side). + struct FlagExitProcess { + exited: Arc, + killed: Arc, + } + impl ServeProcess for FlagExitProcess { + fn exited(&self) -> Option { + self.exited.load(Ordering::SeqCst).then_some(0) + } + fn take_fatal_startup_error(&self) -> Option { + None + } + fn kill(&self) { + self.killed.fetch_add(1, Ordering::SeqCst); + } + } + + struct FlagExitSpawner { + exited: Arc, + killed: Arc, + } + impl ProcessSpawner for FlagExitSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + Ok(Box::new(FlagExitProcess { + exited: self.exited.clone(), + killed: self.killed.clone(), + })) + } + } + + /// The watcher's unrequested-exit arm must WARN + /// `freshagent.opencode.daemon_crash_detected` with the loss reason and + /// the dead daemon's base URL — the diagnosability complement of the + /// Task-2 discard log (a silent shared-daemon death was the incident's + /// undiagnosable half). The watcher task runs on this current-thread + /// runtime, so the thread-local capture sees its WARN; awaiting the + /// `DaemonSignal::Lost` edge first guarantees the loss path already ran. + #[tokio::test] + async fn unrequested_daemon_exit_warns_daemon_crash_detected_with_reason_and_base_url() { + let exited = Arc::new(AtomicBool::new(false)); + let killed = Arc::new(AtomicUsize::new(0)); + let deps = ServeDeps { + spawner: Arc::new(FlagExitSpawner { + exited: exited.clone(), + killed: killed.clone(), + }), + http: Arc::new(RecordingHttp::new()), + ports: Arc::new(FakeAllocator), + events: Arc::new(NoopEventSource), + }; + let config = ServeConfig { + daemon_watch_interval: Duration::from_millis(5), + re_warm_backoff_initial_ms: 5, + re_warm_backoff_max_ms: 50, + ..ServeConfig::default() + }; + let mgr = OpencodeServeManager::new(deps, config); + mgr.ensure_started() + .await + .expect("healthy fake serve starts"); + let mut signals = mgr.subscribe_daemon_signals(); + let (events, _guard) = config_capture::capture(); + + exited.store(true, Ordering::SeqCst); // the daemon "exits" + let signal = tokio::time::timeout(Duration::from_secs(2), signals.recv()) + .await + .expect("loss signal within budget") + .expect("channel alive"); + assert!( + matches!( + signal, + DaemonSignal::Lost { + reason: "process_exit" + } + ), + "got {signal:?}" + ); + assert!( + killed.load(Ordering::SeqCst) >= 1, + "the already-exited daemon is still kill()ed for /proc-reaper parity" + ); + + let events = events.lock().expect("capture lock"); + let warn = events + .iter() + .find(|fields| { + fields.get("message").map(String::as_str) + == Some("freshagent.opencode.daemon_crash_detected") + }) + .expect("an unrequested daemon exit must WARN daemon_crash_detected"); + assert_eq!( + warn.get("reason").map(String::as_str), + Some("process_exit"), + "the crash warn names the loss reason: {warn:?}" + ); + assert_eq!( + warn.get("base_url").map(String::as_str), + Some("http://127.0.0.1:1"), + "the crash warn names the dead daemon's base URL: {warn:?}" + ); + } + /// Spawn-level: a config-supplied inline document is MERGED into the launch /// (sibling keys survive, snapshot pinned), never replaced — and the spawn env /// carries EXACTLY ONE occurrence (the merged value). diff --git a/crates/freshell-opencode/tests/serve_daemon_selfheal.rs b/crates/freshell-opencode/tests/serve_daemon_selfheal.rs new file mode 100644 index 000000000..52c6defa7 --- /dev/null +++ b/crates/freshell-opencode/tests/serve_daemon_selfheal.rs @@ -0,0 +1,496 @@ +//! Manager-level daemon-loss self-heal (the 2026-09-20 incident's silent half): +//! a shared `opencode serve` daemon that dies on its own must not leave a +//! poisoned running entry forever. +//! +//! Pins the Task 3 machinery end-to-end through fully-faked IO (NO real serve, +//! NO live API calls — the `serve_health_bounded.rs` / `serve_idle_edge.rs` +//! conventions): +//! * the daemon exit watcher: an unrequested process exit clears the running +//! entry, emits `SessionSignal::Lost` for in-flight turns, broadcasts +//! `DaemonSignal::Lost { reason: "process_exit" }`, and schedules a re-warm; +//! * the re-warm retry loop: a FAILED re-warm attempt retries (never strands +//! the daemon absent), and the automatic respawn BACKS OFF exponentially +//! (no spawn storm); +//! * the requested-discard arm: a Yes-lane timeout discard also signals +//! `DaemonSignal::Lost` (its reason) and schedules the same backoff-guarded +//! respawn — the runtime self-heal (Task 4) observes BOTH loss classes. +//! +//! The crash-detected WARN (`freshagent.opencode.daemon_crash_detected`) is +//! pinned unit-side in `serve.rs` (the `config_capture` idiom), where the +//! thread-local tracing capture hosts it. + +use std::sync::atomic::{AtomicBool, AtomicU16, AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::Duration; + +use freshell_opencode::serve::{ + build_prompt_body, DaemonSignal, Endpoint, EventSink, EventSource, EventStreamHandle, + OpencodeServeManager, PortAllocator, ProcessSpawner, ServeConfig, ServeDeps, ServeError, + ServeHttp, ServeHttpError, ServeHttpRequest, ServeHttpResponse, ServeProcess, SessionSignal, + SpawnRequest, +}; + +// ── injected fakes ─────────────────────────────────────────────────────────────── + +/// `/global/health` answers healthy; `/prompt_async` NEVER resolves when +/// `prompt_pending` (the Yes-lane timeout discard driver); everything else a +/// benign 200 `{}`. +struct HealthyHttp { + prompt_pending: bool, +} +impl ServeHttp for HealthyHttp { + fn request<'a>( + &'a self, + req: ServeHttpRequest, + ) -> std::pin::Pin< + Box< + dyn std::future::Future> + Send + 'a, + >, + > { + if req.url.contains("/prompt_async") && self.prompt_pending { + // A genuine wedge: the response NEVER resolves — only the caller's + // per-request bound can settle it (the discard driver). + return Box::pin(async { + std::future::pending::<()>().await; + unreachable!() + }); + } + Box::pin(async { Ok(ServeHttpResponse::new(200, b"{}".to_vec())) }) + } +} + +/// Per-generation health scripting: probes to a port in `fail_ports` NEVER +/// resolve (the wedged shape — that generation's bounded health wait fails as +/// `NotHealthy`); every other request is healthy/benign. Ports come from +/// [`CountingAllocator`], so port number == spawn generation. +struct GenerationHealthHttp { + fail_ports: Vec, +} +impl ServeHttp for GenerationHealthHttp { + fn request<'a>( + &'a self, + req: ServeHttpRequest, + ) -> std::pin::Pin< + Box< + dyn std::future::Future> + Send + 'a, + >, + > { + let wedged = req.url.contains("/global/health") + && self + .fail_ports + .iter() + .any(|port| req.url.contains(&format!(":{port}/"))); + if wedged { + return Box::pin(async { + std::future::pending::<()>().await; + unreachable!() + }); + } + Box::pin(async { Ok(ServeHttpResponse::new(200, b"{}".to_vec())) }) + } +} + +/// Hand out ports 1, 2, 3, … — one per cold start, so the spawn generation is +/// addressable in the URL (see [`GenerationHealthHttp`]). +struct CountingAllocator { + next: AtomicU16, +} +impl PortAllocator for CountingAllocator { + fn allocate(&self) -> Result { + let port = self.next.fetch_add(1, Ordering::SeqCst) + 1; + Ok(Endpoint { + hostname: "127.0.0.1".into(), + port, + }) + } +} + +/// A serve that never exits; `kill()` counts. +struct NeverExitsProcess { + killed: Arc, +} +impl ServeProcess for NeverExitsProcess { + fn exited(&self) -> Option { + None + } + fn take_fatal_startup_error(&self) -> Option { + None + } + fn kill(&self) { + self.killed.fetch_add(1, Ordering::SeqCst); + } +} + +/// A serve whose "exit" is test-controlled: `exited()` reports `Some(0)` once +/// the shared flag is set (and stays Some — the dead daemon stays dead). +struct FlagExitProcess { + exited: Arc, + killed: Arc, +} +impl ServeProcess for FlagExitProcess { + fn exited(&self) -> Option { + self.exited.load(Ordering::SeqCst).then_some(0) + } + fn take_fatal_startup_error(&self) -> Option { + None + } + fn kill(&self) { + self.killed.fetch_add(1, Ordering::SeqCst); + } +} + +/// A serve that "dies" immediately after every successful start: the FIRST +/// `exited()` consult is `None` (the readiness wait's liveness check — the +/// health probe answers healthy on that same iteration), every later consult +/// reports the exit. Each instance serves exactly one spawn generation, so +/// every generation is healthy-then-dead one watch interval later. +struct DieAfterHealthProcess { + exited_consults: AtomicUsize, +} +impl ServeProcess for DieAfterHealthProcess { + fn exited(&self) -> Option { + let n = self.exited_consults.fetch_add(1, Ordering::SeqCst) + 1; + (n >= 2).then_some(0) + } + fn take_fatal_startup_error(&self) -> Option { + None + } + fn kill(&self) {} +} + +/// Hands every generation a [`FlagExitProcess`] sharing the flag/counters. +struct FlagExitSpawner { + exited: Arc, + killed: Arc, + spawns: Arc, +} +impl ProcessSpawner for FlagExitSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + self.spawns.fetch_add(1, Ordering::SeqCst); + Ok(Box::new(FlagExitProcess { + exited: self.exited.clone(), + killed: self.killed.clone(), + })) + } +} + +/// Hands every generation a fresh [`DieAfterHealthProcess`]. +struct DieAfterHealthSpawner { + spawns: Arc, +} +impl ProcessSpawner for DieAfterHealthSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + self.spawns.fetch_add(1, Ordering::SeqCst); + Ok(Box::new(DieAfterHealthProcess { + exited_consults: AtomicUsize::new(0), + })) + } +} + +/// Scripted topology: generation 1 is a test-controlled [`FlagExitProcess`] +/// (the healthy daemon the watcher arms on); every later generation is a +/// [`NeverExitsProcess`] — the later generations fail/succeed via the scripted +/// HTTP health, never via their own exit. +struct GenerationSpawner { + first: Arc, + killed: Arc, + spawns: Arc, +} +impl ProcessSpawner for GenerationSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + let n = self.spawns.fetch_add(1, Ordering::SeqCst) + 1; + if n == 1 { + Ok(Box::new(FlagExitProcess { + exited: self.first.clone(), + killed: self.killed.clone(), + })) + } else { + Ok(Box::new(NeverExitsProcess { + killed: self.killed.clone(), + })) + } + } +} + +/// Hands every generation a [`NeverExitsProcess`]; kills/spawns counted. +struct NeverExitsSpawner { + killed: Arc, + spawns: Arc, +} +impl ProcessSpawner for NeverExitsSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + self.spawns.fetch_add(1, Ordering::SeqCst); + Ok(Box::new(NeverExitsProcess { + killed: self.killed.clone(), + })) + } +} + +struct NoopEventHandle; +impl EventStreamHandle for NoopEventHandle {} + +struct NoopEventSource; +impl EventSource for NoopEventSource { + fn connect(&self, _url: String, _sink: EventSink) -> Box { + Box::new(NoopEventHandle) + } +} + +/// The Task 3 self-heal knobs: a tiny watch interval and a tiny backoff. +fn selfheal_config(watch_ms: u64, backoff_initial_ms: u64, backoff_max_ms: u64) -> ServeConfig { + ServeConfig { + daemon_watch_interval: Duration::from_millis(watch_ms), + re_warm_backoff_initial_ms: backoff_initial_ms, + re_warm_backoff_max_ms: backoff_max_ms, + ..ServeConfig::default() + } +} + +async fn started_manager(deps: ServeDeps, config: ServeConfig) -> OpencodeServeManager { + let mgr = OpencodeServeManager::new(deps, config); + mgr.ensure_started() + .await + .expect("healthy fake serve starts"); + mgr +} + +// ── tests ────────────────────────────────────────────────────────────────────── + +/// A daemon that dies on its own must not leave a poisoned running entry +/// forever (the 2026-09-20 incident's silent half): the watcher clears the +/// entry, emits Lost for in-flight turns, signals daemon loss, and schedules +/// the backoff-guarded respawn. +#[tokio::test] +async fn unrequested_daemon_exit_clears_running_emits_lost_and_signals() { + let exited = Arc::new(AtomicBool::new(false)); + let killed = Arc::new(AtomicUsize::new(0)); + let spawns = Arc::new(AtomicUsize::new(0)); + let deps = ServeDeps { + spawner: Arc::new(FlagExitSpawner { + exited: exited.clone(), + killed: killed.clone(), + spawns: spawns.clone(), + }), + http: Arc::new(HealthyHttp { + prompt_pending: false, + }), + ports: Arc::new(CountingAllocator { + next: AtomicU16::new(0), + }), + events: Arc::new(NoopEventSource), + }; + let manager = started_manager(deps, selfheal_config(10, 5, 50)).await; + // Subscribe BEFORE the daemon dies: tokio broadcast does NOT replay + // history to late subscribers, so a post-loss subscribe would miss the + // edge (the contract Task 4's level-triggered design depends on). + let mut signals = manager.subscribe_daemon_signals(); + let mut idle = manager.subscribe("ses_a"); + + exited.store(true, Ordering::SeqCst); // the daemon "exits" + + let signal = tokio::time::timeout(Duration::from_secs(2), signals.recv()) + .await + .expect("loss signal within budget") + .expect("channel alive"); + assert!( + matches!( + signal, + DaemonSignal::Lost { + reason: "process_exit" + } + ), + "an unrequested daemon exit must signal Lost{{process_exit}}, got {signal:?}" + ); + assert!( + manager.base_url().await.is_none(), + "the dead daemon's running entry must be cleared" + ); + assert!( + matches!(idle.try_recv(), Ok(SessionSignal::Lost)), + "in-flight session subscribers must see the Lost edge" + ); + assert!( + killed.load(Ordering::SeqCst) >= 1, + "the already-exited daemon is still kill()ed for /proc-reaper parity" + ); +} + +/// Crash-loop guard: the automatic re-warm must BACK OFF exponentially, not +/// spawn-storm. A daemon that dies immediately after every successful start +/// drives a continuous loss→re-warm cycle; within the 700 ms window the +/// observed spawn count must show the 50→100→200→400 ms escalation (a +/// non-backed-off loop would spawn dozens; no re-warm at all would strand the +/// count at 1). +#[tokio::test] +async fn daemon_loss_re_warm_backs_off_exponentially() { + let spawns = Arc::new(AtomicUsize::new(0)); + let deps = ServeDeps { + spawner: Arc::new(DieAfterHealthSpawner { + spawns: spawns.clone(), + }), + http: Arc::new(HealthyHttp { + prompt_pending: false, + }), + ports: Arc::new(CountingAllocator { + next: AtomicU16::new(0), + }), + events: Arc::new(NoopEventSource), + }; + // The manager binding stays alive for the window: its watcher/re-warm + // tasks hold their own Arc clones, but keeping the binding makes the + // driving ownership explicit. + let _manager = started_manager(deps, selfheal_config(5, 50, 400)).await; + + tokio::time::sleep(Duration::from_millis(700)).await; + let observed = spawns.load(Ordering::SeqCst); + assert!( + (2..=6).contains(&observed), + "re-warm must respawn (>= 2) AND back off exponentially (<= 6) — \ + observed {observed} spawns in 700 ms with 50 ms initial / 400 ms max backoff" + ); +} + +/// A FAILED re-warm attempt must RETRY (the loop), never strand the daemon +/// permanently absent: spawn #1 is healthy and exits on demand; the re-warm's +/// spawns #2 and #3 fail their bounded health waits; spawn #4 is healthy. The +/// daemon must eventually come back, having spawned at least 4 times. +#[tokio::test] +async fn a_failed_re_warm_retries_until_the_daemon_starts() { + let spawns = Arc::new(AtomicUsize::new(0)); + let first_exited = Arc::new(AtomicBool::new(false)); + let killed = Arc::new(AtomicUsize::new(0)); + let deps = ServeDeps { + spawner: Arc::new(GenerationSpawner { + first: first_exited.clone(), + killed: killed.clone(), + spawns: spawns.clone(), + }), + http: Arc::new(GenerationHealthHttp { + fail_ports: vec![2, 3], + }), + ports: Arc::new(CountingAllocator { + next: AtomicU16::new(0), + }), + events: Arc::new(NoopEventSource), + }; + let config = ServeConfig { + health_timeout: Duration::from_millis(40), + health_probe_timeout: Duration::from_millis(10), + health_retry_interval: Duration::from_millis(5), + daemon_watch_interval: Duration::from_millis(5), + re_warm_backoff_initial_ms: 10, + re_warm_backoff_max_ms: 50, + ..ServeConfig::default() + }; + let manager = started_manager(deps, config).await; + let mut signals = manager.subscribe_daemon_signals(); + + first_exited.store(true, Ordering::SeqCst); // the healthy daemon "exits" + let signal = tokio::time::timeout(Duration::from_secs(2), signals.recv()) + .await + .expect("loss signal within budget") + .expect("channel alive"); + assert!( + matches!( + signal, + DaemonSignal::Lost { + reason: "process_exit" + } + ), + "got {signal:?}" + ); + + tokio::time::timeout(Duration::from_secs(2), async { + loop { + if manager.base_url().await.is_some() { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("the re-warm loop must eventually succeed (fail, fail, then healthy)"); + assert!( + spawns.load(Ordering::SeqCst) >= 4, + "the failed re-warm attempts must be retried — observed {} spawns \ + (initial + 2 failed re-warms + success)", + spawns.load(Ordering::SeqCst) + ); +} + +/// A discard (the intentional kill path) must ALSO signal daemon loss (with +/// its reason) and schedule the re-warm, so the runtime self-heal (Task 4) +/// observes the requested-loss class the same as the crash class. Driven +/// through `prompt_async` — a deliberate `DiscardOnTimeout::Yes` lane. +#[tokio::test] +async fn discard_running_signals_daemon_loss_and_schedules_re_warm() { + let spawns = Arc::new(AtomicUsize::new(0)); + let killed = Arc::new(AtomicUsize::new(0)); + let deps = ServeDeps { + spawner: Arc::new(NeverExitsSpawner { + killed: killed.clone(), + spawns: spawns.clone(), + }), + http: Arc::new(HealthyHttp { + prompt_pending: true, + }), + ports: Arc::new(CountingAllocator { + next: AtomicU16::new(0), + }), + events: Arc::new(NoopEventSource), + }; + let config = ServeConfig { + request_timeout: Duration::from_millis(50), + daemon_watch_interval: Duration::from_millis(5), + re_warm_backoff_initial_ms: 20, + re_warm_backoff_max_ms: 100, + ..ServeConfig::default() + }; + let manager = started_manager(deps, config).await; + let mut signals = manager.subscribe_daemon_signals(); + + let err = manager + .prompt_async( + "ses_discard", + build_prompt_body("hi", None, None), + &None, + None, + ) + .await + .expect_err("the prompt POST must time out"); + assert!( + matches!(err, ServeError::RequestTimeout { .. }), + "got {err:?}" + ); + + let lost = tokio::time::timeout(Duration::from_secs(2), signals.recv()) + .await + .expect("loss signal within budget") + .expect("channel alive"); + assert!( + matches!( + lost, + DaemonSignal::Lost { + reason: "request_timeout" + } + ), + "a discard must signal its reason, got {lost:?}" + ); + let respawned = tokio::time::timeout(Duration::from_secs(2), signals.recv()) + .await + .expect("re-warm within budget") + .expect("channel alive"); + assert!( + matches!(respawned, DaemonSignal::Started), + "the discard's scheduled re-warm must respawn the daemon, got {respawned:?}" + ); + assert!( + spawns.load(Ordering::SeqCst) >= 2, + "a respawn happened after the backoff (observed {} spawns)", + spawns.load(Ordering::SeqCst) + ); + assert!( + manager.base_url().await.is_some(), + "the re-warmed daemon serves again" + ); +} From cc9c5f84504848cc0ae674d8bf142c81a39cd6e9 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:31:14 -0700 Subject: [PATCH 09/24] feat(freshopencode): daemon-loss self-heal edge, respawn revival, and bridge restarts --- AGENTS.md | 2 +- crates/freshell-freshagent/src/opencode_ws.rs | 806 +++++++++++++++++- 2 files changed, 788 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b8abb790d..fda7580e3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -260,7 +260,7 @@ live in [docs/development/gcloud-robot.md](docs/development/gcloud-robot.md). **Rename scope contract:** pane/tab labels are layout-local; only an explicit session rename writes a durable session title (terminal renames are terminal-scoped). The four name scopes and the reset-to-provider-title flow live in [docs/development/rename-scope-contract.md](docs/development/rename-scope-contract.md). -**Agent Status Indicators:** Blue/busy status is derived from provider activity slices through `resolvePaneActivity`; green/needs-attention and the idle sound flow through `recordTurnComplete` and `useTurnCompletionNotifications`. Turn-complete (green/sound) is server-authoritative everywhere: terminal CLIs via `terminal.turn.complete`, and fresh-agent panes (freshclaude/kilroy/freshcodex/freshopencode) via a discrete `freshAgent.turn.complete` edge emitted only on a positive completion — freshclaude/kilroy on the SDK `result` with `subtype === 'success'`, freshopencode on the success-only `emitStatus(idle)` path, and freshcodex on `turn/completed` only when `params.turn.status === 'completed'` (the notification also fires on interrupt). The client folds it in via `applyFreshAgentCompletion` using the `at`-monotonic dedupe regime (wall-clock `at`, no per-session counter, so a resumed durable session can't swallow completions across a server restart). The waiting-for-approval edge is ALSO server-authoritative: the Claude/kilroy `SdkBridge` emits a discrete `freshAgent.turn.waiting` edge on the 0→≥1 pending permission/question transition (only Claude/kilroy raise approvals/questions), and the client folds it in via `applyFreshAgentWaiting` under a distinct `${provider}:${sessionId}#waiting` dedupe namespace so it can never poison (or be poisoned by) the turn-complete bucket. The fragile client-side busy→idle derivation AND the client-side waiting-edge hook (`useAgentSessionTurnCompletion`) were both removed — all green/sound edges are now server-emitted. freshcodex additionally self-heals a crashed/disconnected codex sidecar by consuming the runtime `onExit` hook in `subscribe()`, emitting `sdk.status:'exited'` to clear BLUE (no chime — a crash is not a positive completion). freshcodex also runs a wedged-sidecar deadman: after a bounded quiet window (default 10 min, env `FRESHELL_FRESHCODEX_QUIET_WINDOW_MS`) with a turn in flight and no sidecar events, the server stops asserting busy and marks the pane `stuck`, and the client shows an amber "Agent appears stuck" card (`role="alert"`) with "Restart sidecar" (kill + resume re-mint) and "Start new conversation" actions; the deadman never fabricates a turn-complete (no green/chime). `freshopencode` still runs on a shared long-lived `opencode serve` sidecar and uses server-pushed `session.idle`/`session.status` events to drive busy. Gemini and Kimi terminal modes are status-in... [truncated] Separately, the sidebar shows cross-device remote status rings around a session row's icon: a green ring means the session is open on another device, a blue ring means it is busy on another device (blue wins over green), and rings are suppressed entirely when the session is open on this device (derived from `tabs.sync` registry snapshots — producing clients stamp pane payloads with `sessionKeys`/`busySessionKeys`, consumers re-query remote snapshots on a 30s interval, and the server partitions same-device records into `sameDeviceOpen`, which never produces rings). +**Agent Status Indicators:** Blue/busy status is derived from provider activity slices through `resolvePaneActivity`; green/needs-attention and the idle sound flow through `recordTurnComplete` and `useTurnCompletionNotifications`. Turn-complete (green/sound) is server-authoritative everywhere: terminal CLIs via `terminal.turn.complete`, and fresh-agent panes (freshclaude/kilroy/freshcodex/freshopencode) via a discrete `freshAgent.turn.complete` edge emitted only on a positive completion — freshclaude/kilroy on the SDK `result` with `subtype === 'success'`, freshopencode on the success-only `emitStatus(idle)` path, and freshcodex on `turn/completed` only when `params.turn.status === 'completed'` (the notification also fires on interrupt). The client folds it in via `applyFreshAgentCompletion` using the `at`-monotonic dedupe regime (wall-clock `at`, no per-session counter, so a resumed durable session can't swallow completions across a server restart). The waiting-for-approval edge is ALSO server-authoritative: the Claude/kilroy `SdkBridge` emits a discrete `freshAgent.turn.waiting` edge on the 0→≥1 pending permission/question transition (only Claude/kilroy raise approvals/questions), and the client folds it in via `applyFreshAgentWaiting` under a distinct `${provider}:${sessionId}#waiting` dedupe namespace so it can never poison (or be poisoned by) the turn-complete bucket. The fragile client-side busy→idle derivation AND the client-side waiting-edge hook (`useAgentSessionTurnCompletion`) were both removed — all green/sound edges are now server-emitted. freshcodex additionally self-heals a crashed/disconnected codex sidecar by consuming the runtime `onExit` hook in `subscribe()`, emitting `sdk.status:'exited'` to clear BLUE (no chime — a crash is not a positive completion). freshcodex also runs a wedged-sidecar deadman: after a bounded quiet window (default 10 min, env `FRESHELL_FRESHCODEX_QUIET_WINDOW_MS`) with a turn in flight and no sidecar events, the server stops asserting busy and marks the pane `stuck`, and the client shows an amber "Agent appears stuck" card (`role="alert"`) with "Restart sidecar" (kill + resume re-mint) and "Start new conversation" actions; the deadman never fabricates a turn-complete (no green/chime). `freshopencode` still runs on a shared long-lived `opencode serve` sidecar and uses server-pushed `session.idle`/`session.status` events to drive busy, and the runtime self-heals a shared-daemon death (the 2026-09-20 incident class): daemon loss fans a typed `freshAgent.error{code:"OPENCODE_DAEMON_LOST"}` edge out to exactly one frame per materialized session (the client's generic `sessionError` banner + busy-clear; NO chime — a crash is never a positive completion), the manager respawns the daemon on a backoff ladder (fresh-incident reset, crash-loop escalation), every successful cold start drives a level-triggered revival pass that restarts dead session bridges and pushes `freshAgent.session.snapshot{status:"idle"}` (the client's transcript-refetch trigger) while respecting the ownership coordinator (retired, transitioned, or terminal-owned sessions are never revived), and a generation-fenced `freshAgent.attach` is itself a recovery verb that respawns the daemon before re-bridging. Gemini and Kimi terminal modes are status-in... [truncated] Separately, the sidebar shows cross-device remote status rings around a session row's icon: a green ring means the session is open on another device, a blue ring means it is busy on another device (blue wins over green), and rings are suppressed entirely when the session is open on this device (derived from `tabs.sync` registry snapshots — producing clients stamp pane payloads with `sessionKeys`/`busySessionKeys`, consumers re-query remote snapshots on a 30s interval, and the server partitions same-device records into `sameDeviceOpen`, which never produces rings). **Fresh-Agent Orchestration:** The Rust REST agent API (`/api/tabs`, `/api/panes/:id/split`, `/api/panes/:id/send-keys`, `/api/panes/:id/capture`, `/api/panes/:id/wait-for`) and the standalone Node MCP client accept `agent`/`model`/`effort` parameters where the Rust contract supports them. The Rust orchestration layer dispatches to the registered fresh-agent runtimes. On MCP `new-tab`, resume sugar (`resume`/`resumeSessionId`) is honored for `agent: "opencode"`; terminal-mode resume uses an explicit provider-matched `sessionRef` (raw Codex resume IDs are rejected because they are not sufficient restore identity). Unsupported legacy actions return a deterministic unavailable result instead of contacting a removed backend route. diff --git a/crates/freshell-freshagent/src/opencode_ws.rs b/crates/freshell-freshagent/src/opencode_ws.rs index 6c66c1834..d064c48d5 100644 --- a/crates/freshell-freshagent/src/opencode_ws.rs +++ b/crates/freshell-freshagent/src/opencode_ws.rs @@ -65,8 +65,8 @@ use tokio::sync::Mutex as TokioMutex; use freshell_codex::next_monotonic_turn_complete_at; use freshell_opencode::{ - normalize_opencode_effort, normalize_opencode_model, ChangedReason, OpencodeServeManager, - SdkProviderEvent, ServeError, SessionSignal, SnapshotStatus, + normalize_opencode_effort, normalize_opencode_model, ChangedReason, DaemonSignal, + OpencodeServeManager, SdkProviderEvent, ServeError, SessionSignal, SnapshotStatus, }; use freshell_protocol::{ ErrorCode, ErrorMsg, FreshAgentAttach, FreshAgentCompact, FreshAgentConfigure, @@ -159,6 +159,16 @@ pub struct FreshOpencodeState { /// replacement probe re-issues the daemon-side abort through exactly /// this record. Cleared by whichever path settles the acceptance. condemned_sessions: Arc>>, + /// Task 4 (opencode daemon-death recovery): the daemon-loss watcher's + /// arming cell — set-once per state (Arc-shared across every clone), so + /// the FIRST `handle_send`/`handle_attach`/`handle_compact` arms exactly + /// ONE runtime-level listener on the manager's `DaemonSignal` stream for + /// the process lifetime. The watcher makes a shared-daemon death + /// observable (the typed `OPENCODE_DAEMON_LOST` edge per materialized + /// session) and recoverable (the level-triggered bridge revival on every + /// `Started`), mirroring the freshcodex onExit self-heal. See + /// [`Self::ensure_daemon_loss_watcher`]. + daemon_loss_watcher: Arc>, } /// The condemned opencode session's quiescence identity (b8ke focused @@ -551,6 +561,7 @@ impl FreshOpencodeState { fork_in_flight: crate::InFlightRegistry::new(), rollback_in_flight: crate::InFlightRegistry::new(), condemned_sessions: Arc::new(std::sync::Mutex::new(HashMap::new())), + daemon_loss_watcher: Arc::new(std::sync::OnceLock::new()), } } @@ -1429,6 +1440,10 @@ impl FreshOpencodeState { let request_id = msg.request_id.clone(); let session_id = msg.session_id.clone(); + // Task 4: arm the runtime's daemon-loss watcher (idempotent — the + // first opencode WS traffic arms the one process-level listener). + self.ensure_daemon_loss_watcher().await; + let session_arc = { let guard = self.sessions.lock().await; guard.get(&session_id).cloned() @@ -3558,6 +3573,10 @@ impl FreshOpencodeState { return; } }; + // Task 4: arm the runtime's daemon-loss watcher (idempotent — the + // fence parse above stays the FIRST interaction, so a typed refusal + // still mutates nothing). + self.ensure_daemon_loss_watcher().await; let session_arc = { let guard = self.sessions.lock().await; guard.get(&session_id).cloned() @@ -5179,6 +5198,10 @@ impl FreshOpencodeState { return; } }; + // Task 4: arm the runtime's daemon-loss watcher (idempotent — the + // fence parse above stays the FIRST interaction, so a typed refusal + // still mutates nothing). + self.ensure_daemon_loss_watcher().await; let session_arc = { let guard = self.sessions.lock().await; guard.get(&msg.session_id).cloned() @@ -5454,23 +5477,32 @@ impl FreshOpencodeState { let (status_session_id, running, real_session_id) = { let mut session = session_arc.lock().await; - // Ensure the serve-SSE bridge is running (restart it if it died) -- only - // meaningful once a durable session exists; a not-yet-materialized session has - // never started a bridge (`bindServeStream` only fires from `materializeOrSend`). - if let Some(real_id) = session.real_session_id.clone() { - let bridge_dead = session - .serve_bridge - .as_ref() - .map(tokio::task::JoinHandle::is_finished) - .unwrap_or(true); - if bridge_dead { - let manager = self.fresh_agent.ensure_manager().await; - session.serve_bridge = Some(self.spawn_serve_bridge( - manager, - real_id, - session.turn_errored.clone(), - )); - } + // Ensure the serve-SSE bridge is running (restart it if it died) -- + // only meaningful once a durable session exists; a not-yet- + // materialized session has never started a bridge (`bindServeStream` + // only fires from `materializeOrSend`). Task 4: the tail is the + // shared `restart_session_bridge_guarded` helper, which FIRST + // `ensure_started()`s the shared daemon (LB-05 — a restart without + // a daemon re-bridges into nothing; the incident's 3 attach + // attempts recovered NOTHING because nothing respawned the daemon + // for a map-hit). On a bounded respawn failure the attach answers + // the TYPED error path, never a silent half-attached state. + if let Err(err) = self.restart_session_bridge_guarded(&mut session).await { + drop(session); + tracing::warn!(target: "freshell_freshagent::opencode", + session_id = %msg.session_id, error = %err, + "freshagent.opencode.attach_daemon_respawn_failed: the dead-bridge \ + restart could not bring the shared daemon back — the attach answers \ + the typed error instead of a silent half-attached state" + ); + self.emit_fresh_agent_error( + &msg.session_id, + "OPENCODE_ATTACH_RESUME_FAILED", + &format!( + "The opencode serve daemon could not be restarted for this session: {err}" + ), + ); + return; } let status_session_id = session @@ -6022,6 +6054,263 @@ impl FreshOpencodeState { } }) } + + // ── Task 4 (opencode daemon-death recovery): the runtime-level self-heal ── + + /// Arm the runtime's daemon-loss watcher (2026-09-20 incident: the shared + /// `opencode serve` daemon died and NOTHING told the panes — no status + /// edge, no bridge revival; panes dead-ended on the snapshot 409). + /// Idempotent: the set-once [`OnceLock`] guarantees exactly ONE listener + /// per state no matter how many handlers call this. Armed from + /// [`Self::handle_send`] / [`Self::handle_attach`] / [`Self::handle_compact`] + /// — the opencode WS entry points — so the listener exists before any + /// session traffic can depend on it. The watcher task holds a FULL state + /// clone (LB-10), so `spawn_serve_bridge(&self, ..)` is callable directly. + /// + /// - `Lost{reason}`: WARN `freshagent.opencode.daemon_loss_observed`, then + /// fan the typed `OPENCODE_DAEMON_LOST` edge out to every MATERIALIZED + /// session ([`Self::fan_out_daemon_loss_edge`]). The manager's own + /// backoff-guarded re-warm (Task 3) is already scheduled at that point. + /// - `Started`: the LEVEL-TRIGGERED revival pass + /// ([`Self::revive_dead_bridges_if_daemon_running`]) — never dependent + /// on having observed `Lost` (LB-02: tokio broadcast does NOT replay + /// history to late subscribers). + /// - `Lagged`: continue (LB-08: never disarm on lag). + /// - `Closed`: the manager (and its signal sender) is gone — return. + /// + /// NO CHIME (the freshcodex onExit mirror's discipline): neither edge of + /// this watcher ever emits `freshAgent.turn.complete` — a crash is not a + /// positive completion. + async fn ensure_daemon_loss_watcher(&self) { + if self.daemon_loss_watcher.set(()).is_err() { + return; // already armed (the cell is Arc-shared across every clone) + } + let manager = self.fresh_agent.ensure_manager().await; + let state = self.clone(); + let mut signals = manager.subscribe_daemon_signals(); + tokio::spawn(async move { + // Arming-time LEVEL pass (LB-02): broadcast does NOT replay + // history — a daemon that re-warmed before this subscription + // must still get its dead bridges revived now. + state.revive_dead_bridges_if_daemon_running().await; + loop { + match signals.recv().await { + Ok(DaemonSignal::Lost { reason }) => { + tracing::warn!(target: "freshell_freshagent::opencode", + reason = reason, + "freshagent.opencode.daemon_loss_observed: the shared \ + opencode serve daemon was lost — fanning the typed edge \ + out to every materialized session; the manager's \ + backoff-guarded re-warm is already scheduled" + ); + state.fan_out_daemon_loss_edge().await; + } + Ok(DaemonSignal::Started) => { + // LEVEL-TRIGGERED revival (LB-02): revive whatever is + // dead right now — no `saw_loss` heuristic. + state.revive_dead_bridges_if_daemon_running().await; + } + Err(RecvError::Lagged(_)) => continue, + Err(RecvError::Closed) => return, + } + } + }); + } + + /// The daemon-loss fan-out (Task 4): exactly ONE typed edge per + /// MATERIALIZED session — + /// `freshAgent.event{provider:"opencode", sessionType:"freshopencode", + /// event:{type:"freshAgent.error", code:"OPENCODE_DAEMON_LOST", message}}` + /// — which the client folds through the EXISTING generic `sessionError` + /// path (the dismissible "Agent error:" banner + busy-clear). The + /// sessions map is keyed by BOTH the placeholder and the durable id + /// pointing at the SAME session (`remember()` mirror) — dedupe by + /// `real_session_id` (BTreeSet) so each materialized session gets exactly + /// ONE edge. The message names the re-warm so the banner reads as the + /// self-heal it is, and NO chime ever accompanies it. + async fn fan_out_daemon_loss_edge(&self) { + const CODE: &str = "OPENCODE_DAEMON_LOST"; + const MESSAGE: &str = + "The opencode serve daemon was lost unexpectedly - it is restarting automatically."; + for id in self.materialized_session_ids().await { + self.emit_fresh_agent_error(&id, CODE, MESSAGE); + } + } + + /// The distinct durable `ses_*` ids of every MATERIALIZED session in the + /// map (dual-key dedupe), for the loss fan-out and the revival pass + /// alike. LB-01: the sessions-map guard is NEVER held across a + /// per-session lock (the documented contract above — the reverse edge + /// deadlocked production) — the session `Arc`s are cloned out under ONE + /// short map lock, the guard drops, and each session is read outside it. + async fn materialized_session_ids(&self) -> Vec { + let arcs: Vec>> = { + let map = self.sessions.lock().await; + map.values().cloned().collect() + }; + let mut ids = std::collections::BTreeSet::new(); + for arc in arcs { + if let Some(id) = arc.lock().await.real_session_id.clone() { + ids.insert(id); + } + } + ids.into_iter().collect() + } + + /// The LEVEL-TRIGGERED revival pass (Task 4, LB-02): restart dead/absent + /// serve-SSE bridges for MATERIALIZED sessions while the shared daemon + /// runs, and push `freshAgent.session.snapshot{status:"idle"}` ONLY to + /// sessions whose bridge was actually restarted (the client treats that + /// push as snapshot-invalidating → transcript refetch). Called on watcher + /// arming and on every `DaemonSignal::Started` — never dependent on + /// having observed `Lost`. + /// + /// The ownership coordinator (plan-review round 3) is respected at every + /// step: (1) `base_url()` is None → return (daemon absent — nothing to + /// revive into; the next `Started` or a fenced attach drives revival); + /// (2) snapshot the map (clone the `Arc`s under one short lock, drop the + /// guard); (3) per candidate OUTSIDE the map guard: RE-LOOKUP the id at + /// revival time (a killed/handed-off session's keys are gone — never act + /// on the retained `Arc` alone), observe the CANONICAL ownership state + /// fresh, skip on ANY transition (Handoff/Starting/Stopping/Fenced) or a + /// terminal owner, revive only `Live{FreshAgent}` (this runtime's own + /// sessions) behind the SAME `arm_adopt_guard` the attach path uses, + /// then run the shared [`Self::restart_session_bridge_guarded`] tail. An + /// unwired coordinator applies no gate (the pre-wiring legacy: map + /// membership is the only authority). + async fn revive_dead_bridges_if_daemon_running(&self) { + let manager = self.fresh_agent.ensure_manager().await; + if manager.base_url().await.is_none() { + return; + } + for durable in self.materialized_session_ids().await { + // (3a) Re-lookup at revival time — the retained Arc alone is + // stale the moment a kill/handoff removes the keys. + let session_arc = { + let map = self.sessions.lock().await; + map.get(&durable).cloned() + }; + let Some(session_arc) = session_arc else { + continue; + }; + // (3b) The ownership gate — armed with the SAME adopt-guard + // machinery the attach path uses, so the coordinator's + // atomicity rides along instead of being re-implemented. + let mut adopt_guard = None; + if self.fresh_agent.ownership.is_some() { + let snap = self + .fresh_agent + .canonical_ownership_snapshot(PROVIDER, &durable); + match snap.state { + freshell_ownership::OwnershipState::Live { owner, .. } + if owner.kind == freshell_ownership::RuntimeOwnerKind::FreshAgent => + { + let expected = freshell_ownership::OwnerIdentity { + kind: freshell_ownership::RuntimeOwnerKind::FreshAgent, + terminal_id: None, + live_session_key: None, + pid: None, + ownership_id: None, + }; + match crate::ownership_lane::arm_adopt_guard( + &self.fresh_agent.ownership, + PROVIDER, + &durable, + &format!("daemon-revive-{}", uuid::Uuid::new_v4()), + &expected, + freshell_ownership::ObservedFence { + epoch: snap.epoch, + generation: snap.generation, + }, + "freshopencode/daemon-revival", + ) { + crate::ownership_lane::LaneAttachGuard::Armed(guard) => { + adopt_guard = Some(guard) + } + // The coordinator moved between the observe and + // the arm — a lifecycle owns the window; skip, + // never force. + crate::ownership_lane::LaneAttachGuard::Refused => continue, + crate::ownership_lane::LaneAttachGuard::Unwired => {} + } + } + // Any transition (Handoff/Starting/Stopping/Fenced), a + // terminal owner, a vacant key, any other kind: someone + // else's window — never revive into it. + _ => continue, + } + } + // (3c/3d) The shared guarded-restart tail, under the session + // lock; the snapshot push goes ONLY to actually-restarted + // bridges. `Ok(None)` (bridge alive / unmaterialized) is the + // quiet no-op. + let restarted = { + let mut session = session_arc.lock().await; + self.restart_session_bridge_guarded(&mut session).await + }; + match restarted { + Ok(Some(real_id)) => { + self.broadcast(&event_frame(&real_id, snapshot_event(&real_id, "idle"))); + } + Ok(None) => {} + Err(err) => { + tracing::warn!(target: "freshell_freshagent::opencode", + session_id = %durable, error = %err, + "freshagent.opencode.daemon_revival_restart_failed: the \ + bridge restart's bounded daemon respawn failed — the next \ + Started signal or a fenced attach retries" + ); + } + } + // The guard covered the restart; release the window. + drop(adopt_guard); + } + } + + /// The GUARD-HELD bridge-restart tail (the Task 4 refactor): the ONE + /// shared restart both [`Self::handle_attach`]'s dead-bridge arm and the + /// revival pass ([`Self::revive_dead_bridges_if_daemon_running`]) call. + /// Callers arm `ownership_lane::arm_adopt_guard` and hold it ACROSS this + /// call so a handoff beginning inside the window answers the typed + /// Blocked outcome. The caller holds the per-session lock here (NEVER + /// the sessions-map guard — LB-01). + /// + /// LB-05 (falsified → redesign): in the incident the fenced attach was + /// exercised 3× against the dead shared daemon and recovered nothing — + /// the tail only re-subscribed the bridge. A restart must first + /// `ensure_started()` the shared daemon (mirroring + /// `resume_durable_session`'s map-miss behavior); `ensure_started` is + /// single-flighted, so concurrent attach/send/compact callers cannot + /// spawn a second daemon. + /// + /// `Ok(Some(real_id))` — the bridge was (re)started; `Ok(None)` — nothing + /// to do (unmaterialized, or the bridge is alive); `Err` — the BOUNDED + /// respawn failed (the caller answers typed, never a silent + /// half-attached state). + async fn restart_session_bridge_guarded( + &self, + session: &mut OpencodeSession, + ) -> Result, ServeError> { + // Only meaningful once a durable session exists; a not-yet- + // materialized session has never started a bridge (`bindServeStream` + // only fires from `materializeOrSend`). + let Some(real_id) = session.real_session_id.clone() else { + return Ok(None); + }; + let bridge_dead = session + .serve_bridge + .as_ref() + .map(tokio::task::JoinHandle::is_finished) + .unwrap_or(true); + if !bridge_dead { + return Ok(None); + } + let manager = self.fresh_agent.ensure_manager().await; + manager.ensure_started().await?; + session.serve_bridge = + Some(self.spawn_serve_bridge(manager, real_id.clone(), session.turn_errored.clone())); + Ok(Some(real_id)) + } } /// ISO-8601 / RFC-3339 millis-Z timestamp (matches `new Date().toISOString()`) for error @@ -12987,10 +13276,21 @@ mod tests { } /// Insert a directly-materialized session (no send drove it) with the given model. + /// + /// Task 4 fixture fidelity: a materialized session carries a LIVE + /// serve-SSE bridge in production (`bindServeStream` fires at + /// materialization), so the fixture spawns one through the same + /// [`FreshOpencodeState::spawn_serve_bridge`] the materialization path + /// uses — the daemon-loss watcher (now armed by `handle_compact`) + /// otherwise "revives" the bridgeless fixture at its arming-time level + /// pass and pushes idle-snapshot frames these tests never modeled. async fn insert_compact_session(st: &FreshOpencodeState, id: &str, model: Option<&str>) { let mut session = OpencodeSession::new(id.to_string(), None, model.map(str::to_string), None); session.real_session_id = Some(id.to_string()); + let manager = st.fresh_agent.ensure_manager().await; + session.serve_bridge = + Some(st.spawn_serve_bridge(manager, id.to_string(), session.turn_errored.clone())); st.sessions .lock() .await @@ -18081,4 +18381,472 @@ mod tests { assert_eq!(last.settings.model.as_deref(), Some("prov/mdl-b")); assert_eq!(last.settings.effort.as_deref(), Some("low")); } + + // ── Task 4 (opencode daemon-death recovery): the runtime-level self-heal ── + // + // 2026-09-20 incident: the shared daemon died and NOTHING told the panes — + // no status edge, no respawn, no bridge revival; panes dead-ended on the + // snapshot 409. The runtime self-heal must make daemon loss observable and + // recoverable per session (mirroring the freshcodex onExit self-heal). + // (LB-02: revival is LEVEL-TRIGGERED — it runs on arming and on every + // `Started`, never dependent on having observed `Lost`.) + + /// A serve whose "exit" is test-controlled: `exited()` reports `Some(0)` + /// once the shared flag is set (the Task 3 selfheal fixture shape) — the + /// flag-driven daemon death both the manager's exit watcher and the + /// runtime's loss listener observe. + struct FlagExitProcess { + exited: Arc, + } + impl ServeProcess for FlagExitProcess { + fn exited(&self) -> Option { + self.exited.load(Ordering::SeqCst).then_some(0) + } + fn take_fatal_startup_error(&self) -> Option { + None + } + fn kill(&self) {} + } + + /// Every generation hands out a [`FlagExitProcess`] sharing the flag; + /// spawns counted (the respawn accounting the fenced-attach test asserts). + struct FlagExitSpawner { + exited: Arc, + spawns: Arc, + } + impl ProcessSpawner for FlagExitSpawner { + fn spawn(&self, _req: SpawnRequest) -> Result, String> { + self.spawns.fetch_add(1, Ordering::SeqCst); + Ok(Box::new(FlagExitProcess { + exited: self.exited.clone(), + })) + } + } + + /// Task 4 harness: a state wired to a STARTED selfheal-config fake manager + /// (tiny watch + backoff knobs so the manager's own backoff-guarded re-warm + /// runs at test speed) whose daemon's death is flag-controlled, plus the + /// bus receiver every frame assertion reads. + async fn selfheal_state( + backoff_initial_ms: u64, + backoff_max_ms: u64, + ) -> ( + FreshOpencodeState, + tokio::sync::broadcast::Receiver, + Arc, + Arc, + OpencodeServeManager, + ) { + let (tx, rx) = tokio::sync::broadcast::channel::(256); + let fresh_agent = FreshAgentState::new(Arc::new("tok".to_string()), Arc::new(tx)); + let exited = Arc::new(AtomicBool::new(false)); + let spawns = Arc::new(AtomicUsize::new(0)); + let deps = ServeDeps { + spawner: Arc::new(FlagExitSpawner { + exited: exited.clone(), + spawns: spawns.clone(), + }), + http: Arc::new(FakeHttp { + next_session: AtomicUsize::new(0), + }), + ports: Arc::new(FakeAllocator), + events: Arc::new(NoopEventSource), + }; + let config = ServeConfig { + idle_poll_interval: Duration::from_millis(20), + daemon_watch_interval: Duration::from_millis(10), + re_warm_backoff_initial_ms: backoff_initial_ms, + re_warm_backoff_max_ms: backoff_max_ms, + ..ServeConfig::default() + }; + let manager = OpencodeServeManager::new(deps, config); + manager + .ensure_started() + .await + .expect("healthy fake serve starts"); + fresh_agent.set_manager_for_test(manager.clone()).await; + ( + FreshOpencodeState::new(fresh_agent), + rx, + exited, + spawns, + manager, + ) + } + + /// The session's durable `ses_*` id (the fixture must have materialized). + async fn real_session_id_of(st: &FreshOpencodeState, placeholder: &str) -> String { + let session_arc = { + let sessions = st.sessions.lock().await; + sessions.get(placeholder).expect("tracked").clone() + }; + let durable = session_arc + .lock() + .await + .real_session_id + .clone() + .expect("the session materialized"); + durable + } + + /// Is the session's serve-SSE bridge live? False for an unmapped id (a + /// killed/handed-off session) and for a mapped session whose bridge is + /// dead/absent — the exact predicate the revival pass restarts on. + async fn session_serve_bridge_alive(st: &FreshOpencodeState, id: &str) -> bool { + let Some(session_arc) = st.sessions.lock().await.get(id).cloned() else { + return false; + }; + let session = session_arc.lock().await; + session + .serve_bridge + .as_ref() + .map(|b| !b.is_finished()) + .unwrap_or(false) + } + + /// Bounded wait until the coordinator shows the durable id Live under a + /// FRESH-AGENT owner (the materialization commit the revival/attach gates + /// consult). + async fn await_freshagent_live( + registry: &Arc, + durable: &str, + ) { + let deadline = tokio::time::Instant::now() + Duration::from_secs(5); + loop { + if matches!( + registry.observe("opencode", durable).state, + freshell_ownership::OwnershipState::Live { owner, .. } + if owner.kind == freshell_ownership::RuntimeOwnerKind::FreshAgent + ) { + return; + } + assert!( + tokio::time::Instant::now() < deadline, + "the materialization's Live{{FreshAgent}} commit never landed for {durable}" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + /// Model the committed-terminal-handoff shape: `begin_handoff` (granted) + /// then `commit_live` under a TERMINAL owner — the exact pair of calls a + /// finished terminal handoff leaves behind in the coordinator. + async fn commit_terminal_owner( + registry: &Arc, + durable: &str, + ) { + let operation_id = format!("handoff-{durable}"); + let freshell_ownership::BeginOutcome::Granted { generation } = registry.begin_handoff( + "opencode", + durable, + freshell_ownership::RuntimeOwnerKind::Terminal, + &operation_id, + None, + "test", + 0, + ) else { + panic!("the handoff begin must grant for {durable}") + }; + assert!( + matches!( + registry.commit_live( + "opencode", + durable, + &operation_id, + generation, + freshell_ownership::OwnerIdentity { + kind: freshell_ownership::RuntimeOwnerKind::Terminal, + terminal_id: Some(format!("t-{durable}")), + live_session_key: None, + pid: None, + ownership_id: None, + }, + ), + freshell_ownership::CommitOutcome::Committed + ), + "the terminal owner commit must land for {durable}" + ); + } + + /// Materialize one session through the REAL create+send path and settle + /// its turn locally (the existing IdleTimeout-shaped seam) so the only + /// frames after it are the machinery under test. Returns + /// (placeholder, durable). + async fn materialized_selfheal_session( + st: &FreshOpencodeState, + rx: &mut tokio::sync::broadcast::Receiver, + req: &str, + ) -> (String, String) { + st.handle_create(create_msg(req), None).await; + let placeholder = format!("freshopencode-{req}"); + st.handle_send(send_msg(&placeholder, "materialize")).await; + let durable = real_session_id_of(st, &placeholder).await; + st.settle_local_turn_task_for_test(&placeholder).await; + let _ = drain_frames(rx); + (placeholder, durable) + } + + /// Task 4 contract: the incident's silent half — a daemon that dies must + /// (a) fan a TYPED `OPENCODE_DAEMON_LOST` edge out to every MATERIALIZED + /// session (the dismissible banner + busy-clear the client folds through + /// the generic `sessionError` path), exactly ONE edge per session even + /// though the map is DUAL-KEYED (placeholder + durable id → the SAME + /// session), (b) NEVER chime (a crash is not a positive completion), and + /// (c) after the manager's backoff-guarded respawn, restart the dead + /// bridge and push the `status:"idle"` snapshot the client treats as a + /// transcript refetch. + #[tokio::test] + async fn daemon_loss_fans_out_a_typed_edge_then_revives_bridges_after_respawn() { + let (st, mut rx, exited, _spawns, manager) = selfheal_state(5, 50).await; + + let (_placeholder, durable) = + materialized_selfheal_session(&st, &mut rx, "req-daemon-loss").await; + // Fixture honesty: the session is dual-keyed — the dedupe contract's + // whole point (two map keys, ONE materialized session). + assert_eq!( + st.sessions.lock().await.len(), + 2, + "fixture: placeholder + durable keys both map the session" + ); + + exited.store(true, Ordering::SeqCst); // the daemon dies + + // THE TYPED EDGE (the incident's missing half). + let loss_frames = frames_until(&mut rx, |f| { + f["type"] == "freshAgent.event" + && f["event"]["type"] == "freshAgent.error" + && f["event"]["code"] == "OPENCODE_DAEMON_LOST" + }) + .await; + let edge = loss_frames.last().expect("the matching edge"); + assert_eq!(edge["provider"], "opencode"); + assert_eq!(edge["sessionType"], "freshopencode"); + assert_eq!(edge["sessionId"].as_str(), Some(durable.as_str())); + assert_eq!(edge["event"]["sessionId"].as_str(), Some(durable.as_str())); + assert_eq!( + edge["event"]["message"].as_str(), + Some( + "The opencode serve daemon was lost unexpectedly - it is restarting automatically." + ) + ); + + exited.store(false, Ordering::SeqCst); // the re-warm's respawn now succeeds + + // `DaemonSignal::Started` → the LEVEL-TRIGGERED revival: the dead + // bridge restarts and the client sees the snapshot-invalidating idle + // push (never a remembered-`Lost` heuristic). + let revive_frames = frames_until(&mut rx, |f| { + is_event(f, "freshAgent.session.snapshot", Some("idle")) + && f["sessionId"].as_str() == Some(durable.as_str()) + }) + .await; + + // NO chime ever accompanies a daemon loss, and the DUAL-KEYED session + // got exactly ONE edge — audit the whole post-loss window. + let mut all = loss_frames; + all.extend(revive_frames); + all.extend(drain_frames(&mut rx)); + let edges = all + .iter() + .filter(|f| { + f["type"] == "freshAgent.event" + && f["event"]["type"] == "freshAgent.error" + && f["event"]["code"] == "OPENCODE_DAEMON_LOST" + && f["sessionId"].as_str() == Some(durable.as_str()) + }) + .count(); + assert_eq!( + edges, 1, + "exactly ONE loss edge for the dual-keyed session: {all:?}" + ); + assert!( + all.iter() + .all(|f| f["event"]["type"] != "freshAgent.turn.complete"), + "a daemon loss is never a positive completion — no chime: {all:?}" + ); + + assert!( + session_serve_bridge_alive(&st, &durable).await, + "the bridge must be restarted after the respawn" + ); + assert!( + manager.base_url().await.is_some(), + "the manager's backoff-guarded re-warm respawned the daemon" + ); + } + + /// LB-05 (falsified → redesign): in the incident, the pane's fenced + /// attach was exercised 3× against the dead shared daemon and recovered + /// NOTHING, because the attach tail only re-subscribed the bridge — + /// nothing respawns the daemon for a map-hit. The fenced attach must be + /// a REAL recovery verb: `ensure_started` BEFORE the bridge restart, so + /// the map-hit attach respawns the daemon and re-bridges (mirroring + /// `resume_durable_session`'s map-miss behavior). + #[tokio::test] + async fn map_hit_fenced_attach_respawns_the_daemon_and_rebridges() { + // Backoff far beyond the test window: the background re-warm must NOT + // be the respawn this test credits — the ATTACH's own + // `ensure_started` is the recovery under proof. + let (mut st, mut rx, exited, spawns, manager) = selfheal_state(60_000, 120_000).await; + let registry = Arc::new(freshell_ownership::RuntimeOwnershipRegistry::new()); + st.set_ownership(Arc::clone(®istry)); + + let (_placeholder, durable) = + materialized_selfheal_session(&st, &mut rx, "req-attach-recover").await; + await_freshagent_live(®istry, &durable).await; + // The observed runtime-owner pair the fenced attach carries. + let before = registry.observe("opencode", &durable); + + // The shared daemon dies; the session row PERSISTS (the map-hit + // shape). Bounded wait for the manager's watcher to clear the entry. + exited.store(true, Ordering::SeqCst); + let deadline = tokio::time::Instant::now() + Duration::from_secs(5); + loop { + if manager.base_url().await.is_none() { + break; + } + assert!( + tokio::time::Instant::now() < deadline, + "the daemon's running entry must clear after the exit" + ); + tokio::time::sleep(Duration::from_millis(5)).await; + } + let _ = drain_frames(&mut rx); // the loss edge (already fanned out) etc. + let spawns_before = spawns.load(Ordering::SeqCst); + // The respawn can now succeed (the flag-driven fixture's death flag + // clears, exactly like the crash-loop fixture in Task 3's tests). + exited.store(false, Ordering::SeqCst); + + // THE MAP-HIT FENCED ATTACH — the incident's three wasted attempts, + // now the documented recovery verb. + st.handle_attach(FreshAgentAttach { + provider: AgentProvider::Opencode, + session_id: durable.clone(), + session_type: SessionType::Freshopencode, + cwd: None, + observed_epoch: Some(before.epoch), + observed_generation: Some(before.generation), + resume_session_id: None, + session_ref: None, + }) + .await; + + assert!( + spawns.load(Ordering::SeqCst) > spawns_before, + "a map-hit attach against a daemon-absent manager must respawn the \ + shared daemon (observed {} spawns; {} before the attach)", + spawns.load(Ordering::SeqCst), + spawns_before + ); + assert!( + session_serve_bridge_alive(&st, &durable).await, + "the attach must re-bridge the session" + ); + let frames = frames_until(&mut rx, |f| { + is_event(f, "freshAgent.session.snapshot", Some("idle")) + && f["sessionId"].as_str() == Some(durable.as_str()) + }) + .await; + assert!( + !frames.is_empty(), + "the attach tail's snapshot push must arrive: {frames:?}" + ); + } + + /// Plan-review round 3 (the ownership-coordinator gate): a session + /// killed/retired or handed to a terminal owner between the loss and the + /// respawn must NOT be revived — the revival pass re-looks-up the map at + /// revival time (a retired session's keys are gone) and observes the + /// CANONICAL ownership state fresh (a terminal owner or any lifecycle + /// transition owns the window; only Live{FreshAgent} sessions — this + /// runtime's own — revive). + #[tokio::test] + async fn revival_skips_sessions_handed_off_or_removed_after_the_loss() { + let (mut st, mut rx, exited, _spawns, _manager) = selfheal_state(5, 50).await; + let registry = Arc::new(freshell_ownership::RuntimeOwnershipRegistry::new()); + st.set_ownership(Arc::clone(®istry)); + + let (_p_keeps, keeps) = materialized_selfheal_session(&st, &mut rx, "req-keeps").await; + let (p_gone, gone) = materialized_selfheal_session(&st, &mut rx, "req-gone").await; + let (_p_term, term) = materialized_selfheal_session(&st, &mut rx, "req-term").await; + for durable in [&keeps, &gone, &term] { + await_freshagent_live(®istry, durable).await; + } + + // While the daemon is down (modeled pre-loss here), session B is + // retired from the map AND its key completes a terminal handoff (the + // concurrent-handoff shape); session C STAYS mapped but its key is + // terminal-owned (the committed-handoff window). + { + let mut map = st.sessions.lock().await; + map.remove(&p_gone); + map.remove(&gone); + } + commit_terminal_owner(®istry, &gone).await; + commit_terminal_owner(®istry, &term).await; + let _ = drain_frames(&mut rx); + + exited.store(true, Ordering::SeqCst); // the daemon dies (bridges die with it) + // Deterministic anchor: the fan-out reaches the LAST mapped session + // (BTreeSet order — keeps, then term), so term's edge proves the + // whole fan-out ran. + let loss_frames = frames_until(&mut rx, |f| { + f["type"] == "freshAgent.event" + && f["event"]["code"] == "OPENCODE_DAEMON_LOST" + && f["sessionId"].as_str() == Some(term.as_str()) + }) + .await; + + exited.store(false, Ordering::SeqCst); // the re-warm's respawn now succeeds + + // `Started` → the revival pass: keeps IS revived... + let revive_frames = frames_until(&mut rx, |f| { + is_event(f, "freshAgent.session.snapshot", Some("idle")) + && f["sessionId"].as_str() == Some(keeps.as_str()) + }) + .await; + + // ...and the pass has had every opportunity to (wrongly) touch the + // retired and terminal-owned sessions — settle, then audit the whole + // post-loss window. + tokio::time::sleep(Duration::from_millis(100)).await; + let mut all = loss_frames; + all.extend(revive_frames); + all.extend(drain_frames(&mut rx)); + + assert!( + session_serve_bridge_alive(&st, &keeps).await, + "the healthy fresh-agent session revives" + ); + assert!( + !session_serve_bridge_alive(&st, &gone).await, + "the retired session must NOT be revived" + ); + assert!( + !session_serve_bridge_alive(&st, &term).await, + "the terminal-owned session must NOT be revived" + ); + let gone_snapshots = all + .iter() + .filter(|f| { + is_event(f, "freshAgent.session.snapshot", None) + && f["sessionId"].as_str() == Some(gone.as_str()) + }) + .count(); + assert_eq!( + gone_snapshots, 0, + "no snapshot push for the retired session: {all:?}" + ); + let term_snapshots = all + .iter() + .filter(|f| { + is_event(f, "freshAgent.session.snapshot", None) + && f["sessionId"].as_str() == Some(term.as_str()) + }) + .count(); + assert_eq!( + term_snapshots, 0, + "no snapshot push for the terminal-owned session: {all:?}" + ); + } } From 24ef07889fd2063ea70c2b4565bcfab652775906 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:13:21 -0700 Subject: [PATCH 10/24] fix(fresh-agent): recover freshopencode panes from snapshot 409 via fenced attach and refetch --- AGENTS.md | 2 +- src/components/fresh-agent/FreshAgentView.tsx | 100 ++++++++++++- src/store/freshAgentSlice.ts | 28 ++++ .../fresh-agent/FreshAgentView.test.tsx | 132 ++++++++++++++++++ 4 files changed, 260 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fda7580e3..237143ef7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -260,7 +260,7 @@ live in [docs/development/gcloud-robot.md](docs/development/gcloud-robot.md). **Rename scope contract:** pane/tab labels are layout-local; only an explicit session rename writes a durable session title (terminal renames are terminal-scoped). The four name scopes and the reset-to-provider-title flow live in [docs/development/rename-scope-contract.md](docs/development/rename-scope-contract.md). -**Agent Status Indicators:** Blue/busy status is derived from provider activity slices through `resolvePaneActivity`; green/needs-attention and the idle sound flow through `recordTurnComplete` and `useTurnCompletionNotifications`. Turn-complete (green/sound) is server-authoritative everywhere: terminal CLIs via `terminal.turn.complete`, and fresh-agent panes (freshclaude/kilroy/freshcodex/freshopencode) via a discrete `freshAgent.turn.complete` edge emitted only on a positive completion — freshclaude/kilroy on the SDK `result` with `subtype === 'success'`, freshopencode on the success-only `emitStatus(idle)` path, and freshcodex on `turn/completed` only when `params.turn.status === 'completed'` (the notification also fires on interrupt). The client folds it in via `applyFreshAgentCompletion` using the `at`-monotonic dedupe regime (wall-clock `at`, no per-session counter, so a resumed durable session can't swallow completions across a server restart). The waiting-for-approval edge is ALSO server-authoritative: the Claude/kilroy `SdkBridge` emits a discrete `freshAgent.turn.waiting` edge on the 0→≥1 pending permission/question transition (only Claude/kilroy raise approvals/questions), and the client folds it in via `applyFreshAgentWaiting` under a distinct `${provider}:${sessionId}#waiting` dedupe namespace so it can never poison (or be poisoned by) the turn-complete bucket. The fragile client-side busy→idle derivation AND the client-side waiting-edge hook (`useAgentSessionTurnCompletion`) were both removed — all green/sound edges are now server-emitted. freshcodex additionally self-heals a crashed/disconnected codex sidecar by consuming the runtime `onExit` hook in `subscribe()`, emitting `sdk.status:'exited'` to clear BLUE (no chime — a crash is not a positive completion). freshcodex also runs a wedged-sidecar deadman: after a bounded quiet window (default 10 min, env `FRESHELL_FRESHCODEX_QUIET_WINDOW_MS`) with a turn in flight and no sidecar events, the server stops asserting busy and marks the pane `stuck`, and the client shows an amber "Agent appears stuck" card (`role="alert"`) with "Restart sidecar" (kill + resume re-mint) and "Start new conversation" actions; the deadman never fabricates a turn-complete (no green/chime). `freshopencode` still runs on a shared long-lived `opencode serve` sidecar and uses server-pushed `session.idle`/`session.status` events to drive busy, and the runtime self-heals a shared-daemon death (the 2026-09-20 incident class): daemon loss fans a typed `freshAgent.error{code:"OPENCODE_DAEMON_LOST"}` edge out to exactly one frame per materialized session (the client's generic `sessionError` banner + busy-clear; NO chime — a crash is never a positive completion), the manager respawns the daemon on a backoff ladder (fresh-incident reset, crash-loop escalation), every successful cold start drives a level-triggered revival pass that restarts dead session bridges and pushes `freshAgent.session.snapshot{status:"idle"}` (the client's transcript-refetch trigger) while respecting the ownership coordinator (retired, transitioned, or terminal-owned sessions are never revived), and a generation-fenced `freshAgent.attach` is itself a recovery verb that respawns the daemon before re-bridging. Gemini and Kimi terminal modes are status-in... [truncated] Separately, the sidebar shows cross-device remote status rings around a session row's icon: a green ring means the session is open on another device, a blue ring means it is busy on another device (blue wins over green), and rings are suppressed entirely when the session is open on this device (derived from `tabs.sync` registry snapshots — producing clients stamp pane payloads with `sessionKeys`/`busySessionKeys`, consumers re-query remote snapshots on a 30s interval, and the server partitions same-device records into `sameDeviceOpen`, which never produces rings). +**Agent Status Indicators:** Blue/busy status is derived from provider activity slices through `resolvePaneActivity`; green/needs-attention and the idle sound flow through `recordTurnComplete` and `useTurnCompletionNotifications`. Turn-complete (green/sound) is server-authoritative everywhere: terminal CLIs via `terminal.turn.complete`, and fresh-agent panes (freshclaude/kilroy/freshcodex/freshopencode) via a discrete `freshAgent.turn.complete` edge emitted only on a positive completion — freshclaude/kilroy on the SDK `result` with `subtype === 'success'`, freshopencode on the success-only `emitStatus(idle)` path, and freshcodex on `turn/completed` only when `params.turn.status === 'completed'` (the notification also fires on interrupt). The client folds it in via `applyFreshAgentCompletion` using the `at`-monotonic dedupe regime (wall-clock `at`, no per-session counter, so a resumed durable session can't swallow completions across a server restart). The waiting-for-approval edge is ALSO server-authoritative: the Claude/kilroy `SdkBridge` emits a discrete `freshAgent.turn.waiting` edge on the 0→≥1 pending permission/question transition (only Claude/kilroy raise approvals/questions), and the client folds it in via `applyFreshAgentWaiting` under a distinct `${provider}:${sessionId}#waiting` dedupe namespace so it can never poison (or be poisoned by) the turn-complete bucket. The fragile client-side busy→idle derivation AND the client-side waiting-edge hook (`useAgentSessionTurnCompletion`) were both removed — all green/sound edges are now server-emitted. freshcodex additionally self-heals a crashed/disconnected codex sidecar by consuming the runtime `onExit` hook in `subscribe()`, emitting `sdk.status:'exited'` to clear BLUE (no chime — a crash is not a positive completion). freshcodex also runs a wedged-sidecar deadman: after a bounded quiet window (default 10 min, env `FRESHELL_FRESHCODEX_QUIET_WINDOW_MS`) with a turn in flight and no sidecar events, the server stops asserting busy and marks the pane `stuck`, and the client shows an amber "Agent appears stuck" card (`role="alert"`) with "Restart sidecar" (kill + resume re-mint) and "Start new conversation" actions; the deadman never fabricates a turn-complete (no green/chime). `freshopencode` still runs on a shared long-lived `opencode serve` sidecar and uses server-pushed `session.idle`/`session.status` events to drive busy, and the runtime self-heals a shared-daemon death (the 2026-09-20 incident class): daemon loss fans a typed `freshAgent.error{code:"OPENCODE_DAEMON_LOST"}` edge out to exactly one frame per materialized session (the client's generic `sessionError` banner + busy-clear; NO chime — a crash is never a positive completion), the manager respawns the daemon on a backoff ladder (fresh-incident reset, crash-loop escalation), every successful cold start drives a level-triggered revival pass that restarts dead session bridges and pushes `freshAgent.session.snapshot{status:"idle"}` (the client's transcript-refetch trigger) while respecting the ownership coordinator (retired, transitioned, or terminal-owned sessions are never revived), and a generation-fenced `freshAgent.attach` is itself a recovery verb that respawns the daemon before re-bridging. Client-side, a snapshot GET that still answers the typed 409 `RESTORE_UNAVAILABLE` for the pane's own stale fresh-agent claim (e.g. the pane loaded while the server was restarting) does not dead-end on the dismiss-only banner: the pane drives the documented recovery ONCE per pane identity — it refreshes the observed owner fence from the refusal's own `ownerGeneration` (preserving the record's epoch), sends one generation-fenced `freshAgent.attach`, and refetches through the reveal path when reveal-dirty (so the "Refreshing conversation" overlay can clear) or via `manual` otherwise; a suppressed attach restores the once-guard, and repeated 409s fall through to the honest error banner — terminal-owned refusals stay out of this path (their recovery door is the session-directory handoff). Gemini and Kimi terminal modes are status-in... [truncated] Separately, the sidebar shows cross-device remote status rings around a session row's icon: a green ring means the session is open on another device, a blue ring means it is busy on another device (blue wins over green), and rings are suppressed entirely when the session is open on this device (derived from `tabs.sync` registry snapshots — producing clients stamp pane payloads with `sessionKeys`/`busySessionKeys`, consumers re-query remote snapshots on a 30s interval, and the server partitions same-device records into `sameDeviceOpen`, which never produces rings). **Fresh-Agent Orchestration:** The Rust REST agent API (`/api/tabs`, `/api/panes/:id/split`, `/api/panes/:id/send-keys`, `/api/panes/:id/capture`, `/api/panes/:id/wait-for`) and the standalone Node MCP client accept `agent`/`model`/`effort` parameters where the Rust contract supports them. The Rust orchestration layer dispatches to the registered fresh-agent runtimes. On MCP `new-tab`, resume sugar (`resume`/`resumeSessionId`) is honored for `agent: "opencode"`; terminal-mode resume uses an explicit provider-matched `sessionRef` (raw Codex resume IDs are rejected because they are not sufficient restore identity). Unsupported legacy actions return a deterministic unavailable result instead of contacting a removed backend route. diff --git a/src/components/fresh-agent/FreshAgentView.tsx b/src/components/fresh-agent/FreshAgentView.tsx index 333db7440..afc110e6e 100644 --- a/src/components/fresh-agent/FreshAgentView.tsx +++ b/src/components/fresh-agent/FreshAgentView.tsx @@ -21,7 +21,7 @@ import { createLogger } from '@/lib/client-logger' import { api, getFreshAgentModelCapabilities, getFreshAgentThreadSnapshot, setSessionMetadata } from '@/lib/api' import { clearReconcilePendingPane, consumePaneRefreshRequest, mergePaneContent, updatePaneContent } from '@/store/panesSlice' import { FRESH_AGENT_MODEL_CATALOG_UNAVAILABLE_NOTICE } from '@/lib/fresh-agent-model-capabilities' -import { clearPendingCreateFailure, clearRestoreFailure, clearSessionError, clearSessionLost, sessionError, setSessionStatus } from '@/store/freshAgentSlice' +import { applyRefusalFence, clearPendingCreateFailure, clearRestoreFailure, clearSessionError, clearSessionLost, sessionError, setSessionStatus } from '@/store/freshAgentSlice' import { openSessionTab } from '@/store/tabsSlice' import { buildReconcileRequestForPanes, foldVerdicts, isFreshAgentReconcileActive } from '@/lib/pane-reconcile' import { dismissTabGreen } from '@/store/turnCompletionAttention' @@ -462,6 +462,37 @@ function isLostFreshOpencodeThreadError(error: unknown): boolean { return status === 404 && code === 'FRESH_AGENT_LOST_SESSION' } +// LB-05 scoping: fresh-agent owners are the 2026-09-20 incident class (the +// pane's OWN stale-claim refusal — the daemon died while the session key +// stayed Live{FreshAgent}) and the fenced attach proceeds for them. Terminal +// owners are a different scenario (a genuinely terminal-owned session); their +// recovery door is the session-directory handoff, so the client does not +// attempt the (refused) attach for them. +function isRestoreUnavailableSnapshotError(error: unknown): boolean { + if (!error || typeof error !== 'object') return false + const status = 'status' in error ? (error as { status?: unknown }).status : undefined + const details = 'details' in error ? (error as { details?: unknown }).details : undefined + const code = details && typeof details === 'object' && 'code' in details + ? (details as { code?: unknown }).code + : undefined + const ownerKind = details && typeof details === 'object' && 'ownerKind' in details + ? (details as { ownerKind?: unknown }).ownerKind + : undefined + return status === 409 && code === 'RESTORE_UNAVAILABLE' && ownerKind === 'fresh-agent' +} + +// The 409 refusal always names its fence-relevant generation; a malformed +// envelope without one cannot fence the recovery attach, so the caller skips +// the recovery (the honest error surfaces below take it) instead of sending an +// attach bound to a stale or absent generation. +function readRestoreRefusalOwnerGeneration(error: unknown): number | undefined { + if (!error || typeof error !== 'object' || !('details' in error)) return undefined + const details = (error as { details?: unknown }).details + if (!details || typeof details !== 'object' || !('ownerGeneration' in details)) return undefined + const ownerGeneration = (details as { ownerGeneration?: unknown }).ownerGeneration + return typeof ownerGeneration === 'number' ? ownerGeneration : undefined +} + function getRestoreErrorMessage(reason: RestoreErrorReason): string { switch (reason) { case 'invalid_legacy_restore_target': @@ -802,6 +833,13 @@ export function FreshAgentView({ const revealRefreshRetryTimerRef = useRef(null) const snapshotRefreshSerialRef = useRef(0) const [snapshotRevealError, setSnapshotRevealError] = useState(null) + // 2026-09-20 incident (Task 5): the once-per-identity 409 RESTORE_UNAVAILABLE + // recovery latch. The ENTIRE recovery (fenced attach + refetch) runs at most + // once per pane identity (`${createRequestId}:${snapshotThreadId}`); a second + // 409 falls through to the honest error surfaces, never re-fetching. A + // suppressed attach restores the previous value so it does NOT consume the + // latch. + const restoreUnavailableRecoveryRef = useRef(null) // Non-null while the snapshot key is rate-limited (429/backoff): the last // good snapshot stays visible and a single retry is armed at expiry. // Task 17 also consumes this for the snapshot `trigger` query param. @@ -2818,6 +2856,64 @@ export function FreshAgentView({ })) return } + // 2026-09-20 incident: with the daemon dead, daemon-absent snapshot GETs + // answer the typed 409 RESTORE_UNAVAILABLE for as long as the session + // key stays Live{FreshAgent}. The documented recovery is the + // generation-fenced attach (a map-hit freshAgent.attach respawns the + // daemon and re-bridges server-side) — drive it ONCE per pane identity, + // then refetch. Repeated 409s fall through to the honest error surfaces + // below; never reset the pane (that is the 404 lost-thread arm above). + if (paneContent.provider === 'opencode' && isRestoreUnavailableSnapshotError(error)) { + const fresh = paneContentRef.current + const recoveryKey = `${fresh.createRequestId}:${sessionId}` + const refusalOwnerGeneration = readRestoreRefusalOwnerGeneration(error) + if ( + refusalOwnerGeneration !== undefined + && restoreUnavailableRecoveryRef.current !== recoveryKey + ) { + const previousRecoveryKey = restoreUnavailableRecoveryRef.current + restoreUnavailableRecoveryRef.current = recoveryKey + // Bind the fence to the 409's CURRENT generation — refresh the + // observed owner fence from the refusal itself (the refusal names + // the coordinator's live generation; the record's epoch is + // preserved). Without this, a stale owner record sends a + // stale-generation attach the wired server refuses with + // FENCE_REQUIRED — preserving the dead-end. + dispatch(applyRefusalFence({ + provider: fresh.provider, + sessionId, + ownerKind: 'fresh-agent', + ownerGeneration: refusalOwnerGeneration, + })) + attachDecisionSerialRef.current += 1 + const attempt = captureFreshAgentAttachmentAttempt(fresh) + if (sendFencedFreshAgentAttach(attempt)) { + // LB-04: a reveal-lane 409 with snapshotDirty set must refetch + // through the reveal path ('reveal' trigger), or the success-path + // reveal-dirty clear never runs and the pane hides behind the + // "Refreshing conversation" overlay forever. Otherwise refetch + // via 'manual'. + if (trigger === 'reveal' && snapshotDirtyRef.current) { + revealRefreshStartedAtRef.current = null + setSnapshotRevealError(null) + requestRevealRefresh(true) + } else { + setLoadError(null) + requestSnapshotRefresh('manual') + } + return + } + // The attach was suppressed (lifecycle superseded or attempt-key + // mismatch). Do NOT consume the one-shot recovery and do NOT + // refetch — restore the latch and fall through to the honest error + // surfaces below. + restoreUnavailableRecoveryRef.current = previousRecoveryKey + } + // Recovery already attempted for this identity (or the refusal + // carried no fenceable generation): do NOT clear errors and do NOT + // refetch again — fall through to the reveal error arm / + // setLoadError below so the user sees the honest state. + } if (trigger === 'reveal' && snapshotDirtyRef.current) { revealRefreshStartedAtRef.current = null setSnapshotRevealError(error instanceof Error ? error.message : 'Failed to refresh conversation') @@ -2879,6 +2975,7 @@ export function FreshAgentView({ // paneContentRef.current inside the effect. }, [ agentSession?.lost, + captureFreshAgentAttachmentAttempt, claudeSession, isRestoring, dispatch, @@ -2891,6 +2988,7 @@ export function FreshAgentView({ migratePendingAutoTitle, requestRevealRefresh, requestSnapshotRefresh, + sendFencedFreshAgentAttach, setLocalEcho, snapshotThreadId, snapshotRefreshNonce, diff --git a/src/store/freshAgentSlice.ts b/src/store/freshAgentSlice.ts index 9e2792e69..620846216 100644 --- a/src/store/freshAgentSlice.ts +++ b/src/store/freshAgentSlice.ts @@ -694,6 +694,33 @@ const freshAgentSlice = createSlice({ } }, + /** + * 2026-09-20 incident (Task 5): refresh the observed owner fence from a + * typed refusal itself — the REST snapshot 409 RESTORE_UNAVAILABLE always + * names the coordinator's CURRENT generation, which may be newer than + * the client's record (the pane loaded while the server was restarting + * and the record fold lagged). The refusal carries no epoch, so the + * existing record's epoch is PRESERVED (the record's epoch comes from + * the server's own runtime-owner broadcasts); only the generation + * advances. An absent record is left absent: minting one would fabricate + * an epoch the client never observed — the recovery attach then goes out + * unfenced, the wired server refuses it typed, and the pane surfaces + * that honestly instead of the store lying about the boot epoch. + */ + applyRefusalFence(state, action: PayloadAction<{ + provider: string + sessionId: string + ownerKind: 'terminal' | 'fresh-agent' + ownerGeneration: number + }>) { + const refusal = action.payload + const key = `${refusal.provider}:${refusal.sessionId}` + const existing = state.runtimeOwners[key] + if (!existing) return + existing.generation = refusal.ownerGeneration + existing.updatedAt = Date.now() + }, + /** * kata b8ke (round-2 review): the ready handler dispatches this BEFORE * folding the ready.runtimeOwners replay — the client resets its @@ -714,6 +741,7 @@ export const { addUserMessage, appendStreamDelta, applyRuntimeOwner, + applyRefusalFence, clearPendingCreate, clearPendingCreateFailure, clearPendingCreateFailureForSession, diff --git a/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx b/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx index 541d36b77..10761dd2b 100644 --- a/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx +++ b/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx @@ -1929,6 +1929,138 @@ describe('FreshAgentView', () => { expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(1) }) + // 2026-09-20 incident (log-validated): the daemon died, the snapshot GET + // answered the typed 409 RESTORE_UNAVAILABLE for the pane's OWN stale + // Live{FreshAgent, gen 1} claim, and the pane dead-ended on a dismiss-only + // banner forever. The documented recovery is the generation-fenced attach + + // refetch — drive it once. + // LB-09: the mount attach already sends ONE freshAgent.attach on mount, so a + // bare length assertion is vacuous — read the baseline AFTER the mount + // settles and assert the POST-409 delta. + it('recovers a freshopencode pane from a snapshot 409 with one fenced attach and a refetch', async () => { + const store = createStore() + // Seed the runtime-owner record and make the 409 name a NEWER generation — + // the recovery attach MUST carry the 409's generation (fence bound to the + // refusal, not the possibly-stale record), or the wired server refuses it + // with FENCE_REQUIRED and the dead-end persists. + store.dispatch(applyRuntimeOwner({ + type: 'session.runtimeOwner', + provider: 'opencode', + sessionId: 'ses_live', + epoch: 1, + generation: 1, + ownerKind: 'fresh-agent', + operationId: 'incident-live-claim', + transition: 'handoff-committed', + })) + // DEFER the first rejection until after the baseline is read — an + // immediately-rejected mock races the mount fetch (the recovery attach may + // land before the test snapshots the count). + let rejectFirstSnapshot!: (error: unknown) => void + apiMock.getFreshAgentThreadSnapshot + .mockImplementationOnce(() => new Promise((_, reject) => { + rejectFirstSnapshot = reject + })) + .mockResolvedValue({ + ...freshopencodeSnapshot('recovered transcript', 7), + threadId: 'ses_live', + sessionId: 'ses_live', + }) + store.dispatch(initLayout({ + tabId: 'tab-1', + paneId: 'pane-1', + content: { + kind: 'fresh-agent', + sessionType: 'freshopencode', + provider: 'opencode', + createRequestId: 'req-live-409', + sessionId: 'ses_live', + sessionRef: { provider: 'opencode', sessionId: 'ses_live' }, + status: 'connected', + }, + })) + + render( + + + , + ) + await waitFor(() => expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(1)) + // The mount attach, settled (LB-09 baseline). + const attachCountBeforeRecovery = sentFreshAgentMessages('freshAgent.attach').length + expect(attachCountBeforeRecovery).toBe(1) + await act(async () => { + rejectFirstSnapshot(new ApiError(409, 'Session ses_live is still running on the server.', { + code: 'RESTORE_UNAVAILABLE', + ownerKind: 'fresh-agent', + ownerGeneration: 2, + })) + }) + await waitFor(() => { + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(attachCountBeforeRecovery + 1) + const recoveryAttach = sentFreshAgentMessages('freshAgent.attach').at(-1) + expect(recoveryAttach?.observedEpoch).toBe(1) // the record's epoch + expect(recoveryAttach?.observedGeneration).toBe(2) // the 409's CURRENT generation, not the stale record's 1 + }) + await waitFor(() => { + expect(apiMock.getFreshAgentThreadSnapshot).toHaveBeenCalledTimes(2) // exactly one recovery refetch + }) + // The pane kept its identity (the 409 is NOT the 404 lost-thread reset): + expect(getFreshAgentPaneContent(store).sessionId).toBe('ses_live') + expect(getFreshAgentPaneContent(store).createRequestId).toBe('req-live-409') + // And no dead-end banner for the recovered pane: + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()) + }) + + it('does not loop recovery fetches on repeated 409s', async () => { + vi.useFakeTimers() + try { + const store = createStore() + // Every GET rejects with the same real ApiError (an Error instance) so + // handleSnapshotError preserves the 409's own message on the banner. + apiMock.getFreshAgentThreadSnapshot.mockRejectedValue(new ApiError(409, 'Session ses_live is still running on the server.', { + code: 'RESTORE_UNAVAILABLE', + ownerKind: 'fresh-agent', + ownerGeneration: 2, + })) + store.dispatch(initLayout({ + tabId: 'tab-1', + paneId: 'pane-1', + content: { + kind: 'fresh-agent', + sessionType: 'freshopencode', + provider: 'opencode', + createRequestId: 'req-live-409-loop', + sessionId: 'ses_live', + sessionRef: { provider: 'opencode', sessionId: 'ses_live' }, + status: 'connected', + }, + })) + render( + + + , + ) + // Settle the mount fetch and the single recovery refetch (the second 409 + // falls through to the honest banner — the recovery guard already + // consumed this pane identity). Advance the fake clock deterministically + // (the wall-clock debounce races under parallel suites — the sibling + // scheduler tests' note). + await act(async () => { await vi.advanceTimersByTimeAsync(0) }) + await act(async () => { await vi.advanceTimersByTimeAsync(SNAPSHOT_DEBOUNCE_MS) }) + await act(async () => { await vi.advanceTimersByTimeAsync(0) }) + const baseline = sentFreshAgentMessages('freshAgent.attach').length + expect(screen.getByText(/still running on the server/i)).toBeInTheDocument() + await act(async () => { await vi.advanceTimersByTimeAsync(2_000) }) + expect(sentFreshAgentMessages('freshAgent.attach')).toHaveLength(baseline) // one recovery total, not per fetch (LB-03) + expect(apiMock.getFreshAgentThreadSnapshot.mock.calls.length).toBeLessThanOrEqual(3) // mount + recovery only — no loop + } finally { + cleanup() + resetSnapshotSchedulerForTests() + vi.useRealTimers() + } + }) + it('attaches materialized FreshOpenCode panes with durable route metadata on mount and reconnect', async () => { const store = createStore() let reconnectHandler: (() => void) | undefined From 4fad5dd8d8941bbbd88658a90a9e1da9e390b9a3 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:45:48 -0700 Subject: [PATCH 11/24] fix(fresh-agent): monotonic refusal fence, canonical fold key, reveal-lane coverage --- src/components/fresh-agent/FreshAgentView.tsx | 11 +- src/store/freshAgentSlice.ts | 10 +- .../fresh-agent/FreshAgentView.test.tsx | 224 +++++++++++++++++- .../freshAgentSlice.runtime-owner.test.ts | 24 ++ 4 files changed, 263 insertions(+), 6 deletions(-) diff --git a/src/components/fresh-agent/FreshAgentView.tsx b/src/components/fresh-agent/FreshAgentView.tsx index afc110e6e..7fafe1d72 100644 --- a/src/components/fresh-agent/FreshAgentView.tsx +++ b/src/components/fresh-agent/FreshAgentView.tsx @@ -2878,10 +2878,15 @@ export function FreshAgentView({ // the coordinator's live generation; the record's epoch is // preserved). Without this, a stale owner record sends a // stale-generation attach the wired server refuses with - // FENCE_REQUIRED — preserving the dead-end. + // FENCE_REQUIRED — preserving the dead-end. The fold keys the + // CANONICAL session (Task 5 review M2): the recovery attach's + // fence read resolves the stored aliasOf chain, so a pane holding + // a superseded id must fold onto the same record the attach reads + // — the raw pane id would land on the inert alias mirror. + const canonicalSession = resolveCanonicalPaneSession(appStore.getState(), fresh) dispatch(applyRefusalFence({ - provider: fresh.provider, - sessionId, + provider: canonicalSession?.provider ?? fresh.provider, + sessionId: canonicalSession?.sessionId ?? sessionId, ownerKind: 'fresh-agent', ownerGeneration: refusalOwnerGeneration, })) diff --git a/src/store/freshAgentSlice.ts b/src/store/freshAgentSlice.ts index 620846216..20c194b6f 100644 --- a/src/store/freshAgentSlice.ts +++ b/src/store/freshAgentSlice.ts @@ -702,8 +702,13 @@ const freshAgentSlice = createSlice({ * and the record fold lagged). The refusal carries no epoch, so the * existing record's epoch is PRESERVED (the record's epoch comes from * the server's own runtime-owner broadcasts); only the generation - * advances. An absent record is left absent: minting one would fabricate - * an epoch the client never observed — the recovery attach then goes out + * advances. The fold is ADVANCE-ONLY (Task 5 review M1), matching the + * applyRuntimeOwner invariant: a newer broadcast (e.g. gen 3) may fold + * between the server minting the refusal (gen 2) and the client + * processing it — regressing to the refusal's older generation would + * send a stale fence the wired server refuses with FENCE_REQUIRED. An + * absent record is left absent: minting one would fabricate an epoch + * the client never observed — the recovery attach then goes out * unfenced, the wired server refuses it typed, and the pane surfaces * that honestly instead of the store lying about the boot epoch. */ @@ -717,6 +722,7 @@ const freshAgentSlice = createSlice({ const key = `${refusal.provider}:${refusal.sessionId}` const existing = state.runtimeOwners[key] if (!existing) return + if (refusal.ownerGeneration < existing.generation) return existing.generation = refusal.ownerGeneration existing.updatedAt = Date.now() }, diff --git a/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx b/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx index 10761dd2b..205069b60 100644 --- a/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx +++ b/test/unit/client/components/fresh-agent/FreshAgentView.test.tsx @@ -168,9 +168,11 @@ function createStore(tabTitleSetByUser = false, extraMiddleware: Middleware[] = function StoreBackedFreshAgentView({ tabId, paneId, + hidden = false, }: { tabId: string paneId: string + hidden?: boolean }) { const paneContent = useAppSelector((state) => { const layout = state.panes.layouts[tabId] @@ -179,7 +181,7 @@ function StoreBackedFreshAgentView({ } return layout.content }) - return + return