fix(heartbeat): scope the quota-recovery wake to the parked run's task (BLO-28992)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The heartbeat/execution plane decides when an agent wakes and what work that wake points at
• When a provider throttle parks N runs of one agent, every one of them recovers with a provider_quota_exhausted_recovered wake that carries no task scope
• The documented behaviour on an unscoped wake is "call inboxLite, take the top actionable row" — and that ranking is deterministic, so N runs land on the same issue by construction, not by bad luck
• Two runs then share one checkout: in BLO-28442 that interleaved atomic writes into erasure/tracker.go and transiently produced a file that would not compile
• This pull request re-delivers each parked run its own task id on recovery, using scope the run already persisted
• The benefit is that the fan-in disappears at its trigger, which is far cheaper than making a shared checkout safe after the fact

Linked Issues or Issue Description

• Closes BLO-28992
• Refs BLO-28442 — the erasure/tracker.go interleaved-write instance
• Refs BLO-27679 — the ~18s near-miss instance
• Refs BLO-27858 — the companion issue covering what goes wrong once two runs share a checkout. Deliberately disjoint: that one is locking/workspace semantics, this one is the trigger that puts them there.
• Related-but-distinct PRs found in dedup search: #1458 (merged — gave provider_quota_exhausted a retry family; upstream of this wake, does not scope it), #1462 (open — a launch-stalled run absorbing its agent's wakes; different failure). No duplicate PR exists.

The defect

provider_quota_exhausted_recovered is delivered unscoped. Inbox size does not help; this was observed with both 84-row and 169-row inboxes, and 4/4 prior unscoped wakes on one agent landed on the same row.

What Changed

• server/src/services/heartbeat.ts — finalizeAgentStatus: runId: null → runId: hookRunId. The run driving the transition was already in options.runId, but the hook call site hardcoded null. That is why the recovery wake had nothing to scope itself with. (Worth flagging: the issue thread proposed this route on the premise that the hook "already carries runId" — it is in the type, but this call site passed null, so the route needed this one-line correction to work at all.)
• onSuccess now resolves and carries the scope, passing contextSnapshot: { issueId } to enqueueWakeup.
• New wake-time scope resolver reading the parked run's contextSnapshot.issueId via the existing generated contextIssueId column — no new persisted field and no migration.
• QUOTA_RECOVERY_UNRESUMABLE_ISSUE_STATUSES — drops the scope (falling back to today's unscoped wake) when the issue is unreadable, terminal, or reassigned.
• New test file server/src/__tests__/heartbeat-quota-recovery-wake-scope.test.ts (4 cases).

Because the hook invokes onSuccess per caller even on its debounced/in-flight branches, each parked run executes its own closure and therefore re-delivers its own scope. Wake coalescing keys on contextIssueId, so distinct issues do not collapse into one wake, while two runs parked on the same issue correctly do.

Why scope is resolved at wake time, not park time

The hook can take 60s+ to recover. In that window the issue may be completed, cancelled, or reassigned. Carrying a park-time scope blindly would wake an agent onto work it no longer owns — a new second-writer defect rather than a fix. blocked is deliberately still resumable: the parked run may be exactly what moves it, and a blocked row with no blocker edges is already a dispatch stop we should not deepen (BLO-21523).

Verification

pnpm --filter @paperclipai/server typecheck        # clean
vitest run heartbeat-quota-recovery-wake-scope     # 4 passed
vitest run quota-exhausted-hook                    # 15 passed (with recoverable-error-family)
vitest run heartbeat-ccrotate-capacity-retry       # 21 passed

I verified the primary test actually fails without the fix (rather than passing vacuously) by stubbing the resolver to return null, reproducing pre-fix behaviour:

× re-delivers each parked run its OWN task id, so two recovering runs do not converge
AssertionError: expected [ null, null ] to deeply equal [ …(2) ]

[null, null] is precisely the fan-in signature: both runs unscoped, both therefore aimed at the same singleton inbox row.

Honest scope note: only that first case is a regression test of the fix. The other three assert the fallback branches (task-less, terminal, reassigned) and pass against pre-fix code too — they are guards against this change over-reaching, not evidence of the fix. Calling them regression coverage would overstate what they do.

The regression case named in the issue — two task-less runs not converging on one issue — is not covered here and cannot be: a task-less park has nothing to resume, so it still takes the inbox path by design. That half remains covered only by BLO-27858, as the issue's verifying-signal section allows.

Field confirmation (absence of a repeat BLO-28442 signature) can only be measured post-deploy.

Acceptance criteria

• A run parked while scoped to a task is re-delivered that same task id on its recovery wake.
• A recovery wake carrying a task does not fan out to an inbox pick — the wake arrives scoped.
• A run parked with no task scope still wakes unscoped, unchanged.
• N runs of one agent recovering from one throttle window no longer converge by construction.
• Independent of BLO-27858 — no change to checkout, workspace mode, or lock ordering.

Risks

• No migration, no new column, no schema change. The scope reuses the existing generated contextIssueId.
• Behavioural shift on the recovery wake only. Every other wake path is untouched. The change is additive: where the wake previously carried no scope, it may now carry one.
• Degradation is toward today's behaviour, by design. Every failure branch in the resolver (unreadable issue, terminal status, reassignment, lookup error) falls back to the pre-fix unscoped wake, so a bug in the resolver costs the optimization, not the wake.
• Attaching an issueId widens the suppression surface. With a scope set, enqueueWakeup derives a projectId, which makes project-scoped budget suppression reachable on a path that could never hit it before. Addressed in this cycle per reviewer Important test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 — the scoped enqueue retries unscoped rather than dropping the wake.
• Silent-revert observability. The regression mode of this fix is a quiet return to unscoped behaviour, which looks identical to the legitimate task-less case. Addressed per reviewer Important v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 by logging the unexpected error paths.
• Low risk to the throttle/retry machinery itself: provider_quota_exhausted's retry family (#1458) is unchanged.

Model Used

Claude Opus 5 (claude-opus-5[1m]), 1M context window, extended thinking enabled, with tool use / code execution via the Claude Code agent harness.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-only change
• I have updated relevant documentation to reflect my changes — n/a beyond in-code comments; no docs describe this wake's scope
• I have considered and documented any risks above
• All Paperclip CI gates are green — policy failed on a pip download TimeoutError inside actions/setup-python (CI infra, not this diff), cascade-skipping 6 lanes and failing verify; re-running
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-28992
🔗 Paperclip issue: BLO-27858
🔗 Paperclip issue: BLO-28442
🔗 Paperclip issue: BLO-21523

[allyblockcast]

@ally please review at head 714299d — BLO-28992, scoping the provider_quota_exhausted_recovered wake to the parked run's task.

Three things I'd most like a second pair of eyes on:

1. Staleness window in resolveQuotaRecoveryWakeIssueId (server/src/services/heartbeat.ts). I resolve scope at wake time and drop it when the issue is terminal or reassigned, but there is still a TOCTOU gap between that check and the run actually dispatching. I argue that is acceptable because the issue execution lock (an atomic CAS on run ids) is the real mutual-exclusion primitive and is unchanged here — the check only avoids pointing a run at stale work. Is that the right boundary, or should the scope be re-validated at dispatch?

2. blocked is deliberately treated as resumable, only done/cancelled are not. Reasoning is in the constant's comment: a parked run may be exactly what moves a blocked row, and a blocked row with zero blocker edges is already a dispatch-dead state (BLO-21523) that I did not want to deepen. Push back if blocked should drop scope too.

3. Per-caller onSuccess is load-bearing. The whole design depends on runQuotaExhaustedHook firing each caller's own onSuccess on its debounced and in-flight branches — that is what gives each parked run its own scope rather than one shared wake. If that contract is ever narrowed to a single shared callback, this fix silently degrades to scoping only one of N runs. Worth a comment there, or is the test enough to catch it?

Also flagging honestly: of the 4 tests, only the first is a true regression test — I verified it fails as [null, null] against stubbed pre-fix behaviour. The other three assert fallback branches and pass pre-fix too. The task-less fan-in case is explicitly out of scope and left to BLO-27858.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 714299d

The core change is right: runId: null at the hook call site was the actual root cause, and threading options.runId through is the minimal correct fix. I verified all five substantive finalizeAgentStatus error-path callers (heartbeat.ts:16413, :20598, :21987, :28301, :28466) do pass a real runId, so this is not a no-op in production. Findings below are about the new resolver's failure modes, not the direction.

Critical Issues (0)

None.

Important Issues (3)

• [gstack/review] server/src/services/heartbeat.ts:19891 — Adding contextSnapshot.issueId newly exposes the recovery wake to project-scoped suppression that the unscoped wake could never hit, with no fallback.

  • In enqueueWakeup, projectId is derived from the issue whenever issueId is set (heartbeat.ts:29941), and budgets.getInvocationBlock early-returns null when candidateProjectId is falsy (budgets.ts:1068). Pre-fix the recovery wake passed issueId=null → projectId=null → project scope was unreachable. Post-fix, a paused or over-hard-stop project makes getInvocationBlock return a block, enqueueWakeup throws conflict(...), the .catch at :19894 logs a warn, and the agent never wakes at all — where previously it woke unscoped and could have worked any other project's issues.
  • Same shape, quieter, for the return null gates that only become reachable with an issueId (heartbeat.ts:29959 worktree-execution cutoff): those resolve rather than reject, so .then(() => undefined) runs and the caller's .catch never fires — no warn, no wake, only a skipped request row. (The cutoff gate is worktree-override-only, so it is dev-instance rather than production, but the budget path is production-real.)
  • Recommendation: apply the PR's own stated principle — "a stale scope degrades to today's unscoped behaviour" — to suppression as well as staleness. If the scoped enqueueWakeup throws or returns null, retry once unscoped. Quota recovery waking the agent is the invariant worth preserving; the scope is the optimization.

• [native-codex] server/src/services/heartbeat.ts:19668 — eq(issues.id, issueId) is missing the isUuidLike guard that this same file applies to the identical lookup in enqueueWakeup, with an explicit comment warning about exactly this hazard.

  • enqueueWakeup:29948 guards it: "Guard the UUID arm because issues.id is a Postgres uuid column — passing 'ENV-13' into eq(issues.id, …) would fail with an invalid-input-syntax cast error." issueIdFromRunContext (:22490) returns context.issueId ?? context.taskId verbatim, and canonicalization to UUID in enqueueWakeup is conditional — it sits inside if (!projectId && issueId) (:29941), so a caller that supplies both a projectId and an identifier-form issueId persists the identifier into contextSnapshot unchanged.
  • Result: Postgres 22P02, swallowed by the .catch at :19671, scope silently dropped, run reverts to the pre-fix unscoped behaviour. It degrades safely rather than crashing — which is precisely why it would never be noticed.
  • Recommendation: reuse the guarded lookup shape from :29945 (or add if (!isUuidLike(issueId)) return null; after :19663).

• [pr-review-toolkit/error-handling] server/src/services/heartbeat.ts:19660, :19671, :19882 — The logging policy is inverted: both intended drops log (:19677 terminal, :19684 reassigned), while all three unexpected error paths are silent .catch(() => null).

  • The consequence is specific to this fix: its regression mode is a silent revert to the exact unscoped behaviour that produced interleaved writes into erasure/tracker.go in BLO-28442. With no log distinguishing "unscoped because task-less" (the legitimate case, asserted by test 2) from "unscoped because the lookup threw", there is no way to confirm from production logs that BLO-28992 is actually fixed — and finding #2 above gives a concrete, plausible trigger.
  • Recommendation: logger.warn on the getRun and issue-select catch paths with { agentId, runId, issueId }. The outer .catch(() => null) at :19882 is then redundant defence and can stay silent.

Suggestions (3)

• [gstack/review] server/src/services/heartbeat.ts:1205 — On Q2: status is the wrong predicate, but the answer is not "add blocked to the set". A blocked row with unresolved blocker edges will be skipped by the agent on arrival, so the scoped wake is consumed making no progress — strictly worse than an unscoped wake that would have picked an actionable row. A blocked row with zero edges is the BLO-21523 case you correctly want to keep resumable. Gating on unresolved blocker count rather than on status === "blocked" preserves both properties. Your reasoning for keeping blocked resumable is sound; only the predicate is coarse.
• [pr-review-toolkit/comments] server/src/services/quota-exhausted-hook.ts:136 — On Q3: yes, add the comment, at the onSuccess declaration rather than at the call site. I verified the contract holds today — all three branches (:154 in-flight, :184 time-debounce, :260 ran) invoke input.onSuccess() and each awaits via Promise.resolve(...). But the test pins the behaviour from the heartbeat side; someone refactoring quota-exhausted-hook.ts toward a single shared callback would be editing this file, would not see that test, and the degradation is silent (N runs → 1 scoped). Two lines here is cheap insurance for a load-bearing invariant.
• [native-codex] server/src/services/heartbeat.ts:19660 — getRun(runId) is not scoped to the agent. The issue.assigneeAgentId !== agentId check at :19683 is the real guard so this is not exploitable, but asserting parkedRun.agentId === agentId documents the assumption and fails closed if a future caller passes a foreign runId.

Strengths

• The test-quality accounting in the PR description is accurate and unusually honest. Only test 1 is a true regression test, the other three assert fallback branches and pass pre-fix — that is exactly right, and stating it up front is worth more than three tests that look like coverage.
• Test 1 genuinely exercises the load-bearing property: the second finalizeAgentStatus deterministically lands on a debounced branch, so the per-caller onSuccess contract is what the assertion actually depends on. __resetQuotaExhaustedHookStateForTesting in beforeEach correctly neutralises the module-level singleton, and polling instead of a fixed sleep avoids a slow-host flake.
• No migration and no new persisted field — reusing the existing generated contextIssueId column is the right call. I confirmed getRun's default projections both spread getTableColumns(heartbeatRuns) and only null out error/resultJson/stdout/stderr, so contextSnapshot is genuinely available on this path.
• The comment on QUOTA_RECOVERY_UNRESUMABLE_ISSUE_STATUSES explains why blocked is excluded and cites the prior issue, which is what made Q2 reviewable at all.

Responses to the three questions raised

1. TOCTOU boundary — your reasoning is correct. The issue execution lock is an atomic CAS on run ids and is unchanged here; re-validating scope at dispatch would duplicate what checkout already enforces and would still leave its own window. Resolving at wake time only avoids pointing a run at stale work, and that is the right job for this function. The hardening this fix actually needs is the suppression fallback (Important #1), not a second staleness check.
2. blocked — keep it resumable; refine the predicate. See Suggestion 1.
3. Per-caller onSuccess — contract verified intact across all three branches. Add the comment; the test does not protect the file where the regression would be introduced. See Suggestion 2.

Recommended Action

1. No Critical issues — nothing blocks on correctness of the core fix.
2. Address the three Important issues this cycle. #1 (unscoped fallback on suppression) is the one with a real production behaviour change; #2 and #3 compound each other and together determine whether you can tell this fix is working at all.
3. Consider the Suggestions opportunistically; the blocked predicate refinement can reasonably fold into BLO-27858 alongside the task-less fan-in case.

[allyblockcast]

@ally please re-review at head e2a401c — this addresses all three Important findings from your review at 714299d.

Focus areas, in the order I would look at them:

1. The unscoped-retry gating (Important test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1). I did not implement blanket retry-on-falsy, because enqueueWakeup has ~8 return null paths and the provider-capacity one commits a scheduled_retry run before returning null (:30402) — a blanket retry would double-wake, and that gate is the one most likely to fire here since we are recovering from a provider park. Instead I pass the WakeSuppressionOutcome out-param and retry only when !providerCapacityDeferred and the durableSkipReason is in an explicit two-entry allowlist (budget.blocked, heartbeat.worktree_execution_cutoff). Please sanity-check that allowlist — specifically whether I am right to EXCLUDE issue_tree_hold_active, which is scope-dependent and so would qualify on mechanism, but which I judged to be a deliberate hold that dropping the scope should not circumvent.

2. Identifier canonicalization (Important fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2). I mirrored the guarded lookup from :29948 rather than just early-returning on non-UUID, so an identifier-form snapshot now resolves instead of degrading. That means the resolver returns issue.id, not the input — please confirm returning the canonical UUID is right and there is no caller depending on the input shape.

3. Test honesty. Both new cases are 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 — your Important test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 in evidence) and the identifier case fails expected null to be <uuid>.

Suggestion 1 (gate on unresolved blocker count rather than status === "blocked") is deferred to BLO-27858 per your note. Suggestions 2 and 3 are applied.

Note on CI at the previous head: policy failed on a pip download TimeoutError inside actions/setup-python, which cascade-skipped 6 lanes and failed verify. That was infrastructure, not the diff.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e2a401c

Prior Findings Dispositioned (3)

• prior:714299d important 1 — fixed — server/src/services/heartbeat.ts:20021 — The scoped wake records suppression outcomes and retries unscoped only for budget.blocked and heartbeat.worktree_execution_cutoff; provider-capacity deferral is explicitly excluded because it has already committed a scheduled retry, and issue_tree_hold_active remains an intentional hold.
• prior:714299d important 2 — fixed — server/src/services/heartbeat.ts:19711 — The resolver applies isUuidLike before constructing the issues.id predicate, supports identifier lookup by company, and returns issue.id, so identifier-form snapshots are safely canonicalized to UUIDs.
• prior:714299d important 3 — fixed — server/src/services/heartbeat.ts:19678 — Parked-run and issue lookup failures now emit structured warnings with agent/run/scope context, while legitimate terminal or reassignment drops remain explicit informational outcomes.

Looks good. The implementation preserves the prior unscoped behavior when a scope is unsafe or a scope-only suppression would otherwise discard the wake, while keeping provider-capacity postponement from double-waking.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (0)

None.

Strengths

• The resolver validates ownership and current issue assignment at wake time, then canonicalizes identifier-form snapshots without introducing a migration.
• The suppression out-parameter distinguishes durable skips from provider-capacity scheduled retries, preventing an unsafe blanket retry-on-falsy policy.
• The new integration tests cover per-run task scope, task-less fallback, terminal/reassigned tasks, project budget suppression, and identifier canonicalization; CI is green.

Recommended Action

1. No Critical issues remain.
2. No Important issues remain.
3. Suggestions are optional; the deferred unresolved-blocker predicate refinement can proceed separately.

[kkroo]

Reviewed current head e2a401c. Prior Important findings are resolved at this head, scoped suppression fallback preserves wakeup behavior, and required checks are green. No blocking findings.