feat(orchestrator): built-in load balancing across providers#783
Draft
frostebite wants to merge 6 commits intomainfrom
Draft
feat(orchestrator): built-in load balancing across providers#783frostebite wants to merge 6 commits intomainfrom
frostebite wants to merge 6 commits intomainfrom
Conversation
…ity check Adds built-in load balancing: check GitHub runner availability before builds start, auto-route to a fallback provider when runners are busy or offline. Eliminates the need for a separate check-runner job. New inputs: fallbackProviderStrategy, runnerCheckEnabled, runnerCheckLabels, runnerCheckMinAvailable. Outputs providerFallbackUsed and providerFallbackReason for workflow visibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds six new orchestrator inputs and wires them through options and build parameters; implements RunnerAvailabilityService to probe GitHub Actions runners (with tests); integrates pre-selection runner checks, provider init timeout, and retry-on-fallback logic into the orchestrator runtime. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(2,129,204,0.5)
participant Orchestrator
end
rect rgba(34,197,94,0.5)
participant RunnerAvailabilityService
end
rect rgba(249,115,22,0.5)
participant GitHubAPI
end
rect rgba(168,85,247,0.5)
participant Provider
end
Orchestrator->>RunnerAvailabilityService: checkAvailability(owner, repo, token, labels, minAvailable)
RunnerAvailabilityService->>GitHubAPI: GET /repos/{owner}/{repo}/actions/runners (paged)
GitHubAPI-->>RunnerAvailabilityService: runners pages
RunnerAvailabilityService-->>Orchestrator: RunnerCheckResult(shouldFallback, reason, counts)
alt shouldFallback == true
Orchestrator->>Provider: select fallback provider
else shouldFallback == false
Orchestrator->>Provider: use primary provider
end
Orchestrator->>Provider: setupWorkflow() (with providerInitTimeout)
alt setup times out or fails and retryOnFallback == true
Orchestrator->>Provider: switch to fallback and runWithProvider()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Adds retryOnFallback (retry failed builds on alternate provider) and providerInitTimeout (swap provider if init takes too long). Refactors run() into run()/runWithProvider() to support retry loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostebite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/model/orchestrator/services/core/runner-availability-service.ts (1)
136-140: UseSetfor label membership checks.This avoids repeated linear
includeslookups and aligns with theunicorn/prefer-set-hasguidance.⚡ Suggested refactor
return runners.filter((runner) => { - const runnerLabelNames = runner.labels.map((l) => l.name.toLowerCase()); + const runnerLabelNames = new Set(runner.labels.map((label) => label.name.toLowerCase())); - return requiredLabels.every((required) => runnerLabelNames.includes(required.toLowerCase())); + return requiredLabels.every((required) => runnerLabelNames.has(required.toLowerCase())); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.ts` around lines 136 - 140, Replace the per-runner array includes checks with a Set membership test to avoid repeated linear scans: in the runners.filter callback (the block that creates runnerLabelNames and calls requiredLabels.every), build runnerLabelNames as a Set of lower-cased label names and then use runnerLabelNames.has(required.toLowerCase()) inside requiredLabels.every; this keeps the same semantics but changes membership checks from includes to O(1) Set.has lookups.action.yml (1)
197-220: Add new fallback outputs to action metadata.This feature now emits
providerFallbackUsedandproviderFallbackReason, but they are not declared underoutputsinaction.yml, so the action interface is incomplete.📦 Suggested metadata update
outputs: volume: description: 'The Persistent Volume (PV) where the build artifacts have been stored by Kubernetes' buildVersion: description: 'The generated version used for the Unity build' androidVersionCode: description: 'The generated versionCode used for the Android Unity build' engineExitCode: description: 'Returns the exit code from the build scripts. This code is 0 if the build was successful. If there was an error during activation, the code is from the activation step. If activation is successful, the code is from the project build step.' + providerFallbackUsed: + description: 'True when fallbackProviderStrategy was selected.' + providerFallbackReason: + description: 'Reason returned by runner availability check/fallback decision.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 197 - 220, The action metadata is missing declarations for the two new outputs; add entries for providerFallbackUsed and providerFallbackReason under the outputs section in action.yml (matching the style of the existing outputs), giving each a clear description (e.g., providerFallbackUsed: whether a fallback provider was used; providerFallbackReason: brief reason or trigger for fallback) so the action interface accurately advertises these outputs and their exact names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/options/orchestrator-options.ts`:
- Around line 149-157: The runner label parsing in
OrchestratorOptions.runnerCheckLabels should remove empty entries created by
trailing/duplicate commas: after
OrchestratorOptions.getInput('runnerCheckLabels') split and trim, filter out any
zero-length strings (e.g., .filter(Boolean) or s => s.length>0) before
returning. For OrchestratorOptions.runnerCheckMinAvailable, parse the input as
an integer (use parseInt and check isNaN) and clamp it to a minimum of 1 (e.g.,
Math.max(1, parsedValue)) so negative, zero, or non-numeric values cannot
disable fallback; keep using OrchestratorOptions.getInput to fetch the raw
value.
In `@src/model/orchestrator/orchestrator.ts`:
- Around line 81-112: The runner availability check is currently skipped when
Orchestrator.buildParameters.fallbackProviderStrategy is not set; change the
logic so RunnerAvailabilityService.checkAvailability is invoked whenever
Orchestrator.buildParameters.runnerCheckEnabled is true, and only perform the
provider swap and call
core.setOutput('providerFallbackUsed'/'providerFallbackReason') when a
fallbackProviderStrategy exists and result.shouldFallback is true; keep logging
of result.reason and set providerFallbackUsed to 'false' when no fallback is
applied or none configured. Locate the block around
Orchestrator.buildParameters.runnerCheckEnabled,
RunnerAvailabilityService.checkAvailability, and where providerStrategy is
assigned to implement this flow.
In `@src/model/orchestrator/services/core/runner-availability-service.ts`:
- Around line 111-117: Replace the infinite loop and snake_case lint issues by
switching from while (true) to a boolean-controlled loop (e.g., let hasMore =
true; while (hasMore) { ... }) and update pagination termination by inspecting
response.data.runners.length to set hasMore = false or increment page; keep
using octokit.request but pass the snake_case API param as a quoted property
mapped from our camelCase variable (e.g., { owner, repo, "per_page": perPage,
page }) so ESLint camelcase is not violated; adjust any variable increments
(page++) and loop-exit logic in the pagination block that calls octokit.request
to use hasMore.
---
Nitpick comments:
In `@action.yml`:
- Around line 197-220: The action metadata is missing declarations for the two
new outputs; add entries for providerFallbackUsed and providerFallbackReason
under the outputs section in action.yml (matching the style of the existing
outputs), giving each a clear description (e.g., providerFallbackUsed: whether a
fallback provider was used; providerFallbackReason: brief reason or trigger for
fallback) so the action interface accurately advertises these outputs and their
exact names.
In `@src/model/orchestrator/services/core/runner-availability-service.ts`:
- Around line 136-140: Replace the per-runner array includes checks with a Set
membership test to avoid repeated linear scans: in the runners.filter callback
(the block that creates runnerLabelNames and calls requiredLabels.every), build
runnerLabelNames as a Set of lower-cased label names and then use
runnerLabelNames.has(required.toLowerCase()) inside requiredLabels.every; this
keeps the same semantics but changes membership checks from includes to O(1)
Set.has lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8412b762-2293-4bac-a94c-7e99aef74c0e
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (5)
action.ymlsrc/model/build-parameters.tssrc/model/orchestrator/options/orchestrator-options.tssrc/model/orchestrator/orchestrator.tssrc/model/orchestrator/services/core/runner-availability-service.ts
Comment on lines
+149
to
+157
| static get runnerCheckLabels(): string[] { | ||
| const labels = OrchestratorOptions.getInput('runnerCheckLabels'); | ||
|
|
||
| return labels ? labels.split(',').map((l) => l.trim()) : []; | ||
| } | ||
|
|
||
| static get runnerCheckMinAvailable(): number { | ||
| return Number(OrchestratorOptions.getInput('runnerCheckMinAvailable')) || 1; | ||
| } |
There was a problem hiding this comment.
Sanitize runner labels and validate minimum-available input.
Line [152] keeps empty entries (e.g., trailing comma), which can force zero matches. Line [156] also accepts negative values, which can silently disable fallback decisions.
✅ Suggested validation hardening
static get runnerCheckLabels(): string[] {
const labels = OrchestratorOptions.getInput('runnerCheckLabels');
- return labels ? labels.split(',').map((l) => l.trim()) : [];
+ return labels
+ ? labels
+ .split(',')
+ .map((label) => label.trim())
+ .filter((label) => label.length > 0)
+ : [];
}
static get runnerCheckMinAvailable(): number {
- return Number(OrchestratorOptions.getInput('runnerCheckMinAvailable')) || 1;
+ const parsed = Number.parseInt(OrchestratorOptions.getInput('runnerCheckMinAvailable') || '1', 10);
+ return Number.isFinite(parsed) && parsed > 0 ? parsed : 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/options/orchestrator-options.ts` around lines 149 -
157, The runner label parsing in OrchestratorOptions.runnerCheckLabels should
remove empty entries created by trailing/duplicate commas: after
OrchestratorOptions.getInput('runnerCheckLabels') split and trim, filter out any
zero-length strings (e.g., .filter(Boolean) or s => s.length>0) before
returning. For OrchestratorOptions.runnerCheckMinAvailable, parse the input as
an integer (use parseInt and check isNaN) and clamp it to a minimum of 1 (e.g.,
Math.max(1, parsedValue)) so negative, zero, or non-numeric values cannot
disable fallback; keep using OrchestratorOptions.getInput to fetch the raw
value.
Comment on lines
+81
to
+112
| if (Orchestrator.buildParameters.runnerCheckEnabled && Orchestrator.buildParameters.fallbackProviderStrategy) { | ||
| const owner = OrchestratorOptions.githubOwner; | ||
| const repo = OrchestratorOptions.githubRepoName; | ||
| const token = Orchestrator.buildParameters.gitPrivateToken || process.env.GITHUB_TOKEN || ''; | ||
|
|
||
| OrchestratorLogger.log( | ||
| `Checking runner availability (labels: [${Orchestrator.buildParameters.runnerCheckLabels.join(', ')}], min: ${Orchestrator.buildParameters.runnerCheckMinAvailable})`, | ||
| ); | ||
|
|
||
| const result = await RunnerAvailabilityService.checkAvailability( | ||
| owner, | ||
| repo, | ||
| token, | ||
| Orchestrator.buildParameters.runnerCheckLabels, | ||
| Orchestrator.buildParameters.runnerCheckMinAvailable, | ||
| ); | ||
|
|
||
| OrchestratorLogger.log( | ||
| `Runner check: ${result.totalRunners} total, ${result.matchingRunners} matching, ${result.idleRunners} idle — ${result.reason}`, | ||
| ); | ||
|
|
||
| if (result.shouldFallback) { | ||
| const original = Orchestrator.buildParameters.providerStrategy; | ||
| const fallback = Orchestrator.buildParameters.fallbackProviderStrategy; | ||
| OrchestratorLogger.log(`Falling back from '${original}' to '${fallback}' — ${result.reason}`); | ||
| Orchestrator.buildParameters.providerStrategy = fallback; | ||
| core.setOutput('providerFallbackUsed', 'true'); | ||
| core.setOutput('providerFallbackReason', result.reason); | ||
| } else { | ||
| core.setOutput('providerFallbackUsed', 'false'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Runner availability check is incorrectly gated by fallback configuration.
Line [81] currently skips the check unless fallbackProviderStrategy is set. That prevents the intended informational check-only mode when runnerCheckEnabled=true and no fallback is configured.
🔧 Suggested logic fix
- if (Orchestrator.buildParameters.runnerCheckEnabled && Orchestrator.buildParameters.fallbackProviderStrategy) {
+ if (Orchestrator.buildParameters.runnerCheckEnabled) {
const owner = OrchestratorOptions.githubOwner;
const repo = OrchestratorOptions.githubRepoName;
const token = Orchestrator.buildParameters.gitPrivateToken || process.env.GITHUB_TOKEN || '';
@@
- if (result.shouldFallback) {
+ if (result.shouldFallback && Orchestrator.buildParameters.fallbackProviderStrategy) {
const original = Orchestrator.buildParameters.providerStrategy;
const fallback = Orchestrator.buildParameters.fallbackProviderStrategy;
OrchestratorLogger.log(`Falling back from '${original}' to '${fallback}' — ${result.reason}`);
Orchestrator.buildParameters.providerStrategy = fallback;
core.setOutput('providerFallbackUsed', 'true');
core.setOutput('providerFallbackReason', result.reason);
+ } else if (result.shouldFallback) {
+ OrchestratorLogger.log(
+ `Runner check requested fallback, but fallbackProviderStrategy is empty — staying on primary provider`,
+ );
+ core.setOutput('providerFallbackUsed', 'false');
+ core.setOutput('providerFallbackReason', result.reason);
} else {
core.setOutput('providerFallbackUsed', 'false');
+ core.setOutput('providerFallbackReason', result.reason);
}
}🧰 Tools
🪛 ESLint
[error] 87-87: Replace Orchestrator.buildParameters.runnerCheckMinAvailable with ⏎··········Orchestrator.buildParameters.runnerCheckMinAvailable⏎········
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/orchestrator.ts` around lines 81 - 112, The runner
availability check is currently skipped when
Orchestrator.buildParameters.fallbackProviderStrategy is not set; change the
logic so RunnerAvailabilityService.checkAvailability is invoked whenever
Orchestrator.buildParameters.runnerCheckEnabled is true, and only perform the
provider swap and call
core.setOutput('providerFallbackUsed'/'providerFallbackReason') when a
fallbackProviderStrategy exists and result.shouldFallback is true; keep logging
of result.reason and set providerFallbackUsed to 'false' when no fallback is
applied or none configured. Locate the block around
Orchestrator.buildParameters.runnerCheckEnabled,
RunnerAvailabilityService.checkAvailability, and where providerStrategy is
assigned to implement this flow.
Comment on lines
+111
to
+117
| while (true) { | ||
| const response = await octokit.request('GET /repos/{owner}/{repo}/actions/runners', { | ||
| owner, | ||
| repo, | ||
| per_page: perPage, | ||
| page, | ||
| }); |
There was a problem hiding this comment.
Resolve ESLint blockers in pagination loop.
Line [111] and Line [115] currently trigger lint errors (no-constant-condition, camelcase).
🧹 Lint-safe pagination rewrite
- while (true) {
+ let hasMore = true;
+ while (hasMore) {
const response = await octokit.request('GET /repos/{owner}/{repo}/actions/runners', {
owner,
repo,
- per_page: perPage,
+ 'per_page': perPage,
page,
});
const runners = (response.data.runners || []) as GitHubRunner[];
allRunners.push(...runners);
- if (runners.length < perPage) break;
- page++;
+ hasMore = runners.length === perPage;
+ page += 1;
}🧰 Tools
🪛 ESLint
[error] 111-111: Unexpected constant condition.
(no-constant-condition)
[error] 115-115: Identifier 'per_page' is not in camel case.
(camelcase)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/runner-availability-service.ts` around
lines 111 - 117, Replace the infinite loop and snake_case lint issues by
switching from while (true) to a boolean-controlled loop (e.g., let hasMore =
true; while (hasMore) { ... }) and update pagination termination by inspecting
response.data.runners.length to set hasMore = false or increment page; keep
using octokit.request but pass the snake_case API param as a quoted property
mapped from our camelCase variable (e.g., { owner, repo, "per_page": perPage,
page }) so ESLint camelcase is not violated; adjust any variable increments
(page++) and loop-exit logic in the pagination block that calls octokit.request
to use hasMore.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/model/orchestrator/options/orchestrator-options.ts (1)
149-157:⚠️ Potential issue | 🟠 MajorSanitize labels and clamp minimum available runner count.
Line [152] preserves empty labels, and Line [156] allows negative values. Both can distort fallback decisions. This was previously flagged and is still present.
Suggested input hardening
static get runnerCheckLabels(): string[] { const labels = OrchestratorOptions.getInput('runnerCheckLabels'); - return labels ? labels.split(',').map((l) => l.trim()) : []; + return labels + ? labels + .split(',') + .map((label) => label.trim()) + .filter((label) => label.length > 0) + : []; } static get runnerCheckMinAvailable(): number { - return Number(OrchestratorOptions.getInput('runnerCheckMinAvailable')) || 1; + const parsed = Number.parseInt(OrchestratorOptions.getInput('runnerCheckMinAvailable') || '1', 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-options.ts` around lines 149 - 157, Sanitize runnerCheckLabels by splitting the OrchestratorOptions.getInput('runnerCheckLabels') value, trimming each entry, and filtering out any empty strings so no blank labels are preserved (update the static getter runnerCheckLabels). Clamp and validate runnerCheckMinAvailable by parsing the OrchestratorOptions.getInput('runnerCheckMinAvailable') into a number (use Number or parseInt), treat NaN as fallback, and ensure the returned value is at least 1 (no negatives or zero) in the static getter runnerCheckMinAvailable.src/model/orchestrator/orchestrator.ts (1)
80-112:⚠️ Potential issue | 🟠 MajorRunner availability check is still incorrectly gated by fallback config.
Line [81] requires
fallbackProviderStrategy, sorunnerCheckEnabled=truewithout fallback skips the check entirely (no informational mode). This was previously flagged and remains unresolved.Suggested control-flow fix
- if (Orchestrator.buildParameters.runnerCheckEnabled && Orchestrator.buildParameters.fallbackProviderStrategy) { + if (Orchestrator.buildParameters.runnerCheckEnabled) { @@ - if (result.shouldFallback) { + if (result.shouldFallback && Orchestrator.buildParameters.fallbackProviderStrategy) { const original = Orchestrator.buildParameters.providerStrategy; const fallback = Orchestrator.buildParameters.fallbackProviderStrategy; OrchestratorLogger.log(`Falling back from '${original}' to '${fallback}' — ${result.reason}`); Orchestrator.buildParameters.providerStrategy = fallback; core.setOutput('providerFallbackUsed', 'true'); core.setOutput('providerFallbackReason', result.reason); + } else if (result.shouldFallback) { + OrchestratorLogger.log( + `Runner check requested fallback, but fallbackProviderStrategy is empty — staying on primary provider`, + ); + core.setOutput('providerFallbackUsed', 'false'); + core.setOutput('providerFallbackReason', result.reason); } else { core.setOutput('providerFallbackUsed', 'false'); + core.setOutput('providerFallbackReason', result.reason); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/orchestrator.ts` around lines 80 - 112, The runner availability check is incorrectly skipped when fallbackProviderStrategy is not set; change the outer guard to only require Orchestrator.buildParameters.runnerCheckEnabled so RunnerAvailabilityService.checkAvailability always runs, then inside the block keep the existing fallback application only when Orchestrator.buildParameters.fallbackProviderStrategy is present: call RunnerAvailabilityService.checkAvailability (as currently), log results, and if result.shouldFallback and Orchestrator.buildParameters.fallbackProviderStrategy is truthy swap providerStrategy, set core outputs providerFallbackUsed='true' and providerFallbackReason=result.reason; otherwise set providerFallbackUsed='false' and still set providerFallbackReason=result.reason (so teams get informational output even when no fallback is configured).
🧹 Nitpick comments (2)
src/model/orchestrator/options/orchestrator-options.ts (1)
163-165: ClampproviderInitTimeoutto a non-negative integer.Using
Number(...) || 0accepts decimals/negatives implicitly. Parsing as integer and clamping at>= 0makes timeout behavior deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-options.ts` around lines 163 - 165, The static getter OrchestratorOptions.providerInitTimeout currently uses Number(...) || 0 which allows negatives and decimals; change it to parse and clamp to a non-negative integer: retrieve the raw input via OrchestratorOptions.getInput('providerInitTimeout'), parse it to an integer (e.g., parseInt or Math.floor after Number), handle NaN by falling back to 0, and ensure the returned value is Math.max(0, parsedInt) so negatives become 0 and decimals are converted to an integer.action.yml (1)
197-232: Declare fallback outputs inaction.ymlfor a complete action contract.The new fallback flow emits
providerFallbackUsedandproviderFallbackReasonviacore.setOutput(), but they are not declared in theoutputssection. Declaring them explicitly improves discoverability and makes the action's public contract clear to users.Proposed metadata addition
outputs: volume: description: 'The Persistent Volume (PV) where the build artifacts have been stored by Kubernetes' buildVersion: description: 'The generated version used for the Unity build' androidVersionCode: description: 'The generated versionCode used for the Android Unity build' + providerFallbackUsed: + description: 'true if fallback provider was selected' + providerFallbackReason: + description: 'Reason fallback was selected or attempted' engineExitCode: description: 'Returns the exit code from the build scripts. This code is 0 if the build was successful. If there was an error during activation, the code is from the activation step. If activation is successful, the code is from the project build step.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 197 - 232, Add explicit outputs for the fallback flow: declare providerFallbackUsed and providerFallbackReason in the action metadata outputs section so the action's public contract matches the core.setOutput() calls. Update the outputs block to include both names with short descriptions (e.g., whether a fallback was used and the reason) and appropriate value examples or types, ensuring they appear alongside the existing outputs so consumers can discover and document the fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/orchestrator.ts`:
- Around line 358-365: The current Promise.race([setupPromise, timeoutPromise])
can leave setupPromise running and performing side effects after a timeout; make
provider setup cancellable and signal it on timeout: create an AbortController,
pass controller.signal into the setup routine (or replace setupPromise with a
cancellableSetup function that accepts an AbortSignal), and on timeout call
controller.abort() instead of just rejecting; also ensure the setup
implementation (the code behind setupPromise) checks the signal (or a shared
timedOut flag) before performing any external mutations and properly
throws/cleans up when aborted, and await or handle the completion of
setupPromise to avoid unhandled rejections.
---
Duplicate comments:
In `@src/model/orchestrator/options/orchestrator-options.ts`:
- Around line 149-157: Sanitize runnerCheckLabels by splitting the
OrchestratorOptions.getInput('runnerCheckLabels') value, trimming each entry,
and filtering out any empty strings so no blank labels are preserved (update the
static getter runnerCheckLabels). Clamp and validate runnerCheckMinAvailable by
parsing the OrchestratorOptions.getInput('runnerCheckMinAvailable') into a
number (use Number or parseInt), treat NaN as fallback, and ensure the returned
value is at least 1 (no negatives or zero) in the static getter
runnerCheckMinAvailable.
In `@src/model/orchestrator/orchestrator.ts`:
- Around line 80-112: The runner availability check is incorrectly skipped when
fallbackProviderStrategy is not set; change the outer guard to only require
Orchestrator.buildParameters.runnerCheckEnabled so
RunnerAvailabilityService.checkAvailability always runs, then inside the block
keep the existing fallback application only when
Orchestrator.buildParameters.fallbackProviderStrategy is present: call
RunnerAvailabilityService.checkAvailability (as currently), log results, and if
result.shouldFallback and Orchestrator.buildParameters.fallbackProviderStrategy
is truthy swap providerStrategy, set core outputs providerFallbackUsed='true'
and providerFallbackReason=result.reason; otherwise set
providerFallbackUsed='false' and still set providerFallbackReason=result.reason
(so teams get informational output even when no fallback is configured).
---
Nitpick comments:
In `@action.yml`:
- Around line 197-232: Add explicit outputs for the fallback flow: declare
providerFallbackUsed and providerFallbackReason in the action metadata outputs
section so the action's public contract matches the core.setOutput() calls.
Update the outputs block to include both names with short descriptions (e.g.,
whether a fallback was used and the reason) and appropriate value examples or
types, ensuring they appear alongside the existing outputs so consumers can
discover and document the fallback behavior.
In `@src/model/orchestrator/options/orchestrator-options.ts`:
- Around line 163-165: The static getter OrchestratorOptions.providerInitTimeout
currently uses Number(...) || 0 which allows negatives and decimals; change it
to parse and clamp to a non-negative integer: retrieve the raw input via
OrchestratorOptions.getInput('providerInitTimeout'), parse it to an integer
(e.g., parseInt or Math.floor after Number), handle NaN by falling back to 0,
and ensure the returned value is Math.max(0, parsedInt) so negatives become 0
and decimals are converted to an integer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e642e00-c9f8-410e-9c9d-e11af0e07541
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
action.ymlsrc/model/build-parameters.tssrc/model/orchestrator/options/orchestrator-options.tssrc/model/orchestrator/orchestrator.ts
Comment on lines
+358
to
+365
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout( | ||
| () => reject(new Error(`Provider initialization timed out after ${timeoutSeconds}s`)), | ||
| timeoutSeconds * 1000, | ||
| ); | ||
| }); | ||
|
|
||
| await Promise.race([setupPromise, timeoutPromise]); |
There was a problem hiding this comment.
Timeout fallback can double-execute provider setup side effects.
Promise.race times out without canceling setupPromise. If retry/fallback proceeds, the original setup can still mutate external systems in parallel (e.g., K8s service-account creation), causing duplicate/inconsistent resources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/orchestrator.ts` around lines 358 - 365, The current
Promise.race([setupPromise, timeoutPromise]) can leave setupPromise running and
performing side effects after a timeout; make provider setup cancellable and
signal it on timeout: create an AbortController, pass controller.signal into the
setup routine (or replace setupPromise with a cancellableSetup function that
accepts an AbortSignal), and on timeout call controller.abort() instead of just
rejecting; also ensure the setup implementation (the code behind setupPromise)
checks the signal (or a shared timedOut flag) before performing any external
mutations and properly throws/cleans up when aborted, and await or handle the
completion of setupPromise to avoid unhandled rejections.
Covers: no token skip, no runners fallback, busy/offline runners, label filtering (case-insensitive), minAvailable threshold, fail-open on API error, mixed runner states. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/model/orchestrator/services/core/runner-availability-service.test.ts (1)
45-191: Consider deduplicating repeated Octokit mock setup.The same
mockRequest+MockedOctokit.mockImplementationpattern is repeated across many tests; a small helper will reduce noise and improve maintainability.Refactor sketch
+function mockRunnersResponse(runners: ReturnType<typeof createMockRunners>) { + const request = jest.fn().mockResolvedValue({ data: { runners } }); + MockedOctokit.mockImplementation(() => ({ request } as any)); + return request; +} + +function mockRunnersError(error: Error) { + const request = jest.fn().mockRejectedValue(error); + MockedOctokit.mockImplementation(() => ({ request } as any)); + return request; +}Then in tests:
-const mockRequest = jest.fn().mockResolvedValue({ data: { runners } }); -MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any); +mockRunnersResponse(runners);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.test.ts` around lines 45 - 191, Extract the repeated Octokit mocking into a small test helper (e.g., setOctokitMock or mockOctokitRequest) that accepts a resolved/rejected value or a runners array and internally creates the jest.fn() mockRequest and calls MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any); then update tests that call RunnerAvailabilityService.checkAvailability to use this helper instead of duplicating the mockRequest creation; reference MockedOctokit, mockRequest, and RunnerAvailabilityService.checkAvailability when locating places to replace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`:
- Line 47: Several lines in runner-availability-service.test.ts have Prettier
formatting violations (notably parenthesization of "as any" casts and a
multiline array literal) that will fail linting; locate the
MockedOctokit.mockImplementation calls (e.g., the line with
MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any)) and
other similar mocked implementations and adjust parentheses so casts are applied
to the entire object (e.g., wrap the returned object in parentheses before "as
any"), and reformat the multiline array literal to conform to Prettier style
(consistent commas and indentation). Run the project's Prettier/formatter or
apply the same formatting rules across the listed lines (47, 61, 75, 88, 101,
122, 138-140, 142, 155, 170, 184) to ensure tests pass lint checks.
- Around line 24-25: The test helper uses an abbreviated loop index `i` inside
the runners.map callback; rename `i` to a descriptive identifier like `index`
(or `runnerIndex`) to satisfy unicorn/prevent-abbreviations. Update the map
callback signature in the test (the runners.map call in
runner-availability-service.test.ts) and replace all uses of `i` within that
callback (e.g., building the returned object id: i + 1) to the new name so the
behavior remains unchanged.
- Around line 19-21: The import/first lint error is caused by executing code
before the import of Octokit; move the import declaration for Octokit so it
appears before any executable statements (specifically before the MockedOctokit
= Octokit as jest.MockedClass<typeof Octokit> assignment) in
runner-availability-service.test.ts; ensure the top of the file imports Octokit
first, then declare MockedOctokit and any other mocks or executable code.
---
Nitpick comments:
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`:
- Around line 45-191: Extract the repeated Octokit mocking into a small test
helper (e.g., setOctokitMock or mockOctokitRequest) that accepts a
resolved/rejected value or a runners array and internally creates the jest.fn()
mockRequest and calls MockedOctokit.mockImplementation(() => ({ request:
mockRequest }) as any); then update tests that call
RunnerAvailabilityService.checkAvailability to use this helper instead of
duplicating the mockRequest creation; reference MockedOctokit, mockRequest, and
RunnerAvailabilityService.checkAvailability when locating places to replace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb347516-f778-4ffa-a273-8664c38a1cb7
📒 Files selected for processing (1)
src/model/orchestrator/services/core/runner-availability-service.test.ts
Comment on lines
+19
to
+21
| import { Octokit } from '@octokit/core'; | ||
|
|
||
| const MockedOctokit = Octokit as jest.MockedClass<typeof Octokit>; |
There was a problem hiding this comment.
Fix import ordering to satisfy import/first.
Line 19 imports Octokit after executable statements, which fails ESLint and can block CI.
Suggested fix
import { RunnerAvailabilityService } from './runner-availability-service';
+import { Octokit } from '@octokit/core';
// Mock `@octokit/core`
jest.mock('@octokit/core', () => ({
Octokit: jest.fn().mockImplementation(() => ({
request: jest.fn(),
})),
}));
@@
-import { Octokit } from '@octokit/core';
-
const MockedOctokit = Octokit as jest.MockedClass<typeof Octokit>;🧰 Tools
🪛 ESLint
[error] 19-19: Import in body of module; reorder to top.
(import/first)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`
around lines 19 - 21, The import/first lint error is caused by executing code
before the import of Octokit; move the import declaration for Octokit so it
appears before any executable statements (specifically before the MockedOctokit
= Octokit as jest.MockedClass<typeof Octokit> assignment) in
runner-availability-service.test.ts; ensure the top of the file imports Octokit
first, then declare MockedOctokit and any other mocks or executable code.
Comment on lines
+24
to
+25
| return runners.map((r, i) => ({ | ||
| id: i + 1, |
There was a problem hiding this comment.
Rename loop index i to a descriptive name.
Line 24 violates unicorn/prevent-abbreviations; use index (or similar) to clear lint.
Suggested fix
- return runners.map((r, i) => ({
- id: i + 1,
+ return runners.map((r, index) => ({
+ id: index + 1,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return runners.map((r, i) => ({ | |
| id: i + 1, | |
| return runners.map((r, index) => ({ | |
| id: index + 1, |
🧰 Tools
🪛 ESLint
[error] 24-24: The variable i should be named index. A more descriptive name will do too.
(unicorn/prevent-abbreviations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`
around lines 24 - 25, The test helper uses an abbreviated loop index `i` inside
the runners.map callback; rename `i` to a descriptive identifier like `index`
(or `runnerIndex`) to satisfy unicorn/prevent-abbreviations. Update the map
callback signature in the test (the runners.map call in
runner-availability-service.test.ts) and replace all uses of `i` within that
callback (e.g., building the returned object id: i + 1) to the new name so the
behavior remains unchanged.
src/model/orchestrator/services/core/runner-availability-service.test.ts
Outdated
Show resolved
Hide resolved
|
|
frostebite
added
enhancement
Cap pagination at 100 pages (10,000 runners max), detect GitHub API rate limiting (403/429) with reset time reporting, add 30-second total timeout for pagination loop. Log clear diagnostic when no runners found suggesting possible causes (token permissions, runner registration). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/model/orchestrator/services/core/runner-availability-service.ts (1)
141-145:⚠️ Potential issue | 🟡 MinorQuote
per_pagerequest key to satisfy camelcase lint.Line 144 uses
per_pageunquoted and still tripscamelcase.Suggested fix
response = await octokit.request('GET /repos/{owner}/{repo}/actions/runners', { owner, repo, - per_page: perPage, + 'per_page': perPage, page, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.ts` around lines 141 - 145, The request object passed to octokit.request in runner-availability-service.ts uses an unquoted per_page key which trips the camelcase linter; change the request payload to use a quoted key (e.g., 'per_page') for the perPage value when calling octokit.request('GET /repos/{owner}/{repo}/actions/runners', { owner, repo, 'per_page': perPage, page, ... }) so the camelcase rule is satisfied while still sending the correct per-page parameter; locate the call site by the octokit.request invocation in the runner-availability-service function where response is assigned.src/model/orchestrator/services/core/runner-availability-service.test.ts (3)
19-19:⚠️ Potential issue | 🟠 MajorMove
Octokitimport into the top import block.Line 19 still imports after executable statements, which violates
import/firstand can fail lint CI.Suggested fix
import { RunnerAvailabilityService } from './runner-availability-service'; +import { Octokit } from '@octokit/core'; @@ -import { Octokit } from '@octokit/core'; - const MockedOctokit = Octokit as jest.MockedClass<typeof Octokit>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.test.ts` at line 19, The import for Octokit is placed after executable statements which violates the import/first rule; move the line "import { Octokit } from '@octokit/core';" into the top import block alongside the other imports in runner-availability-service.test.ts so all imports are grouped before any code or executable statements to satisfy linting.
47-47:⚠️ Potential issue | 🟠 MajorResolve remaining Prettier/ESLint formatting blockers in this test file.
There are still cast-parenthesization and spacing issues (plus comments spacing) that will fail lint. Please run formatter/lint and commit the normalized output.
Example cast fix pattern
-MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any); +MockedOctokit.mockImplementation(() => ({ request: mockRequest } as any));Also applies to: 61-61, 75-75, 88-88, 101-101, 122-122, 138-140, 142-142, 155-155, 170-170, 184-184, 211-211, 217-217, 248-248, 254-254, 274-274, 280-280, 292-292, 307-307, 313-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.test.ts` at line 47, Tests in runner-availability-service.test.ts contain Prettier/ESLint failures due to incorrect TypeScript cast parenthesization and spacing (e.g., the MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any) line) and inconsistent comment spacing; run the code formatter/linter and update the offending mock and cast expressions so the object is properly parenthesized before the "as" cast and spacing conforms to project rules, and normalize comment spacing; apply the same fix pattern to the other mockImplementation lines (references: MockedOctokit.mockImplementation, mockRequest) noted in the review and commit the formatted file.
24-25:⚠️ Potential issue | 🟡 MinorUse descriptive loop index names instead of
i.Lines 24/201/238/297 use abbreviated index names and still trigger
unicorn/prevent-abbreviations.Suggested fix
- return runners.map((r, i) => ({ - id: i + 1, + return runners.map((r, index) => ({ + id: index + 1, @@ - Array.from({ length: 100 }, (_, i) => ({ - name: `runner-${callCount}-${i}`, + Array.from({ length: 100 }, (_, index) => ({ + name: `runner-${callCount}-${index}`,Also applies to: 201-203, 238-240, 297-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.test.ts` around lines 24 - 25, Replace abbreviated loop variables in the map callbacks with descriptive names: change the callback signature from (r, i) => ... to (runner, index) => ... (and update uses of r to runner and i to index) for the runners.map occurrences (and the other map callbacks at the indicated spots). This removes the unicorn/prevent-abbreviations lint errors while keeping semantics identical—ensure all references inside those callbacks are renamed consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`:
- Around line 287-318: The test temporarily replaces Date.now then restores it
only on the happy path; wrap the Date.now override and test assertions in a
try/finally so Date.now is always restored even if expectations throw.
Concretely, move the originalDateNow capture and Date.now assignment before
calling RunnerAvailabilityService.checkAvailability and place the assertions
inside a try block, with Date.now = originalDateNow in the finally block; ensure
this change touches the test's Date.now handling around mockRequest and the call
to RunnerAvailabilityService.checkAvailability so the global time mock cannot
leak to other tests.
In `@src/model/orchestrator/services/core/runner-availability-service.ts`:
- Around line 197-204: The filterByLabels function currently uses requiredLabels
as-is which can contain empty strings and cause every() to fail; before
matching, normalize and remove empty entries by mapping requiredLabels to
trimmed strings and filtering out falsy/empty values (e.g., const
filteredRequired = requiredLabels.map(r => r.trim()).filter(Boolean)), then use
filteredRequired in the required check (inside filterByLabels and the
requiredLabels.every(...) call) so empty entries are ignored when comparing
against runner.labels (from runner.labels.map(l => l.name.toLowerCase())).
- Around line 149-161: The current handling in RunnerAvailabilityService treats
any 403 like a rate-limit; update the error branch where status is computed
(using requestError.status/response?.status) to distinguish real rate-limit 403s
from permission/auth 403s: for 429 always treat as rate-limited, but for 403
inspect requestError.response?.headers['x-ratelimit-remaining'] === '0' (or
headers['x-ratelimit-remaining'] === 0) and/or the error body/message for
rate-limit markers (e.g., contains "rate limit" or "secondary rate limit"); only
then run the existing rate-limit logic that reads x-ratelimit-reset, logs the
rate-limit warning (OrchestratorLogger.logWarning) and breaks the pagination
loop; otherwise re-throw the requestError (do not log as rate-limit or break) so
permission/auth errors surface. Ensure you reference requestError, status,
resetTime, allRunners and OrchestratorLogger.logWarning when making the change.
---
Duplicate comments:
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`:
- Line 19: The import for Octokit is placed after executable statements which
violates the import/first rule; move the line "import { Octokit } from
'@octokit/core';" into the top import block alongside the other imports in
runner-availability-service.test.ts so all imports are grouped before any code
or executable statements to satisfy linting.
- Line 47: Tests in runner-availability-service.test.ts contain Prettier/ESLint
failures due to incorrect TypeScript cast parenthesization and spacing (e.g.,
the MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any)
line) and inconsistent comment spacing; run the code formatter/linter and update
the offending mock and cast expressions so the object is properly parenthesized
before the "as" cast and spacing conforms to project rules, and normalize
comment spacing; apply the same fix pattern to the other mockImplementation
lines (references: MockedOctokit.mockImplementation, mockRequest) noted in the
review and commit the formatted file.
- Around line 24-25: Replace abbreviated loop variables in the map callbacks
with descriptive names: change the callback signature from (r, i) => ... to
(runner, index) => ... (and update uses of r to runner and i to index) for the
runners.map occurrences (and the other map callbacks at the indicated spots).
This removes the unicorn/prevent-abbreviations lint errors while keeping
semantics identical—ensure all references inside those callbacks are renamed
consistently.
In `@src/model/orchestrator/services/core/runner-availability-service.ts`:
- Around line 141-145: The request object passed to octokit.request in
runner-availability-service.ts uses an unquoted per_page key which trips the
camelcase linter; change the request payload to use a quoted key (e.g.,
'per_page') for the perPage value when calling octokit.request('GET
/repos/{owner}/{repo}/actions/runners', { owner, repo, 'per_page': perPage,
page, ... }) so the camelcase rule is satisfied while still sending the correct
per-page parameter; locate the call site by the octokit.request invocation in
the runner-availability-service function where response is assigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a5a33cd-a3f8-4ba7-a1e4-b3fcbe350dfd
📒 Files selected for processing (2)
src/model/orchestrator/services/core/runner-availability-service.test.tssrc/model/orchestrator/services/core/runner-availability-service.ts
Comment on lines
+287
to
+318
| const originalDateNow = Date.now; | ||
| let callCount = 0; | ||
|
|
||
| const mockRequest = jest.fn().mockImplementation(() => { | ||
| callCount++; | ||
| // After first call, advance time past the timeout | ||
| if (callCount >= 2) { | ||
| Date.now = jest.fn(() => originalDateNow() + 31_000); | ||
| } | ||
| const runners = createMockRunners( | ||
| Array.from({ length: 100 }, (_, i) => ({ | ||
| name: `runner-${callCount}-${i}`, | ||
| status: 'online' as const, | ||
| busy: false, | ||
| labels: ['self-hosted'], | ||
| })), | ||
| ); | ||
|
|
||
| return Promise.resolve({ status: 200, data: { runners } }); | ||
| }); | ||
| MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any); | ||
|
|
||
| const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1); | ||
|
|
||
| // Should have stopped after timeout was detected (2 pages: first succeeds, second triggers timeout check) | ||
| expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3); | ||
| // Should have runners from pages fetched before timeout | ||
| expect(result.totalRunners).toBeGreaterThan(0); | ||
|
|
||
| // Restore | ||
| Date.now = originalDateNow; | ||
| }); |
There was a problem hiding this comment.
Restore Date.now with try/finally to avoid test pollution.
Line 317 restores Date.now, but only on the happy path. If an assertion throws, global time mocking can leak into subsequent tests.
Suggested fix
- const originalDateNow = Date.now;
- let callCount = 0;
+ const originalDateNow = Date.now;
+ let callCount = 0;
- const mockRequest = jest.fn().mockImplementation(() => {
- callCount++;
- // After first call, advance time past the timeout
- if (callCount >= 2) {
- Date.now = jest.fn(() => originalDateNow() + 31_000);
- }
- const runners = createMockRunners(
- Array.from({ length: 100 }, (_, i) => ({
- name: `runner-${callCount}-${i}`,
- status: 'online' as const,
- busy: false,
- labels: ['self-hosted'],
- })),
- );
-
- return Promise.resolve({ status: 200, data: { runners } });
- });
- MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any);
-
- const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1);
-
- // Should have stopped after timeout was detected (2 pages: first succeeds, second triggers timeout check)
- expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3);
- // Should have runners from pages fetched before timeout
- expect(result.totalRunners).toBeGreaterThan(0);
-
- // Restore
- Date.now = originalDateNow;
+ try {
+ const mockRequest = jest.fn().mockImplementation(() => {
+ callCount++;
+ // After first call, advance time past the timeout
+ if (callCount >= 2) {
+ Date.now = jest.fn(() => originalDateNow() + 31_000);
+ }
+ const runners = createMockRunners(
+ Array.from({ length: 100 }, (_, index) => ({
+ name: `runner-${callCount}-${index}`,
+ status: 'online' as const,
+ busy: false,
+ labels: ['self-hosted'],
+ })),
+ );
+
+ return Promise.resolve({ status: 200, data: { runners } });
+ });
+ MockedOctokit.mockImplementation(() => ({ request: mockRequest } as any));
+
+ const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1);
+
+ // Should have stopped after timeout was detected (2 pages: first succeeds, second triggers timeout check)
+ expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3);
+ // Should have runners from pages fetched before timeout
+ expect(result.totalRunners).toBeGreaterThan(0);
+ } finally {
+ Date.now = originalDateNow;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const originalDateNow = Date.now; | |
| let callCount = 0; | |
| const mockRequest = jest.fn().mockImplementation(() => { | |
| callCount++; | |
| // After first call, advance time past the timeout | |
| if (callCount >= 2) { | |
| Date.now = jest.fn(() => originalDateNow() + 31_000); | |
| } | |
| const runners = createMockRunners( | |
| Array.from({ length: 100 }, (_, i) => ({ | |
| name: `runner-${callCount}-${i}`, | |
| status: 'online' as const, | |
| busy: false, | |
| labels: ['self-hosted'], | |
| })), | |
| ); | |
| return Promise.resolve({ status: 200, data: { runners } }); | |
| }); | |
| MockedOctokit.mockImplementation(() => ({ request: mockRequest }) as any); | |
| const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1); | |
| // Should have stopped after timeout was detected (2 pages: first succeeds, second triggers timeout check) | |
| expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3); | |
| // Should have runners from pages fetched before timeout | |
| expect(result.totalRunners).toBeGreaterThan(0); | |
| // Restore | |
| Date.now = originalDateNow; | |
| }); | |
| const originalDateNow = Date.now; | |
| let callCount = 0; | |
| try { | |
| const mockRequest = jest.fn().mockImplementation(() => { | |
| callCount++; | |
| // After first call, advance time past the timeout | |
| if (callCount >= 2) { | |
| Date.now = jest.fn(() => originalDateNow() + 31_000); | |
| } | |
| const runners = createMockRunners( | |
| Array.from({ length: 100 }, (_, index) => ({ | |
| name: `runner-${callCount}-${index}`, | |
| status: 'online' as const, | |
| busy: false, | |
| labels: ['self-hosted'], | |
| })), | |
| ); | |
| return Promise.resolve({ status: 200, data: { runners } }); | |
| }); | |
| MockedOctokit.mockImplementation(() => ({ request: mockRequest } as any)); | |
| const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1); | |
| // Should have stopped after timeout was detected (2 pages: first succeeds, second triggers timeout check) | |
| expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3); | |
| // Should have runners from pages fetched before timeout | |
| expect(result.totalRunners).toBeGreaterThan(0); | |
| } finally { | |
| Date.now = originalDateNow; | |
| } |
🧰 Tools
🪛 ESLint
[error] 292-292: Expected line before comment.
(lines-around-comment)
[error] 297-297: The variable i should be named index. A more descriptive name will do too.
(unicorn/prevent-abbreviations)
[error] 307-307: Replace )·as·any with ·as·any)
(prettier/prettier)
[error] 313-313: Expected line before comment.
(lines-around-comment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`
around lines 287 - 318, The test temporarily replaces Date.now then restores it
only on the happy path; wrap the Date.now override and test assertions in a
try/finally so Date.now is always restored even if expectations throw.
Concretely, move the originalDateNow capture and Date.now assignment before
calling RunnerAvailabilityService.checkAvailability and place the assertions
inside a try block, with Date.now = originalDateNow in the finally block; ensure
this change touches the test's Date.now handling around mockRequest and the call
to RunnerAvailabilityService.checkAvailability so the global time mock cannot
leak to other tests.
Comment on lines
+149
to
+161
| const status = requestError.status ?? requestError.response?.status; | ||
| if (status === 403 || status === 429) { | ||
| const resetTime = | ||
| requestError.response?.headers?.['x-ratelimit-reset'] ?? | ||
| requestError.headers?.['x-ratelimit-reset']; | ||
| const resetMessage = resetTime | ||
| ? ` Resets at ${new Date(Number.parseInt(String(resetTime), 10) * 1000).toISOString()}` | ||
| : ''; | ||
| OrchestratorLogger.logWarning( | ||
| `[RunnerAvailability] GitHub API rate limit reached (HTTP ${status}).${resetMessage} ` + | ||
| `Using ${allRunners.length} runners found so far.`, | ||
| ); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub API HTTP 403 rate limit vs permission error how to distinguish x-ratelimit headers
💡 Result:
To tell “403 because of rate limiting” from “403 because you’re not allowed” in the GitHub REST API, use a combination of response body message + rate-limit headers:
1) Primary rate limit (classic “rate limit exceeded”)
Treat it as primary rate limiting if:
- Status is 403 or 429, and
X-RateLimit-Remaining: 0(this is GitHub’s explicit signal for primary limit exhaustion)
Then wait until the Unix epoch time inX-RateLimit-Resetbefore retrying. [1]
2) Secondary rate limit (burst / abuse-style throttling)
Treat it as secondary rate limiting if:
- Status is 403 or 429, and
- The error message indicates a secondary rate limit (GitHub says the message will indicate this), and/or you receive a
Retry-Afterheader. [1]
Retry rules per GitHub:
- If
Retry-Afteris present: wait that many seconds. - Else if
X-RateLimit-Remaining: 0: wait untilX-RateLimit-Reset. - Else: wait at least 1 minute (then back off exponentially if it continues). [1]
3) Permission / authorization problems (still often HTTP 403)
Treat it as a permission error if:
- Status is 403, and the body says things like “Resource not accessible by integration” / “…by personal access token” (insufficient permissions). [2]
- In these cases, GitHub may still include
X-RateLimit-*headers even though it’s not a rate-limit failure, so do not interpret “403 + presence of X-RateLimit headers” as rate limiting. [3]
For GitHub Apps or fine-grained PATs, GitHub recommends using:
X-Accepted-GitHub-Permissionsto see which permissions are required for the endpoint (e.g.,contents=read,pull_requests=write,contents=read, etc.). [2]
Practical decision checklist
- If
X-RateLimit-Remaining == 0⇒ primary rate limit exceeded (wait forX-RateLimit-Reset). [1] - Else if body mentions secondary rate limit or
Retry-Afterexists ⇒ secondary rate limit (honorRetry-After/ backoff). [1] - Else if body says resource not accessible… / and/or
X-Accepted-GitHub-Permissionsis present ⇒ permissions (fix token/app permissions), not rate limiting. [2]
Sources:
[1] GitHub Docs — “Rate limits for the REST API”
[2] GitHub Docs — “Troubleshooting the REST API” (Resource not accessible + X-Accepted-GitHub-Permissions)
[3] GitHub Community discussion showing 403 permission errors still returning X-RateLimit-* headers
🏁 Script executed:
# First, let's examine the actual code context around lines 149-161
cat -n src/model/orchestrator/services/core/runner-availability-service.ts | sed -n '140,170p'Repository: game-ci/unity-builder
Length of output: 1541
🏁 Script executed:
# Let's also search for how requestError is created/populated in this file
rg -A 5 -B 5 "catch.*requestError" src/model/orchestrator/services/core/runner-availability-service.tsRepository: game-ci/unity-builder
Length of output: 498
Differentiate rate-limit 403 from permission/auth 403 responses.
Line 150 treats every HTTP 403 and 429 as rate limiting. However, GitHub REST API returns HTTP 403 for both rate-limit exhaustion and permission/auth errors (e.g., missing scopes or token restrictions). These cases must be distinguished: permission errors should not break pagination and re-throw; rate-limits should. Currently, any 403 logs an incorrect "rate limit reached" message and breaks the loop, which masks authentication failures and produces misleading warnings.
Per GitHub documentation, rate-limit 403 is indicated by the X-RateLimit-Remaining: 0 header or secondary-limit markers in the error message. Permission errors ("Resource not accessible by integration") are distinct and should not trigger rate-limit handling.
Suggested fix
- const status = requestError.status ?? requestError.response?.status;
- if (status === 403 || status === 429) {
+ const status = requestError.status ?? requestError.response?.status;
+ const remaining =
+ requestError.response?.headers?.['x-ratelimit-remaining'] ??
+ requestError.headers?.['x-ratelimit-remaining'];
+ const message = String(requestError.message ?? '').toLowerCase();
+ const isRateLimited =
+ status === 429 || (status === 403 && (String(remaining) === '0' || message.includes('rate limit')));
+
+ if (isRateLimited) {
const resetTime =
requestError.response?.headers?.['x-ratelimit-reset'] ??
requestError.headers?.['x-ratelimit-reset'];
@@
break;
}
// Re-throw non-rate-limit errors to be handled by the outer catch
throw requestError;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const status = requestError.status ?? requestError.response?.status; | |
| if (status === 403 || status === 429) { | |
| const resetTime = | |
| requestError.response?.headers?.['x-ratelimit-reset'] ?? | |
| requestError.headers?.['x-ratelimit-reset']; | |
| const resetMessage = resetTime | |
| ? ` Resets at ${new Date(Number.parseInt(String(resetTime), 10) * 1000).toISOString()}` | |
| : ''; | |
| OrchestratorLogger.logWarning( | |
| `[RunnerAvailability] GitHub API rate limit reached (HTTP ${status}).${resetMessage} ` + | |
| `Using ${allRunners.length} runners found so far.`, | |
| ); | |
| break; | |
| const status = requestError.status ?? requestError.response?.status; | |
| const remaining = | |
| requestError.response?.headers?.['x-ratelimit-remaining'] ?? | |
| requestError.headers?.['x-ratelimit-remaining']; | |
| const message = String(requestError.message ?? '').toLowerCase(); | |
| const isRateLimited = | |
| status === 429 || (status === 403 && (String(remaining) === '0' || message.includes('rate limit'))); | |
| if (isRateLimited) { | |
| const resetTime = | |
| requestError.response?.headers?.['x-ratelimit-reset'] ?? | |
| requestError.headers?.['x-ratelimit-reset']; | |
| const resetMessage = resetTime | |
| ? ` Resets at ${new Date(Number.parseInt(String(resetTime), 10) * 1000).toISOString()}` | |
| : ''; | |
| OrchestratorLogger.logWarning( | |
| `[RunnerAvailability] GitHub API rate limit reached (HTTP ${status}).${resetMessage} ` + | |
| `Using ${allRunners.length} runners found so far.`, | |
| ); | |
| break; | |
| } | |
| // Re-throw non-rate-limit errors to be handled by the outer catch | |
| throw requestError; |
🧰 Tools
🪛 ESLint
[error] 152-153: Delete ⏎···········
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/runner-availability-service.ts` around
lines 149 - 161, The current handling in RunnerAvailabilityService treats any
403 like a rate-limit; update the error branch where status is computed (using
requestError.status/response?.status) to distinguish real rate-limit 403s from
permission/auth 403s: for 429 always treat as rate-limited, but for 403 inspect
requestError.response?.headers['x-ratelimit-remaining'] === '0' (or
headers['x-ratelimit-remaining'] === 0) and/or the error body/message for
rate-limit markers (e.g., contains "rate limit" or "secondary rate limit"); only
then run the existing rate-limit logic that reads x-ratelimit-reset, logs the
rate-limit warning (OrchestratorLogger.logWarning) and breaks the pagination
loop; otherwise re-throw the requestError (do not log as rate-limit or break) so
permission/auth errors surface. Ensure you reference requestError, status,
resetTime, allRunners and OrchestratorLogger.logWarning when making the change.
Comment on lines
+197
to
+204
| private static filterByLabels(runners: GitHubRunner[], requiredLabels: string[]): GitHubRunner[] { | ||
| if (requiredLabels.length === 0) return runners; | ||
|
|
||
| return runners.filter((runner) => { | ||
| const runnerLabelNames = runner.labels.map((l) => l.name.toLowerCase()); | ||
|
|
||
| return requiredLabels.every((required) => runnerLabelNames.includes(required.toLowerCase())); | ||
| }); |
There was a problem hiding this comment.
Ignore empty required labels before matching.
If requiredLabels contains empty entries (e.g., trailing comma in input), every() can force false negatives and cause unnecessary fallback decisions.
Suggested fix
private static filterByLabels(runners: GitHubRunner[], requiredLabels: string[]): GitHubRunner[] {
- if (requiredLabels.length === 0) return runners;
+ const normalizedRequired = requiredLabels
+ .map((label) => label.trim().toLowerCase())
+ .filter((label) => label.length > 0);
+
+ if (normalizedRequired.length === 0) return runners;
return runners.filter((runner) => {
const runnerLabelNames = runner.labels.map((l) => l.name.toLowerCase());
- return requiredLabels.every((required) => runnerLabelNames.includes(required.toLowerCase()));
+ return normalizedRequired.every((required) => runnerLabelNames.includes(required));
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static filterByLabels(runners: GitHubRunner[], requiredLabels: string[]): GitHubRunner[] { | |
| if (requiredLabels.length === 0) return runners; | |
| return runners.filter((runner) => { | |
| const runnerLabelNames = runner.labels.map((l) => l.name.toLowerCase()); | |
| return requiredLabels.every((required) => runnerLabelNames.includes(required.toLowerCase())); | |
| }); | |
| private static filterByLabels(runners: GitHubRunner[], requiredLabels: string[]): GitHubRunner[] { | |
| const normalizedRequired = requiredLabels | |
| .map((label) => label.trim().toLowerCase()) | |
| .filter((label) => label.length > 0); | |
| if (normalizedRequired.length === 0) return runners; | |
| return runners.filter((runner) => { | |
| const runnerLabelNames = runner.labels.map((l) => l.name.toLowerCase()); | |
| return normalizedRequired.every((required) => runnerLabelNames.includes(required)); | |
| }); | |
| } |
🧰 Tools
🪛 ESLint
[error] 201-201: runnerLabelNames should be a Set, and use runnerLabelNames.has() to check existence or non-existence.
(unicorn/prefer-set-has)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/runner-availability-service.ts` around
lines 197 - 204, The filterByLabels function currently uses requiredLabels as-is
which can contain empty strings and cause every() to fail; before matching,
normalize and remove empty entries by mapping requiredLabels to trimmed strings
and filtering out falsy/empty values (e.g., const filteredRequired =
requiredLabels.map(r => r.trim()).filter(Boolean)), then use filteredRequired in
the required check (inside filterByLabels and the requiredLabels.every(...)
call) so empty entries are ignored when comparing against runner.labels (from
runner.labels.map(l => l.name.toLowerCase())).
7 tasks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/model/orchestrator/services/core/runner-availability-service.ts (3)
196-203:⚠️ Potential issue | 🟡 MinorNormalize and drop empty required labels before matching.
If
requiredLabelsincludes empty strings (e.g., trailing comma input), Line 202 can produce false negatives and trigger unnecessary fallback.Suggested fix
private static filterByLabels(runners: GitHubRunner[], requiredLabels: string[]): GitHubRunner[] { - if (requiredLabels.length === 0) return runners; + const normalizedRequired = requiredLabels + .map((label) => label.trim().toLowerCase()) + .filter((label) => label.length > 0); + + if (normalizedRequired.length === 0) return runners; return runners.filter((runner) => { const runnerLabelNames = runner.labels.map((l) => l.name.toLowerCase()); - return requiredLabels.every((required) => runnerLabelNames.includes(required.toLowerCase())); + return normalizedRequired.every((required) => runnerLabelNames.includes(required)); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.ts` around lines 196 - 203, In filterByLabels, normalize requiredLabels by trimming and removing empty strings before matching to avoid false negatives from inputs like trailing commas; inside the private static filterByLabels(runners, requiredLabels) function, map requiredLabels to lowercased, trimmed values and filter out '' entries (and if the resulting list is empty return runners immediately), then compare against runner.labels (runnerLabelNames) as currently implemented using every/includes to perform the match.
141-146:⚠️ Potential issue | 🟡 MinorQuote
per_pageto satisfy lint while preserving GitHub API param name.Line 144 violates camelcase lint; use a quoted key for the GitHub REST parameter.
Suggested fix
- response = await octokit.request('GET /repos/{owner}/{repo}/actions/runners', { + response = await octokit.request('GET /repos/{owner}/{repo}/actions/runners', { owner, repo, - per_page: perPage, + 'per_page': perPage, page, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.ts` around lines 141 - 146, The octokit.request call uses an unquoted camelCase-violating key per_page; change the request params to use the quoted GitHub API key by replacing per_page: perPage with 'per_page': perPage in the octokit.request invocation (the call that assigns response) so lint passes but the API param name is preserved.
149-161:⚠️ Potential issue | 🟠 MajorDifferentiate rate-limit 403 from permission/auth 403.
Line 150 currently buckets every 403 into rate-limit handling. GitHub also uses 403 for authorization failures, so this can hide real token/scope issues.
Suggested fix
- const status = requestError.status ?? requestError.response?.status; - if (status === 403 || status === 429) { + const status = requestError.status ?? requestError.response?.status; + const remaining = + requestError.response?.headers?.['x-ratelimit-remaining'] ?? + requestError.headers?.['x-ratelimit-remaining']; + const message = String(requestError.message ?? '').toLowerCase(); + const isRateLimited = + status === 429 || (status === 403 && (String(remaining) === '0' || message.includes('rate limit'))); + + if (isRateLimited) { const resetTime = requestError.response?.headers?.['x-ratelimit-reset'] ?? requestError.headers?.['x-ratelimit-reset']; @@ break; } // Re-throw non-rate-limit errors to be handled by the outer catch throw requestError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.ts` around lines 149 - 161, The code currently treats every HTTP 403 as a rate-limit; update the 403 handling in the block around requestError/status to detect true rate-limit 403s (e.g., check response headers like requestError.response?.headers['x-ratelimit-remaining'] === '0' or presence of 'x-ratelimit-reset', or inspect response body/message for "rate limit") and only run the rate-limit path (compute resetTime, log via OrchestratorLogger.logWarning and break) when those indicators are present; otherwise, handle it as an authorization/permission error (log a distinct error via OrchestratorLogger with details from requestError.response?.data or requestError.message and avoid treating it as a transient rate-limit). Ensure you reference requestError, status, resetTime, OrchestratorLogger.logWarning, and allRunners when making the change.src/model/orchestrator/services/core/runner-availability-service.test.ts (2)
283-316:⚠️ Potential issue | 🟡 MinorAlways restore
Date.nowviatry/finallyin the timeout test.Line 315 restores on the happy path only. If any assertion throws first, global time mocking leaks into later tests.
Suggested fix
- const originalDateNow = Date.now; - let callCount = 0; - - const mockRequest = jest.fn().mockImplementation(() => { - callCount++; - // After first call, advance time past the timeout - if (callCount >= 2) { - Date.now = jest.fn(() => originalDateNow() + 31_000); - } - const runners = createMockRunners( - Array.from({ length: 100 }, (_, i) => ({ - name: `runner-${callCount}-${i}`, - status: 'online' as const, - busy: false, - labels: ['self-hosted'], - })), - ); - - return Promise.resolve({ status: 200, data: { runners } }); - }); - MockedOctokit.mockImplementation(() => ({ request: mockRequest } as any)); - - const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1); - - // Should have stopped after timeout was detected (2 pages: first succeeds, second triggers timeout check) - expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3); - // Should have runners from pages fetched before timeout - expect(result.totalRunners).toBeGreaterThan(0); - - // Restore - Date.now = originalDateNow; + const originalDateNow = Date.now; + let callCount = 0; + try { + const mockRequest = jest.fn().mockImplementation(() => { + callCount++; + if (callCount >= 2) { + Date.now = jest.fn(() => originalDateNow() + 31_000); + } + const runners = createMockRunners( + Array.from({ length: 100 }, (_, index) => ({ + name: `runner-${callCount}-${index}`, + status: 'online' as const, + busy: false, + labels: ['self-hosted'], + })), + ); + return Promise.resolve({ status: 200, data: { runners } }); + }); + MockedOctokit.mockImplementation(() => ({ request: mockRequest } as any)); + + const result = await RunnerAvailabilityService.checkAvailability('owner', 'repo', 'token', [], 1); + expect(mockRequest.mock.calls.length).toBeLessThanOrEqual(3); + expect(result.totalRunners).toBeGreaterThan(0); + } finally { + Date.now = originalDateNow; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.test.ts` around lines 283 - 316, The test "should handle pagination timeout gracefully" leaks the mocked Date.now if an assertion throws; wrap the portion that replaces Date.now and uses mockRequest in a try/finally so Date.now is always restored: store originalDateNow, set Date.now to the mock inside the try, run the call to RunnerAvailabilityService.checkAvailability and the assertions, and then restore Date.now = originalDateNow in the finally block (references: the test name, originalDateNow, mockRequest, and RunnerAvailabilityService.checkAvailability).
1-21:⚠️ Potential issue | 🟠 MajorMove
Octokitimport to the top-level import block.Line 19 currently imports inside module body after executable statements, which violates
import/firstand can fail CI linting.Suggested fix
import { RunnerAvailabilityService } from './runner-availability-service'; +import { Octokit } from '@octokit/core'; // Mock `@octokit/core` jest.mock('@octokit/core', () => ({ @@ -import { Octokit } from '@octokit/core'; - const MockedOctokit = Octokit as jest.MockedClass<typeof Octokit>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/runner-availability-service.test.ts` around lines 1 - 21, The import of Octokit must be moved above any executable statements and mock setup to satisfy import/first; relocate "import { Octokit } from '@octokit/core';" into the top-level import block (with RunnerAvailabilityService import) before the jest.mock calls, leaving the type alias "const MockedOctokit = Octokit as jest.MockedClass<typeof Octokit>;" where it is so tests can use the mocked class; ensure no other executable code appears before imports.src/model/orchestrator/orchestrator.ts (1)
345-367:⚠️ Potential issue | 🔴 Critical
Promise.racetimeout does not cancel provider setup and can cause double side effects.At Line 367, timing out only rejects the race; the original
setupWorkflowkeeps running. If fallback retry starts, both setups may mutate external resources concurrently.Use explicit cancellation (e.g.,
AbortController) and make provider setup observeAbortSignalbefore external mutations.#!/bin/bash # Verify all Provider.setupWorkflow signatures/implementations to assess cancellable migration scope. # Read-only reconnaissance. rg -nP --type=ts '\bsetupWorkflow\s*\(' -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/orchestrator.ts` around lines 345 - 367, The Promise.race approach leaves Orchestrator.Provider.setupWorkflow running after timeout causing duplicate side effects; change this by creating an AbortController, pass its signal into Orchestrator.Provider.setupWorkflow (replace calls creating setupPromise so the call accepts an AbortSignal), and on timeout call controller.abort() before rejecting the timeoutPromise; update setupWorkflow implementations to check the AbortSignal and stop/rollback before performing external mutations (or throw on signal.aborted). Ensure any call sites (where setupPromise is created) propagate the new signal parameter and that the timeout path aborts the controller so only one active setup can mutate resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/model/orchestrator/orchestrator.ts`:
- Around line 345-367: The Promise.race approach leaves
Orchestrator.Provider.setupWorkflow running after timeout causing duplicate side
effects; change this by creating an AbortController, pass its signal into
Orchestrator.Provider.setupWorkflow (replace calls creating setupPromise so the
call accepts an AbortSignal), and on timeout call controller.abort() before
rejecting the timeoutPromise; update setupWorkflow implementations to check the
AbortSignal and stop/rollback before performing external mutations (or throw on
signal.aborted). Ensure any call sites (where setupPromise is created) propagate
the new signal parameter and that the timeout path aborts the controller so only
one active setup can mutate resources.
In `@src/model/orchestrator/services/core/runner-availability-service.test.ts`:
- Around line 283-316: The test "should handle pagination timeout gracefully"
leaks the mocked Date.now if an assertion throws; wrap the portion that replaces
Date.now and uses mockRequest in a try/finally so Date.now is always restored:
store originalDateNow, set Date.now to the mock inside the try, run the call to
RunnerAvailabilityService.checkAvailability and the assertions, and then restore
Date.now = originalDateNow in the finally block (references: the test name,
originalDateNow, mockRequest, and RunnerAvailabilityService.checkAvailability).
- Around line 1-21: The import of Octokit must be moved above any executable
statements and mock setup to satisfy import/first; relocate "import { Octokit }
from '@octokit/core';" into the top-level import block (with
RunnerAvailabilityService import) before the jest.mock calls, leaving the type
alias "const MockedOctokit = Octokit as jest.MockedClass<typeof Octokit>;" where
it is so tests can use the mocked class; ensure no other executable code appears
before imports.
In `@src/model/orchestrator/services/core/runner-availability-service.ts`:
- Around line 196-203: In filterByLabels, normalize requiredLabels by trimming
and removing empty strings before matching to avoid false negatives from inputs
like trailing commas; inside the private static filterByLabels(runners,
requiredLabels) function, map requiredLabels to lowercased, trimmed values and
filter out '' entries (and if the resulting list is empty return runners
immediately), then compare against runner.labels (runnerLabelNames) as currently
implemented using every/includes to perform the match.
- Around line 141-146: The octokit.request call uses an unquoted
camelCase-violating key per_page; change the request params to use the quoted
GitHub API key by replacing per_page: perPage with 'per_page': perPage in the
octokit.request invocation (the call that assigns response) so lint passes but
the API param name is preserved.
- Around line 149-161: The code currently treats every HTTP 403 as a rate-limit;
update the 403 handling in the block around requestError/status to detect true
rate-limit 403s (e.g., check response headers like
requestError.response?.headers['x-ratelimit-remaining'] === '0' or presence of
'x-ratelimit-reset', or inspect response body/message for "rate limit") and only
run the rate-limit path (compute resetTime, log via
OrchestratorLogger.logWarning and break) when those indicators are present;
otherwise, handle it as an authorization/permission error (log a distinct error
via OrchestratorLogger with details from requestError.response?.data or
requestError.message and avoid treating it as a transient rate-limit). Ensure
you reference requestError, status, resetTime, OrchestratorLogger.logWarning,
and allRunners when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4963a2bf-96a3-4fe8-be40-c1ce6de16418
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (3)
src/model/orchestrator/orchestrator.tssrc/model/orchestrator/services/core/runner-availability-service.test.tssrc/model/orchestrator/services/core/runner-availability-service.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #783 +/- ##
==========================================
+ Coverage 31.25% 32.03% +0.78%
==========================================
Files 84 85 +1
Lines 4563 4673 +110
Branches 1103 1131 +28
==========================================
+ Hits 1426 1497 +71
- Misses 3137 3176 +39
🚀 New features to boost your workflow:
|
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds built-in load balancing to the orchestrator so builds can be intelligently routed across providers without custom workflow scripting.
When a self-hosted runner is busy or offline, the action automatically routes the build to a cloud provider. Combined with
asyncOrchestrator: true, this means builds never block — they dispatch to the best available provider and return immediately.What it does
Inputs
fallbackProviderStrategy''runnerCheckEnabledfalserunnerCheckLabels''self-hosted,linux)runnerCheckMinAvailable1retryOnFallbackfalseproviderInitTimeout0Outputs
providerFallbackUsedtrueif the build was routed to the alternate providerproviderFallbackReasonUsage — auto-route busy runners to cloud
Self-hosted runner idle? Build runs locally. Runner busy? Dispatches to AWS asynchronously. Either way, the workflow step returns in seconds.
Usage — retry failed cloud builds locally
If AWS fails (transient error, quota exceeded), the build automatically retries on the local Docker provider.
Usage — timeout slow provider startup
If Kubernetes takes more than 2 minutes to provision, switches to AWS.
How it works
GET /repos/{owner}/{repo}/actions/runners, filters by labels, counts idle runners. If below threshold → swaps provider.setupWorkflow()inPromise.racewith a timeout. If provider init exceeds the limit → throws, triggering retry.run()level. If the primary build fails and retry is enabled → re-runs on the alternate provider.Async mode synergy
The runner check + async mode combination is particularly useful: when runners are busy, the build dispatches to a cloud provider asynchronously, freeing the GitHub runner immediately. Without async, the build still routes correctly but occupies the runner for the full build duration.
Test plan
yarn buildproduces valid distDocumentation
🤖 Generated with Claude Code
Summary by CodeRabbit
Tracking: