Skip to content

Dev#697

Merged
ashwin31 merged 2 commits into
masterfrom
dev
Jun 6, 2026
Merged

Dev#697
ashwin31 merged 2 commits into
masterfrom
dev

Conversation

@ashwin31

@ashwin31 ashwin31 commented Jun 6, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed database connection leaks during server-sent events streaming.
    • Improved stream lifecycle management to ensure proper resource cleanup and termination.
  • Tests

    • Added test coverage for server-sent events stream deadline behavior.

ashwin31 added 2 commits June 6, 2026 17:22
…tream timeouts, explicit connection cleanup, and documenting production deployment safety.
Copilot AI review requested due to automatic review settings June 6, 2026 11:55
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f671f02-644e-43f3-962e-d625be3926b9

📥 Commits

Reviewing files that changed from the base of the PR and between f44fcaf and 2cca92a.

📒 Files selected for processing (2)
  • backend/common/tests/test_notification_sse.py
  • backend/common/views/notification_views.py

📝 Walkthrough

Walkthrough

The SSE notification streaming view now enforces a configurable maximum stream lifetime. A MAX_STREAM_SECONDS constant caps the stream duration, and the event loop terminates at a computed deadline. Redis client and pubsub connections are explicitly closed on cleanup. Django database connections are closed in notification fetch and at request start to prevent leaks during long-lived streams.

Changes

SSE Stream Deadline and Resource Management

Layer / File(s) Summary
Deadline configuration and event loop termination
backend/common/views/notification_views.py
time module imported and MAX_STREAM_SECONDS constant defined with documentation of bounded stream lifetime. SSE event loop computes a monotonic deadline and terminates the stream once the deadline is exceeded.
Redis connection lifecycle management
backend/common/views/notification_views.py
_aclose_redis helper closes async Redis objects across variants. _open_pubsub now returns both the Redis client and pubsub instance (or None, None on failure). Stream teardown explicitly unsubscribes and closes both pubsub and the underlying Redis client pool.
Database connection lifecycle management
backend/common/views/notification_views.py
Async notification fetch helper closes the Django DB connection after serialization completes. Request-level streaming view closes the initial DB connection before returning the long-lived StreamingHttpResponse.
Stream deadline termination test
backend/common/tests/test_notification_sse.py
Imports notification_views and adds test_stream_self_terminates_at_deadline which temporarily overrides MAX_STREAM_SECONDS to 0, verifies the initial keepalive frame, and asserts the generator self-terminates by deadline.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A stream that ran and never ceased,
Now bounded by a deadline's peace,
With Redis pooled and DB connections sealed,
The dreaded leak is finally healed,
At MAX_STREAM_SECONDS the rabbit bows. 🌙

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ashwin31 ashwin31 merged commit d960833 into master Jun 6, 2026
5 of 7 checks passed
@ashwin31 ashwin31 deleted the dev branch June 6, 2026 11:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the notifications Server-Sent Events (SSE) endpoint to better manage long-lived streaming connections under ASGI, aiming to prevent worker-thread and database/redis connection exhaustion while keeping client behavior transparent (EventSource reconnects automatically).

Changes:

  • Add a bounded SSE stream lifetime (MAX_STREAM_SECONDS) so streams self-terminate and reconnect periodically.
  • Improve redis resource cleanup by closing both the pubsub and the underlying redis client/pool for owned connections.
  • Add test coverage to ensure the stream self-terminates once the configured deadline elapses.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
backend/common/views/notification_views.py Adds stream lifetime cap and strengthens DB/redis cleanup logic for SSE streaming.
backend/common/tests/test_notification_sse.py Adds a regression test to verify the stream terminates at the configured deadline.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +156 to +164
from django.db import connection

try:
notif = Notification.objects.filter(
pk=notif_id, recipient_id=recipient_id
).first()
if notif is None:
return None
return NotificationSerializer(notif).data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants