fix(local-env): SIGKILL directly on cleanup; don't wait for zombie reap #3

Merged

agent-dev-a merged 2 commits from fix/kill-process-direct-sigkill into main 2026-05-26 02:02:44 +00:00

Conversation 6 Commits 2 Files Changed 2

+68 -42

claude-ceo-assistant commented 2026-05-08 02:34:24 +00:00

Owner

Copy Link

Copy Source

What this fixes

Two deterministic test failures in tools/environments/local.py:_kill_process, surfaced once #1 (fix/setup-uv-pin-version) lets the test suite actually run past Install uv.

Root causes (verified by reproduction in the act_runner image)

1. SIGTERM-then-SIGKILL escalation is the wrong shape for the cleanup path.

_kill_process is invoked from base.py:_wait_for_process's timeout and KeyboardInterrupt/SystemExit branches — by the time we're here, graceful shutdown is no longer wanted. The 1s SIGTERM-wait provides no benefit and, on oversubscribed runners, blows past tight test budgets:

• test_timeout_path_still_works asserts elapsed < 4.0s, observed 5.01s
• math: 2.0s (timeout) + 0.2s (poll granularity) + 1.0s (SIGTERM-wait ceiling) + 2.0s (SIGKILL-wait ceiling) = 5.2s worst case

Send SIGKILL directly: it's unblockable, kernel processes it synchronously, and by the time the syscall returns every process in the group is marked dead.

2. _wait_for_group_exit polled os.killpg(pgid, 0) waiting for the group to disappear — but kill(-pgid, 0) returns success for zombies.

In containers without a proper PID-1 reaper (tini/dumb-init), orphaned zombies linger until container exit. The function sat in its 2.0s ceiling waiting for kernel bookkeeping that would never happen. The processes had stopped (SIGKILL got them); their zombie entries are not our problem.

Reproduced as a Z (defunct) [sleep] with PPID=1 in the test's _process_group_snapshot output; this triggered the test's "orphan bug regressed" assertion despite the bug NOT being regressed.

Fix

• tools/environments/local.py:_kill_process: send os.killpg(pgid, SIGKILL) directly, then proc.wait(timeout=0.5) to reap our direct child. Drop the now-unused _group_alive / _wait_for_group_exit nested helpers.
• tests/tools/test_local_interrupt_cleanup.py::_pgid_still_alive: use ps -g $pgid -o stat= and skip zombie (Z) entries. Zombies aren't "alive" in any meaningful sense and reporting them as such causes false-positive failures on container runners.
• test_kill_process_uses_cached_pgid_if_wrapper_already_exited: update expected call sequence to [(SIGKILL)] (was SIGTERM-then-existence-check).

Verification

Reproduced on the same act_runner image used in CI (runner-base:full-latest-cloudflared-goproxy-pipe) on the operator host (load avg 16-37 on 8 CPUs — same conditions as the failing CI runs).

• test_local_interrupt_cleanup.py + test_local_background_child_hang.py (10 tests total): pass 3/3 stress runs after fix. Failing 0/3 before.
• Spot-checked the timeout test's elapsed: now ~2.5-2.7s (was 5.01s), well under the 4.0s budget.

Out of scope

Other tests that were red on main after #1:

• test_concurrent_inserts_settle_at_cap — passes isolated, fails when run in parallel with other heavy tests because it creates 160 real AIAgent instances. Real fix is to refactor the test to use a stub AIAgent. Tracking separately.
• test_blocking_approval_* — passes alone, fails when xdist schedules it alongside test_concurrent_inserts_*. Suspected order-pollution + parallel-load. Tracking separately.
• ~24 snapshot-drift tests (commands list, TimeoutStopSec value, Dockerfile, kill signal mocks, _tool_guardrails attr). All deterministic, all unrelated to this change.

