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.
This commit is contained in:
Jinwoo Hong
2026-09-03 21:20:27 -04:00
committed by GitHub
parent 90780acb85
commit dd9eaa9585
4 changed files with 88 additions and 20 deletions
+7
View File
@@ -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==$''']
@@ -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<unknown>>()
.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<unknown>>().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)']
@@ -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<RelayDatabase> =>
await openRelayDatabase({ databaseUrl: scopedUrl, dataDir: '' })
const results = await Promise.allSettled(
Array.from({ length: 5 }, async (): Promise<RelayDatabase> =>
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)
})
@@ -22,15 +22,37 @@ function wait(delayMs: number): Promise<void> {
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)
)
}