fix(apps): report a startup version pin that reverts a running app - #176
Draft
nilsmechtel wants to merge 1 commit into
Draft
fix(apps): report a startup version pin that reverts a running app#176nilsmechtel wants to merge 1 commit into
nilsmechtel wants to merge 1 commit into
Conversation
A worker restart replays --startup-applications, but an app rolled out over the API only ever changed the worker's runtime state. When the pin still carries the old version the restart quietly reinstates it: status RUNNING, deployments HEALTHY, no diff against prior state — just older code. deNBI lost model-runner 2.7.2 -> 2.4.2 this way on 2026-09-01 and it was found only because someone compared versions by hand. recover_deployed_applications() runs first, so the worker already holds both numbers. Log a WARNING naming them before redeploying at the pin, and return pinned_version from get_app_status() so the divergence is assertable over the API without reading cluster config. Observability only: what gets deployed is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A worker pod restart can roll a production app backwards by several versions with nothing anywhere reporting it. The app comes back up, passes every health check and serves traffic normally — it is just running older code than it was five minutes earlier. deNBI lost model-runner from 2.7.2 to 2.4.2 this way on 2026-09-01, and it was found only because someone compared versions by hand before an unrelated deployment.
There are two sources of truth for "what version is this app". The worker's runtime state, set by
deploy_app()over the API and reported byget_app_status(); and the Deployment's--startup-applicationsargument, rendered from the cluster'svalues.yamland replayed bydeploy_startup_applications()at boot. An out-of-band roll changes only the first, so the next restart — node drain, OOM kill, liveness kill, helm upgrade, image roll — reinstates the committed pin. Nothing catches it because that redeploy is indistinguishable from a normal successful startup: RUNNING, HEALTHY, no warning. Every probe passes, because the older version is not broken.The worker already holds both numbers at that moment:
recover_deployed_applications()runs first and has just enumerated what survived the restart. It simply never compared them.What changed
deploy_startup_applications()logs a WARNING before redeploying at the pin when the app was found running at a different version of the same artifact. That converts a silent revert into a greppable line naming both versions, and it alone would have caught the deNBI instance.get_app_status()returnspinned_versionalongsideversion/running_version/version_verified, so the divergence is checkable over the API — assertable in CI or a monitor — without reading cluster config. It isNonewhen the app is not a startup application, and short artifact ids in the pin are resolved to the workspace-qualified form before matching.Both are pure observability. Neither changes what gets deployed, so neither can make a rollback worse than it is today.
Three conditions deliberately stay silent, because in each the pin is not reverting anything: a pin whose version matches what is running; a pin carrying no version at all (
deploy_appthen inherits the running version); and a pin whoseapplication_idmatches but whose artifact does not, where the two version strings belong to different release lines and comparing them says nothing.What is deliberately not here
The issue's third candidate fix — refuse a downgrade unless an explicit
allow_downgradeflag is passed — is out of scope. It is not a stronger version of the two above, it is a different decision: refusing a downgrade also blocks deliberate rollback, and the last thing you want failing closed during an incident is the command that puts the old version back. That tradeoff is yours to make, and the WARNING added here is the evidence that argument needs — it shows how often divergence actually occurs before anyone commits to blocking on it.Note that none of this removes the need to bump the pin in the same session as an out-of-band roll. It makes the omission visible instead of silent.
Verification
Seven new tests in
tests/apps/test_startup_pin_divergence.py, covering the warning, the three silent cases, and the status field including short-id resolution. 184 tests pass acrosstests/_app,tests/appsandtests/worker(the 32 errors undertests/apps/model-runnerandtests/apps/cellposeare the pre-existing--noconftestfixture errors, unrelated).Both halves were positive-controlled rather than merely asserted. Removing the
_warn_on_startup_pin_divergencecall makes the warning test fail withassert []— no log record at all, which is exactly the production symptom. Removing thepinned_versionkey makes the three status tests fail withKeyError: 'pinned_version'.tests/apps/test_status_app_missing_from_cluster.pyneeded one line: its hand-built manager now setsstartup_applications = [], since_get_app_statusreads that attribute.Related
#0023describes the same "two sources of truth, only one reported" shape on the deploy path rather than the restart path, and GH #157 adds the frontend variant. Whatever surfaces head-vs-deployed there should reuse this field.