From 714299d6f8daa28d5ab65d579eafdfe851f40bb8 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Tue, 25 Aug 2026 22:29:22 +0000 Subject: [PATCH 1/2] fix(heartbeat): scope the quota-recovery wake to the parked run's task (BLO-28992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `provider_quota_exhausted_recovered` was delivered unscoped. The documented agent behaviour on an unscoped wake is to call `inboxLite` and take the top actionable row, and that ranking collapses to a singleton per agent — so when a provider throttle parks N runs of one agent and capacity returns, all N are aimed at the same issue by construction, not by bad luck. Inbox size does not help: observed with both 84-row and 169-row inboxes. Two runs then share one checkout, which in BLO-28442 interleaved writes into `erasure/tracker.go` and transiently produced a file that would not compile. The parked run already knows its own issue via `contextSnapshot.issueId` (exposed as the generated `contextIssueId` column), so this needs no new persisted field and no migration. `finalizeAgentStatus` already had the driving run in `options.runId` but passed `runId: null` to the hook; passing it through is what makes the scope resolvable. Because the hook invokes `onSuccess` per caller even on its debounced and in-flight branches, each parked run runs its own closure and therefore re-delivers its own scope. Scope is resolved at wake time rather than park time and is dropped when it is no longer safe to resume — issue gone, terminal, or reassigned while parked — falling back to today's unscoped wake. The hook can take 60s+ to recover, and waking an agent onto an issue it no longer owns would be a new second-writer defect rather than a fix. No change to checkout, workspace mode, or lock ordering; independent of BLO-27858 as that issue requires. Co-Authored-By: Claude --- ...eartbeat-quota-recovery-wake-scope.test.ts | 315 ++++++++++++++++++ server/src/services/heartbeat.ts | 105 +++++- 2 files changed, 415 insertions(+), 5 deletions(-) create mode 100644 server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts diff --git a/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts b/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts new file mode 100644 index 000000000000..1a482ecb5aac --- /dev/null +++ b/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts @@ -0,0 +1,315 @@ +/** + * BLO-28992: `provider_quota_exhausted_recovered` must resume the parked run's + * own task instead of arriving unscoped. + * + * The defect these tests pin down is not a race. An unscoped recovery wake sends + * the agent down its documented `inboxLite` path, and that pick is deterministic + * for a given agent — it collapses to a single top row. So when a provider + * throttle parks N runs of one agent and capacity returns, all N are aimed at + * the *same* issue by construction, regardless of inbox size (observed with both + * 84- and 169-row inboxes). Two runs then share one checkout: in BLO-28442 that + * produced interleaved writes into `erasure/tracker.go` and a transiently + * uncompilable file. + * + * The fix carries each parked run's own `contextSnapshot.issueId` through to its + * recovery wake, so N recovering runs resume N different tasks and only + * genuinely task-less runs consult the inbox at all. + */ +import { randomUUID } from "node:crypto"; +import { and, eq, sql } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { + agents, + companies, + createDb, + heartbeatRuns, + issues, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { heartbeatService } from "../services/heartbeat.js"; +import { __resetQuotaExhaustedHookStateForTesting } from "../services/quota-exhausted-hook.js"; +import type { + PenstockAvailabilityGate, + PenstockAvailabilityGateResult, +} from "../services/penstock-availability-gate.js"; + +vi.mock("../telemetry.ts", () => ({ + getTelemetryClient: () => ({ track: vi.fn() }), +})); + +vi.mock("@paperclipai/shared/telemetry", async () => { + const actual = await vi.importActual( + "@paperclipai/shared/telemetry", + ); + return { ...actual, trackAgentFirstHeartbeat: vi.fn() }; +}); + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres quota-recovery wake-scope tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +/** Capacity is available again — the recovery wake must not be capacity-gated. */ +function allowingGate(): PenstockAvailabilityGate { + return { + async checkAdapter(): Promise { + return { allow: true }; + }, + _resetForTesting() {}, + }; +} + +describeEmbeddedPostgres("quota-recovery wake task scoping (BLO-28992)", () => { + let db: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-quota-recovery-wake-scope-"); + db = createDb(tempDb.connectionString); + }); + + beforeEach(() => { + // The hook's debounce/inFlight state is a module-level singleton; leaking it + // across tests would silently change which branch fires onSuccess. + __resetQuotaExhaustedHookStateForTesting(); + // `true` exits 0, so the hook reports recovery and runs onSuccess — the + // branch that issues the wake under test. + process.env.PAPERCLIP_QUOTA_HOOK_ALLOW_ENV = "1"; + process.env.PAPERCLIP_QUOTA_EXHAUSTED_CMD = "true"; + }); + + afterEach(async () => { + delete process.env.PAPERCLIP_QUOTA_HOOK_ALLOW_ENV; + delete process.env.PAPERCLIP_QUOTA_EXHAUSTED_CMD; + await db.execute(sql.raw(`TRUNCATE TABLE "companies" CASCADE`)); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function seedAgent(): Promise<{ companyId: string; agentId: string }> { + const companyId = randomUUID(); + const agentId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + // Run dispatch refuses to seed a run it cannot attribute to a user. The + // scoped and unscoped branches resolve that differently, so pin a company + // default and both reach the assertion rather than one failing on setup. + defaultResponsibleUserId: "test-responsible-user", + }); + await db.insert(agents).values({ + id: agentId, + companyId, + name: "ClaudeCoder", + role: "engineer", + status: "active", + adapterType: "claude_local", + adapterConfig: {}, + // The whole hazard requires an agent that can hold concurrent runs. + runtimeConfig: { heartbeat: { wakeOnDemand: true, maxConcurrentRuns: 3 } }, + permissions: {}, + }); + return { companyId, agentId }; + } + + async function seedIssue(input: { + companyId: string; + assigneeAgentId: string | null; + status?: string; + title?: string; + }): Promise { + const issueId = randomUUID(); + await db.insert(issues).values({ + id: issueId, + companyId: input.companyId, + title: input.title ?? "Parked work", + status: input.status ?? "in_progress", + priority: "high", + assigneeAgentId: input.assigneeAgentId, + }); + return issueId; + } + + /** + * A run that was live on `issueId` (or task-less when null) and is now being + * finalized because the provider threw a quota error. + */ + async function seedParkedRun(input: { + companyId: string; + agentId: string; + issueId?: string | null; + }): Promise { + const runId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: runId, + companyId: input.companyId, + agentId: input.agentId, + invocationSource: "automation", + triggerDetail: "system", + status: "failed", + errorCode: "provider_quota_exhausted", + startedAt: new Date(), + finishedAt: new Date(), + contextSnapshot: input.issueId + ? { issueId: input.issueId, taskId: input.issueId } + : {}, + }); + return runId; + } + + /** + * `runQuotaExhaustedHook` is invoked fire-and-forget from `finalizeAgentStatus`, + * so the wake it produces lands after that await resolves. Poll rather than + * sleep a fixed amount, so a slow host does not turn into a flaky assertion. + */ + async function waitForRecoveryRuns(agentId: string, expected: number) { + const deadline = Date.now() + 15_000; + let rows: Array<{ id: string; contextIssueId: string | null }> = []; + while (Date.now() < deadline) { + rows = await db + .select({ id: heartbeatRuns.id, contextIssueId: heartbeatRuns.contextIssueId }) + .from(heartbeatRuns) + .where( + and( + eq(heartbeatRuns.agentId, agentId), + eq(heartbeatRuns.contextWakeReason, "provider_quota_exhausted_recovered"), + ), + ); + if (rows.length >= expected) return rows; + await new Promise((resolve) => setTimeout(resolve, 100)); + } + return rows; + } + + it("re-delivers each parked run its OWN task id, so two recovering runs do not converge", async () => { + const { companyId, agentId } = await seedAgent(); + const issueA = await seedIssue({ companyId, assigneeAgentId: agentId, title: "Task A" }); + const issueB = await seedIssue({ companyId, assigneeAgentId: agentId, title: "Task B" }); + const runA = await seedParkedRun({ companyId, agentId, issueId: issueA }); + const runB = await seedParkedRun({ companyId, agentId, issueId: issueB }); + + const heartbeat = heartbeatService(db, { + penstockAvailabilityGate: allowingGate(), + skipQueuedRunDispatch: true, + }); + + // Both runs of the same agent die to the same throttle window. The second + // takes the hook's debounce branch, which fires *its own* onSuccess — that + // per-caller closure is what makes per-run scope possible at all. + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId: runA, + }); + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId: runB, + }); + + const recovered = await waitForRecoveryRuns(agentId, 2); + expect(recovered).toHaveLength(2); + + const scopes = recovered.map((row) => row.contextIssueId).sort(); + // The load-bearing assertion: two distinct scopes, not one row twice. + expect(scopes).toEqual([issueA, issueB].sort()); + expect(new Set(scopes).size).toBe(2); + // And neither is unscoped, which is what would send the agent to the inbox. + expect(scopes).not.toContain(null); + }); + + it("still wakes unscoped when the parked run had no task", async () => { + const { companyId, agentId } = await seedAgent(); + const runId = await seedParkedRun({ companyId, agentId, issueId: null }); + + const heartbeat = heartbeatService(db, { + penstockAvailabilityGate: allowingGate(), + skipQueuedRunDispatch: true, + }); + + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId, + }); + + const recovered = await waitForRecoveryRuns(agentId, 1); + expect(recovered).toHaveLength(1); + // Unchanged from today: a task-less park has nothing to resume, so the + // inbox path remains correct for it. + expect(recovered[0]?.contextIssueId).toBeNull(); + }); + + it("drops the scope when the issue reached a terminal status while parked", async () => { + const { companyId, agentId } = await seedAgent(); + const doneIssue = await seedIssue({ + companyId, + assigneeAgentId: agentId, + status: "done", + title: "Completed while parked", + }); + const runId = await seedParkedRun({ companyId, agentId, issueId: doneIssue }); + + const heartbeat = heartbeatService(db, { + penstockAvailabilityGate: allowingGate(), + skipQueuedRunDispatch: true, + }); + + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId, + }); + + const recovered = await waitForRecoveryRuns(agentId, 1); + expect(recovered).toHaveLength(1); + // Resuming onto a closed row would be a new defect, not a fix. Degrading to + // the previous unscoped behaviour is the safe outcome. + expect(recovered[0]?.contextIssueId).toBeNull(); + }); + + it("drops the scope when the issue was reassigned to another agent while parked", async () => { + const { companyId, agentId } = await seedAgent(); + const otherAgentId = randomUUID(); + await db.insert(agents).values({ + id: otherAgentId, + companyId, + name: "OtherCoder", + role: "engineer", + status: "active", + adapterType: "claude_local", + adapterConfig: {}, + runtimeConfig: { heartbeat: { wakeOnDemand: true, maxConcurrentRuns: 1 } }, + permissions: {}, + }); + const reassigned = await seedIssue({ + companyId, + assigneeAgentId: otherAgentId, + title: "Taken over while parked", + }); + const runId = await seedParkedRun({ companyId, agentId, issueId: reassigned }); + + const heartbeat = heartbeatService(db, { + penstockAvailabilityGate: allowingGate(), + skipQueuedRunDispatch: true, + }); + + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId, + }); + + const recovered = await waitForRecoveryRuns(agentId, 1); + expect(recovered).toHaveLength(1); + // Waking an agent onto work it no longer owns is exactly the second-writer + // shape this issue exists to remove. + expect(recovered[0]?.contextIssueId).toBeNull(); + }); +}); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 6ecc850e6a6a..3e073b144e83 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -1196,6 +1196,14 @@ export const RECOVERABLE_AGENT_STATUS_ERROR_CODES = [ "provider_quota", ] as const; +// BLO-28992: issue statuses whose scope must NOT be re-delivered on a +// quota-recovery wake. `done`/`cancelled` are terminal — resuming a run onto +// one manufactures work on a closed row. Every other status (including +// `blocked`) is deliberately resumable: the parked run may be exactly what is +// needed to move it, and a `blocked` row with no blocker edges is a dispatch +// stop we should not deepen (BLO-21523). +const QUOTA_RECOVERY_UNRESUMABLE_ISSUE_STATUSES = new Set(["done", "cancelled"]); + export function readHeartbeatRunErrorFamily( run: Pick, ) { @@ -19616,6 +19624,72 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) return parts.join("; "); } + /** + * BLO-28992: resolve the task scope a quota-parked run should be re-woken + * with, so `provider_quota_exhausted_recovered` stops arriving unscoped. + * + * Why this exists: the recovery wake used to carry no issue at all, and the + * documented agent behaviour on an unscoped wake is to call `inboxLite` and + * take the top actionable row. That ranking collapses to a *singleton* for a + * given agent, so it is not a probabilistic collision — when a throttle parks + * N runs of one agent and capacity returns, every one of them is aimed at the + * same issue by construction. Observed with a 169-row and an 84-row inbox; + * inbox size does not help because the pick is deterministic. Two runs then + * share one workspace (BLO-28442 interleaved writes into `erasure/tracker.go` + * and transiently produced a file that would not compile). + * + * Each parked run already knows its own issue — `contextSnapshot.issueId`, + * exposed as the generated `contextIssueId` column — so no new persisted + * field and no migration is needed. Note the hook fires `onSuccess` per + * caller even on its debounced/in-flight branches, so each parked run runs + * *its own* closure and therefore re-delivers *its own* scope. + * + * Returns null (leaving the wake unscoped, exactly as before) when the run + * had no task, or when the scope is no longer safe to resume. That last check + * is the load-bearing one: the hook can take 60s+ to recover, and in that + * window the issue may have been completed, cancelled, or reassigned. Waking + * an agent onto an issue it no longer owns would be a new defect, not a fix, + * so a stale scope degrades to today's unscoped behaviour rather than being + * carried blindly. + */ + async function resolveQuotaRecoveryWakeIssueId( + runId: string | null, + agentId: string, + ): Promise { + if (!runId) return null; + const parkedRun = await getRun(runId).catch(() => null); + if (!parkedRun) return null; + const issueId = issueIdFromRunContext(parkedRun.contextSnapshot); + if (!issueId) return null; + + const issue = await db + .select({ status: issues.status, assigneeAgentId: issues.assigneeAgentId }) + .from(issues) + .where(eq(issues.id, issueId)) + .limit(1) + .then((rows) => rows[0] ?? null) + .catch(() => null); + + // Unreadable or vanished issue: fall back to unscoped rather than pinning + // the recovered run to an id we could not verify. + if (!issue) return null; + if (QUOTA_RECOVERY_UNRESUMABLE_ISSUE_STATUSES.has(issue.status)) { + logger.info( + { agentId, runId, issueId, issueStatus: issue.status }, + "quota recovery wake: dropping task scope, issue reached a terminal status while parked (BLO-28992)", + ); + return null; + } + if (issue.assigneeAgentId !== agentId) { + logger.info( + { agentId, runId, issueId, assigneeAgentId: issue.assigneeAgentId }, + "quota recovery wake: dropping task scope, issue was reassigned while parked (BLO-28992)", + ); + return null; + } + return issueId; + } + /** * Derive and persist the agent's status after a run reached a terminal state. * @@ -19787,28 +19861,43 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) if (recoverable) { const hookAgentId = existing.id; const hookCompanyId = existing.companyId; + // BLO-28992: the run that drove this transition is the one being parked, + // so its id is what lets the recovery wake resume the same task. This was + // previously hardcoded `null` even though `options.runId` was already in + // scope, which is why the recovery wake had nothing to scope itself with. + const hookRunId = options?.runId ?? null; void runQuotaExhaustedHook({ db, agentId: hookAgentId, companyId: hookCompanyId, - runId: null, + runId: hookRunId, adapterType: existing.adapterType, errorCode: options?.errorCode ?? "provider_quota_exhausted", - onSuccess: () => - enqueueWakeup(hookAgentId, { + onSuccess: async () => { + // Resolve scope at wake time, not park time: the issue's state can + // change while the hook runs, and this is where we can still see it. + const resumeIssueId = await resolveQuotaRecoveryWakeIssueId( + hookRunId, + hookAgentId, + ).catch(() => null); + return enqueueWakeup(hookAgentId, { source: "automation", triggerDetail: "system", reason: "provider_quota_exhausted_recovered", requestedByActorType: "system", requestedByActorId: "quota-exhausted-hook", + // Omitted entirely when there is no resumable scope, so a run that + // parked without a task still wakes unscoped exactly as before. + ...(resumeIssueId ? { contextSnapshot: { issueId: resumeIssueId } } : {}), }) .then(() => undefined) .catch((err) => { logger.warn( - { err, agentId: hookAgentId }, + { err, agentId: hookAgentId, issueId: resumeIssueId ?? null }, "failed to wake agent after quota-exhausted hook", ); - }), + }); + }, }).catch((err) => { logger.warn( { err, agentId: hookAgentId }, @@ -33751,6 +33840,12 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) enqueueWakeup, reconcileDetachedQueuedRuns, + // BLO-28992: exposed so the quota-recovery wake's task-scoping behaviour can + // be driven directly. The alternative is finalizing a run through a full + // adapter dispatch, which cannot pin down which parked run's scope was + // re-delivered — the exact property under test. + finalizeAgentStatus, + buildIssueGraphLivenessAutoRecoveryPreview, reconcileIssueGraphLiveness, From e2a401c34b2b2cde426f36e32aeb01e19e0b6356 Mon Sep 17 00:00:00 2001 From: "allyblockcast[bot]" Date: Tue, 25 Aug 2026 23:10:11 +0000 Subject: [PATCH 2/2] fix(heartbeat): harden the quota-recovery wake scope (Ally review at 714299d) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all three Important findings from the review at exact head 714299d. Each was verified against the real code before acting on it. Important #1 — scoped wake newly exposed to project-scoped suppression. Attaching an issueId makes enqueueWakeup derive a projectId, and budgets.getInvocationBlock early-returns null on a falsy candidateProjectId (budgets.ts:1068). So a budget-paused project made the scoped enqueue throw conflict(...), the catch logged a warn, and the agent NEVER WOKE — strictly worse than pre-fix, where it woke unscoped and could work another project's issue. The scoped wake now retries unscoped: waking is the invariant, the scope is only the optimization. Retry is deliberately NOT blanket-on-falsy, which would have been a double-wake bug: the provider-capacity gate returns null *after* committing a scheduled_retry run (:30402), and that gate is especially likely here because we are recovering from a provider park. Retry is gated on the suppression out-param — providerCapacityDeferred excluded, and only the two skip reasons the scope itself can unlock (budget.blocked, heartbeat.worktree_execution_cutoff) qualify. Scope-independent gates (cooldown, company inactive, heartbeat disabled) would decline the retry identically, and issue_tree_hold_active is an explicit hold that dropping the scope must not circumvent. Important #2 — eq(issues.id, issueId) was missing the isUuidLike guard that enqueueWakeup applies to the identical lookup, with a comment warning about exactly this hazard. issueIdFromRunContext returns context.issueId ?? context.taskId verbatim and canonicalization is conditional, so an identifier form reaches the resolver, raises Postgres 22P02, gets swallowed, and silently reverts to the unscoped fan-in. Now mirrors the guarded lookup shape, scopes by companyId (identifiers collide across tenants), and returns the canonical UUID. Important #3 — logging policy was inverted: intended drops logged while all three unexpected error paths were silent .catch(() => null). Since this fix's regression mode is a silent revert to the exact unscoped behaviour, there was no way to confirm from production that it works. Now warns on the getRun catch, the issue-select catch, and the resolver catch. Suggestion 2 — documented the load-bearing per-caller onSuccess contract at its declaration in quota-exhausted-hook.ts, where a refactor would see it. Suggestion 3 — assert parkedRun.agentId === agentId. Suggestion 1 (gate on unresolved blocker count rather than status === "blocked") deferred to BLO-27858 as the review allows. Tests: 2 new cases, both verified to fail pre-fix rather than pass vacuously — the budget case fails `expected [] to have a length of 1` (the agent never woke at all) and the identifier case fails `expected null to be ` (scope silently dropped). vitest run heartbeat-quota-recovery-wake-scope # 6 passed vitest run quota-exhausted-hook ccrotate-retry # 28 passed pnpm --filter @paperclipai/server typecheck # clean Co-Authored-By: Claude --- ...eartbeat-quota-recovery-wake-scope.test.ts | 92 ++++++++- server/src/services/heartbeat.ts | 191 ++++++++++++++++-- server/src/services/quota-exhausted-hook.ts | 14 ++ 3 files changed, 277 insertions(+), 20 deletions(-) diff --git a/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts b/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts index 1a482ecb5aac..6887878e704e 100644 --- a/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts +++ b/server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts @@ -24,6 +24,7 @@ import { createDb, heartbeatRuns, issues, + projects, } from "@paperclipai/db"; import { getEmbeddedPostgresTestSupport, @@ -128,6 +129,7 @@ describeEmbeddedPostgres("quota-recovery wake task scoping (BLO-28992)", () => { assigneeAgentId: string | null; status?: string; title?: string; + projectId?: string | null; }): Promise { const issueId = randomUUID(); await db.insert(issues).values({ @@ -137,6 +139,7 @@ describeEmbeddedPostgres("quota-recovery wake task scoping (BLO-28992)", () => { status: input.status ?? "in_progress", priority: "high", assigneeAgentId: input.assigneeAgentId, + projectId: input.projectId ?? null, }); return issueId; } @@ -149,6 +152,8 @@ describeEmbeddedPostgres("quota-recovery wake task scoping (BLO-28992)", () => { companyId: string; agentId: string; issueId?: string | null; + /** Overrides the snapshot entirely, for shapes the happy path never writes. */ + contextSnapshot?: Record; }): Promise { const runId = randomUUID(); await db.insert(heartbeatRuns).values({ @@ -161,9 +166,9 @@ describeEmbeddedPostgres("quota-recovery wake task scoping (BLO-28992)", () => { errorCode: "provider_quota_exhausted", startedAt: new Date(), finishedAt: new Date(), - contextSnapshot: input.issueId - ? { issueId: input.issueId, taskId: input.issueId } - : {}, + contextSnapshot: + input.contextSnapshot ?? + (input.issueId ? { issueId: input.issueId, taskId: input.issueId } : {}), }); return runId; } @@ -312,4 +317,85 @@ describeEmbeddedPostgres("quota-recovery wake task scoping (BLO-28992)", () => { // shape this issue exists to remove. expect(recovered[0]?.contextIssueId).toBeNull(); }); + + it("still wakes — unscoped — when the scope itself trips project budget suppression", async () => { + const { companyId, agentId } = await seedAgent(); + // A project paused for budget makes `budgets.getInvocationBlock` return a + // block, which `enqueueWakeup` turns into a thrown conflict. Crucially this + // gate is only REACHABLE once an issueId is attached: projectId is derived + // from the issue, and getInvocationBlock early-returns null on a falsy + // candidateProjectId. So this suppression is one the pre-fix unscoped wake + // could never hit — it is a hazard introduced by scoping. + const projectId = randomUUID(); + await db.insert(projects).values({ + id: projectId, + companyId, + name: "Paused for budget", + status: "active", + pauseReason: "budget", + pausedAt: new Date(), + }); + const issueId = await seedIssue({ + companyId, + assigneeAgentId: agentId, + projectId, + title: "Work inside a budget-paused project", + }); + const runId = await seedParkedRun({ companyId, agentId, issueId }); + + const heartbeat = heartbeatService(db, { + penstockAvailabilityGate: allowingGate(), + skipQueuedRunDispatch: true, + }); + + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId, + }); + + const recovered = await waitForRecoveryRuns(agentId, 1); + // The regression this guards: without the unscoped retry the scoped wake + // throws, the catch logs a warn, and the agent NEVER WAKES — strictly worse + // than pre-fix, where it woke unscoped and could work another project's + // issue. Waking is the invariant; the scope is only the optimization. + expect(recovered).toHaveLength(1); + expect(recovered[0]?.contextIssueId).toBeNull(); + }); + + it("resolves an identifier-form context snapshot instead of silently dropping the scope", async () => { + const { companyId, agentId } = await seedAgent(); + const issueId = await seedIssue({ + companyId, + assigneeAgentId: agentId, + title: "Parked with an identifier-shaped scope", + }); + // `issueIdFromRunContext` returns `contextSnapshot.issueId ?? .taskId` + // verbatim, and canonicalization to a UUID inside `enqueueWakeup` is + // conditional, so an identifier can genuinely reach the resolver. issues.id + // is a Postgres uuid column: feeding it "BLO-123" raises invalid-input- + // syntax (22P02), which the resolver's catch would swallow — silently + // reverting to the unscoped fan-in behaviour this PR exists to remove. + const identifier = "QRWS-4711"; + await db.update(issues).set({ identifier }).where(eq(issues.id, issueId)); + const runId = await seedParkedRun({ + companyId, + agentId, + contextSnapshot: { issueId: identifier, taskId: identifier }, + }); + + const heartbeat = heartbeatService(db, { + penstockAvailabilityGate: allowingGate(), + skipQueuedRunDispatch: true, + }); + + await heartbeat.finalizeAgentStatus(agentId, "failed", "quota", { + errorCode: "provider_quota_exhausted", + runId, + }); + + const recovered = await waitForRecoveryRuns(agentId, 1); + expect(recovered).toHaveLength(1); + // Scoped, and canonicalized to the UUID rather than the identifier string. + expect(recovered[0]?.contextIssueId).toBe(issueId); + }); }); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 3e073b144e83..c2adb3ec641a 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -1204,6 +1204,24 @@ export const RECOVERABLE_AGENT_STATUS_ERROR_CODES = [ // stop we should not deepen (BLO-21523). const QUOTA_RECOVERY_UNRESUMABLE_ISSUE_STATUSES = new Set(["done", "cancelled"]); +// BLO-28992: the only `enqueueWakeup` skip reasons that attaching a task scope +// can *itself* unlock. Attaching an issueId makes the wake derive a projectId, +// which reaches project-scoped budget suppression and the worktree-execution +// cutoff — neither of which an unscoped wake could ever hit. When one of these +// declines the scoped wake, retrying unscoped restores the pre-fix behaviour +// (agent wakes, just without the scope) instead of losing the wake entirely. +// +// Deliberately NOT listed: every scope-independent gate (cooldown, company +// inactive, heartbeat disabled) would decline the retry identically, and +// `issue_tree_hold_active` is an explicit hold that dropping the scope must not +// circumvent. The provider-capacity deferral is excluded separately via +// `providerCapacityDeferred` because it commits a scheduled_retry run before +// returning null — retrying there would double-wake. +const QUOTA_RECOVERY_SCOPE_ATTRIBUTABLE_SKIP_REASONS = new Set([ + "budget.blocked", + "heartbeat.worktree_execution_cutoff", +]); + export function readHeartbeatRunErrorFamily( run: Pick, ) { @@ -19657,18 +19675,60 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) agentId: string, ): Promise { if (!runId) return null; - const parkedRun = await getRun(runId).catch(() => null); + const parkedRun = await getRun(runId).catch((err) => { + // Distinguish "unscoped because the lookup broke" from "unscoped because + // the run had no task". Both degrade to the same wake, so without this + // log there is no way to tell from production whether BLO-28992 is + // actually fixed or has silently reverted to the fan-in behaviour. + logger.warn( + { err, agentId, runId }, + "quota recovery wake: parked-run lookup failed, falling back to an unscoped wake (BLO-28992)", + ); + return null; + }); if (!parkedRun) return null; + // Not a security boundary — the assigneeAgentId check below is the real + // guard — but it documents the assumption and fails closed if a future + // caller ever passes a runId belonging to another agent. + if (parkedRun.agentId !== agentId) { + logger.warn( + { agentId, runId, parkedRunAgentId: parkedRun.agentId }, + "quota recovery wake: parked run belongs to a different agent, refusing to scope (BLO-28992)", + ); + return null; + } const issueId = issueIdFromRunContext(parkedRun.contextSnapshot); if (!issueId) return null; + // `issueIdFromRunContext` returns `contextSnapshot.issueId ?? .taskId` + // verbatim, and canonicalization to a UUID inside `enqueueWakeup` is + // conditional (it sits behind `if (!projectId && issueId)`), so an + // identifier form such as "BLO-123" can reach us here. Match the guarded + // lookup shape used at the `enqueueWakeup` site: issues.id is a Postgres + // uuid column, so feeding it an identifier raises invalid-input-syntax + // (22P02) before the OR is evaluated. Scope by companyId for the same + // reason that site does — identifiers can collide across tenants. + const lookupIsUuid = isUuidLike(issueId); + const idMatch = lookupIsUuid + ? or(eq(issues.id, issueId), eq(issues.identifier, issueId.toUpperCase())) + : eq(issues.identifier, issueId.toUpperCase()); const issue = await db - .select({ status: issues.status, assigneeAgentId: issues.assigneeAgentId }) + .select({ + id: issues.id, + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + }) .from(issues) - .where(eq(issues.id, issueId)) + .where(and(eq(issues.companyId, parkedRun.companyId), idMatch)) .limit(1) .then((rows) => rows[0] ?? null) - .catch(() => null); + .catch((err) => { + logger.warn( + { err, agentId, runId, issueId }, + "quota recovery wake: issue lookup failed, falling back to an unscoped wake (BLO-28992)", + ); + return null; + }); // Unreadable or vanished issue: fall back to unscoped rather than pinning // the recovered run to an id we could not verify. @@ -19687,7 +19747,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) ); return null; } - return issueId; + // Canonicalize: the lookup above accepts an identifier form, so return the + // UUID rather than whatever shape the context snapshot happened to hold. + return issue.id; } /** @@ -19879,21 +19941,116 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) const resumeIssueId = await resolveQuotaRecoveryWakeIssueId( hookRunId, hookAgentId, - ).catch(() => null); - return enqueueWakeup(hookAgentId, { - source: "automation", - triggerDetail: "system", - reason: "provider_quota_exhausted_recovered", - requestedByActorType: "system", - requestedByActorId: "quota-exhausted-hook", - // Omitted entirely when there is no resumable scope, so a run that - // parked without a task still wakes unscoped exactly as before. - ...(resumeIssueId ? { contextSnapshot: { issueId: resumeIssueId } } : {}), - }) + ).catch((err) => { + logger.warn( + { err, agentId: hookAgentId, runId: hookRunId }, + "quota recovery wake: scope resolution threw, falling back to an unscoped wake (BLO-28992)", + ); + return null; + }); + + const wake = (issueId: string | null, suppression?: WakeSuppressionOutcome) => + enqueueWakeup( + hookAgentId, + { + source: "automation", + triggerDetail: "system", + reason: "provider_quota_exhausted_recovered", + requestedByActorType: "system", + requestedByActorId: "quota-exhausted-hook", + // Omitted entirely when there is no resumable scope, so a run + // that parked without a task still wakes unscoped exactly as + // before. + ...(issueId ? { contextSnapshot: { issueId } } : {}), + }, + suppression, + ); + + if (!resumeIssueId) { + return wake(null) + .then(() => undefined) + .catch((err) => { + logger.warn( + { err, agentId: hookAgentId, issueId: null }, + "failed to wake agent after quota-exhausted hook", + ); + }); + } + + // Attaching an issueId makes `enqueueWakeup` derive a projectId + // (:30041), which newly exposes this wake to *project*-scoped + // suppression that the pre-fix unscoped wake could never reach: + // `budgets.getInvocationBlock` early-returns null on a falsy + // candidateProjectId (budgets.ts:1068). So a paused or over-hard-stop + // project would make the scoped enqueue throw `conflict(...)` — and + // the agent would not wake AT ALL, where before it woke unscoped and + // could have worked any other project's issue. The worktree-execution + // cutoff gate has the same shape but quieter: it `return null`s rather + // than throwing, so nothing is enqueued and no catch fires. + // + // Apply this fix's own stated principle — a stale or unusable scope + // degrades to today's unscoped behaviour — to suppression too. Waking + // the agent is the invariant; the scope is only the optimization. + // + // Retry ONLY on the gates the scope itself unlocked. A blanket + // retry-on-falsy would be a double-wake bug: the provider-capacity + // gate returns null *after committing a scheduled_retry run* + // (:30402), and that gate is especially likely here because we are + // recovering from a provider park. Every other `return null` is + // scope-independent (cooldown, company inactive, heartbeat disabled), + // so an unscoped retry would be declined identically — no reason to + // spend the call. `issue_tree_hold_active` is scope-dependent but + // deliberately excluded: an explicit tree pause hold should not be + // circumvented by dropping the scope. + const suppression: WakeSuppressionOutcome = { + durableSkipReason: null, + providerCapacityDeferred: false, + dependencyBlockedRetryAt: null, + }; + let scopedFailure: unknown = null; + try { + const queued = await wake(resumeIssueId, suppression); + if (queued) return undefined; + scopedFailure = new Error( + `scoped wake was suppressed (${suppression.durableSkipReason ?? "no durable skip reason"})`, + ); + } catch (err) { + scopedFailure = err; + } + + const scopeAttributable = + !suppression.providerCapacityDeferred && + suppression.durableSkipReason !== null && + QUOTA_RECOVERY_SCOPE_ATTRIBUTABLE_SKIP_REASONS.has(suppression.durableSkipReason); + + if (!scopeAttributable) { + logger.warn( + { + err: scopedFailure, + agentId: hookAgentId, + issueId: resumeIssueId, + durableSkipReason: suppression.durableSkipReason, + providerCapacityDeferred: suppression.providerCapacityDeferred, + }, + "failed to wake agent after quota-exhausted hook", + ); + return undefined; + } + + logger.warn( + { + err: scopedFailure, + agentId: hookAgentId, + issueId: resumeIssueId, + durableSkipReason: suppression.durableSkipReason, + }, + "quota recovery wake: scoped wake hit a scope-only gate, retrying unscoped (BLO-28992)", + ); + return wake(null) .then(() => undefined) .catch((err) => { logger.warn( - { err, agentId: hookAgentId, issueId: resumeIssueId ?? null }, + { err, agentId: hookAgentId, issueId: resumeIssueId }, "failed to wake agent after quota-exhausted hook", ); }); diff --git a/server/src/services/quota-exhausted-hook.ts b/server/src/services/quota-exhausted-hook.ts index 5cd6115a64b2..1acd4147a48a 100644 --- a/server/src/services/quota-exhausted-hook.ts +++ b/server/src/services/quota-exhausted-hook.ts @@ -133,6 +133,20 @@ export interface RunQuotaExhaustedHookInput { * recovery action per adapter without an extra DB lookup. */ adapterType: string; errorCode: string; + /** Invoked once recovery succeeds. + * + * LOAD-BEARING CONTRACT (BLO-28992): this must be invoked **per caller**, on + * every branch — the in-flight join, the time-debounced return, and the + * branch that actually ran the recovery. Each parked run passes its own + * closure carrying its own task scope, so collapsing this into a single + * shared callback would silently re-deliver one run's scope to all N (or + * none at all), which is exactly the fan-in that made N runs of one agent + * converge on one issue and interleave writes into a shared checkout. + * + * The degradation is silent — N scoped wakes quietly become 1 — and the test + * that pins this behaviour lives on the heartbeat side + * (`heartbeat-quota-recovery-wake-scope.test.ts`), so someone refactoring + * this file would not see it. Keep the per-caller invocation. */ onSuccess?: (() => void | Promise) | null; }