oom: bound the wait for a watcher to stop

[locker95]

Problem

Reported in #13814 as a v2.2.5 → v2.3.2 regression: after a pod with a VFIO/IOMMU passthrough device is deleted, its shim is orphaned — the container is reaped and the sandbox is gone from crictl pods, but the shim process is still alive and the task exit is never delivered. containerd itself stays responsive, yet the node stops creating new sandboxes while still reporting Ready, and only a runtime restart clears it.

The reporter's SIGUSR1 dumps show that as a large, permanent goroutine leak — roughly two thirds to four fifths of all goroutines stuck for ~47h, matching the last runtime restart, clustered exactly where an undelivered task exit would leave them:

43  core/runtime/v2.loadShim.func3                    [IO wait]
41  client.(*task).Wait.func1                         [select]
38  internal/cri/io.(*ContainerIO).Pipe               [IO wait]
25  internal/cri/server.(*criService).startContainerExitMonitor.func1
14  ...podsandbox.(*Controller).RecoverContainer.func2

• Fixes CRI ContainerStatus/ExecSync hang and delayed sandbox teardown after VFIO/IOMMU device-pod deletion #13814

Root cause

The regression is handleProcessExit in cmd/containerd-shim-runc-v2/task/service.go. On v2.2.5 the exit was published immediately:

