fix: return delete-marker ObjectInfo from StatObject alongside the error - #2273
Open
allanrogerr wants to merge 3 commits into
Open
fix: return delete-marker ObjectInfo from StatObject alongside the error#2273allanrogerr wants to merge 3 commits into
allanrogerr wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesStatObject error handling
RDMA workflow dependency pinning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StatObject
participant HTTPServer
Client->>StatObject: request object status
StatObject->>HTTPServer: execute object request
HTTPServer-->>StatObject: response or request error
StatObject-->>Client: ObjectInfo and error metadata
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-stat.go`:
- Around line 94-120: Extend the StatObject regression tests to cover HEAD
responses with HTTP 202 and 204 status codes, asserting they return successfully
without an error. Add the remaining regression cases required by the PR
objectives, including validation of the existing delete-marker metadata
behavior, while keeping assertions focused on the public StatObject result.
- Around line 95-119: Extract the response/error handling block from StatObject
into a focused helper that accepts the response, error, request options, bucket
name, and object name, then returns ObjectInfo and error. Preserve the existing
nil-response behavior, versioned delete-marker 405 ErrorResponse construction,
and metadata population for all other responses; have StatObject delegate to
this helper so the delete-marker logic is independently testable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28d1b5a4-2829-4cb2-9a6f-c3acbde035da
📒 Files selected for processing (2)
api-stat.goapi-stat_test.go
StatObject's delete-marker block became unreachable for its intended 405/404 paths after minio#2115 made executeMethod error on every response outside its success set. Move the block into the error branch so VersionID and IsDeleteMarker plus the MethodNotAllowed code (405 path) and ReplicationReady (other error responses) reach callers again, as before v7.0.93. Fixes minio#2260
allanrogerr
force-pushed
the
issue-2260-statobject-delete-marker-info
branch
from
August 1, 2026 12:05
30f3a86 to
2bd0f11
Compare
The go-rdma workflow checks out microsoft/vcpkg master at run time. Since vcpkg commit 5397c5c9f its port scripts use string(JSON ... STRING_ENCODE), which needs a newer CMake than the ubuntu-24.04-arm runner provides, so every arm64 run fails while building openssl before Go is even set up. Pin vcpkg to the commit behind release 2026.06.24, the last release that predates the change; the amd64 lane is unaffected because vcpkg bootstraps its own newer CMake there. The tag anchor comment records the human-readable version for future bumps. No third-party container images exist in this repo's workflows to digest-pin alongside it.
10 tasks
allanrogerr
marked this pull request as ready for review
August 1, 2026 13:16
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-stat_test.go`:
- Around line 33-236: Extract the repeated httptest server setup and client
construction from TestStatObjectDeleteMarker,
TestStatObjectMethodNotAllowedGeneric, TestStatObjectNoContentSuccess,
TestStatObjectNoResponse, and TestStatObjectErrorHeaders into a shared
newTestStatClient helper. Have the helper accept the request handler, register
server cleanup, construct the configured Client, and fail the test on setup
errors; update each test to use it while preserving its existing handler and
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1317c55f-3682-4bbc-b1c6-71e79e6e3b6a
📒 Files selected for processing (3)
.github/workflows/go-rdma.ymlapi-stat.goapi-stat_test.go
…hygiene Correct the StatObject error-branch comment and godoc to state only what each path surfaces, restore the ReplicationReady field note, extract a newTestStatClient helper with strengthened delete-marker assertions, fix the vcpkg pin comment (CMake >= 4.3 arrives via vcpkg's per-arch tool bootstrap, absent on arm64), and set persist-credentials: false on all three go-rdma.yml checkouts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/go-rdma.yml:
- Line 31: Update every actions/checkout step in the go-rdma workflow, including
the minio-go, minio-cpp, and vcpkg checkout steps, to reference the intended
actions/checkout v4 release by its full immutable commit SHA instead of the
mutable `@v4` tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b853a8f7-d94c-49f8-ab93-b25c80cd27b9
📒 Files selected for processing (3)
.github/workflows/go-rdma.ymlapi-stat.goapi-stat_test.go
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
1 participant
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.
Description
Since #2115 (first released in v7.0.93),
executeMethodreturns a non-nil error for every response outside its success set — 200/204/206 then, 202 added later by #2252 — soStatObjectearly-returns a zeroObjectInfobefore its delete-marker handling block can run — leaving the block unreachable for its intended 405/404 delete-marker paths. This PR moves that logic into theerr != nilbranch, restoring the pre-v7.0.93 behavior:VersionID, answered 405 +x-amz-delete-marker: true) again returnsObjectInfo{VersionID, IsDeleteMarker}alongside aMethodNotAllowedErrorResponse(synthesized exactly as before v7.0.93; the parsed server fields are not merged in, andReplicationReadyis likewise not merged into this return — pinned by the tests).ObjectInfo(VersionID,IsDeleteMarker,ReplicationReady) alongside the errorexecuteMethodalready parsed.resp == nil),StatObjectstill returns a zeroObjectInfowith the error. The old block is removed.executeMethodtreats them as success). No known S3 server returns these for HEAD.go-rdma.ymlvcpkg checkout to an immutable commit (cd61e1e2, tag2026.06.24). vcpkg master now requires CMake >= 4.3, which vcpkg's tool bootstrap downloads on linux-amd64 but not on linux-arm64 (falling back to apt's CMake 3.28), failing every arm64 run since 2026-07-30 — unrelated to this fix, but it blocks CI here.Fixes #2260
Is this a breaking change?
No — this is a regression-fix branch, not a breaking-change branch:
StatObject's signature,ObjectInfo's fields, and all exported identifiers are untouched; every existing caller compiles unchanged.executeMethodbuilds it viahttpRespToErrorResponse, sominio.ToErrorResponse(err)keeps working exactly as before.VersionID/IsDeleteMarker/ReplicationReadyalongside the error is whatStatObjectdid before v7.0.93 and what StatObject no longer returns delete-marker ObjectInfo (VersionID/IsDeleteMarker) alongside the error — regression since v7.0.93 (#2115) #2260 reports as broken; v7.0.93 (Add support of error responses in 200 OK body #2115) was the unintentional behavior change, and this PR undoes it. A caller is only affected if it relied onObjectInfobeing zero-valued whenevererr != nil— an anti-pattern the docs never promised (the new doc comment now spells the contract out).go-rdma.ymlcheckout pin, new unit tests) — no public-API impact.Motivation and Context
Callers that detect delete markers from
StatObjecterrors (behavior added in #1379/#1397) silently lostVersionID,IsDeleteMarker, andReplicationReady, and — for empty-body 405 responses — theMethodNotAllowederror code.How to test this PR?
go test -race -run TestStatObject .— five new regression tests inapi-stat_test.go:TestStatObjectDeleteMarker: 405 + delete-marker headers → populatedObjectInfo,MethodNotAllowedcode and 405 status. Fails on master, passes here.TestStatObjectMethodNotAllowedGeneric: 405 missing either half of the delete-marker shape (no marker header, or no version-targeted stat) → the generic path with the raw status code. Fails on master, passes here.TestStatObjectNoContentSuccess: 202/204 → parsed like a 200 (nil error, header-derived fields). Fails on master, passes here.TestStatObjectErrorHeaders: 404 with delete-marker/replication headers → all three fields survive alongsideNoSuchKey. Fails on master, passes here.TestStatObjectNoResponse: unreachable endpoint → zeroObjectInfowith the error.go test -short -race ./...passes (16 packages) on this branch (base802bd60).MethodNotAllowed/NoSuchKeywith populated info. The repro test also passes on v7.0.92, bracketing the regression to Add support of error responses in 200 OK body #2115. Note: servers that send an XML error body (such as MinIO) supply theMethodNotAllowedcode from the body, so the error-code half of the regression manifests only with empty-body 405s — pinned by the unit tests.Types of changes
Checklist:
Summary by CodeRabbit