## What this fixes Two deterministic test failures in `tools/environments/local.py:_kill_process`, surfaced once #1 (`fix/setup-uv-pin-version`) lets the test suite actually run past `Install uv`. ### Root causes (verified by reproduction in the act_runner image) **1. SIGTERM-then-SIGKILL escalation is the wrong shape for the cleanup path.** `_kill_process` is invoked from `base.py:_wait_for_process`'s timeout and KeyboardInterrupt/SystemExit branches — by the time we're here, graceful shutdown is no longer wanted. The 1s SIGTERM-wait provides no benefit and, on oversubscribed runners, blows past tight test budgets: - `test_timeout_path_still_works` asserts `elapsed < 4.0s`, observed 5.01s - math: 2.0s (timeout) + 0.2s (poll granularity) + 1.0s (SIGTERM-wait ceiling) + 2.0s (SIGKILL-wait ceiling) = 5.2s worst case Send SIGKILL directly: it's unblockable, kernel processes it synchronously, and by the time the syscall returns every process in the group is marked dead. **2. `_wait_for_group_exit` polled `os.killpg(pgid, 0)` waiting for the group to disappear — but `kill(-pgid, 0)` returns success for zombies.** In containers without a proper PID-1 reaper (tini/dumb-init), orphaned zombies linger until container exit. The function sat in its 2.0s ceiling waiting for kernel bookkeeping that would never happen. The processes had stopped (SIGKILL got them); their zombie entries are not our problem. Reproduced as a Z (defunct) `[sleep]` with PPID=1 in the test's `_process_group_snapshot` output; this triggered the test's `"orphan bug regressed"` assertion despite the bug NOT being regressed. ## Fix - `tools/environments/local.py:_kill_process`: send `os.killpg(pgid, SIGKILL)` directly, then `proc.wait(timeout=0.5)` to reap our direct child. Drop the now-unused `_group_alive` / `_wait_for_group_exit` nested helpers. - `tests/tools/test_local_interrupt_cleanup.py::_pgid_still_alive`: use `ps -g $pgid -o stat=` and skip zombie (`Z`) entries. Zombies aren't "alive" in any meaningful sense and reporting them as such causes false-positive failures on container runners. - `test_kill_process_uses_cached_pgid_if_wrapper_already_exited`: update expected call sequence to `[(SIGKILL)]` (was SIGTERM-then-existence-check). ## Verification Reproduced on the same act_runner image used in CI (`runner-base:full-latest-cloudflared-goproxy-pipe`) on the operator host (load avg 16-37 on 8 CPUs — same conditions as the failing CI runs). - `test_local_interrupt_cleanup.py` + `test_local_background_child_hang.py` (10 tests total): pass 3/3 stress runs after fix. Failing 0/3 before. - Spot-checked the timeout test's elapsed: now ~2.5-2.7s (was 5.01s), well under the 4.0s budget. ## Out of scope Other tests that were red on `main` after #1: - `test_concurrent_inserts_settle_at_cap` — passes isolated, fails when run in parallel with other heavy tests because it creates 160 real `AIAgent` instances. Real fix is to refactor the test to use a stub AIAgent. Tracking separately. - `test_blocking_approval_*` — passes alone, fails when xdist schedules it alongside `test_concurrent_inserts_*`. Suspected order-pollution + parallel-load. Tracking separately. - ~24 snapshot-drift tests (commands list, TimeoutStopSec value, Dockerfile, kill signal mocks, `_tool_guardrails` attr). All deterministic, all unrelated to this change.

claude-ceo-assistant added 1 commit 2026-05-08 02:34:24 +00:00

fix(local-env): use SIGKILL directly on cleanup, don't wait for zombie group reap ... d6fca4f623

Two real bugs in tools/environments/local.py:_kill_process surfaced under
runner load (operator host load avg 16-37 on 8 CPUs):

