test(recording): add concurrent-recording saturation test - #86
Conversation
socket.io-client test that ramps N concurrent server-side recordings (each spawns an FFmpeg process + PlainTransport pair + 2 RTP ports) to find the CPU/disk/FD/port saturation point. - load-test/recording-concurrency.js — concurrent start/stop-recording; start-latency percentiles - load-test/results/RECORDING-CONCURRENCY.md — per-recording cost, saturation sampling Authored by Claude (Anthropic) via Claude Code. Not executed against a live backend — results are placeholders, not fabricated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="load-test/recording-concurrency.js">
<violation number="1" location="load-test/recording-concurrency.js:34">
P2: Invalid numeric arguments can silently produce a no-op load test that reports `0` successes, making a mistyped saturation run look valid. Validate all parsed arguments as finite values within their allowed ranges before starting the ramp.</violation>
<violation number="2" location="load-test/recording-concurrency.js:137">
P1: When multiple recorders target this room, concurrent starts can overwrite the shared SDP before another FFmpeg process reads it. Make the SDP path/session identifier unique per recording, or isolate recorders in separate rooms, before using this to measure independent recording saturation.</violation>
<violation number="3" location="load-test/recording-concurrency.js:197">
P1: After every successful stop, this script disconnects without deleting the recording. Retain `recordingId` and invoke the authenticated download/cleanup path, or add an explicit cleanup event, because each run otherwise leaves two ports reserved and an `activeRecordings` entry that contaminates later runs.</violation>
<violation number="4" location="load-test/recording-concurrency.js:220">
P1: When a start takes longer than `ACK_TIMEOUT_MS`, the unconditional disconnect can orphan the in-flight recording. Add cancellation or late-start cleanup on the server, or keep the socket alive until the start resolves and stop any recording that eventually starts.</violation>
</file>
<file name="load-test/results/RECORDING-CONCURRENCY.md">
<violation number="1" location="load-test/results/RECORDING-CONCURRENCY.md:102">
P2: The sampling command counts UDP sockets whose local port is 40000–49999, but the recording RTP ports this test is designed to exhaust are allocated from 20000–30000 (portAllocator.ts: MIN_PORT=20000, MAX_PORT=30000). Following this instruction reports ~zero matches for the recording ports and instead counts mediasoup's unrelated WebRTC rtcPorts (40000–49999), so a user watching for recording-port exhaustion sees the wrong signal. Change the default regex to `:2[0-9]{4}` and note the actual range.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| `recorder ${idx}: ${err?.message || String(err)}` | ||
| ); | ||
| } finally { | ||
| socket.disconnect(); |
There was a problem hiding this comment.
P1: When a start takes longer than ACK_TIMEOUT_MS, the unconditional disconnect can orphan the in-flight recording. Add cancellation or late-start cleanup on the server, or keep the socket alive until the start resolves and stop any recording that eventually starts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/recording-concurrency.js, line 220:
<comment>When a start takes longer than `ACK_TIMEOUT_MS`, the unconditional disconnect can orphan the in-flight recording. Add cancellation or late-start cleanup on the server, or keep the socket alive until the start resolves and stop any recording that eventually starts.</comment>
<file context>
@@ -0,0 +1,294 @@
+ `recorder ${idx}: ${err?.message || String(err)}`
+ );
+ } finally {
+ socket.disconnect();
+ }
+}
</file context>
| }; | ||
|
|
||
| socket.once("recording-started", onStarted); | ||
| socket.emit("start-recording", ROOM_ID); |
There was a problem hiding this comment.
P1: When multiple recorders target this room, concurrent starts can overwrite the shared SDP before another FFmpeg process reads it. Make the SDP path/session identifier unique per recording, or isolate recorders in separate rooms, before using this to measure independent recording saturation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/recording-concurrency.js, line 137:
<comment>When multiple recorders target this room, concurrent starts can overwrite the shared SDP before another FFmpeg process reads it. Make the SDP path/session identifier unique per recording, or isolate recorders in separate rooms, before using this to measure independent recording saturation.</comment>
<file context>
@@ -0,0 +1,294 @@
+ };
+
+ socket.once("recording-started", onStarted);
+ socket.emit("start-recording", ROOM_ID);
+ });
+}
</file context>
| } | ||
|
|
||
| // 3. START RECORDING (spawns FFmpeg + PlainTransport pair + 2 RTP ports) | ||
| const { latencyMs: startLatency } = |
There was a problem hiding this comment.
P1: After every successful stop, this script disconnects without deleting the recording. Retain recordingId and invoke the authenticated download/cleanup path, or add an explicit cleanup event, because each run otherwise leaves two ports reserved and an activeRecordings entry that contaminates later runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/recording-concurrency.js, line 197:
<comment>After every successful stop, this script disconnects without deleting the recording. Retain `recordingId` and invoke the authenticated download/cleanup path, or add an explicit cleanup event, because each run otherwise leaves two ports reserved and an `activeRecordings` entry that contaminates later runs.</comment>
<file context>
@@ -0,0 +1,294 @@
+ }
+
+ // 3. START RECORDING (spawns FFmpeg + PlainTransport pair + 2 RTP ports)
+ const { latencyMs: startLatency } =
+ await startRecordingAndWait(socket);
+
</file context>
| const URL = arg("url", "http://localhost:3000"); | ||
| const ROOM_ID = arg("room", null); | ||
| const TOKEN = arg("token", null); | ||
| const NUM_RECORDERS = parseInt(arg("recorders", "20"), 10); |
There was a problem hiding this comment.
P2: Invalid numeric arguments can silently produce a no-op load test that reports 0 successes, making a mistyped saturation run look valid. Validate all parsed arguments as finite values within their allowed ranges before starting the ramp.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/recording-concurrency.js, line 34:
<comment>Invalid numeric arguments can silently produce a no-op load test that reports `0` successes, making a mistyped saturation run look valid. Validate all parsed arguments as finite values within their allowed ranges before starting the ramp.</comment>
<file context>
@@ -0,0 +1,294 @@
+const URL = arg("url", "http://localhost:3000");
+const ROOM_ID = arg("room", null);
+const TOKEN = arg("token", null);
+const NUM_RECORDERS = parseInt(arg("recorders", "20"), 10);
+const RAMP_MS = parseInt(arg("rampMs", "10000"), 10);
+const RECORD_MS = parseInt(arg("recordMs", "30000"), 10);
</file context>
|
|
||
| ```bash | ||
| ss -u -a -n | wc -l # total UDP sockets | ||
| ss -u -a -n | grep -c ':4[0-9]{4}' # count within your RTP port range (adjust regex) |
There was a problem hiding this comment.
P2: The sampling command counts UDP sockets whose local port is 40000–49999, but the recording RTP ports this test is designed to exhaust are allocated from 20000–30000 (portAllocator.ts: MIN_PORT=20000, MAX_PORT=30000). Following this instruction reports ~zero matches for the recording ports and instead counts mediasoup's unrelated WebRTC rtcPorts (40000–49999), so a user watching for recording-port exhaustion sees the wrong signal. Change the default regex to :2[0-9]{4} and note the actual range.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/results/RECORDING-CONCURRENCY.md, line 102:
<comment>The sampling command counts UDP sockets whose local port is 40000–49999, but the recording RTP ports this test is designed to exhaust are allocated from 20000–30000 (portAllocator.ts: MIN_PORT=20000, MAX_PORT=30000). Following this instruction reports ~zero matches for the recording ports and instead counts mediasoup's unrelated WebRTC rtcPorts (40000–49999), so a user watching for recording-port exhaustion sees the wrong signal. Change the default regex to `:2[0-9]{4}` and note the actual range.</comment>
<file context>
@@ -0,0 +1,142 @@
+
+```bash
+ss -u -a -n | wc -l # total UDP sockets
+ss -u -a -n | grep -c ':4[0-9]{4}' # count within your RTP port range (adjust regex)
+```
+
</file context>
| ss -u -a -n | grep -c ':4[0-9]{4}' # count within your RTP port range (adjust regex) | |
| ss -u -a -n | grep -c ':2[0-9]{4}' # count within your RTP port range 20000-30000 (adjust regex) |
1 participant
socket.io-client test that ramps N concurrent server-side recordings (each spawns an FFmpeg process + PlainTransport pair + 2 RTP ports) to find the CPU/disk/FD/port saturation point.
Authored by Claude (Anthropic) via Claude Code. Not executed against a live backend — results are placeholders, not fabricated.
Summary by cubic
Adds a concurrent server-side recording load test, since nothing in the load-test suite covered how many simultaneous recordings one node can sustain. Each recording costs the server an FFmpeg process, a PlainTransport pair, and 2 RTP UDP ports, so the test ramps N recorders against a single live room and reports start/stop latency percentiles and timeout counts.
start-recordingvia the server'srecording-startedevent because that call has no ack.Written for commit ae9c414. Summary will update on new commits.