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>
This commit is contained in:
Zannis Kalampoukis
GitHub
parent
42c6f8a424
commit
584031af66
|
|
@ -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}"
|
||||
|
|
|
|||
Loading…
Reference in New Issue