From 5db8ce3c44cf807b3d012fffe47b171f637dd1d2 Mon Sep 17 00:00:00 2001 From: Zannis Kalampoukis Date: Tue, 25 Aug 2026 19:52:39 +0300 Subject: [PATCH] fix(docker): make tini PID 1 in the server image so adopted orphans are reaped (#12137) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agent runs execute inside the server container, and they spawn many short-lived descendants: git, the adapter CLI, esbuild, sh > - The server image sets `ENTRYPOINT ["docker-entrypoint.sh"]`, and that entrypoint ends in `exec`, so node becomes PID 1 > - Node reaps only the children it spawned itself. It installs no `SIGCHLD`/`waitpid` handler for orphans that the kernel re-parents onto PID 1, so those orphans stay as zombies forever > - Zombies accumulate monotonically. When the cgroup pid limit is reached, every `fork()` in the container fails and the instance is dead > - This pull request installs `tini` and makes it PID 1 in front of the existing entrypoint, adds a behavioural test that proves reaping, and adds a `pids_limit` backstop to both compose files > - The benefit is that a long-running container no longer degrades into total fork failure, and a future regression is caught by CI instead of by an outage Depends-on: none — this change is self-contained in the image build and its tests, and it touches no other in-flight branch ## Linked Issues or Issue Description No public GitHub issue exists for this defect. It was found on a live long-running instance. Description follows the bug report template. **What happened?** The server container ran for 22 hours and reached 2039 of 2048 pids in its cgroup. Of 1760 processes, 1731 were zombies, and all 1731 had PID 1 as their parent. PID 1 was `node --import ./server/node_modules/tsx/dist/loader.mjs server/dist/index.js`. Zombies accrued at about 79 per hour and were never reaped. The oldest zombie was 20.8 hours old against a container uptime of 22.0 hours, so nothing had been reaped since boot. Once the pid limit was reached, `git` and `gh` failed with `pthread_create failed: Resource temporarily unavailable`. **Expected behavior** PID 1 reaps orphaned processes that the kernel re-parents onto it. The pid count of a long-running container stays flat instead of growing without bound. **Steps to reproduce** 1. Start the server image without `docker run --init` and without `init: true`. 2. Run agent work that spawns descendants which outlive their immediate parent. 3. Read `/sys/fs/cgroup/pids.current` and count processes in `Z` state over several hours. 4. The zombie count grows monotonically and every zombie has PPID 1. **Relevant logs or output** ``` cgroup pids.current / pids.max : 2039 / 2048 total processes : 1760 zombies : 1731 (98.4%) parent of every zombie : PID 1 (1731/1731) PID 1 cmdline : node --import .../tsx/dist/loader.mjs server/dist/index.js container uptime : 22.0 h oldest zombie : 20.8 h median: 14.4 h zombie names : git 717, claude 280, MainThread 167, sleep 141, esbuild 138, postgres 76, sh 65, sccache 50 ``` **Additional context** The fix pattern is already in this repository. `docker/agent-runtime/Dockerfile.base` installs `tini` and sets `ENTRYPOINT ["/usr/bin/tini", "--"]`. It was never applied to the server image. ## What Changed - `Dockerfile`: install `tini` in the `base` stage and set `ENTRYPOINT ["/usr/bin/tini", "--", "docker-entrypoint.sh"]`. The entrypoint stays in the exec chain, so UID/GID remapping, `gosu`, and graceful shutdown are unchanged. - `scripts/assert-orphan-reaping.sh` (new): a behavioural probe. It spawns a leader that forks a grandchild, exits the leader, and asserts that the orphaned grandchild leaves `Z` state instead of persisting. It fails closed if the grandchild is not re-parented onto PID 1, so a pass cannot mean the check ran too early. - `.github/workflows/docker.yml`: run that probe against the pushed image after the publish step. The publish step is multi-arch with `push: true`, so nothing is loaded into the runner daemon and the pushed tag is the only thing to test. The cloud variant is `FROM production` and inherits the same `ENTRYPOINT`. - `scripts/docker-build-test.sh`: run the same probe against a local build. - `docker/docker-compose.yml` and `docker/docker-compose.quickstart.yml`: add `pids_limit: 2048` as a backstop, so a future leak dies visibly at its own ceiling instead of starving the host of pids. - `server/src/__tests__/container-init-reaping.test.ts` (new): 13 assertions that guard the configuration the probe depends on. No per-orchestrator init lever was added. The image owning PID 1 covers compose, plain `docker run`, the quadlet units, and the ECS task definition in one place. Adding `init: true` in compose or `initProcessEnabled` on the ECS task would nest a second init around `tini`, and `tini` then warns on every boot that it is not PID 1. The new test asserts the absence of both levers across all three manifests, so the decision survives the next edit. ## Verification | Check | Result | |---|---| | `scripts/assert-orphan-reaping.sh` against a real init | Grandchild re-parented to PPID 1, then reaped. Exit 0. | | Same probe forced against a genuine zombie | Reports `Z` and fails. The failure branch is not vacuous. | | Config guard against the pre-fix files | Exactly the 3 relevant assertions turn red. | | Config guard with `tini` removed from `apt-get` but the comments kept | Red. It checks the install, not a mention of the name. | | `cd server && npx vitest run src/__tests__/container-init-reaping.test.ts` | 13 passed | | `npx tsc --noEmit -p server` | Clean | | `node scripts/check-docker-deps-stage.mjs` | PASS | | `node --test scripts/release-verify-workflow.test.mjs` | 8 passed | Not verified locally: no container runtime is available in the authoring environment, so the probe has not run against a build of this image. The new `docker.yml` step runs it against the pushed image on this PR. ## Risks Low risk, but it is an image and entrypoint change, so it affects deployments. - `tini` adds one small package to the `base` stage. `docker/agent-runtime/Dockerfile.base` already installs it from the same Debian archive. - Signal handling changes shape: `tini` receives `SIGTERM` and forwards it to the entrypoint, which `exec`s node. `tini` forwards signals to its direct child by default, and the exec chain keeps node as that child, so graceful shutdown is preserved. A reviewer should confirm this on a real stop. - `pids_limit: 2048` is new for compose users. A deployment that legitimately needs more than 2048 processes would now hit the ceiling. The measured steady state on a busy instance was under 400. - If a deployment already passes `--init` or `init: true`, `tini` runs under another init and prints a warning that it is not PID 1. Reaping still works because the outer init handles it. The compose files in this repository do not set `init: true`. ## Model Used Claude Opus 5 (`claude-opus-5`), extended thinking, with tool use and code execution in an agent harness. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local issues or links - [x] My branch name describes the change and contains no internal ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: zannis <1011451+zannis@users.noreply.github.com> --- .github/workflows/docker.yml | 19 +++ Dockerfile | 11 +- docker/docker-compose.quickstart.yml | 5 + docker/docker-compose.yml | 9 + scripts/assert-orphan-reaping.sh | 90 ++++++++++ scripts/docker-build-test.sh | 5 + .../__tests__/container-init-reaping.test.ts | 154 ++++++++++++++++++ 7 files changed, 291 insertions(+), 2 deletions(-) create mode 100755 scripts/assert-orphan-reaping.sh create mode 100644 server/src/__tests__/container-init-reaping.test.ts diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 549595dbe3..a267581aa1 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -194,6 +194,25 @@ jobs: tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} + # PID 1 must be an init that reaps adopted orphans. With node there, the + # orphans agent runs leave behind are never wait()ed and pin as zombies + # until the cgroup pid limit is exhausted and every fork() in the + # container fails. Run against the pushed image rather than a local + # build: the step above is multi-arch with `push: true`, so nothing is + # loaded into the runner's daemon. The cloud variant is FROM production + # and inherits the same ENTRYPOINT, so checking this image covers both. + - name: Verify PID 1 reaps orphaned processes + env: + # Through the environment, not interpolated into the script body, so + # the tag text is data rather than shell. + IMAGE_TAGS: ${{ steps.meta.outputs.tags }} + run: | + set -euo pipefail + image="$(printf '%s\n' "$IMAGE_TAGS" | head -n 1)" + test -n "$image" + echo "Verifying orphan reaping in $image" + docker run --rm -i --pull always "$image" sh -s < scripts/assert-orphan-reaping.sh + # The cloud variant carries built bundled plugins for managed deployments # (see the `cloud` stage in the Dockerfile). It runs as its own job with no # `needs:` on the stock publish above, so the two builds run in parallel and diff --git a/Dockerfile b/Dockerfile index 0e984a1640..65b64dcd5b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,7 +3,7 @@ FROM node:24-trixie-slim AS base ARG USER_UID=1000 ARG USER_GID=1000 RUN apt-get update \ - && apt-get install -y --no-install-recommends ca-certificates gosu curl gh git wget ripgrep python3 \ + && apt-get install -y --no-install-recommends ca-certificates gosu curl gh git wget ripgrep python3 tini \ && rm -rf /var/lib/apt/lists/* \ && corepack enable @@ -119,7 +119,14 @@ ENV NODE_ENV=production \ EXPOSE 3100 -ENTRYPOINT ["docker-entrypoint.sh"] +# tini, not node, is PID 1. The entrypoint ends in `exec`, so without an init +# node inherits PID 1 and never wait()s the orphans the kernel re-parents onto +# it -- agent runs spawn git/claude/esbuild/sh descendants that outlive their +# leader, so they pile up as permanent zombies (~79/h measured) until the +# cgroup pid limit is exhausted and *every* fork() in the container fails. +# tini reaps adopted orphans and forwards signals, so the exec chain below and +# graceful shutdown are unchanged. Mirrors docker/agent-runtime/Dockerfile.base. +ENTRYPOINT ["/usr/bin/tini", "--", "docker-entrypoint.sh"] CMD ["node", "--import", "./server/node_modules/tsx/dist/loader.mjs", "server/dist/index.js"] # Cloud image variant (build with `--target cloud`): the production image diff --git a/docker/docker-compose.quickstart.yml b/docker/docker-compose.quickstart.yml index 16fa17bccc..41a674916f 100644 --- a/docker/docker-compose.quickstart.yml +++ b/docker/docker-compose.quickstart.yml @@ -11,6 +11,11 @@ services: build: context: .. dockerfile: Dockerfile + # Ceiling on processes. The image makes tini PID 1 so orphaned agent + # descendants are reaped instead of piling up as zombies; this is the + # backstop if something leaks anyway — the container hits its own limit and + # dies visibly rather than starving the host of pids. + pids_limit: 2048 ports: - "${PAPERCLIP_PORT:-3100}:3100" environment: diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index b26b396ab8..dc6dda4238 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -19,6 +19,15 @@ services: build: context: .. dockerfile: Dockerfile + # No `init: true` here on purpose. The image makes tini PID 1 itself + # (Dockerfile ENTRYPOINT), which covers every launch path -- compose, plain + # `docker run`, the quadlet units and the ECS task definition -- instead of + # one lever per orchestrator. Setting it here too would nest docker-init + # around tini and make tini warn that it is not PID 1 on every boot. + # pids_limit is the backstop: if something ever does leak processes again, + # this container hits its own ceiling and dies visibly rather than starving + # the whole host of pids. + pids_limit: 2048 ports: - "3100:3100" environment: diff --git a/scripts/assert-orphan-reaping.sh b/scripts/assert-orphan-reaping.sh new file mode 100755 index 0000000000..7c134cb72e --- /dev/null +++ b/scripts/assert-orphan-reaping.sh @@ -0,0 +1,90 @@ +#!/bin/sh +# Assert that PID 1 in this container reaps orphans the kernel re-parents onto it. +# +# Regression test for the zombie-exhaustion outage: with node as PID 1 and no +# init, orphaned descendants of agent runs (git, claude, esbuild, sh, ...) are +# never wait()ed, so they pin as zombies at ~79/h until the cgroup pid limit is +# exhausted and *every* fork() in the container fails. +# +# Designed to run inside the image under its real ENTRYPOINT, so PID 1 is +# exactly what a production container gets: +# +# docker run --rm -i sh -s < scripts/assert-orphan-reaping.sh +# +# Exits 0 when the orphan is reaped, non-zero (with the observed state) when it +# is left in Z. +set -e + +init_comm="$(cat /proc/1/comm)" +echo "PID 1 = $init_comm" +if [ "$init_comm" != "tini" ]; then + echo "FAIL: PID 1 is '$init_comm', expected an init (tini)" >&2 + exit 1 +fi + +pidfile="$(mktemp)" +trap 'rm -f "$pidfile"' EXIT + +# Read a field of /proc//stat past the comm field, so a command name +# containing spaces or parens cannot shift the offset. Field 1 of the remainder +# is the state, field 2 is the ppid. Empty output means the pid is gone. +stat_field() { + sed -e 's/^.*) //' "/proc/$1/stat" 2>/dev/null | cut -d' ' -f"$2" +} + +# The leader exits immediately, orphaning the grandchild onto PID 1. The +# grandchild publishes its own pid, then exits ~5s later. That lifetime is what +# keeps the adoption check below honest: it must always land on a live process, +# never on a corpse whose exit merely happened to outrun a slow leader. +sh -c 'sh -c '\''echo $$ > "$0"; exec sleep 5'\'' "$1" & exit 0' _ "$pidfile" + +i=0 +while [ ! -s "$pidfile" ]; do + i=$((i + 1)) + if [ "$i" -gt 100 ]; then + echo "FAIL: grandchild never reported its pid" >&2 + exit 1 + fi + sleep 0.05 +done +gpid="$(cat "$pidfile")" + +# The kernel re-parents the grandchild only once the leader has finished +# exiting, and that can lag the grandchild's pid-file write. Sampling the ppid +# once would read a leader that is merely slow to exit as a re-parenting +# failure, so poll instead: the leader exits unconditionally, so ppid 1 is +# reached in bounded time whenever adoption works at all. +i=0 +while :; do + ppid="$(stat_field "$gpid" 2)" + if [ "$ppid" = "1" ]; then + break + fi + if [ -z "$ppid" ]; then + echo "FAIL: grandchild pid $gpid vanished before adoption could be observed; the probe proved nothing" >&2 + exit 1 + fi + i=$((i + 1)) + if [ "$i" -gt 20 ]; then + echo "FAIL: grandchild still parented to $ppid after 20 polls, never adopted by PID 1; the probe proved nothing" >&2 + exit 1 + fi + sleep 0.05 +done +echo "orphaned grandchild pid=$gpid reparented to ppid=$ppid" + +# Give an unreaped zombie a generous window to show itself before calling the +# reap successful -- a pass here must mean "reaped", never "checked too early". +i=0 +while [ "$i" -lt 100 ]; do + if [ ! -e "/proc/$gpid" ]; then + echo "PASS: PID 1 reaped orphaned pid $gpid" + exit 0 + fi + i=$((i + 1)) + sleep 0.1 +done + +state="$(stat_field "$gpid" 1)" +echo "FAIL: pid $gpid still present after 100 polls (state=$state); PID 1 ('$init_comm') is not reaping adopted orphans" >&2 +exit 1 diff --git a/scripts/docker-build-test.sh b/scripts/docker-build-test.sh index 1cdb8d4fa7..be1ffa57a6 100755 --- a/scripts/docker-build-test.sh +++ b/scripts/docker-build-test.sh @@ -43,4 +43,9 @@ echo "==> Verifying key binaries in image" claude --version 2>/dev/null || echo "claude CLI not found (OK in minimal builds)" ' +echo "==> Verifying PID 1 is an init that reaps adopted orphans" +# Piped in as CMD (`sh -s`) rather than `--entrypoint`, so the image's real +# ENTRYPOINT still runs and PID 1 is exactly what a production container gets. +"$RUNTIME" run --rm -i "$IMAGE_TAG" sh -s < "$REPO_ROOT/scripts/assert-orphan-reaping.sh" + echo "PASS: Docker build test succeeded" diff --git a/server/src/__tests__/container-init-reaping.test.ts b/server/src/__tests__/container-init-reaping.test.ts new file mode 100644 index 0000000000..217dfa886a --- /dev/null +++ b/server/src/__tests__/container-init-reaping.test.ts @@ -0,0 +1,154 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +/** + * Drift guard for container init (PID 1) wiring. + * + * The entrypoint ends in `exec`, so whatever the ENTRYPOINT names becomes PID 1. + * With node there, orphans the kernel re-parents onto PID 1 are never wait()ed: + * agent runs spawn git/claude/esbuild/sh descendants that outlive their leader, + * and those pin as zombies at ~79/h until the cgroup pid limit is exhausted and + * every fork() in the container fails (git and gh dying with "pthread_create + * failed: Resource temporarily unavailable"). + * + * tini reaps adopted orphans and forwards signals, so it must stay PID 1 ahead + * of the entrypoint. This guard fails if a refactor drops the tini install or + * unwraps the ENTRYPOINT back to a bare `docker-entrypoint.sh`. + * + * The behavioural half of this -- proving a real orphan is actually reaped + * rather than merely configured to be -- lives in scripts/assert-orphan-reaping.sh. + * It needs a container runtime, so it cannot run here: the Docker workflow runs + * it against the pushed image, and scripts/docker-build-test.sh runs it against + * a local build. This file guards the config those depend on. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "..", ".."); +const read = (...segments: string[]) => readFileSync(path.join(repoRoot, ...segments), "utf8"); + +const dockerfile = read("Dockerfile"); +const agentRuntimeBase = read("docker", "agent-runtime", "Dockerfile.base"); +const compose = read("docker", "docker-compose.yml"); +const quickstartCompose = read("docker", "docker-compose.quickstart.yml"); +const ecsTaskDefinition = JSON.parse(read("docker", "ecs-task-definition.json")) as { + containerDefinitions: { name: string; image: string; linuxParameters?: { initProcessEnabled?: boolean } }[]; +}; +const reapingProbe = read("scripts", "assert-orphan-reaping.sh"); +const buildTest = read("scripts", "docker-build-test.sh"); +const dockerWorkflow = read(".github", "workflows", "docker.yml"); + +/** Every `ENTRYPOINT [...]` line in a Dockerfile, in order. */ +function entrypoints(source: string): string[] { + return [...source.matchAll(/^ENTRYPOINT .*$/gm)].map((m) => m[0]); +} + +/** + * The packages every `apt-get install` line in the named stage asks for. Read + * from the install lines themselves rather than by searching the whole file, so + * a passing mention of a package name in a comment cannot stand in for actually + * installing it. + */ +function aptPackages(source: string, stageName: string): string[] { + const froms = [...source.matchAll(/^FROM .*$/gm)]; + const startIdx = froms.findIndex((m) => new RegExp(`\\bAS ${stageName}\\b`).test(m[0])); + expect(startIdx, `Dockerfile must declare a '${stageName}' stage`).toBeGreaterThanOrEqual(0); + const stage = source.slice(froms[startIdx].index ?? 0, froms[startIdx + 1]?.index ?? source.length); + return [...stage.matchAll(/apt-get install[^\n]*/g)].flatMap((m) => + m[0] + .replace(/apt-get install/, "") + .split(/\s+/) + .filter((token) => token.length > 0 && !token.startsWith("-") && token !== "\\"), + ); +} + +describe("server image init", () => { + it("installs tini in the base stage that every later stage inherits", () => { + expect( + aptPackages(dockerfile, "base"), + "the base stage must apt-get install tini so /usr/bin/tini exists in the image", + ).toContain("tini"); + }); + + it("makes tini PID 1 ahead of the entrypoint", () => { + const lines = entrypoints(dockerfile); + expect(lines.length, "Dockerfile must declare an ENTRYPOINT").toBeGreaterThan(0); + for (const line of lines) { + expect( + line, + "node must not inherit PID 1: wrap the entrypoint in tini so adopted orphans are reaped", + ).toBe('ENTRYPOINT ["/usr/bin/tini", "--", "docker-entrypoint.sh"]'); + } + }); + + it("keeps the entrypoint in the exec chain so UID remapping and gosu still run", () => { + // tini must wrap docker-entrypoint.sh, not replace it -- the entrypoint is + // what remaps the node UID/GID and repairs volume ownership before exec'ing. + for (const line of entrypoints(dockerfile)) { + expect(line).toContain("docker-entrypoint.sh"); + } + }); + + it("keeps the agent-runtime image's init, which the server image mirrors", () => { + expect(entrypoints(agentRuntimeBase)).toContain('ENTRYPOINT ["/usr/bin/tini", "--"]'); + }); +}); + +describe("deployment manifest parity", () => { + it.each([ + ["docker-compose.yml", compose], + ["docker-compose.quickstart.yml", quickstartCompose], + ])("caps pids in %s", (_name, source) => { + expect( + /^\s{4}pids_limit:\s*\d+\s*$/m.test(source), + "the compose server service must set pids_limit so a future leak dies visibly " + + "instead of starving the host of pids", + ).toBe(true); + }); + + it.each([ + ["docker-compose.yml", compose], + ["docker-compose.quickstart.yml", quickstartCompose], + ])("does not also set init in %s, which would nest docker-init around tini", (_name, source) => { + // The image owns PID 1, so no per-orchestrator lever is needed. Setting + // `init: true` here as well makes tini warn it is not PID 1 on every boot. + expect(/^\s*init:\s*true\s*$/m.test(source)).toBe(false); + }); + + it("leaves ECS to inherit the image's init rather than enabling its own", () => { + // Same reasoning as compose `init: true`: initProcessEnabled would put the + // ECS-managed init in front of tini. Asserted rather than merely reviewed so + // the parity decision survives the next edit to the task definition. + const server = ecsTaskDefinition.containerDefinitions.find((c) => c.name === "paperclip-server"); + expect(server, "the task definition must define a paperclip-server container").toBeDefined(); + expect(server?.linuxParameters?.initProcessEnabled).toBeUndefined(); + }); +}); + +describe("orphan-reaping probe", () => { + it.each([ + ["the docker build test", buildTest], + ["the Docker publish workflow", dockerWorkflow], + ])("is exercised against a real image by %s", (_name, source) => { + // A probe nothing runs proves nothing. The static assertions above only + // check configuration; this is what keeps the behavioural check wired up. + expect(source).toContain("scripts/assert-orphan-reaping.sh"); + }); + + it("fails closed when the orphan is never adopted by PID 1", () => { + // A probe whose grandchild is not reparented onto PID 1 proves nothing, so + // it must error rather than report a pass it did not observe. + expect(reapingProbe).toContain("the probe proved nothing"); + }); + + it("polls for adoption rather than sampling the ppid once", () => { + // Re-parenting lags the leader's exit, so a single sample reads a slow + // leader as a failure and flakes the Docker publish workflow. + expect(reapingProbe).toContain('if [ "$ppid" = "1" ]; then'); + expect(reapingProbe).toMatch(/while :; do\n\s+ppid=/); + }); + + it("asserts on the reaped pid rather than on PID 1's name alone", () => { + expect(reapingProbe).toContain('if [ ! -e "/proc/$gpid" ]; then'); + }); +});