Skip to content

Commit cad033c

Browse files
committed
feat(storage): real-pool path for Postgres withTransaction + reflection-based createTxView
Two related improvements to the per-instance mutex / proxy machinery introduced in 53ffa5e. createTxView (both SQLite and Postgres): Drop the hardcoded `internalNameByPublic` map. The proxy now routes any public method `foo` whose private sibling `_fooInternal` exists to that sibling, using the naming convention itself as the sync mechanism. Adding a new public method with a matching `_fooInternal` is enough; no map to keep in step. PostgresTabularStorage.withTransaction — dedicated-client path: Real `pg.Pool` (anything exposing `connect()`) now acquires its own client via `pool.connect()` and runs BEGIN/COMMIT/ROLLBACK on it, while the parent's pool stays free for external callers. The per-instance mutex is *not* acquired on this path, so external traffic runs in parallel with the open transaction — the natural Postgres concurrency model. The previous "throw on real pool" stub is gone. The PGlite / PGLitePool path keeps the mutex (single underlying session), and keeps a small `inTransaction` flag so a recursive call via the captured original instance fails fast instead of deadlocking. Per-tx state (the deferred-event queue and the `db` swap) now lives in a per-call context passed into `createTxView` rather than as fields on the parent — that's what enables external callers to keep using the pool unaffected during the transaction. Postgres _putBulkInternal nesting fix: Short-circuit our own BEGIN/COMMIT when called from inside an outer `withTransaction`. Postgres `BEGIN` inside an active transaction is a warning + no-op, and the inner `COMMIT` would commit the OUTER transaction prematurely. The proxy's `inTransaction === true` override is what triggers the short-circuit; the bulk inserts then run on the swapped `db` (the dedicated client on real pool, or the shared session on PGlite). Previously this path was unreachable because real pool threw and PGlite hit the bug silently. Tests: Add `withTransaction (dedicated-client path via fake pool)` describe block in `PostgresTabularStorage.integration.test.ts`. Wraps PGlite with a thin `connect()`-bearing adapter to exercise the new code path without spinning up real Postgres in CI. Three cases: commit, rollback, and the parallel-external-put property — verifies an external `storage.put()` resolves while `withTransaction`'s `fn` is still awaiting, proving the parent's mutex is not held. Also adds two per-tx `tx.putBulk` cases to cover the nesting fix. 767 storage tests + 161 RAG tests passing locally.
1 parent 6e43487 commit cad033c

3 files changed

Lines changed: 337 additions & 177 deletions

File tree

packages/postgres/src/storage/PostgresTabularStorage.ts

Lines changed: 139 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -683,27 +683,22 @@ export class PostgresTabularStorage<
683683
}
684684

685685
/**
686-
* Tracks whether this storage instance is currently inside a
687-
* `withTransaction` call. While true, `put`/`putBulk` route their `put`
688-
* events to {@link deferredPutEvents} instead of emitting immediately, so
689-
* listeners cannot observe rows that are about to roll back.
686+
* True while the parent instance is between `BEGIN` and `COMMIT`/`ROLLBACK`.
687+
* Used only to fail fast when `fn` captures the *original* storage instead
688+
* of the `tx` handle and tries to recursively call `withTransaction` —
689+
* that would deadlock against its own mutex on the PGlite path. Calls
690+
* routed through `tx` hit the proxy's `withTransaction` override before
691+
* reaching here.
690692
*/
691693
private inTransaction: boolean = false;
692694

