Repository navigation
fix(*): a job handed off on Windows outlives the host's exit - #1115
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The breakaway permission is opt-in at process creation, and the detached on-call relay is the only changed caller that requests it. The fallback preserves the prior launch behavior when breakaway is refused, while the new Windows test exercises the host job-object close path that exposed the bug.
Reviewed the repository rules and domain map, the full diff, the ACP launch and on-call submit callers, relevant history, backward compatibility and nested-job behavior, test strength, and the architecture boundary. I found no weakened tests or plain regression.
Verification: uv run --frozen --python 3.12 --extra dev pytest tests/test_acp_windows.py tests/test_agents_oncall_flow_windows_backend.py -q completed with 32 passed and 5 Windows-only skips on this Linux host. The native Windows assertions were therefore reviewed but not independently executed here. Focused Ruff checks also passed.
#951 keeps each ACP agent in a kill-on-close job object so nothing it started is left behind when the host exits. A job the on-call agent hands off is meant to be left behind: the windows backend starts it through a relay that exits at once, so no parent chain leads to it (#1016). But job object membership is inherited, not traced through parents, so the job was still in the agent's job object and died the moment the host exited, mid-run. The job object now allows a process to leave it (JOB_OBJECT_LIMIT_BREAKAWAY_OK), and only the relay asks to (CREATE_BREAKAWAY_FROM_JOB); everything else an agent starts, the agent included, still ends with the job. Where leaving is refused, the relay starts the job as before and its log says the job ends if the host exits; the log says the same when the job is still in a job object after asking to leave, which a launcher with a job object of its own between the host and the relay (a virtual environment's python.exe) can cause without an error. Tests: an agent started through raven.acp_client.windows.launch submits a job, the job object is closed: the agent ends, the job writes its result (fails on the old relay). With leaving not allowed the job still starts and its log carries the note.
949d209 to
33e38e4
Compare
|
Commit message and title fixed for the The failing @gloryfromca thanks for the review: agreed on all three points. Breakaway is opt-in at creation, the relay is the only caller asking for it, and the fallback keeps the prior launch. The new Windows test starts its agent through |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The amended revision has the same tree as the previously reviewed head (ffd543c09d5506e1e55f50bb965166dce1cbbbb7); only the commit and PR scope changed to the valid cross-scope (*). I verified the old-to-new diff is empty and the current title and commit message pass the repository check scripts.
This revision check covered the tree/diff, repository metadata rules, current comments and threads, and focused tests. The prior caller, history, backward-compatibility, test-strength, and architecture assessment remains applicable because the code tree is identical.
Verification: uv run --frozen --python 3.12 --extra dev pytest tests/test_acp_windows.py tests/test_agents_oncall_flow_windows_backend.py -q completed with 32 passed and 5 Windows-only skips on this Linux host.
|
Not a blocker (disclosure for the merger, from my acceptance pass on head 33e38e4 merged onto main 4718421. I reviewed it by reading; the Windows test record is this PR's CI, which includes the windows-latest matrix.) Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. A deliberate loosening of #951's guarantee, worth accepting knowingly. I do not think this changes what an agent can do:
What changes is the clean-up promise. A third-party agent, or a tool it runs, that happens to pass Two smaller points:
|
The relay's check after starting a job called OpenProcess, IsProcessInJob and CloseHandle through ctypes.windll with no declared types. ctypes takes an undeclared return value as a 32-bit int, so on 64-bit Windows the process handle could be cut short and the wrong handle passed on. It only fed the "still in a job object" note, never the job's start, but it should not rely on handle values being small. The three functions are now declared as raven.acp_client.windows declares them: pointer-wide handles, int results.
|
New commit Checked: |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new delta correctly declares pointer-width-safe Win32 handle signatures for the relay's diagnostic check. It does not alter process creation, breakaway, or fallback behavior, and I found no new defect.
For this revision I reviewed the ctypes delta, its surrounding relay control flow, the repository rules, compatibility with the existing Windows bindings, test strength, current comments, CI, and review threads. The earlier full-diff, caller, history, backward-compatibility, and architecture review remains applicable to the unchanged remainder.
Verification: the focused pytest command completed with 32 passed and 5 Windows-only skips on Linux; focused Ruff, relay compilation, and git diff --check passed. The current Windows CI job was still pending when reviewed.
|
Windows compatibility review — PR #1115 @ Scope: full base-to-head diff (4 files: Result: no Windows compatibility defect found. What I checked:
Verification (native Windows 11, isolated detached worktree The run includes the two new Windows-only tests — a job surviving the host closing the agent's job object, and a job still starting (with the log note) when breakaway is refused — plus the pre-existing tree-kill test. Non-blocking observation: the relay catches bare Limitations: verified on this one Windows 11 / CPython 3.12 host only; outer-job and nested-job arrangements were not reproduced locally (the PR body documents them as measured). |
|
Blocking: 1 finding needs correction before this revision can merge; it is stated below rather than inline, because no changed file is in my review scope. Scope: my review covers the This supersedes the "no Windows compatibility defect found" result of the Windows compatibility comment posted from this account earlier on this head (6421127503): that run did not exercise a process tree under Git Bash, which is where the finding below lives. Confirmed findings:
Verification:
Already filed, so not repeated here: the bare |
|
Blocking: Git Bash descendants can escape the shared ACP job and survive host exit. The new evidence changes my previous clean stance. I independently confirmed in the cited Git for Windows MSYS2 source that, when the current job advertises I am not re-grading @LivXue's finding; this corrects a factual premise of my earlier assessment. The focused tests still complete with 32 passed and 5 Windows-only skips on Linux, but they do not cover a Git Bash descendant tree. |
A handed-off job has to outlive the host, but the shared ACP job object cannot allow breakaway: Git for Windows' MSYS2 runtime adds CREATE_BREAKAWAY_FROM_JOB to every native child when the job permits it, so an ordinary Bash tool command would leak past the host again. The job stays kill-on-close with no breakaway. The host serves a named pipe, whose address and key reach the agent in its environment, and starts the hand-off relay outside the job on the agent's request. With no channel the relay starts inside the job and the job's log says it ends with the host; a request whose answer is lost is not retried locally, so one job cannot run twice. Tests: a Git Bash descendant stays in the agent's job; a handed-off job outlives the closed job object; the fallback starts inside the job and says so; the fallback dies with the job; the channel protocol, its failures and the no-duplicate path; the producer and consumer variable names.
780f331 to
a3df989
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the newly added handoff-server tests must pass in the Ubuntu unit suite.
The host-side handoff architecture fixes the Git Bash containment blocker: the shared job again forbids breakaway, the relay is launched outside it, the documented contract is reconciled, and native Windows coverage is added to CI. One newly introduced portability failure remains, marked inline.
This satisfies the late-round blocker bar: this revision introduces the failure, every Linux execution of the affected tests reaches it, and there is no working route to a green required unit suite without correcting it.
I reviewed the current base-to-head diff after its rebase, the handoff protocol and lifecycle, callers, failure and duplicate-start behavior, repository rules and domain terms, backward compatibility, test strength, history/comments, and the architecture boundary. Focused pytest completed with 2 failed, 42 passed, and 7 Windows-only skipped; focused Ruff and git diff --check passed.
`_HandoffServer._serve` passed `subprocess.CREATE_NO_WINDOW` as a keyword value, which does not exist on POSIX, so the two unit tests that mock `subprocess.run` raised AttributeError before reaching the mock. The flag is now read with getattr(..., 0), and the tests delete the attribute so the POSIX shape is exercised on every host.
|
The shared-job breakaway finding is fixed at |
|
Both blocking points are addressed at head
Windows 11 / CPython 3.12: 51 passed over the two files. The two inline |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The portability blocker is fixed: CREATE_NO_WINDOW now safely falls back to 0, and the two affected tests force the POSIX attribute shape. The focused suite reports 44 passed and 7 Windows-only skips; Ruff and git diff --check pass.
This round covered the repository rules, the complete delta from a3df98946e94, the affected handoff method and tests, compatibility with POSIX test collection, test-strength preservation, and the prior host-side handoff architecture and history. The delta does not alter callers or the architecture reviewed in the preceding round. I found no new defect.
|
Windows compatibility review — PR #1115 @ Scope: the PR's own diff at this head is Finding (low–medium, reproduced on Windows): the new hand-off subprocess calls decode relay output with the ANSI code page, not the UTF-8 Raven sets for its process tree.
Verification (native Windows 11; detached worktrees
Coverage gaps: no live end-to-end run of host + ACP agent + pipe + detached job was driven outside the test stubs; the pipe's cross-process behaviour under a real host exit was not exercised; no other locale was tested. |
The one accept thread ran the key handshake inline and returned on the EOFError/BrokenPipeError a client raises by disconnecting mid-handshake; a client that connected and sent nothing blocked it for as long as it lived. The cached server kept advertising the dead pipe, so later submits connected to a stranded instance and hung forever instead of falling back. Accept on a raw PipeListener, run each connection's handshake and request in its own thread (bounded by a slot semaphore), and bound the request wait with poll(). The host recreates a server whose accept loop has died and retries a transient open failure on the next launch. Regression tests cover a real aborted handshake, a real stalled handshake, the retry, and the dead-server replacement. Co-Authored-By: Claude Code <noreply@anthropic.com>
The host decoded the agent's request with Connection.recv(), which unpickles it before any schema check; a process holding the pipe key could therefore run code inside the host instead of only asking it to start a relay. Send the request and the answer as length-prefixed JSON over send_bytes/recv_bytes, cap the request size, and require the env to be an object so an empty request can no longer inherit the host's environment. Co-Authored-By: Claude Code <noreply@anthropic.com>
…fallback _handoff_answer ran Client's blocking handshake, send, and recv on the agent's event-loop thread with no deadline. When the host or its accept loop was wedged, ops_submit never returned and never reached the local fallback; the old subprocess.run(timeout=60) had bounded that wait. Run the exchange on a daemon thread and wait at most _HANDOFF_DEADLINE_S: before the request goes out the caller falls back; after it may have gone out the caller raises the "may have started" error. The local fallback now starts the relay with the same filtered environment the host path sends, so the hand-off key no longer reaches the job body or its log, and the fallback note is written only after the relay started. The note no longer claims the job is in the agent's job object: _in_a_job only answers "any job". The Git Bash probe prefers Git's install over a WSL bash on PATH, and the relay's no-op log flush is gone. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the full base-to-head diff and the three-commit delta from d02c8e226719. The raw named-pipe handshake keeps bad or stalled clients off the accept loop, JSON framing removes pickle decoding and bounds requests, the client deadline preserves the no-duplicate-start rule across races, and both host and fallback paths keep the channel key out of the relay environment. Missing/invalid channels still retain the established local fallback.
Coverage this round included the repository rules and CONTEXT.md, the diff, affected callers, commit history, backward compatibility, architecture constraints, and whether tests were weakened; the new tests strengthen the failure and race cases. Focused suites pass on Python 3.12 and 3.13 with 52 passed and 9 Windows-only skips each; Ruff and git diff --check pass. The PR's dedicated Windows job-object/on-call hand-off CI step also passed at this head. My previously opened thread remains resolved.
The PR diff coverage gate counts the changed lines in windows.py, and the Ubuntu shards skip the Windows-only abort/stall tests, so close(), the accept loop's error/no-slot/thread-start branches, the inline handshake, and the request's poll/validation errors were uncovered (72.32% against the 90% threshold). Drive each branch with the fake listener/connection the other hand-off tests already use; the Windows-only real-pipe tests keep covering the live path. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The delta from e881459ba59a is test-only and additive. It covers the hand-off server's remaining accept, slot, authentication, close, timeout, and schema-rejection branches without weakening existing assertions or changing production behavior.
I checked the repository rules, the base-to-head diff and new commit, the production paths these tests exercise, compatibility and architecture implications, and the existing resolved review record. The focused Python 3.13 suite reports 61 passed and 9 Windows-only skips; Ruff and git diff --check pass. My previously opened thread remains resolved.
|
Windows compatibility re-review — PR #1115 @ Scope: the delta since the previously reviewed head Finding (low–medium, reproduced on Windows — carried over from the previous review of this PR): the hand-off subprocess calls still decode relay output with the ANSI code page, not the UTF-8 Raven sets for its tree.
Windows angle of the rest of the delta: the new auth exchange uses the standard Verification (native Windows 11; detached worktrees
Coverage gaps: no live host + ACP agent + pipe + detached job run outside the test stubs; the persistent-accept-error path was exercised only as far as the tests drive it; the decoding finding above is reproduced by probe, not by an end-to-end relay. |
Create every hand-off pipe instance with an explicit DACL for SYSTEM, Administrators, and the owner, instead of the named-pipe default that also grants Everyone and Anonymous read access. Any local user could otherwise hold the 64 handshake slots without the key and force every later hand-off onto the in-job fallback. The accept loop now backs off and gives up after five consecutive errors so a dead listener can be rebuilt, and close() wakes a blocked accept instead of leaving it serving one more request. Drop the unreachable recv_bytes None branch: the stdlib raises OSError when a request exceeds the cap. Co-Authored-By: Claude Code <noreply@anthropic.com>
Run the relay start through asyncio.to_thread so a wedged host cannot freeze the on-call agent's loop, and make the client deadline (100 s) outlast the host's request (30 s) plus relay (60 s) budget. Resolve the job directory to an absolute path before sending it, since the host resolves a relative cwd against its own working directory. Write the unhosted note only after the relay has actually started and report that the job may end with its job object, which need not be kill-on-close. Build the channel-free environment in one helper so a future variable cannot reach the relay through only one of the two paths. Make the test body publish result.json atomically and wait for a parseable result, which removes the observed local flake. Scope the CONTEXT.md hand-off claim to the hosted path and document the fallback. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The delta from 44a42b02be33 is sound. The explicit pipe DACL removes the default Everyone/Anonymous read access while retaining SYSTEM, Administrators, and owner access; close now wakes the blocked accept, repeated accept failures terminate for replacement, and the oversized-message path matches the stdlib's actual exception. The client work moves off the event loop, its deadline exceeds the host request-plus-relay budget, relative job paths are resolved before crossing process cwd boundaries, and both hosted and fallback relay environments consistently drop the channel capability.
I covered the repository rules and updated CONTEXT.md, the base-to-head diff and two-commit history, affected callers, backward-compatible fallback behavior, test-strength changes, and the established no-breakaway/host-side hand-off architecture. Focused tests report 68 passed and 11 Windows-only skips; Ruff and git diff --check pass. The dedicated Windows job-object/on-call hand-off CI step also passed at this head, including the native DACL and shutdown tests. My previously opened thread remains resolved.
|
Windows compatibility re-review — PR #1115 @ Scope: the delta since the previously reviewed head Finding (low–medium, reproduced on Windows — carried over from the two earlier reviews of this PR): the two hand-off subprocess calls still decode relay output with the ANSI code page, not the UTF-8 Raven sets for its tree.
Windows angle of the rest: the SDDL DACL is correct in shape ( Verification (native Windows 11; detached worktrees
Coverage gaps: the DACL was exercised through the tests and native bindings, not from a second local user account; no live host + ACP agent + pipe + detached job run was driven; the decoding finding is probe-reproduced, not end-to-end. |
Summary
A job the on-call agent hands off on Windows dies with the Raven host
mid-run. Allowing breakaway on the shared ACP job object is not a fix:
Git for Windows' MSYS2 runtime adds CREATE_BREAKAWAY_FROM_JOB to every
native program it starts when its job allows breakaway
(winsup/cygwin/spawn.cc), so an ordinary Bash tool command would outlive
the host too and the leak #951 closed would reopen.
This revision keeps the job object closed to breakaway. The host serves
a hand-off channel over a named pipe: the pipe's address and a random
key reach the agent in RAVEN_ACP_HANDOFF_PIPE and
RAVEN_ACP_HANDOFF_KEY, and the agent asks the host, which is not in the
job, to start the relay. The relay starts the launcher outside the job,
so the handed-off job outlives the host while nothing in the agent's
tree can leave the job.
The channel
raven/acp_client/windows.pystarts one hand-off server per hostprocess on the first ACP launch. It is a thread of the host, not a
child of the agent, so what it starts is outside every job object the
host owns. The pipe uses a 32-byte authentication key and an explicit
DACL that grants full control to the launching user (its owner),
Administrators, and SYSTEM; another local user cannot connect even
before the key is checked.
launch()adds the pipe's name and key to the agent's environment.oncall_flow/windows_backend.pyasks the host to start the relay. Arequest carries the relay argv, the job directory and the agent's
environment, so the job body keeps the environment it had before.
authenticated pipe (
send_bytes/recv_bytes), not pickle; the hostcaps a request at 1 MiB and requires
envto be an object. An emptyrequest environment therefore stays empty instead of inheriting the
host's.
outside an ACP agent), the relay starts inside the job and its log
says
the host's hand-off channel was unavailable; this job was started inside a Windows job object and may end when that job closes.submit raises instead of falling back: the host may have started the
job, and a second relay would run it twice. Before the request goes
out, a deadline falls back to starting the relay locally instead.
Review follow-ups
The reviews found failure paths the first revision did not cover; the
branch carries the follow-ups from two review rounds:
c02041cb2keeps the channel alive across a bad client. The acceptloop now only accepts on a raw
PipeListener; each connection getsits own thread for the key handshake and the request (bounded by a
64-slot semaphore), and the request wait is bounded by
poll. Aclient that aborts or stalls a handshake ends or blocks its own
thread, not the only accept thread; the host retries a transient open
failure and replaces a server whose accept loop died.
1c7ef9713frames the request and the answer as JSON instead ofpickle, caps the request size, and requires
envto be an object.e881459babounds the client's exchange with a deadline and keepsthe old local
subprocess.run(timeout=60)bound: before the requestgoes out the caller falls back, after it may have gone out the caller
raises. It also stops the local fallback from inheriting
RAVEN_ACP_HANDOFF_*, writes the fallback note after the relayprocess returns (the second round tightens this to a successful
start), and makes the Git Bash probe prefer Git's install over a WSL
bashon PATH.44a42b02badds unit coverage for the hand-off server's failurebranches; without it the PR diff coverage gate failed at 72.32%
against the 90% threshold.
Second review round
A second review found the pipe's default DACL, a blocking client wait,
and smaller defects in the failure paths; two commits close them:
21ce1e1f9creates every pipe instance with an explicit DACL forSYSTEM, Administrators, and the owner instead of the named-pipe
default that also granted Everyone and Anonymous read access. Another
local user could otherwise hold the 64 handshake slots without the
key and force every later hand-off onto the in-job fallback. The
accept loop now backs off and gives up after five consecutive errors
so a dead listener can be rebuilt, and
close()wakes a blockedaccept instead of leaving it serving one more request. The
unreachable
recv_bytesNone branch is gone: the stdlib raisesOSErrorwhen a request exceeds the cap.ce2a7b98cruns the relay start throughasyncio.to_thread, so awedged host cannot freeze the on-call agent's event loop; the client
deadline is now 100 seconds, longer than the host's 30-second request
plus 60-second relay budget. It resolves the job directory to an
absolute path before the request, since the host resolves a relative
cwd against its own working directory, and writes the unhosted note
only after a successful relay. The channel-free environment is built
in one helper, test results are published atomically and read only
when parseable, and CONTEXT.md scopes the host-started claim to the
hosted path.
A remaining design point is not changed here: the channel is injected
into every locally launched ACP agent, not only the on-call backend, so
any agent that holds the key can ask the host to start a process
outside the job. The risk section below treats that as the same
capability the agent already has as the user, not a new boundary.
tools/ops.pyalso still treats the new "may have started the job"error as a refusal when it reports "Nothing is running"; that caller
needs a distinct indeterminate state, which is not part of this PR.
Type
Verification
Windows 11, CPython 3.12.13:
ruff checkandruff format --checkover the changed files are clean.ty checkon the two changed source files is clean.lint-importskeeps all ten contracts. The new head's checks all passed (2 skipped:
docs deploy and the claude job), including the Windows ACP job (79
passed), the four Ubuntu unit shards, the coverage gates (diff
coverage 92.14% against the 90% threshold), and the commit-message
check.
The new tests cover:
IsProcessInJobtrue, and does not survive the job object closing;agent's job object;
ends when that job closes, and closing the job ends it mid-run;
connects and sends nothing both leave the server able to serve the
next request (the accept loop survives);
answer, its request-size/object/env validation, and a failed
handshake that must not stop the server;
same way;
that never answers is bounded by the deadline: before the request
goes out it falls back, after it raises;
RAVEN_ACP_HANDOFF_*and writesits note only after the relay started;
replaced;
and an accept loop that keeps failing gives up for a rebuild;
the stdlib's length error;
the client deadline outlasts the host's request plus relay budget;
runs the relay off the event loop;
and a result file that is still being written is not read early;
inline key handshake, and the request's poll/validation errors (the
diff coverage gate).
Mutation checks, each watching its test fail:
the job and writes after the job closes;
thread is gone and the next client hangs; the real stalled-handshake
test fails the same way;
recv/send(the fakes assert they are never called);thread still alive after the join;
inherited key and on a log written for a relay that never started;
_HANDOFF_TRIEDlatch, the retry and dead-server tests fail;Everyone ACE; without the accept failure limit, it counts 50
attempts instead of 5; without the close wake, the closed-server
test leaves the accept thread alive;
job.log; with the relative path left as is, the absolute-path test
fails; with a synchronous relay start, the event-loop test sees the
main thread; with the 60-second deadline, the ordering test fails;
with the old existence-only read, the still-writing test raises
JSONDecodeError;
72.32%.
Known failures on this machine, reproduced on the unmodified head and
not caused by this change:
tests/test_subagent_acp.pyhas the same 19failing ids at the base and at the previous head (the diff of the id
sets is empty);
tests/test_cli_agents_commands.pyhas the same fourfilesystem failures at the base;
tests/test_acp_mirasim_server.pystops at test 47 at the base and at the previous head. The new head's
four Ubuntu unit shards and Windows ACP job passed; the Windows ACP
job runs the two hand-off test files.
Risk
DACL for SYSTEM, Administrators, and the owner, so another local user
cannot connect before the key is checked. The channel can start a
process outside the agent's job, but only when a process in the
agent's tree asks it to, with the key from its environment. This is
narrower than a breakaway permission, which Git for Windows spends
automatically on every native child. An agent already runs as the
user; kill-on-close was cleanup, not a boundary. The request/answer
framing is JSON over the authenticated pipe, so a key holder cannot
execute code in the host by sending a pickle; the payload is limited
to 1 MiB.
Windows, a host without the channel gets the old behaviour plus one
log line; the channel and its JSON framing are new in this branch, so
there is no older wire format to keep.
review rounds and the branch returns to the host-relay design; revert
the whole branch and jobs go back to ending with the host.
Related Issues
#951, #1016