fix: recover issue monitors stuck triggered past timeoutAt with null nextCheckAt

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Issue monitors are the mechanism by which an agent parks on an issue and gets woken again when external state it is waiting on changes — they are the substrate for every "wait for CI / wait for a blocker / wait for a receipt" flow in the fleet
• tickDueIssueMonitors, the periodic sweep in heartbeat.ts, only claims rows where monitorNextCheckAt IS NOT NULL — but dispatchClaimedIssueMonitor nulls that column optimistically the moment a monitor fires, before the woken run has re-armed it
• So when the woken run never calls back (dies, is reassigned, or the assignee simply never re-arms), the row drops out of the sweep's WHERE clause permanently: exhaustedMonitorClearReason never runs again, clearedAt/clearReason are never set, and the configured recoveryPolicy — usually wake_owner, configured for exactly this situation — never fires
• The issue then goes silent indefinitely while still reading as armed to anything that checks monitorScheduledBy or executionState.monitor.status; only nextCheckAt: null gives it away
• This pull request adds a second sweep, tickExpiredIssueMonitors, that finds those stranded rows and routes them through the same clear-and-recover pipeline exhaustion already uses
• The benefit is that monitor expiry becomes a defined terminal state with a recorded reason and a real recovery wake, instead of a silent no-op that strands the issue

Linked Issues or Issue Description

Refs BLO-25865 — https://paperclip.blockcast.net/BLO/issues/BLO-25865

What Changed

• Added tickExpiredIssueMonitors (server/src/services/heartbeat.ts): a second sweep that claims monitors stuck status="triggered" with nextCheckAt IS NULL, a passed timeoutAt, and clearedAt IS NULL, on issues still in_progress/in_review, and routes them through the existing clearIssueMonitorAndRecover → performIssueMonitorRecovery pipeline so recoveryPolicy finally fires.
• Narrowed the monitor parameter on the shared recovery helpers (monitorRecoveryPolicy, monitorRecoveryDetails, performIssueMonitorRecovery, clearIssueMonitorAndRecover) to only the fields they actually read (serviceName / timeoutAt / maxAttempts / recoveryPolicy). By the time a monitor is stuck triggered, buildIssueMonitorTriggeredPatch has already stripped executionPolicy.monitor, so the only surviving source of that metadata is the persisted executionState.monitor, which is shaped like IssueExecutionMonitorState rather than IssueExecutionMonitorPolicy.
• Guarded the expired-monitor claim against state drift between the claim query and the update (server/src/services/issue-execution-policy.ts, plus the claim path in heartbeat.ts).
• Added 5 cases to server/src/__tests__/issue-monitor-scheduler.test.ts covering the new sweep and its controls.
• No monitor interval, maxAttempts default, or agent-health alert threshold is retuned by this PR.

Verification

• npx vitest run src/__tests__/issue-monitor-scheduler.test.ts — 18/18 pass at the rebased head. New cases: (1) a monitor stuck triggered past timeoutAt ends cleared with clearReason="timeout_exceeded" and emits exactly one issue_monitor_recovery wake; (2) repeated ticks do not double-recover; (3) control — a monitor already cleared normally is left untouched; (4) control — a triggered monitor whose timeoutAt has not passed is left untouched.
• npx vitest run src/__tests__/issue-execution-policy.test.ts — 57/57 pass.
• npx tsc --noEmit in server/ — clean, zero errors (after building the @paperclipai/shared and @paperclipai/plugin-sdk workspace packages, whose stale dist is the only source of errors in a cold worktree).
• Live query-based signal for the reconciliation AC: monitor.status = 'triggered' AND monitor.nextCheckAt IS NULL AND monitor.timeoutAt < now() AND monitor.clearedAt IS NULL across non-terminal issues should go from ≥1 (BLO-17091, BLO-17477, BLO-18396 were live instances at authoring time) to 0 after this deploys and the sweep ticks.

Risks