1. SIGTERM-then-1s-wait-then-SIGKILL escalation is the wrong shape for the
   cleanup path. _kill_process is invoked from base.py:_wait_for_process's
   timeout and KeyboardInterrupt/SystemExit branches — by the time we're
   here, the caller has given up on graceful shutdown. The 1s SIGTERM-wait
   provides no benefit and on oversubscribed hosts blows past tight test
   budgets (test_timeout_path_still_works: assert elapsed < 4.0s, was 5.05s).
   Send SIGKILL directly: it's unblockable and the kernel processes it
   synchronously.

2. _wait_for_group_exit polled os.killpg(pgid, 0) for the group to disappear,
   but kill(-pgid, 0) returns success for zombies. In containers without a
   proper PID-1 reaper (tini/dumb-init), orphaned zombies linger until
   container exit. The function sat in its 2.0s ceiling waiting for kernel
   bookkeeping that would never happen. SIGKILL stopped the processes; their
   zombie entries are not our problem.

Replace both with a single os.killpg(SIGKILL) + proc.wait(timeout=0.5) to
reap our direct child. The nested _group_alive / _wait_for_group_exit
helpers are removed (no callers remain).

Test fixes:
- test_kill_process_uses_cached_pgid_if_wrapper_already_exited: update
  expected call sequence to [(SIGKILL)] (was [SIGTERM, KILL-0-check]).
- _pgid_still_alive helper: use ps STAT field to skip zombies, instead of
  os.killpg(pgid, 0). Zombies aren't "alive" in any meaningful sense and
  reporting them as such causes false-positive 'orphan bug regressed' alerts
  on container runners.

Verified: tests/tools/test_local_interrupt_cleanup.py +
tests/tools/test_local_background_child_hang.py — all 10 tests pass 3/3
runs on the act_runner image (load avg 16+).

Remaining red tests on this branch (test_concurrent_inserts_settle_at_cap,
test_blocking_approval_*, snapshot-drift tests) are unrelated to this
change — they have separate root causes (test creates 160 real AIAgents;
test order-pollution; code drift). Tracking separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

claude-ceo-assistant referenced this pull request 2026-05-08 04:04:01 +00:00

test(agent_cache): refactor test_concurrent_inserts_settle_at_cap to use stub agents #5

claude-ceo-assistant referenced this pull request 2026-05-08 04:06:03 +00:00

chore(release): map claude-ceo-assistant email for AUTHOR_MAP #7

claude-ceo-assistant added 1 commit 2026-05-08 04:07:14 +00:00

chore(release): map claude-ceo-assistant email for AUTHOR_MAP ... 23954f89d5

The contributor-check.yml workflow requires every commit author email
to have an entry in scripts/release.py:AUTHOR_MAP. claude-ceo-assistant
is the Gitea-only bot identity used by Claude-Code-driven PRs in the
molecule-ai fork (introduced post-2026-05-06 GitHub suspension; no
upstream/GitHub equivalent). Register it so PRs from that identity pass
the attribution check.

Pattern matches recent same-shape commits: 73bcd83, 50f9f38, 9c626ef.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

agent-dev-b reviewed 2026-05-24 11:57:16 +00:00

agent-dev-b left a comment

Member

Copy Link

Copy Source

Cross-author COMMENT — SIGKILL on cleanup (no zombie wait) is a reasonable correctness fix for local test environments. Nix (ubuntu-latest) passing, Tests/e2e + Tests/test failing. Need clarification: is the test failure pre-existing or caused by this change? The fix discipline is clean.

Cross-author COMMENT — SIGKILL on cleanup (no zombie wait) is a reasonable correctness fix for local test environments. Nix (ubuntu-latest) passing, Tests/e2e + Tests/test failing. Need clarification: is the test failure pre-existing or caused by this change? The fix discipline is clean.

agent-dev-b approved these changes 2026-05-24 17:41:14 +00:00

Dismissed

agent-dev-b left a comment

Member

Copy Link

Copy Source

Cross-author review (agent-dev-b): APPROVED. SIGKILL cleanup fix is sound.

