Optional cache prefix scope (BK_CACHE_LANE) - #2
Conversation
…art sets it Fleet buildkitd is rootless (a uid-999 file is stored under a shifted host uid); metal buildkitd runs as root in the kata guest. Sharing s3://<bucket>/<ns>/ between them hands a build a snapshot with the wrong on-disk ownership and the next non-root stage EACCESes (Wunderflats api, prod 2026-09-03). The chart (>=0.32.12) sets BK_CACHE_LANE=metal on the metal path only; this post step then commits to s3://<bucket>/<ns>/metal/ and the hydrate reads the same prefix. Without the env the key is unchanged. The refresh listing now strips the listed prefix by string, so lane-scoped keys still match the --include patterns (the two-segment strip would have refreshed every object on every build). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018d4NPiB2qJmuJJRggQDhUf
![buildpulse-v2[bot]](https://avatars.githubusercontent.com/u/46661156?s=60&v=4){kind=small}
There was a problem hiding this comment.
Agent code review
Walkthrough
This PR adds lane-scoped S3 cache prefixes so that metal runners (root-in-guest buildkitd, no uid shift) and fleet runners (rootless buildkitd, uid-shifted) store their caches under separate S3 keys (<ns>/<BK_CACHE_LANE>/ vs <ns>/). The change introduces a cacheBase() helper that computes both the S3 base URI and a keyPrefix, threads lane through commitToS3, and fixes a latent correctness bug in parseS3ListAges where the old two-segment strip (<ns>/<prefix>/) produced wrong keys for lane-scoped listings — causing every object to appear missing and refresh every build. Two targeted tests cover both the lane-scoped and fleet-prefix cases.
Findings
No issues found.
Verdict
The diff is correct and coherent. The keyPrefix parameter to parseS3ListAges fixes a real bug (wrong key derivation for lane-scoped listings that would have silently defeated the cost optimization, refreshing every object on every build). Backward compatibility is preserved — callers without keyPrefix continue to use the legacy two-segment strip. The new cacheBase() helper centralises the prefix construction logic, the input validation (SAFE.test(lane)) covers injection, and the dist/ bundles are consistent with the source changes. Tests are added, not weakened.
Confidence
HIGH — all 6 changed files were read in full; key callers of every modified function were traced end-to-end.
💡 Tips for working with the BuildPulse reviewer
- Re-review on demand — comment
@buildpulse reviewon the PR (/buildpulse reviewalso works). The fresh review bills the requester's seat. - Teach it by replying to a finding — reply to any inline comment: "we never do X here" becomes a standing rule for this repository (acknowledged in-thread, not raised again); "not in this PR" mutes the topic for this PR only. Replies are free. Org admins can view or remove learned rules on the AI Features page.
- Every push is re-reviewed automatically — smaller PRs get a fresh full review, larger ones get a delta review of what changed since the last look; an "Update branch" merge with an unchanged diff carries the previous verdict forward without a new billed review.
- Draft PRs are skipped until marked ready for review (repositories can opt in to draft reviews on the AI Features page).
mhodzic93
changed the title
1 participant
Superseded by a follow-up PR; closing.