mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 22:13:59 +01:00
fix(retry): wait for admitted source cleanup before dispatch (#18898)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Connected-task Retry keeps the original request and its current authority. > - One durable Retry can already own a queued successor. > - The old native environment can still have pending cleanup before that successor starts. > - The task execution lock points to the successor, so the old cleanup can escape the queue claim check. > - Dispatch then refuses the source state and fails the new run before provider work. > - This pull request keeps that same run queued and wakes dispatch when cleanup changes. ## Linked Issues or Issue Description Refs #18845, #18853, #18860, #18877, #18882 and #18932. These repairs cover preservation binding, interrupted preparation, old native ownership, manual native holds or other setup guards. This change covers an already admitted connected-task Retry whose native predecessor still owes cleanup. It uses the shared cleanup predicate so the manual-hold repair remains with its existing owner. Search found no duplicate queued-source cleanup repair. **What happened?** A board Retry saved its exact chat request and admitted one queued successor. The source lease was still pending cleanup when that successor reached dispatch. A retained stop receipt did not settle the full cleanup contract. The queue checked the task's current execution owner, which was already the successor. The native authorization guard then rejected the predecessor state and failed the new run before provider dispatch. **Expected behavior** Keep the same canonical Retry and queued run while the exact source cleanup is pending. After authoritative settlement, submit that run through normal dispatch once. Do not infer safe execution from a stop-and-retain receipt alone. **Steps to reproduce** 1. Retry an authorized connected request from a failed native run. 2. Admit one successor while capacity keeps it queued. 3. Leave or restore the source lease to pending cleanup with a matching retained stop receipt. 4. Release capacity and resume the ordinary queue. 5. The old claim path starts the successor and the native source guard refuses it. **Paperclip version or commit** Reproduced against master at `f77499283270d2e966aabd58493f9d35c6bbdf5f`. **Deployment mode** The regression uses the real HTTP route, chat authorization, queue claims, disposable PostgreSQL and the production work coordinator on macOS arm64. A controlled adapter supplies the final result. No hosted provider replay or deployment is claimed. ## What Changed - Validate the exact canonical Retry, chat action, company, task, agent, source and successor before testing source cleanup. - Keep the successor queued under the existing issue claim lock while the shared cleanup predicate remains pending. - Wake only the scoped queued-dispatch worker after cleanup or exact workspace-owner release commits. Add no timer or background scan. - Verify pending cleanup at intake, later cleanup invalidation, one final execution, idempotency, Stop, changed input, unknown stop receipts, a different live controller and signed sandbox absence followed by owner release. - Document the queued-wait contract. No schema, endpoint, credential, model or provider policy changes are required. - Align five native preparation acceptance fixtures with the existing workspace publication contract. Use the real reservation and publication owner. Retain a raw-root refusal case with no provider launch and preserved files. ## Verification - Latest master sync at `e26cad76b8f20ba823d702ed60e7b7294f8f5a7c` includes master `396b7fc62a0a4b1d8aadbda8221c05879d54177c`. The shared test helper preserves #18944's canonical retained-directory default, the real publisher for the five fresh-workspace cases, and the explicit raw-root refusal. Independent remerge review found no dropped upstream behavior, guard or assertion. - The latest head passes the focused six cases, all 580 complete workspace/continuation tests, all 38 selected HTTP Retry cases, fresh full recursive Node 24.22.2 types and full build. No affected tests skipped. An initial cached test-layer attempt read old budget declarations; regenerating them resolved all seven errors without test/mock or production-export changes. - All current-head CI is green at `e26cad76b8f20ba823d702ed60e7b7294f8f5a7c`, including required [ci / verify](https://github.com/paperclipai/paperclip/actions/runs/41846304017/job/126636751314) and [ci / e2e](https://github.com/paperclipai/paperclip/actions/runs/41846304017/job/126636638086). Greptile and Paperclip Review both rate this exact head 5/5, with no findings or open review threads. - Master sync at `3e4f266c2da172a84afd0b9cb4a91ea3b95f3c0d` merges master `b891e69bd23d6dab8ce536569e15bc4a7a31c35c`. The only conflicts were adjacent queue imports and two HTTP Retry test matrices. Both imports and both complete matrices remain unchanged. Independent review found no relaxed guard or assertion. - Node 24.22.2 and pnpm 9.15.4: all 38 selected real HTTP Retry cases, 194 queue/cleanup/finalizer cases, module boundaries, recursive types and build pass at `3e4f266c2da172a84afd0b9cb4a91ea3b95f3c0d`. - That sync head's [server check](https://github.com/paperclipai/paperclip/actions/runs/41838741852/job/126604742761) failed five native acceptance cases in CI and locally with `remote_workspace_published_path_invalid`. The test fixture supplied a raw directory instead of a fresh publisher-generated workspace. The guard from #18932 remains unchanged. The same five cases passed earlier Linux CI before that guard was present. - The test-only alignment at `dcdee1bb6c2e2e6dd494382499d17c23a3ce71a8` obtains the saved authority, path and run reference from real `reserveRemoteWorkspace` and `prepareRemoteWorkspace`. All five original cases and a raw-root no-launch/no-result/files-preserved control pass. Their native result, file, catalog and single-execution assertions remain intact. An initial setup attempt was refused for a noncanonical allocation root; the fixture now resolves that owned root before publication. - At previous fixture head `dcdee1bb6c2e2e6dd494382499d17c23a3ce71a8`, the generated server test-layer typecheck passed. All 580 tests in the complete workspace/continuation family pass, with no skips. That test-only commit left production files byte-identical to the sync head. - Previous published-head CI was green at `dcdee1bb6c2e2e6dd494382499d17c23a3ce71a8`, including required `ci / verify` and `ci / e2e`. The [Linux Node 24.22.2 server shard](https://github.com/paperclipai/paperclip/actions/runs/41842665765/job/126618334638) executed all five original acceptance cases and the new raw-root refusal case successfully. Its whole workspace recovery file passed all 245 tests. - Greptile reviewed previous published head `dcdee1bb6c2e2e6dd494382499d17c23a3ce71a8` at 5/5, with no findings or open review threads. No unchanged-job rerun was used for the reproduced sync-head failure. - Causal red: the real HTTP regression started and failed the successor at the native source-state guard while cleanup remained pending. A second red proved that signed sandbox absence followed by copyback-owner release lacked a queued-dispatch wake. - Green: seven HTTP/PostgreSQL cleanup cases, the exact confirmed-owner release notification control and seven existing retry lineage controls pass. The real coordinator resumes from committed cleanup or workspace-owner release, without a manual queue restart. One controlled adapter execution produces the result after settlement. - Earlier branch verification passed full repository `pnpm -r typecheck`, `pnpm build`, module boundaries and `git diff --check`. - The full stale-queue file control passed all 64 cases with unchanged observer/hook limits. The initial broader affected run had 192 passes and one non-assignee native-owner case miss its existing five-second adapter observer while already running with no error. The isolated case passed; the unchanged base passed all 64 cases but hit its existing 30-second teardown timeout. Those original results are retained. The new queue hint has no dispatch consumer in that fixture. - Earlier-head CI was green: 54 success, three skipped and two neutral checks at `6adb9f1ba682282c512d5e751ba2aa663f614ca2`. One chat job was cancelled by an external runner shutdown; its single unchanged-job rerun passed. No source was changed to obtain that result. - Greptile and Paperclip Review both rated earlier head `6adb9f1ba682282c512d5e751ba2aa663f614ca2` 5/5 with no findings or unresolved threads. These results do not qualify the master-sync head. - At initial verification, a complete local `pnpm test:run` was not repeated while the related setup repair's full local run was active. The full earlier-head CI suites passed. A full local suite has not been repeated for the mechanical master sync. Local verification uses the affected HTTP, producer, lineage and queue suites. No full local-suite pass is claimed. - The fresh clone's missing plugin SDK links were repaired with the repository's own postinstall helper before verification. The embedded PostgreSQL package's own symlink helper was also required. - No staging task was submitted again with this repair, and no deployment is claimed. ## Risks - A pending source cleanup can keep the queued run waiting. Its cleanup owner still owns reconciliation and its existing limits. - Stop, changed source, unknown termination and live ownership retain their existing authority checks. A cleanup notification grants no execution authority. - A new cleanup change after claim still faces the existing final dispatch guard. - Cleanup notifications can change dispatch timing in a running controller. The real coordinator cases verify commit-bound wakeups; the broader current queue-file control passed all 64 cases. The earlier loaded-run and baseline teardown failures are retained in verification. ## Model Used OpenAI GPT-6 through Codex, with reasoning, code editing, tool use, PostgreSQL and browser verification. This session does not expose a more specific deployment 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
5d226ca08a
commit
84e9c8f364
10 files changed
+265
-20
No files matched your search
@@ -1,6 +1,7 @@
|
||||
import { notifyNativeResultCommit } from "../native-result-commit-notifications.js";
|
||||
import { notifyNativeWorkspaceFinalization, releaseNativeWorkspaceFinalization } from "../native-workspace-finalization-notifications.js";
|
||||
import { notifyNativeStatusReassessment } from "../native-status-reassessment-notifications.js";
|
||||
import { notifyQueuedRunDispatch } from "../run-dispatch-notifications.js";
|
||||
import { beginRunPhase } from "../execution-preparation-timing.js";
|
||||
import { logActivity } from "../activity-log.js";
|
||||
import { issueRecoveryActionService } from "../issue-recovery-actions.js";
|
||||
@@ -302,6 +303,7 @@ export async function withNativeWorkspaceFinalizationOwnership<T>(
|
||||
await notifyNativeResultCommit(tx, { companyId: input.companyId, runId: input.runId });
|
||||
await notifyNativeWorkspaceFinalization(tx, { companyId: input.companyId, runId: input.runId });
|
||||
await notifyNativeStatusReassessment(tx, { companyId: input.companyId, runId: input.runId });
|
||||
await notifyQueuedRunDispatch(tx, { companyId: input.companyId });
|
||||
await tx.update(heartbeatRuns).set({
|
||||
runnerProfileJson: sql`coalesce(${heartbeatRuns.runnerProfileJson}, '{}'::jsonb) - ${OWNER_KEY}`,
|
||||
}).where(owned);
|
||||
|
||||
@@ -219,12 +219,19 @@ describe("native workspace finalization recovery", () => {
|
||||
observed.push(db.select({ profile: heartbeatRuns.runnerProfileJson }).from(heartbeatRuns)
|
||||
.where(eq(heartbeatRuns.id, seed.runId)).then(rows => rows[0].profile));
|
||||
});
|
||||
const queued: Array<Promise<unknown>> = [];
|
||||
const stopQueued = subscribeDeliveryWork(db, DELIVERY_QUEUES.queuedRunDispatch, () => {
|
||||
queued.push(db.select({ profile: heartbeatRuns.runnerProfileJson }).from(heartbeatRuns)
|
||||
.where(eq(heartbeatRuns.id, seed.runId)).then(rows => rows[0].profile));
|
||||
});
|
||||
let receipt: Awaited<ReturnType<typeof resumeNativeWorkspaceAfterOwnerStop>>;
|
||||
try {
|
||||
receipt = await resumeNativeWorkspaceAfterOwnerStop(seed);
|
||||
await vi.waitFor(() => expect(observed).toHaveLength(1));
|
||||
expect(await Promise.all(observed)).toEqual([{ preserved: "workspace descriptor" }]);
|
||||
} finally { stop(); }
|
||||
await vi.waitFor(() => expect(queued).toHaveLength(1));
|
||||
expect(await Promise.all(queued)).toEqual([{ preserved: "workspace descriptor" }]);
|
||||
} finally { stop(); stopQueued(); }
|
||||
expect(receipt).toEqual({ runId: seed.runId, resultId: before.resultId, status: "queued" });
|
||||
const [after] = await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, seed.runId));
|
||||
expect(after).toEqual(before);
|
||||
|
||||
@@ -2,6 +2,7 @@ import { notifyNativeResultCommit } from "../native-result-commit-notifications.
|
||||
import { notifyNativeWorkspaceFinalization } from "../native-workspace-finalization-notifications.js";
|
||||
import { tryLockNativeWorkspaceFinalization } from "./native-workspace-finalization-ownership.js";
|
||||
import { notifyNativeStatusReassessment } from "../native-status-reassessment-notifications.js";
|
||||
import { notifyQueuedRunDispatch } from "../run-dispatch-notifications.js";
|
||||
import os from "node:os";
|
||||
import { and, eq, sql } from "drizzle-orm";
|
||||
import { heartbeatRuns, issues, issueRecoveryActions, nativeRunFinalizations, type Db } from "@paperclipai/db";
|
||||
@@ -71,6 +72,7 @@ export async function resumeNativeWorkspaceAfterOwnerStop(input: {
|
||||
await notifyNativeResultCommit(tx, { companyId: input.companyId, issueId: input.issueId, runId: input.runId });
|
||||
await notifyNativeWorkspaceFinalization(tx, { companyId: input.companyId, issueId: input.issueId, runId: input.runId });
|
||||
await notifyNativeStatusReassessment(tx, { companyId: input.companyId, issueId: input.issueId, runId: input.runId });
|
||||
await notifyQueuedRunDispatch(tx, { companyId: input.companyId });
|
||||
await tx.update(heartbeatRuns).set({
|
||||
runnerProfileJson: sql`${heartbeatRuns.runnerProfileJson} - ${OWNER_KEY}`, updatedAt: now,
|
||||
}).where(and(eq(heartbeatRuns.companyId, input.companyId), eq(heartbeatRuns.id, run.id),
|
||||
|
||||
Reference in new issue
Block a user