Fix runner-reconnect cascade: reset retry backoff on success, wait for teardown before recreate - #2244
Open
alytaphoenix wants to merge 1 commit into
Open
Conversation
…r teardown before recreate Investigated a report of a model failing to load on a two-node TB5 cluster. The instance actually loaded and served chat requests fine, then got dragged through a Shutdown/CreateRunner/ConnectToGroup cycle roughly every couple of minutes (not user-initiated), which sometimes crashed with "[jaccl] Changing queue pair to RTR failed with errno 16" (EBUSY) and was eventually left permanently unloaded. Root cause, as far as could be verified on this node's log alone: - plan.py's _kill_runner correctly shuts down every rank of an instance when any one rank fails (a half-pipeline is useless) -- this is by-design and left alone. - But main.py's per-instance retry-backoff counter (_instance_backoff) only ever reset on InstanceDeleted, never on a successful reconnect. So a node dragged into a restart cycle by a *sibling's* crash silently burned its own retry budget for a fault that was never its own, and could independently hit the 5-attempt cap and request deletion of an instance it was otherwise serving fine. - Separately, main.py's Shutdown handling popped the runner out of self.runners (unblocking the next CreateRunner) before the runner's underlying OS process had actually finished tearing down -- runner.shutdown() only requests cancellation; the real process kill and RDMA/queue-pair release happens asynchronously in the RunnerSupervisor.run() task, which nothing awaited. A fast Shutdown->CreateRunner cycle for the same instance could plausibly race that teardown, producing exactly the EBUSY symptom. Fixes: 1. worker/plan.py: new instance_to_reset_backoff() -- resets an instance's retry backoff when its local runner reaches RunnerReady/RunnerRunning (actually serving), not earlier states like RunnerConnected. Resetting at Connected would defeat the circuit breaker: a rank that connects fine but crashes every LoadModel (bad weights, OOM) would loop forever instead of eventually giving up. Wired into worker/main.py's _event_applier alongside the existing InstanceDeleted reset. 2. worker/runner/supervisor.py: RunnerSupervisor now exposes wait_stopped(), backed by an anyio.Event set once run()'s teardown (including the actual runner_process.stop()) has finished. worker/main.py's Shutdown handling now awaits this (bounded to 15s) before considering the slot free, closing the race between tearing down the old runner and creating its replacement. 3. master/main.py: the "kill broken instances" topology-eviction path had zero logging before silently sending InstanceDeleted. Added a warning naming the instance and the missing node, so a recurrence is attributable to this path instead of indistinguishable from the worker-side backoff path. Known limitations / behavior changes, called out for review: - I could not confirm which mechanism actually killed the reported instance. Master runs on a different physical node in this cluster, and its log (which would show whether master's topology-eviction path fired) isn't available from here. Fix 3 (the new logging) is what makes a recurrence diagnosable; this PR should not be read as a confirmed fix for "the model won't load," only for the two concrete bugs found by code inspection. - Behavior change: resetting the backoff on every successful Ready/Running means a *flapping* instance (repeatedly reaches serving, then dies, over and over) will now retry forever instead of giving up after 5 total attempts -- since each successful reconnect resets the counter back to 0. This seems like better UX (a transient hardware hiccup shouldn't cause permanent data loss of a model that keeps mostly working) but is a real change from "give up after 5 lifetime attempts" to "give up after 5 *consecutive* failures." Flagging in case the lifetime cap was intentional. - Fix 2 (wait_stopped) is unit-tested for its actual contract (blocks until the OS process exits, safe to await twice), but the EBUSY race it targets is RDMA-timing on hardware unavailable in this environment -- the fix is unverified against the real failure it's meant to close. Also, in the crash-cascade case, the healthy rank's generator.close() tears down a distributed group whose peer just died, which could itself stall -- the 15s timeout is what bounds that, not a guarantee it resolves quickly. Worth a look from someone with two-node TB5 hardware. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rjSfwDBTkmySmfU6NgHKF
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.
Summary
Investigated a report of a model that loaded and served chat requests fine on a two-node TB5 cluster, then got dragged through repeated
Shutdown -> CreateRunner -> ConnectToGroupcycles roughly every couple of minutes (not user-initiated) — some of which crashed with:(errno 16 = EBUSY). The instance was eventually left permanently unloaded.
Root cause (as far as verifiable from one node's log)
plan.py's_kill_runnercorrectly shuts down every rank of an instance when any one rank fails (a half-pipeline is useless) — this is by-design and untouched here.main.py's per-instance retry-backoff counter (_instance_backoff) only ever reset onInstanceDeleted, never on a successful reconnect. So a node dragged into a restart cycle by a sibling's crash silently burned its own retry budget for a fault that was never its own, and could independently hit the 5-attempt cap and request deletion of an instance it was otherwise serving fine.main.py'sShutdownhandling popped the runner out ofself.runners(unblocking the nextCreateRunner) before the runner's underlying OS process had actually finished tearing down.runner.shutdown()only requests cancellation; the real process kill and RDMA/queue-pair release happens asynchronously in theRunnerSupervisor.run()task, which nothing awaited. A fastShutdown -> CreateRunnercycle for the same instance could plausibly race that teardown — producing exactly the EBUSY symptom.Fixes
worker/plan.py: newinstance_to_reset_backoff()— resets an instance's retry backoff when its local runner reachesRunnerReady/RunnerRunning(actually serving), not earlier states likeRunnerConnected. Resetting atConnectedwould defeat the circuit breaker: a rank that connects fine but crashes everyLoadModel(bad weights, OOM) would loop forever instead of eventually giving up. Wired intoworker/main.py's_event_applieralongside the existingInstanceDeletedreset.worker/runner/supervisor.py:RunnerSupervisornow exposeswait_stopped(), backed by ananyio.Eventset oncerun()'s teardown (including the actualrunner_process.stop()) has finished.worker/main.py'sShutdownhandling now awaits this (bounded to 15s) before considering the slot free, closing the race between tearing down the old runner and creating its replacement.master/main.py: the "kill broken instances" topology-eviction path had zero logging before silently sendingInstanceDeleted. Added a warning naming the instance and the missing node, so a recurrence is attributable to this path instead of indistinguishable from the worker-side backoff path.Known limitations / behavior changes — please read before merging
wait_stopped) is unit-tested for its actual contract (blocks until the OS process exits, safe to await twice) using a real subprocess — but the EBUSY race it targets is RDMA-timing on hardware unavailable in this environment, so the fix is unverified against the real failure it's meant to close. Also worth noting: in the crash-cascade case, the healthy rank'sgenerator.close()tears down a distributed group whose peer just died, which could itself stall — the 15s timeout bounds that stall, it doesn't guarantee a quick resolution. Would appreciate a look from someone with two-node TB5 hardware.Test plan
instance_to_reset_backoff()(test_plan/test_backoff_reset.py): resets on Ready/Running, does not reset on Connected (circuit-breaker-preserving), ignores unknown runners and non-status events.wait_stopped()(test_runner/test_runner_supervisor.py): spins up a real subprocess viaAsyncProcess, confirmswait_stopped()blocks until the process actually exits, and is safe to await again afterward.uv run basedpyright(full repo) — 319 errors / 110 warnings, identical tomainbaseline (pre-existing, mlx not installed in this environment)uv run ruff check(full repo) — all checks passeduv run ruff format --check— all touched files already formatteduv run pytest src/exo(excluding mlx-dependent dirs) — 341 passed, 3 skipped, no regressionsnix fmt/nix flake checknot run in this environment (ruff substituted)🤖 Generated with Claude Code
https://claude.ai/code/session_011rjSfwDBTkmySmfU6NgHKF