Skip to content

fix: return event log table-check connections to pool#34024

Open
aadetya wants to merge 2 commits into
dagster-io:masterfrom
aadetya:fix/postgres-event-log-has-table
Open

fix: return event log table-check connections to pool#34024
aadetya wants to merge 2 commits into
dagster-io:masterfrom
aadetya:fix/postgres-event-log-has-table

Conversation

@aadetya

@aadetya aadetya commented Jul 20, 2026

Copy link
Copy Markdown

Summary & Motivation

Fixes #34020.

PostgresEventLogStorage.has_table() previously created a SQLAlchemy
connection inline without explicitly closing it. The connection could remain
checked out until cyclic garbage collection occurred, allowing concurrent
event-log queries to exhaust the QueuePool.

This change manages the connection with a context manager, ensuring it is
returned to the pool immediately after the table check finishes. It now uses
self._connect(), matching the normal PostgreSQL storage connection path and
retaining its retry behavior for connection acquisition.

A regression test uses a constrained QueuePool, calls has_table()
repeatedly, and verifies that no connections remain checked out. The test keeps
references to every returned Connection, so Python reference counting or
garbage collection cannot hide the original bug. This is especially important
with SQLAlchemy 1.3, where the old implementation could otherwise appear to
work.

Test Plan

  • Regression test passed with SQLAlchemy 1.3.24.
  • Regression test passed with SQLAlchemy 2.0.51.
  • Confirmed that the regression test fails with the original implementation in both versions.
  • Repeated the test 1,000 times in both repository tox environments.
  • Ruff lint, formatting, ty, and git diff --check passed.

Changelog

Fixed PostgreSQL event-log table checks to return database connections to the
pool deterministically.

@aadetya
aadetya marked this pull request as ready for review July 20, 2026 11:52
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a connection-pool leak in PostgresEventLogStorage.has_table(): the original call to self._engine.connect() returned a connection that was never explicitly closed, keeping it checked out until CPython's cyclic GC reclaimed it. Under concurrent load this could exhaust the QueuePool. The fix wraps the call in self._connect() (a context manager backed by create_pg_connection), which closes the connection deterministically in a finally block.

  • event_log.py: has_table() now uses self._connect() as a context manager, matching the pattern used by every other connection-acquiring method in the class (index_connection, run_connection, index_transaction).
  • test_event_log.py: Adds a regression test that constrains the pool to 1 connection, retains every acquired connection to prevent GC from rescuing it, and asserts pool.checkedout() == 0 after each has_table() call.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted fix that closes an open connection leak path and aligns has_table() with the established connection-management pattern used everywhere else in the class.

The change is two lines in the production code, swapping a bare engine.connect() call for the existing self._connect() context manager. The regression test is well-constructed: it uses a retained-connection strategy to prevent GC from masking the bug, a constrained pool to make leaks immediately observable, and directly asserts the pool checkout count after each call. No logic is altered beyond ensuring the connection is closed.

No files require special attention.

Important Files Changed

Filename Overview
python_modules/libraries/dagster-postgres/dagster_postgres/event_log/event_log.py Two-line fix: has_table() now routes through self._connect() context manager, matching all other connection-acquiring methods and ensuring deterministic pool return.
python_modules/libraries/dagster-postgres/dagster_postgres_tests/test_event_log.py Adds a self-contained regression test using a constrained SQLite QueuePool; retains connections via a list to prevent GC rescue, then asserts checkedout()==0 after each call.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant has_table
    participant _connect as _connect() / create_pg_connection
    participant QueuePool

    Note over has_table,QueuePool: Before fix - connection never explicitly closed
    Caller->>has_table: has_table("event_logs")
    has_table->>QueuePool: "engine.connect() [checkedout += 1]"
    has_table-->>Caller: bool result
    Note over QueuePool: connection stays checked out until GC

    Note over has_table,QueuePool: After fix - connection closed via context manager
    Caller->>has_table: has_table("event_logs")
    has_table->>_connect: with self._connect() as conn
    _connect->>QueuePool: "engine.connect() [checkedout += 1]"
    _connect-->>has_table: yield conn
    has_table-->>_connect: (exit context)
    _connect->>QueuePool: conn.close() returns checkedout to 0
    has_table-->>Caller: bool result
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant has_table
    participant _connect as _connect() / create_pg_connection
    participant QueuePool

    Note over has_table,QueuePool: Before fix - connection never explicitly closed
    Caller->>has_table: has_table("event_logs")
    has_table->>QueuePool: "engine.connect() [checkedout += 1]"
    has_table-->>Caller: bool result
    Note over QueuePool: connection stays checked out until GC

    Note over has_table,QueuePool: After fix - connection closed via context manager
    Caller->>has_table: has_table("event_logs")
    has_table->>_connect: with self._connect() as conn
    _connect->>QueuePool: "engine.connect() [checkedout += 1]"
    _connect-->>has_table: yield conn
    has_table-->>_connect: (exit context)
    _connect->>QueuePool: conn.close() returns checkedout to 0
    has_table-->>Caller: bool result
Loading

Reviews (2): Last reviewed commit: "Address event log connection review feed..." | Re-trigger Greptile

Comment thread python_modules/libraries/dagster-postgres/dagster_postgres/event_log/event_log.py Outdated
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.

PostgresEventLogStorage.has_table() does not deterministically return connections to the QueuePool

1 participant