Repository navigation
fix(relay): land the cell backend drain and request logging inside the same-cap wave - #21860
Merged
Merged
Conversation
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
Contributor
|
This run was cancelled 🛑 The workflow was cancelled before completion. Please check the link below for details. |
Contributor
|
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 (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds validation for a reviewed connection-draining timeout update on the selected same-cap cell backend service. The validator reports this update separately from template and MIG changes. Production plans now target the backend service in resume and apply paths. Workflow and script tests cover valid drain updates, invalid backend changes, drain-only results, and exact target scoping. Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 unsupported.)
✨ 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 |
…ogging
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
Jinwoo-H
changed the title
9 tasks done
Jinwoo-H
added a commit
that referenced
this pull request
Sep 21, 2026
…d update on resume (#21865) * fix(relay): bound the same-cap shadow gate and apply a resumed backend update Two findings both adversarial reviews of tonight's merged set agree on. The report-only shadow health gate (#21849) had `continue-on-error: true` but no step timeout. That bounds the step's contribution to the job outcome, not its clock. Its reads are serialised, and a failure that answers nothing slowly — an expired credential, a project-wide Logging 429 storm — makes every read cost its full 3 x 60 s retry budget, so the cost scales with the roll window: roughly 8S + 2 reads for S ten-minute sub-windows. A 40-minute window is about 34 reads, or 108 minutes, against the job's `timeout-minutes: 75`. A cancelled job cannot be absorbed by continue-on-error, fires the failure-gated cleanup isolation on an already-restored cell, and stops the strict next-cell chain. Give the step `timeout-minutes: 5` and the artifact upload `timeout-minutes: 2`. A timed-out step is a failed step, which continue-on-error covers, so the job stays green. Inside the script, stop reading after an overall four-minute deadline and report the remaining checks unverified, so the normal outcome is a written verdict rather than a killed process; the step timeout is then only for a hung process. The census test pins both timeouts and that the deadline leaves the step time to write its verdict. The resume branch (#21860) accepted `changes == 0` with a non-empty `backendUpdate` as complete and applied nothing, so a resumed cell silently kept the 300-second drain and no request logging behind a green resume. That shape means the template and MIG are converged and only this cell's reviewed backend update is left, so apply the saved resume plan — the validator has already bounded it to this cell's backend and neither attribute restarts an instance — then continue as converged. Template-and-MIG drift still applies nothing, which is what a resume means, and a stranded cell's explicit MIG replace is unchanged. Claude-Session: relay-same-cap-gate-timeout-and-resume * fix(relay): raise the shadow gate bounds clear of a healthy gate's read time A healthy gate is already minutes of serial reads on the 2-vcpu runner, so a four-minute deadline would report unverified tails on ordinary days and stop the shadow roll measuring the comparison it exists for. Raise both together: the step to eight minutes and the script's own deadline to seven, keeping the census pin that the deadline leaves the step room to write its verdict. The job budget is unaffected: a ~14-minute cell plus eight is well inside 75. Claude-Session: relay-same-cap-gate-timeout-and-resume
Jinwoo-H
added a commit
that referenced
this pull request
Sep 22, 2026
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
Two settings on Relay's load balancers were written down months ago but never switched on, because the only button that would have pushed them would have restarted every Relay server in the fleet at once. This PR moves both into the existing one-server-at-a-time rollout, so they go out with the routine update, first on the single test server.
The settings: (1) when a server is taken out of rotation, the load balancer waits 1 minute instead of 5 before cutting leftover connections; (2) the load balancer writes one log line per connection when it closes, so we can join "which server did we hand this user to" with "what actually happened to that connection".
What Changed
Before. Two things were declared in
cloud/infra/terraform/relay-gce-cells.tfforgoogle_compute_backend_service.relay_gce_cell[<cell>]and neither had ever reached a live US cell:connection_draining_timeout_sec, moved 300 → 60 by perf(relay): cut the cell LB drain to 60s and widen the same-cap batch to ten cells #21848 (merged as 4f839cc).log_config { enable = true, sample_rate = var.relay_gce_cell_log_sample_rate }, declared since the Terraform root landed in 3eec77c (chore(cloud): add the relay fence broker, ops console, Terraform root, scripts, and 24 cloud-* workflows #18413, 2026-09-03).Neither could be applied. Every production apply since that commit is a per-cell targeted plan naming only the instance template and the MIG, so the backend service has never been in a plan. And a root plan cannot be used: targeting the backend services alone still drags in every cell's template, which standing image drift turns into a fleet-wide roll. Measured on the production root at
main76d8760:All 29 templates "must be replaced" (
metadata_startup_script forces replacement), all 29 MIGs updated. The lead saw the same shape earlier tonight and did not apply.Why an indexed target still pulls in everything. Terraform applies
-targetto the plan graph before dynamic expansion. The index filters the targeted resource's own instances, but the nodes it pulls in as dependencies are whole resources:backend["c1"]→ the entiregoogle_compute_instance_group_manager.relay_gce_cellnode → the entiregoogle_compute_instance_template.relay_gce_cellnode, all 29 instances of each. It is a dependency-expansion effect, not a whole-map reference in the config — the backend's only per-cell reference isgoogle_compute_instance_group_manager.relay_gce_cell[each.key].instance_group, and there is nodepends_onon the backend, the URL map, or the health check pointing at every cell.That also explains why the same-cap job is unaffected: it already names the template and the MIG with explicit indices, so those two nodes are in the target set with an instance filter, and the backend's dependency pull-in cannot widen them.
After. Both settings land one cell at a time, inside the wave that already rolls that cell, starting with the canary. No extra dispatch, no extra restart.
Mechanism.
.github/workflows/cloud-deploy-relay-production-same-cap-job.yml— bothterraform planinvocations (the rollback-resume convergence check and the apply) now pass"-target=google_compute_backend_service.relay_gce_cell[\"${TARGET_CELL_ID}\"]"alongside the template and MIG, the same per-cell triple.github/workflows/cloud-deploy-relay-asia-topology.ymlbuilds. The apply consumes the saved plan file, so no apply re-lists targets. The-var-filemask the job builds (relay-same-cap.tfvars.json— the committedrelay_gce_cellsmap with only the target cell'simageoverridden) is untouched: the backend readslocal.relay_gce_topologyandvar.relay_gce_cell_log_sample_rate, not that map, so the added target does not widen the template mask.cloud/dev/scripts/validate-relay-capacity-plan.mjs— the reviewed plan validator now accepts, on top of everything it accepted before, at most one extra resource: an in-placeupdateof this cell's backend service whose changed paths are a non-empty subset of{connection_draining_timeout_sec, log_config.0}, withconnection_draining_timeout_sec === RELAY_CELL_CONNECTION_DRAIN_SECONDSandlog_configexactly one block withenable === trueandsample_rate === RELAY_CELL_LOG_SAMPLE_RATE. Both constants are imported fromvalidate-relay-asia-topology-plan.mjs; neither number is restated. Each path is optional because a cell that already has one plans no change for it — the Asia cells' backends were created by the Asia topology workflow and already carrylog_config.Everything else still fails closed: a third changed path, a different sample rate,
enable: false, a drain value that is not the constant, a backend for another cell id, a replacement instead of an update, a backend change in any mode other thansame-cap-cell, and any unexpected unknown path other than provider-computedfingerprint/generated_id.RELAY_CELL_LOG_SAMPLE_RATEis new. It is not a topology local but the declared default ofvar.relay_gce_cell_log_sample_rate; the existing constants census test was extended to equate it withvariables.tfand to assert that neitherproduction.tfvarsnorstaging.tfvarsoverrides it, so the constant cannot silently stop describing what a cell gets.The accepted paths are reported as
backendUpdate: [...]and are deliberately not counted inchanges. Two callers readchangesand would misread an inflated count: the apply step's stranded branch (ROLLBACK_STAGE = stranded && .changes == 0→ explicitrolling-action replace), and the resume step's drift branch (.changes == 2, the reviewed template-and-MIG pair). A backend attribute replaces no instance, so it must not suppress the stranded roll. Resume additionally accepts.changes == 0 and ((.backendUpdate // []) | length) > 0: a cell whose template and MIG have converged but whose backend has not. That is safe precisely because neither backend attribute restarts anything, which is the property the resume step protects.Why
The per-cell same-cap job is the only place in the repo that plans and applies Relay cell infrastructure under a per-cell target set and a reviewed plan validator. Alternatives considered:
changes. Silently breaks the stranded branch and the resume branch, both of which are about instance restarts.Linked Issue
N/A — follow-up to #21848 (merged as 4f839cc), which introduced the drain value this makes applicable.
Visual Proof
N/A — CI workflow and plan-validator change, no user-visible UI.
The behavioural proof is on the canary, from the job's own step outputs:
apply-completed-at→ old-VM power-off, about 5m55s under the 300 s drain. Two caveats for whoever reads that number:LB request logging can be confirmed independently after the canary:
resource.type="http_load_balancer"entries fororca-cloud-relay-gce-<cell>, one per WebSocket, written at close.Testing
Proof against the real production plan (read-only; plan only, never applied, every
.tfplandeleted). Reproduced the job's masked per-cell plan on the production root atmain76d8760, buildingrelay-same-cap.tfvars.jsonexactly as the job does, with the three targets:Exactly
{this cell's template, this cell's MIG, this cell's backend}and nothing else. Same result forproduction-gce-c1. The backend's changed paths on that real plan are["connection_draining_timeout_sec", "log_config.0"](before.log_config: [],after.log_config: [{enable: true, sample_rate: 1}], drain 300 → 60), andafter_unknownis{}.This validator on that plan:
Control — the drain-only validator from this branch's first commit, on the same plan:
That is the mid-wave stranding this second commit fixes.
Unit tests.
cloud/dev/scripts/validate-relay-capacity-plan.test.mjs, testa same-cap roll may carry only this cell backend drain and request logging, fixtures for: both attributes alongside the template/MIG pair; drain alone;log_configalone; neither (unchanged{changes: 2}); a drain-and-logging-only plan on a converged cell ({changes: 0, backendUpdate: [...]}); a third changed attribute (fails); a differentsample_rate(fails);enable: false(fails); a drain value that is not the constant (fails); a backend for another cell id (fails); a backend replacement rather than an update (fails); and the same plan under--mode cell(still fails).cloud/dev/scripts/relay-same-cap-script-census.test.mjsgained a contract test pinning the job's target list: everyterraform planthe job runs targets exactly the template, the MIG, and the backend, each scoped to${TARGET_CELL_ID}, and nothing else. Control-run against the pre-change workflow it fails with the backend entry missing, so it is not vacuous.cloud/dev/scripts/validate-relay-asia-topology-plan.test.mjsextends the existing tf-vs-JS constants census to the new sample-rate constant.cloud/dev/scripts/relay-regional-rehome-workflow.test.mjspins the resume step's jq filter verbatim and was updated.Commands (macOS):
The one failure is
dev/scripts/relay-load-control-peer.test.mjs:Cannot find module '@orca-cloud/relay-contract'— a pre-existing local install gap (cloud/node_modulesis absent in this worktree), unrelated to these files; CI installs the workspace.Byte-identical to the same command on
origin/main's copy: one pre-existing finding,label "blacksmith-2vcpu-ubuntu-2204" is unknown [runner-label].cloud/dev/scripts/*.mjsis outside the oxlint target set.Platforms: a GitHub Actions workflow (ubuntu runner) and Node scripts; no platform-dependent code. SSH/remote, mobile, and renderer untouched.
AI Disclosure
Review
Two things worth pushing on:
fingerprintandgenerated_id. The real plan'safter_unknownwas{}, so neither was needed; they are defensive and gated on actually being marked unknown. If some future plan marks a different computed attribute unknown, the wave fails closed and the list needs one entry.Both settings are reviewed per cell before anything is applied, so a wave that meets something unexpected stops before the apply rather than shipping it.
Agent skill upstream boundary
Notes
No security surface change: the validator becomes more permissive for two attributes on one resource, under one mode, at exact values, and stays fail-closed everywhere else. Enabling LB request logging writes one Cloud Logging entry per WebSocket at close (
sample_rate = 1) for the cells the wave reaches; it records client IP, status, and latency, which is what makes assign-to-connection joins exact, and it is the configuration already reviewed and committed. No wire, mobile, or backwards-compatibility impact.Checklist
N/Awith reason for visuals