Stop the import listener idling in a transaction, which ate its NOTIFYs - #318
Merged
Merged
Conversation
The enqueue listener held a plain engine.connect() for the process lifetime and probed it with SELECT 1 every 60 seconds. In the default isolation level that first probe autobegins a transaction, nothing ever ends it, and the connection sits idle in transaction forever. Same pattern, same one-line fix as the leader election. It is not only an observability problem here. Postgres delivers NOTIFY to a listening backend only while that backend is idle, so once the transaction opened the listener stopped receiving notifications entirely. From 60 seconds after startup the accelerator did nothing and every enqueued import waited out the poll interval instead of starting in milliseconds. Nothing was lost: the timed poll is the documented safety net and always picked the job up, which is why this read as "imports feel a bit slow" rather than as a broken feature. Verified directly against Postgres before and after: with the default isolation level the notification never arrives, with AUTOCOMMIT it does. The existing listener test could not catch it, because it notifies within a few seconds and the first heartbeat has not run yet. The new test drives several heartbeats first and then notifies, which fails on the old code with a timeout. That needed the heartbeat interval to become a module constant so a test can turn it down. Swept the rest of the codebase structurally for connections or sessions held across an await sleep: these two were the only ones, and the leader is already fixed.
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.
The leader-election idle-in-transaction fix had a twin, and this one was not only an observability problem.
The bug
_listen_for_enqueuesheld a plainengine.connect()for the lifetime of the process and probed it withSELECT 1every 60 seconds. In the default isolation level SQLAlchemy autobegins a transaction on the first statement, nothing ever ends it, and the heartbeat keeps outrunningidle_in_transaction_session_timeout. Identical to what the leader loop was doing, with the same consequences: it pinnedpg_stat_activity's max transaction age to the process uptime, which makes a long-transaction alert impossible to set.It also broke the feature
Postgres delivers
NOTIFYto a listening backend only while that backend is idle. A listener parked inside a transaction that never ends therefore stops receiving notifications altogether.So from 60 seconds after startup, the enqueue accelerator silently did nothing, and every queued import waited out
import_runner_interval_seconds(default 5s) instead of starting in milliseconds.Nothing was ever lost or stuck. The timed poll is the documented safety net and always picked the job up, which is exactly why this went unnoticed: the symptom was "imports feel slightly slow", not "imports are broken".
Verified directly against a real Postgres, mirroring the production code path, before changing anything:
The fix
One line, matching
leader.py: run the listener connection withisolation_level="AUTOCOMMIT". The LISTEN registration belongs to the session rather than the transaction, exactly like the leader's advisory lock, so nothing else about the listener or its reconnect path changes.Why the existing test missed it
test_enqueue_notify_wakes_the_listenernotifies within a few seconds of starting the listener, so the first heartbeat has not run and no transaction has been opened. It passes on the broken code and on the fixed code.The new test drives several heartbeats before notifying, and fails with a timeout on the old code. That is what the new
_LISTEN_HEARTBEAT_SECONDSmodule constant is for: the bug only exists after the first probe, so a test has to be able to turn the interval down.