fix(worker): rebuild the Hypha client when the server evicts it - #180
Draft
nilsmechtel wants to merge 1 commit into
Draft
fix(worker): rebuild the Hypha client when the server evicts it#180nilsmechtel wants to merge 1 commit into
nilsmechtel wants to merge 1 commit into
Conversation
Hypha can drop the worker's client registration while the socket stays
open from the container's side. hypha-rpc never sees a disconnect, so it
never reconnects, and `<workspace>/<client-id>:bioengine-worker` stops
resolving forever while the process stays alive and every probe passes.
The monitoring loop's `echo("ping")` cannot see this: it keeps succeeding
against a server that has stopped serving us.
The loop now also asks whether Hypha still serves the worker's own
service id, once a minute, and rebuilds the connection when it does not.
The probe rebuilds and never condemns — a Hypha outage that briefly
serves nothing recovers on its own, and a redundant rebuild there costs
one reconnect while feeding the degraded counter would cycle a pod that
was about to come back.
`_connect_to_server` also bounds its `disconnect()`. That close now runs
on the rebuild path, where the transport being closed is the one already
suspected of being wedged.
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.
The worker's own Hypha client never recovers from a server-side eviction: Hypha drops the client registration while the socket stays open from the container's side, so hypha-rpc never sees a disconnect, never reconnects, and
<workspace>/<client-id>:bioengine-workerstops resolving forever. The process stays alive,kill -0 1passes, the liveness probe passes, and nothing is logged. Observed on 2026-09-01 while validating #167, reproduced with adocker pauseof 150–260 s.The monitoring loop's existing
echo("ping")cannot see this. It kept succeeding for ten minutes against a server that had already dropped the registration — an open socket answers regardless of what the registry holds. So the loop now also asks whether Hypha still serves the worker's own service id, once a minute, and rebuilds the connection when it does not. This is the same decision PR #167 made forProxyDeployment; in the incident that motivated this issue the two halves of a single process disagreed, with the worker clientMISSINGand the app proxyRESOLVEDbecause the proxy's maintenance loop noticed within one 60 s probe and rebuilt in ~5 s.The probe rebuilds; it never condemns
This is the part worth reviewing, because the tempting version is wrong.
There is a second, unrelated failure mode on deNBI: four dropouts in 47.4 h in which Hypha briefly served nothing — worker, model-runner and cellpose3-runner all gone together, two different pods, both
restartCount=0, no Killing/Started events. Those episodes self-heal. cute-mare's paired registry sampler puts them at 7 consecutive failed probes per episode (1×500 then 6×404), a failing span of ≥191–192 s, an outage window ≤252 s, and a duration MLE of 222 s (band 206–238 s). Meanwhile/health/livenessreturned 200 on every 30 s sample throughout all four.So a 3-consecutive-miss rule at a 30 s interval would condemn at ~90 s and would have killed a recovering worker four times, and neither endpoint alone yields a correct condemn decision. The discriminator between the two failure modes is recovery, not any single sample: this issue never self-heals, the sibling always does at a fixed ~200 s.
_check_service_registrationtherefore never raises and never reports its outcome upwards — in particular it never touches_monitor_consecutive_errors, which is what flipsget_statusto not-ready and lets the k8s liveness probe cycle the pod. A redundant rebuild during a recoverable outage costs one reconnect. A condemn kills a worker that was about to come back. Two tests pin this, one behavioural and one on the source of the method itself.The bounded disconnect
_connect_to_serverclosed the previous connection with an unboundedawait self.server.disconnect(). That was harmless while the only caller was startup and an echo-detected dead socket; it is not harmless now, because the close runs on the rebuild path, where the transport being closed is by definition the one suspected of being wedged. It is now bounded at 5 s so a hung close cannot stall the monitoring loop on exactly the socket that prompted the rebuild.One correction to the issue's framing while I was in here. The issue expected the worker client to carry "the worker service AND every app service registered through it", making a stranded registration worse than on the proxy. It does not:
register_serviceappears in exactly two places, and app services are registered by eachProxyDeploymenton its own client. The worker client carries one service plus the artifact-manager and app-builder handles derived from it, andcomplete_initializationre-points those at the new connection on every rebuild. The blast radius is smaller than assumed, which is why bounding the close was enough rather than needing a lock or a generation counter.Not folded in
The proxy's
_RECONNECT_GRACE_S/_REREGISTER_BACKOFF_Spair is not lifted. The proxy needs a grace window because it rebuilds from theon_disconnectedhook, which fires for drops hypha-rpc goes on to repair itself — its first version rebuilt three times in 30 s during a real wobble and crashed. The worker has no such hook: the 60 s probe interval is itself the rate limit, at most one rebuild attempt per minute, so a second backoff constant would be dead weight. If review prefers the constants shared between the two rather than chosen per site, that is a reasonable call to make now rather than later.Tests
11 new tests in
tests/worker/test_worker_registration_probe.py. 216 passed on the branch against 205 onorigin/mainunder the same invocation, with the same 55 pre-existing--noconftestcollection errors.Five positive controls, each failing exactly the tests that name the behaviour and no others:
echo("ping")instead of the registration lookuptest_a_live_socket_with_a_dropped_registration_still_rebuildsraiseafter a failed rebuildtest_the_probe_is_not_wired_into_the_readiness_backstoptest_the_probe_costs_one_round_trip_per_intervaldisconnect()test_a_hung_disconnect_does_not_stall_the_rebuildtest_the_monitoring_loop_runs_the_registration_probeNot yet validated on a live cluster — this is
bioengine/**, so it needs a dev image and adocker pausereproduction before it is marked ready. It is the sixth PR queued on that same validation call, after #175, #176, #177, #178 and #179.