[fix] Make the controller survive bad ZMQ messages and repair the pickle fallback - #143
Merged
Conversation
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
CLA Signature Pass0oshowero0, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
CLA Signature Pass0oshowero0, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com> # Conflicts: # transfer_queue/controller.py
CLA Signature Pass0oshowero0, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
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.
Motivation
A long-running RL job hit an occasional failure that took the whole run down:
_process_requesthad no exception handling, so this one message killedTransferQueueControllerProcessRequestThreadpermanently. The controller stayed alive asan actor but stopped answering every request from that point on, and the job hung.
While investigating I found more defects on the same path.
What was wrong
1. One bad message killed the request loop.
_process_requestranrecv_multipart→deserialize→ dispatch →send_multipartwith no guard anywhere. Note that_wait_connectionin the same file already logs and continues; the request loop did not.2. Dropped requests hang the caller forever. Clients issue requests through
with_controller_socketwith noRCVTIMEOand then block onawait socket.recv_multipart(). Any controller path that fails to reply — an undecodable request,an unhandled request type, a handler that raises — leaves that caller waiting for the rest
of the run.
3.
decode()could never detect the pickle fallback marker. It located the marker withframes[0] == _PICKLE_FALLBACK_SENTINEL. Every receiver callsrecv_multipart(copy=False)and therefore holds
zmq.Frameobjects, andzmq.Framedefines no__eq__againstbytes, so the comparison was always False and the marker frame was handed to msgpack.4.
encode()almost never reached that fallback anyway. It caught onlyTypeError/ValueError, but msgspec reports unrepresentable values withOverflowError(an
ArithmeticError),RecursionError(aRuntimeError) and its ownMsgspecError—none of which derive from
ValueError. Those escaped to the caller instead of degradingto pickle, even though pickle handles all of them (arbitrary-precision ints,
self-referential containers).
5. Unhandled request types replied with a stale response. Every branch in the dispatch
chain is an
if/elifwith noelse, so an unknownrequest_typefell through tosend_multipart([identity, *response_msg.serialize()])still holding the previousiteration's
response_msg, sending an unrelated requester someone else's answer.What this changes
_process_requestis split into a thin supervisor plus_process_request_loop; anythingthat escapes the per-request handling restarts the loop, so the thread survives for the
controller's lifetime. The ROUTER socket stays bound across a restart.
_handle_request(), which returns the response orNone.Every request now gets exactly one reply:
ZMQRequestType.REQUEST_ERROR(built by_make_error_response()) whenthe handler raises — the reply body carries
{request_type} failed: {error}, so thecaller raises immediately instead of hanging;
REQUEST_ERROR("no handler for request_type ...") when no branch matches, replacingthe stale-response replay;
REQUEST_ERRORfor undecodable requests too — the ROUTER identity frame is prependedby the transport and survives payload corruption, so the reply still reaches the
requester.
No client change is needed: every client call site already treats an unexpected response
type as
RuntimeError(body["message"]).ZMQMessage.deserializeself-checks the frame layout and raisesZMQMessageDecodeErrorcarrying frame count and per-frame sizes. It lives in
deserialize, so controller,storage, client and manager all get the diagnostics.
decode()compares buffer contents via_is_pickle_fallback(). Size is tested first,so the msgpack path stays copy-free.
encode()catches the errors msgspec actually raises, via a named_ENCODE_FALLBACK_ERRORStuple documenting why none of them areValueError. Fallingback now logs at
warninglevel — it is a performance degradation path and should bevisible at default log levels.
pass_filenames: false); withfollow_imports = "skip", passing only changed files produced false errors.Scope
This does not fix the root cause of the truncation. The evidence says the ZMQ multipart
frame boundaries shift, so frame 0 stops being the msgpack header — decoding an empty
buffer produces exactly
Input data was truncated, and empty frames are routine here sinceempty tensors serialize to zero-length frames. What this PR does is stop that from taking
the job down, and emit the frame layout needed to confirm it. The frame sizes make the
diagnosis immediate:
A healthy message is one small header frame followed by large buffers. A leading
0, amissing small header, or an implausibly small header all point at shifted boundaries.
One caveat to note for rollout: the pickle fallback only works end to end when both peers
run this version — an old receiver (
recv_multipart(copy=False)+==marker check)cannot recognise fallback frames from a new sender, and new senders produce them in more
situations than before. Upgrade controller, storage units and clients together.
Tests
TestPickleFallbackintests/test_serial_utils_on_cpu.py(13 cases): oversized ints andself-referential containers degrade instead of raising, round-trips through
zmq.Frame,marker detection across
bytes/bytearray/memoryview/zmq.Frame, four near-missnegatives, and an end-to-end round trip over a real ROUTER with
recv_multipart(copy=False)— the transport that hid the bug.
TestTransferQueueControllerBadRequestsintests/test_controller.py(3 cases):REQUEST_ERRORreply and thecontroller keeps serving;
PUT_DATAgets aREQUEST_ERRORreply instead of replaying the previousiteration's stale response;
GET_METAbody missing its keys makes the handler raiseKeyError, and the requestergets a
REQUEST_ERROR("GET_META failed: KeyError ...") while the loop keeps answeringsubsequent requests.
All three were confirmed to fail against the pre-fix code and pass after.
Results: 115 passed across the two serialization suites, 25 passed across
tests/test_controller.py(including the 3 new cases), and 76 passed acrosstests/e2e/.pre-commit run --all-filesis green (ruff, ruff-format, mypy).Unrelated CI fix bundled here
The MooncakeStore e2e job started failing on
libcudart.so.12: cannot open shared object file— upstreammooncake-transfer-engine-non-cuda0.3.12 ships amooncake_masterbinary linked against the CUDA 12 runtime (verified by diffing the DT_NEEDED entries of
the 0.3.11.post1 and 0.3.12.post1 wheels; 0.3.11.post1 has no CUDA dependency). The two
workflows that install it now pin
mooncake-transfer-engine-non-cuda<0.3.12until upstreamfixes the wheel.