Follow-up from the PR #192 review (jatmn's F3, deliberately deferred there: our CI is Linux-only, so a Windows fix would ship untested).
The gap
run_bounded in crates/git-remote-gitlawb/tests/real_git_fetch.rs bounds a git process (the "leader") plus its stdout/stderr reader threads by a timeout. git spawns the git-remote-gitlawb helper as a descendant, which can block in a ~300s HTTP request while holding the inherited pipe write-ends.
- Timeout path (deadline hit): reaps the whole tree on both platforms —
#[cfg(unix)] reap_group / #[cfg(not(unix))] reap_tree (taskkill /T), around lines 408-411.
- Clean-exit path (leader exits before the deadline,
completed == true): reaps the group before the reader-thread joins — but only under #[cfg(unix)] (lines 435-442). There is no #[cfg(not(unix))] counterpart.
The code documents the exact hazard at lines 426-434:
A leader that exits on its own does NOT guarantee the pipes are closed: a descendant (the git-remote-gitlawb helper mid-HTTP request) can outlive the leader while still holding the inherited stdout/stderr write-ends, so the reader joins below would block on its ~300s HTTP timeout, past the deadline this function promises.
On Windows, when the leader exits cleanly while the helper is still blocked mid-request, nothing reaps the descendant before out_reader.join() / err_reader.join() (lines 444-445), so those joins block on the descendant-held pipe until the helper's HTTP times out (~300s) — far past the intended 30s bound.
Why it isn't caught today
- No Windows CI: every
pr-checks job is ubuntu-latest, so the Windows path is never exercised.
taskkill /T (the timeout-path Windows reap) can't help on the clean-exit path anyway: after the leader has exited there is no leader pid to anchor the process-tree walk.std::process::Child::drop is a no-op (it neither kills nor waits), and it owns the leader, not the descendant — so there is no RAII rescue.
Severity: latent — a real code gap, Windows-only, not currently triggered by the committed tests (the shim responds promptly) and never run on Windows. The #[cfg(unix)] clean-exit reap's own existence shows the authors consider "leader exits while a descendant holds the pipe" a reachable scenario; Windows simply lacks the counterpart.
Suggested direction
Establish descendant ownership at spawn time on Windows — a Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, assigned before the leader starts — so the whole tree dies when run_bounded returns, on both the clean and timeout paths. Pair it with a Windows CI lane so the behavior is actually exercised; without one the fix would ship untested, which is why it was deferred from #192.
Verified by code-path trace against the current head (3ceba33); not executed, since it is Windows-only and unreproducible on Linux.
Follow-up from the PR #192 review (jatmn's F3, deliberately deferred there: our CI is Linux-only, so a Windows fix would ship untested).
The gap
run_boundedincrates/git-remote-gitlawb/tests/real_git_fetch.rsbounds a git process (the "leader") plus its stdout/stderr reader threads by a timeout. git spawns thegit-remote-gitlawbhelper as a descendant, which can block in a ~300s HTTP request while holding the inherited pipe write-ends.#[cfg(unix)] reap_group/#[cfg(not(unix))] reap_tree(taskkill /T), around lines 408-411.completed == true): reaps the group before the reader-thread joins — but only under#[cfg(unix)](lines 435-442). There is no#[cfg(not(unix))]counterpart.The code documents the exact hazard at lines 426-434:
On Windows, when the leader exits cleanly while the helper is still blocked mid-request, nothing reaps the descendant before
out_reader.join()/err_reader.join()(lines 444-445), so those joins block on the descendant-held pipe until the helper's HTTP times out (~300s) — far past the intended 30s bound.Why it isn't caught today
pr-checksjob isubuntu-latest, so the Windows path is never exercised.taskkill /T(the timeout-path Windows reap) can't help on the clean-exit path anyway: after the leader has exited there is no leader pid to anchor the process-tree walk.std::process::Child::dropis a no-op (it neither kills nor waits), and it owns the leader, not the descendant — so there is no RAII rescue.Severity: latent — a real code gap, Windows-only, not currently triggered by the committed tests (the shim responds promptly) and never run on Windows. The
#[cfg(unix)]clean-exit reap's own existence shows the authors consider "leader exits while a descendant holds the pipe" a reachable scenario; Windows simply lacks the counterpart.Suggested direction
Establish descendant ownership at spawn time on Windows — a Job Object with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, assigned before the leader starts — so the whole tree dies whenrun_boundedreturns, on both the clean and timeout paths. Pair it with a Windows CI lane so the behavior is actually exercised; without one the fix would ship untested, which is why it was deferred from #192.Verified by code-path trace against the current head (
3ceba33); not executed, since it is Windows-only and unreproducible on Linux.