fix(realtime): prevent silent Postgres connection drop on notification listener (fixes #2619) - #2841
Conversation
github-actions
Bot
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PostgreSQL connection task now awaits the connection future directly. This replaces single-message polling and keeps the notification connection active. ChangesPostgreSQL notification listener
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi maintainers, the real-time Postgres listener bug is fixed! This PR directly involves Core Backend Logic Changes / Concurrency & Reliability:
This is a core reliability/database fix that prevents the real-time infrastructure from fatally failing. As such, it satisfies the L3 criteria for ECSoC. Could you please review and apply the |
|
@Diwakar-odds it looks like this PR has some merge conflicts; mind resolving them so we can get it merged? 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/graphql-client.ts`:
- Around line 116-122: Define a single partial-response contract that preserves
both partial data and GraphQL errors, then apply it consistently: update
fetchGraphQL in src/lib/graphql-client.ts (lines 116-122) to expose the selected
signal, adapt useCursorEventsQuery in src/hooks/useCursorEventsQuery.ts (lines
111-115) and the warning handling in src/routes/admin.users.tsx (lines 131-136),
and update the corresponding expectations in src/lib/graphql-client.test.ts
(lines 56-71) and src/hooks/useCursorEventsQuery.test.ts (lines 81-111).
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e086ce80-d300-4708-8999-aa9b4e1c40b8
📒 Files selected for processing (6)
services/realtime-proxy/src/listener.rssrc/hooks/useCursorEventsQuery.test.tssrc/hooks/useCursorEventsQuery.tssrc/lib/graphql-client.test.tssrc/lib/graphql-client.tssrc/routes/admin.users.tsx
| // ── Partial failure: data exists alongside errors ───────────── | ||
| if (json.errors && json.errors.length > 0 && json.data) { | ||
| reportPartialErrors(json.errors); | ||
| // Return the partial data — callers can inspect the error via | ||
| // the thrown GraphQLPartialError if needed, but the default | ||
| // behaviour is to surface partial data gracefully. | ||
| return json.data; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define one partial-response contract and apply it to every consumer.
fetchGraphQL returns partial data at src/lib/graphql-client.ts lines 116-122. Its documentation says it throws GraphQLPartialError, and src/routes/admin.users.tsx catches that error. The route warning branch is therefore unreachable. Select one contract that preserves both partial data and GraphQL errors, then update all consumers and tests.
src/lib/graphql-client.ts#L116-L122: expose GraphQL errors with partial data through the selected contract.src/hooks/useCursorEventsQuery.ts#L111-L115: preserve cursor-query partial-data behavior under the selected contract.src/routes/admin.users.tsx#L131-L136: handle the selected partial-response signal and show the warning.src/lib/graphql-client.test.ts#L56-L71: assert the selected client contract instead of data-only resolution.src/hooks/useCursorEventsQuery.test.ts#L81-L111: assert the cursor consumer behavior for the selected contract.
📍 Affects 5 files
src/lib/graphql-client.ts#L116-L122(this comment)src/hooks/useCursorEventsQuery.ts#L111-L115src/routes/admin.users.tsx#L131-L136src/lib/graphql-client.test.ts#L56-L71src/hooks/useCursorEventsQuery.test.ts#L81-L111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/graphql-client.ts` around lines 116 - 122, Define a single
partial-response contract that preserves both partial data and GraphQL errors,
then apply it consistently: update fetchGraphQL in src/lib/graphql-client.ts
(lines 116-122) to expose the selected signal, adapt useCursorEventsQuery in
src/hooks/useCursorEventsQuery.ts (lines 111-115) and the warning handling in
src/routes/admin.users.tsx (lines 131-136), and update the corresponding
expectations in src/lib/graphql-client.test.ts (lines 56-71) and
src/hooks/useCursorEventsQuery.test.ts (lines 81-111).
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Hi @krushit1307, I noticed this PR was merged/closed but seems to be missing the |
2 participants
Summary
Fixed a severe bug where the Postgres NOTIFY listener stream would silently drop connections because the connection handler task would only poll a single message and immediately terminate.
Motivation
Closes #2619.
The connection task in
services/realtime-proxy/src/listener.rswas incorrectly implemented usingfutures_util::future::poll_fn(|cx| connection.poll_message(cx)).await. This caused the spawnedstream_taskto exit after resolving a single poll, effectively dropping the underlying Postgres connection and breaking all real-time events for the application.By replacing the single
poll_fnwith a continuous.awaiton the connection driver (connection.await), the task correctly maintains the connection lifecycle until it is explicitly aborted or an error occurs.Changes
services/realtime-proxy/src/listener.rs:connection.await) instead of polling for a single message.Acceptance Criteria
if let Err(e) = connection.await.Impact & Side Effects
The listener now robustly maintains long-lived Postgres NOTIFY connections without prematurely dropping. No breaking changes or side effects.
Summary by CodeRabbit