Skip to content

fix: stop logging a failed flush's parameters and row values - #65

Merged
AlexeyShalaev merged 1 commit into
masterfrom
fix/57-flush-log-leak
Oct 16, 2026
Merged

AlexeyShalaev merged 1 commit into
masterfrom
fix/57-flush-log-leak

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

Closes #57.

Problem

When the pre-commit flush in transaction() failed, the unit of work logged str(e) at WARNING as extra={"error": ...} and re-raised. For a constraint violation that string holds row values twice: in PostgreSQL's DETAIL: Key (email)=(...) line and in SQLAlchemy's [parameters: ...]. hide_parameters=True only removes the second. This was on the default path (flush_before_commit=True), so any structured formatter that emits extra fields was writing customer data to the logs.

Change

  • The log line is gone. The flush stays, and its exception propagates exactly as before: same type, same rollback. The now-unused module logger and imports are removed. I grepped every other log site in the package, and none of them logs an exception message or row data (retry logs the attempt count, serialization logs the type name).
  • The flush_before_commit docstring and the docs/agents.md sentence no longer claim the flush surfaces errors "inside the block". It runs as the block exits, like the commit's own flush.
  • The advanced guide gets "a failed commit is the caller's to log", with a short example that logs SQLSTATE and constraint name (no row values) where the exception is handled, plus a note on str(exc) and hide_parameters. There's a new "Fixed since" row on the agents page.

How to debug a failed flush now

The exception that reaches the caller carries everything: exc.orig.sqlstate, exc.orig.__cause__.constraint_name (the asyncpg exception), and str(exc) with DETAIL and parameters. The integration test asserts the first two, so the documented recipe is proven. Log them where you handle the error, at a level you choose. Engine-level echo / sqlalchemy.engine logging still shows statements and parameters when it's enabled on purpose.

Considered and rejected

  • A DEBUG line with type, SQLSTATE and constraint name. It adds nothing the caller doesn't already hold, and every extra log site is one more place a formatter can pick fields up from.
  • Deprecating flush_before_commit. The explicit flush is redundant with the commit's, but removing a parameter is a separate decision that the leak fix doesn't need.

Who notices

Anyone who relied on the WARNING Database flush failed to see integrity errors stops seeing it, because the library no longer logs them. The same exception still reaches their handler. Nobody's control flow changes.

Evidence

New tests against the old uow/sqlalchemy.py (swapped in from origin/master): 2 failed, 1 passed. The failures are the unit test (no record at DEBUG and above, exception carries the value) and the integration test on PostgreSQL 17 with flush_before_commit=True. The False case passes on both, as in the issue.

The reporter's script after the fix:

hide_parameters=False, flush_before_commit=True
   raised to the caller: IntegrityError (SQLSTATE 23505)
hide_parameters=True, flush_before_commit=True
   raised to the caller: IntegrityError (SQLSTATE 23505)
hide_parameters=False, flush_before_commit=False
   raised to the caller: IntegrityError (SQLSTATE 23505)

Gate: make check clean; make test 628 passed.

@codecov

codecov Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@AlexeyShalaev
AlexeyShalaev merged commit 6dd88ba into master Oct 16, 2026
8 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/57-flush-log-leak branch October 16, 2026 15:25
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.

transaction() logs a failed flush's SQL parameters and DETAIL row values at WARNING (flush_before_commit=True by default)

1 participant