fix(storage): browser-safe cross-instance re-entry + actionable ConnectionReentryError - #658
Merged
sroussey merged 1 commit intoJul 24, 2026
Conversation
…ctionReentryError Two follow-ups on the ConnectionMutex introduced in the previous commit. F1 (browser ALS shim cannot detect cross-instance re-entry across await): the ALS-first check missed a legitimate cross-instance sibling call whenever the shim reset its `current` at the first `await`, so a sibling call on a different storage instance could sneak into another's BEGIN/COMMIT window on the browser bundle. Move the `state.txOwner` check ahead of `ensureAls` so the throw path is synchronous and ALS-independent; ALS survives only as an inline optimization on Node (browser shim falls back to chain-wait for same-instance re-entry, which the sanctioned tx-proxy path avoids anyway). Add a `classifyReentry(state, owner, alsStore)` helper and a `__resetAlsForTesting(useShim?)` export so the test suite can exercise both the real ALS and the shim paths. F2 (ConnectionReentryError gave no migration path): thread a `mode: "sibling-op" | "nested-transaction"` discriminator onto the error and rewrite the message to a multi-line block naming both callers, the mode, and the three supported refactors: (a) route the sibling through the `tx` proxy, (b) open a SAVEPOINT on `tx` for a nested rollback boundary, (c) collapse to a single storage instance or give each its own connection. `runOnConnection` throws with `"sibling-op"`, `runInTransactionOnConnection` with `"nested-transaction"`. Tests: new `ConnectionMutex.test.ts` covers F1 across the real-ALS and browser-shim paths (via `__resetAlsForTesting(true)`), asserting the throw path is synchronous on both and same-instance nested inlines under real ALS. Sqlite and Postgres integration tests replace the earlier "sibling blocks" contract with the new "sibling throws" contract and assert the F2 mode/message shape for both sibling-op and nested-transaction reach-throughs; the same-instance tx-proxy regression check is preserved (and added to the Postgres suite for parity). Co-Authored-By: Claude <noreply@anthropic.com>
Coverage Report
File CoverageNo changed files found. |
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.
Stacks on top of #657 — the
ConnectionMutexmodule lives on that branch.This PR addresses two HIGH-severity follow-ups found in review.
F1 — browser ALS shim cannot detect cross-instance re-entry across
awaitFinding. The original detection ran ALS-first:
On Node with real
AsyncLocalStorage, the store survives acrossawaitand this works.On the browser bundle we ship the synchronous shim, whose
run(store, cb)restorescurrentin itsfinallythe momentcb()returns (which for an asynccbisimmediately — the promise, not the resolved value). Any code that runs after the
first
awaitinside the tx body seescurrent === undefined, so a sibling callfrom a different storage instance on the same handle is silently misclassified as
"no ALS context" and falls through to a chain wait — which is precisely the
failure mode the mutex exists to prevent (it can also deadlock when the tx body
is awaiting work that depends on the sibling completing).
Approach.
classifyReentry(state, owner, alsStore, handle): "throw" | "inline" | "chain":state.txOwner !== null && state.txOwner !== owner→"throw"(regardless of ALS)"inline""chain"runOnConnection/runInTransactionOnConnectionnow checkstate.txOwner(whichlives on the handle state, not the ALS store) before awaiting
ensureAls, so thecross-instance throw path is synchronous and ALS-independent — identical on Node and
the browser shim.
AsyncLocalStorageremains only as an inline optimization on Node: when the storesurvives across
awaitand matches, a same-instance re-entry runs inline ratherthan re-taking the chain. On the browser shim, same-instance re-entry falls back
to the chain wait — safe, and the sanctioned path (
txproxy →_fooInternal)bypasses
runOnConnectionentirely anyway.__resetAlsForTesting(useShim?)— clears the cachedalsPromiseandoptionally installs a fresh shim so the test suite can exercise the browser path
on a Node test runner.
F2 —
ConnectionReentryErrorgave no migration pathFinding. The original message was one line, named the two tables, and left the
operator to guess. A dev hitting this in production has no idea whether they should
switch to the
txproxy, open aSAVEPOINT, or split their storage instances apart.Approach.
readonly mode: "sibling-op" | "nested-transaction"as the thirdconstructor arg.
runOnConnectionthrows with"sibling-op";runInTransactionOnConnectionthrows with"nested-transaction".supported refactors:
txproxy on the same instanceSAVEPOINTontxfor a nested rollback boundaryVerification
New file
packages/test/src/test/storage-tabular/ConnectionMutex.test.ts— 10 testscovering both real ALS and the shim path (via
__resetAlsForTesting(true)):ConnectionReentryError(mode="sibling-op")in under 200 ms on both platforms (no hang)
ConnectionReentryError(mode="nested-transaction")in under 200 ms on both platforms
thisinlines under real ALSowner has no
tablepropertyIntegration tests updated:
SqliteTabularStorage.integration.test.ts— the earlier "sibling blocks" testnow asserts the F1 stricter contract ("sibling throws") plus the F2 mode / message
shape; the "nested withTransaction rejects" test asserts
mode === "nested-transaction"and the migration hints. The "same-instance nested writes via tx proxy still work"
regression check is preserved.
PostgresTabularStorage.integration.test.ts— same shape, and the "same-instancenested writes via tx proxy still work" regression check is added to reach parity
with the SQLite suite.
Verified with:
All three files pass: ConnectionMutex (10/10), Sqlite integration (150 passed / 1
pre-existing skip), Postgres integration (146 passed / 1 pre-existing skip).
Behavior change to flag
The "sibling single-op blocks across a transaction on the same handle" test
asserted that a sibling put would silently wait for the outer tx to release and
then succeed. Under F1 that path now throws
ConnectionReentryError(mode="sibling-op")synchronously — the safer contract, and the whole point of the fix — so the test
is rewritten to the new contract rather than preserved. This is the only intentional
behavior break; every other pre-existing test still passes.
Generated by Claude Code