rpc: do not abort on shutdown when a teardown round trip fails - #203
rpc: do not abort on shutdown when a teardown round trip fails#203danielhanchen wants to merge 3 commits into
Conversation
llama-server built with GGML_RPC aborts on exit, after it has printed "cleaning up before exit", inside ggml_backend_rpc_get_device_memory called from common_memory_breakdown_print: ggml-rpc.cpp: Remote RPC server crashed or returned malformed response #3 ggml_backend_rpc_get_device_memory #4 common_memory_breakdown_print(llama_context const*) #5 llama_server(common_params&, int, char**) The memory breakdown is printed after clean_up(), which calls llama_backend_free(). The RPC device answers get_memory with a round trip to the peer, and by that point the round trip fails, so RPC_STATUS_ASSERT aborts the process. Reproduced with a peer rpc-server that is alive and healthy the whole time, so this is not about the peer going away first. Device memory is an informational property, not part of the data path, and the function already has a defined answer for an endpoint it cannot reach: report 0/0. Report the same when the query itself fails. Freeing a remote buffer is released state and is also a teardown operation, so a failure there is logged instead of aborting; if the peer is gone then so is the buffer. Every other RPC_STATUS_ASSERT, including all the data path ones, is unchanged. Confined to ggml/src/ggml-rpc/ggml-rpc.cpp, which is compiled only when GGML_RPC is on (OFF by default, ggml_add_backend(RPC) in ggml/src/CMakeLists.txt), so a build without RPC is untouched.
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
No code change: every remaining line carries a fact the code does not state.
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Simulation to destructionFault injected deliberately, never waited for. Base Why the earlier attempts mis-tested thisInjecting a fault on a timer after SIGINT does not work: the teardown round trip is microseconds wide, so a reset 20 or 50 ms after the signal always lands outside it. Those cells come back clean on both arms and say nothing. Cases c7 and c8 below are exactly that, kept in the table as a negative control. What does work is a proxy that parses the client-to-server RPC framing ( Single node,
|
| case | fault | base | head |
|---|---|---|---|
| c1 | healthy peer, ordinary shutdown | CLEAN rc=0 | CLEAN rc=0 |
| c2 | peer SIGKILLed before shutdown |
ABORT rc=134 | CLEAN rc=0, 1 "reporting 0", 3 "connection is gone" |
| c3 | peer SIGTERMed before shutdown |
ABORT rc=134 | CLEAN rc=0, same |
| c4 | RST injected by the proxy, peer process still alive | ABORT rc=134 | CLEAN rc=0 |
| c5 | orderly FIN from the peer side, peer process still alive | ABORT rc=134 | CLEAN rc=0 |
| c6 | peer accepts and never answers | HANG | HANG, unchanged, see below |
| c7 | peer killed 50 ms after SIGINT | CLEAN | CLEAN (fault missed the window) |
| c8 x5 | RST 20 ms after SIGINT, five repetitions | CLEAN x5 | CLEAN x5 (fault missed the window) |
| c9a | RST the instant RPC_CMD_GET_DEVICE_MEMORY is issued |
ABORT rc=134 at ggml-rpc.cpp:831 |
CLEAN rc=0, 1 "reporting 0", 3 "connection is gone" |
| c9b | RST the instant RPC_CMD_FREE_BUFFER is issued |
ABORT rc=134 at ggml-rpc.cpp:397 |
CLEAN rc=0, 3 "connection is gone" |
c9b is what shows the second hunk is load bearing rather than defensive: with the reset landing on the free, base aborts at ggml_backend_rpc_buffer_free_buffer (ggml-rpc.cpp:397), not at get_device_memory, exactly as the description claims.
The base stack for c2 is the reported one, verbatim:
ggml/src/ggml-rpc/ggml-rpc.cpp:831: Remote RPC server crashed or returned malformed response
#4 get_device_memory (...) ggml-rpc.cpp:831
#5 ggml_backend_rpc_get_device_memory (...) ggml-rpc.cpp:843
#6 common_memory_breakdown_print (...) common/fit.cpp:977
#7 llama_server (...) tools/server/server.cpp:545
Two nodes, the topology the report came from
ggml-rpc-server on the second Spark, llama-server here, over the LAN.
| case | base | head |
|---|---|---|
| n1 healthy peer, ordinary shutdown | CLEAN rc=0 | CLEAN rc=0 |
n2 remote peer SIGKILLed before shutdown |
ABORT rc=134, core dumped | CLEAN rc=0, 1 "reporting 0", 3 "connection is gone" |
n3 remote peer SIGTERMed before shutdown |
ABORT rc=134, core dumped | CLEAN rc=0, same |
The description contradicts its own correction
The "Correction, 2026-09-08" paragraph says a healthy peer does not reproduce the abort. The "What was measured" list immediately below still says
Before, peer alive throughout: abort, core dumped, same stack.
Measured here on both topologies (c1 and n1), base with a healthy peer exits cleanly, rc=0, with no assert line. That line needs deleting.
Two things this does not fix, both worth saying out loud
1. A peer that accepts and never answers still hangs, on both arms (c6). ggml/src/ggml-rpc/transport.cpp sets TCP_NODELAY and SO_REUSEADDR and nothing else: no SO_RCVTIMEO, no SO_SNDTIMEO, no connect timeout. socket_t::impl::recv_data() loops on recv(fd, ..., 0) until it has the bytes, gets 0 (peer sent FIN, POSIX: "if the peer has performed an orderly shutdown, recv() shall return 0", https://pubs.opengroup.org/onlinepubs/9699919799/functions/recv.html) or gets -1. A peer holding the socket open and sending nothing produces none of those, so shutdown blocks forever and SIGINT is never serviced. Separate defect, not a regression, worth a follow up.
2. This fix is only effective in a process that ignores SIGPIPE. transport.cpp calls send(fd, ..., 0) with no MSG_NOSIGNAL, and nothing in the RPC backend installs a handler or sets SO_NOSIGPIPE. On Linux send() to a socket whose peer has sent RST "generates SIGPIPE ... unless MSG_NOSIGNAL is set" (https://man7.org/linux/man-pages/man2/send.2.html) and SIGPIPE's default disposition is Term (https://man7.org/linux/man-pages/man7/signal.7.html). The only place in the tree that ignores it is tools/server/server.cpp:93, added for MCP child processes, which is why llama-server reaches this code path at all. llama-cli, llama-bench and embedders do not. I could not reproduce a SIGPIPE death: with llama-cli --rpc and the peer killed mid-session, the first failing operation on this kernel is a recv() returning 0, so RPC_STATUS_ASSERT in negotiate_hello() (ggml-rpc.cpp:348) fires first, identically on both arms. So this is a source-level hazard I am reporting, not one I demonstrated. Follow up: one MSG_NOSIGNAL on Linux, SO_NOSIGPIPE on macOS/BSD, nothing needed on Windows.
Also unchanged and untouched here: negotiate_hello() still has an RPC_STATUS_ASSERT, so a reconnect to a half-dead peer still aborts. That is a connect-time path, not a teardown path.
Windows and macOS
Neither can be executed here; this is a pair of Linux aarch64 DGX Sparks. From source and documentation:
- Windows. No SIGPIPE on Winsock at all and no
MSG_NOSIGNAL, so point 2 does not apply there.recvreturns 0 on a graceful close and fails withWSAECONNRESETon an abortive one (https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-recv), both of whichrecv_data()already folds intofalse.closesocketwithSO_LINGER {1, 0}sends an RST and "any recv call on the remote side of the circuit will fail with WSAECONNRESET" (https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-closesocket), which is the Windows form of the c4 and c9 injections. The changed code only reads theboolthatsend_rpc_cmd()returns, so it behaves identically. The c6 hang is if anything worse on Windows becauseclosesocketon a non-blocking socket with a non-zero linger can fail withWSAEWOULDBLOCKand leave the handle valid, but that is pre-existing. - macOS.
MSG_NOSIGNALis not portable there, the BSD spelling issetsockopt(SO_NOSIGPIPE); same conclusion as Linux. The Apple RDMA transport (transport-apple.cpp) is a differentsend/recvpath entirely and was neither built nor run.
Answers to the four questions
- What happened before. With an unreachable peer,
llama-serverbuilt withGGML_RPCaborts on exit after printing "cleaning up before exit", insideRPC_STATUS_ASSERTinget_device_memory()fromcommon_memory_breakdown_print(). If that one is survived it aborts a moment later inggml_backend_rpc_buffer_free_buffer(). Reproduced in 8 of the 8 injections that actually reach the teardown round trip, across five fault shapes and two topologies. - What happens after. Both teardown operations log and continue. Device memory reports 0, which is already the answer for an endpoint that cannot be connected to at all.
- Real or fake. Real, and the corrected scoping is right: it is not every shutdown of a server using
--rpc. With a healthy peer base exits cleanly. The trigger is an unreachable peer, which is the normal shutdown order for an orchestrated split. - Does merging break anything. No. Healthy shutdown is unchanged on both topologies, every other
RPC_STATUS_ASSERTincluding all the data path ones is untouched, and the file is only compiled underGGML_RPC, which defaults to OFF. Non-RPC builds, a single Spark, a single device and other multi-GPU systems cannot observe this change at all.
Defect report, not a review of this PRIssues are disabled on this repository, so this is filed here because it is the nearest neighbour What happensOn a layer split, if the peer's Every in-flight request dies with it. The clients see truncated HTTP responses Why this is worth an issue on its ownIt is reachable from a thermal event, not only from an operator killing a process. That is the Observed exactly that way. The thermal guard on the peer node saw 90 C, cancelled GPU work and and eight seconds later the coordinator aborted mid-generation. No operator did anything. Any Relation to PR #203PR #203 fixes two teardown paths, This issue is the broader case #203 explicitly does not cover: the peer disappearing during What a fix would need to decideThe data-path assert is defensible as a default: a graph that half executed on a peer that is gone
ReproductionTwo nodes, 27B layer split, |
1 participant
What happens
llama-serverbuilt withGGML_RPCaborts on exit, after it has already printedcleaning up before exit:Why
common_memory_breakdown_print()is called afterclean_up(), which callsllama_backend_free(). The RPC device answersget_memorywith a round trip to the peer,and by that point the round trip fails, so
RPC_STATUS_ASSERTaborts the process.This was first seen on a two node layer split where the peer
ggml-rpc-serverhad beenstopped before the local server exited.
Correction, 2026-09-08. This paragraph previously claimed the abort reproduces with a
peer that is alive and healthy, and therefore fires on every shutdown of a server using
--rpc. Re-tested on the pair, that is wrong: with a healthy peer the master exitscleanly. The trigger is an unreachable peer, which is the case the original report
came from. Three of the four cells below reproduce exactly as written, including the log
line counts, so the defect and the fix are unchanged; only this claim about how often it
fires was too broad.
That still makes it worth fixing rather than a corner case. A peer going away before the
coordinator exits is the normal shutdown order for a split: the orchestrator tears down
the remote servers first, and any crash, kill or network loss on the peer puts the
coordinator in exactly this state. An informational property should not abort a process
that has already finished its work.
The fix
Device memory is an informational property, not part of the data path, and
ggml_backend_rpc_get_device_memory()already has a defined answer for an endpoint itcannot reach at all: report 0 free and 0 total. Report the same when the query itself
fails, and log an error.
Freeing a remote buffer is released state and is also a teardown operation, so a failure
there is logged instead of aborting: if the peer is gone then so is the buffer, and the
local context is freed either way. Without this second hunk the process still aborts a few
microseconds later in
ggml_backend_rpc_buffer_free_bufferfromggml_backend_sched_free, on the way out of~llama_context.Every other
RPC_STATUS_ASSERT, including all of the data path ones, is unchanged. Afailure while setting or getting a tensor or running a graph still aborts.
The alternative, teaching
common_memory_breakdown_print()to skip backends whoseconnection has dropped, was rejected: that code is shared with every non-RPC build and has
no way to ask a backend whether it is still reachable, so it would need a new backend
interface entry for one caller. Caching the last known figure inside the RPC backend was
also rejected: it adds state to keep in sync in order to print a number that is meaningless
by the time it is printed.
What was measured
CPU only,
stories15M-q4_0.gguf,--rpc HOST:PORT --device RPC0 -ngl 99, one completionserved, then SIGINT. The verdict is the process exit status: 134 is SIGABRT, 0 is a clean exit.
A fixed delay after SIGINT does not test this. The teardown round trip is microseconds wide, so
a reset injected 20 or 50 ms after the signal always lands outside it and comes back clean on
both arms, saying nothing (rows c7 and c8 below, kept as a negative control). What does work is
a proxy that parses the client to server RPC framing (
cmd:1 | size:8 | payload) and resets theconnection the instant a chosen command byte is issued, before forwarding it.
RPC_CMD_GET_DEVICE_MEMORYis 11 andRPC_CMD_FREE_BUFFERis 4.Single node,
127.0.0.1:SIGKILLed before shutdownreporting 0, 3connection is goneSIGTERMed before shutdownRPC_CMD_GET_DEVICE_MEMORYis issuedggml-rpc.cpp:831reporting 0, 3connection is goneRPC_CMD_FREE_BUFFERis issuedggml-rpc.cpp:397connection is goneTwo nodes,
ggml-rpc-serveron a second DGX Spark over the LAN, which is the topology theoriginal report came from:
SIGKILLed before shutdownreporting 0, 3connection is goneSIGTERMed before shutdown8 of the 8 injections that actually reach the teardown round trip abort before and are clean
after, across five fault shapes and two topologies. c9b is the row that shows the second hunk
is load bearing rather than defensive: with the reset landing on the free, the abort is in
ggml_backend_rpc_buffer_free_bufferatggml-rpc.cpp:397, not inget_device_memory.A healthy peer exits cleanly before the change, on both topologies (c1 and n1). An earlier
revision of this description claimed otherwise; that claim was wrong and is corrected here.
Follow ups this does not fix
Neither is a regression and neither is introduced here, but both are in the same file and both
turned up while testing this change.
1. There is no socket timeout anywhere in the RPC transport, so a peer that accepts a
connection and then never answers hangs the shutdown forever.
ggml/src/ggml-rpc/transport.cppsets
TCP_NODELAYandSO_REUSEADDRand nothing else: noSO_RCVTIMEO, noSO_SNDTIMEO, noconnect timeout.
socket_t::impl::recv_data()loops onrecv(fd, ..., 0)until it has thebytes, gets 0 (peer performed an orderly shutdown) or gets -1. A peer holding the socket open
and sending nothing produces none of those, so the process blocks and SIGINT is never serviced.
Reproduced before and after this change alike, row c6 above.
2. The transport sends with no
MSG_NOSIGNAL, so an RPC client that is notllama-servercanbe killed by SIGPIPE before either the old abort or the new log line is reached. Nothing in
the RPC backend installs a SIGPIPE handler or sets
SO_NOSIGPIPE; the only place in the treethat ignores SIGPIPE is
tools/server/server.cpp, added for MCP child processes, which is whyllama-serverreaches this code path at all.llama-cli,llama-benchand embedders do not.This one is a source level hazard I could not demonstrate: with
llama-cli --rpcand thepeer killed mid session, the first failing operation on this kernel is a
recv()that returns0, so
RPC_STATUS_ASSERTinnegotiate_hello()(ggml-rpc.cpp:348) fires first, identicallybefore and after. Stated as unproven rather than as a reproduction.
Also unchanged and out of scope here:
negotiate_hello()still has anRPC_STATUS_ASSERT, so areconnect to a half dead peer still aborts. That is a connect time path, not a teardown path.
Non-RPC builds
The change touches one file,
ggml/src/ggml-rpc/ggml-rpc.cpp. That file is compiled onlythrough
ggml_add_backend(RPC)inggml/src/CMakeLists.txt, which adds the subdirectoryonly if
GGML_RPCis set, andGGML_RPCdefaults to OFF inggml/CMakeLists.txt. Noheader, no public signature and no shared code path changes, so NVIDIA, AMD and CPU only
builds without RPC are byte for byte unaffected.