Skip to content

fix(hub): hold webhook mail while the listener is being replaced - #704

Merged
TheGreatAxios merged 6 commits into
mainfrom
cl-11630-hold-webhook-mail-while-the-listener-is-being-replaced
Oct 28, 2026
Merged

TheGreatAxios merged 6 commits into
mainfrom
cl-11630-hold-webhook-mail-while-the-listener-is-being-replaced

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • A head is marked unrouted on 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.
  • Before recording a pull request's event, the intake checks with stock's router that the listener is routable. It only checks when one of the pull requests named has no manager up. When the listener can't take the mail, nothing is recorded or read.
  • The reconciler queues an unrouted head on its first pass with a listener up, without backoff. Those retries add nothing to the head's attempts.
  • A coalescer mail that reached no listener starts a pass at once. A pass that tried and failed to reach a listener runs again 15 seconds later, for at most 20 passes in a row per head. After that the head logs triage_unrouted_waiting once and waits for the ordinary pass.
  • A roll starts a pass as soon as its new listener runs, or when it finds the host's listener already current and running. That also starts the 20 passes over.
  • A base branch move clears unrouted.
  • A request a person makes while the listener takes no mail now answers 202 instead of 409 deployment_not_ready or 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.
  • The portal no longer waits and retries a backlog start, so "Waiting for the workflow to be ready…" is gone.
  • A head's short-retry count is forgotten once its row is no longer held.
  • A held head is triaged again when its mail goes out: the same head, so the judge's verdict is reused and the comment is replaced in place. Mailing only the approval to the new manager, without the re-triage, is left for a later issue.
  • docs/ARCHITECTURE.md describes all of this. The manual's backlog paragraph says open pull requests are checked once Triage is running, and docs/SELF_HOST.md drops the deployment_not_ready note.

How it was verified:

  • New chain scenario tests/chain/unrouted.test.ts, on the default five-minute reconcile interval:
    • It freezes the listener's sidecar until stock drops it and opens a pull request. Both the webhook's mail and the reconciler's mail reach no listener.
    • It then stops the listener, so the hub rolls a replacement.
    • It checks that the head is mailed within 45 seconds of the replacement running, and that the Pass verdict and label are posted.
    • It passed (154.9 s). Without the unrouted flag, 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.
  • New plan test: an unrouted head at the attempt cap is queued again at once and keeps its attempts.
  • New reconciler test: a head that keeps reaching no listener asks for the short retry for 19 passes in a row. It then logs once and stops asking until a reset.
  • New Postgres intake test: mail for a pull request with no manager up, while the listener is unroutable, records and mails nothing.
  • New Postgres tests: a backlog started while the listener is starting answers 202, its open heads are held and the next pass mails them; Triage again in the gap answers 202 and is mailed by the next pass; a held Do approved in the gap answers 202 and holds its head; the intake records that approval while the listener is unroutable.
  • New reconciler test: a held head's short retries are forgotten once it is no longer held.
  • New portal test: a backlog start is one request with no retry.
  • After the last change: touched hub tests 143 pass, server and code review Postgres suites 12 pass, web tests 280 unit and 11 DOM pass; hub and web typecheck clean; oxlint clean.
  • Hub and contract tests with Postgres (bun --conditions=intx-src test apps/hub packages/triage-contracts): 693 pass, 4 fail, at a load average of about 38 from other builders.
    • Run on their own, the server burst test and rollout.postgres.test.ts pass.
    • One triage-verdicts.postgres.test.ts test, in code this pull request does not touch, still hits its 5-second timeout under that load.
  • bunx bun@1.4.4 check --no-script for apps/hub and tests/chain: no type errors. oxlint is clean on the changed files.

Closes CL-11630

@linear-code

linear-code Bot commented Oct 28, 2026

Copy link
Copy Markdown

CL-11630

@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.

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 attempts never grows (line 319), and the pass that leaves it unrouted asks for another in 15 s. This stops only when no listener is found, but liveListener still returns a listener whose status is running while 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 throws RunTriggerUnroutableError. Scenario: the hub restarts and the listener's sidecar never reconnects. Every 15 s, for each open head up to maxInFlight, the hub then does a claim save, a failDelivery save, an error-level forward_failed log 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 through mailedLive (manager-intake.ts:220): a manager whose run is still running but whose sidecar is gone throws the same error, so a dead manager is classed as "reached no listener" and retried every 15 s. The live hook 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. toListener records the event, runs supersede, and (with code review on) reads the pull request's diff from the host before deployFor (line 223) throws the unroutable error. Reconciler mail has no deliveryId, so named() mints a randomUUID() and every retry inserts a new manager_state event 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 stopping or terminal (workflow_run_stopping / workflow_run_terminal, run-grant-materialization.ts unavailableRunResult), and @corbits/webhooks turns that into a plain Error(grants.message) (deliver.js:44). Nothing was sent, yet reachedNoRun is false. Scenario: a roll cancels the old listener between liveListener and materialize. The head then spends an attempt and backs off, which is the replacement race this PR targets. The skip outcome (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 maxInFlight with long manager runs and one unrouted head in the backlog runs a full reconcileAll (every tenant: watch, supersede, overdue wake, endNeverRan, listings) every 15 s until a slot frees. markBaseMoved (triage-queue.ts:175) also keeps unrouted on a stale row that is not yet due, which does the same thing for baseMoveWindowMs.
  • 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: 0 and backoffMs(0) === 0, so without unrouted it is still due on the first pass after live. Reverting the reconcile-plan/triage-queue changes would most likely leave this test green (unverified: reasoned from backoffMs and isDue, not run). The sent - live <= 15 s bound also has no headroom. If the first pass after workflow_release_live hits DeploymentNotReadyError (deploymentState already returns live when 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). live also fires on the already-current path, which logs no workflow_release_live. docs/ARCHITECTURE.md describes the hook only as "a roll starts a pass as soon as its listener runs, logging workflow_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: false makes parseRow drop 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.

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

All seven findings are fixed in c57c37d and 750843d.

  • reconcile-plan.ts:236 (loop with no ceiling):
    • Only mail the listener took none of is held. A live manager that takes no mail now fails as an ordinary attempt with backoff.
    • A head gets the 15-second retry for at most 20 passes in a row. After that it logs triage_unrouted_waiting once at warn and waits for the ordinary pass.
    • The count starts over when a listener takes the head's mail or a roll finds the host's listener running.
    • forward_failed for held mail is now at info.
  • manager-intake.ts:218 (writes on a send that cannot happen): the intake now checks with stock's router that the listener is routable before recording an event for a pull request with no manager up. When it isn't, nothing is recorded, superseded or read from the host. I didn't mail the listener first, because a manager it deploys could take before its event exists. A send that still fails after the check is classified the same way.
  • triage-queue.ts:19 (stopping or terminal runs): @corbits/webhooks is the published package, not ours to edit. Our materializer wrapper now raises RunUnavailableError for a run that is stopping, terminal or gone, and on the listener path that counts as held.
  • triage-reconciler.ts:297 (15-second loop from rows held back): only a pass that tried and failed an unrouted send asks for the short retry. markBaseMoved clears the flag.
  • unrouted.test.ts:56 (test does not cover slice 1): the scenario now runs on the default five-minute interval. Both the webhook's and the reconciler's mail fail before the listener is replaced, and it asserts the head is mailed within 45 seconds of the replacement running. Without the flag, the reconciler's failed try backs the head off for five minutes, so the test would fail. It passed once locally (154.9 s).
  • rollout.ts:177 (docs): docs/ARCHITECTURE.md now says the pass also starts when a roll finds the listener already current.
  • triage-state.ts:69: unrouted: false now parses as absent.

@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 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): wrapping DeploymentNotReadyError in ListenerUnroutedError changes what the portal sees while a listener is starting, from 409 deployment_not_ready to 503 service_not_running. That changes behaviour on two routes:
    • Backlog start (github/backlog-triage.ts:36). ListenerUnroutedError is a NoLiveDeploymentError, so the request falls through to "Triage is not running on GitHub. Settings, under Triage, shows why." The portal's runBacklog (apps/web/src/lib/portal.tsx:267) waits and retries only on 409 deployment_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 at manager-intake.ts:232 gives 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, with deliver throwing DeploymentNotReadyError as readyMaterializer does for a starting listener, answers 503 service_not_running. On main the same error reaches backlog-triage.ts:36 unwrapped and answers 409.
    • Fix: keep "still starting" apart. For example, check err.cause instanceof DeploymentNotReadyError before the NoLiveDeploymentError branch in both routes, or give ListenerUnroutedError a not-ready variant. Also answer the pre-check's "not routable yet" for a listener that has just started as not ready.
  • apps/hub/src/github/triage-reconciler.ts:33 (low): createUnroutedTries deletes 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.

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Both addressed in c13b086.

  1. The starting listener's 409 is not restored. On the owner's call, a backlog start, Triage again and a held Do's approval made while the listener takes no mail now answer 202. Their heads are written as queued triage rows marked unrouted, which the existing 15 s retry mails once the listener is up. The approval is recorded first. The 409 deployment_not_ready path, the 503 for listener mail during the gap and the portal's readiness polling are gone. A host with no listener still answers 503. Your repro is now the Postgres test "a backlog started while the listener is starting answers 202, and its open heads are queued and mailed once the listener is up".
  2. deliverRepo now forgets the retry count of any head that is no longer held, with a reconciler test.

@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 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/unrouted row per head with attempts 0, and planTenant queues 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. runBacklog still throws a refusal to its caller. The docs match.
  • hub and web typecheck. 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.
@TheGreatAxios
TheGreatAxios force-pushed the cl-11630-hold-webhook-mail-while-the-listener-is-being-replaced branch from c13b086 to 7c63e0d Compare October 28, 2026 08:02

@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 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 stalled route field.
  • main's re-queue in planTenant keeps undeliveredSince. withStatus now drops undeliveredSince and unrouted together. queuedAgain and markBaseMoved drop only unrouted.
  • failDelivery sets undeliveredSince, and its delivery failed: wording still matches undelivered, so the alarm sees held heads. holdHead calls the four-argument queueHead.

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.

@TheGreatAxios
TheGreatAxios marked this pull request as ready for review October 28, 2026 08:05
@TheGreatAxios
TheGreatAxios merged commit 27729d5 into main Oct 28, 2026
17 checks passed
@TheGreatAxios
TheGreatAxios deleted the cl-11630-hold-webhook-mail-while-the-listener-is-being-replaced branch October 28, 2026 08:08
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