mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 15:55:46 +01:00
Nicky LeachandClaude Opus 5.5
a219dd7499
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Paperclip Runner stops a dead runnerd's provider and tool processes from its process lineage journal (`process_lineage::journal::reap_journals`) > - The reap refuses its proof (`unattributed_orphan`) while any live orphan (parent PID 1) of this user started after the dead runnerd's last sample, whoever started it. This rule is deliberate > - `cargo test` runs the runner-core lib tests in parallel in one process, and several of those tests leave orphans of this user on purpose > - When one of those orphans starts during the reap in `reaping_stops_a_dead_runners_provider_and_its_escaped_tool_job`, the reap correctly refuses, and `ci / Verify Paperclip Runner (rust)` fails > - This pull request keeps the orphan-leaving tests apart from that reap with a test-only gate, and fixes a related zombie race in the supervisor integration tests > - The benefit is that the Runner Rust job stops failing on unrelated pull requests ## Linked Issues or Issue Description Refs #20834. Refs #20770. **What happened?** `ci / Verify Paperclip Runner (rust)` failed intermittently in `process_lineage::journal::tests::reaping_stops_a_dead_runners_provider_and_its_escaped_tool_job` (journal.rs:649), for example in run 43044317310 on #20834. The panic was `ReapReport { verified: false, reason: Some("unattributed_orphan"), journals: 1, signalled: 4, remaining: [4178] }`. The test's own thread was TID 4142, so the leftover process started while the test ran. The same test failed once on #20770 and passed on re-run. **Expected behavior** The reap proves the test's provider and escaped tool job gone, and no other test can make it refuse. **Steps to reproduce** 1. Build the runner-core lib tests for Linux (`cargo test --release -p paperclip-runner-core --lib --no-run`). 2. Run the binary 40 times in `docker run --init --cpus 2 -e RUST_TEST_THREADS=8`. 3. See `reaping_stops_a_dead_runners_provider_and_its_escaped_tool_job` fail with `unattributed_orphan` (3 of 40 runs here). ## What Changed - Root cause: the reap's orphan rule covers every process of this user on the host. The reaping test and the orphan-leaving tests run in the same test process at the same time. The test's own processes are always attributed through the recorded group and session, so the leftover was another test's orphan. - `process_lineage::host_orphans` (test-only): a gate. A test that can leave an orphan of this user holds `may_orphan()` until the orphan is gone or a zombie. A test whose reap must verify holds `no_foreign_orphans()`. Holders of `may_orphan()` never wait for each other, so a test can hold several without a deadlock. `wait_until_group_gone()` waits until a killed group has exited, because a signal takes effect after `kill` returns. - `reaping_stops_a_dead_runners_provider_and_its_escaped_tool_job` holds `no_foreign_orphans()`. - These lib tests now hold `may_orphan()`: - `reaping_refuses_an_orphan_born_after_the_last_sample` (deliberate orphan) - the three `process_supervisor` `*_stops_a_tool_job_that_left_its_session` tests (escaped tool job) - `orphaned_startup::restart_keeps_the_fence_while_a_child_survives_its_exited_leader` (`sleep 30` outlives its leader) - `exited_child_reader_deadline_bounds_continuous_descendant_stdout` (flooding subshell outlives the Codex provider) - `a_timed_out_turn_start_never_establishes_a_resumable_rejection` (`sleep 10` can be orphaned for a moment during shutdown) - the five `provider_backend` tests that use `disconnected_receipt_limit_fixture` (`while :; do sleep 1; done`, killed through `fence_local_receipt_limit`). The fixture returns its guard first, so the guard is released after the executor stops the provider - the two `late_park_observation_*` tests (`cat >/dev/null` runs until shutdown) - `a_held_cancellation_that_cannot_reach_the_provider_keeps_the_completed_turn` (shutdown during `sleep 2`) - New test `a_verifying_reap_waits_for_held_orphans_and_holds_new_ones_back` checks the gate's ordering. - `tests/process_supervisor.rs`: `forced_process_group_cleanup_stops_harness_and_worker` and `natural_harness_exit_also_cleans_up_workers` slept 20 ms and then used `kill -0` on a worker that was re-parented to init. `kill -0` succeeds for a zombie, and init reaps it on its own schedule. The tests now check that the process is not running (gone or a zombie), with no sleep. Cleanup already proves that before it returns. - No product code changes. The reap's orphan rule is unchanged. Audit of the other tests in the job (`check:runner`): - Only the runner-core lib binary can overlap the reap. `cargo test --workspace` runs test binaries one after another. - A second sweep, after review, checked every `/bin/sh` launch and every external command (`sleep`, `cat`, `perl`, `nohup`, `touch`, `mv`) in runner-core's `src/`, including `src/tests/*.rs`. The first sweep had missed tests that start a provider through `CodexProvider::start` or that live in `src/tests/`. - Other lib tests that start processes are not affected. They run shells that use only builtins (`configured_environment`, the other `process_supervisor` and `codex_provider` tests, `acpx_provider_session`), or run a short foreground `touch` that finishes long before teardown (`disconnected_codex_output_is_durable...`), or they start direct children that the test waits for (`oom_score`, `exited_writer`, `process_group_leader`). `publisher_crosses_real_codex_tool_boundary...` is `#[ignore]`. - The other `reap_journals` tests refuse before the orphan rule applies, or (the orphan test) require only their own orphan in `remaining`. - The `kill -0` checks in `tests/codex_provider.rs` and `tests/acpx_park_preparation.rs` target direct children that runner-core waits for before it returns, so they are deterministic. ## Verification Linux runs used Colima on macOS (linux/arm64), `rust:1.97.1-bookworm`, `--release`, and `docker run --init`, so orphans go to an init at PID 1, as on CI. - Full runner-core lib suite, `--cpus 2`, `RUST_TEST_THREADS=8`, 40 runs: before the fix, 3 runs failed with the CI signature. After the fix, 0 runs failed, and no other test failed. - The reaping test beside a temporary test that keeps leaving short-lived orphans (not committed), `--test-threads 2`, 10 runs: before the fix, 10 failed. After the fix, with the temporary test holding `may_orphan()`, 0 failed. - An orphan-leaving shell outside the test binary made the unfixed test fail in 9 of 10 runs with the same signature. This confirms the mechanism. - After the review fix: macOS `cargo test --release -p paperclip-runner-core --lib -- receipt_limit late_park_observation a_held_cancellation process_lineage process_supervisor orphaned_startup exited_child_reader_deadline a_timed_out_turn_start`: 65 passed. Linux full lib suite (same settings) ×20: 0 failures. - macOS (darwin-arm64): `cargo fmt --all -- --check` passes. `cargo test --release -p paperclip-runner-core --lib -- process_lineage process_supervisor orphaned_startup exited_child_reader_deadline a_timed_out_turn_start`: 51 passed. `cargo test --release -p paperclip-runner-core --test process_supervisor`: 7 passed. - Worker cleanup tests: with `tini` as init, 60 unfixed runs on 0.5 CPU with busy loops all passed, because `tini` reaps at once. In a container whose PID 1 never reaps (`sleep infinity`), the unfixed tests fail at `assertion failed: !process_exists(worker_pid)` because the killed worker stays a zombie. The fixed tests pass. The unfixed tests depend on how fast init reaps. ## Risks - Low risk. All changes are in tests or `#[cfg(test)]` code. - The gate covers the orphan producers that exist now. A new lib test that leaves an orphan of this user must hold `may_orphan()`. The `host_orphans` doc comment says so. Otherwise, it can make the verifying reap refuse again. - A process outside the test binary that leaves an orphan of this user during the reap still makes the test refuse. This is correct product behavior. CI runs no such process during `cargo test`. - `no_foreign_orphans()` waits until every `may_orphan()` guard is released, so the reaping test can wait a few seconds behind those tests. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, in Claude Code with tool use (shell, file editing, Docker). Extended reasoning was used. ## 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 - [x] All Paperclip 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>