Skip to content

fs/local: make Write atomic, preserve target mode, honor ctx - #171

Merged
srerickson merged 3 commits into
mainfrom
claude/issue-163-robustio-plan-j0gz0n
Aug 25, 2026
Merged

fs/local: make Write atomic, preserve target mode, honor ctx#171
srerickson merged 3 commits into
mainfrom
claude/issue-163-robustio-plan-j0gz0n

Conversation

@srerickson

Copy link
Copy Markdown
Owner

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

claude added 3 commits August 25, 2026 19:00
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
@srerickson
srerickson merged commit 05ccdb3 into main Aug 25, 2026
1 check passed
@srerickson
srerickson deleted the claude/issue-163-robustio-plan-j0gz0n branch August 25, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants