Echo entry_ref on cache expire for scope-aware invalidation - #4281
Echo entry_ref on cache expire for scope-aware invalidation#4281buildkate wants to merge 1 commit into
Conversation
The backend now returns an opaque entry_ref in cache retrieve responses,
identifying exactly the entry that was served. When a restore degrades to
a miss (missing blob, digest mismatch, unreadable archive) the agent now
echoes that ref on expire, so the server deletes only the entry that was
actually retrieved and never a concurrent replacement.
The expire response is now typed ({message, existed}): the agent reports
the entry as invalidated only when the server confirms it existed and was
deleted, and logs a distinct no-op message when it was already gone. The
legacy target_paths/cache_key address is still sent for servers that
don't understand entry_ref yet.
Part of A-1774.
Amp-Thread-ID: https://ampcode.com/threads/T-01a046fc-3919-7509-af75-03f3f25535d8
Co-authored-by: Kate Sy <kate.sy@buildkite.com>
![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: fc25ddb1e5
ℹ️ 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 !expireResp.Existed { | ||
| slog.Info("stale cache entry was already gone, nothing invalidated", "registry", c.registry) | ||
| return false |
There was a problem hiding this comment.
When an unreadable archive triggers invalidateStaleEntry and the server reports existed: false (or invalidation fails), this new branch correctly returns false, but the archive-verification path still unconditionally emits Cache miss (archive failed verification, invalidated stale entry). This gives users contradictory and inaccurate progress output specifically when no entry was deleted; make that completion message conditional or remove the invalidation claim as was done for the missing-blob and digest-mismatch paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agree — I independently hit the same mismatch in the archive-verification path.
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The cache API/protocol and restore-path behavior change is above this repository's L1 approval ceiling, so I'm leaving this as comment-only. The existing Codex thread covers the one progress-message inconsistency I found.
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 22466, 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.
2 participants
Description
When a cache restore degrades to a miss (missing blob, digest mismatch, unreadable archive), the agent expires the stale entry so a later save can re-upload it. Today that expire is addressed by
target_paths+cache_key, which can delete a different entry than the one that was actually retrieved — e.g. a scoped sibling or a concurrent replacement.The backend now mints an opaque
entry_refin retrieve responses identifying exactly the entry that was served. This PR echoes that ref on expire so the server deletes only that entry, and decodes the now-typed expire response ({message, existed}): the restore reports the entry as invalidated only when the server confirms it existed and was deleted, and logs a distinct no-op message when it was already gone. The legacy address fields are still sent for servers that don't understandentry_refyet, and ref values are never logged.Context
Changes
api:CacheEntryRetrieveRespgainsEntryRef;CacheEntryExpireReqgainsEntryRef(omitempty);CacheEntryExpirereturns a typedCacheEntryExpireResp{Message, Existed}.internal/cache:invalidateStaleEntryechoes the ref, keeps the legacy fields, and returns true only onexisted: true; retry behavior unchanged. Progress messages no longer assert the entry was deleted.existed: false) path. Fallback-entry invalidation isn't covered by the mock (it has no fallback matching — pre-existing TODO); that scenario is covered end-to-end by the backend's request specs.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)go test -race ./internal/cacheandgolangci-lint run ./api/... ./internal/cache/...also pass.Disclosures / Credits
This PR was implemented by Amp (Claude), working from a plan reviewed in an Amp thread.