fix(realtime): filter subscription events before they become events - #1649
Merged
Conversation
A sparse-set subscriber (ids: [watched]) was told about every other row's changes. The filter ran inside listen()'s mapping callback, which has no way to say 'do not emit' — returning null still produced an event, and the field resolvers laundered it into data with `p?.event ?? 'UNKNOWN'` and `?? false`. The throttle's 'drop' leaked identically. So the sparse set narrowed the payload but not the timing: a client learned that something it cannot see changed, and when. Filtering moves upstream of the stream. createGatedSubscriber() wraps the pgSubscriber per subscription and owns parse + throttle + sparse-set gate, so a payload that should not be delivered is simply never yielded. Every step after it is total: 'parsed' is non-null by construction, every ?? fallback is gone, and a payload emit_change and this plugin disagree about now throws MalformedNotifyPayloadError instead of arriving as an UNKNOWN-shaped record. 'subscribedIds' leaves the payload object entirely — the gate narrows rowIds, so nothing downstream re-intersects. Second bug fixed on the way: the throttle was constructed once per table at schema build time and shared by every subscriber, so one noisy client throttled everyone. It is per-subscription now, which is what its own comment always claimed. The websocket suite asserted the bug as the contract (one UNKNOWN event with a null rowId), and the unit suite's sparse-set tests re-implemented the intersection inline rather than exercising the plugin — both now drive a real stream and assert on what it yields, which is what would have caught this. graphile-realtime-test comes out of the coverage-check exclusions.
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Summary
A sparse-set subscriber —
onItemChanged(ids: [watched])— was told about every other row's changes. Not the row data, but the fact and the timing, which is the part that leaks.The cause is a sentinel that couldn't be honoured. Filtering ran inside
listen()'s mapping callback, and that callback has no way to say don't emit: returningnullstill produced an event, so the field resolvers had to invent something to put in aString!:The throttle's
dropleaked identically.nullmeant both "filtered" and "no data", and??turned both into data.The fix is to filter upstream of the stream, where "don't emit" is expressible.
createGatedSubscriber()wraps thepgSubscriberper subscription and owns parse + throttle + sparse-set gate, so a payload that shouldn't be delivered is never yielded:Everything after it is total, which is the actual point:
parsedis non-null by construction, so every??fallback is deleted rather than corrected;emit_changeand this plugin disagree about (no colon, empty, unknown op) throwsMalformedNotifyPayloadError— a divergence between trigger and plugin is a deployment fault, and'UNKNOWN'was hiding it behind data the client acts on;subscribedIdsleaves the payload object entirely: the gate narrowsrowIdsto the subscribed set, so nothing downstream re-intersects.Second bug, found on the way. The throttle was one instance per table constructed at schema build time and shared by every subscriber, so one noisy client throttled everyone on that table. It's per-subscription now — which is what its own comment always claimed (
Per-subscriber, per-table event rate tracker).Why the tests didn't catch it
They asserted the bug was the contract. The websocket suite filtered
UNKNOWNout of the received events and then required exactly one of them:And the unit suite's sparse-set tests re-implemented the intersection inline (
parsed.rowIds.some(...)in the test body) or asserted that.get('subscribedIds')had been called — never that the stream withheld anything. They now drive a real async iterable through the gate and assert on what it yields, including the burst→INVALIDATE→silence sequence and per-subscription throttle isolation.graphile-realtime-testcomes out ofEXCLUSIONSinscripts/check-test-coverage.cjsand into thepg-graphile-extrasbatch; 94/94 unit and 16/16 integration pass.Closes the first item of constructive-planning#1426.
Link to Devin session: https://app.devin.ai/sessions/087553534c774929918ec4d378845881
Requested by: @pyramation