fix(apps): advertise an app's service address only once it resolves - #179
Draft
nilsmechtel wants to merge 1 commit into
Draft
fix(apps): advertise an app's service address only once it resolves#179nilsmechtel wants to merge 1 commit into
nilsmechtel wants to merge 1 commit into
Conversation
get_app_status derived service_ids from the worker's client id and gated only on "a ProxyDeployment replica is alive". That is a precondition for Hypha registration, not evidence of it: the proxy reports healthy to Ray while it waits for its siblings to come up and registers afterwards, so the worker handed out an address — and a static_site_url built from it — tens of seconds before anything answered there, with status already RUNNING. Measured gaps of 28.3 s and 20.6 s. The proxy now pushes its registration state to the BioEngineProxyActor (False at replica init, True after _register_services succeeds, False on deregistration) and the worker reads it before advertising. A never- reported app falls back to the replica-alive gate: an app recovered under a newer worker, or one whose actor was recreated, serves fine without ever reporting, and hiding its id would break a working deployment. get_app_status gains a service_registered field so a client can tell "not registered yet" from "this worker cannot tell", and the deploy log no longer claims completion while serve.run is still starting replicas. Closes svamp issue #24.
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.
An address that does not resolve is worse than no address, because the caller has no way to tell "not yet" from "wrong".
get_app_statusderivedservice_idsfrom the worker's client id and gated only on a ProxyDeployment replica is alive. That is a precondition for Hypha registration, not evidence of it: the proxy replica reports healthy to Ray while it waits for its siblings to come up, and only registers with Hypha afterwards. So the worker advertised a service id — and astatic_site_urlbuilt from it — whilestatusalready readRUNNINGand nothing answered at that address. Reported from chiron-platform with measured gaps of 28.3 s and 20.6 s on two consecutive deploys, after four legs of client-side retry (8 s each) all expired inside the window.The proxy reports, the worker reads
ProxyDeploymentpushes its registration state to theBioEngineProxyActor:Falsewhen the replica initialises,Trueonce_register_servicessucceeds,Falseagain on_deregister_services._get_application_service_idsreads that record before building the id, andclear_application_replicasdrops it so a redeployed app never inherits the previous deployment's verdict.The report is fire-and-forget and the read is wrapped: this decides what to advertise, so neither a failed push nor an unreachable actor may affect the replica or the deploy.
Never reported is not "no"
Gating purely on the flag would break the case this worker cannot see.
register_serve_replica— and now the registration report — happen inProxyDeployment.__init__, so an app whose proxy replica started before this actor existed has no record here: a different BioEngine version uses a different actor name, and an actor evicted after long idleness comes back empty. Those apps are serving.Nonetherefore falls back to the existing replica-alive gate, and only an explicitFalsewithholds the id. Same "absence is not a verdict" shape as #177.The seed at replica init is what makes the gate bite on a fresh deploy: without it the brand-new app is indistinguishable from the recovered one, and falls through to the old behaviour.
What callers see
get_app_statusgainsservice_registered(True/False/None), so a client polling aRUNNINGapp with null ids can tell "the proxy has not registered yet, keep waiting" from "this worker cannot tell".static_site_urlfollows the ids, since it embeds the service id as a query parameter — a no-op frontend link was the same bug wearing a different hat.The deployment log claimed
Successfully completed deploymentimmediately afterAppBuilder.submit, which ends inserve.run(..., blocking=False). It now says what actually happened: submitted, replicas starting.docs/glossary.mddocumented the id's suffix as<replica_id>; it has been the first 8 hex digits ofsha1(application_id)since the client_id was made stable across restarts. Corrected, along with a note in both docs to poll for a non-Nonewebsocket_service_idrather than forRUNNING.Tests
tests/apps/test_service_id_registration_gate.py, 16 tests across the three layers: the worker's gate (including both fallbacks), the actor's record, and the proxy's reports. Four positive controls, each failing exactly the expected tests: dropping theFalsegate fails 2 (the reported symptom and the static-site variant), dropping theTruereport fails 1, dropping the clear-on-undeploy fails 1, dropping the init seed fails 1.The e2e startup test waited for
status == "RUNNING"and then asserted a service id was present — which under this change is a genuine race, so it now waits for the id itself.resolve_service's docstring described the bug as the reason for its retries; it is now the propagation delay it actually still guards.Full run: 193 passed (
tests/_app tests/apps tests/worker,--noconftest; the 32 errors are the pre-existing model-runner/cellpose fixture errors that flag needs), against 177 onmain.Not in this PR
deploy_appuntil the service is registered. It returns as soon as the deployment task is spawned, by design; making the address honest is the smaller fix and does not change anyone's call shape.Closes svamp issue #24.