mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 15:12:18 +01:00
fix(native): fence retries when replaying an audited Stop (#18457)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native runs keep Stop intent and retry state in durable records. > - A provider error can leave a failed retry with an earlier acknowledged Stop. > - The replay path returned the existing receipt before it closed that retry. > - This pull request applies the existing retry fence to the validated replay. > - The same Stop can settle without another provider cancellation or a new receipt. ## Linked Issues or Issue Description Refs #14733 for the related caller-bound Stop contract. This change covers a later retry that already has a Stop acknowledgement. The older CI-only #14743 is not a production delivery PR. **What happened?** A failed native run can have a retryable coordinator and an earlier audited Stop. A repeat Stop returns the old acknowledgement without moving the coordinator to `terminal_failure`. The final failed-to-cancelled transition requires that fence, so the Stop cannot settle through that path. **Expected behavior** A valid replay closes the same retry and keeps the original intent and acknowledgement. It does not send a second provider cancellation. Physical stop and workspace cleanup still require their own evidence. **Steps to reproduce** 1. Store a failed native run with a `retryable_failure` coordinator. 2. Store a matching run-scoped Stop with a dispatched acknowledgement and audit IDs. 3. Call `cancelNativeSession` for that same run and inspect the coordinator. **Paperclip version or commit** Reproduced on public master `f2db909393de124caf6a5380a52d4e0e1d621661`. **Deployment mode** Core native execution. The regression uses the real cancellation executor with controlled persistence and provider boundaries. ## What Changed - Reuse the existing guarded coordinator fence for a validated Stop replay. - Test repeated replay, unchanged receipts, one fence, and no second provider cancellation. - Reject foreign company, run, issue, scope, effect, and missing-audit receipts before any write. - Check that an acknowledged Stop with an unconfirmed remote lease still fails the physical-stop guard. - Document the separate retry fence and physical-cleanup requirements. ## Verification - RED on the selected public base: the new executor case returned success but left the coordinator in `retryable_failure` with a future retry. One case executed and failed for that assertion. - GREEN: 62 focused tests passed across `native-session-executor.test.ts` and `native-cancellation-request.test.ts`. These include ownership, caller conflict, changed-phase, and receipt controls. - Command from `server/`: `node ../node_modules/vitest/vitest.mjs run src/services/native-runtime/native-session-executor.test.ts src/services/native-runtime/native-cancellation-request.test.ts -t 'native session cancellation|caller-bound native cancellation|failed native retry caller admission'`. - Canonical Linux CI passed all 1,018 executor cases and all 32 caller-cancellation cases. The exact file completion lines are in [server shard 2](https://github.com/paperclipai/paperclip/actions/runs/41203989386/job/124545366271) and [server shard 10](https://github.com/paperclipai/paperclip/actions/runs/41203989386/job/124545366219). - Node 24.22.2 and Vitest 5.0.3. Map contract checks and `git diff --check` pass. - Local dependencies are linked from an existing checkout. Full local TypeScript validation reports older Runner/shared declarations that lack current Kimi and continuity APIs. The changed ranges have no diagnostics. The canonical clean CI Typecheck + Release Registry gate passed. - A broader two-file run was stopped when an unchanged integration case tried to start embedded Postgres and hit the host listen restriction. It is not a passing full-suite result. No host settings or other services were changed. - Repository-wide `pnpm -r typecheck`, `pnpm test:run`, and `pnpm build` were not completed locally. Current-head CI passed against clean source and dependencies. - Fresh [Greptile review](https://github.com/paperclipai/paperclip/pull/18457#issuecomment-6543772487) on `dbc1c1a5b71450194d568725592969660864cb5a`: 5/5 with no actionable findings. The review-thread inventory has no threads and no pagination. Full [current-head CI](https://github.com/paperclipai/paperclip/actions/runs/41203989386) passed: 59 terminal checks, 55 successful, 3 skipped, and 1 neutral. No failure or pending check remains. Merge status is CLEAN. No live recovery, rollout, or physical termination is claimed. ## Risks - A wrong or stale Stop must not close another retry. Existing run/coordinator locks, caller checks, receipt binding, and the final compare-and-set remain in place. - This fix changes logical retry settlement only. Provider termination, workspace preservation, lease cleanup, and continuation gates remain separate. - The regression controls persistence and provider boundaries. It does not establish the physical state of a remote provider. ## Model Used - OpenAI GPT-6 through Codex, with reasoning, code editing, and tool use. The session does not expose an exact model build ID or context-window capacity. ## 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 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
ab51cabbbd
commit
12d8c3043e
3 files changed
+85
-18
No files matched your search
@@ -330,6 +330,10 @@ persisted cancellations of parked retries through the normal guarded cancellatio
|
||||
path if the original owner stops or its final database write fails. They preserve
|
||||
the original cancellation request and never start a provider turn for that work.
|
||||
|
||||
Replaying an audited native Stop also fences a later same-run retry. It preserves
|
||||
the original intent and acknowledgement. That retry fence does not prove that
|
||||
the provider stopped or that workspace cleanup finished.
|
||||
|
||||
Recovery instructions in a wake describe only an active or escalated recovery
|
||||
action. Resolving or cancelling that action removes its repair instructions from
|
||||
subsequent wakes, so the original worker can resume the task. This prompt change
|
||||
|
||||
@@ -99,6 +99,7 @@ import { buildNativeHeartbeatPreparationSpans, createNativeRunTrace } from "./na
|
||||
import { NativeRunnerOwnershipUnverifiedError } from "./native-runner-ownership.js";
|
||||
import type { AdapterRuntimeEvent } from "../../adapters/index.js";
|
||||
import type { AdapterUsageCheckpoint } from "@paperclipai/adapter-utils";
|
||||
import { acknowledgedNativeStopExecutionHasStopped, isAcknowledgedNativeStop } from "../acknowledged-native-stop.js";
|
||||
|
||||
vi.mock("node:fs", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("node:fs")>();
|
||||
@@ -7852,6 +7853,27 @@ describe("native session cancellation", () => {
|
||||
expect(state.persistActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ companyId: "another-company" }, { runId: "another-run" }, { issueId: "another-issue" },
|
||||
{ scope: "issue" }, { effects: ["cancel_issue"] }, { intentAuditId: null },
|
||||
])("does not fence a failed retry for a foreign or stale Stop receipt %j", async changed => {
|
||||
const intent = { schema: "paperclip.native-cancellation.v1", intentId: "native-cancellation:earlier-stop",
|
||||
...execution.binding, scope: "run", reasonCode: "cancellation_run_only", effects: ["release_run_resources"],
|
||||
dispatchState: "acknowledged", dispatched: true, intentAuditId: "earlier-intent-audit",
|
||||
acknowledgementAuditId: "earlier-ack-audit", ...changed };
|
||||
const coordinator = { runId: execution.binding.runId, assessmentId: null, phase: "retryable_failure" };
|
||||
const persistence = cancellationDb({ status: "failed", coordinator, resultJson: { nativeCancellation: intent } });
|
||||
const before = structuredClone(persistence.getResultJson());
|
||||
state.cancel.mockClear();
|
||||
state.persistActivity.mockClear();
|
||||
await expect(cancelNativeSession(execution.binding.runId, "retry Stop", { db: persistence.db, scope: "run" })).rejects.toThrow();
|
||||
expect(persistence.updates).toEqual([]);
|
||||
expect(coordinator.phase).toBe("retryable_failure");
|
||||
expect(persistence.getResultJson()).toEqual(before);
|
||||
expect(state.cancel).not.toHaveBeenCalled();
|
||||
expect(state.persistActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not mistake a recovered running attempt for the failed retry that reserved Stop", async () => {
|
||||
const id = "11111111-1111-4111-8111-111111111111";
|
||||
const persistence = cancellationDb({ status: "running",
|
||||
@@ -7877,6 +7899,39 @@ describe("native session cancellation", () => {
|
||||
expect(persistence.getResultJsonUpdateCount()).toBe(writes);
|
||||
});
|
||||
|
||||
it("fences a failed retry whose earlier Stop is already acknowledged without changing its receipts", async () => {
|
||||
const intent = { schema: "paperclip.native-cancellation.v1", intentId: "native-cancellation:earlier-stop",
|
||||
...execution.binding, scope: "run", reasonCode: "cancellation_run_only",
|
||||
effects: ["release_run_resources"], dispatchState: "acknowledged", dispatched: true,
|
||||
intentAuditId: "earlier-intent-audit", acknowledgementAuditId: "earlier-ack-audit",
|
||||
acknowledgedAt: "2026-01-01T00:00:00.000Z", decisionId: null, priorCoordinatorDecisionId: null };
|
||||
const coordinator = { runId: execution.binding.runId, assessmentId: null, phase: "retryable_failure",
|
||||
nextAttemptAt: new Date(Date.now() + 30_000) };
|
||||
const persistence = cancellationDb({ status: "failed", coordinator,
|
||||
resultJson: { nativeCancellation: intent, executionCancellation: { state: "requested", requestedAt: "later-stop" } } });
|
||||
const before = structuredClone(persistence.getResultJson());
|
||||
state.cancel.mockClear();
|
||||
state.persistActivity.mockClear();
|
||||
for (let attempt = 0; attempt < 2; attempt++) {
|
||||
await expect(cancelNativeSession(execution.binding.runId, "retry Stop", { db: persistence.db, scope: "run" }))
|
||||
.resolves.toMatchObject({ dispatched: true, auditId: intent.intentAuditId });
|
||||
expect(coordinator).toMatchObject({ phase: "terminal_failure", failureCode: "native_retry_cancelled", nextAttemptAt: null });
|
||||
expect(persistence.getResultJson()).toEqual(before);
|
||||
}
|
||||
expect(persistence.updates.filter(update => update.table === nativeRunFinalizations)).toHaveLength(1);
|
||||
expect(persistence.getResultJsonUpdateCount()).toBe(0);
|
||||
expect(state.cancel).not.toHaveBeenCalled();
|
||||
expect(state.persistActivity).not.toHaveBeenCalled();
|
||||
const stopped = { id: execution.binding.runId, companyId: execution.binding.companyId,
|
||||
nativeIssueId: execution.binding.issueId, status: "cancelled", processPid: null, processGroupId: null,
|
||||
resultJson: { ...persistence.getResultJson(), cancelledByActorType: "user", cancelledByUserId: "operator" } };
|
||||
const lease = { provider: "daytona", providerLeaseId: "unconfirmed-provider-resource", status: "active",
|
||||
releasedAt: null, cleanupStatus: null, metadata: {} };
|
||||
const leaseDb = { select: () => ({ from: () => ({ where: async () => [lease] }) }) } as unknown as Db;
|
||||
expect(isAcknowledgedNativeStop(stopped)).toBe(true);
|
||||
await expect(acknowledgedNativeStopExecutionHasStopped(leaseDb, stopped)).resolves.toBe(false);
|
||||
});
|
||||
|
||||
it("uses the reserved caller UUID and permits same-intent and default retries", async () => {
|
||||
const id = "11111111-1111-4111-8111-111111111111";
|
||||
const persistence = cancellationDb({ resultJson: { startupCancellation: { cancellationRequestId: id } } });
|
||||
|
||||
@@ -7541,6 +7541,27 @@ export async function cancelNativeSession(
|
||||
runId, companyId: cancellationContext.companyId, issueId: cancellationContext.issueId,
|
||||
resultJson, requestId: requestedId, scope: options.scope ?? "run",
|
||||
}, coordinator)) throw conflict("Native retry is no longer cancellable");
|
||||
const fenceRetryCancellation = async () => {
|
||||
if (coordinator.phase !== "retryable_failure") return;
|
||||
await tx
|
||||
.update(nativeRunFinalizations)
|
||||
.set({
|
||||
phase: "terminal_failure",
|
||||
nextAttemptAt: null,
|
||||
leaseOwner: null,
|
||||
leaseExpiresAt: null,
|
||||
recoveryState: "blocked",
|
||||
failureCode: "native_retry_cancelled",
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(
|
||||
and(
|
||||
eq(nativeRunFinalizations.runId, runId),
|
||||
eq(nativeRunFinalizations.companyId, cancellationContext.companyId),
|
||||
eq(nativeRunFinalizations.phase, "retryable_failure"),
|
||||
),
|
||||
);
|
||||
};
|
||||
const existing = record(resultJson.nativeCancellation);
|
||||
const existingIntentId =
|
||||
typeof existing.intentId === "string" && existing.intentId.length > 0
|
||||
@@ -7564,6 +7585,10 @@ export async function cancelNativeSession(
|
||||
: null;
|
||||
if (!existingAuditId)
|
||||
throw new Error("native_cancellation_intent_audit_missing");
|
||||
if (retryCancellation && coordinator.phase === "retryable_failure") {
|
||||
await notifyNativeResultCommit(tx, { companyId: cancellationContext.companyId, runId });
|
||||
await fenceRetryCancellation();
|
||||
}
|
||||
return {
|
||||
intentId: existingIntentId,
|
||||
auditId: existingAuditId,
|
||||
@@ -7643,24 +7668,7 @@ export async function cancelNativeSession(
|
||||
// Cancellation is also an authority fence for a durable retry whose
|
||||
// preceding provider has already failed. No in-memory session is needed.
|
||||
await notifyNativeResultCommit(tx, { companyId: cancellationContext.companyId, runId });
|
||||
await tx
|
||||
.update(nativeRunFinalizations)
|
||||
.set({
|
||||
phase: "terminal_failure",
|
||||
nextAttemptAt: null,
|
||||
leaseOwner: null,
|
||||
leaseExpiresAt: null,
|
||||
recoveryState: "blocked",
|
||||
failureCode: "native_retry_cancelled",
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(
|
||||
and(
|
||||
eq(nativeRunFinalizations.runId, runId),
|
||||
eq(nativeRunFinalizations.companyId, cancellationContext.companyId),
|
||||
eq(nativeRunFinalizations.phase, "retryable_failure"),
|
||||
),
|
||||
);
|
||||
await fenceRetryCancellation();
|
||||
intentPublication = activity.publication;
|
||||
return {
|
||||
intentId,
|
||||
|
||||
Reference in new issue
Block a user