Skip to content

Comments

Use os.replace() instead of os.rename() for cache file moves on Windows#1252

Merged
shi-eric merged 1 commit intoNVIDIA:mainfrom
shi-eric:ershi/fix-aot-cache-skip-windows
Feb 23, 2026
Merged

Use os.replace() instead of os.rename() for cache file moves on Windows#1252
shi-eric merged 1 commit intoNVIDIA:mainfrom
shi-eric:ershi/fix-aot-cache-skip-windows

Conversation

@shi-eric
Copy link
Contributor

@shi-eric shi-eric commented Feb 23, 2026

Summary

  • Fix _compile() file-by-file fallback to use os.replace() instead of os.rename(), which raises FileExistsError on Windows when the destination exists
  • This caused stale binaries to remain when strip_hash=True triggered an intentional overwrite, breaking the test_aot_cache_skip test on Windows GitHub runners

Test plan

  • test_aot_cache_skip passes locally (CPU + CUDA)
  • Verify test_aot_cache_skip passes on Windows GitHub runners

Summary by CodeRabbit

  • Bug Fixes
    • Improved overall reliability of file operations to ensure safer and more consistent handling, particularly in scenarios where target files already exist or operations span across different filesystems
    • Enhanced error handling mechanisms for improved consistency

On Windows, os.rename() raises FileExistsError when the destination
exists, unlike Unix where it atomically replaces. In _compile(), the
file-by-file fallback path (entered when safe_rename cannot atomically
move the build directory) silently caught this error, leaving stale
binaries in place when strip_hash=True triggered an intentional
overwrite.

os.replace() is the cross-platform equivalent recommended by the Python
docs for atomic destination replacement.

Signed-off-by: Eric Shi <ershi@nvidia.com>
@shi-eric shi-eric self-assigned this Feb 23, 2026
@coderabbitai
Copy link

coderabbitai bot commented Feb 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4f818e and 0757307.

📒 Files selected for processing (1)
  • warp/_src/context.py

📝 Walkthrough

Walkthrough

The pull request replaces os.rename() with os.replace() in three file-move operations within the context module, providing atomic overwrite behavior instead of rename semantics. Exception handling is adjusted to catch only OSError. No public API changes are introduced.

Changes

Cohort / File(s) Summary
File operation semantics
warp/_src/context.py
Replaced os.rename() with os.replace() for three file-move operations (binary, metadata, and source finalization). Adjusted exception handling to catch OSError instead of catching both OSError and FileExistsError.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: replacing os.rename() with os.replace() for cache file moves, with platform-specific context (Windows).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Feb 23, 2026

Greptile Summary

This PR fixes a Windows-specific bug where os.rename() raises FileExistsError when attempting to overwrite existing cache files during AOT compilation with strip_hash=True.

  • Changed three instances of os.rename() to os.replace() in the _compile() method
  • os.replace() atomically replaces the destination file on all platforms, which is the intended behavior when strip_hash=True triggers intentional overwrites
  • Simplified exception handling to only catch OSError since os.replace() doesn't raise FileExistsError
  • This fixes the test_aot_cache_skip test failure on Windows GitHub runners where stale binaries remained after intended overwrites

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The change is a straightforward fix for a well-defined platform-specific bug. os.replace() is the correct API for atomic file replacement across all platforms, and the simplified exception handling is appropriate since os.replace() doesn't raise FileExistsError. The fix addresses the exact issue described in the PR (Windows FileExistsError when overwriting with strip_hash=True) and the test plan confirms it passes locally.
  • No files require special attention

Important Files Changed

Filename Overview
warp/_src/context.py Correctly replaces os.rename() with os.replace() to fix Windows file overwrite issue when strip_hash=True

Last reviewed commit: 0757307

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@shi-eric shi-eric merged commit 0aba072 into NVIDIA:main Feb 23, 2026
18 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.

1 participant