fix(snapshot): make snapshot serving range-aware#356
Conversation
worstell
force-pushed
the
fix/snapshot-range-aware-bound-parallelget
branch
3 times, most recently
from
f429e7b to
0a2058f
Compare
worstell
changed the title
![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: 0a2058f62a
ℹ️ 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".
| // A satisfied byte range is served as-is. Bundle negotiation and full-body | ||
| // conditionals apply only to whole-snapshot downloads, so a partial read | ||
| // (e.g. a client.ParallelGet chunk) skips them and returns 206 directly. | ||
| if cr := headers.Get("Content-Range"); cr != "" { |
There was a problem hiding this comment.
When a client sends a satisfiable Range together with a matching If-None-Match (or a stale If-Match), handleSnapshotRequest now opens the cache with RangeOptions, so those validators are not evaluated by the cache; this Content-Range branch then returns before the CheckConditionals call below. That changes conditional range requests from the expected 304/412 into a 206 body response, so revalidating clients redownload chunks and failed If-Match preconditions are not enforced.
Useful? React with 👍 / 👎.
The git snapshot endpoint ignored client Range headers, so it always served the full object. Forward Range/If-Range to the cache on the cache-hit path, return 206/Content-Range (416 when not satisfiable), and advertise Accept-Ranges, so clients can fetch with bounded parallel range requests (client.ParallelGet) instead of one full GET. Snapshot freshen metadata (X-Cachew-Snapshot-Commit / X-Cachew-Bundle-Url) is emitted on 206 responses too, via a shared setSnapshotMetadataHeaders helper, so ranged clients still learn whether to apply a delta bundle. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019ef6fe-55c6-71e8-96dc-ca3ef4301d36
worstell
force-pushed
the
fix/snapshot-range-aware-bound-parallelget
branch
from
0a2058f to
758393a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 758393a234
ℹ️ 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".
| if status := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 { | ||
| w.WriteHeader(status) |
There was a problem hiding this comment.
When a GET or ranged GET includes a matching If-None-Match, this early return writes the 304 before applySnapshotCacheHeaders or setSnapshotMetadataHeaders run. The previous GET path populated the ETag and X-Cachew-* headers before the conditional status, and stale snapshots still need X-Cachew-Bundle-Url so a client revalidating a cached tarball can apply the delta instead of treating that cached snapshot as current.
Useful? React with 👍 / 👎.
2 participants
The git snapshot endpoint ignored client
Rangeheaders, so it always served the full object. It now forwardsRange/If-Rangeto the cache on the cache-hit path, returns206/Content-Range(416when not satisfiable), and advertisesAccept-Ranges: bytes, so clients can fetch with bounded parallel range requests (client.ParallelGet) instead of one full GET.Snapshot freshen metadata (
X-Cachew-Snapshot-Commit/X-Cachew-Bundle-Url) is emitted on206responses too, via a sharedsetSnapshotMetadataHeadershelper used by both the full and ranged paths, so a ranged client still learns whether to apply a delta bundle after a parallel download.Test:
TestSnapshotGetHonorsRangecovers 206/416/Content-Range and matching commit metadata on partial responses, using a non-*os.Filememory cache reader so range support comes from forwardingRangetoOpenrather thanhttp.ServeContent.