Skip to content

Fix a macOS cell stopping when it kills a worker's tools - #91

Merged
flavorjones merged 1 commit into
masterfrom
card-5279-scheduling-deadline-kill-flake
Oct 1, 2026
Merged

flavorjones merged 1 commit into
masterfrom
card-5279-scheduling-deadline-kill-flake

Conversation

@flavorjones

Copy link
Copy Markdown
Member

Motivation

On macOS, Process.kill raises Errno::EPERM, not Errno::ESRCH, for a process group whose only members are zombies. Linux signals such a group without error.

A tool that a worker spawns is in the worker's process group. When the supervisor kills the worker, the tool becomes a zombie until launchd reaps it. If the supervisor reaps the worker before launchd reaps the tool, sweep_group raises Errno::EPERM. It rescues only Errno::ESRCH, so the error unwinds Supervisor#run and stops the cell before the reap answers the caller. The caller then reads end of stream instead of the killed verdict. kill_group has the same gap.

test_a_deadline_kills_what_the_worker_started_too failed intermittently on the macOS job for this reason (run):

SchedulingTest#test_a_deadline_kills_what_the_worker_started_too [test/scheduling_test.rb:50]:
expected a response and got none

Production cells run on Linux and are not affected.

Details

sweep_group and kill_group rescue Errno::EPERM from the group kill. A group kill raises Errno::EPERM only when no member received the signal, the same outcome as Errno::ESRCH. kill_group then falls back to the worker's own pid, as it does for Errno::ESRCH. The fallback still rescues only Errno::ESRCH, because macOS signals a zombie pid without error.

The new tests stub Process.kill in the cell to raise Errno::EPERM for a group:

  1. Refusing the reap's sweep reproduces the macOS failure: before this change the caller read end of stream.
  2. Refusing every group kill covers kill_group: before this change the cell stopped and left the worker running, so the caller timed out.

On macOS, `Process.kill` raised `Errno::EPERM`, not `Errno::ESRCH`, for
a process group whose only members were zombies. A tool killed with its
worker stays a zombie until `launchd` reaps it, so the supervisor's reap
could raise, stop the cell, and leave the caller with no response.
`test_a_deadline_kills_what_the_worker_started_too` failed
intermittently on the macOS CI job for this reason.

Rescue `Errno::EPERM` from the group kill in `sweep_group` and
`kill_group`.
Copilot AI balanced review requested due to automatic review settings October 1, 2026 20:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approved

The narrow exception-handling change preserves existing fallback behavior, covers both paths with regression tests, and has no identified blocking issues.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes macOS cells stopping when signaling a worker’s process group containing only zombies.

Changes:

  • Handles Errno::EPERM in group cleanup and termination, preserving the worker-PID fallback.
  • Adds regression tests for both error paths.
  • Documents the fix in the changelog.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
hotcell-server/​test/​scheduling_test.rb Tests refused group signals during deadline termination and cleanup.
hotcell-server/​lib/​hot_cell/​supervisor.rb Handles group-signal permission errors without stopping the cell.
CHANGELOG.md Records the macOS fix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@flavorjones
flavorjones merged commit 6263bcb into master Oct 1, 2026
15 of 17 checks passed
@flavorjones
flavorjones deleted the card-5279-scheduling-deadline-kill-flake branch October 1, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants