Skip to content

Commit 0d7eeb3

Browse files
fix: prevent nested local shuffle deadlocks (#26055)
## What type of PR is this? - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes Fixes #25922 ## What this PR does / why we need it The unified Shuffle implementation added bounded backpressure to shared local Shuffle pools. Per-bucket credits fixed same-level hot-bucket starvation, but a finite fixed-bucket pool can still deadlock when more than one local Shuffle is duplicated into the same pull-based worker tree. The observed 16-worker wait-for cycle was: 1. an outer Shuffle writer filled its target bucket and waited for that bucket's worker; 2. the target worker was blocked in an inner Shuffle waiting for every inner writer to finish; 3. the missing inner writer was the child of the blocked outer writer, so it could only finish if the outer writer resumed pulling. This change prevents the cyclic topology instead of increasing or removing the memory bound: - keep the first fixed-bucket local Shuffle fast path; - detect a fixed-bucket Shuffle already present in a packed operator tree; - materialize a second join/group Shuffle through the existing producer scopes, bounded dispatch channels, and consumer scopes; - preserve local Shuffle reuse when no second probe Shuffle is introduced; - cancel an active request before connection cleanup waits for migration/reset or transaction rollback, so cleanup is independent of a stuck data path. The fallback remains bounded: its existing dispatch channel capacity is 32, and this PR does not change Shuffle ready credits or introduce an unbounded queue. It also does not introduce a new hard query timeout. ## Regression coverage - 16-way nested local shuffle join, on both probe and build chains; - 16-way nested local shuffle group; - fixed Shuffle below a non-root operator to cover recursive detection; - single-layer local group and reusable local join fast paths remain local; - connection cleanup cancels before a blocked lifecycle operation and before rollback. The nested-join regression failed on `origin/main` by producing one packed local scope and passes with this change by producing 16 independent bucket scopes. ## Validation - `go test -race -count=1 -timeout=600s ./pkg/sql/compile ./pkg/frontend` - focused new concurrency/topology tests under `-race -count=20` - `go test -v -count=1 -timeout=600s ./pkg/sql/compile ./pkg/frontend` - `go test -count=1 ./pkg/sql/colexec/shuffle ./pkg/sql/colexec/dispatch ./pkg/sql/colexec/merge ./pkg/sql/colexec/group` - `go build ./pkg/sql/compile ./pkg/frontend` - `go vet ./pkg/sql/compile ./pkg/frontend` - `git diff --check` --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent b1c11b2 commit 0d7eeb3

18 files changed

Lines changed: 1220 additions & 68 deletions

pkg/cnservice/server_query.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ func (s *service) handleMigrateConnFrom(
531531
}
532532
rm := s.mo.GetRoutineManager()
533533
resp.MigrateConnFromResponse = &query.MigrateConnFromResponse{}
534-
if err := rm.MigrateConnectionFrom(req.MigrateConnFromRequest, resp.MigrateConnFromResponse); err != nil {
534+
if err := rm.MigrateConnectionFromWithContext(ctx, req.MigrateConnFromRequest, resp.MigrateConnFromResponse); err != nil {
535535
logutil.Errorf("failed to migrate conn from: %v", err)
536536
return err
537537
}
@@ -592,7 +592,7 @@ func (s *service) handleResetSession(
592592
}
593593
rm := s.mo.GetRoutineManager()
594594
resp.ResetSessionResponse = &query.ResetSessionResponse{}
595-
if err := rm.ResetSession(req.ResetSessionRequest, resp.ResetSessionResponse); err != nil {
595+
if err := rm.ResetSessionWithContext(ctx, req.ResetSessionRequest, resp.ResetSessionResponse); err != nil {
596596
logutil.Errorf("failed to reset session: %v", err)
597597
return err
598598
}

pkg/frontend/migrate.go

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,17 +14,25 @@
1414

1515
package frontend
1616

17-
import "sync"
17+
import (
18+
"context"
19+
"sync"
20+
)
1821

1922
// migrateController is created in Routine and used to serialize lifecycle
2023
// operations with routine cleanup.
2124
type migrateController struct {
2225
sync.Mutex
2326
migrateOnce sync.Once
27+
// migrateErr is the stable result returned to duplicate migration calls.
28+
migrateErr error
2429
// closed indicates if the session has been closed.
2530
closed bool
2631
// inProgress indicates if a lifecycle operation is in progress.
2732
inProgress bool
33+
// operationCancel cancels the active lifecycle operation. It is published
34+
// together with inProgress while holding the controller lock.
35+
operationCancel context.CancelFunc
2836
// cond coordinates lifecycle operations and routine cleanup.
2937
cond *sync.Cond
3038
// the id of goroutine that executes the migration
@@ -41,50 +49,96 @@ func newMigrateController() *migrateController {
4149
// operation is in progress, it waits for that operation to finish and marks
4250
// the routine closed.
4351
func (mc *migrateController) waitAndClose() {
52+
mc.startClose()
4453
mc.Lock()
4554
defer mc.Unlock()
46-
mc.closed = true
4755
for mc.inProgress {
4856
mc.cond.Wait()
4957
}
5058
}
5159

60+
// startClose seals lifecycle admission and cancels the active operation. It
61+
// does not wait, so connection-liveness control paths can remain independent
62+
// of the operation they are stopping.
63+
func (mc *migrateController) startClose() {
64+
mc.Lock()
65+
mc.closed = true
66+
cancel := mc.operationCancel
67+
mc.cond.Broadcast()
68+
mc.Unlock()
69+
if cancel != nil {
70+
cancel()
71+
}
72+
}
73+
5274
// beginOperation starts a lifecycle operation unless the routine is closed or
5375
// another operation is already running.
5476
func (mc *migrateController) beginOperation() bool {
77+
_, ok := mc.beginOperationWithContext(context.Background())
78+
return ok
79+
}
80+
81+
// beginOperationWithContext starts a cancelable lifecycle operation. A waiting
82+
// caller is released by operation completion, routine close, or its own
83+
// context cancellation.
84+
func (mc *migrateController) beginOperationWithContext(ctx context.Context) (context.Context, bool) {
85+
if ctx == nil {
86+
ctx = context.Background()
87+
}
88+
stopWakeup := context.AfterFunc(ctx, func() {
89+
mc.Lock()
90+
mc.cond.Broadcast()
91+
mc.Unlock()
92+
})
93+
defer stopWakeup()
94+
5595
mc.Lock()
5696
defer mc.Unlock()
57-
for mc.inProgress && !mc.closed {
97+
for mc.inProgress && !mc.closed && ctx.Err() == nil {
5898
mc.cond.Wait()
5999
}
60-
return mc.startOperationLocked()
100+
return mc.startOperationLocked(ctx)
61101
}
62102

63103
// tryBeginOperation starts a lifecycle operation only if it can proceed
64104
// immediately.
65105
func (mc *migrateController) tryBeginOperation() bool {
106+
_, ok := mc.tryBeginOperationWithContext(context.Background())
107+
return ok
108+
}
109+
110+
func (mc *migrateController) tryBeginOperationWithContext(ctx context.Context) (context.Context, bool) {
111+
if ctx == nil {
112+
ctx = context.Background()
113+
}
66114
mc.Lock()
67115
defer mc.Unlock()
68-
return mc.startOperationLocked()
116+
return mc.startOperationLocked(ctx)
69117
}
70118

71-
func (mc *migrateController) startOperationLocked() bool {
72-
if mc.closed {
73-
return false
119+
func (mc *migrateController) startOperationLocked(ctx context.Context) (context.Context, bool) {
120+
if mc.closed || ctx.Err() != nil {
121+
return nil, false
74122
}
75123
if mc.inProgress {
76-
return false
124+
return nil, false
77125
}
126+
operationCtx, cancel := context.WithCancel(ctx)
78127
mc.goroutineID = GetRoutineId()
79128
mc.inProgress = true
80-
return true
129+
mc.operationCancel = cancel
130+
return operationCtx, true
81131
}
82132

83133
// endOperation completes a lifecycle operation and wakes a routine waiting
84134
// to close.
85135
func (mc *migrateController) endOperation() {
86136
mc.Lock()
87137
defer mc.Unlock()
138+
if mc.operationCancel != nil {
139+
mc.operationCancel()
140+
mc.operationCancel = nil
141+
}
88142
mc.inProgress = false
89143
mc.goroutineID = 0
90144
mc.cond.Broadcast()

pkg/frontend/migrate_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package frontend
1616

1717
import (
18+
"context"
1819
"testing"
1920
"time"
2021

@@ -107,6 +108,68 @@ func TestLifecycleControllerCloseRejectsQueuedOperation(t *testing.T) {
107108
}
108109
}
109110

111+
func TestLifecycleControllerCloseCancelsActiveOperation(t *testing.T) {
112+
mc := newMigrateController()
113+
operationCtx, ok := mc.beginOperationWithContext(context.Background())
114+
assert.True(t, ok)
115+
116+
operationDone := make(chan struct{})
117+
go func() {
118+
<-operationCtx.Done()
119+
mc.endOperation()
120+
close(operationDone)
121+
}()
122+
123+
closeDone := make(chan struct{})
124+
go func() {
125+
mc.waitAndClose()
126+
close(closeDone)
127+
}()
128+
129+
select {
130+
case <-operationCtx.Done():
131+
case <-time.After(time.Second):
132+
t.Fatal("close did not cancel the active lifecycle operation")
133+
}
134+
select {
135+
case <-operationDone:
136+
case <-time.After(time.Second):
137+
t.Fatal("active lifecycle operation did not finish after cancellation")
138+
}
139+
select {
140+
case <-closeDone:
141+
case <-time.After(time.Second):
142+
t.Fatal("close did not finish after the active lifecycle operation exited")
143+
}
144+
assert.False(t, mc.beginOperation())
145+
}
146+
147+
func TestLifecycleControllerWaitingCallerHonorsContext(t *testing.T) {
148+
mc := newMigrateController()
149+
assert.True(t, mc.beginOperation())
150+
151+
ctx, cancel := context.WithCancel(context.Background())
152+
waiterReady := make(chan struct{})
153+
result := make(chan bool)
154+
go func() {
155+
close(waiterReady)
156+
_, ok := mc.beginOperationWithContext(ctx)
157+
result <- ok
158+
}()
159+
<-waiterReady
160+
cancel()
161+
162+
select {
163+
case ok := <-result:
164+
assert.False(t, ok)
165+
case <-time.After(time.Second):
166+
t.Fatal("waiting lifecycle operation ignored caller cancellation")
167+
}
168+
169+
mc.endOperation()
170+
mc.waitAndClose()
171+
}
172+
110173
func TestCloseWaitMigrate(t *testing.T) {
111174
mc := newMigrateController()
112175
assert.NotNil(t, mc)

0 commit comments

Comments
 (0)