Skip to content

fix: handle Windows file persistence races#2027

Merged
yottahmd merged 2 commits intomainfrom
fix-windows-approval-required-flake
Apr 22, 2026
Merged

fix: handle Windows file persistence races#2027
yottahmd merged 2 commits intomainfrom
fix-windows-approval-required-flake

Conversation

@yottahmd
Copy link
Copy Markdown
Collaborator

@yottahmd yottahmd commented Apr 22, 2026

Summary

  • replace Windows status-file compaction with MoveFileEx so readers never observe a removed status file between compact and rename
  • make proc heartbeat publication recreate its parent directory if concurrent restart cleanup removes it between directory creation and temp-file creation
  • add coverage for status-file replacement and proc heartbeat publication with a missing parent directory

Testing

  • go test ./internal/cmn/fileutil -count=1
  • go test ./internal/persis/fileproc -count=1
  • go test ./internal/cmd -run TestRestartCommand_BuiltExecutableRestoresExplicitEnv -count=5
  • go test ./internal/cmd -count=1
  • go test ./internal/service/frontend/api/v1 -run 'TestApproveDAGRunStep|TestApproveDAGRunStepWithInputs|TestApproveDAGRunStepMissingRequired' -count=5
  • go test ./internal/persis/filedagrun -count=1
  • go test ./internal/service/frontend/api/v1 -count=1
  • go test ./internal/cmn/fileutil ./internal/persis/fileproc ./internal/persis/filedagrun ./internal/service/frontend/api/v1 -count=1
  • env GOOS=windows GOARCH=amd64 go test -c -o /tmp/fileutil.test.exe ./internal/cmn/fileutil
  • env GOOS=windows GOARCH=amd64 go test -c -o /tmp/api-v1.test.exe ./internal/service/frontend/api/v1
  • env GOOS=windows GOARCH=amd64 go test -c -o /tmp/filedagrun.test.exe ./internal/persis/filedagrun
  • env GOOS=windows GOARCH=amd64 go test -c -o /tmp/fileproc.test.exe ./internal/persis/fileproc
  • env GOOS=windows GOARCH=amd64 go test -c -o /tmp/cmd.test.exe ./internal/cmd
  • git diff --check

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

📝 Walkthrough

Walkthrough

Introduces platform-specific file replacement implementations for Unix and Windows through build-constrained files. Refactors ReplaceFileWithRetry to delegate to the new replaceFile function instead of inlining platform logic. Adds test coverage for file replacement functionality.

Changes

Cohort / File(s) Summary
Platform-specific implementations
internal/cmn/fileutil/replace_unix.go, internal/cmn/fileutil/replace_windows.go
Added Unix and Windows build-constrained implementations of replaceFile. Unix version uses os.Rename; Windows version converts paths to UTF-16 and calls windows.MoveFileEx with MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH flags.
Retry logic refactoring
internal/cmn/fileutil/retry.go
Modified ReplaceFileWithRetry to delegate to replaceFile instead of inlining platform-specific stat, removal, and rename operations. Removed fmt import.
Test coverage
internal/cmn/fileutil/retry_test.go
Added TestReplaceFileWithRetry with subtests verifying file overwrite and creation scenarios, asserting filesystem state changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: handle Windows file persistence races' accurately describes the main change: refactoring file replacement to use platform-specific implementations (MoveFileEx on Windows, os.Rename on Unix) to prevent race conditions during atomic file operations.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-windows-approval-required-flake

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


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.

@yottahmd yottahmd changed the title fix: replace status files atomically on Windows fix: handle Windows file persistence races Apr 22, 2026
@yottahmd yottahmd merged commit e373530 into main Apr 22, 2026
10 checks passed
@yottahmd yottahmd deleted the fix-windows-approval-required-flake branch April 22, 2026 07:32
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