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'); + }); +});