test(media): add network-impairment harness (tc netem) - #84
Open
Harxhit wants to merge 1 commit into
Open
Conversation
Bash + puppeteer harness that applies tc netem profiles (packet loss, jitter, bandwidth caps) to an interface and measures inbound-rtp getStats under each, to validate media resilience/degradation — currently zero coverage. - load-test/media-impairment.sh — root-gated netem driver; cleans up via trap - load-test/impairment-measure.js — puppeteer viewer + getStats sampler (JSON out) - load-test/results/MEDIA-NETWORK-IMPAIRMENT.md — profiles, safety, interpretation Note: simulcast/adaptStreamQuality is currently disabled server-side, so no layer adaptation is expected under impairment (documented as a finding + follow-up). Authored by Claude (Anthropic) via Claude Code. Not executed — 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}
Contributor
There was a problem hiding this comment.
9 issues found across 3 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/media-impairment.sh">
<violation number="1" location="load-test/media-impairment.sh:114">
P2: On Ctrl-C or SIGTERM during `sleep` or the measurer, cleanup runs but the loop can continue and install the next profile. Keep cleanup on `EXIT` and make `INT`/`TERM` exit immediately.</violation>
<violation number="2" location="load-test/media-impairment.sh:125">
P1: When the LIVE room is on another host, this qdisc shapes only packets leaving the measurer, not inbound RTP. Shape ingress through an IFB or the SFU's egress; otherwise the profiles measure essentially an unimpaired stream.</violation>
<violation number="3" location="load-test/media-impairment.sh:170">
P2: If `tc qdisc replace` fails, the sweep labels the subsequent measurement `loss`, `jitter`, or `rate` even though no profile was applied. Abort on a failed `apply_profile` before sampling.</violation>
<violation number="4" location="load-test/media-impairment.sh:177">
P2: When the measurer fails, the script prints `ERR` rows but exits successfully because `tail` masks Node's status. Preserve Node's exit status and return nonzero when a profile measurement fails.</violation>
</file>
<file name="load-test/impairment-measure.js">
<violation number="1" location="load-test/impairment-measure.js:108">
P1: Every run fails before opening `/viewer` because `login()` waits for `window.__csSocket`, but the application keeps its socket module-private and never exposes that global. Wait for an observable dashboard/socket-ready UI state instead, or add the missing page instrumentation.</violation>
<violation number="2" location="load-test/impairment-measure.js:167">
P2: After the socket check is fixed, each viewer still waits 40 seconds for readiness globals that the application never defines, and then silently continues with no media if either wait expires. Replace these checks with an actual join/media signal such as the video element reaching `readyState >= 2`, and treat timeout as a failed viewer rather than a successful measurement.</violation>
</file>
<file name="load-test/results/MEDIA-NETWORK-IMPAIRMENT.md">
<violation number="1" location="load-test/results/MEDIA-NETWORK-IMPAIRMENT.md:45">
P2: The `delay 100ms` netem argument adds roughly 100 ms on an outgoing leg, not 100 ms RTT. Label this profile as one-way delay, or shape both directions with the appropriate per-leg delay for a 100 ms RTT target.</violation>
<violation number="2" location="load-test/results/MEDIA-NETWORK-IMPAIRMENT.md:70">
P2: This prerequisite hard-codes the author's checkout and says the committed harness lives in `/tmp`, so it fails on other machines. Document `cd load-test && npm ci` and remove the machine-specific `NODE_PATH` guidance.</violation>
<violation number="3" location="load-test/results/MEDIA-NETWORK-IMPAIRMENT.md:75">
P2: The setup refers to `sfu-capacity.js`, but that script is absent from this repository, so operators cannot create the required LIVE room using the documented flow. Point to the application's broadcaster flow or add the referenced load-test script.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| else | ||
| # `replace` is idempotent whether or not a root qdisc already exists. | ||
| # shellcheck disable=SC2086 # intentional word-splitting of netem args | ||
| tc qdisc replace dev "$IFACE" root netem $netem |
Contributor
There was a problem hiding this comment.
P1: When the LIVE room is on another host, this qdisc shapes only packets leaving the measurer, not inbound RTP. Shape ingress through an IFB or the SFU's egress; otherwise the profiles measure essentially an unimpaired stream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/media-impairment.sh, line 125:
<comment>When the LIVE room is on another host, this qdisc shapes only packets leaving the measurer, not inbound RTP. Shape ingress through an IFB or the SFU's egress; otherwise the profiles measure essentially an unimpaired stream.</comment>
<file context>
@@ -0,0 +1,219 @@
+ else
+ # `replace` is idempotent whether or not a root qdisc already exists.
+ # shellcheck disable=SC2086 # intentional word-splitting of netem args
+ tc qdisc replace dev "$IFACE" root netem $netem
+ fi
+}
</file context>
| ); | ||
|
|
||
| await page.waitForFunction( | ||
| () => window.__csSocket && window.__csSocket.connected === true, |
Contributor
There was a problem hiding this comment.
P1: Every run fails before opening /viewer because login() waits for window.__csSocket, but the application keeps its socket module-private and never exposes that global. Wait for an observable dashboard/socket-ready UI state instead, or add the missing page instrumentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/impairment-measure.js, line 108:
<comment>Every run fails before opening `/viewer` because `login()` waits for `window.__csSocket`, but the application keeps its socket module-private and never exposes that global. Wait for an observable dashboard/socket-ready UI state instead, or add the missing page instrumentation.</comment>
<file context>
@@ -0,0 +1,347 @@
+ );
+
+ await page.waitForFunction(
+ () => window.__csSocket && window.__csSocket.connected === true,
+ { timeout: WAIT_TIMEOUT_MS }
+ );
</file context>
| cleanup() { | ||
| tc qdisc del dev "$IFACE" root 2>/dev/null || true | ||
| } | ||
| trap cleanup EXIT INT TERM |
Contributor
There was a problem hiding this comment.
P2: On Ctrl-C or SIGTERM during sleep or the measurer, cleanup runs but the loop can continue and install the next profile. Keep cleanup on EXIT and make INT/TERM exit immediately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/media-impairment.sh, line 114:
<comment>On Ctrl-C or SIGTERM during `sleep` or the measurer, cleanup runs but the loop can continue and install the next profile. Keep cleanup on `EXIT` and make `INT`/`TERM` exit immediately.</comment>
<file context>
@@ -0,0 +1,219 @@
+cleanup() {
+ tc qdisc del dev "$IFACE" root 2>/dev/null || true
+}
+trap cleanup EXIT INT TERM
+
+apply_profile() {
</file context>
Suggested change
| trap cleanup EXIT INT TERM | |
| trap cleanup EXIT | |
| trap 'exit 130' INT | |
| trap 'exit 143' TERM |
|
|
||
| # Run the measurer. Diagnostics -> stderr (dropped here); the pure JSON | ||
| # summary is the only stdout line, so tail -n 1 grabs it reliably. | ||
| json="$(node "$MEASURE" \ |
Contributor
There was a problem hiding this comment.
P2: When the measurer fails, the script prints ERR rows but exits successfully because tail masks Node's status. Preserve Node's exit status and return nonzero when a profile measurement fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/media-impairment.sh, line 177:
<comment>When the measurer fails, the script prints `ERR` rows but exits successfully because `tail` masks Node's status. Preserve Node's exit status and return nonzero when a profile measurement fails.</comment>
<file context>
@@ -0,0 +1,219 @@
+
+ # Run the measurer. Diagnostics -> stderr (dropped here); the pure JSON
+ # summary is the only stdout line, so tail -n 1 grabs it reliably.
+ json="$(node "$MEASURE" \
+ --baseUrl "$BASE_URL" \
+ --roomId "$ROOM_ID" \
</file context>
| echo " netem: $netem" | ||
| fi | ||
|
|
||
| apply_profile "$netem" |
Contributor
There was a problem hiding this comment.
P2: If tc qdisc replace fails, the sweep labels the subsequent measurement loss, jitter, or rate even though no profile was applied. Abort on a failed apply_profile before sampling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/media-impairment.sh, line 170:
<comment>If `tc qdisc replace` fails, the sweep labels the subsequent measurement `loss`, `jitter`, or `rate` even though no profile was applied. Abort on a failed `apply_profile` before sampling.</comment>
<file context>
@@ -0,0 +1,219 @@
+ echo " netem: $netem"
+ fi
+
+ apply_profile "$netem"
+
+ # Let the shaping take effect before measuring.
</file context>
Suggested change
| apply_profile "$netem" | |
| apply_profile "$netem" || exit 1 |
| await page.$eval("form", (form) => form.requestSubmit()); | ||
|
|
||
| try { | ||
| await page.waitForFunction(() => Boolean(window.__csJoinedAt), { |
Contributor
There was a problem hiding this comment.
P2: After the socket check is fixed, each viewer still waits 40 seconds for readiness globals that the application never defines, and then silently continues with no media if either wait expires. Replace these checks with an actual join/media signal such as the video element reaching readyState >= 2, and treat timeout as a failed viewer rather than a successful measurement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/impairment-measure.js, line 167:
<comment>After the socket check is fixed, each viewer still waits 40 seconds for readiness globals that the application never defines, and then silently continues with no media if either wait expires. Replace these checks with an actual join/media signal such as the video element reaching `readyState >= 2`, and treat timeout as a failed viewer rather than a successful measurement.</comment>
<file context>
@@ -0,0 +1,347 @@
+ await page.$eval("form", (form) => form.requestSubmit());
+
+ try {
+ await page.waitForFunction(() => Boolean(window.__csJoinedAt), {
+ timeout: WAIT_TIMEOUT_MS,
+ });
</file context>
| | loss 1% | `loss 1%` | Mild packet loss | | ||
| | loss 3% | `loss 3%` | Moderate packet loss | | ||
| | loss 5% | `loss 5%` | Heavy packet loss | | ||
| | jitter | `delay 100ms 20ms distribution normal` | 100 ms RTT ± 20 ms normal jitter | |
Contributor
There was a problem hiding this comment.
P2: The delay 100ms netem argument adds roughly 100 ms on an outgoing leg, not 100 ms RTT. Label this profile as one-way delay, or shape both directions with the appropriate per-leg delay for a 100 ms RTT target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/results/MEDIA-NETWORK-IMPAIRMENT.md, line 45:
<comment>The `delay 100ms` netem argument adds roughly 100 ms on an outgoing leg, not 100 ms RTT. Label this profile as one-way delay, or shape both directions with the appropriate per-leg delay for a 100 ms RTT target.</comment>
<file context>
@@ -0,0 +1,147 @@
+| loss 1% | `loss 1%` | Mild packet loss |
+| loss 3% | `loss 3%` | Moderate packet loss |
+| loss 5% | `loss 5%` | Heavy packet loss |
+| jitter | `delay 100ms 20ms distribution normal` | 100 ms RTT ± 20 ms normal jitter |
+| rate 1mbit | `rate 1mbit` | Bandwidth-constrained link |
+
</file context>
| files live in `/tmp` for review and are not wired into that `node_modules`). | ||
| - `iproute2` (`tc`) installed. | ||
| - An **existing LIVE room** (start a broadcaster first, e.g. via | ||
| `sfu-capacity.js`, and note the room id). |
Contributor
There was a problem hiding this comment.
P2: The setup refers to sfu-capacity.js, but that script is absent from this repository, so operators cannot create the required LIVE room using the documented flow. Point to the application's broadcaster flow or add the referenced load-test script.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/results/MEDIA-NETWORK-IMPAIRMENT.md, line 75:
<comment>The setup refers to `sfu-capacity.js`, but that script is absent from this repository, so operators cannot create the required LIVE room using the documented flow. Point to the application's broadcaster flow or add the referenced load-test script.</comment>
<file context>
@@ -0,0 +1,147 @@
+ files live in `/tmp` for review and are not wired into that `node_modules`).
+- `iproute2` (`tc`) installed.
+- An **existing LIVE room** (start a broadcaster first, e.g. via
+ `sfu-capacity.js`, and note the room id).
+- Test credentials in the environment:
+ `CROWDSTREAM_TEST_EMAIL`, `CROWDSTREAM_TEST_PASSWORD`.
</file context>
| ## Prerequisites | ||
|
|
||
| - Node with `puppeteer` resolvable from where `impairment-measure.js` runs. | ||
| The repo's copy lives in `load-test/node_modules`, so run from there or set |
Contributor
There was a problem hiding this comment.
P2: This prerequisite hard-codes the author's checkout and says the committed harness lives in /tmp, so it fails on other machines. Document cd load-test && npm ci and remove the machine-specific NODE_PATH guidance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At load-test/results/MEDIA-NETWORK-IMPAIRMENT.md, line 70:
<comment>This prerequisite hard-codes the author's checkout and says the committed harness lives in `/tmp`, so it fails on other machines. Document `cd load-test && npm ci` and remove the machine-specific `NODE_PATH` guidance.</comment>
<file context>
@@ -0,0 +1,147 @@
+## Prerequisites
+
+- Node with `puppeteer` resolvable from where `impairment-measure.js` runs.
+ The repo's copy lives in `load-test/node_modules`, so run from there or set
+ `NODE_PATH=/home/harshit/CrowdStream/load-test/node_modules` (these harness
+ files live in `/tmp` for review and are not wired into that `node_modules`).
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bash + puppeteer harness that applies tc netem profiles (packet loss, jitter, bandwidth caps) to an interface and measures inbound-rtp getStats under each, to validate media resilience/degradation — currently zero coverage.
Note: simulcast/adaptStreamQuality is currently disabled server-side, so no layer adaptation is expected under impairment (documented as a finding + follow-up).
Authored by Claude (Anthropic) via Claude Code. Not executed — results are placeholders, not fabricated.
Summary by cubic
Adds a
tc netem-based load-test harness that measures WebRTC receive quality under packet loss, jitter, and bandwidth caps. Media resilience under degraded networks previously had zero coverage; this adds a bash driver, a Puppeteer stats measurer, and a results/interpretation doc underload-test/.Harness details
media-impairment.shapplies each netem profile to the target interface and always tears the qdisc down via an EXIT/INT/TERM trap; baseline runs unshaped.impairment-measure.jsjoins an existing live room with Puppeteer viewers, hooksRTCPeerConnectionbefore page scripts run, samplesinbound-rtpvideo stats each second, and emits a single JSON summary line.Important findings
adaptStreamQuality.tsare disabled server-side, so expect flat quality degradation under impairment rather than adaptive layer downgrade; the doc records this as a follow-up.Written for commit 80c0f72. Summary will update on new commits.