Skip to content

adapter/s3: object-level admin RPCs (Phase 2b)#811

Merged
bootjp merged 7 commits into
mainfrom
feat/admin-s3-object-crud
May 23, 2026
Merged

adapter/s3: object-level admin RPCs (Phase 2b)#811
bootjp merged 7 commits into
mainfrom
feat/admin-s3-object-crud

Conversation

@bootjp
Copy link
Copy Markdown
Owner

@bootjp bootjp commented May 23, 2026

Summary

Phase 2b of docs/design/2026_05_22_implemented_admin_data_browser.md — the S3 object-level admin RPCs that mirror the Phase 2a (PR #805) DynamoDB item-level surface. Four methods on *adapter.S3Server, each delivered as its own commit:

Slice Method Role Notes
1 AdminDeleteObject(ctx, principal, bucket, key) error write Idempotent (missing object → nil, matching AWS S3 + SigV4 path)
2 AdminPutObject(ctx, principal, bucket, key, body io.Reader, contentType string) error write Streaming upload, 100 MiB cap (ErrAdminUploadTooLarge); chunk-pipeline matches SigV4 putObject
3 AdminGetObject(ctx, principal, bucket, key) (io.ReadCloser, AdminObject, error) read Lazy chunk-fetching reader bounded to ~1 chunk; caller MUST Close to release the read-pin
4 AdminListObjects(ctx, principal, bucket, opts) (AdminObjectListing, error) read Prefix + delimiter; reuses SigV4 s3ContinuationToken cursor shape; MaxKeys [1, 1000] default 100

Total: ~1100 LOC code + ~800 LOC tests across 4 commits, no changes to existing code paths.

Authorization

Operation Gate
Get / List principal.Role.canRead() (RoleReadOnly or RoleFull)
Put / Delete principal.Role.canWrite() (RoleFull only)
All isVerifiedS3Leader(ctx) up-front check + validateS3BucketName for the bucket arg

Sentinels

Sentinel Maps to
ErrAdminForbidden (reused from dynamodb_admin.go) 403
ErrAdminNotLeader (reused) 503
ErrAdminBucketNotFound (reused from s3_admin.go) 404
ErrAdminInvalidBucketName (reused) 400
ErrAdminObjectNotFound (new) 404 (only on AdminGetObject; AdminDeleteObject is idempotent)
ErrAdminInvalidObjectKey (new) 400
ErrAdminUploadTooLarge (new) 413

Caller audit (semantic-change rule)

No existing signatures changed. The SigV4 paths (getObject, putObject, deleteObject, listObjectsV2) and their helpers (loadBucketMetaAt, loadObjectManifestAt, cleanupManifestBlobsAsync, encode/decodeS3ContinuationToken, s3keys.*) are unchanged. The admin RPCs are gated wrappers that produce the same Raft OperationGroup shapes as the SigV4 path.

Two new private helpers: adminDeleteObjectTxn (slice 1), adminPutObjectStream (slice 2), adminLoadObjectForRead (slice 3). All file-local.

The chunk-pipeline //nolint:cyclop,gocognit,nestif on adminPutObjectStream and the scan-loop nolint on AdminListObjects mirror the existing precedent on SigV4 putObject / listObjectsV2 (same N-stage sequential flow with per-stage cleanup-on-error).

Risk

Low. The Admin methods are gated wrappers over the same primitives (coordinator.Dispatch, store.ScanAt, store.GetAt, s3keys.*) the SigV4 path uses today. No new write codepath, no new on-disk format.

Self-review (5 passes)

  1. Data loss — All writes go through the same OperationGroup shape as SigV4 (manifest put + bucket-fence put on AdminPutObject; manifest Del on AdminDeleteObject). Cleanup-on-error during chunked upload synchronously deletes orphan blobs before returning to the caller, matching SigV4's cleanupManifestBlobs discipline.
  2. Concurrency — Leader-only via isVerifiedS3Leader. Reads pin readTS for the duration of the manifest+chunks fetch (released by ReadCloser.Close); list-path pin released by defer. retryS3Mutation wraps AdminDeleteObject per the SigV4 precedent.
  3. PerformanceAdminGetObject memory is bounded to ~s3ChunkSize (1 MiB) regardless of object size. AdminPutObject chunks at s3ChunkBatchOps per dispatch. AdminListObjects paginates at s3ListPageSize.
  4. Consistency — Manifest reads use the same loadObjectManifestAt path as SigV4. List cursors are generation-fenced (token mismatch → reject) so a re-created bucket cannot leak stale entries to a continuing walk.
  5. Test coverage — 28 new unit tests across the four slices: happy paths + role denials + missing-bucket + missing-object + idempotency + chunk-boundary streaming + pagination round-trip + content-type defaulting + Close-after-Close + oversized-body rejection.

Test plan

  • go test -race -count=1 -timeout=180s ./adapter/... — passes
  • golangci-lint --config=.golangci.yaml run ./adapter/... — clean (the four nolint markers match the SigV4 precedents)
  • CI on this PR (Jepsen / build / CodeQL)

Phase plan

  • Phase 2a: DynamoDB item RPCs — shipped (adapter/dynamodb: item-level admin RPCs (Phase 2a) #805)
  • Phase 2b: this PR (S3 object RPCs)
  • Phase 3: HTTP handlers + bridges + integration tests
  • Phase 4: SPA DynamoDetail Items tab
  • Phase 5: SPA S3Detail Objects tab + Upload
  • Phase 6: Doc rename _proposed__implemented_

bootjp added 4 commits May 23, 2026 21:02
First of four S3 object-level admin RPCs from
docs/design/2026_05_22_proposed_admin_data_browser.md §3.1.2.

Mirrors the Phase 2a admin item RPCs on the Dynamo side (gating
pattern, sentinel translation, delegation to internal helpers
the SigV4 path already exercises). Subsequent slices add
AdminPutObject, AdminGetObject, AdminListObjects in the same
branch.

## Surface

  - AdminDeleteObject(ctx, principal, bucket, key) error — write
    role; idempotent (missing object → nil, matching the SigV4
    deleteObject path and AWS S3 semantics)
  - ErrAdminObjectNotFound — for the read paths landing in
    later slices; AdminDeleteObject does NOT return this since
    delete is idempotent
  - ErrAdminInvalidObjectKey — empty / malformed key

Reuses ErrAdminBucketNotFound / ErrAdminForbidden /
ErrAdminNotLeader / ErrAdminInvalidBucketName from the existing
S3 admin sentinel set.

## Authorization

  - principal.Role.canWrite() (RoleFull only)
  - isVerifiedS3Leader up-front check
  - Bucket-name validation matches AdminCreateBucket's contract

## Caller audit (semantic-change rule)

adminDeleteObjectTxn is a new private helper; the SigV4
deleteObject path is unchanged. The Admin call is a pure gated
wrapper that produces the same Raft op-group (single Del on the
manifest key) followed by the same async blob-chunk cleanup
(cleanupManifestBlobsAsync). No new write paths introduced.

## Tests

Five unit tests:
  - HappyPath — put via SigV4, delete via admin, verify HEAD
    returns 404
  - Idempotent — second delete of same key returns nil; delete
    of never-existed key returns nil (matches AWS contract)
  - MissingBucket — returns ErrAdminBucketNotFound
  - RejectsReadOnly — returns ErrAdminForbidden and leaves
    state unchanged (HEAD still 200)
  - RejectsEmptyKey — returns ErrAdminInvalidObjectKey
  - RejectsInvalidBucketName — returns ErrAdminInvalidBucketName

## Verification

  - go test -race ./adapter/... passes (5 new tests).
  - golangci-lint clean (cyclop satisfied via the
    adminDeleteObjectTxn split, mirroring AdminCreateBucket's
    txn-body extraction pattern).
Second of four S3 object-level admin RPCs from
docs/design/2026_05_22_proposed_admin_data_browser.md §3.1.2.

## Surface

  - AdminPutObject(ctx, principal, bucket, key, body io.Reader,
    contentType string) error — write role; create-or-replace
    semantics
  - adminS3UploadMaxBytes = 100 MiB (design §3.3.3)
  - ErrAdminUploadTooLarge sentinel (HTTP bridge → 413)

body is consumed exactly once; caller owns lifecycle (admin code
does NOT Close the reader). contentType is stored verbatim on
the manifest; empty defaults to application/octet-stream.

## Implementation

adminPutObjectStream is a single-attempt (no retry) chunked
upload mirroring the SigV4 putObject pipeline:
  1. load bucket meta + previous manifest
  2. stream body through s3ChunkSize buffers
  3. dispatch in batches of s3ChunkBatchOps chunks
  4. final commit (manifest + bucket fence) in one OperationGroup
  5. async cleanup of the previous manifest's blob chunks

Per-stage error path runs cleanupManifestBlobs synchronously so
a partial upload that fails mid-way does not leak orphan blobs.
On commit-success the caller fires cleanupManifestBlobsAsync
for the REPLACED manifest only.

The carrier nolint matches the existing SigV4 putObject
precedent (s3.go's `//nolint:cyclop,gocognit,gocyclo,nestif`):
the chunk pipeline is a sequential N-stage flow with per-stage
cleanup that needs the local manifest view at every stage.

## Caller audit (semantic-change rule)

adminPutObjectStream is a new private helper; SigV4 putObject is
unchanged. The admin path does NOT do the SigV4 path's
sha256 / x-amz-content-sha256 / IfMatch / chunked-transfer
preconditions — those are SigV4 wire concerns and the in-process
Go API does not need them. The dispatch + manifest commit shape
is identical to SigV4: same OperationGroup layout, same key
prefixes, same cleanup contract.

## Tests

Seven unit tests:
  - HappyPath_RoundTripsViaSigV4Get — admin PUT followed by SigV4
    GET; verifies body + Content-Type round-trip
  - DefaultsContentType — empty content-type stored as
    application/octet-stream
  - ReplacesExisting — second PUT overwrites first
  - MissingBucket — returns ErrAdminBucketNotFound
  - RejectsReadOnly — returns ErrAdminForbidden
  - RejectsEmptyKey — returns ErrAdminInvalidObjectKey
  - RejectsOversizedBody — body > 100 MiB returns
    ErrAdminUploadTooLarge AND no manifest is committed
  - AcceptsNilBody — nil reader treated as zero-byte payload;
    object is readable as 0 bytes

## Verification

  - go test -race ./adapter/... passes (7 new tests including
    the 100 MiB cap boundary).
  - golangci-lint clean.
Third of four S3 object-level admin RPCs from
docs/design/2026_05_22_proposed_admin_data_browser.md §3.1.2.

## Surface

  - AdminObject — admin-facing metadata projection (Key, Size,
    ContentType, ETag, LastModified time.Time, StorageClass).
    No internal-only state (uploadID, chunk sizes, ...) leaks
    to admin handlers / SPA.
  - AdminGetObject(ctx, principal, bucket, key) (io.ReadCloser,
    AdminObject, error) — read role; caller MUST Close the body
    to release the read-tracker pin.

## Implementation

s3AdminObjectReader is a lazy chunk-fetcher that walks the
manifest parts on demand. Memory bounded to ~s3ChunkSize per
in-flight chunk regardless of object size (so older objects
predating the 100 MiB admin PUT cap stream without ballooning
the leader's heap).

Lifecycle: the reader owns the pin acquired by adminLoadObjectForRead
and releases it on Close. Close is idempotent (second call is a
no-op); Read after Close returns io.ErrClosedPipe.

adminLoadObjectForRead is the per-call read-path preamble:
acquire pin, load bucket meta, load manifest, with strict
release-on-error discipline so a caller hitting any early-exit
path never has to handle pin cleanup itself.

## Caller audit (semantic-change rule)

s3AdminObjectReader is a new private type; adminLoadObjectForRead
is a new helper. SigV4 getObject + streamObjectChunks are
unchanged. The admin GET path does NOT do the SigV4 path's
Range / If-Match / Content-Range work — admin reads are full-body
only (the Web Console downloads always serve the whole object).

## Tests

Eight unit tests:
  - HappyPath — admin GET after admin PUT; verifies body + all
    metadata fields
  - StreamsMultipleChunks — payload of 3*s3ChunkSize+17 bytes
    exercises the multi-chunk reader path
  - MissingObject — returns ErrAdminObjectNotFound
  - MissingBucket — returns ErrAdminBucketNotFound
  - AllowsReadOnly — read role suffices (regression: a future
    canWrite() refactor would silently break the read-only
    operator dashboard)
  - DefaultsContentType — empty stored content-type renders as
    application/octet-stream on the admin shape
  - CloseIsIdempotent — second Close is a no-op; Read after
    Close errors
  - RejectsEmptyKey — returns ErrAdminInvalidObjectKey

## Verification

  - go test -race ./adapter/... passes (8 new tests).
  - golangci-lint clean (cyclop on AdminGetObject satisfied via
    the adminLoadObjectForRead split).
Final S3 object-level admin RPC from
docs/design/2026_05_22_proposed_admin_data_browser.md §3.1.2.

## Surface

  - AdminListObjectsOptions { Prefix, Delimiter, ContinuationToken,
    MaxKeys [1, 1000] default 100 }
  - AdminObjectListing { Objects, CommonPrefixes,
    NextContinuationToken }
  - AdminListObjects(ctx, principal, bucket, opts) (listing, error)
    — read role; reuses the SigV4 s3ContinuationToken cursor
    shape so the encoder + decoder stay in one place
  - defaultS3ContentType constant extracted (3 use sites in
    this file)
  - clampAdminListMaxKeys helper for the [1, 1000] / default-100
    folding

## Implementation

Mirrors the SigV4 listObjectsV2 scan-and-collapse pipeline:
  1. decode continuation token (uses ReadTS from token if set)
  2. load bucket meta at that readTS (404 if absent)
  3. validate token-bucket / generation / prefix / delimiter
     match the request (cursor stability)
  4. pin readTS
  5. scan manifest keys in s3ListPageSize batches
  6. per row: prefix-filter, then either emit a CommonPrefix
     (delimiter mode) or decode the manifest and emit an
     AdminObject
  7. on emitted >= maxKeys, encode the next cursor and stop

Carries `//nolint:cyclop,gocognit,gocyclo,nestif` matching the
SigV4 listObjectsV2 precedent — the scan-collapse-emit pipeline
is sequential with per-stage error returns; splitting further
would obscure the cursor lifecycle.

## Caller audit (semantic-change rule)

Reuses the existing `s3ContinuationToken` JSON shape and
encode/decode functions. SigV4 listObjectsV2 is unchanged. The
admin path uses the same generation-fence on tokens (mismatch
→ rejected with ErrAdminInvalidBucketName-wrapped error) so
operators cannot accidentally walk a stale cursor through a
post-recreated bucket.

## Tests

Eight unit tests:
  - Empty — returns non-nil Objects: [] (JSON contract)
  - FlatListing — three keys, lexically ordered
  - PrefixFiltersAndCollapsesWithDelimiter — verifies both
    pure-prefix (no collapsing) and delimiter (subfolders
    collapse to CommonPrefixes) modes against the same fixture
  - MaxKeysClamped — MaxKeys=2 paginates, MaxKeys=0 defaults to
    100, MaxKeys=10000 clamps to 1000
  - PagesViaContinuationToken — three pages of 2/2/1 with
    cursor round-trip; concatenation matches the full set
  - MissingBucket — ErrAdminBucketNotFound
  - AllowsReadOnly — read role suffices (regression guard)
  - RejectsAnonymous — AdminPrincipal{} (zero-role) → ErrAdminForbidden

## Phase 2b complete

This slice completes the four S3 object-level admin RPCs the
design's §3.1.2 calls for. Combined with slices 1-3 (Delete /
Put / Get) the *adapter.S3Server type now mirrors the
*adapter.DynamoDBServer's Phase-2a item-level surface:
  - read: AdminListObjects, AdminGetObject
  - write: AdminPutObject, AdminDeleteObject

Phase 3 (HTTP handlers + bridges + integration tests) is next.

## Verification

  - go test -race ./adapter/... passes (all 28 admin object
    tests across the 4 slices).
  - golangci-lint clean (cyclop / gocognit / gocyclo / nestif
    nolints match the SigV4 precedent at s3.go).
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented May 23, 2026

@claude review

Phase 2b — S3 object-level admin RPCs. Four commits, one per method:

  • 1ffc27f: AdminDeleteObject (idempotent)
  • 04d3168: AdminPutObject (streaming, 100 MiB cap)
  • adadde6: AdminGetObject (lazy chunk-fetching ReadCloser)
  • cc31e7a: AdminListObjects (prefix + delimiter + cursor pagination)

Mirrors Phase 2a (#805) DynamoDB pattern: gated wrappers over the same primitives the SigV4 path uses. No changes to existing SigV4 codepaths. 28 unit tests; golangci-lint clean.

Self-review notes are in the PR body.

@claude
Copy link
Copy Markdown

claude Bot commented May 23, 2026

Claude finished @bootjp's task in 4m 0s —— View job


Code Review — PR #811: adapter/s3: object-level admin RPCs (Phase 2b)

Tasks

  • Gather context (PR body, CLAUDE.md, changed files list)
  • Read adapter/s3_admin_objects.go
  • Read adapter/s3_admin_objects_test.go
  • Read existing context files (adapter/s3_admin.go, adapter/s3.go relevant sections)
  • Review Pass 1: Data loss
  • Review Pass 2: Concurrency / distributed failures
  • Review Pass 3: Performance
  • Review Pass 4: Data consistency
  • Review Pass 5: Test coverage
  • Post final review

Overall the implementation is solid and closely mirrors the SigV4 path. The adminLoadObjectForRead ownership-transfer idiom (releaseOnError sentinel nil'd on success) and the s3AdminObjectReader double-Close safety are done correctly. The pattern deviation from SigV4's putObject retry behaviour is documented and acceptable for an admin surface. Two medium issues and several minor points follow.


Pass 1 — Data loss

No data-loss vectors found. AdminDeleteObject dispatches through adminDeleteObjectTxn (manifest key only, OCC-protected via IsTxn+StartTS; matching the SigV4 deleteObject pattern at s3.go:1236-1242) and defers async blob cleanup to cleanupManifestBlobsAsync. AdminPutObject commits manifest + bucket-fence atomically after all chunks are flushed, and calls cleanupManifestBlobs synchronously on every error path before returning. The "previous manifest" async cleanup after a successful replace matches the SigV4 putObject discipline.


Pass 2 — Concurrency / distributed failures

Medium — AdminListObjects does not handle store.ErrReadTSCompacted on paginated scans.

The SigV4 listObjectsV2 checks for this explicitly (s3.go:2105-2108):

if token != nil && errors.Is(err, store.ErrReadTSCompacted) {
    writeS3Error(w, http.StatusBadRequest, "InvalidArgument", "continuation token has expired", ...)
    return
}

AdminListObjects passes store.ErrReadTSCompacted up raw from s.store.ScanAt. When a long-running paginated admin scan outlives the MVCC retention window, the Phase 3 HTTP bridge will receive an opaque internal error instead of a diagnosable sentinel. The bridge cannot currently map this to a useful 400/410 response.

A small addition in the scan loop would close the gap:

page, err := s.store.ScanAt(ctx, cursor, end, pageLimit, readTS)
if err != nil {
    if token != nil && errors.Is(err, store.ErrReadTSCompacted) {
        return AdminObjectListing{}, errors.Wrap(ErrAdminInvalidContinuationToken, "token readTS has been compacted")
    }
    return AdminObjectListing{}, errors.WithStack(err)
}

(Requires adding ErrAdminInvalidContinuationToken as a new sentinel — see Pass 4.)

Fix this →

No other concurrency issues. adminDeleteObjectTxn pins and releases per-attempt inside retryS3Mutation. adminLoadObjectForRead correctly transfers pin ownership to the caller on success and releases on all error paths via the releaseOnError idiom.


Pass 3 — Performance

No new hot-path allocations. Memory is bounded to ~s3ChunkSize for streaming reads. The AdminListObjects per-loop pageLimit calculation correctly caps at remaining when approaching maxKeys. The commonPrefixSeen map and the emitted counter mirror listObjectsV2's commonPrefixSeen + result.KeyCount exactly — no quadratic scan paths.

adminPutObjectStream not retrying on OCC conflict is intentional and documented. The trade-off (lost upload vs. leaked blobs) is correct for the admin surface; just note callers should retry the full AdminPutObject call at the application layer if resilience is needed.


Pass 4 — Data consistency

Medium — Token mismatch sentinel is semantically wrong.

AdminListObjects:637-641:

return AdminObjectListing{}, errors.Wrap(ErrAdminInvalidBucketName,
    "continuation token does not match request")

A mismatch between the token's generation/prefix/delimiter and the current request is not an invalid bucket name. errors.Is(err, ErrAdminInvalidBucketName) returns true for both the actual-bucket-name-invalid path and the token-mismatch path, making them indistinguishable to callers.

The SigV4 path returns a plain HTTP 400 with "InvalidArgument" code for both cases, so there's no SigV4 precedent to mirror here — the admin API has richer sentinel semantics.

Suggest adding ErrAdminInvalidContinuationToken (maps to 400) alongside the existing sentinels and using it for both the mismatch case and the compaction case from Pass 2. Fix this →

Minor — readPin acquired after loadBucketMetaAt in AdminListObjects (lines 629→643).

This matches the SigV4 listObjectsV2 pattern exactly (s3.go:2051→2067), so it is an established codebase idiom, not a new deviation. Noting it here for completeness.

Manifest reads in AdminGetObject use the same loadObjectManifestAt as the SigV4 path; list cursors are generation-fenced (token mismatch → reject). The VersionedBlobKey call in ensureBuffered correctly passes part.PartVersion, which isolates chunks from pre-existing parts. No consistency issues beyond the two items above.


Pass 5 — Test coverage

28 tests cover the happy paths, role denials, bucket-not-found, object-not-found, idempotency, chunk-boundary streaming, content-type defaulting, Close idempotency, and oversized-body rejection. The TestS3Server_AdminGetObject_StreamsMultipleChunks test at 3*s3ChunkSize+17 bytes confirms multi-chunk traversal in ensureBuffered. The round-trip tests via the SigV4 HTTP handlers (putObjectForAdminTest, newS3TestRequest) validate the admin path produces data readable by the public endpoint.

Test gaps tied to the above findings:

Gap Test needed
store.ErrReadTSCompacted on paginated scan TestS3Server_AdminListObjects_ExpiredToken
Token bucket/generation/prefix mismatch TestS3Server_AdminListObjects_MismatchedToken

The existing TestS3Server_AdminListObjects_PagesViaContinuationToken is strong (3-page walk, full reconstitution), but does not cover the rejection paths.

Minor nit — truncated nolint comment on AdminListObjects:

//nolint:cyclop,gocognit,gocyclo,nestif // Sequential scan-loop with

The rationale is cut off mid-sentence (compare the full explanation on adminPutObjectStream:199). Not a functional issue.


Summary

Severity Finding Location
Medium Missing ErrReadTSCompacted guard on paginated scan s3_admin_objects.go:669
Medium Token mismatch returns ErrAdminInvalidBucketName instead of a dedicated sentinel s3_admin_objects.go:639
Minor Two missing test cases for the above s3_admin_objects_test.go
Nit Truncated nolint comment s3_admin_objects.go:607

The code is otherwise clean, well-structured, and consistent with the SigV4 and Phase 2a DynamoDB precedents. The two medium items are straightforward to address and the rest of the implementation is ready.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements object-level admin RPCs for S3, providing AdminDeleteObject, AdminPutObject, AdminGetObject, and AdminListObjects with integrated role-based access control and leader verification. The changes include support for chunked streaming and paginated listings, accompanied by comprehensive unit tests. Review feedback identifies opportunities to improve error reporting by using a more specific error for continuation token mismatches and handling expired read timestamps in the listing operation to maintain consistency with existing SigV4 paths.

Comment on lines +637 to +641
if token != nil && (token.Bucket != bucket || token.Generation != meta.Generation ||
token.Prefix != opts.Prefix || token.Delimiter != opts.Delimiter) {
return AdminObjectListing{}, errors.Wrap(ErrAdminInvalidBucketName,
"continuation token does not match request")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of ErrAdminInvalidBucketName for a continuation token mismatch is misleading. While it correctly maps to a 400 Bad Request at the HTTP boundary, the error message will suggest a bucket name issue rather than a pagination token issue. Consider using a more specific sentinel or a generic validation error.

Comment on lines +669 to +671
if err != nil {
return AdminObjectListing{}, errors.WithStack(err)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When a continuation token is used, the storage layer might return store.ErrReadTSCompacted if the snapshot timestamp has expired. The SigV4 path in s3.go (line 2105) explicitly handles this to return a 400 error. AdminListObjects should likely do the same to ensure consistent error reporting for expired tokens.

page, err := s.store.ScanAt(ctx, cursor, end, pageLimit, readTS)
		if err != nil {
			if token != nil && errors.Is(err, store.ErrReadTSCompacted) {
				return AdminObjectListing{}, errors.Wrap(ErrAdminInvalidBucketName, "continuation token has expired")
			}
			return AdminObjectListing{}, errors.WithStack(err)
		}
References
  1. Centralize common complex logic, such as retry/backoff mechanisms, into shared helper functions to avoid code duplication and improve maintainability.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc31e7aa83

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +668 to +670
page, err := s.store.ScanAt(ctx, cursor, end, pageLimit, readTS)
if err != nil {
return AdminObjectListing{}, errors.WithStack(err)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Translate compacted continuation snapshots to client error

Handle store.ErrReadTSCompacted explicitly in AdminListObjects when a continuation token is supplied. As written, a paginated read resumed after MVCC retention has advanced returns a generic wrapped storage error from ScanAt, so callers cannot distinguish an expired token from a true backend failure (and a future HTTP bridge will likely surface this as a 500). The existing S3 listObjectsV2 path treats the same condition as an invalid/expired continuation token, which lets clients restart listing cleanly.

Useful? React with 👍 / 👎.

…d handling (r1)

Addresses claude-bot r1 + gemini r1 mediums on PR #811:

## Token-mismatch sentinel

Pre-fix, AdminListObjects wrapped continuation-token mismatches
in ErrAdminInvalidBucketName — semantically wrong (the bucket
name is fine; the token is the problem), and
errors.Is(err, ErrAdminInvalidBucketName) returns true for both
the actual bucket-name-invalid path and the token-mismatch
path, making them indistinguishable to callers.

Fix: new ErrAdminInvalidContinuationToken sentinel. The mismatch
path now wraps this instead.

## store.ErrReadTSCompacted on paginated scan

Pre-fix, AdminListObjects passed store.ErrReadTSCompacted up raw
from s.store.ScanAt. The SigV4 listObjectsV2 at s3.go:2105
explicitly handles this — when a continuation token's readTS
has been MVCC-GC'd, return 400 InvalidArgument "continuation
token has expired" rather than a 500.

Fix: same translation, into the same new ErrAdminInvalidContinuationToken
sentinel (wrapped with "continuation token readTS has been
compacted" so operator diagnostics distinguish the case).

## Carrier comment cleanup

The AdminListObjects nolint comment was truncated mid-sentence
(claude-bot r1 nit). Reorganised the comment block so the
prose comes BEFORE the nolint marker, matching the
adminPutObjectStream layout (the second-pass gci auto-fix
disturbed the original).

## Caller audit (semantic-change rule)

  - ErrAdminInvalidContinuationToken is a new sentinel. Only
    caller is AdminListObjects in this file. No external
    callers exist yet (Phase 3 HTTP bridge doesn't ship until
    next phase).
  - The change from ErrAdminInvalidBucketName → the new sentinel
    for the mismatch path NARROWS the error vocabulary. Any
    future caller doing errors.Is(err, ErrAdminInvalidBucketName)
    on AdminListObjects results will now miss the token-mismatch
    case — but since the previous reuse was a bug, this is the
    correct contract.
  - The ErrReadTSCompacted translation is a fail-closed
    enhancement: previously the bridge would render an opaque
    500; now it renders a diagnosable 400. No existing caller
    relies on the 500 shape.

## Regression tests

Two new unit tests:
  - TestS3Server_AdminListObjects_RejectsMismatchedToken
    — verifies both bucket-mismatch AND prefix-mismatch routes
    to ErrAdminInvalidContinuationToken
  - TestS3Server_AdminListObjects_RejectsExpiredToken
    — injects store.ErrReadTSCompacted via a wrapping
    scanAtErrStore on the second page's ScanAt; asserts
    ErrAdminInvalidContinuationToken surface

The scanAtErrStore is a tiny test-only wrapper that pass-through
embeds store.MVCCStore and overrides ScanAt to return a
pre-set error. The pattern can be reused for other
admin-error-translation tests.

## Verification

  - go test -race ./adapter/... passes (2 new + all existing
    admin tests).
  - golangci-lint clean.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2026

Warning

Review limit reached

@bootjp, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 1 review/hour. Refill in 21 minutes and 3 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4cf4261c-a166-45ec-890b-0a57ab145598

📥 Commits

Reviewing files that changed from the base of the PR and between ccae199 and d3f5df6.

📒 Files selected for processing (2)
  • adapter/s3_admin_objects.go
  • adapter/s3_admin_objects_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/admin-s3-object-crud

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented May 23, 2026

@claude review

Round 1 fixes (commit 975d0070) — both gemini + claude-bot mediums on AdminListObjects:

Token-mismatch sentinel: New ErrAdminInvalidContinuationToken sentinel. The mismatch path now wraps this instead of the semantically-wrong ErrAdminInvalidBucketName. errors.Is(err, ErrAdminInvalidBucketName) and errors.Is(err, ErrAdminInvalidContinuationToken) are now distinguishable.

store.ErrReadTSCompacted handling: AdminListObjects now translates the underlying scan error into ErrAdminInvalidContinuationToken (wrapped with "continuation token readTS has been compacted"). Matches the SigV4 listObjectsV2 precedent at s3.go:2105.

Nit: Also fixed the truncated AdminListObjects nolint comment (claude-bot r1 nit — the second-pass gci auto-fix had reordered prose vs marker).

Regression tests:

  • TestS3Server_AdminListObjects_RejectsMismatchedToken — bucket-mismatch AND prefix-mismatch both surface as ErrAdminInvalidContinuationToken
  • TestS3Server_AdminListObjects_RejectsExpiredToken — uses a scanAtErrStore wrapper that injects store.ErrReadTSCompacted on the second-page ScanAt; asserts the translation

Caller audit: ErrAdminInvalidContinuationToken is new, single-caller (this file). The narrowing from ErrAdminInvalidBucketName for the mismatch path NARROWS the error vocabulary — Phase 3 HTTP bridge doesn't ship yet so no callers to migrate. The ErrReadTSCompacted translation is fail-closed (500 → 400, strictly better for the bridge).

go test -race ./adapter/... passes; golangci-lint clean.

Re: the prior CI "failure" — that was reviewdog hitting 401 Bad Credentials trying to post inline comments; the actual lint check reported 0 issues. Pre-existing CI hygiene issue, not a code defect.

@claude
Copy link
Copy Markdown

claude Bot commented May 23, 2026

Claude finished @bootjp's task in 2m 5s —— View job


Code Review — PR #811 Round 2 (commit 975d0070)

Tasks

  • Read adapter/s3_admin_objects.go (sentinel + ErrReadTSCompacted + nolint fixes)
  • Read adapter/s3_admin_objects_test.go (new regression tests)
  • Verify Round 1 fixes are correct and complete
  • Post final review

All three Round 1 medium findings are correctly addressed. Details:

Pass — ErrAdminInvalidContinuationToken sentinel

s3_admin_objects.go:56-66 — The sentinel is well-defined. The doc comment covers all three shapes it represents (malformed, field-mismatch, compacted readTS) and cites the r1 review for traceability. The token-mismatch path at s3_admin_objects.go:652-653 now wraps the correct sentinel; errors.Is(err, ErrAdminInvalidBucketName) and errors.Is(err, ErrAdminInvalidContinuationToken) are distinguishable as intended.

Pass — store.ErrReadTSCompacted guard

s3_admin_objects.go:689-692 — The guard is token != nil && errors.Is(err, store.ErrReadTSCompacted), correctly mirroring the SigV4 precedent at s3.go:2105. Returns errors.Wrap(ErrAdminInvalidContinuationToken, "continuation token readTS has been compacted") — the wrapped message distinguishes this from the field-mismatch case without requiring callers to parse text.

Pass — Nolint comment

s3_admin_objects.go:620//nolint:cyclop,gocognit,gocyclo,nestif // see comment above is complete. The referenced "comment above" (lines 615-618) contains the full rationale. Consistent with adminPutObjectStream:212.

Pass — Regression tests

TestS3Server_AdminListObjects_RejectsMismatchedToken (lines 638-669): covers both bucket-mismatch and prefix-mismatch, both asserting errors.Is(err, ErrAdminInvalidContinuationToken). Clean.

TestS3Server_AdminListObjects_RejectsExpiredToken (lines 682-708): the scanAtErrStore wrapper (lines 714-721) embeds store.MVCCStore and overrides only ScanAt, so loadBucketMetaAt (which uses GetAt) still passes through cleanly on the second-page request. The test correctly exercises the token != nil guard path. The assertion is precise.


One remaining nit — stale Sentinels list in AdminListObjects docstring

s3_admin_objects.go:605-613:

// Sentinels:
//   - ErrAdminForbidden          — principal lacks read role
//   - ErrAdminNotLeader          — follower
//   - ErrAdminBucketNotFound     — bucket absent
//   - ErrAdminInvalidBucketName  — malformed bucket name
//   - ErrAdminDynamoValidation NOT used here — admin-list-objects
//     reuses ErrAdminInvalidBucketName for the only validation path
//     (bucket-name shape); a malformed continuation token comes back
//     as a wrapped json/decode error from the SigV4 helper.

ErrAdminInvalidContinuationToken is now also returned by this function but isn't in the list. The prose "reuses ErrAdminInvalidBucketName for the only validation path" is now inaccurate — the token mismatch/compacted-readTS paths are also sentinel-typed returns. The last sentence ("a malformed continuation token comes back as a wrapped json/decode error") is still accurate for the JSON-decode-failure path at line 633-635, but the framing around it is misleading after the r1 changes. Suggested update:

// Sentinels:
//   - ErrAdminForbidden                  — principal lacks read role
//   - ErrAdminNotLeader                  — follower
//   - ErrAdminBucketNotFound             — bucket absent
//   - ErrAdminInvalidBucketName          — malformed bucket name arg
//   - ErrAdminInvalidContinuationToken   — token field mismatch or compacted readTS
//     (a structurally invalid / non-base64 token comes back as a
//     wrapped json/decode error; the sentinel covers the semantic rejections)

This is a doc-only nit; the runtime behavior is correct. Fix this →


Summary

Both r1 mediums (wrong sentinel for token mismatch; missing ErrReadTSCompacted guard) and the nolint nit are correctly fixed. The regression tests are well-structured and cover exactly the right paths. The implementation is ready to merge; the only open item is the doc-comment nit above.

Claude-bot r2 nit on PR #811: the Sentinels list pre-dates the
r1 ErrAdminInvalidContinuationToken addition. The list omits the
new sentinel and the surrounding prose ("reuses
ErrAdminInvalidBucketName for the only validation path") is now
inaccurate.

Refresh the list to:
  - add ErrAdminInvalidContinuationToken with both shapes it covers
    (field-mismatch + compacted-readTS)
  - drop the stale ErrAdminDynamoValidation-not-used line (vestigial
    from a much earlier draft; never applicable)
  - call out that a structurally invalid (non-base64) token still
    surfaces as a raw json/decode wrapped error, not the sentinel

Doc-only — no runtime behavior change.

go test -race ./adapter/... passes; golangci-lint clean.
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented May 23, 2026

@claude review

Round 2 (commit f642f496, doc-only) — applies the r2 nit you flagged:

The AdminListObjects Sentinels list now:

  • adds ErrAdminInvalidContinuationToken with both shapes it covers (field-mismatch + compacted-readTS)
  • drops the stale "reuses ErrAdminInvalidBucketName for the only validation path" line (no longer accurate after r1)
  • drops the vestigial ErrAdminDynamoValidation not-used note (never applicable in the S3 file)
  • preserves the note that a structurally invalid (non-base64-decodable) token surfaces as a wrapped json/decode error, not as a sentinel

No runtime behavior change. go test -race ./adapter/... passes; golangci-lint clean.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f642f496a6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +647 to +650
meta, exists, err := s.loadBucketMetaAt(ctx, bucket, readTS)
if err != nil {
return AdminObjectListing{}, errors.WithStack(err)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Translate compacted token failures from bucket-meta lookup

When a continuation token’s read_ts has already been compacted, AdminListObjects can fail before ScanAt: loadBucketMetaAt calls GetAt at that stale timestamp and returns store.ErrReadTSCompacted, but this branch currently wraps and returns it as a generic storage error. That means expired tokens still surface as backend failures instead of ErrAdminInvalidContinuationToken in real compaction scenarios (the current translation only handles ScanAt), so callers cannot reliably treat this as a restartable pagination error.

Useful? React with 👍 / 👎.

…eta load (Codex r2 P2)

The r1 ErrReadTSCompacted translation only guarded the ScanAt
call. But AdminListObjects loads the bucket meta FIRST via
loadBucketMetaAt -> GetAt at the token's readTS - if the token
has aged out far enough that even the bucket-meta entry is no
longer fetchable, GetAt returns ErrReadTSCompacted BEFORE the
scan loop runs.

Pre-fix the GetAt error surfaced as errors.WithStack(err) - a
generic 500 to any future bridge - making expired tokens
indistinguishable from a true backend failure on that branch.

Fix: same translation as the ScanAt branch - guard on
`token != nil && errors.Is(err, store.ErrReadTSCompacted)` and
return ErrAdminInvalidContinuationToken wrapped with the same
"continuation token readTS has been compacted" message. Now
both paths a token can reach the compaction error from end at
the same sentinel.

## Caller audit

Single caller (AdminListObjects in this file). Same wrapped
sentinel as the ScanAt branch - no new error shape, just
applied to a second branch.

## Regression test

TestS3Server_AdminListObjects_RejectsExpiredTokenAtBucketMeta
uses a new getAtErrStore wrapper (GetAt-side counterpart of
the existing scanAtErrStore) to inject ErrReadTSCompacted at
the GetAt boundary. Asserts errors.Is(err,
ErrAdminInvalidContinuationToken).

## Verification

  - go test -race ./adapter/... passes (1 new test, all existing
    tests still pass).
  - golangci-lint clean.
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented May 23, 2026

@claude review

Round 3 (commit d3f5df63) — Codex r2 P2:

Translate ErrReadTSCompacted at the bucket-meta load too. The r1 fix only guarded the ScanAt call. But AdminListObjects loads the bucket meta FIRST via loadBucketMetaAtGetAt at the token's readTS — if the token has aged out far enough that even the bucket-meta entry is no longer fetchable, GetAt returns ErrReadTSCompacted BEFORE the scan loop runs. Pre-fix that path surfaced as a generic 500 to the future bridge.

Now both compaction error paths a token can hit end at the same wrapped ErrAdminInvalidContinuationToken sentinel.

Regression test: TestS3Server_AdminListObjects_RejectsExpiredTokenAtBucketMeta uses a new getAtErrStore wrapper (GetAt-side counterpart of the existing scanAtErrStore) to inject ErrReadTSCompacted at the GetAt boundary. Asserts errors.Is(err, ErrAdminInvalidContinuationToken).

Caller audit: single caller; same wrapped sentinel as the ScanAt branch — no new error shape, just applied to a second branch.

go test -race ./adapter/... passes; golangci-lint clean.

Re: the other codex r2 finding ("Translate compacted continuation snapshots to client error") — that's the same issue I addressed in r1 commit 975d0070, but codex was reviewing against the older commit before r1. The new GetAt-side fix in this commit completes the coverage.

@claude
Copy link
Copy Markdown

claude Bot commented May 23, 2026

Claude finished @bootjp's task in 2m 17s —— View job


Code Review — PR #811 Round 3 (commit d3f5df63)

Tasks

  • Load tools
  • Read adapter/s3_admin_objects.go (bucket-meta GetAt compaction guard)
  • Read adapter/s3_admin_objects_test.go (new getAtErrStore + RejectsExpiredTokenAtBucketMeta)
  • Verify the fix is correct and complete
  • Post final review

The Codex r2 P2 finding is correctly addressed. Detailed verification:

Pass — loadBucketMetaAt compaction guard (lines 647–660)

The guard mirrors the ScanAt guard exactly:

if token != nil && errors.Is(err, store.ErrReadTSCompacted) {
    return AdminObjectListing{}, errors.Wrap(ErrAdminInvalidContinuationToken,
        "continuation token readTS has been compacted")
}

Condition, sentinel, and wrapped message are identical to the scan-loop guard at lines 704–707. The token != nil predicate is correct — when no token is supplied readTS is a fresh s.readTS() that cannot have been compacted, so the guard correctly does not fire on first-page requests.

Pass — Comparison with SigV4 precedent

Notably, the admin path is now more correct than the SigV4 path in this dimension: listObjectsV2 at s3.go:2051-2055 calls loadBucketMetaAt at the token's readTS and sends a generic 500 (writeS3InternalError) if that returns ErrReadTSCompacted. The admin path translates both compaction sites — bucket-meta GetAt and scan-loop ScanAt — to the dedicated sentinel. The difference is intentional and desirable: the admin surface has richer sentinel semantics than the SigV4 HTTP path.

Pass — getAtErrStore test wrapper (lines 729–736)

Correctly embeds store.MVCCStore and overrides only GetAt. All other methods — ScanAt, PutAt, etc. — pass through to the real store. The struct pattern is consistent with the existing scanAtErrStore from r1.

Pass — TestS3Server_AdminListObjects_RejectsExpiredTokenAtBucketMeta (lines 744–767)

Test flow is correct:

  1. Creates a real bucket + objects via the real store
  2. Takes a real continuation token from page 1 (so the token has a real encoded ReadTS)
  3. Swaps server.store to getAtErrStore (all GetAt calls poisoned)
  4. Re-runs AdminListObjects with the token → readTS = token.ReadTSloadBucketMetaAt calls GetAt at that readTS → getAtErrStore returns ErrReadTSCompacted → guard at line 655 fires → returns ErrAdminInvalidContinuationToken
  5. Asserts errors.Is(err, ErrAdminInvalidContinuationToken)

The assertion is precise and the test correctly exercises the path that was missing before this commit.


One cosmetic nit (no action required)

The ErrAdminInvalidContinuationToken sentinel doc-comment at line 63 still says:

store.ErrReadTSCompacted on the underlying scan

With the r3 fix it now also fires on the underlying GetAt (bucket-meta load). The high-level description ("the token's readTS has been MVCC-GC'd") is still accurate and the operational meaning to callers is unchanged, so this is purely cosmetic and does not need a follow-up commit.


Summary

All three review rounds are resolved:

Round Finding Status
r1 medium Token mismatch returned ErrAdminInvalidBucketName Fixed (r1 commit 975d007)
r1 medium Missing ErrReadTSCompacted guard on paginated ScanAt Fixed (r1 commit 975d007)
r2 nit Stale Sentinels list in AdminListObjects docstring Fixed (r2 commit f642f49)
r3 P2 Missing ErrReadTSCompacted guard on loadBucketMetaAt GetAt Fixed (r3 commit d3f5df6)

The implementation is clean and complete. Ready to merge.

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.

1 participant