Repository navigation
fix(proxy): detect incomplete whole-object cache hits - #249
perfloop[bot] wants to merge 4 commits into
Conversation
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[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !cw.committed { | ||
| return fmt.Errorf("cache body empty for %s/%s", bucket, key) | ||
| } | ||
| if bodyErr == nil && meta.ContentLength > 0 && cw.written != meta.ContentLength { |
There was a problem hiding this comment.
Content-Length, ocache propagates the write error. Because this condition requires bodyErr == nil, TAG skips invalidation after committing the response. Later GETs can encounter the same damaged entry repeatedly. The overlong-body test uses a response recorder that accepts the extra bytes, so it does not cover this path.
There was a problem hiding this comment.
Yes, this is valid. A Content-Length write error can make the cache stream return non-nil before the current nil-error length check runs, leaving an overlong entry in place. I’ll detect bytes offered beyond the recorded length, clean up only the observed metadata version, and test with a writer that enforces Content-Length.
| } else if cacheBodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start, func() { | ||
| s.invalidateStaleMeta(bucket, key, meta.ETag) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, this is valid. An ETag does not distinguish an identical-content replacement, so the streaming cleanup must be tied to the exact metadata version observed by the request. I’ll use the version-guarded deletion path and test a replacement committed while the older stream is held.
| if errors.Is(serveErr, errCacheBodyLengthMismatch) { | ||
| // A damaged body still produces the mode's authoritative miss, but it | ||
| // must not be counted as a successful local object read. | ||
| return s.originlessMissWithRequestStatus(w, r, operation, start, "error") |
There was a problem hiding this comment.
"error" request, while the streaming path separately assigns that status. The repository requires new error classification to be centralized in the metrics layer rather than duplicated in worker code. This requirement must be satisfied before merging so the two paths do not drift.
Rule Used: When adding error classification logic, prefer to centralize it in the metrics layer rather than duplicating the logic across multiple call sites in worker code. Move error classification functions to metrics.go and handle the branching logic within ... (source)
Learned From
tigrisdata/tigris-os#3043
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Yes. The incomplete-body outcome should not set the request status independently in buffered and streaming worker branches. I’ll centralize the local request status classification in the metrics layer and route both paths through it.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ef96731. Configure here.
| // A damaged body still produces the mode's authoritative miss, but it | ||
| // must not be counted as a successful local object read. | ||
| return s.originlessMissWithRequestStatus(w, r, operation, start, "error") | ||
| } |
There was a problem hiding this comment.
HEAD disagrees with short-body GET
Medium Severity
A short buffered local body now fails the new length check and becomes NoSuchKey on GET, but entryServable still treats first-byte presence as full existence. HEAD and conditional 304 therefore succeed from metadata for the same object. Callers that use those answers as an existence signal can skip the fallback this GET now depends on.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ef96731. Configure here.
There was a problem hiding this comment.
This is valid for the tiered local-store path: a confirmed short local body makes GET miss, while metadata-only HEAD or conditional 304 can still signal a usable object. I will align those tiered availability answers for a confirmed mismatch without changing proxy modes that intentionally answer HEAD from metadata.
1 participant
TAG now checks whole-object cache bytes against the positive recorded
ContentLengthbefore treating the local body as a successful GET. A short nil-error stream can otherwise expose a prefix while advertising the full length. Whether this occurs in production remains unmeasured.In transparent and signing modes, a short buffered body is rejected before headers commit; when the origin fallback succeeds, TAG serves its complete response. If fallback fails before upstream headers arrive, TAG returns the error without committing the cached prefix. A short stream that has already committed headers cannot be replaced: TAG records a local request error, keeps the metadata
Content-Length, and does not append origin bytes.Tiered local reads have no upstream body path. For a short buffered local body, TAG discards the prefix, attempts version-guarded metadata cleanup, records a local error, and returns the documented
NoSuchKeymiss so the caller can fall back to its authoritative store. A committed short tiered stream remains incomplete, is recorded as a local error, and has no upstream append. Ordinary tiered misses keep their existing response accounting.The shared whole-body helper also serves non-range 304 and stale responses. Proxy GET, revalidation, and stale paths use the existing ETag guard; tiered local cleanup uses the metadata version read for the request. Cleanup is best effort. If it fails, another request may encounter the same mismatch until metadata expires. Range and block-cache reads, zero-length objects, and unknown-length objects keep their separate existing paths.
The original-defect test
TestHandleGetObject_CacheBodyLengthinjects a short nil-error body into authorized, non-rangeBlockSize == 0GETs below and above 64 KiB. It checks the response body and advertised length, origin calls, request accounting, exact-size and zero-length controls, and tiered local outcomes. The preservation testTestTieredLargeShortBodyCleanupKeepsNewerSameETagPutgates a concurrent identical-content replacement and verifies that cleanup retains its newer metadata version. Additional tests cover repeated GET recovery, non-range revalidation/stale paths, overlong bodies, and pre-header fallback errors.The correctness check recorded violated on test-only comparison revision
2d3e6fede525d4f8127c9b13c5358122f37f5c1dand satisfied on this change. The Python assertion adapter is proof-only support and is not included in the pull request. The injected cache fault does not measure production incidence.The recorded local repository checks passed:
make lint-ci,make build,make test,make test-integration,make test-race, andmake test-coverage.make test-sdkandmake s3-testswere not run because AWS credentials and a running local TAG were unavailable. Relevant SDK coverage includesTestSDK_GetObject_WithCache,TestSDK_LocalAuth_CacheHitAfterKeyLearning, and the sub-block case ofTestSDK_BlockCache_FullGET_SizeRegimesintests/s3compat/sdk; the Python suite selectstest_s3.py::test_object_write_read_update_read_delete. Full repository CI remains pending.Generated by Perfloop.
Note
Medium Risk
Changes core GetObject cache-serve, revalidation, tiered miss semantics, and broadcast error propagation—incorrect length or invalidation logic could cause wrong bodies, stuck listeners, or aggressive cache eviction.
Overview
Whole-object cache GETs now compare bytes actually served to metadata
ContentLength(when length is positive) so a truncated or corrupt body is not treated as a successful local hit.Proxy / transparent modes: A short body detected before headers commit falls through to upstream (no cached prefix on the wire). If upstream fails before headers, the error is returned without committing. After headers are committed on a large stream, TAG records a local request error, runs version- or ETag-guarded metadata cleanup via a new
invalidateOnLengthMismatchhook onserveFromCache, and does not append origin bytes.Tiered mode: There is no upstream body fallback—a pre-commit short body becomes an authoritative
NoSuchKeymiss (counted as error, no prefix leaked); post-commit short streams stay incomplete with error accounting and best-effort version-guarded cleanup.Broadcast coalescing now signals terminal errors before
SetHeadersthroughWaitForHeaders, so late listeners and failed fetches do not hang or commit a false 200.README, architecture, and tiered-mode docs describe the new behavior; tests cover length mismatch, revalidation/stale paths, tiered concurrency, and pre-header upstream/broadcast failures.
Reviewed by Cursor Bugbot for commit ef96731. Bugbot is set up for automated code reviews on this repo. Configure here.