Repository navigation
fix(acp_client): end an agent with its host on macOS and sweep what a dead host left - #953
Conversation
gloryfromca
left a comment
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 diff and surrounding AcpClient launch/close call chain, the launch-record history, backward compatibility of version-1 records, test changes for weakening, and the repository rules in AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and the ACP terminology in CONTEXT.md. I also checked the macOS process APIs used by the wrapper and sweeper and found no concrete failure.
Verification: uv sync --locked --extra dev --dev succeeded, then uv run pytest tests/test_acp_leftovers.py tests/test_subagent_host_env.py -x completed with 110 passed and 10 macOS-only tests skipped on Linux. The first run in the default environment stopped during fixture setup because the dev-extra raven_everos package was absent; no selected test had run before the environment was synchronized.
|
Windows compatibility review at No Windows compatibility regression found in the inspected five-file change. Read the complete diff, ACP launch/close callers, renamed helper references, platform guards and tests, plus existing discussion. Native Windows / Python 3.13.12: 48 passed, 79 skipped. The focused suite passed 47 tests and skipped 79 Linux/macOS-specific cases. An additional external probe passed through a real Verification used a detached exact-head checkout outside the active repository, verified source imports, isolated home/temp/token paths and a fresh uv environment. Setup: $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/0a65ac5061d248b5a51ea46664349e4b'
uv run pytest tests/test_acp_leftovers.py tests/test_subagent_host_env.py tests/test_acp_client_teardown.py tests/test_acp_client_launch.py -n 0 -x --tb=short --basetemp "$R/checks/953-focused/pytest" --junitxml "$R/checks/953-focused/junit.xml"
# 47 passed, 79 skipped
uv run pytest "$R/test_windows_leftovers_guard.py" -s -n 0 -x --tb=short --basetemp "$R/checks/953-native-guard/pytest" --junitxml "$R/checks/953-native-guard/junit.xml"
# 1 passedLimits: this verifies the Windows side of this PR. Native macOS watcher/sysctl/libproc behavior, native Linux behavior, the full suite, real vendor adapters and integration with separately proposed changes were not exercised. |
gloryfromca
left a comment
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 new test-and-coverage commit and re-read the full change in context. The coverage exclusions are limited to native macOS declaration/call shims, while the parsing, dispatch, growth, and error behavior is now exercised with fakes on Linux; the real macOS integration tests remain intact. I also rechecked the callers, history, backward compatibility, repository rules and domain terms, architecture boundaries, and that tests were extended rather than weakened.
Verification: git diff --check github/main...HEAD passed, and uv run pytest tests/test_acp_leftovers.py tests/test_subagent_host_env.py -x completed with 127 passed and 10 macOS-only tests skipped on Linux. The completed CI checks are green; the sharded unit and coverage-dependent jobs were still running when reviewed.
|
Windows compatibility re-review at No Windows compatibility issue found in the inspected scope. Reconciled the full five-file PR diff with the previous review: three files are unchanged; the two-file revision adds reader tests and coverage annotations, without changing production logic. Rechecked platform guards, launch callers and all discussion pages. Native Windows / Python 3.13.12: 65 passed, 79 skipped. The 17 new platform/reader cases pass using replaced native calls. The real ACP launch/request/close probe also passes at this head from a space/non-ASCII path; the Windows launch does not enter the macOS loader or POSIX lifetime hooks. Detached exact-head checkout, fresh uv environment, verified source imports and isolated home/temp/token paths. Setup: $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/e46b3308eaef431f8bb15118118840f1'
uv run pytest tests/test_acp_leftovers.py tests/test_subagent_host_env.py tests/test_acp_client_teardown.py tests/test_acp_client_launch.py -n 0 -x --tb=short --basetemp "$R/checks/953-focused/pytest" --junitxml "$R/checks/953-focused/junit.xml"
# 64 passed, 79 skipped
uv run pytest "$R/test_windows_leftovers_guard.py" -s -n 0 -x --tb=short --basetemp "$R/checks/953-native-guard/pytest" --junitxml "$R/checks/953-native-guard/junit.xml"
# 1 passedLimits: Linux/macOS-specific cases skip on this host. Passing the reader tests with fakes does not verify native macOS APIs or watcher behavior. No full-suite run, real vendor-adapter run, or integration with separate proposed changes. |
… dead host left An ACP agent runs in a session of its own and, while its host lives, only AcpClient.close ends it. A host killed, crashed or OOM-killed before its teardown left every agent it launched running. Linux was closed by the parent-death wrapper and the launch-record sweep; on macOS both were no-ops. - The exec wrapper now has a macOS binding. macOS has no PR_SET_PDEATHSIG, so before the exec the wrapper double-forks a watcher that launchd adopts. It stays in the agent's process group, closes every inherited descriptor, leaves the agent's directory, waits on the host's exit with kqueue (EVFILT_PROC / NOTE_EXIT) and then ends the whole group, so a launcher's grandchild goes too. It ends the group only while the agent leads it, and any failure leaves the agent as it was. close() ends the watcher with the rest of the group. - The launch records and the sweep now run on macOS, with each /proc read replaced: process start time from sysctl KERN_PROC_PID, group members from proc_listpids(PROC_PGRP_ONLY), the launch mark from KERN_PROCARGS2, the boot from kern.bootsessionuuid and kern.boottime. macOS has no PID namespaces and hides the environment of its platform binaries, so only a member that is not one can show the mark. What cannot be read still leaves a group alone. - The Linux wrapper string is unchanged. Windows is not covered and stays tracked in #928. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
The coverage gate runs on Linux, where none of the macOS readers execute, so the diff measured 35.83% against a 90% threshold. - The three functions that only declare or call native macOS APIs (_darwin_libc, _sysctl, _sysctl_by_name) are marked no cover, as the repo does for its native Windows declarations. - The rest is tested on every platform against faked native answers: the start-time, environment and boot parsers, the member list growing until the group fits, the error paths, and the platform dispatch. The tests that read the real system on macOS stay as they were. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
8ea3a16 to
c7e35e3
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision rebases the change onto the merged Windows ACP teardown. I checked the range diff and the full diff against the updated main: the macOS commits retain their behavior, the POSIX host-binding wrapper and Windows job-object launcher remain mutually exclusive at the launch call site, and the conflict resolutions update only the shared documentation and wrapper wording. I also rechecked callers, history, compatibility, test strength, repository rules and domain terms, and architecture boundaries.
Verification: git diff --check github/main...HEAD passed. uv run pytest tests/test_acp_leftovers.py tests/test_acp_windows.py tests/integration/test_acp_windows_real_process.py tests/test_subagent_host_env.py tests/test_subagent_acp.py -x completed with 460 passed and 10 macOS-only tests skipped on Linux. Completed CI checks are green; sharded unit jobs were still running when reviewed. No review threads are open.
|
Windows compatibility re-review at No Windows compatibility issue found in the inspected scope. Read the complete five-file diff, shared ACP launch/close callers, platform guards, tests and existing discussions. The macOS implementation and its test file are byte-identical to the previously reviewed head; the Windows Job Object implementation and tests are byte-identical to the new base. On Windows, the POSIX wrapping/mark/record/sweep paths still return before native POSIX calls, and the shared launch proceeds through Native Windows / Python 3.13.12: 97 passed, 79 skipped. This includes 11 real-process tests covering host kill/abrupt exit/close, an agent exiting before its descendant, a nested job, host death around job assignment, independent connections, a Verification used a detached exact-head checkout, verified source imports, isolated home/temp/token paths and a fresh environment from $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/0351d0adb12c4b0696a97af5e5511105'
# 85 passed, 79 skipped
uv run pytest tests/test_acp_leftovers.py tests/test_subagent_host_env.py tests/test_acp_client_teardown.py tests/test_acp_client_launch.py tests/test_acp_windows.py -n 0 -x --tb=short --basetemp "$R/checks/953-focused/pytest" --junitxml "$R/checks/953-focused/junit.xml"
# 11 passed, native Windows processes; integration marker explicitly enabled
uv run pytest tests/integration/test_acp_windows_real_process.py -m integration -n 0 -x --tb=short --basetemp "$R/checks/953-real-process/pytest" --junitxml "$R/checks/953-real-process/junit.xml"
# 1 passed, real launch/request/close with POSIX-hook sentinels
uv run pytest -c pyproject.toml -s "$R/test_windows_leftovers_guard.py" -n 0 -x --tb=short --basetemp "$R/checks/953-native-guard/pytest" --junitxml "$R/checks/953-native-guard/junit.xml"Limits: Linux/macOS-specific cases skip on this host. Reader tests with replaced native calls do not verify macOS sysctl/libproc/kqueue behavior. No native macOS/Linux run, full suite or real vendor adapter was exercised. |
…nce, a dead-link cancel (#957) ## Summary Seven edges of a run on a registered machine, left by the review of #934 and by timing probes on the real machine. Each is a fix the host or the node makes on its own; nothing changes for an entry without `machine`. - **A Raven node killed outright no longer leaves agents behind or a bare `exit -1`.** The next daemon brings every record left at `running` to `lost` at its start (`Daemon.recover`), ends the agent when its pid is still the process it was started as (group leader, the recorded command, and the start mark the launch recorded: Linux's `/proc` start tick, elsewhere `ps -o lstart`), and writes why; a record without the mark, or a pid whose mark differs, is left alone and recorded as gone. `agent/exit` and the attach answer carry that `reason`; the host hands it on in place of the exit code (`ChannelEndpoint.exited(rc, reason)`), so a cli run says "the Raven node on this machine was restarted while the agent ran, and the agent is gone" and a picked-up acp run says the same. On Linux the agent is also tied to the daemon's life (`PR_SET_PDEATHSIG`, a SIGTERM). Protocol number unchanged: the field is optional and old nodes never send it. - **A failed remote run's error survives a draining-refused announce.** The manager's two error branches read whether the announce was taken, as the reply branch does (`announced = taken is not False`), and the acp backend keeps the record of a prompt that went out and failed, the cli backend that of a timed-out command, for the caller that reports it. The next start reads how the task ended off the machine and announces the error. - **A cli run picked up after a restart binds its instance to the session id in its output** (`_bind_picked_up`), as the dispatch's deferred commit would have; with no id in the output the reply carries the same "not resumable" warning the dispatch would. A `provisioned` id or a stateless run binds nothing. - **A cancel while the machine's link is known dead returns at once.** `_cancel_turn` reads `ChannelEndpoint.offline` (parked, or the link not alive), marks the session unsettled and leaves the held `session/cancel` for the next link, instead of waiting the settle budget for a settlement that cannot arrive. In the gap before the heartbeat notices nothing changes. - **An openai row's tunnel gives up a taken local port for another.** The port is checked again just before ssh starts (`_still_free`), and ssh's own "Address already in use" / "Could not request local forwarding" ends in another try (`_TUNNEL_TRIES` = 3) rather than in a request sent to whoever took the port; each attempt reads only what ssh wrote in its own run of the log. - **A run record that is not JSON, or not UTF-8, is set aside once** (`<name>.json.unreadable-<ts>`), named in the log that once, instead of being warned about at every start; a record a later version wrote is left alone as before. (The first revision read the record as text and let a `UnicodeDecodeError` out of the whole scan, found by the Windows review; the record is read as bytes now and decoded by `json.loads`, so a decoding failure is set aside like a JSON one, and one bad record never stops the others from being read.) - **A path written as remote that names no registered machine** is refused in one sentence for `write_file` and `edit_file` too, never taken for a relative path here. This has been the case since the debug switch went (#934); a regression test pins it. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Every fix has a test that failed on the old code first. - `.venv/bin/python -m pytest -n 6 -q tests/test_node_server.py tests/test_node_client.py tests/test_remote_runs.py tests/test_subagent_manager.py tests/test_subagent_cli_remote.py tests/test_remote_file_tools.py tests/test_subagent_acp_remote.py tests/test_resume_remote.py tests/test_remote_resume.py tests/test_subagent_acp.py` - all green on macOS (the Linux-only pact test skipped there); the last run of the four fast files 331 passed, cli remote 52, remote file tools 24, the acp remote recovery tests 3 - Full suite: 31577 passed, 234 skipped, 2 xfailed in 6 min 14 s; the 3 failures are the ones `main` has on this machine (a host without LibreOffice, two simulation records). Changed-line coverage: 98.18% of 165 executable changed lines (`scripts/coverage_gate.py diff --base-ref origin/main`); the 3 uncovered lines are the Linux-only `prctl` import block, which CI's Linux shards run. - `ruff check` / `ruff format --check`, `ty check` on the changed modules, `lint-imports` (10 contracts kept), `scripts/check_commit_file.py`, `scripts/check_source_language.py`, `scripts/check_large_files.py`: clean. - Real machine (ravenx2, an isolated registry copy and Raven home per tree; the node of each tree installed under its own `node_dir`; the same probe script on this branch and on `main`): | Probe | `main` | this branch | |---|---|---| | P1 `kill -9` the daemon under a running cli turn | 4.9 s after the kill: `CLI agent 'x' exited -1`; the loop the daemon had started was still running (1 left) | 5.1 s: `CLI agent 'x': the Raven node on this machine was restarted while the agent ran, and the agent is gone`; 0 left; a new daemon pid | | P2 an acp prompt that fails after it went out, `reported_by_caller` | the prompt failed (`exit 5`); no record left | record kept; the next start announced the error with `exit 5` and removed the record | | P3 cancel while the link is known dead; the cancel on the next link | the cancel returned after 2.35-2.46 s, held until the link was back (a 2 s backoff in the probe; up to the 5 s budget in production); it landed on the next link (task frames 3->4) | 0.00 s; landed the same way (3->4 within 0.1 s of the restore) | | P4 the chosen local tunnel port is taken by a listener | the tunnel used the taken port; the request timed out against the listener that held it | the next port; HTTP 200 through the machine | | P6 a derived-id cli create left running, picked up by the next start | picked up ok, no binding (`commits=[]`) | picked up ok, the handle bound to `edge-123` | After the merge of `main` (which brought #947's MCP streams, #953's host-death sweep and #963's install fixes into the same files): the related groups (node server/client/bridge, remote runs, resume, cli remote, acp remote, manager, remote file tools, acp client, import-cycle budget) 1006 passed; diff coverage 97.30% of 185 changed lines (the 5 uncovered are the Linux-only `prctl` lines, run by CI's Linux shards); the real-machine probes re-run on the merged tree: 8/8 PASS (P1 5.2 s, 0 orphans; P2 record kept and the error announced; P3 0.00 s and the cancel landed on the next link; P4 the next port, HTTP 200; P6 bound to `edge-123`). Docs checked against the code before pushing: CONTEXT.md's Raven node entry (records of failures, set-aside records, the daemon's recovery and the `reason`), the module docstrings of `remote.tunnel`, `remote_runs.pending`, `Daemon.recover`, `_still_ours`, `_death_pact`, `ChannelEndpoint.exited/offline`, `_bind_picked_up`, `_track_on_machine`, the `agent/exit` comment in `protocol.py`, and the CHANGELOG entry. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] User-facing docs or screenshots are updated when needed ## Risk - [x] Security impact considered - the daemon ends only a pid that is still the process it started: group leader of its own group, running the recorded command, and started when the record says (`born`, written at the launch); a reused pid fails the last mark even for the same program and is left alone, the record then saying the agent is gone. No new network surface; the `reason` is the node's fixed sentence, never user or agent text. - [x] Backward compatibility considered - `agent/exit`'s `reason` is optional, protocol number unchanged; a host on this branch reads an old node's exits as before, an old host ignores the field. Records written before this branch read unchanged. - [x] Rollback path is clear for risky changes - a plain revert; nothing is migrated. ## Related Issues Follow-up to #934 (the two non-blocking notes of its review) and to the timing probes run on #921/#922/#934. --------- Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
Summary
On macOS an ACP agent outlives a Raven host that dies without closing it (killed, crashed, OOM-killed). The agent runs in a session of its own, so it gets none of the host's signals, and only
AcpClient.closeends it. #942 closed this on Linux with a parent-death wrapper and a launch-record sweep. On macOS both were no-ops. This PR brings both layers to macOS.1. The agent ends with its host (
raven/acp_client/leftovers.py,_DARWIN_BINDING). macOS has noPR_SET_PDEATHSIG. Instead, the exec wrapper double-forks a small watcher before it execs the agent, then reaps the middle child./.EVFILT_PROC/NOTE_EXIT), then ends the whole group. Annpxlauncher's grandchild goes with it, which the Linux signal does not reach.ESRCH, and that counts as an exit.close()ends the watcher along with the rest of the group.2. The sweep at the next launch runs on macOS. This is the backstop for when the watcher itself is gone. The decision logic is unchanged; each
/procread has a macOS equivalent:sysctl KERN_PROC_PID(kinfo_proc.p_starttime, in microseconds). It reads any process, including launchd and zombies;proc_pidinforefuses launchd with EPERM.proc_listpids(PROC_PGRP_ONLY)sysctl KERN_PROCARGS2. EINVAL (gone, zombie, or another user's process) counts as no mark.kern.bootsessionuuidandkern.boottime. macOS has no PID namespaces, so the namespace is a fixed value.macOS hides the environment of its own platform binaries (
/bin/sh,/bin/sleep). Only a group member that is not one can show the mark, such as the agent's node or python process, or the watcher. What cannot be read still leaves a group alone.Linux is unchanged. The Linux wrapper string is byte-identical to main's
_WRAPPER._start_ticksis renamed to_start_time, which now dispatches by platform.CONTEXT.md(Leftover agent, Launch mark) is updated to match.Windows lifetime handling is provided by #951. This PR adds the macOS wrapper and sweep; these helpers remain a no-op on Windows.
Type
Verification
Before rebasing onto the merged Windows lifetime change, on macOS 26.6.2, arm64, Python 3.12 (project venv):
uv run pytest tests/test_acp_leftovers.py tests/test_subagent_host_env.py tests/test_acp_client_teardown.py tests/test_acp_client_launch.py -q -p no:cacheprovider -m "slow or not slow"-> 117 passed, 9 skipped (Linux-only mechanisms).tests/test_*acp*.pyandtests/test_*subagent*.pyfile, run one file at a time -> all pass excepttests/test_subagent_dag_runner.py. On this Mac that file hangs at the same two points with this branch and with main'sleftovers.py:TestBackgroundRun::test_a_run_that_ignores_cancellation_fails_named_instead_of_hanging_loop_closefails;TestBackgroundRun::test_the_failure_path_reaps_a_subagent_child_left_running(21 passed in both runs).It is unrelated to this change; Linux CI covers that file.
AcpClient.launch, thenkill -9of the host). The stand-in agent ignores SIGTERM, never reads stdin, and starts a child of its own.kill -9the host. The agent stays (ppid 1). The next host's launch ends the agent and its child.uv run ruff format --checkanduv run ruff checkon the changed files -> clean.make check-source-languageandmake check-large-files-> clean.New macOS tests cover:
Most existing Linux tests now run on macOS too.
After rebasing onto the merged Windows lifetime change, on Linux / Python 3.12:
CONTEXT.md: retain both platform descriptions, scope the record sweep to Linux and macOS, and remove the stale sentence that macOS only has explicit teardown.uv run --no-sync pytest tests/test_acp_leftovers.py tests/test_acp_windows.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_subagent_host_env.py tests/test_subagent_acp.py tests/test_living_docs.py -q -x-> 468 passed, 10 macOS-only tests skipped.uv run --no-sync ty check raven/acp_clientanduv run --no-sync lint-imports-> passed; all 10 import contracts kept.uv run --no-sync pre-commit run --from-ref origin/main --to-ref HEAD-> passed for every applicable hook.make check-large-files check-source-language check-commitswith the existing environment andUV_NO_SYNC=1-> passed.Native macOS and Windows tests were not repeated after this documentation-only conflict resolution. The current-head Windows re-review before the rebase reported 65 passed and 79 platform-specific skips.
Risk
User-visible: on macOS an ACP agent is now SIGKILLed with its whole process group when its host exits without closing it. On Linux the parent-death signal ends the direct child; the launch-record sweep handles surviving marked descendants. An agent that relied on outliving its host no longer does.
Each launched agent on macOS carries one extra idle process (about 4 MiB) in its group until the group is closed or the host exits.
The sweep only acts on groups recorded by a dead host in the same boot, whose leader start time matches and which still hold a member carrying that host's mark. A recycled pid or group number is left alone.
Rollback: revert this commit. Record files a macOS host left under
acp-launches/are then inert, because the sweep is a no-op on macOS again.Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
Related Issues
#928 (macOS); #951 provides the Windows lifetime handling.