Hold the LISTEN connection once, not once per projection - #633
Merged
Conversation
`ListenNotifyWakeup` is a single instance shared by every registered projection, and each projection's advance loop calls `wait()` on its own schedule. `_ensure_listening` had no lock, so at boot dozens of coroutines all observed `self._conn is None`, all called `pool.acquire()`, and each overwrote `self._conn` with its own connection. Only the last one is ever released by `close()`. The rest sit on `LISTEN "events"` for the lifetime of the process. Measured on the 2-BM pilot, 10 minutes after a restart: pid last query idle 4075423 LISTEN "events" 10:46 4075424 LISTEN "events" 10:46 4075427 LISTEN "events" 10:46 4075428 LISTEN "events" 10:46 4075429 LISTEN "events" 10:46 4075430 LISTEN "events" 10:46 4075413 pg_advisory_unlock_all(); CLOSE A... 00:04 ... 3 more cycling normally Six connections parked on LISTEN where the design calls for one, out of a pool whose max_size is 10. Five leaked, so half the pool was gone until the next restart, leaving four for all real work. The race is timing-dependent, so an unluckier boot can take the whole pool. The visible symptom was different and misleading: 15 logged `InterfaceError: another operation is in progress` across 10 projections, raised when a racer called `add_listener` on a connection another coroutine was mid-operation on. Those are transient, each projection backs off a second and retries, and they look like the whole story. The leak is the part that persists silently. Fixed with a double-checked lock: the fast path stays lock-free once listening, and the second check inside the lock catches the coroutines that queued behind the winner. This shipped because `ListenNotifyWakeup` had NO unit coverage at all (the test module said it "is exercised in the integration suite"), and the integration test drives a single consumer, so it cannot see a race between consumers. The connection-budget invariant does not need a real database: a fake pool that counts acquisitions pins it in the fast lane. Verified as a negative control, with the lock temporarily removed exactly the two new concurrency tests fail and the other seven pass. Verified: 30897 unit + architecture pass, the 5 real-Postgres listen/notify integration tests pass, pyright clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
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.
Found while investigating a
projection.advance.erroron the 2-BM pilot.ListenNotifyWakeupis a single instance shared by every registered projection,and each projection's advance loop calls
wait()on its own schedule._ensure_listeninghad no lock, so at boot dozens of coroutines all observedself._conn is None, all calledpool.acquire(), and each overwroteself._conn. Only the last is ever released byclose(); the rest sit onLISTEN "events"for the lifetime of the process.The visible symptom was not the problem
The log showed 15
InterfaceError: another operation is in progressacross 10projections, raised when a racer called
add_listeneron a connection anothercoroutine was mid-operation on. Those are transient: each projection backs off a
second and retries. They look like the whole story.
The leak is what persists. Measured on the pilot, 10 minutes after a restart:
Six connections parked on LISTEN where the class docstring calls for one, out of
a pool whose
max_sizeis 10. Five leaked, so half the pool was gone until thenext restart and four remained for all real work. The race is timing-dependent,
so an unluckier boot can take the whole pool.
Fix
A double-checked lock. The fast path stays lock-free once listening; the second
check inside the lock catches the coroutines that queued behind the winner.
Why it shipped
ListenNotifyWakeuphad no unit coverage at all. The test module said it "isexercised in the integration suite", and that suite drives a single consumer, so
it structurally cannot observe a race between consumers.
The connection-budget invariant needs no database: a fake pool that counts
acquisitions pins it in the fast lane. Six new tests, including the notify path
and the release-on-close path.
Verified as a negative control rather than assumed: with the lock temporarily
removed, exactly the two new concurrency tests fail and the other seven pass.
Verification
Deployment note
The pilot is still running the leaky version; it needs this merged plus a
restart to take effect there.
🤖 Generated with Claude Code