From 302389d5cb85e738c203db1a949e0df95a16daf5 Mon Sep 17 00:00:00 2001 From: Will Griffin Date: Sun, 16 Aug 2026 22:16:17 -0600 Subject: [PATCH 1/3] fix(profiles,users): serialize backfill setup with OIDC provisioning transactions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `coordinateOidcProvisioning` established the shared `_smrt_backfills` table before acquiring its `adapter-transaction` lock. SQLite and DuckDB root handles multiplex one native connection, so that pre-lock statement raced a concurrent flow's open transaction on the same connection: DuckDB then failed the losing prepared statement (`Failed to execute prepared statement`) or aborted the worker outright. The window is small, which is why it surfaced on 4-vCPU GitHub-hosted runners and only rarely on the slower metal fleet. The initialization now runs inside the coordinator locks, still outside the provisioning transaction, so every root-handle statement the coordinator owns — tracker setup, the transaction, and the post-commit rebind — is serialized per database URL. Both `@happyvertical/smrt-profiles` and `@happyvertical/smrt-users` route through this one coordinator, so the single change covers both failing suites. Adapter-level evidence: a statement left in flight when `BEGIN TRANSACTION` runs on the same DuckDB connection fails immediately, while concurrent root statements with no open transaction are fine over 400 rounds. Adds a deterministic coordinator ordering test that blocks one flow inside its provisioning transaction and asserts a second flow issues no statement on the shared root handle; it reproduces the defect (`SELECT 1 FROM _smrt_backfills LIMIT 1`) when the initialization is moved back outside the lock. Refs #2352 --- packages/profiles/AGENTS.md | 11 +- .../oidc-provisioning-coordinator.test.ts | 131 ++++++++++++++++++ .../src/auth/oidcProvisioningCoordinator.ts | 16 ++- packages/users/AGENTS.md | 9 +- 4 files changed, 154 insertions(+), 13 deletions(-) create mode 100644 packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts diff --git a/packages/profiles/AGENTS.md b/packages/profiles/AGENTS.md index 4e28b3637c..9546705df5 100644 --- a/packages/profiles/AGENTS.md +++ b/packages/profiles/AGENTS.md @@ -90,10 +90,13 @@ import { smrtProfilesGenerateBioPrompt } from '@happyvertical/smrt-profiles'; `@happyvertical/smrt-profiles/internal/oidc-provisioning` subpath instead of duplicating adapter probing, locking, or retry policy, and supply both exact issuer/subject and normalized-email lock keys in deterministic order. The - coordinator additionally serializes all SQLite/DuckDB provisioning - transactions per database URL because those adapters cannot overlap - unrelated root transactions safely and retries bounded PostgreSQL - deadlock/serialization failures. New OIDC Profiles use per-profile, + coordinator additionally serializes every root-handle statement it owns per + database URL on SQLite/DuckDB — shared `_smrt_backfills` initialization, the + provisioning transaction, and the post-commit rebind — because those adapters + multiplex one native connection and cannot overlap unrelated root + transactions safely; a statement issued outside that window races an open + transaction and DuckDB fails the losing prepared statement. It retries + bounded PostgreSQL deadlock/serialization failures. New OIDC Profiles use per-profile, non-semantic slugs so duplicate display names never invoke natural-key upsert. Caller-owned transactions never execute `_smrt_backfills` DDL; paths that perform canonical email lookup or reservation require the table to already diff --git a/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts b/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts new file mode 100644 index 0000000000..f2322b74e3 --- /dev/null +++ b/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts @@ -0,0 +1,131 @@ +import { getDatabase } from '@happyvertical/sql'; +import { describe, expect, it } from 'vitest'; +import { coordinateOidcProvisioning } from '../auth/oidcProvisioningCoordinator'; + +type DatabaseInterface = Awaited>; + +interface Deferred { + promise: Promise; + resolve: () => void; +} + +function createDeferred(): Deferred { + let resolve!: () => void; + const promise = new Promise((settle) => { + resolve = settle; + }); + return { promise, resolve }; +} + +/** Give a blocked flow every chance to reach its first statement. */ +async function settleEventLoop(): Promise { + for (let tick = 0; tick < 5; tick += 1) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } +} + +describe('coordinateOidcProvisioning shared-connection ordering', () => { + /** + * #2352: SQLite and DuckDB root handles multiplex one native connection, so + * any coordinator statement issued while a concurrent flow holds that + * connection's transaction races it. DuckDB reports the loser as + * `Failed to execute prepared statement` or aborts the worker outright, and + * the shared `_smrt_backfills` initialization used to run before the adapter + * lock. Every statement the coordinator owns must run inside that lock. + */ + it('issues no statement on a shared DuckDB root handle while another flow holds its provisioning transaction', async () => { + const db = (await getDatabase({ + type: 'duckdb', + url: ':memory:', + __smrtSkipVitestSchemaPreparation: true, + })) as DatabaseInterface; + const statementsDuringTransaction: string[] = []; + let openTransactions = 0; + + const observeRoot = (database: DatabaseInterface): DatabaseInterface => + new Proxy(database, { + get(target, property, receiver) { + if (property === 'query') { + return async (sql: string, ...params: unknown[]) => { + if (openTransactions > 0) { + // Record instead of executing. Really racing the connection + // would corrupt it and hide the ordering defect behind an + // adapter error. + statementsDuringTransaction.push( + sql.replace(/\s+/gu, ' ').trim(), + ); + throw new Error( + 'A coordinator statement raced an open provisioning transaction.', + ); + } + return target.query(sql, ...params); + }; + } + if (property === 'transaction') { + const transaction = target.transaction; + if (!transaction) return undefined; + return async ( + callback: (tx: DatabaseInterface) => Promise, + ): Promise => { + openTransactions += 1; + try { + return await transaction.call(target, callback); + } finally { + openTransactions -= 1; + } + }; + } + const value = Reflect.get(target, property, receiver); + return typeof value === 'function' ? value.bind(target) : value; + }, + }) as DatabaseInterface; + + const firstTransactionOpen = createDeferred(); + const releaseFirstTransaction = createDeferred(); + const coordinate = ( + lockKey: string, + provision: () => Promise, + ): Promise => + coordinateOidcProvisioning({ + db: observeRoot(db), + lockKeys: [lockKey], + isRaceConflict: () => false, + createTransactionError: (message, cause) => + new Error(message, cause === undefined ? undefined : { cause }), + createConcurrencyError: (cause) => + new Error( + 'Concurrent provisioning did not converge.', + cause === undefined ? undefined : { cause }, + ), + provision, + }); + + let first: Promise | undefined; + let second: Promise | undefined; + try { + first = coordinate('identity:first', async () => { + firstTransactionOpen.resolve(); + await releaseFirstTransaction.promise; + return 'first'; + }); + await firstTransactionOpen.promise; + + // The second flow must block on the adapter lock rather than touch the + // connection the first flow's transaction owns. + second = coordinate('identity:second', async () => 'second'); + await settleEventLoop(); + expect(statementsDuringTransaction).toEqual([]); + + releaseFirstTransaction.resolve(); + await expect(Promise.all([first, second])).resolves.toEqual([ + 'first', + 'second', + ]); + expect(statementsDuringTransaction).toEqual([]); + } finally { + releaseFirstTransaction.resolve(); + await Promise.allSettled([first, second]); + await db.close?.(); + } + }); +}); diff --git a/packages/profiles/src/auth/oidcProvisioningCoordinator.ts b/packages/profiles/src/auth/oidcProvisioningCoordinator.ts index bc3a436c89..946bf2d0c4 100644 --- a/packages/profiles/src/auth/oidcProvisioningCoordinator.ts +++ b/packages/profiles/src/auth/oidcProvisioningCoordinator.ts @@ -82,15 +82,19 @@ export interface OidcProvisioningCoordinatorOptions { export async function coordinateOidcProvisioning( options: OidcProvisioningCoordinatorOptions, ): Promise { - // Establish the shared backfill table outside the provisioning transaction. - // Transaction-bound callers are handled below via savepoints and must not - // leak DDL into their caller's transaction. const rootBackfillTableReady = typeof options.db.beginTransaction === 'function'; - if (rootBackfillTableReady) { - await new BackfillTracker({ db: options.db }).initialize(); - } return withProvisioningLocks(options.db, options.lockKeys, async () => { + // Establish the shared backfill table outside the provisioning + // transaction but inside the coordinator locks. Transaction-bound callers + // are handled below via savepoints and must not leak DDL into their + // caller's transaction, and SQLite and DuckDB root handles multiplex one + // native connection: a statement issued here while a concurrent flow + // holds that connection's transaction races it, and DuckDB then fails the + // in-flight prepared statement outright (#2352). + if (rootBackfillTableReady) { + await new BackfillTracker({ db: options.db }).initialize(); + } const result = await retryProvisioning(options, () => withProvisioningTransaction( options.db, diff --git a/packages/users/AGENTS.md b/packages/users/AGENTS.md index c94af50055..dba5ea69a6 100644 --- a/packages/users/AGENTS.md +++ b/packages/users/AGENTS.md @@ -295,9 +295,12 @@ the package root). private `oidc_profile_email_reservations.email_key`, `User.emailKey`, and the unique `User.profileId`; local callbacks acquire exact issuer/subject and normalized email locks in deterministic order so changed email claims also serialize. - SQLite and DuckDB callbacks additionally serialize transactions per database - URL because one adapter cannot safely overlap unrelated root transactions; - owner-authorized DuckDB callbacks use that same root-handle serialization; + SQLite and DuckDB callbacks additionally serialize every root-handle statement + the coordinator owns per database URL — `_smrt_backfills` initialization, the + transaction, and the post-commit rebind — because one adapter multiplexes a + single native connection and cannot safely overlap unrelated root + transactions; owner-authorized DuckDB callbacks use that same root-handle + serialization; PostgreSQL deadlock and serialization errors use a bounded transaction retry. Newly provisioned Profiles use non-semantic per-profile slugs so equal IdP display names cannot trigger a natural-key upsert; From 5edce0dbdf7af8eefedc2a22f4df976d57702f21 Mon Sep 17 00:00:00 2001 From: Will Griffin Date: Sun, 16 Aug 2026 22:24:45 -0600 Subject: [PATCH 2/3] test(profiles): keep the coordinator ordering guard's failure signal clean Both provisioning flows in the ordering guard are started before anything awaits them, so a flow that rejects while the guard is catching a regression surfaced as an unhandled rejection alongside the assertion. Mark each flow handled at creation, and race the first flow against its own transaction-open signal so an early failure reports that failure instead of timing out on a signal that never arrives. The red path now reports only the assertion that names the offending statement. Refs #2352 --- .../__tests__/oidc-provisioning-coordinator.test.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts b/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts index f2322b74e3..6382bc2bc6 100644 --- a/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts +++ b/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts @@ -108,11 +108,18 @@ describe('coordinateOidcProvisioning shared-connection ordering', () => { await releaseFirstTransaction.promise; return 'first'; }); - await firstTransactionOpen.promise; + first.catch(() => undefined); + // Racing the flow itself turns an early failure into that failure + // rather than a hook timeout waiting for a signal that never arrives. + await Promise.race([firstTransactionOpen.promise, first]); // The second flow must block on the adapter lock rather than touch the - // connection the first flow's transaction owns. + // connection the first flow's transaction owns. Mark it handled up + // front: when this guard catches a regression the flow rejects while + // nothing awaits it yet, and the unhandled rejection would drown the + // assertion that actually names the offending statement. second = coordinate('identity:second', async () => 'second'); + second.catch(() => undefined); await settleEventLoop(); expect(statementsDuringTransaction).toEqual([]); From c7bca9561be10d2feaf15fb448ace3e73efc9f56 Mon Sep 17 00:00:00 2001 From: Will Griffin Date: Sun, 16 Aug 2026 22:48:32 -0600 Subject: [PATCH 3/3] fix(profiles,users): stop overlapping statements on one OIDC provisioning connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hosted CI on the previous head proved the lock-ordering fix was necessary but not sufficient. Shard 1/3 failed all three retries at `UserCollection.ts:726` and shard 2/3 died as a silent vitest worker abort in the profiles DuckDB case — both inside the coordinator lock, where no other flow can be running. The remaining overlap is a flow racing itself: the post-commit rebind issued its two or three primary-key reads through `Promise.all` on one native DuckDB connection. macOS tolerates that; the hosted x86 runners fail the losing prepared statement or abort the process. Rebind reads and the owner/email candidate reads are now sequential. They are primary-key lookups, so the cost is one or two extra round trips on a first login, and Postgres pooling makes the difference immaterial there. The new overlap guard counts in-flight statements across both the root handle and its transaction handle during two concurrent provisionings, so it fails on structure rather than on a race: restoring either `Promise.all` turns it red naming the duplicated statement. Refs #2352 --- packages/profiles/AGENTS.md | 6 +- .../oidc-provisioning-coordinator.test.ts | 79 +++++++++++++++++++ packages/profiles/src/auth/resolveIdentity.ts | 11 ++- packages/users/AGENTS.md | 6 +- .../users/src/collections/UserCollection.ts | 41 ++++++---- 5 files changed, 118 insertions(+), 25 deletions(-) diff --git a/packages/profiles/AGENTS.md b/packages/profiles/AGENTS.md index 9546705df5..75c8ce0212 100644 --- a/packages/profiles/AGENTS.md +++ b/packages/profiles/AGENTS.md @@ -94,8 +94,10 @@ import { smrtProfilesGenerateBioPrompt } from '@happyvertical/smrt-profiles'; database URL on SQLite/DuckDB — shared `_smrt_backfills` initialization, the provisioning transaction, and the post-commit rebind — because those adapters multiplex one native connection and cannot overlap unrelated root - transactions safely; a statement issued outside that window races an open - transaction and DuckDB fails the losing prepared statement. It retries + transactions safely. **Never overlap two statements on one such handle, + transaction-bound or not** — no `Promise.all` over reads, not even + primary-key rebinds — because DuckDB fails the losing prepared statement or + aborts the process outright. It retries bounded PostgreSQL deadlock/serialization failures. New OIDC Profiles use per-profile, non-semantic slugs so duplicate display names never invoke natural-key upsert. Caller-owned transactions never execute `_smrt_backfills` DDL; paths that diff --git a/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts b/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts index 6382bc2bc6..cb6ac7a101 100644 --- a/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts +++ b/packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.ts @@ -1,6 +1,9 @@ import { getDatabase } from '@happyvertical/sql'; import { describe, expect, it } from 'vitest'; +import { createProfileFromOidc } from '../auth/index.js'; import { coordinateOidcProvisioning } from '../auth/oidcProvisioningCoordinator'; +import { ProfileTypeCollection } from '../collections/ProfileTypeCollection.js'; +import { backfillProfileEmailKeys } from '../migrations/backfillProfileEmailKeys.js'; type DatabaseInterface = Awaited>; @@ -135,4 +138,80 @@ describe('coordinateOidcProvisioning shared-connection ordering', () => { await db.close?.(); } }); + + /** + * #2352: the ordering guard above only covers statements the coordinator + * itself owns. A single flow can just as easily overlap its own statements + * — the post-commit rebind used `Promise.all` over two or three primary-key + * reads — and on one native connection that is the same defect. Counting + * in-flight statements catches both without racing: an overlap is a + * structural property of the call, not a timing accident. + */ + it('never overlaps two statements on one shared DuckDB connection during concurrent provisioning', async () => { + const db = (await getDatabase({ + type: 'duckdb', + url: ':memory:', + })) as DatabaseInterface; + await ProfileTypeCollection.create({ db }); + await backfillProfileEmailKeys(db); + + let inFlight = 0; + let maxInFlight = 0; + const overlapped: string[] = []; + const observe = (database: DatabaseInterface): DatabaseInterface => + new Proxy(database, { + get(target, property, receiver) { + if (property === 'query') { + return async (sql: string, ...params: unknown[]) => { + inFlight += 1; + maxInFlight = Math.max(maxInFlight, inFlight); + if (inFlight > 1) { + overlapped.push(sql.replace(/\s+/gu, ' ').trim()); + } + try { + return await target.query(sql, ...params); + } finally { + inFlight -= 1; + } + }; + } + if (property === 'transaction') { + const transaction = target.transaction; + if (!transaction) return undefined; + return async ( + callback: (tx: DatabaseInterface) => Promise, + ): Promise => + transaction.call(target, (tx) => callback(observe(tx))); + } + const value = Reflect.get(target, property, receiver); + return typeof value === 'function' ? value.bind(target) : value; + }, + }) as DatabaseInterface; + + const sharedClaims = { + sub: 'overlap-guard-subject', + iss: 'https://auth.example.com', + email_verified: true, + name: 'Overlap Guard', + }; + try { + await Promise.all([ + createProfileFromOidc( + { ...sharedClaims, email: 'overlap-guard-first@example.com' }, + 'example', + { db: observe(db) }, + ), + createProfileFromOidc( + { ...sharedClaims, email: 'overlap-guard-second@example.com' }, + 'example', + { db: observe(db) }, + ), + ]); + + expect(overlapped).toEqual([]); + expect(maxInFlight).toBe(1); + } finally { + await db.close?.(); + } + }); }); diff --git a/packages/profiles/src/auth/resolveIdentity.ts b/packages/profiles/src/auth/resolveIdentity.ts index c5f2a8d69c..0c323b16eb 100644 --- a/packages/profiles/src/auth/resolveIdentity.ts +++ b/packages/profiles/src/auth/resolveIdentity.ts @@ -519,10 +519,13 @@ async function rebindOidcProfileResult< const [profile, oidcIdentity] = await withSystemContext(async () => { const profiles = await ProfileCollection.create({ db: rootDb }); const identities = await OidcIdentityCollection.create({ db: rootDb }); - return Promise.all([ - profiles.get({ id: profileId }), - identities.get({ id: identityId }), - ]); + // Read one statement at a time. SQLite and DuckDB handles multiplex a + // single native connection, so overlapping statements on it fail the + // losing prepared statement or abort the process outright (#2352). These + // are primary-key reads, so the sequential cost is one extra round trip. + const rebound = await profiles.get({ id: profileId }); + const reboundIdentity = await identities.get({ id: identityId }); + return [rebound, reboundIdentity] as const; }); if (!profile || !oidcIdentity) { throw new Error( diff --git a/packages/users/AGENTS.md b/packages/users/AGENTS.md index dba5ea69a6..81f793d370 100644 --- a/packages/users/AGENTS.md +++ b/packages/users/AGENTS.md @@ -299,8 +299,10 @@ the package root). the coordinator owns per database URL — `_smrt_backfills` initialization, the transaction, and the post-commit rebind — because one adapter multiplexes a single native connection and cannot safely overlap unrelated root - transactions; owner-authorized DuckDB callbacks use that same root-handle - serialization; + transactions. Never overlap two statements on one such handle, inside a + transaction or not: rebind and owner/email candidate reads are sequential, + never `Promise.all`. Owner-authorized DuckDB callbacks use that same + root-handle serialization; PostgreSQL deadlock and serialization errors use a bounded transaction retry. Newly provisioned Profiles use non-semantic per-profile slugs so equal IdP display names cannot trigger a natural-key upsert; diff --git a/packages/users/src/collections/UserCollection.ts b/packages/users/src/collections/UserCollection.ts index 77a4775add..53b98a64e7 100644 --- a/packages/users/src/collections/UserCollection.ts +++ b/packages/users/src/collections/UserCollection.ts @@ -640,12 +640,15 @@ export class UserCollection extends SmrtCollection { LIMIT 2${lockClause}`, profileId, ); - const owners = await Promise.all( - result.rows.map((row) => - typeof row.id === 'string' ? this.get({ id: row.id }) : null, - ), - ); - return owners.filter((owner): owner is User => owner !== null); + // Sequential by design: this runs on the caller's transaction handle, and + // SQLite/DuckDB multiplex one native connection per handle (#2352). + const owners: User[] = []; + for (const row of result.rows) { + if (typeof row.id !== 'string') continue; + const owner = await this.get({ id: row.id }); + if (owner !== null && owner !== undefined) owners.push(owner); + } + return owners; } private async validateProfileOwnerAuthorization( @@ -723,11 +726,12 @@ export class UserCollection extends SmrtCollection { const users = await UserCollection.create({ db: rootDb }); const profiles = await ProfileCollection.create({ db: rootDb }); const identities = await OidcIdentityCollection.create({ db: rootDb }); - const [user, profile, oidcIdentity] = await Promise.all([ - users.get({ id: userId }), - profiles.get({ id: profileId }), - identities.get({ id: identityId }), - ]); + // Read one statement at a time. SQLite and DuckDB handles multiplex a + // single native connection, so overlapping statements on it fail the + // losing prepared statement or abort the process outright (#2352). + const user = await users.get({ id: userId }); + const profile = await profiles.get({ id: profileId }); + const oidcIdentity = await identities.get({ id: identityId }); if (!user || !profile || !oidcIdentity) { throw new OidcProvisioningError( 'concurrency_conflict', @@ -805,12 +809,15 @@ export class UserCollection extends SmrtCollection { for (const row of result.rows) { this.assertUserEmailKeyCurrent(row, email); } - const users = await Promise.all( - result.rows.map((row) => - typeof row.id === 'string' ? this.get({ id: row.id }) : null, - ), - ); - return users.filter((user): user is User => user !== null); + // Sequential by design: this runs on the caller's transaction handle, and + // SQLite/DuckDB multiplex one native connection per handle (#2352). + const matched: User[] = []; + for (const row of result.rows) { + if (typeof row.id !== 'string') continue; + const user = await this.get({ id: row.id }); + if (user !== null && user !== undefined) matched.push(user); + } + return matched; } /** Require the deploy-time backfill marker before indexed identity reads. */