From 925965a09b8931c1791f2402f3e7595e2845e68e Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 3 Nov 2026 18:05:40 -0800 Subject: [PATCH] fix(task-retry): wait out brief chat authorization contention (#18895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A user can click Retry on a task that is connected to an external chat conversation. > - That Retry prepares the chat retry inside one admission transaction. The chat step takes a per-principal authorization lock with `pg_try_advisory_xact_lock`. > - When another chat transaction holds that lock for a short time, the chat step throws `NativeChatReviewPresentationContentionError`. This error is not an `HttpError`. The route returns HTTP 500, and the error handler reports a server crash. > - The contention is routine and short. The admission transaction does not contact a provider, and it rolls back completely. The owning service can safely try it again. > - This pull request retries the admission a bounded number of times. If the lock stays busy, the service returns a retryable 409 instead of a 500. > - The benefit: the user's click usually succeeds without an error, and Paperclip stops reporting expected lock contention as a crash. ## Linked Issues or Issue Description **What happened?** A hosted staging server reported `NativeChatReviewPresentationContentionError: Chat response authorization is temporarily busy; no provider delivery was attempted` from `POST /api/issues/:id/retry`. The stack is `requestTaskRetry` → `writeExecutionControl` → `prepareFailedChatRunRetry` → `failedChatRetrySource`. The error came from the `chat-identity::` advisory try-lock. The route returned HTTP 500 for each occurrence. **Expected behavior** Short lock contention must not fail a deliberate Retry click. The service must wait it out. If the lock stays busy, the service must return a clear, retryable client error and must not report a server crash. **Steps to reproduce** 1. Bind a task to a restricted chat endpoint and conversation. Make the latest task run a failed run. 2. In a different database transaction, hold `pg_try_advisory_xact_lock(hashtextextended('chat-identity::', 0))`. 3. Call `POST /api/issues/:id/retry`. Before this change, the chat retry step throws `NativeChatReviewPresentationContentionError` and the route returns 500. **Paperclip version or commit** Reproduced on master `8771606fd87f69ed4057779e3dce3a9bca8088e6`. The server release that reported it was `d59791828482f213713ca55000c40e6c9c6c03e5`. **Deployment mode** Hosted staging, Node 24, PostgreSQL. Related: #18845 and #18621 changed nearby Retry admission. They do not handle this contention. ## What Changed - `server/src/services/task-retry.ts`: `requestTaskRetry` now calls the unchanged admission transaction (`admitTaskRetry`) in a bounded loop: 10 attempts, 100 ms apart. It retries only `NativeChatReviewPresentationContentionError` and lock-not-available contention (`isExternalChatWaitAuthorizationContention`, which matches `55P03`). Every other error, including 403/404/409 decisions, propagates unchanged on the first attempt. - If the lock stays busy for all attempts, the service throws `conflict(..., { code: "task_retry_busy", retryable: true })`. The UI's existing 4xx handling resets the Retry button, and the error handler does not report a 4xx as a crash. - `server/src/services/task-retry.test.ts`: three PostgreSQL cases: - Real advisory-lock contention. A second transaction holds the same `chat-identity` lock that production checks, then releases it. Retry succeeds and persists exactly one wake request. A replay with the same idempotency key returns the same request. - Persistent contention returns 409 `task_retry_busy` after exactly 10 attempts and persists no wake request. - A 403 from the chat step does not retry. The transaction body is unchanged. It was moved into `admitTaskRetry` without edits. ## Verification - RED: with the old `task-retry.ts` and the new tests, the contention case fails with the exact production error (`NativeChatReviewPresentationContentionError: Chat response authorization is temporarily busy…`). The persistent case fails because the error is not an `HttpError`. - GREEN: `pnpm exec vitest run src/services/task-retry.test.ts` passes all 39 cases against embedded PostgreSQL. - `pnpm --filter @paperclipai/server typecheck` passes (all layers, including tests). - `src/services/heartbeat/workspace-preparation.test.ts` (another `requestTaskRetry` caller): 165 of 167 cases pass. The 2 failures (`remote_workspace_migration_source_changed` in "resumes an imported path/file-url Git source…") also fail on unchanged master in the same environment, so this change does not cause them. - Runtime assumption (rule `runtime-dependency-boundaries`): the retry is safe only if a failed attempt leaves no persisted state. The contention test uses a real PostgreSQL advisory lock held by a separate connection and checks that only one wake row exists after the failed attempts. Each attempt is a separate transaction, so the loop holds no row lock or advisory lock while it waits. - The full repository `pnpm test:run` and `pnpm build` were not run locally. I rely on CI for those. ## Risks - Low risk. The worst case adds about 1 second of request time when the lock stays busy, and then returns 409 instead of 500. - An attempt can fail after an earlier step in the same transaction (for example, workspace preservation registration). The whole transaction rolls back, so the next attempt repeats those steps from a clean state. Those steps run in the same transaction and do not contact a provider. - If `requestTaskRetry` is called inside an outer transaction, each attempt rolls back only to its savepoint. Locks that the outer transaction holds stay held. In that case the loop ends with the same 409 after its bounded budget. - No schema, API shape, or UI change. ## Model Used Claude Opus 5.5 (`claude-opus-5-5`, Anthropic), through Claude Code with tool use (shell, file editing, test execution) and extended reasoning. ## 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 (focused suites; see Verification for the two pre-existing failures) - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes (no behavior or command documentation changes are needed) - [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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- server/src/services/task-retry.test.ts | 73 +++++++++++++++++++++++++- server/src/services/task-retry.ts | 35 ++++++++++-- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/server/src/services/task-retry.test.ts b/server/src/services/task-retry.test.ts index 363663258be..a05fd80146c 100644 --- a/server/src/services/task-retry.test.ts +++ b/server/src/services/task-retry.test.ts @@ -1,10 +1,12 @@ import { randomUUID } from "node:crypto"; import { and, eq, sql } from "drizzle-orm"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { agents, agentWakeupRequests, companies, createDb, environmentLeases, environments, executionWorkspaces, heartbeatRuns, issueComments, issues, issueRecoveryActions, nativeRunFinalizations, toolApplications, toolConnections, toolInvocations, applyPendingMigrations } from "@paperclipai/db"; +import { agents, agentWakeupRequests, chatConversations, chatEndpoints, companies, createDb, type Db, environmentLeases, environments, executionWorkspaces, heartbeatRuns, issueComments, issues, issueRecoveryActions, nativeRunFinalizations, toolApplications, toolConnections, toolInvocations, applyPendingMigrations } from "@paperclipai/db"; import { startEmbeddedPostgresTestDatabase } from "../__tests__/helpers/embedded-postgres.js"; import { priorRecoveryApiReceipt, unresolvedRecoveryToolInvocation } from "./recovery-effect-receipts.js"; import { requestTaskRetry } from "./task-retry.js"; +import { HttpError } from "../errors.js"; +import { NativeChatReviewPresentationContentionError } from "./native-runtime/native-chat-review-presentation.js"; import { readFreshWorkspaceReadiness } from "./fresh-workspace-continuation.js"; import { settleUnrecoverableExecutions } from "./execution-recovery-resolution.js"; import { createSystemRecovery, failSystemRecovery, readSystemRecovery, SystemRecoveryFailure } from "./system-recovery-state.js"; @@ -441,4 +443,73 @@ describe("durable task Retry admission", () => { .toMatchObject({ status: "skipped", error: "task_retry_superseded" }); }); + async function seedChatTask() { + const input = await seed(); + const [issue] = await db.select().from(issues).where(eq(issues.id, input.issueId)); + const [application] = await db.insert(toolApplications).values({ companyId: input.companyId, name: "Slack", type: "chat" }).returning(); + const [connection] = await db.insert(toolConnections).values({ companyId: input.companyId, applicationId: application.id, + name: "Slack", uid: randomUUID(), transport: "chat_sdk", connectionPurpose: "channel" }).returning(); + const [endpoint] = await db.insert(chatEndpoints).values({ companyId: input.companyId, connectionId: connection.id, + provider: "slack", publicId: randomUUID(), assignedAgentId: issue.assigneeAgentId!, status: "active" }).returning(); + await db.insert(chatConversations).values({ companyId: input.companyId, endpointId: endpoint.id, issueId: input.issueId, + externalConversationId: "channel", externalThreadId: randomUUID(), externalLabel: "#retry", state: "waiting" }); + return input; + } + const chatIdentityLock = (companyId: string, principalId: string) => + sql`pg_try_advisory_xact_lock(hashtextextended(${`chat-identity:${companyId}:${principalId}`}, 0))`; + + it("waits out brief chat authorization contention without a second request", async () => { + const input = await seedChatTask(); + const principalId = randomUUID(), actionId = randomUUID(); + let calls = 0, locked!: () => void, release!: () => void; + const held = new Promise(resolve => { locked = resolve; }); + const mayRelease = new Promise(resolve => { release = resolve; }); + // Another chat transaction owns this principal's authorization lock. + const holder = db.transaction(async tx => { + expect((await tx.execute<{ locked: boolean }>(sql`select ${chatIdentityLock(input.companyId, principalId)} as locked`))[0].locked).toBe(true); + locked(); await mayRelease; + }); + await held; + const prepareChatRetry = async (tx: Db) => { + calls += 1; + if (calls === 2) release(); + // Mirror the production authorization check, including its rollback. + const [row] = await tx.execute<{ locked: boolean }>(sql`select ${chatIdentityLock(input.companyId, principalId)} as locked`); + if (!row?.locked) throw new NativeChatReviewPresentationContentionError(); + return { actionId }; + }; + try { + const receipt = await requestTaskRetry({ ...input, prepareChatRetry }); + expect(receipt).toMatchObject({ accepted: true, chatActionId: actionId }); + expect(calls).toBeGreaterThan(1); + const rows = await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.companyId, input.companyId)); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ id: receipt.requestId, status: "deferred_issue_execution", + payload: { taskRetry: { chatActionId: actionId } } }); + expect((await requestTaskRetry({ ...input, prepareChatRetry })).requestId).toBe(receipt.requestId); + } finally { release(); await holder; } + }); + + it("returns a retryable conflict when chat authorization stays busy", async () => { + const input = await seedChatTask(); + let calls = 0; + const failure = await requestTaskRetry({ ...input, prepareChatRetry: async () => { + calls += 1; + throw new NativeChatReviewPresentationContentionError(); + } }).catch(error => error); + expect(failure).toBeInstanceOf(HttpError); + expect(failure).toMatchObject({ status: 409, details: { code: "task_retry_busy", retryable: true } }); + expect(calls).toBe(10); + expect(await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.companyId, input.companyId))).toHaveLength(0); + }); + + it("does not retry an authorization denial", async () => { + const input = await seedChatTask(); + let calls = 0; + await expect(requestTaskRetry({ ...input, prepareChatRetry: async () => { + calls += 1; + throw new HttpError(403, "denied"); + } })).rejects.toMatchObject({ status: 403 }); + expect(calls).toBe(1); + }); }); diff --git a/server/src/services/task-retry.ts b/server/src/services/task-retry.ts index 31180616cbb..930e753529c 100644 --- a/server/src/services/task-retry.ts +++ b/server/src/services/task-retry.ts @@ -21,6 +21,8 @@ import { deferredChatInputPredicate, resumeDeferredChatWake } from "./durable-ch import { notifyDeferredExecutionWork } from "./deferred-execution-notifications.js"; import { hasPendingRunCleanup } from "./execution-blocker.js"; import { PersistedExecutionWaitRefusal } from "./deferred-execution-wait.js"; +import { NativeChatReviewPresentationContentionError } from "./native-runtime/native-chat-review-presentation.js"; +import { isExternalChatWaitAuthorizationContention } from "./native-runtime/chat-attachment-reuse.js"; export async function taskRetryMutationAllowed(db: Db, input: { companyId: string; issueId: string; actorId: string; actorSource: TaskRetryIntent["actorSource"]; @@ -206,14 +208,39 @@ export async function retireSupersededTaskRetry(db: Db, input: { return retired.length > 0; } -/** Persist a deliberate click under dispatch's issue-first lock order. This - * transaction never contacts a provider. The normal wake queue owns execution. */ -export async function requestTaskRetry(input: { +type TaskRetryRequest = { db: Db; companyId: string; issueId: string; actorId: string; idempotencyKey: string; observedRunId: string | null; actorSource?: string; prepareChatRetry?: (tx: Db, scope: { companyId: string; issueId: string; agentId: string; failedRunId: string; initiatedByUserId: string; retryIntentId?: string }) => Promise<{ actionId: string }>; -}) { +}; + +const TASK_RETRY_CONTENTION_ATTEMPTS = 10; +const TASK_RETRY_CONTENTION_DELAY_MS = 100; + +function isTaskRetryContention(error: unknown) { + return error instanceof NativeChatReviewPresentationContentionError || isExternalChatWaitAuthorizationContention(error); +} + +/** Persist a deliberate click under dispatch's issue-first lock order. This + * transaction never contacts a provider. The normal wake queue owns execution. */ +export async function requestTaskRetry(input: TaskRetryRequest) { + for (let attempt = 1; ; attempt += 1) { + try { + return await admitTaskRetry(input); + } catch (error) { + if (!isTaskRetryContention(error)) throw error; + // Chat authorization for the same principal is briefly locked. The whole + // admission rolled back, so a fresh attempt rereads every fact. + if (attempt >= TASK_RETRY_CONTENTION_ATTEMPTS) + throw conflict("Retry is busy because chat authorization is in progress. Try again.", + { code: "task_retry_busy", retryable: true }); + } + await new Promise(resolve => setTimeout(resolve, TASK_RETRY_CONTENTION_DELAY_MS)); + } +} + +function admitTaskRetry(input: TaskRetryRequest) { return writeExecutionControl(input.db, DELIVERY_QUEUES.workspaceRecovery, async tx => { const db = tx as unknown as Db; const [issue] = await tx.select().from(issues).where(and(eq(issues.companyId, input.companyId),