Skip to content

connection transactions: deactivate the ALS store and refuse nesting - #845

Merged
sroussey merged 1 commit into
claude/multi-repo-transactions-708from
claude/branch-security-review-xs0tph-libs-tx-nesting
Aug 20, 2026
Merged

connection transactions: deactivate the ALS store and refuse nesting#845
sroussey merged 1 commit into
claude/multi-repo-transactions-708from
claude/branch-security-review-xs0tph-libs-tx-nesting

Conversation

@sroussey

Copy link
Copy Markdown
Collaborator

Stacked on #842 (claude/multi-repo-transactions-708). This is PR A of a three-PR sequence; it is the blocking one for the downstream withConnectionTransaction consumers.

The CRITICAL defect: withConnectionTransaction has no nesting guard

A nested withConnectionTransaction call made from an already-enlisted owner is classified "inline" by the connection mutex (defineConnectionMutex.ts), which returns fn() with no store scope of its own. The inner runNativeConnectionTransaction therefore re-runs begin() inside the outer transaction's still-live ALS store. There is no autonomous BEGIN in SQLite or PostgreSQL, so the outer transaction boundary is corrupted — differently on each backend, and silently on two of the three:

backend what happens today
SQLite The inner BEGIN throws from outside the try in runNativeConnectionTransaction, so no ROLLBACK runs. The inner teardown's trailing .finally then clears inTransaction on the outer's participants, so every later write in the outer body silently takes its own BEGIN — outside the transaction it believes it is in.
PGlite The nested BEGIN only warns. The inner COMMIT commits the outer's work, and the outer's eventual ROLLBACK is a no-op. Data that was supposed to roll back is durable.
real pg.Pool The inner call checks out a second client and setConnectionTxQuery overwrites the shared (still-active) store's txQuery; the inner teardown then clears it. The outer's remaining writes fall through db to this.pool and autocommit on arbitrary pooled clients.

Verified end to end: the new SQLite integration test fails on the base branch with SqliteError: cannot start a transaction within a transaction, and the new PGlite one fails with expected undefined to be an instance of NestedConnectionTransactionError — i.e. the nested call raises nothing at all.

Root cause, shared with two other defects

store.run(...) is awaited by the mutex, so the ALS store is still reachable from every continuation of the transaction body and from the afterCommit callbacks — long after COMMIT ran. Every accessor treated "a store is present" as "a transaction is open", which is what makes the nesting case reachable and also causes:

  • setConnectionTxQuery(undefined) never runs. The pool branch's finally sits outside runNativeConnectionTransaction, so its continuation executes in the caller's async context where getAlsStore() is undefined and the setter early-returns. The store survived with txQuery bound to a client that had already been release()d back to the pool.
  • flushAlsDeferredPuts emits inside the still-active store. A put listener that writes in response re-entered enqueueDeferredPut, which — still active, owner still enlisted — pushed onto a fresh queue nothing drains. On SQLite that listener's write also still saw inTransaction === true (cleared only by the trailing .finally), so it skipped its own BEGIN and ran with no transaction at all.

The fix

