Skip to content

Balance pooled job distribution across runner children (first-fit → round-robin)#953

Open
kaspernj wants to merge 1 commit into
masterfrom
feat/round-robin-pooled-child-selection
Open

Balance pooled job distribution across runner children (first-fit → round-robin)#953
kaspernj wants to merge 1 commit into
masterfrom
feat/round-robin-pooled-child-selection

Conversation

@kaspernj

Copy link
Copy Markdown
Owner

Problem

worker.js._runPooledJob selected the first non-retiring pooled child with a free concurrency slot (first-fit), so jobs packed the earliest child until full before touching the next. Long-running RunBuildJobs — each pinning a projectTenant connection for its whole multi-minute run — therefore concentrated in the early children: one child's tenant pool carried ~19 builds plus the planner while later children sat idle, and early children hit their maxJobs/RSS retirement thresholds far faster.

Fix

Select children round-robin: among non-retiring children with a free slot, pick the one dispatched least recently (a monotonic per-child lastDispatchSeq stamped on each dispatch). Jobs — notably builds — spread evenly across all children, so no single child's pool absorbs the whole build + planner load, and retirement wear evens out. The lazy-spawn fallback is unchanged: a new child is created only when every non-retiring child is full.

Round-robin, not pick-least-loaded (deliberate)

Picking the child with the fewest in-flight jobs would spread builds too, but a freshly spawned or replacement child (0 in-flight) would be picked repeatedly until it "caught up" to the others — a burst that slams a just-booted child. Round-robin gives a fresh child its fair share one job at a time as its turn comes up, avoiding that burst while still spreading load.

Tests

spec/background-jobs/worker-pooled-distribution-spec.js:

  • 6 jobs across 3 children land [2, 2, 2] (round-robin), not first-fit [4, 2, 0].
  • a freshly added child among already-loaded children takes one job, then the rotation moves on (no catch-up burst).
  • retiring children are skipped, and selection returns undefined (→ lazy spawn) only when every non-retiring child is full.

⚠️ Note: master is currently red on typecheck (pre-existing, not this change)

npx tsc --noEmit on origin/master (commit 9ddc6c5e, #952 "Bound frontend-model WebSocket lifecycle controls") already fails with two errors in src/frontend-models/base.js (signal/timeoutMs not in the websocket post/connect option types) — unrelated to this PR. This change's own worker.js typechecks clean and its spec passes. The pre-existing errors will need fixing (in #952's code) for the package lint to go green.

🤖 Generated with Claude Code

…round-robin)

`_runPooledJob` selected the FIRST non-retiring child with a free slot, so jobs
packed the earliest child until full before touching the next. Long-running
RunBuildJobs (each pinning a projectTenant connection for its whole multi-minute
run) therefore concentrated in the early children — one child's tenant pool
carried ~19 builds plus the planner while later children sat idle — and early
children hit their retirement (maxJobs/RSS) thresholds far faster.

Select children round-robin instead: among non-retiring children with a free
concurrency slot, pick the one dispatched least recently (a monotonic per-child
`lastDispatchSeq` stamped on each dispatch). This spreads jobs — notably builds —
evenly across all children so no single child's pool absorbs the whole build +
planner load, and evens out retirement wear. The lazy-spawn fallback is
unchanged: a new child is created only when every non-retiring child is full.

Round-robin rather than pick-least-loaded (by in-flight count) is deliberate: a
freshly spawned or replacement child takes its fair share one job at a time as
its turn comes up, instead of a fresh (0-in-flight) child being picked
repeatedly to "catch up" to the others — a burst that would slam a just-booted
child.

Adds a distribution spec: even spread (not first-fit), no fresh-child burst, and
retiring children skipped with lazy spawn only when all are full.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6fc305e484

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


if (!state || state.retiring || state.inflight.size >= this.pooledRunnerConcurrency) continue

if (state.lastDispatchSeq < selectedSeq) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Default unstamped pooled child states

When a seeded pooled child state does not include the new lastDispatchSeq field, state.lastDispatchSeq is undefined, so undefined < Infinity is false and _selectPooledChild() returns undefined even though the child has capacity. Existing worker specs create fake pooledChildStates this way and then call _runPooledJob() on a worker with no configuration, so this now falls through to _createPooledChild() and throws instead of using the fake child; please either default missing sequence values to 0 here or update all seeded child states.

Useful? React with 👍 / 👎.

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.

2 participants