Skip to content

fix(heartbeat): promote parked scheduled retries when a human comment wake coalesces into them#9732

Open
hawikk wants to merge 2 commits into
paperclipai:masterfrom
hawikk:fix/human-wake-promotes-scheduled-retry
Open

fix(heartbeat): promote parked scheduled retries when a human comment wake coalesces into them#9732
hawikk wants to merge 2 commits into
paperclipai:masterfrom
hawikk:fix/human-wake-promotes-scheduled-retry

Conversation

@hawikk

@hawikk hawikk commented Jul 16, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The server heartbeat service queues, coalesces, retries, and dispatches agent runs; failed runs with transient errors are parked as scheduled_retry rows with bounded exponential backoff (2m/10m/30m/2h with jitter)
  • Issue comment wakes are enqueued with source: "automation", so enqueueWakeup coalesces them into any active same-scope run, including a parked scheduled_retry
  • When that happens, the comment's wakeCommentId is merged into the parked run's context but the retry keeps its original schedule, so the person who messaged the agent gets no response until the backoff expires, which can be hours after the underlying failure (for example a Claude session limit) has cleared
  • From the operator's point of view the agent is completely unresponsive: commenting on the issue does nothing visible and only the on-demand "Run Heartbeat" button unsticks it
  • This pull request promotes a parked scheduled retry through the standard retry gate whenever the wake that coalesced into it was requested by a human (requestedByActorType: "user"), mirroring retryScheduledRetryNow
  • Agent- and system-initiated wakes keep the existing coalesce-without-promotion behavior, so bounded retry backoff still protects against automated wake storms
  • The benefit is that messaging an agent always gets it moving again as soon as a human asks, instead of leaving them staring at a silent, hours-long backoff

Linked Issues or Issue Description

Refs #9730 (this PR fixes the second half of that report: "commenting on the issue does not wake the agent while a run sits in scheduled_retry"; the first half, backoff ignoring the provider-reported reset time, is addressed by #9626 and #8692)

Related PRs found while searching for duplicates:

Incident evidence (from a production instance on @paperclipai/server 2026.707.0, full timeline in #9730): an agent hit a provider session limit and its retry was parked for 2h13m. A user comment posted 9 seconds after the limit reset was coalesced into the parked run (its contextSnapshot.wakeCommentId was updated) but the run kept its schedule, more than two hours away. Clicking "Run Heartbeat" one minute later dispatched a run instantly, proving capacity was back. The issue was in in_review at the time, so the existing route-level path that cancels a scheduled retry when a human comments (shouldHumanCommentResumeInProgressScheduledRetry) did not apply; that path only covers in_progress issues, and the wake was swallowed at the heartbeat layer instead.

What Changed

  • server/src/services/heartbeat.ts: added promoteScheduledRetryForHumanWake inside enqueueWakeup. When a wake with requestedByActorType: "user" coalesces into a scheduled_retry run (either coalesce path: the issue-execution transaction path or the same-scope active-run path), the retry's scheduledRetryAt is pulled forward to now, a lifecycle run event is recorded, and the run is promoted through the standard promoteScheduledRetryRun gate (budget, invokability, issue state), then dispatched via startNextQueuedRunForAgent.
  • Review follow-up (5f38458): the same-scope coalesce path no longer writes the merged context snapshot and the retry pull-forward as two separate updates. promoteScheduledRetryForHumanWake now receives the merged snapshot and folds the merge plus the scheduledRetryAt pull-forward into a single UPDATE guarded by status = 'scheduled_retry', so a concurrent snapshot write can no longer land between the two writes and be clobbered. If the guarded update loses the race, the merged snapshot is still persisted so the coalesced wake is not dropped.
  • Review follow-up (5f38458): mirroring retryScheduledRetryNow, the promotion now also stamps the retry run's original agentWakeupRequests.payload with scheduledRetryAt and retryNowRequestedAt inside the same transaction, closing the observability gap for support tooling that reads the wakeup request to explain a promotion.
  • server/src/__tests__/heartbeat-human-comment-wake-promotes-retry.test.ts: embedded-postgres coverage for the new behavior: an agent comment wake keeps the retry parked; a human comment wake on an in_review issue (the incident shape) promotes and executes the parked run with the comment merged into its context; a human wake promotes an agent-scoped parked retry; an already-promoted (queued) run is not re-promoted.

Verification

  • pnpm exec vitest run src/__tests__/heartbeat-human-comment-wake-promotes-retry.test.ts in server/: 4 tests pass (run twice to confirm stability)
  • pnpm exec vitest run src/__tests__/heartbeat-retry-scheduling.test.ts src/__tests__/heartbeat-comment-wake-batching.test.ts in server/: 39 tests pass, no regressions in the adjacent retry and comment-wake suites
  • tsc --noEmit in server/ passes
  • The human-promotion test now also asserts the agentWakeupRequests.payload stamp; that assertion fails against the pre-fix code and passes after the fix, shown below.

Full suite passing at 5f38458 (after the review fixes):

vitest run: 4 tests passing after the review fixes

Same test file run against the pre-fix heartbeat.ts (8c31865): the new payload-stamp assertion fails with retryNowRequestedAt missing, proving the regression coverage is real:

vitest run: new payload-stamp assertion fails against pre-fix code

Risks

  • Behavioral shift: a human comment (or any human-attributed wake) now front-runs the retry backoff. This is intentional and scoped: the promotion still goes through evaluateScheduledRetryGate, so budget blocks, agent invokability, terminal issue states, pause holds, and review-participant checks all still suppress it. Attempt counting is unchanged, so a promoted retry that fails again re-parks with the next bounded backoff step and retries stay finite.
  • Local-CLI agents that authenticate as users are treated as human here, matching how the rest of the codebase interprets requestedByActorType: "user". Worst case they pull a retry forward that would have fired later anyway, still bounded by max attempts.
  • No schema changes, no migrations, no telemetry changes.

Model Used

Claude Fable 5 (Anthropic, model ID claude-fable-5), running in Claude Code / Claude Agent SDK with extended thinking and tool use. The change and tests were authored by the model and reviewed by the account owner.

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 not referenced internal/instance-local Paperclip issues or links (only public GitHub #NNN / github.com/paperclipai/paperclip URLs)
  • My branch name describes the change (e.g. docs/..., fix/...) and contains no internal Paperclip ticket id or instance-derived details
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • I have updated relevant documentation to reflect my changes (no docs affected)
  • I have considered and documented any risks above
  • All Paperclip CI gates are green (will confirm once CI runs on this PR)
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending automated review)
  • I will address all Greptile and reviewer comments before requesting merge

…alesces into them

Comment wakes are enqueued with source "automation", so when the target
run is parked in scheduled_retry the wake is absorbed into the run's
context without pulling its schedule forward. The person who commented
gets no response until the bounded backoff expires, which can be hours
after the underlying failure (for example a provider session limit) has
cleared. Promote the parked retry through the standard gate whenever the
coalesced wake was requested by a user, mirroring retryScheduledRetryNow.
Agent- and system-initiated wakes keep the existing behavior so bounded
retry backoff still protects against wake storms.
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes an agent responsiveness bug: when a human comments on an issue whose agent run is parked in scheduled_retry (e.g., after hitting a provider session limit), the comment wake was silently coalesced into the parked run without pulling its backoff forward, leaving the agent unresponsive for hours.

  • Adds promoteScheduledRetryForHumanWake inside enqueueWakeup: when a wake with requestedByActorType: "user" coalesces into a scheduled_retry run, the function folds the context snapshot merge and scheduledRetryAt pull-forward into a single UPDATE guarded by status = 'scheduled_retry', then promotes the run through the standard promoteScheduledRetryRun gate (budget, invokability, issue state, etc.).
  • Both previous review findings are addressed at 5f38458: the two-write race in the same-scope coalesce path is eliminated (merge + pull-forward in one guarded UPDATE), and the agentWakeupRequests.payload stamp (scheduledRetryAt / retryNowRequestedAt) is applied inside the same transaction, mirroring retryScheduledRetryNow.
  • Adds 4 embedded-postgres integration tests covering the incident shape (in_review issue, human comment, context merge, payload stamp), agent-scoped promotion, agent-wake no-op, and idempotent double-promotion guard.

Confidence Score: 5/5

Safe to merge — the promotion still goes through the full gate (budget, invokability, issue state, pause holds), attempt counting is unchanged, and agent/system wakes are unaffected; only human-attributed wakes trigger the pull-forward.

Both race-condition and observability gaps from the first review are closed: the snapshot merge and scheduledRetryAt pull-forward are now a single guarded UPDATE, and the original wakeup request payload is stamped in the same transaction. The fallback path on a lost race still writes an unguarded snapshot update, but that window only opens after the scheduler has already promoted the run, making a conflict there benign. Test coverage directly exercises the incident-shaped scenario and the payload-stamp regression.

No files require special attention.

Important Files Changed

Filename Overview
server/src/services/heartbeat.ts Adds promoteScheduledRetryForHumanWake inner function and hooks it into both coalesce paths; the single guarded UPDATE (status = 'scheduled_retry') correctly folds the snapshot merge and scheduledRetryAt pull-forward, and the agentWakeupRequests.payload stamp mirrors retryScheduledRetryNow in the same transaction — both first-round findings addressed.
server/src/tests/heartbeat-human-comment-wake-promotes-retry.test.ts New embedded-postgres test file with 4 cases: agent-wake keeps retry parked, human-wake on in_review issue promotes and executes (the incident shape, including wakeCommentId and payload-stamp assertions), agent-scoped promotion, and no-op on already-queued run.

Reviews (3): Last reviewed commit: "fix(heartbeat): make human-wake retry pr..." | Re-trigger Greptile

Comment thread server/src/services/heartbeat.ts
Comment thread server/src/services/heartbeat.ts
…wakeup payload

Address two review findings on the human-wake promotion path:

1. The same-scope coalesce path wrote the merged context snapshot and
   then promoted the retry in a second unguarded update that spread the
   in-memory snapshot, so a concurrent snapshot write landing between
   the two could be silently clobbered. The promotion now receives the
   merged snapshot and folds the merge and the scheduledRetryAt
   pull-forward into a single UPDATE guarded by status =
   scheduled_retry. When the guarded update loses the race the merged
   snapshot is still persisted so the coalesced wake is not dropped.

2. Mirror retryScheduledRetryNow by stamping the retry run's original
   agentWakeupRequests.payload with scheduledRetryAt and
   retryNowRequestedAt inside the same transaction, so support tooling
   reading the wakeup request can see the retry was pulled forward.
@hawikk

hawikk commented Jul 17, 2026

Copy link
Copy Markdown
Author

@greptileai review

Both P2 findings from the first review are addressed in 5f38458 (single guarded UPDATE for the merge plus pull-forward, and the agentWakeupRequests.payload stamps mirroring retryScheduledRetryNow), with regression coverage and before/after evidence in the PR body. Requesting a re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant