mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 01:50:13 +01:00
fix: preserve database-only failed controller evidence (#19714)
## Thinking Path > - Paperclip preserves failed work as well as successful work. > - Ephemeral enrollment must retain each selected controller source. > - A failed source can already exist entirely in an encrypted database checkpoint. > - Its original inline snapshot does not create a local filesystem dependency. > - The migration owner must prove that both legacy files are absent before accepting that case. > - This change preserves the failed evidence without creating execution authority. ## Linked Issues or Issue Description Refs: #19678. **What happened?** Startup still reports `native_controllers_require_migration` for a failed retained source with a valid database checkpoint, its original inline snapshot, and no local controller or supervisor. The online archive owner reports that the source is already absent. The existing database-only preservation case accepts only a missing inline snapshot. **Expected behavior** Accept the exact database-only failed evidence after full checkpoint and identity validation. Keep a present local payload blocked. Retain the failed run, original snapshot, coordinator, task pointer and queued work. **Steps to reproduce** 1. Store a valid encrypted checkpoint for a failed native run. 2. Keep its matching inline session snapshot and terminal-failure coordinator. 3. Remove both legacy controller files. 4. Check local controller obligations. 5. The old owner reports an obligation even though the evidence is already in the database. **Paperclip version or commit** The fix builds on current master `d5c4f7dd02`. **Deployment mode** Hosted native Runner with durable controller checkpoints. The local regression uses the existing real PostgreSQL and encrypted checkpoint fixture. ## What Changed - Add a narrow database-only failed-source preservation case to the existing migration owner. - Require the decoded checkpoint, protocol header, admitted execution, selected run and inline snapshot to match their exact scope. - Require both the controller and supervisor paths to be absent. A file or link keeps the local obligation blocked. - Retain the earlier null-snapshot predicate and all original failed execution evidence. Create no recovery metadata or executable authority. - Exercise the actual startup and maintenance consumers. Document the preservation boundary. ## Verification - Final revision `1b1da543dafd576e3c420657e3c2c6d0c882172d`: all checks are terminal, with 56 successful and four skipped, zero pending and zero failures. The [required CI aggregate](https://github.com/paperclipai/paperclip/actions/runs/42329501690/job/128275664414) passed. Build, types, server/Runner/serialized suites, end-to-end aggregate and Snyk passed. [Fresh exact-head review](https://github.com/paperclipai/paperclip/pull/19714#issuecomment-6701018015) is 5/5 with no open threads. - Review found that the first non-recovery assertion stopped at the queued fixture's missing current binding. The fixture now binds the exact native issue/session before calling the real resolver and asserts `unsupported:metadata_format` from the selected failed source. That corrected case passes in 4.58 seconds, with wrapper exit 0 and cleanup complete. The production predicate is unchanged. The original startup RED remains valid. The final source/test type graph, module boundaries and diff checks pass. Fresh revision CI and review remain required. - The actual startup regression failed before the source change: one case, 16.62 seconds, expected the absent-files obligation to be false but received true. The wrapper exited 1 and completed cleanup. This failure is retained. - The final focused owner run passed nine cases in 10.79 seconds. It covers the new DB-only case, controller/supervisor presence, every inline scope field, foreign checkpoint Runner/session identities, the unchanged null-snapshot case, and existing online/offline archival controls. The wrapper exited 0 and completed cleanup. - The new case verifies unchanged run, coordinator, checkpoint and task-session rows, retained queued work, no upload, and no invented recoverable session. - The full qualified server/test source type graph passes with db/shared/Runner source aliases. Module boundaries and diff checks pass. Required broad package builds and regression suites remain in CI. - Read-only source diagnosis validated one exact existing checkpoint with the canonical decoder and matching header, run, Runner and session bindings. No source profile or secret values were emitted. No hosted mutation, admission hold, cancellation, status change, or writers-stopped claim was performed. ## Risks - A false database-only classification could hide local bytes. Both exact legacy paths must be absent, and every protocol and database binding must match. - Corrupt or mismatched checkpoints still fail closed. Existing failed evidence does not become successful or executable. - The original null-snapshot case is unchanged. Present local files still require the existing preservation path. ## Model Used OpenAI GPT-6 through Codex, with reasoning, tool use and code execution. The host does not expose a more specific backend model ID or context-window size. ## 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
a5fe5deebe
commit
55c9c44e41
3 files changed
+110
-2
No files matched your search
@@ -0,0 +1,26 @@
|
||||
# Preserve database-only failed controller evidence
|
||||
|
||||
A retained failed run can already have a complete encrypted controller checkpoint
|
||||
and its original inline session snapshot while both legacy controller files are
|
||||
absent. This is preserved evidence. It does not become successful or executable
|
||||
recovery authority merely because the instance changes storage mode.
|
||||
|
||||
The migration owner accepts this case only after the canonical checkpoint decoder
|
||||
validates every record and its company/issue/run encryption binding. The selected
|
||||
run, admitted execution and inline snapshot must match company, agent, issue, run
|
||||
and normalized session. The decoded checkpoint and protocol header must also match
|
||||
the run, Runner instance and normalized session. The exact controller and supervisor
|
||||
paths must both be absent. A remaining file, symbolic link, unsafe path, invalid
|
||||
binding or failed read keeps the obligation blocked.
|
||||
|
||||
The earlier failed checkpoint case with no inline snapshot keeps its existing
|
||||
predicate. The new case does not replace an inline snapshot, create recovery
|
||||
metadata, change a failed status or coordinator, settle an action, or move a task
|
||||
pointer. Present legacy payloads still need the existing preservation path.
|
||||
|
||||
The focused regression uses the existing encrypted PostgreSQL checkpoint fixture.
|
||||
It first reproduces the startup blocker. It then verifies the absent-files case,
|
||||
remaining controller and supervisor files, foreign inline identities and foreign
|
||||
checkpoint Runner/session identities. Existing null-snapshot and online/offline
|
||||
archive controls run with it. The original queued turn, run, coordinator and
|
||||
checkpoint rows stay unchanged, and failed evidence stays non-recoverable.
|
||||
@@ -23,7 +23,7 @@ import { legacyNativeControllerArchive } from "./native-controller-archive.js";
|
||||
import { restoreDurableNativeHarnessBackup } from "./native-harness-backup-storage.js";
|
||||
|
||||
describeEmbeddedPostgres.each(["archive_online", "archive_online_owned", "archive_online_changed", "archive_online_commit", "archive_online_active", "archive_online_foreign",
|
||||
"online_success", "online_owned", "online_same_source", "online_busy", "online_changed", "online_evidence", "import", "archive", "success", "batch", "durable_failed",
|
||||
"online_success", "online_owned", "online_same_source", "online_busy", "online_changed", "online_evidence", "import", "archive", "success", "batch", "durable_failed", "durable_inline_failed",
|
||||
"stale_confirmed", "stale_foreign", "stale_changed", "stale_queued", "stale_overlap", "stale_success", "stale_busy", "stale_durable", "stale_durable_absent",
|
||||
"stale_durable_runner", "stale_durable_session", "stale_durable_snapshot",
|
||||
"controller_confirmed", "controller_foreign", "controller_thread", "controller_runner", "controller_success", "controller_busy", "controller_invalid", "controller_durable",
|
||||
@@ -331,6 +331,60 @@ describeEmbeddedPostgres.each(["archive_online", "archive_online_owned", "archiv
|
||||
expect(await readFile(f.file, "utf8")).toBe(f.source);
|
||||
} finally { f.db.$client.options.debug = previousDebug; }
|
||||
}, 60_000);
|
||||
if (scenario === "durable_inline_failed") it("preserves exact DB-only failed inline evidence while retaining every local payload and binding guard", async () => {
|
||||
const f = await fixture(), binding = { companyId: f.companyId, issueId: f.issueId, runId: f.runId };
|
||||
const storage = createStorageService(createLocalDiskStorageProvider(join(f.runtimeRoot, "unused-durable-objects")));
|
||||
const put = vi.spyOn(storage, "putFile");
|
||||
const checkpoint = captureLegacyDurablePrpCheckpoint(f.source, { runId: f.runId,
|
||||
runnerInstanceId: f.native.runnerInstanceId, normalizedSessionId: f.native.normalizedSessionId },
|
||||
{ version: "migration-fixture", digest: `sha256:${"a".repeat(64)}` });
|
||||
await f.db.transaction(tx => insertMigratedNativeRunnerCheckpoint(tx, binding, checkpoint));
|
||||
await f.db.update(nativeRunFinalizations).set({ recoveryState: "blocked", failureCode: "original_failure" })
|
||||
.where(eq(nativeRunFinalizations.runId, f.runId));
|
||||
expect(await hasLocalNativeControllerObligations(f.db, storage)).toBe(true);
|
||||
await rm(f.file);
|
||||
await mkdir(dirname(f.paths.supervisorPath), { recursive: true });
|
||||
await writeFile(f.paths.supervisorPath, "retained newer supervisor\n");
|
||||
expect(await hasLocalNativeControllerObligations(f.db, storage)).toBe(true);
|
||||
expect(await readFile(f.paths.supervisorPath, "utf8")).toBe("retained newer supervisor\n");
|
||||
await rm(f.paths.supervisorPath);
|
||||
expect(await hasLocalNativeControllerObligations(f.db, storage)).toBe(false);
|
||||
for (const field of ["companyId", "agentId", "issueId", "runId", "sessionId"] as const) {
|
||||
const profile = { ...f.profile, sessionCheckpoint: { ...f.snapshot, identity: { ...f.snapshot.identity, [field]: randomUUID() } } };
|
||||
await f.db.update(heartbeatRuns).set({ runnerProfileJson: profile }).where(eq(heartbeatRuns.id, f.runId));
|
||||
expect(await hasLocalNativeControllerObligations(f.db, storage)).toBe(true);
|
||||
}
|
||||
await f.db.update(heartbeatRuns).set({ runnerProfileJson: f.profile }).where(eq(heartbeatRuns.id, f.runId));
|
||||
for (const field of ["runnerInstanceId", "normalizedSessionId"] as const) {
|
||||
const state = JSON.parse(f.source); state.identity[field] = randomUUID();
|
||||
const foreign = captureLegacyDurablePrpCheckpoint(JSON.stringify(state), { runId: f.runId,
|
||||
runnerInstanceId: state.identity.runnerInstanceId, normalizedSessionId: state.identity.normalizedSessionId },
|
||||
{ version: "migration-fixture", digest: `sha256:${"a".repeat(64)}` });
|
||||
await f.db.delete(nativeRunnerCheckpoints).where(eq(nativeRunnerCheckpoints.runId, f.runId));
|
||||
await f.db.transaction(tx => insertMigratedNativeRunnerCheckpoint(tx, binding, foreign));
|
||||
expect(await hasLocalNativeControllerObligations(f.db, storage)).toBe(true);
|
||||
}
|
||||
await f.db.delete(nativeRunnerCheckpoints).where(eq(nativeRunnerCheckpoints.runId, f.runId));
|
||||
await f.db.transaction(tx => insertMigratedNativeRunnerCheckpoint(tx, binding, checkpoint));
|
||||
const [run] = await f.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.runId));
|
||||
const [owner] = await f.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId));
|
||||
const rows = await f.db.select().from(nativeRunnerCheckpoints).where(eq(nativeRunnerCheckpoints.runId, f.runId));
|
||||
expect(await migrateNativeControllers(f.db, { writersStopped: true, signal: AbortSignal.timeout(30_000), storage }))
|
||||
.toMatchObject({ preservedFailed: 1, imported: 0, archivedFailed: 0 });
|
||||
expect(await hasLocalNativeControllerObligations(f.db, storage)).toBe(false);
|
||||
expect((await f.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.runId)))[0]).toEqual(run);
|
||||
expect((await f.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId)))[0]).toEqual(owner);
|
||||
expect(await f.db.select().from(nativeRunnerCheckpoints).where(eq(nativeRunnerCheckpoints.runId, f.runId))).toEqual(rows);
|
||||
expect((await f.db.select().from(agentTaskSessions).where(eq(agentTaskSessions.id, f.session.id)))[0].lastRunId).toBe(f.runId);
|
||||
expect((await f.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.queuedId)))[0].status).toBe("queued");
|
||||
expect(put).not.toHaveBeenCalled();
|
||||
expect((await loadNativeRunnerCheckpoint(f.db, binding))?.records).toEqual(checkpoint.records);
|
||||
await f.db.update(heartbeatRuns).set({ nativeIssueId: f.issueId, nativeSessionId: f.native.normalizedSessionId })
|
||||
.where(eq(heartbeatRuns.id, f.queuedId));
|
||||
const recovery = await resolveNativeControllerRecovery(f.db, { companyId: f.companyId, agentId: f.agentId,
|
||||
issueId: f.issueId, normalizedSessionId: f.native.normalizedSessionId, currentRunId: f.queuedId });
|
||||
expect(recovery).toEqual({ kind: "unsupported", reason: "metadata_format" });
|
||||
});
|
||||
if (scenario === "durable_failed") it("preserves a failed durable checkpoint without inventing a missing session snapshot", async () => {
|
||||
const f = await fixture();
|
||||
const binding = { companyId: f.companyId, issueId: f.issueId, runId: f.runId };
|
||||
|
||||
@@ -3,7 +3,7 @@ import { lstat, open, readdir } from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
import { and, eq, inArray, or, sql } from "drizzle-orm";
|
||||
import { heartbeatRuns, issues, nativeRunFinalizations, type Db } from "@paperclipai/db";
|
||||
import { captureLegacyDurablePrpCheckpoint, parseNativeExecutionInput } from "../../vendor/paperclip-runner/index.js";
|
||||
import { captureLegacyDurablePrpCheckpoint, inspectDurablePrpCheckpointHeader, parseNativeExecutionInput } from "../../vendor/paperclip-runner/index.js";
|
||||
import { offlineMigrationExecutionWhere } from "../offline-migration-execution.js";
|
||||
import { findNativeSessionResumeRun } from "./native-session-resume.js";
|
||||
import { nativeControllerMigrationDescriptor } from "./native-session-executor.js";
|
||||
@@ -150,6 +150,31 @@ function preservedFailedCheckpoint(source: NonNullable<Awaited<ReturnType<typeof
|
||||
&& checkpoint.recoveryEvidence.normalizedSessionId === sessionId;
|
||||
}
|
||||
|
||||
async function preservedDatabaseOnlyFailedCheckpoint(selected: Selected,
|
||||
source: NonNullable<Awaited<ReturnType<typeof findNativeSessionResumeRun>>>,
|
||||
checkpoint: Awaited<ReturnType<typeof loadNativeRunnerCheckpoint>>) {
|
||||
const snapshot = record(source.runnerProfileJson.sessionCheckpoint), identity = record(snapshot.identity);
|
||||
if (source.status !== "failed" || source.runtimeMode !== "native" || !checkpoint
|
||||
|| source.companyId !== selected.companyId || source.agentId !== selected.agentId
|
||||
|| source.nativeIssueId !== selected.issueId || source.nativeSessionId !== selected.sessionId
|
||||
|| identity.companyId !== selected.companyId || identity.agentId !== selected.agentId
|
||||
|| identity.issueId !== selected.issueId || identity.runId !== source.id || identity.sessionId !== selected.sessionId
|
||||
|| typeof snapshot.sessionId !== "string" || !snapshot.sessionId.trim()
|
||||
|| checkpoint.runId !== source.id || checkpoint.recoveryEvidence.runnerInstanceId !== source.runnerInstanceId
|
||||
|| checkpoint.recoveryEvidence.normalizedSessionId !== selected.sessionId) return false;
|
||||
const header = inspectDurablePrpCheckpointHeader(checkpoint.records.get("header") ?? "");
|
||||
if (header?.runId !== source.id || header.runnerInstanceId !== source.runnerInstanceId
|
||||
|| header.normalizedSessionId !== selected.sessionId) return false;
|
||||
const paths = await localPaths(source);
|
||||
for (const file of [paths.controlFile, paths.supervisorPath]) {
|
||||
const present = await lstat(file).catch((error: NodeJS.ErrnoException) => {
|
||||
if (error.code === "ENOENT") return undefined; throw error;
|
||||
});
|
||||
if (present) return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
export async function hasLocalNativeControllerObligations(db: Db, storage?: StorageService | null): Promise<boolean> {
|
||||
const roots = await readdir(nativeHarnessStateBase()).catch((error: NodeJS.ErrnoException) => {
|
||||
if (error.code === "ENOENT") return [] as string[]; throw error;
|
||||
@@ -165,6 +190,8 @@ export async function hasLocalNativeControllerObligations(db: Db, storage?: Stor
|
||||
const present = await lstat(paths.controlFile).catch((error: NodeJS.ErrnoException) => {
|
||||
if (error.code === "ENOENT") return undefined; throw error;
|
||||
});
|
||||
if (!present && source.status === "failed" && source.runnerProfileJson.sessionCheckpoint != null
|
||||
&& await preservedDatabaseOnlyFailedCheckpoint(selected, source, await loadNativeRunnerCheckpoint(db, binding))) continue;
|
||||
if (present && source.status === "failed") {
|
||||
const archiveStorage = storage === undefined ? durableNativeHarnessStorage() : storage;
|
||||
const controller = await readPrivateSource(paths.controlFile, 256 * 1024 * 1024);
|
||||
@@ -234,6 +261,7 @@ export async function migrateNativeControllers(db: Db, input: { writersStopped:
|
||||
const binding = { companyId: selected.companyId, issueId: selected.issueId, runId: source.id };
|
||||
const checkpoint = await loadNativeRunnerCheckpoint(db, binding);
|
||||
if (preservedFailedCheckpoint(source, selected.sessionId, checkpoint)) { counts.preservedFailed++; continue; }
|
||||
if (await preservedDatabaseOnlyFailedCheckpoint(selected, source, checkpoint)) { counts.preservedFailed++; continue; }
|
||||
if (hasMatchingControllerRecovery(selected, source, checkpoint)) { counts.unchanged++; continue; }
|
||||
const { paths, legacyControllerState, legacyRunnerArtifact, supervisor } = await readMigrationMaterial(db, selected, source, checkpoint);
|
||||
if (!checkpoint && legacyControllerState === undefined) { counts.sourceAlreadyAbsent++; continue; }
|
||||
|
||||
Reference in new issue
Block a user