Repository navigation
fix(hub): hold webhook mail while the listener is being replaced - #704
TheGreatAxios merged 6 commits into
Conversation
TheGreatAxios
left a comment
There was a problem hiding this comment.
Holds a head whose mail reached no listener as unrouted on its triage-state row, retries it without backoff or attempt spend, and starts a pass on a coalescer miss, every 15 s while a head stays unrouted, and when a roll's listener runs.
- apps/hub/src/github/reconcile-plan.ts:236 (high). An unrouted head has no cap at all: it is due on every pass and
attemptsnever grows (line 319), and the pass that leaves it unrouted asks for another in 15 s. This stops only when no listener is found, butliveListenerstill returns a listener whose status isrunningwhile its sidecar is disconnected.createListenerWatch(workflow-release/triggers.ts) leaves that listener to stock "until it stays away past its deployment's limit", and every send to it throwsRunTriggerUnroutableError. Scenario: the hub restarts and the listener's sidecar never reconnects. Every 15 s, for each open head up tomaxInFlight, the hub then does a claim save, afailDeliverysave, an error-levelforward_failedlog and the intake work in the next bullet, with nothing logged once and no ceiling. Before this PR the same head spent its attempts and stopped. The same thing happens throughmailedLive(manager-intake.ts:220): a manager whose run is stillrunningbut whose sidecar is gone throws the same error, so a dead manager is classed as "reached no listener" and retried every 15 s. Thelivehook never ends that loop, because the listener is fine. Suggest counting unrouted tries against a separate ceiling, or logging once and falling back to the normal backoff after N tries. - apps/hub/src/manager-intake.ts:218 (medium). "Nothing was sent" does not hold on the reconciler path.
toListenerrecords the event, runssupersede, and (with code review on) reads the pull request's diff from the host beforedeployFor(line 223) throws the unroutable error. Reconciler mail has nodeliveryId, sonamed()mints arandomUUID()and every retry inserts a newmanager_stateevent row. In the loop above that is about 240 rows an hour per head, plus a host diff read each time, all while the head is supposedly held untouched. Retries need a stable event id per held head, or the classification needs to run before the event is recorded. - apps/hub/src/github/triage-queue.ts:19 (low). It misses the reverse case. Stock rejects mail to a run that is
stoppingorterminal(workflow_run_stopping/workflow_run_terminal, run-grant-materialization.tsunavailableRunResult), and @corbits/webhooks turns that into a plainError(grants.message)(deliver.js:44). Nothing was sent, yetreachedNoRunis false. Scenario: a roll cancels the old listener betweenliveListenerand materialize. The head then spends an attempt and backs off, which is the replacement race this PR targets. Theskipoutcome (the run row is already gone) is the same. - apps/hub/src/github/triage-reconciler.ts:297 (low). Any unrouted row the in-flight limit held back keeps the whole loop at 15 s, even though no listener is missing. Scenario: a workspace at
maxInFlightwith long manager runs and one unrouted head in the backlog runs a fullreconcileAll(every tenant: watch, supersede, overdue wake,endNeverRan, listings) every 15 s until a slot frees.markBaseMoved(triage-queue.ts:175) also keepsunroutedon a stale row that is not yet due, which does the same thing forbaseMoveWindowMs. - tests/chain/unrouted.test.ts:56 (medium). The test does not cover slice 1 or the 15 s loop. The coalescer's failed row has
attempts: 0andbackoffMs(0) === 0, so withoutunroutedit is still due on the first pass afterlive. Reverting the reconcile-plan/triage-queue changes would most likely leave this test green (unverified: reasoned frombackoffMsandisDue, not run). Thesent - live <= 15 sbound also has no headroom. If the first pass afterworkflow_release_livehitsDeploymentNotReadyError(deploymentStatealready returnslivewhen there is no allocation row), the retry fires 15 s after that pass ends, so a slow CI pass fails the assertion. A load-bearing version would have the first post-live mail fail as unrouted and assert it went out on the retry, with the bound set to retry plus pass slack. - apps/hub/src/workflow-release/rollout.ts:177 (nit).
livealso fires on the already-current path, which logs noworkflow_release_live. docs/ARCHITECTURE.md describes the hook only as "a roll starts a pass as soon as its listener runs, loggingworkflow_release_live". It is harmless because kicks coalesce, but the docs should say it also fires when a roll finds its listener already current. - packages/triage-contracts/src/triage-state.ts:69 (nit). A row with
unrouted: falsemakesparseRowdrop the whole row. The other optional fields degrade to absent (optionalString), so dropping one head's whole state over this flag is out of line with them.
|
All seven findings are fixed in c57c37d and 750843d.
|
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at 750843d. The hub now holds only mail the listener took none of as unrouted. Before recording anything, the intake checks that the listener is routable. Stopping, terminal and skip runs count as unrouted. The 15 s retry is capped at 20 passes per head. The chain test now makes the reconciler's own mail fail before the replacement runs. Findings 1-7 are closed. markBaseMoved drops the flag, unrouted: false parses as absent, and a manager that took no mail keeps its attempts and backoff. A held mail is never sent twice: a head is only marked unrouted when nothing went out. The chain test asserts the verdict, the label and the timing, so it stays load-bearing.
apps/hub/src/manager-intake.ts:87(medium): wrappingDeploymentNotReadyErrorinListenerUnroutedErrorchanges what the portal sees while a listener is starting, from 409deployment_not_readyto 503service_not_running. That changes behaviour on two routes:- Backlog start (
github/backlog-triage.ts:36).ListenerUnroutedErroris aNoLiveDeploymentError, so the request falls through to "Triage is not running on GitHub. Settings, under Triage, shows why." The portal'srunBacklog(apps/web/src/lib/portal.tsx:267) waits and retries only on 409deployment_not_ready(untilDeploymentReady). So a backlog run started right after a listener deploys, such as on enabling a repository, now shows an error instead of "Waiting for the workflow to be ready…". The new pre-check atmanager-intake.ts:232gives the same 503 while the new listener's sidecar has not connected yet. - Held-Do approvals (
github/held-dos.ts:143). Mail to the listener takes the same 503 path, so the 409 branch there can no longer fire for it. - Reproduced 3/3: the real intake plus
createBacklogStart, withdeliverthrowingDeploymentNotReadyErrorasreadyMaterializerdoes for a starting listener, answers503 service_not_running. On main the same error reachesbacklog-triage.ts:36unwrapped and answers 409. - Fix: keep "still starting" apart. For example, check
err.cause instanceof DeploymentNotReadyErrorbefore theNoLiveDeploymentErrorbranch in both routes, or giveListenerUnroutedErrora not-ready variant. Also answer the pre-check's "not routable yet" for a listener that has just started as not ready.
- Backlog start (
apps/hub/src/github/triage-reconciler.ts:33(low):createUnroutedTriesdeletes a key only when the reconciler gets the head's mail through (took) or a roll's listener runs (reset). A head that closes, gets a new commit, or goes out through the coalescer keeps its entry until the host's next roll. That is a slow leak on a host that rarely rolls. It also means a head the coalescer delivered after it ran out of tries gets no short retries if it is held again later. Clear the entry when the coalescer's send succeeds, and drop keys for heads no longer in the plan.
|
Both addressed in c13b086.
|
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at c13b086. A person's request made while the host's listener is starting or being replaced now answers 202. Triage again holds its head, a backlog run holds each open non-draft head, and a held Do's approval is recorded and holds its head. Each one starts a pass at once. A host with no listener at all still answers 503 with the Settings hint.
- The repro now gets 202. Starting a backlog twice in the gap leaves one
failed/unroutedrow per head withattempts0, andplanTenantqueues it with no attempt spent. This held 3/3 both with no route to the listener and with a listener that is routable but still starting. - With no route, the pre-check records no events. When the listener is routable but still starting, each backlog start records one event per head before the send fails. That is harmless: the manager's first pass takes every pending event at once.
- Rows are not doubled. In
held-dos, the 502 catch-all wraps only the intake call, and authorization runs before it. The unrouted retry counter is now trimmed to held open heads each pass (keep). Nothing in apps, packages, docs or tests still uses the deleted 409 helpers.runBacklogstill throws a refusal to its caller. The docs match. hubandwebtypecheck. The touched suites pass 111/111 and the web tests 291/291.
No findings.
…t pass A head whose mail found no listener, a listener still starting, or a listener the hub holds no route to is marked unrouted. The reconciler queues it again on its next pass with a listener up, without backoff, and those tries add nothing to its attempts.
A pass or coalescer mail that left a head unrouted asks for another pass after the 15-second retry, and a roll starts a pass as soon as its listener runs, logging workflow_release_live, before it waits for the listeners it replaces to stop.
…s its verdict The scenario stops the listener through stock's stop-run route on the default five-minute pass, opens a pull request while none runs, and checks the head is mailed within 15 seconds of the replacement running and gets its verdict. docs/ARCHITECTURE.md describes unrouted heads.
…r it, and bound its short retry Mail a live manager did not take fails as usual. The intake checks stock's router before recording an event for a pull request with no manager up, and a run Interchange refuses mail to as stopping, ended or gone is held like an unroutable one. A head gets the 15-second retry for at most 20 passes in a row, then logs triage_unrouted_waiting once and waits for the ordinary pass until a listener takes its mail or a roll finds its listener running. Only a pass that tried and failed asks for the short retry, a base move clears the flag, and unrouted: false parses as absent.
…he replacement listener runs The scenario freezes the listener's sidecar until stock drops it, opens a pull request whose webhook and reconciler mail both reach no listener, stops the listener, and checks the head is mailed within 45 seconds of the replacement running and gets its verdict.
A backlog start, Triage again and a held Do's approval made while the listener takes no mail answer 202: their heads are written as queued triage rows marked unrouted, and the reconciler mails them once a listener is up. The approval is recorded first, so nothing a person did is dropped. The 409 deployment_not_ready answer, the 503 for listener mail during the gap and the portal's readiness polling are removed; a host with no listener still answers 503. A head's short-retry count is forgotten once its row is no longer held.
c13b086 to
7c63e0d
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review of the rebase onto main at 7c63e0d. I checked only the conflict resolutions in coalescer.ts, reconcile-plan.ts, triage-queue.ts, triage-reconciler.ts, triage-state.ts, server.ts and backlog-triage.ts, diffed against origin/main. Nothing from #702 or #703 is lost.
- The liveness wiring is intact: the take ledger, its seed at boot, the 60 s timer and its shutdown clear, and the
stalledroute field. - main's re-queue in
planTenantkeepsundeliveredSince.withStatusnow dropsundeliveredSinceandunroutedtogether.queuedAgainandmarkBaseMoveddrop onlyunrouted. failDeliverysetsundeliveredSince, and itsdelivery failed:wording still matchesundelivered, so the alarm sees held heads.holdHeadcalls the four-argumentqueueHead.
I reproduced the alarm for a head that was held and then delivered against Postgres, 3/3. It raises while the head is held. It stays up after a pass is taken while the re-queued row is still queued. It clears on the first check after the reconciler sees the run. That is the same delay of up to one reconcile pass as on main.
Backlog started twice in the gap is unchanged, 3/3: 202 both times, one held row per head, and no attempts spent.
hub, web and tests/chain typecheck. The touched suites pass 137/137 with --timeout 60000.
No findings.
A pull request opened while a host's listener was being replaced used to wait for the next reconcile pass, which is five minutes by default. Webhook mail is addressed to the listener's run, and stock holds no mail for a run that has gone. Now the hub holds that head in its triage state row and mails it as soon as a listener runs again.
What changed:
unroutedon its triage state row when its mail reached no listener. That covers three cases: the host had no listener; the listener took no mail because it was starting, stopping or gone; or the hub held no route to it. Mail that a live manager did not take fails as before.triage_unrouted_waitingonce and waits for the ordinary pass.unrouted.deployment_not_readyor 503. Backlog triage writes each open, non-draft head as a held row; Triage again holds its head; a held Do's approval is recorded and holds the head it was made on. Each starts a pass at once, and the reconciler mails them once a listener is up. A host with no listener at all still answers 503 with its Settings hint. Review again and integrations are unchanged.deployment_not_readynote.How it was verified:
tests/chain/unrouted.test.ts, on the default five-minute reconcile interval:unroutedflag, the reconciler's failed try spends the head's first attempt and backs it off for five minutes, so the head would not be mailed within 45 seconds and the test would fail.bun --conditions=intx-src test apps/hub packages/triage-contracts): 693 pass, 4 fail, at a load average of about 38 from other builders.bunx bun@1.4.4 check --no-scriptfor apps/hub and tests/chain: no type errors. oxlint is clean on the changed files.Closes CL-11630