Skip to content

fix(apps): key redeploy on source content and verify recovered apps - #171

Draft
nilsmechtel wants to merge 3 commits into
mainfrom
fix/stale-actor-holes
Draft

fix(apps): key redeploy on source content and verify recovered apps#171
nilsmechtel wants to merge 3 commits into
mainfrom
fix/stale-actor-holes

Conversation

@nilsmechtel

@nilsmechtel nilsmechtel commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Fixes two holes in the delete-before-rebuild policy that let a redeploy report success while the old replica keeps serving. Reported as svamp issue #50 by bioengine:live-kudu, whose stated mechanism ("a version bump does not recreate the actors") turns out to be false on current maindeploy_app has deleted before rebuild on a version/artifact change since 20e13d3 (0.11.27). The split-brain they observed is real; it comes from these two gaps instead.

Hole 1 — the content check keyed on the version string

content_changed compared the requested version and artifact_id against the tracked pin, before the build. A version string cannot see content that changed underneath it, so a re-staged version — same string, different files — looked unchanged and took the in-place serve.run path. _ensure_source had already refreshed the files on disk by then, so the symptom is precisely "the source on disk is new, the running process is old": with num_replicas=1 and no surge headroom (a single GPU) serve.run reuses the existing replica, which keeps the module it imported at first start.

The fix moves the check after the build and compares a content fingerprint, because only the build has fingerprinted the files it actually synced. The check stays before _check_resources, so the old app's reservation is already released when free capacity is measured — otherwise a same-app GPU redeploy would falsely fail admission in single-machine/SLURM mode.

The fingerprint is the source_hash that already existed: an md5 over each file's relpath plus bytes, excluding __pycache__, computed in the submit task and baked onto every user class as code_hash. It is now a shared hash_source_tree helper called by both the introspect task (returned to the worker as source_signature) and the submit task (baked into the replica), so the worker's expected value and the replica's reported value are the same quantity computed the same way. This also removes the inline copy of the hasher.

When either signature is unknown — an app recovered from an older worker carries none — the check falls back to version/artifact identity rather than restarting a healthy app on a config-only update.

Note on what this deliberately does not change: deploy_app(version=None) on an existing app inherits existing_app["version"], along with every other unspecified parameter. The pin does not advance to "latest", so nothing has changed and taking the in-place path is correct. Callers who want a newer version must pass it, as they already must for disable_gpu and authorized_users.

Hole 2 — recovered apps were invisible to the verification

