test: own the unitd process tree in conftest — group kill ladder, pgid file - #34
test: own the unitd process tree in conftest — group kill ladder, pgid file#34andypost wants to merge 6 commits into
Conversation
…d file
- unit_run(): spawn unitd with start_new_session=True so it leads its own
session/process-group (pgid == pid); record the pgid in-memory and to
<temp_dir>/unitd.pgid immediately at spawn, so the whole family (main,
controller, router, app workers) can be signalled as a unit and never
confused with the runner's own group or an unrelated unitd on the box.
- unit_stop(): replace the bare `except: p.kill()` with an escalation ladder —
SIGQUIT main, wait STOP_TIMEOUT, then on overrun SIGTERM the process GROUP,
bounded wait, SIGKILL the group. KeyboardInterrupt reaps the group before
re-raising. STOP_TIMEOUT from UNIT_TEST_STOP_TIMEOUT (default 15; slow/32-bit
CI should set 60+). Guards never signal group <=1 / our own group.
- Startup-timeout leak fix: on waitforfiles() timeout, reap the spawned group
before sys.exit('Could not start unit'); start budget is env-tunable via
UNIT_TEST_START_TIMEOUT (default 5s keeps prior behavior). Stop paths now use
unit_instance.get(...) so a failed startup can't KeyError.
- atexit hook reaps any recorded-but-alive pgid as a best-effort safety net,
plus a SIGTERM-safe path; a hard SIGKILL/SIGBUS of the runner still defeats
in-process handlers (external containment is the D4 wrapper's job) — the
on-disk pgid file is the contract external sweepers consume.
- test_idle_close_wait.py: scope the router lookup to OUR daemon
(pgrep -P <main> -f 'unit: router') so it can never sample another unitd
instance on a shared box. (_check_processes already word-boundary matches.)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces robust process group management and cleanup for unitd processes spawned during testing, preventing leaked processes when tests are interrupted or fail. It leverages process groups to signal and reap the entire process tree, registers an atexit handler, and handles SIGTERM signals. Additionally, it introduces configurable start and stop timeouts via environment variables and scopes the router PID lookup in test_idle_close_wait.py to the specific unit PID. Feedback was provided to optimize the process state check in _group_alive by reading /proc/<pid>/stat directly instead of repeatedly spawning ps subprocesses, which can cause high CPU usage and slow down tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Spawning a ps per pid per 0.2 s poll tick adds real load on exactly the slow builders the ladder timeouts exist for; read /proc/<pid>/stat directly instead (parse from the last ')' -- comm may contain spaces). The pgrep -g group listing stays: name-based, one call per tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 63ca7d4478
ℹ️ 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".
- Run the atexit/SIGTERM reap ONLY in the pytest runner process: with the fork start method (the multiprocessing default on Linux through 3.13) run_process helpers inherit both the SIGTERM disposition and _pgids, and stop_processes terminates them with SIGTERM -- an inherited sweep then TERM/KILLed the still-active Unit tree mid-session, cascading every later test (reproduced with a fork-context helper; the fix verified both ways). - Enumerate process groups from /proc instead of pgrep: removes the subprocess-per-poll-tick and the ambiguity between 'no processes' and 'pgrep failed/absent' -- the latter mistaken for an empty group would skip the TERM/KILL escalation and leak the tree. /proc unavailable is treated as alive (killpg needs no /proc; worst case is a timeout wait). - Forget pgids once their tree is confirmed gone (_forget_pgid at every unit_stop exit): the kernel reuses pid/pgid numbers, and sweeping a stale entry at interpreter exit could signal an unrelated group. - Reap the group when SIGQUIT exits nonzero: an abnormal graceful shutdown can leave group members behind, and returned before cleanup. - Keep unitd.pgid across _clear_temp_dir: deleting it right after startup left a hard-killed runner's orphan tree unidentifiable to external sweepers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Round-2 review addressed in 155084b. The P1 was real and live: it explains the python-3.11/3.12 CI failures on the previous head (fork-default multiprocessing inherits the SIGTERM sweeper into run_process helpers; the helper's teardown SIGTERM then killed the active Unit group at test_proxy teardown — my local 3.14 box defaults to forkserver, which masked it). Reproduced with a fork-context helper pre-fix, green post-fix. Also: /proc-based group enumeration replaces pgrep (removes the failure/empty ambiguity and the per-tick subprocess), pgids are forgotten once their tree is confirmed dead, the nonzero-SIGQUIT path reaps before returning, and unitd.pgid survives temp-dir cleanup. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 155084b3a7
ℹ️ 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".
…d dead - Probe the group with killpg(pgid, 0) first: fully portable ESRCH fast path for the suite's supported non-Linux platforms. The /proc scan stays as the Linux refinement (killpg succeeds on zombie-only groups), with an explicit fallback to the killpg verdict on unrecognized procfs layouts; a process vanishing mid-scan is a routine race and keeps scanning. - _forget_pgid also unlinks <temp_dir>/unitd.pgid once the group is confirmed gone: --save-log retains temp dirs, and a stale on-disk kill contract could point an external sweeper at a reused pid/pgid long after the tree stopped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9336ff448
ℹ️ 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".
- Track each pgid with its temp dir (_pgids is now a mapping) so BOTH kill contracts -- the in-memory id and the on-disk unitd.pgid file -- are retired together once the group is confirmed gone, including from the atexit/SIGTERM safety net, which runs exactly in the aborted- session cases whose temp dirs stay behind for external sweepers. - Reap the group after a clean zero-retcode SIGQUIT too: the master exiting does not prove the group is empty, and a surviving worker is reparented -- outside _check_processes' ppid filter -- so it would silently ride into the next test. _reap_group is a no-op when the group is already gone. Verified: suite subset green; a --save-log run retains the temp dir with unit.log but no unitd.pgid after clean stop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22711af676
ℹ️ 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".
A graceful-shutdown overrun is a real bug class (SIGQUIT not completing in time) and the pre-ladder code always surfaced it as 'Could not terminate unit'; returning success after a working TERM/KILL escalation would mask such hangs. Keep the cleanup, keep the error -- with a distinct message so a wedge that even SIGKILL couldn't clear remains distinguishable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
went into freeunitorg#150 |
1 participant
D2 from the lifecycle roadmap on #32; shell-harness analogue landed there as e0a68ed. Related audit effort: #33.
Why
The pytest suite can orphan the entire unitd tree, and downstream packaging already works around it: Alpine's
community/freeunitAPKBUILD carriespkill -f 'unitd.*--no-daemon'— "kill orphaned unitd left by crashed pytest (armhf/armv7 SIGBUS)" (aports 8791acf1). Three leak paths inconftest.py:unit_stop()escalated top.kill()on the main pid only after a fixed 15 s wait — routinely exceeded on slow 32-bit builders — so router/controller/app processes re-parented to init and survived;waitforfiles()startup timeoutsys.exited without killing the unitd it had already spawned (and later stop paths could KeyError on the unset pid);pytest_sessionfinishentirely, and nothing on disk identified the tree to sweep.What
unit_run()spawns unitd withstart_new_session=True— the whole family (main, controller, router, app workers) shares one process group (pgid == leader pid), recorded in-memory and to<temp_dir>/unitd.pgidat spawn. The pgid file is the contract external sweepers can consume (e.g. the APKBUILD replacing its blanket pkill withkill -- -$(cat .../unitd.pgid)).unit_stop(): SIGQUIT main → waitUNIT_TEST_STOP_TIMEOUT(default 15 s; slow/32-bit CI should set 60+) → on overrun SIGTERM the group → bounded wait → SIGKILL the group. The bareexcept: p.kill()is gone; KeyboardInterrupt reaps the group before re-raising; a main that already exited still gets its surviving children reaped.UNIT_TEST_START_TIMEOUT(default preserves the 5 s behavior); stop paths use.get()so failed startup can't KeyError.atexit+ SIGTERM-safe best-effort reap of every recorded pgid (installed only when nothing else owns SIGTERM). A SIGKILL/SIGBUS of the runner still defeats in-process handlers — that is the containment wrapper's job (roadmap D4) — but the pgid file survives for external sweeps.test_idle_close_wait: router lookup scoped topgrep -P <main>— a barepgrep -f 'unit: router'matches any unitd on a shared box and sampled the wrong process (observed flake).Verification (executed)
UNIT_TEST_STOP_TIMEOUT=3→ ladder walked QUIT→TERM→KILL (~9.8 s), zero survivors.UNIT_TEST_START_TIMEOUT=2→ cleanCould not start unitexit, zero live survivors.test_idle_close_waitcross-instance flake is fixed by the scoping change.🤖 Generated with Claude Code
https://claude.ai/code/session_01MTsmYPCoxj2D5wW6jrwZ5S