Skip to content

feat(cache): support byte ranges on Cache.Open#345

Closed
alecthomas wants to merge 4 commits into
mainfrom
aat/range-support
Closed

feat(cache): support byte ranges on Cache.Open#345
alecthomas wants to merge 4 commits into
mainfrom
aat/range-support

Conversation

@alecthomas

Copy link
Copy Markdown
Collaborator

Summary

Adds single byte-range support to Cache.Open, mirroring the existing ETag conditional-request pattern end to end (client SDK → cache backends → server serving → remote wire).

  • client.Range(start, end int64) — a typed, half-open [start, end) byte range; a negative end means "to the end of the object". e.g. Range(0, 500) is the first 500 bytes, Range(0, -1) the whole object. Out-of-bounds ranges return ErrRangeNotSatisfiable.
  • All backends slice the body efficiently: disk seeks + io.LimitReader, memory slices, S3 issues a single ranged GET (pinned to the object ETag), Remote forwards over the wire. Tiered skips backfill on partial reads so a truncated object is never cached.
  • Serving (httputil) emits 206 Partial Content / 416 Range Not Satisfiable, advertises Accept-Ranges: bytes, and forwards an external client's verbatim Range header (incl. suffix bytes=-N) on the proxy handlers. Stat/HEAD ignores Range.
  • Request-only headers (Range, If-Range, Content-Range) are stripped on PUT so they can't be stored and replayed.

Deliberate scope

  • Single byte range only; multi-range and syntactically-invalid Range headers fall back to a full 200.
  • If-Range supports the entity-tag form only (compared against the stored ETag).

Test plan

  • client: Range formatting, ResolveRange parser edge cases, 206/416 wire mapping.
  • cachetest conformance suite: partial/full/416/If-Range-mismatch/Stat-ignores-Range across all backends (disk, memory, S3, tiered, remote).
  • strategy/apiv1: end-to-end 206/416, suffix range, multi-range fallback, If-Range, HEAD-ignores-Range, and a stored-Content-Range regression test.
  • Full go test, go vet, and golangci-lint clean.

🤖 Generated with Claude Code

alecthomas and others added 4 commits June 23, 2026 16:28
Add Range/If-Range support to Cache.Open mirroring the existing ETag
conditional pattern. RequestOptions gains Range/IfRange fields and a
ResolveRange resolver for a single byte range, gated on the stored ETag
via If-Range. Backends slice the body (disk seek, memory slice, S3
ranged GET) and set Content-Range; out-of-bounds ranges return
ErrRangeNotSatisfiable. Tiered skips backfill on partial reads to avoid
caching truncated objects. Serving emits 206/416 and advertises
Accept-Ranges; Stat ignores Range. Multi-range falls back to a full 200.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address code-review findings on the byte-range support:

- Strip Range, If-Range and Content-Range on PUT (httputil.TransportHeaders),
  alongside the existing If-Match/If-None-Match. A stored Content-Range would
  otherwise be replayed and make a plain GET spuriously answer 206.
- Return the stored headers (not nil) on the disk Seek error path, matching the
  416 path.
- Add tests: stored-Content-Range regression, full-size and suffix
  Content-Length assertions, and a zero-length-object 416 case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the raw-string Range(spec) option with a typed Range(start, end)
taking a half-open byte interval; a negative end means "to the end of the
object" (Content-Length). e.g. Range(0, 500) is the first 500 bytes and
Range(0, -1) the whole object.

The raw HTTP header form is retained as RangeHeader(spec) for forwarding a
client's Range verbatim (used by httputil and for suffix ranges that the
typed form can't express). The wire format and server-side parsing are
unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The client SDK targets the apiv1 endpoint and only needs the typed
Range(start, end) form, so remove the raw RangeHeader option (and its
cache re-export). The server's proxy handlers still forward an external
client's verbatim Range header (e.g. suffix "bytes=-N") by setting the
shared RequestOptions.Range field directly in httputil.

Suffix coverage stays via the existing apiv1 end-to-end test; the
redundant suite subtest is removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alecthomas alecthomas requested a review from a team as a code owner June 23, 2026 10:07
@alecthomas alecthomas requested review from stuartwdouglas and removed request for a team June 23, 2026 10:07
@alecthomas alecthomas closed this Jun 23, 2026

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

Copy link
Copy Markdown

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: 047fa1e5ce

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread internal/cache/range.go
Comment on lines +29 to +30
headers.Set("Content-Range", fmt.Sprintf("bytes */%d", size))
return 0, 0, false, ErrRangeNotSatisfiable

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 Clear Content-Length for 416 range responses

In this RangeNotSatisfiable branch, headers already contains the full object's Content-Length because each backend sets it before calling rangeShortCircuit. ServeCacheHit copies these headers for a 416 response and writes no body, so an out-of-bounds Range request advertises bytes that are never sent, which can make HTTP clients hang or report an unexpected EOF. Clear Content-Length or set it to the actual 416 body length/0 before returning the error.

Useful? React with 👍 / 👎.

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