recover_deployed_applications stores "built_app": None (there is nothing to resubmit; the app was adopted from the previous worker's Serve deployments). _verify_running_identities opened with if not spec: return None, None, so for every recovered app version_verified was permanently null in get_app_status and the monitor's delete-on-mismatch could never fire. That is exactly backwards: an app that survived a worker roll is the one most likely to be running code its version pin no longer describes.

The spec is only needed to name the entry deployment for the running_version field. The verification itself walks every deployment's RUNNING replicas and needs nothing from the spec. So it now runs regardless, and running_version simply stays None when the entry deployment can't be named.

Relaxing that guard alone would have been a regression: the monitor would serve.delete a mismatched recovered app, and the redeploy path would then call submit(None) and destroy it permanently. Both action sites are now guarded on built_app is None instead — the monitor logs and leaves the app serving, and _fire_redeploy returns early with an explanation. The latter also closes a pre-existing bug on the same path: _recover_from_controller_loss already had this guard, the general unhealthy path did not, so any unhealthy recovered app would have raised inside the redeploy task.

Observability: code_verified

get_app_status gains code_verified alongside running_version / version_verified. Replicas already push code_hash to the proxy actor and nothing read it; comparing it against the deployed bundle's source_signature is what detects same-version staleness while it is happening, rather than after a lost release cycle.

It is report-only — a false value never triggers a delete. If the introspect-side and submit-side hashes ever disagreed systematically the monitor's self-heal would become a redeploy loop on a healthy app, and that failure mode is worse than the one being detected. The monitor logs it once per occurrence (_identity_warned); both non-self-healing conditions persist until a human redeploys and the monitor ticks every ~10s, so without de-duplication the log would fill with one repeated line for the worker's lifetime.

Residual limitation

The proxy actor is named BIOENGINE_PROXY_ACTOR_{version} (ray_cluster.py:197). It survives a worker roll at the same bioengine version, but a version-bumped roll creates a new actor with an empty identity cache. Recovered-app verification is therefore inert across a version bump until each replica re-registers — checked == 0, so it returns None and nothing acts on partial data. It fails safe, but it means the case this PR opens up is not covered on the first roll onto a new worker version. Fixing that means unpinning the actor name from the version, which is a separate change with its own upgrade-path questions.

Validation

0.16.6-dev1 was validated in both modes required by CLAUDE.md, using a copy of demo-app whose ping() returns a marker string.

Single-machine (local Docker, workspace bioengine-devtest):

step action result
1 fresh deploy at 0.1.0 RUNNING, version_verified / code_verified true, serves MARKER_A
2 redeploy at version=None after committing a 0.2.0 pin stays 0.1.0, no delete — correct, the pin is inherited
3 redeploy at version="0.2.0" delete fires (pre-existing behaviour), serves MARKER_B
4 overwrite 0.2.0's content in place, redeploy at "0.2.0" delete fires on an unchanged version string, serves MARKER_C
5 redeploy at "0.2.0" with new application_kwargs only no delete, in-place rolling update, still MARKER_C

Step 4 is the discriminating case: the only input that changed was source_signature, and the worker logged Deleted Ray Serve application 'hole1-probe' before redeploy so replicas are recreated with the new source. On main that redeploy takes the in-place path. Step 5 confirms the tracked signature is refreshed afterwards, so the fix does not turn every subsequent config-only update into a restart. (upload_app refuses a non-increasing version, so step 4's in-place overwrite was produced through the artifact manager directly.)

External cluster (KTH, side-by-side bioengine-worker-devtest release on the shared Ray cluster): deployed the app, then destroyed and replaced the worker pod at the same bioengine version. The new worker logged Recovered 1 application(s) from existing Ray Serve state, and get_app_status returned recovered_app: true, version_verified: true, code_verified: true — the two verification fields that were permanently null on main. running_version is null as designed (no spec to name the entry deployment). The app kept serving across the roll on its original replicas and was not deleted or stranded by the monitor.

Unit tests pass (173 in tests/_app + tests/apps). The failures in this environment are unrelated and reproduce on origin/main: tests/apps/cellpose needs typing.Self (py3.11), tests/apps/model-runner hits a live Hypha service, and tests/test_gpu_sizing.py / tests/worker/test_startup_app_env_expansion.py crash the xdist child at import (verified by swapping origin/main's builder.py back in — same crash).

Delete-before-rebuild only fired when the requested version or artifact_id
differed from the tracked pin. That misses the two cases the pin cannot
express: deploy_app(version=None) inherits the existing pin, and a
re-staged version keeps its string. Both left the warm replica serving the
module it imported at first start.

Move the check after the build, where the version is resolved and the
synced source is fingerprinted, and compare that fingerprint. It stays
before _check_resources so the freed reservation is visible.

Recovered apps carry no built_app, so _verify_running_identities returned
early and both version_verified and the monitor's self-heal were blind for
exactly the apps most likely to be stale. The verification needs the spec
only to name the entry deployment, so run it regardless and guard the
delete and _fire_redeploy on built_app instead — deleting a recovered app
would strand it.

Also surface code_verified in get_app_status: report-only, since a
systematic hash disagreement would otherwise loop the monitor.
deploy_app(version=None) on an update inherits the existing pin, so the
pin genuinely does not advance and taking the in-place path is correct
there. The case the fingerprint catches is a version whose content
changed underneath it.
@nilsmechtel
nilsmechtel force-pushed the fix/stale-actor-holes branch from 00d4116 to 06240e9 Compare September 5, 2026 21:19
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