Skip to content

[Data][1/N] add external shuffle runtime library - #64828

Open
ShockYoungCHN wants to merge 13 commits into
ray-project:masterfrom
ShockYoungCHN:ext-shuffle-1-runtime
Open

[Data][1/N] add external shuffle runtime library#64828
ShockYoungCHN wants to merge 13 commits into
ray-project:masterfrom
ShockYoungCHN:ext-shuffle-1-runtime

Conversation

@ShockYoungCHN

Copy link
Copy Markdown
Contributor

Adds the file-transport runtime for a new external (on-disk) shuffle variant: TCP wire protocol, per-node ShuffleManager actor, connection / fetch primitives, and error classification.

This PR lands only the runtime library and its testers required by external shuffle tasks. It's not wired into the plan yet and there's no user-visible behavior change.

description

  • Reducers open one keep-alive TCP connection per source ShuffleManager, authenticate with a per-shuffle token, and stream FETCH responses straight to disk (fetch_into via _PwriteSink).
  • ShuffleManager will be started by map task to serve reducer's file read request. Upon init, ShuffleManager will bind file serving function to a random available port.
  • Fault tolerance is driven by Ray's actor state (ActorDied / ActorUnavailable / ActorUnschedulable). On mid-fetch TCP failure, the reducer compares endpoints: if the manager was restarted (where the endpoint are likely to be different) we retry in place; if the endpoint is unchanged (which is Ray RPC still works but our TCP path doesn't) it's almost always a network-configuration problem (NetworkPolicy, firewall, routing) that retries to the same manager can't fix, so we surface a terminal ShuffleManagerAnomalyError.
  • ShuffleManager wraps its serve loop with os._exit(1) so a server-thread crash surfaces as ActorUnavailableError, thus Ray auto-restart.

Additional information

The complete implementation of shuffle is in #64733, including runtime libraries, external shuffle task definition, plan wiring and a user-visible flag. I plan to do three PRs (1 is runtime libraries, 2 is task definition and wiring, and 3 is the user-visible flag) to gradually merge the complete implementation.

ShockYoungCHN and others added 2 commits July 16, 2026 17:02
Adds the file-transport runtime for a new external (on-disk) shuffle
variant: TCP wire protocol, per-node ShuffleManager actor, connection /
fetch primitives, and error classification.

This PR lands only the runtime library. It's not wired into the plan
yet — no user-visible behavior change until later PRs turn on the
feature flag.

Design summary:
- Reducers open one keep-alive TCP connection per source ShuffleManager,
  authenticate with a per-shuffle token, and stream FETCH responses
  either into memory (fetch) or straight to disk (fetch_into via
  _PwriteSink).
- Fault tolerance is driven by Ray's actor state (ActorDied /
  ActorUnavailable / ActorUnschedulable) rather than an external
  ray.nodes() probe. On mid-fetch TCP failure the reducer compares
  endpoints: if the manager was restarted we retry in place; if the
  endpoint is unchanged we surface a terminal ShuffleManagerAnomalyError.
- ShuffleManager wraps its serve loop with os._exit(1) so a server-thread
  crash surfaces as ActorUnavailableError → Ray auto-restart, preserving
  the "actor alive ⇒ TCP endpoint alive" invariant reducers rely on.

Runtime unit / integration tests cover:
- _PwriteSink write + reset semantics
- ShuffleManager actor lifecycle + endpoint reachability
- Handshake: good token, bad token (PermissionError), unreachable peer
- fetch: single/multi-range, multi-source, path-outside-base-dir,
  missing file (FileNotFoundError)
- fetch_into: wire framing on disk + reset-then-refetch idempotence
- Error class hierarchy sanity checks

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
@ShockYoungCHN
ShockYoungCHN requested a review from a team as a code owner July 17, 2026 01:39

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an external-shuffle file-transport runtime (external_shuffle_runtime.py) implementing a TCP wire protocol, a per-node ShuffleManager actor, prefetch layout, and associated unit and integration tests. The review feedback focuses on improving robustness, security, and resource management. Key recommendations include reading socket data in chunks and implementing a discard helper to prevent uncontrolled memory allocation, validating incoming lengths and counts (such as num_ranges and msg_len) to avoid denial-of-service vectors, setting appropriate socket timeouts on both client and server sides, adding a retry mechanism with exponential backoff for transient network errors, and dynamically supporting IPv6 environments.

