fix(heartbeat): stop park/promote from bypassing timer suppression (BLO-31344)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Each agent runs in heartbeats, and a timer tick that finds nothing to do still costs a full paid adapter invocation — so skipTimerWhenNoActionableWork exists to decline those ticks before they reach an adapter
• That knob was provably not firing: a lane with zero assigned todo/in_progress rows and the flag set to true kept receiving dispatched runs, one of which cost $7.76
• The reported deduction assumed the suppression gate is on the only path from a tick to a run. It is not — the penstock capacity gate runs first and postpones a wake by committing a scheduled_retry, and promotion of that row never re-enters wakeup(), so the suppression gate is simply never evaluated for any wake that got parked
• That inverts the severity: the knob works on an idle fleet and stops working under exactly the capacity pressure that makes a wasted paid attempt most expensive. It is also not capacity-specific — dependency_blocked and transient_failure parks promote through the same function
• This pull request evaluates suppression before the capacity gate, and re-probes it at promotion for a park whose original wake was generic-timer-shaped
• The benefit is that an unscoped tick with nothing to do costs no run row, no park, no promotion and no invocation, while every scoped wake still promotes untouched

Linked Issues or Issue Description

Fixes: BLO-31344

Related prior work found in the dedup search below:

• fix(heartbeat): gate timer skip predicate on checkout availability (BLO-19749) #1108 (merged, BLO-19749) — the inverse case: rows present in todo/in_progress but unavailable to checkout, making the predicate wrongly return true. This PR is the opposite: zero such rows exist, the predicate correctly returns false, and a run is dispatched anyway.
• fix: retry-now can now promote ccrotate_capacity parks (no retryOfRunId) #1316 (open) and fix(heartbeat): bound dep-blocked park lifetime by age, not attempts alone (BLO-29055) #1452 (merged, BLO-29055) — both touch promoteScheduledRetryRun. This change adds a block ahead of the capacity re-probe rather than inside it, to avoid entangling with BLO-28919's extension of that branch.

What Changed

• Reordered the genericTimerWake suppression gate in enqueueWakeup to run before the penstock capacity gate. The capacity gate parks rather than declines, so anything after it never evaluates for a parked wake.
• Added a suppression re-probe in promoteScheduledRetryRun, ahead of the existing capacity re-probe, cancelling via the existing cancelScheduledRetryForGate path.
• Added timer_no_actionable_work to the ScheduledRetryGate error-code union. heartbeatRuns.errorCode is plain text(), not a pgEnum — no migration needed.
• Added isGenericTimerWakeSnapshot, a persisted-snapshot sibling of the live genericTimerWake predicate, placed beside the heartbeat task-key fallback that creates the trap it has to work around.
• Added server/src/__tests__/heartbeat-timer-suppression-park-bypass.test.ts (10 tests).

Why two changes and not one

Neither half closes the bypass alone. The reorder cannot help a row that is already parked, nor a dependency_blocked/transient_failure park whose original wake was generic — those paths never consult the wake gate at all. The promotion re-probe alone would still pay for the park and its promotion cycle.

The __heartbeat__ trap

isGenericTimerWakeSnapshot deliberately does not test deriveTaskKey(snapshot) for null. enqueueWakeup evaluates !taskKey against the value returned by enrichWakeContextSnapshot — plain deriveTaskKey, null for a generic tick. The snapshot that gets persisted has by then had deriveTaskKeyWithHeartbeatFallback stamp taskKey: "__heartbeat__" into it; the fallback writes the sentinel to the snapshot and never to the returned value.

So on a stored row the sentinel is evidence the wake was generic, not evidence it was scoped. A non-null check would be false for every row the predicate exists to match — the fix would compile, deploy, and silently never fire. A dedicated test pins this, because it is precisely the case a plausible-looking simplification breaks.

The helper errs toward "not generic" (it checks commentId alongside wakeCommentId): suppressing a genuinely scoped retry would drop the wake its scope was for, which is far worse than promoting one redundant no-op.

Verification

pnpm --filter @paperclipai/server typecheck                 # clean
pnpm --filter @paperclipai/server exec vitest run \
  src/__tests__/heartbeat-timer-suppression-park-bypass.test.ts \
  src/__tests__/heartbeat-stale-queue-invalidation.test.ts \
  src/__tests__/heartbeat-ccrotate-capacity-retry.test.ts    # 69 passed
pnpm --filter @paperclipai/server exec vitest run \
  src/__tests__/heartbeat-retry-scheduling.test.ts \
  src/__tests__/heartbeat-dependency-scheduling.test.ts      # 96 passed

Negative control. The tests are only worth anything if they fail without the fix. Reverting server/src/services/heartbeat.ts to HEAD while keeping the new test file:

test	result on unfixed HEAD
declines a generic tick instead of capacity-parking it	FAIL — expected length 0, got 1: a park row was created
suppresses an already-parked tick at promotion	FAIL — expected 1 to be 0: promoted anyway
suppresses regardless of which path parked it	FAIL — expected 1 to be 0: same for transient_failure
3 × isGenericTimerWakeSnapshot unit tests	FAIL — helper does not exist
4 preserved-behaviour guards	PASS on both sides — correct for guards

That reproduces the defect at both bypass sites independently. With the fix restored, 10/10 pass.

The four guards are deliberate: still capacity-parks when actionable, promotes when actionable again, never suppresses a scoped park, and todo with unresolved blockers stays actionable. They assert preserved behaviour, so passing on both sides is the correct outcome for them.

Risks

• Low risk on the reorder. It can only change behaviour for a wake that would have been declined outright anyway. Declining such a wake cannot lose work; parking it demonstrably did, because the park outlives the reason it was created. A guard test confirms a generic tick with actionable work is still capacity-parked, so suppression does not shadow the capacity gate.
• The promotion re-probe is the sharper edge, because suppressing a scoped retry would silently drop the wake its scope was for. Mitigated by gating strictly on the reconstructed generic-timer shape and by erring toward "not generic"; a guard test asserts a task-scoped park still promotes into a drained lane.
• No migration. The new gate code is a text() value.
• AC4 preserved. hasActionableTimerWork is unchanged, so a todo/in_progress row with unresolved blockers still counts as actionable — only status: blocked is excluded, by the pre-existing status filter. Pinned by a test.
• Not verified, stated rather than hidden: agent_wakeup_requests is the linked issue's primary arbiter and is not readable from an agent seat, so the post-deploy assertion on live rows still needs an operator query. Also note the heartbeat scheduler runs in StatefulSet/paperclip, not Deployment/paperclip-api — confirm the digest on that tier before calling the fix live.

Model Used

Claude Opus 4.5 (claude-opus-5[1m] as served, Anthropic), 1M context, extended thinking enabled, with tool use and code execution via Claude Code running as a Paperclip heartbeat agent.

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 — the load-bearing invariant is documented in-tree at both edit sites, per this file's existing convention
• 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
• I will address all Greptile and reviewer comments before requesting merge

_{Dedup search run: gh pr list --state all --search over skipTimerWhenNoActionableWork, hasActionableTimerWork, no_actionable_work, promoteScheduledRetryRun, 31344 — results linked above.}

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-31344

[allyblockcast]

🔗 Paperclip issue: BLO-31344

[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: 3c8c0cf

Both halves of the fix are real, and I verified the mechanism rather than taking the description on trust. The __heartbeat__ sentinel reasoning in the new helper's doc comment is correct: enrichWakeContextSnapshot stamps contextSnapshot.taskKey = HEARTBEAT_TASK_KEY via the fallback at heartbeat.ts:8198 while returning the plain deriveTaskKey value, so a naive non-null check really would never fire. The capacity park's snapshot spreads enrichedContextSnapshot and re-asserts wakeSource (heartbeat.ts:31549), so the predicate does match production rows. The wake-side reorder is minimal — suppression and the penstock gate swap, nothing else moves — and every local it reads (policy, taskKey, enrichedContextSnapshot, agent) is bound well before the new position.

Critical Issues (0)

Important Issues (2)

• [code / gstack-review] server/src/services/heartbeat.ts:17468 — the promotion-time gate cancels continuation parks, not just never-started ticks, and cancelled is terminal.
  An issueless max_turns_continuation park reaches this block unimpeded: evaluateScheduledRetryGate short-circuits with { allowed: true } when the snapshot carries no issueId (heartbeat.ts:17154), and the issueless case is an explicitly supported park state (heartbeat.ts:18683-18692 branches on if (issueId) … else …). Such a park exists precisely because the agent ran out of turns mid-task; if the lane's assigned queue happens to be empty at promotion — the normal situation for an agent doing issueless work such as sweeps, reports or PR review — the continuation is now cancelled with no path back to it. The same reaches this block for session_unavailable / zero_token_session_reset / job_failed / capacity_blocked, all requiresIssueExecutionRetryLock reasons deliberately designed to survive a retry cycle.

  • The knob's contract is "skip a timer tick with nothing to do", and a fresh tick is separable from a continuation by row shape: the capacity park this PR targets is inserted without retryOfRunId (heartbeat.ts:31655-31686), whereas every scheduleBoundedRetryForRun park sets retryOfRunId: run.id (heartbeat.ts:18852). Adding dueRun.retryOfRunId === null to the condition — or excluding requiresIssueExecutionRetryLock(dueRun.scheduledRetryReason) — keeps the stated target intact and cannot eat a continuation. If cancelling continuations is deliberate, please say so in the comment and on the issue: it is a larger behavior change than the title claims.

• [tests] server/src/__tests__/heartbeat-timer-suppression-park-bypass.test.ts:390 — the "regardless of which path parked it" test asserts generality against a row shape that path never writes.
  seedPark (:227-246) hardcodes invocationSource: "timer", wakeupRequestId: null and no retryOfRunId — that is a capacity park with the reason string swapped to "transient_failure" at :399. A real bounded-retry park is inserted with invocationSource: "automation", a non-null wakeupRequestId bound to a queued wakeup request, and retryOfRunId: run.id (heartbeat.ts:18845, :18848, :18852). So the test never exercises the wakeupRequestId cancellation branch in cancelScheduledRetryForGate, and it would keep passing under the retryOfRunId discriminator suggested above — i.e. it cannot detect the finding it is shaped to cover.

  • Relatedly, nothing pins the coupling the whole promotion-side fix rests on: that a real park's stored snapshot carries wakeSource: "timer" + taskKey: "__heartbeat__". The suite already produces one genuine capacity park at :284; reading that row back and asserting isGenericTimerWakeSnapshot(parked.contextSnapshot) === true closes both gaps in a few lines, and is exactly the regression the helper's own doc comment warns fails silently.

Suggestions (2)

• [comments] server/src/services/heartbeat.ts:17456 — the comment claims this block is needed for "a dependency_blocked / transient_failure park whose original wake was generic". A dependency_blocked park cannot be generic: it is only written on an issue-scoped wake, with issueId in the snapshot (heartbeat.ts:32529 and its enclosing block), so isGenericTimerWakeSnapshot structurally excludes it. The transient_failure half stands; dropping dependency_blocked avoids overstating what the block covers.
• [code] server/src/services/heartbeat.ts:31728 — the ~35-line "Heartbeat ccrotate-awareness" comment immediately above documents the gateAppliesToWake penstock gate, but the new genericTimerWake const and suppression block now sit between it and the if it describes, so it reads as documentation for the suppression gate. Inserting the new block above that comment (or moving the comment down onto its if) restores the adjacency this file otherwise relies on.

Strengths

• The sentinel trap is documented at the helper rather than at the call site, with the concrete failure mode ("would silently never fire") spelled out — the one thing a future simplification would otherwise break.
• isGenericTimerWakeSnapshot errs toward "not generic" (checking commentId alongside wakeCommentId), and the guard test at :369 pins that a scoped park still promotes — the right asymmetry for a gate that can drop work.
• The AC4 test keeps todo-with-unresolved-blockers counted as actionable, protecting the deliberate BLO-19749 behavior this fix could easily have narrowed.
• Both halves are argued as non-redundant in the code, with the reason each alone is insufficient — and that reasoning checks out: promoteDueScheduledRetries never re-enters wakeup().

Recommended Action

1. Decide the continuation-park question (Important #1) — either add the retryOfRunId / retry-reason discriminator, or state that cancelling max_turns_continuation is intended and cover it with a test.
2. Tighten the promotion tests (Important #2) so the generality claim and the snapshot coupling are actually exercised.
3. Take the comment fixes opportunistically.