fs/local: make Write atomic, preserve target mode, honor ctx - #171
Merged
Conversation
Write opened the destination with O_CREATE|O_TRUNC and copied into it at a
hardcoded 0644. Three defects followed: a failed or canceled copy left the
target truncated or partial with the previous complete file gone; overwriting
a 0600 file silently published it at 0644; and io.Copy never consulted ctx, so
a canceled write kept running.
Write now copies to a unique temp file (".<base>.tmp-<random>",
O_CREATE|O_EXCL|O_WRONLY) in the target's own directory, syncs it, closes it,
and renames it over the target. A reader of the target sees the old file or
the new file in full, never a partial one; every path before the rename
removes the temp file, so failures leave no litter. The containing directory
is fsynced best-effort afterwards so the rename itself survives a crash.
The target's mode is carried onto the replacement with chmod (not umask-
masked, so the mode is copied exactly), but only when Lstat reports a regular
file: for a symlink neither mode on hand describes the file being created —
the referent's belongs to a file that is not being written, and the link's own
is 0777 on POSIX, which would publish a world-writable file. IsRegular rejects
both, plus directories and devices. An explicit havePerm bool, rather than a
preserveMode != 0 sentinel, keeps a target legitimately at 0000 from being
widened to the default. A new file gets 0666 subject to umask, matching
os.Create, in place of the hardcoded 0644.
The copy runs through ctxReader, which checks ctx before each read and
deliberately exposes only Read: *strings.Reader, *bytes.Reader and *os.File
implement io.WriterTo, which io.Copy consults first, so forwarding it would
hand the copy a fast path that never checks the context. A copy that stops
because the context ended reports the context error, so callers can match
context.Canceled and context.DeadlineExceeded.
The rename goes through robustio.Rename rather than a hand-written Windows
helper. os.Rename on Windows has been MoveFileEx(MOVEFILE_REPLACE_EXISTING)
with fixLongPath applied since Go 1.16, so a MoveFileEx helper would only
duplicate it minus long-path handling and degrade to a non-atomic
Remove+Rename fallback. The real Windows hazard is a scanner or indexer
holding a transient handle on the just-closed temp file; robustio retries
those errors with bounded backoff and keeps the atomic replace. On Linux it is
a plain passthrough to os.Rename. Cost is one go.mod line and no transitive
modules, and golang.org/x/sys stays indirect.
The temp name is capped at NAME_MAX with the cut backed off to a UTF-8 rune
boundary, since long multibyte OCFL content names would otherwise produce an
invalid name that strict filesystems reject.
Any failure before the rename now returns 0 bytes written: nothing reached the
target, so reporting the bytes that reached the temp file would describe a
file that does not exist.
Closes item 1 of #164 as a side effect: Write never opens the target path, so
a symlink at name has its link entry replaced rather than being written
through to its referent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0174A5w1tx7rA1BRUgeLpfb7
The two subtests #162 left skipped in the shared WriteFS suite now run for both backends: a source error must leave the previous content intact, and a source error on a new file must leave nothing behind. imptest's package doc drops #163 from its list of outstanding skips. Backend-specific coverage in fs/local: atomic_write_test.go pins the temp-file-and-rename sequence itself — a large multi-chunk payload lands complete and exact; cancellation mid-copy leaves a new target absent and an existing target at its previous content; a failing source preserves the previous content; concurrent readers sampling a slow write only ever observe one complete version; the temp file lives in the target's own directory during the write and is gone after both success and failure. It also pins the returned count as 0 on a pre-rename failure, and BenchmarkWriteMany puts the per-file cost of the two fsyncs on the record (~660us for 1KiB, ~4.2ms for 1MiB on the CI-class machine used here) so any future argument for an opt-out knob starts from a number. TestFS_WriteCtxReaderHidesWriterTo pins the reason ctxReader exposes only Read, using *strings.Reader specifically — the source type whose io.WriterTo fast path defeats a naive wrapper. Three levels: ctxReader must not satisfy io.WriterTo, io.Copy from a canceled one must move no bytes, and a canceled Write fed a *strings.Reader must not commit. A context that reports its cancellation from the second Err() call onward places the cancellation inside the copy without a sleep or a race. localfs_symlink_test.go (POSIX-only) pins the mode decisions: a symlinked target's replacement is a regular file at the default new-file mode, with neither the link's 0777 nor the referent's mode leaking onto it, and the referent untouched; a target at 0000 keeps 0000; a missing target is written as a new file rather than failing on the stat. tempname_test.go pins tempFileName across short, sub-limit multibyte, long ASCII and >255-byte multibyte names: within NAME_MAX, valid UTF-8, creatable with O_EXCL, and still prefix-correlated with the target. localfs_test.go's cancellation subtest now asserts the error wraps context.Canceled, a deadline subtest asserts context.DeadlineExceeded, and the permission subtest measures the umask with a reference file instead of hardcoding 0644 — which only held under umask 022 — alongside a new subtest for mode preservation on overwrite. Mutation tested: forwarding io.WriterTo from ctxReader fails all three fast-path subtests, dropping the IsRegular guard fails the symlink test with -rwxrwxrwx, and restoring the preserveMode != 0 sentinel fails the mode-0000 test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174A5w1tx7rA1BRUgeLpfb7
WriteFS.Write had no doc comment at all while Remove and RemoveAll both carried their contract. It now states what implementations should provide — name is never observable with partial contents, and a failed write leaves the previous contents intact — and how each backend gets there: object-upload semantics for s3, temp-file-and-rename for local. The atomicity claim is qualified per platform rather than stated flat, since os.Rename's own doc still says it is not atomic on non-Unix platforms even though the Windows implementation is MoveFileEx(MOVEFILE_REPLACE_EXISTING). The README mentions the guarantee where the local backend is introduced. CI runs ubuntu only, so nothing exercises the windows build. GOOS=windows go vet ./... is the cheap cross-compile check; a windows-latest job would be better still, but nothing available here can verify such a run would be green, so it is left as a follow-up rather than shipped red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174A5w1tx7rA1BRUgeLpfb7
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Write opened the destination with O_CREATE|O_TRUNC and copied into it at a
hardcoded 0644. Three defects followed: a failed or canceled copy left the
target truncated or partial with the previous complete file gone; overwriting
a 0600 file silently published it at 0644; and io.Copy never consulted ctx, so
a canceled write kept running.
Write now copies to a unique temp file ("..tmp-",
O_CREATE|O_EXCL|O_WRONLY) in the target's own directory, syncs it, closes it,
and renames it over the target. A reader of the target sees the old file or
the new file in full, never a partial one; every path before the rename
removes the temp file, so failures leave no litter. The containing directory
is fsynced best-effort afterwards so the rename itself survives a crash.
The target's mode is carried onto the replacement with chmod (not umask-
masked, so the mode is copied exactly), but only when Lstat reports a regular
file: for a symlink neither mode on hand describes the file being created —
the referent's belongs to a file that is not being written, and the link's own
is 0777 on POSIX, which would publish a world-writable file. IsRegular rejects
both, plus directories and devices. An explicit havePerm bool, rather than a
preserveMode != 0 sentinel, keeps a target legitimately at 0000 from being
widened to the default. A new file gets 0666 subject to umask, matching
os.Create, in place of the hardcoded 0644.
The copy runs through ctxReader, which checks ctx before each read and
deliberately exposes only Read: *strings.Reader, *bytes.Reader and *os.File
implement io.WriterTo, which io.Copy consults first, so forwarding it would
hand the copy a fast path that never checks the context. A copy that stops
because the context ended reports the context error, so callers can match
context.Canceled and context.DeadlineExceeded.
The rename goes through robustio.Rename rather than a hand-written Windows
helper. os.Rename on Windows has been MoveFileEx(MOVEFILE_REPLACE_EXISTING)
with fixLongPath applied since Go 1.16, so a MoveFileEx helper would only
duplicate it minus long-path handling and degrade to a non-atomic
Remove+Rename fallback. The real Windows hazard is a scanner or indexer
holding a transient handle on the just-closed temp file; robustio retries
those errors with bounded backoff and keeps the atomic replace. On Linux it is
a plain passthrough to os.Rename. Cost is one go.mod line and no transitive
modules, and golang.org/x/sys stays indirect.
The temp name is capped at NAME_MAX with the cut backed off to a UTF-8 rune
boundary, since long multibyte OCFL content names would otherwise produce an
invalid name that strict filesystems reject.
Any failure before the rename now returns 0 bytes written: nothing reached the
target, so reporting the bytes that reached the temp file would describe a
file that does not exist.
Closes item 1 of #164 as a side effect: Write never opens the target path, so
a symlink at name has its link entry replaced rather than being written
through to its referent.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_0174A5w1tx7rA1BRUgeLpfb7