3 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
3 people	0cc40037ac	fix(runner): accept the indeterminate command result after a runner restart (#12646) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The runner subsystem pairs a Rust runner process with a durable control plane in TypeScript. The control plane is the authority for every command the runner executes > - That pair has a crash-recovery contract. If the runner dies between journaling a command and confirming the command's effect, it must not run the command twice > - The runner keeps its side of the contract. On restart it promotes such a command to the `indeterminate` status and reports that status back > - The control plane did not accept `indeterminate`. It closed the connection without a diagnostic, the runner reconnected and replayed the same result, and the loop repeated forever > - This pull request accepts `indeterminate` as a terminal command status > - The benefit is that a session survives a runner crash during a tool call, instead of hanging until a 30 second deadline expires ## Linked Issues or Issue Description No public issue exists for this defect, so it is described here. **What happened?** A live session cannot resume after the runner process is killed during a governed tool call. The resumed transport waits for the provider identity for 30 seconds and then fails with `runnerd did not report its provider identity`. `packages/paperclip-runner/src/live/live-session.test.ts` covers this exact sequence in "terminates real runnerd after a durable receipt and resumes its exact provider thread". That test has a 15 second budget, so it reports the defect as `Test timed out in 15000ms` and reads like a flake. **Expected behavior** The resumed control plane accepts the runner's recovery report, the runner reports its provider identity, and the session resumes on its original provider thread. **Steps to reproduce** Build the runner binary, then run the test: ``` cargo build --manifest-path packages/paperclip-runner/runner/Cargo.toml --locked --workspace --bins cd packages/paperclip-runner npx vitest run src/live/live-session.test.ts -t "terminates real runnerd" ``` It fails every time on an idle machine. It also fails at `560e7e48b`, the commit that added the test, so the defect is not a recent regression. **Paperclip version or commit** Reproduced on `master` at `0a422fda5`, which is the base of this branch. **Deployment mode** Local development, running the package test suite. **Root cause** `DurablePrpControlPlane.#commandResult` accepted only `completed`, `failed` and `rejected`. The runner reports a journaled-but-unconfirmed command as: ```json { "status": "indeterminate", "result": { "code": "execution_indeterminate", "message": "runner recovered after journaling this command; it will not execute twice" } } ``` That status fell through to a silent `connection.close()`. The runner reconnected after 250 ms, replayed the same result, and was closed again. No durable event ever reached the control plane, so the transport never saw `harness.ready`. `indeterminate` is a deliberate part of the runner's contract. See `reconcile_pending_commands` in `packages/paperclip-runner/runner/crates/runner-core/src/durable/state.rs`. The rest of the TypeScript code already models the status; only this control plane did not. ## What Changed - `DurablePrpControlPlane.#commandResult` accepts `indeterminate` as a terminal command status. - The persisted-state validation accepts `indeterminate`, so a control plane restarted over the same directory can read its own saved state back. Without this, accepting the status would make the next restart throw. - `DurableRecoveryCoreCommand.status` includes `indeterminate` in both declarations of that interface. - Added an integration test that drives the exact recovery frame the runner sends. It asserts the connection stays open, the next command is delivered, the status is persisted, a restarted control plane reloads it, and a replayed duplicate is absorbed rather than treated as a conflict. ## Verification All commands run from `packages/paperclip-runner`. - New test fails before the change and passes after it. Before: `expected null to match object { kind: 'command' }` — `null` is the closed connection. `npx vitest run src/control-plane/durable-prp-control-plane.test.ts` → 4 passed. - The live runner test that exposed this reproduced **deterministically** on an idle machine before the change, and now passes in 3.3 s, well inside its existing 15 s budget. Ran it 10 times in a row: 10/10 pass, 0 failures. `npx vitest run src/live/live-session.test.ts -t "terminates real runnerd"` - Full package suite: `npx vitest run` → 1298 passed, 1 failed. The one failure is `src/mock-core/local-runner.test.ts > cleans up the harness process group when the controller closes`. It fails identically on an unmodified checkout in the same container, so it is a pre-existing environment issue and not related to this change. - Typecheck: `tsc -p tsconfig.json --noEmit` → clean. I did **not** raise the test's timeout. The budget was never the problem — with a 600 s budget the same test still failed, at 31 s, with the real error. ## Risks Low risk, and it widens rather than narrows what is accepted. - Behaviour only changes for a status that is currently rejected, so no previously working path is affected. - `indeterminate` is terminal, not successful. A caller waiting on such a command still receives an error from the transport, which is correct: the effect is genuinely unconfirmed. This change does not make an unconfirmed command look like it succeeded. - The persisted-state change only widens an allow-list, so existing state files stay valid. Open topics for a reviewer: - The control plane closes connections without any diagnostic. That silence is why this defect looked like a flaky test. Adding a diagnostic channel is a larger change and is not included here. - `DurableRecoveryProcessedCommand` in `src/contracts/durable-recovery.ts` drifts from the Rust `StoredCommandResult` by more than this status: it declares `commandDigest` and `logicalEffectCount`, which Rust does not have, and omits `commandType`, which Rust does. That is a separate correction and is deliberately not folded in here. ## Model Used Claude Opus 5 (`claude-opus-5`), extended thinking, with tool use and code execution. Depends-on: none — this is a self-contained fix with no dependent changes. ## 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 Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip 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 - [ ] All Paperclip CI gates are green - [ ] 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> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 10:21:38 -07:00
Zannis Kalampoukisandzannis	584031af66	test(runner): bound codex provider exit polls by wall clock (#12596) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The runner subsystem drives a Codex provider process and reads its events with `CodexProvider::poll` > - The Codex provider integration tests wait for those events in poll loops > - Two of those loops count iterations instead of measuring time, so they stop waiting too early > - This makes `cargo test` fail at random on branches that change no Rust code > - This pull request bounds the two loops by wall clock, like every other wait in the same file > - The benefit is that a red CI job now means a real defect ## Linked Issues or Issue Description **What happened?** `packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs` fails `cargo test` at random. The failure appears in the `ci / Build` job with exit code 101. It appears on branches that change no Rust code. Two tests fail: - `ambiguous_or_dead_replacement_start_preserves_result_not_exit_authority` at line 1274 - `ambiguous_replacement_turn_adopts_one_later_completion_identity` at line 1443 Both assertions report `left: None`. The value is not wrong. The loop never saw the `CodexProviderEvent::Exited` event at all. **Expected behavior** The tests must wait for the provider process to exit. A test must fail only when the provider gives a wrong result. **Steps to reproduce** 1. Build the integration test: `cargo test --test codex_provider --no-run`. 2. Run one of the two named tests 25 times in a row. 3. About 8 of the 25 runs fail with `left: None`. **Paperclip version or commit** Reproduced on `master` at `2e5a24e17`. **Related pull requests** Refs #12241. That pull request also edits `packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs`. It does not fix these two loops. The two changes may need a merge if both land. **Root cause** `CodexProvider::poll` (`crates/runner-core/src/codex_provider.rs:824`) reads with a 1 ms timeout. That timeout does not apply on every path. `ProcessSupervisor::receive_stdout_line` (`crates/runner-core/src/process_supervisor.rs:293`) returns at once, and uses none of the 1 ms budget, in two cases: `StdoutClosed` at line 309 and `RecvTimeoutError::Disconnected` at line 315. A child process closes its pipes before its exit status is ready to reap. In that window every `poll()` call returns `Ok(None)` in nanoseconds. A loop of 64 or 128 iterations then ends in microseconds, before the exit status is available. The failing run above ends in 0.06 s. ## What Changed - `tests/codex_provider.rs`: bound the exit wait at line 1256 by a 5 second deadline instead of 64 iterations. - `tests/codex_provider.rs`: bound the exit wait at line 1397 by a 5 second deadline instead of 128 iterations. - Both loops now sleep 1 ms when `poll()` returns no event. This copies the pattern that the same file already uses at line 1511 and in every `wait_for_*` helper. - No production code changes. The change is test-only. ## Verification Measured before and after the change. Each test ran 25 times in sequence, on an idle machine, with `--test-threads=1`. | test | before | after | |---|---|---| | `ambiguous_or_dead_replacement_start_preserves_result_not_exit_authority` | 8 / 25 failed | 0 / 25 failed | | `ambiguous_replacement_turn_adopts_one_later_completion_identity` | 9 / 25 failed | 0 / 25 failed | The full `codex_provider` suite also ran 12 times with `--test-threads=4` after the change. Every run passed. Commands: ``` cargo test --test codex_provider --no-run cargo test --test codex_provider ``` ## Risks Low risk. The change touches test code only. It makes two waits longer in the failure case: a genuinely broken provider now takes up to 5 seconds to fail these two tests instead of microseconds. Every other wait in this file already uses the same 5 second deadline. ## Model Used Claude Opus 5 (`claude-opus-5`), extended thinking, with tool use and code execution. Depends-on: none — this is a self-contained test-only change with no prerequisite pull request. ## 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 Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [ ] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip 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 — none apply. This change is test-only and alters no public interface, so no docs page and no end-to-end test change is needed. - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: zannis <1011451+zannis@users.noreply.github.com>	2026-09-01 10:11:06 -07:00
Zannis Kalampoukisandzannis	5db8ce3c44	fix(docker): make tini PID 1 in the server image so adopted orphans are reaped (#12137) ... ## 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>	2026-08-25 09:52:39 -07:00