Skip to content

fix(erofs): remove tmp file on rename error#213

Open
austinvazquez wants to merge 1 commit into
containerd:mainfrom
austinvazquez:minor-touchups-to-212
Open

fix(erofs): remove tmp file on rename error#213
austinvazquez wants to merge 1 commit into
containerd:mainfrom
austinvazquez:minor-touchups-to-212

Conversation

@austinvazquez
Copy link
Copy Markdown
Member

@austinvazquez austinvazquez commented Jun 2, 2026

Follow-up to #212

Revert the write to tmp, file sync + rename bits to simplify creating the vmdk files. I dove through the annals and found the review comment which sent my agent down this path. Let's remove it to simplify. This bit of code is not really executed concurrently for the current use case.

  1. .tmp suffix is not randomised
    DumpVMDKDescriptorToFile uses a fixed path + ".tmp" suffix. Two concurrent callers (unlikely but possible given transformMounts holds no lock) would silently clobber each other's .tmp file. A safer approach is os.CreateTemp(descDir, "merged_fs.vmdk.*.tmp") + rename.

This is low-risk in practice (single-threaded per container lifecycle) but worth noting for robustness.

Copilot AI review requested due to automatic review settings June 2, 2026 14:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR is a follow-up to #212 that ensures DumpVMDKDescriptorToFile best-effort cleans up its temporary merged_fs.vmdk.*.tmp file when the final os.Rename step fails, preventing stray temp files from persisting on rename errors.

Changes:

  • Convert DumpVMDKDescriptorToFile to use a named return error so a deferred cleanup can run on any returned error.
  • Add a deferred best-effort os.Remove(tmpName) cleanup path when the function exits with an error (including rename failures).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/erofs/vmdk.go Outdated
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
@austinvazquez austinvazquez force-pushed the minor-touchups-to-212 branch from c29f118 to 7805a43 Compare June 2, 2026 18:54
@austinvazquez
Copy link
Copy Markdown
Member Author

@hsiangkao , sorry for the churn. I decided to dive into where my agent brought this tmp file + rename from and found it was just a minor comment from itself during its own review. Let's just revert it back to simply write the file with content. We can keep the unit tests though. WDYT?

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.

3 participants