fix: keep candidate builds outside upgrade transactions - #85
Conversation
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f972e9884
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| return | ||
| fi | ||
| install_release | ||
| build_candidate |
There was a problem hiding this comment.
During a configuration-only or same-engine repair, the candidate and installed environments both use ci-fleet-runner:<engine-prefix> (scripts/desired_state.py:385-404). Building before the controller is paused can therefore replace the tag that the live controller resolves when creating new runners; because the runner Dockerfile installs mutable base/apt content, the rebuilt image is not guaranteed to be byte-identical. If the runner build completes and the controller build then fails, reconciliation reports a pre-transaction failure while the old controller remains running and dispatches subsequent jobs into that newly tagged image. Build under a unique candidate tag and promote it after draining, or retain the old ordering when the candidate tag matches the installed tag.
AGENTS.md reference: AGENTS.md:L39-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. The live controller resolves new runners by the shared image tag, so a partially successful pre-drain build can change subsequent jobs. This needs a separate candidate-image promotion design. I am leaving the thread unresolved and the PR unmerged.
There was a problem hiding this comment.
Valid. Current head checks both installed and live runner-image tags before any pre-drain build and defers a shared-tag rebuild until after drain. Disposition applies to exact PR head 524b5f44a401.
| return | ||
| fi | ||
| install_release | ||
| build_candidate |
There was a problem hiding this comment.
Moving the build here leaves the host-enrollment contract describing the opposite sequence: docs/ADDING-A-HOST.md:105 says the installer creates a checkpoint before building images, and docs/DESIRED-STATE.md:139-141 says it checkpoints and drains before managed preflight/build. That ordering determines whether a failed build can leave Docker artifacts and whether builds overlap active jobs, so operators now receive an inaccurate failure and downtime model; update these lifecycle documents if the new order is retained.
AGENTS.md reference: AGENTS.md:L18-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. These lifecycle descriptions still state checkpoint and drain before build and now conflict with the implementation. I am leaving the thread unresolved and the PR unmerged for a follow-up correction.
There was a problem hiding this comment.
Valid. Current head updates the operator lifecycle documentation to match the implemented build, checkpoint, drain, activation, and rollback order. Disposition applies to exact PR head 524b5f44a401.
| # One hour for the installer lock, plus one hour for a cold build and rollback. | ||
| TimeoutStartSec=2h |
There was a problem hiding this comment.
remote-reconcile.sh may spend the permitted 3600 seconds waiting for the installer lock, while the new comment allocates the remaining hour to both an unbounded cold build and rollback. If that second hour expires after transaction_active=true—for example during the five-minute drain or activation after a near-hour build—systemd terminates ExecStart, but the installer traps only ERR, not TERM, so restore_checkpoint is not run and the controller can remain paused, stopped, or partially activated. Add termination handling that restores an active checkpoint, or enforce internal phase deadlines that guarantee the transaction and rollback finish before this service timeout.
AGENTS.md reference: AGENTS.md:L57-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. The installer has an ERR rollback trap but no TERM rollback path, so the service timeout can interrupt an active transaction without checkpoint restoration. This changes the transaction failure model. I am leaving the thread unresolved and the PR unmerged.
There was a problem hiding this comment.
Valid. Current head traps TERM during an active transaction, targets the direct installer child, restores the checkpoint, and preserves exit status 143. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b980f677b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| return | ||
| fi | ||
| install_release | ||
| build_candidate |
There was a problem hiding this comment.
When a host already violates a preflight condition unrelated to active containers—such as a missing PEM, incorrect socket GID, or Docker filesystem usage at or above 80%—this now runs the potentially long and disk-intensive image build first, then creates a checkpoint and drains the live controller before run_candidate_preflight rejects the host at line 1077 and rolls back. Split the active-managed-container check from the other preflight checks so the static host checks can still fail before the build and transaction while the container check runs after draining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Invalid for the cited cases. require_commands rejects Docker disk use at or above 80% before perform_converge, verify_host_files rejects a missing or invalid PEM before build, and render_candidate derives CI_FLEET_DOCKER_GID from the live socket through docker_gid. The required correction keeps run_candidate_preflight intact after drain.
There was a problem hiding this comment.
Invalid for the cited cases. The installer rejects excessive Docker usage and invalid host files before converge, and derives the socket GID before build. The managed-container preflight remains after drain. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e1488b861
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| local status=$? | ||
| rollback_and_exit "$status" | ||
| } | ||
| on_term() { rollback_and_exit 143; } |
There was a problem hiding this comment.
When TimeoutStartSec sends TERM while drain_current is still waiting for managed runners, the controller has already been paused. on_term calls restore_checkpoint, whose try_drain_current sees the paused container's status as running and invokes docker compose pause again; Moby rejects that operation with “container is already paused.” Restoration therefore returns before unpausing, and rollback_and_exit ignores the failure, leaving the old controller paused after the reconcile exits. The added regression test only signals after DRAIN_OK, so it does not cover this timeout phase; restoration needs to recognize and recover an already-paused controller.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head recognizes an already-paused controller during rollback and unpauses it before stop and restoration. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4f05168b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| && installed_runner_image=$(awk -F= '$1 == "CI_FLEET_RUNNER_IMAGE" {count++; value=substr($0, index($0, "=") + 1)} END {if (count != 1) exit 1; print value}' "$rendered_env"); then | ||
| [[ "$candidate_runner_image" == "$installed_runner_image" ]] || build_before_drain=true |
There was a problem hiding this comment.
When repairing runtime drift where the running controller's CI_FLEET_RUNNER_IMAGE differs from the trusted rendered environment, the candidate tag can differ from installed_runner_image yet equal the tag actually used by the live controller. This sets build_before_drain=true, so line 1098 rebuilds and retags the image while that controller can still create runners from it; controller_environment_matches explicitly recognizes such live-environment divergence as repairable drift. Inspect the running container's runner-image value and defer the build whenever either installed or live state uses the candidate tag.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head checks the running controller's runner-image tag as well as installed state before allowing a pre-drain build. Disposition applies to exact PR head 524b5f44a401.
| fi | ||
| ;; | ||
| esac | ||
| if $build_before_drain; then build_candidate; fi |
There was a problem hiding this comment.
When Docker usage starts just below 80%, a successful distinct-tag build can add enough layers to cross the enforced threshold, but the next disk check is run_candidate_preflight after checkpoint and drain. The installer therefore stops a healthy controller only to reject the candidate and roll back, defeating the pre-transaction build's downtime isolation. Fresh evidence beyond the earlier preflight thread is that the newly moved build itself can change utilization after require_commands performed its initial check; rerun the static capacity check after this early build and before make_checkpoint.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head reruns the Docker capacity check after an early build and before checkpoint or drain. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84fe237d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| candidate_runner_image=$(awk -F= '$1 == "CI_FLEET_RUNNER_IMAGE" {count++; value=substr($0, index($0, "=") + 1)} END {if (count != 1) exit 1; print value}' "$candidate_env") || die 'rendered candidate runner image is invalid' | ||
| [[ "$testing" != 1 ]] || expected_owner=$(id -u) | ||
| case "$existing_status" in | ||
| ''|exited|created|dead) build_before_drain=true ;; |
There was a problem hiding this comment.
When a same-engine repair observes the controller as exited, created, or dead, this unconditionally enables the pre-drain build without comparing the candidate tag to the installed tag. The controller uses restart: unless-stopped, and the installer lock does not prevent Docker or an operator from starting it while drift_count, release installation, and the build run; the build can then retag the image from which the newly running controller creates runners. Fresh evidence beyond the earlier live-tag finding is this non-running branch, which bypasses the installed-tag safeguard entirely; defer an installed-tag build regardless of the transient container status, or otherwise prevent/recheck restart before building.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head defers an installed-tag rebuild regardless of transient controller status and rechecks restartable live state before building. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 545a1313d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if [[ $(docker inspect --format '{{.State.Paused}}' "$controller_container" 2>/dev/null || true) == true ]]; then | ||
| paused=true | ||
| elif compose "$old_release" "$drain_env" pause controller >/dev/null; then |
There was a problem hiding this comment.
When TimeoutStartSec sends TERM while drain_current is waiting for runners, Docker reports the container’s .State.Status as paused (Engine API status values), so rollback enters the wildcard branch and attempts docker compose stop before reaching this new .State.Paused check; Docker’s stop implementation rejects paused containers, causing ROLLBACK_FAILED and leaving the controller paused. Fresh evidence beyond the earlier comment is that this check is unreachable for Docker’s real paused status, whereas the fake Docker test continues reporting running; handle paused explicitly and unpause it before stopping.
AGENTS.md reference: AGENTS.md:L57-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head handles Docker's actual paused status explicitly and unpauses before attempting to stop the controller. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3621b5ae9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| fi | ||
| controller_id=$desired_controller_id | ||
| run_candidate_preflight | ||
| if ! $build_before_drain; then build_candidate; fi |
There was a problem hiding this comment.
When a same-engine repair makes the candidate runner tag equal the installed or live tag, this deferred build retags both images after the checkpoint, but the checkpoint records no Docker image IDs. If the runner build completes and the controller build, activation, or health check subsequently fails, restore_checkpoint restarts the prior controller without restoring the old runner tag; subsequent jobs can therefore run on newly rebuilt, mutable base/apt content despite ROLLBACK_RESTORED. The new post-drain branch is fresh evidence beyond the earlier pre-drain retagging issue; snapshot and restore the prior image IDs, or build under unique candidate tags and promote them only after success.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head checkpoints the exact prior runner and controller image IDs and restores those mappings during transactional rollback. Disposition applies to exact PR head 524b5f44a401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65d000600b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| runner_id=$(docker image inspect --format '{{.Id}}' "${checkpoint_images[0]}" 2>/dev/null) || die 'installed runner image mapping is unavailable' | ||
| controller_id=$(docker image inspect --format '{{.Id}}' "${checkpoint_images[1]}" 2>/dev/null) || die 'installed controller image mapping is unavailable' |
There was a problem hiding this comment.
When either installed image tag has been deleted or no longer resolves, managed_images_match correctly reports drift, but every repair and even --uninstall now exits here before checkpointing or draining. In particular, a missing runner tag can already prevent the active controller from creating jobs, while reconciliation cannot rebuild it; represent absent prior mappings explicitly and restore their absence on rollback rather than requiring both old tags to resolve.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head records a missing prior image tag as an explicit absent state and restores that absence during rollback. Disposition applies to exact PR head 524b5f44a401.
| if [[ -f "$checkpoint_dir/ci-fleet.env" ]]; then | ||
| if [[ ! -f "$checkpoint_dir/image-ids.env" || -L "$checkpoint_dir/image-ids.env" || $(stat -c %a "$checkpoint_dir/image-ids.env") != 600 ]] \ | ||
| || ! load_checkpoint_images "$checkpoint_dir/ci-fleet.env" "$checkpoint_dir/image-ids.env"; then | ||
| note 'ROLLBACK_FAILED reason=checkpoint image mappings are invalid' | ||
| return 1 |
There was a problem hiding this comment.
When --rollback selects any checkpoint created before this change, that checkpoint contains ci-fleet.env but cannot contain the newly introduced image-ids.env, so this condition rejects it before restoring any state. This makes the documented rollback command unusable precisely for historical checkpoints that may be needed after updating to this installer; treat a missing image map as a legacy checkpoint format and retain the prior rollback behavior for it.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Current head marks the new checkpoint format and retains the previous rollback behavior when image metadata is absent from a legacy checkpoint. Disposition applies to exact PR head 524b5f44a401.
|
@codex review Please review exact PR head Report only concrete, reproducible current-head contract violations with file and line evidence. Do not merge, deploy, change repository settings, or expose credentials. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 524b5f44a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for index in 0 1; do | ||
| if [[ ${checkpoint_images[index + 2]} == absent ]]; then | ||
| if docker image inspect --format '{{.Id}}' "${checkpoint_images[index]}" >/dev/null 2>&1; then | ||
| docker image rm "${checkpoint_images[index]}" >/dev/null || failed=1 |
There was a problem hiding this comment.
When the installed controller-image tag is missing, the checkpoint records it as absent; if candidate startup succeeds but the health check or timer activation then fails, try_drain_current stops the candidate without removing its container. Docker Compose documents that stop leaves containers in place, and Docker image removal cannot delete the candidate's sole-tagged image while that stopped container still references it unless forced. This command therefore fails, keeps failed=1, and prevents the prior controller restart at line 1124, contradicting the rollback guarantee in docs/DESIRED-STATE.md:244; remove the candidate container or explicitly force-un-tag the image before restoring an absent mapping.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Reproduced at exact head 524b5f44a40112a19bcbc30194f65f4b639b605c: candidate startup followed by timer activation failure reached ROLLBACK_FAILED because the stopped candidate container blocked restoration of the previously absent controller tag. Corrected at c4e8e05f3021bd3b015f017360ae2351cda0507c: rollback-only try_drain_current true removes the stopped candidate before restoring image mappings and restarting the prior controller. The focused regression and exact-head Actions run https://github.com/RandomDevelopment/ci-fleet/actions/runs/33853924243 passed.
|
@codex review Please review exact PR head Report only concrete, reproducible current-head contract violations with file and line evidence. Do not merge, deploy, change repository settings, or expose credentials. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4e8e05f30
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ! controller_id=$(docker image inspect --format '{{.Id}}' "${checkpoint_images[1]}" 2>/dev/null); then | ||
| docker info >/dev/null 2>&1 || die 'Docker daemon is unavailable' | ||
| controller_id=absent |
There was a problem hiding this comment.
When a running managed controller has lost only its controller-image tag—a drift case exercised by scripts/test-install-worker-controller.sh:880-901—these lines record absent even though the container's .Image still identifies the exact prior image. After candidate activation replaces that container, rollback removes the candidate container and tag before compose up (scripts/install-worker-controller.sh:1104-1133), leaving no mapping to the prior image; Compose therefore cannot restore that exact controller and may fail or rebuild mutable content. Preserve the live container image ID in this case and re-tag it before restarting; the fake test currently misses this because its up implementation does not require the configured image tag to exist.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Reproduced at exact head c4e8e05f3021bd3b015f017360ae2351cda0507c: with the controller tag absent, rollback removed the candidate but could not recreate the prior controller because its exact live image ID was not checkpointed. Corrected at ae7399019607c923aa9bc184006b9037615ee598: version-2 image-ids.env now records the live controller image ID only alongside an absent controller tag, uses it to recreate and health-check the prior controller, then removes the temporary tag to restore absence. The focused regressions, complete installer suite, and exact-head Actions run https://github.com/RandomDevelopment/ci-fleet/actions/runs/33864800081 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae73990196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| old_release=$(current_runtime_release) | ||
| [[ -n "$old_release" ]] || old_release=$fallback_release | ||
| [[ -n "$old_release" ]] || { drain_error="cannot stop restartable controller state without its runtime release: $status"; return 1; } |
There was a problem hiding this comment.
When /opt/ci-fleet/current is missing or dangling and the controller is exited, created, or dead, current_runtime_release and the unset fallback are both empty, so reconciliation aborts here after drift_count reports engine_release drift—even though install_release has already made the validated desired release available. The same condition prevents --uninstall; pass a validated release fallback to the drain or otherwise stop the fixed managed container without depending on the drifted symlink.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Reproduced at exact head ae7399019607c923aa9bc184006b9037615ee598: convergence with a missing current link and dead controller, and uninstall with a dangling link and created controller, both failed at the runtime-release guard before controller mutation. Corrected at 81682202c2cbc54fb5457c656cc0d3fbc8d7bc60: both callers now use try_drain_current's existing fallback argument. Convergence passes its installed, Compose-validated candidate release. Uninstall selects only a complete release with a valid engine marker. Both focused regressions, the complete installer suite, and exact-head Actions run https://github.com/RandomDevelopment/ci-fleet/actions/runs/33874524704 passed.
|
@codex review Please review exact PR head Report only concrete, reproducible current-head contract violations with file and line evidence. Do not merge, deploy, change repository settings, or expose credentials. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae73990196
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ! controller_id=$(docker image inspect --format '{{.Id}}' "${checkpoint_images[1]}" 2>/dev/null); then | ||
| docker info >/dev/null 2>&1 || die 'Docker daemon is unavailable' | ||
| controller_id=absent | ||
| if ! controller_live_id=$(docker inspect --format '{{.Image}}' "$controller_container" 2>/dev/null); then |
There was a problem hiding this comment.
When the configured controller tag still resolves but has been retagged from image A used by the running controller to image B, this branch checkpoints only B and never inspects the live container's .Image. A subsequent build or activation failure removes the stopped/candidate container, restores B at lines 1136-1138, and recreates the controller from B at line 1150; if B is unrelated or invalid, rollback fails even though the previously healthy A was available before reconciliation. Record the live image ID whenever it differs from the tag ID so rollback can restart the actual prior controller while separately restoring the prior tag mapping.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Reproduced at exact head 81682202c2cbc54fb5457c656cc0d3fbc8d7bc60: when the controller tag mapped to image B but the prior controller ran image A, rollback recreated B and lost the exact healthy runtime. Corrected at e473725020b4be98d41502192ccc2e24e963811f: version-2 checkpoints now record the live controller image whenever it differs from the tag mapping, temporarily use that live image to recreate and health-check the prior controller, then restore the exact prior tag mapping. The focused regression, prior rollback checks, complete installer suite, and exact-head Actions run https://github.com/RandomDevelopment/ci-fleet/actions/runs/33883004982 passed.
| compose "$old_release" "$drain_env" unpause controller >/dev/null || { | ||
| drain_error='failed to unpause the signaled controller for graceful shutdown' | ||
| if [[ "$force_nonterminal" == true && -n "$old_release" ]]; then | ||
| compose "$old_release" "$drain_env" rm -f controller >/dev/null || { |
There was a problem hiding this comment.
If the candidate starts a job and its runner exits without being removed before activation or timer setup fails, rollback's drain considers it finished because managed_runner_count uses running containers only, while this cleanup removes only the controller. The stopped runner can therefore still reference the newly built runner image when line 1125 tries to remove its previously absent tag; that removal fails, sets failed=1, and prevents the prior active controller from being restarted. Remove stopped candidate runner containers for this instance before restoring the checkpointed image mappings.
AGENTS.md reference: AGENTS.md:L57-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Reproduced at exact head 81682202c2cbc54fb5457c656cc0d3fbc8d7bc60: a stopped candidate runner retained the newly built runner image and blocked restoration of a previously absent runner tag. Corrected at e473725020b4be98d41502192ccc2e24e963811f: rollback now reuses remove_inactive_managed_runners after candidate drain and before image restoration, preserving the existing instance ownership filters. The focused regression, prior rollback checks, complete installer suite, and exact-head Actions run https://github.com/RandomDevelopment/ci-fleet/actions/runs/33883004982 passed.
|
@codex review Please review exact PR head Report only concrete, reproducible current-head contract violations with file and line evidence. Do not merge, deploy, change repository settings, or expose credentials. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81682202c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| die '--adopt requires a trusted installed controller identity' | ||
| fi | ||
| drain_current | ||
| drain_current false "$rendered_env" "$release_dir" |
There was a problem hiding this comment.
With a same-engine repair where /opt/ci-fleet/current is missing or dangling and the prior controller is running, this fallback lets the forward drain stop that controller, but make_checkpoint records no release-target. If the post-drain preflight or deferred build then fails, restore_checkpoint recomputes an empty drain release and, at lines 1067-1074, does not substitute the candidate fallback when the rendered environment exists; it therefore fails on the now-exited controller before restoring it. Fresh evidence beyond the earlier missing-link comment is that current head carries the validated fallback only into the forward drain, not its rollback path. Persist a trusted drain release for restoration as well.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid at exact head e473725020b4be98d41502192ccc2e24e963811f. The validated fallback reaches the forward drain, but a missing original release link leaves no checkpoint release target, so rollback can fail before restoring the stopped prior controller. This remains an in-contract blocker. Further code is parked pending an explicit fixed-budget rollback-invariant batch.
| old_ref=$(<"$candidate/.ci-fleet-engine-ref") | ||
| if [[ "$old_ref" =~ ^[0-9a-f]{40}$ ]] && runtime_release_complete "$candidate" "$old_ref"; then old_release=$candidate; break; fi | ||
| done | ||
| [[ -n "$old_release" ]] || die 'a trusted complete release is required to uninstall the controller' |
There was a problem hiding this comment.
When both current and manager links are missing or dangling and the controller container is already absent, invoking --uninstall from the reviewed Git checkout documented in docs/DESIRED-STATE.md:120 leaves every candidate empty or without the generated release markers, so this unconditional guard exits before removing units, rendered state, or timers. This contradicts the explicit contract at docs/DESIRED-STATE.md:118 that uninstall does not require host-release metadata, and prevents cleanup of precisely the damaged installation that recovery mode should handle. Require a complete Compose release only when one is actually needed to stop a live controller.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid at exact head e473725020b4be98d41502192ccc2e24e963811f. perform_uninstall currently requires a complete release before checking whether a controller exists, so a damaged installation with no controller and no release links cannot remove managed units and state. This remains an in-contract blocker. Further code is parked pending an explicit fixed-budget rollback-invariant batch.
| if $remove_restored_controller_tag && ((failed == 0)); then | ||
| docker image rm "${checkpoint_images[1]}" >/dev/null || failed=1 |
There was a problem hiding this comment.
When the prior controller tag was absent and its image had no other tags, rollback temporarily tags the checkpointed live image at lines 1118-1122 and starts the restored controller from it at line 1150, then this command attempts to remove that sole tag while the running controller references the image. Docker's image-removal documentation states that an image used by a running container cannot be removed without force, so this returns an error, reports ROLLBACK_FAILED, and leaves the tag that the checkpoint recorded as absent. The fake Docker test misses this because its image rm implementation blocks only on the stopped-controller marker; preserve the absent mapping without attempting an ordinary image removal after starting the restored container.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid and still applicable at exact head e473725020b4be98d41502192ccc2e24e963811f, despite this thread being outdated. The current rollback path still uses ordinary docker image rm after restarting a controller from a temporarily restored sole tag; Docker may reject that removal while the running container references the image. This remains an in-contract blocker. Further code is parked pending an explicit fixed-budget rollback-invariant batch.
|
@codex review Please review exact PR head Report only concrete, reproducible current-head contract violations with file and line evidence. Do not merge, deploy, change repository settings, or expose credentials. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Goal
Keep candidate validation and safe image builds outside the installed-controller transaction, defer builds that could retag a runner image used by either installed or live controller state until after drain, recheck Docker capacity after an early build, and restore the checkpoint plus prior image tags if reconciliation fails or receives SIGTERM.
Acceptance gates
DRAIN_OK, recognize the paused state, complete rollback, and restore the active controller instead of leaving it paused.Supported failure model
Compose validation, safe distinct-tag build failures, and post-build Docker capacity failures occur before the transaction. Builds that could retag an image used by installed or live state, managed preflight, activation, health, and SIGTERM failures after checkpoint creation use the existing rollback path. New-format checkpoints preserve each prior runner and controller tag as an exact image ID or explicit absence, so rollback restores the exact prior tag state before restarting the controller. Legacy checkpoints without a format marker retain the previous state-restoration behavior and make no image-identity guarantee. A failed automatic rollback remains recoverable through explicit
--rollback.Trust boundaries
Desired state remains validated administrative input. Installed configuration, checkpoints, units, and credentials remain root-owned host state. Job workloads remain untrusted and receive no controller credentials.
Concurrency model
The existing installer lock still serializes reconciliation. The reconcile timer remains the only scheduler. No concurrent image-prune or image-tag writer is supported during the transaction. The drain stops exited, created, and dead controller containers before a deferred build so their restart policy cannot revive the old controller against a retagged image. During rollback, the installer suppresses repeated SIGTERM so one checkpoint restoration can finish.
Non-goals
Follow-up policy
Unique candidate-tag promotion and broader image-reference redesign remain separate reviewed changes. This PR is limited to checkpointing and restoring the two existing managed image-tag states, plus recognizing pre-change checkpoints as legacy.
Stop condition
This correction stops after the focused non-Docker checks pass, one additive commit is pushed normally, and the exact-head Actions run is reported. Merge and deployment remain excluded.
Local verification
Passing on the final candidate tree:
CI_FLEET_TEST_STOP_AFTER_CHECKPOINT_COMPAT=1 timeout 20m scripts/test-install-worker-controller.shbash -n scripts/install-worker-controller.sh scripts/test-install-worker-controller.shCI_FLEET_TEST_STOP_AFTER_RESTARTABLE_TAG_BUILD=1 timeout 20m scripts/test-install-worker-controller.shCI_FLEET_TEST_STOP_AFTER_IMAGE_ROLLBACK=1 timeout 20m scripts/test-install-worker-controller.shCI_FLEET_TEST_STOP_AFTER_EMPTY_LIVE_IMAGE_BUILD=1 timeout 20m scripts/test-install-worker-controller.shCI_FLEET_TEST_STOP_AFTER_LIVE_DRIFT_BUILD=1 timeout 20m scripts/test-install-worker-controller.shCI_FLEET_TEST_STOP_AFTER_POSTBUILD_CAPACITY=1 timeout 20m scripts/test-install-worker-controller.shCI_FLEET_TEST_STOP_AFTER_PAUSED_TERM=1 timeout 20m scripts/test-install-worker-controller.shpython3 scripts/test_remote_reconcile.pyscripts/test-capacity-preflight.shpython3 scripts/check_doc_links.pytimeout 20m scripts/test-install-worker-controller.shGOFLAGS='-mod=mod -modfile=/tmp/ci-fleet-pr85-final.mod' go test ./...fromcontroller/GOFLAGS='-mod=mod -modfile=/tmp/ci-fleet-pr85-final.mod' go test -race ./...fromcontroller/git diff --checkscripts/scan_committed_secrets.py --repository "$PWD" --commit "$commit"Docker was not used locally. Pull-request CI remains authoritative for Docker-backed validation.