fs/local: Write is not atomic and discards the target file mode

[exe-dev-github-integration]

local.FS.Write writes directly to the destination path:

// fs/local/localfs.go (main)
dst, err := os.OpenFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, filePerm)
if err != nil { ... }
n, err := io.Copy(dst, src)

with filePerm = 0644. Three defects follow from those two lines.

1. A failed write destroys the previous file

O_TRUNC empties the target before the first byte of new content is written. If io.Copy fails, the context is canceled, the source reader errors, or the process dies mid-write, name is left truncated or partially written — and the complete file that was there is gone. There is no window in which a reader sees the old contents; it sees a corrupt file.

This matters more than usual here: OCFL's whole premise is that content files are immutable once written and that an inventory can be trusted to describe what is on disk. A half-written inventory.json is a broken object root, and the failure is silent.

2. Overwriting discards the target's mode

filePerm is hardcoded. Writing to an existing file with mode 0600 yields 0644 — a private file becomes world-readable with no diagnostic.

3. Cancellation is not observed

io.Copy(dst, src) never consults ctx. A canceled context is noticed only if the source reader independently returns an error, so a long write to a slow source keeps running after its caller has given up.

---

Plan

Step 0 — what is already in place

#162 landed as PR #170, as fs/internal/imptest (not internal/testutil). Two subtests there are written out in full and skipped waiting on this issue:

fs/internal/imptest/writefs.go:90   t.Skip("local Write is not atomic; see #163")   // source error leaves previous content intact
fs/internal/imptest/writefs.go:108  t.Skip("local Write is not atomic; see #163")   // source error on a new file leaves nothing behind

Deleting those two lines is the acceptance test for this issue. fs/internal/imptest/imptest.go:62 lists this as one of three outstanding skips and should lose its mention of #163 too. fs/local/imptest_test.go already runs the suite against the local backend, so no wiring is needed.

Step 1 — temp file, copy, rename

Restructure FS.Write into the sequence the reference implementation uses (e7d6ca3):

1. createTempFile(parent, fullPath) — O_CREATE|O_EXCL|O_WRONLY at mode 0666 (umask applies, matching os.Create), retrying on the practically-impossible name collision. O_EXCL means it can never clobber an existing file.
2. Preserve the target's mode. os.Lstat(fullPath); only when info.Mode().IsRegular(), tmp.Chmod(info.Mode().Perm()) — chmod is not umask-masked, so the mode is copied exactly. Track this with an explicit havePerm bool, not a mode != 0 sentinel, or a target legitimately at 0000 silently gets the default.
   Write it this way the first time. The reference reached it by landing os.Stat (b85ed2d) and fixing it in fea16d5: os.Stat reads the referent's mode, and a symlink's own mode is 0777 on POSIX, so either value on hand is wrong for the regular file being created. IsRegular() rejects both, plus directories and devices, in one condition.
3. Copy through a context-aware reader.
4. tmp.Sync(), then tmp.Close(). Close must precede the rename — Windows cannot move a file that is still open.
5. Rename over the target (Step 3).
6. Best-effort fsync of the containing directory.

A defer closes and removes the temp file on every path before the rename commits, so a failed or canceled write leaves no litter.

Side effect worth recording: this also closes item 1 of #164. O_CREATE|O_TRUNC on fullPath is what follows a symlink at name and writes through to its referent; once Write never opens fullPath, the rename replaces the link entry and the referent is untouched. #164's remaining scope is RemoveAll via os.Root, plus the regression tests that pin both halves.

Step 2 — the context-aware reader must hide io.WriterTo

type ctxReader struct {
    ctx context.Context
    src io.Reader
}

func (r ctxReader) Read(p []byte) (int, error) {
    if err := r.ctx.Err(); err != nil {
        return 0, err
    }
    return r.src.Read(p)
}

Exposing only Read is the point. If the wrapper forwarded the underlying reader's io.WriterTo (as strings.Reader, bytes.Reader and *os.File all have), io.Copy and os.File.ReadFrom would take a fast path that never calls Read and never checks the context. The reference used a hand-rolled copyWithContext chunk loop instead; either shape works, but the wrapper composes with io.Copy's buffer reuse and is less code. Whichever ships, a test must pass a *strings.Reader specifically — that is the type whose fast path defeats a naive wrapper.

