fix(pusher): prevent unbounded audiobuffer growth#4826
Conversation
…udio apps Only accumulate audio into trigger_audiobuffer when has_audio_apps_enabled, and into audiobuffer when audio_bytes_webhook_delay_seconds is set. Without this guard, both bytearrays extend() on every audio chunk but never get cleared, growing ~16KB/s indefinitely (~57MB/hour per connection). Found during deep memory leak audit (follow-up to PR #4784).
There was a problem hiding this comment.
Code Review
This pull request aims to fix an unbounded memory growth issue in the audio buffers by adding guards to only accumulate audio data when there is an active consumer. The approach is generally correct, but a critical issue was found with the implementation for webhook audio buffers, where a 0-second delay configuration is incorrectly handled, effectively disabling the feature for that case. The review includes a suggestion to fix this logic.
| if audio_bytes_webhook_delay_seconds: | ||
| audiobuffer.extend(audio_data) |
There was a problem hiding this comment.
This check is not quite right. If audio_bytes_webhook_delay_seconds is 0, this condition will evaluate to False, and audiobuffer will never be extended. This would break the feature for users who want immediate webhook triggers with a 0-second delay.
To fix this, you should check for None explicitly.
Additionally, the corresponding condition for sending the buffer around line 430 also uses a truthiness check (if audio_bytes_webhook_delay_seconds and ...) and will fail for a 0-second delay. That should also be updated to if audio_bytes_webhook_delay_seconds is not None and ... to ensure correct behavior.
| if audio_bytes_webhook_delay_seconds: | |
| audiobuffer.extend(audio_data) | |
| if audio_bytes_webhook_delay_seconds is not None: | |
| audiobuffer.extend(audio_data) |
There was a problem hiding this comment.
Fixed in 81c10df. Changed both the audiobuffer.extend() guard (line 280) and the downstream send condition (line 430) from truthiness check to is not None:
# Guard: accumulate if webhook is configured (including delay=0)
if audio_bytes_webhook_delay_seconds is not None:
audiobuffer.extend(audio_data)
# Send: trigger when buffer exceeds threshold (works for delay=0 → immediate)
if (
audio_bytes_webhook_delay_seconds is not None
and len(audiobuffer) > sample_rate * audio_bytes_webhook_delay_seconds * 2
):
Chaos Engineering Test Results — Audiobuffer LeakTest: 30s of continuous header-101 audio chunks with
Verdict: PASS — Vulnerable grows linearly, fixed stays at zero. Reproducer: cd backend/testing/chaos-audiobuffer/
pip install fastapi uvicorn websockets
./run_chaos_test.shTest harness at |
Per Gemini review: a delay of 0 seconds means "immediate trigger", not "disabled". Truthiness check would incorrectly treat 0 as falsy, breaking the feature for users with 0-delay webhook config. Also fix the downstream send condition at line 430 for consistency.
13 tests covering all guard condition combinations: - has_audio_apps_enabled True/False x delay None/0/positive - Webhook send threshold: None never sends, 0 sends immediately - Repeated accumulation and zero-growth verification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- test_delay_zero_immediate_flush_cycle: 10 chunks arrive, each flushes immediately (threshold=0), buffer stays at 0 after each cycle - test_positive_delay_accumulates_before_flush: contrast showing delay=5 accumulates to >80KB before flushing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
lgtm |
Summary
audiobuffer.extend()andtrigger_audiobuffer.extend()— only accumulate when there's a consumertrigger_audiobufferonly extends whenhas_audio_apps_enabledaudiobufferonly extends whenaudio_bytes_webhook_delay_secondsis setPart of #4825 (Fix 1/3). Follow-up to PR #4784.
Test plan
audio_bytes_webhook_delay_secondsis set🤖 Generated with Claude Code