Repository navigation
fix: stop logging a failed flush's parameters and row values - #65
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AlexeyShalaev
force-pushed
the
fix/57-flush-log-leak
branch
from
October 16, 2026 15:19
f2a14a7 to
05d525b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #57.
Problem
When the pre-commit flush in
transaction()failed, the unit of work loggedstr(e)at WARNING asextra={"error": ...}and re-raised. For a constraint violation that string holds row values twice: in PostgreSQL'sDETAIL: Key (email)=(...)line and in SQLAlchemy's[parameters: ...].hide_parameters=Trueonly removes the second. This was on the default path (flush_before_commit=True), so any structured formatter that emitsextrafields was writing customer data to the logs.Change
flush_before_commitdocstring and thedocs/agents.mdsentence no longer claim the flush surfaces errors "inside the block". It runs as the block exits, like the commit's own flush.str(exc)andhide_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), andstr(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-levelecho/sqlalchemy.enginelogging still shows statements and parameters when it's enabled on purpose.Considered and rejected
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 failedto 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 fromorigin/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 withflush_before_commit=True. TheFalsecase passes on both, as in the issue.The reporter's script after the fix:
Gate:
make checkclean;make test628 passed.