Skip to content

fix(hub): cancel overdue reviewers and stuck managers (CL-12015) - #813

Merged
TheGreatAxios merged 3 commits into
mainfrom
cl-12015-cancel-overdue-running-judge-and-stuck-manager-runs-so
Nov 4, 2026
Merged

TheGreatAxios merged 3 commits into
mainfrom
cl-12015-cancel-overdue-running-judge-and-stuck-manager-runs-so

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • isStuck() was dead code: manager runs were stamped startedAt: now() on every pass. It now reads passAt, the time the current run took its open pass (the later of pass.takenAt and claim.claimedAt), so a manager that resumes a gone run's pass is timed from its takeover.
  • New endOverdue sweep runs each reconcile pass, beside endNeverRan, and cancels through stock requestCancellation:
    • live judge and code-review runs one reconcile interval past their claim deadline with no result. Reviewers stay on the claim, so the overdue wake still runs.
    • managers that have held their open pass longer than the reconciler's stuckAfterMs.
    • managers that have neither claimed nor closed a pass within MANAGER_POLL_LIMIT_MS. The manager's own loop (PASSES) would have ended by then, so it is wedged.
  • Each cancel logs stock's outcome. A failed cancel is logged and retried on the next pass, and never aborts the reconcile pass.
  • This deviates from the issue (explained on CL-12015): passAt instead of overwriting startedAt, because sinceQueued/sinceStale filter by startedAt.

Changes

  • apps/hub/src/github/reconcile-plan.ts: isStuck uses passAt.
  • apps/hub/src/manager-state.ts: endOverdue. The reviewer query joins workflow_run, so reviewers that already ended cost nothing. ManagedPull.live.passAt replaces live.pass.
  • apps/hub/src/github/manager-runs.ts: passes live.passAt through.
  • apps/hub/src/server.ts: runs endOverdue with the reconcile policy's stuckAfterMs, and with the reconcile interval as the reviewer grace.
  • packages/triage-contracts/src/triage-state.ts: MANAGER_LIFETIME_MS (moved from triage-manager's LIFETIME_MS) and MANAGER_POLL_LIMIT_MS. PASSES derives from the latter.
  • Tests cover: reviewers past deadline and grace, the grace itself, stuck and resumed passes, idle managers, and the stuck-pass reconcile. Fixtures that modelled stuck via startedAt are updated.
  • docs/ARCHITECTURE.md: describes the sweep.

Testing

  • bun run check and bun run lint are clean.
  • apps/hub (including Postgres), tests/model, packages/triage-manager and packages/triage-contracts: 936/936.
  • bun run test:chain: 22/22.
  • No change to how the manager judges or reviews. The constant moved and its value is unchanged, so no eval run.

Closes CL-12015

@linear-code

linear-code Bot commented Nov 3, 2026

Copy link
Copy Markdown

CL-12015

@TheGreatAxios
TheGreatAxios marked this pull request as draft November 3, 2026 18:54
@corbits-triage

corbits-triage Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Triage: Pass

No changes needed from you. A maintainer will review this pull request.

Notes:

  • added code matches a code pattern: apps/hub/src/manager-state.ts:395 empty-list-fallback: Invented fallback `?? []`; handle the missing value instead.
  • Risk: medium (6 recently fixed files)

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Self-Review Summary

Reviewed origin/main...HEAD. After the fixes below: check, lint, hub tests including Postgres plus tests/model (540/540), and test:chain (22/22) are all green.

Issues Found and Resolved

  • A resumed pass kept its original takenAt, so every manager that took over a dead run's pass would read as stuck at once and be cancelled in a loop. Fixed: passHeldSince (the later of pass.takenAt and claim.claimedAt), with a test.
  • endOverdue logged "ended" whatever requestCancellation returned, and one throw aborted reconcileAll. Fixed: the outcome is logged, and each run's failure is caught and logged.
  • The late-reviewer SQL was duplicated from overduePulls. Fixed: both now use the shared lateReviewer.

Known, not addressed

  • A manager that hangs between passes, with no open pass, is never stuck. isStuck never fired before this change either, and a manager can legitimately wait hours on CI or a reviewer. Fixing this needs a separate liveness signal.
  • Reviewers are cancelled at the deadline with no grace. This matches the manager, which already gives up at the deadline.

@corbits-triage corbits-triage Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No defects reported.

corbits-triage[bot]
corbits-triage Bot previously approved these changes Nov 3, 2026
@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Self-Review Follow-up

Triage reported no defects. The items listed as "Known, not addressed" earlier are now fixed in 8d329f6:

  • Manager hung between passes: cancelled once it has neither claimed nor closed a pass within MANAGER_POLL_LIMIT_MS. That limit (2× lifetime) is the manager loop's own PASSES bound, now shared through contracts.
  • No grace on reviewer deadlines: a reviewer is cancelled only one reconcile interval after its deadline, so a result in flight can land.
  • Rescans grew with reviewers that had ended: the reviewer query joins workflow_run and returns only runs that are still cancellable.
  • Two sources for the stuck limit: the sweep takes the reconcile policy's stuckAfterMs.
  • Duplicated late-reviewer SQL: gone. The sweep has its own joined query, and overduePulls is back to its original form.

The reconcile order (endOverdue after wakeOverdue) is unchanged on purpose: a run being cancelled reads as up until stock ends it, so moving the sweep earlier would not wake anyone sooner.

Rerun: check, lint, 936/936 tests, chain 22/22.

Comment on lines 128 to 130
for (const afterMs of [MINUTE, 11 * MINUTE, 2 * 60 * MINUTE, 7 * 60 * MINUTE]) {
expect(await reconcile(afterMs)).toMatchObject({ status: "running", runId: "run_manager" });
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 · Test expects a manager idle 7h between passes to stay running, but the new idle sweep cancels it after 6h

After done(..., { wait: true }) the claim is live with no open pass. The test then expects the head to stay running with this run at 2h and 7h. The new endOverdue sweep cancels a manager that has neither claimed nor closed a pass within MANAGER_POLL_LIMIT_MS (2 × 3h = 6h). This test does not run the sweep, so the 7h assertion checks a state production would not reach. Under its new title ('runs for hours between passes'), it documents behaviour that the PR's own idle-manager rule rules out.

Reconcile at 7 * 60 * MINUTE after a wait done, with no new pass, which is more than MANAGER_POLL_LIMIT_MS (6h).

Resolve this comment. Triage will not finish while it is open.

@TheGreatAxios
TheGreatAxios added this pull request to the merge queue Nov 4, 2026
Merged via the queue into main with commit c2d9406 Nov 4, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant