Repository navigation
feat(manager): end the manager after it posts and wake a fresh one by mail - #684
Conversation
4634379 to
67b7e21
Compare
|
Triage: Unsure Asking a maintainer about:
Not yet confirmed, a maintainer will check:
Notes:
|
corbits-triage
Bot
added
Triage: Needs Human Review
and removed
Triage: Waiting for CI
labels
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
This change ends a triage manager whose wake mail failed, so the next pull request event deploys a fresh one that can post the verdict. The listener's mail step is meant to stop failing the body, a new unwoken step cancels the affected runs, and the hub's claim stops counting a run as up once cancellation is requested. The most important defect is that the hub still treats a cancelling manager as live, so the fresh manager is never deployed. The listener changes described in the description also do not appear in the code at head.
Not reviewed in full: 6 files, over the size limit.
packages/triage-manager/src/actions/report.tspackages/triage-manager/src/actions/review.tspackages/triage-manager/src/triage-manager.test.tspackages/triage-manager/src/triage-manager.tstests/chain/chain.test.tstests/chain/model.ts
P1 · A manager whose cancellation was requested still counts as up, so the hub mails it and never deploys a fresh one · apps/hub/src/manager-state.ts:248
The description says the hub's claim stops treating a run as up once its cancellation is requested. At head, runIsRunning and runIsUp never read cancellationRequestedAt, and liveManager relies on them. A cancelling manager is therefore still treated as live, so mailedLive mails it and deployFor is skipped. Its take is refused, so the pull request waits on a run that is ending.
Trigger: a manager is cancelled after its wake mail failed, then the pull request gets a new event or a reviewer result. liveManager returns the cancelling run's id, the event is mailed to it, and no fresh manager is deployed.
Fix: check cancellationRequestedAt in runIsRunning and runIsUp, or return undefined from liveManager when cancellation was requested, e.g. return (await runGone(tx, claim.runId, now)) || claim.cancellationRequestedAt ? undefined : claim.runId;
P1 · The listener's mail step still fails the body on a failed mail, and no unwoken step cancels the manager · packages/triage-manager/src/triage-listener.ts:27
The description says the mail step no longer fails the body and that a new unwoken step cancels each manager whose mail failed. At head the mail step is still tolerate: false, no unwoken step exists in the repository, and Mailed in mail.ts is still just { mailed: boolean } with no reason. A manager whose wake mail did not go is never cancelled and stays deployed with nothing to end it, which is the defect this change is meant to fix.
Trigger: a wake mail from the listener to a freshly deployed manager fails. The step errors the body and nothing cancels that manager.
Fix: set tolerate: true on the mail step, add reason to Mailed, and add an unwoken step after mail that calls endRun for each run whose mail failed, using the manage grant.
| await recordOverdueWake(deps.db, tenantId, pr); | ||
| if (await deps.wake(tenantId, pr, head)) deps.log({ level: "info", msg: "manager_woken_overdue", tenantId, source: pr.source, repo: pr.repo, number: pr.number, head }); |
There was a problem hiding this comment.
P2 · Overdue wake is recorded before the wake is attempted, so a failed wake is held off for 30 minutes
recordOverdueWake writes overdueWokenAt before deps.wake runs. If the wake throws, for example because delivery fails, no manager claims the pull request. recentlyWoken then applies OVERDUE_UNSTARTED_GRACE_MS (30 minutes), so later reconcile passes skip this pull request. That contradicts the stated behaviour that the next pass tries again. The timestamp is also recorded when wake returns false because there is no listener.
Trigger: delivery to the listener fails during an overdue wake, or the workspace has no listener.
Fix: call wake first and record overdueWokenAt only after it succeeds, or clear the timestamp when the wake fails or returns false.
| try { | ||
| for (const { pr, head } of await overduePulls(deps.db, tenantId)) { | ||
| await recordOverdueWake(deps.db, tenantId, pr); | ||
| if (await deps.wake(tenantId, pr, head)) deps.log({ level: "info", msg: "manager_woken_overdue", tenantId, source: pr.source, repo: pr.repo, number: pr.number, head }); | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
P2 · One failing pull request aborts the overdue wake for every other pull request in the tenant
A single try/catch wraps the whole loop. An error from recordOverdueWake or wake for one pull request, such as a missing claim or a delivery failure, skips every remaining overdue pull request in this pass. Combined with the grace hold-off above, those pull requests can stay stuck for a long time.
Trigger: two or more overdue pull requests exist and the wake for the first one throws.
Fix: move the try/catch inside the loop body so each pull request is handled and logged independently, e.g. for (const pr of overdue) { try { ... } catch (err) { log(...) } }.
| if (!(await wake(tenantId, pr, head))) log({ level: "warn", msg: "reviewer_result_unrouted", tenantId, source, repo, number, kind: reported.kind, head, runId, reason: "the workspace has no listener for the host" }); | ||
| return c.json({ recorded }); |
There was a problem hiding this comment.
P2 · A wake failure after the result is recorded can leave the pull request with no manager
The event is committed before wake runs, and the wake's errors are not caught, so the route returns 500. The reviewer's report step then fails and moves on to stop. On a retry, recorded is false, but the wake is attempted again; if it keeps failing, the result stays recorded with no manager woken. overduePulls treats the reviewer as reported, so it never wakes a manager either, and the verdict is not posted until the next host event.
Trigger: delivery to the live manager or to the listener throws while the reviewer-result route handles a result.
Fix: catch wake errors and either keep the event unrecorded until the wake succeeds, or make overduePulls also consider recorded results that have no woken manager.
| const due = recorded.some((reviewer) => reviewer.head === head && reviewer.result === undefined && Date.parse(reviewer.deadline) > now.getTime()); | ||
| const rows = await tx.select({ key: managerState.key, state: managerState.state }).from(managerState).where(pending(tenantId, pr)); | ||
| const waiting = passHeadOf(eventsOf(rows), history) !== undefined; | ||
| const kept = { claimId: claim.claimId, claimedAt: claim.claimedAt, reviewers: recorded, history, starts: claim.starts }; |
There was a problem hiding this comment.
P2 · done rewrites the claim's reviewers from the manager's stale take, undoing supersede's removal
supersede() takes older reviewers off the claim under the row lock, and the hub then stops their runs. done() does not reread the claim's reviewers. It writes back the list the manager sent, which was read at take time, so a superseded reviewer on an older head is restored. abandoned() no longer stops older-head reviewers, so that reviewer stays on the claim as running until its deadline, and the code review view keeps showing it.
Trigger: a live manager is mid-pass when a newer head is recorded. supersede removes the older reviewer, then the manager's done commits its take-time reviewers list, which still includes that reviewer.
Fix: in done(), reread the claim's current reviewers under the lock and merge only the fields the manager is allowed to change, instead of writing the take-time list back.
| const current = await newestHead(db, tenantId, pr); | ||
| if (current !== undefined && current !== head) { | ||
| log({ level: "info", msg: "reviewer_result_stale", tenantId, source, repo, number, kind: reported.kind, head, current, runId }); | ||
| return c.json({ recorded: false }); | ||
| } | ||
| const recorded = await recordEvent(db, tenantId, pr, runId, { type: "reviewer-result", head, payload: reported }); |
There was a problem hiding this comment.
P3 · Stale-head check and record are not atomic
newestHead is read outside any lock or transaction, separately from recordEvent. A push recorded between the two lets a superseded result be recorded and wake a manager, and supersede may already have removed that reviewer from the claim.
Trigger: a reviewer reports at the same moment a new head's webhook is recorded.
Fix: perform the head comparison and the record in one transaction, locking the pull request's claim row, e.g. SELECT ... FOR UPDATE on the claim before comparing newestHead and inserting the event.
…eded reviewers off the claim
…for the manager start grace
There was a problem hiding this comment.
This change ends a manager run when its wake mail fails, and makes the hub stop treating a cancelling run as up so a fresh manager can claim the pull request. The most important defect is that the listener's mail step still fails the body on a failed mail and no unwoken step exists, so the core behaviour is missing.
Not reviewed in full: 12 files, over the size limit.
packages/triage-judge/src/triage-judge.test.tspackages/triage-judge/src/triage-judge.tspackages/triage-manager/README.mdpackages/triage-manager/src/actions/judge.tspackages/triage-manager/src/actions/manager.tspackages/triage-manager/src/actions/report.test.tspackages/triage-manager/src/actions/report.tspackages/triage-manager/src/actions/review.tspackages/triage-manager/src/triage-manager.test.tspackages/triage-manager/src/triage-manager.tstests/chain/chain.test.tstests/chain/model.ts
P1 · Listener mail step still fails the body on a failed mail, and no unwoken step cancels the manager · packages/triage-manager/src/triage-listener.ts:27
The description says the listener's mail step no longer fails the body and that a new unwoken step cancels each manager whose mail failed. At head the mail step still uses mailAgentOf(mailDirector, judgeAgent.inference, { tolerate: false }), there is no unwoken step, and Mailed in mail.ts is still { mailed: boolean } with no reason field.
Trigger: the listener's mail to a just-deployed manager fails or times out. The body fails, nothing cancels that run, and the manager stays deployed with no claim-holder to end it.
Fix: make the mail step tolerate failures, return the failure reason on Mailed, and add an unwoken step after mail that calls endRun through the stop-run route for each run whose mail failed, reporting each run and reason in its output.
P1 · A run whose cancellation was requested still counts as up for the claim · apps/hub/src/manager-state.ts:218–221
The description says the hub's claim no longer treats a run as up once its cancellation has been requested. runIsRunning ignores cancellationRequestedAt, and liveManager and claimFor depend on runIsRunning/runIsUp. A cancelling manager therefore keeps the claim, gets mailed events, and has its takes accepted, while the replacement that should claim the pull request is refused.
Trigger: a manager is cancelled, so cancellationRequestedAt is set while its status is still running or allocated with a sidecar. A new pull request event or reviewer result then arrives, liveManager returns that run, and the hub mails it instead of letting the listener deploy a fresh manager.
Fix: have runIsRunning return false when cancellationRequestedAt is set, so liveManager and claimFor skip cancelling runs.
| for (const reviewer of await supersede(deps.db, tenantId, pr)) { | ||
| const entry = { tenantId, source: pr.source, repo: pr.repo, number: pr.number, kind: reviewer.kind, head: reviewer.head, runId: reviewer.runId }; | ||
| try { | ||
| await deps.cancel(tenantId, reviewer.runId, "superseded by a newer head"); | ||
| deps.log({ level: "info", msg: "reviewer_superseded", ...entry }); | ||
| } catch (err) { | ||
| deps.log({ level: "error", msg: "reviewer_supersede_failed", ...entry, error: String(err) }); | ||
| } |
There was a problem hiding this comment.
P2 · Superseded reviewer is removed from the claim before the stop, so a failed cancel is never retried
supersede commits the reviewer's removal from the claim in its own transaction before deps.cancel runs. If the cancel throws, the error is only logged. No record of the run remains, so neither a later push nor the reconcile pass attempts to stop it, and it keeps running until its own deadline.
Trigger: a push records a new head while the stock lifecycle's cancellation call fails, for example on a transient database or network error.
Fix: record the cancellation request in the same transaction as the claim removal, or leave the reviewer on the claim until the stop succeeds, so the reconcile pass can retry it.
| function mergedReviewers(held: readonly Reviewer[], reported: readonly Reviewer[], newest: string | undefined): Reviewer[] { | ||
| const keys = new Set(held.map(reviewerKey)); | ||
| return reported.filter((reviewer) => keys.has(reviewerKey(reviewer)) || newest === undefined || reviewer.head === newest); | ||
| } |
There was a problem hiding this comment.
P2 · done silently drops a newly deployed reviewer on an older head without stopping it
mergedReviewers discards a reported reviewer that the claim does not hold when its head is not the newest. That reviewer was deployed during the pass, after supersede had already run for the push, so it was never on the claim and nothing cancels it. It runs out its full deadline, spending model calls, and its result is later dropped as stale.
Trigger: a push arrives while a manager's pass on the old head is deploying a judge or code review, and the manager then calls done reporting that reviewer.
Fix: when mergedReviewers drops a reported reviewer that is not on the claim, cancel that run through the stop-run route instead of discarding it silently.
| if (result !== undefined) { | ||
| if (now.getTime() - Date.parse(result.receivedAt) >= MANAGER_START_GRACE_MS) overdue.push({ pr, head: head ?? result.head }); | ||
| continue; | ||
| } | ||
| if (recentlyWoken(claim, now) || head === undefined) continue; |
There was a problem hiding this comment.
P2 · Unread-result wake ignores overdueWokenAt, so a manager slow to start is redeployed every reconcile pass
For a claim with an unread reviewer result, the wake is pushed once two minutes have passed since the result. This branch never consults recentlyWoken, so the 30-minute unstarted grace used by the deadline path does not apply. A manager still reaching a sidecar counts as gone once the start grace passes, and another one is deployed on each five-minute pass.
Trigger: the reviewer-result route's wake fails, or the deployed manager takes longer than MANAGER_START_GRACE_MS to reach a sidecar. Each reconcile pass then deploys another manager.
Fix: check recentlyWoken in this branch before pushing the wake, so it uses the same grace as the deadline path.
Supersede now requests a reviewer's cancellation first and drops it from the claim only once that request succeeded, so a failed stop leaves it for the reconcile pass, which stops superseded reviewers on every claim. Done records the reviewers the manager reports and then stops any on an older head, so a reviewer deployed while a push arrived is no longer dropped without a stop. The unread-result wake now holds off while the claim's last overdue wake is within its grace, as the deadline wake does.
When a wake mail from the listener to a manager it just deployed fails for any reason, the listener now cancels that manager. Before this change the manager stayed deployed with nothing to end it.
unwokenstep after it cancels each manager whose mail failed. It uses the stock stop-run route (DELETE /workflows/runs/:runId) through the existingendRunhelper, under themanagegrant stock gives a deployment's creator. The step's output lists each cancelled run with the reason its mail failed, so it shows in the stock run events. The tolerant mail reply typeMailednow carries that reason, and the judge and code review share it.apps/hub/src/manager-state.ts) no longer treats a run as up once its cancellation has been requested. The hub mails such a run nothing and its own take is refused. The pull request's next event deploys a fresh manager that claims it, and so does the reconciler re-queuing the head, which has no verdict.How it was verified:
bun --conditions=intx-src test packages/triage-manager apps/hub: 501 pass, 0 fail. New tests:bun run test:chain: 8 pass, 0 fail. In the new scenario, "a manager whose wake mail failed is cancelled, and the next event deploys a fresh one that posts the verdict", the manager's public key is kept from being recorded, as when a sidecar never acknowledges a deploy. The manager run endscancelled, and the listener'sunwokenoutput names that run with the timeout as the reason. Nothing is written to the pull request meanwhile, and the next push deploys a second manager that posts Triage: Pass and ends the claim.bun run checkandbun run lintpass, with no new warnings.Closes CL-11594