Skip to content

fix(exec): Relay a guest exec's output before reporting its exit - #1059

Closed
ananos wants to merge 1 commit into
feat/unchanged_containersfrom
fix/agent-exec-output-drain
Closed

ananos wants to merge 1 commit into
feat/unchanged_containersfrom
fix/agent-exec-output-drain

Conversation

@ananos

@ananos ananos commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Description

A short-lived nerdctl exec into a container boot guest sometimes prints nothing and still exits 0. For instance, 100 runs of nerdctl exec <id> stat -f -c %T /root against one running guest returned empty stdout 30 times. Adding ; sleep 0.2 after the command makes it go away, so output written just before the process exits is what gets lost.

The loss is in urunit-agent, and it happens in two ways:

  • urunc exec ends the session as soon as the Exit frame arrives (os.Exit in runStreamSession / runTTYSession). The agent sends Exit right after cmd.Wait() returns, and nothing orders it after the relays of the process's output. Output relayed after the Exit frame is dropped. This hits both the pipe and the pty path.
  • On the pipe path, cmd.Wait() closes the pipes from StdoutPipe / StderrPipe as soon as the process exits, so the last write can be discarded before the relay reads it. The os/exec documentation says it is incorrect to call Wait before all reads from the pipe have completed.

This patch relays stdout and stderr through plain os.Pipes that the agent closes itself, tracks the relays of both paths with a WaitGroup, and sends Exit only after they reach EOF. A process left running in the background can hold the output open, so the wait is bounded at 2 seconds after the exit. containerd's runc shim bounds the same drain at 10 seconds when it deletes an exec; 2 seconds keeps the exit of an interactive -it session prompt when the user leaves a background job holding the terminal. Happy to change it if we prefer to match containerd.

The host side (cmd/urunc/exec.go, exec_tty.go) is unchanged: it already writes every frame before it acts on Exit.

Not in this PR: serveVsock accepts connections with unix.Accept, which does not set SOCK_CLOEXEC, so every exec'd process inherits the agent's vsock connection (visible as an extra socket: fd in /proc/<pid>/fd). It does not affect the output, so it is left for a separate change.

Related issues

  • Fixes: none. There is no issue for this yet; the exec path it fixes exists only on feat/unchanged_containers.

How was this tested?

Unit tests (cmd/urunit-agent, which make unittest does not run). TestAgentExecOutputBeforeExit runs 200 short /bin/echo sessions on each path and checks that each one's output arrives before its Exit. TestAgentExecBackgroundHoldsOutput checks that a background process holding stdout for 10 seconds does not hold the exit report back past the bound. runSession now ignores frames of other streams, so late output of one session cannot count toward the next.

With the fix reverted (the new tests kept), the new test fails, and so does the existing TestAgentStdinRoundTrip:

=== RUN   TestAgentExecOutputBeforeExit
    agent_e2e_test.go:148: session 111 (tty=true) lost its output: got "", want "marker-111"
--- FAIL: TestAgentExecOutputBeforeExit (0.01s)
=== RUN   TestAgentStdinRoundTrip
    agent_e2e_test.go:208: cat did not echo stdin, got ""
--- FAIL: TestAgentStdinRoundTrip (0.00s)
FAIL	github.com/urunc-dev/urunc/cmd/urunit-agent	0.011s

A throwaway loop of 1000 sessions per path (not committed) lost output in 220/1000 pipe and 393/1000 pty sessions before the fix, and in 999/1000 and 997/1000 with GOMAXPROCS=1. After the fix it lost none in either setting.

With the fix (Linux container, go1.26.8):

$ go test -count=20 ./cmd/urunit-agent/
ok  	github.com/urunc-dev/urunc/cmd/urunit-agent	42.747s
$ go test -race -count=5 ./cmd/urunit-agent/
ok  	github.com/urunc-dev/urunc/cmd/urunit-agent	11.807s
$ GOMAXPROCS=1 go test -count=5 ./cmd/urunit-agent/
ok  	github.com/urunc-dev/urunc/cmd/urunit-agent	10.584s
$ go vet ./cmd/... ./pkg/agentproto/ && gofmt -l cmd pkg
(clean)

