Skip to content

fix(channels): make subscribe cancellation-safe to prevent subscriber leak#4872

Open
lesnik512 wants to merge 5 commits into
litestar-org:mainfrom
lesnik512:fix/channels-subscribe-cancellation-leak
Open

fix(channels): make subscribe cancellation-safe to prevent subscriber leak#4872
lesnik512 wants to merge 5 commits into
litestar-org:mainfrom
lesnik512:fix/channels-subscribe-cancellation-leak

Conversation

@lesnik512

@lesnik512 lesnik512 commented Jun 20, 2026

Copy link
Copy Markdown

Description

ChannelsPlugin.subscribe registers the new subscriber into self._channels synchronously, before it awaits backend.subscribe and put_subscriber_history. The history fetch performs real I/O (e.g. Redis xrevrange), so a cancellation there raises CancelledError without returning the subscriber — it stays registered in _channels, unreachable by the caller (which never received it) and so never unsubscribed, while the backend stays subscribed.

It surfaces with SSE / long-lived subscriptions on ASGI servers that cancel the request task when the client disconnects. A client that connects and disconnects while subscribe is still fetching history leaks a subscriber every time; under bursty connect/disconnect (reconnect storms) _channels and the backend's subscribed-channel set grow without bound — rising memory and, for the Redis stream backend, rising CPU as XREAD keeps scanning orphaned channels. Cleared only by a restart.

start_subscription inherited the flaw: it does subscriber = await self.subscribe(...) and only enters its try/finally (which unsubscribes) after subscribe returns, so a cancellation inside subscribe skipped the cleanup.

Fix

Fetch history before registering the subscriber. put_subscriber_history is the only real cancellation window here, so doing it first means a cancellation discards an unregistered subscriber instead of leaking it. start_subscription needs no separate change — subscribe now leaves nothing behind on cancel, and there is no await between subscribe returning and the try, so the context manager is safe too.

Behavior note

History is now enqueued strictly before any live event (previously live events could interleave ahead of history). The trade-off is a small window: events published between the history snapshot and backend.subscribe completing are captured by neither — comparable to the existing history-vs-live ordering races.

Tests

Added test_subscribe_cancellation_does_not_leak and test_start_subscription_cancellation_does_not_leak, using a MemoryChannelsBackend subclass whose get_history suspends (standing in for any backend whose history fetch awaits I/O). Each cancels subscribe/start_subscription mid-history and asserts no subscriber is left in _channels and the backend is not left subscribed. Both fail on main and pass with the fix; the full test_plugin.py suite remains green. No external services required.

Closes

Closes #4871


📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/4872

… leak

ChannelsPlugin.subscribe registered the new subscriber into self._channels
synchronously, before awaiting backend.subscribe and put_subscriber_history.
The history fetch performs real I/O (e.g. redis xrevrange), so a cancellation
there raised CancelledError without returning the subscriber: it stayed
registered in _channels, unreachable by the caller and never unsubscribed, and
the backend stayed subscribed. Under bursty connect/disconnect (e.g. SSE clients
that disconnect mid-subscribe) _channels grew without bound until restart.

Fetch history before registering the subscriber so a cancellation discards an
unregistered subscriber instead of leaking it. start_subscription is fixed by
the same change, since subscribe now leaves nothing behind on cancel.

Closes litestar-org#4871

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lesnik512 lesnik512 requested review from a team as code owners June 20, 2026 08:38
@codecov

codecov Bot commented Jun 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.28%. Comparing base (64aa847) to head (2cf9244).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4872   +/-   ##
=======================================
  Coverage   67.28%   67.28%           
=======================================
  Files         293      293           
  Lines       15226    15226           
  Branches     1727     1727           
=======================================
  Hits        10245    10245           
  Misses       4834     4834           
  Partials      147      147           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(not a full review)

Comment thread tests/unit/test_channels/test_plugin.py Outdated
@lesnik512 lesnik512 requested a review from sobolevn June 27, 2026 18:44
@lesnik512

Copy link
Copy Markdown
Author

@sobolevn Hi! Is it OK now?

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.

ChannelsPlugin.subscribe is not cancellation-safe: a subscriber is leaked into _channels if the request is cancelled mid-subscribe

2 participants