Guard the listener's Running phase write with resourceVersion

[nikola-jokic]

Stacked on #4652 (which is stacked on #4647).

The problem

HandleJobStarted is a read-modify-write across two API calls:

1. GET the EphemeralRunner to decide whether it is still eligible for Running (not Failed/Succeeded/Outdated, not being deleted).
2. PATCH /status applying that decision.

The patch carried no precondition, so nothing tied it to the state the decision was made on. If the ephemeral runner controller wrote a terminal phase between the two calls — markAsFailed, markAsSucceeded, markAsOutdated, all non-optimistic merge patches themselves — the listener's patch won last and resurrected a runner that had already finished.

The existing guard is not wrong, it just only covers the lag case it was written for (a short job that completes before the listener drains the JobStarted message). The narrow interleaving was unprotected.

This is reproducible against a real API server: TestHandleJobStartedAgainstAPIServer/does_not_resurrect_a_runner_that_failed_after_the_read fails on the parent commit with expected: "Failed", actual: "Running".

The fix

Attach the observed metadata.resourceVersion to the merge patch whenever the phase is transitioned, and wrap the read-modify-write in retry.RetryOnConflict. The API server then rejects a patch built on a stale read with a 409, and the runner is re-read so the eligibility decision is made again against fresh state.

The job detail fields (jobId, jobRequestId, jobRepositoryName, workflowRunId, jobWorkflowRef, jobDisplayName) are deliberately left unguarded. They are write-once metadata, and the runner set only consults them (HasJob()) for runners that are neither done nor being deleted — both call sites are behind !isDone &&, and deletion is classified before phase. Patching them onto a terminal runner cannot change a scaling decision, so putting them behind a precondition would only add avoidable conflict retries on a path that has no correctness requirement.

Tests

• scaler_test.go: the fake API server now tracks resourceVersion and emulates the optimistic concurrency check, with a hook that mutates the runner between the read and the patch. New cases cover the precondition being sent (and correctly not sent for terminal runners), no resurrection for each of Failed/Succeeded/Outdated, and conflict-retry exhaustion.
• scaler_apiserver_test.go (new): runs HandleJobStarted against envtest so the fake's emulation is pinned to real kube-apiserver behaviour. The race is made deterministic by writing the terminal phase from inside the client transport, immediately before the scaler's patch is forwarded. Skips when KUBEBUILDER_ASSETS is unset.

Verified that every new test fails on the parent commit and passes here.

[Copilot]

Copilot review overview

🟢 Approval recommended

The reviewed changes address stale writes with comprehensive conflict and API-server coverage, with no unresolved blocking issues.

Review tier: Lite
Findings: None

What changed in this PR

Prevents HandleJobStarted from resurrecting terminal runners by guarding phase writes with resourceVersion and retrying conflicts.

Changes:

• Adds optimistic concurrency protection and conflict retries.
• Expands fake API tests for terminal-state races.
• Adds envtest coverage against a real API server.

File	Description
cmd/​ghalistener/​scaler/​scaler.go	Guards phase updates and retries stale writes.
cmd/​ghalistener/​scaler/​scaler_test.go	Tests preconditions, conflicts, and retries.
cmd/​ghalistener/​scaler/​scaler_apiserver_test.go	Validates behavior against kube-apiserver.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.