Update: never report success on a box whose container is not running#137
Update: never report success on a box whose container is not running#137danielrmerskine wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the update flow by introducing a container liveness check (LAGER_PROBE_LAGER_RUNNING) to detect if the lager container is down, ensuring that a rebuild is triggered even if the code is in sync. It also adds a recovery mechanism to restart the box on its previous version if a build fails, along with corresponding unit tests. The review feedback correctly points out that OSError should be caught alongside subprocess.SubprocessError when calling store_build_hash and run_ssh_command_with_output to prevent the CLI from crashing if the ssh executable is missing or fails to execute.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| store_build_hash('FAILED') | ||
| except subprocess.SubprocessError: | ||
| pass |
There was a problem hiding this comment.
The store_build_hash function internally calls run_ssh_command_with_output, which can raise OSError (e.g., if the ssh executable is missing or cannot be executed). Since OSError is not a subclass of subprocess.SubprocessError in Python, any OSError raised here will propagate and crash the CLI. Consider catching (subprocess.SubprocessError, OSError) to ensure this best-effort operation is truly safe.
| try: | |
| store_build_hash('FAILED') | |
| except subprocess.SubprocessError: | |
| pass | |
| try: | |
| store_build_hash('FAILED') | |
| except (subprocess.SubprocessError, OSError): | |
| pass |
| try: | ||
| recover = run_ssh_command_with_output( | ||
| 'cd ~/box && chmod +x start_box.sh && LAGER_SKIP_BUILD=1 ./start_box.sh', | ||
| timeout_secs=600, | ||
| ) | ||
| # Exit 3 = container up but post-start installs failed; the | ||
| # box is serving, which is all this recovery promises. | ||
| restarted = recover.returncode in (0, 3) | ||
| except subprocess.SubprocessError: | ||
| restarted = False |
There was a problem hiding this comment.
Similarly, run_ssh_command_with_output can raise OSError if the ssh command is unavailable or fails to execute. Since OSError is not caught by subprocess.SubprocessError, an unhandled exception could crash the update command during the recovery phase. Catching (subprocess.SubprocessError, OSError) ensures the recovery fallback is robust.
| try: | |
| recover = run_ssh_command_with_output( | |
| 'cd ~/box && chmod +x start_box.sh && LAGER_SKIP_BUILD=1 ./start_box.sh', | |
| timeout_secs=600, | |
| ) | |
| # Exit 3 = container up but post-start installs failed; the | |
| # box is serving, which is all this recovery promises. | |
| restarted = recover.returncode in (0, 3) | |
| except subprocess.SubprocessError: | |
| restarted = False | |
| try: | |
| recover = run_ssh_command_with_output( | |
| 'cd ~/box && chmod +x start_box.sh && LAGER_SKIP_BUILD=1 ./start_box.sh', | |
| timeout_secs=600, | |
| ) | |
| # Exit 3 = container up but post-start installs failed; the | |
| # box is serving, which is all this recovery promises. | |
| restarted = recover.returncode in (0, 3) | |
| except (subprocess.SubprocessError, OSError): | |
| restarted = False |
A box update that fails during the Docker build exits after the containers
have already been stopped and removed, leaving the box with no services. On
the next run the early-exit gate read only source state (git in sync, no
flatten work, build-inputs hash matching), so it printed a green "already at
version X" and exited 0 on a box that was actually dead. Found via a field
report of an update that failed during Docker image export; the retry then
claimed the box was up to date and the user had to restart it by hand over
SSH.
Fixes, all in cli/commands/utility/update.py:
- The single-SSH probe now reports whether the lager container is running
(tri-state: running / not running / unknown when docker itself does not
answer). The early-exit gate requires it: source-in-sync but container-down
falls through to the rebuild path with an explanatory message instead of
early-exiting. Unknown fails open, matching the BuildKit preflight.
- `lager update --check` reports the stopped container ("Container: NOT
RUNNING (will rebuild and restart)") and exits 1 so scripted checks see
that the box needs work.
- A failed build stores the sentinel value FAILED as the build-inputs hash.
The sentinel never matches a recomputed hash, so the retry always
invalidates the cached image and rebuilds. This closes the case where
recovery (below) brings the box back up on the old image and source state
alone would again read as "up to date".
- After a failed build, the update restarts the box on its previous image
(unless the image was wiped by --force / a hash change) instead of leaving
it with no services, and reports plainly that the update did NOT apply.
- New hint for BuildKit build-cache corruption ("failed to prepare extraction
snapshot ... parent snapshot does not exist"): clear with `docker builder
prune -af` and re-run.
Unit tests cover the probe parsing, the gate verdict, and the sentinel
predicate. Hardware-validated on a real box: a no-op update still early-exits
in ~2s; with the containers removed, update rebuilds and restarts instead of
claiming success; a failed build leaves the sentinel, restarts the previous
version, and the following update heals with a clean rebuild.
The liveness gate closes the dead-box case, but a prior update can pull the new code and then exit BEFORE the container stop (a BuildKit preflight failure, or an interrupt in the pull->stop window). The box then reads as in sync AND running while the container still serves the previous build, and the early-exit would stamp the new version onto it. /etc/lager/version records the last version a successful update actually deployed, so the skip path now also requires it to match the tree's version (the semver target, or the tree's declared __version__). A mismatch falls through to the rebuild path with an explanatory message; unknown values on either side fail open so legacy boxes with a missing or empty version file do not rebuild forever. `lager update --check` reports the same state as "running a STALE build (will rebuild and restart)" and exits 1. Also adds shell-level tests for the probe's liveness fact (stubbed docker: running / exact-name-only matching / no rows / docker failing) and unit tests for the staleness predicate. Hardware-validated end to end on a real box: a staged stale version record is flagged by --check, the update rebuilds and restarts instead of early-exiting, and the record reconciles; the no-op fast path stays ~2s. The build-failure recovery messaging for the wiped-image case (--force / hash change) was also exercised on hardware: honest failure text, manual recovery command, exit 1.
43b027f to
1cac896
Compare
1 participant
Update: never report success on a box whose container is not running
The bug
lager updatestops and removes thelager/pigpiocontainers before rebuilding theimage. If the build then fails, the command exits with the box left running no services at
all. On the next run, the early-exit gate checked only source state — git in sync, no
layout work, docker-build-inputs hash matching — so it printed a green
"already at version X" and exited 0 on a dead box.
Found via a field report: an update failed during the Docker image export (BuildKit
snapshot-cache corruption on the box), and the retry immediately claimed the box was up to
date. The user had to ssh in and run
./start_box.shby hand to actually recover.The gate's own comment acknowledged the hole and pointed at
--force— advice a user whowas just told "Complete!" has no reason to reach for.
The fix
All in
cli/commands/utility/update.py:whether the
lagercontainer is running (tri-state:1running,0definitely not,empty when docker itself did not answer — unknown fails open, same philosophy as the
BuildKit preflight). Source-in-sync with the container down now falls through to the
rebuild path with an explanatory message instead of early-exiting. The gate condition is
extracted into
_rebuild_gate_verdict()so it is unit-testable.lager update --checksurfaces the state:Container: NOT RUNNING (will rebuild and restart)and exit 1, so scripted checks see that the box needs work.FAILED. The sentinelnever equals a recomputed sha256, so the retry always invalidates the cached image and
performs a clean rebuild. This closes the case the liveness check alone cannot: recovery
(next bullet) brings the box back up on the old image, where source state alone would
again read as "up to date". (Deleting the hash file would not work —
_build_hash_mismatchrequires both values non-empty.)(
--force/ hash change), the previous image is still taggedlagerand its baked-incode matches what was running before the pull, so the update restarts it
(
LAGER_SKIP_BUILD=1 ./start_box.sh), waits for/health, and reports plainly:"The update FAILED and was not applied. The box has been restarted on its previous
version." Exit code stays 1.
failed to prepare extraction snapshot ... parent snapshot ... does not exist): clear withdocker builder prune -afand re-run (the next build runs cold).Testing
cli/tests/test_update_gate.py(21 tests): probe parsing, the gate verdict (livenesstri-state, source-divergence dominance), and the sentinel/mismatch predicate.
--checkreports NOT RUNNING andexits 1; a real update prints the explanatory message and does a cached rebuild +
restart in ~32s instead of falsely reporting success.
FAILEDlandsin
/etc/lager/build-hash, the box is restarted on its previous image (/health200),exit 1 with the honest message.
--checkpredicts a freshbuild instead of "Nothing to do"; the next update wipes the cached image, rebuilds
clean in ~28s, and stores a real hash again.