fix(cloud): harden the push deploy workflow and size the gateway to the budget (#8129)

- Roll traffic back on a failed post-shift check; delete a candidate that
  never took traffic; retry the origin probe and the FCM probe.
- Assert Terraform-owned scaling instead of mutating it from the workflow.
- Build before taking the Cloud SQL rollout lease.
- Declare the database pool in Terraform (2 per instance, max 2 instances)
  and add the gateway to the connection budget; the previous default put the
  shared instance 65 connections over its ceiling.
- State plainly that the shared deploy identity's relay authority is inherited.

cloud/dev/scripts/push-gateway-workflow.test.mjs

@@ -1,7 +1,13 @@
 import assert from 'node:assert/strict'
 import { readFileSync } from 'node:fs'
 import test from 'node:test'
-import { concurrencyBlocks, jobIf, jobs, leaseSteps } from './cloud-sql-rollout-lock-census.mjs'
+import {
+  concurrencyBlocks,
+  jobIf,
+  jobs,
+  LEASE_ACTION,
+  leaseSteps
+} from './cloud-sql-rollout-lock-census.mjs'
 import { readRelayWorkflow, relayWorkflowFile } from './relay-repository.mjs'
 
 // Why: the push gateway holds the APNs key and is the only thing standing between a paired
@@ -81,18 +87,66 @@ test('the candidate revision takes no traffic and is addressed by its own tag',
   assert.match(workflow, /^ {12}--no-traffic \\$/m)
   assert.match(workflow, /--tag "\$\{tag\}"/)
   assert.match(workflow, /test "\$\{CANDIDATE_REVISION\}" != "\$\{ROLLBACK_REVISION\}"/)
-  // A tagged revision is directly addressable and sits outside the service-wide cap, so the
-  // candidate needs its own ceiling or it doubles the gateway's Cloud SQL draw while probing.
-  assert.match(workflow, /--max-instances "\$\{PUSH_MAX_INSTANCES\}"/)
-  assert.match(workflow, /PUSH_MAX_INSTANCES: 4$/m)
-  assert.match(terraform('variables.tf'), /variable "push_max_instances"[\s\S]*?default {5}= 4/)
   assert.ok(
-    indexOfStep('Record the serving revision before the rollout') <
+    indexOfStep('Record the serving revision and require its Terraform-owned scaling') <
       indexOfStep('Deploy the candidate revision with no traffic'),
     'the rollback target must be captured before the candidate exists'
   )
 })
 