Comment on lines +208 to +218
def handle(self):
srv = self.server
sock = self.request
_tune_shuffle_socket(sock)
try:
if not self._handshake(sock, srv):
return
self._serve_loop(sock, srv)
except (ConnectionError, OSError):
# Peer closed mid-read, or socket dead
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Set a timeout on the accepted socket to prevent leaked or idle connections from hanging indefinitely and exhausting server resources. Also, catch general Exception to avoid unhandled exceptions bubbling up and printing noisy tracebacks to stderr.

Suggested change
def handle(self):
srv = self.server
sock = self.request
_tune_shuffle_socket(sock)
try:
if not self._handshake(sock, srv):
return
self._serve_loop(sock, srv)
except (ConnectionError, OSError):
# Peer closed mid-read, or socket dead
pass
def handle(self):
srv = self.server
sock = self.request
_tune_shuffle_socket(sock)
sock.settimeout(300.0) # Prevent leaked/idle connections from hanging indefinitely
try:
if not self._handshake(sock, srv):
return
self._serve_loop(sock, srv)
except (ConnectionError, OSError):
# Peer closed mid-read, or socket dead
pass
except Exception:
logger.exception(f"Unexpected error handling connection from {self.client_address}")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good bot

Comment on lines +274 to +279
num_ranges = _recv_u32(sock)
ranges = []
for _ in range(num_ranges):
offset = _recv_u64(sock)
length = _recv_u64(sock)
ranges.append((offset, length))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

Validate num_ranges to prevent extremely large values from causing CPU/memory exhaustion via the loop.

Suggested change
num_ranges = _recv_u32(sock)
ranges = []
for _ in range(num_ranges):
offset = _recv_u64(sock)
length = _recv_u64(sock)
ranges.append((offset, length))
num_ranges = _recv_u32(sock)
if num_ranges > 1000000:
raise ValueError(f"Too many ranges requested: {num_ranges}")
ranges = []
for _ in range(num_ranges):
offset = _recv_u64(sock)
length = _recv_u64(sock)
ranges.append((offset, length))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant design, because the num_ranges won't be so large that it causes a problem

Comment on lines +776 to +809
while True:
try:
endpoint = _resolve()
with open_shuffle_connection(endpoint, token) as conn:
for batch in _chunk_members_by_bytes(members, max_bytes_per_fetch):
sources = [(m.path, m.ranges) for m in batch]
conn.fetch_into(sources, out_file_obj)
return
except PermissionError:
raise
except (ConnectionError, TimeoutError) as e:
# If _resolve() returns, the actor is alive; endpoint compare tells us
# whether the manager restarted (retry in-place) or the reducer-manager
# TCP path is broken (terminal).
out_file_obj.reset()
with _ENDPOINT_CACHE_LOCK:
_ENDPOINT_CACHE.pop(key, None)
fresh = _resolve()
if fresh == endpoint:
# Endpoint unchanged: actor is alive but TCP is blocked. Most
# likely a network configuration issue (NetworkPolicy, firewall,
# routing); retrying to the same manager won't help.
raise ShuffleManagerAnomalyError(
f"TCP fetch from node {node_id} failed ({e}) but "
f"ShuffleManager at {fresh} is still reachable via Ray. "
f"Likely a network configuration issue (NetworkPolicy, "
f"firewall, routing) between reducer and manager. "
f"Check the network config."
) from e
logger.warning(
f"TCP fetch from node {node_id} failed ({e}); ShuffleManager "
f"restarted (endpoint {endpoint} → {fresh}). Retrying in place."
)
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Allow a limited number of retries (e.g., 3) with exponential backoff for the same endpoint before raising a terminal ShuffleManagerAnomalyError. This prevents transient network drops or temporary server overloads from immediately failing the entire shuffle.

    retries = 0
    max_retries = 3
    while True:
        try:
            endpoint = _resolve()
            with open_shuffle_connection(endpoint, token) as conn:
                for batch in _chunk_members_by_bytes(members, max_bytes_per_fetch):
                    sources = [(m.path, m.ranges) for m in batch]
                    conn.fetch_into(sources, out_file_obj)
            return
        except PermissionError:
            raise
        except (ConnectionError, TimeoutError) as e:
            out_file_obj.reset()
            with _ENDPOINT_CACHE_LOCK:
                _ENDPOINT_CACHE.pop(key, None)
            fresh = _resolve()
            if fresh == endpoint:
                retries += 1
                if retries >= max_retries:
                    raise ShuffleManagerAnomalyError(
                        f"TCP fetch from node {node_id} failed {max_retries} times ({e}) but "
                        f"ShuffleManager at {fresh} is still reachable via Ray. "
                        f"Likely a network configuration issue (NetworkPolicy, "
                        f"firewall, routing) between reducer and manager."
                    ) from e
                logger.warning(
                    f"TCP fetch from node {node_id} failed ({e}); endpoint unchanged. "
                    f"Retrying ({retries}/{max_retries}) after a brief delay..."
                )
                time.sleep(2.0 * retries)
                continue
            else:
                retries = 0  # Reset retry count if endpoint changed (manager restarted)
                logger.warning(
                    f"TCP fetch from node {node_id} failed ({e}); ShuffleManager "
                    f"restarted (endpoint {endpoint}{fresh}). Retrying in place."
                )
                continue

