Skip to content

ci: charge newer runs one root runner each when gui runners are on - #15124

Merged
teamleaderleo merged 2 commits into
mainfrom
fix/picker-root-charge
Sep 28, 2026
Merged

teamleaderleo merged 2 commits into
mainfrom
fix/picker-root-charge

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The PR runner picker charged each newer run's whole marker peak on an owned pool against that pool's root runners. With gui runners configured, a newer run's shards, lag and cli-product take the gui label and its side lanes never hold a root runner, so the run holds exactly one root runner (its admission).

In run 36371179217 three newer runs with a combined 32-machine peak were charged against 15 root runners (12 busy, 2 replayed): the queue bound came out at 15 x 3 - 12 - 32 - 2 = -1, so the run got a root budget of 0. Only claude-wrapper stayed owned; admission, the 7 app-host shards, lag and cli-product went to Blacksmith while owned capacity sat idle.

Fix

  • choose() reads, per owned pool, whether its gui label has a count in the slots (the same test gui_runner() uses). On such a pool it passes the newer runs' run count (Routed.runs()) to decide() as their root charge, for the bound and for "now". With the runners read live those are the live window's runs; older runs' admissions show busy on the runners or queued through older.
  • decide() gains root_since / root_now, and pick() gains root_taken / root_taken_now. The root room, the "starts now" filter and the "N of M root runners free" text use them. None keeps the old charge (the machines), so the E2E and iOS pickers are unchanged.
  • On a pool without gui runners the whole peak stays the charge: a run's root share there is its peak less the side lanes on the pool, which a marker does not split out.
  • Docstrings updated; the "whole marker peak" claim in decide() now describes the new parameters.

Tests

python3 -m unittest tests/test_ci_pr_runner_pool.py: 216 tests OK.

  • test_newer_runs_hold_one_root_runner_each_with_gui_runners reproduces the run's numbers (15 root online, 12 busy, 2 replayed, 3 newer runs with a 32-machine peak). With gui runners the run takes std with a root budget of 28 and its admission is placed owned. With a gui count only on another pool, std keeps the whole-peak charge and the budget stays 0, as before.
  • test_decide_charges_the_root_runners_newer_runs_hold checks the root text reads the passed root charge.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the PR runner picker charging each newer run's whole marker peak against root runners when gui runners are on; it now charges one root runner per newer run (their admission).

With gui runners, a newer run's shards, lag and cli-product take the gui label and side lanes never hold a root runner, so it holds exactly one root runner. Charging the whole peak left a run with a root budget of 0 out of 15 root runners while three newer runs held only 3, sending its admission to Blacksmith (run 36371179217).

  • choose() passes the newer runs' run count as their root charge per pool where that pool's gui label has a slot count; on other pools the whole peak stays the charge since a marker does not split side lanes from root jobs.
  • decide() and pick() now accept the root charge separately; None keeps the old behavior, so the E2E and iOS pickers are unchanged.
  • Tests reproduce run 36371179217 and verify the root text.

Written for commit 69a4efe. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Runner-pool capacity calculations now account separately for root-runner usage by newer runs when GUI runners are enabled. This improves estimates of available root runners and can affect which runs are admitted to the pool.
    • When GUI runners are not enabled, root-runner accounting continues to follow existing machine-usage behavior. Calls that omit the new accounting information retain the previous peak-based fallback.
  • Tests
    • Added coverage for root-runner availability and capacity decisions with newer runs.

The picker charged a newer run's whole marker peak on the std pool to its
root runners. With gui runners, shards, lag and cli-product take the gui
label and side lanes never hold a root runner, so a newer run holds only
its admission there. Three newer runs with a 32-machine peak left 0 of 15
root runners for a run while they held 3, and its admission and follow-on
jobs went to Blacksmith (run 36371179217).

choose() now passes the newer runs' run count as their root charge when
the slots name gui runners; without gui runners the whole peak stays the
charge, since a marker does not split side lanes from root jobs.

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

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: be8be423-73a5-4854-baad-205eac36675e

📥 Commits

Reviewing files that changed from the base of the PR and between 3606617 and 254430a.

📒 Files selected for processing (2)
  • scripts/ci/pr_runner_pool.py
  • tests/test_ci_pr_runner_pool.py
 __________________________
< Grammarly for your code. >
 --------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

Review: a pool's newer runs hold one root runner only where that pool's
gui label has a slot count (gui_runner()); any other pool keeps the peak.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@teamleaderleo
teamleaderleo merged commit f1c54d0 into main Sep 28, 2026
52 checks passed
@teamleaderleo
teamleaderleo deleted the fix/picker-root-charge branch September 28, 2026 03:45
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 69a4efe764: every check was green at merge (13 verified; 18 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 28, 2026
10505af fix(cloud): recover machine list on app foreground (manaflow-ai#15100) (manaflow-ai#15104)
22835e8 Docs: raise search field contrast (manaflow-ai#14365)
18abc85 Report a non-running terminal as surface_unavailable in read_text (manaflow-ai#15101)
b23420c Document and tool in-place cmux-tui upgrades for running Cloud machines (manaflow-ai#15122)
f1c54d0 ci: charge newer runs one root runner each when gui runners are on (manaflow-ai#15124)
b7ce8d0 Bound the Iroh release-gate launcher
3606617 Stop CLI Sentry floods from caller state and unattributed journal failures (manaflow-ai#15103)
446581e ci: send owned gui jobs past a round of the gui queue to Blacksmith (manaflow-ai#15115)
507890c ci: refit the warm-distance model on 741 owned admissions (manaflow-ai#15117)
5bee212 Match the CMUX_NO_GIT_WATCH contract to bash without a PR poller (manaflow-ai#15099)

# Conflicts:
#	.github/workflows/ci-macos.yml
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