-
Notifications
You must be signed in to change notification settings - Fork 16.3k
fix(browse): stop isProcessAlive from flashing a Windows console every watchdog tick#1979
Open
jbetala7 wants to merge 1 commit into
Open
fix(browse): stop isProcessAlive from flashing a Windows console every watchdog tick #1979jbetala7 wants to merge 1 commit into
jbetala7 wants to merge 1 commit into
Conversation
...y watchdog tick On Windows, isProcessAlive() shelled out to `tasklist` via Bun.spawnSync. Windows gives that child its own console window, so the terminal-agent watchdog's per-tick existence check (default 60s) flashed a conhost.exe window for the entire headed session — purely cosmetic but a constant, unexplained blink (garrytan#1952). macOS/Linux stayed silent because their branch is a `process.kill(pid, 0)` syscall that spawns nothing. Unify on signal-0 across all platforms — exactly what the parent watchdog two functions away already does. Node/Bun implement `process.kill(pid, 0)` on Windows via OpenProcess: a pure existence check, no child process, no window. This also fixes a latent correctness edge: the old branch collapsed every error to false, so a live-but-unsignalable process (EPERM) read as dead. Now ESRCH -> dead, EPERM -> alive. Tests: existing alive/dead probes still pass; add an EPERM-as-alive case and a static tripwire asserting the probe never reintroduces `tasklist`/spawn (proven to fail on the old body). 9/9 green in error-handling.test.ts. Fixes garrytan#1952 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merging to main in this repository is managed by Trunk.
- To merge this pull request, check the box to the left or comment
/trunk mergebelow.
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here
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
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.
Problem
On Windows, a connected headed
browsesession blinks aconhost.execonsole window on a fixed interval for the entire session — no CLI command, no user action. The terminal-agent watchdog callsisProcessAlive()once persetIntervaltick (default 60s) to check the agent PID, and on WindowsisProcessAlive()shelled out totasklist, which Windows gives its own console window. macOS/Linux stayed silent because their branch is aprocess.kill(pid, 0)syscall that spawns nothing.It is purely cosmetic — each window self-closes — but it blinks on a fixed interval for the whole session with no indication of what it is.
Root cause
isProcessAlive()(browse/src/error-handling.ts) had a Windows-only branch that spawnedtasklist:Every watchdog tick → a
tasklistspawn → a console window. The parent watchdog two functions away already avoids exactly this by callingprocess.kill(pid, 0)directly; onlyisProcessAlive()took the spawn path.Fix
Unify on signal-0 across all platforms. Node/Bun implement
process.kill(pid, 0)on Windows viaOpenProcess— a pure existence check, no child process, no window. The Windows-only branch (and its now-unusedIS_WINDOWSconst) are removed.This also corrects a latent edge: the old branch collapsed every error to
false, so a live-but-unsignalable process (EPERM) read as dead. NowESRCH → dead,EPERM → alive.The same
isProcessAliveis compiled into thebrowseCLI binary's polling loops, so the one-file fix covers those on rebuild too. The watchdog's alive/dead branch is unchanged — signal-0 returns the same boolean it already keys on, so dead-agent detection and respawn still work.Testing
bun test browse/test/error-handling.test.ts→ 9/9 pass. Added:EPERM-as-alive regression case (probes PID 1, which a non-root user can't signal), andisProcessAlivesource never reintroducestasklist/spawn— the idiomatic gstack guard for a Windows-only cosmetic bug that can't be exercised on macOS CI. Proven to fail on the oldtasklistbody before the fix.The watchdog/terminal-agent suites (
terminal-agent-watchdog.test.ts,watchdog.test.ts, etc.) stay green. One unrelated pre-existing failure interminal-agent.test.ts(a source-guard grepping forspawnClaude(, since renamed tomaybeSpawnPty) reproduces on cleanmainwith this branch stashed — not touched here (this PR only changesbrowse/src/error-handling.ts+ its test).Fixes #1952