feat: add optional, Windows-gated zombie-killer-tray integration - #2
Conversation
T-20260926-368033290 (c) / T-20260926-212716751: adds an OPTIONAL integration with zombie-killer-tray (conservative cleanup of orphaned MCP and language-server processes), mirroring the pattern already used by dev-bricks/CareCenter-for-Codex for this same tool -- a git-pinned pip dependency, launched as its own subprocess via `python -m zombie_killer_tray watch ...`, coordinated through the shared zombie_events.jsonl audit log, never an in-process import. Safe Start for Codex is explicitly cross-platform (Windows/Linux/macOS, zero required runtime dependencies); zombie-killer-tray is Win32-only. This integration is therefore doubly gated to keep that intact: - New `[project.optional-dependencies].zombie-killer` extra (never installed implicitly), itself carrying a PEP 508 `sys_platform == 'win32'` marker so `pip install .[zombie-killer]` can't even attempt to resolve zombie-killer-tray's own Win32 dependencies on Linux/macOS. - `launch_zombie_killer_watch()`/`build_zombie_killer_status()` check a small `_is_windows()` seam and refuse/report cleanly on any other platform rather than attempting a doomed subprocess spawn. (Deliberately NOT monkeypatching `os.name` directly in tests for this -- doing so on an actual Windows test runner breaks pathlib's own class dispatch for any Path() constructed afterwards; verified this the hard way before introducing the `_is_windows()` seam.) New `zombie_killer_integration.py`: target resolution (local sibling checkout override via `SAFE_START_ZOMBIE_KILLER_SOURCE`, else the pinned GitHub spec), install/launch/status functions. Three new CLI subcommands in cli.py mirroring the rest of its subparser style: `zombie-killer-report` / `-install` / `-watch`. Updated THIRD_PARTY_LICENSES.txt/.md and tests/test_third_party_licenses.py's exact-dependency-set assertion to cover the new optional dependency. Pinned to dev-bricks/zombie-killer-tray@6c8eb2c (that repo's PR #4, src/-packaging, not yet merged -- move this pin to the merge commit once it lands, same as the CareCenter-for-Codex integration PR). 139/139 tests green (128 baseline + 11 new), ruff clean. Not merging -- per the two-model rule, a different model/session should review before merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk
|
Welcome! 👋 Thanks for your first pull request in this repository. A maintainer will review it soon. Please make sure:
Thanks for contributing! |
zombie-killer-tray#4 (src/-Paketierung) merged as 039b4f2. Updates the pinned dependency in pyproject.toml's zombie-killer extra, the module-level ZOMBIE_KILLER_PACKAGE_SPEC constant, and both THIRD_PARTY_LICENSES entries from the pre-merge branch head (6c8eb2c) to the actual merge commit. Verified: `import zombie_killer_tray.killer` resolves against this pin in a clean venv. 139/139 tests green, ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk
|
Pin updated: |
lukisch
left a comment
lukisch
left a comment
There was a problem hiding this comment.
Review (merge-reviewer, claude-opus) auf Head f39a504: nicht gemergt. Lokal 139 passed, das Windows-Gating in launch_zombie_killer_watch ist sauber (Nicht-Windows: unsupported-platform, kein Start), der Kill-Pfad bleibt der von zombie-killer-tray (nur Parent tot).
Blockierend:
- Paket nicht mehr installierbar: Die direkte Git-Referenz im Extra
zombie-killerbraucht bei hatchling[tool.hatch.metadata] allow-direct-references = true. Ohne das scheitert schonpip install -e .(CI: test 3.13 und alle smoke-Jobs rot; lokal reproduziert: „cannot be a direct reference unless field tool.hatch.metadata.allow-direct-references is set to true“). CareCenter hat die Einstellung. - Watcher stirbt sofort (gleicher Fehler wie CareCenter#2):
zombie-killer-watchübergibt--parent-pid os.getpid()und das CLI beendet sich danach. zombie-killer-tray beendet sich mit dem Parent. Empirisch mit Stand 039b4f2 nachgestellt: der Watcher überlebt keine Sekunde. Bitte Lebenszyklus neu lösen und mit einem echten, nicht gemockten Test belegen.
Review findings on this PR: (1) LIFECYCLE BUG (blocking): launch_zombie_killer_watch() passed --parent-pid <this process's own PID>. zombie-killer-tray's watch_parent thread kills the watch process as soon as whatever PID it was given for --parent-pid exits -- and the CLI process that spawns the watcher exits immediately after Popen returns, so the watcher died within ~1s of being started, before doing any real work. Mocked-Popen tests never caught it because they don't run the real subprocess lifecycle at all. Fix: drop --parent-pid entirely. The watcher is now a genuinely detached, long-lived process; its lifecycle is governed by its own PID file (<state_dir>/watch.pid) rather than any parent/child relationship. Added stop_zombie_killer_watch() (reads the PID file, verifies via psutil that the PID still belongs to zombie-killer-tray before signalling it) and a new `zombie-killer-stop` CLI subcommand. Added a REAL integration test (no mocked Popen, skipped on non-Windows) that spawns the actual zombie-killer-tray package as a subprocess exactly the way production code does, asserts it is still running 2s after the launching call returns (the old bug would already have killed it by then), then asserts zombie-killer-stop actually terminates it. Requires the package installed; added the same pip-install step to ci.yml (windows-latest only, matching zombie-killer-tray's own platform requirement -- the cross-platform source-platform-smoke.yml workflow only runs tests/source_platform_smoke.py and never collects this file). (3) [tool.hatch.metadata] allow-direct-references (blocking): the new `zombie-killer` optional extra declares a direct `git+https://...` reference, which hatchling refuses to build without this explicit opt-in -- `pip install -e .` failed with a ValueError during metadata generation. Reproduced the exact failure in a clean venv before adding the fix, then reproduced the fix working the same way. (2) is CareCenter-for-Codex-specific (taskkill /T, broad MCP markers) -- addressed in that repo's own PR, not this one; safe-start-for-codex has no equivalent code path. 143/143 tests green (128 pre-existing/unrelated-to-this-PR + this PR's own 15), ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk
|
Addressed both applicable review findings ( (1) Lifecycle bug — same root cause as CareCenter's: (3) (2) N/A — that finding is CareCenter-for-Codex-specific (taskkill /T, broad MCP markers); this repo has no equivalent code path. 143/143 tests green (128 pre-existing + this PR's own 15), ruff clean. |
lukisch
left a comment
lukisch
left a comment
There was a problem hiding this comment.
Re-Review (merge-reviewer, claude-opus) auf Head 3ca6652: nicht gemergt. Die Punkte aus dem ersten Review sind im Kern behoben: kein --parent-pid mehr, /T ist entfernt (CareCenter), die MCP-Regex ist grenzbewusst.
Real nachgestellt, und zwar bewusst mit watch ohne --yes (Preview, keine Kills), Launcher als eigener, sofort endender Prozess wie das CLI, zombie-killer-tray 039b4f2:
- Watcher lebt 3 s nach Ende des Launchers → ja.
stop→ok, Watcher beendet → ja. Keine Rest-Prozesse. - Stale PID auf einen fremden, lebenden Prozess →
not-found, nicht beendet → ok.
Blockierend:
3. Stop ist fail-open: Ist psutil nicht importierbar, gibt _is_our_watch_process None zurück, und stop beendet die PID aus der Datei ungeprüft. Nachgestellt: stale PID auf einen fremden Python-Prozess, psutil-Pfad None → stop meldet ok und tötet den Fremdprozess. Das Paket deklariert psutil nicht selbst; wer das Extra später entfernt, hat genau diesen Zustand. Der Test test_stop_reports_already_stopped_for_a_dead_pid legt dieses Verhalten sogar fest. Fix: bei None nicht beenden (fail-closed), Status unverifiable.
4. Doppelstart hinterlässt einen Waisen-Watcher: Ein zweites zombie-killer-watch überschreibt watch.pid, der erste Watcher läuft weiter und ist per stop nicht mehr erreichbar. Nachgestellt: nach dem Stop lebte der erste Watcher noch. Fix: vor dem Start prüfen, ob die PID-Datei auf einen lebenden, verifizierten Watcher zeigt, dann ablehnen oder melden.
5. Der „echte“ Integrationstest macht echte Kills: test_real_watch_subprocess_outlives_its_launcher_and_stop_terminates_it startet watch --yes --interval 3 --min-age 30. Der erste Zyklus läuft sofort und beendet auf der Maschine, die den Test ausführt (Dev-Rechner, CI), reale Waisen ab 30 s Alter. Ich habe ihn deshalb bewusst nicht ausgeführt. Ein Test darf keine realen Prozesse beenden. Fix: launch_zombie_killer_watch(..., apply=False) bzw. ohne --yes im Test (der Lebenszyklus ist derselbe).
6. py -3 als Kandidat: In gefrorenen Builds (sys.frozen, also der ausgelieferte EXE-Fall) ist py -3 der erste Kandidat. Dann steht die PID des py.exe-Launchers in watch.pid, der eigentliche Watcher ist dessen Kind. stop beendet nur den Launcher (die cmdline-Prüfung besteht, weil sie den Modulnamen enthält), der Watcher läuft als Waise weiter. Fix: die echte Watcher-PID bestimmen (Kind von py.exe) oder py als Kandidat für den Watch-Start ausschließen.
Nicht blockierend: Die PID-Verifikation per cmdline-Substring trifft auch einen fremden zombie_killer_tray-Prozess (z. B. den Watcher des Schwesterprojekts). Robuster wäre es, beim Start create_time in die PID-Datei zu schreiben und beim Stop zu vergleichen.
Zusätzlich blockierend (safe-start): CI rot auf Python 3.11. ERROR: Package zombie-killer-tray requires a different Python: 3.11.9 not in >=3.12. safe-start unterstützt 3.11, zombie-killer-tray verlangt >=3.12. Das Extra braucht zusätzlich den Marker python_version >= "3.12", und der CI-Schritt darf es unter 3.11 nicht installieren. pip install -e . geht jetzt. Lokal 142 passed (ohne den realen Kill-Test).
…rker
Re-review findings, all 4 addressed (mirrors CareCenter-for-Codex#2):
(1) stop was fail-open on unverifiable PIDs -- replaced _is_our_watch_process
with _verify_watch_process(pid, expected_create_time); stop now refuses
(status="verification-unavailable", process untouched) whenever
verification returns None instead of falling through to os.kill(). The PID
file now stores {"pid", "create_time"} (from the real watcher's own
psutil.Process(pid).create_time() right after spawn) for a create_time
cross-check on top of the cmdline check -- catches PID reuse the cmdline
check alone might miss. psutil declared explicitly in the `zombie-killer`
extra (was only relying on it arriving transitively).
(2) A second `zombie-killer-watch` silently overwrote watch.pid, orphaning
the first watcher. launch_zombie_killer_watch() now verifies any existing
PID file before spawning: confirmed-alive refuses a second start
(status="already-running"); unverifiable refuses too, fail-closed, same
direction as (1) (status="verification-unavailable"); only a confirmed-
dead/stale entry is cleared and a fresh watcher started.
(3) N/A for this repo in the strict sense (safe-start's launch function
never took a MaintenanceConfig, so there was no separate "test vs
production" config split) -- added the same apply: bool = True parameter
anyway for parity with CareCenter and switched the integration test to
apply=False, since the real subprocess it spawns could otherwise reap
real, qualifying orphans on whatever machine runs it (including CI).
(4) In a frozen build, sys.executable is the packaged host app and `py -3`
spawns py.exe as a WRAPPER that itself spawns the real interpreter as ITS
OWN child (Windows has no exec()) -- the PID Popen returned was the
launcher's, not the worker's, so stop never reached the actual watcher.
Replaced the multi-candidate launch loop with
_resolve_watch_python_executable(): not frozen -> sys.executable directly;
frozen -> resolve the real path once via each launcher candidate's own
`-c "import sys; print(sys.executable)"` stdout, then spawn directly
against that resolved path. Added a unit test simulating the frozen case.
Safe-start-specific: the `zombie-killer` extra's git dependency now also
carries a `python_version >= '3.12'` marker (zombie-killer-tray's own
floor; this project's is >=3.11) alongside the existing `sys_platform ==
'win32'` marker, so `pip install .[zombie-killer]` is a correct no-op on
3.11 instead of failing. ci.yml's separate zombie-killer-tray install
step (needed for the real integration test) is now gated with
`if: matrix.python-version != '3.11'` for the same reason -- verified the
marker syntax by actually installing `.[zombie-killer]` in a clean venv
(psutil + zombie-killer-tray both resolved correctly on 3.12/win32).
150/150 tests green (128 pre-existing + this PR's own 22), ruff clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk
|
All 4 re-review findings addressed ( (1)/(2)/(4) — same fixes: fail-closed (3) — added Safe-start-specific: the 150/150 tests green (22 new/updated), ruff clean. |
|
Runde-3-Review (merge-reviewer, claude-opus): real nachgestellt mit zombie-killer-tray 039b4f2, Watcher im Preview-Modus. (1) Ohne psutil → verification-unavailable, Fremdprozess lebt weiter. (2) Passende cmdline, aber create_time um 100 s abweichend → not-found, Watcher unberührt. (3) Zweiter Start → already-running. (4) Der reale Integrationstest ruft apply=False auf, im Kommando steht kein --yes, geprüft im Code und per Assertion. (5) Gefrorener Fall (sys.frozen): Aufgelöst wird der echte python.exe (nicht py.exe), die PID-Datei zeigt auf den Watcher selbst, sein einziges Kind ist conhost, das mit ihm endet. Stop beendet sauber, danach keine zombie_killer_tray-Prozesse mehr. Kill-Kriterium bleibt das von zombie-killer-tray (Parent tot). Nicht blockierend: Eine PID-Datei im Altformat (nackte Zahl aus Runde 2) würde in _read_watch_pid_file an AttributeError scheitern. Runde 2 wurde nie gemergt, im Feld existiert das Format also nicht. Merge. Das Extra hat den Marker python_version>='3.12', die CI installiert es nur ab 3.12. Lokal 150 passed, CI grün. |
1 participant
Summary
dev-bricks/CareCenter-for-Codexfor this same tool — git-pinned pip dependency, launched as its own subprocess (python -m zombie_killer_tray watch ...), coordinated via the sharedzombie_events.jsonlaudit log.[project.optional-dependencies].zombie-killerextra (never installed implicitly), itself carrying asys_platform == 'win32'PEP 508 marker.launch_zombie_killer_watch()/build_zombie_killer_status()check a small_is_windows()seam and refuse/report cleanly on any other platform.zombie_killer_integration.py+ 3 CLI subcommands (zombie-killer-report/-install/-watch) mirroring the existing subparser style incli.py.THIRD_PARTY_LICENSES.txt/.mdandtests/test_third_party_licenses.py's exact-dependency-set assertion.dev-bricks/zombie-killer-tray@6c8eb2c(PR #4, src/-packaging, not yet merged — move this pin to the merge commit once it lands, same note as the CareCenter-for-Codex integration PR).A note on the platform gate
Found this the hard way while writing tests: monkeypatching
os.namedirectly on an actual Windows test runner breakspathlib's own class dispatch for anyPath()constructed afterwards (NotImplementedError: cannot instantiate 'PosixPath'). Introduced a small_is_windows()seam instead of patchingos.name, and verified both the real-Windows and simulated-non-Windows code paths.Test plan
tests/test_zombie_killer_integration.py(target resolution, install ok/fail, launch args/cwd/failure, platform gate on both branches, status readback)ruff check .cleanNot merging — per the two-model rule, a different model/session should review before merge.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk