fix(auth): route databaseHooks context.adapter through the transaction lane - #1554
Conversation
…n lane better-auth hands a databaseHooks before/after hook a GenericEndpointContext whose .context.adapter is the Auth adapter's ROOT instance — better-auth never swaps that reference for the transaction-bound one (only its own AsyncLocalStorage, read via getCurrentAdapter, does). A hook that queried through it during sign-up therefore ran on the outer lane while the sign-up transaction held the database's only connection: a hang on the Dev database, and a read outside the transaction (surviving a rollback) on pooled Postgres. The root instance now reads its lane from the same AsyncLocalStorage store the transaction-bound instance does. Both instances run inside the same boundLane.run(...) call for the life of the transaction, so a hook reaching through context.context.adapter lands on the transaction-bound connection without needing to touch better-auth's own ALS routing. Closes #1252 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEEWCWcKXFFYY3PXE2zBH6
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: a1078a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…llision 0070-the-auth-plugins-id-strategy-is-config-not-a-hardcoded-pin.md (#1550) and 0070-the-generated-clients-identity-is-a-process-wide-registry-entry.md (#1548) landed within a minute of each other and both claimed 0070, tripping check:adr-duplicates on every PR against main. Renumbered the id-strategy ADR to 0071 (2 filename-link updates in ADR-0048 and ADR-0060) rather than the client-identity one, which has 7+ scattered bare "(ADR-0070)" comment references across packages/core, packages/cli and packages/auth that would otherwise need updating instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEEWCWcKXFFYY3PXE2zBH6
|
The Opened #1555 to fix it (renumbers the id-strategy ADR to Generated by Claude Code |
Review: PR #1554 — databaseHooks / transaction-lane fixHigh-effort review against the actual diff ( 1. AsyncLocalStorage scoping / concurrency — no regression foundTraced the whole path through the real runtime: With the fix, the root instance's For concurrency: 2.
|
docs/content/reference/auth.md's "Known limits" section and the pending #1251 changeset (.changeset/silent-moons-gather.md) both still described the databaseHooks-hook-runs-outside-the-transaction limit this PR closes. Left unedited, both would have shipped in the same release as this PR's own changeset claiming it fixed — a contradiction in the same changelog entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEEWCWcKXFFYY3PXE2zBH6
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEEWCWcKXFFYY3PXE2zBH6
|
Thanks for the thorough review. Addressed both non-blocking findings:
Re the bundled ADR-0070→0071 commit: correct guess, that's not scope creep — All pushed (f690c53, a1078a7). Generated by Claude Code |
Coverage Report for Core Package Coverage (./packages/core)
File CoverageNo changed files found. |
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
Summary
better-auth hands a
databaseHooks.<model>.create.before/afterhook aGenericEndpointContextwhose.context.adapteris the Auth adapter's root instance — better-auth swaps in the transaction-bound instance only through its ownAsyncLocalStorage(runWithTransaction→getCurrentAdapter), which hooks never go through. A hook that queriedcontext.context.adapterduring sign-up therefore ran on the outer lane while the sign-up transaction held the database's only connection:This was a stated known limit on
opensaasAuthAdaptersince #1251 (adapter transactions). This PR closes it.Fix
The root factory instance (what
opensaasAuthAdapteritself returns, and therefore whatAuthContext.adapteris) now reads its lane from the sameAsyncLocalStoragestore the transaction-bound instance already used, instead of a closed-overunsafe. Both instances execute inside the sameboundLane.run(lane, …)call for the life of the transaction — the outerconfig.transactioncallback is that call — so a hook reaching throughcontext.context.adapternow lands on the transaction-bound connection, exactly like a call through better-auth's owngetCurrentAdapterwould. Outside a transaction the store is empty and the outer lane answers, unchanged.This is a one-line change (
packages/auth/src/adapter/index.ts): the root instance'sfactoryOn(...)call now passes the sharedlaneOfinstead of() => unsafe.packages/auth/src/adapter/index.ts's doc comment andpackages/auth/CLAUDE.md's "Known limits" section are updated to describe the new (fixed) behaviour instead of the limit.Test plan
packages/auth/tests/adapter-databasehooks-transaction.test.ts:databaseHooks.user.create.beforehook that reads viacontext.context.adapter.findOne(...)during a successful sign-up completes without hanging.databaseHooks.user.create.beforehook that writes viacontext.context.adapter.create(...), where the enclosing sign-up later fails (aCHECK (false)constraint onaccount, same technique asadapter-signup-atomicity.test.ts), rolls that write back together with the rest of sign-up — proving the hook's work is atomic with the transaction, not merely non-hanging.pnpm build(auth package)pnpm testinpackages/auth: 525 passed / 79 skipped (including the new tests)pnpm testinpackages/core: 1694 passed / 7 skipped (unaffected)pnpm lint,pnpm manypkg fix,pnpm formatCloses #1252
🤖 Generated with Claude Code
https://claude.ai/code/session_01HEEWCWcKXFFYY3PXE2zBH6
Generated by Claude Code