fix: re-validate PID identity before killing host processes (#43846) - #50468
Merged
Conversation
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-assignment |
1 |
unresolved-attribute |
1 |
unsupported-operator |
1 |
unresolved-import |
1 |
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`
tests/gateway/test_whatsapp_bridge_pidfile.py:179: [unresolved-attribute] unresolved-attribute: Attribute `readline` is not defined on `None` in union `IO[Any] | None`
tests/gateway/test_whatsapp_bridge_pidfile.py:85: [unsupported-operator] unsupported-operator: Operator `+` is not supported between objects of type `int | None` and `Literal[1]`
tests/gateway/test_whatsapp_bridge_pidfile.py:20: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues (2):
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`
run_agent.py:2984: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
Unchanged: 5970 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
alt-glitch
added
type/bug
Collaborator
|
Related: #43846 (original by @valentt, salvaged here with authorship preserved), #43847 (follow-up hardening tracker), #43044 (MCP orphan-reaper recycled-PGID sibling), #50417 / #14073 (merged browser-daemon analog of the same hazard class), #15008 (SIGTERM→SIGKILL escalation — a separate concern, intentionally not folded in). This salvage PR is the canonical kill-safety fix for |
teknium1
force-pushed
the
salvage/pr-43846-kill-safety
branch
from
June 21, 2026 23:54
75658b5 to
d57998a
Compare
…rocesses The background-process registry signalled host PIDs (recovery adoption, detached-session kill, tree-kill) using a number captured at spawn, guarded only by a bare liveness check. Once a session's process exits and is reaped the kernel recycles that PID onto an unrelated process, so an alive-but-different PID passed the check and got tree-killed. Observed in the wild: a recycled background-session PID landed on Firefox's session leader; a later kill/refresh walked its process tree and SIGTERMed every tab — Firefox "closing" at irregular intervals with no crash/coredump. This is the same PID/PGID-recycling class fixed for the MCP orphan reaper in 7bd1f8a, but the process_registry subsystem was never guarded — so the bug persisted. Fix: record each host process's kernel start time (/proc/<pid>/stat field 22) at spawn, persist it in the checkpoint, and re-validate it before every signal via `_host_pid_is_ours`. A PID whose start time no longer matches — or that is gone — is never signalled: - recover_from_checkpoint: a recycled PID is not adopted as a session. - _refresh_detached_session: a recycled detached PID is marked exited. - kill_process / _terminate_host_pid: refuse to tree-kill a stranger. Legacy checkpoints and platforms without /proc (no baseline) degrade to the prior best-effort liveness behaviour, so nothing else changes. Adds TestPidReuseGuard: real-process tests proving a mismatched start time refuses termination while a matching one still kills, plus recovery/refresh recycling paths. 74 registry + 22 MCP-stability tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ile entry
`_kill_stale_bridge_by_pidfile` SIGTERMed the PID recorded in `bridge.pid`
after only a bare liveness check. Once the bridge exits and is reaped the
kernel recycles that PID onto an unrelated process; because the WhatsApp bridge
crash-loops ("Bridge process died (exit code 1)" repeating), this cleanup ran
on every restart and could SIGTERM a recycled PID that had landed on the user's
browser — closing Firefox at irregular intervals with no crash and no coredump
(a clean kill of a stranger).
Same PID-recycling class as the MCP reaper (7bd1f8a) and the process-registry
host-PID guard (e6a99cef2); this was the third, and most actively-fired, path.
Fix: `_write_bridge_pidfile` now also records the leader's kernel start time
(line 2). `_kill_stale_bridge_by_pidfile` re-validates identity via
`_bridge_pid_is_ours` before signalling — the (pid, start time) pair must match,
or for legacy single-line pidfiles the live cmdline must name `node` + this
session's unique path. A recycled PID (different start time / cmdline) is logged
and skipped, never signalled. Legacy pidfiles stay readable.
Adds TestWhatsappBridgePidfile: real-process tests proving a genuine bridge is
reaped while a recycled PID (start-time mismatch, or non-bridge cmdline) is
spared. 7 new + 108 gateway/registry tests green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r clients This is the bug that was actually closing Firefox. `_kill_port_process`, run on every bridge (re)start to free the port, used `lsof -ti :PORT` / `fuser PORT/tcp` — both of which match a process whose socket merely *involves* that port number in ANY state, including ESTABLISHED client connections. It then SIGTERMed every match. The bridge defaults to port 3000 — a ubiquitous local dev-server port. With a browser tab open on localhost:3000, `lsof -ti :3000` returned Firefox's PID, so each restart of the (crash-looping) WhatsApp bridge SIGTERMed Firefox, closing the whole browser at irregular intervals with no crash and no coredump. Proven live with the kernel `signal:signal_generate` tracepoint: hermes-gateway(3396516) -> sig=15 (code=0/SI_USER) -> comm=firefox pid=3371585 captured immediately after a gateway start, while Firefox held a socket on the bridge port. Demonstrated over-match: `lsof -ti :8080` returns the listener AND the gateway's own client connection; `lsof -ti tcp:8080 -sTCP:LISTEN` returns only the listener. Fix: `_listener_pids_on_port` resolves only LISTEN-state sockets (`lsof -ti tcp:PORT -sTCP:LISTEN`, with an `ss -ltnp` fallback) and `_kill_port_process` signals just those. A client whose connection happens to involve the port number is never touched — which is also more correct, since a client never blocks the new bridge from binding. Windows already filtered LISTENING; the broad `fuser -k` path is removed. Adds TestKillPortProcess: real-socket tests proving a separate client process is excluded from the listener lookup and survives port cleanup. 9 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pter relocation Follow-up to the salvaged #43846 commits: the WhatsApp adapter moved from gateway/platforms/whatsapp.py to plugins/platforms/whatsapp/adapter.py since the PR was authored. The cherry-pick brought _listener_pids_on_port's `re.finditer` ss-fallback and the new test's import, but the new module location doesn't import `re` (latent NameError on the lsof-absent fallback path) and the test imported the old module path. Add `import re` to the adapter and repoint the test import.
…nce + retry connect) The salvaged test spawned a listener subprocess that printed its port immediately after bind() but BEFORE listen(), so under CI's loaded 8-worker box the parent connected before the socket was listening -> ConnectionRefused (flaked on test slice 2/6). Reorder the child to listen() then print the port, and make the client connect with a short bounded retry to absorb scheduler jitter. 15/15 green locally including direct hammering.
The PID-reuse guard (#43846) reads /proc/<pid>/stat field 22, which only exists on Linux — on macOS/Windows it returned None and the guard silently degraded to a bare liveness check (a no-op, safety-wise). Add a psutil.create_time() fallback (psutil is a hard dep, cross-platform), quantized to centiseconds for stable equality, so the recycled-PID guard actually protects macOS/Windows too. /proc always wins first on Linux and always misses on macOS/Windows, so the two sources never mix on one host and same-source equality is all the guard needs.
teknium1
force-pushed
the
salvage/pr-43846-kill-safety
branch
from
June 22, 2026 00:12
d57998a to
670104b
Compare
This was referenced Jun 22, 2026
2 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The gateway can no longer SIGTERM an unrelated process because a stored PID got recycled onto it. Three independent kill paths now re-validate process identity (kernel start time) before signalling, and WhatsApp port cleanup only ever kills LISTENers — never clients. Salvages #43846 (@valentt). Same hazard class as the browser orphan-reaper fix (#14073 / #50417), in different subsystems.
Root cause: once a tracked process exits and is reaped, the kernel recycles its PID number onto an unrelated process. Code that signalled a stored PID guarded only by a liveness check then tree-killed the stranger — proven live by @valentt with a kernel
signal:signal_generatetracepoint catchinggateway → SIG15 → firefox.Changes
tools/process_registry.py: record kernel start time (/proc/<pid>/statfield 22) at spawn (host_start_time), persisted in the checkpoint;_terminate_host_pid(pid, expected_start=…), checkpoint recovery, detached-session refresh, andkill_processall re-validate(pid, start_time)before signalling. No baseline (legacy) → degrades to prior liveness behavior.plugins/platforms/whatsapp/adapter.py:_kill_stale_bridge_by_pidfilegates on identity (bridge.pidnow records start time; legacy pidfiles fall back to anode+session-path cmdline signature);_kill_port_processtargets only LISTEN-state sockets (lsof -sTCP:LISTEN,ss -ltnpfallback) instead offuser/barelsof -i, which also matched client connections sharing the port (default 3000 → a browser tab on:3000was killed on every bridge restart).import reto the relocated adapter (thessfallback'sre.finditerwas a latent NameError — the adapter movedgateway/platforms/whatsapp.py→plugins/platforms/whatsapp/adapter.pysince the PR was authored);fuser-asserting change-detector tests with LISTEN-only contract tests.Validation
test_process_registry.py+test_whatsapp_bridge_pidfile.py+test_whatsapp_connect.pyssfallback runs without NameErrorAuthorship: @valentt's 3 commits preserved (rebased onto current main); 2 follow-up commits are ours. Closing #15008 (@tkwong, SIGTERM→SIGKILL escalation) separately — it's a different concern, 6.7k commits stale, and adds
BROWSER_*env vars that violate the config.yaml-not-.env policy.Infographic