Add pools.sh autoscale: resize pools to their queued jobs

[leonarduk]

Closes #110

Summary

• New ./pools.sh autoscale [--once] [--dry-run] [--interval <s>], driven by a gitignored autoscale.conf (name min max [idle_minutes]), so pools.conf is never rewritten each pass.
• Demand = queued jobs (from queued and in-progress runs) whose labels a pool's runners carry, including the host label; a job several pools could take goes to the least-specialised one (issue-worm vs issue-worm-pro).
• Grow to busy + queued, capped at max, raised to min; starting containers count as capacity. Shrink to min only after idle_minutes with nothing busy or queued, re-checked with require_idle right before. GitHub unreachable -> pool left alone.
• README section + autoscale.conf.example.

Follow-ups: #111 (memory/CPU sizing + host budget), #112 (Windows fleet), leonarduk/issue-worm-pro#1397 (dashboard).

Test plan

• tests/pools_test.sh — all checks pass, including 20 new autoscale cases (grow/cap/min/idle timeout/busy refusal/label matching/GitHub down/dry-run)
• autoscale --once --dry-run on StevesBigLaptop against live GitHub: busy counts matched running jobs, idle pools reported countdowns, no state written
• Run the loop on a real host with a small autoscale.conf and watch one grow/shrink cycle

🤖 Generated with Claude Code

[coderabbitai]

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: b6ebca2e-95de-4dbb-9d3b-67bdca807de2

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

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.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Most AC items are covered, but there are gaps:

• Grow, cap at max, raise to min, idle-timeout shrink, busy refusal, label matching, GitHub-unreachable skip — all exercised in tests/pools_test.sh. Good.
• shellcheck clean — not verifiable from the diff; the PR description claims it, but nothing in the diff enforces it (no CI hook shown). Worth confirming.
• README documents autoscale.conf and how to keep the loop running — done, but the README references autoscale.conf.example which is not in the diff. cmd_autoscale also tells the user to "copy autoscale.conf.example", so if that file doesn't exist the error message is misleading and the documented workflow is broken. This is a real gap against the AC.
• autoscale.conf gitignored — the issue says it should be gitignored; the diff doesn't show a .gitignore change. If it's already covered by a wildcard, fine, but nothing in the diff confirms it.

2. Bugs and logic errors

• queued_jobs pagination is broken for the runs list. gh api "repos/$repo/actions/runs?status=$status&per_page=100" fetches only the first page of runs. With --paginate absent, a repo with >100 queued or in-progress runs will under-count demand. The jobs call uses --paginate, but the runs call does not. This is a real correctness bug on busy repos — exactly the case autoscale exists for.

• queued_jobs swallows the gh failure of the inner jobs call but not the outer. The outer gh api ... --jq is guarded by || return 1, good. The inner gh api ... --paginate --jq ... 2>/dev/null || return 1 is also guarded. But 2>/dev/null on both hides auth/scope errors, so a token missing actions:read looks identical to "no jobs" only if the exit code is 0 — it won't be, so this is OK. Still, the 2>/dev/null on the outer call means the operator sees "could not ask GitHub" with no reason. Minor.

• repo_runners failure is treated the same as queued_jobs failure, which is correct, but the combined if ! jobs=... || ! runners=... short-circuits: if queued_jobs fails, repo_runners is never called, and the state-preservation loop runs. That's fine, but note the state line is re-emitted for every autoscaled pool in that repo — including pools that were not idle. state_line returns nothing for those, so state+="$(state_line "$name")"$'\n' appends an empty line. The final grep -v '^$' strips them, so no corruption. OK.

