Skip to content

fix: prevent UNSAFE_TRANSACTION under concurrent sql.begin() - #1207

Open
dominikwagner wants to merge 1 commit into
porsager:masterfrom
essecca:clean-fix/unsafe-transaction
Open

fix: prevent UNSAFE_TRANSACTION under concurrent sql.begin()#1207
dominikwagner wants to merge 1 commit into
porsager:masterfrom
essecca:clean-fix/unsafe-transaction

Conversation

@dominikwagner

Copy link
Copy Markdown

Fixes the UNSAFE_TRANSACTION race reported in #823 (likely related to #827).

Root cause

When you call sql.begin(), the driver:

  1. Picks a connection from the pool
  2. Marks it reserved — "this connection belongs to this transaction, don't give it to anyone else"
  3. Sends BEGIN to PostgreSQL
  4. Runs your queries
  5. Sends COMMIT and un-reserves the connection

Under concurrent load, the "reserved" mark can be lost or overwritten before step 4 finishes. Another sql.begin() grabs the same connection and sends its own BEGIN. PostgreSQL sees a second BEGIN while already in a transaction, warns 25001 "there is already a transaction in progress", and the driver throws UNSAFE_TRANSACTION.

Three code paths in src/connection.js and src/index.js can produce this:

  1. execute() short-circuitonexecute (which sets connection.reserved) is at the tail of an && chain. When sent.length >= max_pipeline (or write() returns false), the chain short-circuits and onexecute is skipped, so the connection is never marked reserved.
  2. handler() pipelining — a queued BEGIN can be dispatched onto a busy connection that already holds another transaction.
  3. drain() on reserved connection — the socket drain callback can call onopen() on a connection that is currently reserved, returning it to the pool mid-transaction.

Fix

Three small, focused changes:

Where What it does
src/connection.js execute() Fire onexecute unconditionally when write() succeeds, and return false so the connection is marked full (no further pipelining onto it).
src/connection.js drain() Don't call onopen() on a reserved connection.
src/index.js handler() Never dispatch an onexecute query (BEGIN) onto a busy connection — always queue it.

Reproduction

tests/race-condition.js reproduces the bug against unmodified PostgreSQL on localhost. It runs the scenario 20 times: pool max: 3, max_pipeline: 2, hold all 3 connections busy with pg_sleep, then fire 6 regular queries + 5 sql.begin() concurrently.

  • On unfixed master: 12–16 / 20 iterations fail, PostgreSQL reports 25001 "there is already a transaction in progress"
  • On this fix: 20 / 20 iterations pass

Setup:

createuser postgres_js_test
createdb -O postgres_js_test postgres_js_test
node tests/race-condition.js

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.

1 participant