Skip to content

feat(manager): end the manager while it waits on CI - #706

Merged
TheGreatAxios merged 11 commits into
mainfrom
cl-11632-end-the-manager-while-it-waits-on-ci
Oct 28, 2026
Merged

TheGreatAxios merged 11 commits into
mainfrom
cl-11632-end-the-manager-while-it-waits-on-ci

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

A pull request's manager no longer stays up while its head waits only on CI. It posts that the head is waiting, and then it ends. It stays up only while a judge or code review it sent out is still running and inside its deadline.

What changed

  • Manager: a pass whose head waits only on the host (CI starting, CI running, or mergeability) posts the waiting state and ends. Its done call no longer asks to wait.
    • When a reviewer's result arrives and the head is still waiting on the host, the manager posts the waiting state and ends. The judge's answer stays on its reviewer, so the pass after CI uses it again on the facts at that point.
    • The manager's own 5-minute and 2-minute re-reads are removed.
    • The CI wait (60 minutes) and the CI start grace (2 minutes) are now shared constants in the contracts package, used by both the manager and the hub.
  • Hub, waking a waiting head: each reconcile pass reads the checks of every head whose manager ended while waiting, once per head per pass. It logs waiting_ci_read with how many heads it read.
    • It queues the head again (ci_settled) once CI is no longer pending, or once the head still has no checks after the start grace.
    • After 60 minutes it queues the head without reading, and the woken manager decides with noWait.
    • This is what settles Bitbucket heads, whose builds send no webhook, and GitHub heads whose check_run never arrived.
  • Hub, portal state: the portal's "Waiting on CI" now also comes from the claim's last pass on the newest head after the manager ended. That pass counts as completed, so it takes no in-flight slot.
  • 3-hour stop clock: it is kept. The clock starts at the claim, and a manager that ends while waiting on CI starts a new claim next time. So the clock now only limits a manager that its reviewers keep alive. The docs say so.
  • Reviewer results: the hub's reviewer-result route already wakes the pull request's manager after recording the result. It mails the live manager, or has the listener deploy one. manager-intake.postgres.test.ts covers both cases, so this PR does not change it.
  • Docs: ARCHITECTURE, PRODUCT, BITBUCKET_DATACENTER and the user manual.
  • Hub, one manager per wake: when a pull request's manager has ended, the hub records on its claim that it asked the listener for a new one. A second mail that arrives before that manager takes the claim is recorded but does not deploy another manager; the one on its way reads it. The hold lasts until a manager takes the claim, or 2 minutes at most. Before this, a check event and the reconciler's ci_settled wake arriving together each deployed a manager, and the second was refused at take.

How it was verified

  • Manager tests cover these cases:
    • a head waiting on the host with no reviewer out posts the waiting state and ends;
    • a manager with a reviewer out stays up and polls;
    • a reviewer result that arrives while the host is still pending posts the waiting state, ends, and keeps the judgement;
    • a manager woken after the start grace decides the head as having no CI;
    • an idle manager whose head is waiting ends.
  • The reconciler test covers four cases:
    • CI pending: no wake, and one read is logged;
    • CI finished: the head is queued with ci_settled;
    • no checks within the grace: no wake; after the grace: woken;
    • past 60 minutes: queued without a read.
  • Postgres tests cover two cases:
    • an ended waiting claim shows as completed with waitingSince, and is not mailed again;
    • the triage verdicts route answers waiting-ci for it.
  • Targeted run of manager, workflows, contracts, reconciler, reconcile-plan, manager-runs, triage-verdicts, manager-routes and manager-intake tests: 722 pass, 0 fail.
  • bun check reports no type errors for triage-manager, hub and tests/chain.
  • manager-intake.postgres.test.ts: a mail for a pull request whose manager ended asks the listener once until a manager takes the claim, then asks again. Hub suite: 455 pass, 0 fail.
  • Chain reread and waiting, run one at a time with nothing else running: 3 of 3 passes each.

Closes CL-11632

@linear-code

linear-code Bot commented Oct 28, 2026

Copy link
Copy Markdown

CL-11632

@TheGreatAxios
TheGreatAxios force-pushed the cl-11632-end-the-manager-while-it-waits-on-ci branch from 4f11a9c to 977e5bc Compare October 28, 2026 07:08

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ends a manager whose head waits only on CI. The hub's reconcile pass wakes a fresh manager once CI settles, the start grace passes with no checks, or the hour is up, and the listener is asked once for a manager still on its way.

  • apps/hub/src/manager-state.ts:614 (medium). askDeploy commits deployAskedAt before deployFor has sent anything. If that send then fails, the claim still says a manager is on its way. Scenario: the listener is briefly unroutable, so deployFor throws and the coalescer marks the head failed. The reconciler retries inside MANAGER_START_GRACE_MS. This time askDeploy returns false, toListener returns true without sending anything, and the head is recorded as queued and logged triage_requeued. No manager ever comes. The head sits until the reconciler marks it never started (10 minutes), then waits out its backoff. Each retry also spends an attempt, although no request reached the listener. createManagerWake (manager-intake.ts:86) has the same gap: a reviewer's result is recorded and reported as woken, but nothing was asked for. Record deployAskedAt only once deployFor succeeded, or clear it when it throws.
  • apps/hub/src/manager-state.ts:611 (low). The one-request rule applies only to a pull request that already has a claim. askDeploy returns true without writing anything when there is no claim row, and that row is created only when a manager takes it. So two mails for a brand-new pull request that land before its first manager starts still ask the listener twice. For example, the coalescer's window closes, then a check event arrives outside the window. The commit message ("two mails arriving before the new manager takes make one listener deploy request") says otherwise. Either the listener already removes the duplicate, in which case this commit is not needed for that case, or the first ask needs a placeholder claim to record on.
  • apps/hub/src/github/triage-reconciler.ts:211 (medium). wakeWaiting requeues every settled head through requeue, and nothing caps that at maxInFlight. CI waits now all end this way, so this is the main path. Scenario: a shared CI run, or a CI outage, finishes for 30 waiting pull requests. The next pass mails all 30 at once, and the listener deploys 30 managers, each judging. The tenant's in-flight limit is skipped, and the 8 local sidecars are exceeded, so the rest go to sandbox overflow or stock's 10-attempt placement retry and fail. Before this PR, each of those heads had a live manager re-reading CI, so they woke one by one. Queue these through planTenant's budget instead, or cap the requeues per pass at the free in-flight slots.
  • docs/ARCHITECTURE.md (nit). The last commit's behaviour is not documented: one listener request per manager on its way (deployAskedAt on the claim, within the start grace). Only the claim schema shows it.

Checked and fine:

  • CI event between a pass ending and the claim write: no lost wake. The event is recorded before mailedLive, and done reads pending events under the claim lock.
  • 60-minute noWait: uses CI_WAIT_MS from the claim history on both sides, from one constant in triage-contracts. No other copy of the 2- or 60-minute values remains.
  • Ended-waiting claim: takes no in-flight slot (observed completed).
  • Bitbucket: goes through the same claim, reconciler and portal paths.
  • Re-posting after CI: the chain tests assert one comment, replaced in place.

Tests:

  • The new reconciler test fails without wakeWaiting, and the manager-intake test fails without askDeploy.
  • Typecheck is clean for apps/hub, packages/triage-manager and tests/chain.
  • On 977e5bc, packages/triage-manager plus the manager-intake postgres test and the reconciler/plan/loop tests pass: 167 pass, 0 fail.
  • The chain stack was not run.

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

All four are fixed in 8dbfd23.

  • Ask recorded before the send (manager-state.ts:614): askDeploy now returns the time it asked. If the mail to the listener then fails, deployAsked withdraws that ask, but only while the claim still holds that same time, and rethrows. Intake and createManagerWake both go through it. New test: when the listener is unroutable, the mail rejects, and a retry within the start grace mails the listener.
  • No claim before the first manager (manager-state.ts:611): askDeploy now creates the claim row under the same lock: ended, starts: 0, with deployAskedAt set. A claim counts as taken only once it has a start whose claimedAt is at or after the ask. The intake test is extended: a brand-new pull request mailed twice asks the listener once. The two intake tests that expected one listener mail per repeated mail now expect a single ask.
  • No in-flight cap on waking (triage-reconciler.ts:211): settledWaiting now only collects settled heads across the tenant's repositories. After pollBuilds, they are requeued longest-waiting first, up to freeSlots (maxInFlight less the heads queued or running, parked managers aside). The rest wait for a later pass. New test: with maxInFlight 1 and two settled heads, only the older one is mailed.
  • Docs (ARCHITECTURE.md): one sentence added on the single listener ask per manager start, held until a take or for the 2-minute grace, and withdrawn when the send fails.

Verification:

  • Typecheck is clean for apps/hub, packages/triage-manager and tests/chain.
  • apps/hub plus packages/triage-manager: 543 pass, 2 fail. Both failures are the beforeAll 5s timeout in workflow-release/rollout.postgres.test.ts, which this PR does not touch. It also fails when run alone, while it creates its database and runs migrations. That it is unrelated to this PR is an unverified hypothesis: I did not run it on main.
  • manager-intake postgres: 14 pass. Reconciler and plan tests: 59 pass.
  • The chain stack was not run.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Withdraws a listener ask whose mail failed, records the ask on a new claim with no start for a pull request no manager has taken, and wakes waiting heads oldest first within the tenant's free in-flight slots. This closes all three findings from the last round.

  • tests/chain/sidecar-cap.test.ts:104 (high, CI red). askDeploy now writes an ended claim with starts: 0 the first time the listener is asked. The cap scenario still asserts that a pull request whose manager could not be placed has no claim. CI run 40650771034, chain (4/5), fails there with expect(received).toBeUndefined() and a received claim. That is the behaviour this commit intends, so the assertion is what needs to change: assert the claim was never taken, e.g. toMatchObject({ status: "ended", starts: 0 }).

Checked and fine:

  • Withdraw race: withdrawDeploy clears only the ask it made (deployAskedAt === at), so it never removes a newer ask. On a claim a manager took in between, deploying already counts it as taken, so clearing the field has no effect.
  • freeSlots: counts what planTenant counts: running heads that are not parked, plus plan rows queued or running that are not parked, including heads the plan or pollBuilds already queued this pass. So woken heads only take what is left.
  • The two older intake tests: the new expectations are right, not masking anything. Six mails, and two gated mails, for one pull request still record every event (both tests assert the events), and now ask the listener once within the start grace, as designed.
  • New tests are load-bearing: each fails without its fix. Without the withdraw nothing is mailed on the retry. Without freeSlots both heads are mailed. Without the inserted claim a fresh pull request is asked twice.
  • Local checks: typecheck is clean for apps/hub, packages/triage-manager and tests/chain. manager-intake.postgres, manager-runs.postgres, reconcile-loop, reconcile-plan and triage-reconciler pass, 79/79.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Asserts that the second pull request's claim was asked for and never taken, which resolves the sidecar-cap finding above.

  • tests/chain/sidecar-cap.test.ts:104 (nit). The new comment restates the assertion under it; drop it.
  • CI on 43ac3fb (run 40651424594). chain (1/5) failed in local-sidecars.test.ts, timing out on "the second manager's claim". The hub log shows the manager's sidecar push rejected (pack rejected by receiver), the manager redeployed at generation 2, and the old sidecar disconnecting. The same test passed on 8dbfd23, so this looks like a sidecar redeploy problem rather than this PR's code (unverified). Rerun the shard before merging; if it fails again, check whether askDeploy's 2-minute start grace kept the hub from asking the listener again while the redeployed manager came up.

@TheGreatAxios
TheGreatAxios marked this pull request as ready for review October 28, 2026 07:54

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review of c21b4ef. When a manager's wake mail fails, the listener now deploys one more manager for that pull request and wakes it with the same mail. If that mail fails too, unwokenAgain cancels it and no third is deployed.

  • The redeploy runs through ctx.perform under redeploy:<n>, the same pattern as forward's deploy:<n>, so a listener restart mid-step replays the recorded run id instead of deploying again.

  • The retry keeps the original mail's content and tenantId, and takes its source and repository from that content. manageMail always writes repoSource.

  • Only runs unwoken itself cancelled are redeployed, so a manager cancelled for any other reason is not.

  • remail maps over at most the body's own forward mails.

  • The unit tests assert the second wake's content and that no third deploy happens. The chain scenario still asserts the exact deployed list (both cancelled, then the fresh manager the next event deploys) and the Pass verdict, so it proves no extra manager beyond the retry.

  • triage-manager and tests/chain typecheck, and the manager tests pass 96/96.

  • packages/triage-manager/src/actions/listener.ts (retry; low, traced from the code, not run): the hub's hold on a pull request (deploying(), MANAGER_START_GRACE_MS = 2 min from deployAskedAt) is as long as the first wake window (MANAGER_READY_MS = 2 min). So it lapses just as the retry deploys. Any mail for the pull request in the next 2 minutes, while the retried manager is still starting, gets a new askDeploy, and the listener deploys a second manager alongside it. When the retry succeeds, which is the slow-sidecar case it exists for, one of the two is refused at take and stops. It is harmless, but it spends a deploy and a sidecar slot, and it undoes the one-manager-per-wake hold for that window. The ARCHITECTURE sentence 'after a double failure the claim's 2-minute start grace lapses' is true, but the grace actually lapses after the first failure. Fix: lengthen the hub's grace to cover both wake windows, or say in the docs that a mail during the retry may deploy a parallel manager.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review of 99ad8e0. deploying() now holds a deploy ask for DEPLOY_ASK_HOLD_MS (2 × MANAGER_READY_MS + 30 s = 4.5 min), and MANAGER_READY_MS is shared from triage-contracts with the listener. That closes the low finding.

Traced case: the hub asks at T0. The first wake fails around T0+2m and the retried manager's wake would fail around T0+4m, plus deploy and mail latency inside the 30 s slack. A mail at T0+3m now finds deploying() true and is only recorded, and the retried manager takes it with the claim. Overdue wakes during that window go through askDeploy too and are held the same way. Once a double failure lapses the hold, the next mail or overdue pass asks again.

MANAGER_START_GRACE_MS stays 2 min for what it measures: how long a just-deployed run counts as up without a sidecar (runIsUp), and the overdue-wake and reviewer-result graces. None of those overlaps the ask hold.

hub and triage-manager typecheck. The manager-state and manager-intake Postgres tests pass 43/43.

No findings.

@TheGreatAxios
TheGreatAxios force-pushed the cl-11632-end-the-manager-while-it-waits-on-ci branch from 99ad8e0 to 25176cd Compare October 28, 2026 08:23
@TheGreatAxios
TheGreatAxios force-pushed the cl-11632-end-the-manager-while-it-waits-on-ci branch from 25176cd to 8a27b7a Compare October 28, 2026 08:47
@TheGreatAxios
TheGreatAxios merged commit 56f5137 into main Oct 28, 2026
17 checks passed
@TheGreatAxios
TheGreatAxios deleted the cl-11632-end-the-manager-while-it-waits-on-ci branch October 28, 2026 08:56
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