+// Why: scaling is a Terraform-owned field that `lifecycle.ignore_changes` does not cover, so a
+// deploy that passed --max-instances would revert a later push_max_instances raise on every run.
+// The workflow asserts the shape instead of writing it, on the serving revision before the
+// candidate exists and on the candidate that inherits it.
+test('the deploy asserts the Terraform-owned scaling instead of mutating it', () => {
+  assert.doesNotMatch(workflow, /--max-instances/, 'the deploy must not write a scaling field')
+  assert.doesNotMatch(workflow, /--min-instances "/, 'the deploy must not write a scaling field')
+  // The floor is the variables.tf default; production.tfvars overrides only the ceiling, down to
+  // the two instances the Cloud SQL connection budget leaves room for.
+  assert.match(workflow, /PUSH_MIN_INSTANCES: 1$/m)
+  assert.match(workflow, /PUSH_MAX_INSTANCES: 2$/m)
+  assert.match(terraform('variables.tf'), /variable "push_min_instances"[\s\S]*?default {5}= 1/)
+  assert.match(terraform('environments/production.tfvars'), /^push_max_instances {9}= 2$/m)
+  const gate = indexOfStep('Record the serving revision and require its Terraform-owned scaling')
+  assert.ok(gate < indexOfStep('Deploy the candidate revision with no traffic'))
+  assert.match(workflow, /autoscaling\.knative\.dev\/minScale/)
+  assert.match(workflow, /\[\[ "\$\{floor:-0\}" -lt "\$\{PUSH_MIN_INSTANCES\}" \]\]/)
+  assert.match(workflow, /test "\$\{ceiling\}" = "\$\{PUSH_MAX_INSTANCES\}"/)
+  assert.match(workflow, /test "\$\{candidate_ceiling\}" = "\$\{PUSH_MAX_INSTANCES\}"/)
+})
+
+// Why: the image build is not a Cloud SQL operation, and the lease is a global serialization
+// point. A build inside it blocks every relay deploy and rehome for its duration.
+test('the image is built before the rollout lease is taken', () => {
+  const lease = workflow.indexOf(`- uses: ${LEASE_ACTION}`)
+  assert.notEqual(lease, -1)
+  const build = workflow.indexOf('- name: Build and publish the immutable gateway image')
+  const deployCandidate = workflow.indexOf('- name: Deploy the candidate revision with no traffic')
+  assert.ok(build < lease, 'the build must finish before the run takes the lease')
+  assert.ok(lease < deployCandidate, 'the lease must still cover the deploy, probe, and shift')
+})
+
+// Why: the gateway's Cloud SQL draw is instances x pool, and the root that takes the rollout
+// lease can only account for a pool it declares. Leaving it at the application default hid it.
+test('the database pool size is Terraform-owned and bounded at plan time', () => {
+  const source = terraform('push-gateway.tf')
+  assert.match(source, /name {2}= "ORCA_PUSH_DATABASE_POOL_MAX"/)
+  assert.match(source, /value = tostring\(var\.push_database_pool_max\)/)
+  assert.match(terraform('variables.tf'), /variable "push_database_pool_max"[\s\S]*?default {5}= 2/)
+  const block = /resource "google_cloud_run_v2_service" "push"[\s\S]*?\n  lifecycle \{([\s\S]*?)\n  \}/.exec(source)
+  assert.ok(block, 'the push service no longer declares a lifecycle block')
+  assert.match(
+    block[1],
+    /var\.push_max_instances \* var\.push_database_pool_max <= 4/,
+    'instances x pool must be bounded at plan time'
+  )
+  assert.match(
+    readFileSync(new URL('../../apps/push/src/config.ts', import.meta.url), 'utf8'),
+    /ORCA_PUSH_DATABASE_POOL_MAX/,
+    'the gateway must read the variable Terraform sets'
+  )
+})
+
 test('the candidate is probed on its own URL before any traffic moves', () => {
   const probe = indexOfStep('Probe the candidate readiness endpoint')
   assert.ok(probe > indexOfStep('Deploy the candidate revision with no traffic'))
@@ -115,6 +169,15 @@ test('the FCM probe is validate-only and separates a bad token from a bad creden
   assert.match(workflow, /orca-push-deploy-probe-invalid-token/)
   assert.match(workflow, /test "\$\{status\}" = INVALID_ARGUMENT/)
   assert.match(workflow, /test "\$\{status\}" = PERMISSION_DENIED/)
+  // Only those four answers are conclusive; a 429 or a 5xx says nothing about the credential, so
+  // it is retried rather than read as either verdict. A denied credential still fails at once.
+  assert.match(workflow, /for attempt in \$\(seq 1 5\); do/)
+  const probe = workflow.slice(
+    workflow.indexOf('- name: Prove the runtime identity can reach FCM'),
+    workflow.indexOf('- name: Shift all traffic to the verified candidate')
+  )
+  assert.match(probe, /for attempt in \$\(seq 1 5\); do/)
+  assert.match(probe, /test "\$\{code\}" = 401 \|\| test "\$\{code\}" = 403; then\n {14}break/)
   assert.match(
     workflow,
     /--impersonate-service-account "\$\{PUSH_RUNTIME_SERVICE_ACCOUNT\}"/,
@@ -150,11 +213,80 @@ test('the traffic shift is all-or-nothing and is verified after the fact', () =>
   assert.match(workflow, /"\$\{PUSH_ORIGIN\}\/ready"/)
 })
 
-test('the run reports a rollback target and always drops its traffic tag', () => {
+// Why: the origin can lag the traffic move by seconds, and a single unlucky curl would otherwise
+// roll a healthy deploy back. It retries on the same schedule as the candidate probe.
+test('the post-shift origin check retries like the candidate probe', () => {
+  const check = workflow.slice(
+    workflow.indexOf('- name: Verify the public origin after the shift'),
+    workflow.indexOf('- name: Roll traffic back to the previous revision')
+  )
+  assert.match(check, /for attempt in \$\(seq 1 30\); do/)
+  assert.match(check, /sleep 5/)
+  assert.match(check, /test "\$\{code\}" = 200/)
+})
+
+// Why: the summary carries the rollback target. Writing it after the origin check meant the one
+// run that needed it, the run whose check failed, was the one run that never got it.
+test('the summary is written before anything that can fail after the shift', () => {
+  const summary = indexOfStep('Publish the rollout summary')
+  assert.ok(summary > indexOfStep('Shift all traffic to the verified candidate'))
+  assert.ok(summary < indexOfStep('Verify the public origin after the shift'))
   assert.match(workflow, /--to-revisions \$\{ROLLBACK_REVISION\}=100/)
+  assert.match(workflow, /GITHUB_STEP_SUMMARY/)
+})
+
+// Why: everything after the shift runs with production on the candidate, so a failure there is a
+// live gateway that has to go back. The marker is what separates that case from a failure before
+// the shift, where production never moved and the candidate is the thing to clean up.
+test('a failure after the shift rolls production back automatically', () => {
+  const rollback = indexOfStep('Roll traffic back to the previous revision')
+  assert.ok(rollback > indexOfStep('Verify the public origin after the shift'))
+  assert.match(workflow, /echo "TRAFFIC_SHIFTED=true" >> "\$\{GITHUB_ENV\}"/)
+  const shift = workflow.indexOf('- name: Shift all traffic to the verified candidate')
+  assert.ok(
+    workflow.indexOf('echo "TRAFFIC_SHIFTED=true"') > shift,
+    'the marker must be set only once the shift has been verified'
+  )
+  const body = workflow.slice(
+    workflow.indexOf('- name: Roll traffic back to the previous revision'),
+    workflow.indexOf('- name: Delete the candidate revision that never took traffic')
+  )
+  assert.match(
+    body,
+    /if: \$\{\{ failure\(\) && env\.TRAFFIC_SHIFTED == 'true' \}\}/,
+    'the rollback must be conditioned on both failure and the shift marker'
+  )
+  assert.match(body, /test -n "\$\{ROLLBACK_REVISION:-\}"/)
+  assert.match(body, /--to-revisions "\$\{ROLLBACK_REVISION\}=100"/)
+  assert.match(body, /test "\$\{serving\}" = "\$\{ROLLBACK_REVISION\}"/)
+  assert.match(body, /GITHUB_STEP_SUMMARY/, 'the rollback must be reported in the summary')
+})
+
+// Why: a candidate that never took traffic still holds a warm instance and a Cloud SQL pool. Its
+// tag comes off first, because Cloud Run refuses to delete a revision a traffic target names.
+test('a failure before the shift deletes the candidate it created', () => {
+  const body = workflow.slice(
+    workflow.indexOf('- name: Delete the candidate revision that never took traffic'),
+    workflow.indexOf('- name: Drop the candidate traffic tag')
+  )
+  assert.match(
+    body,
+    /if: \$\{\{ failure\(\) && env\.TRAFFIC_SHIFTED != 'true' \}\}/,
+    'the cleanup must be conditioned on both failure and the absence of the shift marker'
+  )
+  assert.match(body, /test -n "\$\{CANDIDATE_REVISION:-\}" \|\| exit 0/)
+  assert.ok(
+    body.indexOf('--remove-tags') < body.indexOf('gcloud run revisions delete'),
+    'the tag must come off before the revision is deleted'
+  )
+  assert.match(body, /echo "CANDIDATE_TAG=" >> "\$\{GITHUB_ENV\}"/)
+})
+
+test('the run always drops its traffic tag', () => {
   const cleanup = indexOfStep('Drop the candidate traffic tag')
   assert.equal(cleanup, stepNames().length - 1, 'tag cleanup must be the last step')
   assert.match(workflow, /--remove-tags "\$\{CANDIDATE_TAG\}"/)
   const body = workflow.slice(workflow.indexOf('- name: Drop the candidate traffic tag'))
   assert.match(body, /if: always\(\)/)
+  assert.match(body, /test -n "\$\{CANDIDATE_TAG:-\}" \|\| exit 0/)
 })

cloud/dev/scripts/relay-cloud-sql-connection-budget.mjs

@@ -33,6 +33,13 @@ function requiredInteger(source, pattern, label) {
   return value
 }
 
+// A tfvars file states only what it overrides, so an absent key means the variable default holds.
+// Reading the default as the fallback keeps this honest either way.
+function overriddenInteger(override, overridePattern, source, pattern, label) {
+  if (!overridePattern.test(override)) return requiredInteger(source, pattern, label)
+  return requiredInteger(override, overridePattern, label)
+}
+
 function productionCells(source, defaultPoolMax) {
   const fencedMatch = source.match(/relay_gce_fenced_cells\s*=\s*\[([^\]]*)\]/)
   if (!fencedMatch) throw new Error('could not read fenced Relay cells')
@@ -52,11 +59,13 @@ function productionCells(source, defaultPoolMax) {
 }
 
 export function calculateRelayCloudSqlConnectionBudget(inputs) {
+  const pushDraw = inputs.pushInstances * inputs.pushPoolMax
   const consumers = {
     cells: inputs.cellPoolTotal + inputs.asiaCellCount * inputs.asiaPoolMax,
     directors: inputs.directorInstances * inputs.directorPoolMax,
     auth: inputs.authInstances * inputs.authPoolMax,
-    api: inputs.apiInstances * inputs.apiPoolMax
+    api: inputs.apiInstances * inputs.apiPoolMax,
+    push: pushDraw
   }
   const configuredMaximum = Object.values(consumers).reduce((total, value) => total + value, 0)
   const retainedDirectorRollback = inputs.directorInstances * inputs.directorPoolMax
@@ -64,6 +73,11 @@ export function calculateRelayCloudSqlConnectionBudget(inputs) {
     relayDirectorCandidate: retainedDirectorRollback * 2,
     apiCandidate: retainedDirectorRollback + inputs.apiInstances * inputs.apiPoolMax,
     authCandidate: retainedDirectorRollback + inputs.authInstances * inputs.authPoolMax,
+    // The push candidate doubles rather than adding one copy, like the director candidate and
+    // unlike the API and auth ones: cloud-push-deploy.yml probes a *tagged* revision, which is
+    // directly addressable and so sits outside the service-wide instance cap, letting the
+    // candidate and the serving revision each reach push_max_instances at the same time.
+    pushCandidate: retainedDirectorRollback + pushDraw * 2,
     relayCells: retainedDirectorRollback
   }
   const rolloutOverlap = Math.max(...Object.values(candidateOverlap))
@@ -131,6 +145,20 @@ export function readRelayCloudSqlConnectionBudget({
       /variable\s+"relay_director_database_pool_max"[\s\S]*?default\s*=\s*(\d+)/,
       'director pool maximum'
     ),
+    // The mobile push gateway shares this instance. Its draw was invisible here until Terraform
+    // declared the pool: docs/push-gateway.md, "Shape".
+    pushInstances: overriddenInteger(
+      productionTfvars,
+      /^\s*push_max_instances\s*=\s*(\d+)/m,
+      terraformVariables,
+      /variable\s+"push_max_instances"[\s\S]*?default\s*=\s*(\d+)/,
+      'push gateway instances'
+    ),
+    pushPoolMax: requiredInteger(
+      terraformVariables,
+      /variable\s+"push_database_pool_max"[\s\S]*?default\s*=\s*(\d+)/,
+      'push gateway pool maximum'
+    ),
     authInstances: apps.authInstances,
     authPoolMax: apps.authPoolMax,
     apiInstances: apps.apiInstances,

cloud/dev/scripts/relay-cloud-sql-connection-budget.test.mjs

@@ -6,28 +6,91 @@ import {
   readRelayCloudSqlConnectionBudget
 } from './relay-cloud-sql-connection-budget.mjs'
 
-test('production plus three Asia pools preserves allowance and reserve below the ceiling', () => {
+// Why these numbers are this tight: the shared instance's 400 connections were already spoken
+// for, and the relay shape below leaves exactly five. The gateway is sized to fit in four, two
+// instances times a two-connection pool, and its rollout overlap of 23 stays under the API
+// candidate's 65, so the Math.max is the API candidate rather than the gateway.
+//
+// `Deploy Relay Asia Topology` gates on `withinBudget == true`, so the single remaining
+// connection is the whole margin. Anything that raises a pool or an instance count moves it.
+test('production plus the push gateway keeps allowance and reserve below the ceiling', () => {
   const report = readRelayCloudSqlConnectionBudget()
 
-  assert.deepEqual(report.consumers, { cells: 230, directors: 15, auth: 20, api: 50 })
+  assert.deepEqual(report.consumers, { cells: 230, directors: 15, auth: 20, api: 50, push: 4 })
   assert.deepEqual(report.asia, { cells: 3, poolMax: 10 })
-  assert.equal(report.configuredMaximum, 315)
+  assert.equal(report.configuredMaximum, 319)
   assert.equal(report.rolloutOverlap.relayDirectorCandidate, 30)
   assert.equal(report.rolloutOverlap.apiCandidate, 65)
   assert.equal(report.rolloutOverlap.authCandidate, 35)
+  assert.equal(report.rolloutOverlap.pushCandidate, 23)
   assert.equal(report.rolloutOverlap.relayCells, 15)
   assert.equal(report.rolloutOverlap.retainedDirectorRollback, 15)
+  // The gateway does not set the maximum; the API candidate does, as it did before it existed.
   assert.equal(report.rolloutOverlap.maximum, 65)
   assert.equal(report.maintenanceAdminAllowance, 5)
   assert.equal(report.explicitReserve, 10)
   assert.equal(report.usableCeiling, 390)
+  assert.equal(report.operatingMaximum, 389)
+  assert.equal(report.remainingWithinUsableCeiling, 1)
+  assert.equal(report.budgetedTotal, 399)
+  assert.equal(report.unallocated, 1)
+  assert.equal(report.withinBudget, true)
+})
+
+// Why: the same relay shape without a push gateway is the before picture, and it stood at five
+// connections clear. Holding it here keeps the gateway's cost visible as the four it takes,
+// rather than letting drift elsewhere in the budget hide inside the same margin.
+test('the same relay shape without the gateway stays inside the ceiling', () => {
+  const report = calculateRelayCloudSqlConnectionBudget({
+    cellPoolTotal: 200,
+    asiaCellCount: 3,
+    asiaPoolMax: 10,
+    directorInstances: 5,
+    directorPoolMax: 3,
+    authInstances: 2,
+    authPoolMax: 10,
+    apiInstances: 10,
+    apiPoolMax: 5,
+    pushInstances: 0,
+    pushPoolMax: 0,
+    maxConnections: 400,
+    maintenanceAdminAllowance: 5,
+    explicitReserve: 10
+  })
+
+  assert.equal(report.consumers.push, 0)
+  assert.equal(report.rolloutOverlap.maximum, 65)
   assert.equal(report.operatingMaximum, 385)
   assert.equal(report.remainingWithinUsableCeiling, 5)
-  assert.equal(report.budgetedTotal, 395)
-  assert.equal(report.unallocated, 5)
   assert.equal(report.withinBudget, true)
 })
 
+// Why: a tagged candidate is directly addressable and sits outside the service-wide cap, so both
+// push revisions can reach the ceiling at once. The API and auth candidates add one copy; this
+// one adds two, like the director candidate.
+test('the push rollout scenario doubles the gateway draw over the retained director', () => {
+  const report = calculateRelayCloudSqlConnectionBudget({
+    cellPoolTotal: 0,
+    asiaCellCount: 0,
+    asiaPoolMax: 0,
+    directorInstances: 5,
+    directorPoolMax: 3,
+    authInstances: 0,
+    authPoolMax: 0,
+    apiInstances: 0,
+    apiPoolMax: 0,
+    pushInstances: 2,
+    pushPoolMax: 2,
+    maxConnections: 400,
+    maintenanceAdminAllowance: 5,
+    explicitReserve: 10
+  })
+
+  assert.equal(report.consumers.push, 4)
+  // 15 retained director rollback, plus the 4-connection draw counted twice.
+  assert.equal(report.rolloutOverlap.pushCandidate, 23)
+})
+
 test('fails closed when pool growth consumes the explicit reserve', () => {
   const report = calculateRelayCloudSqlConnectionBudget({
     cellPoolTotal: 200,
@@ -39,12 +102,14 @@ test('fails closed when pool growth consumes the explicit reserve', () => {
     authPoolMax: 10,
     apiInstances: 20,
     apiPoolMax: 5,
+    pushInstances: 4,
+    pushPoolMax: 10,
     maxConnections: 400,
     maintenanceAdminAllowance: 5,
     explicitReserve: 10
   })
 
-  assert.equal(report.operatingMaximum, 515)
+  assert.equal(report.operatingMaximum, 555)
   assert.equal(report.withinBudget, false)
 })
 
@@ -63,7 +128,11 @@ test('excludes fenced cell pools and reads per-cell pool overrides', () => {
           }
         }
       `,
-      terraformVariables: 'variable "relay_director_database_pool_max" { default = 3 }',
+      terraformVariables: [
+        'variable "relay_director_database_pool_max" { default = 3 }',
+        'variable "push_max_instances" { default = 1 }',
+        'variable "push_database_pool_max" { default = 2 }'
+      ].join('\n'),
       relayConfig: 'export const RELAY_DATABASE_POOL_MAX = 10'
     },
     maxConnections: 100,
@@ -72,8 +141,42 @@ test('excludes fenced cell pools and reads per-cell pool overrides', () => {
   })
 
   assert.equal(report.consumers.cells, 14)
-  assert.equal(report.operatingMaximum, 46)
-  assert.equal(report.budgetedTotal, 47)
+  // No push_max_instances in this tfvars, so the variable default of one instance holds.
+  assert.equal(report.consumers.push, 2)
+  assert.equal(report.operatingMaximum, 48)
+  assert.equal(report.budgetedTotal, 49)
+})
+
+// Why: production.tfvars overrides push_max_instances down to 2 while variables.tf still defaults
+// to 4, so reading the default instead of the override would overstate the live draw by half.
+test('a tfvars push_max_instances override wins over the variable default', () => {
+  const report = readRelayCloudSqlConnectionBudget({
+    proposedAsiaCellCount: 1,
+    appConsumers: { authInstances: 1, authPoolMax: 10, apiInstances: 1, apiPoolMax: 5, maxConnections: 100 },
+    sources: {
+      productionTfvars: `
+        relay_max_instances = 1
+        push_max_instances  = 3
+        relay_gce_fenced_cells = []
+        relay_gce_cells = {
+          "production-gce-c2" = { database_pool_max = 4
+          }
+        }
+      `,
+      terraformVariables: [
+        'variable "relay_director_database_pool_max" { default = 3 }',
+        'variable "push_max_instances" { default = 1 }',
+        'variable "push_database_pool_max" { default = 2 }'
+      ].join('\n'),
+      relayConfig: 'export const RELAY_DATABASE_POOL_MAX = 10'
+    },
+    maxConnections: 100,
+    maintenanceAdminAllowance: 1,
+    explicitReserve: 1
+  })
+
+  assert.equal(report.consumers.push, 6)
+  assert.equal(report.rolloutOverlap.pushCandidate, 15)
 })
 
 test('requires strict headroom below the physical ceiling', () => {
@@ -87,12 +190,14 @@ test('requires strict headroom below the physical ceiling', () => {
     authPoolMax: 10,
     apiInstances: 1,
     apiPoolMax: 5,
+    pushInstances: 1,
+    pushPoolMax: 2,
     maxConnections: 50,
     maintenanceAdminAllowance: 9,
     explicitReserve: 3
   })
 
-  assert.equal(report.budgetedTotal, 63)
+  assert.equal(report.budgetedTotal, 65)
   assert.equal(report.withinBudget, false)
 })