mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 13:11:31 -10:00
* fix(specialist): make overwrites atomic * fix(specialist): harden atomic replacement * fix(specialist): replace through a Windows-atomic, DACL-preserving primitive Addresses both P1 findings on #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. * fix(fsutil): stop ignoring ACL merge errors, recover partial ReplaceFileW Addresses jatmn's two #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) <[email protected]> * fix(fsutil): preserve Windows replacement backups 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 <[email protected]> * fix(specialist): report committed cleanup warnings Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * docs(specialist): describe what a failed overwrite actually leaves behind 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) <[email protected]> * fix(specialist): serialize managed loads during replacement Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Amp <[email protected]>
238 lines
9.6 KiB
Go
238 lines
9.6 KiB
Go
//go:build windows
|
|
|
|
package fsutil
|
|
|
|
import (
|
|
"errors"
|
|
"fmt"
|
|
"os"
|
|
"path/filepath"
|
|
"syscall"
|
|
"time"
|
|
"unsafe"
|
|
)
|
|
|
|
// replaceFileFlags is deliberately ZERO. Every REPLACEFILE_* flag ReplaceFileW
|
|
// accepts either defeats the reason this function exists or does nothing:
|
|
//
|
|
// - REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) and REPLACEFILE_IGNORE_ACL_ERRORS
|
|
// (0x4). Microsoft documents BOTH with the same consequence: "if you specify
|
|
// this flag and do not have WRITE_DAC access, the function succeeds but the
|
|
// ACLs are not preserved." A silent success that publishes the temporary
|
|
// file's inherited directory DACL over an explicitly restricted specialist —
|
|
// exposing its system prompt — is precisely the failure this function exists
|
|
// to prevent, so a merge failure MUST surface as an error and leave the
|
|
// destination untouched. Passing 0x2 while omitting only 0x4 buys nothing:
|
|
// ACL merging is part of the metadata merge 0x2 covers.
|
|
// - REPLACEFILE_WRITE_THROUGH (0x1) is documented as "This value is not
|
|
// supported", so it cannot be relied on to flush anything.
|
|
const replaceFileFlags = 0
|
|
|
|
const replaceBackupPattern = ".zero-replace-*.backup"
|
|
|
|
const (
|
|
// Returned when the volume or redirector cannot provide ReplaceFileW's
|
|
// DACL-preserving semantics. Existing destinations must fail closed.
|
|
errorInvalidFunction = syscall.Errno(1)
|
|
errorNotSupported = syscall.Errno(50)
|
|
|
|
// Partial-failure codes: ReplaceFileW got far enough to move or delete
|
|
// something, so the on-disk state needs repair rather than a bare error. See
|
|
// recoverPartialReplace.
|
|
errorUnableToRemoveReplaced = syscall.Errno(1175)
|
|
errorUnableToMoveReplacement = syscall.Errno(1176)
|
|
errorUnableToMoveReplacement2 = syscall.Errno(1177)
|
|
)
|
|
|
|
var (
|
|
replaceKernel32 = syscall.NewLazyDLL("kernel32.dll")
|
|
replaceProcReplaceFil = replaceKernel32.NewProc("ReplaceFileW")
|
|
)
|
|
|
|
// replaceExisting publishes src over dst with ReplaceFileW rather than
|
|
// MoveFileEx (what os.Rename uses) to preserve destination metadata. The
|
|
// replacement is a freshly created temporary file, so it carries the directory's
|
|
// inherited DACL. Renaming it over the destination would therefore replace the
|
|
// destination's ACL - silently widening access to a file that had been restricted
|
|
// explicitly (os.File.Chmod cannot express that on Windows; Go only maps the
|
|
// owner-write bit). ReplaceFileW carries the replaced file's DACL and selected
|
|
// metadata over to the replacement instead.
|
|
//
|
|
// ReplaceFileW combines multiple filesystem steps and is not observer-atomic: an
|
|
// external reader can briefly see dst absent even when replacement succeeds.
|
|
// Callers that cannot tolerate that window must synchronize their own readers.
|
|
//
|
|
// No REPLACEFILE_* flag is passed at all — see replaceFileFlags for why each one
|
|
// would either silently lose the descriptor this function exists to preserve or
|
|
// do nothing. A merge failure therefore surfaces as an error, leaving the
|
|
// destination untouched and the caller free to clean up its temporary file,
|
|
// except in the partial-failure states recoverPartialReplace repairs.
|
|
func replaceExisting(src, dst string) error {
|
|
return replaceExistingWith(src, dst, callReplaceFile, nil)
|
|
}
|
|
|
|
func replaceExistingWith(src, dst string, replace func(string, string, string) error, restore func(string, string) error) error {
|
|
return replaceExistingWithCleanup(src, dst, replace, restore, removeReplaceBackup)
|
|
}
|
|
|
|
func replaceExistingWithCleanup(src, dst string, replace func(string, string, string) error, restore func(string, string) error, removeBackup func(string) error) error {
|
|
info, err := os.Lstat(dst)
|
|
if err != nil {
|
|
if os.IsNotExist(err) {
|
|
// Nothing to replace and no descriptor to preserve.
|
|
return os.Rename(src, dst)
|
|
}
|
|
return err
|
|
}
|
|
if info.Mode()&os.ModeSymlink != 0 {
|
|
return fmt.Errorf("refusing to replace symlink destination: %s", dst)
|
|
}
|
|
|
|
backup, err := prepareReplaceBackup(dst)
|
|
if err != nil {
|
|
return fmt.Errorf("prepare replacement backup: %w", err)
|
|
}
|
|
callErr := replace(dst, src, backup)
|
|
if callErr == nil {
|
|
if err := removeBackup(backup); err != nil {
|
|
return &CommittedReplacementCleanupError{BackupPath: backup, Cause: err}
|
|
}
|
|
return nil
|
|
}
|
|
if errors.Is(callErr, errorInvalidFunction) || errors.Is(callErr, errorNotSupported) {
|
|
callErr = fmt.Errorf("DACL-preserving replacement is not supported for %s: %w", dst, callErr)
|
|
}
|
|
return recoverPartialReplace(callErr, dst, backup, restore)
|
|
}
|
|
|
|
func callReplaceFile(replacedPath, replacementPath, backupPath string) error {
|
|
replaced, err := syscall.UTF16PtrFromString(replacedPath)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
replacement, err := syscall.UTF16PtrFromString(replacementPath)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
backup, err := syscall.UTF16PtrFromString(backupPath)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
result, _, callErr := replaceProcReplaceFil.Call(
|
|
uintptr(unsafe.Pointer(replaced)),
|
|
uintptr(unsafe.Pointer(replacement)),
|
|
uintptr(unsafe.Pointer(backup)),
|
|
uintptr(replaceFileFlags),
|
|
0,
|
|
0,
|
|
)
|
|
if result != 0 {
|
|
return nil
|
|
}
|
|
if callErr == nil || errors.Is(callErr, syscall.Errno(0)) {
|
|
return fmt.Errorf("replace %s: ReplaceFileW failed", replacedPath)
|
|
}
|
|
return callErr
|
|
}
|
|
|
|
func prepareReplaceBackup(dst string) (string, error) {
|
|
file, err := os.CreateTemp(filepath.Dir(dst), replaceBackupPattern)
|
|
if err != nil {
|
|
return "", err
|
|
}
|
|
path := file.Name()
|
|
if err := file.Close(); err != nil {
|
|
cleanupErr := removeReplaceBackup(path)
|
|
return "", fmt.Errorf("close backup placeholder %s: %w (cleanup error: %v)", path, err, cleanupErr)
|
|
}
|
|
if err := removeReplaceBackup(path); err != nil {
|
|
return "", fmt.Errorf("release backup path %s: %w", path, err)
|
|
}
|
|
return path, nil
|
|
}
|
|
|
|
func removeReplaceBackup(path string) error {
|
|
var err error
|
|
for i := 0; i < 10; i++ {
|
|
err = os.Remove(path)
|
|
if err == nil || os.IsNotExist(err) {
|
|
return nil
|
|
}
|
|
if os.IsPermission(err) {
|
|
// ReplaceFileW can leave the original's read-only attribute on the
|
|
// backup. Chmod clears that bit on Windows without changing its DACL.
|
|
_ = os.Chmod(path, 0o600)
|
|
}
|
|
if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) {
|
|
time.Sleep(10 * time.Millisecond)
|
|
continue
|
|
}
|
|
break
|
|
}
|
|
return err
|
|
}
|
|
|
|
func cleanupReplaceBackup(callErr error, backup string) error {
|
|
if err := removeReplaceBackup(backup); err != nil {
|
|
return fmt.Errorf("%w (backup %s could not be removed: %v)", callErr, backup, err)
|
|
}
|
|
return callErr
|
|
}
|
|
|
|
// recoverPartialReplace restores the original after ReplaceFileW reports a
|
|
// partial failure, then returns the original error. Supplying a managed backup
|
|
// changes the dangerous failure states documented by Microsoft:
|
|
//
|
|
// - ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) leaves both files under their
|
|
// original names, so the failed write can discard src normally.
|
|
// - ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leaves src in place and moves the
|
|
// original destination to backup. Moving backup back to dst rolls the failed
|
|
// overwrite back without losing the original content or its DACL.
|
|
//
|
|
// The replacement is left alone either way — nothing here deletes what it was
|
|
// handed — but the errors below deliberately say nothing about where it ended up,
|
|
// which is also why its path is not a parameter. Whether it still exists when the
|
|
// error surfaces is the caller's business: the specialist writer, the only caller
|
|
// today, removes its temporary file in a deferred cleanup, so an error promising
|
|
// a replacement "remains at" that path would send an operator looking for a file
|
|
// that was already gone. What these errors do describe is what this function
|
|
// itself left on disk — the state of dst, and of the managed backup holding the
|
|
// original.
|
|
func recoverPartialReplace(callErr error, dst, backup string, restore func(string, string) error) error {
|
|
if !errors.Is(callErr, errorUnableToMoveReplacement2) {
|
|
// For 1175, 1176, unsupported volumes, and ordinary failures, Windows
|
|
// documents that dst remains at its original name. Remove any redundant
|
|
// backup only after confirming that the destination still exists.
|
|
if _, err := os.Lstat(dst); err == nil {
|
|
return cleanupReplaceBackup(callErr, backup)
|
|
} else if !os.IsNotExist(err) {
|
|
return fmt.Errorf("replace %s: %w (inspect destination during recovery: %v)", dst, callErr, err)
|
|
}
|
|
}
|
|
|
|
if _, err := os.Lstat(backup); err != nil {
|
|
return fmt.Errorf("replace %s: %w (original backup expected at %s is unavailable: %v)", dst, callErr, backup, err)
|
|
}
|
|
if _, err := os.Lstat(dst); err == nil {
|
|
return fmt.Errorf("replace %s: %w (destination unexpectedly exists; the original also remains at backup %s)", dst, callErr, backup)
|
|
} else if !os.IsNotExist(err) {
|
|
return fmt.Errorf("replace %s: %w (inspect destination before restoring backup %s: %v)", dst, callErr, backup, err)
|
|
}
|
|
if err := RenameWithRetry(backup, dst, restore); err != nil {
|
|
// Only callErr is wrapped. Exposing a sharing violation from the exhausted
|
|
// rollback would make the outer ReplaceWithRetry call retry after the
|
|
// filesystem was already mutated.
|
|
//
|
|
// This is the one terminal state that needs a human: nothing is at dst, and
|
|
// the original's only copy is under a name no tool looks for (the specialist
|
|
// loader reads *.md and skips everything else), so it is invisible until
|
|
// somebody moves it back. Say that outright rather than leaving an operator
|
|
// to infer it from a bare error code.
|
|
return fmt.Errorf(
|
|
"replace %s: %w (rolling the original back failed: %v; %s no longer exists and the original survives only as %s, which must be moved back by hand to restore it)",
|
|
dst, callErr, err, dst, backup,
|
|
)
|
|
}
|
|
return fmt.Errorf("replace %s: %w (the original was restored)", dst, callErr)
|
|
}
|