mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 16:02:36 +01:00
fix(recovery): retain worker-stop audit in preparation retries (#19612)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Recovery must preserve saved files before it admits another execution. > - A failed preparation can need a verified stop of its previous workspace worker. > - Recovery keeps that verified stop receipt in the run profile for audit. > - The preparation retry guard treats the retained receipt as unknown execution evidence. > - This pull request recognizes a valid same-run audit receipt while it retains all current execution and file checks. > - A saved Retry can then continue once after recovery completes. ## Linked Issues or Issue Description **What happened?** A native preparation failed before provider dispatch. The recovery worker verified that the previous workspace worker had stopped and saved its files. The same HTTP Retry stayed deferred because the run profile retained `nativeWorkspaceOwnerStopProof`. **Expected behavior** The saved Retry should continue once after all preparation, stop, file, ownership, and authorization checks pass. The old run and its audit receipt should remain unchanged. **Steps to reproduce** 1. Fail native preparation with a retained remote workspace and no provider activity. 2. Let the recovery worker acquire the old workspace owner through verified platform stop evidence. 3. Submit one HTTP Retry and let preservation complete. 4. Observe that the old guard retains the retry hold even after saved-file verification passes. **Paperclip version or commit** Reproduced against `d728be8fbf`. Related work: #18908, #19021, #19026, and #19466. This change covers the retained workspace-worker stop audit left by a later repair. ## What Changed - Add a typed reader for a host-persisted same-run worker-stop audit receipt. Keep signature and expiry validation at ownership acquisition. - Recognize only that validated audit field in failed-preparation settlement. Keep unknown profiles and active owners blocked. - Exercise real preservation, HTTP Retry delivery, native dispatch, saved files, accepted results, and task completion. Add malformed, foreign-run, unstopped-worker, and active-owner cases. - Document the distinction between retained audit evidence and current execution authority. ## Verification - Failing before the fix: the current-worker preservation case passed; the stopped-worker case produced zero successor runs after the same Retry, where one was expected. - Passing after the fix: 47 recovery cases, 10 native dispatch/preservation cases, and 6 platform-proof tests. The two preservation success cases overlap between the first two groups. - Server typecheck passed all seven layers after the final test changes. The Runner and SDK dependencies were built in this isolated worktree. - Final comment-free commit `44f8f33f8d917dddb51852283a479786102eab4d`: a fresh combined run passed all 61 affected cases in 67.92s. Fresh server typecheck passed all seven layers. - The earlier `95d0376c` combined run passed 60 cases and hit one `socket hang up` in the existing `catalog_provider_warning` HTTP test. That case passed in a bounded isolated run (12.48s), and the final 61-case combined run passed. The socket failure remains recorded as an intermittent HTTP harness limitation. No retry or weaker assertion was added. - Final commit `44f8f33f8d917dddb51852283a479786102eab4d` completed all required CI gates: 57 checks succeeded, 3 intentional skips, and the advisory Paperclip review is neutral with a substantive 5/5 verdict. Greptile is 5/5 and there are no unresolved review threads. [Final CI run](https://github.com/paperclipai/paperclip/actions/runs/42293625696). The follow-up commit only removes six new code-comment lines to follow CONTRIBUTING. - The native provider and signed platform API are controlled test boundaries. The tests use real PostgreSQL, recovery, ownership, file persistence, retry admission, dispatch, and finalization services. Deployed recovery remains unverified until normal rollout. ## Risks - The main risk is mistaking an audit receipt for current stop authority. The reader checks its exact shape and run binding. The receipt cannot replace stopped leases, saved-file receipts, settled preparation, current authorization, or the absence of an active owner or provider evidence. - The negative matrix runs with the audit field present. Provider activity, unknown usage, bad file receipts, Stop, actor changes, and unsettled leases still block continuation. - No schema or migration change is required. ## Model Used OpenAI GPT-6 in Codex. The exact deployment model ID and context-window size are not exposed in this session. Used reasoning, code editing, shell execution, and test tools. ## 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 - [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
02a2299173
commit
c09fb60ca6
5 files changed
+81
-8
No files matched your search
@@ -479,6 +479,12 @@ never started. Recovery must not create another unknown-outcome incident for
|
||||
that admission denial. Historical incidents with the same verified evidence are
|
||||
nonblocking without deleting their audit records. The original hold and any
|
||||
workspace repair obligation remain effective until their own recovery completes.
|
||||
A completed failed-preparation repair can retain the host's same-run worker-stop
|
||||
receipt as audit evidence. Its original expiry controls ownership acquisition;
|
||||
it does not expire the saved repair. Retry still requires settled preparation,
|
||||
no live workspace owner or provider activity, stopped leases, verified saved
|
||||
files, and current execution authority. The audit receipt grants none of these.
|
||||
|
||||
Once all current gates clear, startup and periodic queue recovery reconsider the
|
||||
saved input through normal admission. The cancelled admission is not replayed,
|
||||
and the user need not certify infrastructure state in a reconciliation form.
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { isRetainedWorkspaceWorkerStopProof } from "./native-runtime/native-workspace-platform-proof.js";
|
||||
import { isPreDispatchReviewWait } from "./pre-dispatch-review-wait.js";
|
||||
import { createHash } from "node:crypto";
|
||||
import { and, eq, inArray, isNotNull, isNull, lte, or, sql } from "drizzle-orm";
|
||||
@@ -88,8 +89,9 @@ async function hasSettledFailedPreparationEvidence(db: Db, run: Run, historicalR
|
||||
"lastOutputAt", "lastOutputStream", "lastOutputBytes"].includes(key)) &&
|
||||
!(emptyLog && ["logStore", "logRef", "logBytes", "logSha256", "stdoutExcerpt", "stderrExcerpt"].includes(key)) && value !== null)
|
||||
|| !hasOnlyPreparationAccountingContext(source.usageJson, run.issueId ?? run.contextSnapshot?.issueId)
|
||||
|| Object.keys(run.runnerProfileJson ?? {}).some(key => !["adapterDispatch", "remoteWorkspace",
|
||||
"chatControlRecoveryAdmission"].includes(key))
|
||||
|| Object.entries(run.runnerProfileJson ?? {}).some(([key, value]) => !["adapterDispatch", "remoteWorkspace",
|
||||
"chatControlRecoveryAdmission"].includes(key) && !(key === "nativeWorkspaceOwnerStopProof"
|
||||
&& isRetainedWorkspaceWorkerStopProof(value, run.id)))
|
||||
|| run.resultJson?.stopReason !== "adapter_failed"
|
||||
|| Object.keys(run.resultJson).some(key => !["stopReason", "timeoutFired", "timeoutSource", "executionRecovery",
|
||||
"timeoutConfigured", "effectiveTimeoutSec", "systemRecoveryAdoption", "workspaceRestoreFailure",
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { generateKeyPairSync, sign } from "node:crypto";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { verifyWorkspaceWorkerStopReceipt, requestWorkspaceWorkerStopProof, currentWorkspaceWorker } from "./native-workspace-platform-proof.js";
|
||||
import { verifyWorkspaceWorkerStopReceipt, requestWorkspaceWorkerStopProof, currentWorkspaceWorker, isRetainedWorkspaceWorkerStopProof } from "./native-workspace-platform-proof.js";
|
||||
const { privateKey, publicKey } = generateKeyPairSync("ed25519");
|
||||
const env = { PAPERCLIP_CLOUD_STACK_ID: "stack", PAPERCLIP_CLOUD_API_ORIGIN: "https://cloud.example", PAPERCLIP_CLOUD_TENANT_SERVER_TOKEN: "fixture-token", PAPERCLIP_CLOUD_RUNTIME_IDENTITY_JWKS: JSON.stringify({ keys: [{ ...publicKey.export({ format: "jwk" }), kid: "test", alg: "EdDSA", use: "sig" }] }) };
|
||||
const worker = { provider: "railway" as const, projectId: "project", environmentId: "environment", serviceId: "service", deploymentId: "old-deployment" };
|
||||
@@ -19,6 +19,19 @@ describe("workspace platform stop proof", () => {
|
||||
}
|
||||
expect(verifyWorkspaceWorkerStopReceipt(receipt().slice(0,-5)+"wrong", binding, env, now)).toBeNull();
|
||||
});
|
||||
it("retains verified same-run audit evidence after acquisition expiry without granting new stop authority", () => {
|
||||
const proof = verifyWorkspaceWorkerStopReceipt(receipt(), binding, env, now)!;
|
||||
const later = new Date(now.getTime() + 120_000);
|
||||
expect(verifyWorkspaceWorkerStopReceipt(receipt(), binding, env, later)).toBeNull();
|
||||
expect(isRetainedWorkspaceWorkerStopProof(proof, binding.runId)).toBe(true);
|
||||
for (const invalid of [null, [], { stopped: true }, { ...proof, runId: "other-run" },
|
||||
{ ...proof, stopped: false }, { ...proof, exp: proof.iat }, { ...proof, exp: proof.iat + 61 },
|
||||
{ ...proof, iat: "100" }, { ...proof, ownerToken: "" }, { ...proof, sub: "" },
|
||||
{ ...proof, worker: { ...worker, deploymentId: "" } }, { ...proof, worker: { ...worker, provider: "other" } },
|
||||
{ ...proof, worker: { ...worker, providerStarted: true } }, { ...proof, providerStarted: true }]) {
|
||||
expect(isRetainedWorkspaceWorkerStopProof(invalid, binding.runId)).toBe(false);
|
||||
}
|
||||
});
|
||||
it("fails closed on live, unknown, unreachable and unsigned platform responses", async () => {
|
||||
for (const response of [{ stopped: false }, { stopped: true }, { receipt: "forged" }]) {
|
||||
const fetchImpl = vi.fn().mockResolvedValue(Response.json(response));
|
||||
|
||||
@@ -17,6 +17,20 @@ function validWorker(worker: unknown): worker is WorkspaceWorker {
|
||||
return value.provider === "railway" && ["projectId", "environmentId", "serviceId", "deploymentId"].every(key =>
|
||||
typeof value[key] === "string" && value[key].length > 0 && value[key].length <= 200);
|
||||
}
|
||||
export function isRetainedWorkspaceWorkerStopProof(value: unknown, runId: string): value is WorkspaceWorkerStopProof {
|
||||
if (!value || typeof value !== "object" || Array.isArray(value)) return false;
|
||||
const proof = value as Record<string, unknown>;
|
||||
return proof.v === 1 && proof.iss === "paperclip-cloud" && proof.aud === "paperclip-workspace-worker-stop/v1"
|
||||
&& proof.runId === runId && proof.stopped === true
|
||||
&& typeof proof.sub === "string" && proof.sub.length > 0 && proof.sub.length <= 200
|
||||
&& typeof proof.ownerToken === "string" && proof.ownerToken.length > 0 && proof.ownerToken.length <= 200
|
||||
&& Number.isInteger(proof.iat) && Number.isInteger(proof.exp)
|
||||
&& Number(proof.exp) > Number(proof.iat) && Number(proof.exp) - Number(proof.iat) <= 60
|
||||
&& validWorker(proof.worker)
|
||||
&& Object.keys(proof.worker).every(key => ["provider", "projectId", "environmentId", "serviceId", "deploymentId"].includes(key))
|
||||
&& Object.keys(proof).every(key => ["v", "iss", "aud", "sub", "stopped", "iat", "exp", "runId", "ownerToken", "worker"].includes(key));
|
||||
}
|
||||
|
||||
export function currentWorkspaceWorker(env: NodeJS.ProcessEnv = process.env): WorkspaceWorker | undefined {
|
||||
const ids = [env.RAILWAY_PROJECT_ID, env.RAILWAY_ENVIRONMENT_ID, env.RAILWAY_SERVICE_ID, env.RAILWAY_DEPLOYMENT_ID];
|
||||
if (ids.some(id => !id?.trim())) return undefined;
|
||||
|
||||
@@ -2424,8 +2424,24 @@ describe("automatic retained workspace recovery", () => {
|
||||
expect((await action(f.actionId)).evidence.explicitUserContinuation).toBeUndefined();
|
||||
}, 30_000);
|
||||
|
||||
it("resumes the same failed-preparation HTTP Retry from the real preservation worker", async () => {
|
||||
async function stoppedWorkspaceWorker(f: { runId: string }) {
|
||||
const source = await run(f.runId);
|
||||
const deploymentWorker = { provider: "railway" as const, projectId: randomUUID(), environmentId: randomUUID(), serviceId: randomUUID(), deploymentId: randomUUID() };
|
||||
const owner = { token: randomUUID(), hostname: "previous-controller", pid: 123,
|
||||
processStartedAt: new Date().toISOString(), worker: deploymentWorker };
|
||||
const stopProof = { v: 1 as const, iss: "paperclip-cloud" as const, aud: "paperclip-workspace-worker-stop/v1" as const,
|
||||
sub: "test-stack", stopped: true as const, iat: Math.floor(Date.now() / 1000),
|
||||
exp: Math.floor(Date.now() / 1000) + 30, runId: f.runId, ownerToken: owner.token, worker: deploymentWorker };
|
||||
const proof = vi.spyOn(workerProof, "requestWorkspaceWorkerStopProof").mockResolvedValue({ status: "verified", proof: stopProof });
|
||||
await db.update(heartbeatRuns).set({ runnerProfileJson: {
|
||||
...source.runnerProfileJson, nativeWorkspaceFinalizationOwner: owner,
|
||||
} }).where(eq(heartbeatRuns.id, f.runId));
|
||||
return { proof, stopProof, owner, deploymentWorker };
|
||||
}
|
||||
|
||||
it.each(["current worker", "stopped worker"] as const)("resumes the same failed-preparation HTTP Retry from the real preservation worker: %s", async mode => {
|
||||
const f = await failedRemotePreparation();
|
||||
const stoppedWorker = mode === "stopped worker" ? await stoppedWorkspaceWorker(f) : undefined;
|
||||
const [lease] = await db.select().from(environmentLeases).where(eq(environmentLeases.id, f.lease.id));
|
||||
await db.update(environmentLeases).set({ status: "active", releasedAt: null, cleanupStatus: null,
|
||||
metadata: { ...lease.metadata, remoteExecutionTermination: null } }).where(eq(environmentLeases.id, lease.id));
|
||||
@@ -2457,6 +2473,12 @@ describe("automatic retained workspace recovery", () => {
|
||||
expect(successor.contextSnapshot).toMatchObject({ previousRunId: f.runId, taskRetryRequestId: requestId });
|
||||
expect(await readFreshWorkspaceReadiness(db, successor)).toMatchObject({ mode: "resume_preserved_workspace" });
|
||||
expect(await hasVerifiedWorkspaceRepair(db, await run(f.runId))).toBe(true);
|
||||
if (mode === "stopped worker") {
|
||||
expect(stoppedWorker!.proof).toHaveBeenCalledWith({ runId: f.runId, ownerToken: stoppedWorker!.owner.token,
|
||||
worker: stoppedWorker!.deploymentWorker });
|
||||
expect((await run(f.runId)).runnerProfileJson?.nativeWorkspaceOwnerStopProof).toEqual(stoppedWorker!.stopProof);
|
||||
expect((await run(f.runId)).runnerProfileJson?.nativeWorkspaceFinalizationOwner).toBeUndefined();
|
||||
}
|
||||
const remote = (await run(f.runId)).runnerProfileJson!.remoteWorkspace as { snapshotId: string };
|
||||
const saved = await createSavedWorkspaceLoader(db)({ companyId: f.companyId, workspaceId: f.workspaceId, snapshotId: remote.snapshotId });
|
||||
try { expect(await readFile(path.join(saved.root, "saved.txt"), "utf8")).toBe("finished work\n"); }
|
||||
@@ -2466,10 +2488,10 @@ describe("automatic retained workspace recovery", () => {
|
||||
expect(f.runtime.resumeRunLease).toHaveBeenCalledOnce();
|
||||
expect((await run(f.runId))).toMatchObject({ status: "failed", errorCode: "setup_failed" });
|
||||
expect(errors).not.toHaveBeenCalled();
|
||||
} finally { releaseStop(); await coordinator.stop(); await worker.stop(); }
|
||||
} finally { releaseStop(); await coordinator.stop(); await worker.stop(); stoppedWorker?.proof.mockRestore(); }
|
||||
}, 40_000);
|
||||
|
||||
it.each(["delayed_provider_event", "native_input", "session_checkpoint", "session_after", "controller_recovery", "process", "coordinator",
|
||||
it.each(["malformed_worker_stop", "foreign_run_worker_stop", "unstopped_worker", "active_workspace_owner", "delayed_provider_event", "native_input", "session_checkpoint", "session_after", "controller_recovery", "process", "coordinator",
|
||||
"runtime_selected", "live_controller", "missing_error", "missing_measurement", "foreign_error", "unknown_result",
|
||||
"wrong_snapshot", "wrong_generation", "active_writer", "missing_stop", "foreign_stop", "additional_unstopped_lease",
|
||||
"operator_stop", "newer_input", "reassigned", "wrong_actor", "actor_permission_revoked",
|
||||
@@ -2481,6 +2503,15 @@ describe("automatic retained workspace recovery", () => {
|
||||
await db.update(heartbeatRuns).set({ sessionIdBefore: randomUUID() }).where(eq(heartbeatRuns.id, f.runId));
|
||||
await recoverRetainedWorkspace(f.input);
|
||||
const source = await run(f.runId), heartbeat = heartbeatService(db);
|
||||
const stoppedWorkerAudit = { v: 1, iss: "paperclip-cloud", aud: "paperclip-workspace-worker-stop/v1",
|
||||
sub: "test-stack", stopped: true, iat: 100, exp: 160, runId: f.runId, ownerToken: randomUUID(),
|
||||
worker: { provider: "railway", projectId: randomUUID(), environmentId: randomUUID(), serviceId: randomUUID(), deploymentId: randomUUID() } };
|
||||
source.runnerProfileJson = { ...source.runnerProfileJson, nativeWorkspaceOwnerStopProof:
|
||||
gate === "malformed_worker_stop" ? { stopped: true } : {
|
||||
...stoppedWorkerAudit, ...(gate === "foreign_run_worker_stop" ? { runId: randomUUID() } : {}),
|
||||
...(gate === "unstopped_worker" ? { stopped: false } : {}),
|
||||
}, ...(gate === "active_workspace_owner" ? { nativeWorkspaceFinalizationOwner: { token: randomUUID() } } : {}) };
|
||||
await db.update(heartbeatRuns).set({ runnerProfileJson: source.runnerProfileJson }).where(eq(heartbeatRuns.id, f.runId));
|
||||
const actorId = gate === "actor_permission_revoked" ? randomUUID() : "local-board";
|
||||
if (gate === "actor_permission_revoked") {
|
||||
await db.insert(authUsers).values({ id: actorId, name: "Retry owner", email: `${actorId}@example.invalid`, createdAt: new Date(), updatedAt: new Date() });
|
||||
@@ -2633,12 +2664,13 @@ describe("automatic retained workspace recovery", () => {
|
||||
expect(await readFile(path.join(cwd, "saved.txt"), "utf8")).toBe("existing task work\n");
|
||||
});
|
||||
|
||||
it.each(["complete", "automatic_lost", "catalog", "catalog_warning", "catalog_unknown_warning", "catalog_provider_warning", "invalid_published_root"] as const)(
|
||||
it.each(["complete", "stopped_worker", "automatic_lost", "catalog", "catalog_warning", "catalog_unknown_warning", "catalog_provider_warning", "invalid_published_root"] as const)(
|
||||
"dispatches the recovered failed-preparation Retry once through the native provider boundary (%s)", async mode => {
|
||||
const unavailableCatalog = mode !== "complete" && mode !== "automatic_lost" && mode !== "invalid_published_root", withWarning = mode.includes("warning");
|
||||
const unavailableCatalog = mode !== "complete" && mode !== "stopped_worker" && mode !== "automatic_lost" && mode !== "invalid_published_root", withWarning = mode.includes("warning");
|
||||
const localStorage = getStorageService();
|
||||
const databaseUrl = process.env.DATABASE_URL, masterKey = process.env.PAPERCLIP_SECRETS_MASTER_KEY;
|
||||
const experimental = await instanceSettingsService(db).getExperimental();
|
||||
let stoppedWorker: Awaited<ReturnType<typeof stoppedWorkspaceWorker>> | undefined;
|
||||
let storage: { mockRestore(): void } | undefined;
|
||||
const starts = vi.spyOn(agentStarts, "withAgentStartLock");
|
||||
const retryResumes: Promise<void>[] = [];
|
||||
@@ -2660,7 +2692,12 @@ describe("automatic retained workspace recovery", () => {
|
||||
await withheldPreparationRetry(f);
|
||||
}
|
||||
if (mode === "complete") await db.update(heartbeatRuns).set({ sessionIdBefore: randomUUID() }).where(eq(heartbeatRuns.id, f.runId));
|
||||
if (mode === "stopped_worker") stoppedWorker = await stoppedWorkspaceWorker(f);
|
||||
await recoverRetainedWorkspace(f.input);
|
||||
if (stoppedWorker) {
|
||||
expect((await run(f.runId)).runnerProfileJson?.nativeWorkspaceOwnerStopProof).toEqual(stoppedWorker.stopProof);
|
||||
expect((await run(f.runId)).runnerProfileJson?.nativeWorkspaceFinalizationOwner).toBeUndefined();
|
||||
}
|
||||
const source = await run(f.runId), nextDirectory = path.join(root, randomUUID(), "next");
|
||||
await mkdir(nextDirectory, { recursive: true });
|
||||
const nextRemote = await realpath(nextDirectory);
|
||||
@@ -2866,6 +2903,7 @@ describe("automatic retained workspace recovery", () => {
|
||||
await ownedHeartbeat?.drainActiveRunExecutions();
|
||||
} finally {
|
||||
resumeSpy?.mockRestore(); starts.mockRestore();
|
||||
stoppedWorker?.proof.mockRestore();
|
||||
storage?.mockRestore();
|
||||
if (databaseUrl === undefined) delete process.env.DATABASE_URL; else process.env.DATABASE_URL = databaseUrl;
|
||||
if (masterKey === undefined) delete process.env.PAPERCLIP_SECRETS_MASTER_KEY; else process.env.PAPERCLIP_SECRETS_MASTER_KEY = masterKey;
|
||||
|
||||
Reference in new issue
Block a user