[runner] Stop reading job output once the command has exited - #4276
Merged
Merged
Conversation
Previously, `execJob` copied the command's output to completion and only then waited for the command. A read on the pty master returns EIO only once every process holding the slave has closed it, so a job that leaves one behind blocked `io.Copy` forever. `cmd.Wait()` was then never reached: the shell stayed an unreaped zombie, the terminal job state was never reported, and the run hung until the container was destroyed. Now the command is waited for first, and the copy is given `logsDrainDelay` to drain what the command already wrote before the master is closed, which unblocks the read. Closing the master only works if it is pollable. `pty.Open()` wraps the descriptor with `os.NewFile` in blocking mode, so it never reaches the runtime poller, and closing such a file does not interrupt a `Read` already in flight -- the close is deferred until that read returns, which may be never. `/dev/ptmx` is now opened with `O_NONBLOCK`, which was the last use of `creack/pty`. The log quota watchdog moves into a goroutine, so output keeps being copied until the command exits and a full pty buffer cannot keep it from exiting. What gets killed is unchanged: the processes the job leaves behind still survive, and are cleaned up when the container is destroyed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
un-def
changed the title
un-def
added a commit
that referenced
this pull request
Sep 10, 2026
Previously, stopping a job sent SIGINT to `cmd.Process`, the wrapper shell. That reached nothing that mattered. The server runs commands under a shell with `-i -c`, and an interactive shell turns on job control, which puts the job in a process group of its own, while the shell ignores SIGINT for as long as it is waiting for that job. The signal reached neither, so nothing stopped the job until WaitDelay's SIGKILL went to the shell alone ten seconds later, leaving the workload running and reparented to PID 1. This looked intermittent because some shells exec the command in place. `bash -i -c` and `busybox ash -i -c` do so for a single simple command, leaving no shell at all, and the SIGINT then reached the workload directly. dash does not, so the same configuration behaved differently between a Debian and an Alpine image. Now Cancel writes the terminal's INTR character to the pty master, and the line discipline raises SIGINT in the terminal's foreground process group -- what pressing Ctrl-C does. The workload receives it whether or not a shell stands in between, and whether or not job control moved it into a process group of its own. The write is bounded by a deadline. A job that never reads its stdin could fill the terminal's input buffer and block the write, and Cmd only starts the WaitDelay timer once Cancel has returned -- so an unbounded write would disable the very backstop meant to catch a job that ignores the interrupt. The deadline is honoured because the master has been pollable since #4276. `max_duration` now interrupts the job too, rather than killing it. A job can still ignore all of this: a program that puts the terminal in raw mode clears ISIG, and the INTR character then delivers no signal. WaitDelay stays the backstop, and it still kills only the shell. Note, the shell process may still linger if the foreground group is _killed_ by SIGINT (doesn't handle it) -- some shells (dash, ash, but not bash) in the interactive mode unwind the interrupt to its top level and go back to reading the terminal -- the equivalent of returning to a prompt; e.g., with `image: debian`, `shell: /bin/sh` or unset -> dash: * `sleep inf` -- `/bin/sleep` doesn't handle SIGINT, killed, dash goes back to reading the terminal, lingers, killed after killDelay (10 seconds). * `python -m http.server` -- the server handles SIGINT, exits with 0, dash exits as well. This issue is considered insignificant for now and can be addressed in the future with a separate fix (one possible option is to send SIGHUP to the shell process after a graceful timeout). Fixes: #2233 Co-authored-by: Claude Opus 5 (1M context) <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
1 participant
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.
Previously,
execJobcopied the command's output to completion and only then waited for the command. A read on the pty master returns EIO only once every process holding the slave has closed it, so a job that leaves one behind blockedio.Copyforever.cmd.Wait()was then never reached: the shell stayed an unreaped zombie, the terminal job state was never reported, and the run hung until the container was destroyed.Now the command is waited for first, and the copy is given
logsDrainDelayto drain what the command already wrote before the master is closed, which unblocks the read.Closing the master only works if it is pollable.
pty.Open()wraps the descriptor withos.NewFilein blocking mode, so it never reaches the runtime poller, and closing such a file does not interrupt aReadalready in flight -- the close is deferred until that read returns, which may be never./dev/ptmxis now opened withO_NONBLOCK, which was the last use ofcreack/pty.The log quota watchdog moves into a goroutine, so output keeps being copied until the command exits and a full pty buffer cannot keep it from exiting.
What gets killed is unchanged: the processes the job leaves behind still survive, and are cleaned up when the container is destroyed.