Repository navigation
Conversation
… client The TSO path had no deadline, keepalive, or health check anywhere: a request parked in pending_requests waits on responses.next() unbounded. When the stream terminates that is recoverable — the pending oneshots drop, callers error, the retry layer reconnects — but a stream that goes silent WITHOUT terminating (the peer's VM paused by its hypervisor, the process frozen under resource starvation, a middlebox silently discarding the connection) strands every pending and future timestamp request forever. The PD dial had the same defect: a frozen peer's kernel still completes the TCP handshake, then the HTTP/2 preface never arrives and connect() hangs unbounded — Connection:: connect even took a timeout parameter and ignored it (_timeout), and its get_members call carried no bound either. That combination is the mechanism behind the tikv#516 CI hang. The runner VM froze (all three TiKVs logged "monotonic time jumped back" by ~65s; PD measured a 2.6s scheduling gap against its 1.67s election keep-alive, resigned, and closed its TSO allocator). PD recovered ~15s later, but the test's in-flight current_timestamp() hung against the zombie stream until nextest's 600s kill. Reproduced locally with docker pause on PD (SIGSTOP: TCP open, no FIN): the in-flight request waited the full 45s pause, resolving only because unpause terminated the stream; reconnects hung the same way at the dial. Follow client-go's shape — its TSO dispatcher arms a deadline per batch with defaultPDTimeout = 3s and cancels the stream on expiry (pd/client/clients/tso/ dispatcher.go), bounds stream creation via checkStreamTimeout (stream.go), and bounds dials — using the client's configured request timeout (Config::timeout, whose documented contract covers PD requests). Every bound is armed only while something is owed: - The TSO receive loop waits in whole timeout windows while batches are outstanding: a batch pending across one full silent window declares the stream stalled and the worker exits. A batch dispatched mid-window is granted one full window, so teardown happens only after between one and two windows of genuine silence. Idle streams (nothing owed) are never torn down. - Stream creation is guarded by a watchdog with the same window rule, armed only while a batch is pending: PD (a gRPC-go server) sends response headers with its first response, so tso(..) legitimately waits while the client is idle — HTTP/2 lets request batches flow (and become pending) before response headers arrive, so a stalled creation is still detected. - The worker's exit is SIGNALED to callers: get_timestamp selects on a watch channel the worker closes on exit — while ENQUEUEING as well as while awaiting the response, since a dead worker's request channel can remain open with a full buffer (held by the frozen connection's task, which may not run again until the connection thaws) and send would otherwise block forever. - Dials are bounded, HTTP/2 handshake included: SecurityManager grows connect_with_timeout (the existing connect signature is preserved, now bounded by the default request timeout), Connection::connect uses its hitherto-ignored timeout, and its get_members call is bounded like every other request in that path. Stream termination is also made observable instead of silent: a clean end with nothing owed stays info-level; stalls, creation timeouts, termination with batches outstanding, and stream status errors (previously swallowed) surface as warnings with the outstanding count — resolving the old TODO about indistinguishable terminations. Considered and omitted: a timer-based backstop in get_timestamp (client-go has no analogue — ambient contexts cover it there — and the worker-exit signal is event-driven and precise), and gRPC keepalive (connection-level health, client-go applies it to TiKV connections; a follow-up). Verification: 10 unit tests — the receive loop and the creation watchdog under a paused clock (stall detected within [1, 2] windows; a mid-window batch granted its full window; idle streams and idle creation never torn down; termination clean only when nothing is owed; a stream status error surfaced; the happy path), get_timestamp failing fast when the worker dies, and enqueueing observing worker death with a full request buffer. The stall test bites: with the stall branch disabled it runs unbounded. 82 lib tests and make check green. Manually, with docker pause on PD for 45s: before, one silent 44.5s hang ended only by the unpause; after, a 5s-idle client keeps its worker silently, the stall is detected in ~4s, reconnects against the frozen peer fail in 2s each, callers receive bounded, descriptive errors while the freeze lasts, and the client recovers within ~3s of the peer thawing. Ref: tikv#516 Signed-off-by: Eduard R. <eduard@ralphovi.net>
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
contribution
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 67.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 2 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: bounding waits across the PD client path so a non-responsive peer cannot hang the client. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 67.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Contributor
Author
|
@pingyu for attention |
Brings tikv#562 (client: fall back to followers when leader is unreachable) and the two OWNERS syncs under this PR. Conflict: src/common/security.rs. Both sides had independently grown a method named `connect_with_timeout`, by different mechanisms: - this PR bounds the dial with `tokio::time::timeout` around `channel.connect()`, and makes bare `connect()` delegate to it with the default request timeout, so every dial is bounded; - tikv#562 added a private `connect_inner(addr, Option<Duration>, factory)` whose `Some` arm sets tonic's `Endpoint::connect_timeout`, leaving `connect()` unbounded. Resolved by keeping this PR's shape and mechanism, because the two are not interchangeable. tonic 0.12.3 installs `connect_timeout` on hyper's `HttpConnector` (transport/channel/endpoint.rs: `Endpoint::connect` calls `http.set_connect_timeout` and only then hands off to `Channel::connect`), so it bounds the TCP connection phase — TLS and the HTTP/2 handshake both run after the connector returns. tikv#516's hang is precisely a frozen peer whose kernel completes the TCP handshake and then never answers the preface, so adopting tikv#562's mechanism alone would have reintroduced the bug this PR exists to fix. tikv#562's bound is not discarded, though: the endpoint still receives `connect_timeout`, which caps the TCP phase and keeps applying to reconnects the channel makes on its own — neither of which the outer deadline reaches. The two run concurrently rather than additively; whichever trips first decides the error a caller sees, which is noted at the call site. `connect_inner` is dropped as unreachable: with `connect()` bounded by the default timeout, nothing constructs the `None` arm. It was private, so no API is lost, and the public `connect()` signature is unchanged. src/store/client.rs no longer appears in this PR's diff against master — tikv#562 made the identical change there, so the two sides converged. Verified: cargo check/fmt/clippy -D warnings clean; 112 lib tests, including all 11 of this PR's pd::timestamp tests and tikv#562's kv-client-cache tests; 27 integration, 8 failpoint and 28 sync-transaction tests green against a local api-v2 TiKV v8.5.5 (client-rust's own config/tikv.toml). Signed-off-by: Eduard R. <eduard@ralphovi.net>
pingyu
reviewed
Oct 17, 2026
pingyu
left a comment
pingyu
left a comment
Collaborator
There was a problem hiding this comment.
Two correctness findings from reviewing c8027da against the PR base and comparing with client-go / the PD Go client, TiDB callers, and PD server handling. All 10 existing TSO tests passed (cargo test --offline --lib pd::timestamp::tests). Separate temporary transport and wakeup reproductions confirmed the cases below; no live-cluster integration tests were run.
ti-chi-bot
Bot
added
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…wakeup Two review findings on the TSO deadlines: 1. Lost wakeup. The watchdog and the receive loop inspected the pending queue through a tokio Mutex. If `TsoRequestStream::poll_next` met that lock held, it dropped its lock future (and with it the mutex waiter) and returned Pending; releasing the inspection's guard then woke nobody, and a queued request was never dispatched. The queue is now a std Mutex, never held across an await, so `poll_next` waits out the lock instead of parking. 2. Queued requests ignored. Only batches already taken by the transport counted as owed. The transport polls the request stream only when it can send, so a request enqueued while it cannot (connection not ready, no send window) left the deadline disarmed and the caller waiting forever on a live worker. What is owed is now pending batches plus requests still in the channel, read through a WeakSender so the worker does not hold the channel open; client-go likewise arms its batch deadline before the RPC is sent. Regression tests: the stream polled while another thread holds the lock; the creation watchdog and the receive loop each with a request queued but not dispatched (paused clock); and end to end through a tonic channel whose connection never completes. Each fails or hangs with its fix disabled. Ref: tikv#516 Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
force-pushed
the
tso-deadline
branch
from
October 17, 2026 11:22
cfba5c7 to
f60393d
Compare
This was referenced Oct 20, 2026
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.
Why: #516's
txn_cleanup_2pc_lockshang, diagnosed end to end. In two CI runs (31042420081, 31043996146), the phase logs from #553 show every phase of the test completing in milliseconds; the only statement after the last phase iscount_locks, which begins withclient.current_timestamp()— a TSO request. The cluster-logs artifact shows the trigger: all three TiKVs loggedmonotonic time jumped backby ~65s simultaneously (a hypervisor-level pause of the runner VM), PD then measured a 2.6s scheduling gap on its 50ms TSO loop, lost its 1.67s election lease, resigned, and closed the TSO allocator — and recovered ~15s later, serving normally for the rest of the job, while the test stayed hung until nextest's 600s kill. Whatever holds the client after that point is client-side. And nothing about the trigger is CI-specific: any hypervisor pause or live migration, cgroup throttling under Kubernetes, or a middlebox silently expiring an idle long-lived connection produces the same frozen-open stream in production.The defects. Nothing on the PD path bounded a wait against a peer that goes silent without terminating the connection:
responses.next()unbounded — a request parked inpending_requestson a frozen-open stream hangs forever. (Termination is recoverable; silence is not.)pd_client.tso(...)) is unbounded the same way.connect()hangs unbounded.Connection::connecteven took a timeout parameter and ignored it (_timeout), and itsget_memberscall carried no bound either.What: follow client-go's shape — its TSO dispatcher arms a per-batch deadline with
defaultPDTimeout= 3s and cancels the stream on expiry (pd/client/clients/tso/dispatcher.go), bounds stream creation viacheckStreamTimeout(stream.go), and bounds dials — using the client's configured request timeout (Config::timeout, whose documented contract covers PD requests). Every bound is armed only while something is owed:tso(..)legitimately waits while the client is idle; HTTP/2 lets request batches flow (and become pending) before response headers arrive, so a stalled creation is still detected.get_timestampselects on a watch channel the worker closes on exit — while enqueueing as well as while awaiting the response, since a dead worker's request channel can remain open with a full buffer andsendwould otherwise block forever.SecurityManagergrowsconnect_with_timeout(the existingconnectsignature is preserved, now bounded by the default request timeout, so no API break),Connection::connectuses its hitherto-ignored timeout, andget_membersis bounded like every other request in that path.Stream termination is also observable now: a clean end stays info-level; stalls, creation timeouts, termination with batches outstanding, and stream status errors (previously swallowed) surface as warnings with the outstanding count — resolving the old TODO about indistinguishable terminations.
Considered and omitted: a timer-based backstop in
get_timestamp(client-go has no analogue — ambient contexts cover it — and the worker-exit signal is event-driven and precise), and gRPC keepalive (connection-level health; client-go applies it to TiKV connections; a natural follow-up).What this means for #516: the unbounded hang is gone. Under a short freeze the test recovers and passes; under a freeze longer than the retry budget it fails fast with a descriptive error instead of a 600s timeout — the trigger itself is outside any client's control.
Verification: 10 new unit tests — the receive loop and the creation watchdog driven under a paused clock (stall detected within [1, 2] windows; a mid-window batch granted its full window; idle streams and idle creation never torn down; termination clean only when nothing is owed; a stream status error surfaced; the happy path),
get_timestampfailing fast when the worker dies, and enqueueing observing worker death with a full request buffer. The stall test bites: with the stall branch disabled it runs unbounded. 82 lib tests,make check, and the txn/raw/failpoint integration suites green against a local api-v2 cluster.Manually, with
docker pauseon PD for 45s (SIGSTOP: TCP open, no FIN — the client-side view of a VM pause): before, one silent 44.5s hang ended only by the unpause; after, a 5s-idle client keeps its worker silently, the stall is detected in ~4s, reconnects against the frozen peer fail in 2s each, callers receive bounded, descriptive errors while the freeze lasts, and the client recovers within ~3s of the peer thawing.Ref: #516
Summary by CodeRabbit