Skip to content

fix(record_sim_video): reject incomplete recordings - #562

Open
natew wants to merge 2 commits into
getsentry:mainfrom
natew:tm/rsi-video-finalization
Open

natew wants to merge 2 commits into
getsentry:mainfrom
natew:tm/rsi-video-finalization

Conversation

@natew

@natew natew commented Oct 25, 2026

Copy link
Copy Markdown

record_sim_video stop reported success for any path AXe printed, then copied the file. With current code, inert fake-process probes showed stopped: true plus a parsed path for a child that was force-killed after the 5s stop timeout, one that exited 3, and one killed by SIGINT. Nothing checked the MP4.

Changes:

  • stopSession records whether it had to SIGKILL the child, plus exit code and signal, and returns them.
  • checkMp4Finalized walks top-level MP4 boxes and requires ftyp, moov, and every declared box fully present. No new dependency.
  • record_sim_video stop fails with "video file is incomplete" on a forced kill or a failed check. The AXe output, exit code and signal go into diagnostics, and the source file is left in place. A finalized file after a nonzero exit still succeeds.

No timeout or retry changes. The cause of the original missing-moov file is not established here.

Checked: unit tests with synthetic finalized and truncated boxes (moov before and after mdat, cut mdat, cut moov, empty), a real ffmpeg MP4 (accepted) and its 3000-byte truncation (rejected), and fake child processes for the kill and clean-exit paths. No simulator, AXe or real recording was used.

Not run: snapshot and smoke suites. src/server/__tests__/start-mcp-server.test.ts fails here on the missing generated version.ts, unrelated to this change.

natew added 2 commits October 25, 2026 00:42
Report force-killed stops and MP4s without a complete moov box as failures instead of copying them as a successful recording. Process output and the source file are preserved.
@natew
natew marked this pull request as ready for review October 25, 2026 10:46

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a37c7c2. Configure here.


const outcome = await Promise.race([closePromise, createTimeoutPromise(timeoutMs)]);
if (outcome === 'timed_out') {
session.forcedKill = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force-kill omits process exit status

Low Severity

After the stop timeout, waitForChildToStop sends SIGKILL and returns without waiting for the child to exit, so stopSession reads exitCode and signal while the process is still running. Diagnostics then show none for both on the force-kill path, which is the case those fields were added to explain.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a37c7c2. Configure here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant