Skip to content

fix(windows): share one bounded, tree-killing git probe across both probe call sites - #68997

Merged
OutThisLife merged 1 commit into
mainfrom
bb/salvage-git-probe-deadlock
Jul 22, 2026
Merged

fix(windows): share one bounded, tree-killing git probe across both probe call sites#68997
OutThisLife merged 1 commit into
mainfrom
bb/salvage-git-probe-deadlock

Conversation

@OutThisLife

Copy link
Copy Markdown
Collaborator

Supersedes #68622 and #66038.

Problem

subprocess.run(["git", ...], timeout=...) deadlocks on Windows. run()'s post-timeout cleanup calls an unbounded communicate() after killing git. Killing the PATH-resolved launcher can leave a suspended descendant git.exe holding duplicates of the captured stdout/stderr handles, so the pipes never reach EOF and the reader-thread join blocks forever — leaking a process + two reader threads per fired timeout. That accumulating git.exe activity is what drives the Windows Defender CPU load users have reported (#68609).

Two fail-open probe call sites had the identical flaw:

Two open PRs each fixed one site: #68622 (git_probe, with the Windows tree-kill) and #66038 (coding_context, proc.kill() only, no tree-kill). Both edit the same test file, so they collide if merged separately — and leave two divergent copies of the same fix, one of them missing the descendant tree-kill that actually reaps the leaked git.exe.

Fix

One shared bounded_git_probe(argv, *, timeout) in hermes_cli/_subprocess_compat.py (both files already import from there → no new import surface). Both call sites collapse to a one-line delegation and keep their own timeout (1.5s / 2.5s).

  • Explicit communicate(timeout), then on any failure a tree-kill (proc.kill() and, on Windows, best-effort taskkill /T /F) so the suspended descendant holding the pipe writers dies too, letting the bounded 1s post-kill drain reach EOF. If the pipes are still held, they're abandoned (orphaned reader threads are daemonic and cost nothing).
  • Fail open to "" on every path: spawn error, timeout, kill() raising (access denied / already reaped — a raise inside the except handler previously escaped the contract), and non-timeout communicate() failures now also terminate the child instead of leaving it running.
  • The taskkill spawn can't re-enter the deadlock class: it captures no pipes (DEVNULL), so its own timeout cleanup has no reader threads to join.

Normal-path spawn contract preserved byte-for-byte: PIPE/PIPE/DEVNULL, text + UTF-8 errors="replace", hidden-window creationflags on Windows only, nonzero returncode → "".

Verification

tests/test_windows_subprocess_no_window_flags.py — 25 passed. The git-probe cases are consolidated onto the shared helper: fast-path spawn contract, off-Windows no-flags, nonzero returncode, timeout→kill→bounded-drain ordering, Windows timeout escalates to taskkill /T /F (previously untested), kill-failure fail-open, non-timeout-failure kill ordering, cleanup-failure swallowed, spawn-failure fail-open, plus per-call-site delegation tests for run_git and _git. Consumer suites green: tests/tui_gateway/ + tests/agent/test_coding_context.py (449 passed).

Credit

Salvages #68622 (@Sora-bluesky — original git_probe fix + the Windows tree-kill approach) and #66038 (@iamwongeeeee — coding_context fix). Both preserved via Co-authored-by.

