From 753e3b01aeb043ff8f923adc6817a75a87ed589c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 19:08:39 -0700 Subject: [PATCH] fix(ci): actually prune the BuildKit layer cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The max-cache-size-mb input added in 8fa7f0cc4e never did anything. setup-docker-builder v1 accepted it and pruned in its own post step, but the v2 rewrite dropped the input, and the repo pins v2.1.0. GitHub only WARNS on an unknown composite input, so every build since has logged Unexpected input(s) 'max-cache-size-mb', valid inputs are ['cache-key', ...] and pruned nothing. Scanned every tag to confirm: the input exists in v1.8.0 through v1.12.0 and in none of v2.0.0, v2.0.1, v2.1.0. Rather than downgrade a builder rewrite to reach a config knob, run the prune ourselves — v1's command verbatim, against the fixed address v2 itself uses for `buildctl du` and `debug workers`: sudo buildctl --addr tcp://127.0.0.1:1234 prune --all --keep-storage Both flags read from BuildKit master rather than assumed. buildctl's --all is not `docker buildx prune --all`: it means "include internal/frontend references", and cache/manager.go shows the only records skipped without it are those typed internal or frontend plus refs shared with an external source. It does not wipe the cache. --keep-storage maps onto the modern MaxUsedSpace field, so it is buildctl's spelling of --max-used-space, not a deprecated alias, and it is the only size flag buildctl exposes. Two guards, both for failure modes that are silent and expensive: Reject a non-positive-integer budget. buildctl parses --keep-storage as a float, and BuildKit treats keepBytes==0 as "no cap" (`gcMode := opt.keepBytes != 0`), pruning everything eligible instead of trimming. A typo like '40GB' — valid in turbo.json, but this flag is a bare MB number — would empty the cache and make every later build cold, costing far more than the storage saved. Wait for `du` to settle after pruning. buildctl prune returns before buildkitd has finished deleting (moby/buildkit#1198), and the builder's post step SIGTERMs buildkitd then SIGKILLs it after 30s (shutdownBuildkitd: `const a=3e4`); on SIGKILL it sets sigkillUsed and skips the sticky disk commit outright, discarding the run's cache and risking a corrupt bbolt metadata DB. The wait is bounded, and a steady-state trim settles almost immediately — it is the first catch-up prune against a 200 GB backlog that would otherwise run into that window. Warn rather than fail throughout, since an oversized cache is not worth failing a deploy over — but print `buildctl du` either side, because a silent no-op is exactly the failure mode that hid this regression for a day. --- .github/actions/docker-build/action.yml | 97 +++++++++++++++++++++---- 1 file changed, 82 insertions(+), 15 deletions(-) diff --git a/.github/actions/docker-build/action.yml b/.github/actions/docker-build/action.yml index b8727501f52..382562760e6 100644 --- a/.github/actions/docker-build/action.yml +++ b/.github/actions/docker-build/action.yml @@ -21,10 +21,13 @@ inputs: required: true max-cache-size-mb: description: >- - Layer cache to retain after the post-job prune, in MB. Must stay above one + Layer cache to retain after this action prunes, in MB. Must stay above one build's working set (base + dependency layers + RUN --mount=type=cache dirs) or every build evicts what the next one needs. Falls back to the - small-image default below when empty. + small-image default in the prune step when empty — the fallback lives there + rather than here because callers pass this from a matrix field, and an unset + matrix key arrives as the empty string, which counts as "provided" and would + bypass an input `default:` entirely. required: false # Registry logins must precede this action. provenance/sbom stay off: attestation @@ -49,24 +52,16 @@ runs: PLATFORMS: ${{ inputs.platforms }} run: echo "value=${GITHUB_REPOSITORY##*/}/${FILE#./}/${PLATFORMS//\//-}" >> "$GITHUB_OUTPUT" - # max-cache-size-mb is what bounds the disk: BuildKit's default GC is - # time-based only (layers unused for 8 days), and setup-docker-builder skips - # pruning altogether when the value is empty. On a repo that builds this - # often nothing ever ages out, so the disks grew without limit — - # app.Dockerfile/linux-amd64 reached 351 GB inside a day, and realtime, whose - # image is under 300 MB, sat at 249 GB. Sticky disks bill at ~$0.51/GB-month, - # so that was real money for layers no build would ever read again. - # - # The fallback is here rather than an input `default:` because callers pass - # this from a matrix field, and an unset matrix key arrives as the empty - # string — which counts as "provided", so a `default:` would never apply and - # a row that forgot the field would silently go back to unbounded growth. + # This action does NOT bound the disk — see the prune step below. BuildKit's + # own GC is time-based only (layers unused for 8 days), and these disks are + # mounted many times a day, so nothing ever ages out: app.Dockerfile/linux-amd64 + # reached 351 GB inside a day of being created, and realtime, whose image is + # under 300 MB, sat at 249 GB. Sticky disks bill at ~$0.51/GB-month. - name: Set up Blacksmith builder if: inputs.provider == '' || inputs.provider == 'blacksmith' uses: useblacksmith/setup-docker-builder@a5256a73e30f09e37e3eceb8ca36043d17621d24 # v2 with: cache-key: ${{ steps.cache-key.outputs.value }} - max-cache-size-mb: ${{ inputs.max-cache-size-mb || '25600' }} - name: Build and push (Blacksmith) if: inputs.provider == '' || inputs.provider == 'blacksmith' @@ -80,6 +75,78 @@ runs: provenance: false sbom: false + # Bound the layer cache ourselves. setup-docker-builder v1 took a + # max-cache-size-mb input and pruned in its own post step, but the v2 rewrite + # dropped it — and GitHub only WARNS on an unknown composite input, so passing + # it to v2 silently did nothing for a day while the app disk sat at 200+ GB. + # + # This is v1's command verbatim (its dist/index.js pruneBuildkitCache), against + # the fixed address v2 itself uses for `buildctl du` and `debug workers`: + # sudo buildctl --addr tcp://127.0.0.1:1234 prune --all --keep-storage + # + # Note buildctl's --all is NOT `docker buildx prune --all`. Here it means + # "include internal/frontend references" (cache/manager.go: without it, records + # typed internal or frontend, and any ref shared with an external source, are + # skipped). It does not wipe the cache, and --keep-storage still caps what is + # retained -- it maps straight onto the modern MaxUsedSpace field, so it is the + # buildctl spelling of --max-used-space rather than a deprecated alias. + # `RUN --mount=type=cache` dirs are typed exec.cachemount and are reclaimed + # either way; --all is here because it is what v1 used and it prunes strictly + # more. Runs before the builder's post step, which is what commits the disk. + # + # Warn rather than fail: a cache that is too large is not worth failing a + # deploy over. The du either side is what makes a silent no-op visible — the + # failure mode that hid the v2 input regression in the first place. + - name: Prune the layer cache + if: (inputs.provider == '' || inputs.provider == 'blacksmith') && !cancelled() + shell: bash + env: + KEEP_MB: ${{ inputs.max-cache-size-mb || '25600' }} + run: | + addr='tcp://127.0.0.1:1234' + + # A zero or non-numeric value is NOT a no-op. buildctl parses + # --keep-storage as a float, and BuildKit's cache manager treats + # keepBytes==0 as "no cap" (`gcMode := opt.keepBytes != 0`), pruning + # everything eligible rather than trimming to a limit. A typo such as + # '40GB' — valid in turbo.json, but this flag is a bare MB number — would + # silently empty the cache and make every later build cold, costing far + # more than the storage it saves. Refuse instead. + if ! [[ "$KEEP_MB" =~ ^[1-9][0-9]*$ ]]; then + echo "::warning::max-cache-size-mb must be a positive whole number of MB, got '${KEEP_MB}' — skipping prune rather than risk wiping the cache" + exit 0 + fi + + # Print the whole Total line rather than picking a column: buildctl's du + # table is whitespace-aligned and its layout is not a stable contract. + total() { sudo buildctl --addr "$addr" du 2>/dev/null | grep -iE '^total:' | tr -s ' \t' ' '; } + echo "before prune -> $(total)" + + if sudo buildctl --addr "$addr" prune --all --keep-storage "$KEEP_MB"; then + # buildctl prune returns BEFORE buildkitd has finished deleting + # (moby/buildkit#1198). The builder's post step then SIGTERMs buildkitd + # and SIGKILLs it after 30s (shutdownBuildkitd: `const a=3e4`); on + # SIGKILL it sets sigkillUsed and SKIPS the sticky disk commit, throwing + # away this run's cache and risking a corrupt bbolt metadata DB. So wait + # for du to stop moving before handing back. Bounded — this is hygiene, + # not correctness, and the steady-state trim settles almost at once. + prev=''; stable=0 + for _ in $(seq 1 60); do + cur="$(total)" + if [ "$cur" = "$prev" ]; then + stable=$((stable + 1)) + [ "$stable" -ge 2 ] && break + else + stable=0 + fi + prev="$cur" + sleep 2 + done + echo "after prune -> $(total) (keep-storage ${KEEP_MB} MB)" + else + echo "::warning::Layer cache prune failed; this sticky disk is unbounded for this run" + fi + - name: Set up Docker Buildx if: inputs.provider != '' && inputs.provider != 'blacksmith' uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4