Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f0fd034. Configure here.
| default: "168h" | ||
| cache-keep-storage: | ||
| description: "Amount of build cache to retain after pruning, in MB (e.g., 1000 for 1GB). If not set, no minimum is enforced." | ||
| required: false |
There was a problem hiding this comment.
New cache config inputs are ignored during pruning
High Severity
The new cache-max-age and cache-keep-storage inputs are declared in action.yml, parsed in getInputs(), and stored in state — but maybeShutdownBuildkitd() hardcodes cacheMaxAge = null and keepStorageMB = 1000 instead of reading from stateHelper.inputs. Users who set these inputs will see no effect: cache-max-age (which defaults to "168h") is always overridden to null (no age pruning), and cache-keep-storage is always 1000 MB regardless of configuration.
Reviewed by Cursor Bugbot for commit f0fd034. Configure here.
bruce-y
changed the title
bruce-y
changed the title
| } | ||
|
|
||
| const cacheKeepStorageInput = core.getInput("cache-keep-storage"); | ||
| let cacheKeepStorage = 20480; |
There was a problem hiding this comment.
@bruce-y should we entirely skip running prune if they don't specify this? Do you anticipate any regression if we do that? Our storage agent logic already evict when we near 500GB so we don't really need to default to anything imo
|
|
||
| /** | ||
| * Prunes buildkit cache data. | ||
| * @param keepStorageMB Storage to retain in cache, in MB. Defaults to 20480 (20GB). |
There was a problem hiding this comment.
20GB is too small for dockr layer cache, this usually runs to atleast 200-300GB. imo we should default to not pruning if not specified and should maybe have docs around recommended sizes here based on our average stickydisk size.
| // Disable automatic garbage collection, since we will prune manually. Automatic GC | ||
| // has been seen to negatively affect startup times of the daemon. | ||
| gc: false, | ||
| gc: true, |
There was a problem hiding this comment.
The PR's fix of setting gc: true is correct for getting access-time tracking back. One subtlety worth noting: with gc: true and no explicit gcPolicy entries in the toml, BuildKit will use its default GC policies which can be fairly aggressive (e.g., keeping cache under a few GB, evicting unused entries after 48h). If the intent is "track access times but only prune manually at shutdown," you'd want to also define very permissive gcPolicy entries in the toml to prevent the background GC from fighting with your manual prune logic. That said, if the PR is moving toward capping cache at ~20GB via --keep-storage, the default policies probably won't conflict since they're more lenient than 20GB.
from a devin session I was chatting with
There was a problem hiding this comment.
also @bruce-y can you check we have a buildkit startup time metric? As the comment suggests it would not be good to regress that, and if we see signs of that we might have to dig in further
2 participants
GC flag being disabled causes buildkit to not track last access times. It looks like the max-age option for buildctl overrides any additional options, especially when all of the last accessed values are unset. Disabling this option and only setting max cache size sidesteps this issue.
Codesmith can help with this PR — just tag
@codesmithor enable autofix.