Live. On an x86_64 Ubuntu 24.04 host with a rootless nerdctl + containerd install: a generic container boot of ubuntu:latest on cloud-hypervisor (hybrid vsock transport), one vCPU, a 6.18 guest kernel and the container initrd from packaging/container-initrd. Only /urunit-agent in the initrd changed between the two columns (agent built with go1.26.4 from b2c3cb1 and from this branch); the urunc binary is the same.

100 runs each against one running guest before after
nerdctl exec <id> stat -f -c %T /root 30/100 empty 0/100 empty
nerdctl exec -it <id> uname -r 15/100 empty 0/100 empty
nerdctl exec <id> sh -c "uname -r; sleep 0.2" 0/100 empty 0/100 empty
nerdctl exec <id> uname -r (500 runs) 188/500 empty 0/500 empty
nerdctl exec -it <id> uname -r (500 runs) 70/500 empty 0/500 empty
nerdctl exec <id> head -c 1048576 /dev/zero | wc -c 983040 1048576

No run exited nonzero. After the fix, nerdctl exec <id> sh -c 'echo early; sleep 10 &' prints early and returns in 2.04s, and a -it session that leaves a SIGHUP-immune background job on the terminal returns in 2.36s. Exit codes (exit 7) and stderr still pass through.

Lint. make lint stops on an arm64 host (Unsupported architecture: arm64), so I ran its container command directly (golangci/golangci-lint:v2.9, golangci-lint run -v --timeout=5m). It reports 13 issues on this branch, all present on feat/unchanged_containers, which has 14: the gocyclo finding on (*connState).open is gone. --new-from-rev=origin/feat/unchanged_containers reports 0 issues. commitlint with .github/linters/commitlint.config.js passes on the commit.

LLM usage

The analysis, the patch, the tests and this description were produced with Claude Code, using Claude Opus 5.5 (claude-opus-5-5). The measurements above come from commands it ran. The PR stays a draft until I have reviewed it.

Checklist

  • I have read the contribution guide.
  • The linter passes locally (make lint). No new findings, see "Lint" above: the base branch already carries 14.
  • The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl). Not run; the live nerdctl exec loops above were run instead.
  • If LLMs were used: I have read the llm policy.

🤖 Generated with Claude Code

A short-lived `nerdctl exec` into a container boot guest sometimes prints
nothing and still exits 0. urunc exec ends the session as soon as the
agent's Exit frame arrives, so output relayed after that frame is lost.
The agent sends Exit as soon as it has waited for the process, and
nothing orders it after the relays of the process's output.

The pipe path loses output a second way. cmd.Wait closes the pipes from
StdoutPipe and StderrPipe as soon as the process exits, so its last write
can be discarded before the relay reads it. The os/exec documentation
says it is incorrect to call Wait before all reads from the pipe have
completed.

Relay stdout and stderr through plain pipes that the agent closes itself.
Track the relays of both the pipe and the pty path, and send Exit only
after they reach EOF. A process left in the background can hold the
output open, so the wait is bounded at two seconds after the exit.

TestAgentExecOutputBeforeExit runs 200 short sessions on each path. It
fails within the first few sessions without this change, and the existing
TestAgentStdinRoundTrip fails there too at times. On an Ubuntu 24.04 host
with a rootless nerdctl install, `nerdctl exec <id> stat -f -c %T /root`
returned empty stdout in 30 of 100 runs before this change and in none
after it. `nerdctl exec -it <id> uname -r` went from 15/100 to 0/100.

Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
ananos added a commit to NOFireAI/brig-standalone-linux that referenced this pull request Sep 25, 2026
build-bundle.sh named urunc and urunit by branch only. It cloned the tip of
feat/unchanged_containers and of urunit_agent, and recorded whatever commit
that was in pins.env after the fact. A push to either branch changed the
next bundle with no change here. Two builds of one bundle version could
carry different code, and no review ever saw the move.

Each is now pinned to one commit, URUNC_REF_DEFAULT and URUNIT_REF_DEFAULT.
The build fetches that commit by its SHA, since a --depth 1 --branch clone
stops containing a pinned commit as soon as the branch moves on. It then
fails if HEAD is anything else. pins.env records the commits in URUNC_REF
and URUNIT_REF as before, and adds URUNC_PINNED and URUNIT_PINNED.