func (s *service) handleProcessExit(e runcC.Exit, c *runc.Container, p process.Process) {
	p.SetExited(e.Status)
	s.send(&eventstypes.TaskExit{...})

On 2.3.x an unbounded wait sits in front of that send. It arrived in 8ac7e3c0 ("cmd/containerd-shim-runc-v2: use experimental OOM package") from #12714, "Update OOMKilled event handling", merged 2026-01-07 — in 2.3.x, not in 2.2.5, which matches the reported version boundary:

	p.SetExited(e.Status)
	_, isInit := p.(*process.Init)
	if isInit {
		if err := s.cg2oom.Stop(c.ID); err != nil {   // <-- waits, with no bound
			...
		}
	}
	s.send(&eventstypes.TaskExit{...})

Stop ends in internal/oom:

func (w *watcher) stop() error {
	cerr := w.eventFD.Close()
	...
	werr := <-w.errCh          // only resolved when the watcher goroutine returns
	return errors.Join(cerr, werr)
}

so publishing a task exit is made to depend on another goroutine returning. That goroutine, before it closes errCh, both reads the container's cgroup files (memory.events) and can call eventFn → publisher.Publish. Neither is guaranteed to come back promptly while a device is being torn down, which fits the VFIO/IOMMU trigger.

Two things turn that into a permanently wedged node rather than a slow exit:

• It runs on the shim's only exit goroutine. processExits is a single for e := range s.ec loop that calls the handlers inline, and for an init process with no running execs handleInitExit calls handleProcessExit directly on it. So the stall stops every subsequent exit for that shim, not just this one.
• Nothing retries. The exit is never published, so task.Wait, the IO fifos and the CRI exit monitors never return — and on restart RecoverContainer re-attaches and gets stuck again, which is why the dumps show RecoverContainer goroutines too.

I could not pin down which syscall the watcher goroutine ultimately parks in (the reporter could not dump an orphaned shim, since its log target is already deleted). That is worth saying plainly — but it does not change what needs fixing: the dependency is unbounded by construction, and any stall in it costs the shim all of its exit events.

Fix

Bound the wait in watcher.stop() and report a timeout instead of blocking forever.

The wait is deliberately kept rather than removed. Closing the event FD makes the watcher do one final memory.events check, so waiting is what lets a last TaskOOM be delivered before TaskExit — the ordering #12714 set out to fix. Draining is sub-millisecond on a healthy system, so the bound only trips when something is already wrong, and there losing the last OOM annotation is far cheaper than never publishing a task exit.

Alternative considered

The obvious smaller change is to move s.cg2oom.Stop() after s.send(TaskExit), restoring 2.2.x ordering. I did not do that, for two reasons:

• it would give up Update OOMKilled event handling #12714's ordering guarantee for every container, not just stalled ones, so a real OOM kill could report its exit before (or without) its OOM event;
• it would not actually unwedge the shim. Stop would still run unbounded on the processExits goroutine right after the send, so this container's exit would get through and every later exit on that shim would still be lost.

Bounding fixes both. Happy to switch if maintainers prefer the ordering change, or to take the timeout as a context.Context argument on Interface.Stop instead of a package constant — there is exactly one production caller, so either is cheap. 2s is a starting point, not a considered SLO; say the word if you want a different value.

Also worth flagging, but left alone here: oomWatchers.Stop never deletes the watcher from its map, so every exited container leaves an entry behind. Unrelated to this hang, so I did not fold it in.

Testing

internal/oom/watcher_test.go gets a watcher whose goroutine never resolves errCh — which is what a goroutine parked in a cgroup read looks like from stop's side — and asserts stop still returns. A second test asserts the watcher's own error is still propagated, so bounding the wait did not swallow it. Neither needs root or cgroups, unlike the existing TestWatcher.

This package is //go:build linux and I am on darwin, so I ran the tests through go test -overlay, substituting darwin stubs for the inotify/cgroup-v2 symbols in utils.go (and the one inotify constant start() uses). stop() itself and both tests are the real code, unmodified — the stubs are not on the path either test takes. Repo tree untouched; the overlay files live outside it.

Before, against stop() as it is on main:

$ go test -overlay=... ./internal/oom/ -run TestWatcherStop -count=1 -race -v
=== RUN   TestWatcherStopDoesNotBlockForever
    watcher_test.go:71: watcher.stop never returned: the shim would stop publishing task exits
--- FAIL: TestWatcherStopDoesNotBlockForever (32.00s)
=== RUN   TestWatcherStopReportsWatcherError
--- PASS: TestWatcherStopReportsWatcherError (0.00s)
FAIL

After:

=== RUN   TestWatcherStopDoesNotBlockForever
--- PASS: TestWatcherStopDoesNotBlockForever (2.00s)
=== RUN   TestWatcherStopReportsWatcherError
--- PASS: TestWatcherStopReportsWatcherError (0.00s)
ok  	github.com/containerd/containerd/v2/internal/oom	3.632s

Native linux checks, since the overlay run is not the real build:

$ gofmt -l internal/oom/
$ GOOS=linux go vet ./internal/oom/... ./cmd/containerd-shim-runc-v2/...
$ GOOS=linux go test -c -o /dev/null ./internal/oom/     # test binary builds
$ GOOS=linux go build ./...

I have not run the pre-existing TestWatcher, which needs root and cgroup v2. The end-to-end behaviour (deleting a VFIO passthrough pod and watching the shim exit cleanly) needs the reporter's hardware and cannot be covered here either.

This does not overlap #13635, which touches start() in the same file for a different issue.

[Copilot]

Pull request overview

This PR addresses a regression in containerd’s cgroup v2 OOM watcher shutdown path that could indefinitely block shim exit processing, preventing TaskExit from being published and causing long-lived goroutine leaks and node-level “stuck” behavior as described in #13814.

Changes:

• Add a bounded timeout to internal/oom watcher shutdown so Stop() can’t block forever waiting for the watcher goroutine to finish.
• Add unit tests covering the non-blocking timeout behavior and ensuring watcher errors are still propagated.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/oom/watcher.go	Bounds watcher.stop() with a timer so shim exit processing can’t wedge on an unbounded wait for the watcher goroutine.
internal/oom/watcher_test.go	Adds targeted tests ensuring stop() times out instead of hanging and still reports real watcher errors.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[locker95]

The only red job is VM integration (almalinux-9, systemd, runc). Looking at the log, failures are from registry network flakes during image pull (not the OOM watcher change):

TestImagePullWithDiscardContent: connection reset by peer (pkg-containers.githubusercontent.com)
TestIDMappedOverlay/...: connection reset by peer

Other matrix cells look green, including:

• all Linux Integration (ubuntu 22.04/24.04, arm, cgroupfs/systemd)
• VM integration (almalinux-10, …)
• CRI-in-UserNS, E2E Kubernetes Node, fuzz, linters, binaries

This PR only touches internal/oom/watcher.go (+ tests). Empty commit pushed to re-run CI.

[locker95]

Latest CI red jobs still look environmental:

1. E2E / Kubernetes Node — 535 passed, 1 failed: [sig-node] Summary API … should report resource usage through the stats api [NodeConformance]. That path is kubelet/stats summary, not the OOM watcher wait change.

2. VM integration (almalinux-8, systemd, runc) — only this matrix cell failed; almalinux-8 cgroupfs, almalinux-9 both, almalinux-10 both, and all fedora-44 VMs passed. Full Linux Integration matrix also green.

PR still only touches internal/oom/watcher.go + unit tests. Empty commit to re-run.