Skip to content

fix(cloud): validate protocol-0 same-cap cell plans without rehome trust lines - #18818

Merged
Jinwoo-H merged 1 commit into
mainfrom
relay-same-cap-asia-plan-validator
Sep 5, 2026
Merged

Jinwoo-H merged 1 commit into
mainfrom
relay-same-cap-asia-plan-validator

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​283 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​280
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​35 $\color{#cf222e}{\Huge{\mathbf{−}}}$​9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​26

What happened

Run 33957151726 rolled the first Asia same-cap cell, production-gce-c27 (asia-east2). It failed after the reversible isolate/drain and before any apply, in "Apply only the selected same-cap template and MIG":

cell plan does not contain the reviewed image and capacity

The Terraform plan was correct (template replaced to the target image, MIG updated in place). The validator was wrong. The failsafe held: c27 stayed migration-only with rehome disabled, and nothing was applied.

Root cause

In same-cap-cell mode, requireDesiredStartupScript unconditionally required exactly one ORCA_RELAY_REHOME_DIRECTOR_SERVICE_ACCOUNT line and one ORCA_RELAY_REHOME_AUDIENCE line with exact values.

relay-gce-startup.sh.tftpl emits those two lines only when rehome_source_enabled, which is contains(var.relay_region_rehome_source_cell_ids, each.key). That list in environments/production.tfvars holds the 16 US cells (c7-c10, c13-c16, c19-c26). The Asia cells c27-c29 are not rehome sources, so their startup scripts have no rehome lines and their runtime reports regionalRehomeProtocol: 0. The expectation was written when every same-cap cell was a US rehome source.

Fix

validate-relay-capacity-plan.mjs takes a new --regional-rehome-protocol 0|1, required in same-cap-cell mode and rejected in every other mode.

  • Protocol 1: unchanged. Both rehome lines required, exactly once, with the exact reviewed values.
  • Protocol 0: both rehome lines must be absent. A protocol-0 cell that gains rehome trust is real drift and still fails closed.

The job passes --regional-rehome-protocol "${DESIRED_REHOME_PROTOCOL}" on both same-cap-cell invocations, the resume drift check and the apply-step plan validation. That is the same value already asserted against the live runtime by verify-relay-capacity-transition.mjs and by the preflight runtime-status check, so the plan check and the runtime check now agree by construction instead of by coincidence.

normalizedStartupScript's strip-rehome behaviour is unchanged; for protocol 0 it is a harmless no-op since neither side has those lines.

Unchanged

  • cloud-deploy-relay-production-capacity-job.yml (US-only, --mode bootstrap-cell).
  • same-cap-image mode, used by the staging c4 recovery and staging capacity proof workflows.
  • The protocol-1 US path: every existing check still runs with the same exact-value expectations.
  • The rehome-trust probe step, already gated on rehome-protocol == '1', so it correctly skips for Asia.

Tests

validate-relay-capacity-plan.test.mjs:

  • protocol-0 asia-east2 fixture at cap 3000 with no rehome lines is accepted
  • protocol-0 with rehome lines present is rejected
  • the same script under protocol 1 is accepted, the trust-free one under protocol 1 is rejected
  • 0, '', 2, yes and a missing value are all rejected as an invalid protocol
  • the argument is required in same-cap-cell mode and rejected in bootstrap-cell mode

relay-same-cap-script-census.test.mjs:

  • both same-cap-cell validator invocations in the job carry --regional-rehome-protocol "${DESIRED_REHOME_PROTOCOL}"
  • for all 19 SAME_CAP_CELLS, the protocol derived from relay_region_rehome_source_cell_ids in production.tfvars matches the region the job's own cell-shape block resolves, and the validator accepts a minimal correct roll plan at that protocol while rejecting the same plan at the other one

relay-regional-rehome-workflow.test.mjs: the pinned resume-call assertion now requires the protocol flag between host-drain and the jq -e '.changes == 2' pipe.

Full cloud suite: 529 passing, 0 failing. pnpm typecheck in cloud clean.

Jinwoo-H added a commit that referenced this pull request Sep 5, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor suggestion inline.

Reviewed changes

  • Protocol-aware plan validation — validate-relay-capacity-plan.mjs gains a --regional-rehome-protocol 0|1 flag: protocol 1 keeps the exact rehome-trust expectations, protocol 0 requires both rehome lines to be absent and fails closed on any rehome drift (unexpectedRehome).
  • Workflow plumbing — the same-cap job now passes the protocol on both validator invocations (the resume drift check and the apply-step validation), so the plan check and the runtime-status check agree by construction.
  • Tests — a unit matrix for the protocol rules plus a census test that derives each same-cap cell's protocol from relay_region_rehome_source_cell_ids in production.tfvars, cross-checks it against the job's region mapping, and validates a correct roll plan at each cell's protocol while asserting the opposite protocol rejects it.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

!values['rehome-director-service-account'] ||
!values['rehome-audience'])
!values['rehome-audience'] ||
!['0', '1'].includes(values['regional-rehome-protocol']))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bad value like --regional-rehome-protocol 2 falls into this clause and reports "requires rollback image and rehome trust config", even though those flags were supplied — the accurate "invalid regional rehome protocol" message in rehomeProtocol is only reached on a direct validateCapacityPlan call, never through the CLI. Consider a dedicated protocol check with its own message for the parse path.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds --regional-rehome-protocol to same-capacity validation. Protocol 0 rejects rehome trust lines, while protocol 1 requires them. Argument parsing now exports as parseCapacityPlanArguments. Deployment workflow invocations pass the desired protocol. Tests cover argument validation, startup-script drift, every wave cell, and workflow command construction.

