mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
fix(native): wait out a run-event append before saving provider-pack admission (#20682)
## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
6e524d8a69
commit
7435abcc63
2 files changed
+36
-2
No files matched your search
@@ -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<void>(resolve => { acquired = resolve; });
|
||||
const gate = new Promise<void>(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<typeof recordNativeProviderPackAdmission>[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;
|
||||
|
||||
@@ -132,8 +132,11 @@ export async function recordNativeProviderPackAdmission(db: Db, input: {
|
||||
}): Promise<boolean> {
|
||||
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<string, unknown> | undefined;
|
||||
if (current?.schema !== "paperclip.native-provider-pack-admission-state.v1" || current.admissionEpoch !== input.admissionEpoch
|
||||
|
||||
Reference in new issue
Block a user