Skip to content

fix(lambda): fail-open isJobQueued — assume queued on API errors #5130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vegardx wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(lambda): fail-open isJobQueued — assume queued on API errors #5130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94092a3
fix(lambda): fail-open isJobQueued — assume queued on API errors
vegardx May 26, 2026
0999e80
refactor: sanitize error object in isJobQueued warning log
vegardx May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,20 @@ describe('scaleUp with GHES', () => {
}),
);
});

it('Should assume job is queued when isJobQueued throws (fail-open)', async () => {
mockOctokit.actions.getJobForWorkflowRun.mockRejectedValue(new Error('GitHub API 502'));

const messages = createTestMessages(2);
await scaleUpModule.scaleUp(messages);

// All messages processed despite API error — fail-open prevents job drops
expect(createRunner).toHaveBeenCalledWith(
expect.objectContaining({
numberOfRunners: 2,
}),
);
});
});
});

Expand Down
19 changes: 15 additions & 4 deletions lambdas/functions/control-plane/src/scale-runners/scale-up.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,21 @@ export async function scaleUp(payloads: ActionRequestMessageSQS[]): Promise<stri
},
});

if (enableJobQueuedCheck && !(await isJobQueued(githubInstallationClient, message))) {
messageLogger.info('No runner will be created, job is not queued.');

continue;
if (enableJobQueuedCheck) {
let jobQueued = true;
try {
jobQueued = await isJobQueued(githubInstallationClient, message);
} catch (e) {
const err = e as Error & { status?: number };
messageLogger.warn('isJobQueued check failed, assuming job is still queued (fail-open)', {
error: err.message,
status: err.status,
});
}
if (!jobQueued) {
messageLogger.info('No runner will be created, job is not queued.');
continue;
}
}
Comment on lines +419 to 434

@vegardx vegardx May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Intentionally broad. The fail-open philosophy here is that dropping a job is always worse than creating an ephemeral runner that self-terminates in ~30s when no work is available.

Specific cases:

  • 404 (job not found): GitHub's API is eventually consistent — a 404 immediately after webhook delivery is a race condition, not a definitive "job doesn't exist". Failing closed here drops the job permanently.
  • Auth/permission errors: Transient in practice (token refresh, installation permission propagation delays).
  • Unsupported event type: isJobQueued already returns false for unsupported events (the throw path is only reached on actual API call failures, not event-type mismatches).

Narrowing to specific status codes adds a maintenance surface that breaks when GitHub changes error responses. The max downside of fail-open is one extra idle runner for 30s; the max downside of fail-closed is a permanently dropped job.


scaleUp++;
Expand Down