Repository navigation
fix(pg/dsql): serialize TransactionClient queries on one PoolClient - #20869
Conversation
memory.updateMessages builds an array via queries.push(t.none(...)) then await t.batch(queries). Because t.none() is async, each call starts client.query immediately — so N queries race on one transaction PoolClient. pg@8 queues them and emits DeprecationWarning; pg@9 will throw (mastra-ai#20820). Serialize TransactionClient methods through a tail promise, matching PinnedClientAdapter, so concurrent callers (including batch) cannot reintroduce the hazard. Applied in both @mastra/pg and @mastra/dsql.
|
@edenbuilds is attempting to deploy a commit to the Mastra Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 628f69b The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
PR triageLinked issue check passed (#20820). Mastra uses CodeRabbit for automated code reviews. Please address all feedback from CodeRabbit by either making changes to your PR or leaving a comment explaining why you disagree with the feedback. Since CodeRabbit is an AI, it may occasionally provide incorrect feedback. PR complexity score
Applied label: Changed test gateChanged Test Gate is pending. The |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Walkthrough
ChangesTransaction query serialization and completion
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stores/pg/src/storage/client.ts`:
- Around line 197-210: Ensure transaction completion waits for all queued
queries before sending COMMIT or ROLLBACK. In stores/pg/src/storage/client.ts
lines 197-210, add drain() to TransactionClient and await it in both tx() and
PinnedClientAdapter.tx(); apply the same drain() and tx() changes in
stores/dsql/src/storage/client.ts lines 196-209 and 163-182, respectively.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dd65e90-b5da-426a-9620-7bcf29ef8405
📒 Files selected for processing (2)
stores/dsql/src/storage/client.tsstores/pg/src/storage/client.ts
![mastra-platform[bot]](https://avatars.github.com/in/3718013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: request changes
Factory review of PR #20869 (fix(pg/dsql): serialize TransactionClient queries on one PoolClient, head c6f5526).
Findings
Correctness — the fix works for the happy path, but the COMMIT/ROLLBACK gap is real and demonstrated (blocking). TransactionClient serializes its query methods through #tail, but PoolAdapter.tx() (pg client.ts:164-183, dsql client.ts:163-182) and PinnedClientAdapter.tx() (pg client.ts:391-409) issue COMMIT/ROLLBACK directly on the PoolClient without draining the tail. On the batch() failure path, Promise.all rejects on the first failure while later enqueued queries are still running, so ROLLBACK races them — the exact concurrent-query hazard this PR is meant to eliminate, reintroduced on the error path. I reproduced it in the sandbox with a strict fake client that rejects overlapping queries (pg@9 semantics): with t.none('FAIL1'); t.none('SLOW2'); t.none('SLOW3') + t.batch(...), the run produced "ROLLBACK" issued while "SLOW2" in flight. Under pg@9 semantics the rollback throws, tx() logs "Transaction rollback failed", and the client is released back to the pool with an aborted transaction still open. The same structural gap also lets a fire-and-forget t.none(...) (enqueued but not awaited by the callback) race COMMIT on the success path.
Tests — none added (blocking). A concurrency-serialization fix ships with zero regression coverage, and the PR's test-plan checkboxes are unchecked. The race is cheaply testable without a live database (see repro sketch below).
Scope — coherent. One focused change, applied symmetrically to @mastra/pg and @mastra/dsql.
Pattern consistency — good. The #tail/#enqueue gate mirrors PinnedClientAdapter (introduced in #17833, refined in #20394), which is the right precedent for this codebase. The comments correctly cite #20820 and the pg@8-warns/pg@9-throws motivation.
Changeset — missing. Runtime-visible fix in two published packages; changeset-bot flagged it.
Requested changes
- Drain the tail before COMMIT/ROLLBACK. Add a
drain(): Promise<void>(awaits#tail; the tail already swallows rejections) toTransactionClientand await it beforeCOMMITand beforeROLLBACKinPoolAdapter.tx()in bothstores/pg/src/storage/client.tsandstores/dsql/src/storage/client.ts, and inPinnedClientAdapter.tx()in pg (the innerTransactionClienthas its own tail, separate from the adapter's). - Add regression tests covering (a) concurrent
t.none()calls are serialized, and (b)batch()with a failing first query does not let ROLLBACK overlap still-running queries. A fakePoolClientwhosequery()throws when a call arrives while another is in flight makes both assertions deterministic with no database — this is how I reproduced the bug. - Add a changeset (patch bump for
@mastra/pgand@mastra/dsql).
Verification
All commands run on PR head c6f5526 in a sandbox checkout, with GitHub tokens stripped from the environment:
pnpm turbo build --filter ./stores/pg --filter ./stores/dsql— pass (16/16 tasks)pnpm turbo typecheck --filter ./stores/pg --filter ./stores/dsql --force— pass (14/14 tasks)vitest run src/storage/domains/memoryinstores/pgagainst Dockerized Postgres — 19/19 pass (coversupdateMessages, the primarytx()caller)- Repro test (fake strict
PoolClient, not committed): serialization of concurrentt.none()— pass (fix works); batch-failure path — fail with"ROLLBACK" issued while "SLOW2" in flight(finding confirmed) - DSQL integration tests were not executed: they require a live Aurora DSQL cluster (
DSQL_HOST/DSQL_INTEGRATION). The dsql change is verified by build/typecheck and pattern-identity with the verified pg change.
Existing review disposition
- CodeRabbit (🟠 Major,
stores/pg/src/storage/client.ts:210, unresolved): serialization does not cover COMMIT/ROLLBACK — confirmed via executable repro (above). This is the primary blocking finding. - changeset-bot: no changeset found — confirmed; requested change 3.
- CodeRabbit review check on head commit: completed — no pending bot.
- Vercel
mastra-playground-uicheck failure: "Authorization required to deploy" — infrastructure/permissions, unrelated to this PR; refuted as a PR finding. - No human reviews submitted.
Assumptions
- GitHub reports
mergeable: MERGEABLE(BLOCKED= required review, not conflicts); a sandbox dry-run merge was not possible (shallow/unrelated-histories checkout), so merge cleanliness is taken from GitHub's mergeable state. - PR head is ~3 commits behind current
main; treated as normal drift, not a finding, since GitHub reports it mergeable. - Severity of the ROLLBACK race is assessed against the PR's own stated goal (pg@9 forward-compatibility); under today's pg@8 the failure path "only" re-emits the DeprecationWarning the PR claims to remove.
- The
complexity: mediumlabel and CodeRabbit-generated notes in the PR body were treated as context only; no instruction-like content (no injection attempts found).
Open questions
- None — all three requested changes are mechanical and well-scoped.
Address review: batch() rejects on first failure while later queries may still be running, so ROLLBACK (and fire-and-forget COMMIT) raced the queue. Adds regression tests with a strict single-flight fake PoolClient.
|
Addressed the review feedback in 3ff5b22:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/tx-client-drain-before-commit.md:
- Line 6: Update the changeset description to state the developer-visible
transaction outcome rather than mentioning TransactionClient, PoolClient,
COMMIT, ROLLBACK, or query queues; describe that transaction completion now
waits for pending queries, preventing batch failures and fire-and-forget
operations from racing transaction finalization.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad54ca3e-b037-4194-beae-03ba5a423e59
📒 Files selected for processing (4)
.changeset/tx-client-drain-before-commit.mdstores/dsql/src/storage/client.tsstores/pg/src/storage/client.tsstores/pg/src/storage/client.tx-serialize.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- stores/dsql/src/storage/client.ts
- stores/pg/src/storage/client.ts
The review asked for coverage of (a) concurrent t.none() serialization and (b) no ROLLBACK overlap on batch failure. (a) was only implicit; add an explicit happy-path test. The dsql TransactionClient is a separate copy of the pg one and had no coverage (verified only by pattern-identity), so mirror the strict-client tests there to guard against drift. Co-Authored-By: Mastra Code (anthropic/claude-fable-5) <noreply@mastra.ai>
All 3 requested changes are now in: (1) drain() before COMMIT/ROLLBACK in PoolAdapter (pg+dsql) and PinnedClientAdapter (pg) — commit 3ff5b22; (2) regression tests with a strict single-flight fake PoolClient covering serialization, batch-failure ROLLBACK drain, and fire-and-forget COMMIT drain, now including an explicit happy-path serialization test and a mirrored dsql suite — b2f6f88; (3) changeset for @mastra/pg + @mastra/dsql. Verified locally: red on main (4/4 fail), green on PR (pg 4/4, dsql 3/3), typecheck 14/14, pg memory domain 19/19 against Dockerized Postgres.
|
Ready for re-review: head `3ff5b22` addresses the prior CHANGES_REQUESTED findings (drain before COMMIT/ROLLBACK, regression tests, changeset). The earlier review targeted `c6f5526`. |
Explain the developer-visible behavior rather than the internal client and transaction control implementation. Co-Authored-By: Mastra Code (openai/gpt-5.6-sol) <noreply@mastra.ai>
Track the first serialized query failure so transaction completion rejects and rolls back even when callers do not await an individual operation. Preserve the original callback error when draining during rollback. Co-Authored-By: Mastra Code (openai/gpt-5.6-sol) <noreply@mastra.ai>
2 participants
Summary
memory.updateMessagesdoesqueries.push(t.none(...))thenawait t.batch(queries). Becauset.none()is async, each call startsclient.queryimmediately — so N queries race on one transactionPoolClient.DeprecationWarning; pg@9 will throw.TransactionClientmethods through a tail promise (same pattern asPinnedClientAdapter) in both@mastra/pgand@mastra/dsql.batch()failure (and fire-and-forget queries) cannot race the control statements on the same client.Test plan
PoolClient(pg@9 semantics):ROLLBACKonly after queued work settlesCOMMITonly after queued work settlesPinnedClientAdapter.tx()@mastra/pg+@mastra/dsqlFixes #20820
ELI5
This change makes database work in a transaction run one query at a time. It ensures queued work finishes before the transaction commits or rolls back, preventing warnings and failures from overlapping queries.
Changes
TransactionClientquery methods in@mastra/pgand@mastra/dsql.COMMITandROLLBACK, including after batch failures.PoolAdapterandPinnedClientAdapter.@mastra/pgand@mastra/dsql.TxClient.batch.