Skip to content

fix(internet-connection-monitor): reap Chrome's orphaned zygote/GPU/renderer processes - #33

Closed
NickBorgers wants to merge 1 commit into
mainfrom
fix/reap-chrome-child-zombies
Closed

NickBorgers wants to merge 1 commit into
mainfrom
fix/reap-chrome-child-zombies

Conversation

@NickBorgers

@NickBorgers NickBorgers commented Aug 29, 2026

Copy link
Copy Markdown
Owner

Problem

internet-connection-monitor leaks a zombie process on every probe. Left running, it exhausts its container's PID limit and stops being able to fork() at all.

Impact (production, 1.3.0 on dockergeneric)

Every ~96 minutes, for at least 18 days, the monitor has been hitting its cgroup PID ceiling and getting fork-rejected by the kernel:

cgroup: fork rejected by pids controller in /system.slice/docker-8fae0138....scope

That fired 253 times between Aug 11 and Aug 29, 2026, evenly spaced ~96 minutes apart — roughly 14x/day, across the entire journal retention window. It went unnoticed because the container recovers fast enough that 10-minute Elasticsearch buckets never went empty, so the dashboards looked fine the whole time.

Measured leak rate, growing linearly with no plateau (~5.4 zombies per probe — one Chrome launch forks a zygote, a GPU process, and a renderer):

uptime=606s  pids=1313  zombies=1300
uptime=696s  pids=1553  zombies=1486
uptime=786s  pids=1743  zombies=1677

At ~2.1 zombies/sec against pids.max=11741, that fills in ~96 minutes — matching the kernel log cadence exactly.

Root cause

This binary runs as PID 1 in the container (ENTRYPOINT ["/app/internet-monitor"], no tini, no --init).

Each probe launches headless Chrome through chromedp.NewExecAllocator. Chrome then forks its own zygote, GPU, and per-navigation renderer processes. chromedp's ExecAllocator starts and waits on exactly one process — the top-level Chrome it launched. Chrome's own children are never something it knows about.

When the allocator context is cancelled, chromedp SIGKILLs that one Chrome process. Its children survive the instant, lose their parent, and get re-parented onto PID 1 — this program. Nothing ever calls wait() on them, so each becomes a permanent zombie.

This is not a skipped-cleanup bug. The defer cancelAlloc() / defer cancel() calls in TestSite already run on every path, and chromedp already waits correctly for the process it directly started. The gap is structural: Chrome's grandchildren are never something our Go code Start()ed, so no ordinary exec.Cmd call can ever Wait() for them.

One monitored site has been failing 100% of the time with ERR_CERT_DATE_INVALID for 21 days, but that is not the cause — it only makes the leak more visible. Every probe leaks identically, success or failure.

Fix

internal/reaper — a background goroutine, started once from main(), that reaps any exited child via SIGCHLD + non-blocking wait4(-1, ...). This is the same pattern an init process uses, and it is the only way to collect processes this program never directly started.

internal/browser — defense in depth. Chrome is placed in its own process group (Setpgid), and the exec.Cmd's Cancel is overridden to SIGKILL the whole group rather than just the Chrome PID. This kills Chrome's children with it at the end of each probe, instead of leaving them to be caught later by the reaper.

Note that chromedp.ModifyCmdFunc replaces chromedp's allocateCmdOptions rather than layering on it (allocate.go:183-186 is an if/else), so the Linux Pdeathsig default it would otherwise set is reimplemented here deliberately.

Why an in-process reaper rather than --init / tini

Correction to an earlier claim in this PR's history: --init does fix this. I originally wrote that tini could not help, on the reasoning that a live parent must reap its own children. That reasoning was wrong here, because the zombies are re-parented orphans — the app is PID 1, so Chrome's grandchildren land on it. Add --init and they land on tini instead, which reaps them.

Measured directly, unpatched 1.3.0, --pids-limit 400, identical workload:

pids.current zombies outcome
no --init 400 (ceiling) at t=40s 348 container dead by t=120s
--init 66–81, stable 0 healthy through t=160s

So init: true in the deployment is a valid one-line mitigation, and worth applying regardless of this PR.

This PR is still worth having, for a different reason than originally stated: it makes the binary correct however it is run — plain docker run, podman, Kubernetes without shareProcessNamespace, systemd, or bare metal — rather than depending on every future operator remembering --init. The two compose fine.

Verification

  • go build / go vet clean for darwin/arm64, linux/amd64, windows/amd64.
  • go test -race ./internal/reaper/... passes. The regression test Start()s children without ever calling Wait() — the exact shape of the leak — and asserts they are collected within 5s.
  • Reproduced from this repo's own Dockerfile at the pre-fix commit: --pids-limit 300, real expired.badssl.com (same ERR_CERT_DATE_INVALID class as production) plus google.com / github.com / example.com, 500ms inter-test delay. Zombies grew unbounded: 71/78 processes at t=90s, 131/138 at t=180s.
  • Same workload against the patched image — logs showed successes, ERR_CERT_DATE_INVALID, and a timeout all firing — held flat at total=7, zombies=0 at t=20/40/60/80s.

Not verified

  • Production behavior.
  • The full ~96-minute window that previously filled pids.max.
  • Panic and concurrent-probe paths. Probes run serially (internal/testloop), so this is unlikely to matter, but it was not specifically tested.

Known coupling, worth watching

The reaper's wait4(-1, ...) can consume Chrome's exit status before chromedp's own cmd.Wait() does. That is safe today only because chromedp discards that error (allocate.go:219, with an explicit TODO: do we care about this error). A future chromedp bump that starts checking it could break this quietly. There are no other os/exec call sites in this project, so nothing else is exposed.

Pre-existing failures, not from this change