• Idle-clock reset on grow is missing. When a pool grows (target > current), the code does not clear its .autoscale-state entry. Consider: pool is idle at 2 containers, state says "idle since T". A job arrives, pool grows to 4. Next pass, job is gone, pool is at 4 with 0 busy/0 queued. since is read from state as T (old), so now - since >= idle*60 is immediately true and the pool shrinks to min on the very next pass — bypassing the idle timeout entirely. The state should be cleared whenever busy > 0 || queued > 0, not only when the shrink branch runs. The test "a busy runner resets the idle clock" passes only because the busy case happens to fall through the target == current && queued == 0 && busy == 0 guard, so state is not appended — but the file still contains the old entry from the previous pass, and the test asserts ! grep -q worm after a single pass where the file was rewritten without worm. That works because the whole file is rewritten each pass from state. So actually the file is cleared. Re-reading: state is rebuilt fresh each tick, and only appended to in the idle branch or the GitHub-unreachable branch. So a busy pass produces no worm line and the file loses it. This is correct. Withdrawing the concern.

• since is used uninitialized in the shrink-failure path. In the block:

  if (( target < current )) && ! ( require_idle "$name" ) < /dev/null; then
    echo "$name: not shrinking after all" >&2
    state+="$name $since"$'\n'
  

  since is only assigned inside the idle branch above. If target < current for a reason other than idle (e.g. current > min and need < current and busy == 0 && queued == 0 — which is the idle branch, so since is set), it's fine. But if target < current because current > min and the idle branch didn't run (e.g. queued > 0 but need < current), then since is empty and state+="$name " writes a malformed line. Can target < current with queued > 0? need = busy + queued; if queued > 0 then need >= 1; target starts at current, is raised to min if below, then to need if need > target. So target >= current whenever queued > 0. So target < current implies queued == 0 && busy == 0 && current > min, which is exactly the idle branch, which sets since. Not a bug, but fragile — a future edit could break it. Non-blocking.

• labels_cover uses IFS=, on $2 but $1 is compared with ",$1,". If a label in want contains a comma (impossible for GitHub labels) or if have has spaces, this is fine. OK.

• host_label reads .env with sed -n 's/^RUNNER_HOST_LABEL=//p' | tail -1. If the value is quoted (RUNNER_HOST_LABEL="box"), the quotes are kept and the label won't match. docker compose strips quotes. This is a real mismatch: a user with a quoted .env value gets a pool that never matches its own jobs. Worth fixing or documenting.

• conf_line "$name" is called twice per pool per pass (once to build decl, once inside the per-pool loop). Minor inefficiency, not a bug.

• grep -cxF "$name" <<< "$demand" counts lines exactly equal to $name. demand is built as "$best"$'\n' per job, so this is correct. But grep -c returns exit 1 when count is 0, hence the || true. OK.

• repo_runners and pool_cids are called with < /dev/null — good, since the outer loop reads from a here-string. But cmd_up is also called with < /dev/null, and cmd_up may itself read stdin (e.g. for prompts). If cmd_up ever prompts, this silently feeds it EOF. Currently cmd_up doesn't prompt, so OK.

3. CLI and API safety

• No new dependencies — good, pure shell.
• CLI backward compatibility — autoscale is a new subcommand; no existing signature changed. Good.
• GitHub API auth — relies on gh being authenticated, same as list --json. The README notes the token scope requirement. Good.
• Rate limits — the README claims "a few API calls per repo per minute, well inside the 5,000 an hour". But queued_jobs makes 1 + N calls per repo per pass, where N is the number of queued/in-progress runs. On a busy repo with 50 in-progress runs, that's 51 calls per pass, ×2 statuses = ~102 calls per minute per repo. With several autoscaled repos, this can approach the limit. The README's claim is optimistic. Not blocking, but the runs-list pagination bug above compounds it.
• Injection — $name, $repo, $label are interpolated into cmd_up arguments. name comes from autoscale.conf (user-controlled, gitignored) and pools.conf. repo comes from pools.conf. These are passed as separate argv elements to cmd_up, which presumably passes them to docker compose as argv, not through a shell. No eval visible. OK.
• --interval validation — ^[1-9][0-9]*$ rejects 0 and negatives. Good. But it accepts arbitrarily large values; sleep handles that. OK.

4. Test coverage