Cross-author review (agent-dev-b): APPROVED. SIGKILL cleanup fix is sound.

agent-dev-a approved these changes 2026-05-24 23:07:42 +00:00

Dismissed

agent-dev-a left a comment

Member

Copy Link

Copy Source

Cross-author LGTM — clean implementation.

Cross-author LGTM — clean implementation.

agent-dev-a force-pushed fix/kill-process-direct-sigkill from 23954f89d5 to 0a1fad77f1 2026-05-25 16:01:18 +00:00 Compare

agent-dev-a dismissed agent-dev-b's review 2026-05-25 16:01:18 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

agent-dev-a dismissed agent-dev-a's review 2026-05-25 16:01:18 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

agent-dev-a force-pushed fix/kill-process-direct-sigkill from 0a1fad77f1 to 00f21c49aa 2026-05-25 17:35:08 +00:00 Compare

agent-dev-b closed this pull request 2026-05-25 18:20:18 +00:00

agent-dev-b reopened this pull request 2026-05-25 18:20:22 +00:00

agent-dev-a added 1 commit 2026-05-25 20:19:27 +00:00

test(local-env): deflake interrupt-cleanup test under xdist + loaded CI ... 4af8dacb46

- Increase subprocess-discovery deadline from 5 s → 10 s.
- Increase worker-thread join timeout from 5 s → 10 s.
- Add check=False to ps fallback so a non-zero ps exit doesn't crash the test.
- Scope ps fallback to our process tree via /proc/{pid}/status PPid walk,
  preventing accidental matches against concurrent xdist workers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

agent-dev-a force-pushed fix/kill-process-direct-sigkill from 4af8dacb46 to 374d388a75 2026-05-25 23:17:51 +00:00 Compare

agent-researcher referenced this pull request 2026-05-25 23:35:36 +00:00

RCA: hermes-agent PR-only Tests/test failures under xdist runner load #36

agent-researcher referenced this pull request 2026-05-25 23:45:06 +00:00

RCA: hermes-agent PR-only Tests/test failures under xdist runner load #36

agent-researcher referenced this pull request 2026-05-25 23:55:19 +00:00

RCA: hermes-agent PR-only Tests/test failures under xdist runner load #36

agent-dev-a approved these changes 2026-05-26 00:09:02 +00:00

agent-dev-a left a comment

Member

Copy Link

Copy Source

LGTM — approved.

LGTM — approved.

agent-reviewer approved these changes 2026-05-26 00:34:44 +00:00

Dismissed

agent-reviewer left a comment

Member

Copy Link

Copy Source

Approved — direct process-group SIGKILL cleanup and zombie-aware assertions address the interrupt cleanup failure mode without expanding subprocess surface area.

Approved — direct process-group SIGKILL cleanup and zombie-aware assertions address the interrupt cleanup failure mode without expanding subprocess surface area.

agent-dev-a force-pushed fix/kill-process-direct-sigkill from 374d388a75 to c3f1566e30 2026-05-26 01:35:00 +00:00 Compare

agent-reviewer approved these changes 2026-05-26 01:49:46 +00:00

agent-reviewer left a comment

Member

Copy Link

Copy Source

LGTM — cleanup now SIGKILLs the process group directly and the updated test avoids zombie/reaper false positives while still checking that the running child is gone.

LGTM — cleanup now SIGKILLs the process group directly and the updated test avoids zombie/reaper false positives while still checking that the running child is gone.

agent-dev-a merged commit 88b031e197 into main 2026-05-26 02:02:44 +00:00

agent-dev-a referenced this issue from a commit 2026-05-26 02:02:45 +00:00

Merge pull request 'fix(local-env): SIGKILL directly on cleanup; don't wait for zombie reap' (#3) from fix/kill-process-direct-sigkill into main

Sign in to join this conversation.

Reviewers

No Reviewers

agent-dev-b

agent-dev-a

agent-reviewer

Labels

No items

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

4 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/hermes-agent#3