Uh oh!
There was an error while loading. Please reload this page.
fix(cli): terminate whole --web-bg process tree on launch-gate failure - #450
Merged
Jason Robert (jrob5756) merged 2 commits intoAug 16, 2026
Merged
Conversation
The launch gate's four failure paths only terminated subprocess.Popen.pid. Under a trampoline sys.executable (e.g. a Windows uv tool install), that pid is a re-exec shim rather than the process actually running the workflow, so a launch-gate failure could kill the shim and leave the real workflow running, undiscoverable, and still burning tokens. On Windows the child is now created suspended and assigned to a fresh job object before it can run, so it cannot re-exec out of reach; TerminateJobObject reaches the whole tree regardless of exec depth, and the job intentionally omits JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so the tree still survives the launcher exiting. On POSIX, os.killpg now reaches the process group the detached child already leads. After the tree kill, a final liveness sweep independently confirms every pid the gate knew about is actually dead rather than assuming it. A survivor is named explicitly in the error message (with a `conductor status` / `conductor stop --port` pointer) instead of unconditionally claiming the process was terminated. A run record is only removed once its pid is confirmed dead, so a surviving orphan keeps the record that is `conductor stop`'s only remaining handle on it. Adds a web-bg-smoke CI job that drives a real `uv tool install` + `conductor run --web-bg` launch on Windows and Ubuntu to catch trampoline-related regressions that mocked unit tests cannot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ee kill - Hoist the rung-1 process-tree kill (terminate_tree/killpg) out of the `proc.poll()` guard in `_terminate_child` so it still fires on the StartProbe.CHILD_EXITED path -- the trampoline case issue #447 exists for, where the outer shim has already exited while the re-exec'd workflow lives on. Replace the duck-typed `getattr(proc, "terminate_tree", None)` with an `isinstance(proc, _WindowsDetachedProcess)` check now that the union is closed. - Extract `_read_record_or_fail` so both run-record reads inside `_finalize_background_launch`'s poll loop terminate the child and raise on failure, matching the function's own documented contract. - Release the Windows process/job handles on every launch-gate failure path, not just the success path, by wrapping the `_finalize_background_launch` call in `except BaseException: _release_child_handles(proc); raise`. - Fix the new `web-bg-smoke` CI job: export UV_TOOL_DIR/UV_TOOL_BIN_DIR in the same shell as `uv tool install .` instead of relying on $GITHUB_ENV, which only takes effect in later steps. - Fix the `os.killpg` footgun-guard test, which passed vacuously because a bare MagicMock auto-creates `terminate_tree`; use `MagicMock(spec=subprocess.Popen)` so the POSIX arm is actually exercised. - Guard two test_resume_command.py tests against sending a real SIGKILL to an unrelated process on the developer's machine by patching `conductor.cli.pid.is_process_alive` to False. - Correct AGENTS.md's claim that `_cleanup_record_after_termination` is reused by all four gate failure branches: there are five terminate-and-raise branches, and two of them never have a confirmed pid to clean up with. Also drop a stale "(now _confirmed_pid_from_record)" rename note. - Update test_stop_ladder.py's source-inspection assertion for the `_read_record_or_fail` extraction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 16, 2026 15:28
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 16, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Issue #444 fixed the launch gate's false-positive port conflicts but left its four failure paths terminating only
subprocess.Popen.pid. Under a trampolinesys.executable(e.g. a Windowsuv tool install, the documented install path), that pid is a re-exec shim rather than the process actually running the workflow, so a launch-gate failure could kill the shim and leave the real workflow running, undiscoverable, and still burning tokens.TerminateJobObjectreaches the whole tree regardless of exec depth. The job intentionally omitsJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEso the tree still survives the launcher exiting.os.killpgnow reaches the process group the detached child already leads.conductor status/conductor stop --portpointer) instead of unconditionally claiming the process was terminated.conductor stop's only remaining handle on it.web-bg-smokeCI job that drives a realuv tool install+conductor run --web-bglaunch on Windows and Ubuntu to catch trampoline-related regressions mocked unit tests cannot.Closes#447
Testing
tests/test_cli/test_bg_runner.pyand related test suites for the new termination behavior.