fix(lambda): add post-deregister busy check to prevent terminating active runners

[npwolf]

Summary

Rebases #5086 by @JVenberg on top of current main to resolve merge conflicts and get this fix moving. All credit for the original design and tests goes to Jack — this PR only adapts the change to the deleteGitHubRunner/per-runner error handling that landed on main since #5086 was opened.

Fixes #5085

The scale-down lambda can terminate an EC2 instance while it's actively running a job. This happens because a job can be assigned to a runner between checking its busy state and calling TerminateInstances.

The race condition

Current flow in removeRunner:

1. Check GitHub API: "Is this runner busy?" -> false
2. A job gets assigned to the runner here
3. Deregister the runner from GitHub
4. Terminate the EC2 instance
5. The in-flight job is killed

The fix

Add a post-deregistration busy re-check:

1. Check busy (fast-path to skip obviously busy runners)
2. Deregister from GitHub (prevents new job assignment server-side)
3. Re-check busy (now stable, since no new jobs can be assigned after deregistration)

If the re-check finds the runner busy, we skip termination and let the instance be cleaned up as an orphan once the job finishes.

Why this is safe

Deregistering a runner does not affect in-flight jobs. The runner worker uses job-scoped OAuth credentials from the job message, not the runner registration:

• JobRunner.cs lines 80-95: the worker creates its own VssConnection using systemConnection credentials from the job message
• The worker never checks runner registration status during execution
• Deregistration only affects the listener (no new job pickup), not the worker (current job)

What changed vs #5086

main gained a deleteGitHubRunner helper with per-runner de-registration error handling (retry-friendly, doesn't terminate on partial failure) after #5086 was opened, which is what caused the merge conflict. This PR keeps that behavior intact and layers the post-deregister busy re-check on top of it.

Test plan

• All existing tests pass
• New test: runner that becomes busy between deregister and re-check is NOT terminated
• New test: runner that returns 404 on post-deregister busy check IS terminated (runner fully removed from GitHub)

[npwolf]

🤖 Automated review summary (via /datagrail:code-review --report)

Ran a backend-correctness pass and a security pass on this diff. Both independently converged on the same core issue, which I verified directly against the code: the post-deregister busy re-check can't observe "busy" in production, because deregistering the runner makes it 404 on the next GET, and 404 is mapped to "not busy." So the fix doesn't close the TOCTOU race it targets — whether the re-check sees idle (terminate immediately) or the rare eventually-consistent "busy" (terminate via orphan cleanup ~2 cycles later), the in-flight job dies either way. See the 3 inline comments for details, including a test that mocks a response the real API can't return.

The rebase itself (resolving conflicts against current main) is clean and preserves existing behavior — this is a pre-existing design gap inherited from the original #5086, not something introduced by the rebase. Flagging so the mechanism can be revisited before merge.

[npwolf]

🤖 [HIGH] The post-deregister busy re-check can never see "busy" in production — the race this PR targets stays open

Step 2 deletes the runner from GitHub (only proceeds on 204). Step 3 then GETs that same runner ID, which now returns 404. getGitHubRunnerBusyState maps 404 → null → false ("treating as not busy", lines 84–91). So postDeregisterStates.every(busy => busy === false) is essentially always true in production → the instance is always terminated. If a job was assigned during the real race window (between the Step 1 check at :176 and the delete at :191), it is still running on the box and still gets killed. The re-check reduces to if (allDeregisterSucceeded) terminate plus a wasted API round-trip — the only real guard remains the pre-deregister Step 1 check, which is what already existed before this PR.

Fix: a busy signal that survives deregistration is required. Either re-check busy before deleting, or don't treat a post-delete 404 as authoritative "idle" — after your own successful DELETE, 404 is guaranteed and tells you nothing about job state. This needs a rethink of the mechanism, not a tweak.

(Independently confirmed by both a backend-correctness review and a security review of this diff.)

[npwolf]

🤖 [HIGH] This test asserts a response the real GitHub API can't return, masking the issue above

This "became busy between deregister and re-check" test mocks the second (post-deregister) busy check to return { busy: true }. A runner deleted with a 204 returns 404 on a subsequent GET — as the sibling 404 test elsewhere in this file correctly assumes. So this test exercises an impossible production state; it passes green while the production code actually takes the opposite branch (terminate). It gives false confidence that the guard works.

Fix: mock the post-deregister check to return 404 (matching real GitHub behavior after a successful delete) and assert what the code actually does in that case, rather than asserting the desired-but-unreachable behavior.

[npwolf]

🤖 [MEDIUM] Even if this branch were reachable, orphan cleanup terminates the still-busy runner within ~2 cron cycles anyway

In the (production-unreachable) branch where Step 3 sees busy and leaves the instance running: next cycle, listGitHubRunners no longer includes the deregistered runner → markOrphan tags it as an orphan. Following cycle, terminateOrphan → lastChanceCheckOrphanRunner GETs it → 404 → state === null → isOrphan = true → terminateRunner. The safety check that spares offline && busy runners can never fire for a deregistered runner, since it's 404, not offline+busy. So the "leave it as an orphan, the job will complete" promise in the comments above is false — the job dies regardless of which branch is taken.

Fix: if you intend to spare busy runners, they need to stay registered (so busy state remains queryable) or be tagged with a marker the orphan-cleanup path explicitly honors as "keep until GitHub reports it idle/gone," rather than relying on 404 as a safe-to-kill signal.

[npwolf]

Closing — the review above surfaced that this mechanism (and the original #5086) doesn't actually close the TOCTOU race: the post-deregister busy re-check can't observe "busy" once the runner is deregistered (it 404s, which is treated as idle), so in-flight jobs can still be killed. Will re-approach with a design that keeps the busy signal alive post-deregistration, or reorders the checks differently, and open a fresh PR.