From dd9eaa958545de250e504bc5a0ff0e05afc90da1 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:20:27 -0400 Subject: [PATCH] fix(cloud): retry the committed-winner collision codes in relay schema startup (#18553) * fix(cloud): retry the committed-winner collision codes in relay schema startup `CREATE TABLE IF NOT EXISTS` only checks the name before the catalog inserts, so the loser of a concurrent CREATE fails in one of two ways depending on timing: on the catalog unique index (23505, which the startup retry already handled) or, when the winner has committed by the time the loser reaches TypeCreate / heap_create_with_catalog, on the name check those routines repeat (42710 duplicate type, 42P07 duplicate relation). The predicate treated the latter as fatal, so a director could fail startup on a table it was about to find present. This is what turned `postgres-schema-concurrency-postgres.test.ts` red on main and on every relay PR (CI's shared runner loses the race more often than a dev box): a throwaway diagnostic run in CI reported 42710 from TypeCreate and 42P07 from heap_create_with_catalog as the only rejection reasons. Treat 42710/42P07 as retryable for `CREATE TABLE IF NOT EXISTS` and 42P07 for `CREATE [UNIQUE] INDEX IF NOT EXISTS`; every other statement shape still fails fast. The concurrency test now runs ten rounds and reports the loser's SQLSTATE instead of a bare boolean. * chore(cloud): allowlist the RFC 6455 example Sec-WebSocket-Key for upgrade tests Cloud Verify's Secret scan runs gitleaks over --all refs, so the raw-socket upgrade test on fix/relay-upgrade-malformed-uri (#18547) trips every cloud PR's scan until its allowlist reaches main. Land the allowlist here first. --- cloud/.gitleaks.toml | 7 ++++ .../src/database-postgres-timeout.test.ts | 33 ++++++++++++++++++ ...stgres-schema-concurrency-postgres.test.ts | 34 +++++++++++-------- .../apps/relay/src/postgres-schema-startup.ts | 34 +++++++++++++++---- 4 files changed, 88 insertions(+), 20 deletions(-) diff --git a/cloud/.gitleaks.toml b/cloud/.gitleaks.toml index fc5c725c37d..0bb1f966fae 100644 --- a/cloud/.gitleaks.toml +++ b/cloud/.gitleaks.toml @@ -13,3 +13,10 @@ description = "Cloud SQL rollout lease holder keys in the action's unit tests" regexTarget = "secret" paths = ['''\.github/actions/cloud-sql-rollout-lease/[a-z-]+\.test\.mjs$'''] regexes = ['''^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+/[0-9]+$'''] + +# RFC 6455 ยง1.3 example handshake nonce ("the sample nonce" in base64), sent by the raw-socket +# upgrade tests; the generic key rule reads any base64 header value as a secret. +[[allowlists]] +description = "RFC 6455 example Sec-WebSocket-Key in upgrade tests" +regexTarget = "secret" +regexes = ['''^dGhlIHNhbXBsZSBub25jZQ==$'''] diff --git a/cloud/apps/relay/src/database-postgres-timeout.test.ts b/cloud/apps/relay/src/database-postgres-timeout.test.ts index a04a9eea042..7aba1234f7f 100644 --- a/cloud/apps/relay/src/database-postgres-timeout.test.ts +++ b/cloud/apps/relay/src/database-postgres-timeout.test.ts @@ -118,6 +118,39 @@ describe('PostgreSQL schema startup', () => { expect(query).toHaveBeenCalledTimes(2) }) + it.each([ + ['42710', 'CREATE TABLE IF NOT EXISTS test'], + ['42P07', 'CREATE TABLE IF NOT EXISTS test'], + ['42P07', 'CREATE INDEX IF NOT EXISTS test_index ON test(id)'], + ['42P07', 'CREATE UNIQUE INDEX IF NOT EXISTS test_index ON test(id)'] + ])('retries the committed-winner %s collision for %s', async (code, statement) => { + vi.spyOn(console, 'warn').mockImplementation(() => undefined) + const collision = Object.assign(new Error('already exists'), { code }) + const query = vi + .fn<(statement: string) => Promise>() + .mockRejectedValueOnce(collision) + .mockResolvedValue(undefined) + + await applyPostgresSchema([statement], query, { wait: async () => undefined }) + + expect(query).toHaveBeenCalledTimes(2) + }) + + it.each([ + ['42710', 'CREATE INDEX IF NOT EXISTS test_index ON test(id)'], + ['42710', 'CREATE TABLE test'], + ['42P07', 'CREATE TABLE test'], + ['42P07', 'CREATE INDEX test_index ON test(id)'] + ])('does not retry %s for %s', async (code, statement) => { + const error = Object.assign(new Error('already exists'), { code }) + const query = vi.fn<(statement: string) => Promise>().mockRejectedValue(error) + const pause = vi.fn(async () => undefined) + + await expect(applyPostgresSchema([statement], query, { wait: pause })).rejects.toBe(error) + + expect(pause).not.toHaveBeenCalled() + }) + it.each([ ['pg_type_typname_nsp_index', 'CREATE TABLE test'], ['pg_class_relname_nsp_index', 'CREATE INDEX test_index ON test(id)'] diff --git a/cloud/apps/relay/src/postgres-schema-concurrency-postgres.test.ts b/cloud/apps/relay/src/postgres-schema-concurrency-postgres.test.ts index 5208f137ae8..9ed3cb2f324 100644 --- a/cloud/apps/relay/src/postgres-schema-concurrency-postgres.test.ts +++ b/cloud/apps/relay/src/postgres-schema-concurrency-postgres.test.ts @@ -34,22 +34,28 @@ describePostgres('PostgreSQL schema concurrency', () => { }) it('opens five directors when one new table is absent', async () => { - const initial = await openRelayDatabase({ databaseUrl: scopedUrl, dataDir: '' }) - await initial.query(`DROP TABLE relay_cell_legacy_fence_adoptions`) - await initial.close() + // Which catalog step the race loser fails on depends on scheduling, so run several rounds and + // keep the loser's SQLSTATE in the failure instead of a bare boolean. + for (let round = 0; round < 10; round += 1) { + const initial = await openRelayDatabase({ databaseUrl: scopedUrl, dataDir: '' }) + await initial.query(`DROP TABLE relay_cell_legacy_fence_adoptions`) + await initial.close() - const results = await Promise.allSettled( - Array.from({ length: 5 }, async (): Promise => - await openRelayDatabase({ databaseUrl: scopedUrl, dataDir: '' }) + const results = await Promise.allSettled( + Array.from({ length: 5 }, async (): Promise => + await openRelayDatabase({ databaseUrl: scopedUrl, dataDir: '' }) + ) + ) + const databases = results.flatMap((result) => + result.status === 'fulfilled' ? [result.value] : [] ) - ) - const databases = results.flatMap((result) => - result.status === 'fulfilled' ? [result.value] : [] - ) - try { - expect(results.every((result) => result.status === 'fulfilled')).toBe(true) - } finally { await Promise.all(databases.map(async (database) => await database.close())) + const rejections = results.flatMap((result) => + result.status === 'rejected' + ? [{ round, code: (result.reason as { code?: unknown }).code, message: String(result.reason) }] + : [] + ) + expect(rejections).toEqual([]) } - }) + }, 60_000) }) diff --git a/cloud/apps/relay/src/postgres-schema-startup.ts b/cloud/apps/relay/src/postgres-schema-startup.ts index 5e6260ad2fb..ba9efc6a792 100644 --- a/cloud/apps/relay/src/postgres-schema-startup.ts +++ b/cloud/apps/relay/src/postgres-schema-startup.ts @@ -22,15 +22,37 @@ function wait(delayMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, delayMs)) } +const CREATE_TABLE_IF_NOT_EXISTS = /^\s*CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS\b/i +const CREATE_INDEX_IF_NOT_EXISTS = /^\s*CREATE\s+(?:UNIQUE\s+)?INDEX\s+IF\s+NOT\s+EXISTS\b/i + +// `IF NOT EXISTS` only checks the name before the catalog inserts, so the loser of a concurrent +// CREATE can fail on the catalog unique index (23505) or, when the winner has already committed by +// the time the loser reaches TypeCreate/heap_create_with_catalog, on the name check those routines +// repeat (42710 duplicate type, 42P07 duplicate relation). Each is a no-op on the next attempt. +function concurrentCreateCollision( + value: { code?: unknown; constraint?: unknown }, + statement: string +): boolean { + if (CREATE_TABLE_IF_NOT_EXISTS.test(statement)) { + return ( + (value.code === '23505' && value.constraint === 'pg_type_typname_nsp_index') || + value.code === '42710' || + value.code === '42P07' + ) + } + if (CREATE_INDEX_IF_NOT_EXISTS.test(statement)) { + return ( + (value.code === '23505' && value.constraint === 'pg_class_relname_nsp_index') || + value.code === '42P07' + ) + } + return false +} + function retryableSchemaError(error: unknown, statement: string): boolean { const value = error as { code?: unknown; constraint?: unknown } return ( - RETRYABLE_SCHEMA_CODES.has(String(value.code)) || - (value.code === '23505' && - ((value.constraint === 'pg_type_typname_nsp_index' && - /^\s*CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS\b/i.test(statement)) || - (value.constraint === 'pg_class_relname_nsp_index' && - /^\s*CREATE\s+(?:UNIQUE\s+)?INDEX\s+IF\s+NOT\s+EXISTS\b/i.test(statement)))) + RETRYABLE_SCHEMA_CODES.has(String(value.code)) || concurrentCreateCollision(value, statement) ) }