Also: when the copy fails and ctx.Err() != nil, report the context error rather than the underlying read error, so callers can match context.Canceled / context.DeadlineExceeded.

Step 3 — Windows: use robustio, and drop the MoveFileEx helper

The premise behind rename_windows.go (3a1ad19) is false, and the helper should not be ported. Its doc comment says "os.Rename cannot replace an existing destination file on Windows". Verified against the go1.25 toolchain in this environment:

os/file_windows.go:221                            e := windows.Rename(fixLongPath(oldname), fixLongPath(newname))
internal/syscall/windows/syscall_windows.go:364   return MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING)

os.Rename on Windows is MoveFileEx(..., MOVEFILE_REPLACE_EXISTING). 2041a91's commit message already noticed this ("os.Rename there already maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING)") but kept the helper anyway. So the helper is strictly worse than the standard library on two counts:

• Its first strategy duplicates os.Rename minus fixLongPath, which os.rename applies and the helper drops. Long OCFL content paths past MAX_PATH are exactly the case Step 4's NAME_MAX truncation exists for, and the helper regresses them.
• Its os.Remove + os.Rename fallback is a real downgrade of the guarantee this issue exists to establish: a failure between the two leaves no file at the target. It trades away atomicity to work around a problem that does not exist.

The Windows failure this code actually needs to survive is different: after tmp.Close(), a virus scanner or the Search Indexer can hold a transient handle to the just-closed temp file, and the rename fails with ERROR_SHARING_VIOLATION or ERROR_ACCESS_DENIED. That is what robustio is for — a bounded retry that keeps the atomic replace instead of degrading it:

// fs/local/localfs.go — no build tags, no platform files
import "github.com/rogpeppe/go-internal/robustio"

if err := robustio.Rename(tmpPath, fullPath); err != nil { ... }

robustio.Rename wraps os.Rename and retries ERROR_ACCESS_DENIED / ERROR_FILE_NOT_FOUND / ERROR_SHARING_VIOLATION with randomized backoff up to ~2s. On !windows && !darwin (robustio_other.go) it is a plain passthrough to os.Rename, so Linux behavior is bit-for-bit unchanged.

Note that golang.org/x/tools/internal/robustio and cmd/go/internal/robustio are both internal/ and cannot be imported. github.com/rogpeppe/go-internal/robustio is the same code, published at the top level of that module. Cost, measured against this repo's go.mod:

go.mod   +1 line: github.com/rogpeppe/go-internal v1.16.0
go.sum   +2 lines

Zero transitive modules — go mod tidy pulls in nothing else; robustio's only non-stdlib import is a sibling internal/syscall/windows package inside the same module. BSD-3 (Go Authors), compatible with this repo's MIT. It also means golang.org/x/sys stays an indirect dependency, where 3a1ad19 promoted it to direct, and it deletes ~110 lines of platform code plus its two test files from this repo before they are ever written here.

Two honest caveats:

• robustio retries on darwin too, on ENOENT, for up to ~2s. Here the rename source is a temp file this function just created and closed, so ENOENT means a genuine bug or an external cleanup; the cost is a delay before an error that was going to be returned regardless.
• robustio.retry sleeps without consulting a context. After the trouble Step 2 takes to make the copy cancellable, the final rename can still block ~2s past cancellation. This is acceptable: the copy is already complete at that point and the write is one syscall from committing — abandoning it there is worse than finishing it. Worth a one-line comment saying so.

If a new module dependency is unwanted, the fallback is to vendor the ~50 relevant lines as fs/local/internal/robustio with BSD-3 attribution retained. ERROR_SHARING_VIOLATION is not in stdlib syscall; define it as syscall.Errno(32) or take it from x/sys/windows. Recommendation: take the dependency — three lines of manifest against a maintained upstream is the better trade.

Step 4 — temp name must cut at a UTF-8 rune boundary

Carry 0ac1238 over. The name is "." + base + ".tmp-" + <16 hex>, so base must be capped at 255 - 22 = 233 bytes to stay inside NAME_MAX. Slicing raw bytes at 233 can split a multibyte rune, producing invalid UTF-8 that strict filesystems reject with a confusing PathError. Back the limit off to the nearest rune start with a utf8.RuneStart scan. Names long enough to hit this are legal per fs.ValidPath and ordinary in OCFL content paths.

