Skip to content

fix: reject short cache range reads - #251

Open
perfloop[bot] wants to merge 6 commits into
mainfrom
perfloop-pr-open-y103a1fep6
Open

perfloop[bot] wants to merge 6 commits into
mainfrom
perfloop-pr-open-y103a1fep6

Conversation

@perfloop

@perfloop perfloop Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Short positive cache range reads are rejected as incomplete instead of accepted as successful cache reads. After a block response commits, TAG can recover the unwritten suffix from an upstream range only when the cached ETag and exact range match. Changed-ETag bytes are rejected, and client write failures are not retried.

The guard applies to block and whole-object cache range reads. Cache-bypass and upstream-miss paths are unchanged; no new setting or S3 route is added. X-Cache: HIT identifies a response selected through a cache entry when headers commit. A block stream may include a verified upstream suffix, and this header does not guarantee per-byte cache provenance or transfer completion. Cache-hit counters follow that committed status; request success/error metrics record completion separately. The metrics guide's source=local share describes the request-handling path, not upstream avoidance or per-byte origin.

If a changed ETag is seen after a full block response commits, TAG does not append the new version's suffix. The handler reports an error, but HTTP 200 and the original Content-Length remain; the client may receive only the cached prefix. A whole-object cached Range GET commits its 206 after the first byte and cannot restart upstream; a later short read returns an error while its 206 and Content-Length remain. The README and S3/cache-control documentation describe these boundaries.

The regression uses the repository-pinned ocache v1.13.0 in-memory client with stored bytes shorter than metadata, plus an httptest.NewServer and the real forwarder for suffix responses. It covers same-ETag recovery, changed-ETag rejection, whole-object Range short reads, and client-write failures. This fixture shows a possible backend short read, not that normal population creates short entries or how often deployed traffic sees one.

The correctness assertion is satisfied on the final source and violated on 4df832ad38e8d9cc6cea919603be1ccd7657b75f. The standalone claim runner is retained proof support and is not part of this pull request.

The block-mode S3 compatibility suite was not run here. make s3-test-local-blocks built TAG but stopped before startup because this runtime has no AWS credentials for Tigris, so make s3-tests did not run. Run those commands in a credentialed environment before merge.

Local validation passed: make lint-ci, env TAR_OPTIONS=--no-same-owner make test, env TAR_OPTIONS=--no-same-owner make test-integration, env TAR_OPTIONS=--no-same-owner make test-race, and env TAR_OPTIONS=--no-same-owner make test-coverage. These checks used Go 1.27.1; the GitHub workflow uses Go 1.24.2. Full repository CI remains pending.


Generated by Perfloop.


Note

Medium Risk
Changes GetObject streaming, block degraded-serve recovery, and request metrics on the cache serve path; committed responses may still be partial on error, but behavior is now explicit and tested.

Overview
Cached range reads must deliver the full requested byte span. getRangeStreamByKey now errors on short or over-long reads from ocache (after writing any partial bytes), so a “successful” cache read cannot silently truncate a Range or block-local slice.

Proxy behavior and observability follow that contract. Block-mode serves can still finish a committed X-Cache: HIT by fetching a same-ETag upstream suffix; changed-ETag remainders are rejected. Whole-object cached Range GETs stop on client write failures without masking them as success. RecordRequestResult records request success/error separately from committed cache hits; docs clarify that HIT and tag_cache_hits_total reflect the cache decision at header commit, not per-byte origin or transfer completion.

Regression coverage adds integration tests for suffix recovery, ETag mismatch, short whole-body ranges, and post-commit write failures.

Reviewed by Cursor Bugbot for commit af4effd. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5 Tier: plus

[Medium risk] Adds validation for incomplete cache range reads.

The reviewed changes appear safe to merge; no actionable issue was found.

What we checked:

  • Valid tail blocks still work: blockLocalRange limits each read to the requested bytes and the object's end. The separate byte-zero path still reads a one-byte block.
  • Recovery cannot mix object versions: streamRemainderFromUpstream checks the ETag and exact range before copying any response bytes. A different ETag returns an error instead.
  • Failed client writes stop: serveRangeFromCache skips the remaining copy after the first write fails and closes the pipe. It returns served=true, so the caller does not forward another response.

Summary

Rejects incomplete cached ranges, stops after a first-byte client-write error, and counts failed cache streams as request errors.

  • Adds regression cases for same-ETag recovery, changed-ETag rejection, short whole-object ranges, and client-write failures.
  • Clarifies that X-Cache: HIT describes the committed cache decision, not every byte's origin or successful completion.
  • perfloop[bot] explicitly acknowledges that failed recovery can leave the sent status and Content-Length unchanged with a short body. Those documented limits are intentional.
  • perfloop[bot] reports that credentialed block-mode S3 tests were not run and full CI remains pending. No tests were run during this review.

Reviews (1) · Last reviewed commit: "docs: label local request-source metric ..." · Reviewed by Greptile

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