Skip to content

Restore a working app's service address when its replacement replica never comes up - #195

Merged
nilsmechtel merged 3 commits into
mainfrom
fix/proxy-registration-self-heal
Sep 27, 2026
Merged

nilsmechtel merged 3 commits into
mainfrom
fix/proxy-registration-self-heal

Conversation

@nilsmechtel

@nilsmechtel nilsmechtel commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

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__ calls claim_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_tick only reports registration from the branch that performs a registration, and that branch is skipped once needs_registration is 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_id is 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 reports websocket_service_id: None and service_registered: False with 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 True from 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_id above establishes the bug, but it does not mean two generations can never be registered at once. worker_client_id is read at build time from the live worker connection (builder.py:768), and --client-id defaults to auto-generated, so after a worker restart plus app recovery a redeploy of the same application_id builds a successor whose Hypha id differs from the still-serving predecessor's, and nothing refuses it. manager.py:1079-1088 exists solely to handle that case.

In that situation the sequence is: the successor registers and reports True, the predecessor's own probe then re-asserts True and takes record ownership back, and the predecessor's __del__ writes a False that is now honoured — against an app the successor is serving. Driven through the real ProxyDeployment and BioEngineProxyActor, that ends at False; the same sequence on the parent commit ends at True, because the replica-tag guard drops the departing replica's write.

So this introduces a wrong-False window 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 False permanently, and now heals on the serving replica's next probe.

Behavioural changes

  • The registration record is periodically re-asserted by whichever replica can prove its address resolves, rather than written only at registration and deregistration time. Nothing else about the gate changes: the worker still withholds a service id and the static_site_url on False, and still falls back to the replica-alive gate when an app has never reported.
  • The re-assert sits below the entry_deployment_ready gate 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.
  • A failing probe schedules a client rebuild and asserts nothing.

Test plan

Four tests added to tests/apps/test_service_id_registration_gate.py, all driving a real ProxyDeployment into a real BioEngineProxyActor rather than asserting on call shape:

  • test_a_successor_that_never_registers_cannot_pin_a_serving_app_at_false reproduces 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 with AssertionError: assert False is True and passes with the fix.
  • test_the_probe_does_not_re_register_an_app_that_deregistered_itself pins 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_registered pins 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_register holds 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 skipped against a 518 passed, 25 skipped baseline on the parent commit — additive, no regressions.

Files

  • bioengine/apps/proxy_deployment.py — report True on the successful probe branch of _maintenance_tick, gated on a re-read of entry_deployment_ready.
  • bioengine/cluster/proxy_actor.py — docstring on report_service_registration no longer claims nothing re-reports True after 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

nilsmechtel and others added 3 commits September 27, 2026 01:55
… 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>
@nilsmechtel
nilsmechtel marked this pull request as ready for review September 27, 2026 01:05
@nilsmechtel
nilsmechtel merged commit e6781b8 into main Sep 27, 2026
2 checks passed
@nilsmechtel
nilsmechtel deleted the fix/proxy-registration-self-heal branch September 27, 2026 01:06
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