Step 5 — document the guarantee where callers see it

• WriteFS.Write in fs/fs.go has no doc comment at all today (fs/fs.go:50), while Remove and RemoveAll both carry their contract. Add one: implementations should make the replacement atomic where the backend allows; a failed write must leave the previous contents intact and must not create a partial file; s3 gets this from object-upload semantics, local from temp-file-and-rename.
• local.FS.Write documents the concrete sequence, the cleanup guarantee, mode preservation and the symlink-entry replacement. It should not repeat 2041a91's "not atomic on Windows" disclaimer — with Step 3 there is no degraded path to disclaim.
• README: the Write guarantee is not mentioned anywhere in it today. Add it where the local backend is introduced.

Step 6 — tests and CI

Flip the two imptest skips (Step 0), then add to fs/local:

test	pins
new-file mode	0666 &^ umask, not a hardcoded 0644
overwrite preserves mode	write 0600, overwrite, still 0600
target at mode 0000	preserved — the case a mode != 0 sentinel loses
symlinked target	replacement is a regular file and is not 0777; referent untouched (!windows)
mid-copy cancel	old file intact, no partial, no temp left in the directory
context.DeadlineExceeded	surfaces as itself, not as a read error
*strings.Reader source cancel	the io.WriterTo fast path does not bypass the ctx check
reader error	temp file removed, previous content intact
large payload	multi-chunk copy across the buffer boundary
concurrent writers to one name	every observed read is one complete version, never a mix
tempFileName	short / sub-limit multibyte / long ASCII / >255-byte multibyte all land ≤255 bytes, valid UTF-8, creatable

CI runs ubuntu-latest only (.github/workflows/go.yml:10), so nothing here exercises Windows. Choosing robustio over a hand-written rename_windows.go largely removes the exposure — there is no untested platform-specific code left in this repo. Still add GOOS=windows go vet ./... as a cross-compile check; a windows-latest matrix entry would be better and is cheap.

Resolved open questions

Is the directory fsync worth its cost? Keep it. It is what makes the rename itself survive a machine crash, and the durability of a committed inventory is the whole point. Add a BenchmarkWriteMany in the PR so the per-file cost is on the record, and defer any opt-out knob until someone has a number to argue from — if it ever arrives it should be an option on NewFS, never a silent drop of durability.

The Windows fallback is not atomic — accept, document, or fail loudly? None of the three. Delete it (Step 3). robustio.Rename either completes the atomic replace or returns an error; there is no third state to document.

Should Write succeed when the target is not a regular file? Symlink: yes, replacing the link entry — that is #164's requirement and Step 1 satisfies it. Directory: let the rename fail and wrap the error in a PathError. Do not add an Lstat pre-check to reject it; that is a TOCTOU race that buys only a nicer error message.

Smaller ones for review: Write currently returns bytes-copied alongside a non-nil error even though nothing reached name — returning 0 on any pre-rename failure reads truer and could be pinned in imptest. And a crash between create and rename leaves a .<base>.tmp-<hex> file that an OCFL validator will flag as unexpected content; accept and document it (a crash mid-write leaves a broken object regardless) rather than filtering temp names out of DirEntries, which would hide real filesystem state from a validator.

Reference

Retained branch fs-fixes — one working implementation, not a specification. Note that 3a1ad19 and the Windows half of 2041a91 are superseded by Step 3, and b85ed2d/fea16d5 are a detour Step 1 skips.

git show e7d6ca3   # fs/local: atomic Write via temp file and rename
git show 0ac1238   # fs/local: cut tempFileName at UTF-8 rune boundary
git show 3a1ad19   # fs/local: add Windows rename-overwrite helper using MoveFileEx  (superseded)
git show 2041a91   # fs/local: wire Windows rename helper into Write                 (partly superseded)
git show fea16d5   # fs/local: don't stamp a symlink's 0777 onto the replacement file
git show 9417c97   # fs: document atomic write guarantee on WriteFS and local Write
git show 70238b9   # tests: large payload, cancel cleanup, reader concurrency, temp lifecycle
git show 20331de   # tests: ctx-cancel, deadline-exceeded, traversal rejection

