s3: honor If-Match on DELETE

[bootjp]

Fixes the third P2 from #1222 ("Make the bundled S3 endpoint honor delete preconditions"), split out because it changes adapter/s3.go rather than internal/snapshotoffload.

Behavior change

DELETE /{bucket}/{key} now honors If-Match. A request whose If-Match does not equal the object's current ETag gets 412 PreconditionFailed and the object is left in place. Requests without the header are unchanged.

If-None-Match is deliberately not honored on DELETE: "only if absent" is meaningless for an operation whose purpose is to remove something that exists, and honoring it would refuse every delete of a present object.

Why this matters beyond spec compliance

validateS3PutPreconditions already enforces If-Match on the write path. deleteObject loaded the object manifest — so it had the current ETag in hand — and then dispatched kv.Del without consulting the header.

Accepting a precondition and ignoring it is worse than not supporting it. A caller that reads an object, decides it is reclaimable, and sends a conditional delete believes the condition protects it against a concurrent rewrite. It did not: the object was removed whatever its current ETag, so the rewrite was silently lost.

internal/snapshotoffload retention depends on this directly — DeleteObjectIfUnmodified sends If-Match and maps the 412 to ErrObjectModified — so against the bundled endpoint that precondition was inert, and retention could reclaim a payload that had been republished after it was observed.

The check runs against the manifest the surrounding transaction already loaded at its read timestamp, so it compares against the same version the delete will remove, not a separately-read one that could have moved in between.

Risk

Low, and confined to requests that send a header the endpoint previously ignored:

• No header → unchanged (test).
• Absent key → still idempotent 204, with or without the header (test). A precondition cannot be evaluated against something that does not exist.
• Matching ETag → still deletes (test), quoted or unquoted (test), since quoted is the wire form AWS uses.

A client that was sending If-Match and relying on the delete happening regardless would now see a 412 — but that client was already relying on a precondition not being enforced, which is the bug.

Test evidence

• go test ./adapter/ -race -count=1 -timeout 40m — ok 681.541s (the 600s default is not enough for this package under -race)
• golangci-lint --config=.golangci.yaml run ./adapter/... — 0 issues
• Revert-check: dropping the precondition call makes TestS3ConditionalDeleteRefusesAStaleETag FAIL ("a delete conditioned on a superseded ETag must fail with 412"); the file restores byte-exact.

The end-to-end tests drive the real HTTP handler — PUT, concurrent rewrite, conditional DELETE, then HEAD to confirm the object survived — rather than calling the predicate directly, so they cover the wiring and not just the decision. TestValidateS3DeletePreconditions covers the predicate's cases separately.

Self-review

1. Data loss — this prevents a loss: the point is that a conditional delete no longer removes a version the caller never saw. Nothing new is written, and a refused delete dispatches nothing.
2. Concurrency / distributed failures — the comparison uses the manifest the enclosing transaction already loaded at its read timestamp, inside retryS3Mutation, so it cannot compare against a version other than the one being deleted. A rewrite landing after the check is handled by the existing OCC retry, the same as before.
3. Performance — no extra read: the manifest was already loaded. One header lookup and a string compare on a path that is already doing a Raft round trip.
4. Data consistency — no timestamps issued; readTS and startTS flow through untouched, so MVCC visibility and OCC validation are unchanged. The new 412 is produced before the dispatch, so no partial mutation is possible.
5. Test coverage — stale ETag, matching ETag, quoted ETag, no header, absent key with and without the header, plus the predicate in isolation including the deliberate If-None-Match non-behavior.

deleteObject needed the mutation body extracted into dispatchConditionalObjectDelete to stay inside its cyclop budget; that helper is pure motion plus the new check.

@codex review
@claude review

[coderabbitai]

Warning

Review limit reached

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 697fa779-7571-4b01-8cad-d358fb798c2c

📥 Commits

Reviewing files that changed from the base of the PR and between 7a06175 and 47959f5.

📒 Files selected for processing (2)

• adapter/s3.go
• adapter/s3_conditional_delete_test.go

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.