• The stub gh in tests/pools_test.sh matches *"runs?status=queued"* and *"/jobs"*. The real code calls repos/$repo/actions/runs?status=queued&per_page=100 — the stub matches. Good.
• No test for the runs-list pagination bug (because the stub returns a single run id). A test with >100 runs would catch it, but the stub can't easily simulate that. Worth a follow-up.
• No test for quoted RUNNER_HOST_LABEL in .env. The test sets RUNNER_HOST_LABEL=box via env, bypassing the .env parsing path entirely. The host_label .env branch is untested.
• No test for autoscale.conf with a comment line and blank lines — the parser handles them ([[ -z "$name" || "$name" == \#* ]] && continue), but no test exercises it. Minor.
• No test for the --interval loop (only --once). The loop is simple enough that this is acceptable, but a test that the loop survives a failing pass would be nice.
• No test that autoscale.conf.example exists — because it doesn't.

5. Suggested follow-up issues

• Add autoscale.conf.example (referenced by README and by cmd_autoscale's error message).
• Confirm autoscale.conf and .autoscale-state are in .gitignore.
• Paginate the actions/runs?status=... call in queued_jobs (currently only the first 100 runs are seen).
• Strip surrounding quotes from RUNNER_HOST_LABEL when reading .env in host_label.
• Add a CI step running shellcheck on pools.sh and tests/pools_test.sh.
• Add a test exercising the .env path of host_label (quoted and unquoted values).
• Revisit the README's rate-limit claim once runs-list pagination is fixed.

REQUEST CHANGES — autoscale.conf.example is referenced by both the README and the runtime error message but is not in the diff; the actions/runs call in queued_jobs is not paginated, so demand is under-counted on repos with >100 queued/in-progress runs; and host_label's .env parsing keeps surrounding quotes, so a quoted RUNNER_HOST_LABEL silently breaks label matching.

---

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

[leonarduk]

Addressed the DeepSeek review in the latest commit:

• Runs list not paginated: fixed, queued_jobs now passes --paginate for actions/runs.
• Quoted RUNNER_HOST_LABEL in .env: fixed, one pair of surrounding "/' is stripped like compose does. New tests cover the .env path: unquoted, quoted, and absent.
• README rate-limit claim: reworded. The cost is one call per in-flight run, so busy repos should raise --interval.
• since fragile in the refused-shrink path: now initialised per pool, so a future edit can't write a malformed state line.
• autoscale.conf.example / .gitignore missing: not reproducible. Both are in this PR (gh pr diff 113 --name-only), so the review probably saw a truncated diff.
• shellcheck in CI: already runs on this PR (ShellCheck check is green).

tests/pools_test.sh: all checks pass.

[github-actions]

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.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

The diff covers the ACs well: grow, cap at max, raise to min, idle-timeout shrink, busy refusal, label matching, and GitHub-unreachable skip all have corresponding tests in tests/pools_test.sh. README documents autoscale.conf and how to keep the loop running. pools.conf is never rewritten (verified by cmp test).

One gap worth flagging: the AC says "shellcheck clean" — the diff doesn't show a shellcheck invocation, and the reviewer comment claims CI runs it, but I can't verify from the diff. If shellcheck isn't actually wired up, that AC is unmet. Assuming the comment is accurate, this is fine.

2. Bugs and logic errors

queued_jobs pagination is still wrong for the jobs endpoint. The comment says --paginate was added for actions/runs, and the diff confirms it. But the inner call:

gh api "repos/$repo/actions/runs/$id/jobs?filter=latest&per_page=100" --paginate \
  --jq '.jobs[] | select(.status == "queued") | .labels | map(ascii_downcase) | join(",")'

--paginate with --jq on gh api applies the jq filter per page, which is correct here. However, filter=latest combined with --paginate is a known footgun: filter=latest is only meaningful on the first page in some GitHub API versions, and paginating past page 1 with filter=latest can return duplicate or stale jobs. More importantly, a run with >100 jobs is rare but possible; the pagination is correct in principle. Not blocking.

state_line returns the whole line, but callers only use field 2. In the refused-shrink path:

if (( target < current )) && ! ( require_idle "$name" ) < /dev/null; then
  echo "$name: not shrinking after all" >&2
  state+="$name $since"$'\n'
  continue
fi

$since here is whatever was set earlier in the loop body. The comment says it's "now initialised per pool" — looking at the code, since="$now" is set in the target="$current" reason="steady" since="$now" line, so it is initialised. But note: if the pool is growing (target > current) and require_idle somehow fails, we'd write $since=$now into state, which would reset the idle clock. That's actually the safe direction (delays shrink), so not a bug.

Real issue: require_idle is called with < /dev/null but require_idle may itself read from stdin. I can't see require_idle's definition in the diff (it's pre-existing), but the pattern of redirecting stdin everywhere suggests the author is aware. If require_idle internally calls gh without redirecting stdin, it inherits /dev/null and gh may hang or fail. Worth verifying require_idle doesn't rely on stdin.

grep -cxF "$name" <<< "$demand" counts lines exactly equal to $name. demand is built as "$best"$'\n' per job, so each line is a bare pool name. -x anchors the whole line, -F treats it literally. Correct. But if a pool name contained a newline (impossible from conf_names) or was empty, this would misbehave. Not a real concern.

queued_jobs returns 1 on any gh failure, including a single 404 on one run's jobs. This means one transient failure on one run aborts the whole repo's demand calculation and the pool is left as-is. That's the documented "GitHub-unreachable skip" behaviour, but it's coarse: a single flaky run ID poisons the whole repo. Acceptable for a first cut, but worth a follow-up.

host_label reads .env with sed -n 's/^RUNNER_HOST_LABEL=//p' | tail -1. This doesn't handle export RUNNER_HOST_LABEL=... or leading whitespace. Compose's .env parser does handle export prefixes in some versions. Minor divergence; the test only covers the plain form.

3. CLI and API safety

• No new CLI commands break existing signatures; autoscale is additive.
• No new Python dependencies (this is a shell-only PR despite the "Python CLI" framing — the diff is entirely pools.sh, tests, and README).
• GitHub API calls use gh api which inherits the user's auth. Rate-limit awareness is documented in the README with a concrete estimate. The --paginate on runs is correct.
• Shell injection: $name, $repo, $label, $mem, $pids are passed as positional args to cmd_up, not interpolated into a shell string. queued_jobs interpolates $repo into a URL — $repo comes from pools.conf which is user-controlled but local. Low risk.
• AUTOSCALE_STATE=".autoscale-state" is written in the CWD. If pools.sh is run from a different directory than where autoscale.conf lives, state and conf diverge. The script already assumes CWD-relative pools.conf, so this is consistent.

4. Test coverage

Tests are thorough and cover the ACs. Missing cases I'd want:

• --interval loop behaviour is not tested (only --once). The while :; do ... sleep loop is untested; a regression that breaks the loop or the subshell isolation wouldn't be caught.
• Multiple repos in one pass — the test config has o/r and o/idle, and the "brings a pool at 0 up" test exercises o/idle, but no test asserts both repos are processed in a single tick with independent demand.
• in_progress runs with queued jobs — the stub returns nothing for in_progress, so the "a run already in progress can have jobs still waiting" path is never exercised. This is a stated feature; it should have a test.
• --dry-run with a shrink — the dry-run test checks the grow/shrink message but the check_absent ":: up -d" only proves no compose call; it doesn't prove state isn't written. A test that .autoscale-state is untouched under --dry-run would be stronger.

5. Suggested follow-up issues

• queued_jobs: don't abort the whole repo on a single run's jobs 404; skip that run and continue
• host_label: handle export RUNNER_HOST_LABEL= and leading whitespace in .env like compose does
• Add a test for the --interval loop and for in_progress runs with queued jobs
• Document that .autoscale-state is CWD-relative and must live beside autoscale.conf

APPROVE — the ACs are met, the logic is sound, and the remaining concerns are non-blocking follow-ups.

---

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