fix: supervise local agent sessions across desktop crashes - #239
Conversation
Signed-off-by: Mongo <81cabd2ca1792494372ed410283c00770c0cf35afca052e940fa522883212b45@buzz.block.builderlab.xyz>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Three cleanup/error-reporting issues below. Desktop crash/relaunch with the packaged runtime remains unvalidated; no approval submitted.
| // timeout drops the channel and lets the guardian own cleanup. | ||
| if state == *b"O" || state == *b"X" { | ||
| let _ = child.wait(); | ||
| } |
There was a problem hiding this comment.
🤖 [P3] Reap the guardian on a failed startup handshake
When serve reports E after a non-contention ownership-acquisition failure or a read-timeout setup failure, this path drops Child without waiting; only O and X reap it. The exited guardian therefore remains a zombie until the desktop exits, accumulating across repeated failed Starts. Reap the guardian on E as well, and preserve explicit child ownership/cleanup on handshake read failures.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in 7692ffb9330bd5c4943264e779815f2857f6b04f. Successfully read E/O/X startup replies synchronously reap the child; uncertain reads and timeout-setup failure close the socket and transfer the Child to an asynchronous waiter. The guardian alone retains lock/session/temp cleanup authority. confirmed_failure_waits_and_uncertain_handshake_reaps_later covers terminal replies, socket EOF, nonblocking uncertain return, and eventual child reaping; the existing missing-executable lock/temp cleanup regression also passes.
| saved?; | ||
| Ok(()) | ||
| self.store.enabled(id, false)?; | ||
| stopped |
There was a problem hiding this comment.
🤖 [P3] Preserve the cleanup error when disabling also fails
If self.stop(id) fails with ownership still held and store.enabled(id, false) also fails, this ordering returns the store error and discards the cleanup error. The user loses the diagnostic explaining why the session remains owned. Both operations were already attempted with the previous ordering; preserve that behavior but propagate stopped before saved.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in 7692ffb9330bd5c4943264e779815f2857f6b04f. Stop and durable disable are both attempted, with the cleanup result taking precedence and recorded before any persistence-error return. stop_reports_cleanup_before_durable_disable_failure pins both-fail precedence and retained diagnostic. The existing malformed-store/vanished-row regression still passes, so a failed disable cannot become an empty successful snapshot.
| ); | ||
| self.errors | ||
| .entry(agent.id.clone()) | ||
| .or_insert_with(|| "Agent listener exited; restart to retry".into()); |
There was a problem hiding this comment.
🤖 [P3] Replace unrelated stale errors when the listener exits
A failed Restart credential read can call record_error while the existing listener is still running. If that listener subsequently exits, or_insert_with retains the old credential error instead of reporting the listener exit. Preserve the supervisor cleanup error in the corresponding Err branch, removing the running entry when stop is confirmed, and keep ordinary listener-exit handling as a replacing insert.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in 7692ffb9330bd5c4943264e779815f2857f6b04f. Ordinary listener exit replaces unrelated stale errors. Confirmed cleanup-error completion retires the running entry in Stop or snapshot while retaining its specific diagnostic; F/timeout retain the entry. listener_exit_replaces_stale_credential_error and listener_self_exit_with_failed_cleanup_retires_entry_and_reports_error cover both paths. Existing Stop/native Restart error-preservation and explicit-retry coverage remains green.
Signed-off-by: Mongo <81cabd2ca1792494372ed410283c00770c0cf35afca052e940fa522883212b45@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking code findings. Reviewed head 7692ffb9330bd5c4943264e779815f2857f6b04f against base 1383b39747bc5fbd680f6d46c0ba2113c1521d97, including independent process-lifetime and controller-state lanes. The prior three comments are addressed. One nonblocking P3 on confirmed-cleanup Quit behavior is inline. This is a COMMENT review, not approval.
Validation: source review plus verified hosted Rust/tool results on PR merge commit 120eab205c0252bda45c5b4015556c6ebe729c3d: agent-controller 51 passed/1 ignored; foundation 63 passed/3 ignored. I did not rerun local suites or the author’s production-dispatch probe. Packaged GUI/live-provider acceptance and actual OS cleanup failure remain unverified by this review.
Merge gates are separate: DCO passed, but JavaScript CI failed ProfileAgentIdentity.test.tsx:202 (expected one query, observed two); WebKit shards were still running at inspection. That test is outside this native-only diff; its cause was not investigated. Required CI must be resolved before merge.
| #[cfg(unix)] | ||
| if run.process.stopped() { | ||
| // E confirms worker exit even when private-dir removal failed. | ||
| self.running.remove(id); |
There was a problem hiding this comment.
[P3, nonblocking] Avoid silently rejecting Quit after confirmed process exit
When the supervisor returns E, this branch correctly retires the stopped run, but result? still sends the directory-removal error through Controller::shutdown → AgentHost::shutdown. src-tauri/src/lib.rs:410–415 treats every such error as unconfirmed execution, prevents exit, and only writes stderr. Thus a private-directory removal failure makes the first Quit silently do nothing even though all execution has stopped; a second Quit succeeds because the entry is gone.
Consider distinguishing confirmed-stop cleanup warnings from unconfirmed teardown at the Quit boundary, allowing exit for the former while preserving fail-closed behavior for the latter. Add coverage for Quit receiving E. This is a source-traced recovery UX issue, not a containment blocker.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Request changes: the new handshake test failed in 1 of 6 full-package runs; please make its EOF assertion deterministic and retry interrupted supervisor reads. Findings apply to 7692ffb.
Separately, EOF-only app-death detection deserves targeted validation: an inherited peer descriptor could delay cleanup after an app crash. Descriptor inheritance and the production outcome remain unproven; this is not a confirmed defect. Packaged desktop crash/relaunch and successful relay reconnect remain untested.
| assert_eq!(unsafe { libc::kill(pid, 0) }, 0); | ||
| guardian.set_nonblocking(true).unwrap(); | ||
| let mut byte = [0]; | ||
| assert_eq!(guardian.read(&mut byte).unwrap(), 0); // socket closed |
There was a problem hiding this comment.
🤖 [P2] Wait for EOF instead of assuming immediate completion
The nonblocking read immediately after dropping the peer can return WouldBlock. At this commit, bin/cargo test -p buzz-agent-controller failed here in 1 of 6 full-suite runs (os error 35); the other five passed. Ten isolated runs passed, so isolated or serialized success does not establish determinism. This makes CI flaky. Use a deadline-bounded EOF wait or a blocking read with a timeout. The reason another peer reference remained open was not established.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in ae327da297a3dd926e6f9365096814bdd92ccb12. The test no longer assumes EOF is immediate: it waits for the observable EOF within a bounded deadline, retries WouldBlock, and still fails on timeout or unexpected input/errors. A deliberately retained peer clone proves the pre-EOF WouldBlock state before release. A panic guard prevents an assertion failure from leaving the sleep fixture running; asynchronous child reaping remains asserted.
Full bin/cargo test -p buzz-agent-controller: 50 passed, 1 existing ignored, on the final working-tree bytes verified against this commit. Independent bounded source review clear; formatting and diff checks passed.
This fixes the test's scheduling assumption; it does not establish the cause of the previously reported transient peer reference or claim packaged-GUI validation.
| } | ||
| } | ||
| } | ||
| Err(_) => break, |
There was a problem hiding this comment.
🤖 [P3] Retry interrupted reads rather than treating them as app death
This catch-all error branch also handles ErrorKind::Interrupted. On Linux, a timed socket read can return EINTR after process stop/resume, causing this branch to tear down a healthy listener and require a restart. Add Interrupted to the retry arm at lines 93–96. This is source-supported, not runtime-reproduced on Linux.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in ae327da297a3dd926e6f9365096814bdd92ccb12. ErrorKind::Interrupted now joins the existing timeout/WouldBlock retry arm, including the existing listener liveness check, rather than falling through to session teardown. EOF and explicit Stop retain their existing behavior. No new lifecycle owner or retry mechanism.
Full bin/cargo test -p buzz-agent-controller: 50 passed, 1 existing ignored, on the final working-tree bytes verified against this commit. Independent bounded source review clear; formatting and diff checks passed.. Linux signal-stop/resume is not newly runtime-reproduced locally; fresh merged-head CI remains separate.
| Ok(lock) => lock, | ||
| Err(error) => { | ||
| let _ = std::fs::remove_dir_all(temp); | ||
| let _ = socket.write_all(if error.starts_with("Another buzz-app profile") { |
There was a problem hiding this comment.
🤖 [P3] Preserve typed startup failure categories and actionable diagnostics
The O/E classification depends on the ownership error text prefix, so rewording an error can silently change the protocol response. Distinct ownership and listener-spawn failures also collapse into the generic message at lines 161–165, losing the previous diagnostic specificity. Preserve typed failure categories and their actionable messages rather than classifying display text. Earlier executable validation limits the spawn-failure impact.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Valid diagnostic/maintainability follow-up, deferred from this bounded repair rather than claimed fixed. Ownership currently returns one fixed contention literal, and runtime executable preflight remains in place. The generic fallback does lose useful startup detail, but it does not change containment or failure reporting into success. This revision addresses the reproduced EOF-test failure and Interrupted shutdown path without expanding the handshake protocol or ownership error types. Independent review agrees this P3 is nonblocking for those corrections.
Signed-off-by: Mongo <81cabd2ca1792494372ed410283c00770c0cf35afca052e940fa522883212b45@buzz.block.builderlab.xyz>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Approved. The interrupted-read and handshake EOF fixes address the reviewed blockers. The existing startup-diagnostic issue and unverified packaged crash/relaunch and relay-reconnect coverage remain.
Summary
Prevent duplicate app-owned agent execution after abrupt app exit on Unix.
The app previously owned the lock while launching a detached listener. SIGKILL released the lock without stopping that listener or its workers, so relaunch could execute the same identity twice. A new same-binary supervisor is the smallest separate lifetime owner that survives the app without adding a sidecar packaging pipeline or changing the pinned ACP.
Validation
The first review-comment repair was validated at
b085478396f9b36bd6b72791750696d5a0e5466dplus its three-file working diff, then verified byte-for-byte against commit7692ffb9330bd5c4943264e779815f2857f6b04f(supervisor SHA-25607a187152a5c0662a136eefecc52febaf3dc8913735782fb7a378c98ef394a29). Local results do not certify integration with newermaincommits; fresh PR CI remains the merged-tree gate.bin/cargo test -p buzz-agent-controller: 50 passed, 1 existing staged-runtime test ignored; includes gated crash, missing-listener startup abort, listener self-exit, Stop/native Restart directory-cleanup errors and explicit retry, sticky failed-cleanup protocol and eight-second Stop timeout recovery. Four added regressions cover confirmed/uncertain startup child reaping, cleanup-versus-disable failure precedence, stale-error replacement, and listener self-exit with private-directory cleanup failure.bin/cargo test -p buzz-foundation: 40 passed, 2 existing staged-runtime tests ignored. Native binary check, formatting, Clippy-D warningsand diff check passed.Production-dispatch integration uses the real pinned ACP revision
48884848f566d02c42ce07636636c4ad5f164c27, a loopback relay, synthetic signing identity and fake ACP worker. A test-only shell shim rewrites the relay scheme from wss to ws and execs ACP without changing its PID/session. This is not an unchanged shipped bundle or full Tauri GUI acceptance run.The probe asserts active-turn Stop, parent SIGKILL with a frozen supervisor, immediate competing Start rejection, complete old session/temp cleanup, explicit retry and a new live turn. It verifies the worker has a separate process group in the listener session and leaves no recorded fixture process alive.
The timeout/F tests inject a protocol peer; they do not force real OS cleanup failure. Death during the readiness handshake and listener self-exit with a persistent separate-group worker are source-reviewed combinations of the tested paths, not individually executed scenarios. Full packaged GUI, live-provider execution and non-macOS behavior were not exercised locally.
No browser tests added or removed. Broad platform checks and the merged native dependency graph are left to existing CI.
Limits and remaining gates
At
7692ffb9, Rust/tool integration, every browser shard and DCO passed. JavaScript failedProfileAgentIdentity.test.tsx:202(one expected query, two observed), matching a failure on main before commit119195ea/ #234. That main commit corrected the test to count profile reads; its JavaScript job passed. The new repair push receives fresh merged-tree CI including that main-side correction, without adding frontend changes to this PR. The earlier WebKit anchor failure is not the current-head failure.Bounded follow-up review repair:
ae327da297a3dd926e6f9365096814bdd92ccb12ErrorKind::Interruptedin the supervisor's existing timeout/liveness-check arm rather than interpreting interruption as app death.d8699d58d7267eb80514ca7bbfebf0ffb44929739468c353454874ddc547f586. Full controller package: 50 passed, 1 existing ignored; doc-tests 0. The committed bytes match the checked working-tree repair. Independent narrow source review found no blocker. Linux signal-stop/resume and packaged-GUI behavior were not newly exercised; the earlier production-dispatch probe is historical evidence, not rerun evidence for this commit.Merge still requires current-head CI/DCO and code-owner approval. No merge performed.
Origin
Buzz channel:
45c5ed4c-7fe8-470b-9abf-af2ad9bed50fBuzz thread:
03dbe0ed545579ec25f57f102fdfb6124009cd5e1cd07a1467c566c37be16b39Originating conversation