[sandbox audit] Make command supervision survive PID reuse, timeouts and long runs - #21
Draft
Wauplin wants to merge 2 commits into
Draft
Conversation
Four supervision problems, all of them things that go wrong quietly.
**Timeout watchers slept past the child's exit.** Every `timeout=` command
spawned a thread that slept to the deadline whether or not the child was
still alive, so a `timeout=3600` command that finished in a millisecond
left a thread asleep for an hour. Twenty short commands with a long timeout
took the thread count from 6 to 26 and held it there. The watcher now waits
on a cancellation channel *or* the deadline, whichever comes first, and the
channel is dropped as soon as `wait()` returns.
**And then signalled a raw PID.** After sleeping, the watcher checked
`kill(pid, 0)` and killed the group -- using a PID captured when the command
started. A PID stops identifying a process the moment it exits, and the
kernel may hand the number to something else; we are root and often PID 1,
so the recycled PID could be anything. Commands now carry a `ProcHandle`
holding a pidfd, which refers to the process rather than the number. The
PID is kept only for the process-group sweep, which runs after the pidfd
confirms the leader is alive, so the group cannot have been recycled
underneath it. Kernels without `pidfd_open` fall back to the old behaviour.
`DELETE /processes/{id}` likewise no longer signals a process it already
knows has exited.
**A foreground command was invisible to the idle watchdog.** It was never
registered in `ProcRegistry`, so `running_count()` returned 0, and
`last_activity_ms` was stamped only when a request *arrived*. A single
`run()` lasting longer than `idle_timeout` with no other traffic therefore
shut down its own job mid-command -- with the default 10 minute timeout,
that is any long build or training step. Two fixes, either of which would
do, and both are cheap: an RAII `ActiveOp` guard the watchdog counts
alongside running processes, and a `last_activity_ms` stamp on every
streamed output event, since output is evidence of life.
**Teardown reported success it had not achieved.** `kill_uid` gave up after
50 passes and returned nothing, so a sweep that left processes running was
indistinguishable from a clean one -- and the caller then deleted the home
directory and answered `{"deleted": true}`. It now reports whether it
converged (over ~1s rather than 250ms), and `DELETE /v1/sandboxes[/{id}]`
answers 500 with the reason when something survived. The sandbox is still
removed from the registry either way, since the client should stop using it
regardless; what changes is that it is told.
The uid sweep is also what catches a descendant that escaped its process
group with `setsid()`: the group is gone but the uid is not. That is why
deleting a sandbox terminates such a descendant while `kill()` on an
individual process does not -- now stated where the code does it.
Finally, the process registry no longer grows without bound: finished
entries are capped per scope, keeping the most recent, while running
processes are never dropped. Previously every background command a
long-lived sandbox ever started stayed in memory and in `/processes`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The server usually runs as PID 1 in its container, which makes it the
adoptive parent of every orphaned grandchild -- and it only ever waited on
its own `Child` handles, so anything re-parented to it stayed a zombie for
the life of the job.
A reaper thread now collects them. The delicate part is not stealing an exit
status from `Child::wait()`, which would turn a command's exit code into
garbage, so it peeks before reaping and skips any PID we spawned ourselves.
The first version of that peek used `waitpid(-1, WNOHANG | WNOWAIT)`, which
is wrong: `WNOWAIT` is a `waitid`-only flag and `waitpid` rejects it with
EINVAL -- silently turning the whole reaper into a no-op. It uses `waitid`
now. Worth recording because the failure mode was invisible: the code ran,
returned no error, and did nothing.
`PR_SET_CHILD_SUBREAPER` covers the case where we are not PID 1.
Also: zombies are no longer counted as live processes by the uid kill sweep.
A zombie holds no memory, files or CPU and cannot be killed, so counting one
made the sweep spin until it gave up and then report a teardown failure that
was not one. That combination -- an unreaped zombie plus a sweep that
counted it -- turned a clean `DELETE /v1/sandboxes/{id}` into a 500 in
testing, which is how both bugs surfaced.
Validation: `scripts/process-supervision-regression.sh` now passes end to
end, including that a detached grandchild leaves no zombie, that exit codes
(0, 1, 3, 7) are still reported correctly with the reaper running, and that
a clean delete answers 200 while a `setsid` survivor is still caught by the
uid sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 8, 2026
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
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.
Why
Five supervision problems, all of which fail quietly.
1. Timeout watchers slept past the child's exit. Every
timeout=command spawned a threadthat slept to the deadline whether or not the child was still alive. On the parent commit, 20
commands that finish instantly but ask for
timeout=3600:Those 21 threads stay for an hour. The watcher now waits on a cancellation channel or the
deadline, whichever comes first.
2. And then signalled a raw PID. After sleeping it did
kill(pid, 0)and killed thegroup — using a PID captured when the command started. A PID stops identifying a process the
moment it exits, and the kernel may reuse the number; we run as root and often as PID 1, so
the recycled PID could be anything. Commands now carry a
ProcHandleholding a pidfd,which refers to the process, not the number. The PID is kept only for the process-group sweep,
which runs after the pidfd confirms the leader is alive. Kernels without
pidfd_openfallback to the old behaviour.
3. A foreground command was invisible to the idle watchdog. It was never registered in
ProcRegistry, sorunning_count()returned 0, andlast_activity_mswas stamped only whena request arrived. A single
run()longer thanidle_timeoutwith no other traffic shutdown its own job mid-command — with the 10-minute default, that is any long build or training
step. On the parent commit, a 12s command against a 3s idle timeout:
Fixed twice over, both cheap: an RAII
ActiveOpguard the watchdog counts, and alast_activity_msstamp on every streamed output event (output is evidence of life).4. Teardown reported success it hadn't achieved.
kill_uidgave up after 50 passes andreturned nothing, so a sweep that left processes running was indistinguishable from a clean
one — and the caller then deleted the home directory and answered
{"deleted": true}. It nowreports convergence, and
DELETE /v1/sandboxes[/{id}]answers 500 with the reason. Thesandbox is still removed from the registry either way (the client should stop using it
regardless); what changes is that it's told.
5. Orphans were never reaped. The server is the adoptive parent of re-parented
grandchildren and only ever waited on its own
Childhandles, so they stayed zombies for thejob's lifetime.
Plus the registry no longer grows without bound: finished entries are capped per scope
(newest kept), running ones never dropped.
One bug worth recording
My first reaper used
waitpid(-1, WNOHANG | WNOWAIT)to peek before reaping.WNOWAITis awaitid-only flag —waitpidrejects it with EINVAL, which silently turned the wholereaper into a no-op. It ran, returned no error, and did nothing. It uses
waitidnow.That surfaced only because a second bug made it visible: zombies were being counted as live
processes by the uid kill sweep, so an unreaped zombie made the sweep spin until it gave up
and turned a clean delete into a 500. A zombie holds no memory, files or CPU and cannot be
killed, so it is now excluded from the sweep's view.
Validation
scripts/process-supervision-regression.sh— root container, all passing:timed_out: true;fires once things really are idle (the fix must not disable it);
that matters most, since a reaper that steals a status corrupts every command's result;
evicted;
setsidsurvivor is caught by the uid sweep.own foreground command.
One honest gap: the zombie check did not reproduce on the parent commit (0 → 0), so that
particular assertion is not proven to catch the original bug — the timing of when the
grandchild's parent exits matters. The mechanism is plain in the code, and the check did catch
my own broken reaper, which is the next best evidence.
Behaviour changes
DELETE /v1/sandboxes[/{id}]can now answer 500 with{"deleted": true, "error": …}when teardown was incomplete. Previously always 200. A client treating any non-2xx as "not
deleted" would now retry a delete that did remove the sandbox — worth a look on the client
side, and the reason the body still says
deleted: true./processesno longer lists every finished process forever (capped, newest kept). Theclient docstring promising otherwise is updated in the companion client PR.