fix: harden media server under load#2007
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
| for (const entry of await readdir(dirPath)) { | ||
| if (!entry.endsWith(".m3u8") && !entry.endsWith(".mpd")) continue; | ||
| const content = await file(join(dirPath, entry)).text(); | ||
| if (/\b(?:file|data):/i.test(content)) { |
There was a problem hiding this comment.
Nice belt-and-suspenders check. One gap: file:/data: are just the explicit schemes — an attacker-controlled manifest can still reference local paths via relative URLs (e.g. ../../etc/passwd) once the manifest itself is materialized to disk.
Might be worth validating that all resolved segment/resource URLs are http(s) (or at least rejecting any non-http(s) scheme / bare paths) before handing the manifest to ffprobe.
|
hey @greptileai, please re-review the PR |
Summary
Incident addressed
A long-running segment mux could retain multiple complete media copies while Linux page cache grew outside the process RSS signal used for admission. This allowed a Railway replica to exceed its memory limit under sustained work. A separate upstream object-storage failure could also leave a worker doing unnecessary transfer work.
Validation
Greptile Summary
This PR hardens the media server against memory and CPU exhaustion by replacing RSS-based admission with real cgroup v1/v2 metrics, streaming segment downloads and concatenations through disk instead of buffering full files in memory, aborting in-flight transfers on fatal failures, and retrying transient upstream errors with exponential backoff.
container-cpu.ts,container-memory.ts) read kernel-reported limits and usage;job-manager.tswires them into admission thresholds (throttle at 85%, reject at 90%) with a CPU utilisation signal from delta-sampledcpu.stat.muxSegmentsAsyncpipeline streams each segment to disk, returns ordered paths, early-deletes input directories to release page cache, and wraps the FFmpeg mux inwithMuxMemoryGuardfor a 1 s polling abort.checkHasAudioTrackmaterialises HLS/MPD manifests to disk before probing with a boundedAbortSignal;resolveResourceUrlnow blocks non-HTTP protocols with a post-hoc manifest scan as defence-in-depth.Confidence Score: 5/5
Safe to merge; the hardening logic is well-tested and the two observations are conservative design choices rather than correctness defects.
The cgroup-reading modules are straightforward and have dedicated unit tests with injected paths. The streaming pipeline correctly orders segment output, aborts in-flight downloads on the first fatal error, and cleans up intermediate files eagerly. The two flagged items are edge cases that cause unnecessary retries rather than data loss or corruption.
The withMuxMemoryGuard block and the two init-file downloadUrlToFile calls in muxSegmentsAsync in video.ts are worth a second look if retry pressure under sustained load becomes observable in production.
Important Files Changed
Comments Outside Diff (1)
apps/media-server/src/lib/job-manager.ts, line 581-619 (link)getContainerCpuLimit()read twice pergetSystemResources()callgetSystemResources()first callsgetCpuCapacity()(which readscpu.max/cfs_quota_us), then passes the result ascpuCapacitytogetCpuPressure(), wheregetContainerCpuLimit()is called a second time to checkcpuLimit > 0. On every/healthpoll or/video/statusfetch, two synchronous filesystem reads are issued to the same cgroup path. The redundant read can be eliminated by using the already-computedcpuCapacityto infer whether a container limit is present: ifcpuCapacity !== hostCpuCount, a limit exists and the usage path should be taken. Alternatively,getCpuCapacity()can return a structured result that includes the limit flag.Prompt To Fix With AI
Reviews (11): Last reviewed commit: "fix: decode DASH resource attributes" | Re-trigger Greptile