fix(conpty): correctly implement pidAlive using processalive on Windows#3082
Conversation
Pulkit7070
left a comment
There was a problem hiding this comment.
Reviewed. The fix is correct and the bug analysis is accurate.
What I verified:
- processalive.Alive (windows) opens the SYNCHRONIZE handle and then calls WaitForSingleObject(handle, 0), returning alive only when the result is WAIT_TIMEOUT. That is exactly the missing step: a still-open handle to a recently-terminated process keeps OpenProcess succeeding, but the process object is signaled, so WaitForSingleObject returns WAIT_OBJECT_0 rather than WAIT_TIMEOUT and Alive correctly returns false. Delegating pidAlive to it fixes the false-positive that forced the 500ms teardown hang.
- Signatures match (Alive(pid int) bool), and Alive exists on both windows and unix builds.
- Since this file is build-tagged windows, I cross-compiled: GOOS=windows go build and GOOS=windows go vet on the conpty package and processalive are clean, so the new import resolves and there is no leftover unused windows import. Host (darwin) go build ./... is unaffected.
One completeness point worth addressing, since it is the same bug class in the same subtree. backend/internal/adapters/runtime/conpty/ptyregistry/pidalive_windows.go defaultPidAlive uses windows.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, ...) and treats OpenProcess success as alive, with no WaitForSingleObject check. That is the exact pattern this PR is fixing, so it very likely has the same false-positive on recently-terminated PIDs (it is used for PtyHostPID liveness in ptyregistry/registry.go). Consider routing that one through processalive.Alive in this PR as well, or confirm why it is exempt, so the fix is consistent across both conpty liveness probes.
Minor: no test was added. The conpty pidAlive is now a thin delegation, so a test there would only assert delegation, but there is no regression test pinning the original hang. That is hard to unit test on Windows, so it is non-blocking, though a small processalive-level test for the WAIT_TIMEOUT vs WAIT_OBJECT_0 distinction would be the durable place if you want one.
|
Follow-up: stress test and edge-case pass. Stress (what is runnable on a non-Windows host):
Edge-case analysis of processalive.Alive on Windows:
Two things worth calling out, neither blocking:
Severity note for context: the original bug is latency only, not a leak. The caller polls for 500ms and then force-kills sess.pid regardless (runtime.go), so the flawed pidAlive only cost a 500ms teardown stall, it did not leave processes alive. The fix correctly restores the early-exit. |
What
Updates the
pidAlivefunction inbackend/internal/adapters/runtime/conpty/pidalive_windows.goto delegate toprocessalive.Alive.Why
Fixes #3079.
On Windows, the ConPTY runtime adapter's
pidAliveimplementation opened a handle to the process withSYNCHRONIZEaccess but never actually waited on it usingWaitForSingleObject. Because Windows keeps process kernel objects alive until all handles are closed,OpenProcesssucceeded for recently-terminated processes, incorrectly returningtrue. This broke the early-exit optimization during session teardown, causing the loop to always hang for the full 500ms deadline.How
OpenProcesslogic inpidAliveby directly callingprocessalive.Alive(pid).processalive.Aliveis already implemented and proven elsewhere in the codebase, and correctly usesWaitForSingleObject(handle, 0)to check forWAIT_TIMEOUT."golang.org/x/sys/windows"import with"github.com/aoagents/agent-orchestrator/backend/internal/processalive".Testing
cd backend && go build ./... && go test ./...Checklist
mainprocessalivepackage).go, frontend, etc.)