Repository navigation
rpc : do not abort the process when the remote server fails - #26724
erwinzhang7 wants to merge 1 commit into
Conversation
|
Hi @erwinzhang7, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
Why should it not take the whole process down? This is a fail fast approach. Server can be restarted automatically if it is running as a service(or under a watchdog) and use selfhealing to mitigate some of the issues. If situation was caused by a transient problem automatic system recovery is possible through restart.
If I'm reading this right the server would return a healthy status while being nonoperational. |
|
Yeah, you're right wrt the health check and it's a defect this PR introduces. On fail-fast, I think the two halves differ. The server half replaces a GGML_ASSERT with the Proposed fixes:
I suggest 1, fallback 2. Either way the "after: /health still ok" row in my description is wrong |
… plan All three Codex review comments on PR #15 were valid. P1, and it is the substantive one. I called upstream's zero-fill of get_tensor "defensible" and recommended adopting it as Phase 4.2. Two of this repository's own statements rule that out as a COMPLETE answer: - task_plan.md:3-4 sets the milestone goal as "the ring detects it, re-plans, and keeps working, WITH NO CORRUPTED OUTPUT and no deadlock". Zeroed logits are corrupted output. - Load-bearing fact #4 holds that silent corruption is worse than a hang, "because a hang is visible". Zero-fill converts an uncatchable abort into exactly that silent case for any caller that does not decode again. And this project would be a direct victim rather than a bystander, which the reviewer did not say and which sharpens it: T1 and T2 are BOUNDED SINGLE GENERATIONS. A peer failing mid-run under zero-fill hands the harness a complete-looking result and a plausible tokens/sec number from a broken run -- F27's failure mode exactly, in the measurement the product decision rests on. Corrected position, which is narrower than either "superseded" or a rejection: - For upstream, zero-fill is a clear improvement. An abort takes the process down; a zeroed decode that fails cleanly on the next one is recoverable. ggml-org/llama.cpp#26724 deserves to land. - For ShareCompute it is necessary but NOT SUFFICIENT. The host must be able to ask whether an endpoint has failed BEFORE consuming logits -- the other option F26 listed, now required rather than optional. - Phase 4.2 = adopt #26724, then add that query and check it in the token loop. Materially smaller than writing the whole thing, but not zero. P2, cross-repo references. Bare #26724 and #28487 render as links to ShareCompute in GitHub markdown -- wrong repo or a 404 -- regardless of the surrounding prose naming llama.cpp, which is exactly what the reviewer said. Every reference is now either fully qualified as ggml-org/llama.cpp#NNNNN or backticked where it is a short back-reference. This matters more than usual for a research-only change whose entire value is pointing readers at upstream. P2, the canonical plan. I updated CLAUDE.md and findings.md but not task_plan.md, which the README directs contributors to for phase status -- so someone following it would still have scheduled the parallel implementation this change exists to prevent. task_plan.md now records the supersession, the residual work, and the next concrete action. Verified: roster lint clean; every bare cross-repo reference confirmed gone by grep; diff is three markdown files and touches no code. Not verified: could NOT run swift test -- the container was recycled and the Swift toolchain at /opt/swift is gone along with the llama.cpp clone and the models. Nothing here touches Swift, and CI's "swift test on Linux" job covers it. Also still unverified, unchanged from the previous commit: ggml-org/llama.cpp#26724 has not been checked out, built or run here, and its behaviour against T1's harness is untested. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mw3YcQ8S2yKc7SLLbkMXkk
F32 read ggml-org/llama.cpp#26724 on GitHub and closed with "has not been checked out, built, or tested here". This does that. Method: the PR's own merge-base (9ba73fd1f) is the control, not master and not F25's 9d57ce4. The PR is four weeks newer than F25's measurement, so without a matched control any change would be attributed to the PR rather than to unrelated upstream churn. Both builds confirmed distinct before running. Result, REPEATS=5, same host and model: control 9ba73fd1f 5/5 aborts 402-417 ms exit 134 pr26724 a911a9bf4 0/5 aborts 124-159 ms exit 0 The abort is gone and detection is ~2.9x faster. The chain to llama_decode ret = -3 has now executed rather than been read. Two results the PR's own testing does not have: - Zero-fill emits one corrupted token, 5/5. A stray punctuation token is sampled from the zeroed logits 0.8 ms after the fill and 18 ms before graph_compute notices, so it reaches user-visible output. Always a low token id, because equal logits leave top-k holding the lowest indices. F32 argued this from task_plan.md's "no corrupted output" goal; it is now measured. - llama-cli exits 0 after a peer dies. Checked against the control before attributing it: the control also exits 0 against an unreachable endpoint. Pre-existing CLI behaviour, not the PR's. Also: the failed-endpoint latch is insert-only with no public query, and add_server's reg_map has the same shape and predates the PR. That is MLX's function-local static group cache in a second runtime, so epochs-not-mutation is the shape these runtimes force rather than an MLX workaround. New load-bearing fact #11. Harness fixes, both found by running it: - Ports sat inside ip_local_port_range (32768-60999), so llama-cli's own outbound connections could take the port a server then failed to bind. One run in three, and an intermittent bind failure silently yields a one-peer run. Ports moved below the range, with retry. - serve()'s bind guard called exit 1 from inside a command substitution, so it killed the subshell and nothing else, while carrying a comment claiming it exits the script. Now sets globals. throughput.sh had the identical defect at all three call sites. run.sh also gained distinct-device assertions on every run, a generation detector that counts only bytes after the echoed prompt, split stdout/stderr, a diagnostic regex matching both message forms, and a summary line so a mixed result cannot read as uniform. F27's absolute throughput did not reproduce (133.0 PP vs 217.3). Cause not isolated; recorded rather than republished. Ratios hold, and ratios are all F27 supported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mw3YcQ8S2yKc7SLLbkMXkk
|
Independent before/after test of this PR, in case it's useful for review. I built Setup: two
The control aborts at Works as described. One observation on the
|
| run | generated text | final token |
|---|---|---|
| 1 | The ocean, a vast and complex ecosystem, is a vital part & |
5 '&' |
| 2 | The ocean is a vast and beautiful body of water that covers the - |
12 '-' |
| 3 | The Ocean: A Vast and Intricate Realm … The + |
10 '+' |
| 4 | The ocean, often referred to as the "sea", is an + |
10 '+' |
| 5 | The ocean, a vast expanse of water that envelops the * |
9 '*' |
Always a low token id, which is what all-zero logits produce: equal values leave the top-k filter holding the lowest indices, and those are punctuation in this vocab. The control never produces one, because it aborts first.
I don't think this argues against the approach — a stray token followed by a hard error is plainly better than an uncatchable abort(). It just means the comment at that site, "zero the destination so that partial or stale data is never used", is narrower in practice than it reads: the zeroed data isn't used by ggml, but it does reach the sampler. Might be worth either saying so explicitly in the comment, or having llama_context discard the ubatch when the decode that produced it failed.
Question on the failed-endpoint set
get_failed_endpoints() is insert-only — no erase, no clear — and rpc_endpoint_is_failed is file-static with nothing exposed in ggml-rpc.h. Your own test notes this ("a failed endpoint is latched in a process-wide set that is never cleared").
For a one-shot CLI that's fine. For a long-lived host that wants to reconnect after a worker restarts, it means the endpoint string is permanently unusable and there's no way to ask why. Is a public query and/or a reset intended as follow-up work, or deliberately out of scope here? Happy to test a patch if you add one.
Not tested
Loopback only, no real network hop. Only a hard kill -9 — a graceful shutdown or a silently dropped connection are different paths. I didn't run tests/test-rpc.cpp, only read it. Also worth noting llama-cli exits 0 after this failure, so the error is visible only in the log — but that's pre-existing CLI behaviour, not this PR's: the control build also exits 0 against an unreachable endpoint.
…erver loss Perf: implement cpy_tensor_async for host-readable and same-endpoint copies so split uploads queue on the ordered stream and overlap local work instead of blocking behind two synchronizes plus a sync round trip. Large copies keep the sync path. Coalesce the command header into one write. Grow socket buffers to 4 MiB, tunable via GGML_RPC_TCP_BUFFER_SIZE. Hardening (adapted from upstream ggml-org#26724 for the dispatcher design): a lost RPC server marks its endpoint failed instead of aborting the process. Graph compute returns FAILED so the server answers HTTP 500 and stays up. Adds MSG_NOSIGNAL/SO_NOSIGPIPE so a dead peer cannot raise SIGPIPE. Assisted-by: opencode
Losing an RPC server took down both processes: the worker asserted on a failed graph compute, and the client aborted on any failed send or recv. Client: rpc_dispatcher latches the failure and graph_compute then returns GGML_STATUS_FAILED. The latch lives on the dispatcher, so it goes away when the connection is recreated. Responses are zeroed on failure so a caller never reads stale data. Sockets set SO_NOSIGPIPE / MSG_NOSIGNAL so a send to a dead peer returns an error instead of raising SIGPIPE. Server: a failed graph compute logs and returns false, like the deserialization errors above it, and the accept loop keeps serving. llama-server: llama_decode returning a fatal error now marks the HTTP server unhealthy, so /health reports 503 instead of ok once the device is gone.
…n-54mhwx Phase 4.2 — ggml-org/llama.cpp#26724 built and run against a matched control (F33)
|
Thanks for the review @joeydd032995-pixel, and @Kononnable, I've updated the code as suggested. Changes:
|
a911a9b to
731149a
Compare
Overview
Fixes the second half of #26384. Losing an RPC server is not recoverable, but it should not take the whole process down with it. Today it takes down both.
Server,
ggml-rpc.cpp:1456and:1472: a failed graph computation hitsGGML_ASSERT, so any backend error kills the worker and closes its socket mid-operation.Client,
RPC_STATUS_ASSERTatggml-rpc.cpp:30, which expands toGGML_ABORT, reached from 18 call sites: any failed send or receive kills the coordinator. A worker dying for any reason at all, OOM, a crash, a dropped link, a closed laptop lid, takes the coordinator with it.The Change, The server logs the failed status and returns
false, the same shape as the deserialization errors a few lines above it.rpc_serve_clientcloses that connection and the accept loop keeps the daemon up for the next client. The client marks the endpoint failed and returnsGGML_STATUS_FAILEDfromggml_backend_rpc_graph_compute. That status is already plumbed the whole way:ggml_backend_sched_compute_splitspropagates it,llama_decodeturns it into-3, and the server turns that into an HTTP 500 with the slots released. Nothing outsideggml-rpc.cppchanges. The buffers the remote held are gone and every local tensor pointing into them is invalid, so a failed endpoint stays failed. Later operations fail fast rather than pretending to work. This is "fail cleanly", not recovery: there is no reconnect, retry or failover here, and a hung peer still blocks inrecvbecause the sockets have no timeouts. Marking the endpoint follows thehas_errorlatch the Metal backend uses.Results
Same fault injected in both directions, on the same two machines:
ggml-rpc.cpp:1457: GGML_ASSERT(status == GGML_STATUS_SUCCESS ...) failedgraph computation failed on device 0, status: -1/healthok — a defect, see discussion500 Compute error.immediatelyBoth halves were exercised separately: a forced
GGML_STATUS_FAILEDingraph_computefor the server side, and killing the worker mid-generation for the client side. The error surfaces asNormal multi-node generation is unaffected. Measured with
llama-benchover the same two machines, against a binary built from the parent commit with only this patch differing:The added cost on the hot path is one mutex and one hash lookup per graph compute per RPC device, which is not measurable next to the network round trip already being paid.
Also run:
CI (rpc)(build-rpc.yml, the workflow gated onggml/src/ggml-rpc/**): green, includingctest -L maintest-backend-ops: 3/3 backends pass. This does not exercise the RPC backend, since RPC devices are only registered through--rpcincommon/arg.cppand that binary does not link it. The RPC coverage is the two-machine testing above.ci/run.shwithGG_BUILD_METAL=1: 100% passed, 0 failed, across both suites (45 and 48 tests)Additional information
Reproduction
Two Macs, RPC over a Thunderbolt bridge,
llama-serveron one andggml-rpc-serveron the other with--tensor-split. Killing the worker during generation:The coordinator is gone, mid-request, along with every other slot it was serving.
No protocol change
There is no wire format change and no version bump. Returning a status byte from
GRAPH_COMPUTEwould need a major bump rather than a minor one:ggml-rpc.cpp:347accepts a client newer than the server, so a new client against an old server would sendGRAPH_COMPUTE, wait for a response that never comes, and block forever, while the server blocks reading the next command. There are no socket timeouts anywhere in the transport, so that is a permanent silent hang, which is worse than the abort being fixed here. 15d2b46 also removed that response deliberately to save a round trip, and re-adding it would not have prevented the crash above, which came throughbuffer_get_tensor.Known limitation
ggml_backend_tensor_getreturnsvoid, so one read path has no way to report failure to its caller. It zeroes the destination and marks the endpoint, so the worst case is a single decode returning zeroed output before the next one fails cleanly, rather than stale or partial data. Closing that properly means givingget_tensora return status, which changes an interface every backend implements and does not belong in a crash fix.Cost on the hot path is one mutex and one hash lookup per graph compute per RPC device.
Requirements