Repository navigation
fix: avoid stale refills after lost DeleteObjects replies - #241
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}
| // or legacy reader generation for every distinct requested key using a bounded | ||
| // context independent of the client. Keep tiered's success-only converge: its | ||
| // local entry may be the only copy. | ||
| if err != nil && attempted && requestedCounts != nil && tieredPriors == nil { |
There was a problem hiding this comment.
requestedCounts nil. That skips both the existing invalidations and this lost-reply cleanup, so the origin can delete an object while TAG continues serving its cached copy.
There was a problem hiding this comment.
Valid. HandleDeleteObjects counts keys by unmarshalling the raw request body before the signing forwarder decodes aws-chunked framing, so a valid framed request leaves requestedCounts nil and skips both invalidations. I will make the key parser consume the decoded payload while preserving the body/signature semantics used for forwarding.
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| for key := range requestedCounts { | ||
| s.convergeInvalidation(ctx, bucket, key) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Valid. All keys currently share one ten-second context, so a slow early invalidation can leave later keys with an expired context. I will give distinct-key cleanup independent bounded attempts under bounded concurrency, while retaining the original upstream error and recording each invalidation failure.
| current, found, err := c.generationToken(ctx, bucket, key) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if !found || current != expected.version { |
There was a problem hiding this comment.
meta-gen, so that late Put carries a generation that still matches. An updated TAG reader then accepts and serves the old object despite the delete. The separate-key CAS operations in ocache's client do not make the tombstone check and metadata Put atomic.
There was a problem hiding this comment.
Valid. A v1.20 writer does not participate in the meta-gen protocol, so a current node cannot fence its plain metadata Put after that writer's tombstone check. This needs an explicit cluster-version/rollout boundary; another sidecar check cannot make an old writer honor the fence. I will keep the guarantee scoped to peers using the new protocol and verify the rollout guidance rather than claim mixed-version safety.
| headCtx, cancel := context.WithTimeout(ctx, 5*time.Second) | ||
| defer cancel() | ||
| resp, err := s.forwarder.DoConditionalHeadRequest(headCtx, bucket, key, accessKey, secretKey, "", 0) |
There was a problem hiding this comment.
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.
Valid. A cacheable 200 revalidation waits for the unconditional HEAD, with a five-second timeout, before it writes response headers. I will remove that synchronous confirmation and instead carry a decision token captured before the conditional GET, so an invalidation during the GET can reject a late cache publish without delaying the response for another origin request.
| generation, found, err := c.generationToken(ctx, bucket, key) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| if !found || meta.cacheGeneration != generation { |
There was a problem hiding this comment.
meta-gen. The two keys can route to different owners through ocache's key-based client, adding an RPC to every hit; failure of that second read also makes an otherwise available entry unusable. This is a non-blocking performance concern worth measuring on distributed cache hits.
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.
Valid. In legacy mode, getMeta reads the plain metadata and then meta-gen; a sidecar-read error turns that entry into a miss. This is a real added lookup and failure point on the default cache-hit path, and the current correctness proof does not measure its distributed latency. I will preserve the fence while addressing this cost rather than remove the check without a safe replacement.
1 participant
A DeleteObjects transport error does not tell TAG whether the origin applied the batch. TAG now performs a second invalidation for each distinct requested key before returning the original error.
For cache-enabled, parseable requests in transparent and signing proxy modes, the second pass runs only after
http.Client.Dowas attempted and returned an error. It uses a single ten-second deadline independent of client cancellation. A pre-Do failure gets no second pass. A received 2xx still uses per-key<Error>counts, so a refill is preserved only when every requested delete for that key failed. Tiered mode keeps its success-only convergence because its local cache can be the only copy.Legacy mode keeps the v1.20 plain metadata and tombstone protocol for wire-compatible rollouts. Current TAG readers also validate a CAS generation sidecar, so they reject late plain metadata writes; if a cache owner cannot perform sidecar CAS, current readers fail closed and bypass that cache entry. Unmodified v1.20 readers do not check the sidecar and can still serve a late plain Put during a mixed-version rollout. The freshness guarantee is limited to readers running this source revision.
A cacheable 200 revalidation now waits for a follow-up HEAD to confirm the same strong ETag and content length before streaming and caching the response. If HEAD fails or the representation changed, TAG still returns the 200 body but does not cache it. The HEAD is bounded at five seconds; its latency impact is not measured. The controlled test proves the lost-reply ordering is reachable, not how often it occurs in deployed traffic.
The cache package's exported
GetMetaWithVersion,PutMetaIfVersion,PutWithMetaStreamIfVersion, andDeleteMetaIfVersionnow exchangeMetaVersionTokeninstead of a rawuint64. This opaque value carries both the legacy tombstone timestamp and sidecar generation.AnyMetaVersionToken()keeps unconditional writes explicit.Testing
The Go regression tests ship. The shell result mapper and assertion text are proof-only support and are not part of this pull request.
Recorded proof ran with retained support that is not included in this pull request.
sh proof/run_native_test.sh proxy TestDeleteObjectsLostResponseRefill: violated at comparison revisionee1acea2154388d89935da54c02155270e55e854; final source satisfied.sh proof/run_native_test.sh proxy TestDeleteObjectsLostResponseRealGET: violated at comparison revisionee1acea2154388d89935da54c02155270e55e854; final source satisfied.sh proof/run_native_test.sh cache TestLegacyFenceRefusesLateCommitAfterMarkerEviction: violated at comparison revisionee1acea2154388d89935da54c02155270e55e854; final source satisfied.sh proof/run_native_test.sh proxy TestDeleteObjectsLostResponseRevalidation: violated at comparison revisionee1acea2154388d89935da54c02155270e55e854; final source satisfied.make lint-ci,make build,make test,make test-integration,make test-race, andmake test-coverage.Generated by Perfloop. Measurements and checks. Reproduce this comparison (tenant access required).
Note
High Risk
Changes core cache coherence (generation sidecar, token API) and delete/revalidation paths where stale reads or double-invalidation bugs would be user-visible.
Overview
Hardens metadata invalidation and populate ordering for mixed-version clusters and ambiguous delete outcomes.
Legacy coordination now keeps v1.20 tombstones/plain metadata for wire compatibility while current TAG tags metadata with a separate
meta-genCAS generation sidecar. Reads require a matchingcache_generation; untagged or stale rows are misses. Decision-time markers are TTL-bounded and fail closed if evicted or if sidecar CAS is unavailable. Public APIs useMetaVersionToken(replacing rawuint64) plusAnyMetaVersionToken()for unconditional writes.DeleteObjects in proxy modes: when capture forwarding attempted
Doand got an error, TAG runs a second invalidation for every distinct requested key (bounded context, independent of client cancel). Pre-dispatch failures skip this pass; tiered mode unchanged. Forwarders exposeforwardWithCaptureAttemptedto distinguish those cases.Revalidation: a cacheable 200 from conditional GET is stored only after a follow-up HEAD confirms the same strong ETag and content length; otherwise the body is still returned but not cached.
Docs and tests cover mixed-version limits, lost-reply races, marker eviction, and concurrent generation advances.
Reviewed by Cursor Bugbot for commit 3c4818c. Bugbot is set up for automated code reviews on this repo. Configure here.