mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-14 00:26:28 +01:00
fix(native): make Retry work after a turn start cut off once Codex recorded the turn (#20516)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A native Codex run sends `turn/start` through its runner. A control-plane restart can kill the runner and Codex while that request is in flight > - #20507 lets the runner resume by itself when Codex's thread history proves the turn was never created. When Codex did record the turn, the turn may have done work, so the runner still fails closed and nothing replays it > - That branch must still end in a Retry that works. The next run on the same conversation is refused at the runner-state fence before it asks for a runner. #20507 let Retry continue after that run by matching the refusal's error text > - Driving that branch end to end with the real runner exposed a second dead end. When the owner's runner dies, its provider-session close cannot prove suspension, so this server process can keep an operator-required cleanup quarantine for the task. Retry was then refused outright ("Send a new message") > - This pull request replaces the text match with a server-authored run-log event, lets Retry take the same verified-stop path as a new message, and adds a real-runner test of the whole branch: the cut-off owner fails closed, the follow-up is refused with structured proof, Retry is admitted, and the fresh Retry run completes > - The benefit is that this restart window cannot strand a task, and the Retry gate no longer depends on error wording ## Linked Issues or Issue Description **What happened?** Follow-up to #20507 (restart-chaos wave `w-20261110T033426-s108`, t03). #20507 left one item open: prove that a person's Retry produces a working run when the restart cut off `turn.start` after Codex recorded the turn. It also matched the fence refusal by its error text. **Expected behavior** After that cut-off, the owner stays failed closed and the turn is never started again by itself. A person's Retry starts a new run that completes. It never fails with `quarantine_owner_unverified` and never returns 202 with no run. **Steps to reproduce** 1. Run `pnpm --filter @paperclipai/server exec vitest run src/services/native-runtime/native-cut-turn-start-retry.integration.test.ts`. 2. Without the new event, the test fails: the refused follow-up has no proof that it never launched, so Retry after it would wait on `process_identity_missing`. ## What Changed - `native-session-executor.ts`: when the runner-state fence refuses a run before launch (`prior_owner_*` or `legacy_prior_owner_*`), the executor records the server-authored event `native.prior_owner_fence_refused` with the closed refusal `reason`, then fails the run as before. A failed write leaves the run held (the safe side). - `native-local-process-stop.ts`: `nativeRunNeverLaunched` accepts a failed run that has this event (and still no launch, identity, `adapter.invoke` or runner-sourced event). It no longer reads the error text. - `explicit-native-continuation.ts`: when the task's in-process cleanup quarantine is operator-required, a Retry no longer returns `cleanup_quarantined` at once. Like a new message, it must first pass `verifyStoppedNativeSessionForContinuation`, which releases the quarantine and records `native.stopped_conversation_verified`. A native Retry always starts a fresh conversation (`forceFreshSession`), so it never resumes the interrupted turn. If the proof fails, Retry still returns `cleanup_quarantined`. - `verifyStoppedNativeSessionForContinuation` (`native-session-executor.ts`), three narrow additions: - a run with `native.prior_owner_fence_refused` may stand in for the saved state's owner, like a run refused by the cleanup quarantine (both were refused before launch); - when the session root is gone, it reads the fence's single `identity_indeterminate` quarantined copy of the owner's state (several copies stay ambiguous); - a provider startup attempt in phase `initialization_failed` whose direct child exited and whose process and process group are gone no longer blocks the proof. A same-run restore that fails closed leaves exactly this record. Those PIDs are also re-checked before the quarantine is released. - New real-runnerd integration test `native-cut-turn-start-retry.integration.test.ts` (real runnerd and fake Codex with `--hang-turn-start after`): 1. The owner run's `turn/start` hangs after Codex recorded the turn; the runner and Codex are killed. The owner's provider state keeps the ambiguous start, and the thread history keeps the turn. 2. The same-run resume restores the runner and fails closed with "cannot safely recover an ambiguous Codex turn start". Codex saw exactly one `turn/start`. 3. A follow-up on the same conversation is refused with `prior_owner_*`, never `quarantine_owner_unverified`. It records `native.prior_owner_fence_refused`, launches nothing, and `nativeRunNeverLaunched` is true for it. 4. The owner's operator-required in-process cleanup quarantine is pinned (whether the real close lands there is timing-dependent; about half of the local runs did). 5. Retry of the refused follow-up passes `admitExplicitNativeContinuation` (not a dry run) with no block reason, releases the quarantine and records `native.stopped_conversation_verified`. 6. The Retry run, prepared with a fresh conversation as task Retry does (`forceFreshSession`), uses a different runner-state root and completes with exit code 0. Codex saw one new `thread/start` and one new `turn/start`; the cut-off turn was never started again. - `explicit-native-continuation.test.ts`: the fence rows now insert the event. A new row shows that the refusal text alone keeps the run held. The "worker loss (retry)" case now expects a verified Retry to be admitted like a new message. - `native-recovered-owner-followup.test.ts`: a fixture event moves to a free sequence number, because the refused follow-up now writes its own event. - Docs: `doc/run-log-events.md` (new event) and `packages/paperclip-runner/docs/durable-recovery.md`. ## Verification - Red/green: - Without the event write, the integration test fails (no fence event, so the follow-up is not proven unlaunched). - Without the gate and proof changes, it fails with `expected [ 'cleanup_quarantined' ] to deeply equal []`: Retry is refused while the owner's quarantine is held. - With all changes, it passes. - `vitest run` of `native-recovered-owner-followup`, `explicit-native-continuation` (427 tests), `quarantined-task-retry`, `issue-confirm-run-stopped-routes`, `native-finalization-recovery`, `task-interruption-query`, `execution-projection`, `chat-run-publications`, `closed-native-receipt-accounting`, `agent-live-run-routes` and the new integration test pass. - Server `tsc --noEmit` passes. - Runner code and the pack `contractDigest` do not change. ## Risks - Low-medium risk. The new event is written only on the fence refusal path, which already fails the run. It is local run-log data (no Telemetry or OpenTelemetry). - Retry after an operator-required cleanup quarantine is a behaviour change: it was always refused, and now it is admitted only after the same exact stop proof a new message needs. That proof checks the owner's runner stop record, every recorded provider and startup PID and process group, and the unchanged state fingerprint before it releases the quarantine. - A run refused by a build between #20507 and this change has only the error text. It stays held on `process_identity_missing` until a person sends a new message or the board confirms the stop. #20507 has not shipped in a release yet. - The test models the restart in-process: it kills the runner and Codex and calls the executor again. It does not drive the HTTP Retry route; the route's own behaviour (fresh conversation, admission gate) is covered by existing tests and by `nativeRunNeverLaunched` here. - Open decision, unchanged: a turn Codex recorded before the restart is never replayed or adopted. Continuing is a person's decision. > 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`), via Claude Code with tool use (shell, file edits, test execution). ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Refs #20507 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
1 parent
93faca77ef
commit
bf45700ca3
10 files changed
+374
-29
No files matched your search
@@ -115,7 +115,7 @@ import { stoppedCodexTurnIsTextOnly } from "./stopped-codex-turn.js";
|
||||
import { prepareVerifiedRemoteProviderPack, useVerifiedPreinstalledRemoteProviderPack, shouldUseBundledAcpxImageAssets } from "./remote-provider-pack.js";
|
||||
import { uploadVerifiedProviderPack } from "./provider-pack-upload.js";
|
||||
import { selectRemotePiCompanion } from "./remote-pi-companion.js";
|
||||
import { readNativeLocalProcessStop, recordedLocalExecutionStopped, PROCESS_IDENTITY_RECORDED, PROCESS_START_REQUESTED, withReleasedNativeRunProcess } from "../native-local-process-stop.js";
|
||||
import { readNativeLocalProcessStop, recordedLocalExecutionStopped, PRIOR_OWNER_FENCE_REFUSED, PROCESS_IDENTITY_RECORDED, PROCESS_START_REQUESTED, withReleasedNativeRunProcess } from "../native-local-process-stop.js";
|
||||
import { remoteLeaseCleanupScope } from "../remote-execution-termination.js";
|
||||
import { connectorToolDefinitionsForSkills } from "../connector-runtime.js";
|
||||
import { NATIVE_PREPARATION_FENCING_VERSION, setNativeExecutionActive } from "./native-active-runs.js";
|
||||
@@ -3143,6 +3143,28 @@ export function rebaseRetainedNativeCleanupProviderHome(
|
||||
* Retain the old durable root permanently; only the exact in-memory cleanup
|
||||
* owner is retired, and the successor must use a fresh normalized session.
|
||||
*/
|
||||
/** A provider launch that failed its own admission (for example a restore
|
||||
* that refused an ambiguous turn start), whose direct child exited and whose
|
||||
* process and process group are gone, owns nothing. Any other recorded
|
||||
* launch may own a process that never reported its session identity. */
|
||||
function failedStartupRetired(attempt: unknown): boolean {
|
||||
if (attempt == null) return true;
|
||||
const startup = record(attempt);
|
||||
return startup.phase === "initialization_failed" && startup.directChildExitObserved === true &&
|
||||
cleanupProcessAbsent(startup.processId) &&
|
||||
(startup.processGroupId == null || cleanupProcessAbsent(startup.processGroupId));
|
||||
}
|
||||
|
||||
/** The session's state root, or, when only the fence's quarantine holds it,
|
||||
* that one `identity_indeterminate` copy. Several copies are ambiguous. */
|
||||
function quarantinedOwnerStateRoot(scoped: string): string {
|
||||
if (existsSync(scoped)) return scoped;
|
||||
const quarantine = resolve(runnerdStateBase(), "quarantine");
|
||||
if (!isSafeNativeStateDirectory(quarantine)) return scoped;
|
||||
const copies = readdirSync(quarantine).filter(name => name.startsWith(`${basename(scoped)}.identity_indeterminate.`));
|
||||
return copies.length === 1 ? resolve(quarantine, copies[0]!) : scoped;
|
||||
}
|
||||
|
||||
export async function verifyStoppedNativeSessionForContinuation(
|
||||
db: Db,
|
||||
run: Pick<typeof heartbeatRuns.$inferSelect, "id" | "companyId" | "agentId" | "runtimeMode" |
|
||||
@@ -3165,14 +3187,21 @@ export async function verifyStoppedNativeSessionForContinuation(
|
||||
eq(environmentLeases.companyId, run.companyId), eq(environmentLeases.heartbeatRunId, run.id)));
|
||||
if (leases.some(lease => lease.provider !== "local" || !lease.releasedAt ||
|
||||
lease.status === "pending_cleanup" || lease.cleanupStatus === "failed")) return null;
|
||||
const root = scopedRunnerdStateRoot(execution);
|
||||
// A run refused at the prior-owner fence moved the owner's state to
|
||||
// quarantine before it failed. That single exact copy is the owner's state.
|
||||
const root = quarantinedOwnerStateRoot(scopedRunnerdStateRoot(execution));
|
||||
const providerFile = runnerProviderStateFilename(execution);
|
||||
const snapshot = cleanupStateSnapshot(root, providerFile);
|
||||
const identity = record(snapshot.control.identity);
|
||||
let owner = run;
|
||||
let ownerExecution = execution;
|
||||
if (identity.runId !== run.id) {
|
||||
if (run.errorCode !== "native_session_cleanup_quarantined" || typeof identity.runId !== "string") return null;
|
||||
// Only a run refused before launch can stand in for the saved state's
|
||||
// owner: the cleanup quarantine or the prior-owner runner-state fence.
|
||||
const [fenced] = run.errorCode === "native_session_cleanup_quarantined" ? [] : await db.select({ id: heartbeatRunEvents.id })
|
||||
.from(heartbeatRunEvents).where(and(eq(heartbeatRunEvents.companyId, run.companyId), eq(heartbeatRunEvents.runId, run.id),
|
||||
isNull(heartbeatRunEvents.sourceEventId), eq(heartbeatRunEvents.eventType, PRIOR_OWNER_FENCE_REFUSED))).limit(1);
|
||||
if ((run.errorCode !== "native_session_cleanup_quarantined" && !fenced) || typeof identity.runId !== "string") return null;
|
||||
const [started] = await db.select({ id: heartbeatRunEvents.id }).from(heartbeatRunEvents).where(and(
|
||||
eq(heartbeatRunEvents.companyId, run.companyId), eq(heartbeatRunEvents.runId, run.id),
|
||||
inArray(heartbeatRunEvents.eventType, [PROCESS_START_REQUESTED, PROCESS_IDENTITY_RECORDED,
|
||||
@@ -3230,7 +3259,7 @@ export async function verifyStoppedNativeSessionForContinuation(
|
||||
snapshot.runner.schema !== RUNNERD_STATE_SCHEMA ||
|
||||
snapshot.provider.schema !== (execution.provider.kind === "codex" ? "paperclip.runner.codex-provider-state.v1" : ACPX_PROVIDER_STATE_SCHEMA) ||
|
||||
!["turn_active", "prepared", "suspended", "session_open"].includes(String(snapshot.provider.lifecycle)) ||
|
||||
snapshot.provider.startupAttempt != null) return null;
|
||||
!failedStartupRetired(snapshot.provider.startupAttempt)) return null;
|
||||
const pending = JSON.stringify([snapshot.runner.outbox, snapshot.provider.pendingEvents, snapshot.provider.queuedEvents]);
|
||||
if (/session\.(started|resumed|reconciled)/.test(pending)) return null;
|
||||
const events = snapshot.control.committedEvents.map(entry => record(record(record(entry).envelope).payload));
|
||||
@@ -3290,6 +3319,8 @@ export async function verifyStoppedNativeSessionForContinuation(
|
||||
if (!cleanupProcessAbsent(pid)) return null;
|
||||
providerPids.add(pid);
|
||||
}
|
||||
const startup = record(snapshot.provider.startupAttempt);
|
||||
for (const pid of [startup.processId, startup.processGroupId]) if (pid != null) providerPids.add(pid as number);
|
||||
const unchanged = () => idle() && !activeNativeSessions.has(owner.id) && cleanupProcessAbsent(stopped.processPid) &&
|
||||
[...providerPids].every(cleanupProcessAbsent) && cleanupStateSnapshot(root, providerFile).fingerprint === snapshot.fingerprint;
|
||||
return {
|
||||
@@ -5761,6 +5792,20 @@ async function rollBackUndeliveredAttachRotation(root: string, execution: Native
|
||||
`[paperclip-runner] Run ${execution.binding.runId} stopped before its runner received the session; resuming from the previous turn.\n`);
|
||||
}
|
||||
|
||||
/** The fence refused this run before any launch, because another run's saved
|
||||
* state could not be proven finished. Retry and new input read this event as
|
||||
* proof that the run never had a runner (native-local-process-stop.ts). A
|
||||
* failed write leaves the run held, which is the safe side. */
|
||||
async function recordPriorOwnerFenceRefusal(input: { db: Db; execution: NativeExecutionInput }, reason: string): Promise<void> {
|
||||
const { binding } = input.execution;
|
||||
await appendHeartbeatRunEvent(input.db, {
|
||||
companyId: binding.companyId, runId: binding.runId, agentId: binding.agentId,
|
||||
eventType: PRIOR_OWNER_FENCE_REFUSED, stream: "system", level: "warn",
|
||||
message: "The saved runner state of a previous run could not be proven finished; this run started no runner.",
|
||||
payload: { reason },
|
||||
}).catch(() => undefined);
|
||||
}
|
||||
|
||||
async function migrateRunnerdStateRootForExecution(input: {
|
||||
db: Db;
|
||||
execution: NativeExecutionInput;
|
||||
@@ -5949,6 +5994,7 @@ async function migrateRunnerdStateRootForExecution(input: {
|
||||
: "identity_indeterminate",
|
||||
);
|
||||
}
|
||||
await recordPriorOwnerFenceRefusal(input, `prior_owner_${verification}`);
|
||||
throw new Error(`runner_state_identity_mismatch: prior_owner_${verification}${retirementRefusal}`);
|
||||
}
|
||||
}
|
||||
@@ -5990,6 +6036,7 @@ async function migrateRunnerdStateRootForExecution(input: {
|
||||
// untouched because the legacy name may still belong to them.
|
||||
quarantineRunnerdStateRoot(legacy, "identity_indeterminate");
|
||||
}
|
||||
await recordPriorOwnerFenceRefusal(input, `legacy_prior_owner_${verification}`);
|
||||
throw new Error(`runner_state_identity_mismatch: legacy_prior_owner_${verification}`);
|
||||
}
|
||||
verifiedPriorRunId = identity.runId;
|
||||
|
||||
Reference in new issue
Block a user