Depends on #162 (landed, PR #170). Overlaps #164 (same function) — land this one first; Step 1 shrinks #164 to its RemoveAll half plus regression tests.

[exe-dev-github-integration]

Reviewed this plan against main (3dd99de), the fs-fixes reference branch, the go1.27 toolchain sources, and the robustio module. The plan is sound and unusually well-verified; the core correction in Step 3 is right and is the most valuable part. Confirming the load-bearing claims, then a few gaps.

Verified

• Current localfs.go does exactly what the issue quotes: O_CREATE|O_TRUNC|O_WRONLY at hardcoded 0644, no ctx check in the copy loop (fs/local/localfs.go:17,82).
• The two acceptance skips exist verbatim at fs/internal/imptest/writefs.go:90,108, and imptest.go:62 lists fs/local: Write is not atomic and discards the target file mode #163 as one of three outstanding skips.
• WriteFS.Write at fs/fs.go:50 has no doc comment while Remove/RemoveAll carry contracts.
• The rename_windows.go premise is false: os/file_windows.go:260 → internal/syscall/windows → MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING), byte-identical to the helper's strategy 1. The helper also drops fixLongPath (which os.rename applies), regressing >MAX_PATH OCFL content paths, and its Remove+Rename fallback destroys the atomicity this issue exists to create. Deleting rather than porting is correct.
• go get github.com/rogpeppe/go-internal/robustio@v1.16.0 adds exactly 1 go.mod line, 2 go.sum lines, zero transitive modules; robustio_other.go is a plain os.Rename passthrough on Linux. The cost accounting in the issue is accurate.
• The real unhandled Windows hazard is as described: a scanner/indexer holding a transient handle on the just-closed temp file → ERROR_SHARING_VIOLATION at rename, which plain os.Rename turns into a spurious failed write (data safe, but the write errors out). That is the right failure to defend against.
• robustio does retry on darwin on ENOENT (robustio_darwin.go) and retry sleeps without consulting ctx — both disclosed caveats are real.
• The e7d6ca3 reference does carry the preserveMode != 0 sentinel + os.Stat bugs that fea16d5 fixed; writing IsRegular() + havePerm correctly the first time skips that detour.

Gaps / nits to address in the PR

1. Toolchain floor. CI runs everything inside devcontainers/ci on ubuntu (.github/workflows/go.yml), and the devcontainer image pins the Go toolchain. The os.Rename = MoveFileEx claim holds for any modern Go (since ~go1.16), but the PR should state the assumed toolchain floor — it's the one premise that isn't verified by anything in CI today.

2. The strings.Reader fast-path rationale (Step 2) is slightly overstated but the conclusion is right. io.Copy checks io.WriterTo first, before ReaderFrom. So a ctxReader that wraps *strings.Reader and exposes only Read does force the copy through Read — but the reason the naive wrapper fails is specifically that forwarding WriterTo lets io.Copy skip the wrapper entirely. Worth phrasing precisely so the test's purpose (pass a *strings.Reader and prove the ctx check still fires) is clear.

3. robustio.Rename on Windows also retries ERROR_FILE_NOT_FOUND. That includes a genuinely missing destination on a first write. Harmless here — rename to a nonexistent dst succeeds, so ENOENT at rename means the source is gone — but the "retries file-not-found" caveat is narrower than the code actually is.

4. Umask interaction with havePerm + Chmod. Confirm the ported code chmods the temp file after the umask-affected create (it does — chmod is explicit, not umask-masked), matching fea16d5. Just call it out so the ordering doesn't regress.

5. Don't overclaim Windows atomicity in the docs (Step 5). os.Rename's own doc comment still says "on non-Unix platforms Rename is not an atomic operation," which is stale relative to the implementation. The plan asserts same-dir rename is atomic on Windows via MoveFileEx. If WriteFS.Write's new doc promises atomicity unqualified, that claim will get re-litigated. Soften to "atomic on POSIX; atomic replace on Windows via MoveFileEx" or similar rather than an unqualified guarantee.

Verdict: ship it, with #1 (state the toolchain floor) and #5 (don't overclaim Windows atomicity in docs) addressed in the PR.