fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns - #6904
Open
wpfleger96 wants to merge 4 commits into
Open
fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns#6904wpfleger96 wants to merge 4 commits into
wpfleger96 wants to merge 4 commits into
Conversation
…hell spawns ## Why PR #6330 split harness/runtime detection into a cheap (cache-only) path and a forced (spawning) path. Two regressions followed, both reported as every harness showing "(not installed)" across the create/edit picker, Agents > Agent defaults, and Settings > Agents — blocking agent save until the user clicked Install: - The resolve cache is in-memory, so it starts cold every launch. The cheap path consulted only the managed-shim dirs plus that cold cache, and the managed-shim allowlist structurally excludes `buzz-agent`. The bundled sidecar could therefore never resolve on the cheap path until a forced pass warmed the cache, so cheap-path surfaces rendered all-missing at boot. - `run_in_login_shell` used an untimeouted `Command::output()`; a wedged login shell froze the whole forced pipeline, leaving "Check again" spinning forever. ## What - `resolve_command_cached` now also calls `resolve_workspace_command`, resolving the bundled sidecar via a filesystem stat (no spawn) — the same class of work the managed-shim check already performs. `buzz-agent` can no longer show missing, even during the boot warm window. - Extract `output_with_timeout`, a shared piped/background-drained/SIGTERM-on- deadline spawn helper; refactor `probe_auth_status` onto it. - Bound each login-shell candidate with a 10s timeout via that helper, falling through to the next candidate on timeout instead of aborting the resolve. - Warm the ACP runtime catalog once at AppShell mount so the shared React Query cache is populated before the cheap-path surfaces render. Regression tests cover cold-cache sidecar resolution and the timeout helper's success and kill-on-deadline paths. Origin: Buzz channel harness-detection thread (#5ef5d5bb-643f-4b87-bbf4-e8b64585ffeb). Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
…ildren ## Why Review found `output_with_timeout` could still hang indefinitely: it sent only `SIGTERM` to the direct child (no-op on Windows, ignorable on Unix) and then unconditionally joined the wait thread and both pipe-drain threads. A child that traps `SIGTERM`, or a forked descendant that keeps the stdout/stderr pipes open after the direct child exits, left a join blocked forever — recreating the "Check again" spins-forever failure this work removes. ## What Extract the helper into `discovery/bounded_command.rs` and rebuild it on the repo's existing bounded-process pattern (the codex `--version` probe): - Capture stdout/stderr to regular temp files instead of pipes. A regular file returns EOF at its write position regardless of who inherits the descriptor, so a pipe-retaining descendant can no longer block the post-exit read. There are no drain threads to join. - Poll the child with `try_wait` against the deadline instead of a wait thread. - On timeout, spawn the child in its own process group and tear the group down with `SIGTERM`, a bounded grace, then an escalating `SIGKILL` (Unix), or `Child::kill` (Windows) — guaranteed termination, not signal cooperation. Extraction also returns `discovery.rs` (1438 -> 1380) and `discovery/tests.rs` (1820 -> 1820) to within the file-size ratchet, which had been skipping the Rust/unit/desktop-core/Windows CI lanes. Adversarial tests (a `SIGTERM`-ignoring child and a descendant retaining the output descriptors, each with an outer wall-clock bound) verify the hard deadline; the sidecar cheap-path regression moves to the path-resolution suite. Origin: Buzz channel harness-detection thread (#5ef5d5bb-643f-4b87-bbf4-e8b64585ffeb). Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
The bounded-command helper only tore down the process group on timeout or spawn error. On a successful exit it reaped the leader and returned, leaving any descendant the child had backgrounded (a login-shell rc worker, an auth CLI daemon) alive with the captured-output descriptors and whatever it was writing. Windows used a direct Child::kill that never touched descendants. Kill the group/tree unconditionally after the leader exits, errors, or times out, before reading captured output. The timeout path keeps its SIGTERM + grace before the final SIGKILL; every other path goes straight to the forced group kill. Windows delegates to taskkill_tree (taskkill /T /F), matching the probe_node discipline already in the tree. Strengthen the adversarial tests: each now runs the helper under an independent wall-clock watchdog thread (a hung helper fails the test instead of hanging it) and asserts the backgrounded descendant is dead after both successful return and timeout. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
force-pushed
the
duncan/harness-boot-detection
branch
from
August 26, 2026 23:01
9ad9212 to
8ed97e8
Compare
wpfleger96
had a problem deploying
to
codex-review
August 26, 2026 23:02 — with 
GitHub Actions
Error
GitHub Actions
Error
Two pass-3 review findings. (1) Windows success-path tree leak. taskkill /T /PID is a live-root lookup: once the direct child has exited (the success and post-SIGKILL paths), there is no root for it to enumerate, so a backgrounded descendant orphaned. Take a kill-on-close Job Object at spawn and hold it across the whole bounded operation, mirroring the harness worker-reaping discipline in process_lifecycle.rs. Closing the job reaps the tree even after the root exits. Job assignment failure degrades to killing the direct child. The spawn/wait/kill lifecycle is wrapped in a BoundedChild guard so both platforms own the tree rather than looking it up after the fact. (2) Vacuous descendant assertions. The Unix fixtures recorded $$ inside a ( ... ) subshell, which in /bin/sh is the invoking shell PID, not the background child — so both tests asserted the already-reaped leader was dead, not the descendant. Record the real background PID via $!. Verified non-vacuous by mutation: neutering kill_tree now fails reaps_backgrounded_descendant_on_success (descendant survives). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
had a problem deploying
to
codex-review
August 26, 2026 23:24 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
|
@wpfleger96 may be related to #6872 I see that a lot (and not a slow machine though) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PR #6330 split agent harness/runtime detection into a cheap (cache-only) path and a forced (spawning) path. Two regressions followed, both surfacing as every harness showing "(not installed)" / "CLI missing" across the agent create/edit picker, Agents > Agent defaults, and Settings > Agents — blocking agent create/edit until the user clicked Install in Settings > Agents.
Root cause
One underlying bug, two victims:
resolve_command_cached(the cheap path) consulted only the Buzz-managed shim dirs plus that cold cache, andbuzz_managed_command_path's allowlist structurally excludesbuzz-agent. The bundled sidecar could therefore never resolve on the cheap path until a forced pass warmed the cache, so cheap-path surfaces rendered all-missing at boot. App setup never warms the cache.run_in_login_shellused an untimeoutedCommand::output(); a wedged login shell froze the whole forced pipeline, leaving "Check again" spinning forever.What
resolve_command_cachednow also callsresolve_workspace_command, resolving the bundled sidecar via a filesystem stat (no spawn) — the same class of work the managed-shim check already performs.buzz-agentcan no longer report missing, even inside the boot warm window.discovery/bounded_command.rsruns any discovery child under a hard wall-clock deadline. It captures stdout/stderr to regular temp files (not pipes), so a forked descendant that keeps the descriptors open cannot block the post-exit read, and polls the child withtry_wait. On timeout it spawns the child in its own process group and tears the group down withSIGTERM→ bounded grace →SIGKILLon Unix, orChild::killon Windows — guaranteed termination rather than signal cooperation.probe_auth_statusis refactored onto it.AppShellmount (refreshAcpRuntimes, fire-and-forget) so the shared React Query catalog cache is populated before the cheap-path surfaces render.Known window
On slow machines the forced probe can take 20–65s; during that boot warm the other harnesses (not the bundled sidecar) may still read "(not installed)" until the forced pass lands. The sidecar case is now structurally fixed and shows correctly immediately. Signalling "checking" vs "not installed" during the warm is a follow-up UI question.
Origin: Buzz thread