Add withConnectionTransaction for sibling storages on one handle. - #842
Draft
sroussey wants to merge 6 commits into
Draft
Add withConnectionTransaction for sibling storages on one handle.#842sroussey wants to merge 6 commits into
sroussey wants to merge 6 commits into
Conversation
Widen ConnectionMutex to an enlisted owner set so SQLite, Postgres, and DuckDB tabular storages can commit together, with contract coverage for commit, rollback, and non-participant sibling-op.
…ransactionHost utility Updated the runNativeConnectionTransaction.ts file to utilize the new isConnectionTransactionHost function for improved type checking of participants. This change enhances code clarity and maintainability by centralizing the logic for determining connection transaction hosts. Additionally, minor adjustments were made to the withConnectionTransaction.ts file to support this refactor.
`withConnectionTransaction` had no nesting guard. A nested call from an already-enlisted owner was classified "inline" by the connection mutex and returned `fn()` with no store scope, so the inner transaction re-ran `begin()` inside the outer's still-live store — corrupting the outer boundary differently on each backend: - SQLite: the inner BEGIN throws from outside the try, so no ROLLBACK runs, and the inner teardown clears `inTransaction` on the OUTER's participants — every later write in the outer body silently takes its own BEGIN. - PGlite: the nested BEGIN only warns, so the inner COMMIT commits the outer's work and the outer ROLLBACK no-ops. - Real pg.Pool: the inner call checks out a second client and overwrites the shared store's txQuery, and its teardown clears it — the outer's remaining writes fall through to the pool and autocommit. The root cause behind that and two related defects is that the ALS store outlives the transaction: `store.run(...)` is awaited by the mutex, so the store is still reachable from `afterCommit` and from any continuation of the body, and every accessor treated "store present" as "transaction open". Changes: - `AlsContext` gains `active` (cleared at COMMIT/ROLLBACK) and `groupHandle` (the physical connection, separate from the chain-slot key). The store is deactivated from INSIDE the ALS scope, with the mutex's own `finally` mutating the context directly as a backstop. - Accessors split on `active`: `connectionTxQuery`, `isEnlistedInConnectionTx`, `enqueueDeferredPut` and `activeConnectionTxGroupHandle` honor it; `takeDeferredPuts` / `discardDeferredPuts` deliberately do not, since they run in the deactivated window by design. - `assertSharedConnectionHandle` — the one choke point every backend reaches before checking out a client or issuing BEGIN — throws `NestedConnectionTransactionError` when a live transaction already owns this connection. Keyed on connection identity, so a transaction on a different database may still nest, and on `active`, so a sequential second transaction is not nesting. - Teardown order is now commit/rollback -> deactivate -> `onDeactivate` (backends clear `inTransaction`) -> `afterCommit`. A `put` listener that writes in response now commits normally instead of queueing onto a fresh buffer nothing drains, and on SQLite instead of running with no transaction. - `withConnectionTransaction([])` throws instead of silently running `fn` unwrapped; an all-best-effort list keeps the documented no-op and logs it. - Provider-package seam exports are annotated `@internal`; the dead `setConnectionTxQuery(undefined)` in the pool branch is removed. - `assertSharedConnectionHandle`'s parameter no longer declares a public `table` property. It is `protected` on every storage, so all three provider packages failed `build-types` against the previous signature. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SGqCtLFGeSJM2nxMbsaDkJ
…ew-xs0tph-libs-tx-nesting connection transactions: deactivate the ALS store and refuse nesting
…n chain slot (#846) The public putBulk takes the shared connection-chain slot in guardedWrite, and _putBulkInternal took it again via runOnConnection. The inner take awaits the outer holder's promise, which only resolves once the outer call returns — so the call waits on itself. Remove the inner take, restoring symmetry with SqliteTabularStorage._putBulkInternal. Introduced by 1eef8db and hidden because the branch's build-types job was broken, so every test job was skipped.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a connection-scoped transaction API to the storage layer so multiple tabular storages that share a single underlying connection (SQLite / DuckDB / single-session Postgres) can BEGIN once and commit/rollback together, while preserving cross-instance re-entry safety.
Changes:
- Introduces
withConnectionTransaction(...)plus a backend-agnosticrunNativeConnectionTransaction(...)host that coordinates one shared chain slot, ALS state, and deferredputevents. - Extends
ConnectionMutexto support an enlisted owner set (not just a single owner) so sibling storages can safely “join” the open connection transaction. - Updates SQLite/Postgres/DuckDB storages + tests/contracts to exercise commit, rollback, nesting refusal, and sibling-op behavior.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| providers/sqlite/src/storage/SqliteTabularStorage.ts | Implements connection-scoped transaction hosting for SQLite and routes writes through the shared connection chain + deferred put flushing. |
| providers/sqlite/src/storage/SqliteAiVectorStorage.ts | Aligns vector-storage writes with the new guarded write path (avoids re-taking chain slots). |
| providers/postgres/src/storage/PostgresTabularStorage.ts | Adds pool-vs-client transaction routing, connection-scoped transaction hosting, and deferred put handling for Postgres/PGlite. |
| providers/duckdb/src/storage/DuckDbTabularStorage.ts | Implements connection-scoped transaction hosting + deferred put flushing for DuckDB. |
| packages/test/src/test/storage-tabular/SqliteTabularStorage.integration.test.ts | Adds integration coverage for withConnectionTransaction semantics on SQLite. |
| packages/test/src/test/storage-tabular/PostgresTabularStorage.integration.test.ts | Adds nesting-refusal coverage for withConnectionTransaction on the shared-session Postgres test path. |
| packages/test/src/test/storage-tabular/DuckDbTabularStorage.integration.test.ts | Updates DuckDB contract setup to create sibling storages on the same handle. |
| packages/test/src/contract/tabular-storage/types.ts | Extends the contract harness with createSiblingStorage + new assertion name. |
| packages/test/src/contract/tabular-storage/runTabularStorageContract.ts | Registers the new contract assertion block. |
| packages/test/src/contract/tabular-storage/assertions/withConnectionTransaction.ts | Adds contract-level tests for commit/rollback and non-enlisted sibling-op behavior. |
| packages/storage/src/tabular/withConnectionTransaction.ts | Introduces the public free-function API + host type guard and participant validation. |
| packages/storage/src/tabular/runNativeConnectionTransaction.ts | Adds the backend-agnostic transaction host, nesting guard, and deferred put queue APIs. |
| packages/storage/src/tabular/ITabularStorage.ts | Documents withConnectionTransaction as the supported way to coordinate sibling storages. |
| packages/storage/src/tabular/defineConnectionMutex.ts | Widen ConnectionMutex from one owner to an enlisted owner set; add ALS context fields. |
| packages/storage/src/tabular/ConnectionMutex.server.ts | Re-exports additional mutex/ALS seams needed by provider storages. |
| packages/storage/src/tabular/ConnectionMutex.browser.ts | Mirrors server exports for browser builds. |
| packages/storage/src/tabular/connectionAls.shared.ts | Expands ALS context structure (owners set, groupHandle, active flag, deferred puts, txQuery). |
| packages/storage/src/tabular/BaseSqlTabularStorage.ts | Adds sharedConnectionHandle() to support participant grouping for withConnectionTransaction. |
| packages/storage/src/tabular/tests/withConnectionTransaction.test.ts | Adds unit tests for participant-list behavior (empty list, all best-effort logging). |
| packages/storage/src/tabular/tests/runNativeConnectionTransaction.test.ts | Adds unit tests for store deactivation, nesting guard, deferred put behavior, and shim detection. |
| packages/storage/src/tabular/tests/ConnectionMutex.test.ts | Adds tests for enlisted sibling-owner inlining vs non-enlisted sibling-op. |
| packages/storage/src/common.ts | Exports withConnectionTransaction from the storage public surface. |
| packages/storage/src/common-server.ts | Exposes (as internal seam) the mutex + connection-tx helpers for provider packages. |
| packages/storage/src/browser.ts | Exposes (as internal seam) the mutex helpers for browser builds. |
| packages/storage/CHANGELOG.md | Documents the new withConnectionTransaction feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+907
to
+913
| protected guardedWrite<T>(fn: () => Promise<T>): Promise<T> { | ||
| const handle = this.connectionHandle(); | ||
| if (handle !== null) { | ||
| return runOnConnection(handle, this, () => this.mutex(fn)); | ||
| } | ||
| return this.mutex(fn); | ||
| } |
Comment on lines
+931
to
+934
| return await runNativeConnectionTransaction({ | ||
| handle, | ||
| participants, | ||
| begin: async () => { |
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.
Widen ConnectionMutex to an enlisted owner set so SQLite, Postgres, and DuckDB tabular storages can commit together, with contract coverage for commit, rollback, and non-participant sibling-op.