From 584031af6616ea048b5f3671e28bf30897e25b9d Mon Sep 17 00:00:00 2001 From: Zannis Kalampoukis Date: Tue, 1 Sep 2026 20:11:06 +0300 Subject: [PATCH] test(runner): bound codex provider exit polls by wall clock (#12596) 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 > - 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> --- .../runner-core/tests/codex_provider.rs | 122 ++++++++++-------- 1 file changed, 68 insertions(+), 54 deletions(-) diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs index 3d9fcab756..0010ae6051 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs @@ -1286,24 +1286,31 @@ fn ambiguous_or_dead_replacement_start_preserves_result_not_exit_authority() { provider .start_turn("Accept replacement work before failing.", &config.cwd) .expect_err("the accepted replacement turn has no valid response"); - let ambiguous_start_exit = (0..64).find_map(|_| { - match provider - .poll() - .expect("poll exit after ambiguous replacement start") - { - Some(CodexProviderEvent::Exited { - success, - completed_turn_authoritative, - completion_reconciles_exit, - .. - }) => Some(( - success, - completed_turn_authoritative, - completion_reconciles_exit, - )), - _ => None, - } - }); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + let ambiguous_start_exit = (0..) + .take_while(|_| std::time::Instant::now() < deadline) + .find_map(|_| { + match provider + .poll() + .expect("poll exit after ambiguous replacement start") + { + Some(CodexProviderEvent::Exited { + success, + completed_turn_authoritative, + completion_reconciles_exit, + .. + }) => Some(( + success, + completed_turn_authoritative, + completion_reconciles_exit, + )), + Some(_) => None, + None => { + std::thread::sleep(std::time::Duration::from_millis(1)); + None + } + } + }); assert_eq!( ambiguous_start_exit, Some((false, true, false)), @@ -1427,44 +1434,51 @@ fn ambiguous_replacement_turn_adopts_one_later_completion_identity() { let mut replacement_started = false; let mut replacement_completed = false; - let replacement_exit = (0..128).find_map(|_| { - match provider - .poll() - .expect("poll evidence for accepted replacement turn") - { - Some(CodexProviderEvent::Notification { method, params }) - if method == "turn/started" => + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + let replacement_exit = (0..) + .take_while(|_| std::time::Instant::now() < deadline) + .find_map(|_| { + match provider + .poll() + .expect("poll evidence for accepted replacement turn") { - assert_eq!( - params.pointer("/turn/id").and_then(Value::as_str), - Some("provider-turn-2") - ); - replacement_started = true; - None + Some(CodexProviderEvent::Notification { method, params }) + if method == "turn/started" => + { + assert_eq!( + params.pointer("/turn/id").and_then(Value::as_str), + Some("provider-turn-2") + ); + replacement_started = true; + None + } + Some(CodexProviderEvent::Notification { method, params }) + if method == "turn/completed" => + { + assert_eq!( + params.pointer("/turn/id").and_then(Value::as_str), + Some("provider-turn-2") + ); + replacement_completed = true; + None + } + Some(CodexProviderEvent::Exited { + success, + completed_turn_authoritative, + completion_reconciles_exit, + .. + }) => Some(( + success, + completed_turn_authoritative, + completion_reconciles_exit, + )), + Some(_) => None, + None => { + std::thread::sleep(std::time::Duration::from_millis(1)); + None + } } - Some(CodexProviderEvent::Notification { method, params }) - if method == "turn/completed" => - { - assert_eq!( - params.pointer("/turn/id").and_then(Value::as_str), - Some("provider-turn-2") - ); - replacement_completed = true; - None - } - Some(CodexProviderEvent::Exited { - success, - completed_turn_authoritative, - completion_reconciles_exit, - .. - }) => Some(( - success, - completed_turn_authoritative, - completion_reconciles_exit, - )), - _ => None, - } - }); + }); assert!( replacement_started, "the replacement identity should be established before replaying its output for {label}"