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.
WalkthroughSpecialist storage now uses exclusive creation for new files and temporary-file replacement for overwrites. Platform-specific replacement handles retries, rollback, metadata preservation, and cleanup. CLI and tool output surface manifest warnings. ChangesSpecialist overwrite safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StorageCreate
participant writeSpecialistReplacement
participant ReplaceWithRetry
participant FileSystem
StorageCreate->>writeSpecialistReplacement: format and write specialist content
writeSpecialistReplacement->>FileSystem: create, sync, and close temporary file
writeSpecialistReplacement->>ReplaceWithRetry: publish temporary file
ReplaceWithRetry->>FileSystem: replace destination with platform-specific semantics
FileSystem-->>ReplaceWithRetry: replacement result
writeSpecialistReplacement->>FileSystem: sync destination directory
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
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.
…imitive Addresses both P1 findings on Gitlawb#757. os.Rename — which fsutil.RenameWithRetry wraps — is documented non-atomic outside Unix, and the retry loop only handles sharing violations. Worse, the replacement is a freshly created temporary file, so renaming it over the destination also publishes the directory's inherited DACL in place of the destination's own: overwriting a specialist that had been explicitly restricted inside a group-readable directory would expose its system prompt to that group. temp.Chmod(0o600) cannot prevent that on Windows, where Go maps only the owner-write bit. New fsutil.ReplaceWithRetry publishes through a platform primitive: - Windows uses ReplaceFileW with REPLACEFILE_WRITE_THROUGH, one operation that carries the replaced file's security descriptor, attributes, and streams over to the replacement. REPLACEFILE_IGNORE_ACL_ERRORS is deliberately not passed — silently losing the descriptor is the failure being fixed — so a merge failure surfaces and leaves the destination untouched. A missing destination falls back to a rename (nothing to replace or preserve), as do volumes that cannot perform a replace at all (FAT/exFAT, some network redirectors), which carry no ACL either. The transient-lock retry is unchanged. - Unix keeps rename(2), which is already atomic within one filesystem. Tests: Windows coverage asserts the destination's creation time survives (a rename would not, proving the call goes through ReplaceFileW) and that a protected owner-only DACL survives both at the fsutil layer and through the real `Create --force` path; a plain rename was verified to replace that DACL with the directory's inherited ACEs, so the assertion has teeth. POSIX covers both destination shapes.
|
Pushed [P1] Use an atomic replacement primitive on Windows — the publish no longer goes through [P1] Preserve the existing Windows DACL — this is the same API, and it is why Two narrow fallbacks to Tests, and their teeth
I checked the DACL test actually has teeth by running the old rename-based publish against it. It replaces That is the exposure you described, and @coderabbitai's open thread on Validation (Windows host, Go 1.26.5): Note that @jatmn @Vasanthdev2004 @gnanam1990 — ready for another look. I can't use the reviewer-request button on this repo, hence the mention. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/fsutil/replace_windows.go`:
- Around line 50-56: Update the destination validation in the Windows
replacement flow around os.Lstat(dst) to inspect the returned FileInfo and
reject destinations where info.Mode()&os.ModeSymlink is nonzero before invoking
ReplaceFileW. Preserve the existing not-found rename behavior and return
non-symlink stat errors unchanged, while maintaining the caller’s existing
symlink-safety error behavior.
- Around line 45-48: Update the ReplaceFileW invocation in the
destination-existing branch to pass only replaceFileWriteThrough, removing
replaceFileIgnoreMergeErrors. Preserve the behavior for new destinations and
ensure ACL merge failures are returned as errors without silently dropping the
destination DACL.
🪄 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 Plus
Run ID: 2926c881-1b80-4db8-93ef-cf8745bdc833
📒 Files selected for processing (7)
internal/fsutil/rename.gointernal/fsutil/replace_other.gointernal/fsutil/replace_other_test.gointernal/fsutil/replace_windows.gointernal/fsutil/replace_windows_test.gointernal/specialist/storage.gointernal/specialist/storage_dacl_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/specialist/storage.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/fsutil/replace_windows_test.go`:
- Around line 93-98: Remove REPLACEFILE_IGNORE_MERGE_ERRORS from the
ReplaceFileW flags in ReplaceWithRetry so metadata merge failures propagate
instead of allowing replacement to succeed with incorrect ACLs. Preserve the
fail-closed DACL behavior, and add a regression test covering merge failure
rather than leaving a TODO when the ACL failure path cannot be exercised.
In `@internal/fsutil/replace_windows.go`:
- Around line 65-82: Update the ReplaceFileW failure handling in the replace
flow around replaceProcReplaceFil.Call to detect
ERROR_UNABLE_TO_MOVE_REPLACEMENT when the replacement remains at src, then move
that surviving file back to dst before returning. Preserve the existing fallback
handling for unsupported functions and other errors, and ensure the recovery
result is propagated appropriately.
- Line 69: Update the ReplaceFileW flag argument in the replacement path to
remove both replaceFileWriteThrough and replaceFileIgnoreMergeErrors, and delete
their now-unused constant definitions. Preserve normal ACL/attribute merge
failure propagation; do not claim write-through durability unless an explicitly
supported mechanism is implemented.
🪄 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 Plus
Run ID: c81ef329-b822-4b3c-bbc8-7e5dd60fd65c
📒 Files selected for processing (7)
internal/fsutil/rename.gointernal/fsutil/replace_other.gointernal/fsutil/replace_other_test.gointernal/fsutil/replace_windows.gointernal/fsutil/replace_windows_test.gointernal/specialist/storage.gointernal/specialist/storage_dacl_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/specialist/storage.go
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] Do not suppress the DACL merge failure
internal/fsutil/replace_windows.go:69
The call passesREPLACEFILE_IGNORE_MERGE_ERRORS(0x2), despite the comment and PR description saying that an ACL merge failure is fail-closed. Microsoft documents that this flag makesReplaceFileWsucceed without preserving ACLs when it lacksWRITE_DAC, so a--forceoverwrite can publish the temp file's inherited directory DACL over a restricted specialist and expose its system prompt. Remove this flag and cover the denied-merge path; the current DACL tests only exercise a successful merge. -
[P1] Recover the documented partial ReplaceFileW failure
internal/fsutil/replace_windows.go:82
With no backup path,ERROR_UNABLE_TO_MOVE_REPLACEMENT(1176) means Windows has already removeddstwhile leaving the replacement atsrc. This returns that error unchanged, after whichwriteSpecialistAtomicWithremovessrcin its deferred cleanup. A failedspecialist create --forcecan therefore delete both the old manifest and the only surviving new copy. Detect this state and restore or otherwise preserve the source before allowing cleanup to run, with a regression test.
…ileW Addresses jatmn's two Gitlawb#757 review findings: - The call passed REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) while the comment claimed ACL failures were fail-closed. Microsoft documents 0x2 and REPLACEFILE_IGNORE_ACL_ERRORS (0x4) identically: with either set, a call lacking WRITE_DAC "succeeds but the ACLs are not preserved". Passing 0x2 therefore let a --force overwrite silently publish the temp file's inherited directory DACL over a restricted specialist and expose its system prompt. Pass no flags at all. - With no backup file, ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) and ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leave the destination deleted (1176) or renamed to an unreachable name (1177) while the replacement survives only under its temporary name. Returning those unrepaired let the caller's deferred temp-file cleanup remove the sole surviving copy, so one failed 'specialist create --force' destroyed both the old manifest and the new one. recoverPartialReplace now moves the replacement into place and still reports the error, since the destination's descriptor died with the destination. Also drops the doc claim that REPLACEFILE_WRITE_THROUGH (0x1) flushes before returning: Microsoft documents that value as "not supported". The two fsutil regression tests are verified to fail against the pre-fix code. The specialist-side test guards the caller half of the recovery contract and does not fail pre-fix; its comment says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed [P1] Do not suppress the DACL merge failure ( Confirmed exactly as described. My comment claimed
So omitting only Bonus, from the same doc page: On covering the denied-merge path: I could not build an honest live test for it. Windows grants an object's owner [P1] Recover the documented partial ReplaceFileW failure ( Confirmed. For 1176 with no backup, the docs say "the replaced file no longer exists and the replacement file exists under its original name" — so returning it unrepaired let I also handled 1177, which the finding didn't name but has the same shape: the replaced file "still exists with a different name" — a name only
Tests:
Validation:
@jatmn ready for another look — particularly on whether returning an error after a successful recovery is the behavior you want. The alternative is reporting success, but that would hide the lost descriptor, so I chose the noisier option. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/fsutil/replace_windows.go`:
- Around line 109-152: Update ReplaceFileW and recoverPartialReplace in
internal/fsutil/replace_windows.go to provide and track a managed backup path,
especially for ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177), so any renamed
original is known and removed after publishing the replacement or during
recovery failure; preserve error reporting and avoid orphaned content. In
internal/fsutil/replace_windows_test.go lines 166-212, update the affected tests
to cover backup cleanup and recovery behavior for 1177.
🪄 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 Plus
Run ID: bf8644c1-48e4-4ec1-82a8-087cd07f6f49
📒 Files selected for processing (3)
internal/fsutil/replace_windows.gointernal/fsutil/replace_windows_test.gointernal/specialist/storage_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/specialist/storage_test.go
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] Do not fall back to a non-atomic rename when ReplaceFileW is unsupported
internal/fsutil/replace_windows.go:100
ERROR_INVALID_FUNCTIONandERROR_NOT_SUPPORTEDtake the plainos.Renamepath, which is precisely the non-atomic Windows replacement the change is intended to eliminate. Aspecialist create --forceon one of those volumes can again expose a missing/intermediate manifest; it also publishes the temp file's inherited DACL on an ACL-capable redirector rather than the restricted destination DACL. Return the unsupported error for an existing destination, or use a replacement operation that supplies the promised atomic and descriptor-preserving semantics. -
[P1] Preserve the temporary replacement when partial-failure recovery cannot rename it
internal/fsutil/replace_windows.go:149
After 1176/1177, the old destination is gone andsrcis the only new copy. If this recoveryos.Renamefails (for example, a transient share lock), the error is returned with the recovery error only interpolated, soReplaceWithRetrydoes not retry it;writeSpecialistAtomicWiththen unconditionally removestempPathin its deferred cleanup. The failed--forceoperation therefore deletes both the old manifest and the only surviving new one. Retry/preserve the recovery source, or make the caller retain it when recovery fails. -
[P2] Track and clean up the original file in the 1177 recovery path
internal/fsutil/replace_windows.go:89
ERROR_UNABLE_TO_MOVE_REPLACEMENT_2leaves the replacement atsrcand the original at a different name. Because this passes a null backup name, that original name is unobservable; recovery only movessrctodst, leaving an orphaned old specialist (including its prior system prompt) behind after a failed overwrite. Supply a managed sibling backup path and define cleanup/rollback for it; the current test fabricates only an absent destination and cannot exercise the retained original.
Use a managed ReplaceFileW backup so partial failures can restore the original file and DACL. Fail closed when atomic replacement is unsupported, retry rollback and backup cleanup, and retain a known backup if rollback cannot complete.\n\nCover unsupported volumes, 1176/1177 semantics, cleanup, rollback retries, and symlink rejection. Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-be8e-75eb-b274-b7e22af373a0 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Final follow-up on current head
The PR is mergeable. PierrunoYT requests fresh reviews to supersede the prior |
…c-specialist-overwrite
|
Follow-up on head Fresh review auditI re-read every submitted review and every inline thread against the current code. There are now no unresolved review threads, and CodeRabbit approved the new head with no actionable comments. Current behavior covers every still-valid request:
The three previously unresolved threads were outdated, not skipped as invalid: the old plain-rename retry finding is superseded by Validation on
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Rechecked head f15f8dc against the earlier P1 round. The core overwrite work looks solid: existing destinations publish through ReplaceFileW with flags 0, unsupported volumes fail closed, managed backup covers 1176/1177, and DACL preservation is tested at both the fsutil and Create --force layers. What remains is mostly post-success caller contract hygiene and catastrophic partial-failure polish.
Findings
-
[P2] Successful overwrite is reported as failure when backup cleanup fails
internal/fsutil/replace_windows.go:171-176andinternal/specialist/storage.go:138-139
AfterReplaceFileWsucceeds,cleanupReplaceBackupreturns a hard error ifremoveReplaceBackupexhausts its retries ("replacement succeeded, but backup … could not be removed"). That is intentional soReplaceWithRetrydoes not rerun the replace, butwriteSpecialistAtomicWithstill surfaces it asreplace specialist file: …, soStorage.Create(..., Overwrite: true)returns(Manifest{}, err)even though the destination already holds the new manifest. A leftover.zero-replace-*.backupfile in the specialist directory can also retain the pre-overwrite body until manual removal. Please treat a committed replace as success (surface backup cleanup separately or best-effort) so--forcedoes not false-fail after publish. -
[P2] Directory sync failure after a successful replace reports failure while the file is already updated
internal/specialist/storage.go:138-143
On non-Windows platforms,fsutil.ReplaceWithRetryruns beforesyncSpecialistDir. If the parent-directory sync fails,writeSpecialistAtomicWithreturnssync specialist directory: …andCreatereturns(Manifest{}, err)with no rollback, even thoughpathalready contains the new bytes. This is a new failure mode introduced by the post-rename directoryfsync; the old truncate path did not have it. Please align with the sessions store pattern: best-effort directory sync after a successful replace, or return success with a distinct cleanup warning rather than a full overwrite failure. -
[P2]
syncSpecialistDirspuriously fails after a successful replace when the directory cannot be opened
internal/specialist/storage.go:151-153
After a successful replace,syncSpecialistDirreturns anyos.Open(dir)error. The sharedsyncDirhelper ininternal/sessions/store.go:934-936treats an open failure as best-effort (return nil), so specialist overwrite is stricter than other atomic-write paths and can return a full failure solely because the directory handle could not be opened after the manifest was already published. Please match the sessions best-effort behavior or fold this into the post-success hygiene fix above. -
[P3] Catastrophic 1177 rollback failure leaves the specialist off its canonical path
internal/fsutil/replace_windows.go:214-218,internal/specialist/storage.go:106-109, andinternal/specialist/manifest.go:479-480
IfReplaceFileWreturnsERROR_UNABLE_TO_MOVE_REPLACEMENT_2and rollback cannot move the managed backup back todst, recovery preserves the original at the opaque.zero-replace-*.backuppath anddstis absent.loadDirectoryonly reads*.md, so the specialist is invisible to normal load/list until manual restore.writeSpecialistAtomicWith’s deferredos.Remove(tempPath)also deletes the attempted new bytes on this path, so the error text that the replacement remains atsrcis misleading. This is an extreme edge case and the old content is not lost, but consider a specialist-layer integration test and clearer operator guidance (or an automatic restore attempt) for this terminal state. -
[P3] Residual symlink TOCTOU between the check and
ReplaceFileW
internal/specialist/storage.go:124-138andinternal/fsutil/replace_windows.go:78-94
Symlink rejection is still a point-in-timeos.LstatbeforeprepareReplaceBackupandReplaceFileW. A concurrent same-user actor could swap the destination in that window. This is much better than the pre-PRLstat+O_TRUNCpath andreplaceExistingWithre-checks at publish time, but it is the same audit class as issue #753. Optional hardening if you want TOCTOU-free symlink defense; not a regression on the P1 atomicity/DACL fixes.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the committed-overwrite result semantics in
Validation: affected CLI, specialist, and fsutil suites pass with |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P3] Align 1177 rollback-failure messaging with what the caller actually leaves on disk
internal/fsutil/replace_windows.go:209-222andinternal/specialist/storage.go:117-120
On failedERROR_UNABLE_TO_MOVE_REPLACEMENT_2rollback,recoverPartialReplacereports that the replacementremains atthe temp path for caller cleanup, butwriteSpecialistAtomicWithalways runsos.Remove(tempPath)in its deferred cleanup before the error returns. The old manifest is not lost when the managed backup exists, but the error text is misleading for anyone trying to recover from the failure. Please either skip temp removal on these recovery paths or stop claiming the replacement survives. -
[P3] Document the 1177 rollback-failure terminal state for operators
internal/fsutil/replace_windows.go:216-220,internal/specialist/manifest.go:479, andinternal/specialist/storage.go:80-86
IfReplaceFileWreturns 1177 and restoring the managed backup todstexhausts its retries, the original survives only at the opaque.zero-replace-*.backuppath whiledststays absent.loadDirectoryingests only*.md, so the specialist disappears from normal list/load until manual restore. This is the same extreme edge case from the prior review; the hardening is much better than before, but a short operator note in the error text or docs would help.
Optional follow-ups (not merge blockers)
- GenerateSpecialist
MetavsOutput: warnings land in textOutput(matching the PR claim) but not inMeta. Only matters if a consumer readsMetaalone; no currenttools.Resultwarnings field exists. - Orphan
.zero-replace-*.backupsiblings: repeated cleanup failures can leave multiple backups; each failure already warns once. A sweep on later writes would be nice hygiene. - Unix mode on
--force: atomic replace publishes a new0600inode, whereas the old in-placeO_TRUNCpath preserved an existing mode on POSIX. The PR description and tests already expect0600; this only affects manually widened permissions.
…hind The Windows partial-replace recovery errors told an operator the replacement "remains at" the caller's temporary path, but the only caller removes that file in a deferred cleanup before the error ever surfaces — it named a file that was already gone. Recovery from a failed overwrite is always about the original, so the errors now describe only what the replace itself left on disk: the state of the destination and of the managed backup. The replacement path is no longer a parameter, since nothing reports it. The one terminal state that needs a human — 1177 whose rollback exhausted its retries — now says so outright. The original then exists only under an opaque .zero-replace-*.backup name, and the loader reads *.md, so the specialist stays invisible until somebody moves it back. The error names both paths and the action; docs/SPECIALISTS.md walks through it; and loadDirectory records why the non-.md siblings an interrupted overwrite can leave are skipped rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed both findings in [P3] Rollback-failure messaging now matches what is actually on disk. I took the second of your two options — stopped claiming the replacement survives — because the temporary file is the wrong thing to point an operator at either way: it holds the content they were trying to write, which re-running the command reproduces, while the original is the only irreplaceable copy. [P3] The 1177 rollback-failure terminal state is spelled out. The error now reads:
Three places back it up:
I left the three optional follow-ups alone as you flagged them non-blocking. On the Unix Validation
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/SPECIALISTS.md`:
- Around line 144-152: Update the Windows overwrite documentation near the
ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) description to also cover error 1176,
including Zero’s recovery behavior and whether users need to take action. Ensure
the documented outcome matches the replacement contract for partial replacement
failures.
- Around line 162-165: Update the recovery instruction in the specialist
documentation to explicitly state that Zero’s error message includes both the
backup path and the destination path, replacing the ambiguous wording while
preserving the existing lock-closing and rename guidance.
In `@internal/specialist/manifest.go`:
- Around line 479-486: Update the explanatory comment near the specialist
manifest filtering logic to say “the two kinds of sibling files an interrupted
overwrite can leave,” correcting the malformed wording while preserving the
existing explanation about ignored recovery artifacts.
🪄 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 Plus
Run ID: f44afbc9-8ca2-47ed-b496-3d4ef68513a8
📒 Files selected for processing (5)
docs/SPECIALISTS.mdinternal/fsutil/replace_windows.gointernal/fsutil/replace_windows_test.gointernal/specialist/manifest.gointernal/specialist/storage.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/fsutil/replace_windows.go
- internal/specialist/storage.go
- internal/fsutil/replace_windows_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for this, and sorry it sat a while. The Unix half is a genuine fix: temp file plus rename(2) really does close the torn-read window from #753. I am requesting changes because the Windows half does not do what the PR says it does, and the PR says it in six places including the docs.
ReplaceFileW is not atomic for an observer
ReplaceFileW renames the destination aside, renames the replacement in, then deletes the original. Between the first two steps the destination path does not exist. I measured the bare primitive on Windows 11 / NTFS, no code from this PR involved, one writer and one observer:
ReplaceFileW: publishes=2017 failures=0 | observer stats=188450 ABSENT=13996 (7.4%)
os.Rename : publishes=8806 | observer stats=144857 ABSENT=984 (0.68%)
Eleven times the exposure of a plain rename, with every publish succeeding. lpBackupFileName was NULL, so this is not about the managed backup this PR adds.
The MSDN line the comments lean on says ReplaceFile "combines several steps within a single function" and then enumerates those steps. A single function, not a single operation.
Why it matters here rather than being academic
loadDirectory (internal/specialist/manifest.go:465) enumerates with os.ReadDir before reading each *.md. During the window the entry is simply absent from the listing, so the specialist drops out of the load result with no warning at all rather than reaching the "skipped unreadable specialist manifest" path at :497. There is no lock anywhere in internal/specialist, and generate_tool.go writes specialists mid-session, so a single process can hit this against itself.
A crash inside the window leaves no *.md at the destination at all; the original survives only as .zero-replace-*.backup.
What I would like
- Correct the atomicity claims at
internal/fsutil/replace_windows.go:28-29and:59,internal/fsutil/rename.go:34, andinternal/specialist/storage.go:153. - Fix
docs/SPECIALISTS.md:144-146. It states the Windows safety property as holding on the ordinary success path, which is where it fails. A wrong guarantee in docs is worse than no guarantee. - Either accept the window and say so plainly, or use
MoveFileExwithMOVEFILE_REPLACE_EXISTING, which is whatos.Renamealready does and what the control above measures.
Two smaller things
TestStorageCreateForceAtomicallyReplacesFile (internal/specialist/storage_test.go:99) passes verbatim against the fully reverted pre-PR code at the merge base, so on Windows it does not discriminate against the bug it exists for. It asserts final content, final mode and absence of temp files, none of which the old in-place write violated.
On Unix, --force discards the destination's mode: storage.go:112 creates via os.CreateTemp and :128 hard-sets 0600, and the Lstat at :141 feeds only the symlink check. A specialist file the user had chmod'd comes back 0600.
Happy to look again quickly once the Windows story is either fixed or described accurately. The Unix fix is worth landing either way.
Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <amp@ampcode.com>
|
PierrunoYT addressed the latest Windows atomicity review in The maintainer's measurement is valid: Changes:
Validation:
Ready for CI and maintainer re-review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_test.go`:
- Around line 134-206: Run TestStorageCreateForceSerializesConcurrentLoad with
Go’s race detector and CGO enabled, using the repository’s standard test command
or an equivalent targeted command. Verify the concurrent specialistFilesMu
regression test passes without race reports before merging.
🪄 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 Plus
Run ID: a05f4fc5-4d20-4f63-9252-6934770b23d0
📒 Files selected for processing (8)
docs/SPECIALISTS.mdinternal/fsutil/rename.gointernal/fsutil/replace_windows.gointernal/fsutil/replace_windows_test.gointernal/specialist/manifest.gointernal/specialist/storage.gointernal/specialist/storage_dacl_windows_test.gointernal/specialist/storage_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/SPECIALISTS.md
- internal/fsutil/rename.go
- internal/specialist/storage_dacl_windows_test.go
- internal/fsutil/replace_windows.go
- internal/specialist/storage.go
- internal/fsutil/replace_windows_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. You went further than I asked and the result is better than what my review implied.
I asked you to either fix the Windows window or describe it accurately. Serializing in-process loads against mutations is the third answer and the right one: ReplaceFileW cannot be made observer-atomic, so closing the gap for the case Zero controls and saying plainly that external editors are not covered is honest in a way "we fixed it" would not have been. The docs now say exactly that, and dropping "atomic" from the names and comments means nobody re-derives the old claim from the code.
Mutation checked the part that matters: removing the RLock in Load fails TestStorageCreateForceSerializesConcurrentLoad with the loader observing the replacement gap. That is a real guarantee now, not a described one.
The Unix mode preservation and the symlink refusal I did not ask for are both good. I could not verify the mode assertion on this machine, since the production code and the test are both gated on non-Windows, so treat that one as reviewed by reading only.
Renaming TestStorageCreateForceAtomicallyReplacesFile was the right call too. It was the test whose name promised more than it checked.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. You went further than I asked and the result is better than what my review implied.
I asked you to either fix the Windows window or describe it accurately. Serializing in-process loads against mutations is the third answer and the right one: ReplaceFileW cannot be made observer-atomic, so closing the gap for the case Zero controls and saying plainly that external editors are not covered is honest in a way "we fixed it" would not have been. The docs now say exactly that, and dropping "atomic" from the names and comments means nobody re-derives the old claim from the code.
Mutation checked the part that matters: removing the RLock in Load fails TestStorageCreateForceSerializesConcurrentLoad with the loader observing the replacement gap. That is a real guarantee now, not a described one.
The Unix mode preservation and the symlink refusal I did not ask for are both good. I could not verify the mode assertion on this machine, since the production code and the test are both gated on non-Windows, so treat that one as reviewed by reading only.
Renaming TestStorageCreateForceAtomicallyReplacesFile was the right call too. It was the test whose name promised more than it checked.
|
@jatmn your approval here was dismissed by @PierrunoYT's fix push, not by anything you reviewed changing shape. This is at two of three now, so it needs you back plus one more. Worth knowing what moved since you looked. The Windows publish is no longer described as atomic anywhere, because it is not: Also in: Unix mode preservation on @kevincodex1 not ready for you yet, flagging so you can skip it until the approvals land. |
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
stderrfor human output and includes them in JSON output.Tests
Documentation
--forceoverwrites.