Skip to content

Commit c87d865

Browse files
authored
Merge pull request #845 from workglow-dev/claude/branch-security-review-xs0tph-libs-tx-nesting
connection transactions: deactivate the ALS store and refuse nesting
2 parents ff4b971 + 4b22a85 commit c87d865

15 files changed

Lines changed: 833 additions & 54 deletions

packages/storage/src/browser.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,21 @@
88

99
export * from "./common";
1010

11+
// Connection-mutex seam. Everything below except `ConnectionReentryError` is a
12+
// provider-package internal, versioned in lockstep with this package rather
13+
// than covered by semver. Application code should use
14+
// `withConnectionTransaction` instead.
1115
export {
16+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1217
__resetAlsForTesting,
1318
ConnectionReentryError,
19+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1420
getAlsStore,
21+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
22+
isSynchronousAls,
23+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1524
runInTransactionOnConnection,
25+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1626
runOnConnection,
1727
} from "./tabular/ConnectionMutex.browser";
1828

packages/storage/src/common-server.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,47 @@
88

99
export * from "./common";
1010

11+
// Connection-mutex seam. Everything below except `ConnectionReentryError` and
12+
// `NestedConnectionTransactionError` is a provider-package internal: the
13+
// vendor storages (`@workglow/sqlite`, `@workglow/postgres`,
14+
// `@workglow/duckdb`) build their `withConnectionTransaction` support on it
15+
// and are versioned in lockstep with this package. Application code should use
16+
// `withConnectionTransaction` instead; these names are not covered by semver.
1117
export {
18+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1219
__resetAlsForTesting,
1320
ConnectionReentryError,
21+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1422
getAlsStore,
23+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
24+
isSynchronousAls,
25+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1526
runInTransactionOnConnection,
27+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
1628
runOnConnection,
1729
} from "./tabular/ConnectionMutex.server";
1830

1931
export {
32+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
33+
activeConnectionTxGroupHandle,
34+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2035
assertSharedConnectionHandle,
36+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2137
connectionTxQuery,
38+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
39+
deactivateConnectionTxStore,
40+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2241
discardDeferredPuts,
42+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2343
enqueueDeferredPut,
44+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2445
isEnlistedInConnectionTx,
46+
NestedConnectionTransactionError,
47+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2548
runNativeConnectionTransaction,
49+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2650
setConnectionTxQuery,
51+
/** @internal — provider-package seam for withConnectionTransaction; not covered by semver. */
2752
takeDeferredPuts,
2853
} from "./tabular/runNativeConnectionTransaction";
2954

packages/storage/src/tabular/ConnectionMutex.browser.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,13 @@ import { defineConnectionMutex } from "./defineConnectionMutex";
99

1010
export { ConnectionReentryError } from "./defineConnectionMutex";
1111

12-
export const { runOnConnection, runInTransactionOnConnection, getAlsStore, __resetAlsForTesting } =
13-
defineConnectionMutex({
14-
ensureAls,
15-
__resetAlsForTesting: resetAlsForTesting,
16-
});
12+
export const {
13+
runOnConnection,
14+
runInTransactionOnConnection,
15+
getAlsStore,
16+
isSynchronousAls,
17+
__resetAlsForTesting,
18+
} = defineConnectionMutex({
19+
ensureAls,
20+
__resetAlsForTesting: resetAlsForTesting,
21+
});

packages/storage/src/tabular/ConnectionMutex.server.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,13 @@ import { defineConnectionMutex } from "./defineConnectionMutex";
99

1010
export { ConnectionReentryError } from "./defineConnectionMutex";
1111

