Fix logservice client fallback from slow replica addresses by jiangxinmeng1 · Pull Request #25334 · matrixorigin/matrixone

jiangxinmeng1 · 2026-07-01T09:36:03Z

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

This PR improves logservice client creation when candidate replica addresses include stale or unreachable endpoints.

Previously, connectToLogService tried replica addresses sequentially. If one address accepted a TCP connection but did not respond, client creation could wait until the outer context deadline before trying later
candidates.

Changes:

Keep the existing shuffled address order.
Start connection attempts with staggered fallback instead of strict sequential dialing.
Apply a per-address attempt timeout.
Return the first successfully connected client.
Cancel remaining attempts and close losing clients after a winner is selected.
Add a regression test covering a slow/unresponsive address followed by a reachable address.

This reduces the impact of stale or slow replica addresses returned by discovery/gossip without changing HAKeeper or gossip behavior.

(cherry picked from commit e21b43d)

qodo-code-review · 2026-07-01T09:36:08Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

XuPeng-SH

I think this needs another pass before merge. The direction is right, but the current implementation introduces new failure modes in the exact connection-fallback path this PR is trying to harden.

Findings:

pkg/logservice/client.go:572 / pkg/logservice/client.go:581 leaks non-winning successful clients. results is buffered to len(addresses), so after the first success is selected at pkg/logservice/client.go:619, cancel() is called and wg.Wait() waits for losers, but a loser goroutine can still take the always-ready results <- result{client: c} branch instead of the ctx.Done() branch. That client is then left in results and never closed. This can happen in normal multi-replica cases when fallback launches a second reachable address and both connect successfully. Please either make loser-success send non-buffered/cancel-aware, or drain results after wg.Wait() and close every unreturned client. Add a regression test where two reachable attempts both succeed after fallback has launched, and assert the non-winning client is closed/no goroutine or fd leak remains.
pkg/logservice/client.go:476 builds a leader-first list for discovery/reverse-proxy, but pkg/logservice/client.go:498 now shuffles that list before dialing. That regresses the intended and previous discovery behavior: the leader returned by GetShardInfo should be tried first, with other replicas as fallback. This is also explicitly part of issue #25274 expected behavior. Keep shuffle for configured ServiceAddresses, but have connectToLogServiceByReverseProxy call the ordered/staggered dialer directly, or pass an explicit shuffle bool. Please add coverage that discovery preserves leader-first ordering.
pkg/logservice/client.go:524 changes the production logservice client backend read timeout from 0 to 10s by passing time.Second*10 into getRPCClient, which is applied at pkg/logservice/client.go:905. This is not just a per-address connect-attempt limit; it changes the read loop behavior for the returned client and can force idle clients or long in-flight operations to reset/fail after 10s. The per-attempt context already bounds the CONNECT handshake, so this should remain 0 unless there is a separate, tested reason to change client read timeout semantics.

Suggested shape:

Keep connectToLogServiceAddress behavior identical to the old client except for the per-attempt context.
Separate address ordering from dial orchestration: direct configured addresses may be shuffled, discovery addresses must preserve leader-first.
Ensure every successful but non-returned client is closed deterministically.
Add unhappy-path tests for all-addresses-fail, parent context cancellation, multiple-success loser cleanup, and discovery leader-first ordering.

Local validation I ran on this head:

go test ./pkg/logservice -run TestClientCreationFallsBackFromUnreachableReplicaAddress -count=1
go test ./pkg/logservice -run TestClientCreationFallsBackFromUnreachableReplicaAddress -count=10
go test ./pkg/logservice -run "TestClientCanBeConnectedByReverseProxy|TestClientCreationFallsBackFromUnreachableReplicaAddress" -count=1
same targeted tests with -race
go test ./pkg/logservice -count=1
gofmt -d pkg/logservice/client.go pkg/logservice/client_test.go
git diff --check origin/3.0-dev...HEAD

XuPeng-SH · 2026-07-01T09:54:20Z

Reposting the change-request findings here for visibility. I already submitted a GitHub review with CHANGES_REQUESTED: #25334 (review)

Blocking findings:

pkg/logservice/client.go:572 / pkg/logservice/client.go:581: non-winning successful clients can leak. results is buffered to len(addresses), so after a winner is selected and cancel() is called, another successful goroutine can still choose the ready results <- result{client: c} branch. That client is then left in the channel and never closed. Please close/drain all successful non-returned clients and add a regression test with multiple successful attempts after fallback has launched.
pkg/logservice/client.go:476 builds discovery addresses leader-first, but pkg/logservice/client.go:498 shuffles them before dialing. This regresses the intended discovery behavior and conflicts with issue [Bug]: logservice client connects to unreachable replicas with 30s timeout, blocking DN commit pipeline #25274 expected behavior. Please preserve leader-first for discovery/reverse-proxy and only shuffle configured ServiceAddresses if needed. Add a leader-first ordering test.
pkg/logservice/client.go:524: passing time.Second*10 into getRPCClient changes the returned production client backend read timeout from old 0 semantics to 10s. This is not just the per-address connect-attempt timeout; it can reset/fail idle or long in-flight clients. The per-attempt context already bounds the CONNECT handshake, so this should remain 0 unless separately justified and tested.

Suggested shape: keep connectToLogServiceAddress behavior identical to the old client except for the per-attempt context; separate ordered discovery dialing from shuffled direct-address dialing; deterministically close every loser success; add unhappy-path tests for all-addresses-fail, parent cancellation, multiple-success loser cleanup, and discovery leader-first ordering.

LeftHandCold

I confirmed the existing change-request findings on the current head, so I cannot approve this as-is.

Blocking points:

Non-winning successful clients can still leak. A successful loser can enqueue result{client: c} into the buffered results channel after cancellation, and the winner path returns after wg.Wait() without draining and closing that client.
Discovery/reverse-proxy addresses are built leader-first, but the new connectToLogService now shuffles all inputs. On 3.0-dev, the old code effectively preserved leader-first for discovery because len(cfg.ServiceAddresses) is zero in that path.
connectToLogServiceAddress changes the returned client's backend read timeout from the old 0 semantics to 10s. The per-attempt context should bound connect/handshake without changing the long-lived client's read-timeout behavior.

Please fix these paths and add coverage for multiple successful attempts, discovery leader-first ordering, all-addresses-fail, and parent-context cancellation.

- Restore getRPCClient read timeout to 0 (per-attempt ctx already bounds the connection handshake; read timeout can reset idle/in-flight clients) - Preserve leader-first ordering for discovery/reverse-proxy by extracting shardInfoReplicaAddresses and bypassing the shuffle in connectToLogService - Add closeUnusedConnectResultClients to drain and close all non-winning successful clients after winner selection - Add regression tests: leader-first ordering, all-addresses-fail, parent context cancellation, multiple-success loser cleanup, and fallback timer Co-Authored-By: Claude <noreply@anthropic.com>

XuPeng-SH

Re-reviewed the latest head f0b6d82ad14cf6c83a0ee756c57065a8fa12c7d2.

The previous three blocking items look mostly addressed in this revision:

successful loser clients are now closed after cancel()/wg.Wait()/drain;
discovery/reverse-proxy addresses now preserve leader-first ordering instead of being shuffled;
the production RPC backend read timeout is back to the previous 0 behavior.

I still need to request changes because the new concurrent fallback path can return the wrong terminal error when an earlier address has failed but the parent context later expires before the whole search finishes.

In connectToLogServiceAddresses, the <-ctx.Done() branch currently does this:

cancel()
wg.Wait()
closeUnusedConnectResultClients(results)
if lastErr != nil {
    return nil, lastErr
}
return nil, ctx.Err()

That means a mixed unhappy path such as fast stale address failure -> next replica hangs -> caller deadline fires returns the stale address error instead of context.DeadlineExceeded. This is not just error cosmetics: the PR is specifically fixing stale/slow logservice replica fallback, and callers/diagnostics lose the real terminal cause when their connect deadline is reached before all candidates complete.

I verified this locally with a temporary regression test using connectToLogServiceAddressFn: first address returns a sentinel error immediately, second waits for ctx.Done() and then returns. Expected context.DeadlineExceeded; actual error chain was only "first failed".

Suggested fix:

In the <-ctx.Done() branch, after cancel/wait/drain, return ctx.Err() (or the attached context cause if this codebase wants to preserve causes), not lastErr.
Keep lastErr only for the normal path where all launched attempts have completed and all failed.
Consider pre-checking ctx.Err() before launching the first/next attempt so an already-canceled parent does not start a dial and race back a non-context error.
Add regression coverage for:
1. previous address failed, active address is still pending, parent deadline fires -> returns context.DeadlineExceeded;
2. already-canceled parent context -> returns context.Canceled and does not mask it with a dial error.

Local verification performed:

New PR-targeted tests: go test ./pkg/logservice -run 'TestClientCreationFallsBackFromUnreachableReplicaAddress|TestShardInfoReplicaAddressesKeepsLeaderFirst|TestConnectToLogServiceWithNoTargets|TestConnectToLogServiceAddressesReturnsLastError|TestConnectToLogServiceAddressesReturnsContextError|TestConnectToLogServiceAddressesClosesSuccessfulLosers|TestStopConnectFallbackTimer' -count=1 passed with the required MO CGo flags.
Full package: go test ./pkg/logservice -count=1 -timeout 120s passed locally with the same CGo setup.

XuPeng-SH

Re-reviewed latest head b4b67948314d7cc4258d5d5a7c232621c3e3d059.

The previous functional blockers are addressed: discovery keeps leader-first ordering, the returned RPC client read timeout is still 0, successful non-winning clients are drained/closed, and the mixed previous failure + parent deadline case now returns a context deadline error while preserving the previous failure in the message.

I still need one change before approving:

pkg/logservice/client.go:590 adds a production context.WithTimeout for the per-address attempt. This new timeout is now the primary unhappy path for stale/hanging replicas, so it should preserve MO's timeout cause semantics instead of returning a plain context.DeadlineExceeded. The repo convention/static check also explicitly pushes production code toward context.WithTimeoutCause (Makefile:951). Please use context.WithTimeoutCause here, with moerr.CauseNewLogServiceClient or a more specific new cause, and attach/preserve that attempt cause when sending lastErr back to the coordinator. Add a regression test where all candidate replicas hang until the per-attempt timeout while the parent context is still alive, and assert the returned error remains deadline-classified and carries the MO cause/diagnostic context.

One non-blocking cleanup suggestion: connectContextError currently formats lastErr with %v, so errors.Is/Unwrap cannot see the last connect error. If callers ever need structured classification of that inner failure, prefer preserving both errors structurally instead of only embedding the text.

Local validation I ran on this head:

go test ./pkg/logservice -run 'TestClientCreationFallsBackFromUnreachableReplicaAddress|TestShardInfoReplicaAddressesKeepsLeaderFirst|TestConnectToLogServiceWithNoTargets|TestConnectToLogServiceAddressesReturnsLastError|TestConnectToLogServiceAddressesReturnsContextError|TestConnectToLogServiceAddressesReturnsContextErrorAfterPreviousFailure|TestConnectToLogServiceAddressesWithCanceledContextDoesNotDial|TestConnectToLogServiceAddressesClosesSuccessfulLosers|TestStopConnectFallbackTimer' -count=1 -timeout 120s
go test ./pkg/logservice -count=1 -timeout 180s
go test -race ./pkg/logservice -run 'TestClientCreationFallsBackFromUnreachableReplicaAddress|TestConnectToLogServiceAddressesReturnsContextErrorAfterPreviousFailure|TestConnectToLogServiceAddressesWithCanceledContextDoesNotDial|TestConnectToLogServiceAddressesClosesSuccessfulLosers' -count=1 -timeout 180s
go test ./pkg/logservice -run 'TestConnectToLogServiceAddressesReturnsContextErrorAfterPreviousFailure|TestConnectToLogServiceAddressesClosesSuccessfulLosers' -count=50 -timeout 180s

CI is still in progress, so this review is based on code inspection plus the local checks above.

XuPeng-SH

Requesting changes for one blocker:

pkg/logservice/client.go:590-596 now wraps every candidate in a hard 5s context.WithTimeoutCause(...) before connectToLogServiceAddress.
That same context is reused by CONNECT / CONNECT_RO, whose server-side paths keep retrying ErrShardNotReady / ErrSystemBusy until the passed context expires (pkg/logservice/service.go:426-441, pkg/logservice/store.go:679-717).
TN callers still budget 30s for logservice client creation by default (pkg/tnservice/cfg.go:48,109-110, pkg/tnservice/factory.go:105-116). So this changes behavior from “wait up to the callers connect timeout for a healthy shard to come up” to “kill each replica attempt after 5s”, which can reject healthy replicas during leader election / recovery.

Suggested fix: keep the staggered fallback, but do not put a fixed 5s deadline around the full connectReadWrite / connectReadOnly path. Either keep at least one viable attempt on the callers full budget, or restrict the short timeout to transport establishment / initial dial only.

XuPeng-SH

I did not find a remaining blocker on the current head.

The latest preserve caller connect timeout change fixes the previous regression cleanly: fallback still staggers new attempts behind a slow/hanging address, but each attempt now inherits the caller timeout instead of being hard-capped at 5s. That preserves the callers readiness budget while still allowing early fallback and loser cleanup/cancellation.

I also rechecked the unhappy-path surface on the current implementation: cancellation still terminates in-flight attempts, losing successful clients are closed, and the targeted logservice tests plus a package compile both pass on the PR head.

mergify · 2026-07-06T09:16:59Z

Merge Queue Status

✅ Entered queue — 2026-07-06 09:16 UTC · Rule: release-3.0 · triggered by rule Automatic queue on approval for release-3.0
🟠 Checks running · in-place
🚫 Left the queue — 2026-07-06 09:18 UTC · at 76d9f2971860ad8084edc07fa7a00129cf0f7056

This pull request spent 1 minute 41 seconds in the queue, with no time running CI.

Waiting for

All conditions

Reason

Pull request #25334 has been dequeued

Queue conditions are not satisfied:

#changes-requested-reviews-by<=0

approved-reviews-by=XuPeng-SH

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Requeued — the merge queue status continues in this comment ↓.

…-dev

XuPeng-SH

Superseding my earlier approve: the latest head still has two blockers.

pkg/logservice/client.go:589, pkg/common/morpc/future.go:55-57
- The new preserve caller connect timeout change passes the caller context straight into connectToLogServiceAddressFn(ctx, ...). If the caller uses context.Background() / WithCancel() without a deadline, the first successful TCP connect now reaches morpc with a deadline-less context and panics in Future.init (context deadline not set). Earlier revisions wrapped each attempt in a timeout, so this failure mode did not exist.
- Suggested fix: preserve an attempt-scoped deadline when the caller context has none, or fail explicitly before dialing instead of letting morpc panic.
pkg/logservice/client.go:632-653, pkg/common/morpc/backend.go:872-920
- On first success or on caller cancellation, connectToLogServiceAddresses still does cancel(); wg.Wait(); ... before returning. But a launched loser can still be stuck in morpc backend creation, where the initial rb.conn.Connect(..., rb.options.connectTimeout) does not observe the caller context. So a blackholed loser dial can still delay both success return and cancellation return until that internal connect timeout expires.
- Suggested fix: make backend creation/connect context-aware, or stop waiting on uncancelable loser dials before returning the winner / cancellation.

XuPeng-SH

Reviewed latest head 5fe0ab4.

The previous blockers look closed now:

caller contexts without deadlines get an attempt-scoped timeout, so morpc Future.init will not panic on deadline-less contexts;
caller contexts with deadlines preserve the caller deadline instead of forcing a shorter per-attempt timeout;
winner and cancellation paths no longer wait synchronously for loser dials that may be inside backend connect;
buffered successful loser clients are drained and closed asynchronously;
reverse-proxy discovery keeps leader-first ordering, while explicit target dialing still shuffles.

Unhappy-path coverage is now reasonable for this change: empty targets, canceled context before dial, previous failure plus caller timeout, deadline-less caller context, winner-with-slow-loser, cancel-with-slow-loser, and successful loser cleanup.

Verified:

go test ./pkg/logservice targeted fallback tests
go test -race ./pkg/logservice targeted async cleanup tests
go test ./pkg/logservice
GitHub CI for 3.0 is green

mergify · 2026-07-06T13:18:13Z

fix(logservice): fallback from stale replica addresses

0ac1864

(cherry picked from commit e21b43d)

jiangxinmeng1 requested review from LeftHandCold and volgariver6 as code owners July 1, 2026 09:36

mergify Bot requested a review from XuPeng-SH July 1, 2026 09:36

mergify Bot added the kind/bug Something isn't working label Jul 1, 2026

jiangxinmeng1 temporarily deployed to ci July 1, 2026 09:36 — with GitHub Actions Inactive

jiangxinmeng1 temporarily deployed to ci July 1, 2026 09:41 — with GitHub Actions Inactive

jiangxinmeng1 had a problem deploying to ci July 1, 2026 09:41 — with GitHub Actions Failure

jiangxinmeng1 temporarily deployed to ci July 1, 2026 09:41 — with GitHub Actions Inactive

matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jul 1, 2026

XuPeng-SH requested changes Jul 1, 2026

View reviewed changes

mergify Bot requested a review from XuPeng-SH July 1, 2026 09:49

LeftHandCold requested changes Jul 1, 2026

View reviewed changes

jiangxinmeng1 temporarily deployed to ci July 1, 2026 12:22 — with GitHub Actions Inactive

jiangxinmeng1 had a problem deploying to ci July 1, 2026 12:22 — with GitHub Actions Failure

XuPeng-SH requested changes Jul 3, 2026

View reviewed changes

LeftHandCold approved these changes Jul 3, 2026

View reviewed changes

fix(logservice): preserve terminal context errors

3f3c7a2

jiangxinmeng1 temporarily deployed to ci July 3, 2026 08:08 — with GitHub Actions Inactive

jiangxinmeng1 had a problem deploying to ci July 3, 2026 08:08 — with GitHub Actions Error

jiangxinmeng1 temporarily deployed to ci July 3, 2026 08:08 — with GitHub Actions Inactive

jiangxinmeng1 had a problem deploying to ci July 3, 2026 08:08 — with GitHub Actions Error

jiangxinmeng1 temporarily deployed to ci July 3, 2026 08:08 — with GitHub Actions Inactive

jiangxinmeng1 had a problem deploying to ci July 3, 2026 08:11 — with GitHub Actions Error

jiangxinmeng1 temporarily deployed to ci July 3, 2026 08:11 — with GitHub Actions Inactive

matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Jul 3, 2026

fix(logservice): include last connect error on timeout

b4b6794

jiangxinmeng1 temporarily deployed to ci July 3, 2026 08:45 — with GitHub Actions Inactive

XuPeng-SH requested changes Jul 3, 2026

View reviewed changes

fix logservice connect attempt timeout cause

40cb45a

XuPeng-SH requested changes Jul 6, 2026

View reviewed changes

fix(logservice): preserve caller connect timeout

26d56ad

XuPeng-SH approved these changes Jul 6, 2026

View reviewed changes

Merge branch '3.0-dev' into fix-logservice-stale-replica-fallback-3.0…

76d9f29

…-dev

XuPeng-SH requested changes Jul 6, 2026

View reviewed changes

fix(logservice): avoid blocking on stale connect attempts

5fe0ab4

XuPeng-SH approved these changes Jul 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix logservice client fallback from slow replica addresses#25334

Fix logservice client fallback from slow replica addresses#25334
mergify[bot] merged 10 commits into
matrixorigin:3.0-devfrom
jiangxinmeng1:fix-logservice-stale-replica-fallback-3.0-dev

jiangxinmeng1 commented Jul 1, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Uh oh!

XuPeng-SH left a comment

Uh oh!

XuPeng-SH commented Jul 1, 2026

Uh oh!

LeftHandCold left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

mergify Bot commented Jul 6, 2026 •

edited

Loading

Uh oh!

XuPeng-SH left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

mergify Bot commented Jul 6, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

jiangxinmeng1 commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Qodo reviews are paused for this user.

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH commented Jul 1, 2026

Uh oh!

LeftHandCold left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jiangxinmeng1 commented Jul 1, 2026 •

edited

Loading

mergify Bot commented Jul 6, 2026 •

edited

Loading

mergify Bot commented Jul 6, 2026 •

edited

Loading