Skip to content

Do not recreate a whole pool when scaling it up - #164

Merged
leonarduk merged 1 commit into
mainfrom
fix/scale-up-recreates-pool
Sep 29, 2026
Merged

leonarduk merged 1 commit into
mainfrom
fix/scale-up-recreates-pool

Conversation

@leonarduk

Copy link
Copy Markdown
Owner

Problem

Adding runners to a busy Docker pool (startRunners.ps1/.sh, pools.sh scale, autoscale growth) recreated every existing container in the pool, cancelling in-progress jobs.

Cause

cmd_up runs docker compose up -d --build --scale runner=N. buildx attaches a provenance attestation by default, which records the build time, so every build -- even a fully cached one -- yields a new image ID. Compose then treats all running containers as out of date and recreates them.

Fix

Run the build with BUILDX_NO_DEFAULT_ATTESTATIONS=1, making an unchanged build produce the same image ID. (provenance: false in compose.yaml was tried first but Compose 5 drops it from the bake definition.)

Verification

  • Two consecutive builds: different IDs before, identical after.
  • Throwaway compose project (entrypoint sleep): before, scaling 1→2 recreated runner-1; after, scaling 3→4 left runners 1–3 Running and only created runner-4.
  • tests/pools_test.sh passes.

Note

The first up after this lands still recreates each pool once, since running containers are on the old attested images. Do it while idle. Genuine Dockerfile/entrypoint/label/limit changes still recreate the pool, by design.

🤖 Generated with Claude Code

buildx's default provenance attestation records when the build ran, so
every `up --build` -- even one served entirely from cache -- produced a
new image ID. Compose then saw every running container as out of date and
recreated them all, cancelling their jobs: a scale-up, an autoscale grow
or a rerun of startRunners.sh restarted the pool instead of adding to it.

Set BUILDX_NO_DEFAULT_ATTESTATIONS=1 for the build so an unchanged build
is the same image. compose.yaml's `provenance: false` does not work here:
compose 5 drops it from the bake definition it hands buildx.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 28, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 9595fc02-063f-463b-b959-88a8364fb756


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

DeepSeek AI Code Review

2. Bugs and logic errors

The fix is correct in intent, but the comment is misleading about why it works and could cause a future maintainer to remove it.

The comment claims compose.yaml's provenance: false "can't do this: compose 5 drops it from the bake definition it hands buildx." That's a strong, specific claim about a specific compose version. If that's accurate, fine — but it's the kind of claim that rots. More importantly, BUILDX_NO_DEFAULT_ATTESTATIONS=1 is a global env var that disables provenance attestations for all buildx invocations in this shell, not just the one on the next line. Since it's set as a per-command prefix (VAR=1 cmd), it only applies to that one docker compose up invocation, so scope is fine. Good.

One real concern: BUILDX_NO_DEFAULT_ATTESTATIONS is deprecated in newer buildx in favor of --provenance=false on the build command. If the project pins a buildx version, this is fine; if not, this env var may silently stop working on a future buildx upgrade, reintroducing the exact bug this PR fixes — and the failure mode is silent (image IDs change, pool gets recreated, jobs cancelled). Worth a comment noting the buildx version assumption, or a follow-up to migrate to --provenance=false via build.args/buildx bake config.

4. Test coverage

No test or validation step is included. The bug being fixed is exactly the kind that's hard to catch in CI (it manifests as "pool got recreated and jobs were cancelled"), but it's also exactly the kind that a cheap regression test could catch: run cmd_up twice with the same inputs and assert the container IDs are unchanged. Without that, this fix can silently regress on a buildx upgrade (see above) and nobody will notice until jobs start getting cancelled again.

5. Suggested follow-up issues

  • Add a regression test asserting cmd_up is idempotent (container IDs stable across repeated invocations with unchanged inputs)
  • Migrate from deprecated BUILDX_NO_DEFAULT_ATTESTATIONS env var to --provenance=false build flag
  • Document the buildx version assumption behind the attestation workaround in pools.sh

APPROVE — the fix is correct and scoped; the deprecation risk and missing regression test are real but non-blocking.


Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

@github-actions

Copy link
Copy Markdown

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

  1. Go to Settings > Secrets and variables > Actions in this repo.
  2. Click New repository secret.
  3. Name: OPENAI_API_KEY
  4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED


Reviewed by GPT via gpt-pr-review.yml. Advisory only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant