kvcache-ai / kvcache-ai/Mooncake

[Bug][TENT] First RPC to a peer after it goes away fails: the pool cannot tell an idle connection is dead

Open
#4,174 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
6.6k
Forks
1.2k
Avg merge
3d 5h
Merged PRs (30d)
312

Description

**Summary**

A pooled RPC connection whose peer is gone fails the first call after that, with
`io_error: End of file`. The pool flushes on that failure and the next attempt
succeeds, so the retry is left to each caller — and not every caller retries.
Reproduced with a real peer process restarted at the same address, so a peer
restart, or an idle connection dropped by the network, costs the first RPC to
that address in production too.

It surfaced as a flake in `tent_hp_tcp_transport_test`:
`WriteAndSlicedRead/HighPerformanceTcpLaneDistributionTest.InterleavedPeersUseEveryConfiguredLane/1`
fails at `client.submitTransferTasks(batch, {request}).ok()`
(`hp_tcp_transport_test.cpp:893`).

**Reproduction**

*Production-shaped.* A two-mode binary: `server ` runs a `ControlService`
peer in its own process on a fixed port; `client ` fetches a descriptor,
waits, then fetches twice more, keeping its connection pool across the restart.
The driver starts the peer, runs the client, `kill -9`s the peer, starts a new
peer on the same port, and lets the client continue. Upstream code, no patch:

```
BEFORE_RESTART ok=1 msg=OK
FIRST_AFTER_RESTART ok=0 msg=RpcServiceError: Failed to call RPC function. server: 127.0.0.1:40123, func_id: 1, message: End of file
SECOND_AFTER_RESTART ok=1 msg=OK
```

*Test-shaped and quicker.* The same failure appears when a segment server's
ephemeral port is reused inside one process:

```bash
cmake -G Ninja -B build-tent -DUSE_TENT=ON -DUSE_HTTP=ON -DBUILD_UNIT_TESTS=ON \
-DBUILD_EXAMPLES=ON -DENABLE_DEBUG_SYMBOLS=OFF -DUSE_CUDA=OFF -DUSE_UB=ON \
-DCMAKE_BUILD_TYPE=Debug
cmake --build build-tent --target tent_hp_tcp_transport_test -j128
cd build-tent/mooncake-transfer-engine/tent/tests
./tent_hp_tcp_transport_test \
--gtest_filter='*InterleavedPeersUseEveryConfiguredLane/1' --gtest_repeat=300
```

A fresh process passes (0 failures in 100 runs) because its pool is empty;
repeating inside one process fails 142 of 300 iterations.

**Root cause**

The pool is keyed by `server_addr` (`tent/include/tent/rpc/rpc.h:119`) and lives
on a `CoroRpcAgent`, which for `ControlClient` is process-wide, so an entry
outlives its peer. Client-side trace of the production-shaped run:

```
CALL server=127.0.0.1:40123 fid=1 from_pool=0 closed_at_acquire=0
CALL server=127.0.0.1:40123 fid=1 from_pool=1 closed_at_acquire=0
CALL_FAIL server=127.0.0.1:40123 from_pool=1 closed_at_acquire=0 errc=1 msg=End of file
CALL server=127.0.0.1:40123 fid=1 from_pool=0 closed_at_acquire=0
```

Every failure uses a pooled connection (33 of 33 over 200 repeated runs), and
`coro_rpc_client::has_closed()` was false each time, so the pool cannot tell that
an idle connection's peer is gone.

**Impact**

`rpc_reconnect_test.cpp` pins the contract
(`StalePoolIsFlushedSoTheNextCallSucceeds`, `AFailedCallIsNotRetried`), which puts
the retry on each caller:

| call site | retries today |
| --- | --- |
| `TcpTransport` send/recv (`tcp_transport.cpp:284,288`) | yes, `max_retry_count` loop |
| `RdmaEndPoint` bootstrap (`endpoint.cpp:514`) | yes |
| notifications (`transfer_engine_impl.cpp:2543`) | yes, retried every pass |
| `PeerSegmentRegistry::getSegmentDesc` (`segment_registry.cpp:73`) | no |
| `ub_transport.cpp:478`, `proxy_manager.cpp:286,301,312` | to confirm |

A retry inside the RPC layer is not available: `End of file` does not say whether
the peer ran the handler, which is why `AFailedCallIsNotRetried` uses a
side-effecting handler.

**Open questions**

- Is "every caller retries once" the contract? If yes, the call sites above
without a retry are bugs. One retry in the descriptor fetch makes the
production-shaped run report `FIRST_AFTER_RESTART ok=1`, and turns 142/300 into
0/300.
- Should the failure be distinguishable? A peer that answered with an error and a
peer that dropped the connection both arrive as `RpcServiceError`, so a caller
cannot retry only the safe case.
- Is revalidating on acquire viable? `has_closed()` cannot detect it (0/33) and
`coro_rpc_client` exposes no socket, so it would need a keepalive on idle
connections or an upstream hook.
- Should a pool entry be dropped when its address is rebound by a different
server, so a new peer cannot inherit a stale connection?

Contributor guide

Open the contributing guide

Research direction

Start with tent/include/tent/rpc/rpc.h:119 and rpc_reconnect_test.cpp, then reproduce with tent_hp_tcp_transport_test using the provided CMake and gtest commands. Compare the listed call sites, including segment_registry.cpp:73 and the transport files, against the existing stale-pool contract; done requires an agreed handling of the first call after a peer disappears and a regression test showing the intended behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design, networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.