Repository navigation
test(verify): chaos suite for every engine, and fixes for the 58 crashes and hangs it found - #2709
Conversation
…id-stream instead of panicking
…another pixel format
…their recording and NUL bytes in media paths on load
… and return errors for unreadable media instead of panicking
…ts and settings through every engine
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptileai review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !self.accepts(&frame) && !self.rebuild_for(&frame) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Rebuilding the resampler changes the units of input timestamps, but min_next_pts still holds a sample count from the old rate. After ten seconds at 48 kHz, switching to 8 kHz clamps the next frame to roughly 480,000 samples instead of 80,000. Rescaling that timestamp puts the audio near 60 seconds and can insert a long silent gap.
AudioEncoderBase::send_frame also stamps the first changed frame using the old rate. Keep timestamp tracking in fixed units, or convert all stored timestamps when the input rate changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/enc-ffmpeg/src/audio/buffered_resampler.rs
Line: 106-108
Comment:
**Audio jumps after rate changes**
Rebuilding the resampler changes the units of input timestamps, but `min_next_pts` still holds a sample count from the old rate. After ten seconds at 48 kHz, switching to 8 kHz clamps the next frame to roughly 480,000 samples instead of 80,000. Rescaling that timestamp puts the audio near 60 seconds and can insert a long silent gap.
`AudioEncoderBase::send_frame` also stamps the first changed frame using the old rate. Keep timestamp tracking in fixed units, or convert all stored timestamps when the input rate changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let size = std::fs::metadata(&path).map(|m| m.len()).unwrap_or(0); | ||
| if !expect_media { | ||
| return Ok(Verdict::Ok(format!("{size} bytes"))); | ||
| } |
There was a problem hiding this comment.
Every fuzz export passes expect_media = false, so this branch returns Verdict::Ok even when the output file is missing or empty. export_config also reports success without checking the output. These probes can therefore miss the wrong-success failures the suite is meant to catch.
Use a shared output check after a successful export. Damaged input may return an error, but success should still produce a readable file with frames.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/verify/tests/chaos_support/project.rs
Line: 1070-1073
Comment:
**Empty exports pass the suite**
Every fuzz export passes `expect_media = false`, so this branch returns `Verdict::Ok` even when the output file is missing or empty. `export_config` also reports success without checking the output. These probes can therefore miss the wrong-success failures the suite is meant to catch.
Use a shared output check after a successful export. Damaged input may return an error, but success should still produce a readable file with frames.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| Err(_) => ("panicked", String::new()), | ||
| }; | ||
| log_line(&["verdict", &seed.to_string(), kind, &one_line(&text)]); | ||
| let _ = std::fs::remove_dir_all(&dir); |
There was a problem hiding this comment.
The child deletes each seed directory after writing its verdict, even when CAP_VERIFY_CHAOS_KEEP_SCRATCH=1. The flag only prevents the parent from deleting the outer directory. Developers lose the damaged inputs and outputs from completed panic and wrong-success failures.
Honor the keep flag here too.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/verify/tests/chaos_support/harness.rs
Line: 224
Comment:
**Saved failure files disappear**
The child deletes each seed directory after writing its verdict, even when `CAP_VERIFY_CHAOS_KEEP_SCRATCH=1`. The flag only prevents the parent from deleting the outer directory. Developers lose the damaged inputs and outputs from completed panic and wrong-success failures.
Honor the keep flag here too.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| "fast": { | ||
| "macos": 0 | ||
| }, | ||
| "full": { | ||
| "macos": 0 |
There was a problem hiding this comment.
Both chaos case floors are zero, so the check in floor.py passes even when no chaos cases ran. Skipped tests or dropped groups can silently remove coverage without failing this gate.
Set nonzero floors based on the expected fast and full case counts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/verify/suites.json
Line: 158-162
Comment:
**Missing chaos runs pass**
Both chaos case floors are zero, so the check in `floor.py` passes even when no chaos cases ran. Skipped tests or dropped groups can silently remove coverage without failing this gate.
Set nonzero floors based on the expected fast and full case counts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| Step::DropCamera => { | ||
| camera = None; | ||
| Ok(()) | ||
| } | ||
| Step::DropMic => { | ||
| mic = None; | ||
| Ok(()) |
There was a problem hiding this comment.
DropCamera and DropMic only drop the local synthetic handles. The recording keeps cloned feed locks, and neither synthetic handle has a drop action that disconnects its feed. These probes stop sending frames but never exercise a device disconnect.
Add an explicit disconnect operation and use it here, keeping frame-stall probes separate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/verify/tests/chaos_support/actors.rs
Line: 324-330
Comment:
**Disconnect probes only stop frames**
`DropCamera` and `DropMic` only drop the local synthetic handles. The recording keeps cloned feed locks, and neither synthetic handle has a drop action that disconnects its feed. These probes stop sending frames but never exercise a device disconnect.
Add an explicit disconnect operation and use it here, keeping frame-stall probes separate.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Split into two kinds: camera-stalls / mic-stalls (frames stop, the feed stays) and camera-disconnects / mic-disconnects, which send the feed's RemoveInput through a new synthetic remove_input, the message the app sends when a device goes away. The native stream dying below the feed (CoreAudio/AVFoundation) is not reachable without a real device, so that layer stays out of the synthetic suite.
There was a problem hiding this comment.
That distinction is correct, and the updated probes now cover it: camera-stalls/mic-stalls drop only the producer handle, while camera-disconnects/mic-disconnects call remove_input() and exercise the feed’s RemoveInput path. Keeping native CoreAudio/AVFoundation device loss out of this synthetic suite is appropriate since it cannot be reproduced without a real device. With that split, this finding is resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let root = ctx.path("fx"); | ||
| copy_dir(&fixture()?, &root)?; | ||
| let studio = root.join("studio"); | ||
| let n = mutate_tree(ctx, &studio.join("content"), 11)?; |
| let root = ctx.path("fx"); | ||
| copy_dir(&fixture()?, &root)?; | ||
| let content = root.join("instant/content"); | ||
| let n = mutate_tree(ctx, &content, 12)?; |
| 30, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_json_file(ctx, &dir.join("project-config.json"), 1)?; |
| 90, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_json_file(ctx, &dir.join("project-config.json"), 2)?; |
| 90, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_json_file(ctx, &dir.join("project-config.json"), 3)?; |
| 90, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_json_file(ctx, &dir.join(SEGMENT).join("cursor.json"), 6)?; |
| 90, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_bytes_file(ctx, &dir.join(SEGMENT).join("display.mp4"), 7)?; |
| 90, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_bytes_file(ctx, &dir.join(SEGMENT).join("camera.mp4"), 8)?; |
| 90, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_bytes_file(ctx, &dir.join(SEGMENT).join("audio-input.ogg"), 9)?; |
| 30, | ||
| target(|ctx| { | ||
| let dir = project::prepare(ctx).context("prepare")?; | ||
| let n = mutate_bytes_file(ctx, &dir.join(SEGMENT).join("display.mp4"), 10)?; |
…le rate changes mid-stream
…of writing an empty file
… probe real disconnects, keep scratch on request and set coverage floors
|
@greptileai review |
| for f in &mut empty { | ||
| f.samples = 0; | ||
| } | ||
| out.push(ok("empty-buffers", empty)); |
There was a problem hiding this comment.
empty-buffers supplies no audio samples, but ok() sets expect_media to true. The Ogg path can finish cleanly without audio packets, which this probe reports as wrong-success. This creates a false failure in the chaos gate.
Set expect_media to false for the single-source empty-buffer cases. Keep the mixed-source case strict because its second source supplies audio.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/verify/tests/chaos_support/record.rs
Line: 722
Comment:
**Empty audio fails the suite**
`empty-buffers` supplies no audio samples, but `ok()` sets `expect_media` to `true`. The Ogg path can finish cleanly without audio packets, which this probe reports as `wrong-success`. This creates a false failure in the chaos gate.
Set `expect_media` to `false` for the single-source empty-buffer cases. Keep the mixed-source case strict because its second source supplies audio.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
@greptileai review |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| let previous_rate = self.resampler.input().rate; | ||
| let input_rate = if frame.rate() > 0 { | ||
| frame.rate() | ||
| } else { | ||
| previous_rate | ||
| }; | ||
| self.inner | ||
| .rescale_input_rate(f64::from(previous_rate), f64::from(input_rate)); | ||
| self.inner | ||
| .update_pts_with_rate(&mut frame, timestamp, input_rate); | ||
| .update_pts_with_rate(&mut frame, timestamp, f64::from(input_rate)); |
There was a problem hiding this comment.
Rejected rate changes leave encoder timestamps in the wrong units
Medium Severity
When a frame has a new positive rate but an unsupported format or layout, send_frame rescales first_pts and last_frame_pts before BufferedResampler::rebuild_for rejects it. The resampler retains its old input rate, so the next valid frame at that old rate skips rescaling even though the encoder state now uses the rejected rate. For example, a 48 kHz stream starting at timestamp 1 s, followed by a rejected 8 kHz frame and then valid 48 kHz input, retains first_pts=8000 instead of 48000, inserting about 0.83 s of silence and shifting subsequent audio. Ensure the resampler accepts the new input before committing timestamp changes, or track the timestamp state's rate independently of the resampler.
| }); | ||
| break; | ||
| } | ||
| next = after; |
There was a problem hiding this comment.
Crashes between fuzz seeds can disappear from the results
Medium Severity
A background engine thread can abort after a seed's verdict is logged but before the next seed's start event, including during scratch cleanup. In that case unfinished is None, and when more seeds remain, this branch advances next without recording the abnormal exit. Earlier verdicts remain passes and a successful replacement child can make the entire probe pass despite SIGABRT, SIGSEGV, or an Objective-C exception. Record abnormal termination against the last completed seed before restarting, not only when after >= end.
This branch was successfully deployed
2 participants
A new engine-verify suite,
chaos, feeds hostile device data, files, projects and settings through the recording, editor, playback, export and recovery engines, one probe per child process, and fails on any panic, abort, signal, Objective-C exception, hang or success with an empty output. Its first run on main found 58 failing probes from 19 root causes. This PR adds the suite and fixes all of them.The suite
crates/verify/tests/chaos.rs+chaos_support/: 764 probes in six areas (chaos/record/,chaos/files/,chaos/export/,chaos/editor/,chaos/fragments/,chaos/fuzz/). Each probe re-runs the test binary as a child, so an abort or a hang ends the child, not the run; a panic hook records panics on every thread. Fuzzing is seeded and failing seeds are shrunk; the failure prints its repro.scripts/verify/suites.jsonfor macOS (fast and full tiers);run.shbuildscap-muxerfor it like thecrashsuite.MicrophoneFeed's synthetic test handle gainspush_rawso probes can deliver raw callback buffers in any format, rate and channel count; the synthetic camera and microphone gainremove_inputso actor probes exercise a real device loss (RemoveInput), separately from probes where frames just stop.Fixes
Recording (each of these killed the recording or its audio track):
AudioInfo::wrap_framecopied the callback's bytes into a frame sized for whole samples only: a callback with no whole sample (CoreAudio sends empty buffers around device changes) panicked on a frame with no planes, and one ending mid-sample copied past the frame. Partial trailing samples are now dropped and empty callbacks give an empty frame.BufferedResampler::add_frameunwrappedswr's "Input changed" error. It now rebuilds the resampler for the new input and keeps the output, and drops (with a warning) frames no resampler accepts, such as 0 channels or rate 0. Its check is exactly the onerun()makes, so unchanged streams take the same path. On a rate change the encoder's stored timestamps (first/last pts and the resampler's minimum next pts) are rescaled to the new rate, so 10 s at 48 kHz followed by 8 kHz input continues at 10 s instead of jumping to 60 s.copy_plane_datacopied past the pooled frame (allocated at the size the encoder was opened with). The copy is now bounded by both planes, so every buffer that fits copies byte for byte as before and a larger one is cropped; a buffer whose pixel format differs from the frame's is rejected (every such case either panicked or wrote garbage before).Opening, editing and exporting damaged projects:
ProjectRecordingsMeta::new(expect); they now return the same error multi-segment projects do.RenderingError::MissingRecordingSegment/InvalidTimelineSegmentfor non-finite or backwards clips); the editor preview stops at that clip like it does when no segment is found.fpsof 0 (recovery's "not recorded" marker) left the editor with no frame; the decoders use the legacy 30 fps for it. The meta value itself is unchanged, so recovery still detects the capture rate.Performance:
Verification
scripts/verify/run.sh full --suites chaoson macOS: 760 of 764 probes pass (main: 702 pass, 58 fail).chaos/files/text-hostileuses 600 characters of NUL / right-to-left override / emoji at font weight 100 rather than 6,000: at weight 100 every such glyph runs a full font-fallback search (about 45 ms each in a debug build, linear), so 6,000 only measured the debug build's speed.cargo testfor the touched crates,cargo clippyandcargo fmtclean.
The latest change appears safe to merge, with one non-blocking gap left in the mixed-audio probe.Findings
Fix with agent prompt
Summary
The PR adds isolated chaos probes across recording, editing, export and recovery, along with fixes for crashes and hangs they exposed.
Reviews (4) · Last reviewed commit: "test(verify): expect no audio from the e..." · Reviewed by Greptile