• Low. The new sweep's predicate (status="triggered" AND nextCheckAt IS NULL AND timeoutAt < now AND clearedAt IS NULL) is disjoint from the existing sweep's (nextCheckAt IS NOT NULL), so it cannot touch any row the existing sweep already handles — it covers precisely the gap the existing sweep structurally cannot reach.
• It reuses the existing claim pattern (advisory lock plus monitorWakeRequestedAt staleness guard), so it is safe under concurrent ticks, and reuses exhaustedMonitorClearReason / clearIssueMonitorAndRecover / performIssueMonitorRecovery verbatim — no new clearReason values and no new recovery-policy branches are introduced.
• The main behavioural shift is that previously-silent stranded monitors will now emit wake_owner recoveries. Deploying this is the reconciliation step for the existing backlog of stuck rows, so expect a one-off burst of recovery wakes on the first few ticks as the accumulated backlog drains, ordered oldest-updatedAt-first and capped at 50 per tick. No DB migration is required.

Model Used

Claude Opus 4.8 (claude-opus-4-8), 1M context, extended thinking, with tool use and code execution — via Claude Code.

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 similar open and closed PRs and confirmed this is not a duplicate
• 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-side scheduler change only)
• I have updated relevant documentation to reflect my changes (n/a — no user-facing surface changed)
• 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

[allyblockcast]

🔗 Paperclip issue: BLO-17477
🔗 Paperclip issue: BLO-17091
🔗 Paperclip issue: BLO-18396
🔗 Paperclip issue: BLO-25865
🔗 Paperclip issue: BLO-22798
🔗 Paperclip issue: BLO-21020

[allyblockcast]

🔗 Paperclip issue: BLO-17477
🔗 Paperclip issue: BLO-17091
🔗 Paperclip issue: BLO-18396
🔗 Paperclip issue: BLO-25865
🔗 Paperclip issue: BLO-22798
🔗 Paperclip issue: BLO-21020

[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: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".
• 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: 568196f

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/heartbeat.ts:11089 — the expired-monitor claim update only rechecks id, monitorNextCheckAt, expiry, and claim staleness; it omits the initial query's assigneeUserId IS NULL, agent-assignee, and status IN (in_progress, in_review) guards. If an issue is reassigned to a user or becomes terminal after the select but before this update, the scheduler can still claim it and clearIssueMonitorAndRecover can clear/recover a monitor outside the eligible state.
  • Keep the claim predicate consistent with the select and tickDueIssueMonitors claim predicate, and add a concurrency regression test that changes assignee/status between selection and claim to verify no recovery occurs.

Suggestions (0)

Strengths

• The new sweep is narrowly scoped to triggered monitors with null nextCheckAt and an expired timeout.
• It reuses the existing clear-and-recover pipeline and adds coverage for recovery, idempotency, and control states.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Addressed Ally's important finding in commit feacfa2bf.

• The expired-monitor claim predicate now mirrors the initial select and tickDueIssueMonitors: assigneeUserId IS NULL, agent assignment present, and status in_progress/in_review.
• Added a deterministic select/claim race regression covering user reassignment and terminal status; neither clears or recovers the monitor.
• Verification: pnpm exec vitest run server/src/__tests__/issue-monitor-scheduler.test.ts passed, 15/15 tests. Server typecheck was attempted but exceeded the 120s runtime limit during plugin build-dependency preparation.

PR: #1326

[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: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".
• 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: feacfa2

Prior Findings Dispositioned (1)

• prior:568196f important 1 — fixed — server/src/services/heartbeat.ts:11094 — the expired-monitor claim now rechecks assigneeUserId IS NULL, and lines 11095-11096 recheck agent assignment and eligible issue status before claiming; the added race tests at server/src/__tests__/issue-monitor-scheduler.test.ts:225 verify reassignment and terminal-status races do not recover.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The new sweep is narrowly scoped to triggered monitors with null nextCheckAt and an expired timeout.
• It reuses the existing clear-and-recover pipeline and adds coverage for recovery, idempotency, control states, and claim races.

Recommended Action

1. No blocking issues found; this review is clean.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The expired-monitor sweep is narrowly scoped to triggered monitors with a null next check and an expired timeout.
• The select and claim predicates both recheck assignee and issue status, addressing the prior claim-race finding.
• The added tests cover recovery, repeat-tick idempotency, normal-clear and not-yet-expired controls, and reassignment/status races.

Recommended Action

1. No blocking issues found; this review is clean.