Skip to content

Commit bbca3f3

Browse files
VioletQwQ-0ULookupmergify[bot]
authored
fix(txn): restore recovery routes and cancellation (#25956)
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #25951 ## 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> --------- Co-authored-by: ULookup <CHBulookup@outlook.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent ddf4675 commit bbca3f3

14 files changed

Lines changed: 1263 additions & 30 deletions

File tree

pkg/clusterservice/cluster.go

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,33 @@ func GetCNServiceWithoutWorkingStateWithContext(
121121
return ctx.Err()
122122
}
123123

124+
// GetAllTNServicesWithContext returns a TN service snapshot without waiting
125+
// past ctx for the built-in cluster's initial HAKeeper refresh.
126+
func GetAllTNServicesWithContext(
127+
ctx context.Context,
128+
service MOCluster,
129+
) ([]metadata.TNService, error) {
130+
if ctx == nil {
131+
ctx = context.Background()
132+
}
133+
if err := ctx.Err(); err != nil {
134+
return nil, err
135+
}
136+
if service == nil {
137+
return nil, moerr.NewInternalErrorNoCtx("mocluster service is not initialized")
138+
}
139+
if builtIn, ok := service.(*cluster); ok {
140+
if err := builtIn.waitReadyWithContext(ctx); err != nil {
141+
return nil, err
142+
}
143+
services := builtIn.services.Load()
144+
return append([]metadata.TNService(nil), services.tn...), ctx.Err()
145+
}
146+
147+
services := service.GetAllTNServices()
148+
return services, ctx.Err()
149+
}
150+
124151
func lookupMOCluster(service string) (MOCluster, bool, error) {
125152
rt := runtime.ServiceRuntime(service)
126153
if rt == nil {
@@ -339,9 +366,14 @@ func (c *cluster) Refresh(ctx context.Context) error {
339366
}
340367

341368
func (c *cluster) Close() {
342-
c.waitReady()
343369
c.stopper.Stop()
344-
close(c.forceRefreshC)
370+
// A failed initial refresh leaves readiness waiters blocked. Once the
371+
// refresh task has stopped, release them so shutdown does not depend on
372+
// HAKeeper becoming available.
373+
c.readyOnce.Do(func() {
374+
c.ready.Store(true)
375+
close(c.readyC)
376+
})
345377
}
346378

347379
// DebugUpdateCNLabel implements the MOCluster interface.

pkg/clusterservice/cluster_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,16 @@ func TestCNServiceSnapshotHonorsCancellationWhileClusterStarts(t *testing.T) {
7070
require.ErrorIs(t, err, context.DeadlineExceeded)
7171
}
7272

73+
func TestTNServiceSnapshotHonorsCancellationWhileClusterStarts(t *testing.T) {
74+
c := &cluster{readyC: make(chan struct{})}
75+
c.services.Store(&services{})
76+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond)
77+
defer cancel()
78+
79+
_, err := GetAllTNServicesWithContext(ctx, c)
80+
require.ErrorIs(t, err, context.DeadlineExceeded)
81+
}
82+
7383
func TestClusterForceRefresh(t *testing.T) {
7484
runClusterTest(
7585
time.Hour,

pkg/tnservice/replica.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ type replica struct {
3535
logger *log.MOLogger
3636
shard metadata.TNShard
3737
service service.TxnService
38+
serviceC chan struct{}
3839
startedC chan struct{}
3940
createCtx context.Context
4041
cancelCreate context.CancelFunc
@@ -81,6 +82,7 @@ func newReplica(shard metadata.TNShard, rt runtime.Runtime) *replica {
8182
rt: rt,
8283
shard: shard,
8384
logger: rt.Logger().With(util.TxnTNShardField(shard)),
85+
serviceC: make(chan struct{}),
8486
startedC: make(chan struct{}),
8587
createCtx: ctx,
8688
cancelCreate: cancel,
@@ -114,6 +116,7 @@ func (r *replica) startReserved(txnService service.TxnService) error {
114116
}
115117
r.service = txnService
116118
r.mu.Unlock()
119+
close(r.serviceC)
117120

118121
err := txnService.Start()
119122
r.finishStart(err)
@@ -159,13 +162,39 @@ func (r *replica) close(destroy bool) error {
159162
return r.closeErr
160163
}
161164

165+
func (r *replica) cancelRecovery() {
166+
r.mu.RLock()
167+
starting := r.mu.starting
168+
txnService := r.service
169+
r.mu.RUnlock()
170+
if !starting {
171+
return
172+
}
173+
if txnService == nil {
174+
// Once start is reserved, startReserved either publishes the service or
175+
// finishStart reports that startup ended without one.
176+
select {
177+
case <-r.serviceC:
178+
case <-r.startedC:
179+
}
180+
r.mu.RLock()
181+
txnService = r.service
182+
r.mu.RUnlock()
183+
}
184+
if txnService != nil {
185+
txnService.CancelRecovery()
186+
}
187+
}
188+
162189
func (r *replica) closeOnceFn() error {
163190
r.mu.RLock()
164191
starting := r.mu.starting
165192
r.mu.RUnlock()
166193
if !starting {
167194
return nil
168195
}
196+
// Recovery may block Start indefinitely while waiting for a participant.
197+
r.cancelRecovery()
169198

170199
r.waitStartCompleted()
171200
r.mu.Lock()

pkg/tnservice/replica_test.go

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,20 @@ package tnservice
1717
import (
1818
"context"
1919
"errors"
20+
"sync"
2021
"testing"
2122
"time"
2223

24+
"github.com/matrixorigin/matrixone/pkg/clusterservice"
2325
"github.com/matrixorigin/matrixone/pkg/common/runtime"
26+
"github.com/matrixorigin/matrixone/pkg/defines"
27+
"github.com/matrixorigin/matrixone/pkg/fileservice"
28+
logpb "github.com/matrixorigin/matrixone/pkg/pb/logservice"
29+
"github.com/matrixorigin/matrixone/pkg/pb/metadata"
2430
"github.com/matrixorigin/matrixone/pkg/pb/txn"
2531
"github.com/matrixorigin/matrixone/pkg/txn/service"
2632
"github.com/matrixorigin/matrixone/pkg/txn/storage"
33+
"github.com/matrixorigin/matrixone/pkg/txn/storage/mem"
2734
"github.com/stretchr/testify/assert"
2835
"github.com/stretchr/testify/require"
2936
)
@@ -39,6 +46,39 @@ type startErrorStorage struct {
3946
destroyCalls int
4047
}
4148

49+
type closeUnblocksStartTxnService struct {
50+
service.TxnService
51+
started chan struct{}
52+
closed chan struct{}
53+
startOnce sync.Once
54+
closeOnce sync.Once
55+
}
56+
57+
type signalingRecoveryCluster struct {
58+
clusterservice.MOCluster
59+
entered chan struct{}
60+
once sync.Once
61+
}
62+
63+
func (c *signalingRecoveryCluster) GetAllTNServices() []metadata.TNService {
64+
c.once.Do(func() { close(c.entered) })
65+
return c.MOCluster.GetAllTNServices()
66+
}
67+
68+
func (s *closeUnblocksStartTxnService) Start() error {
69+
s.startOnce.Do(func() { close(s.started) })
70+
<-s.closed
71+
return context.Canceled
72+
}
73+
74+
func (s *closeUnblocksStartTxnService) CancelRecovery() {
75+
s.closeOnce.Do(func() { close(s.closed) })
76+
}
77+
78+
func (s *closeUnblocksStartTxnService) Close(bool) error {
79+
return nil
80+
}
81+
4282
type closeTrackingTxnService struct {
4383
service.TxnService
4484
closeCalls int
@@ -210,6 +250,133 @@ func TestCloseFailedStartReplica(t *testing.T) {
210250
}
211251
}
212252

253+
func TestCloseCancelsBlockedReplicaStart(t *testing.T) {
254+
txnService := &closeUnblocksStartTxnService{
255+
started: make(chan struct{}),
256+
closed: make(chan struct{}),
257+
}
258+
r := newReplica(newTestTNShard(1, 2, 3), runtime.DefaultRuntime())
259+
startResult := make(chan error, 1)
260+
go func() {
261+
startResult <- r.start(txnService)
262+
}()
263+
264+
select {
265+
case <-txnService.started:
266+
case <-time.After(time.Second):
267+
t.Fatal("replica start did not begin")
268+
}
269+
270+
closeResult := make(chan error, 1)
271+
go func() {
272+
closeResult <- r.close(false)
273+
}()
274+
275+
select {
276+
case err := <-closeResult:
277+
require.ErrorIs(t, err, context.Canceled)
278+
case <-time.After(time.Second):
279+
t.Fatal("close did not cancel blocked replica start")
280+
}
281+
require.ErrorIs(t, <-startResult, context.Canceled)
282+
}
283+
284+
func TestRemoveReplicaCancelsBlockedStartBeforeWaiting(t *testing.T) {
285+
txnService := &closeUnblocksStartTxnService{
286+
started: make(chan struct{}),
287+
closed: make(chan struct{}),
288+
}
289+
r := newReplica(newTestTNShard(1, 2, 3), runtime.DefaultRuntime())
290+
startResult := make(chan error, 1)
291+
go func() { startResult <- r.start(txnService) }()
292+
select {
293+
case <-txnService.started:
294+
case <-time.After(time.Second):
295+
t.Fatal("replica start did not begin")
296+
}
297+
298+
fs, err := fileservice.NewMemoryFS(
299+
defines.LocalFileServiceName,
300+
fileservice.DisabledCacheConfig,
301+
nil,
302+
)
303+
require.NoError(t, err)
304+
s := &store{
305+
cfg: &Config{UUID: "test"},
306+
rt: runtime.DefaultRuntime(),
307+
metadataFileService: fs,
308+
replicas: &sync.Map{},
309+
}
310+
s.replicas.Store(r.shard.ShardID, r)
311+
312+
removed := make(chan error, 1)
313+
go func() { removed <- s.removeReplicaLocked(r.shard.ShardID) }()
314+
select {
315+
case err := <-removed:
316+
require.ErrorIs(t, err, context.Canceled)
317+
case <-time.After(time.Second):
318+
t.Fatal("removeReplicaLocked waited for Start before canceling recovery")
319+
}
320+
require.ErrorIs(t, <-startResult, context.Canceled)
321+
require.Nil(t, s.getReplica(r.shard.ShardID))
322+
}
323+
324+
func TestCloseCancelsReplicaBlockedInRecovery(t *testing.T) {
325+
meta := service.NewTestTxn(1, 1, 1)
326+
meta.Status = txn.TxnStatus_Prepared
327+
meta.PreparedTS = service.NewTestTimestamp(2)
328+
meta.TNShards = append(meta.TNShards, metadata.TNShard{
329+
TNShardRecord: metadata.TNShardRecord{ShardID: 99},
330+
})
331+
mlog := mem.NewMemLog()
332+
data := (&mem.KVLog{Txn: meta}).MustMarshal()
333+
record := mlog.GetLogRecord(len(data))
334+
record.Type = logpb.UserRecord
335+
record.Data = data
336+
_, err := mlog.Append(context.Background(), record)
337+
require.NoError(t, err)
338+
339+
sender := service.NewTestSender()
340+
t.Cleanup(func() { require.NoError(t, sender.Close()) })
341+
txnService := service.NewTestTxnServiceWithLog(
342+
t, 1, sender, service.NewTestClock(0), mlog)
343+
baseCluster := clusterservice.NewMOCluster(
344+
"dn-uuid", nil, time.Hour,
345+
clusterservice.WithDisableRefresh(),
346+
clusterservice.WithServices(nil, nil),
347+
)
348+
t.Cleanup(baseCluster.Close)
349+
cluster := &signalingRecoveryCluster{
350+
MOCluster: baseCluster,
351+
entered: make(chan struct{}),
352+
}
353+
runtime.ServiceRuntime("dn-uuid").SetGlobalVariables(runtime.ClusterService, cluster)
354+
355+
r := newReplica(newTestTNShard(1, 2, 3), runtime.DefaultRuntime())
356+
startResult := make(chan error, 1)
357+
go func() { startResult <- r.start(txnService) }()
358+
select {
359+
case <-cluster.entered:
360+
case <-time.After(time.Second):
361+
t.Fatal("recovery did not reach the missing participant route wait")
362+
}
363+
364+
closeResult := make(chan error, 1)
365+
go func() { closeResult <- r.close(false) }()
366+
select {
367+
case err := <-closeResult:
368+
require.NoError(t, err)
369+
case <-time.After(time.Second):
370+
t.Fatal("replica close did not cancel real transaction recovery")
371+
}
372+
select {
373+
case err := <-startResult:
374+
require.NoError(t, err)
375+
case <-time.After(time.Second):
376+
t.Fatal("replica start remained blocked after recovery cancellation")
377+
}
378+
}
379+
213380
func TestWaitStarted(t *testing.T) {
214381
r := newReplica(newTestTNShard(1, 2, 3), runtime.DefaultRuntime())
215382
c := make(chan struct{})

pkg/tnservice/store.go

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -238,16 +238,20 @@ func (s *store) Start() error {
238238
}
239239

240240
func (s *store) Close() error {
241-
s.stopper.Stop()
242-
s.moCluster.Close()
243-
244-
var err error
245241
// Reject new replica calls and cancel active call contexts before waiting
246-
// for the RPC server to drain. Storage remains open until the drain ends.
242+
// for store tasks. A published service may be blocked in recovery, so its
243+
// cancellation must be delivered before joining the store stopper. Storage
244+
// remains open until the RPC server drains below.
247245
s.replicas.Range(func(_, value any) bool {
248-
value.(*replica).cancelStart(false)
246+
r := value.(*replica)
247+
r.cancelStart(false)
248+
r.cancelRecovery()
249249
return true
250250
})
251+
s.stopper.Stop()
252+
s.moCluster.Close()
253+
254+
var err error
251255
if s.queryService != nil {
252256
err = errors.Join(err, s.queryService.Close())
253257
}
@@ -333,9 +337,7 @@ func (s *store) createReplicaLocked(shard metadata.TNShard) error {
333337
}
334338

335339
err := s.stopper.RunTask(func(stopperCtx context.Context) {
336-
stopCancelPropagation := context.AfterFunc(stopperCtx, func() {
337-
r.cancelStart(false)
338-
})
340+
stopCancelPropagation := propagateReplicaStopperCancellation(stopperCtx, r)
339341
defer stopCancelPropagation()
340342

341343
for {
@@ -403,6 +405,16 @@ func (s *store) createReplicaLocked(shard metadata.TNShard) error {
403405
return nil
404406
}
405407

408+
func propagateReplicaStopperCancellation(
409+
stopperCtx context.Context,
410+
r *replica,
411+
) func() bool {
412+
return context.AfterFunc(stopperCtx, func() {
413+
r.cancelStart(false)
414+
r.cancelRecovery()
415+
})
416+
}
417+
406418
func waitCreateRetry(stopperCtx, createCtx context.Context) error {
407419
timer := time.NewTimer(retryCreateStorageInterval)
408420
defer timer.Stop()
@@ -418,8 +430,6 @@ func waitCreateRetry(stopperCtx, createCtx context.Context) error {
418430

419431
func (s *store) removeReplicaLocked(tnShardID uint64) error {
420432
if r := s.getReplica(tnShardID); r != nil {
421-
r.cancelStart(true)
422-
r.waitStartCompleted()
423433
err := r.close(true)
424434
s.replicas.CompareAndDelete(tnShardID, r)
425435
s.removeTNShardLocked(tnShardID)

pkg/tnservice/store_rpc_handler_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ func (s *leaseCancelReadTxnService) Start() error {
4747
return nil
4848
}
4949

50+
func (s *leaseCancelReadTxnService) CancelRecovery() {}
51+
5052
func (s *leaseCancelReadTxnService) Close(bool) error {
5153
return nil
5254
}

0 commit comments

Comments
 (0)