feat(cache): support ranged reads via io.ReadSeekCloser#342
Conversation
alecthomas
requested review from
jrobotham-square
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afbf5f8299
ℹ️ 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".
alecthomas
force-pushed
the
aat/cache-open-readseekcloser
branch
2 times, most recently
from
f13125c to
f7b0947
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7b09471f5
ℹ️ 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".
alecthomas
force-pushed
the
aat/cache-open-readseekcloser
branch
from
f7b0947 to
cdf9675
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdf9675d39
ℹ️ 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".
Cache.Open now returns io.ReadSeekCloser so cached objects can be served with HTTP Range requests. A reader may be seeked freely before the first read (including io.SeekEnd, resolved against the known size); reading then begins at the final offset and is sequential, with no further seeks permitted. Shared helper: client.SeekReadCloser implements the seek-then-open-once contract — seeks only record the offset (no I/O), the first read opens the stream once at that offset, and a seek to or past EOF reads nothing. Backends: - disk: returns the *os.File directly, which is already seekable. - memory: wraps the *bytes.Reader with a no-op Close. - noop: signature only; still always reports a miss. - s3: stats for headers and size, then opens a single bounded ranged GetObject pinned to the stat'd ETag and extent. The old parallel-get is removed (download parallelism will move to a higher layer); its download-concurrency/download-part-size-mb knobs are retained but deprecated and ignored, warning when set. - remote: a thin pass-through to client.Open, which issues a HEAD for the size and a single ranged GET on the first read, pinned via If-Match to the revision the HEAD observed (so an overwrite in between fails the precondition rather than pairing stale metadata with a newer body). A 200 response to a ranged request is rejected so an ignored Range cannot return bytes from the wrong offset. - tiered: backfill commits tier 0 via the cheap tee only after a full sequential read from offset 0. Any incomplete read — a ranged read (including a zero-based prefix range) or a partial read — discards the tee and warms tier 0 with a deduplicated, request-independent background full copy instead; a close-without-read (e.g. a 304) commits nothing. Serving: httputil.ServeCacheHit serves single byte ranges parsed against the stored Content-Length (206 with Content-Range and Accept-Ranges, 416 for unsatisfiable, multi-range served in full), preserving If-Match and If-None-Match and honouring If-Range (serving the full body when the If-Range validator is stale). Fixes #341 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alecthomas
force-pushed
the
aat/cache-open-readseekcloser
branch
from
cdf9675 to
8a22f2e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
| w.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", start, end, size)) | ||
| w.Header().Set("Content-Length", strconv.FormatInt(length, 10)) | ||
| w.WriteHeader(http.StatusPartialContent) | ||
| _, copyErr := io.CopyN(w, body, length) |
There was a problem hiding this comment.
somewhat of a nit, but this appears true from amp:
The issue says a bytes=0- whole-object range keeps the cheap tee, but it doesn't in practice: this io.CopyN(w, body, size) stops at size without reading the trailing EOF, so backfillReadCloser.completed stays false and teeCommitted is false → the fully-written tee is discarded and a redundant background full copy runs. Functionally fine (tier 0 still ends up warm with the whole object), just an extra full read. Worth either fixing the doc or, if the optimization matters, detecting the full-length 206 case
| // seekable) performs the actual seek. Once reading has begun Seek returns an | ||
| // error. | ||
| func (c *Client) Open(ctx context.Context, key Key, opts ...RequestOption) (io.ReadSeekCloser, http.Header, error) { | ||
| headers, err := c.Stat(ctx, key, opts...) |
There was a problem hiding this comment.
comment from amp review that seems worthwhile:
Concern: every remote
Opennow costs 2 round trips (HEAD + GET) on the hot path.This eager
Stat(HEAD) followed by a lazy GET on first read replaces what used to be a single GET returning headers + body together.Remote.Openpasses straight through, so every remote-tier hit pays an extra request. Nearly all consumers (apiv1, generic handler, gomod, git snapshot, tiered backfill) read the body fully and sequentially and never seek — the HEAD is only strictly needed to resolveio.SeekEnd/range bounds, which only happens for an actualRangerequest.Magnitude: connections are pooled (keep-alive + HTTP/2), so the marginal cost is +1 RTT, not a new handshake — negligible for large objects or a same-region tier. It bites for small-object + high-RTT + serialized workloads (e.g. many small go-module fetches from a distant cache, approaching ~2× wall-clock), and more universally ~doubles request/metadata QPS on the shared cache's busiest endpoint regardless of object size.
Suggested fix: defer the HEAD until a
SeekEnd(or non-zero offset) is actually requested, and let the common offset-0 first read be a plain GET whose 200 supplies the headers. That keeps the no-seek path at one request and only range requests pay for the HEAD. At minimum, worth measuring rather than landing silently.
There was a problem hiding this comment.
It can't be deferred, as the enclsoing function returns the headers from the HEAD.
|
overall LGTM 👍 |
|
I'm actually going to close this out in favour of a simpler approach. Leveraging |
2 participants
Cache.Open now returns io.ReadSeekCloser so cached objects can be served
with HTTP Range requests. A reader may be seeked freely before the first
read (including io.SeekEnd, resolved against the known size); reading
then begins at the final offset and is sequential, with no further seeks
permitted.
Shared helper: client.SeekReadCloser implements the seek-then-open-once
contract — seeks only record the offset (no I/O), the first read opens
the stream once at that offset, and a seek to or past EOF reads nothing.
Backends:
GetObject pinned to the stat'd ETag and extent. The old parallel-get
is removed (download parallelism will move to a higher layer); its
download-concurrency/download-part-size-mb knobs are retained but
deprecated and ignored, warning when set.
size and a single ranged GET on the first read, pinned via If-Match to
the revision the HEAD observed (so an overwrite in between fails the
precondition rather than pairing stale metadata with a newer body). A
200 response to a ranged request is rejected so an ignored Range cannot
return bytes from the wrong offset.
sequential read from offset 0. Any incomplete read — a ranged read
(including a zero-based prefix range) or a partial read — discards the
tee and warms tier 0 with a deduplicated, request-independent background
full copy instead; a close-without-read (e.g. a 304) commits nothing.
Serving: httputil.ServeCacheHit serves single byte ranges parsed against
the stored Content-Length (206 with Content-Range and Accept-Ranges, 416
for unsatisfiable, multi-range served in full), preserving If-Match and
If-None-Match and honouring If-Range (serving the full body when the
If-Range validator is stale).
Fixes #341