Merge Risk: 🟡 Moderate · up to d6832

This change enables protocol-0 cells without rehome trust configuration, but validation can still accept some retained rehome-variable lines. That can leave unintended rehome settings in production startup scripts, so the matcher and regression coverage should be corrected before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: protocol-0 same-cap plans no longer require rehome trust lines.
Description check ✅ Passed The description clearly explains the failure, root cause, fix, unchanged behavior, and test coverage. It is mostly complete, but it does not include the required Linked Issue section or the required V…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: ce35d41c-212e-4589-95a6-5a78f03f8918

📥 Commits

Reviewing files that changed from the base of the PR and between 0bbf86b and d6832af.

📒 Files selected for processing (5)
  • .github/workflows/cloud-deploy-relay-production-same-cap-job.yml
  • cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs
  • cloud/dev/scripts/relay-same-cap-script-census.test.mjs
  • cloud/dev/scripts/validate-relay-capacity-plan.mjs
  • cloud/dev/scripts/validate-relay-capacity-plan.test.mjs

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

const unexpectedRehome =
config.mode === 'same-cap-cell' &&
!rehomeTrusted &&
lines.some((line) => REHOME_CONFIG.test(line))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Detect all rehome-variable assignments for protocol 0.

REHOME_CONFIG matches only canonical assignments with a non-empty value. If both scripts retain printf 'ORCA_RELAY_REHOME_AUDIENCE=%s\n' '', or another noncanonical assignment form, unexpectedRehome is false and the normalized comparison still passes after image replacement. This accepts a protocol-0 script that contains a rehome line.

Use a presence matcher that detects both rehome variable names regardless of assignment value or rendering. Add a regression test where the same unmatched assignment exists before and after the replacement.

@Jinwoo-H
Jinwoo-H merged commit 9f2a9a2 into main Sep 5, 2026
26 checks passed
@Jinwoo-H
Jinwoo-H deleted the relay-same-cap-asia-plan-validator branch September 5, 2026 09:42
paidaxingyo666 pushed a commit to paidaxingyo666/Manta that referenced this pull request Sep 5, 2026
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
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