fix: confirm warm-restart runner registration is stable before start succeeds (#67)#68
Merged
Merged
Conversation
…succeeds (#67) On a reuse:stop warm restart the reused instance 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 consumer's gated stop job never fired — leaking a running instance that kept holding its whitelisted EIP. The wait loop now requires the runner to report online on `confirmChecks` CONSECUTIVE polls before declaring it ready (3 on a warm restart, 2 on a cold launch; default 1 keeps the library function backward compatible). A registration that flaps offline within that window resets the streak and is logged (wait_for_runner outcome:flap), and if it never stabilises the start step fails via the normal timeout path — which already captures the instance console output and cleans up — instead of returning a false success. Unique per-run labels mean no cross-run stale registration is possible, so a stability window is sufficient without a pre-start snapshot. Docs stress scheduling the cleanup reaper for warm pools: it is the backstop that reaps a leaked running instance whose runner never came up (runner-not-registered past the grace period). Signed-off-by: kurok <22548029+kurok@users.noreply.github.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
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.
Fixes #67.
Problem
On a
reuse: stopwarm restart, the reused instance could re-register and reportonlineon the wait loop's very first poll (elapsed_s=0), then drop before the dependent job was scheduled. The start step exited green, the consuming job sat inqueuedforever, and the consumer'sstopjob — gated on that job reaching a terminal state — never fired, leaving a running instance holding its whitelisted EIP until manual intervention.Root cause:
waitForRunnerReadytreated a single instantaneousonlineobservation as terminal success (src/wait.js). Nothing distinguished a healthy registration from one that flapped offline seconds later.Fix
Require the registration to prove stable, not just appear once. The wait loop now needs the runner to report
onlineonconfirmChecksconsecutive polls before declaring it ready:marketType === 'reused') — exactly the reported failure path.waitForRunnerReadystays backward compatible and all existing tests are unchanged.A registration that flaps offline within the window resets the streak and is logged (
wait_for_runner … outcome:flap) — the only instance-side-adjacent evidence in the workflow log. If it never stabilises, the start step fails via the existing timeout path, which already captures the instance console output and runs cleanup — so this also delivers the issue's "diagnostics on this path" ask (fix #3), instead of the previous silent false success.Why no pre-start "freshness snapshot" (issue's fix #1)
Labels are unique per run (
config.generateUniqueLabel()), so a runner carrying this run's label can only have been registered by this boot — there is no cross-run stale record to filter out. The remaining risk is a within-run flap, which the stability window covers directly. A snapshot would add API calls for no additional protection.Leaked-instance backstop (issue's fix #4)
The existing
cleanupreaper already reaps a running managed instance with no registered runner past the grace period (runner-not-registered). The README now stresses scheduling it for warm pools as the backstop that recovers a stranded-job leak. No reaper code change needed.Changes
src/wait.js—confirmChecksconsecutive-online confirmation + flap logging; timeout log now carriesconfirm_checks/online_streak.src/index.js—start()passesconfirmChecks(3 warm / 2 cold) based on placement.tests/wait.test.js— 5 new tests (consecutive requirement, flap-reset, perpetual-flap timeout, fast-fail during confirmation, default-1 legacy path). Full suite: 249 passing.dist/index.js— rebuilt vianpm run package(ncc 0.44.0).README.md/CHANGELOG.md— warm-pool registration-stability note + reaper-as-backstop guidance.Verification
npm run lintclean.npm test— 249 tests pass (244 prior + 5 new).dist/reproducible: onlydist/index.jschanges on rebuild (satisfiesverify-dist).