693695
/**
694-
* `put` events emitted by writes inside an active `withTransaction` are
695-
* queued here and flushed after `COMMIT` succeeds (or discarded on
696-
* `ROLLBACK`).
696+
* Emits a `put` event. Overridden on the {@link createTxView} proxy to
697+
* push into a per-transaction buffer instead, so listeners never observe
698+
* rows that are about to roll back.
697699
*/
698-
private deferredPutEvents: Entity[] = [];
699-
700-
/** Emit `put` immediately, or queue if inside a withTransaction. */
701-
private emitPut(entity: Entity): void {
702-
if (this.inTransaction) {
703-
this.deferredPutEvents.push(entity);
704-
} else {
705-
this.events.emit("put", entity);
706-
}
700+
protected emitPut(entity: Entity): void {
701+
this.events.emit("put", entity);
707702
}
708703

709704
/**
@@ -750,6 +745,24 @@ export class PostgresTabularStorage<
750745
private async _putBulkInternal(entities: InsertType[]): Promise<Entity[]> {
751746
if (entities.length === 0) return [];
752747

748+
// Already inside an outer transaction (called via the `tx` view inside
749+
// `withTransaction`)? Skip our own BEGIN/COMMIT — Postgres `BEGIN`
750+
// inside an active transaction is a warning + no-op, and the inner
751+
// `COMMIT` would commit the OUTER transaction prematurely. Run the
752+
// inserts directly on `this.db`, which the proxy has already swapped to
753+
// the transaction-bound client (real pool) or the shared session
754+
// (PGlite).
755+
if (this.inTransaction) {
756+
const updated: Entity[] = [];
757+
for (const entity of entities) {
758+
const { sql, params } = this.buildPutSql(entity);
759+
const result = await this.db.query(sql, params);
760+
updated.push(this.hydrateRow(result.rows[0]));
761+
}
762+
for (const entity of updated) this.emitPut(entity);
763+
return updated;
764+
}
765+
753766
const conn = await this.acquireConnection();
754767
const updatedEntities: Entity[] = [];
755768
try {
@@ -773,52 +786,36 @@ export class PostgresTabularStorage<
773786
conn.release();
774787
}
775788

776-
for (const entity of updatedEntities) {
777-
this.emitPut(entity);
778-
}
779-
789+
for (const entity of updatedEntities) this.emitPut(entity);
780790
return updatedEntities;
781791
}
782792

783793
/**
784-
* Runs `fn` inside a `BEGIN` / `COMMIT` on `this.db`. This is only safe
785-
* when `this.db` is a single-connection wrapper (PGLitePool, raw PGlite) —
786-
* see the comment on {@link acquireConnection}. With a real `pg.Pool`,
787-
* subsequent `this.db.query` calls inside `fn` could be dispatched to
788-
* different sessions and the surrounding `BEGIN`/`COMMIT` would not bind
789-
* them together, so we throw rather than provide a false guarantee.
790-
*
791-
* `put` events emitted from inside `fn` are buffered and delivered after
792-
* `COMMIT`; if `fn` throws, they are discarded along with the rolled-back
793-
* rows.
794+
* Build a Proxy view of `this` for the `withTransaction` callback. The
795+
* proxy:
794796
*
795-
* **Concurrency contract** — see {@link ITabularStorage.withTransaction}.
796-
* Do not invoke methods on this storage instance from outside `fn` while a
797-
* `withTransaction` call is in flight; on the supported single-connection
798-
* wrappers those calls would also run through the open transaction.
799-
*/
800-
/**
801-
* Build a Proxy view of `this` that routes public-method names to their
802-
* private `_*Internal` siblings, bypassing the mutex. Handed to the
803-
* `withTransaction` callback so inner calls do not deadlock against the
804-
* mutex held by the surrounding transaction.
797+
* - Swaps `db` for the transaction-bound handle so every query inside
798+
* `fn` runs on it: the dedicated client returned by `pool.connect()`
799+
* for a real `pg.Pool`, or the shared session for PGlite/PGLitePool.
800+
* - Routes any public method `foo` whose private sibling `_fooInternal`
801+
* exists to that sibling, so calls made through `tx` bypass the
802+
* mutex (PGlite path) and do not deadlock. The naming convention is
803+
* the only sync mechanism — adding a public method with a matching
804+
* `_fooInternal` is enough; no explicit map to keep in step.
805+
* - Reports `inTransaction === true`, which is what
806+
* {@link _putBulkInternal} keys off to skip its own BEGIN/COMMIT
807+
* and run on the swapped `db` directly.
808+
* - Overrides {@link emitPut} to queue events on a per-transaction
809+
* buffer; the outer `withTransaction` flushes that buffer after
810+
* `COMMIT` (or discards on `ROLLBACK`).
811+
* - Throws on nested `withTransaction` — Postgres has no autonomous
812+
* `BEGIN`. Use SAVEPOINT directly for nested rollback boundaries.
805813
*/
806-
private createTxView(): this {
814+
private createTxView(
815+
txDb: { query: Pool["query"] },
816+
deferredPutEvents: Entity[]
817+
): this {
807818
const target = this;
808-
const internalNameByPublic: Record<string, keyof this> = {
809-
put: "_putInternal" as keyof this,
810-
putBulk: "_putBulkInternal" as keyof this,
811-
get: "_getInternal" as keyof this,
812-
delete: "_deleteInternal" as keyof this,
813-
getAll: "_getAllInternal" as keyof this,
814-
deleteAll: "_deleteAllInternal" as keyof this,
815-
size: "_sizeInternal" as keyof this,
816-
count: "_countInternal" as keyof this,
817-
getBulk: "_getBulkInternal" as keyof this,
818-
deleteSearch: "_deleteSearchInternal" as keyof this,
819-
query: "_queryInternal" as keyof this,
820-
queryIndex: "_queryIndexInternal" as keyof this,
821-
};
822819
return new Proxy(target, {
823820
get(t, prop, receiver) {
824821
if (prop === "withTransaction") {
@@ -829,71 +826,117 @@ export class PostgresTabularStorage<
829826
);
830827
};
831828
}
832-
if (typeof prop === "string" && prop in internalNameByPublic) {
833-
const internalKey = internalNameByPublic[prop];
834-
const internal = t[internalKey] as unknown;
829+
if (prop === "db") return txDb;
830+
if (prop === "inTransaction") return true;
831+
if (prop === "emitPut") {
832+
return (entity: Entity) => deferredPutEvents.push(entity);
833+
}
834+
if (typeof prop === "string") {
835+
const internal = (t as unknown as Record<string, unknown>)[`_${prop}Internal`];
835836
if (typeof internal === "function") {
836-
return (internal as (...args: unknown[]) => unknown).bind(t);
837+
return (...args: unknown[]) =>
838+
(internal as (...a: unknown[]) => unknown).apply(receiver, args);
837839
}
838840
}
839841
const value = Reflect.get(t, prop, receiver);
840-
return typeof value === "function" ? value.bind(t) : value;
842+
return typeof value === "function" ? value.bind(receiver) : value;
841843
},
842844
}) as this;
843845
}
844846

847+
/**
848+
* Runs `fn` inside a single Postgres transaction.
849+
*
850+
* **Real `pg.Pool`** — acquires a dedicated client via `pool.connect()`,
851+
* runs `BEGIN`/`COMMIT`/`ROLLBACK` on that client, and routes every
852+
* query inside `fn` through it. The parent storage instance keeps fanning
853+
* external traffic across the pool, so external callers run *in parallel*
854+
* with the open transaction (no per-instance mutex). This is the natural
855+
* Postgres concurrency model.
856+
*
857+
* **PGlite / PGLitePool** — single underlying session: the parent's mutex
858+
* is acquired for the duration of `fn` so external callers queue behind
859+
* the transaction instead of slipping into it. `BEGIN`/`COMMIT` run on
860+
* the shared `this.db`.
861+
*
862+
* `put` events emitted from inside `fn` (whether via `tx.put`,
863+
* `tx.putBulk`, or any other writer) are buffered on a per-transaction
864+
* queue and flushed to the parent's event emitter after `COMMIT`. If `fn`
865+
* throws, the buffer is discarded along with the rolled-back rows so
866+
* listeners never observe writes that did not actually commit.
867+
*
868+
* Recursive calls — either through the original instance or through `tx`
869+
* — throw rather than reusing the outer transaction implicitly. Use
870+
* SAVEPOINT directly for nested rollback boundaries.
871+
*/
845872
override async withTransaction<T>(fn: (tx: this) => Promise<T>): Promise<T> {
846873
const supportsConnect =
847874
typeof (this.db as unknown as { connect?: unknown }).connect === "function";
875+
848876
if (supportsConnect) {
877+
// Real pg.Pool: dedicate a client to this transaction; the parent's
878+
// pool stays available for external callers, who run in parallel on
879+
// other clients. We deliberately do NOT set `this.inTransaction` here
880+
// — it would make external `_putBulkInternal` calls short-circuit
881+
// their own BEGIN/COMMIT even though they are not actually nested.
882+
// Nested calls via a captured original `this` simply acquire another
883+
// pool client and run as an independent transaction; Postgres handles
884+
// the concurrency, so no nesting guard is required on this path.
885+
const client = await (
886+
this.db as unknown as {
887+
connect: () => Promise<{ query: Pool["query"]; release: () => void }>;
888+
}
889+
).connect();
890+
try {
891+
return await this.runInTransaction(fn, { query: client.query.bind(client) });
892+
} finally {
893+
client.release();
894+
}
895+
}
896+
897+
// PGlite/PGLitePool: single underlying session. Serialize against
898+
// external callers via the mutex, and set `inTransaction` so that a
899+
// nested `withTransaction` invoked via the original (rather than `tx`)
900+
// throws instead of deadlocking on its own mutex.
901+
if (this.inTransaction) {
849902
throw new Error(
850-
"PostgresTabularStorage.withTransaction is not supported on a multi-connection pg.Pool, " +
851-
"because methods on this storage dispatch each query through the pool independently. " +
852-
"Use putBulk for atomic bulk inserts, or wrap the underlying pool yourself."
903+
"PostgresTabularStorage.withTransaction does not support nesting. " +
904+
"Use SAVEPOINT directly or refactor to a single transaction."
853905
);
854906
}
855-
856907
return this.mutex(async () => {
857-
if (this.inTransaction) {
858-
throw new Error(
859-
"PostgresTabularStorage.withTransaction does not support nesting. " +
860-
"Use SAVEPOINT directly or refactor to a single transaction."
861-
);
862-
}
863-
864-
// Flag and buffer setup wrapped in try/finally so a failing BEGIN cannot
865-
// leave inTransaction stuck at true forever.
866908
this.inTransaction = true;
867-
this.deferredPutEvents = [];
868909
try {
869-
await this.db.query("BEGIN");
870-
let result: T;
871-
try {
872-
result = await fn(this.createTxView());
873-
await this.db.query("COMMIT");
874-
} catch (err) {
875-
try {
876-
await this.db.query("ROLLBACK");
877-
} catch {
878-
// prefer the original error if rollback fails
879-
}
880-
throw err;
881-
}
882-
// Flush deferred events only on commit success.
883-
const events = this.deferredPutEvents;
884-
this.deferredPutEvents = [];
885-
this.inTransaction = false;
886-
for (const entity of events) {
887-
this.events.emit("put", entity);
888-
}
889-
return result;
910+
return await this.runInTransaction(fn, { query: this.db.query.bind(this.db) });
890911
} finally {
891912
this.inTransaction = false;
892-
this.deferredPutEvents = [];
893913
}
894914
});
895915
}
896916

917+
private async runInTransaction<T>(
918+
fn: (tx: this) => Promise<T>,
919+
txDb: { query: Pool["query"] }
920+
): Promise<T> {
921+
const deferredPutEvents: Entity[] = [];
922+
await txDb.query("BEGIN");
923+
let result: T;
924+
try {
925+
result = await fn(this.createTxView(txDb, deferredPutEvents));
926+
await txDb.query("COMMIT");
927+
} catch (err) {
928+
try {
929+
await txDb.query("ROLLBACK");
930+
} catch {
931+
// prefer the original error if rollback fails
932+
}
933+
throw err;
934+
}
935+
// Flush deferred events only on commit success.
936+
for (const entity of deferredPutEvents) this.events.emit("put", entity);
937+
return result;
938+
}
939+
897940
/**
898941
* Retrieves a value from the database by its primary key.
899942
*

0 commit comments

Comments
 (0)