diff --git a/CHANGELOG.md b/CHANGELOG.md index f2a6ca5b..b61eb3f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ All notable changes to this action are documented here. This project follows [Semantic Versioning](https://semver.org/). The moving major tag (e.g. `v4`) always points at the latest release in that major line. +## [Unreleased] + +### Fixed + +- **Warm-restart registration race** (#67): on a `reuse: stop` warm restart the + runner could re-register and report `online` on the very first poll + (`elapsed_s=0`) and then drop before the dependent job was scheduled — the + start step exited green, the job sat `queued` forever, and the gated `stop` + job never fired, leaking a running instance that kept holding its EIP. The + start step now requires the registration to stay online across several + consecutive polls before declaring the runner ready (3 on a warm restart, 2 + on a cold launch); a registration that flaps within that window fails the + start step — routing into the existing console-output capture and cleanup — + instead of returning a false success. A flap is logged + (`wait_for_runner … outcome:flap`) so the failure mode is debuggable. Docs + now stress scheduling the `cleanup` reaper for warm pools, since it is the + backstop that reaps a leaked *running* instance whose runner never came up. + ## [4.0.0] - 2026-07-02 A capability wave across cost, reliability, reach, and toil. 10 features, each diff --git a/README.md b/README.md index 67926e16..69dd7c36 100644 --- a/README.md +++ b/README.md @@ -462,9 +462,12 @@ Set `reuse: stop` on **both** the start and stop steps: - **Start** looks for a *stopped* instance with this action's tags + matching `reuse-pool-tag` + same instance type/arch; if found it `StartInstances` and the runner re-registers (a boot-time hook reads a fresh registration token from the instance's IMDS user-data — the token never lives in a readable tag). If the pool is empty, it cold-launches an instance that **joins the pool**. (Warm reuse applies when `count` is 1; batches cold-launch.) - **Stop** stops the instance instead of terminating it, so the next job reuses it. - **Hygiene**: `reuse-max-cycles` (default 20) recycles an instance after N jobs; `max-lifetime-minutes` bounds wall-clock age; the [`cleanup` reaper](#reaping-orphaned-runners-mode-cleanup) drains stopped instances older than `reaper-stopped-max-age`. Warm caches (e.g. Docker layers) are a feature; unbounded state is not — these keep pools from accreting cost. +- **Registration stability**: a reused instance can re-register and report `online` on the very first poll and then drop before your dependent job is scheduled — leaving that job stuck in `queued`. On a warm restart the start step therefore waits for the registration to stay online across several consecutive polls before declaring the runner ready; a registration that flaps within that window fails the start step (with console output captured) instead of returning a false success. This adds a few seconds to a warm start. > ⚠️ **Security:** reuse means job N+1 runs on job N's disk. This is fine for a **single trusted repo's** CI but **unsafe for public repositories or untrusted pull requests** — see [the security section](#self-hosted-runner-security-with-public-repositories). +> 💡 **Always schedule the [`cleanup` reaper](#reaping-orphaned-runners-mode-cleanup) for warm pools.** If a start ever *does* return before its runner is durably online (or a runner dies mid-job), the dependent job never runs, so a `stop` step that `needs:` that job never fires — the reused instance is left **running** and keeps holding its EIP. The reaper terminates a running managed instance that has no registered runner past the grace period, which is the backstop that recovers this leak. + Pool sizing: match the pool tag to a concurrency tier. A pool naturally grows to your peak concurrency (cold launches join it) and drains via the reaper when idle. ## Matrix builds (multiple runners) diff --git a/dist/index.js b/dist/index.js index 50786e5b..54250cdb 100644 --- a/dist/index.js +++ b/dist/index.js @@ -106743,8 +106743,16 @@ const FAILED_PREFIX = 'failed:'; // `failed:` value means cloud-init aborted — fail fast within one // poll interval, naming the step, instead of waiting out the full // registration timeout. -// 2. GitHub runner registration. Success is authoritative here: the runner -// showing up as `online` is what lets downstream jobs run. +// 2. GitHub runner registration. Success requires the runner to report +// `online` on `confirmChecks` CONSECUTIVE polls, not just once — a +// single online observation is necessary but not sufficient. On a warm +// restart (reuse:stop) the runner can report online on the very first +// poll (elapsed_s=0) and then drop before the dependent job is even +// scheduled; the job then sits queued forever and the consumer's +// gated stop job never fires, leaking a running instance and its EIP +// (see issue #67). Confirming the registration stays online turns that +// flap into a start-step failure the caller can act on, instead of a +// false success. // // All I/O and timing are injected so the loop is unit-testable without real // AWS/GitHub calls or wall-clock sleeps: @@ -106752,6 +106760,10 @@ const FAILED_PREFIX = 'failed:'; // - deps.isRunnerOnline(): Promise // - deps.sleep(ms): Promise (defaults to setTimeout) // +// opts.confirmChecks (default 1) is how many consecutive online polls are +// required before success. 1 keeps the legacy accept-first-online behaviour; +// the start action passes a higher value on warm restarts (see src/index.js). +// // Throws on `failed:` (err.bootstrapStep set) or on timeout // (err.timedOut set) so the caller can capture diagnostics and clean up. async function waitForRunnerReady(deps, opts = {}) { @@ -106761,6 +106773,7 @@ async function waitForRunnerReady(deps, opts = {}) { const timeoutMinutes = opts.timeoutMinutes ?? 5; const intervalSeconds = opts.intervalSeconds ?? 10; const quietSeconds = opts.quietSeconds ?? 30; + const confirmChecks = Math.max(1, opts.confirmChecks ?? 1); core.info(`Waiting ${quietSeconds}s for the AWS EC2 instance to bootstrap and register as a self-hosted runner`); await sleep(quietSeconds * 1000); @@ -106768,6 +106781,10 @@ async function waitForRunnerReady(deps, opts = {}) { const deadlineSeconds = timeoutMinutes * 60; let waitedSeconds = 0; + // Count of consecutive polls that observed the runner online. Resets to 0 + // on any poll that does not (a flap), so only a sustained registration + // reaches confirmChecks. + let onlineStreak = 0; for (;;) { // 1. Fast-fail on a phoned-home bootstrap failure. @@ -106788,16 +106805,30 @@ async function waitForRunnerReady(deps, opts = {}) { throw error; } - // 2. Success when the runner has registered and is online. + // 2. Success requires confirmChecks CONSECUTIVE online observations, so a + // registration that flaps offline within seconds (the issue-#67 warm- + // restart race) never counts as ready. if (await isRunnerOnline()) { - log.info('wait_for_runner', { outcome: 'online', elapsed_s: waitedSeconds }); - core.info('GitHub self-hosted runner is registered and ready to use'); - return; + onlineStreak += 1; + if (onlineStreak >= confirmChecks) { + log.info('wait_for_runner', { outcome: 'online', elapsed_s: waitedSeconds, confirm_checks: confirmChecks }); + core.info('GitHub self-hosted runner is registered and ready to use'); + return; + } + log.debug('wait_for_runner_confirm', { elapsed_s: waitedSeconds, streak: onlineStreak, needed: confirmChecks }); + core.info(`GitHub self-hosted runner is online; confirming stability (${onlineStreak}/${confirmChecks})`); + } else if (onlineStreak > 0) { + // The runner was online on a prior poll and now isn't: the registration + // flapped. Reset the streak and surface it — on the warm-restart path + // this is the only instance-side-adjacent evidence in the workflow log. + log.warn('wait_for_runner', { outcome: 'flap', elapsed_s: waitedSeconds, confirmed: onlineStreak, needed: confirmChecks }); + core.warning('GitHub self-hosted runner went offline while confirming stability; continuing to wait for a stable registration'); + onlineStreak = 0; } // 3. Bounded wait — give up after the timeout. if (waitedSeconds >= deadlineSeconds) { - log.error('wait_for_runner', { outcome: 'timeout', timeout_minutes: timeoutMinutes }); + log.error('wait_for_runner', { outcome: 'timeout', timeout_minutes: timeoutMinutes, confirm_checks: confirmChecks, online_streak: onlineStreak }); const error = new Error( `A timeout of ${timeoutMinutes} minutes is exceeded. Your AWS EC2 instance was not able to register itself in GitHub as a new self-hosted runner.`, ); @@ -106805,7 +106836,7 @@ async function waitForRunnerReady(deps, opts = {}) { throw error; } - log.debug('wait_for_runner_poll', { elapsed_s: waitedSeconds, bootstrap: status || null }); + log.debug('wait_for_runner_poll', { elapsed_s: waitedSeconds, bootstrap: status || null, online_streak: onlineStreak }); core.info('Checking...'); await sleep(intervalSeconds * 1000); waitedSeconds += intervalSeconds; @@ -111695,6 +111726,16 @@ function setOutput(label, placement) { core.setOutput('market-type-used', marketType); } +// How many consecutive online polls the wait loop must observe before it +// declares the runner ready (see waitForRunnerReady's confirmChecks). Warm +// restarts get a longer confirmation window: a reused instance can re-register +// and report online on the very first poll and then drop before the dependent +// job is scheduled, which strands that job in `queued` forever (issue #67). +// Cold launches also confirm (one extra poll) to shrink the same flap window +// on a fresh registration. +const WARM_CONFIRM_CHECKS = 3; +const COLD_CONFIRM_CHECKS = 2; + // Warm-pool fast path: reuse a stopped pool instance (count 1 only). Returns // a placement, or null to fall back to a cold launch (empty pool or a failed // start — the cold launch then joins the pool). @@ -111746,13 +111787,19 @@ async function start() { // Watch the bootstrap phone-home tags and GitHub registration together. // The batch is ready only when ALL N runners are online; a bootstrap - // failure on ANY instance fails fast. On failure or timeout, capture the - // console output of every instance and (by default) terminate them all — - // no half-fleet is left leaking billing. The token is redacted. + // failure on ANY instance fails fast. Registration must hold online for + // confirmChecks consecutive polls so a warm restart's flap (issue #67) + // fails here rather than stranding the downstream job. On failure or + // timeout, capture the console output of every instance and (by default) + // terminate them all — no half-fleet is left leaking billing. The token + // is redacted. + const warmRestart = placement.marketType === 'reused'; try { await waitForRunnerReady({ getBootstrapStatus: () => aws.getBatchBootstrapStatus(instanceIds), isRunnerOnline: async () => (await gh.countOnlineRunners(label)) >= instanceIds.length, + }, { + confirmChecks: warmRestart ? WARM_CONFIRM_CHECKS : COLD_CONFIRM_CHECKS, }); } catch (waitError) { await aws.handleStartFailure(instanceIds, { redactValues: [githubRegistrationToken] }); diff --git a/src/index.js b/src/index.js index e8d65071..514b6db5 100644 --- a/src/index.js +++ b/src/index.js @@ -49,6 +49,16 @@ function setOutput(label, placement) { core.setOutput('market-type-used', marketType); } +// How many consecutive online polls the wait loop must observe before it +// declares the runner ready (see waitForRunnerReady's confirmChecks). Warm +// restarts get a longer confirmation window: a reused instance can re-register +// and report online on the very first poll and then drop before the dependent +// job is scheduled, which strands that job in `queued` forever (issue #67). +// Cold launches also confirm (one extra poll) to shrink the same flap window +// on a fresh registration. +const WARM_CONFIRM_CHECKS = 3; +const COLD_CONFIRM_CHECKS = 2; + // Warm-pool fast path: reuse a stopped pool instance (count 1 only). Returns // a placement, or null to fall back to a cold launch (empty pool or a failed // start — the cold launch then joins the pool). @@ -100,13 +110,19 @@ async function start() { // Watch the bootstrap phone-home tags and GitHub registration together. // The batch is ready only when ALL N runners are online; a bootstrap - // failure on ANY instance fails fast. On failure or timeout, capture the - // console output of every instance and (by default) terminate them all — - // no half-fleet is left leaking billing. The token is redacted. + // failure on ANY instance fails fast. Registration must hold online for + // confirmChecks consecutive polls so a warm restart's flap (issue #67) + // fails here rather than stranding the downstream job. On failure or + // timeout, capture the console output of every instance and (by default) + // terminate them all — no half-fleet is left leaking billing. The token + // is redacted. + const warmRestart = placement.marketType === 'reused'; try { await waitForRunnerReady({ getBootstrapStatus: () => aws.getBatchBootstrapStatus(instanceIds), isRunnerOnline: async () => (await gh.countOnlineRunners(label)) >= instanceIds.length, + }, { + confirmChecks: warmRestart ? WARM_CONFIRM_CHECKS : COLD_CONFIRM_CHECKS, }); } catch (waitError) { await aws.handleStartFailure(instanceIds, { redactValues: [githubRegistrationToken] }); diff --git a/src/wait.js b/src/wait.js index 4a4cbfe6..1cfe2e69 100644 --- a/src/wait.js +++ b/src/wait.js @@ -16,8 +16,16 @@ const FAILED_PREFIX = 'failed:'; // `failed:` value means cloud-init aborted — fail fast within one // poll interval, naming the step, instead of waiting out the full // registration timeout. -// 2. GitHub runner registration. Success is authoritative here: the runner -// showing up as `online` is what lets downstream jobs run. +// 2. GitHub runner registration. Success requires the runner to report +// `online` on `confirmChecks` CONSECUTIVE polls, not just once — a +// single online observation is necessary but not sufficient. On a warm +// restart (reuse:stop) the runner can report online on the very first +// poll (elapsed_s=0) and then drop before the dependent job is even +// scheduled; the job then sits queued forever and the consumer's +// gated stop job never fires, leaking a running instance and its EIP +// (see issue #67). Confirming the registration stays online turns that +// flap into a start-step failure the caller can act on, instead of a +// false success. // // All I/O and timing are injected so the loop is unit-testable without real // AWS/GitHub calls or wall-clock sleeps: @@ -25,6 +33,10 @@ const FAILED_PREFIX = 'failed:'; // - deps.isRunnerOnline(): Promise // - deps.sleep(ms): Promise (defaults to setTimeout) // +// opts.confirmChecks (default 1) is how many consecutive online polls are +// required before success. 1 keeps the legacy accept-first-online behaviour; +// the start action passes a higher value on warm restarts (see src/index.js). +// // Throws on `failed:` (err.bootstrapStep set) or on timeout // (err.timedOut set) so the caller can capture diagnostics and clean up. async function waitForRunnerReady(deps, opts = {}) { @@ -34,6 +46,7 @@ async function waitForRunnerReady(deps, opts = {}) { const timeoutMinutes = opts.timeoutMinutes ?? 5; const intervalSeconds = opts.intervalSeconds ?? 10; const quietSeconds = opts.quietSeconds ?? 30; + const confirmChecks = Math.max(1, opts.confirmChecks ?? 1); core.info(`Waiting ${quietSeconds}s for the AWS EC2 instance to bootstrap and register as a self-hosted runner`); await sleep(quietSeconds * 1000); @@ -41,6 +54,10 @@ async function waitForRunnerReady(deps, opts = {}) { const deadlineSeconds = timeoutMinutes * 60; let waitedSeconds = 0; + // Count of consecutive polls that observed the runner online. Resets to 0 + // on any poll that does not (a flap), so only a sustained registration + // reaches confirmChecks. + let onlineStreak = 0; for (;;) { // 1. Fast-fail on a phoned-home bootstrap failure. @@ -61,16 +78,30 @@ async function waitForRunnerReady(deps, opts = {}) { throw error; } - // 2. Success when the runner has registered and is online. + // 2. Success requires confirmChecks CONSECUTIVE online observations, so a + // registration that flaps offline within seconds (the issue-#67 warm- + // restart race) never counts as ready. if (await isRunnerOnline()) { - log.info('wait_for_runner', { outcome: 'online', elapsed_s: waitedSeconds }); - core.info('GitHub self-hosted runner is registered and ready to use'); - return; + onlineStreak += 1; + if (onlineStreak >= confirmChecks) { + log.info('wait_for_runner', { outcome: 'online', elapsed_s: waitedSeconds, confirm_checks: confirmChecks }); + core.info('GitHub self-hosted runner is registered and ready to use'); + return; + } + log.debug('wait_for_runner_confirm', { elapsed_s: waitedSeconds, streak: onlineStreak, needed: confirmChecks }); + core.info(`GitHub self-hosted runner is online; confirming stability (${onlineStreak}/${confirmChecks})`); + } else if (onlineStreak > 0) { + // The runner was online on a prior poll and now isn't: the registration + // flapped. Reset the streak and surface it — on the warm-restart path + // this is the only instance-side-adjacent evidence in the workflow log. + log.warn('wait_for_runner', { outcome: 'flap', elapsed_s: waitedSeconds, confirmed: onlineStreak, needed: confirmChecks }); + core.warning('GitHub self-hosted runner went offline while confirming stability; continuing to wait for a stable registration'); + onlineStreak = 0; } // 3. Bounded wait — give up after the timeout. if (waitedSeconds >= deadlineSeconds) { - log.error('wait_for_runner', { outcome: 'timeout', timeout_minutes: timeoutMinutes }); + log.error('wait_for_runner', { outcome: 'timeout', timeout_minutes: timeoutMinutes, confirm_checks: confirmChecks, online_streak: onlineStreak }); const error = new Error( `A timeout of ${timeoutMinutes} minutes is exceeded. Your AWS EC2 instance was not able to register itself in GitHub as a new self-hosted runner.`, ); @@ -78,7 +109,7 @@ async function waitForRunnerReady(deps, opts = {}) { throw error; } - log.debug('wait_for_runner_poll', { elapsed_s: waitedSeconds, bootstrap: status || null }); + log.debug('wait_for_runner_poll', { elapsed_s: waitedSeconds, bootstrap: status || null, online_streak: onlineStreak }); core.info('Checking...'); await sleep(intervalSeconds * 1000); waitedSeconds += intervalSeconds; diff --git a/tests/wait.test.js b/tests/wait.test.js index 3482a56a..04d297c2 100644 --- a/tests/wait.test.js +++ b/tests/wait.test.js @@ -11,6 +11,7 @@ const noSleep = () => Promise.resolve(); beforeEach(() => { core.error.mockClear(); + core.warning.mockClear(); }); describe('waitForRunnerReady', () => { @@ -151,4 +152,84 @@ describe('waitForRunnerReady', () => { expect(caught.message).toBe('EC2 runner bootstrap failed during the "downloading" step. See the captured console output below.'); }); }); + + // Stability confirmation (confirmChecks): a single online poll is necessary + // but not sufficient. On a warm restart the runner can report online on the + // first poll and then drop before the dependent job is scheduled (issue + // #67); requiring N CONSECUTIVE online polls turns that flap into a failure + // here instead of a downstream job queued forever. + describe('online-stability confirmation (confirmChecks)', () => { + test('requires confirmChecks consecutive online polls before resolving', async () => { + const getBootstrapStatus = jest.fn().mockResolvedValue(null); + const isRunnerOnline = jest.fn().mockResolvedValue(true); + + await waitForRunnerReady( + { getBootstrapStatus, isRunnerOnline, sleep: noSleep }, + { quietSeconds: 0, intervalSeconds: 1, confirmChecks: 3 }, + ); + + // An instant first-poll match is no longer terminal: it must be + // confirmed by two further consecutive online polls. + expect(isRunnerOnline).toHaveBeenCalledTimes(3); + }); + + test('a flap resets the streak, so an instant online then offline does not count as ready', async () => { + const getBootstrapStatus = jest.fn().mockResolvedValue(null); + // online (streak 1) -> offline (flap, reset) -> online, online, online + const isRunnerOnline = jest.fn() + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(true); + + await waitForRunnerReady( + { getBootstrapStatus, isRunnerOnline, sleep: noSleep }, + { quietSeconds: 0, intervalSeconds: 1, confirmChecks: 3 }, + ); + + expect(isRunnerOnline).toHaveBeenCalledTimes(5); + // The flap is surfaced in the log so the failure mode is debuggable. + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining('offline while confirming stability')); + }); + + test('an unstable (perpetually flapping) registration times out rather than resolving', async () => { + const getBootstrapStatus = jest.fn().mockResolvedValue(null); + // Alternating online/offline never reaches 3 in a row. + let poll = 0; + const isRunnerOnline = jest.fn().mockImplementation(() => Promise.resolve(poll++ % 2 === 0)); + + await expect( + waitForRunnerReady( + { getBootstrapStatus, isRunnerOnline, sleep: noSleep }, + { quietSeconds: 0, intervalSeconds: 10, timeoutMinutes: 0.5, confirmChecks: 3 }, + ), + ).rejects.toMatchObject({ timedOut: true }); + }); + + test('a bootstrap failure during confirmation still fast-fails', async () => { + // Online on the first poll (streak 1), then cloud-init phones a failure + // before the streak completes — the fast-fail must win. + const getBootstrapStatus = jest.fn() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce('failed:configuring'); + const isRunnerOnline = jest.fn().mockResolvedValue(true); + + await expect( + waitForRunnerReady( + { getBootstrapStatus, isRunnerOnline, sleep: noSleep }, + { quietSeconds: 0, intervalSeconds: 1, confirmChecks: 3 }, + ), + ).rejects.toMatchObject({ bootstrapStep: 'configuring' }); + }); + + test('confirmChecks defaults to 1 (legacy accept-first-online)', async () => { + const getBootstrapStatus = jest.fn().mockResolvedValue(null); + const isRunnerOnline = jest.fn().mockResolvedValue(true); + + await waitForRunnerReady({ getBootstrapStatus, isRunnerOnline, sleep: noSleep }, { quietSeconds: 0 }); + + expect(isRunnerOnline).toHaveBeenCalledTimes(1); + }); + }); });