From 7435abcc63355e504f9f89ac78e1c66eaf5b12db Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 10 Nov 2026 19:39:42 -0600 Subject: [PATCH] fix(native): wait out a run-event append before saving provider-pack admission (#20682) 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 native OpenCode run in a remote sandbox records a provider-pack admission receipt on its run row after the provider session opens. Artifact retention then requires that receipt. > - `recordNativeProviderPackAdmission` locked the run row with `FOR UPDATE NOWAIT`. > - A run-event append locks the same run row for a few milliseconds (`FOR NO KEY UPDATE` since #20670, `FOR UPDATE` before). Both conflict with `FOR UPDATE`. Startup appends many events at the same moment. > - When the two met, the admission write failed with `55P03`. The admission state stayed `pending`. Artifact retention then failed the run with `native_provider_artifacts_admission_unverified`. The task stopped as "Recovery needed" with no Retry. > - This pull request makes the admission write wait for the run-row lock within the existing 1000 ms `lock_timeout`. The coordinator claim keeps its `NOWAIT` fence. > - The benefit is that an ordinary concurrent event append no longer fails a healthy run before its first turn. ## Linked Issues or Issue Description Refs #20670 (changed the append lock mode; the conflict with `FOR UPDATE NOWAIT` remains). **What happened?** On a hosted staging instance, three OpenCode runs on remote sandboxes failed in the first minute with `native_provider_artifacts_admission_unverified`. Each run log shows `native provider-pack admission receipt could not be saved` about 0.2–0.7 s after the runner authenticated. The run profile kept `nativeProviderPackAdmissionState.state = "pending"`. The tasks stayed blocked. **Steps to reproduce** 1. Start a native run and hold its `heartbeat_runs` row lock in another transaction, as a run-event append does. 2. Call `recordNativeProviderPackAdmission` for that run. 3. Observe `could not obtain lock on row in relation "heartbeat_runs"` (`55P03`). **Expected behavior** The admission waits a short, bounded time for the append to commit, then saves the receipt. ## What Changed - `recordNativeProviderPackAdmission` sets `lock_timeout = 1000ms` for every call, not only `waitForClaim` calls. - The run-row lock is now `FOR UPDATE` without `NOWAIT`, so it waits for a concurrent append within that bound. - The coordinator (`native_run_finalizations`) lock keeps `NOWAIT` when the caller does not wait for the claim. The existing fence tests still pass. - New PostgreSQL test: `waits for a concurrent run-event append's run-row lock instead of losing the admission` (both `waitForClaim` modes). ## Verification - RED on `origin/master` (`c444da7d05`): the new test fails with `55P03` in both modes. - GREEN with this change: `pnpm --filter @paperclipai/server exec vitest run src/services/native-runtime/native-provider-pack-admission.test.ts` (37 passed). - `pnpm --filter @paperclipai/server typecheck` passes. - `pnpm verify`: see the result below. ## Risks - Low risk. The normal path has no contention and does not wait. Under contention the admission waits at most 1000 ms, before any provider turn. - A lock wait between the coordinator row and the run row can now meet a deadlock. PostgreSQL detects it and raises an error. That error takes the same caught path as the earlier `55P03`, so the result is no worse than before. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edits, read-only staging API reads). ## 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 - [ ] 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 --- .../native-provider-pack-admission.test.ts | 31 +++++++++++++++++++ .../native-provider-pack-admission.ts | 7 +++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/server/src/services/native-runtime/native-provider-pack-admission.test.ts b/server/src/services/native-runtime/native-provider-pack-admission.test.ts index 5d5af36841e..8ac60525f12 100644 --- a/server/src/services/native-runtime/native-provider-pack-admission.test.ts +++ b/server/src/services/native-runtime/native-provider-pack-admission.test.ts @@ -226,6 +226,37 @@ describe("provider pack admission PostgreSQL fencing", () => { } finally { release(); await owner; } await expect(recordNativeProviderPackAdmission(db, f)).resolves.toBe(true); }); + it.each([false, true])("waits for a concurrent run-event append's run-row lock instead of losing the admission (waitForClaim: %s)", async waitForClaim => { + // Staging failure: a run-event append locks the run row + // (FOR NO KEY UPDATE, as appendHeartbeatRunEvent does) for milliseconds. A + // NOWAIT admission write failed, left the state pending, and artifact + // retention then failed the run as native_provider_artifacts_admission_unverified. + const f = await fixture(); + let release!: () => void, acquired!: () => void; + const holding = new Promise(resolve => { acquired = resolve; }); + const gate = new Promise(resolve => { release = resolve; }); + const append = db.transaction(async tx => { + await tx.select({ id: heartbeatRuns.id }).from(heartbeatRuns).where(eq(heartbeatRuns.id, f.identity.runId)).for("no key update"); + acquired(); await gate; + }); + await holding; + let settled = false; + const recording = recordNativeProviderPackAdmission(db, { ...f, waitForClaim } as Parameters[1]) + .then(value => { settled = true; return { value }; }, error => { settled = true; return { error }; }); + try { + // Release only after the admission is proven to wait on the run row, so + // the old NOWAIT code cannot pass by running after the append commits. + await vi.waitFor(async () => { + const [row] = await db.execute<{ waiting: boolean }>(sql`select exists(select 1 from pg_stat_activity + where datname = current_database() and wait_event_type = 'Lock' and state = 'active' + and query like '%heartbeat_runs%') as waiting`); + expect(settled || row.waiting).toBe(true); + }, { timeout: 800 }); + } finally { release(); await append; } + expect(await recording).toEqual({ value: true }); + expect((await read(f.identity.runId)).runnerProfileJson?.nativeProviderPackAdmissionState) + .toEqual({ ...f.profile.nativeProviderPackAdmissionState, state: "verified" }); + }); it.each([false, true])("waits within the durable-capture bound and rechecks the claim after its lock (revoked: %s)", async revoked => { const f = await fixture(); let release!: () => void, acquired!: () => void; diff --git a/server/src/services/native-runtime/native-provider-pack-admission.ts b/server/src/services/native-runtime/native-provider-pack-admission.ts index 8476f852b10..d815e427f63 100644 --- a/server/src/services/native-runtime/native-provider-pack-admission.ts +++ b/server/src/services/native-runtime/native-provider-pack-admission.ts @@ -132,8 +132,11 @@ export async function recordNativeProviderPackAdmission(db: Db, input: { }): Promise { const receipt = admittedNativeProviderPack(input.session, input.identity); return db.transaction(async tx => { + // Bounds every lock wait below, including the run-row lock. A run-event + // append holds that row FOR UPDATE for milliseconds; NOWAIT there lost the + // admission and then failed the run as admission_unverified. + await tx.execute(sql`select set_config('lock_timeout', '1000ms', true)`); if (input.waitForClaim) { - await tx.execute(sql`select set_config('lock_timeout', '1000ms', true)`); await assertNativeRunnerCheckpointOwner(tx, { companyId: input.identity.companyId, issueId: input.identity.issueId, runId: input.identity.runId, authorityRunId: input.identity.runId, bootId: input.claim.controllerBootId, leaseOwner: input.claim.leaseOwner, @@ -151,7 +154,7 @@ export async function recordNativeProviderPackAdmission(db: Db, input: { eq(heartbeatRuns.id, input.identity.runId), eq(heartbeatRuns.companyId, input.identity.companyId), eq(heartbeatRuns.agentId, input.identity.agentId), eq(heartbeatRuns.nativeIssueId, input.identity.issueId), eq(heartbeatRuns.runtimeMode, "native"), eq(heartbeatRuns.status, "running"), - )).for("update", { noWait: true }).limit(1); + )).for("update").limit(1); if (!run) return false; const current = run.profile?.nativeProviderPackAdmissionState as Record | undefined; if (current?.schema !== "paperclip.native-provider-pack-admission-state.v1" || current.admissionEpoch !== input.admissionEpoch