diff --git a/.gds/bundle.lock.yaml b/.gds/bundle.lock.yaml index 6714fa66..04736116 100644 --- a/.gds/bundle.lock.yaml +++ b/.gds/bundle.lock.yaml @@ -10,7 +10,7 @@ bundle: attestation_identity_digest: "sha256:e1b787c63de916f750e2cbcbf664967669024e1062964515f612b2c826c9e10d" projection: - input_digest: "sha256:460039c6ea070da7eb3a88a53f5c5cfcc7bf852209b4f7949411b6af43c442bc" + input_digest: "sha256:16d00b8c8cfe471cc00e14ab983f28cbed63448fc13f2f7f35977d827a172f11" output_digest: "sha256:6df8425d9b3ebe48a39fad7d21493444364c697256721cd692ea35e1658abf0e" files: - path: ".gds/compiled-policy.json" diff --git a/.gds/repository.yaml b/.gds/repository.yaml index c3a3b85c..05551f79 100644 --- a/.gds/repository.yaml +++ b/.gds/repository.yaml @@ -77,8 +77,7 @@ verification: - "lint" - "test" - "build" - required_contexts: - - "Gate" + required_contexts: [] agent: context_profile: "project-default" diff --git a/.github/branch-protection.yaml b/.github/branch-protection.yaml index c2701450..2e20c2d5 100644 --- a/.github/branch-protection.yaml +++ b/.github/branch-protection.yaml @@ -1,7 +1,6 @@ branch: main required_status_checks: - strict: true - contexts: - - Gate + strict: false + contexts: [] required_pull_request_reviews: {} enforce_admins: true diff --git a/.github/rulesets/branch-main.json b/.github/rulesets/branch-main.json index 55923841..1ed24185 100644 --- a/.github/rulesets/branch-main.json +++ b/.github/rulesets/branch-main.json @@ -25,14 +25,6 @@ "dismissal_restriction": {"enabled": false, "allowed_actors": []}, "allowed_merge_methods": ["merge"] } - }, - { - "type": "required_status_checks", - "parameters": { - "strict_required_status_checks_policy": true, - "do_not_enforce_on_create": false, - "required_status_checks": [{"context": "Gate"}] - } } ] } diff --git a/AGENTS.md b/AGENTS.md index 76069fbd..2314ecbc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,3 +14,7 @@ identity, state/schema integrity, one-job isolation and release provenance. Formatting and generated artifacts should be repaired automatically. Run `make verify` after changes. Preserve unrelated worktree changes. + +Ordinary merge does not wait on a required general CI status check. The `Gate` +job still runs as truthful advisory evidence. Live GitHub settings are not +applied from this tree. diff --git a/CHANGELOG.md b/CHANGELOG.md index f84b24a4..51e51535 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ ## Unreleased +- GARM `v0.2.1-nddev.93` retires excess undemanded idle ephemeral JIT + registrations through the Actions service RemoveRunner path (204 then 404) + after two matching observations. REST `busy` must be explicit false; an + omitted field is unknown, not idle. Scale-set statistics of all zeros are + not proof that nothing is running. Identity changes, demand, min-idle, + bootstrap/active states and JobStillRunningException refuse the delete. + Provider delete remains GARM's ordinary absent-runner reconcile. Idle + retirement is best-effort: a remote read or delete failure is a classified + warning and cannot block absent-runner consolidation. The original + consolidation lock lifetime is unchanged: runner Unlock stays deferred until + consolidateRunnerState returns. Candidate age and + min-idle are decided locally before any new API call. The `.92` patches are + unchanged. REST overlay fallback for acknowledged never-started jobs is not + in this derivative: JobAssigned does not bind GitHub job ID or run attempt. +- Ordinary merge in this repository does not require a general CI status + check. `Gate` remains truthful advisory evidence. + - Confirm current GitHub scale-set demand before each new JIT registration. A retained local assignment requests reconciliation but cannot allocate a runner against a fresh zero-demand snapshot. Reads have a five-second diff --git a/config/example-runner-1.yaml b/config/example-runner-1.yaml index 844073ac..5b7d3c5a 100644 --- a/config/example-runner-1.yaml +++ b/config/example-runner-1.yaml @@ -6,7 +6,7 @@ platform: control_plane: manager: garm - manager_version: v0.2.1-nddev.92 + manager_version: v0.2.1-nddev.93 scheduling_mode: scale-set provider: incus provider_version: v0.1.5-nddev.129 diff --git a/config/example-runner-2.yaml b/config/example-runner-2.yaml index 9e09bd0f..56caddfb 100644 --- a/config/example-runner-2.yaml +++ b/config/example-runner-2.yaml @@ -6,7 +6,7 @@ platform: control_plane: manager: garm - manager_version: v0.2.1-nddev.92 + manager_version: v0.2.1-nddev.93 scheduling_mode: scale-set provider: incus provider_version: v0.1.5-nddev.129 diff --git a/config/example-runner-3.yaml b/config/example-runner-3.yaml index f6124603..25a32515 100644 --- a/config/example-runner-3.yaml +++ b/config/example-runner-3.yaml @@ -6,7 +6,7 @@ platform: control_plane: manager: garm - manager_version: v0.2.1-nddev.92 + manager_version: v0.2.1-nddev.93 scheduling_mode: scale-set provider: incus provider_version: v0.1.5-nddev.129 diff --git a/config/example-runner-4.yaml b/config/example-runner-4.yaml index a48e7bb8..f2d755cf 100644 --- a/config/example-runner-4.yaml +++ b/config/example-runner-4.yaml @@ -6,7 +6,7 @@ platform: control_plane: manager: garm - manager_version: v0.2.1-nddev.92 + manager_version: v0.2.1-nddev.93 scheduling_mode: scale-set provider: incus provider_version: v0.1.5-nddev.129 diff --git a/config/example-services.yaml b/config/example-services.yaml index 6701a587..2c4837b3 100644 --- a/config/example-services.yaml +++ b/config/example-services.yaml @@ -24,7 +24,7 @@ platform: control_plane: manager: garm - manager_version: v0.2.1-nddev.92 + manager_version: v0.2.1-nddev.93 scheduling_mode: scale-set provider: incus provider_version: v0.1.5-nddev.129 diff --git a/config/fleet-contract.yaml b/config/fleet-contract.yaml index 93f70ec7..5f568745 100644 --- a/config/fleet-contract.yaml +++ b/config/fleet-contract.yaml @@ -19,7 +19,7 @@ schema_version: 2 # a runner class withdrawn, a tenancy boundary narrowed, an admission rule # changed. Adding a class or a tenant does not bump it -- that only widens what # is on offer. -contract_version: 5 +contract_version: 6 execution: worker_kind: incus-container diff --git a/config/garm-derivative.yaml b/config/garm-derivative.yaml index 99cd6c73..db722c32 100644 --- a/config/garm-derivative.yaml +++ b/config/garm-derivative.yaml @@ -1,6 +1,6 @@ schema_version: 1 artifact: garm -derivative_version: v0.2.1-nddev.92 +derivative_version: v0.2.1-nddev.93 upstream: repository: https://github.com/cloudbase/garm release: v0.2.1 @@ -97,6 +97,9 @@ patches: - path: third_party/garm/patches/0030-confirm-live-scale-set-demand.patch sha256: 102aebd1fb51a7d4bd619fbbb5ab6e3e5706677e35cdbd6c76663640a24f1497 purpose: Confirm current GitHub scale-set demand with a bounded read before JIT registration, preserving admission limits and queued identities while rejecting stale local-only demand. + - path: third_party/garm/patches/0031-retire-excess-idle-online-runners.patch + sha256: 62f8a273e86ed19fe80905ad7205fbf5d6fdefdf1847cc085303c613db3f1345 + purpose: Require Actions RemoveRunner to return 204 and call warnIdleRetirement before the original consolidateRunnerState body so idle-retirement failures cannot block absent-runner consolidation while runner locks stay held until that function returns. overlays: - path: third_party/garm/overlay/workers/scaleset/queue_intent.go sha256: be85eac2126d7723483467958222f401061bd08a68113af61ff4b51208f717d7 @@ -116,6 +119,12 @@ overlays: - path: third_party/garm/overlay/workers/scaleset/confirmed_demand_test.go sha256: 32f7485aeae73c15b28148d8d244e93a35cd8fb6dccea99a418db890107604d8 purpose: Prove fresh zero demand prevents allocation, stale persisted zero can recover against fresh positive demand, positive observations are not reused, and missing, mismatched or unavailable evidence fails closed. + - path: third_party/garm/overlay/workers/scaleset/idle_online_retire.go + sha256: a7eb84571633d76ce6f3134b362e90b695beb7f4f36c882363bef087ad2d3d69 + purpose: Retire excess undemanded idle ephemeral JIT registrations through Actions RemoveRunner after two matching observations, requiring REST busy to be explicit false and refusing omitted busy, identity change, demand, or JobStillRunningException. Local age and min-idle decide whether any API call happens; unseen observation IDs are pruned. + - path: third_party/garm/overlay/workers/scaleset/idle_online_retire_test.go + sha256: 94c4d4ba7368adba4e9620c89b116acccba6c09771413896150c4b0c5379dbc4 + purpose: Prove omitted REST busy is unknown, Actions list Busy defaults are untrusted, all-zero scale-set statistics do not prove idle, mixed identity restarts observation, two matching observations are required before RemoveRunner, empty aged-candidate plans make no API call, and helper RemoveRunner 204+404, 409, unexpected 2xx and non-404 read-back outcomes. These are predicate and fake-client helpers, not Worker/DB/HTTP integration. build: container_image: docker.io/library/golang@sha256:116d58cbd88c1297624acc6e967a060012422bacf9930927e23fb719189c6f36 go_version: go1.26.6 @@ -130,7 +139,7 @@ build: - sqlite_omit_load_extension reproducible_rebuilds: 2 maximum_required_glibc: "2.34" - binary_sha256: c112777344debf7e9c0b82524b6de381872ff6576e700cc4c4469b136770b560 + binary_sha256: 8f6519500e90ad028c62b7fc3f5c8f2c7784fd82ef653c37c8db9e45afcfa841 runtime_contract: queue_intent_schema_version: 6 event_driven_scale_set_wake: true @@ -168,6 +177,7 @@ runtime_contract: authoritative_repository_binding: true authoritative_running_repository_binding: true authoritative_idle_offline_runner_reaping: true + authoritative_idle_online_runner_retirement: true authoritative_access_refusal_backoff_seconds: 900 job_started_runner_identity: true job_available_correlation_binding: true diff --git a/docs/runbooks/fleet-alerts.md b/docs/runbooks/fleet-alerts.md index 0fe04c09..d43dd92d 100644 --- a/docs/runbooks/fleet-alerts.md +++ b/docs/runbooks/fleet-alerts.md @@ -9,6 +9,22 @@ use the exact journal identity and its first-queued/state-entry timestamps to reconstruct the wait. A lower aggregate maximum can mean that one job advanced or was cancelled while other work remains stalled. Check its actual outcome. +Idle online JIT registrations with local pending/idle status are not reaped +by the `.92` offline-only timeout path. Excess undemanded idle capacity is +retired only after two matching observations, an exact identity match, age +of at least thirty minutes, REST `busy` explicitly false, and Actions +RemoveRunner returning 204 with a 404 read-back. An omitted REST `busy` +field is unknown, not idle. Scale-set statistics of all zeros are not +proof that nothing is running; a later REST `busy=true` must refuse the +delete. GARM then deletes the provider instance only after the +registration is absent. Never use the public GitHub REST force-delete. + +`lifecycle_assigned_stall` is the assigned *state clock*, not +`FirstQueuedAt`. Assigned TTL is 600 seconds; a lower aggregate maximum +can mean the oldest assigned job started, cancelled, or demoted while a +queued waiter with an hours-old `FirstQueuedAt` remains. Do not shorten +that alert to hide age. + A REST workflow job can remain queued while the scale-set API reports no jobs assigned to that scale set. Preserve both observations. GARM confirms the scale-set API's current `statistics.TotalAssignedJobs` before creating a new @@ -17,6 +33,8 @@ old local `JobAssigned` record is a reason to reconcile; it is not independent proof of dispatchable demand. Conversely, an old persisted zero must not block an authoritative read that could show new demand. GitHub documents this distinction in the [scale-set client autoscaling contract](https://github.com/actions/scaleset#autoscaling). +Idle-online retirement is best-effort and cannot block absent-runner +consolidation. It does not repair a lost upstream assignment. Compare a start with its completion in the same message batch when diagnosing fast jobs. Completion is terminal even if no intermediate running observation diff --git a/internal/fleetcontract/contract.go b/internal/fleetcontract/contract.go index 576225f2..f92821be 100644 --- a/internal/fleetcontract/contract.go +++ b/internal/fleetcontract/contract.go @@ -366,8 +366,8 @@ func loadMerge(path string) (Merge, error) { if err := yaml.Unmarshal(raw, &declared); err != nil { return Merge{}, fmt.Errorf("parse branch protection declaration: %w", err) } - if declared.Branch == "" || len(declared.RequiredStatusChecks.Contexts) == 0 { - return Merge{}, fmt.Errorf("branch protection declaration names no branch or no required context") + if declared.Branch == "" { + return Merge{}, fmt.Errorf("branch protection declaration names no branch") } contexts := slices.Clone(declared.RequiredStatusChecks.Contexts) slices.Sort(contexts) diff --git a/internal/fleetcontract/contract_test.go b/internal/fleetcontract/contract_test.go index 80a5a360..21968213 100644 --- a/internal/fleetcontract/contract_test.go +++ b/internal/fleetcontract/contract_test.go @@ -29,10 +29,10 @@ func TestPublicExampleContractBuildsWithoutEstateAccess(t *testing.T) { t.Fatal(err) } if contract.Repository != "NDDev-OpenNetwork/github-actions" || len(contract.RunnerClasses) == 0 || - len(contract.Tenants) == 0 || len(contract.Merge.RequiredContexts) != 1 || contract.Merge.RequiredContexts[0] != "Gate" { + len(contract.Tenants) == 0 || len(contract.Merge.RequiredContexts) != 0 { t.Fatalf("public contract = %#v", contract) } - if contract.SchemaVersion != 2 || contract.ContractVersion != 5 || contract.Execution.WorkerKind != "incus-container" || + if contract.SchemaVersion != 2 || contract.ContractVersion != 6 || contract.Execution.WorkerKind != "incus-container" || !contract.Execution.Ephemeral || contract.Execution.JobsPerWorker != 1 || !contract.ResourceSemantics.HardMemoryExcludesEmergencySwap || contract.ResourceSemantics.EmergencySwapSchedulable || contract.ResourceSemantics.CPUMode != "weighted-overcommit" { t.Fatalf("contract v2 semantics = %#v", contract) diff --git a/internal/garmderivative/script.go b/internal/garmderivative/script.go index d591e0e2..3e31c865 100644 --- a/internal/garmderivative/script.go +++ b/internal/garmderivative/script.go @@ -117,6 +117,7 @@ var runtimeContractFields = []string{ "authoritative_repository_binding", "authoritative_running_repository_binding", "authoritative_idle_offline_runner_reaping", + "authoritative_idle_online_runner_retirement", "authoritative_access_refusal_backoff_seconds", "job_started_runner_identity", "job_available_correlation_binding", diff --git a/internal/garmderivative/types.go b/internal/garmderivative/types.go index c6167427..f114865f 100644 --- a/internal/garmderivative/types.go +++ b/internal/garmderivative/types.go @@ -100,6 +100,7 @@ type RuntimeContract struct { AuthoritativeRepositoryBinding bool `json:"authoritative_repository_binding" yaml:"authoritative_repository_binding"` AuthoritativeRunningRepositoryBinding bool `json:"authoritative_running_repository_binding" yaml:"authoritative_running_repository_binding"` AuthoritativeIdleOfflineRunnerReaping bool `json:"authoritative_idle_offline_runner_reaping" yaml:"authoritative_idle_offline_runner_reaping"` + AuthoritativeIdleOnlineRunnerRetirement bool `json:"authoritative_idle_online_runner_retirement" yaml:"authoritative_idle_online_runner_retirement"` AuthoritativeAccessRefusalBackoffSeconds int `json:"authoritative_access_refusal_backoff_seconds" yaml:"authoritative_access_refusal_backoff_seconds"` JobStartedRunnerIdentity bool `json:"job_started_runner_identity" yaml:"job_started_runner_identity"` JobAvailableCorrelationBinding bool `json:"job_available_correlation_binding" yaml:"job_available_correlation_binding"` diff --git a/scripts/build-garm-nddev.sh b/scripts/build-garm-nddev.sh index 02725425..4b9d3112 100755 --- a/scripts/build-garm-nddev.sh +++ b/scripts/build-garm-nddev.sh @@ -19,7 +19,7 @@ set -Eeuo pipefail # Every value below is the manifest's. Editing one here detaches the build # from the provenance it is reviewed against, which is why the region is # regenerated and compared rather than maintained. -readonly derivative_version="v0.2.1-nddev.92" +readonly derivative_version="v0.2.1-nddev.93" readonly upstream_repository="https://github.com/cloudbase/garm" readonly upstream_commit="154638445c3949c1958b01812f69d9a1e4d82684" readonly build_image="docker.io/library/golang@sha256:116d58cbd88c1297624acc6e967a060012422bacf9930927e23fb719189c6f36" @@ -32,7 +32,7 @@ readonly build_module_mode="vendor" readonly build_tags="osusergo,netgo,sqlite_omit_load_extension" readonly build_reproducible_rebuilds="2" readonly build_maximum_required_glibc="2.34" -readonly expected_binary_sha256="c112777344debf7e9c0b82524b6de381872ff6576e700cc4c4469b136770b560" +readonly expected_binary_sha256="8f6519500e90ad028c62b7fc3f5c8f2c7784fd82ef653c37c8db9e45afcfa841" readonly patch_paths=( "third_party/garm/patches/0001-event-driven-reconciliation.patch" "third_party/garm/patches/0002-central-queue-admission.patch" @@ -64,6 +64,7 @@ readonly patch_paths=( "third_party/garm/patches/0028-terminal-workflow-run-reconciliation.patch" "third_party/garm/patches/0029-scale-up-from-admitted-intent.patch" "third_party/garm/patches/0030-confirm-live-scale-set-demand.patch" + "third_party/garm/patches/0031-retire-excess-idle-online-runners.patch" ) readonly patch_sha256s=( "2f0571f141e7388d6ea0cb0341549ba5bf5dab26d0006382a71b76655e272d34" @@ -96,6 +97,7 @@ readonly patch_sha256s=( "3bc2722e87f1721c6094130d0f07310a796bf8074504ef121acac8ca7402887c" "e63c57a3c0a9d492ba45bc35f02e1f9abbc84e7e7923717b3be6ff45206f8a0d" "102aebd1fb51a7d4bd619fbbb5ab6e3e5706677e35cdbd6c76663640a24f1497" + "62f8a273e86ed19fe80905ad7205fbf5d6fdefdf1847cc085303c613db3f1345" ) readonly overlay_paths=( "third_party/garm/overlay/workers/scaleset/queue_intent.go" @@ -104,6 +106,8 @@ readonly overlay_paths=( "third_party/garm/overlay/workers/provider/nddev_create_retry_test.go" "third_party/garm/overlay/workers/scaleset/confirmed_demand.go" "third_party/garm/overlay/workers/scaleset/confirmed_demand_test.go" + "third_party/garm/overlay/workers/scaleset/idle_online_retire.go" + "third_party/garm/overlay/workers/scaleset/idle_online_retire_test.go" ) readonly overlay_sha256s=( "be85eac2126d7723483467958222f401061bd08a68113af61ff4b51208f717d7" @@ -112,6 +116,8 @@ readonly overlay_sha256s=( "c52b783f1b420a3bb15fdc3ca7a445e295251e82e64a85f4d2b41de474546eff" "5c289dc30a52e5329e3d4887660629e7d12f855b5c8204d2d99e68c003f39458" "32f7485aeae73c15b28148d8d244e93a35cd8fb6dccea99a418db890107604d8" + "a7eb84571633d76ce6f3134b362e90b695beb7f4f36c882363bef087ad2d3d69" + "94c4d4ba7368adba4e9620c89b116acccba6c09771413896150c4b0c5379dbc4" ) readonly overlay_targets=( "workers/scaleset/queue_intent.go" @@ -120,6 +126,8 @@ readonly overlay_targets=( "workers/provider/nddev_create_retry_test.go" "workers/scaleset/confirmed_demand.go" "workers/scaleset/confirmed_demand_test.go" + "workers/scaleset/idle_online_retire.go" + "workers/scaleset/idle_online_retire_test.go" ) # END GENERATED REGION diff --git a/third_party/garm/overlay/workers/scaleset/idle_online_retire.go b/third_party/garm/overlay/workers/scaleset/idle_online_retire.go new file mode 100644 index 00000000..173a00fd --- /dev/null +++ b/third_party/garm/overlay/workers/scaleset/idle_online_retire.go @@ -0,0 +1,474 @@ +package scaleset + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + "time" + + runnerErrors "github.com/cloudbase/garm-provider-common/errors" + commonParams "github.com/cloudbase/garm-provider-common/params" + "github.com/cloudbase/garm/locking" + "github.com/cloudbase/garm/params" + "github.com/google/go-github/v84/github" +) + +const ( + idleRetirementMinAge = 30 * time.Minute + idleRetirementConfirmAfter = 30 * time.Second + idleRetirementReadTimeout = 20 * time.Second +) + +// idleRetirementGate remembers the first eligible observation per agent ID so +// a later cycle can require two matching observations before RemoveRunner. +type idleRetirementGate struct { + seen map[int64]idleObservation +} + +type idleObservation struct { + AgentID int64 + Name string + ScaleSetID int + FirstSeen time.Time +} + +type restBusyState int + +const ( + restBusyUnknown restBusyState = iota + restBusyFalse + restBusyTrue +) + +func actionsStatusOnline(status string) bool { + return status == "online" +} + +func actionsStatusString(status any) string { + text, ok := status.(string) + if !ok { + return "" + } + return text +} + +func restBusyFromPointer(busy *bool) restBusyState { + if busy == nil { + return restBusyUnknown + } + if *busy { + return restBusyTrue + } + return restBusyFalse +} + +// actionsListBusy is the GARM RunnerReference.Busy bool. Encoding Busy as a +// non-pointer means a missing Actions-service field unmarshals to false. That +// default is not proof the runner is idle. +func actionsListBusyUntrusted(_ bool) {} + +type idleRetirementDecision struct { + Eligible bool + Ready bool + Reason string +} + +type idleRetirementEvidence struct { + AgentID int64 + Name string + ScaleSetID int + LocalStatus params.RunnerStatus + CreatedAt time.Time + MinIdle uint + IdleCount int + Now time.Time + RESTID int64 + RESTName string + RESTStatus string + RESTBusy *bool + ActionsID int64 + ActionsName string + ActionsScaleSetID int + ActionsEphemeral bool + ActionsEnabled bool + ActionsState string + ActionsStatus string + AssignedJobs int + AcquiredJobs int + AvailableJobs int + RunningJobs int + BusyRunners int + StatisticsPresent bool +} + +func evaluateIdleRetirement(ev idleRetirementEvidence) idleRetirementDecision { + if ev.AgentID <= 0 || ev.Name == "" || ev.ScaleSetID <= 0 { + return idleRetirementDecision{Reason: "missing-identity"} + } + switch ev.LocalStatus { + case params.RunnerPending, params.RunnerIdle: + default: + return idleRetirementDecision{Reason: "local-not-idle-or-pending"} + } + if ev.Now.Sub(ev.CreatedAt) < idleRetirementMinAge { + return idleRetirementDecision{Reason: "too-young"} + } + if ev.IdleCount <= int(ev.MinIdle) { + return idleRetirementDecision{Reason: "at-or-below-min-idle"} + } + if !ev.StatisticsPresent { + return idleRetirementDecision{Reason: "scale-set-statistics-absent"} + } + if ev.AssignedJobs < 0 || ev.AcquiredJobs < 0 || ev.AvailableJobs < 0 || ev.RunningJobs < 0 || ev.BusyRunners < 0 { + return idleRetirementDecision{Reason: "scale-set-statistics-invalid"} + } + if ev.AssignedJobs > 0 || ev.AcquiredJobs > 0 || ev.AvailableJobs > 0 || ev.RunningJobs > 0 || ev.BusyRunners > 0 { + return idleRetirementDecision{Reason: "scale-set-has-demand"} + } + if ev.RESTID != ev.AgentID || ev.ActionsID != ev.AgentID || + ev.RESTName != ev.Name || ev.ActionsName != ev.Name || + ev.ActionsScaleSetID != ev.ScaleSetID { + return idleRetirementDecision{Reason: "identity-mismatch"} + } + if !ev.ActionsEphemeral || !ev.ActionsEnabled || ev.ActionsState != "Provisioned" { + return idleRetirementDecision{Reason: "not-provisioned-ephemeral"} + } + if ev.RESTStatus != "online" || !actionsStatusOnline(ev.ActionsStatus) { + return idleRetirementDecision{Reason: "not-online"} + } + switch restBusyFromPointer(ev.RESTBusy) { + case restBusyUnknown: + return idleRetirementDecision{Reason: "rest-busy-omitted"} + case restBusyTrue: + return idleRetirementDecision{Reason: "rest-busy"} + case restBusyFalse: + default: + return idleRetirementDecision{Reason: "rest-busy-omitted"} + } + return idleRetirementDecision{Eligible: true, Reason: "eligible"} +} + +func (g *idleRetirementGate) confirm(ev idleRetirementEvidence) idleRetirementDecision { + decision := evaluateIdleRetirement(ev) + if !decision.Eligible { + if g.seen != nil { + delete(g.seen, ev.AgentID) + } + return decision + } + if g.seen == nil { + g.seen = map[int64]idleObservation{} + } + prev, ok := g.seen[ev.AgentID] + if !ok || prev.Name != ev.Name || prev.ScaleSetID != ev.ScaleSetID { + g.seen[ev.AgentID] = idleObservation{ + AgentID: ev.AgentID, Name: ev.Name, ScaleSetID: ev.ScaleSetID, FirstSeen: ev.Now, + } + decision.Reason = "first-observation" + return decision + } + if ev.Now.Sub(prev.FirstSeen) < idleRetirementConfirmAfter { + decision.Reason = "confirmation-pending" + return decision + } + decision.Ready = true + decision.Reason = "confirmed" + return decision +} + +func (g *idleRetirementGate) forget(agentID int64) { + if g.seen != nil { + delete(g.seen, agentID) + } +} + +func (g *idleRetirementGate) prune(live map[int64]struct{}) { + if g.seen == nil { + return + } + for agentID := range g.seen { + if _, ok := live[agentID]; !ok { + delete(g.seen, agentID) + } + } +} + +func idleRetirementLocalPlan(runners map[string]params.Instance, minIdle uint, now time.Time) (idleCount int, aged []params.Instance) { + for _, runner := range runners { + if providerRemovalProtected(runner.Status) || runner.AgentID <= 0 { + continue + } + switch runner.RunnerStatus { + case params.RunnerPending, params.RunnerIdle: + idleCount++ + if now.Sub(runner.CreatedAt) >= idleRetirementMinAge { + aged = append(aged, runner) + } + } + } + if idleCount <= int(minIdle) { + return idleCount, nil + } + return idleCount, aged +} + +func warnIdleRetirement(ctx context.Context, err error) { + if err == nil { + return + } + slog.WarnContext(ctx, "idle retirement failed; continuing runner consolidation", "error", err, "error_class", "idle-retirement") +} + +type idleActionsClient interface { + RemoveRunner(context.Context, int64) error + GetRunner(context.Context, int64) (params.RunnerReference, error) +} + +type idleRemoveOutcome string + +const ( + idleRemoveMarkAbsent idleRemoveOutcome = "mark-absent" + idleRemoveConflict idleRemoveOutcome = "conflict" + idleRemoveNone idleRemoveOutcome = "none" +) + +func decideIdleRemoveRunner(removeErr error, getErr error, getFound bool) (idleRemoveOutcome, error) { + if removeErr != nil { + if errors.Is(removeErr, runnerErrors.ErrNotFound) { + return idleRemoveMarkAbsent, nil + } + var conflict interface{ Conflict() bool } + if errors.As(removeErr, &conflict) || isConflictError(removeErr) { + return idleRemoveConflict, nil + } + return idleRemoveNone, fmt.Errorf("Actions RemoveRunner: %w", removeErr) + } + if getFound { + return idleRemoveNone, fmt.Errorf("Actions runner still present after RemoveRunner") + } + if getErr != nil && !errors.Is(getErr, runnerErrors.ErrNotFound) { + return idleRemoveNone, fmt.Errorf("read back Actions runner: %w", getErr) + } + return idleRemoveMarkAbsent, nil +} + +func applyIdleRemoveRunner(ctx context.Context, cli idleActionsClient, runnerID int64) (idleRemoveOutcome, error) { + removeErr := cli.RemoveRunner(ctx, runnerID) + getFound := false + var getErr error + if removeErr == nil { + _, getErr = cli.GetRunner(ctx, runnerID) + getFound = getErr == nil + } + return decideIdleRemoveRunner(removeErr, getErr, getFound) +} + +func (w *Worker) restRunner(ctx context.Context, runnerID int64) (*github.Runner, error) { + cli, err := w.GetScaleSetClient() + if err != nil { + return nil, err + } + ghCli, err := cli.GetGithubClient() + if err != nil { + return nil, err + } + entity := ghCli.GetEntity() + type repoGetter interface { + GetRunner(context.Context, string, string, int64) (*github.Runner, *github.Response, error) + } + type orgGetter interface { + GetOrganizationRunner(context.Context, string, int64) (*github.Runner, *github.Response, error) + } + switch entity.EntityType { + case params.ForgeEntityTypeRepository: + getter, ok := ghCli.(repoGetter) + if !ok { + return nil, fmt.Errorf("github client does not expose repository GetRunner") + } + runner, _, err := getter.GetRunner(ctx, entity.Owner, entity.Name, runnerID) + return runner, err + case params.ForgeEntityTypeOrganization: + getter, ok := ghCli.(orgGetter) + if !ok { + return nil, fmt.Errorf("github client does not expose organization GetRunner") + } + runner, _, err := getter.GetOrganizationRunner(ctx, entity.Owner, runnerID) + return runner, err + default: + return nil, fmt.Errorf("entity type %s has no REST runner getter", entity.EntityType) + } +} + +func (w *Worker) retireExcessIdleCapacity() error { + now := time.Now().UTC() + live := make(map[int64]struct{}, len(w.runners)) + for _, runner := range w.runners { + if runner.AgentID > 0 { + live[runner.AgentID] = struct{}{} + } + } + w.idleRetire.prune(live) + idleCount, aged := idleRetirementLocalPlan(w.runners, w.scaleSet.MinIdleRunners, now) + if len(aged) == 0 { + return nil + } + cli, err := w.GetScaleSetClient() + if err != nil { + w.resetIdleObservations(aged) + return fmt.Errorf("getting scale set client: %w", err) + } + readCtx, cancel := context.WithTimeout(w.ctx, idleRetirementReadTimeout) + defer cancel() + remote, err := cli.GetRunnerScaleSetByID(readCtx, w.scaleSet.ScaleSetID) + if err != nil { + w.resetIdleObservations(aged) + return fmt.Errorf("read scale set for idle retirement: %w", err) + } + if remote.ID != w.scaleSet.ScaleSetID || remote.Name != w.scaleSet.Name || + remote.Enabled == nil || !*remote.Enabled || remote.Statistics == nil { + w.resetIdleObservations(aged) + return fmt.Errorf("idle retirement scale-set identity, enabled state or statistics missing") + } + + for _, runner := range aged { + actionsRunner, err := cli.GetRunner(readCtx, runner.AgentID) + if err != nil { + w.idleRetire.forget(runner.AgentID) + if errors.Is(err, runnerErrors.ErrNotFound) { + continue + } + slog.WarnContext(w.ctx, "idle retirement skipped candidate after Actions read failure", "runner_name", runner.Name, "agent_id", runner.AgentID, "error", err, "error_class", "idle-retirement") + continue + } + restRunner, err := w.restRunner(readCtx, runner.AgentID) + if err != nil { + w.idleRetire.forget(runner.AgentID) + slog.WarnContext(w.ctx, "idle retirement skipped candidate after REST read failure", "runner_name", runner.Name, "agent_id", runner.AgentID, "error", err, "error_class", "idle-retirement") + continue + } + if restRunner == nil { + w.idleRetire.forget(runner.AgentID) + slog.WarnContext(w.ctx, "idle retirement skipped candidate after empty REST payload", "runner_name", runner.Name, "agent_id", runner.AgentID, "error_class", "idle-retirement") + continue + } + actionsListBusyUntrusted(actionsRunner.Busy) + evidence := idleRetirementEvidence{ + AgentID: runner.AgentID, + Name: runner.Name, + ScaleSetID: w.scaleSet.ScaleSetID, + LocalStatus: runner.RunnerStatus, + CreatedAt: runner.CreatedAt, + MinIdle: w.scaleSet.MinIdleRunners, + IdleCount: idleCount, + Now: now, + RESTID: restRunner.GetID(), + RESTName: restRunner.GetName(), + RESTStatus: restRunner.GetStatus(), + RESTBusy: restRunner.Busy, + ActionsID: actionsRunner.ID, + ActionsName: actionsRunner.Name, + ActionsScaleSetID: actionsRunner.RunnerScaleSetID, + ActionsEphemeral: actionsRunner.Ephemeral, + ActionsEnabled: actionsRunner.Enabled, + ActionsState: actionsRunner.ProvisioningState, + ActionsStatus: actionsStatusString(actionsRunner.Status), + AssignedJobs: remote.Statistics.TotalAssignedJobs, + AcquiredJobs: remote.Statistics.TotalAcquiredJobs, + AvailableJobs: remote.Statistics.TotalAvailableJobs, + RunningJobs: remote.Statistics.TotalRunningJobs, + BusyRunners: remote.Statistics.TotalBusyRunners, + StatisticsPresent: true, + } + decision := w.idleRetire.confirm(evidence) + if !decision.Eligible { + slog.DebugContext(w.ctx, "idle retirement refused", "runner_name", runner.Name, "reason", decision.Reason) + continue + } + if !decision.Ready { + slog.InfoContext(w.ctx, "idle retirement first observation", "runner_name", runner.Name, "agent_id", runner.AgentID, "reason", decision.Reason) + continue + } + if ok := locking.TryLock(runner.Name, w.consumerID); !ok { + continue + } + defer locking.Unlock(runner.Name, false) + if err := w.removeIdleRunnerAfterConfirmation(cli, runner); err != nil { + w.idleRetire.forget(runner.AgentID) + return err + } + w.idleRetire.forget(runner.AgentID) + return nil + } + return nil +} + +func (w *Worker) resetIdleObservations(runners []params.Instance) { + for _, runner := range runners { + w.idleRetire.forget(runner.AgentID) + } +} + +func applyConfirmedIdleRemoval( + ctx context.Context, + cli idleActionsClient, + runner params.Instance, + markAbsent func(params.Instance) error, +) (idleRemoveOutcome, error) { + if markAbsent == nil { + return idleRemoveNone, fmt.Errorf("idle pending-delete writer is required") + } + outcome, err := applyIdleRemoveRunner(ctx, cli, runner.AgentID) + if err != nil { + return outcome, err + } + if outcome == idleRemoveMarkAbsent { + return outcome, markAbsent(runner) + } + return outcome, nil +} + +func (w *Worker) removeIdleRunnerAfterConfirmation(cli idleActionsClient, runner params.Instance) error { + readCtx, cancel := context.WithTimeout(w.ctx, idleRetirementReadTimeout) + defer cancel() + outcome, err := applyConfirmedIdleRemoval(readCtx, cli, runner, w.markIdleRunnerAbsent) + if err != nil { + return err + } + switch outcome { + case idleRemoveConflict: + slog.InfoContext(w.ctx, "idle retirement refused; Actions service reported conflict", "runner_name", runner.Name, "agent_id", runner.AgentID) + w.idleRetire.forget(runner.AgentID) + case idleRemoveMarkAbsent: + slog.InfoContext(w.ctx, "retired excess idle runner from Actions service", "runner_name", runner.Name, "agent_id", runner.AgentID) + } + return nil +} + +func (w *Worker) markIdleRunnerAbsent(runner params.Instance) error { + instance, err := w.setRunnerDBStatus(runner.Name, commonParams.InstancePendingDelete) + if err != nil { + if errors.Is(err, runnerErrors.ErrNotFound) { + return nil + } + terminal, refreshErr := w.runnerRemovalAlreadyOwned(runner.Name) + if refreshErr != nil { + return errors.Join(err, refreshErr) + } + if terminal { + return nil + } + return err + } + w.runners[instance.ID] = instance + return nil +} + +func isConflictError(err error) bool { + if err == nil { + return false + } + message := strings.ToLower(err.Error()) + return strings.Contains(message, "conflict") || strings.Contains(message, "jobstillrunningexception") +} diff --git a/third_party/garm/overlay/workers/scaleset/idle_online_retire_test.go b/third_party/garm/overlay/workers/scaleset/idle_online_retire_test.go new file mode 100644 index 00000000..ede342fc --- /dev/null +++ b/third_party/garm/overlay/workers/scaleset/idle_online_retire_test.go @@ -0,0 +1,390 @@ +package scaleset + +import ( + "context" + "errors" + "testing" + "time" + + runnerErrors "github.com/cloudbase/garm-provider-common/errors" + commonParams "github.com/cloudbase/garm-provider-common/params" + "github.com/cloudbase/garm/params" +) + +func TestActionsListBusyDefaultIsUntrusted(t *testing.T) { + t.Parallel() + var encoded params.RunnerReference + if encoded.Busy { + t.Fatal("zero RunnerReference.Busy must stay false so omitted JSON cannot look busy") + } + actionsListBusyUntrusted(encoded.Busy) +} + +func TestRestBusyOmittedIsUnknown(t *testing.T) { + t.Parallel() + if restBusyFromPointer(nil) != restBusyUnknown { + t.Fatal("omitted REST busy must be unknown, not false") + } + busy := false + if restBusyFromPointer(&busy) != restBusyFalse { + t.Fatal("explicit false must be distinct from omitted") + } + busy = true + if restBusyFromPointer(&busy) != restBusyTrue { + t.Fatal("explicit true must refuse retirement") + } +} + +func baseEvidence(now time.Time) idleRetirementEvidence { + busy := false + return idleRetirementEvidence{ + AgentID: 42, + Name: "example-runner", + ScaleSetID: 7, + LocalStatus: params.RunnerPending, + CreatedAt: now.Add(-45 * time.Minute), + MinIdle: 0, + IdleCount: 4, + Now: now, + RESTID: 42, + RESTName: "example-runner", + RESTStatus: "online", + RESTBusy: &busy, + ActionsID: 42, + ActionsName: "example-runner", + ActionsScaleSetID: 7, + ActionsEphemeral: true, + ActionsEnabled: true, + ActionsState: "Provisioned", + ActionsStatus: "online", + StatisticsPresent: true, + } +} + +func TestEvaluateIdleRetirementPositive(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + got := evaluateIdleRetirement(baseEvidence(now)) + if !got.Eligible || got.Reason != "eligible" { + t.Fatalf("got %#v", got) + } +} + +func TestEvaluateIdleRetirementRefusesOmittedBusy(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + ev := baseEvidence(now) + ev.RESTBusy = nil + got := evaluateIdleRetirement(ev) + if got.Eligible || got.Reason != "rest-busy-omitted" { + t.Fatalf("got %#v", got) + } +} + +func TestEvaluateIdleRetirementRefusesBusyTrue(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + ev := baseEvidence(now) + busy := true + ev.RESTBusy = &busy + got := evaluateIdleRetirement(ev) + if got.Eligible || got.Reason != "rest-busy" { + t.Fatalf("got %#v", got) + } +} + +func TestEvaluateIdleRetirementStatsZeroIsNotABlocker(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + ev := baseEvidence(now) + ev.AssignedJobs, ev.AcquiredJobs, ev.AvailableJobs, ev.RunningJobs = 0, 0, 0, 0 + got := evaluateIdleRetirement(ev) + if !got.Eligible { + t.Fatalf("all-zero statistics must not prove demand and must not block REST-idle retirement: %#v", got) + } +} + +func TestEvaluateIdleRetirementRefusesPositiveDemand(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + ev := baseEvidence(now) + ev.AssignedJobs = 1 + got := evaluateIdleRetirement(ev) + if got.Eligible || got.Reason != "scale-set-has-demand" { + t.Fatalf("got %#v", got) + } +} + +func TestEvaluateIdleRetirementRefusesIdentityChange(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + ev := baseEvidence(now) + ev.RESTName = "example-other" + got := evaluateIdleRetirement(ev) + if got.Eligible || got.Reason != "identity-mismatch" { + t.Fatalf("got %#v", got) + } +} + +func TestEvaluateIdleRetirementRespectsMinIdle(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + ev := baseEvidence(now) + ev.MinIdle = 4 + ev.IdleCount = 4 + got := evaluateIdleRetirement(ev) + if got.Eligible || got.Reason != "at-or-below-min-idle" { + t.Fatalf("got %#v", got) + } +} + +func TestIdleRetirementGateRequiresTwoObservations(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + var gate idleRetirementGate + first := gate.confirm(baseEvidence(now)) + if first.Ready || first.Reason != "first-observation" { + t.Fatalf("first %#v", first) + } + tooSoon := gate.confirm(baseEvidence(now.Add(10 * time.Second))) + if tooSoon.Ready || tooSoon.Reason != "confirmation-pending" { + t.Fatalf("too soon %#v", tooSoon) + } + ready := gate.confirm(baseEvidence(now.Add(idleRetirementConfirmAfter))) + if !ready.Ready || ready.Reason != "confirmed" { + t.Fatalf("confirmed %#v", ready) + } +} + +func TestIdleRetirementGateDropsChangedIdentity(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + var gate idleRetirementGate + gate.confirm(baseEvidence(now)) + changed := baseEvidence(now.Add(time.Minute)) + changed.RESTName = "example-replaced" + changed.Name = "example-replaced" + changed.ActionsName = "example-replaced" + got := gate.confirm(changed) + if got.Ready || got.Reason != "first-observation" { + t.Fatalf("replacement must restart observation: %#v", got) + } +} + +func TestIsConflictError(t *testing.T) { + t.Parallel() + if !isConflictError(errString("conflict while calling agents/42: JobStillRunningException")) { + t.Fatal("JobStillRunningException must refuse retirement") + } + if isConflictError(errString("not found")) { + t.Fatal("not-found is not a busy conflict") + } +} + +func TestIdleRetirementLocalPlanSkipsNetworkWhenNothingIsAged(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + runners := map[string]params.Instance{ + "example-young": { + Name: "example-young", AgentID: 7, Status: commonParams.InstanceRunning, + RunnerStatus: params.RunnerIdle, CreatedAt: now.Add(-time.Minute), + }, + "example-creating": { + Name: "example-creating", AgentID: 8, Status: commonParams.InstanceCreating, + RunnerStatus: params.RunnerPending, CreatedAt: now.Add(-time.Hour), + }, + } + idleCount, aged := idleRetirementLocalPlan(runners, 0, now) + if idleCount != 1 || len(aged) != 0 { + t.Fatalf("idleCount=%d aged=%d", idleCount, len(aged)) + } +} + +func TestIdleRetirementGatePrunesUnseenIDs(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + var gate idleRetirementGate + gate.confirm(baseEvidence(now)) + gone := baseEvidence(now) + gone.AgentID = 99 + gone.RESTID = 99 + gone.ActionsID = 99 + gate.confirm(gone) + gate.prune(map[int64]struct{}{42: {}}) + if _, ok := gate.seen[99]; ok { + t.Fatal("unseen agent ID must not keep a first observation") + } + if _, ok := gate.seen[42]; !ok { + t.Fatal("live agent ID must keep its first observation") + } +} + +func TestIdleRetirementReadErrorResetsConfirmation(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + var gate idleRetirementGate + gate.confirm(baseEvidence(now)) + gate.forget(42) + again := gate.confirm(baseEvidence(now.Add(time.Minute))) + if again.Ready || again.Reason != "first-observation" { + t.Fatalf("failed freshness must not keep the old first observation: %#v", again) + } +} + +func TestWarnIdleRetirementDoesNotReturn(t *testing.T) { + t.Parallel() + // Helper evidence only: production consolidateRunnerState calls + // warnIdleRetirement then keeps the original body. This does not construct + // a Worker or exercise DB/provider consolidation. + warnIdleRetirement(context.Background(), errors.New("Actions GetRunner 500")) + warnIdleRetirement(context.Background(), nil) +} + +func TestRetireExcessIdleCapacityNoAgedCandidatesMakesNoClientCall(t *testing.T) { + now := time.Now().UTC() + w := &Worker{ + ctx: context.Background(), + scaleSet: params.ScaleSet{MinIdleRunners: 0, ScaleSetID: 7, Name: "example-integration"}, + runners: map[string]params.Instance{ + "example-young": { + Name: "example-young", AgentID: 7, Status: commonParams.InstanceRunning, + RunnerStatus: params.RunnerIdle, CreatedAt: now.Add(-time.Minute), + }, + }, + } + w.idleRetire.seen = map[int64]idleObservation{99: {AgentID: 99, Name: "example-gone", ScaleSetID: 7, FirstSeen: now.Add(-time.Hour)}} + if err := w.retireExcessIdleCapacity(); err != nil { + t.Fatalf("empty aged-candidate path must not call GitHub/Actions clients: %v", err) + } + if _, ok := w.idleRetire.seen[99]; ok { + t.Fatal("unseen observation must be pruned without a network call") + } +} + +func TestDecideIdleRemoveRunnerHTTPSequence(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + removeErr error + getErr error + getFound bool + want idleRemoveOutcome + wantErr bool + }{ + {name: "204-then-404", getErr: runnerErrors.ErrNotFound, want: idleRemoveMarkAbsent}, + {name: "remove-already-404", removeErr: runnerErrors.ErrNotFound, want: idleRemoveMarkAbsent}, + {name: "409-job-still-running", removeErr: errString("conflict: JobStillRunningException"), want: idleRemoveConflict}, + {name: "unexpected-200", removeErr: errString("removing runner 42: unexpected status 200"), wantErr: true}, + {name: "remove-500", removeErr: errString("removing runner 42: unexpected status 500"), wantErr: true}, + {name: "missing-readback", getErr: errors.New("timeout"), wantErr: true}, + {name: "non-404-readback-still-present", getFound: true, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := decideIdleRemoveRunner(tc.removeErr, tc.getErr, tc.getFound) + if tc.wantErr { + if err == nil || got != idleRemoveNone { + t.Fatalf("got %s err=%v", got, err) + } + return + } + if err != nil || got != tc.want { + t.Fatalf("got %s err=%v want %s", got, err, tc.want) + } + }) + } +} + +type fakeIdleActions struct { + removeErr error + getErr error + removed int + got int +} + +func (f *fakeIdleActions) RemoveRunner(context.Context, int64) error { + f.removed++ + return f.removeErr +} + +func (f *fakeIdleActions) GetRunner(context.Context, int64) (params.RunnerReference, error) { + f.got++ + if f.getErr != nil { + return params.RunnerReference{}, f.getErr + } + return params.RunnerReference{ID: 42, Name: "example-runner"}, nil +} + +func TestApplyIdleRemoveRunnerWires204And404Once(t *testing.T) { + t.Parallel() + cli := &fakeIdleActions{getErr: runnerErrors.ErrNotFound} + got, err := applyIdleRemoveRunner(context.Background(), cli, 42) + if err != nil || got != idleRemoveMarkAbsent || cli.removed != 1 || cli.got != 1 { + t.Fatalf("got %s err=%v removed=%d got=%d", got, err, cli.removed, cli.got) + } +} + +func TestApplyIdleRemoveRunnerConflictDoesNotReadBack(t *testing.T) { + t.Parallel() + cli := &fakeIdleActions{removeErr: errString("conflict: JobStillRunningException")} + got, err := applyIdleRemoveRunner(context.Background(), cli, 42) + if err != nil || got != idleRemoveConflict || cli.removed != 1 || cli.got != 0 { + t.Fatalf("got %s err=%v removed=%d got=%d", got, err, cli.removed, cli.got) + } +} + +func TestApplyConfirmedIdleRemovalPendingDeleteOnce(t *testing.T) { + // Helper evidence only: fake Actions client plus injected writer, not a + // Worker, database, or live HTTP transport. + var marked []string + runner := params.Instance{Name: "example-runner", AgentID: 42} + cli := &fakeIdleActions{getErr: runnerErrors.ErrNotFound} + outcome, err := applyConfirmedIdleRemoval(context.Background(), cli, runner, func(got params.Instance) error { + marked = append(marked, got.Name) + return nil + }) + if err != nil || outcome != idleRemoveMarkAbsent || len(marked) != 1 || marked[0] != "example-runner" || cli.removed != 1 || cli.got != 1 { + t.Fatalf("outcome=%s err=%v marked=%v removed=%d got=%d", outcome, err, marked, cli.removed, cli.got) + } +} + +func TestApplyConfirmedIdleRemovalConflictDoesNotPendingDelete(t *testing.T) { + var marked []string + cli := &fakeIdleActions{removeErr: errString("conflict: JobStillRunningException")} + outcome, err := applyConfirmedIdleRemoval(context.Background(), cli, params.Instance{Name: "example-runner", AgentID: 42}, func(got params.Instance) error { + marked = append(marked, got.Name) + return nil + }) + if err != nil || outcome != idleRemoveConflict || len(marked) != 0 || cli.got != 0 { + t.Fatalf("conflict must not pending-delete: outcome=%s err=%v marked=%v got=%d", outcome, err, marked, cli.got) + } +} + +func TestApplyConfirmedIdleRemovalRefusesUnexpectedStatus(t *testing.T) { + for _, tc := range []struct { + name string + removeErr error + getErr error + }{ + {name: "unexpected-200", removeErr: errString("removing runner 42: unexpected status 200")}, + {name: "remove-500", removeErr: errString("removing runner 42: unexpected status 500")}, + {name: "non-404-readback", getErr: errors.New("timeout")}, + {name: "still-present", getErr: nil}, + } { + t.Run(tc.name, func(t *testing.T) { + var marked []string + cli := &fakeIdleActions{removeErr: tc.removeErr, getErr: tc.getErr} + _, err := applyConfirmedIdleRemoval(context.Background(), cli, params.Instance{Name: "example-runner", AgentID: 42}, func(got params.Instance) error { + marked = append(marked, got.Name) + return nil + }) + if err == nil || len(marked) != 0 { + t.Fatalf("err=%v marked=%v", err, marked) + } + }) + } +} + +type errString string + +func (e errString) Error() string { return string(e) } diff --git a/third_party/garm/patches/0031-retire-excess-idle-online-runners.patch b/third_party/garm/patches/0031-retire-excess-idle-online-runners.patch new file mode 100644 index 00000000..84816b41 --- /dev/null +++ b/third_party/garm/patches/0031-retire-excess-idle-online-runners.patch @@ -0,0 +1,36 @@ +diff --git a/util/github/scalesets/runners.go b/util/github/scalesets/runners.go +index e467049..423eee2 100644 +--- a/util/github/scalesets/runners.go ++++ b/util/github/scalesets/runners.go +@@ -183,7 +183,9 @@ func (s *ScaleSetClient) RemoveRunner(ctx context.Context, runnerID int64) (err + if err != nil { + return fmt.Errorf("request failed for %s: %w", req.URL.String(), err) + } +- +- resp.Body.Close() ++ defer resp.Body.Close() ++ if resp.StatusCode != http.StatusNoContent { ++ return fmt.Errorf("removing runner %d: unexpected status %d", runnerID, resp.StatusCode) ++ } + return nil + } +diff --git a/workers/scaleset/scaleset.go b/workers/scaleset/scaleset.go +index dd6a316..9883ec9 100644 +--- a/workers/scaleset/scaleset.go ++++ b/workers/scaleset/scaleset.go +@@ -90,6 +90,7 @@ type Worker struct { + // reconciliation safety net. + autoscaleWake chan struct{} + demandGate confirmedDemandGate ++ idleRetire idleRetirementGate + } + + func (w *Worker) ensureScaleSetInGitHub() error { +@@ -489,6 +490,7 @@ func (w *Worker) consolidateRunnerState(runners []params.RunnerReference) error + return fmt.Errorf("getting scale set client: %w", err) + } + dbRunnersByName := w.runnerByName() ++ warnIdleRetirement(w.ctx, w.retireExcessIdleCapacity()) + // Cross check what exists in github with what we have in the database. + for name, runner := range ghRunnersByName { + status := runner.GetStatus()