fix(profiles,users): serialize backfill setup with OIDC provisioning transactions - #2353
Merged
Merged
Conversation
…transactions `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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a DuckDB/SQLite shared-connection race in OIDC provisioning by ensuring _smrt_backfills initialization is serialized under the same coordinator locks as the provisioning transaction and post-commit rebind, and adds a deterministic regression test to enforce that ordering contract.
Changes:
- Move
_smrt_backfillstracker initialization insidewithProvisioningLocks(but still outside the provisioning transaction) to prevent root-handle statements from overlapping a concurrent flow’s open transaction. - Add a new coordinator ordering regression test for DuckDB shared root handles.
- Update package
AGENTS.mddocumentation in Profiles and Users to reflect the expanded “serialize every coordinator-owned root-handle statement” contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/profiles/src/auth/oidcProvisioningCoordinator.ts | Serializes backfill tracker initialization under coordinator locks to prevent overlapping root-handle statements with open transactions. |
| packages/profiles/src/tests/oidc-provisioning-coordinator.test.ts | Adds deterministic regression coverage ensuring no root-handle statements occur while another flow holds the provisioning transaction. |
| packages/profiles/AGENTS.md | Documents the updated coordinator serialization contract for SQLite/DuckDB. |
| packages/users/AGENTS.md | Mirrors the updated coordinator serialization contract documentation for Users. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…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
…ning connection 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2352
Root cause (verified, not assumed)
The issue's hypothesis is correct, and I confirmed it two ways.
1. Adapter-level. The DuckDB adapter guards
transaction()/beginTransaction()with an internalconnectionLock, but top-levelquery()takes no lock. A statement left in flight whenBEGIN TRANSACTIONruns on the same native connection fails immediately:So concurrent root reads are not the hazard (the post-commit rebind's
Promise.allis fine); a root statement overlapping an open transaction on the same connection is.2. In the failing test. Instrumenting
oidc-account-linking.test.tswith sub-millisecond start/end markers on the shared root handle shows the exact overlap on this machine, even on a green run:coordinateOidcProvisioningestablished the shared_smrt_backfillstable before acquiring itsadapter-transactionlock ("outside the provisioning transaction"). Both flows therefore issue a root statement while unserialized; whichever flow wins the lock opensBEGIN TRANSACTIONon the same native connection a few hundred microseconds later, while the other flow's statement is still in flight. The window is ~50µs here, which is why the metal pods mostly get away with it and 4-vCPU hosted runners do not.Everything else the coordinator does on the root handle — the transaction and the post-commit rebind — was already inside the lock, which is why the reported stack surfaced in
rebindOidcProfileResult: it is a victim of the poisoned connection, not the cause.Fix
Move the tracker initialization inside
withProvisioningLocks, still outside the provisioning transaction. Every root-handle statement the coordinator owns — tracker setup, the transaction, the post-commit rebind — is now serialized per database URL on SQLite/DuckDB. Transaction-bound callers are unchanged: they still never run_smrt_backfillsDDL.@happyvertical/smrt-profilesand@happyvertical/smrt-usersboth route through this one coordinator (@happyvertical/smrt-profiles/internal/oidc-provisioning), so the single change covers both failing suites — there is no second copy insmrt-users. No assertion was weakened:maxActiveLookups/maxActiveResolversstay1and every row-count assertion is untouched.Postgres root handles are unaffected in substance: they never take the
adapter-transactionkey, so initialization simply moves under the identity/email locks, and the shared initialization promise makes repeat calls a single cached check.Regression guard
packages/profiles/src/__tests__/oidc-provisioning-coordinator.test.tsasserts the ordering contract deterministically rather than by racing: it blocks one flow inside its provisioning transaction, starts a second flow, and asserts the second issues no statement on the shared DuckDB root handle. Reverting only the fix turns it red with the precise offender:Validation
@happyvertical/smrt-profilesfull suite — 17 files, 209 passed (was 208; +1 new test)@happyvertical/smrt-users—oidc-provisioning-safety.test.ts93 passed; whole node suite 22 files / 421 passed. The 7 Svelte component files cannot load in an agent worktree (vite8 + rolldownCould not resolve 'node:module'), so CI is the gate for those.oidc-account-linking.test.ts -t "independent DuckDB handles"oidc-provisioning-safety.test.ts -t "one DuckDB root handle"--sequence.shuffleprobe: 6/6 green on each OIDC file (43 and 93 tests)pnpm typecheck(profiles, users),biome checkon touched files,pnpm check:agents-chain,pnpm smrt dev:knowledge-check— cleanaffected-package-testsshards.No changeset is committed — release automation generates them on merge.
Update: hosted CI found a second, dominant hazard
Run 31994388100 on
5edce0dbd(lock-ordering fix only) shows the fix above was necessary but not sufficient:@happyvertical/smrt-users#test, all 3 retries,Failed to execute raw queryatUserCollection.ts:726insidewithProvisioningLock@happyvertical/smrt-profiles#test, silent worker abort (16/17 files, 201/209 tests) — the issue's other reported signatureBoth failures are inside the coordinator lock, where no other flow can run. So the remaining overlap is a flow racing itself:
rebindOidcProvisioningResult/rebindOidcProfileResultissued their two or three primary-key reads throughPromise.allon one native DuckDB connection. That is why the originally reported profiles stack pointed atSELECT * FROM profiles WHERE id = $1in the rebind — it was the direct cause, not a downstream victim. My local probe passed 400 rounds of that shape on macOS/arm64; the hosted x86 runners do not tolerate it.Second fix (
c7bca9561): the rebind reads and theLIMIT 2owner/email candidate reads are now sequential in both packages. They are primary-key lookups — one or two extra round trips on a first login, immaterial on pooled Postgres.Second guard:
never overlaps two statements on one shared DuckDB connection during concurrent provisioningcounts in-flight statements across the root handle and its transaction handle while two realcreateProfileFromOidcflows run. It fails on structure, not on a race — restoring eitherPromise.allturns it red naming the duplicated statement:This guard would have caught the original defect on any machine, which the timing-dependent suites could not.
Revalidation: profiles 17 files / 210 passed; users node suite 22 files / 421 passed (7 Svelte files need CI — vite8+rolldown cannot load the plugin in an agent worktree); 20/20 repeats of the users DuckDB test; 6/6 shuffle on the guard file; typecheck, biome, agents-chain, knowledge-check clean.
{"schema":"hv-agent-run:v1","policy_revision":"1.0.0","runtime":"claude","session":"bb4c7a21-74b8-4fce-972c-107c341951d9","issue":"https://github.com/happyvertical/smrt/issues/2352"}