12-
export const { runOnConnection, runInTransactionOnConnection, getAlsStore, __resetAlsForTesting } =
13-
defineConnectionMutex({
14-
ensureAls,
15-
__resetAlsForTesting: resetAlsForTesting,
16-
});
12+
export const {
13+
runOnConnection,
14+
runInTransactionOnConnection,
15+
getAlsStore,
16+
isSynchronousAls,
17+
__resetAlsForTesting,
18+
} = defineConnectionMutex({
19+
ensureAls,
20+
__resetAlsForTesting: resetAlsForTesting,
21+
});
Lines changed: 291 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,291 @@
1+
/**
2+
* @license
3+
* Copyright 2026 Steven Roussey <sroussey@gmail.com>
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import {
8+
__resetAlsForTesting,
9+
activeConnectionTxGroupHandle,
10+
assertSharedConnectionHandle,
11+
connectionTxQuery,
12+
enqueueDeferredPut,
13+
isEnlistedInConnectionTx,
14+
isSynchronousAls,
15+
NestedConnectionTransactionError,
16+
runNativeConnectionTransaction,
17+
setConnectionTxQuery,
18+
takeDeferredPuts,
19+
type AnyTabularStorage,
20+
type ConnectionTransactionHost,
21+
} from "@workglow/storage";
22+
import { afterEach, describe, expect, it } from "vitest";
23+
24+
/**
25+
* A participant stands in for a storage instance: `runNativeConnectionTransaction`
26+
* only ever uses it as an identity key, and `assertSharedConnectionHandle` only
27+
* reads `sharedConnectionHandle()` and the `table` label.
28+
*/
29+
function makeHost(handle: object, table: string): AnyTabularStorage & ConnectionTransactionHost {
30+
return {
31+
table,
32+
sharedConnectionHandle: () => handle,
33+
runConnectionTransaction: async <T>(
34+
_participants: readonly AnyTabularStorage[],
35+
fn: () => Promise<T>
36+
) => fn(),
37+
} as unknown as AnyTabularStorage & ConnectionTransactionHost;
38+
}
39+
40+
interface RunOptions {
41+
readonly handle: object;
42+
readonly participants: readonly AnyTabularStorage[];
43+
readonly fn: () => Promise<unknown>;
44+
readonly onDeactivate?: () => void;
45+
readonly afterCommit?: () => void;
46+
readonly afterRollback?: () => void;
47+
readonly begin?: () => void;
48+
}
49+
50+
function run(options: RunOptions): Promise<unknown> {
51+
return runNativeConnectionTransaction({
52+
handle: options.handle,
53+
participants: options.participants,
54+
begin: options.begin ?? ((): void => {}),
55+
commit: (): void => {},
56+
rollback: (): void => {},
57+
onDeactivate: options.onDeactivate,
58+
afterCommit: options.afterCommit ?? ((): void => {}),
59+
afterRollback: options.afterRollback ?? ((): void => {}),
60+
fn: options.fn,
61+
});
62+
}
63+
64+
describe("runNativeConnectionTransaction: the store is deactivated at COMMIT", () => {
65+
afterEach(() => {
66+
__resetAlsForTesting();
67+
});
68+
69+
/**
70+
* Registers a continuation from INSIDE the transaction body that runs only
71+
* after the transaction has settled. `AsyncLocalStorage` propagates through
72+
* promise continuations, so this observer still carries the store — which is
73+
* the whole point: an observer called from the test body carries none and
74+
* would report "not enlisted" no matter what the code does.
75+
*/
76+
function observeAfterSettle<T>(
77+
handle: object,
78+
owner: AnyTabularStorage & ConnectionTransactionHost,
79+
observe: () => T,
80+
begin?: () => void
81+
): Promise<T> {
82+
let release!: () => void;
83+
const settled = new Promise<void>((resolve) => {
84+
release = resolve;
85+
});
86+
let observation!: Promise<T>;
87+
return run({
88+
handle,
89+
participants: [owner],
90+
begin,
91+
fn: async () => {
92+
observation = settled.then(observe);
93+
},
94+
}).then(async () => {
95+
release();
96+
return observation;
97+
});
98+
}
99+
100+
it("stops reporting enlistment once the transaction has settled", async () => {
101+
const handle = {};
102+
const owner = makeHost(handle, "table_a");
103+
104+
const after = await observeAfterSettle(handle, owner, () => ({
105+
enlisted: isEnlistedInConnectionTx(owner),
106+
group: activeConnectionTxGroupHandle(),
107+
}));
108+
109+
expect(after.enlisted).toBe(false);
110+
expect(after.group).toBeUndefined();
111+
});
112+
113+
it("clears txQuery even though the caller-context setter cannot", async () => {
114+
const handle = {};
115+
const owner = makeHost(handle, "table_a");
116+
const client = { query: async (): Promise<unknown> => undefined };
117+
118+
const after = await observeAfterSettle(
119+
handle,
120+
owner,
121+
() => connectionTxQuery(),
122+
() => {
123+
setConnectionTxQuery(client);
124+
}
125+
);
126+
127+
expect(after).toBeUndefined();
128+
});
129+
130+
it("sees the live store from inside the body", async () => {
131+
const handle = {};
132+
const owner = makeHost(handle, "table_a");
133+
const client = { query: async (): Promise<unknown> => undefined };
134+
135+
await run({
136+
handle,
137+
participants: [owner],
138+
begin: () => setConnectionTxQuery(client),
139+
fn: async () => {
140+
expect(isEnlistedInConnectionTx(owner)).toBe(true);
141+
expect(activeConnectionTxGroupHandle()).toBe(handle);
142+
expect(connectionTxQuery()).toBe(client);
143+
},
144+
});
145+
});
146+
147+
it("runs onDeactivate before afterCommit, and refuses to defer puts there", async () => {
148+
const handle = {};
149+
const owner = makeHost(handle, "table_a");
150+
const order: string[] = [];
151+
152+
await run({
153+
handle,
154+
participants: [owner],
155+
fn: async () => {
156+
enqueueDeferredPut(owner, "during");
157+
order.push("body");
158+
},
159+
onDeactivate: () => order.push("deactivate"),
160+
afterCommit: () => {
161+
order.push("afterCommit");
162+
// The body's put is still drainable here — that is the whole point of
163+
// this window — but a NEW put must not be swallowed into a fresh queue.
164+
expect(takeDeferredPuts(owner)).toEqual(["during"]);
165+
expect(enqueueDeferredPut(owner, "listener-write")).toBe(false);
166+
},
167+
});
168+
169+
expect(order).toEqual(["body", "deactivate", "afterCommit"]);
170+
});
171+
172+
it("deactivates on the rollback path too", async () => {
173+
const handle = {};
174+
const owner = makeHost(handle, "table_a");
175+
const order: string[] = [];
176+
177+
let observe!: () => boolean;
178+
await expect(
179+
run({
180+
handle,
181+
participants: [owner],
182+
fn: async () => {
183+
observe = () => isEnlistedInConnectionTx(owner);
184+
throw new Error("boom");
185+
},
186+
onDeactivate: () => order.push("deactivate"),
187+
afterRollback: () => order.push("afterRollback"),
188+
})
189+
).rejects.toThrow("boom");
190+
191+
expect(order).toEqual(["deactivate", "afterRollback"]);
192+
expect(observe()).toBe(false);
193+
});
194+
195+
it("deactivates when BEGIN itself fails", async () => {
196+
const handle = {};
197+
const owner = makeHost(handle, "table_a");
198+
const order: string[] = [];
199+
let rolledBack = false;
200+
201+
await expect(
202+
runNativeConnectionTransaction({
203+
handle,
204+
participants: [owner],
205+
begin: () => {
206+
throw new Error("begin failed");
207+
},
208+
commit: (): void => {},
209+
rollback: () => {
210+
rolledBack = true;
211+
},
212+
onDeactivate: () => order.push("deactivate"),
213+
afterCommit: (): void => {},
214+
afterRollback: () => order.push("afterRollback"),
215+
fn: async () => undefined,
216+
})
217+
).rejects.toThrow("begin failed");
218+
219+
// No BEGIN took, so there is nothing to roll back.
220+
expect(rolledBack).toBe(false);
221+
expect(order).toEqual(["deactivate"]);
222+
});
223+
});
224+
225+
describe("assertSharedConnectionHandle: nesting guard", () => {
226+
afterEach(() => {
227+
__resetAlsForTesting();
228+
});
229+
230+
it("refuses a second connection transaction on the same handle", async () => {
231+
const handle = {};
232+
const a = makeHost(handle, "table_a");
233+
const b = makeHost(handle, "table_b");
234+
235+
let nested: unknown;
236+
await run({
237+
handle,
238+
participants: [a, b],
239+
fn: async () => {
240+
try {
241+
assertSharedConnectionHandle(a, [a, b]);
242+
} catch (err) {
243+
nested = err;
244+
}
245+
},
246+
});
247+
248+
expect(nested).toBeInstanceOf(NestedConnectionTransactionError);
249+
expect((nested as Error).message).toContain("table_a");
250+
expect((nested as Error).message).toContain("SAVEPOINT");
251+
});
252+
253+
it("allows a transaction on a different connection to nest", async () => {
254+
const outerHandle = {};
255+
const innerHandle = {};
256+
const outer = makeHost(outerHandle, "outer");
257+
const inner = makeHost(innerHandle, "inner");
258+
259+
let resolved: object | undefined;
260+
await run({
261+
handle: outerHandle,
262+
participants: [outer],
263+
fn: async () => {
264+
resolved = assertSharedConnectionHandle(inner, [inner]);
265+
},
266+
});
267+
268+
expect(resolved).toBe(innerHandle);
269+
});
270+
271+
it("allows a sequential second transaction on the same handle", async () => {
272+
const handle = {};
273+
const a = makeHost(handle, "table_a");
274+
275+
await run({ handle, participants: [a], fn: async () => undefined });
276+
expect(assertSharedConnectionHandle(a, [a])).toBe(handle);
277+
});
278+
});
279+
280+
describe("isSynchronousAls", () => {
281+
afterEach(() => {
282+
__resetAlsForTesting();
283+
});
284+
285+
it("reports the shim, and not a real AsyncLocalStorage", () => {
286+
__resetAlsForTesting(true);
287+
expect(isSynchronousAls()).toBe(true);
288+
__resetAlsForTesting();
289+
expect(isSynchronousAls()).toBe(false);
290+
});
291+
});

0 commit comments

Comments
 (0)