From f98bb183da94e80d3dfd5b5cd06c3d79c61b2010 Mon Sep 17 00:00:00 2001 From: SergeantPanda Date: Mon, 13 Apr 2026 19:33:22 -0500 Subject: [PATCH] Bug Fix: Fixed a provider TCP connection leak in the VOD proxy --- CHANGELOG.md | 2 ++ .../vod_proxy/multi_worker_connection_manager.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72f0a925..a746703f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fixed two race conditions in the VOD proxy that caused the `profile_connections` counter to go permanently negative, allowing connections beyond the configured profile limit. (1) `_decrement_profile_connections()` used a GET-before-DECR guard: two concurrent decrements could both read the same positive value, both pass the guard, and both fire, driving the counter below zero. Replaced with an unconditional `DECR` followed by a clamp-to-zero if the result is negative. (2) The `stream_generator` decremented `active_streams` and then checked `has_active_streams()` in two separate Redis round-trips without locking. A concurrent generator on another worker could read `active_streams=0` in the window between those two calls and also decrement the profile counter, producing a double-decrement. A new `decrement_active_streams_and_check()` method performs both operations under a single distributed lock, and a `profile_decremented` flag guards all four call sites in the generator so the profile counter is only ever decremented once per stream. (Closes #1125) — Thanks [@firestaerter3](https://github.com/firestaerter3) +- Fixed a provider TCP connection leak in the VOD proxy `stream_generator`. When a stream ended via an unhandled exception path that reached the `finally` block without any of the three exception handlers having run (e.g. an error raised before the first `yield`), the `finally` block decremented counters but never called `redis_connection.cleanup()`. The upstream `requests.Response` and `requests.Session` were left open until garbage collection. The `finally` block now starts a `delayed_cleanup` daemon thread (matching the 1-second delay used by the normal-completion and `GeneratorExit` paths) so that seeking clients have time to reconnect and increment `active_streams` before `cleanup()` checks whether it is safe to close the connection. - Fixed manual stream selection from the Stats page not enforcing M3U profile connection limits in multi-worker deployments. When a non-owning worker handled the `change_stream` request it correctly packaged `stream_id` and `m3u_profile_id` into the Redis pubsub message, but the owning worker's pubsub handler only consumed `url` and `user_agent` silently dropping both IDs before calling `stream_manager.update_url()`. Because `update_url` only calls `update_stream_profile()` when a `stream_id` is provided, the `profile_connections` counter was never updated after the switch, causing subsequent capacity checks to see incorrect counts and bypass the full-profile guard. The handler now extracts `stream_id` and `m3u_profile_id` from the event and forwards them to `update_url()`. The bug did not affect single-worker / dev-mode deployments because the owning worker handles those requests directly without pubsub. - Fixed the `next_stream` rotation endpoint applying the same class of bug: `get_stream_info_for_switch()` was called and returned `m3u_profile_id`, but the result was dropped when forwarding to `ChannelService.change_stream_url()`, so `update_stream_profile()` was never called and `profile_connections` counters were not updated after an automatic stream rotation. - Fixed stream switch metadata (`url`, `user_agent`, `stream_id`, `m3u_profile`) being written to Redis before the switch was confirmed to succeed. If the switch failed, URL unchanged or exception during teardown, Redis described a URL not actually in use. Metadata is now written only after `update_url()` returns `True`; on failure the owner writes `stream_manager.url` back as the ground truth. The non-owner no longer pre-writes metadata at all, all needed info is carried in the pubsub payload and written by the owner after confirmation. diff --git a/apps/proxy/vod_proxy/multi_worker_connection_manager.py b/apps/proxy/vod_proxy/multi_worker_connection_manager.py index 97cdcecc..073df2ca 100644 --- a/apps/proxy/vod_proxy/multi_worker_connection_manager.py +++ b/apps/proxy/vod_proxy/multi_worker_connection_manager.py @@ -1131,6 +1131,22 @@ class MultiWorkerVODConnectionManager: profile_decremented = True logger.info(f"[{client_id}] Profile counter decremented for profile {profile_id} in finally block") + # Delayed cleanup: wait 1s for seeking clients to reconnect + # before closing the provider connection and Redis keys. + # cleanup() re-checks active_streams under lock, so a + # reconnecting client that increments active_streams in + # time will prevent Redis key deletion. + def delayed_cleanup(): + time.sleep(1) + logger.info(f"[{client_id}] Worker {self.worker_id} - Checking for smart cleanup in finally block") + # No connection_manager — profile already decremented above + redis_connection.cleanup(current_worker_id=self.worker_id) + + import threading + cleanup_thread = threading.Thread(target=delayed_cleanup) + cleanup_thread.daemon = True + cleanup_thread.start() + # Create streaming response response = StreamingHttpResponse( streaming_content=stream_generator(),