Skip to content

fix: end each psycopg2 persister call's transaction - #950

Open
sangkyoonnam wants to merge 1 commit into
apache:mainfrom
sangkyoonnam:fix/psycopg2-transaction-boundaries
Open

sangkyoonnam wants to merge 1 commit into
apache:mainfrom
sangkyoonnam:fix/psycopg2-transaction-boundaries

Conversation

@sangkyoonnam

Copy link
Copy Markdown

PostgreSQLPersister now commits or rolls back after is_initialized, list_app_ids, load and save, so one failed insert no longer breaks the persister for the rest of the process, and reads no longer leave a transaction open.

psycopg2 runs with autocommit off, so the first statement on the connection opens a transaction. save commits on success but doesn't roll back on failure. After a rejected insert, such as a duplicate (partition_key, app_id, sequence_id, position) or a state string with a NUL character, every later load, list_app_ids and save on that persister raises InFailedSqlTransaction. On main the repro is: save a row, save it again (UniqueViolation), then load the same app. AsyncPostgreSQLPersister isn't affected, since asyncpg doesn't open a transaction unless asked to.

load, list_app_ids and is_initialized never end their transaction, so the session sits in idle in transaction until the next save commits. created_at defaults to CURRENT_TIMESTAMP, which is the transaction start time, so that save is stamped with the time of the earlier read. With two connections, a read on A, a save on B, then a save on A stores A's row with an earlier created_at than B's, and load(partition_key, None) returns B's row as the latest.

Changes

  • is_initialized, list_app_ids, load and save get their cursor from a small _transaction() context manager that commits on success and rolls back on any exception, KeyboardInterrupt included. If the rollback itself fails, usually because the connection is already gone, the original exception is the one raised. It doesn't use with self.connection:, because psycopg2 refuses to re-enter that from a caller's own with connection: block, and that pattern worked on main.
  • The class docstring now names those four methods as ending their transaction and asks for a connection of the persister's own.

How I tested this

  • Five new tests: a duplicate save raises UniqueViolation and the next load, list_app_ids and save succeed; the connection reports TRANSACTION_STATUS_IDLE right after load (hit and miss), list_app_ids and an uncached is_initialized; a save on the fixture connection after a read and another connection's save gets a created_at at or after that save's; a KeyboardInterrupt raised by the cursor mid-call leaves the connection idle; and the persister still works inside a caller's with connection: block. The first four fail on main and pass here. The status checks are the deterministic ones; the created_at test relies on the server clock not going backwards.
  • The Postgres tests ran against a local postgres:15 container with BURR_CI_INTEGRATION_TESTS=true: 16 passed.
  • pytest tests --ignore=tests/integrations/persisters/test_postgresql.py --ignore=tests/integrations/test_bip0042_bedrock.py: 680 passed, 4 skipped.
  • pre-commit run on the changed files passes.

Notes

  • Callers who shared a connection with the persister and relied on a load and a save landing in one transaction no longer get that; each call ends its own transaction, including any statements the caller had pending on that connection. Nothing in the repo does this.
  • This touches the same list_app_ids body as fix: list each app once in SQL persister list_app_ids, newest save first #942, so whichever lands second needs a rebase.
  • create_table is left as is. It already commits, and a failed CREATE TABLE would still leave the transaction aborted; that seemed out of scope for this fix.
  • I wrote this with Claude Code and reviewed and ran it myself.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added area/storage Persisters, state storage area/integrations External integrations (LLMs, frameworks) labels Oct 5, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/integrations External integrations (LLMs, frameworks) area/storage Persisters, state storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant