mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
0f71ab796e
commit
79bf733012
3 files changed
+31
-50
No files matched your search
@@ -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
|
||||
|
||||
@@ -115,16 +115,6 @@ fn try_acquire_provider_lifetime_fence(
|
||||
Ok(None)
|
||||
}
|
||||
|
||||
fn acquire_provider_lifetime_fence(
|
||||
candidates: [u16; 3],
|
||||
) -> Result<Vec<TcpListener>, 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
|
||||
|
||||
+1
-36
@@ -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();
|
||||
|
||||
Reference in new issue
Block a user