go vet / go test already fail on main: internal/browser/controller_impl_test.go references an undefined categorizeError, and internal/health/health_test.go references an undefined HealthServer.Shutdown. Confirmed by testing main directly. This means CI has no green baseline to gate the new tests against.

Not merging — opening for review.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VJHwLHJ9R5jKKiVbgXgVTc

Zombies accumulated without bound in production (ghcr.io/nickborgers/
internet-connection-monitor:1.3.0 on dockergeneric): ~2.1 zombies/sec,
reaching the container's cgroup pids.max (11741) roughly every 96
minutes, 253 times between Aug 11-29 2026.

Root cause: this binary is the container's PID 1 (Dockerfile:
`ENTRYPOINT ["/app/internet-monitor"]`, no tini/`--init`). Each probe
launches headless Chrome via chromedp.NewExecAllocator, and Chrome
forks its own zygote, GPU, and per-navigation renderer processes.
chromedp only ever exec.Cmd.Start()s and Wait()s the single top-level
Chrome process - Chrome's own children are never its concern.
Cancelling the allocator context (on success, error, or timeout) has
chromedp send Chrome's process a lone SIGKILL. Its children survive
that, lose their parent, and - because there is no init process in
the container - are re-parented directly onto this program. Nothing
was ever waiting on them, so once each one exits it becomes a
permanent zombie. The already-correct `defer cancel()` /
`defer cancelAlloc()` calls in TestSite (including on the
ERR_CERT_DATE_INVALID hot error path) don't help: they were never the
gap, since chromedp does properly wait for the one process it starts.

Fix, two parts:

1. internal/reaper: a background goroutine, started once from main(),
   that reaps any exited child of this process via SIGCHLD + wait4(-1,
   WNOHANG) - the same pattern an init process (tini) uses. This is
   the actual fix: it is the only way to collect processes we never
   directly started, including ones re-parented onto us.

2. internal/browser: put Chrome in its own process group
   (Setpgid) and override the exec.Cmd's Cancel to SIGKILL the whole
   group instead of just the Chrome PID. This kills Chrome's children
   alongside it immediately instead of leaving them running as
   orphans until they exit on their own, minimizing how long they
   exist as reapable zombies before the reaper part 1 catches them.

Verified:
- go build / go vet clean for darwin/arm64, linux/amd64, windows/amd64
- go test -race ./internal/reaper/... passes; regression test starts
  child processes via exec.Cmd.Start() *without* calling Wait() (the
  exact "leaked child" shape) and asserts they are gone within 5s.
- Reproduced the bug locally: built the pre-fix image from this repo's
  own Dockerfile, ran it with --pids-limit 300 against a real
  expired.badssl.com (ERR_CERT_DATE_INVALID, same error class as
  production) plus google.com/github.com/example.com at a 500ms
  inter-test delay. Zombie count in /proc grew unbounded: 71/78 procs
  at t=90s, 131/138 at t=180s.
- Same test against the patched image, same workload (successes,
  ERR_CERT_DATE_INVALID, and a timeout all observed in the logs):
  total=7 zombies=0 held flat at t=20/40/60/80s.
- Not run in production; not verified over the full ~96 minute
  window that would fill pids.max, and not verified under concurrent/
  panic conditions.

Pre-existing, unrelated to this change (present on master too, before
and after): internal/browser/controller_impl_test.go references an
undefined `categorizeError` function, and internal/health/health_test.go
references an undefined `HealthServer.Shutdown` method - both fail
`go vet`/`go test` on master already.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJHwLHJ9R5jKKiVbgXgVTc
@NickBorgers

Copy link
Copy Markdown
Owner Author

Closing in favor of #34, which fixes the same leak by requiring an init process (init: true / docker run --init) rather than reimplementing one inside the binary.

Adversarial review flagged a genuine hazard in this approach that I had not spotted. The in-process reaper's wait4(-1, ...) can reap Chrome before chromedp's own cmd.Wait() does, which frees Chrome's PID for reuse — while cmd.Cancel still holds that stale number and would kill(-pid, SIGKILL) it. That could signal an unrelated process group. It is also only safe alongside chromedp at all because chromedp currently discards the cmd.Wait() error (allocate.go:219, with an explicit TODO: do we care about this error); a future bump that starts checking it would break this quietly.

Three smaller findings were also confirmed:

  • GOOS=plan9 fails to build — undefined: setProcessGroupAndGroupKill. reaper_other.go uses !unix, but the browser procgroup files only cover unix and windows.
  • The Linux Pdeathsig reimplementation is incomplete: chromedp skips it when LAMBDA_TASK_ROOT is set (allocate_linux.go), this did not.
  • internal/reaper's regression test is flaky. It calls Wait4 on specific PIDs, competing with the reaper it is testing, and fails after ~4µs rather than honoring its own 5s deadline. Reproduced: go test ./internal/reaper -count=50 -race fails repeatedly. The original "tests pass" claim held for a single run only.

The correction that mattered: I had claimed tini could not help here, reasoning that a live parent must reap its own children. That was wrong. The leaked processes are re-parented orphans — the binary is PID 1, so they land on it by default rather than by ownership. Add an init and they land on tini, which reaps them. Measured with --pids-limit 400: without --init, the ceiling was hit at t=40s with 348 zombies and the container died by t=120s; with --init, 0 zombies and stable.

One real trade is being given up. A process-group kill alone does not fix the leak — killing the group SIGKILLs Chrome and its children together, so Chrome cannot reap them and they still orphan onto PID 1. Reaping genuinely requires an init. So #34 makes the binary depend on being run correctly, rather than being self-sufficient. Given that a standard init is the normal way to run any container that spawns subprocesses, that seems like the right trade.

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