Honor the server's configured cache retention for backing blobs - #4339
Honor the server's configured cache retention for backing blobs#4339ss1909 wants to merge 4 commits into
Conversation
The agent hard-coded the Namespace artifact lifetime to 72h on upload (--expires_in) and on refresh (--ensure_minimum), so a cache registry whose TTL is raised above 72h (via the new cache-retention service quota) would keep live metadata past the point its backing blob had already expired — turning later exact hits into hollow misses. Thread the retention the server now returns (retention_seconds on the cache store and retrieve responses) into the blob store: Upload and RefreshRetention take a duration, and NscStore formats it for --expires_in / --ensure_minimum via nscExpiry, falling back to 72h when the server sends nothing (older server). S3 and local stores ignore it (S3 lifetime is governed by the bucket lifecycle policy). Stacks on the A-1775 confirm-restore work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The constant holds a retention duration (a fallback used only when the server omits one), and the helper formats a retention for the nsc CLI — 'expiry' misnamed both. Keep the nsc prefix: these are NSC-specific (the 72h default and hours formatting are nsc artifact conventions; S3/local ignore retention). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ss1909
added
the
change
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
No concrete issues found. This changes cache save/restore behavior and the backend-agent response contract, so it still needs a human sanity-check.
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 26576, 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.
There was a problem hiding this comment.
No concrete issues found. This changes the cache save/restore path and the backend-agent response contract, so it still needs a human sanity-check.
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 26600, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Match the server switching retention_seconds -> retention_days: the field is now RetentionDays and the store conversions use time.Duration(days) * 24 * time.Hour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fb9534c to
b43b2c7
Compare
There was a problem hiding this comment.
The days-based contract now matches the companion backend response, and I found no concrete issues. This still changes cache save/restore behavior and the backend-agent response contract, so it needs a human sanity-check.
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 26630, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
1 participant
Summary
Makes the agent honor the cache registry's configured retention when it stamps a backing blob's lifetime, instead of a hard-coded
72h.Companion to the backend work in buildkite/buildkite#33814 (A-1831, cache retention configurable via a service quota). The backend now returns
retention_secondson the cache store (upload) and retrieve responses; this PR consumes it.Why
NscStoreset the Namespace artifact lifetime tonscDefaultExpiry = "72h"on both upload (--expires_in) and refresh (--ensure_minimum). If an org's registry TTL is raised above 72h, the registry metadata outlives its backing blob: a later exact hit returns from the registry, the blob download 404s, and it collapses to a (self-invalidating) miss. So raising the quota bought hosted customers nothing. This threads the real retention to the blob so the two clocks stay aligned.Changes
api/cache.go: addRetentionSecondstoCacheEntryCreateRespandCacheEntryRetrieveResp.store.Blob.Uploadandstore.RetentionRefresher.RefreshRetentiontake aretention time.Duration.NscStoreuses it for--expires_in(upload) and--ensure_minimum(refresh) via a newnscExpiryhelper that rounds up to whole hours and falls back to72hwhen the server sends nothing (older server) — backward compatible in both directions.S3Blob/LocalFileBlobaccept the duration but ignore it (S3 lifetime is governed by the bucket lifecycle policy; local has no TTL).save.gopasses the store response's retention on upload;restore.gopasses the retrieve response's retention on the exact-hit refresh (fallback hits still skip the refresh).Verification
go build ./...,go vet ./internal/cache/...,go test ./internal/cache/...all pass.TestNscExpiry(fallback, whole-hours, multi-day, round-up);nsc_testasserts the formatted--expires_in/--ensure_minimum;restore_testasserts the retention propagates throughmaybeRefreshRetention.🤖 Generated with Claude Code