From 23a25eaa58b0caec4e671fba57e4f401191e25f3 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 7 Oct 2026 21:00:44 -0400 Subject: [PATCH] fix(relay): let the same-cap roll's post-roll verify outlast one DB stall (#26372) * fix(relay): let the same-cap roll's post-roll verify outlast one DB stall The verify step's admin reads gave up after three 2 s retries; on 2026-10-07 c18 failed its roll on 503 'no healthy upstream' during a DB stall and was healthy 16 s later. The step now retries 5xx for up to 60 s, which covers a stall plus readiness grace and two LB health checks, and still fails a cell that stays down. 4xx stays final. * test(relay): surface curl stderr when the verify retry test fails * fix(relay): retry the post-roll verify reads in bash, since curl 7.81 ignores --retry under --fail-with-body * fix(relay): never report a stale body after a refused verify read; stop pinning inert curl retry flags --- ...d-deploy-relay-production-same-cap-job.yml | 36 ++++-- ...lay-admin-endpoint-retry-workflow.test.mjs | 106 +++++++++++++++++- 2 files changed, 124 insertions(+), 18 deletions(-) diff --git a/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml b/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml index 8b13c4fa3f1..333c1baebe3 100644 --- a/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml +++ b/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml @@ -716,19 +716,31 @@ jobs: env: ORCA_RELAY_ADMIN_ID_TOKEN: ${{ steps.post-auth.outputs.id_token }} run: | - # A single transient 5xx (LB warm-up behind a fresh instance) must not - # fail a canary; 4xx (auth, generation mismatch) still fails fast. + # Rides out one DB stall: 5-7 s, then readiness grace and two 10 s LB + # health checks (c18, 10-07 18:49Z: 503 "no healthy upstream", healthy + # 16 s later). A cell still down after 60 s fails; other 4xx fail fast. + # Retried in this loop: the runner's 7.81 client (Ubuntu 22.04) never + # retries under --fail-with-body, so its --retry flags were inert. admin_post() { - local out="${RUNNER_TEMP}/$1.json" - if ! curl --fail-with-body --max-time 30 \ - --retry 3 --retry-delay 2 --retry-connrefused --output "${out}" \ - --request POST "$2" \ - --header "Authorization: Bearer ${ORCA_RELAY_ADMIN_ID_TOKEN}" \ - --header 'Content-Type: application/json' --data "$3"; then - cat "${out}" >&2 - return 1 - fi - cat "${out}" + local out="${RUNNER_TEMP}/$1.json" window=60 delay=5 status + local deadline=$((SECONDS + window)) + while true; do + # A refused or timed-out attempt writes no body; never report the last one's. + rm -f "${out}" + status="$(curl --silent --show-error --max-time 30 --output "${out}" \ + --write-out '%{http_code}' --request POST "$2" \ + --header "Authorization: Bearer ${ORCA_RELAY_ADMIN_ID_TOKEN}" \ + --header 'Content-Type: application/json' --data "$3")" || status=000 + case "${status}" in + 2??) cat "${out}"; return 0 ;; + 000|408|429|5??) ((SECONDS + delay < deadline)) || break ;; + *) break ;; + esac + sleep "${delay}" + done + echo "admin_post $1: HTTP ${status}" >&2 + cat "${out}" >&2 2>/dev/null || true + return 1 } node dev/scripts/verify-relay-capacity-transition.mjs \ --director-origin "${DIRECTOR_ORIGIN}" --cell-origin "${CELL_ORIGIN}" \ diff --git a/cloud/dev/scripts/relay-admin-endpoint-retry-workflow.test.mjs b/cloud/dev/scripts/relay-admin-endpoint-retry-workflow.test.mjs index 196fff9edf3..0f5845a732c 100644 --- a/cloud/dev/scripts/relay-admin-endpoint-retry-workflow.test.mjs +++ b/cloud/dev/scripts/relay-admin-endpoint-retry-workflow.test.mjs @@ -1,5 +1,9 @@ import assert from 'node:assert/strict' -import { readFileSync } from 'node:fs' +import { spawn } from 'node:child_process' +import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { createServer } from 'node:http' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import { test } from 'node:test' import { fileURLToPath } from 'node:url' import { relayWorkflowUrl } from './relay-repository.mjs' @@ -13,13 +17,28 @@ function workflow(name) { return readFileSync(fileURLToPath(relayWorkflowUrl(name)), 'utf8') } -// A single transient 5xx from a warming instance behind the global load balancer -// must not fail a canary, so no admin endpoint may be read by a bare curl. -test('no admin endpoint is reached by a curl without a bounded retry', () => { +const VERIFY_STEP = 'name: Verify new incarnation, exact image, protocol, and durable safety' +const STALL_WINDOW = 'local out="${RUNNER_TEMP}/$1.json" window=60 delay=5 status' + +function verifyStep() { + return workflow('deploy-relay-production-same-cap-job.yml') + .split(VERIFY_STEP)[1] + .split('\n - name:')[0] +} + +function verifyAdminPost() { + const step = verifyStep() + const start = step.indexOf('admin_post() {') + return step.slice(start, step.indexOf('\n }', start) + '\n }'.length) +} + +// curl 7.81 (the Ubuntu 22.04 runners) never retries under --fail-with-body, so the +// remaining --retry flags are inert and only the verify loop below really retries. +// What holds for every admin curl: it is bounded and never retries a final 4xx. +test('every admin curl is bounded and never retries a final 4xx', () => { for (const name of WORKFLOWS) { for (const invocation of workflow(name).split(/\bcurl\b/).slice(1)) { const flags = invocation.split('\n }')[0] - assert.match(flags, /--retry 3 --retry-delay 2 --retry-connrefused/, name) assert.match(flags, /--max-time 30/, name) // --retry-all-errors would also retry 401, 403, and 409, which are final. assert.doesNotMatch(flags, /--retry-all-errors/, name) @@ -39,5 +58,80 @@ test('every retried admin request captures only the final attempt body', () => { /TARGET_RUNTIME="\$\(admin_post target-runtime/, /TARGET_DIRECTOR_STATUS="\$\(admin_post target-cell-status/ ]) assert.match(job, call) - assert.doesNotMatch(job, /\$\(curl /) + // The verify loop captures only the status code; its body goes to the file. + assert.doesNotMatch(job.replace(verifyAdminPost(), ''), /\$\(curl /) +}) + +// c18, 2026-10-07 18:49Z: a DB stall made the fresh cell answer 503 "no healthy upstream"; +// three 2 s retries gave up and the cell was healthy 16 s later. +// 60 s covers a 7 s stall, 16 s readiness grace and two 10 s LB health checks. +test('the post-roll verify outlasts one DB stall and its health-check recovery', () => { + assert.ok(verifyAdminPost().includes(STALL_WINDOW)) + assert.match(verifyAdminPost(), /--max-time 30/) +}) + +// Runs the step's own admin_post, with its retry clock scaled down twelve-fold. +async function runVerifyAdminPost(respond) { + const scaled = verifyAdminPost().replace('window=60 delay=5', 'window=5 delay=1') + assert.notEqual(scaled, verifyAdminPost()) + let calls = 0 + const startedAt = Date.now() + const server = createServer((request, response) => { + calls += 1 + const { status, body, drop } = respond(Date.now() - startedAt) + if (drop) return void request.socket.destroy() + response.writeHead(status, { 'content-type': 'application/json' }) + response.end(body) + }) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const runnerTemp = mkdtempSync(join(tmpdir(), 'relay-verify-admin-post-')) + try { + const url = `http://127.0.0.1:${server.address().port}/v1/admin/runtime-status` + const child = spawn( + 'bash', + ['-euo', 'pipefail', '-c', `${scaled}\nadmin_post target-runtime "${url}" '{"v":1}'`], + { env: { ...process.env, RUNNER_TEMP: runnerTemp, ORCA_RELAY_ADMIN_ID_TOKEN: 't' } } + ) + let stdout = '' + let stderr = '' + child.stdout.on('data', (chunk) => { stdout += chunk }) + child.stderr.on('data', (chunk) => { stderr += chunk }) + const code = await new Promise((resolve) => child.on('close', resolve)) + return { code, stdout, stderr, calls, elapsedMs: Date.now() - startedAt } + } finally { + server.close() + rmSync(runnerTemp, { recursive: true, force: true }) + } +} + +const UNHEALTHY = { status: 503, body: 'no healthy upstream' } + +test('the verify read recovers when the cell comes back inside the retry window', async () => { + const result = await runVerifyAdminPost((elapsedMs) => + elapsedMs < 2_500 ? UNHEALTHY : { status: 200, body: '{"ok":true}' } + ) + assert.equal(result.code, 0, result.stderr) + assert.equal(result.stdout, '{"ok":true}') + // The old three-retry budget would have given up here. + assert.ok(result.calls > 3, `calls ${result.calls}`) +}) + +test('the verify read still fails a cell that stays down past the window', async () => { + const result = await runVerifyAdminPost(() => UNHEALTHY) + assert.notEqual(result.code, 0) + assert.ok(result.elapsedMs >= 3_000, `gave up after ${result.elapsedMs} ms: ${result.stderr}`) + assert.ok(result.elapsedMs < 8_000, `bounded at ${result.elapsedMs} ms`) +}) + +test('the verify read fails a final 4xx without waiting out the window', async () => { + const result = await runVerifyAdminPost(() => ({ status: 409, body: '{"error":"generation"}' })) + assert.notEqual(result.code, 0) + assert.equal(result.calls, 1) +}) + +test('a final connection failure does not report the previous attempt body', async () => { + const result = await runVerifyAdminPost((elapsedMs) => (elapsedMs < 1_000 ? UNHEALTHY : { drop: true })) + assert.notEqual(result.code, 0) + assert.match(result.stderr, /admin_post target-runtime: HTTP 000/) + assert.doesNotMatch(result.stderr, /no healthy upstream/) })