Confirm a restore with the server once its blob is verified - #4323
Conversation
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The synchronous confirmation needs a total latency bound before this is safe on the restore path. I also left one non-blocking telemetry note inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 24976, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only when repository policy or a verified grant allows it; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical pipeline paths). Human review always required.
e90f1c8 to
87bd2d3
Compare
There was a problem hiding this comment.
The previous concerns remain open in the current revision; I found no new issues in this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 25365, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Addresses buildsworth review on #4323: the confirm call's retry loop had no overall deadline, so a network partition could stall every successful exact-match restore for up to 5 client timeouts. Also move TotalDuration's assignment after confirmRestoreSucceeded so it reflects the true end-to-end restore time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@buildsworth-bk review |
Previous concerns have been addressed.
There was a problem hiding this comment.
Both previous concerns are addressed in the current revision; I found no new issues. This changes the latency-sensitive agent cache restore/API path, so it remains outside this review's L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 25380, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The earlier concerns remain addressed, and I found no new issues in the current revision. This changes the latency-sensitive agent cache restore/API path, so it remains outside this review's L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 26026, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
Implementation looks good! Could we add two regression tests for the best-effort confirmation behaviour?
|
Covers a successful restore surviving a failed confirmation call, and confirmRestoreSucceeded's deadline bounding the whole retry loop rather than resetting per attempt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
The earlier concerns remain addressed, and the new best-effort and deadline regression coverage exercises the requested paths. This remains outside the L1 approval ceiling because it changes the latency-sensitive agent cache restore/API path.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 26116, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
@blaknite , I've added the tests - thanks for the review Grant! |
There was a problem hiding this comment.
The earlier concerns remain addressed, and I found no new issues in the current revision. This still changes the latency-sensitive agent cache restore/API path, so it remains outside this review’s L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 26163, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| // without waiting out the real timeout. | ||
| var confirmRestoreTimeout = 5 * time.Second | ||
|
|
||
| // confirmRestoreSucceeded tells the server this restore's blob was verified, |
There was a problem hiding this comment.
Can you please shorten some of these code comments? This one for example could just be:
// confirmRestoreSucceeded tells the server this restore's blob was verified,
/ so the registry can safely refresh the entry's retention.
// Skipped for a fallback matched.
3 participants
Description
retrieve's response used to trigger a registry TTL bump server-side, before the agent had downloaded or verified anything -- the server has no visibility into whether the backing Namespace/S3 blob actually exists, so that bump was purely optimistic. If the blob turned out to be missing or corrupt, the entry's retention had already been extended for something about to be invalidated moments later.Adds a
CacheEntryConfirmcall, mirroringCacheEntryExpire's shape (client-echoed scopes, best-effort roko retry), fired fromRestore()only once the blob has been downloaded, digest-verified, and extracted -- the strictest "this restore is good" point available, not just the byte-for-byte digest check. Skipped for a fallback match, mirroring the backend's own (now-removed) bump guard on the same distinction.Alternatives considered: none -- this is the client-side half of a companion server-side change (linked below), matching a design already settled there.
Context
Linear: https://linear.app/buildkite/issue/A-1775/refresh-cache-registry-expiry-only-after-backing-blob-is-verified
Server change: https://linear.app/buildkite/issue/A-1775/refresh-cache-registry-expiry-only-after-backing-blob-is-verified
Changes
api/cache.go-- newCacheEntryConfirmReq/CacheEntryConfirmResp+CacheEntryConfirmclient method, identical shape toCacheEntryExpire.internal/cache/client.go-- newCacheEntryConfirmmethod on thecacheAPIinterface.internal/cache/restore.go-- newconfirmRestoreSucceeded, mirroringinvalidateStaleEntryalmost line-for-line (same retry/swallow pattern); called fromRestore()after extraction succeeds, alongside a newcache.confirmedspan attribute (matching the existingcache.invalidatedconvention).confirmRestoreSucceeded(scopes echoed through, skipped on fallback, missing-address guard), a wire-level round trip inapi/cache_test.go, and a full public-Restore()-path integration test proving confirm fires exactly once on a successful exact-match restore and not at all on a restore that ends in invalidation.Testing
go test ./...)go tool gofumpt -extra -w .)Disclosures / Credits
Claude Code implemented this change end-to-end (TDD: failing test first, then the fix), under my direction and review -- the client-side half of a fix designed together with its server-side companion.