Skip to content

[key-server] Apply the configured RPC timeout to the fullnode gRPC client#623

Merged
joyqvq merged 2 commits into
MystenLabs:mainfrom
tamashi095:fix/rpc-timeout
Jul 24, 2026
Merged

[key-server] Apply the configured RPC timeout to the fullnode gRPC client#623
joyqvq merged 2 commits into
MystenLabs:mainfrom
tamashi095:fix/rpc-timeout

Conversation

@tamashi095

@tamashi095 tamashi095 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #624

[key-server] Apply the configured RPC timeout to the fullnode gRPC client

Summary

RpcConfig.timeout (default 60s) is exposed to operators in KeyServerOptions
and documented as "Timeout for individual RPC requests", but it was never
applied anywhere:

  • Server::new (server.rs) passed only rpc_config.retry_config into
    SuiRpcClient::new, and
  • build_grpc_client (sui_rpc_client.rs) constructed the tonic client with
    no per-request timeout.

As a result, every fullnode RPC the key server makes — package resolution,
MVR lookup, address-alias check, policy simulation, zkLogin signature
verification — could hang indefinitely.

This change threads the configured timeout into the gRPC client via
SuiGrpcClient::with_response_headers_timeout, for both the key server and
the aggregator. Expiry surfaces as DeadlineExceeded, which the existing
retry layer already handles, so behavior on a healthy fullnode is unchanged.

Why it matters

The key server performs several fullnode RPCs per /v1/fetch_key request
before the certificate signature is verified, and has no in-process
concurrency limit. With a stalled or slow fullnode (or an exhausted fullnode
rate quota — which unauthenticated traffic itself can cause via these
pre-signature RPCs), every inbound request pins a task, a connection, and
memory for an unbounded time. Under modest request volume this grows
linearly until the process exhausts file descriptors or memory and needs a
restart.

With this fix, each RPC is bounded by rpc_config.timeout (× retry attempts

  • backoff), so requests fail cleanly and resource usage stays bounded during
    fullnode incidents.

Reproduced locally with a stub fullnode that answers GetEpoch but hangs all
other RPCs: at 100 unauthenticated requests/second the stock server
accumulated ~6,000 in-flight requests / open FDs in 60s with linearly growing
RSS and never answered a single request; with the timeout applied (set to 2s
for the test) every request failed within ~7s and in-flight stayed flat.

Test plan

  • Added sui_rpc_client::tests::test_build_grpc_client_applies_timeout: spins
    up a gRPC LedgerService that never responds and asserts the call fails
    with DeadlineExceeded shortly after the configured timeout instead of
    hanging forever.
  • Existing callers updated to pass a timeout (options.rpc_config.timeout in
    the key server and aggregator; a fixed 30s in test helpers).

Notes

  • Follow-ups worth considering (kept out of scope to keep this small): verify
    the certificate signature before issuing outbound RPCs, negative-cache MVR
    resolution failures, and add a global in-flight request limit.

RpcConfig.timeout (default 60s) was exposed to operators but never
applied: build_grpc_client set no per-request timeout on the tonic
client, so every fullnode RPC (package resolution, MVR lookup, alias
check, policy simulation, zkLogin verification) could hang
indefinitely, pinning a task/connection per inbound request until the
process needs a restart.

Thread the configured timeout into the client via
with_response_headers_timeout for both the key server and the
aggregator. Expiry surfaces as DeadlineExceeded, which the existing
retry layer already handles.

Adds a regression test using a hanging LedgerService stub.
joyqvq
joyqvq approved these changes Jul 24, 2026

@joyqvq joyqvq left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

small edits, lgtm, thanks for the contribution

@joyqvq
joyqvq enabled auto-merge (squash) July 24, 2026 13:03
@joyqvq
joyqvq merged commit 5f8b42a into MystenLabs:main Jul 24, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[key-server] RpcConfig.timeout is never applied — fullnode RPCs can hang indefinitely

2 participants