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

CHANGELOG.md

@@ -34,6 +34,10 @@ Some actions that application developers should consider taking when upgrading f
 
 * A request that finds an idle worker no longer waits for `fork`. The supervisor forks a worker into each free slot at boot, and forks a replacement as soon as it reaps a worker that served a request. A request that waits in the queue still waits for `fork`. The supervisor runs `Process.warmup` once, at boot, before the first fork.
 
+#### Fixed
+
+* A cell on macOS no longer stops when it kills a worker that started another process. Previously, `Process.kill` on the worker's process group could raise `Errno::EPERM` while that process was a zombie. The supervisor did not rescue the error, so the cell stopped and the caller received no response.
+
 ### HotCell::Client
 
 #### Added

hotcell-server/lib/hot_cell/supervisor.rb

@@ -872,18 +872,23 @@ def enforce_retirements
       # the sweep must not signal it. An empty group is the common case, so ESRCH is expected. It is safe to
       # kill the group by the leader's pid even though the leader is reaped, because the supervisor is single
       # threaded and mints group leaders only in `spawn`, which cannot run between the `wait2` above and here.
+      #
+      # macOS answers EPERM rather than ESRCH for a group whose only members are zombies, and a tool killed
+      # with its worker stays one until launchd reaps it. Raised, it unwound `run` and ended the cell before
+      # this reap answered the caller. EPERM means no member received the signal, the same outcome as ESRCH.
       def sweep_group(child)
         Process.kill :KILL, -child.pid
-      rescue Errno::ESRCH
+      rescue Errno::ESRCH, Errno::EPERM
         nil
       end
 
       # The whole process group, which is the worker and everything it started. Negative pid is the group.
       # Falls back to the worker alone if the group is already gone, so a worker that died between the check
-      # and the signal is not an error.
+      # and the signal is not an error. A group answering EPERM falls back as well, for the reason
+      # `sweep_group` gives; the bare pid does not, because macOS signals a zombie pid without complaint.
       def kill_group(child)
         Process.kill :KILL, -child.pid
-      rescue Errno::ESRCH
+      rescue Errno::ESRCH, Errno::EPERM
         begin
           Process.kill :KILL, child.pid
         rescue Errno::ESRCH

hotcell-server/test/scheduling_test.rb

@@ -59,6 +59,27 @@ def test_a_deadline_kills_what_the_worker_started_too
     ENV.delete "HOTCELL_SPAWNED_PID_PATH"
   end
 
+  # macOS refuses a signal to a process group whose only members are zombies, with EPERM rather than ESRCH.
+  # A killed worker's tool is one such zombie until launchd reaps it, so the reap's sweep raised, unwound the
+  # run loop and ended the cell, and the caller read end of stream instead of the verdict.
+  def test_a_deadline_kill_is_answered_when_the_reap_sweep_is_refused
+    refuse_group_signals(after: 1) do
+      TestCell.boot(deadline: 0.3, concurrency: 1) do |cell|
+        assert_failed "killed", cell.call("test.uninterruptible", timeout: 20), cause: "deadline"
+      end
+    end
+  end
+
+  # The deadline kill meets the same refusal when the worker has exited unreaped. Refused, it falls back to
+  # the worker's own pid. Here the worker is still alive, so the fallback is what kills it.
+  def test_a_deadline_kill_is_answered_when_the_group_kill_is_refused
+    refuse_group_signals do
+      TestCell.boot(deadline: 0.3, concurrency: 1) do |cell|
+        assert_failed "killed", cell.call("test.uninterruptible", timeout: 20), cause: "deadline"
+      end
+    end
+  end
+
   # A reused worker is not one operation. Serving A, then B, then A left the shared library configured by B
   # while A ran, because the memo asked "has this ever booted" rather than "is this what it is set up for".
   def test_a_reused_worker_reconfigures_when_the_operation_changes
@@ -260,4 +281,22 @@ def alive?(pid)
     rescue Errno::ESRCH
       false
     end
+
+    # The cell forks from this process, so a stub installed here is inherited by the supervisor.
+    def refuse_group_signals(after: 0)
+      original = Process.method(:kill)
+      allowed = after
+      Process.define_singleton_method(:kill) do |signal, *pids|
+        if pids.any?(&:negative?)
+          raise Errno::EPERM, "induced" if allowed.zero?
+
+          allowed -= 1
+        end
+
+        original.call signal, *pids
+      end
+      yield
+    ensure
+      Process.define_singleton_method(:kill, original)
+    end
 end