From 9ed6fc540b3b5242d0ec036bfdc7834719d418ee Mon Sep 17 00:00:00 2001 From: Jon <523261+jcfrei@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:55:51 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(db):=20release=20read=20transactions=20?= =?UTF-8?q?=E2=80=94=20no=20more=20idle-in-transaction=20on=20Postgres?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With autocommit=False every SELECT opened a transaction that nothing ended, so pooled request threads and the chat websocket idled inside transactions, pinning ACCESS SHARE locks. Lock-safe plugin migrations (ALTER TABLE) then timed out on every boot in production, columns were never added, and saves silently dropped fields for the missing columns (200 OK, edits lost). _PgConn now tracks pending writes and rolls back right after a read when no write is uncommitted and no explicit transaction (submit/cancel) is active. psycopg's client-side cursor has already buffered the rows, so fetching after the release works. Write flows and their atomicity are byte-for-byte unchanged. tests/test_txn_release.py covers the semantics plus the real failure mode: a quiet reader thread + ensure_column now succeeds. Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 5 ++ CHANGELOG.md | 17 ++++++ lambda_erp/database.py | 49 +++++++++++++++--- tests/test_txn_release.py | 105 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 tests/test_txn_release.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e7ea314..c1e1f06 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -138,3 +138,8 @@ jobs: env: LAMBDA_ERP_TEST_DB: postgresql://postgres:postgres@localhost:5432/lambda_test run: python -m tests.test_adjacent + + - name: Transaction release (no idle-in-transaction) — PostgreSQL + env: + LAMBDA_ERP_TEST_DB: postgresql://postgres:postgres@localhost:5432/lambda_test + run: python -m tests.test_txn_release diff --git a/CHANGELOG.md b/CHANGELOG.md index 19ddd32..9239ea6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,23 @@ semver-governed public surface — a breaking change to a seam is a major bump. ## [Unreleased] +## [0.6.5] - 2026-07-28 + +### Fixed +- **Postgres connections no longer sit "idle in transaction" after reads.** + With `autocommit=False`, even a plain SELECT opens a transaction, and nothing + ended it — so every pooled request thread (and the chat websocket) left an + open transaction pinning `ACCESS SHARE` locks on each table it had read. + Those idle locks starved the lock-safe plugin migrations (`ensure_column`/ + `drop_column`) of their `ACCESS EXCLUSIVE` lock on **every** boot: the ALTERs + timed out forever, columns were never added, and — because `_persist()` only + writes columns that exist — the UI's saves returned 200 while silently + dropping edits to the missing fields (observed in production). `_PgConn` now + rolls back immediately after a read when no write is pending and no explicit + transaction (submit/cancel) is active, so connections never idle inside a + transaction. Write atomicity is unchanged. Regression-tested in + `tests/test_txn_release.py`, including the migration-starvation scenario. + ## [0.6.4] - 2026-07-28 ### Added diff --git a/lambda_erp/database.py b/lambda_erp/database.py index 93f86b1..b12821b 100644 --- a/lambda_erp/database.py +++ b/lambda_erp/database.py @@ -99,10 +99,38 @@ def make(values): class _PgConn: """Adapts a psycopg (v3) connection to the sqlite3 connection API the code uses. autocommit is off so the existing `_in_transaction` + single commit() - / rollback() flow gives the same atomicity as SQLite's implicit transaction.""" - - def __init__(self, raw): + / rollback() flow gives the same atomicity as SQLite's implicit transaction. + + With autocommit off, even a plain SELECT opens a transaction that stays + open until commit/rollback — and these thread-local connections live as + long as their (pooled) thread, so a request whose last statement was a read + left the connection 'idle in transaction' forever, pinning ACCESS SHARE + locks on every table it touched. That starves any ALTER TABLE of its + ACCESS EXCLUSIVE lock, which made lock-safe plugin migrations time out on + every boot in production. The fix: after a read, when there is no pending + write and no explicit transaction, end the implicit transaction right away + (_maybe_release), so connections never idle inside one.""" + + def __init__(self, raw, in_explicit_txn=None): self._raw = raw + self._dirty = False # uncommitted write on this connection + self._in_explicit_txn = in_explicit_txn or (lambda: False) + + @staticmethod + def _is_read(sql): + s = sql.lstrip().lower() + return s.startswith("select") and "for update" not in s and "for share" not in s + + def _maybe_release(self, sql): + if self._in_explicit_txn(): + return + if self._is_read(sql): + # psycopg's default cursor has already buffered the full result set + # client-side, so the caller can still fetch after the rollback. + if not self._dirty: + self._safe_rollback() + else: + self._dirty = True def execute(self, sql, params=None): try: @@ -114,7 +142,6 @@ def execute(self, sql, params=None): cur.execute(sql.replace("%", "%%").replace("?", "%s"), list(params)) else: cur.execute(sql.replace("?", "%s")) - return cur except Exception: # A failed statement leaves the connection in an aborted transaction # (autocommit=False); every later query on this pooled/thread-local @@ -122,28 +149,34 @@ def execute(self, sql, params=None): # so the connection stays usable, then re-raise the real error. self._safe_rollback() raise + self._maybe_release(sql) + return cur def executemany(self, sql, seq_params): try: cur = self._raw.cursor() cur.executemany(sql.replace("%", "%%").replace("?", "%s"), [list(p) for p in seq_params]) - return cur except Exception: self._safe_rollback() raise + self._dirty = True + return cur def _safe_rollback(self): try: self._raw.rollback() + self._dirty = False except Exception: pass def commit(self): self._raw.commit() + self._dirty = False def rollback(self): self._raw.rollback() + self._dirty = False def close(self): self._raw.close() @@ -224,9 +257,11 @@ def _open_pg_conn(self): # autocommit off: matches SQLite's implicit transaction so the existing # _in_transaction + commit()/rollback() atomicity (GL submit/cancel) is - # preserved exactly. + # preserved exactly. in_explicit_txn lets the wrapper see the + # thread-local flag so reads inside submit/cancel keep their snapshot. return _PgConn(psycopg.connect(self.db_path, autocommit=False, - row_factory=_pg_row_factory)) + row_factory=_pg_row_factory), + in_explicit_txn=lambda: self._in_transaction) @property def conn(self): diff --git a/tests/test_txn_release.py b/tests/test_txn_release.py new file mode 100644 index 0000000..fd14fac --- /dev/null +++ b/tests/test_txn_release.py @@ -0,0 +1,105 @@ +#!/usr/bin/env python3 +"""Tests that Postgres connections never idle inside an implicit transaction. + +With autocommit off, a plain SELECT opens a transaction; before 0.6.5 nothing +ended it, so thread-local connections sat 'idle in transaction' after every +read-only request, pinning ACCESS SHARE locks that starved lock-safe plugin +migrations (ALTER TABLE) on every boot. _PgConn._maybe_release now rolls back +right after a read when no write is pending and no explicit transaction is +active. Postgres-only semantics — a no-op on SQLite. + +Run: LAMBDA_ERP_TEST_DB=postgresql://... python -m tests.test_txn_release +""" +import os +import sys + + +def main(): + url = os.environ.get("LAMBDA_ERP_TEST_DB") + if not url: + print("SKIP: transaction-release semantics are Postgres-only " + "(set LAMBDA_ERP_TEST_DB=postgresql://...)") + return + + import psycopg + from psycopg.pq import TransactionStatus + + with psycopg.connect(url, autocommit=True) as conn: + conn.execute("DROP SCHEMA public CASCADE") + conn.execute("CREATE SCHEMA public") + + from lambda_erp.database import Database + + db = Database(url) + status = lambda: db.conn._raw.info.transaction_status + + # Schema setup commits; the connection must start outside a transaction. + assert status() == TransactionStatus.IDLE, status() + + # 1. A read releases its implicit transaction immediately — and the rows + # are still fetchable (psycopg buffers the result set client-side). + rows = db.sql('SELECT key FROM "Settings"') + assert status() == TransactionStatus.IDLE, f"read left txn open: {status()}" + assert isinstance(rows, list) + + # Read helpers funnel through the same path. + db.exists("Settings", filters={"key": "no-such-key"}) + db.get_all("Settings", limit=1) + assert status() == TransactionStatus.IDLE, f"helper left txn open: {status()}" + + # 2. A write keeps its transaction open (atomicity with the later commit), + # and interleaved reads must NOT roll it back. + db.sql('INSERT INTO "Settings" (key, value) VALUES (?, ?)', ["txn-test", "1"]) + assert status() == TransactionStatus.INTRANS, f"write did not open txn: {status()}" + got = db.sql('SELECT value FROM "Settings" WHERE key = ?', ["txn-test"]) + assert status() == TransactionStatus.INTRANS, "read rolled back a pending write" + assert got and got[0]["value"] == "1" + db.conn.commit() + assert status() == TransactionStatus.IDLE + got = db.sql('SELECT value FROM "Settings" WHERE key = ?', ["txn-test"]) + assert got and got[0]["value"] == "1", "committed write lost" + + # 3. Inside an explicit transaction (submit/cancel flow) reads keep the + # transaction — and its snapshot — open. + db._in_transaction = True + try: + db.sql('SELECT 1 FROM "Settings"') + assert status() == TransactionStatus.INTRANS, "explicit txn was released" + db.conn.rollback() + finally: + db._in_transaction = False + assert status() == TransactionStatus.IDLE + + # 4. The payoff — the production failure mode: a second thread (its own + # thread-local connection) does a read and then goes quiet, exactly like + # a pooled request thread between requests. Before the fix its idle + # transaction held ACCESS SHARE on "Settings" and this ALTER timed out + # after 8 retries; now the read released the lock and the ALTER wins. + import threading + + read_done, release = threading.Event(), threading.Event() + + def quiet_reader(): + db.sql('SELECT * FROM "Settings"') + read_done.set() + release.wait(timeout=60) # keep the thread (and its connection) alive + + t = threading.Thread(target=quiet_reader, daemon=True) + t.start() + assert read_done.wait(timeout=10), "reader thread never ran" + try: + db.ensure_column("Settings", "txn_release_probe", "TEXT") + finally: + release.set() + t.join(timeout=10) + assert "txn_release_probe" in db._get_table_columns("Settings") + + print("OK: reads release their transaction; writes stay atomic") + + +if __name__ == "__main__": + try: + main() + except AssertionError as e: + print(f"FAIL: {e}") + sys.exit(1) From efb0bae9609fdb5a4ecf8104e7c101c12385884e Mon Sep 17 00:00:00 2001 From: Jon <523261+jcfrei@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:00:00 +0200 Subject: [PATCH 2/2] =?UTF-8?q?test:=20use=20Customer=20(has=20name=20colu?= =?UTF-8?q?mn)=20for=20helper=20checks=20=E2=80=94=20Settings=20is=20key/v?= =?UTF-8?q?alue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- tests/test_txn_release.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_txn_release.py b/tests/test_txn_release.py index fd14fac..c158dc4 100644 --- a/tests/test_txn_release.py +++ b/tests/test_txn_release.py @@ -42,9 +42,10 @@ def main(): assert status() == TransactionStatus.IDLE, f"read left txn open: {status()}" assert isinstance(rows, list) - # Read helpers funnel through the same path. - db.exists("Settings", filters={"key": "no-such-key"}) - db.get_all("Settings", limit=1) + # Read helpers funnel through the same path (Customer has the standard + # name/creation columns; Settings is a bare key/value table). + db.exists("Customer", "CUST-does-not-exist") + db.get_all("Customer", limit=1) assert status() == TransactionStatus.IDLE, f"helper left txn open: {status()}" # 2. A write keeps its transaction open (atomicity with the later commit),