Restore a working app's service address when its replacement replica never comes up - #195
Merged
Merged
Conversation
… serving A successor proxy replica claims the record as unregistered at init. If it then never registers — a redeploy where the predecessor still holds the shared Hypha client_id, or a crash-loop — nothing put the record back: a registered replica takes the probe branch of the maintenance tick and never re-reported True. The worker withheld a working service address indefinitely. The reachability probe already establishes that our address resolves, so report True there. The record becomes self-correcting instead of write-once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe suspends, so check_health's sibling-down path can deregister this replica while it is in flight. Reporting True on the answer to a question asked before that would leave a deregistered app advertised with no way back, since every later tick returns at the readiness gate. Also retires the stale rationale in the gate test that still claimed nothing re-reports True after a registration — the twin of the sentence already fixed in the proxy actor docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 could report an application as having no service address while that application was in fact serving requests perfectly well — and stay wrong about it indefinitely. It happened on redeploy: the incoming copy of an app's proxy is built while the outgoing one is still answering, and the incoming copy immediately takes ownership of the "is this app reachable?" record and marks it unreachable. If the incoming copy then fails to come up, nothing ever corrected the record, so the dashboard and every API consumer saw the app as unavailable for as long as it kept working.
Problem
ProxyDeployment.__init__callsclaim_service_registration, which replaces the record unconditionally. That is the right behaviour on its own: it is what closes the case where a replica is SIGKILLed or loses its node, never runs__del__, and leaves a dead service id advertised for its successor's whole startup window.The residual is that nothing else ever writes
True._maintenance_tickonly reports registration from the branch that performs a registration, and that branch is skipped onceneeds_registrationis false. So a record that diverges from reality stays diverged.What makes the divergence easy to hit on a redeploy rather than exotic is that within one worker generation the proxy's Hypha
client_idis the same string for both replica generations — it is derived from the worker client id and an app hash, not from the replica. While the predecessor still holds it, Hypha refuses the successor's connection with "Client already exists and is active", which the proxy correctly treats as transient and retries. The successor is therefore expected to spend the overlap claimed-but-unregistered, and if it never gets past that — it crash-loops, or its config is broken — the predecessor keeps serving its address while the worker reportswebsocket_service_id: Noneandservice_registered: Falsewith no path back.The symptom is the mirror image of the one the unconditional claim fixed: withholding an address that works, instead of advertising one that does not.
Solution
Report
Truefrom the successful reachability-probe branch of_maintenance_tick, gated on the replica still being ready to serve.The probe already asks Hypha whether it still serves this replica's own service id — it exists precisely because a frozen replica can keep a socket open against a server that has dropped its registration. A replica that has just had that question answered is the one entitled to assert the app is reachable, so the record reconverges on what is actually true once per probe interval instead of being written once and trusted forever.
The window this introduces, and why it is still the right trade
This change is not free, and the cost is worth stating rather than glossing. The shared
client_idabove establishes the bug, but it does not mean two generations can never be registered at once.worker_client_idis read at build time from the live worker connection (builder.py:768), and--client-iddefaults to auto-generated, so after a worker restart plus app recovery a redeploy of the sameapplication_idbuilds a successor whose Hypha id differs from the still-serving predecessor's, and nothing refuses it.manager.py:1079-1088exists solely to handle that case.In that situation the sequence is: the successor registers and reports
True, the predecessor's own probe then re-assertsTrueand takes record ownership back, and the predecessor's__del__writes aFalsethat is now honoured — against an app the successor is serving. Driven through the realProxyDeploymentandBioEngineProxyActor, that ends atFalse; the same sequence on the parent commit ends atTrue, because the replica-tag guard drops the departing replica's write.So this introduces a wrong-
Falsewindow that the parent commit does not have, bounded by one probe interval (_REACHABILITY_PROBE_INTERVAL_S, 60s) because the successor's next probe heals it. That is a trade of a permanent false-negative for a bounded one, and it is bounded precisely because the re-assert that causes it is also the cure. The alternative — reasoning about which single writer is authoritative — is what produced a record that could be pinned wrong for good in the first place.The same reconvergence also covers the ordering hazard the record has by construction: an init-time claim delivered after its successor had already registered would previously reset the record to
Falsepermanently, and now heals on the serving replica's next probe.Behavioural changes
static_site_urlonFalse, and still falls back to the replica-alive gate when an app has never reported.entry_deployment_readygate and re-reads it after the probe returns. The probe suspends, so a sibling-down deregistration can land while it is in flight; answering a question asked before that would leave a deregistered app advertised with no way back, since every later tick returns at the gate.Test plan
Four tests added to
tests/apps/test_service_id_registration_gate.py, all driving a realProxyDeploymentinto a realBioEngineProxyActorrather than asserting on call shape:test_a_successor_that_never_registers_cannot_pin_a_serving_app_at_falsereproduces the reported case end to end — a predecessor registers through a real maintenance tick, a real successor is constructed and claims the record, and the predecessor's next probe must restore it. It fails on the parent commit withAssertionError: assert False is Trueand passes with the fix.test_the_probe_does_not_re_register_an_app_that_deregistered_itselfpins that the re-assert respects the readiness gate. Killed by a mutant that hoists the report above the gate.test_a_failing_probe_does_not_assert_the_app_as_registeredpins that only a confirmed probe asserts reachability. Killed by a mutant that moves the report ahead of the probe call.test_a_probe_answered_after_a_deregistration_does_not_re_registerholds a probe suspended, deregisters the replica underneath it, then releases it. Killed by a mutant that drops the post-probe readiness re-read.The pre-existing invariants around the replica tag were re-run and still hold: a live replica can deregister itself, a departed replica's late
__del__is still dropped, an untagged successor can claim from a tagged predecessor and then deregister itself, and a replica that died without deregistering still loses the record at its successor's init.Full suite:
522 passed, 25 skippedagainst a518 passed, 25 skippedbaseline on the parent commit — additive, no regressions.Files
bioengine/apps/proxy_deployment.py— reportTrueon the successful probe branch of_maintenance_tick, gated on a re-read ofentry_deployment_ready.bioengine/cluster/proxy_actor.py— docstring onreport_service_registrationno longer claims nothing re-reportsTrueafter a registration.tests/apps/test_service_id_registration_gate.py— four regression tests, a contract note in the module docstring, and the same stale rationale retired in the late-deregistration test.🤖 Generated with Claude Code