- Support IPv6 nodes: pick AF_INET6 ThreadingServer subclass when
  ray.util.get_node_ip_address() returns an IPv6 literal.
- Add socket timeouts to catch stuck / crashed peers: server side
  5 min per-syscall idle (settimeout(300)), client side 60 s on
  open_shuffle_connection.
- Broaden _FetchHandler.handle() except so unexpected exceptions get
  logged via logger.exception instead of a traceback to stderr.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
@ray-gardener ray-gardener Bot added data Ray Data-related issues community-contribution Contributed by the community labels Jul 17, 2026
…_id.

Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
while total < n_total:
total += os.pwrite(self._fd, mv[total:], self._pos + total)
self._pos += total
return total

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infinite loop if os.pwrite returns zero

Medium Severity

_PwriteSink.write unconditionally accumulates the return value of os.pwrite into total. If os.pwrite ever returns 0 (which can happen on certain filesystems or under unusual kernel conditions), the loop spins forever because total never advances past n_total. This is a known real-world pitfall — the coreweave/tensorizer project encountered the same pattern and shipped a dedicated fix. A guard checking for a non-positive return and either raising or breaking would prevent the hang.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit affc97d. Configure here.

@ShockYoungCHN ShockYoungCHN changed the title [Data][1/N add external shuffle runtime library [Data][1/N] add external shuffle runtime library Jul 17, 2026
ShockYoungCHN and others added 2 commits July 17, 2026 14:46
Add missing Args and Returns sections to the docstring; pydoclint
in --style=google mode was flagging DOC101 / DOC103 / DOC201.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
raise ShuffleDiskError(
f"Disk exhausted writing prefetch for node {node_id}: {e}"
) from e
raise

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial prefetch on fetch failure

Medium Severity

_prefetch_node_into calls _PwriteSink.reset() only on ConnectionError or TimeoutError. When max_bytes_per_fetch splits work into multiple FETCH batches, a later batch failing with FileNotFoundError, PermissionError, or other errors leaves earlier batches written in the node’s prefetch region without reset.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 637fce1. Configure here.

Pre-commit black hook was reformatting these files on every CI run;
run it locally so the tree stays clean.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
@Kunchd Kunchd assigned Kunchd and unassigned Kunchd Jul 21, 2026
Add _encode_shard and switch _read_ipc to the whole-frame wire format
([u64 uncompressed_size][zstd(whole IPC stream)]) -- one zstd frame per shard,
vs Arrow's per-buffer IPC compression (~40 zstd blobs/shard). One decompress +
one alloc per shard at the reducer; inner IPC uncompressed so buffers read
zero-copy. Library-only; the map/reduce tasks that call these land in a later PR.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 13b4661. Configure here.

…fer)

_read_ipc advertised memoryview support but only wrapped bytes/bytearray; a
memoryview fell through to src.slice(), which memoryview lacks -> AttributeError.
Normalize any non-pa.Buffer input via pa.py_buffer.
…rdcoded zstd)

Whole-frame shard codec is now a parameter, resolved via _codec_for, instead of a
module-level pa.Codec("zstd"). Callers pass data_context.hash_shuffle_compression
(same field v2 uses) so map and reduce agree without an import-time env read. Lib
only on this branch; the map/reduce call sites land with the tasks in ext-2. Default
stays zstd; no wire change.
…test

Signed-off-by: Yuanzhuo Yang <yuanzhuoyang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community data Ray Data-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants