Skip to content

fix(apps,cli): stop silently redeploying the version already running - #178

Draft
nilsmechtel wants to merge 1 commit into
mainfrom
fix/deploy-version-inheritance
Draft

nilsmechtel wants to merge 1 commit into
mainfrom
fix/deploy-version-inheritance

Conversation

@nilsmechtel

Copy link
Copy Markdown
Collaborator

A stale-source deploy that reports success is worse than one that fails, because nothing prompts you to look. deploy_app with an application_id and no version inherits 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_version runs on the update path whenever version was 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 the version= 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: version stays None, 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 deploy deploys what it just uploaded

apps run accepts --version; apps deploy accepts 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_app just created (_enforce_version_increases rejects 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 deploy handed a browser the new index.html against the old backend, with apps status reporting RUNNING/HEALTHY throughout.

Tests

tests/apps/test_deploy_version_inheritance.py, 8 tests, including a CliRunner test that drives apps deploy against a stub worker and asserts the recorded deploy_app kwargs. Three positive controls, each failing exactly one test: dropping the CLI version pin, flattening the WARNING branch to INFO, and removing the call from deploy_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

  • The return value. Remove manual mcp service #23's first bullet asks deploy_app to return the resolved version. That means changing -> str to a mapping, which breaks app_id = await worker.deploy_app(...) — the form written in CLAUDE.md and the user-facing skill, so every agent following those docs. Worth doing, but as its own deliberate break rather than folded in here.
  • Artifact-HEAD-vs-deployed skew in 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 sibling bioimage.io skills repo.
  • No version bump — that lands as a separate commit just before this leaves draft.

Closes svamp issue #23.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant