fs: correctness fixes and cross-backend contract tests for the local and s3 backends - #161
Closed
exe-dev-github-integration[bot] wants to merge 67 commits into
Closed
fs: correctness fixes and cross-backend contract tests for the local and s3 backends#161exe-dev-github-integration[bot] wants to merge 67 commits into
exe-dev-github-integration[bot] wants to merge 67 commits into
Conversation
added 30 commits
August 24, 2026 04:15
Add DeleteObjects with the AWS SDK v2 signature to the RemoveAllAPI interface so removeAll can batch-delete keys. Update the mock S3API implementation to record batch deletions in its Deleted map.
Copy() previously decided whether to use the optimized CopyFS path by comparing FS interface values with ==, which is unreliable: two distinct values for the same backend (e.g. two *BucketFS for one bucket) compared unequal and silently fell back to a slow read+write. Add an optional SameBackend interface (SameBackend(FS) bool). Copy() now takes the CopyFS path only when both srcFS and dstFS implement SameBackend and dstFS.SameBackend(srcFS) returns true. dstFS is the receiver because it performs the copy. FS types that don't implement SameBackend keep using the fallback read+write path; the FIXME is updated accordingly.
HeadObject on a missing key returns a *smithyhttp.ResponseError with status 404 (no body to deserialize types.NotFound), which errIsNotExist does not currently recognize. Add failing regression tests for the OpenFile and Copy paths plus an integration test gated on $OCFL_TEST_S3.
copy() previously fell back to multipart copy only when CopyObject failed with an error whose text matched "copy source is larger than the maximum allowable size". That message is not part of the AWS API contract, so S3-compatible stores (MinIO, GCS, etc.) can phrase the failure differently, breaking large copies outright. The HeadObject response was already in hand, so use it directly: when the source ContentLength exceeds maxCopySize (5 GiB), skip CopyObject and call MultiCopier immediately; small objects (<= 5 GiB) go through CopyObject as before, with any error propagated. No error-message string matching remains in copy().
local.FS now implements fs.SameBackend: it reports true only when other is also a local *FS whose root path resolves to the same directory. Both roots are absolutized with filepath.Abs (which also cleans) before comparing, so trailing slashes and relative/.-suffixed variants of the same root compare equal. fs.Copy can now use the optimized Copy path for local-to-local copies on the same directory instead of the slow read+write fallback.
Replace per-key DeleteObject calls in removeAll with a single DeleteObjects request per ListObjectsV2 page (up to 1000 keys each). Prefix semantics and error handling are preserved.
BucketFS now satisfies the optional fs.SameBackend interface: two *BucketFS values refer to the same S3 backend when they share the same client (compared by pointer identity, with an interface-equality fallback for comparable non-pointer client types) and the same bucket name. Only the client and bucket are compared -- never the whole BucketFS struct, since distinct values built from the same client and bucket are the same backend. fs.Copy can now take the optimized CopyFS path when both sides are BucketFS values for the same bucket and client. Includes unit tests for SameBackend semantics and integration tests proving fs.Copy uses the CopyObject fast path for same-backend values and the read+write fallback for different clients.
…tion errors Strengthen TestFS_Write: cancellation and deadline-exceeded contexts now assert the returned error wraps context.Canceled / context.DeadlineExceeded (not just non-nil), and invalid paths including '../escape' assert fs.ErrInvalid plus PathError op/path. Pins the security property that osPath/fs.ValidPath blocks traversal before any file is touched.
fs.Copy previously decided whether to use the optimized CopyFS path with dstFS == srcFS, comparing interface values. These tests pin down the new SameBackend-based decision in both directions: - two distinct FS values that confirm the same backend take dstFS.Copy() (the old == comparison would have missed this and fallen back); - the same FS value with SameBackend false falls back to read+write (the old == comparison would wrongly have taken the Copy path); - fallback (no panic) when either side, or both, lack SameBackend; - a plain FS without WriteFS/CopyFS yields ErrOpUnsupported, not a panic. The spy types record Copy/Write calls so the tests observe which path was taken rather than relying on side effects.
…code, nested slashes) TestCopySourceSpecialKeys_Integration and TestCopySourceSpecialKeys_Multipart_Integration write a source object with spaces/unicode/nested-slash/plus/percent/hash/question keys, copy it via CopySource (CopyObject and UploadPartCopy paths), verify content equality, and clean up the temp bucket. Gated on $OCFL_TEST_S3. Fails against the old url.QueryEscape copy-source encoding (verified 404 on MinIO) and must stay green against the per-segment encoding fix.
copy() now picks the copy strategy from the HEAD ContentLength before calling CopyObject (was: error-string matching of the AWS-only "copy source is larger than the maximum allowable size" message). - mock: copy.go strategy tests plus a rewritten TestCopy_Mock multipart subtest drive a >5 GiB copy through MultiCopier using virtual objects (declared ContentLength, no materialized body), asserting CopyObject is never invoked. - mock.S3API gains CopyObjectCalls to capture CopyObject invocations and headSize() so HeadObject reports a virtual object's declared length; UploadPartCopy derives deterministic part ETags for virtual objects so the multipart round trip completes end to end. - TestCopy_CopyObjectErrorPropagates proves a CopyObject failure on a small source is returned unchanged even when its text matches the old AWS error string (no fallback). Verified: all new tests fail against the old error-string implementation and pass against the new one; go build ./..., go vet ./fs/s3/ and the fs/s3 suite are green (pre-existing red: the two Smithy-404 tests from 3b7d7ee whose errIsNotExist fix is still uncommitted).
s3File keeps mutable state (body reader, offset) across Read/Seek with no
synchronization, so concurrent use (or a consumer wrapping it in something
that parallelizes) could corrupt the offset and issue overlapping GetObject
range requests. Guard body/offset with a sync.Mutex across Read, Seek, and
Close; Seek now closes+resets the body and updates the offset atomically
under the lock, and the struct/Seek comments state the guarantee.
Seek also no longer silently discards the old body's Close() error: it is
logged at debug level via the BucketFS slog logger ("s3:seek:close" with
bucket/key/error attrs). A close error still never fails the Seek.
openFile() gains a logger parameter plumbed from BucketFS.OpenFile.
Tests: TestConcurrentReadSeek_Mock (8 goroutines x 200 Seek+Read iters;
fails with data races against the unguarded code, race-clean with the
mutex) and fs/s3/s3_seek_test.go covering the logged-close-error path, a
nil logger, same-position no-close, and the full openFile path.
Write() no longer opens the final path with O_TRUNC. It now writes to a unique temp file (.<base>.tmp-<random>) created with O_CREATE|O_EXCL| O_WRONLY in the same directory, fsyncs and closes it, then atomically renames it over the final path (same-directory rename is atomic on POSIX and Windows). Errors and context cancellation remove the temp file best-effort, so the final path always holds either the old file or the new complete file, never a truncated one. ctx is checked before and between copy chunks; an aborted copy reports ctx.Err(). Existing target permissions are preserved via chmod before the rename; new files get 0666 subject to umask. The containing directory is fsynced best-effort after the rename. Tests: umask-relative new-file permissions, permission preservation on overwrite, mid-write cancellation (no partial file / old file kept), reader-error cleanup, and concurrent-writer atomicity.
Pin write()'s ContentLength auto-detection with a recording UploadAPIClient over the single-PutObject path: - *os.File and *strings.Reader (plain io.Seekers, neither an fs.File) report their length; ContentLength is set to the detected size. - A seekSpy pins the sniffing seek sequence (current, end, restore), proving the original read position is restored, and that the upload content is read from the restored position. - Non-seekable readers and seekers whose seeks fail fall back to nil ContentLength with the reader undisturbed. - Existing behavior preserved: fs.File, *bytes.Reader, *io.LimitedReader, and explicit ContentLength options.
The WriteFS interface now states that implementations should provide atomic writes where possible, the s3 object-upload semantics, and the local temp-file-and-rename behavior. The local Write() method and the module README document the temp-file-and-rename sequence, the cleanup guarantee on failure, and that a partial file is never visible.
dirEntries() treats a prefix with no objects and no common prefixes as a missing directory (fs.ErrNotExist via the prefixHasContent guard), returns subdirectory entries for prefixes that exist only as common prefixes, and returns fs.ErrNotExist for dir='.' on an empty bucket. Pin all three cases plus the non-empty-root contrast with mock-client tests.
…er concurrency, temp lifecycle) TestFS_WriteAtomic pins the write-to-temp-file + rename contract with 7 subtests in a self-contained test file (public API only, no helpers shared with localfs_test.go): - large payload lands at the final path completely and exactly; - cancellation mid-write (new and existing target) leaves the final path unchanged and removes the temp file; - a failing overwrite preserves the previous content; - concurrent readers only ever observe old or new complete content, never a truncated file, while a slow write is in flight; - the temp file appears in the target's own directory mid-write and is removed on both success and failure. Verified failing against the pre-atomic implementation (O_TRUNC + io.Copy in place, commit e7d6ca3^): cancellation is ignored, overwrites leave partial content, readers observe truncated reads, and no temp file ever exists.
… backends) Choose option B for the WriteFS.Remove contract: removing a file that does not exist returns an error satisfying errors.Is(err, fs.ErrNotExist) on every backend, matching the local backend and os.Remove semantics. The S3 backend must check existence (HEAD) before DeleteObject, whose idempotency would otherwise silently succeed for missing keys. The '.' special case remains backend-specific (fs.ErrNotExist on S3, descriptive PathError on local).
…y -> ErrNotExist) Implement the WriteFS.Remove contract (fs/fs.go) on the S3 backend: removing a missing file must return an error satisfying errors.Is(err, fs.ErrNotExist). DeleteObject alone is idempotent (204 even for missing keys), so probe with HeadObject first: a not-found HEAD maps to fs.ErrNotExist, other HEAD errors are preserved, and a successful HEAD proceeds to DeleteObject. RemoveAPI now requires HeadObject. The name == "." guard is unchanged (fs.ErrNotExist).
Pin the Option B Remove contract documented in fs/fs.go on both backends:
- S3: Remove of a missing key returns an error satisfying
errors.Is(err, fs.ErrNotExist). Regression: the old implementation
relied on the idempotent DeleteObject, which silently succeeds (204)
for missing keys, so Remove returned nil. The new tests also pin the
missing-key HEAD error shapes (types.NotFound, types.NoSuchKey,
smithy 404 response error, MinIO generic code), that non-404 HEAD
errors propagate as-is, that DeleteObject is never called for a
missing key, and the Remove(".") guard (fs.ErrNotExist, no API calls,
bucket untouched). Integration coverage for real S3/MinIO included.
- local: missing-file Remove satisfies errors.Is(err, fs.ErrNotExist);
the "." guard returns the documented descriptive *fs.PathError (not
fs.ErrNotExist) and leaves the root directory intact.
- Shared WriteFS.Remove contract test in internal/testutil runs against
both backends with each backend's documented "."-error classification.
- mock.PutObject now materializes the uploaded object so the mock
round-trips Write followed by OpenFile like real S3 (required by the
shared test; no existing tests depended on writes not materializing).
Verified: new S3 tests fail against the pre-172b1a9 remove() behavior
(DeleteObject without an existence check) and pass at HEAD.
S3 has no empty directories, so a non-root prefix with no objects and no
common prefixes is indistinguishable from a path that never existed and
dirEntries reports it as fs.ErrNotExist, matching the local backend's
readdir of a missing directory. The root is the one exception: "." names
the bucket itself, which always exists (a missing bucket surfaces as a
ListObjectsV2 error), so an empty bucket must read back as zero entries
with no error, matching the local backend's readdir of an existing but
empty directory.
The asymmetry on empty non-root prefixes is deliberate and documented in
the dirEntries doc comment: local storage can represent an empty
directory, S3 cannot, and a valid OCFL object never depends on one
(every version directory contains inventory.json; storage-root and
object layouts create no empty directories; extensions may create them on
local storage, and on S3 the same path simply reads as missing).
Root behavior matters because Root.NewRoot (root.go) and
ocflfs.RemoveAll(".") (fs/fs.go) both start by reading dir=".": an
empty bucket is a valid (new) storage root. NewRoot tolerates
fs.ErrNotExist, but RemoveAll(".") would otherwise fail instead of being
the no-op it is on local storage.
Tests: TestDirEntries_RootEmptyBucket_Empty (flipped from
_ErrNotExist), TestDirEntries_RootMissingBucket_ListError (missing
bucket surfaces the ListObjectsV2 error, not ErrNotExist), and
TestDirEntries_RootEmptyBucket_Integration (live store, gated by
$OCFL_TEST_S3; verified against MinIO).
The io.Seeker case in write() recorded size = end (absolute end offset) instead of the REMAINING length end - cur. Uploading a partially-consumed seekable reader (strings.Reader reused after a first Write, *os.File at a nonzero offset) therefore declared ContentLength = total object size while the body carried fewer bytes, and the SDK request died with 'net/http: ContentLength=N with Body length 0' before reaching the store, surfacing as a transport error instead of a clean API error. Compute cur := Seek(0, io.SeekCurrent) and end := Seek(0, io.SeekEnd), restore cur, and use size = end - cur (only when end >= cur); any failed seek keeps the nil-ContentLength fallback. Unit tests: new '*strings.Reader partially consumed' case asserts ContentLength and uploaded body equal the remaining bytes; the RestoresPosition test now expects the remaining 6 bytes (was total 10). The integration test TestWriteWithOptions (reused reader + IfNoneMatch) passes again against MinIO.
…der ContentLength
TestWriteWithOptions (fs_test.go) asserts that a second conditional write
(If-None-Match: "*") surfaces a smithy.APIError with ErrorCode
"PreconditionFailed". That mapping only works when the request reaches the
store: the seek-based ContentLength sniffing used to declare the stream's
total length for an exhausted reader, killing the request on the wire
('ContentLength=N with Body length 0') so the server's 412 never came back.
Pin both properties without a live store:
- TestWriteConditionalPutErrorMapping: a rejected conditional PUT (SDK-shaped
error chain wrapping GenericAPIError PreconditionFailed, plus bare and
double-wrapped variants) still surfaces through write() as a *fs.PathError
that satisfies errors.As into smithy.APIError with the service code.
- TestWriteExhaustedReaderContentLength: an exhausted seekable reader must
sniff ContentLength 0 (remaining bytes), keeping the request well-formed.
…issing keys to ErrNotExist - x-amz-copy-source now percent-encoded per path segment (minio EncodePath semantics) instead of url.QueryEscape, which broke on spaces/unicode - walkFiles skips zero-byte directory-placeholder objects (keys ending in '/') - errIsNotExist maps HeadObject/GetObject missing-key failures to fs.ErrNotExist - multicopy uses the same copy-source encoding
S3 DeleteObjects returns HTTP 200 with per-key failures in the response body's Errors list, so a successful API call does not by itself mean all objects were deleted. removeAll now inspects out.Errors after each batch delete and returns a joined *fs.PathError (Op removeAll) listing each failed key when any objects survive; empty Errors keeps the previous nil behavior. API-level errors are unchanged.
DirEntriesFS permits yielding (nil, err) pairs, and fs.DirEntries does exactly this when the FS isn't a DirEntriesFS. fileWalk yielded the error but then fell through to path.Join(subDir, e.Name()) with e == nil, panicking. The e.Info() error path had the same fall-through problem, yielding a FileRef with a nil Info field. After an error yield, propagate the error and continue to the next iteration instead of dereferencing e. Treat a nil entry without an error (a contract violation) defensively by skipping it. Valid entries are walked exactly as before; subdirectories still recurse.
…oveAll RemoveAll must surface a non-nil *fs.PathError whose message names the failed key when a batch DeleteObjects response is HTTP 200 but carries a non-empty Errors list (one key denied, one deleted). Adds a mock deleteObjectsFailer wrapper that injects per-key failures into the DeleteObjects response. Verified red against e21977f (pre-fix) and green against 655d5bc (the fix).
write()'s ContentLength sniff for a generic io.Seeker moved the caller's reader to its end and only restored the position when every seek succeeded; a failed restore-seek left the reader at EOF and the upload silently delivered an empty body. sniffSeekerLength() now owns the probe: it always restores the original position before returning a length, and a failed restore is a hard error (write returns a *fs.PathError and never issues PutObject) instead of a silent EOF upload. Failed probes that leave the position restorable keep ContentLength nil and the body streams without a declared length. Tests: shared-reader position coherence, restore-failure -> error with no PutObject, end-seek failure with recoverable/unrecoverable position, and a reader positioned past the end.
…backend batch
The '.' special case returned on the first error, abandoning the rest of
the root and hiding how much was deleted. It also passed bare entry
names into recursion, which is only correct if DirEntries('.') yields
top-level basenames, and it never reached the backend's batched
removeAll: every top-level entry was deleted individually, so S3
RemoveAll('.') degraded to per-key HEAD+DeleteObject instead of one
bucket-wide list and batched DeleteObjects.
Refactor the '.' path: prefer writeFS.RemoveAll('.') so backends whose
removeAll can empty the root (S3) use the batch path; when the backend
refuses to remove the top-level directory (local, whose storage root
must survive), fall back to a per-entry walk. The walk is best-effort
and collects per-entry errors with errors.Join, continuing past
failures, and threads the accumulated prefix through recursion with
path.Join instead of passing bare entry names. Non-'.' RemoveAll is
unchanged.
Write's mode-preservation step used os.Stat, which follows symlinks: when the write target was a symlink, the referent's mode was stamped onto the replacement temp file and the link entry was replaced by a regular file carrying the referent's permissions. Use os.Lstat so the symlink's own mode is preserved instead; a failed Lstat (missing target) is tolerated and treated as a new file. Adds a POSIX-only test (localfs_symlink_test.go) covering the full scenario: a symlink renamed over an existing regular file stays a symlink with the symlink's mode, and writing through that symlinked target replaces it with a regular file inheriting the symlink's mode, never the referent's.
Exercises the full upload path against a live S3-compatible store (MinIO) with two scenarios pinned by the content-length fixes: - a partially-consumed *os.File (both fs.File and io.Seeker): the seeker remaining length wins, so declared ContentLength equals the delivered body and net/http accepts the request; the object stores exactly the remaining bytes - a seeker whose restore seek fails: write() surfaces a *fs.PathError mentioning the restore and no object is created (no silent empty upload)
Copy used to write default PartSize/Concurrency values back into the shared receiver on every call, racing when one MultiCopier is reused across concurrent Copy calls (fixed in 767142f by copying the knobs into locals). Add TestMultiCopy_ConcurrentReuse: eight goroutines copy through one receiver, in both the zero-knob regime (where the old code overwrote the receiver with the defaults) and an explicit-knob regime, asserting every copy succeeds with the full source size, the MPU completes, and the receiver's PartSize/Concurrency are never modified. The mock's multipart bookkeeping (UpdatedETags, MPU* flags) is now guarded by a mutex so -race reports a genuine receiver race rather than a mock bookkeeping race; real S3 clients are safe for concurrent use and the mock now matches that contract on the multipart path. Verified: go test -race ./fs/s3/ passes with the fix and fails (race + mutated receiver fields) against the pre-fix multicopy.go.
Add a comment above os.RemoveAll(fullPath+"/") explaining that the trailing slash marks directory semantics but is not what prevents symlink escape: os.RemoveAll strips trailing separators and never follows symlinks, so a symlink at the target path is removed as a link and its referent (even outside the storage root) is untouched. Add a regression test pinning that property: a symlink inside the storage root pointing at an external directory is removed as a link, and the external directory and its file survive RemoveAll. Verified RED against a link-following recursive walk (the hazard class this guards).
Read held f.mu for the entire body.Read call, so a concurrent Seek stalled behind an arbitrary-duration network read before it could close a partially drained HTTP/2 body. Restructure the locking: - offset is an atomic.Int64: Seek stores it under mu, a completing Read adds its byte count without holding mu. - Read holds mu only to snapshot body/offset/generation, and issues GetObject and body.Read with no locks held; a readMu serializes body (re)creation and body.Read so concurrent Reads share one body instead of interleaving. - Seek closes/discards the body and bumps a generation counter whenever the position changes. An in-flight Read whose snapshot generation no longer matches drops its byte count, so a Read racing a Seek may return data from the pre-seek position (documented on Read and Seek); a Read whose GetObject was in flight when the Seek landed discards the stale fetch and retries at the new offset. - a no-op seek (same position) keeps the body and leaves the generation unchanged so in-flight Reads keep their byte counts. Verified with go test -race ./fs/s3/ including MinIO integration legs, plus concurrent Read/Seek probes (Seek returns in <1s while Read is blocked; stale fetch retried at new offset; concurrent Reads share one body).
…agation Extend the internal/testutil shared contract suites with TestWalkFilesContract, run against both the local and s3 backends. The suite pins the WalkFiles contract documented by BucketFS.WalkFiles (fs/s3/fs.go): (1) iteration stops when the yield callback returns false — verified with iter.Pull2, where a stop() mid-iteration leaves no further paths pullable; (2) a walk failure is delivered to the caller as the error element of the (file, error) pair as an *fs.PathError naming the walked path, after which the iterator yields nothing further — exercised via a per-backend fixture (s3: an API whose ListObjectsV2 fails for the 'blocked/' prefix; local: a regular file where the walk expects a directory); and (3) every yielded FileRef has its FS field set to the backend's own instance (s3 *BucketFS, local *local.FS), verified functionally by opening and reading each file through the ref. The seed (a.txt, sub/b.txt, sub/c.txt) enumerates identically on both backends, so the shared suite pins exact paths, ordering, and BaseDir. s3 additionally pins its error shape: Op list_files, underlying API error preserved through errors.Is.
WriteFS.RemoveAll now states that removal is best-effort: on error the remaining entries are still attempted and all errors are joined, so a partial deletion may remain. Documents the backend-dependent behavior for name == "." (empty the top-level directory, as S3 does, or refuse, as the local backend does) and the package-level fallback to a per-entry walk.
os.RemoveAll opens the parent directory by full path (OpenFile(parentDir)
in os.removeAll), so a symlink at an INTERMEDIATE path component is
followed: with root/link -> external outside the storage root,
RemoveAll("link/subdir") silently deleted ext/subdir (err == nil) while
leaving root/link in place. Drive removal through os.Root.RemoveAll
instead: every component is walked with openat-family operations that
validate symlink targets stay within the root, so an escaping
intermediate symlink (absolute target, or a relative target resolving
outside the root) makes RemoveAll fail with an error instead of
deleting, while a relative in-root symlink is still followed like any
other directory. The final component is unlinked without being
followed, so a symlink at the name itself is still removed as a link
with its referent untouched (behavior pinned by the earlier symlink
safety regression), and a missing name remains a nil no-op.
Add regression subtests: an intermediate symlink escaping the root is
refused with a PathError and the external target and its contents
survive (RED before this change: the old code deleted them and returned
nil); an intermediate symlink staying inside the root is followed, so
the fix does not over-reject.
…threading The '.' special case in fs.RemoveAll refactored in 656a194 needs tests: - RemoveAll('.') on a backend that refuses to remove the root walks every top-level entry, continues past per-entry failures, and returns all of them errors.Join'ed (regression: the old code returned on the first error). - RemoveAll('.') delegates to the backend's own RemoveAll('.') when it can empty the root in one batch (S3), with no per-entry walk at all. - The fallback walk threads the accumulated prefix through path.Join, so entries need not be clean top-level basenames ('./sub' reaches the backend as 'sub'). - fs/s3: generic fs.RemoveAll('.') on BucketFS reaches the batched removeAll (one bucket-wide DeleteObjects, no per-key DeleteObject degradation). All new tests fail against 656a194^ and pass against the fixed code.
…cs, add rename tests FS.Write's final swap now goes through renameReplace, which is os.Rename on POSIX (atomic replacement) and renameReplaceWindows on Windows (MoveFileEx, then Remove+Rename fallback). The helper is used unconditionally on Windows: os.Rename there already maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING), so trying it first could only fail where the helper's first strategy already failed, adding a guaranteed failing syscall to every overwrite. The Write doc comment no longer claims atomic behavior on Windows: it now describes the atomic POSIX replace, the best-effort Windows path (a failure between Remove and Rename can leave no file at name), and that a symlink target is replaced as an entry, with the replacement inheriting the link's own mode, never the referent's. Inline comments in Write updated to match. Tests: - rename_test.go (cross-platform): renameReplace overwrite of an existing regular file; FS.Write overwrite of an existing regular file; FS.Write mode preservation across an overwrite (equality of mode before/after, meaningful on POSIX where the unpreserved default differs). - rename_symlink_test.go (!windows): renameReplace replacing an existing symlink target with a regular file (referent untouched), and renaming a symlink source over an existing regular file (destination remains a symlink with the link's own mode, referent untouched).
BucketFS.WalkFiles no longer re-yields walkFiles output to set the FS field on each FileRef. walkFiles now receives the backend FS and sets FileRef.FS at construction, so BucketFS directly exposes the local backend's WalkFiles semantics: early termination on a false yield and errors delivered as the pair's error element with nothing yielded after. Identical observable behavior; the contract suite pins all three behaviors for both backends.
s3File carried two mutexes, an atomic offset and a generation counter so a Seek could return promptly while a Read was blocked streaming a large object. Nothing needs that: fs.File and io.ReadSeeker make no concurrency guarantee (*os.File makes none either), the only Seek consumer in the repo is fs/s3/example/rangeserver, which uses one handle per HTTP request from a single goroutine, and the design was introduced defensively rather than for a real caller. The machinery had also already drifted from its own documentation: every access to the "atomic" offset was made under mu, so the two-lock-domain rationale in the struct comment described a scheme the code did not implement. Revert Read/Seek/Close to the straightforward single-owner form and state the guarantee plainly on s3File and BucketFS.OpenFile, matching the convention for fs.File values. The Seek body-close debug logging and the no-op-seek body reuse added along the way are kept. Tests: drop the four concurrency tests (SeekPromptWhileReadBlocked, ConcurrentReadsShareOneBody, ReadRacingSeekMayReturnPreSeekData, ReadSeekConcurrentStress) and TestConcurrentReadSeek_Mock, which pinned the semantics being removed. The HTTP/2 connection-reuse regression test is sequential and still valuable, so it moves to s3_connreuse_test.go with the helpers it actually uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D9DAbTSrBvN6zLnM6zwYW2
RemoveAll(".") probed the backend's own RemoveAll(".") and treated ANY
error as "this backend refuses to remove its storage root", falling through
to the per-entry walk. That conflates two very different outcomes: the
local backend's deliberate guard, and an S3 bulk delete that failed partway
through a large bucket. A real mid-operation failure was discarded, the
walk re-ran (two requests per surviving object, since Remove now
HEAD-checks existence), and a successful fallback returned nil — hiding
that the backend had failed at all.
The capability cannot be recovered by sniffing the error, so declare it by
type instead. Backends that can empty their own root implement the new
optional RootRemover interface; RemoveAll calls RemoveRoot and returns its
error unchanged, with no fallback. Backends that must keep their root, such
as local, simply do not implement it and get the per-entry walk directly.
This follows the SameBackend precedent already used by Copy.
BucketFS implements RemoveRoot with the same bucket-wide listing and
batched DeleteObjects as RemoveAll(ctx, "."), which continues to work when
called directly. Local is unchanged: its RemoveAll(".") still refuses.
Tests: rework the fs-level dispatch tests around the new interface, add
coverage for error propagation (no fallback after a RemoveRoot failure) and
for the local fallback walk emptying the root while the root directory
survives. Both new behaviors verified by mutation: restoring the old
error-sniffing dispatch fails four tests, and swallowing a RemoveRoot error
fails the propagation test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D9DAbTSrBvN6zLnM6zwYW2
FS.Write preserved the target's mode by Lstat'ing it and chmod'ing the temp file to match. Two mode values were handled wrongly. A symlinked target contributed the link's own mode. Using Lstat rather than Stat was deliberate — Stat would follow the link and stamp the referent's permissions onto a file that is not the referent — but the conclusion drawn from it was wrong: on POSIX a symlink's own mode is 0777, so writing through a symlinked target published a world-writable regular file. Neither mode on hand describes the file being created, so the symlink case is now treated as a new file and takes the default temp mode. A target whose mode is legitimately 0000 was also not preserved, because the "is there a mode to keep" signal was `preserveMode != 0`, which cannot distinguish that from "no target". An explicit havePerm bool replaces it. The symlink test previously asserted the 0777 behavior, so it is rewritten around the corrected semantics; it measures the default new-file mode by writing a probe file rather than assuming an umask. Both fixes were mutation tested: restoring the Lstat-without-symlink-check fails the symlink test with -rwxrwxrwx, and reinstating the preserveMode != 0 sentinel fails the new mode-0000 test.
sameClient compared pointer-like client kinds by pointer identity and fell through to interface equality for everything else. A client whose dynamic type is a non-comparable struct (one holding a slice, map or func field) is not a pointer-like kind, so it reached the == and panicked with "comparing uncomparable type". The client is arbitrary caller-supplied code, so a kind switch cannot rule this out. Guard the fallback with reflect.Value.Comparable and report false when identity cannot be established, which is what SameBackend's contract already requires of implementations that cannot tell. Tests cover both halves: a non-comparable value client (recovering so an unguarded implementation is reported as a failure rather than crashing the run) and a comparable value client, which must still compare by value. Mutation tested: removing the guard panics the first test.
Three small removals, no behavior change beyond the Copy dispatch: fs.Copy required BOTH srcFS and dstFS to implement SameBackend before using the optimized CopyFS path, but only dstFS was ever asked. The extra srcFS assertion added no safety — a destination that answers true for storage it does not share is already violating the interface contract, and it can do that just as easily for a source that implements SameBackend — while costing a genuine same-backend pair the fast path whenever the source happens not to implement the interface. Only the destination is consulted now. The subtest that pinned the two-sided requirement is inverted to pin the new behavior (fast path taken when only dstFS implements SameBackend); the "dstFS does not implement SameBackend" fallback case is unchanged and still covers the guard that does matter. RemoveAllAPI declared DeleteObject, which removeAll never calls — it lists a prefix and deletes each page with a single DeleteObjects request. Callers supplying a narrow RemoveAllAPI had to provide a method that is never invoked. Single-key deletion stays in RemoveAPI, which does use it. copyWithContext hand-rolled io.Copy to keep a per-chunk context check that io.Copy's WriterTo shortcut would bypass. Wrapping the source in a context-checking reader that exposes nothing but Read achieves the same thing and drops the loop, the short-write handling and an == io.EOF comparison. Mutation tested: removing the wrapper fails four cancellation tests. Also documents BucketFS.Remove's HeadObject-then-DeleteObject round trip (the price of the fs.ErrNotExist contract on S3's idempotent delete) and renames a local variable that shadowed the max builtin.
Roughly a dozen test doc comments explained what a past implementation did
wrong ("the old code…", "before the fix…", "it fails there", "Option B") in
place of stating what the code must do. That framing rots: the narrative
already outlived the change it described, and none of it tells a reader
arriving fresh what the test protects.
Each is rewritten to state the invariant and the concrete failure mode that
threatens it, in present tense. Where the historical detail was genuinely
load-bearing it survives as a named hazard rather than a story — the
x-amz-copy-source tests still record that whole-string url.QueryEscape 404s
against MinIO and that per-segment url.PathEscape leaves a '+' the store reads
back as a space, because both are plausible "fixes" a future reader might
reach for.
Comments only; no test logic changed.
…irEntries The shared contract suite covered Remove and WalkFiles. Write, RemoveAll and DirEntries were each tested per backend, in backend-specific terms, so nothing asserted that the two agree — which is the whole premise of the ocflfs.FS abstraction. Three new contracts, each aimed at where the hierarchical and key-value backends drift apart rather than at the happy path: Write — a shorter overwrite must fully replace (a tail of the previous content is the tell for an in-place write), a failing source must leave the previous file intact, and an invalid path must be rejected with fs.ErrInvalid and no side effect. RemoveAll — absent is success, not fs.ErrNotExist (the documented inversion of Remove); the subtree goes and nothing else does; and "a" is a path element, not a string prefix, so "ab", "abc.txt" and "a-sibling.txt" survive. DirEntries — entries are base names, one level deep, sorted. A flat listing naturally yields full keys and the whole subtree, and a caller that joins the entry onto the directory it asked for then builds "dir/dir/file". Two knobs were needed (RemoveAll on "." and on a file's own path, both genuinely backend-dependent). A third was drafted for a missing directory and then dropped: both backends already report fs.ErrNotExist, so the contract asserts it outright. Writing these turned up a gap in the S3 mock: DeleteObject and DeleteObjects recorded the key in a Deleted map but never removed the object, so it stayed visible to HeadObject, GetObject and ListObjectsV2. Every S3 deletion test therefore asserted which requests were sent, not what the bucket ended up containing — the new RemoveAll contract failed 5 of 8 subtests against it. The mock now deletes for real; Deleted stays as call bookkeeping, documented as such. No existing test depended on the old behavior. Mutation tested: building the S3 listing prefix as name instead of name+"/" fails the sibling subtest with all three survivors named.
Three test files each defined a wrapper embedding *mock.S3API that overrode two methods to append keys to a slice. removeAllRecorder and removeAllDotRecorder were byte-for-byte identical apart from the type name; removeRecorder differed only in which two methods it wrapped. The mock now keeps an ordered call log itself — operation name plus the keys each request named — with query helpers over it: Calls, CallsFor, CallCount which requests ran, in what order KeysFor every key an operation named, flattened KeyBatchesFor the keys per request, so batching is visible KeyBatchesFor is the one that matters for RemoveAll: a regression from one batched DeleteObjects to one request per key leaves KeysFor identical and KeyBatchesFor completely different. Mutation tested — splitting the batch in removeAll fails all four batch tests. Recording lives in the mock rather than in a wrapper so it covers every method, not just the two a given test remembered to wrap, and so a test that wants to assert request shape does not have to declare a type to do it. The log has its own mutex: every method records, including those already holding the mock's MPU lock. Not consolidated: headErrAPI, listErrAPI, nilLengthAPI, closeErrAPI, cancelAwareAPI, deleteObjectsFailer and stubWalkAPI. These look like the same pattern but are not duplication — a fixed error, an error after N calls, a malformed output, a body whose Close fails, a context cancel as a side effect, per-key errors inside a successful response. Forcing them through one injection mechanism would produce a hook table less readable than the seven small wrappers.
fs_test.go held tests for eight different operations, four of which already had a dedicated file in the same package — so "where does a new RemoveAll test go?" had two answers. TestReadDir/TestReadDir_Mock move to direntries_test.go, TestRemove_Mock to remove_contract_test.go, TestRemoveAll_Mock to removeall_batch_test.go, and TestCopy_Mock to copy_strategy_test.go. fs_test.go drops from 996 lines to 627 and keeps what has no better home. isPathError, isInvalidPathError, compareFileInf and comparDirEntries move to a new helpers_test.go. Three of the four are used from several files; they were at the bottom of fs_test.go because that is where the first test needing one happened to be. Pure movement — no test logic or assertions changed. Two things the review flagged here did not hold up on inspection, and are left alone deliberately: The remaining file count is not sprawl. Six s3 test files are package s3 (testing unexported functions: copySourcePath, errIsNotExist, s3File.Seek, content-length detection) and twenty are package s3_test. Pairs that looked like arbitrary splits — copysource_test.go vs copysource_mock_test.go, errnotexist_test.go vs errnotexist_generic_test.go — are that boundary, and cannot be merged. fs/local splits the same way, with localfs_test.go as the internal-package file and every external-package file a contract caller. There is also no duplicated helper left to collapse: every non-Test function across the s3 test files is defined exactly once.
Author
|
Closing without merging. 67 commits touching 63 files, rewriting local atomic writes, S3 delete Refiled as themed issues, each sized for a single focused PR:
Each issue documents the defect against current The |
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.
Part 1 of 2. Behavior and test coverage on the
fsbackends sincev0.11.2: 67 commits, 63 files, +8,158 / −499.Part 2 (#160) is the test-file reorganization, stacked on this branch. It contains no behavior change, so this PR is the one that needs careful review.
Correctness
Bugs found by reviewing the branch, each confirmed by a failing test before the fix and mutation-tested after:
fs/local: writing through a symlink published a world-writable file.Write's atomic rename replaces the link entry itself, and the code stamped the link's own mode (0777on POSIX) onto the replacement regular file.Statwould have been worse — it follows the link and stamps the referent's mode onto a file that is not the referent. Neither mode at the target is the right one to keep, so the symlink case now falls back to the default new-file mode.fs/local: a target whose mode is legitimately0000was treated as "no mode to preserve". ThepreserveMode != 0sentinel could not tell the two apart; an explicithavePermcan.fs/local:RemoveAllcould follow an intermediate symlink out of the storage root.fs/local:Writewas not atomic on Windows —os.Renamecannot replace an existing destination there. Now wired through aMoveFileExhelper, with the atomicity docs corrected to say what each platform actually guarantees.fs/local:tempFileNamecould cut a long name mid-rune. Now cuts at a UTF-8 boundary.fs/s3:SameBackendpanicked on a non-comparable client. A client whose dynamic type is a struct with a slice, map, or func field reached==and panicked withcomparing uncomparable type. Guarded withreflect.Value.Comparable, returning false — which is what the interface's contract already requires of an implementation that cannot establish identity.fs/s3:s3File.Readheld a mutex across network I/O, stalling a concurrentSeek. The type is now documented as not concurrency-safe and the locking machinery is gone.fs/s3: per-key failures inside a200 DeleteObjectsresponse were silently dropped byremoveAll. S3 reports partial batch failures in a successful response body.fs/s3: nilContentLengthon HEAD crashedopenFileandcopy.fs/s3: copy-source paths were not percent-encoded, so keys containing spaces,+,%, or non-ASCII failed on the wire.fs/s3:errNotExistreplaced the underlying error instead of wrapping it, discarding the API detail.fs/s3: the multipart-copy strategy was chosen by matching error text. Now decided from the HEADContentLength.fs/s3: the deferred multipart abort/complete ran on an already-canceled context, so cleanup was skipped exactly when it was needed.fs:fileWalknil-dereferenced on an error-yieldingDirEntries.fs:RemoveAll(".")now dispatches through aRootRemoverinterface, joins per-entry errors rather than returning the first, and threads prefixes correctly through recursion.fs:CopycompareddstFS == srcFSdirectly, which is both wrong for distinct values on one backend and a panic risk. Replaced with theSameBackendoptional interface, implemented by both backends.Test coverage
internal/testutilfor Write, Remove, RemoveAll, DirEntries and WalkFiles, so the local and S3 backends are held to the same behavior. A configuration knob exists only where the two genuinely disagree.DeleteObject/DeleteObjectsrecorded keys but never removed the object, which stayed visible to Head/Get/List. Every S3 deletion test had been asserting which requests were sent, never what the bucket contained. The new RemoveAll contract failed 5 of 8 subtests against it. The mock now deletes for real; no existing test depended on the old behavior.Read/Seek, HTTP/2 connection reuse after a partial read, concurrentMultiCopierreuse, and ContentLength sniffing against a live endpoint.file.go:NNline references removed — they are wrong the first time anything above them changes.Verification
Every behavioral fix was mutation-tested to prove the test is not vacuous: reverting the symlink skip fails with
-rwxrwxrwx; restoring the0000sentinel fails; removing theComparableguard panics; dropping the context-checking reader fails four cancellation tests; building the S3 prefix without the trailing/fails the sibling subtest naming all three survivors; splitting the delete batch fails all four batch tests.Green at every commit:
gofmt,go vet ./..., the full suite,-raceon./fs/..., andGOOS=windows go build ./... && GOOS=windows go vet ./fs/local/.Not verified: the S3 integration tests.
OCFL_TEST_S3was not set in this environment, so the integration-gated tests skipped throughout and have not run against a live endpoint. The Windows paths are compile-checked only.Review notes
Two findings from my own earlier review did not survive scrutiny and were deliberately not acted on, with the reasoning recorded in the commit messages rather than churning code:
fs/s3's test-file split was arbitrary was mostly wrong. Six files arepackage s3and twenty arepackage s3_test; the pairs I had called arbitrary splits are that boundary and cannot be merged.Closefails, a context cancel, per-key errors inside a successful response — and forcing them through one hook table would be less readable than seven small wrappers.🤖 Generated with Claude Code
https://claude.ai/code/session_01D9DAbTSrBvN6zLnM6zwYW2