Let go of a process that will not die - #75
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb38f31c50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
47d2272 to
f700b39
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f700b39623
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self._generation == generation: | ||
| self._returncode = returncode | ||
|
|
||
| async def _join_within(self, deadline: float) -> None: |
There was a problem hiding this comment.
_join_within is called only once and merely computes the remaining deadline before delegating to _join_tasks; inline this fragment into stop() instead of adding a one-use wrapper, as required by the repository's explicit convention.
AGENTS.md reference: AGENTS.md:L2-L2
Useful? React with 👍 / 👎.
|
|
||
| assert supervisor.state is JsonlProcessState.STOPPED | ||
| assert elapsed < 10 | ||
| assert not supervisor.is_running |
There was a problem hiding this comment.
In this held-pipe test, assert not supervisor.is_running is a redundant negative assertion immediately after the positive STOPPED contract; remove it and retain the positive observable checks because the repository explicitly prohibits reverse/exact assertions in tests.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| subprocess.Popen( | ||
| [sys.executable, "-c", "import time; time.sleep(60)"], | ||
| stdout=sys.stdout, | ||
| stderr=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
Every run of this test detaches a Python grandchild and loses its handle when the parent exits, so after the assertion completes that interpreter continues sleeping for 60 seconds; the second test and its restart create additional orphans, allowing repeated or parallel test runs to accumulate processes. Record the grandchild PID and terminate it in test cleanup after the supervisor behavior has been observed.
Useful? React with 👍 / 👎.
| future.set_exception( | ||
| JsonlProcessExited(returncode=-1, stderr_tail=self.stderr_tail) |
There was a problem hiding this comment.
When the direct process has already exited but an inherited pipe forces the task-join abandonment path, process.returncode and usually _returncode contain the real status, yet every pending request is failed with the fabricated status -1. Callers therefore receive incorrect diagnostics—for example, a clean exit is reported as -1—so use the known process return code and reserve an unknown value for the forced-kill case where it has not arrived yet.
Useful? React with 👍 / 👎.
f700b39 to
3523bd5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3523bd5cc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._abandon(process) | ||
| self._state = ProcessState.STOPPED | ||
| self._exit_event.set() |
There was a problem hiding this comment.
When start() is called after this forced-abandonment branch, the replacement process inherits a set _exit_event and the _CLOSED item queued by _abandon(), because Claude's start() increments the generation but resets neither object. Consequently, wait() returns immediately with the abandoned run's status and receive() can report that the new, still-running process has exited; clear the previous lifecycle's event and queue before spawning the replacement.
Useful? React with 👍 / 👎.
| self._abandon(process) | ||
| self._state = ProcessState.STOPPED | ||
| self._exit_event.set() |
There was a problem hiding this comment.
When the forced-stop supervisor is started again, this remains set because start() only increments the generation; wait() therefore returns immediately with the prior or still-unknown result instead of waiting for the replacement process. Likewise, _abandon() leaves _CLOSED in the incoming queue, so the replacement's first receive() reports the abandoned exit. Clear the exit event and stale queue state when beginning the new lifecycle.
Useful? React with 👍 / 👎.
3523bd5 to
cb4384e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4384e68b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| script = "import time; time.sleep(30)" | ||
| supervisor = ProcessSupervisor( | ||
| ProcessSpec(sys.executable, ("-c", script), tmp_path, os.environ) | ||
| ) |
There was a problem hiding this comment.
This test substitutes a Python sleeper for the external Claude process and places the scenario in the unit-test suite, so it cannot verify the actual provider process lifecycle. Cover this with the real Claude executable as an E2E test, using TestChannel as the control plane, instead of a replacement executable.
AGENTS.md reference: AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
| assert supervisor.returncode is None | ||
| # a caller asking about this run has to be kept waiting | ||
| with pytest.raises(TimeoutError): | ||
| await supervisor.wait(timeout=0.2) | ||
| with pytest.raises(TimeoutError): | ||
| async with asyncio.timeout(0.2): |
There was a problem hiding this comment.
The test establishes correctness solely by asserting that no return code, wait completion, or received item appears during fixed 200 ms windows. These reverse and timing-exact assertions are prohibited by the repository test convention and can pass without demonstrating that the replacement lifecycle eventually behaves correctly; assert a positive observable result from the restarted lifecycle instead.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
cb4384e to
f9fc4ae
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9fc4ae558
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| process.kill() | ||
| await process.wait() | ||
| self._abandon(process) |
There was a problem hiding this comment.
When a Claude child ignores SIGTERM but dies normally on SIGKILL, this calls _abandon() immediately after sending the kill, without using the final 10% of the caller's timeout to observe termination. As a result, an ordinary forced termination is reported to blocked receive() callers as ClaudeProcessExited(None) and is unnecessarily detached into a reaper, even though the documented close sequence reserves the remaining timeout for bounded reap; wait through the final deadline after kill() and abandon only if that wait also expires.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
c2339b5 to
1f2ce3d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f2ce3dcba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # the process may already have exited here, and then its status is the | ||
| # honest answer; -1 is reserved for a kill whose result has not arrived | ||
| exited = JsonlProcessExited( | ||
| returncode=process.returncode if process.returncode is not None else -1, |
There was a problem hiding this comment.
When forced abandonment occurs before process.wait() observes the kill, -1 is not an unknown-status sentinel: on POSIX, negative return codes identify signals, so this falsely reports SIGHUP termination. The fresh code now preserves known statuses, but JsonlProcessExited already accepts None and renders it as unknown; pass None here until the reaper obtains the actual result.
Useful? React with 👍 / 👎.
| assert supervisor.returncode == -signal.SIGKILL | ||
| assert await supervisor.wait(timeout=1) == -signal.SIGKILL |
There was a problem hiding this comment.
These assertions make the new test depend on an exact platform-specific return-code representation, contrary to the repository's explicit prohibition on exact assertions; verify the positive lifecycle outcome without requiring equality to -SIGKILL.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| _kill_process(process) | ||
| await process.wait() | ||
| await self._join_tasks() | ||
| self._abandon(process) |
There was a problem hiding this comment.
When the child consumes the whole deadline by ignoring stdin closure and SIGTERM but terminates normally on SIGKILL, this sequence abandons it immediately after sending the kill instead of waiting for that result. stop() can therefore report completion while the process and transport are still live, allowing shutdown or a replacement connection to overlap an ordinary forced termination; reserve a final bounded slice of the timeout for process.wait() and abandon only if that expires.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1f2ce3d to
042572f
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 042572fb33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await supervisor.start(timeout=5) | ||
| await supervisor.stop(timeout=1) |
There was a problem hiding this comment.
On a slow or heavily loaded host, start() only confirms that the subprocess was created; it does not guarantee that the script has installed its SIGTERM handler. Calling stop() immediately can therefore deliver SIGTERM before signal.signal(...) runs, and the assertions still pass because any termination produces a non-None return code, so this test never exercises the advertised SIGKILL path. Have the child publish a readiness marker after installing the handler and wait for it before calling stop().
Useful? React with 👍 / 👎.
1 participant
stop()waited for a process to exit and for its readers to see EOF, both without a bound, and both while holding the lifecycle lock thatstart()needs. When a grandchild inherited the pipe — the ordinary case on Windows, whereCreateProcesshands every inheritable handle to the child — that EOF never arrived, so shutting a runtime down blocked forever and no new inbound could start a replacement.Reaching the kill step now means we have already given up on this process, so it stops being ours: this side of the pipes is closed, the readers and watcher are cancelled, and the process is handed to a reaper task. Closing our own end is what unblocks everything, since the exit waiters only complete once the pipes count as disconnected — whoever still holds the write end is welcome to keep it.
stop()marks the state and returns, so the lock is free immediately.The reaper waits for the returncode without a deadline, warning once it has waited too long. A timeout there would only abandon the process: nothing would reap it, nothing would retry, and we would not know it existed. It publishes the exit when it arrives, unless a newer process already owns the supervisor.
Verified on both platforms. A test with real processes — a child that ignores SIGTERM plus a grandchild holding stdout — took 60.42s and failed before the change and passes in 1.36s after it. On Windows the same scenario returns from
stop(timeout=1)in 1.006s with no process left behind.Also fixes four tests that assumed a POSIX host, which the first full Windows run exposed: two hardcoded POSIX values for
creationflagsand the upgrade instructions, one stripped a realO_BINARYand so wrote in text mode, and one gave a node about two seconds to publish its endpoint, which is not enough for a loaded Windows box. That last one now waits on the clock and reports the child's exit and stderr when it gives up.