fs/s3/internal/mock: give delete and not-found their real shapes - #183
Merged
exe-dev-github-integration[bot] merged 5 commits intoAug 26, 2026
Merged
Conversation
Two of the mock's answers did not match a real endpoint's, and both hid a
defect in the s3 backend.
DeleteObject returned NoSuchKey for a key the bucket did not hold. A real
endpoint answers 204 whether or not the key was there and reports nothing
about which it was -- that idempotency is exactly what makes the backend's
Remove unable to tell a missing key from a deleted one. The mock's opposite
behavior made Remove look strict under test while returning nil in
production. It now deletes unconditionally, still recording the key it
targeted, and still refusing a request naming the wrong bucket.
A missing-key lookup returned a bare *types.NoSuchKey from every operation.
A real one comes back as the operation's own typed error inside the
*smithyhttp.ResponseError carrying the 404, inside the AWS transport's
ResponseError carrying the request ID -- and the typed error differs by
operation: HeadObject has no response body, so the SDK derives the code from
the status and deserializes *types.NotFound, while GetObject, CopyObject and
UploadPartCopy read NoSuchKey out of the body. An S3-compatible store whose
code the SDK cannot resolve to a type produces a *smithy.GenericAPIError
carrying the code as a string instead, so the shape is selectable:
mock.New(bucket) // real-S3
mock.New(bucket).WithNotFoundStyle(mock.NotFoundStyleGeneric) // generic
Contrary to the issue's expectation, this does not turn CI red on its own.
The AWS-style shape keeps the typed error reachable through errors.As, so
errIsNotExist still matches it, and the one assertion that would have failed
-- Remove of a missing key -- is skipped in imptest pending its fix. Both
defects are real and reproduce against the corrected mock; the next two
commits fix them and turn on the assertions that catch them:
Remove("missing.txt") => nil, want fs.ErrNotExist
OpenFile("missing.txt") // generic style => opaque 404 API error,
want fs.ErrNotExist
Refs #166
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXEU5UFn1kPMGpiZGkzjV5
errIsNotExist matched two typed errors and nothing else. Both deserialize
from a response body, and what a store sends back depends on the operation
and on the store:
- GetObject, CopyObject and UploadPartCopy carry NoSuchKey in the body,
which is where the typed errors come from;
- a HEAD response has no body, so the SDK derives the code from the 404
status. Against AWS that still resolves to *types.NotFound, so the
typed check happens to hold;
- an S3-compatible store whose code the SDK cannot resolve to a type
yields a *smithy.GenericAPIError holding the code as a string, which
matched nothing and surfaced as an opaque API error from openFile and
copy.
The check now covers all four: the two typed errors, the API error codes
NoSuchKey and NotFound, and, failing those, a 404 response. A missing bucket
is deliberately excluded even though it is also a 404 -- it is a
configuration error, not a missing file, and a caller told fs.ErrNotExist
would conclude the object was never written when in fact nothing can be read
or written at all.
Separately, both not-exist paths replaced the cause with fs.ErrNotExist,
throwing away the status code, the request ID and the API error code --
most of what makes a failure against a real endpoint diagnosable. notExistErr
wraps instead, so errors.Is still matches and errors.As still reaches the
API error underneath.
The new tests synthesize all four shapes plus the ones the check must not
claim, and assert the cause survives the wrap. Each was checked against a
mutated errIsNotExist and a mutated notExistErr.
Refs #166
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXEU5UFn1kPMGpiZGkzjV5
DeleteObject is idempotent: it answers 204 whether or not the key was there and says nothing about which it was. So Remove returned nil for a key that never existed, while the local backend returned fs.ErrNotExist -- a silent change of meaning when storage moves from a directory to a bucket, and one a caller already depends on. update-plan.go reverts a content copy with Remove and reads fs.ErrNotExist as "nothing to undo"; on s3 that branch was dead code that happened to produce the right answer for the wrong reason. Remove now HEADs the key before deleting it, mapping the probe's not-exist through the wrapping introduced in the previous commit. RemoveAPI gains HeadObject; S3API already embeds OpenFileAPI, so only a standalone RemoveAPI implementer is affected. Two accepted consequences, documented on the method rather than engineered around: the probe is not atomic with the delete, so a key another writer removes in between is reported as removed; and Remove costs two round trips instead of one. Nothing in a commit, read or validate calls Remove at all -- the callers are update-plan's revert paths and the storage root's layout file -- so the extra HEAD lands off the happy path. Both skips naming this issue are gone: the imptest subtest that asserts the contract, and the copy of it quoted in the package doc. The suite now passes against both backends unskipped. Refs #166 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AXEU5UFn1kPMGpiZGkzjV5
removeAll listed a page of up to 1000 keys and then deleted them one request at a time, so removing a 10,000-file OCFL object cost 10,000 sequential round trips. DeleteObjects takes a whole page at once. Batching alone would be a regression, which is why this is one commit and not two: DeleteObjects answers 200 even when individual keys fail, reporting them in the response body. Reading only the transport error would call a RemoveAll successful for a prefix that is still partly populated -- worse than the per-key loop it replaces, which at least saw every failure. Each batch's Errors list is now joined into the returned error, naming the key each entry is about. Both properties #181 established still hold. Deletion is best-effort across the whole listing: a batch that reports failures does not stop the next batch or the next page, and only a failed listing ends the walk. Failures are joined and name the keys, except a transport-level failure, which says nothing about individual keys and is reported once against the name the caller passed rather than repeated for up to a thousand of them. The page size and the batch limit are separate numbers that happen to be equal today, and the equality was accidental. maxDeleteBatch names the API limit, maxKeys is defined in terms of it, and each page is chunked to that size -- so a future page-size bump cannot silently send a request S3 rejects outright. The mock enforces the limit the way a real endpoint does, and a test drives a page larger than a batch through it. RemoveAllAPI now requires DeleteObjects instead of DeleteObject, which breaks any standalone implementer of that interface. S3API is unchanged in substance for implementers of the whole thing only if they add the new method; the mock gains it here. Tests: one request per page with the continuation token threaded, driven by clamping MaxKeys rather than seeding a thousand objects; KeyBatchesFor to catch a regression to per-key deletes, which KeysFor cannot see; quiet mode on every request; a populated Errors list naming the failed key; and a page whose delete fails outright still followed by the next page. TestRemoveAllBestEffort keeps its shadow-one-method fixture, ported to DeleteObjects, and gains a paged case. Each assertion was checked against a mutated implementation: per-key deletes, ignored Errors list, stop-on-first- failure, no chunking, and no quiet mode. Refs #166 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AXEU5UFn1kPMGpiZGkzjV5
The package had no doc comment, so nothing said which SDK calls an operation makes or why the narrow per-operation interfaces exist -- and the two interface changes in this branch break any implementer that does not embed *s3.Client, with no note anywhere to warn them. doc.go covers the construction, the three places S3 semantics differ from a directory's and what the package pays to hide them (Remove's existence probe, RemoveAll's batching and best-effort joining, the not-exist mapping), and a compatibility section naming both interface changes and who each one affects. ocflfs.WriteFS.Remove now says outright that the missing-file case is a guarantee rather than a best effort, and that a backend whose delete cannot supply it pays a round trip to establish it -- the way RemoveAll's comment states its own contract after #181. Callers read fs.ErrNotExist as "there was nothing to remove", so it has to mean the same thing on every backend. Refs #166 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AXEU5UFn1kPMGpiZGkzjV5
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
2 participants
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.
Two of the mock's answers did not match a real endpoint's, and both hid a
defect in the s3 backend.
DeleteObject returned NoSuchKey for a key the bucket did not hold. A real
endpoint answers 204 whether or not the key was there and reports nothing
about which it was -- that idempotency is exactly what makes the backend's
Remove unable to tell a missing key from a deleted one. The mock's opposite
behavior made Remove look strict under test while returning nil in
production. It now deletes unconditionally, still recording the key it
targeted, and still refusing a request naming the wrong bucket.
A missing-key lookup returned a bare *types.NoSuchKey from every operation.
A real one comes back as the operation's own typed error inside the
*smithyhttp.ResponseError carrying the 404, inside the AWS transport's
ResponseError carrying the request ID -- and the typed error differs by
operation: HeadObject has no response body, so the SDK derives the code from
the status and deserializes *types.NotFound, while GetObject, CopyObject and
UploadPartCopy read NoSuchKey out of the body. An S3-compatible store whose
code the SDK cannot resolve to a type produces a *smithy.GenericAPIError
carrying the code as a string instead, so the shape is selectable:
Contrary to the issue's expectation, this does not turn CI red on its own.
The AWS-style shape keeps the typed error reachable through errors.As, so
errIsNotExist still matches it, and the one assertion that would have failed
-- Remove of a missing key -- is skipped in imptest pending its fix. Both
defects are real and reproduce against the corrected mock; the next two
commits fix them and turn on the assertions that catch them:
Remove("missing.txt") => nil, want fs.ErrNotExist
OpenFile("missing.txt") // generic style => opaque 404 API error,
want fs.ErrNotExist
Refs #166
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01AXEU5UFn1kPMGpiZGkzjV5