Structural. AlsContext gains two fields:

  • activefalse once COMMIT/ROLLBACK has run. Cleared by deactivateConnectionTxStore() called from inside the ALS scope (a mutation from the caller's context would not be visible to descendants); the mutex's own finally additionally mutates the context object directly as a backstop for a body that threw before reaching the deactivation point.
  • groupHandle — the physical connection, kept separate from handle (the chain-slot key). runInTransactionOnConnection takes an optional 4th groupHandle param, defaulting to handle, so nesting detection can key on connection identity while chaining keys on the slot. runNativeConnectionTransaction gains the matching chainHandle option.

Accessor gating — the split is deliberate and documented in the module header:

accessor honors active why
connectionTxQuery yes the client is released at deactivation
isEnlistedInConnectionTx yes nothing is open to enlist in
enqueueDeferredPut yes post-commit emits must not be deferred
activeConnectionTxGroupHandle yes that is the question it answers
takeDeferredPuts no it drains the queue in that very window
discardDeferredPuts no same, on the rollback path

The guard. Every backend calls assertSharedConnectionHandle first, and before pool.connect() / BEGIN, so it is the single choke point where nesting can be refused while the outer transaction is still intact. It now throws NestedConnectionTransactionError when activeConnectionTxGroupHandle() === handle. Three properties are load-bearing:

  • keyed on groupHandle, not the chain key, so the pool path stays covered once PR B re-keys the chain onto the client;
  • identity against this connection only, so a transaction on a different database may still nest;
  • gated on active, so a sequential second transaction is not nesting.

Teardown order is now commit/rollback → deactivate → onDeactivate (new option; backends clear inTransaction here) → afterCommit/afterRollback, on every exit path including a failed BEGIN (which correctly issues no ROLLBACK). Backends keep their trailing .finally as belt-and-braces. That ordering is what makes a deferred-put listener's own write commit normally and emit its own event.

Smaller items in scope: withConnectionTransaction([]) now throws instead of silently running fn unwrapped (a dynamically-built list that came out empty asked for atomicity and would not get it); an all-best-effort list keeps its documented no-op but now logs it at debug; the provider-package seam exports in common-server.ts / browser.ts are annotated @internal with a comment above each group; the now-dead setConnectionTxQuery(undefined) is removed from the pool branch.

One fix outside the plan

assertSharedConnectionHandle's parameter declared & { readonly table?: string }. table is protected on every concrete storage, so it cannot satisfy a public structural type — all three provider packages failed build-types on the base branch with TS2345. The label is now read through the same cast the rest of the function already used. @workglow/sqlite, @workglow/postgres and @workglow/duckdb all build types clean again.

Tests

Every test below was verified in both directions — run against a deliberately degraded (pre-fix) implementation with the test files unchanged, then against the fix.

New packages/storage/src/tabular/__tests__/runNativeConnectionTransaction.test.ts (9 tests). The observers are registered as promise continuations from inside the transaction body, because an observer called from the test body carries no store and would report "not enlisted" no matter what the code does:

  • enlistment / txQuery / activeConnectionTxGroupHandle all go dead once the transaction settles
  • onDeactivate runs before afterCommit; the body's queued put is still drainable there but a new put is refused
  • deactivation on the rollback path, and on a failed BEGIN (asserting no ROLLBACK is attempted)
  • the nesting guard: refuses same-handle, permits a different connection, permits a sequential second transaction
  • isSynchronousAls() distinguishes the shim from a real AsyncLocalStorage

New packages/storage/src/tabular/__tests__/withConnectionTransaction.test.ts (2 tests): empty list rejects and never calls fn; all-best-effort resolves and logs at debug.

SqliteTabularStorage.integration.test.ts (4 new): nesting rejected with the outer's own write still committing; a write after a rejected nesting attempt still joins the outer BEGIN and still rolls back with it; a transaction on a second Sqlite.Database may nest; a put listener that writes back through the same storage persists and its own put event fires.

PostgresTabularStorage.integration.test.ts (1 new): the PGlite mirror of the nesting rejection.

# unit, after the fix
 Test Files  2 passed (2)
      Tests  12 passed (12)

# unit, against the degraded pre-fix implementation
     × stops reporting enlistment once the transaction has settled
     × clears txQuery even though the caller-context setter cannot
     × runs onDeactivate before afterCommit, and refuses to defer puts there
     × deactivates on the rollback path too
     × deactivates when BEGIN itself fails
     × refuses a second connection transaction on the same handle
     × rejects an empty participant list instead of silently running unwrapped
     × runs an all-best-effort list unwrapped, and says so at debug
      Tests  8 failed | 4 passed (12)

# SQLite integration, against the degraded pre-fix implementation
     × withConnectionTransaction refuses to nest on the same connection
       AssertionError: expected SqliteError: cannot start a transaction w… to be an instance of NestedConnectionTransactionError
     × a rejected nested transaction leaves the outer transaction able to write
     × a deferred put listener that writes commits, and its own put event fires
       AssertionError: expected [ 'in-tx' ] to include 'marker'

# PGlite integration, against the degraded pre-fix implementation
     × withConnectionTransaction refuses to nest on the same connection
       AssertionError: expected undefined to be an instance of NestedConnectionTransactionError

Full suites, after the fix (Node 24.19, bun scripts/test.ts storage unit vitest plus the integration files by name):

storage unit section:            Test Files  46 passed | 1 skipped (47)   Tests  826 passed | 14 skipped (840)
--project storage:               Test Files  22 passed | 1 skipped (23)   Tests  355 passed | 2 skipped (357)
Sqlite + DuckDb integration:     Test Files  2 passed (2)                 Tests  336 passed | 2 skipped (338)
Postgres + Scoped integration:   Test Files  3 passed (3)                 Tests  171 passed | 1 skipped (172)
build-types (storage, sqlite, postgres, duckdb):   6 successful, 6 total
eslint on every touched path:    clean

One pre-existing note: PostgresTabularStorage.integration.test.ts > shared-connection safety (PGlite path) > sibling single-op throws … exceeds the 15 s default timeout in this container. Confirmed identical on the base branch with these changes stashed — PGlite instantiation alone is slower than the timeout here; it passes at --testTimeout=120000.

Downstream impact (embarc-data #53/#55)

The withConnectionTransaction(participants, fn) signature is unchanged, but four behaviors change:

  1. withConnectionTransaction([]) now throws. Guard dynamically-built participant lists before calling.
  2. Nested withConnectionTransaction on the same handle now throws NestedConnectionTransactionError. Hoist the inner participants into the outer call (enlisted writes join the open BEGIN, so the inner call is unnecessary), or use a SAVEPOINT.
  3. storage.withTransaction() inside a connection transaction will throw on Postgres — embarc-data reverted to exactly this pattern in b038de3. (PR B, not this PR.) Ordinary put/putBulk join the transaction and need no change.
  4. Two concurrent connection transactions on one pg.Pool will run in parallel instead of serializing, so overlapping writes can hit real row-lock contention. (PR B, not this PR.)

sec / embarc-data on SQLite exercise only the single-session path. On Postgres they build a real pg.Pool, so the entire pool path is live for them and is currently untested. Findings 1, 2 and 8 (this PR) must land before #53/#55 are correct at all; findings 4, 5 and 7 (PR B) must land before either is deployed against Postgres.

Sequencing

  • PR A — this one. Shared structural change + nesting guard + empty-list throw + @internal. Backend-agnostic, fully testable on SQLite + PGlite. Blocking for downstream.
  • PR B — real pg.Pool: parallel transactions (chainHandle), isSynchronousAls() fail-fast, withTransaction-inside rejection, poisoned-client release. Depends on this PR; confined to PostgresTabularStorage.ts. The chainHandle / onDeactivate / isSynchronousAls plumbing added here is unused until then, by design — splitting it out would mean changing these same signatures twice.
  • PR C — cover the real pg.Pool path: an env-gated block plus a postgres:16 CI service. Every line of the pool branch is currently unexecuted (PGlite as unknown as Pool exposes no connect(), so serializeOps is always true, and no PG_URL-style variable exists anywhere in the repo).

Generated by Claude Code

`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
@sroussey
sroussey merged commit c87d865 into claude/multi-repo-transactions-708 Aug 20, 2026
10 of 11 checks passed
@sroussey
sroussey deleted the claude/branch-security-review-xs0tph-libs-tx-nesting branch August 24, 2026 18:50
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.

2 participants