Repository navigation
fix(controller): migrate to scaleset 0.5.0 - #156
Conversation
Bumps the routine-go group in /controller with 1 update: [github.com/actions/scaleset](https://github.com/actions/scaleset). Updates `github.com/actions/scaleset` from 0.4.0 to 0.5.0 - [Release notes](https://github.com/actions/scaleset/releases) - [Commits](actions/scaleset@v0.4.0...v0.5.0) --- updated-dependencies: - dependency-name: github.com/actions/scaleset dependency-version: 0.5.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: routine-go ... Signed-off-by: dependabot[bot] <support@github.com>
Nickfost
left a comment
There was a problem hiding this comment.
The v0.5.0 listener.Scaler interface requires Scale(context.Context, *scaleset.RunnerScaleSetMessage) error. The controller and its lifecycle tests still pass *Scaler with only the old callbacks, so this dependency-only diff cannot compile. The migration must also own job acquisition, handle initial/nil polls, and preserve cleanup/cancellation while accommodating ack-after-success and replay. Update docs/CONTROLLER-DESIGN.md and regression coverage. Upstream go.mod requires Go 1.27.3; first resolve #153's reviewed builder update. The earlier failed run stopped at title validation and supplies no build evidence. Keep the update open for that focused migration and a full required-check result.
Nickfost
left a comment
There was a problem hiding this comment.
The earlier source-compatibility findings are addressed in ce5f606. It implements Scale, session job acquisition, retained idle-poll demand and cancellation-safe admitted batches, and retains runner tracking until cleanup succeeds. Replay tests cover both a partially failed batch and a failed acknowledgment. Controller race tests, vet, module verification and amd64/arm64 compilation passed locally. #153's Go 1.27.3 prerequisite is included by ordinary merge with implicit toolchain downloads disabled. This is now my implementation, so independent review remains required. I am not granting approval or treating local tests as the absent required GitHub build check; the owner-controlled queue and the workflow-activation authorization gate remain explicit.
|
@codex review Please independently review final SOURCE Audit the actions/scaleset 0.5.0 Scale/session API migration, acquisition before callbacks, initial/nil polls and retained demand, cancellation admission and admitted-batch cleanup, ack-after-success and replay after partial cleanup or acknowledgment failure, retained runner tracking until confirmed removal, strict late/unassigned-event handling, and concurrent exit/completion cleanup. Check the module/transitive checksums, merged Go 1.27.3 prerequisite, GOTOOLCHAIN=local and unchanged runtime/credential boundaries. Check the regression tests and controller/image documentation. The historical changes-requested review on abdf455 described the missing migration; this final source implements it, but has no independent current-head verdict yet. Fresh source/blob assertions, 46 Markdown link checks, whitespace, five-commit/title policy and all five proposed-tree secret scans passed. Reused ce5f606 evidence includes the full controller race test package, focused lifecycle/message tests, vet, module verification and Linux amd64/arm64 compilation. Controller tree The execution owner retains the supported isolated-native required-check route, exclusive VM admission, one complete supervised run/export/cleanup and eligible merge. Review findings only; no branch/source changes or workflow/host action. No independent review or review request existed for this final head when checked, so this is the single current-head request. Cursor Bugbot is unavailable and is not requested. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This was my review of dependency-only abdf455, whose missing Scale/session migration and Go prerequisite have been implemented. Final source 74e504e now has independent Codex code review completed without findings at 2026-11-01T05:43:59.311369Z, with no unresolved threads: #156 (comment) and #156 (comment). Dismissing only this obsolete review; this is not self-approval of the implementation. The required exact-head Build without registering a runner check remains enforced and pending with the execution owner.
Migrates the controller to the actions/scaleset 0.5.0 listener API. The dependency-only update could not compile against the controller's previous callbacks. The controller now owns batch acquisition and processing through Scale, while the SDK owns polling and acknowledgment.
Available jobs are acquired before start/completion callbacks. Empty polls retain the latest assigned-job count for reconciliation. A canceled context prevents admission; an admitted real batch finishes acquisition, callbacks, cleanup and reconciliation without caller cancellation, preserving the previous cleanup behavior. The SDK acknowledges only successful batches. Cleanup failure keeps runner tracking, so a replay can retry it. Earlier successful effects may repeat after partial-batch or acknowledgment failure. Unknown events and uncertain Docker results still fail.
Merged #153 already supplies the pinned Go 1.27.3 builder on BASE. This PR aligns the module minimum with SDK 0.5.0 and sets GOTOOLCHAIN=local in the build stage to prevent implicit compiler downloads. The test-only SDK retry option uses its replacement API. Base-image digests, the Docker SDK pin, runner inputs, package pins, build flags and workflow bytes retain BASE's selections.
Frozen source
SOURCE: 74e504e
BASE: 8629dfd
BASE TREE: f0ceeb0d578c1069224a7662d7786238efb98b5c
TREE: ddfc10532a09d22295138e4508ef536bf837bfe3
Ordered source parents:
The ordinary merge had no conflicts. Every proposed change blob exactly matches the prepared migration. The only changes from prepared ce5f606 are the three accepted #159 files: docs/EXPLICIT-DRAINED-RECOVERY.md, scripts/remote-reconcile.sh and scripts/test_remote_reconcile.py. The updated #159 guidance is retained exactly.
Exact scope against BASE
15 files, 446 additions and 107 deletions:
The source changes implement the listener/session callback and safe replay/cleanup bookkeeping. Tests cover acquisition ordering/failure, partial cleanup replay, acknowledgment replay, cancellation during admitted cleanup, pre-admission cancellation, retained demand on empty polls, and existing late/duplicate/unassigned events. Module versions and checksums remain pinned.
Focused verification
Fresh checks passed:
Reusable preparation evidence on ce5f606: the controller Go test package passed with race detection and a five-minute test timeout; focused lifecycle/message tests, go vet and go mod verify passed; Linux amd64 and arm64 compilation passed. The entire controller tree remains a5adbd629c3e71d5eff31e3957806ef097bb92f2. The three inherited #159 files do not change those test inputs. This refresh does not claim a new complete engine result, native arm64 execution or managed-image acceptance.
Independent review and execution
Independent Codex code review completed without findings on full SOURCE 74e504e at 2026-11-01T05:43:59.311369Z. The dashboard records the full commit and no findings; the receipt resolves to this same head. There are no unresolved inline threads. This is the established independent code-review evidence; no separate completed security review or formal self-approval is claimed.
My historical changes-requested review on dependency-only abdf455 was dismissed after the independent final-head review confirmed that its callback/toolchain findings were addressed. The required build check remains enforced and passed on the frozen source in run 41278578534 attempt 1. No Cursor Bugbot review was requested, retried or relied upon.
The execution owner retains the already-authorized isolated-native required-check producer used for #159 and #153, all VM admission controls, the one complete supervised run, export/verification, cleanup and eligible merge. This thread does not enable workflows, dispatch validation, prepare/use the VM or touch hosts. No additional human approval is requested. Required Build without registering a runner evidence from GitHub Actions app 15368 passed on this exact SOURCE in run 41278578534.
The completed #153 and private #104 prerequisites do not grant a general slot release. Later #152 and #155 and all held operations remain held. SOURCE and BASE remained frozen throughout the owner's single complete run. Its sole trigger was the owner-controlled PR-description edit under the established isolated producer route.
Documentation and rollout
docs/CONTROLLER-DESIGN.md documents acquisition, acknowledgment, replay, cancellation and the Go minimum. docs/MANAGED-IMAGES.md aligns module and builder requirements. The historical upstream-example NOTICE is retained. No visual experience changes.
The callback is internal to the engine. No schema, capability, initializer, image-catalog format or operator activation contract changes, so no standalone-template follow-up issue is needed at this scope. Engine/template/private/runtime pin promotion and live-host acceptance remain separate; this source preparation performs none.
Admitted native required-check run
The execution owner admitted the single complete supervised required-check run on the frozen SOURCE and BASE above after exact-head independent review. It uses the existing unchanged workflow on scoped one-job validation runners. The trigger changes only this PR description. No additional full local run, source change, pin promotion or host operation is authorized by this entry. The workflow and temporary runner selection will return to their previous settings after the build job is assigned. Complete results and cleanup remain required before merge.
Complete required-run acceptance
Run
41278578534, attempt 1, completed successfully on this unchanged SOURCE/BASE. Both the convention and required build jobs passed on guarded one-job validation runners. All eight exact-source full-suite stages passed with no expected or actual test skips. The official nine-file artifact and all eight stage hashes were independently verified. Artifact ZIP SHA256:442887abdddb7af736de27d396a6ebe30d53231f318bad2e53770c81d0ff8965.The complete Actions logs and remaining runner diagnostics were exported outside Git. Normal runner temp cleanup removed the worker's result copy after upload, so no independent worker-byte comparison is claimed. The official artifact is complete and verified.
Post-run cleanup removed temporary JIT inputs/grants/registrations and restored the accepted executor689 through its supported preparation path. Final worker admission passed with no active validation processes/cgroups or containers; all four retained840 image proofs and operator access were preserved. The workflow is disabled again and its temporary runner variable is absent. No budget, capacity, live engine/private pin or host operation changed. Existing reviews remain valid; no duplicate validation or Bugbot request was made.