fix(apps,cli): stop silently redeploying the version already running - #178
Draft
nilsmechtel wants to merge 1 commit into
Draft
nilsmechtel wants to merge 1 commit into
nilsmechtel wants to merge 1 commit into
Conversation
deploy_app with an application_id and no version inherits the version already running, which is documented and deliberate -- but it left no trace anywhere. A caller who had just uploaded newer code got a deploy that reported success while redeploying the code it had replaced, and every status field agreed with it. Reported independently over the API and from the CLI (#157). The worker now names the inherited version, and warns when it is not the artifact's newest -- the case that is almost always a mistake -- naming both versions and the version= argument that rolls forward. Pinning to an older version on purpose stays legal; it just announces itself. An unpinned running app is exempt: that update resolves latest again and does roll forward. The version lookup never blocks the deploy. `bioengine apps deploy` now passes the version it just uploaded. It has no --version flag, so before this there was no way to express "deploy this app directory to that running app" -- the command uploaded a new artifact version and then redeployed the old one, silently, exit code 0. Refs: svamp issue #23, #157
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 stale-source deploy that reports success is worse than one that fails, because nothing prompts you to look.
deploy_appwith anapplication_idand noversioninherits the version already running — documented, deliberate, and the right default — but it left no trace anywhere. A caller who had just uploaded newer code got a deploy that reported success while redeploying the code it had replaced, and every status field agreed with it. Reported twice independently: over the API (svamp #23, 0.16.1) and from the CLI (#157, 0.14.0,bioengine-worker-europa).The inheritance rule itself is not changed. Defaulting to "latest on update" would silently upgrade apps that are pinned on purpose, which is a worse failure than this one.
Worker: the inherited version stops being silent
_report_inherited_versionruns on the update path wheneverversionwas omitted. When the inherited version is not the artifact's newest — the case that is almost always a mistake, because the caller has just uploaded something newer — it logs a WARNING naming the running version, the newer version, and theversion=argument that rolls forward. Otherwise it logs the resolved version at INFO, so the resolution is always readable after the fact.Two things it deliberately does not do. An app that is itself unpinned is exempt:
versionstaysNone, the builder resolves latest again, and it genuinely does roll forward — warning there would be false, and it is the same unversioned-pin case as #176. And the artifact read is wrapped: this is observability, so a failure to look up versions logs at DEBUG and lets the deploy proceed rather than raising into the caller.CLI:
apps deploydeploys what it just uploadedapps runaccepts--version;apps deployaccepts no version flag at all, so there was no way to express "deploy this directory to that running app" through the command the docs point users at. It uploaded a new artifact version, targeted a running--app-id, inherited that app's old version and reported success — exit code 0, no warning, never entering DEPLOYING. The reporter polled 25 times over 8 minutes before spotting it.It now passes the manifest's version, which is by construction the version
upload_appjust created (_enforce_version_increasesrejects anything that isn't strictly greater). The version is echoed on all three lines, so a no-op is visible without reading worker logs.This also fixes the frontend variant in #157: the static site serves the artifact HEAD while the backend serves its deployed version, so a no-op
apps deployhanded a browser the newindex.htmlagainst the old backend, withapps statusreporting RUNNING/HEALTHY throughout.Tests
tests/apps/test_deploy_version_inheritance.py, 8 tests, including a CliRunner test that drivesapps deployagainst a stub worker and asserts the recordeddeploy_appkwargs. Three positive controls, each failing exactly one test: dropping the CLI version pin, flattening the WARNING branch to INFO, and removing the call fromdeploy_app.Full run: 185 passed (
tests/_app tests/apps tests/worker,--noconftest; the 32 errors are the pre-existing model-runner/cellpose fixture errors that flag needs).Not in this PR
deploy_appto return the resolved version. That means changing-> strto a mapping, which breaksapp_id = await worker.deploy_app(...)— the form written inCLAUDE.mdand the user-facing skill, so every agent following those docs. Worth doing, but as its own deliberate break rather than folded in here.apps status. Doing it properly means an artifact read per app per poll, on a path the dashboard polls continuously. The deploy-time warning above covers the moment the skew is created; the status-side view needs its own design.references/service_ids.md— its stated rationale for the replica suffix ("changes on every redeploy") is wrong, and apps deploy --app-id silently no-ops on a running app, leaving artifact/backend version skew #157's reporter used the unchanged suffix as a false "redeploy succeeded" signal. That file lives in the siblingbioimage.ioskills repo.Closes svamp issue #23.