Repository navigation
feat(manager): end the manager while it waits on CI - #706
Conversation
4f11a9c to
977e5bc
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
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).
askDeploycommitsdeployAskedAtbeforedeployForhas sent anything. If that send then fails, the claim still says a manager is on its way. Scenario: the listener is briefly unroutable, sodeployForthrows and the coalescer marks the head failed. The reconciler retries insideMANAGER_START_GRACE_MS. This timeaskDeployreturns false,toListenerreturns true without sending anything, and the head is recorded as queued and loggedtriage_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. RecorddeployAskedAtonly oncedeployForsucceeded, 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.
askDeployreturns 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).
wakeWaitingrequeues every settled head throughrequeue, and nothing caps that atmaxInFlight. 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 throughplanTenant'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 (
deployAskedAton 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, anddonereads pending events under the claim lock. - 60-minute
noWait: usesCI_WAIT_MSfrom 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 withoutaskDeploy. - 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.
|
All four are fixed in 8dbfd23.
Verification:
|
TheGreatAxios
left a comment
There was a problem hiding this comment.
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).
askDeploynow writes anendedclaim withstarts: 0the 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 withexpect(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:
withdrawDeployclears only the ask it made (deployAskedAt === at), so it never removes a newer ask. On a claim a manager took in between,deployingalready counts it as taken, so clearing the field has no effect. freeSlots: counts whatplanTenantcounts: running heads that are not parked, plus plan rows queued or running that are not parked, including heads the plan orpollBuildsalready 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
freeSlotsboth 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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.performunderredeploy:<n>, the same pattern asforward'sdeploy:<n>, so a listener restart mid-step replays the recorded run id instead of deploying again. -
The retry keeps the original mail's
contentandtenantId, and takes its source and repository from that content.manageMailalways writesrepoSource. -
Only runs
unwokenitself cancelled are redeployed, so a manager cancelled for any other reason is not. -
remailmaps 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-managerandtests/chaintypecheck, 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 fromdeployAskedAt) 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 newaskDeploy, 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
left a comment
There was a problem hiding this comment.
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.
99ad8e0 to
25176cd
Compare
…ing heads within the in-flight cap
25176cd to
8a27b7a
Compare
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
waiting_ci_readwith how many heads it read.ci_settled) once CI is no longer pending, or once the head still has no checks after the start grace.noWait.check_runnever arrived.reviewer-resultroute 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.tscovers both cases, so this PR does not change it.ci_settledwake arriving together each deployed a manager, and the second was refused at take.How it was verified
ci_settled;waitingSince, and is not mailed again;waiting-cifor it.bun checkreports no type errors for triage-manager, hub andtests/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.rereadandwaiting, run one at a time with nothing else running: 3 of 3 passes each.Closes CL-11632