fix: reap orphaned processes instead of leaking zombies (#28)

[shipyard-ci]

Closes #28

Problem

Spacebot runs as PID 1 in the container, which makes it the namespace's init — it inherits every orphaned process, not just the ones it spawned. Shell commands routinely leave grandchildren behind (sh -c "cargo build" exits while its cargo/node/build-script descendants outlive it), and nothing was collecting them. 101 zombies in ~29 h.

I reproduced this live on the running instance while investigating — spawning 20 orphan-producing shells left 20 zombies parked on PID 1, exactly the reported shape:

  12190       1 Z    sleep
  12192       1 Z    sleep
  ...  (20 total, all PPID 1)

Each zombie holds a PID until reaped, so the count only grows for the life of the container. At the observed rate an uncapped container reaches the PID ceiling in days — after which nothing can spawn and worker launches fail for a reason that looks nothing like the cause.

Fix

Both options from the issue, because they cover different windows:

1. tini as PID 1 (ENTRYPOINT, issue option 2) — a real init that reaps anything re-parented to it, including processes that outlive spacebot's own startup and shutdown. -g forwards signals to the process group so shutdown stays clean.

2. src/process/reaper.rs (issue option 1) — reaps in-process when spacebot is PID 1, covering deployments that bypass the entrypoint (bare docker run without --init).

Why not waitpid(-1, ...)

The obvious implementation races Tokio's process driver. Both listen for SIGCHLD, and a blanket wait reaps whichever child exited first — the caller only learns which after the status is already consumed. If it was Tokio-owned, that status is gone and the awaiting task fails with ECHILD.

My first draft had exactly this bug, and the test I wrote for it caught it (No child processes on a child that should have been protected). The reaper now enumerates children from /proc/self/task/*/children and waits per-PID, skipping any a spawn site has claimed. Ownership is checked before the status is consumed, which makes the guarantee real rather than best-effort.

The reverse leak

run_streaming drops its Child when the 5 s wait times out — Tokio never reaps it and the SIGCHLD has already come and gone. Releasing a claim now reaps that PID directly, so it is collected immediately instead of parked until some unrelated process happens to exit.

Observability

GET /api/status now reports the acceptance metric directly, so it is checkable without docker exec:

curl -s localhost:19898/api/status | jq '{zombie_processes, reaped_orphans}'

zombie_processes is Option — absent rather than a misleading 0 where /proc is unavailable.

Verification

Build note: cargo check on the full crate fails on ethnum 1.5.2 (lockfile-pinned, transitive via lancedb→datafusion) under rustc 1.97 — error[E0512]: cannot transmute between types of different sizes. I confirmed this is pre-existing and unrelated: a bare crate with only ethnum@=1.5.2 reproduces it, and 1.5.3 compiles fine. No error references any file in this PR.

So I verified the module standalone against the same rustc:

Both properties, together:

  claimed exit 0: preserved
  claimed exit 7: preserved
  claimed exit 42: preserved
PASS 1/2: claimed children keep exit statuses under 50 concurrent sweeps

  zombies before reap: 25
  zombies after reap:  0
PASS 2/2: 25 orphaned zombies -> 0

Unit tests: 10/10 passing, deterministic across 3 consecutive runs. Clippy: clean under -D warnings. rustfmt: clean.

tini (extracted from the actual bookworm package, verified at /usr/bin/tini) in subreaper mode: 0 zombies, against 35 accumulating on the real PID 1 at the same moment.

Scope

Touches 8 files. Three unrelated modified files in the shared checkout (src/llm/pricing.rs, src/llm/anthropic/params.rs, src/agent/worker.rs) belong to a concurrent worker and were deliberately not staged, per spacedriveapp#224.

[shipyard-ci]

Code Review — PR #29: reap orphaned processes instead of leaking zombies

1. Summary

This PR adds a process reaper to prevent zombie accumulation from orphaned child processes. It introduces src/process/reaper.rs (+394) with a spawn() background reaper, a claim() guard used by the shell tool to track owned PIDs, and zombie_count()/reaped_count() observability hooks surfaced via src/api/system.rs. The Dockerfile additionally installs tini as PID 1 (with -g) so container-level signal handling and init duties are correct, with matching notes in docs/docker.md.

2. Findings

P1 (Blocking)

None.

P2 (Should Fix)

None.

P3 (Nit)

docs/docker.md — stale PID 1 statement
The added docs still say Spacebot runs as PID 1 in the container, which is no longer accurate now that the Dockerfile makes tini the entrypoint. Suggest rewording to "Spacebot runs as a direct child of tini (PID 1)".

src/process/reaper.rs — incomplete SAFETY comment on waitpid
The // SAFETY: comment for the libc::waitpid call justifies WNOHANG (non-blocking, so the async runtime isn't stalled) but doesn't state why passing &mut status is sound. A one-line addition ("waitpid writes only through &mut status, a valid stack local") makes the invariant explicit for future maintainers.

src/tools/shell.rs — _owned binding
let _owned = child.id().map(...) correctly binds the claim guard so it isn't dropped immediately, and the preceding comment explains the intent. Underscore-prefixed binding is idiomatic here; no change required, noted only for consistency if a more descriptive name is preferred.

(The earlier per-part notes about reaper.rs being "missing" and a truncated test function were artifacts of diff chunking, not defects in the code — no action needed.)

3. Security

No security issues identified.

• The unsafe FFI in the reaper is narrowly scoped to libc::waitpid with WNOHANG, which is the intended, non-blocking use for reaping.
• tini -g in the Dockerfile forwards signals to the whole process group; this is the correct choice for clean shutdown of spawned shell children and does not broaden the container's privileges.
• No secrets, auth changes, or user-controlled input paths are introduced by this diff. zombie_count()/reaped_count() expose only aggregate counters.

4. Verdict

APPROVE — the change is well-scoped and correct; remaining items are documentation/comment nits that can be folded into a follow-up or fixed before merge.

---

AI Review · Verdict: APPROVE · Diff-Score: 0.90
Routed as code_review (100%) → github_code_review_flow · View AI traces