Repository navigation
fix: cmd_serve reports success (exit 0) on a half-assembled pair — head up, worker gone - #17
Merged
Merged
Conversation
cmd_serve short-circuits on the HEAD container alone:
if docker ps ... | grep -qx "${HEAD_CONTAINER}"; then
ok "${HEAD_CONTAINER} already running"
curl ... || info "API not ready yet (still booting?)"
exit 0
A TP=2 group is not running because rank 0 is. If the head is up and the worker
is gone, rank 0 waits in NCCL rendezvous forever -- or against a stale rank 1 --
and this path prints 'already running', downgrades the failed readiness probe to
an advisory 'still booting?', and exits 0. The readiness curl cannot rescue it:
its result is discarded either way.
We hit exactly this. A head node was power-cycled; its container carried a
restart policy, so Docker restored rank 0 unordered against a leftover rank 1 on
the worker. The pair sat at 'Init torch distributed begin' indefinitely and
./start.sh reported success on it.
Now both ranks must be present to claim 'already running'. Head up + worker
absent is reported as a half-assembled pair with the recovery command, and exits
1. Deliberately does NOT auto-repair: tearing down a head that may still be
serving is the operator's call, and stop-then-start is already the documented
flow.
Validated on a live 2x GB10 pair. Both ranks up: unchanged, 'already running' +
'API is ready', exit 0. Half-assembled branch exercised by pointing
WORKER_CONTAINER at a nonexistent name so no container was touched: the four
error lines print and exit is 1, while the head stayed up and serving
(/v1/models 200, restarts=0). The worker predicate was checked in both
directions -- true for the real name, false for a bogus one.
Owner
|
Reviewed and merging. A TP=2 group is not running because rank 0 is. The old Requiring both ranks to claim “already running”, reporting half-assembled with The inverse (worker up, head down) correctly falls through into the normal worker-first launch. Isolated to the |
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.
cmd_servedecides the pair is already running by looking at the head container only:A TP=2 group isn't running because rank 0 is. With the head up and the worker gone, rank 0 waits in NCCL rendezvous indefinitely — or worse, against a stale rank 1 — and this path prints
already running, downgrades the failed readiness probe to an advisoryAPI not ready yet (still booting?), and exits 0. The curl can't rescue it: its result is discarded on both branches.How we hit it
A head node needed a power cycle. Its container carried a restart policy, so on boot Docker restored rank 0 unordered, against a leftover rank 1 still up on the worker from before the outage. The pair sat at
Init torch distributed beginand would have waited forever — and./start.shreported success on it. Recovery was: kill the worker container by hand, then./start.sh stop && ./start.shto get the worker-first ordering the script normally does so carefully.Worth noting the ordering point generally: dockerd cannot order two nodes, so worker-first is only expressible from the head, in this script. That makes this check the only place the invariant can be enforced.
The change
Both ranks must be present to claim "already running". Head up + worker absent is reported as a half-assembled pair, with the recovery command, exit 1.
It deliberately does not auto-repair. Tearing down a head that may still be serving is the operator's call, and
stopthenstartis already the documented flow.Validation
On a live 2× GB10 pair:
already running,API is ready, exit 0.WORKER_CONTAINERat a nonexistent name, so no container was touched: all four error lines print, exit is 1, and the head stayed up and serving throughout (/v1/models200,restarts=0). This seemed better than deliberately dismantling a serving pair to prove a branch.truefor the real container name,falsefor a bogus one.bash -n start.shclean. Branched frommainindependently of #13/#14/#15/#16; touches only thecmd_serveregion.