Skip to content

fix(export): preserve MOV destinations on failed exports - #2907

Merged
richiemcilroy merged 3 commits into
CapSoftware:mainfrom
yashranaway:fix/export-flow
Oct 24, 2026
Merged

richiemcilroy merged 3 commits into
CapSoftware:mainfrom
yashranaway:fix/export-flow

Conversation

@yashranaway

@yashranaway yashranaway commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

mov exports (the cursor track ones) encoded straight into the file you picked. if the export was cancelled or failed partway and that path already had a video, it was left truncated or replaced with a broken file. gif and mp4 already stage their output in a temp file.

now mov does the same: it encodes to a temp file next to the destination and only swaps it in after both the render and the encode finish, keeping the old file's permissions when it replaces one. the temp file helper moved its permission check into a shared function, mp4 behaviour is unchanged. tests cancel over an existing file, export to a new one and fail the final replace.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; the previous cleanup issue is fixed and no new blocking issues were found.

Summary

MOV exports now write to a temporary file beside the destination and replace the destination only after rendering and encoding succeed.

  • MOV exports replace the destination only after the movie is complete.

The previous cleanup finding is fixed. No new actionable issues were found. The tests were not run during this review.

Reviews (2) · Last reviewed commit: "fix(export): retain MOV temporary output..." · Reviewed by Greptile

@vercel

vercel Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

@yashranaway is attempting to deploy a commit to the Cap Software Inc Team on Vercel.

A member of the Team first needs to authorize it.

@yashranaway

Copy link
Copy Markdown
Contributor Author

@greptileai review

Comment thread crates/export/src/mov.rs Outdated
Comment on lines +82 to +83
let temporary_output = temporary_export_output(&mov_output_path)?;
let encoder_output_path = temporary_output.to_path_buf();

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.

P2 Temporary files escape cleanup

temporary_output stays in the export future, but the blocking encoder receives only its path. The estimate caller can drop this future on cancellation or timeout. If the encoder has not opened the file yet, cleanup removes it before MOVFile::init creates it again, leaving a partial file with no cleanup owner.

Keep a shared owner in the encoder until it exits, as the MP4 path does.

Knowledge Base Used: Export and editor rendering

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/export/src/mov.rs
Line: 82-83

Comment:
**Temporary files escape cleanup**

`temporary_output` stays in the export future, but the blocking encoder receives only its path. The estimate caller can drop this future on cancellation or timeout. If the encoder has not opened the file yet, cleanup removes it before `MOVFile::init` creates it again, leaving a partial file with no cleanup owner.

Keep a shared owner in the encoder until it exits, as the MP4 path does.

**Knowledge Base Used:** [Export and editor rendering](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/export-and-editor-rendering.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed in 68c5948. the mov encoder now holds a shared owner of the temp file until it exits, same as mp4 does, and there's a cancellation test that fails without it.

@richiemcilroy

Copy link
Copy Markdown
Member

Thanks @yashranaway, writing to a temp file and swapping it in is the right approach. Greptile found one real gap at crates/export/src/mov.rs:83: temporary_output is owned by the export future, but the blocking encoder only gets its path. If the estimate caller drops the future (cancel or timeout) before MOVFile::init opens the file, cleanup runs first and the encoder then recreates a partial file that nothing removes. Could you keep a shared owner of the temp file inside the encoder until it exits, like the MP4 path does, and add a test for a cancel before the encoder opens the file? Then comment @greptileai review. Thanks!

@richiemcilroy

Copy link
Copy Markdown
Member

@greptileai review

@richiemcilroy
richiemcilroy merged commit a169c44 into CapSoftware:main Oct 24, 2026
47 of 49 checks passed
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.

2 participants