Deliberate scope

  • Readiness decoupling (don't make Desktop session readiness synchronously depend on git metadata) is a separate _start_agent_build change — left for follow-up. This removes the unbounded hang that made the dependency fatal.
  • branch() failure caching/backoff (its two-probe fallback doubles cost in this state) — noted for follow-up; with the tree-kill each occurrence is now bounded and leaks nothing.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

running on 4208930

CI timings

CI timings · View job

Wall time 7m41s vs 7m42s (-0.2%). 13 job(s) slower, 6 faster, 1 unchanged.

  • Build&Test Docker image / build (amd64, ubuntu-latest, linux/amd64, type=gha,scope=docker-amd64, type=gha,mode=max,scope=do...: +19.0s
  • Python tests / Run tests slice 7/8: -18.0s
  • OSV scan / Scan lockfiles / osv-scan: +15.0s
  • Python tests / Run tests slice 8/8: +14.0s
  • Python tests / Run tests slice 2/8: +7.0s

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) platform/windows Native Windows-specific behavior or breakage P2 Medium — degraded but workaround exists sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 22, 2026
…all sites

subprocess.run(["git", ...], timeout=...) deadlocks on Windows: run()'s
post-timeout cleanup calls an unbounded communicate() after killing git.
Killing the PATH-resolved launcher can leave a suspended descendant git.exe
holding duplicates of the captured stdout/stderr handles, so the pipes never
reach EOF and the reader-thread join blocks forever — leaking a process +
two reader threads per fired timeout (the accumulating git.exe load behind
Windows Defender CPU spikes).

Two fail-open probe call sites had this identical flaw:
  - tui_gateway/git_probe.py::run_git — on the Desktop agent-build path
    (_start_agent_build -> _session_info -> branch() -> run_git), where the
    hang turned an optional branch label into "agent initialization timed
    out" (#68609).
  - agent/coding_context.py::_git — hangs the agent turn inside
    build_coding_workspace_block under an ACP host (#66037).

Consolidate both onto one shared bounded_git_probe() in
hermes_cli/_subprocess_compat.py (both files already import from there, so
no new import surface):
  - explicit communicate(timeout), then on ANY failure a tree-kill —
    proc.kill() AND, on Windows, best-effort taskkill /T /F so the suspended
    descendant that holds the pipe writers dies too — plus a bounded 1s
    post-kill drain; if the pipes are still held they're abandoned (the
    orphaned reader threads are daemonic and cost nothing).
  - fail open to "" on every path: spawn error, timeout, kill() raising
    (access denied / already reaped — a raise inside the except handler
    previously escaped the contract), and non-timeout communicate() failures
    now also terminate the child instead of leaving it running.
  - the taskkill spawn can't re-enter the deadlock class: it captures no
    pipes (DEVNULL), so its own timeout cleanup has no reader threads to join.

Normal-path spawn contract is preserved byte-for-byte: PIPE/PIPE/DEVNULL,
text + utf-8 errors="replace", hidden-window creationflags on Windows only,
nonzero returncode -> "". Each call site keeps its own timeout (1.5s / 2.5s).

Supersedes #68622 (Sora-bluesky — git_probe fix + tree-kill) and #66038
(iamwongeeeee — coding_context fix), folding both into one shared helper so
the two sites can't drift and every timeout tree-kills the descendant. Tests
consolidated onto the helper, incl. the previously-missing assertion that a
Windows timeout escalates to taskkill /T /F.

Co-authored-by: Sora-bluesky <sora.bluesky.dev@gmail.com>
Co-authored-by: iamwongeeeee <wykim777@naver.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@OutThisLife
OutThisLife force-pushed the bb/salvage-git-probe-deadlock branch from f812cb8 to 4208930 Compare July 22, 2026 00:54
@OutThisLife
OutThisLife merged commit 967e078 into main Jul 22, 2026
36 checks passed
@OutThisLife
OutThisLife deleted the bb/salvage-git-probe-deadlock branch July 22, 2026 01:18
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…all sites (NousResearch#68997)

subprocess.run(["git", ...], timeout=...) deadlocks on Windows: run()'s
post-timeout cleanup calls an unbounded communicate() after killing git.
Killing the PATH-resolved launcher can leave a suspended descendant git.exe
holding duplicates of the captured stdout/stderr handles, so the pipes never
reach EOF and the reader-thread join blocks forever — leaking a process +
two reader threads per fired timeout (the accumulating git.exe load behind
Windows Defender CPU spikes).

Two fail-open probe call sites had this identical flaw:
  - tui_gateway/git_probe.py::run_git — on the Desktop agent-build path
    (_start_agent_build -> _session_info -> branch() -> run_git), where the
    hang turned an optional branch label into "agent initialization timed
    out" (NousResearch#68609).
  - agent/coding_context.py::_git — hangs the agent turn inside
    build_coding_workspace_block under an ACP host (NousResearch#66037).

Consolidate both onto one shared bounded_git_probe() in
hermes_cli/_subprocess_compat.py (both files already import from there, so
no new import surface):
  - explicit communicate(timeout), then on ANY failure a tree-kill —
    proc.kill() AND, on Windows, best-effort taskkill /T /F so the suspended
    descendant that holds the pipe writers dies too — plus a bounded 1s
    post-kill drain; if the pipes are still held they're abandoned (the
    orphaned reader threads are daemonic and cost nothing).
  - fail open to "" on every path: spawn error, timeout, kill() raising
    (access denied / already reaped — a raise inside the except handler
    previously escaped the contract), and non-timeout communicate() failures
    now also terminate the child instead of leaving it running.
  - the taskkill spawn can't re-enter the deadlock class: it captures no
    pipes (DEVNULL), so its own timeout cleanup has no reader threads to join.

Normal-path spawn contract is preserved byte-for-byte: PIPE/PIPE/DEVNULL,
text + utf-8 errors="replace", hidden-window creationflags on Windows only,
nonzero returncode -> "". Each call site keeps its own timeout (1.5s / 2.5s).

Supersedes NousResearch#68622 (Sora-bluesky — git_probe fix + tree-kill) and NousResearch#66038
(iamwongeeeee — coding_context fix), folding both into one shared helper so
the two sites can't drift and every timeout tree-kills the descendant. Tests
consolidated onto the helper, incl. the previously-missing assertion that a
Windows timeout escalates to taskkill /T /F.

Co-authored-by: Sora-bluesky <sora.bluesky.dev@gmail.com>
Co-authored-by: iamwongeeeee <wykim777@naver.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants