Repository navigation
perf(relay): cut the cell LB drain to 60s and widen the same-cap batch to ten cells - #21848
Conversation
…h to ten cells Two independent sources of relay roll wall clock, neither of which protects a host: 1. `connection_draining_timeout_sec` on the per-cell backend services was 300s. The same-cap job drains every host off the cell to a restart-safe condition before Terraform runs, so the LB drain only ever covers a host still mid-handshake. Measured 2026-09-16 over ten same-cap cell jobs, it sat as ~5m55s of dead time between `Apply complete` and the old VM powering off, inside an 8.5-minute `wait-until --stable` step. Now 60s, and pinned in the topology `check` block beside the other fixed-one invariants. 2. The same-cap wave capped a batch at four cells, so a 22-cell roll needed six batches, six single-use monitor gates, and a human handoff per batch. The wave workflow now declares cell_1..cell_10 with the identical serial shape and chaining, and the validator accepts two to ten. The shared wave-index rule (`relay-monitor-evidence.mjs` and the relay-ops preflight CLI) widens from 0-3 to 0-9 so the later cells can present the same evidence; each job workflow keeps its own narrower range, so the capacity wave stays at four. Cells remain strictly serial, one at a time behind the rollout lease, each with its own live preflight. Claude-Session: https://claude.ai/session/relay-roll-drain-timeout-and-batch-cap
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe same-cap relay deployment now supports two to ten serial cells, with wave indexes 0 through 9. Validation and tests cover the ten-cell workflow and final-wave evidence-age limits. The release lease waits for all ten cells. Relay GCE connection draining changes from 300 to 60 seconds, with matching topology validation and documentation. Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Update the CLI usage range. · incident-live-preflight-cli.ts:100-103
cloud/apps/relay-ops/src/incident-live-preflight-cli.ts:100-103
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the CLI usage range.
WAVE_INDEX_PATTERNaccepts4through9, butPREFLIGHT_USAGEstill advertises<0-3>on both paths. Update both ranges to<0-9>so an invalid command does not direct the operator to a false limit.Proposed fix
- 'usage: --state-file <verified-monitor-state> [--wave-index <0-3>]' + + 'usage: --state-file <verified-monitor-state> [--wave-index <0-9>]' + ... - ' --selector-membership-file <json> [--wave-index <0-3>]' + + ' --selector-membership-file <json> [--wave-index <0-9>]' +
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 09db59ca-b840-4ed5-9caa-b9132c784491
📒 Files selected for processing (11)
.github/workflows/cloud-deploy-relay-production-same-cap-job.yml.github/workflows/cloud-deploy-relay-production-same-cap.ymlcloud/apps/relay-ops/src/incident-live-preflight-cli.test.tscloud/apps/relay-ops/src/incident-live-preflight-cli.tscloud/dev/scripts/relay-monitor-evidence.mjscloud/dev/scripts/relay-monitor-evidence.test.mjscloud/dev/scripts/relay-production-same-cap-wave.mjscloud/dev/scripts/relay-production-same-cap-wave.test.mjscloud/dev/scripts/relay-regional-rehome-workflow.test.mjscloud/infra/terraform/README.mdcloud/infra/terraform/relay-gce-cells.tf
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
Important
The new 60-second drain conflicts with the Asia topology plan validator, which still pins 300 and will reject its own plans (including a no-op readback) once the root apply lands. One inline comment; details below.
Reviewed changes — reviewed the single commit 8f5266a against main.
- Drain 300 → 60 —
connection_drain_secondscut in the per-cell backend service, pinned in therelay_gce_fixed_one_topologycheck, README updated. - Same-cap batch cap 4 → 10 — six new serial cell jobs (
cell_5..cell_10) added to theneedschain and torelease_lease; validator bound widened to 2..10;cell-idsdescription updated. - Wave-index 0-3 → 0-9 — both shared freshness verifiers (
relay-monitor-evidence.mjs,incident-live-preflight-cli.ts) widened together, with the cross-language contract test and new wave-9 age edges. - Tests — new test asserts the workflow really declares ten strictly serial cell jobs with
release_leaseneeding each and nocell_11.
ℹ️ Nitpicks
cloud/docs/relay-workflows.md:380still says same-capbatch-apply"rolls two to four cells sequentially" — stale now that the cap is ten. Line 366 is the capacitywave-applypath and correctly stays at four.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…drain `validate-relay-asia-topology-plan.mjs` rejected any Asia backend whose `connection_draining_timeout_sec` was not 300, and `cloud-deploy-relay-asia-topology.yml` targets `google_compute_backend_service.relay_gce_cell["<cell>"]` per cell. With the Terraform local at 60 that workflow would have failed its own plan review. The validator's two restated topology values are now named exports, and a new census test reads `relay-gce-cells.tf` and equates three statements of each: the `relay_gce_topology` local, the topology `check` assert that pins it, and the validator constant. Terraform cannot export a local to JS, so reading the source is the only way to stop them drifting; the test was confirmed to fail when the local alone is moved back to 300. Repo-wide grep finds no other pin of the drain value. Claude-Session: https://claude.ai/session/relay-roll-drain-timeout-and-batch-cap
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — this incremental review covers fdcc9aaa, the one commit added since pullfrog's prior review at 8f5266a.
- Aligned the Asia topology plan validator with the 60-second drain —
validate-relay-asia-topology-plan.mjsno longer hardcodes86_400/300; it exportsRELAY_CELL_BACKEND_TIMEOUT_SECONDSandRELAY_CELL_CONNECTION_DRAIN_SECONDSand compares the backend shape against them. - Pinned all three copies of each value together — a new census test in
validate-relay-asia-topology-plan.test.mjsreadsrelay-gce-cells.tfand equates therelay_gce_topologylocal, the topologycheckassert, and the validator constant, closing the drift the prior review flagged.
This addresses the prior pullfrog thread: the Asia workflow's saved plan and idempotent readback now expect 60, so it stays valid once the root apply lands. I mutation-checked the census test — moving the Terraform local alone back to 300 fails it with connection_drain_seconds local differs from the validator constant. A repo-wide grep found no remaining drain pin.
One minor carry-over: cloud/docs/relay-workflows.md:380 still describes the same-cap batch-apply path as rolling "two to four cells" (line 366's capacity wave-apply correctly stays at four). Non-blocking, and it was already noted in the prior review.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…gs inside each same-cap wave (#21860) * chore(relay): target each cell's backend service from the same-cap job The 60 s connection drain timeout merged in #21848 has no safe apply path. A root plan scoped to the backend services alone still pulls every `google_compute_instance_template.relay_gce_cell` in as a dependency, and standing image drift turns all 29 into replacements, so applying it would roll the fleet at once. Add `google_compute_backend_service.relay_gce_cell["${TARGET_CELL_ID}"]` to both plan invocations in the per-cell same-cap job, next to the template and MIG it already targets, and teach the reviewed plan validator to allow exactly one extra change: an in-place update of that one cell's backend whose only changed attribute is `connection_draining_timeout_sec`, landing on the constant `validate-relay-asia-topology-plan.mjs` exports. Any other attribute, any other resource, or a backend for another cell still fails the validator. The accepted update is reported as `connectionDrainUpdate` and kept out of `changes`, so the apply step's stranded branch and the resume step's drift branch keep reading the template-and-MIG count they were written against; the resume branch additionally accepts a plan whose only pending change is that drain update, which restarts nothing. Claude-Session: relay-same-cap-targets-cell-backend * fix(relay): also let the same-cap wave apply this cell's LB request logging A read-only production plan for production-gce-c7 showed the live US cell backends carry no `log_config` at all, while relay-gce-cells.tf has declared `log_config { enable = true, sample_rate = var.relay_gce_cell_log_sample_rate }` on every cell backend since the Terraform root landed in 3eec77c (#18413). Nothing has applied it because every production apply since is a per-cell targeted plan that names only the template and the MIG. So the real canary plan's backend moves two paths, not one: `["connection_draining_timeout_sec", "log_config.0"]`. The drain-only validator rejected exactly that plan, which would have stranded the cell mid-wave after the drain had already started. Accept both, each optional, for this cell's backend only: the drain landing on RELAY_CELL_CONNECTION_DRAIN_SECONDS, and a log_config of exactly one block with `enable = true` and `sample_rate` equal to RELAY_CELL_LOG_SAMPLE_RATE, the declared default of a variable no environment file overrides. Any third path, a different sample rate, disabled logging, another cell, or a replacement still fails. The accepted paths are reported as `backendUpdate`, which the resume branch now reads instead of the drain-only flag. Verified against the real production plan: the masked, three-target plan for production-gce-c7 contains exactly that cell's template, MIG, and backend service and nothing else, and this validator returns `{"changes":2,"backendUpdate":["connection_draining_timeout_sec","log_config.0"]}`. Claude-Session: relay-same-cap-targets-cell-backend
…pacity role cannot update it (#22140) The capacity role the same-cap wave authenticates as, orcaRelayProductionCapacity, has no compute.backendServices.update. Since #21860 added `google_compute_backend_service.relay_gce_cell["${TARGET_CELL_ID}"]` to both of the job's plan invocations, every wave has therefore created the new instance template, modified the MIG, and then failed 403 on the backend, leaving the cell isolated with its trust probe, admission restore, and shadow gate all skipped. Run 35684694704 on production-gce-c7 is the first one that hit it in production. Drop the backend target from both plans and restore the resume gate to exactly `.changes == 2` (the template-and-MIG rollback-image drift) or a converged plan, removing the backend-only resume apply #21865 added on top. A resume applies nothing again, which is what a resume means. The validator keeps its bound on a cell backend update, so it still reports one and refuses anything wider, but a wave plan can no longer contain one. The drain timeout from #21848 and the log_config from #21860 need a root apply by a principal that holds the permission; granting the capacity role that permission is itself a root apply, so it can follow as its own change rather than blocking every wave in the meantime. Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

ELI5
Rolling a new relay image across the fleet is slow for two reasons that have
nothing to do with keeping users connected. The load balancer waits five
minutes for connections that were already moved off the machine, and the
rollout can only do four machines per run, so a full fleet roll needs six runs
and six human handoffs. This shortens the pointless wait and lets one run do
ten machines.
What Changed
Before, as an operator experiences it. A full 22-cell roll is six batches.
Each batch needs its own fresh 15-minute monitor dry-run (the evidence is
single-use), its own confirmation string, and a human to notice the batch
finished and start the next one. Each cell job takes about 14 minutes measured
over ten same-cap jobs on 2026-09-16. Inside that, the
gcloud compute instance-groups managed wait-until --stablestep is 8.5minutes, and roughly 5 minutes 55 seconds of it is dead time between Terraform
printing
Apply completeand the old VM actually powering off — before any VMwork starts. Wall clock: 22 x 14 min ~= 5 h 8 m of cell time, plus 7 gates
(1 canary + 6 batch monitor dry-runs) and 6 handoffs.
After. A cell job should take about 9 minutes, and a roll is one canary
plus two batches of ten. Wall clock: 22 x ~9 min ~= 3 h 18 m, plus 3 gates and
2 handoffs.
Mechanism 1 — the drain.
connection_draining_timeout_secon everyper-cell backend service in
cloud/infra/terraform/relay-gce-cells.tfwas 300seconds. That is the only 300-second constant on the
wait-until --stablepath, it carried no comment, and
git log -Lon the line shows only theoriginal bulk file-creation commit. It is now 60. The same-cap job already
drains every host off the cell to a restart-safe condition before Terraform
runs (the drain step, roughly 3 minutes, condition-driven, not a sleep), so by
the time the LB starts draining there is no serving session behind it. The
value is now also pinned in the
relay_gce_fixed_one_topologycheckblocknext to
max_surge,max_unavailable,backend_group_count,public_access_config_countand the 86,400-second WebSocket timeout, with aone-line why, so a future raise cannot silently re-add rollout wall clock.
Mechanism 2 — the batch cap.
cloud-deploy-relay-production-same-cap.ymlhardwired
cell_1..cell_4, each callingcloud-deploy-relay-production-same-cap-job.ymland chained throughneeds,and
relay-production-same-cap-wave.mjs validaterefused more than four cells.cell_5..cell_10are added with identical shape — same reusable workflow,same inputs,
needs: [gate, cell_{N-1}],if: needs.cell_{N-1}.result == 'success' && fromJSON(needs.gate.outputs.cells)[N-1] != null,wave-index: '{N-1}'— and torelease_lease.needs. The validator now acceptstwo to ten, and the
cell-idsinput description says "two to ten batch cells".Consequence that needed widening too. The wave index feeds the monitor
evidence freshness bound (
5 min + waveIndex x 75 min, 75 min being the celljob timeout), and that rule is written twice — in
cloud/dev/scripts/relay-monitor-evidence.mjsand incloud/apps/relay-ops/src/incident-live-preflight-cli.ts, with a contract testpinning the two copies to each other. Both go from
^[0-3]$to^[0-9]$, sothe aggregate dry-run ceiling for the last cell of a ten-cell batch becomes
11 h 20 m instead of 3 h 50 m. Nothing else about that gate moves: the evidence
is still single-use, the jobs are still
needs-chained, and every cell stillruns its own live preflight against the same thresholds immediately before it
mutates anything, which is what actually holds the mutation to current
health. Each job workflow keeps its own narrower range check, so the capacity
wave (
cloud-deploy-relay-production-capacity-job.yml) stays pinned at^[0-3]$and its four-cell cap is untouched.Unchanged: the canary path, the confirmation format, monitor consumption
and the consumed-artifact marker, the break-glass override, per-cell isolate /
drain / plan-review / apply / restore, the Terraform plan validator, the
rollout lease, and strict serial execution — still exactly one cell at a time.
Which workflow applies the Terraform change, and when
Not the same-cap wave. Both the same-cap job and the capacity job plan and
apply under a resource mask of exactly two addresses:
google_compute_backend_service.relay_gce_cellis not in that mask, so nosame-cap wave — canary or batch — will ever apply this value, and the change
also cannot disturb the
changes == 2plan review those jobs enforce.The Asia topology workflow does target the backend service, and its plan
validator (
validate-relay-asia-topology-plan.mjs) independently restated theold 300. That validator now pins 60 from an exported constant, tied to the
Terraform source by a census test, so the two cannot drift.
It needs a separate apply, before the first canary wave. Two paths reach
the backend service:
pnpm infra:apply --env production(cloud/dev/scripts/infra.mjs), theuntargeted relay-root apply an operator runs. This is the intended path and
it applies the new value to all per-cell backend services at once.
cloud-deploy-relay-asia-topology.yml, whose target list does includegoogle_compute_backend_service.relay_gce_cell["<cell>"], but only for thecells named in its
cell-idsinput. That is an additive topology workflow,not the general path.
So the speedup is not measurable until that root apply runs. Until it does,
this PR is inert in production: merging it changes no live infrastructure.
Why 60 seconds and not 0
Zero would be correct for a serving session, because there is none left by the
time Terraform runs. 60 seconds is a margin for a host that is still
mid-handshake at the load balancer when the VM goes away — the drain step
proves hosts left, not that no client is mid-connect. Sixty seconds costs
under a minute per cell and removes the remaining failure mode; zero saves one
more minute and buys a class of 503 nobody has measured.
Why not a dynamic job matrix
A matrix over the cell list would make the cap a runtime value and delete the
copy-paste. GitHub Actions cannot do it: a reusable-workflow call
(
jobs.<id>.uses) cannot be driven by amatrixin a way that also guaranteesstrict serial order —
max-parallel: 1orders a matrix's own jobs butdoes not give each entry a
needsedge on its predecessor's success, and amatrix job cannot
needsa specific sibling matrix entry. Serial execution isthe safety property here: cell N+1 must not start until cell N succeeded and
the selector generation advanced by exactly its class's delta. A statically
declared
needschain is the only construct that expresses that, so the chainis written out and its length is bounded and asserted in a test instead.
Why
A 22-cell roll is 5+ hours of cell time and a person babysitting six handoffs,
and roughly 40% of each cell job is a load-balancer drain covering connections
that were deliberately moved off that cell minutes earlier. Both costs are
accidental. Neither change touches what makes a single cell roll safe.
Alternatives considered and rejected:
mid-handshake margin. See above.
wait-until --stabletimeout instead. That is a ceiling, notthe cost; it would convert a slow roll into a failed one.
is built on.
chaining. See above.
single-use per wave, so each batch costs a fresh 15-minute dry-run regardless
of how fast the human is.
Proof plan
The first US canary wave after the root apply is the measurement:
Apply complete-> old VM power-off. Baseline is~5 min 55 s (2026-09-16, median over 10 same-cap cell jobs); expect ~1 min
40 s. Total cell job should fall from ~14 min to ~9 min.
window, measured from container boot rather than wave start.
connection_drain_secondsto 300 and re-apply.The
checkblock pin means the revert is a one-line change that CIre-validates.
The batch cap is proven separately, on the first ten-cell batch: every cell
job must report its own live preflight pass and the expected selector
generation for its wave index, and
release_leasemust free the lease exactlyonce after the last one.
Linked Issue
N/A — operational latency work, no tracking issue.
Fixes #
Visual Proof
N/A— infrastructure configuration and CI workflow change with no UI surface.Testing
All run locally on macOS from a clean
origin/mainworktree.terraform -chdir=infra/terraform fmt -check -recursive-> exit 0.terraform -chdir=infra/terraform init -backend=false -input=falsethenterraform -chdir=infra/terraform validate->Success! The configuration is valid.The module validates offline with no credentials and no backend, thesame two commands
cloud-verify.ymlruns. Noterraform applywas run andno production mutation of any kind was made.
actionlinton both same-cap workflows -> 4 findings, all[runner-label]for the custom Blacksmith runner label. The unmodified
origin/maincopiesof the same two files produce the identical 4
[runner-label]findings, sothis PR adds none.
node --testover the fullcloudtestscript list -> 529 tests, 0 fail.node --testover the fullcloudpretestlist -> 157 tests, 0 fail.vitest runincloud/apps/relay-ops-> 12 files, 134 tests, 0 fail.tsc -p tsconfig.json --noEmitincloud/apps/relay-ops-> exit 0.oxlinton the changed.mjs/.tsfiles -> "No files found to lint." Theroot
.oxlintrc.jsonlistscloud/**underignorePatterns, so the rootlint gate does not cover these files; the cloud workspace lints with
tsc --noEmit, which passes.Tests added or updated:
relay-production-same-cap-wave.test.mjs: a batch of ten is accepted, elevenis refused, one is refused; and a new test walks the wave workflow asserting
all ten cell jobs exist with the right
needs,if, cell index andwave-index, thatrelease_leaseneeds each of them, and that there is nocell_11— so the validator's bound and the workflow chain cannot drift.relay-monitor-evidence.test.mjs: wave 9 accepts evidence at exactly5 min + 9 x 75 minand rejects it one millisecond later;'10'is now theinvalid-index case.
incident-live-preflight-cli.test.ts: the same two edges for the TypeScriptcopy at its own 10-minute base, and
'10'as the usage error.relay-regional-rehome-workflow.test.mjs: the cross-language pin now reads^[0-9]$, the per-cell override count is 10, and all ten wave indices areasserted present in the wrapper.
validate-relay-asia-topology-plan.test.mjs: a census test readsrelay-gce-cells.tfand equates therelay_gce_topologylocal, the topologycheckassert, and the validator constant, for both the backend timeout andthe drain. Confirmed failing when the local alone is moved back to 300.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Internal contributor.
Review
Worth a careful look at the wave-index widening: it is the one place where a
number that used to bound staleness got larger. The argument is that the
aggregate dry-run was never the per-cell gate — the live preflight immediately
before each mutation is — and that the jobs are still
needs-chained and theevidence still single-use. If a reviewer disagrees, the alternative is to lower
WAVE_PREDECESSOR_TIMEOUT_MSfrom 75 minutes in step with the faster cell job,which would tighten the bound for every wave; that is deliberately left out of
this PR because it also changes the capacity wave.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
change. The workload-identity attribute condition already admits
cloud-deploy-relay-production-same-cap-job.ymlcalled fromcloud-deploy-relay-production-same-cap.yml; the new jobs are the same call,so no provider condition changes.
provenance set, so a canary authority sealed before this merge will not
verify against a batch dispatched after it. That is the existing designed
behaviour for any change to these files — seal a fresh canary on the merged
commit.
desktop, renderer, terminal or path handling is touched.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)