From 79bf733012c7dff6f9961194370a7db7f4b8fe90 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Mon, 9 Nov 2026 11:36:44 -0800 Subject: [PATCH] fix(runner): share the bounded ACPX cleanup proof (#20332) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Runner keeps a lifetime fence until the original ACPX provider stops. > - A recovered Runner must prove that fence is free before it clears saved cleanup state. > - Suspension and other recovery paths already wait for transient listener holders. Recovered shutdown used one immediate probe. > - That difference made parallel Rust tests fail after they dropped their original listeners. > - This pull request uses the existing bounded proof for recovered shutdown and removes the repeated test retry. > - The benefit is one cleanup rule, with the same fail-closed proof and no new work on normal task dispatch. ## Linked Issues or Issue Description Fixes: #20210 Thanks to @genius-0963 for the test-only fix proposed in #20321. That related alternative adds a test-only shutdown retry. This change applies the existing bounded cleanup proof in the shared shutdown path and removes the same retry pattern from native-provider integration tests. It also tests a single shutdown call while the original quorum is still held. ## What Changed - Use `await_provider_lifetime_fence` for a recovered executor with no live session and unconfirmed provider exit. Delete the unused one-shot helper. - Keep the acquired quorum held through the existing durable state write. A failed proof leaves `providerExitUnconfirmed` set. - Give the shared timeout message wording that applies to shutdown and suspension. - Extend the recovery test with a worker-start signal. Hold the original listeners until the single shutdown call is waiting, then release them and assert successful cleanup without a provider session. - Use the same bounded proof in the retired-quorum fixture test. Remove the duplicate retry loop from the native-provider test helper. - Document the bounded recovery cleanup wait. ## Verification - macOS arm64, Rust `1.97.1`, source base `3a2f71db31`, head `368fc814f7ccf88ac4bf68ca9eb7f1a6c294307b`, isolated worktree. Only targeted checks ran locally. - `cargo fmt --manifest-path packages/paperclip-runner/runner/Cargo.toml --all -- --check`: passed. - `cargo test --manifest-path packages/paperclip-runner/runner/Cargo.toml --locked -p paperclip-runner-core --lib acpx_provider_backend::tests`: 45 passed on the rebased head (5.21 seconds). The initial head also passed five more parallel-test runs (45 tests per run), with no failure. - `cargo test --manifest-path packages/paperclip-runner/runner/Cargo.toml --locked -p paperclip-runner-core --test native_provider_backend acpx_`: 14 passed on the rebased head (9.43 seconds). These tests use the real Rust executor and controlled sidecars. - Negative control: restore only shutdown's original one-shot proof. The new regression fails at `shutdown must wait while the original lifetime still holds a quorum`. Restore the fix and the affected module passes. - Independent parent verification on current commit `368fc814f7`: the three exact recovery, held-quorum, and retired-quorum tests passed (3 tests, 5.06 seconds). Independent source review confirmed the unchanged three-file scope and found no actionable issue. - The active-turn recovery test still holds a live quorum through the five-second bound. It checks that shutdown refuses cleanup, saved state remains unconfirmed, and the provider never starts. It then releases the quorum and confirms cleanup. - Standard Greptile scored current commit `368fc814f7ccf88ac4bf68ca9eb7f1a6c294307b` 5/5 with no review threads. - Initial GitHub general-server shard 7 failed in unchanged `remote-workspace-nested-verification.test.ts`, at its 15-second wait for a child-process import checkpoint (expected one row, got zero; both Vitest attempts). [Failed job](https://github.com/paperclipai/paperclip/actions/runs/42668667033/job/129339371815). That fixture runs Node, PostgreSQL, Git, and storage migration; it does not run the native Runner or the changed Rust cleanup. The cause of the checkpoint timing miss remains unknown. The focused retry passed on the unchanged source ([successful retry](https://github.com/paperclipai/paperclip/actions/runs/42668667033/job/129347569863)). This proves recovery, not the cause of the original timing miss. GitHub kept the reusable-workflow aggregate failed after that job-only retry and rejected both a final-gate retry and a failed-job retry. A subsequent run-level retry materialized only the gate and aggregate jobs; its aggregate inputs reported prior jobs as `abandoned`, so it failed without a new test failure. A new `full-ci` label event requests a fresh workflow graph on the same commit. The reason for those rerun-state results is not established. - The fresh `full-ci` run [42674033208](https://github.com/paperclipai/paperclip/actions/runs/42674033208) then lost several runners mid-job. The maintained [runner-loss recovery 42677096525](https://github.com/paperclipai/paperclip/actions/runs/42677096525) classified all four failed jobs as runner loss: typecheck, E2E shards 2 and 8, and server shard 4. The evidence was a shutdown log or lost-communication annotation. It retried the failed jobs together; canceled sibling jobs were also requeued. This is runner-interruption evidence, separate from the earlier checkpoint assertion. - The maintained recovery attempt again failed the same partial-checkpoint assertion in unchanged `remote-workspace-nested-verification.test.ts`, twice ([job 129361525237](https://github.com/paperclipai/paperclip/actions/runs/42674033208/job/129361525237)). Neither checkpoint-failure job logged runner shutdown or lost communication; their resource summaries reported no significant constraint. The child phase at the timeout remains unknown. No further blind retry or TypeScript change is included here. Related startup-handshake work exists in #20339; its controlled-delay evidence establishes a deadline-boundary defect but does not establish the phase of these original CI failures. - Rebased onto current master after upstream #20339 merged. That separate change fixes a demonstrated child-startup/checkpoint-deadline boundary. It does not prove which child phase caused the earlier zero-checkpoint assertions. This PR still changes only its original Rust and documentation files. Fresh GitHub CI on the new base will establish recovery, not retrospective cause. - Full local tests and a local release build were not run. Current-head [PR CI 42681719411](https://github.com/paperclipai/paperclip/actions/runs/42681719411), [Runner Windows 42681719803](https://github.com/paperclipai/paperclip/actions/runs/42681719803), and [Docker Runner 42681719601](https://github.com/paperclipai/paperclip/actions/runs/42681719601) all passed on `368fc814f7ccf88ac4bf68ca9eb7f1a6c294307b`. The full check list has 59 successes and 5 intentional skips. GitHub supplied broad typecheck, build, and optimized Rust coverage. This is new-base recovery; the cause of the original checkpoint assertion failures remains unproved. ## Risks - A genuinely unconfirmed original provider can now hold recovered shutdown for up to the existing five-second proof deadline. The proof returns immediately when two candidate ports are free. - The changed branch requires both no in-memory provider session and unconfirmed saved cleanup. Live-session shutdown and ordinary dispatch keep their existing paths. Outer controller close deadlines remain authoritative. - State saved by the previous release carries forward unchanged. No schema, launch identity, profile digest, protocol, or retry-budget change. - No live provider session or hosted deployment was tested. The tests cover cleanup proof, refusal, durable state, and controlled provider execution. ## Model Used - OpenAI GPT-6 through Codex, with reasoning, repository inspection, code editing, and targeted test execution. The exact serving revision and context window are not exposed in this session. ## 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 #` 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 references) - [x] My branch name describes the change 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 Co-authored-by: Paperclip --- .../paperclip-runner/docs/durable-recovery.md | 6 +++ .../runner-core/src/acpx_provider_backend.rs | 38 ++++++++++++------- .../tests/native_provider_backend.rs | 37 +----------------- 3 files changed, 31 insertions(+), 50 deletions(-) diff --git a/packages/paperclip-runner/docs/durable-recovery.md b/packages/paperclip-runner/docs/durable-recovery.md index 63e25852e53..617398eec88 100644 --- a/packages/paperclip-runner/docs/durable-recovery.md +++ b/packages/paperclip-runner/docs/durable-recovery.md @@ -92,6 +92,12 @@ returns as soon as the drain is proved; it adds no fixed delay to a ready Runner ## ACPX checkpoints after a rejected turn start +Recovered ACPX shutdown uses the same bounded five-second lifetime-fence proof +as suspension. It waits for a transient listener holder to release the original +quorum, then returns immediately when cleanup is proved. A timeout leaves +`providerExitUnconfirmed` set and refuses reuse. Live-session shutdown and normal +task dispatch keep their existing paths. + A matching `turn.start` response with `SESSION_RESUME_REQUIRED` and `retryable: true` can retain the exact saved conversation. The session must have no active turn, pending request or buffered event. The saved identity must still diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs index da20602953d..6797bdcb6d0 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs @@ -115,16 +115,6 @@ fn try_acquire_provider_lifetime_fence( Ok(None) } -fn acquire_provider_lifetime_fence( - candidates: [u16; 3], -) -> Result, DurableRunnerError> { - try_acquire_provider_lifetime_fence(candidates)?.ok_or_else(|| { - DurableRunnerError::invalid( - "ACPX original provider lifetime remains active; cleanup is not yet proven", - ) - }) -} - fn turn_start_request(payload: &Value) -> Result<(&str, String), DurableRunnerError> { let text = payload .get("text") @@ -153,7 +143,7 @@ fn await_provider_lifetime_fence( } if Instant::now() >= deadline { return Err(DurableRunnerError::invalid( - "ACPX original provider lifetime remains active after suspension; provider exit is not confirmed", + "ACPX original provider lifetime remains active; provider exit is not confirmed", )); } std::thread::sleep(PROVIDER_LIFETIME_CONFIRMATION_RETRY); @@ -3166,7 +3156,7 @@ impl CommandExecutor for AcpxCommandExecutor { .and_then(|state| state.identity.as_ref()) .expect("provider cleanup state has a validated identity") .provider_lifetime_fence_candidates; - Some(acquire_provider_lifetime_fence(candidates)?) + Some(await_provider_lifetime_fence(candidates)?) } else { None }; @@ -5611,8 +5601,28 @@ mod tests { assert!(attach_error .to_string() .contains("requires the same settled ACPX provider profile and session")); + let (started_tx, started_rx) = std::sync::mpsc::channel(); + let (shutdown_tx, shutdown_rx) = std::sync::mpsc::channel(); + let cleanup = std::thread::spawn(move || { + started_tx.send(()).unwrap(); + let result = executor.shutdown(); + shutdown_tx.send((executor, result)).unwrap(); + }); + started_rx.recv_timeout(Duration::from_secs(1)).unwrap(); + assert!( + matches!( + shutdown_rx.recv_timeout(Duration::from_millis(100)), + Err(std::sync::mpsc::RecvTimeoutError::Timeout) + ), + "shutdown must wait while the original lifetime still holds a quorum" + ); drop(original_lifetime_fence); - executor.shutdown().unwrap(); + let (mut executor, result) = shutdown_rx + .recv_timeout(PROVIDER_LIFETIME_CONFIRMATION_TIMEOUT + Duration::from_secs(1)) + .unwrap(); + cleanup.join().unwrap(); + result.unwrap(); + assert!(executor.session.is_none()); let recovered = executor.state.as_ref().unwrap(); assert_eq!(recovered.lifecycle, "prepared"); assert!(!recovered.provider_exit_unconfirmed); @@ -5640,7 +5650,7 @@ mod tests { "another fixture must not impersonate a retired provider lifetime" ); assert_eq!( - acquire_provider_lifetime_fence(original_candidates) + await_provider_lifetime_fence(original_candidates) .expect("unrelated live fixture must not block the original cleanup proof") .len(), 2 diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs index d6ccb98ee10..a8922c17568 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs @@ -605,42 +605,7 @@ fn assert_no_replayed_turn_or_request(events: &[PolledEvent]) { } fn shutdown_recovered_acpx_fixture(executor: &mut NativeProviderCommandExecutor, directory: &Path) { - // The fake sidecar advertises the same three lifetime-fence ports for all - // fixtures. Parallel recovered shutdowns hold a quorum through state fsync, - // temporarily preventing another fixture from proving cleanup. Retry only - // that rejection, using the real production quorum proof on every attempt. - // This is fixture synchronization, not a production cleanup workaround. - let deadline = std::time::Instant::now() + Duration::from_secs(5); - let mut blocked_attempts = 0; - loop { - match executor.shutdown() { - Ok(()) => break, - Err(error) => { - assert_eq!( - error.to_string(), - "ACPX original provider lifetime remains active; cleanup is not yet proven", - "unexpected recovered fixture shutdown failure" - ); - let persisted: Value = serde_json::from_slice( - &fs::read(directory.join("acpx-provider-state.json")).unwrap(), - ) - .unwrap(); - assert_eq!(persisted["providerExitUnconfirmed"], true); - blocked_attempts += 1; - if blocked_attempts == 1 { - eprintln!("recovered fixture cleanup waiting for the shared lifetime quorum"); - } - assert!( - std::time::Instant::now() < deadline, - "recovered fixture cleanup never acquired the lifetime quorum" - ); - std::thread::sleep(Duration::from_millis(10)); - } - } - } - if blocked_attempts > 0 { - eprintln!("recovered fixture cleanup proved after {blocked_attempts} quorum rejections"); - } + executor.shutdown().unwrap(); let persisted: Value = serde_json::from_slice(&fs::read(directory.join("acpx-provider-state.json")).unwrap()) .unwrap();