fix(specialist): make overwrites atomic#757
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens specialist manifest overwrites by switching from in-place truncation writes to a write-to-temp + atomic replace flow, addressing the race/non-atomic overwrite risks described in issue #753.
Changes:
- Implement atomic overwrite via sibling temp file creation (0600), sync/close, then rename into place.
- Add symlink rejection to prevent overwriting a symlinked specialist destination.
- Add regression tests covering atomic replacement behavior, permissions, symlink safety, and temp cleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/specialist/storage.go | Reworks overwrite path to write to a synced temp file and atomically replace the destination, rejecting symlink destinations. |
| internal/specialist/storage_test.go | Adds regression coverage for overwrite replacement semantics, permissions, and temp-file cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := os.Rename(tempPath, path); err != nil { | ||
| return fmt.Errorf("replace specialist file: %w", err) | ||
| } |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSpecialist storage creation now uses exclusive file creation for new files and atomic temporary-file replacement for overwrites. Tests cover replacement content, permissions, retry behavior, sync errors, and temporary-file cleanup. ChangesSpecialist overwrite safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/specialist/storage.go (1)
124-128: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo directory fsync after rename — rename durability isn't guaranteed on crash.
os.Renameat Line 124 is not followed by anfsyncon the parent directory. On some filesystems (notably ext4 withoutdata=journal), a rename's directory-entry update is not guaranteed durable until the containing directory is fsynced — a crash immediately after a successful rename can leave the destination reverted to its pre-rename state. This is directly relevant to the PR's stated goal of avoiding files left "empty or partially written after disk, process, or I/O failures."♻️ Proposed fix: fsync the parent directory after rename
if err := os.Rename(tempPath, path); err != nil { return fmt.Errorf("replace specialist file: %w", err) } + if dir, derr := os.Open(filepath.Dir(path)); derr == nil { + _ = dir.Sync() + _ = dir.Close() + } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/storage.go` around lines 124 - 128, Update the atomic replacement flow containing os.Rename to open the destination’s parent directory, fsync the directory after a successful rename, and close it while propagating any open, sync, or close errors. Preserve the existing wrapped rename error and nil success behavior, and ensure cleanup handles failures without masking the primary error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/specialist/storage.go`:
- Around line 117-126: Update writeSpecialistAtomic to provide a
Windows-specific replacement path that guarantees atomic, crash-safe file
replacement instead of relying solely on os.Rename; isolate platform-specific
behavior behind appropriate build-specific helpers while preserving the existing
symlink checks and error handling.
---
Nitpick comments:
In `@internal/specialist/storage.go`:
- Around line 124-128: Update the atomic replacement flow containing os.Rename
to open the destination’s parent directory, fsync the directory after a
successful rename, and close it while propagating any open, sync, or close
errors. Preserve the existing wrapped rename error and nil success behavior, and
ensure cleanup handles failures without masking the primary error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48974b34-0cac-4aa8-8a9a-e20b6875d0f7
📒 Files selected for processing (2)
internal/specialist/storage.gointernal/specialist/storage_test.go
| info, err := os.Lstat(path) | ||
| if err != nil && !os.IsNotExist(err) { | ||
| return fmt.Errorf("inspect specialist file: %w", err) | ||
| } | ||
| if err == nil && info.Mode()&os.ModeSymlink != 0 { | ||
| return fmt.Errorf("refusing to overwrite symlink specialist file: %s", path) | ||
| } | ||
| if err := os.Rename(tempPath, path); err != nil { | ||
| return fmt.Errorf("replace specialist file: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== go.mod ==\n'
sed -n '1,80p' go.mod
printf '\n== relevant file excerpt ==\n'
nl -ba internal/specialist/storage.go | sed -n '100,150p'
printf '\n== search for Rename/atomicity notes ==\n'
rg -n "atomic|Rename|MoveFileEx|MOVEFILE_REPLACE_EXISTING|MOVEFILE_WRITE_THROUGH" -S .Repository: Gitlawb/zero
Length of output: 1706
🌐 Web query:
Go os.Rename Windows atomic non-Unix platforms atomicity Go docs
💡 Result:
According to the official Go documentation, the os.Rename function is not an atomic operation on non-Unix platforms, including Windows [1][2][3]. While os.Rename serves to rename or move a file—and will replace an existing destination file if it is not a directory—it does not provide the same atomic guarantees on Windows that it does on Unix-like systems [1][3]. Explicit documentation added to the Go standard library clarifies this limitation: "Even within the same directory, on non-Unix platforms Rename is not an atomic operation" [1][4]. Although some Windows API calls (such as certain uses of MoveFileEx) can provide atomic-like behaviors under specific conditions (e.g., when the source and destination are on the same volume/filesystem), the Go os.Rename implementation does not guarantee this atomicity for the programmer [5][6][7]. Consequently, when building applications that require strictly atomic file replacement across different operating systems, developers are cautioned against relying on os.Rename for that purpose on non-Unix platforms [4].
Citations:
- 1: https://pkg.go.dev/os
- 2: https://pkg.go.dev/os?GOOS=windows
- 3: https://pkg.go.dev/os@go1.26.4
- 4: https://git.jordan.im/go/commit/?h=go1.20.8&id=c0799f7015e6cae37c21294bb94f56050fda5f4e
- 5: os: make Rename atomic on Windows golang/go#8914
- 6: os: Make os.Rename act more similar to *NIX on Windows golang/go#10773
- 7: https://stackoverflow.com/questions/30385225/is-there-an-os-independent-way-to-atomically-overwrite-a-file
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== internal/specialist/storage.go ==\n'
awk 'NR>=1 && NR<=220 { printf "%4d %s\n", NR, $0 }' internal/specialist/storage.go
printf '\n== windows-specific checks in tests/code ==\n'
rg -n "GOOS != \"windows\"|windows|MoveFileEx|atomic" internal -S
printf '\n== os.Rename docs excerpt from local stdlib if available ==\n'
go doc os.Rename | sed -n '1,120p'Repository: Gitlawb/zero
Length of output: 50369
🌐 Web query:
Go os.Rename doc Windows non-Unix not atomic operation same directory
💡 Result:
The Go documentation explicitly states that os.Rename is not an atomic operation on non-Unix platforms, including Windows, even when renaming files within the same directory [1][2]. Unlike POSIX-compliant operating systems where rename is guaranteed to be atomic [3], Windows does not provide a standard, reliable mechanism for atomic file replacement via the APIs typically utilized by os.Rename [2][3]. Consequently, if an error occurs during the operation, it can leave the system in an inconsistent state [4][5]. Users requiring atomic file replacement on Windows often must implement their own solutions using specialized system calls (such as certain NtSetInformationFile flags available in modern Windows versions) or higher-level patterns, though these are not exposed directly through the standard os.Rename function [5][3][6].
Citations:
- 1: https://pkg.go.dev/os?GOOS=windows
- 2: https://git.jordan.im/go/commit/?h=go1.20.8&id=c0799f7015e6cae37c21294bb94f56050fda5f4e
- 3: https://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows
- 4: os: Make os.Rename act more similar to *NIX on Windows golang/go#10773
- 5: proposal: os: add a package to atomically create or replace a file golang/go#22397
- 6: https://bugs.python.org/issue8828
writeSpecialistAtomic still depends on os.Rename, which is not atomic on Windows. If this helper is meant to guarantee crash-safe replacement cross-platform, add a Windows-specific replace path; otherwise, scope the atomicity promise/docs to Unix-like systems.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/specialist/storage.go` around lines 117 - 126, Update
writeSpecialistAtomic to provide a Windows-specific replacement path that
guarantees atomic, crash-safe file replacement instead of relying solely on
os.Rename; isolate platform-specific behavior behind appropriate build-specific
helpers while preserving the existing symlink checks and error handling.
Source: Coding guidelines
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Use an atomic replacement primitive on Windows
internal/specialist/storage.go:131
This still reachesos.RenamethroughRenameWithRetry; retries only handle sharing violations and do not change the replacement operation. Go explicitly documents thatos.Renameis not atomic on non-Unix platforms, even for sibling paths, so a Windows--forceoverwrite can expose a missing/intermediate manifest if it is interrupted or observed concurrently—the exact failure this change is meant to eliminate. Add a Windows-specific replacement path that provides the required atomic/crash-safe semantics (and test that operation); the current Windows test only proves a retry followed by the same non-atomic rename. -
[P1] Preserve the existing Windows DACL when publishing the replacement
internal/specialist/storage.go:101
Overwriting now publishes a newly created temporary file, so its inherited directory ACL replaces the destination file's ACL.temp.Chmod(0o600)does not make the file private on Windows—Go only uses the owner-write bit there—whereas the old in-place truncate retained an existing restrictive DACL. Consequently, overwriting a specialist that has been explicitly restricted inside a directory readable by a group can expose its system prompt to that group. Preserve the destination security descriptor (or apply an equivalent private DACL) as part of the Windows replacement, and cover that behavior with a Windows test.
Summary
Verification
n- go test ./...n- go run ./cmd/zero-release buildn- go run ./cmd/zero-release smoken- git diff HEAD --check`nEnvironment limitations
Fixes fix(specialist): overwrite is race-prone and non-atomic #753
Summary by CodeRabbit
Bug Fixes
Tests