Repository navigation
fix: clean up after a launcher that a signal stops or kills - #25
Merged
Merged
Conversation
A confined run on Linux stopped when the harness wrote more than 64 KiB to standard output. The process that becomes pid 1 forks from an intermediate process. The intermediate does not exec, so it kept open all the descriptors of the launcher. One of these is the write end of the exec-error socket that std reads in spawn. Thus spawn did not return until the run ended, nothing read the capture pipe, and the harness stopped when the pipe was full. Now the intermediate closes all descriptors from 3 up after its fork (close_range, or a close loop on a kernel without it). Pid 1 keeps its own copy of the exec-error socket, thus a failure before its exec still gets to spawn. Add a launch test: a confined prompted run whose harness writes 200000 bytes completes in time, and all of the output gets to the launcher. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Before this change, a confined launch that got SIGTERM, SIGINT, SIGHUP or SIGKILL left these items: the intermediate process, the relay and the harness (still running), the FUSE mount (still held by the intermediate, thus a stat of it stopped), the directory /tmp/memory-sandbox-<pid>, the egress socket, the render and the mountpoint directory. For SIGTERM, SIGINT and SIGHUP, the launcher now catches the signal. It kills the sandbox (dm_sandbox::stop), lets the run in flight unmount and remove its scratch directory, finishes the session (socket and render) and exits with 128 plus the signal number. A run that starts after the signal is refused or killed. The launcher says the stop one time, and does not report the SIGKILL that ends the sandbox as a death. The sandbox now ends with its launcher. The intermediate gets SIGKILL when the launcher thread that forked it ends, and pid 1 gets SIGKILL when the intermediate ends (PR_SET_PDEATHSIG). Thus after a SIGKILL of the launcher, no process of the run continues and its mount is dead. SIGKILL cannot be caught, thus the next run sweeps what is left: - The scratch directory is locked (flock) by its launcher during the run. The sweep removes a memory-sandbox-<pid> directory only if the name has this form, it is a directory of this user and not a link, the pid is not a process, no process holds its lock and it is empty. - The mount sweep now does all harness directories under <root>/state/mount, does not follow links, and finds a dead mount with an open: the kernel can give a stat from its cache for one TTL after the server is gone. The socket and the render of a launcher that SIGKILL stopped stay. Tests: a SIGTERM to a confined launch leaves nothing, and the next launch sweeps the scratch directory and the mount of a launch that SIGKILL stopped and of an old launch, and keeps those of a live launch. Unit tests for the stop registry, the scratch sweep and the mount sweep. This commit is on top of the fix for the capture hang: a stop needs the pid that spawn gives, and spawn returned only at the end of the run before that fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- SIGHUP from a terminal that hung up: the launcher now points standard output and standard error at /dev/null before the flow continues. Thus print! and eprintln! do not panic, and the session finishes. The signals now have priority over the flow in the select. Also when the launcher cannot write its stop line, it does this. - An --unconfined run now runs on a blocking thread. Thus the launcher receives SIGTERM during the run and ends the run at once with 143. - Mount records: each launcher holds a flock on <mountpoint>.lock from before it makes the mountpoint until after the mountpoint is gone. The sweep does not probe or remove a mount whose lock is held. Thus a launcher that is stopped (SIGSTOP) does not stop the next launch, and the sweep does not remove a mountpoint that is about to be used. The sweep removes a lock file that has no holder and no mountpoint. - A second signal while the launcher waits for the cleanup ends the launcher at once. - amem work: the worker holds the in-flight count from the claim until the task is settled, and claims no task after a stop. Thus a SIGTERM during a run releases the task with a handoff. - The next launcher removes egress-<pid>.sock when no process has that pid. The render stays, because no record gives its owner. - The scratch sweep and Scratch::make compare the device and inode of the path with the locked directory. command() does not make the scratch directory again. - Pid 1 opens a pidfd of the intermediate before the fork. After its parent-death request, it fails if the intermediate is gone. Tests: a hung-up terminal (script(1)) leaves nothing; an unconfined launch ends at once on SIGTERM; the mount of a stopped launcher is kept and the next launch completes; a worker that gets SIGTERM releases its task with a handoff and does not claim the other task; after a SIGKILL no process of the run stays, and the next launch removes the socket; unit tests for the mount records, the socket sweep and the inode check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A reader of standard output that is gone (a closed pipe) does not stop the cleanup. The answer of a run is written with a write that ignores the error, not with print!, which panics on EPIPE. - A launcher that started with SIGHUP or SIGINT ignored (nohup, a background job of a script) does not listen for that signal. Thus the signal stays ignored. On SIGHUP, the launcher points standard output and standard error at /dev/null only when a write fails or one of them is a terminal, thus nohup.out keeps the output. - A headless unconfined run is in a process group of its own, and a stop kills the group. Thus a child that the harness left in the background does not keep the read of the output open. An interactive run stays in the foreground group of the terminal. - A second signal during the cleanup removes the socket and the render and releases the task that the worker holds, then the launcher exits. - The worker counts the run in flight from before the claim. A task that it claims while a stop comes is released. - The socket sweep removes a socket only when a connection to it is refused, because a pid names a process only in one pid namespace. - The mount record is taken again one time if a sweep removed it between the open and the lock. - Scratch::make refuses a directory of another user or a directory that all users can write. Tests: a worker and a launch whose output reader is gone, a launcher that ignores SIGHUP, an unconfined worker whose harness left a background child, a worker that gets SIGINT two times, and unit tests for the group kill, the ignored check, the socket that still answers and the scratch mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A headless unconfined run goes in a process group of its own only when the launcher has no controlling terminal. On a terminal the harness stays in the foreground group, thus a prompt or stty does not stop it with SIGTTOU or SIGTTIN. - measure::run reads the output on a thread of its own and waits for the process at the same time. After a stop, it reads for 500 ms more at most when the process has exited, because a child that the run left in the background can hold the output open. - measure::run removes the run from the stop list after the process exits and before it is reaped, thus a stop cannot signal a pid or a process group that another process got. - Scratch::make makes its directory with mode 0700, thus umask 000 does not cause a refusal. It checks owner and mode only for a directory that was there before. - The socket probe connects without waiting. A full backlog counts as a listener. - On SIGHUP the launcher points standard output and standard error at /dev/null unless both go to regular files. - The worker clears the held task before settle sends its release or completion, thus a second signal cannot release a task that another worker now holds. - The code says that the 5 s cap of the hard cleanup bounds only the waits in it. Tests: a headless unconfined run on a terminal uses the terminal and exits 0; a worker on a terminal whose harness left a background child ends on SIGTERM; the twice-signal test sends the second signal after the first is in progress and expects the task back with no handoff; a confined run under umask 000 runs; the socket probe answers at once for a full backlog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…acOS - measure::run keeps the run in the stop list until the output is read, and reaps the process only after that. Before, the run left the list when the harness exited, thus a stop during the read did not kill a child that held the output. The unreaped process keeps its pid and its process group number from other processes. - The read waits up to 2 s, not 500 ms, after a stop. - The socket probe makes the socket non-blocking and close-on-exec with fcntl. macOS has no SOCK_NONBLOCK or SOCK_CLOEXEC, thus dm-sandbox did not compile for Apple targets. - has_a_terminal opens /dev/tty with O_NONBLOCK and O_NOCTTY, thus the open does not wait on a serial line. - Scratch::make always checks owner and mode. - stops.rs says what a stop on a terminal does to the background children of a harness. Tests: a stop kills the child that an exited harness left holding the output. The background-child tests use a child that does not exit when its output closes. The twice-signal test sends both signals with kill(2) from the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sstriker
requested review from
andrewmusselman
and
a balanced review from Copilot
October 25, 2026 02:09
- The socket_refuses test uses a socket that is bound and does not listen, thus a fork by another test thread cannot make a connection succeed. The socket sweep test in amem sweeps again for a short time for the same reason. - The full-backlog part of that test runs on Linux only. macOS refuses a connection to a full backlog. The doc of socket_refuses says this and why the pid check before the probe makes it harmless there. - On Linux, Android and FreeBSD the probe socket gets SOCK_NONBLOCK and SOCK_CLOEXEC in the socket call. Other systems use fcntl. - The scratch directory check compares the owner with geteuid. - measure::run decides if a SIGKILL came from a stop when the process exits, not after the read of its output. Thus a harness that is killed before a stop (for example by the OOM killer) is reported. - The launch tests also find a process by its working directory, thus they see and kill the "sleep 300" child of a harness. The comment of the twice-signal test gives the correct window. Tests: a harness killed by SIGKILL before a stop is reported as killed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test a_harness_killed_before_a_stop_is_said_to_have_been_killed failed on CI. Cause: an unconfined run was not counted as a run in flight. After a stop, the launcher did not wait for the run, and it could exit before the run was reaped and its end was reported. Under taskset -c 0, the old code failed 3 of 40 runs, and 1 of 40 runs when the test also waited for the death of the harness. - run_unconfined holds an InFlight until the harness is reaped. - A run handle records whether a stop killed the run while it was still running (a waitid check without a wait before the kill). measure::run uses this record, not the stop flag at the time of the exit, to decide if a SIGKILL came from the stop. - The test waits until the harness is dead (a zombie or gone, from the pid that it writes) before it sends SIGTERM. It has no fixed sleep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
A confined launch that got SIGTERM, SIGINT, SIGHUP or SIGKILL left its run behind. The harness container survey in #2 found this. On
main, after the signal, these remained:statof it stopped);/tmp/memory-sandbox-<pid>, the egress socket, the render and the mountpoint directory.This PR contains the commit of #23 (
01a5998), because a stop needs the pid thatspawn()returns, and before #23spawn()returned only at the end of the run. Merge #23 first.The change
A signal the launcher can catch (SIGTERM, SIGINT, SIGHUP;
crates/memory/src/stops.rs):crates/sandbox/src/stop.rs). The registry holds a pidfd for each run, so a reused pid is never signalled. A run that starts after the stop is refused or killed.amem work, the task in flight is settled as a harness that exits settles it: it goes back with a handoff. No task is claimed after a stop.nohup, a background job of a script) is not caught./dev/nullunless both are regular files, so output to a dead terminal or a closed pipe cannot stop the cleanup. The run's answer is written without a panic on a closed pipe.The sandbox ends with its launcher. The intermediate and pid 1 get
PR_SET_PDEATHSIG(SIGKILL). Pid 1 also checks a pidfd of the intermediate after itsprctl. So after a SIGKILL of the launcher, no process of the run goes on, and its mount is dead.An unconfined headless run without a controlling terminal runs in its own process group, and a stop kills the group. On a terminal, the harness stays in the terminal's foreground group, so it can use the terminal. After a stop, the launcher reads the output for at most 2 s more. A background child that the harness left on a terminal is not killed by a signal to the launcher alone. Ctrl-C reaches it.
What a SIGKILL leaves, the next start sweeps:
Scratch directories (
crates/sandbox/src/scratch.rs). Each launcher holds a flock on its directory. The sweep removes amemory-sandbox-<pid>directory only if all of these are true:The device and inode are compared with the locked descriptor.
Mounts (
crates/mount). Each mount has a<mountpoint>.lockthat its launcher holds. The sweep does not probe a mount whose lock is held, so a stopped launcher (Ctrl-Z) cannot hang it. It finds a dead mount with an open, not astat, because the kernel can answer astatfrom its cache.Egress sockets. The sweep removes
egress-<pid>.sockonly when the pid is not running and a non-blocking connect is refused.The render stays. Nothing records its owner.
Tests
crates/memory/tests/launch.rs, confined and unconfined, withCI=1so they fail and do not skip:work --loop: a stop releases the task with a handoff and claims no other; two signals release it without one.Unit tests cover the stop registry, the scratch sweep (a live pid, a held lock, a link, a non-empty directory, other names), the mount sweep, and the socket probe.
Failing first
Against
main's code, with only the new tests:left: None right: Some(143).Each later commit shows its new tests failing first, against the commit before it, or under a mutation where the old code cannot show it. Examples:
left: ["egress-410778.sock"].failed printing to stdout: Broken pipe,left: Some(1) right: Some(143).left: Some(1) right: Some(0).sh -c sleep 300was still alive.make checkwithCI=1: 683 tests, 683 passed, 6 skipped.dm-sandboxpasses clippy foraarch64-apple-darwinandx86_64-pc-windows-gnu.Adversarial passes
Five fresh agents attacked this branch. Each round of findings is fixed in the commit after it.
On
cf45d20. It found these problems, fixed inace1e54:work --loopleft the task claimed.It ruled out a risk with the parent-death signal: spawn and wait run on the same thread, and a 15 s run went through.
On
ace1e54. It found these problems, fixed in4ce0e81:On
4ce0e81. It found these problems, fixed ine6a7a4a:On
e6a7a4a. It found that an exited harness's child was not killed, and thatdm-sandboxdid not build on macOS. Fixed in732e673.On
732e673. It found the cause of the first CI failure. In the socket probe test, a fork by another test thread kept the dropped listener open, and the connect succeeded. On macOS, a full backlog refuses a connect. It also found these small points:All are fixed in
e284717. Thedm-sandboxunit tests passed 30 of 30 runs in a loop.CI then found a bug in
e284717. The new test for a SIGKILL before a stop failed on Linux. The cause: an unconfined run did not count as in flight. After a SIGTERM, the launcher could exit before the run was reaped and reported. Undertaskset -c 0, the test failed 3 of 40 times.4c24109holds the run in flight until it is reaped, as a confined run already did. Each run handle now also records whether the stop killed it while it still ran. The test waits until the harness is dead, with no fixed sleep. After the fix, it failed 0 of 40 times undertaskset -c 0and 0 of 20 without it.Known limits
cmd_inside(the wslc runner) gets no signal handling.releasechecks only the holder's name, which every worker of one owner shares. This is older than this PR.Posted by Claude on behalf of @sstriker