The urunc pin moves to 0818ff1 on feat/unchanged_containers-exec-fixes.
That is feat/unchanged_containers (b2c3cb1, which rc6 to rc8 built) plus
the two urunit-agent exec fixes: urunc-dev/urunc#1059, which relays a guest
exec's output before reporting its exit, and urunc-dev/urunc#1060, which
accepts agent connections with close-on-exec. The agent in the initrd is
built from the same checkout. The urunit pin is 71bfdee, the tip of
urunit_agent and what rc6 to rc8 shipped.

URUNC_REF and URUNIT_REF use ${VAR-default}. Unset takes the pin, a SHA
builds that commit, and an empty value builds the tip of *_BRANCH, with a
warning in the build log and *_PINNED=false in pins.env. A branch set
without a ref is refused, and so is a ref that is not a full SHA. The stock
variant keeps its paths: with --urunc-version it takes the release, and it
builds no urunit.

README.md, DESIGN.md and docs/variants.md name the pinned commits and say
how to move a pin.

tests/source-pins.sh runs build-bundle.sh against a stub git and docker, so
nothing is fetched. It checks that a default build fetches both pins by
SHA, that a checkout other than the pin fails the build, that an empty ref
builds the branch tip and warns, and that a branch without a ref or a
short ref is refused. Against the previous build-bundle.sh, with the pins
filled in by hand, it fails the first check: the build asks for
`git clone --depth 1 --branch feat/unchanged_containers`. CI runs it in a
new Script tests step. CI's bundle job now also checks that each built
pins.env carries the pinned commits and *_PINNED=true.

Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
ananos added a commit to NOFireAI/brig-standalone-linux that referenced this pull request Sep 25, 2026
build-bundle.sh named urunc and urunit by branch only. It cloned the tip of
feat/unchanged_containers and of urunit_agent, and recorded whatever commit
that was in pins.env after the fact. A push to either branch changed the
next bundle with no change here. Two builds of one bundle version could
carry different code, and no review ever saw the move.

Each is now pinned to one commit, URUNC_REF_DEFAULT and URUNIT_REF_DEFAULT.
The build fetches that commit by its SHA, since a --depth 1 --branch clone
stops containing a pinned commit as soon as the branch moves on. It then
fails if HEAD is anything else. pins.env records the commits in URUNC_REF
and URUNIT_REF as before, and adds URUNC_PINNED and URUNIT_PINNED.

The urunc pin moves to 0818ff1 on feat/unchanged_containers-exec-fixes.
That is feat/unchanged_containers (b2c3cb1, which rc6 to rc8 built) plus
the two urunit-agent exec fixes: urunc-dev/urunc#1059, which relays a guest
exec's output before reporting its exit, and urunc-dev/urunc#1060, which
accepts agent connections with close-on-exec. The agent in the initrd is
built from the same checkout. The urunit pin is 71bfdee, the tip of
urunit_agent and what rc6 to rc8 shipped.

URUNC_REF and URUNIT_REF use ${VAR-default}. Unset takes the pin, a SHA
builds that commit, and an empty value builds the tip of *_BRANCH, with a
warning in the build log and *_PINNED=false in pins.env. A branch set
without a ref is refused, and so is a ref that is not a full SHA. The stock
variant keeps its paths: with --urunc-version it takes the release, and it
builds no urunit.

README.md, DESIGN.md and docs/variants.md name the pinned commits and say
how to move a pin.

tests/source-pins.sh runs build-bundle.sh against a stub git and docker, so
nothing is fetched. It checks that a default build fetches both pins by
SHA, that a checkout other than the pin fails the build, that an empty ref
builds the branch tip and warns, and that a branch without a ref or a
short ref is refused. Against the previous build-bundle.sh, with the pins
filled in by hand, it fails the first check: the build asks for
`git clone --depth 1 --branch feat/unchanged_containers`. CI runs it in a
new Script tests step. CI's bundle job now also checks that each built
pins.env carries the pinned commits and *_PINNED=true.

Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
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.

1 participant