fix(storage): decide the join pushdown on enlistment, not the instance flag

[sroussey]

What was wrong

BaseSqlTabularStorage.planSqlJoin guards against pushing a join down while the right-hand storage sits inside a connection transaction the left side is not enlisted in. Its own comment names the pooled-Postgres case explicitly — "on a real pool it would run on a different client from the one the right side is enlisted on and miss its uncommitted rows entirely… hand this case to it." — but the test it used was:

if (right.inTransaction && !this.inTransaction) return null;

inTransaction is an instance field written only by setConnectionTransactionActive, which only runSingleSessionConnectionTransaction calls — the SQLite / DuckDB / PGlite arm. PostgresTabularStorage.runConnectionTransaction's pooled branch calls runNativeConnectionTransaction directly with ownsSession: false and flags no participant, deliberately: the transaction runs on one checked-out client while the same instances keep serving unrelated callers off the pool, so enlistment there is a property of the async context, not of the instance. withTransaction on that arm says the same thing in a comment ("We deliberately do NOT set this.inTransaction here"), and _putBulkInternal already asks the right question — this.inTransaction || isEnlistedInConnectionTx(this).

So on a real pg.Pool the guard was dead.

The concrete failure

1. Pooled Postgres, storages A and B on the same Pool.
2. withConnectionTransaction([B], async () => { await B.put(row); … }).
3. Inside the body, something calls A.join(spec, B) — A is not a participant. A join is a read, so assertNotForeignConnectionTx (write-only) raises nothing.
4. planSqlJoin: same dialect, same shared handle (both the pool), B.inTransaction === false → pushdown chosen.
5. runSqlJoin executes through A.db; connectionTxQuery(A) is undefined, so a fresh pooled client runs the JOIN.
6. row is invisible. The inner join returns zero rows where the same call on SQLite — or through the hash fallback, which goes via B.query() and resolves the enlisted client — returns it.

A wrong answer with no error, differing between backends and between the two join strategies for identical inputs.

What changed

• BaseSqlTabularStorage gains protected isEnlistedInConnectionTransaction(), defaulting to false, plus a private insideOpenTransaction() that unions it with the inTransaction flag. The base class cannot import the connection-transaction module itself — that module is instantiated once per runtime entry (NativeConnectionTransaction.server / .browser), and pulling either in would drag node:async_hooks into the browser build — so the seam is a virtual method, in the same shape as the existing setConnectionTransactionActive / sharedConnectionHandle hooks.
• PostgresTabularStorage overrides it with isEnlistedInConnectionTx(this) — the predicate its pooled write path and connectionTxQuery already use.
• The guard becomes if (right.insideOpenTransaction() && !this.insideOpenTransaction()) return null;. The inTransaction half is kept, so the tx-proxy case (where createTxView's extras sets inTransaction: true on a pooled backend whose instance flag stays false) is unchanged; the proxy binds methods to the receiver, so the union reads the override.

No behavior change on the single-session arm, where the flag is set on every participant and the union is already true.

Test

packages/test/src/test/storage-tabular/PostgresPooledJoinTransaction.test.ts builds two PostgresTabularStorage instances over a pg.Pool-shaped facade around one PGlite session. The facade exposes connect(), which is exactly how PostgresTabularStorage decides it is on the pooled arm — so runConnectionTransaction takes the pooled branch and flags no participant, reproducing the state that made the guard dead. It then enlists only the right side and joins from the un-enlisted left, asserting the hash fallback ran (the right side's query was called). A second case asserts the pushdown is still chosen when neither side is in a transaction.

Run against the unfixed source first: the first case failed (expected 0 to be greater than 0). With the fix, both pass.

What could not be exercised here

There is no live Postgres in this environment, so the test cannot demonstrate the visibility half — under one PGlite session the pushdown sees the uncommitted row anyway, and the row assertion passes on both paths. What it pins is the routing decision, made from the same pooled state a real pg.Pool produces; the "different client, invisible rows" consequence of that decision is pg.Pool behavior, not something this repo implements. The existing PostgresTabularStorage.integration.test.ts (real server, skipped without one) was not run.

Verified

Run in a clean worktree after bun install, with bun run use-dist (real dist) for the type checks:

• bunx vitest run packages/test/src/test/storage-tabular/PostgresPooledJoinTransaction.test.ts — 2 passed; 1 failed before the fix.
• bun scripts/test.ts storage unit vitest — 61 passed, 2 skipped (63 files, 1178 tests).
• bun scripts/test.ts storage unit bun — 1182 pass, 100 skip, 0 fail (same 63 files).
• bun scripts/test.ts --check-sections — every test file discoverable; bunx vitest run scripts/testDiscovery.test.ts — 11 passed.
• bunx tsc --noEmit in packages/storage, providers/postgres, packages/test — clean.
• bunx oxlint --type-aware over the three touched trees — clean.
• bunx oxfmt --check on the touched trees — clean. (packages/storage/src/vector/README.md reports a pre-existing issue on main; untouched here.)

Not verified here

• Integration suites needing a live Postgres, and the full repo test/lint/typecheck, were not run.
• DuckDB and the other single-session backends were not re-run beyond the storage unit section above; they take the default false override and their behavior is unchanged by construction.

🤖 Generated with Claude Code

https://claude.ai/code/session_01T8DU24G9GWuRUJncc5TJFZ

---

Generated by Claude Code

[Copilot]

🟢 Approval recommended

The change is narrowly scoped (default behavior preserved for non-overriding backends) and is backed by a focused regression test covering the previously unguarded pooled Postgres case.

Pull request overview

This PR fixes incorrect SQL join pushdown decisions for pooled Postgres connection transactions by basing the decision on actual enlistment in the current async context (not just an instance-level inTransaction flag), preventing joins from being executed on a different pooled client than the one holding uncommitted writes.

Changes:

• Add an overridable isEnlistedInConnectionTransaction() seam to BaseSqlTabularStorage, and use a unified insideOpenTransaction() check when deciding join pushdown eligibility.
• Implement the enlistment check for pooled Postgres via isEnlistedInConnectionTx(this) in PostgresTabularStorage.
• Add a regression test that forces the pooled-branch behavior and asserts the hash-join fallback is chosen when only the RHS is enlisted.

File summaries

File	Description
providers/postgres/src/storage/PostgresTabularStorage.ts	Overrides transaction-enlistment detection for pooled Postgres so join planning can correctly avoid unsafe pushdown.
packages/storage/src/tabular/BaseSqlTabularStorage.ts	Introduces an enlistment hook and updates join pushdown planning to use a unified “inside open tx” predicate.
packages/test/src/test/storage-tabular/PostgresPooledJoinTransaction.test.ts	Adds coverage to lock in the pooled-transaction join routing decision (pushdown vs hash fallback).

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.