Skip to content

feat: add transaction-aware listPrimaryUsersByEmail/ByPhoneNumber reads - #415

Draft
supertokens-agent-runner[bot] wants to merge 1 commit into
fix/nested-conn-acquisition-cleanupfrom
agent/issue-413-list-primary-users-transaction
Draft

supertokens-agent-runner[bot] wants to merge 1 commit into
fix/nested-conn-acquisition-cleanupfrom
agent/issue-413-list-primary-users-transaction

Conversation

@supertokens-agent-runner

Copy link
Copy Markdown
Contributor

Part of PLAN-018, Unit 3.

Problem / root cause

AuthRecipeSQLStorage callers that already hold a transaction connection had no
connection-reusing way to run the "list primary users by email / phone" reads.
Calling the non-transaction listPrimaryUsersByEmail / listPrimaryUsersByPhoneNumber
from inside a transaction borrows a second connection from the same Hikari pool
while the caller still holds the first — a nested same-pool acquisition that, under
load, contributes to pool exhaustion. PLAN-018 eliminates these nested acquisitions.

Fix

Implements the two new plugin-interface _Transaction reads (contract from
supertokens-plugin-interface#227) in postgres:

  • Start.listPrimaryUsersByEmail_Transaction(tenantIdentifier, con, email)
  • Start.listPrimaryUsersByPhoneNumber_Transaction(tenantIdentifier, con, phoneNumber)

Both delegate to new GeneralQueries.*_Transaction methods that mirror the existing
non-transaction forms exactly — same migration-mode dispatch (_new vs _legacy),
same query strings, same de-duplication and time_joined ordering — but run every read
on the caller's connection via QueryExecutorTemplate.execute(con, …) instead of
execute(start, …). To keep every read on that one connection, connection-taking twins
were added to the recipe query helpers the fan-out uses:

  • EmailPasswordQueries.getPrimaryUserIdUsingEmail_Transaction
  • PasswordlessQueries.getPrimaryUserIdUsingEmail_Transaction /
    getPrimaryUserByPhoneNumber_Transaction
  • ThirdPartyQueries.getPrimaryUserIdUsingEmail_Transaction
  • AccountInfoQueries.listPrimaryUserIdsByEmail_Transaction /
    listPrimaryUserIdsByPhoneNumber_Transaction

WebAuthN already exposed a connection-taking twin
(getPrimaryUserIdForTenantUsingEmail_Transaction) and
getPrimaryUserInfoForUserIds_Transaction already existed — both are reused, not
duplicated. These are plain reads: no FOR UPDATE.

No schema change, no new index, no migration script, no manifest.json entry — this is
purely a connection-ownership change over the existing tables/SQL. No version bump.

Tests added

src/test/java/.../ListPrimaryUsersTransactionParityTest.java: seeds three unlinked
primary users sharing an email across emailpassword / thirdparty / passwordless (plus a
phone on the passwordless user), then asserts the _Transaction reads return exactly the
same users, in the same order, as the non-transaction reads — run in both migration-mode
branches (LEGACY = read-old and DUAL_WRITE_READ_NEW = read-new).

What I ran locally

  • Compile parity against the real dependency. Compiled src/main/java (javac 21)
    against the plugin-interface#227 branch classes + the plugin's compile dependencies:
    clean. Compiled the same sources against the integration base without fix: removing restriction of connection pool size for bulk import #227: fails
    exactly on the two @Override methods in Start.java (method does not override … a supertype) — confirming the cross-repo contract is the only thing gating compilation
    and that this PR wires to it correctly.
  • The new test file compiles clean (the only errors in a full src/test batch compile
    were pre-existing, in other test files, from unrelated test-only deps not on my
    classpath).

What I did NOT verify locally

  • The full integration run of the new test. It needs the plugin-interface#227
    contract wired into supertokens-root together with a matching core and a live postgres.
    In CI that wiring happens automatically via same-branch-name pairing once fix: removing restriction of connection pool size for bulk import #227 is on
    the shared integration base; locally it requires standing up the whole trio, which I did
    not do. Expected CI note: until plugin-interface#227 merges into
    fix/nested-conn-acquisition-cleanup, the "Run tests" job resolves plugin-interface to
    its default branch (which lacks the new interface methods) and this repo will fail to
    compile there — that failure is the cross-repo dependency, not a defect in this diff, and
    clears once fix: removing restriction of connection pool size for bulk import #227 merges.

Cross-SDK note

supertokens-node is the reference implementation; supertokens-plugin-interface#227 is the
contract and the core-side caller (PLAN-018 Unit 3, core repo) consumes these methods. No
port to this PR is needed beyond that. Per policy, supertokens-mysql-plugin is out of scope
and the in-memory (sqlite) impl travels with the core Unit-3 ticket.

Blocked by supertokens/supertokens-plugin-interface#227

Part of PLAN-018

Fixes #413

Implement listPrimaryUsersByEmail_Transaction and listPrimaryUsersByPhoneNumber_Transaction
(PLAN-018 Unit 3): the same reads as the non-transaction forms, run on the caller's
transaction connection instead of borrowing a fresh pooled connection, to avoid nested
same-pool connection acquisition.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@supertokens-agent-runner

Copy link
Copy Markdown
Contributor Author

CI triage

Two red checks, both expected and neither a defect in this diff:

  1. Schema change requires a matching migration script — red at build, not at schema comparison.
    The job never reaches its schema/migration diff: it fails first at
    :supertokens-postgresql-plugin:compileAspectj (Compilation failed with exit code 19)
    in both the "Build with the base-branch plugin" and "Swap in the PR-head plugin" steps.
    Root cause: this PR overrides two brand-new AuthRecipeSQLStorage methods
    (listPrimaryUsersByEmail_Transaction, listPrimaryUsersByPhoneNumber_Transaction) whose
    contract lives in Add listPrimaryUsersByEmail_Transaction / listPrimaryUsersByPhoneNumber_Transaction to AuthRecipeSQLStorage supertokens-plugin-interface#227. CI resolves
    supertokens-plugin-interface to its default branch, which does not yet have those methods,
    so the plugin cannot compile there. This is the cross-repo dependency, not a schema change —
    there is no migration script, index, or manifest.json change in this diff. It clears once
    fix: removing restriction of connection pool size for bulk import #227 merges into the shared integration base (fix/nested-conn-acquisition-cleanup), at which
    point same-branch-name pairing resolves the interface with the new methods.

    Reproduced locally to confirm the cause: javac of src/main/java against the fix: removing restriction of connection pool size for bulk import #227 branch
    compiles clean; against the base without fix: removing restriction of connection pool size for bulk import #227 it fails on exactly the two @Override
    methods in Start.java (method does not override or implement a method from a supertype) —
    the only compilation gate is the interface contract.

  2. Check if "Run tests" action succeeded — structural gate, red repo-wide.
    Fails at the "Calling github API" step (exit 1), i.e. the gate that looks for a companion
    "Run tests" workflow run to check. It is not driven by this diff and reds independently of the
    change here.

Lint PR title and changelog are green.

@supertokens-agent-runner

Copy link
Copy Markdown
Contributor Author

CI triage — update after base merge-forward

Stack-sync merge-forwarded two chore: update implementationDependencies.json
commits from the base (fix/nested-conn-acquisition-cleanup) onto this branch,
re-running CI. Both checks are red for the same reasons as before, with one
clarification to my earlier note:

  • Schema change requires a matching migration script still fails at
    :supertokens-postgresql-plugin:compileAspectj (exit 19), never reaching the
    schema/migration diff. My earlier comment said it fails on "exactly the two
    @Override methods" — after the base advanced, the compile now fails on a
    broader, base-inherited set of _Transaction/activity-log overrides that
    are not in this diff (signUp_Transaction, createUser_Transaction,
    removeUserIdFromTenant_Transaction, ActivityLogSQLStorage,
    createActivityLogEntry, hasUnfoldedActivitySince, …). All fail for the
    same cross-repo reason: CI resolves supertokens-plugin-interface to its
    default branch, which lacks the interface methods the base branch already
    depends on. This clears once
    feat: add listPrimaryUsersByEmail/ByPhoneNumber _Transaction reads to AuthRecipeSQLStorage supertokens-plugin-interface#228 merges into the shared
    integration base, at which point same-branch-name pairing resolves the
    interface with the new methods. My two methods
    (listPrimaryUsersByEmail_Transaction, listPrimaryUsersByPhoneNumber_Transaction)
    compile clean against feat: OAuth provider support #228 locally (javac 21).

  • Check if "Run tests" action succeeded — unchanged structural gate, red
    repo-wide, not driven by this diff.

Lint PR title and changelog remain green. No change pushed; this PR stays a
draft blocked on plugin-interface#228.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants