fix(txn): restore recovery routes and cancellation by VioletQwQ-0 · Pull Request #25956 · matrixorigin/matrixone

VioletQwQ-0 · 2026-07-21T08:31:09Z

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Recovered prepared/committing transaction metadata may contain only durable participant identity. Sending recovery RPCs with that metadata currently uses an empty ReplicaID and address. At the same time, a missing participant can make TxnService.Start() wait indefinitely, while replica or store shutdown waits for Start() before it can close the service.

This PR establishes one recovery invariant: recovery RPCs use complete current participant routes, and TN shutdown can terminate every recovery producer, consumer, route wait, and blocked Start().

Failure	Change
Recovered remote participant has no RPC route	Resolve `ShardID` through the current TN cluster snapshot before `GetStatus` or `CommitTNShard`; preserve a durable `LogShardID` when the snapshot omits it.
Participant or cluster metadata is temporarily unavailable	Retry lookup and authoritative refresh without sending to an incomplete route.
A historical Prepared record names a retired participant but a later Committed record supersedes it	Fold the complete recovery stream first; resolve routes only for coordinator-owned transactions that remain Prepared/Committing.
Route never appears	Add a recovery cancellation context and let replica shutdown cancel it before waiting for `Start()`.
Store shutdown joins a replica task blocked in recovery	Deliver `cancelStart` and `CancelRecovery` to replicas before stopping the store task group; keep storage open until RPC drain completes.
HAKeeper never produces the initial cluster snapshot	Stop the cluster refresh task before releasing readiness waiters, so `Store.Close()` does not depend on metadata readiness.
Storage producer blocks on a read or a full recovery channel	Treat context-canceled MEMKV reads as normal termination and make publication select on `ctx.Done()`.
Multiple surviving recovery transactions share the same current TN routes	Refresh one authoritative TN snapshot per recovery pass, reuse it across transactions, and refresh/retry only when the shared snapshot lacks a required route.

Compatibility and non-goals:

Single-participant and non-coordinator recovery keep the existing local replay path; only coordinators with final unresolved Prepared/Committing state refresh participant routes before sending recovery RPCs.
No wire or persisted format changes.
No AUTO_INCREMENT behavior, allocator changes, maximum prepared timestamp repair, or replay/logtail ordering changes are included.

Validation:

go test ./pkg/clusterservice ./pkg/txn/service ./pkg/txn/storage/mem -count=1
go test ./pkg/tnservice -count=1
go test ./pkg/txn/... -count=1
focused recovery and store-close lifecycle tests under -race
both new timing-sensitive regressions repeated 20 times
in-flight MEMKV log-read cancellation and non-coordinator missing-route startup
go vet ./pkg/clusterservice ./pkg/txn/service ./pkg/tnservice ./pkg/txn/storage/mem ./pkg/vm/engine/test/testutil
go test ./pkg/... -run '^$' -count=1
git diff --check

Diff size: 8 production/interface/helper files / about 260 added handwritten lines; 6 test files / about 850 added test lines; no generated code.

Co-authored-by: ULookup CHBulookup@outlook.com

Resolve incomplete participant routes before recovery RPCs and make the full TN startup recovery chain cancellable during shutdown. Co-authored-by: ULookup <CHBulookup@outlook.com>

qodo-code-review · 2026-07-21T09:21:03Z

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 →

gouhongshen

Codex automated review

Two blocking recovery lifecycle regressions remain. Focused package and race tests pass but do not cover them.

P1 - Cancellation of a MEMKV log read panics the TN (pkg/txn/storage/mem/kv_txn_storage.go:145)

startRecovery now passes the cancellable recovery context into StartRecovery. During replica shutdown, CancelRecovery can therefore cancel an in-flight MEMKV logClient.Read; the real log-service RPC future returns ctx.Err() on cancellation. This code panics on every read error, so shutting down a MEMKV-backed TN while its recovery read is slow/unavailable crashes the process instead of terminating recovery. The added test only cancels a full channel send, not an in-flight read. Treat context cancellation as normal producer termination.

P1 - A non-coordinator replica now blocks startup on unrelated participant routes (pkg/txn/service/service_recovery.go:50)

Route resolution runs for every recovered prepared/committing record before addLog. But end only sends recovery RPCs when this service is the transaction coordinator (txnMeta.TNShards[0]). For a non-coordinator B in [A, B, C], if C is absent from the snapshot, B now retries forever in route resolution even though B will never issue a recovery RPC; on origin/main it added the log and end skipped recovery work, allowing B to start. This makes B unavailable for unrelated work while another participant is missing. Resolve/wait only on the coordinator path and add a non-coordinator missing-route regression test.

XuPeng-SH

Deep re-review completed on exact head a4d302b. No blocking or non-blocking correctness finding remains.

I re-audited the complete diff and every previously reported edge:

recovery first folds the complete durable stream, then resolves routes only for surviving coordinator-owned Prepared/Committing transactions;
one authoritative TN snapshot is reused per recovery pass, with refresh/retry only when a required route is absent;
missing/stale/transient routes, cluster readiness, retry timers, MEMKV reads and bounded channel publication all terminate through the recovery context;
store shutdown cancels both create and recovery before stopper join, including replicas admitted after the initial Range through stopper cancellation propagation;
the reserveStart to service publication window terminates through serviceC or startedC, and close remains idempotent with storage kept alive until RPC drain.

Q1 partial construction: service publication and startup completion both have explicit terminal signals. Q2 blocking operations: every new wait has recovery/store cancellation. Q3 partial cleanup: recovery completion is once-guarded; replica close remains the single service/storage cleanup owner.

Fresh validation on this exact head:

all tests in clusterservice, txn/service, txn/storage/mem and tnservice;
full pkg/txn/... suite;
all new route/fold/cancel/late-generation regressions under race, count 20;
affected package build and vet;
clean merge with current main c0486a2, followed by the four package suites and the same race matrix at count 10;
PR diff-check clean.

The current-head CI jobs are still running, so this approval does not claim those unfinished jobs passed.

aptend

Request changes: one P1 shutdown closure gap remains on exact head a4d302b.

P1 — Store.Close still hangs when the cluster never becomes ready

Store.Close now correctly cancels replica recovery before joining the store stopper, but it then calls s.moCluster.Close(). The built-in cluster.Close() executes waitReady() before stopper.Stop(). If the initial HAKeeper GetClusterDetails calls keep failing, readyC is never closed, so shutdown blocks forever at moCluster.Close() and cannot stop the refresh task that owns the only path to readiness.

I reproduced this on the exact head with a cluster client that continuously returns a HAKeeper error and a 10 ms refresh interval: Close was still blocked after 100 ms and returned only after readyC was manually released. This leaves the PR stated shutdown invariant incomplete for the cluster-metadata-unavailable case.

Please make cluster shutdown cancel/stop the refresh task before waiting for readiness, and explicitly release readiness waiters (or model a separate closed state). Add an end-to-end regression where the initial cluster refresh never succeeds and Store.Close still terminates.

Relevant paths:

matrixone/pkg/tnservice/store.go

Lines 240 to 252 in a4d302b

    
           func (s *store) Close() error { 
        
           	// Reject new replica calls and cancel active call contexts before waiting 
        
           	// for store tasks. A published service may be blocked in recovery, so its 
        
           	// cancellation must be delivered before joining the store stopper. Storage 
        
           	// remains open until the RPC server drains below. 
        
           	s.replicas.Range(func(_, value any) bool { 
        
           		r := value.(*replica) 
        
           		r.cancelStart(false) 
        
           		r.cancelRecovery() 
        
           		return true 
        
           	}) 
        
           	s.stopper.Stop() 
        
           	s.moCluster.Close()

matrixone/pkg/clusterservice/cluster.go

Lines 368 to 371 in a4d302b

    
           func (c *cluster) Close() { 
        
           	c.waitReady() 
        
           	c.stopper.Stop() 
        
           	close(c.forceRefreshC)

Other validation on this head passed: affected package tests, focused recovery/lifecycle tests under -race -count=3, and go vet.

VioletQwQ-0 · 2026-07-22T03:34:19Z

Addressed the latest shutdown CR in commit 3cee83bf4.

The failure reproduced on a4d302b6e9: with every initial GetClusterDetails call failing, Store.Close() remained blocked in cluster.Close() because it waited on readyC before stopping the refresh task that owned the only readiness path.

The fix now stops the cluster refresh/GC tasks first and then uses readyOnce to release any readiness waiters. It also no longer closes forceRefreshC; after the consumer has stopped, leaving this bounded channel owned by the cluster avoids a concurrent send-on-closed-channel panic and keeps repeated Close() safe.

Added TestStoreCloseWhenClusterNeverBecomesReady, which starts a real TN store with a HAKeeper client that always fails cluster refresh and verifies Store.Close() terminates.

Validation on 3cee83bf4:

regression test failed before the fix and passed -count=20 after it;
focused cluster/store lifecycle tests passed under -race -count=10;
full tests for pkg/clusterservice, pkg/txn/service, pkg/txn/storage/mem, and pkg/tnservice;
full pkg/txn/... tests;
go vet for the affected packages;
full go test ./pkg/... -run "^$" compile check;
git diff --check.

I also re-audited Q1–Q3 for cluster shutdown: both refresh and close terminate readyC waiters, stopper-owned tasks are joined, and the remaining force-refresh channel is bounded to one entry.

aptend

Re-reviewed exact head 3cee83bf4d. The previous P1 shutdown gap is fixed: cluster close now stops refresh/GC tasks before releasing readiness waiters through readyOnce, while the bounded force-refresh channel remains safe for concurrent/repeated close paths.

Validation passed: affected-package tests, focused lifecycle/recovery tests under -race -count=5, go vet, and git diff --check. No remaining findings.

mergify · 2026-07-22T04:47:40Z

Merge Queue Status

✅ Entered queue — 2026-07-22 04:47 UTC · Rule: main · triggered by rule Automatic queue on approval for main
🟠 Checks running · in-place
🚫 Left the queue — 2026-07-22 05:10 UTC · at 45a08afaac9d41390924292cf554b0a49e69a35d

This pull request spent 22 minutes 37 seconds in the queue, with no time running CI.

Waiting for

All conditions

Reason

Pull request #25956 has been dequeued

Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.

The author needs to enable "Allow edits from maintainers" on this pull request.

Failing checks:

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.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

Requeue this pull request

fix(txn): restore recovery routes and cancellation

f433a26

Resolve incomplete participant routes before recovery RPCs and make the full TN startup recovery chain cancellable during shutdown. Co-authored-by: ULookup <CHBulookup@outlook.com>

VioletQwQ-0 temporarily deployed to ci July 21, 2026 08:31 — with GitHub Actions Inactive

VioletQwQ-0 mentioned this pull request Jul 21, 2026

feat(plan): support ALTER TABLE AUTO_INCREMENT = value #25883

Closed

7 tasks

VioletQwQ-0 temporarily deployed to ci July 21, 2026 08:31 — with GitHub Actions Inactive

VioletQwQ-0 had a problem deploying to ci July 21, 2026 08:31 — with GitHub Actions Error

matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Jul 21, 2026

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

fix: close transaction recovery lifecycle gaps

dc4c066

VioletQwQ-0 temporarily deployed to ci July 21, 2026 09:11 — with GitHub Actions Inactive

VioletQwQ-0 marked this pull request as ready for review July 21, 2026 09:20

VioletQwQ-0 requested review from XuPeng-SH, aptend, iamlinjunhong and volgariver6 as code owners July 21, 2026 09:20

VioletQwQ-0 mentioned this pull request Jul 21, 2026

ALTER TABLE AUTO_INCREMENT = value not supported #23143

Open

gouhongshen requested changes Jul 21, 2026

View reviewed changes

fix(txn): close recovery cancellation regressions

fa55d75

VioletQwQ-0 temporarily deployed to ci July 21, 2026 10:53 — with GitHub Actions Inactive

VioletQwQ-0 temporarily deployed to ci July 21, 2026 10:54 — with GitHub Actions Inactive

VioletQwQ-0 requested review from XuPeng-SH and gouhongshen July 22, 2026 02:44

XuPeng-SH approved these changes Jul 22, 2026

View reviewed changes

aptend requested changes Jul 22, 2026

View reviewed changes

fix(clusterservice): unblock close before readiness

3cee83b

VioletQwQ-0 requested a review from aptend July 22, 2026 03:34

VioletQwQ-0 temporarily deployed to ci July 22, 2026 03:36 — with GitHub Actions Inactive

aptend approved these changes Jul 22, 2026

View reviewed changes

mergify Bot added the queued label Jul 22, 2026

Merge branch 'main' into codex/issue-25951-txn-recovery-route-cancel

45a08af

mergify Bot temporarily deployed to ci July 22, 2026 05:09 Inactive

XuPeng-SH merged commit bbca3f3 into matrixorigin:main Jul 22, 2026
23 of 24 checks passed

XuPeng-SH had a problem deploying to ci July 22, 2026 05:09 — with GitHub Actions Failure

XuPeng-SH temporarily deployed to ci July 22, 2026 05:09 — with GitHub Actions Inactive

This was referenced Jul 22, 2026

TN recovery can use incomplete participant routes and block shutdown #25951

Closed

feat(auto-increment): add transactional epoch fence #26027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(txn): restore recovery routes and cancellation#25956

fix(txn): restore recovery routes and cancellation#25956
XuPeng-SH merged 8 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/issue-25951-txn-recovery-route-cancel

VioletQwQ-0 commented Jul 21, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 21, 2026

Uh oh!

gouhongshen left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

aptend left a comment

Uh oh!

VioletQwQ-0 commented Jul 22, 2026

Uh oh!

aptend left a comment

Uh oh!

mergify Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	func (s *store) Close() error {
	// Reject new replica calls and cancel active call contexts before waiting
	// for store tasks. A published service may be blocked in recovery, so its
	// cancellation must be delivered before joining the store stopper. Storage
	// remains open until the RPC server drains below.
	s.replicas.Range(func(_, value any) bool {
	r := value.(*replica)
	r.cancelStart(false)
	r.cancelRecovery()
	return true
	})
	s.stopper.Stop()
	s.moCluster.Close()

	func (c *cluster) Close() {
	c.waitReady()
	c.stopper.Stop()
	close(c.forceRefreshC)

Uh oh!

Conversation

VioletQwQ-0 commented Jul 21, 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 21, 2026

Qodo reviews are paused for this user.

Uh oh!

gouhongshen left a comment

Choose a reason for hiding this comment

Codex automated review

P1 - Cancellation of a MEMKV log read panics the TN (pkg/txn/storage/mem/kv_txn_storage.go:145)

P1 - A non-coordinator replica now blocks startup on unrelated participant routes (pkg/txn/service/service_recovery.go:50)

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

aptend left a comment

Choose a reason for hiding this comment

P1 — Store.Close still hangs when the cluster never becomes ready

Uh oh!

VioletQwQ-0 commented Jul 22, 2026

Uh oh!

aptend left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

VioletQwQ-0 commented Jul 21, 2026 •

edited

Loading

mergify Bot commented Jul 22, 2026 •

edited

Loading