fix(server): eagerly cancel stale queued runs on issue reassignment (RENA-55610) - #11085
Conversation
…RENA-55610) Reassigning an issue previously left the old assignee's queued heartbeat run alive until it lazily reached the queue head with a free concurrency slot in claimQueuedRun — which can take hours behind a busy queue, and until then the stale run kept holding issues.executionRunId, locking out the new assignee. Add cancelStaleQueuedRunsForIssue, reusing evaluateQueuedRunStaleness/ cancelQueuedRunForStaleIssue so interaction-wake and review-participant exceptions still apply, and call it right after a reassignment PATCH commits. Also sweep queued runs against a cheap in-memory prefilter in startNextQueuedRunForAgent even when the agent has no free concurrency slots, so a backed-up per-agent queue still gets its obviously-stale entries (reassigned/terminal/deleted issue) cancelled on every scheduling pass instead of only once a run reaches the front of the line. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
Greptile SummaryThe PR eagerly cancels queued heartbeat runs made stale by issue reassignment and adds a scheduler sweep that performs stale-run cleanup even when an agent has no free concurrency slots.
Confidence Score: 4/5The concurrent claim-and-cancel race should be fixed before merging because eager cleanup can mark an already executing run as cancelled. The new post-commit cleanup evaluates a queued snapshot and later performs an unconditional status update, while the normal scheduler can transition that same row to running in between. Files Needing Attention: server/src/services/heartbeat.ts
|
| Filename | Overview |
|---|---|
| server/src/services/heartbeat.ts | Adds both eager and scheduler-driven stale-run cancellation, but the eager path can race a concurrent claim and overwrite a running status. |
| server/src/routes/issues.ts | Invokes best-effort stale-run cleanup after a committed assignee change. |
| server/src/tests/heartbeat-stale-queue-invalidation.test.ts | Covers reassignment and zero-slot cleanup behavior, but not concurrent claiming during eager cancellation. |
Prompt To Fix All With AI
### Issue 1
server/src/services/heartbeat.ts:12835
**Prevent claimed-run cancellation**
If reassignment cleanup overlaps a scheduler claim, `claimQueuedRun` can transition the selected row to `running` before this call reaches the unconditional status update. The cleanup then marks an executing run as cancelled and its wakeup as skipped, leaving active work attached to a terminal run record; cancellation must require that the run is still queued.
### Issue 2
server/src/routes/issues.ts:9002-9008
**Complete the required PR description**
The description explains the change and verification, but it omits the required Thinking Path and Risks sections, and the model note lacks the exact model version, context window, and capability details. Please update it to follow the PR template so maintainers have the required rationale and risk assessment for this concurrency-sensitive change.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(server): eagerly cancel stale queued..." | Re-trigger Greptile
| const context = parseObject(run.contextSnapshot); | ||
| const staleness = await evaluateQueuedRunStaleness(run, issueId, context); | ||
| if (!staleness.stale) continue; | ||
| const cancelled = await cancelQueuedRunForStaleIssue(run, issueId, staleness); |
There was a problem hiding this comment.
Prevent claimed-run cancellation
If reassignment cleanup overlaps a scheduler claim, claimQueuedRun can transition the selected row to running before this call reaches the unconditional status update. The cleanup then marks an executing run as cancelled and its wakeup as skipped, leaving active work attached to a terminal run record; cancellation must require that the run is still queued.
Knowledge Base Used: Agent Execution Runtime
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/heartbeat.ts
Line: 12835
Comment:
**Prevent claimed-run cancellation**
If reassignment cleanup overlaps a scheduler claim, `claimQueuedRun` can transition the selected row to `running` before this call reaches the unconditional status update. The cleanup then marks an executing run as cancelled and its wakeup as skipped, leaving active work attached to a terminal run record; cancellation must require that the run is still queued.
**Knowledge Base Used:** [Agent Execution Runtime](https://app.greptile.com/paperclip-org-3/-/custom-context/knowledge-base/paperclipai/paperclip/-/docs/agent-execution-runtime.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (existing.assigneeAgentId && nextAssigneeAgentId !== existing.assigneeAgentId) { | ||
| // The previous assignee may still have a queued run for this issue. | ||
| // Cancel it now instead of leaving it for the lazy staleness check in | ||
| // claimQueuedRun, which only runs once that run reaches the queue head | ||
| // — behind a busy queue that can take hours and leaves the issue's | ||
| // execution lock held by a run nobody will ever start. | ||
| await heartbeat.cancelStaleQueuedRunsForIssue(existing.companyId, existing.id).catch((err) => { |
There was a problem hiding this comment.
Complete the required PR description
The description explains the change and verification, but it omits the required Thinking Path and Risks sections, and the model note lacks the exact model version, context window, and capability details. Please update it to follow the PR template so maintainers have the required rationale and risk assessment for this concurrency-sensitive change.
Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/routes/issues.ts
Line: 9002-9008
Comment:
**Complete the required PR description**
The description explains the change and verification, but it omits the required Thinking Path and Risks sections, and the model note lacks the exact model version, context window, and capability details. Please update it to follow the PR template so maintainers have the required rationale and risk assessment for this concurrency-sensitive change.
**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
cancelQueuedRunForStaleIssue wrote the "cancelled" status via an unconditional UPDATE ... WHERE id = runId. Between evaluateQueuedRunStaleness's read and that write, a concurrent claimQueuedRun could already have promoted the run to "running" - the unconditional write would then overwrite a live run's status to "cancelled" and strand active work on a terminal record. Guard the write with the same compare-and-set primitive already used by setRunStatusIfRunning (WHERE status = 'queued'), and skip every cancel side effect (wakeup skip, execution-lock clear, run event) when the guard matches zero rows. Adds a targeted regression test using a new HeartbeatServiceOptions.beforeQueuedRunCancelWrite test hook to deterministically land a concurrent claim inside the exact window between staleness evaluation and the cancel write. RENA-55706
No code change. Retriggers commitperclip and Greptile against the now-completed PR description (Thinking Path, What Changed, Verification, Risks, Model Used, in-PR issue description, and dedup-search sections).
…beat mocks The eager-cancel-on-reassignment call in routes/issues.ts (added by an earlier commit on this PR for RENA-55610) invokes heartbeat.cancelStaleQueuedRunsForIssue on every PATCH /api/issues/:id that changes assigneeAgentId. Route test files that hoist their own mockHeartbeatService double did not define that method, so any reassignment-path test threw "cancelStaleQueuedRunsForIssue is not a function" synchronously - before the route's own .catch() around that call could ever run - and the request failed with a 500. This surfaced as unrelated-looking failures across three "Verify serialized server suites" shards in CI (issue-agent-mutation-ownership-routes, issue-assignee-invokability-routes, issue-comment-reopen-routes), all "expected 500 to be 200" on PATCH reassignment paths. Add the missing mock (resolving to []) to every hoisted mockHeartbeatService in server/src/__tests__ that mounts routes/issues.ts, so reassignment tests exercise the real .catch()-guarded call path instead of throwing before it. Verified locally (serialized, extended timeouts to offset this box's resource contention): the three previously-failing files now pass 174/174. RENA-55706
Thinking Path
Linked Issues or Issue Description
No matching public GitHub issue was found (see "Duplicate/related PR search" below), so this section follows the bug report template fields directly.
What happened?
Reassigning an issue away from an agent left that agent's queued heartbeat run alive. The run kept the issue's
executionRunIdlock until the lazy staleness check inclaimQueuedRunhappened to reach it, which can take hours behind a busy queue.Expected behavior
A queued run that is made stale by reassignment (or by the issue reaching a terminal status, or by another change covered by the existing staleness rules) should be cancelled promptly, and the issue's execution lock should be released, without waiting for the run to reach the front of the queue.
Steps to reproduce
executionRunId.This PR adds
cancelStaleQueuedRunsForIssue(called from the reassignment path) and a zero-free-slot sweep instartNextQueuedRunForAgent, so both cases are handled eagerly instead of lazily. This update on top of that also closes a claim-race window a reviewer found in the eager cancel write itself (see "What Changed").Duplicate/related PR search
Searched open PRs in this repo for related work on queued-run staleness and cancellation. No exact duplicate of this change. Related but distinct open PRs in the same area, from other contributors:
What Changed
cancelQueuedRunForStaleIssue(server/src/services/heartbeat.ts) with a compare-and-set onstatus = 'queued', using the same atomic pattern assetRunStatusIfRunning/setRunStatusFromLive.claimQueuedRunpromoted it to "running"), the function now returns without touching the wakeup request, without clearing the issue's execution lock, and without appending a run event. The run that won the claim race is left untouched.HeartbeatServiceOptions.beforeQueuedRunCancelWrite, a test-only hook invoked immediately before the guarded write. Production call sites do not set it, so behavior is unchanged in production. Tests use it to land a concurrent status flip deterministically inside the exact window the guard protects.heartbeat-stale-queue-invalidation.test.ts→ "does not cancel a queued run that wins the claim race between staleness evaluation and the cancel write", covering this exact window.issue-agent-mutation-ownership-routes.test.ts,issue-assignee-invokability-routes.test.ts,issue-comment-reopen-routes.test.ts) hoist their ownmockHeartbeatServicedouble and did not definecancelStaleQueuedRunsForIssue. The reassignment branch inroutes/issues.ts(added earlier on this PR, guarded by its own.catch) calls that method on every PATCH that changesassigneeAgentId; against the incomplete mock, the call threw synchronously before the.catchcould run, turning any reassignment-path test into a 500. Added the missing mock (vi.fn(async () => [])) to every hoistedmockHeartbeatServiceinserver/src/__tests__that mountsroutes/issues.ts(14 files total — 3 that were actually exercising the broken path, plus 11 more with the same latent gap that had not yet hit a reassignment test case).Verification
corepack pnpm exec tsc --noEmitinserver/— clean, no errors (confirmed by the "Typecheck + Release Registry" CI check on this branch; localtscin this sandbox reports unrelated@paperclipai/plugin-sdkresolution errors from an intentionally partialpnpm install --filter @paperclipai/server..., not from this diff).corepack pnpm exec vitest run src/__tests__/heartbeat-stale-queue-invalidation.test.tsinserver/(embedded Postgres) — 27/27 passing (26 pre-existing + 1 new); also confirmed green in CI's "General tests (server)" shards for this head commit.corepack pnpm exec vitest run --no-file-parallelism --maxWorkers=1 src/__tests__/issue-comment-reopen-routes.test.ts src/__tests__/issue-agent-mutation-ownership-routes.test.ts src/__tests__/issue-assignee-invokability-routes.test.ts(the CI "Verify serialized server suites" flags) — 174/174 passing, versus 3 files / 8+ tests failing with500before the fix.evaluateQueuedRunStalenessreports it stale), injects a concurrent status flip to "running" right before the cancel write via the new test hook, then asserts the run stays "running", the wakeup request stays "queued" (not "skipped"), the issue keeps itsexecutionRunId, and no run event is appended.Risks
WHERE status = 'queued'guard to a write that was previously unconditional, and only skips side effects when that guard already tells us the run is no longer queued. It cannot cause an eligible stale run to stay queued longer than before; it only stops a live run from being wrongly marked cancelled.beforeQueuedRunCancelWriteoption is optional and unset by every production call site (heartbeatService(db)is constructed without it outside tests), so there is no behavior change outside the test suite.CONTRIBUTING.md→ "No Internal Issue References" / "Branch Naming" asks contributors to avoid. Renaming the branch would require opening a new PR against a renamed head, which conflicts with keeping this update on the existing open PR. This description itself avoids internal ticket references and only links public GitHub PR numbers above.Model Used
Anthropic Claude, model id
claude-sonnet-5, running inside Claude Code (Claude Agent SDK) with tool use enabled (shell/git commands, file read/edit, GitHub REST API calls). The runtime does not expose an explicit extended-thinking-mode flag or a context-window figure to this session, so neither is stated as a specific number.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template#NNN/github.com/paperclipai/paperclipURLs)docs/...,fix/...) and contains no internal Paperclip ticket id or instance-derived details — see "Risks" above for why this one is not fixed here