Skip to content

Commit a55820b

Browse files
authored
test: stabilize asynchronous cleanup waits (#26256)
## What type of PR is this? - [x] BUG - [x] Test and CI ## Which issue(s) this PR fixes: Fixes #26254 Fixes #26255 ## What this PR does / why we need it: Two unrelated asynchronous subsystems were made flaky by test-only timing assumptions under the parallel Ubuntu/x86 UT workload. ### Root cause - TAE checkpoint tests wrapped real flush/checkpoint I/O in 5, 10, or 20 second deadlines. A single storage `writer.Sync` can legitimately exceed 20 seconds under parallel CI, so `TestSnapshotGC` could cancel `ForceFlush` before the asynchronous work completed. - The user-level-lock test service latched `blockUnlock=true` when `Unlock` entered. Clearing the flag did not release calls already in flight; all four detached-cleanup workers remained occupied until their one-second contexts expired. A saturated 1,024-entry test queue could therefore miss its three-second recovery assertion. ### Changes - Add a shared one-minute checkpoint/flush timeout for storage-backed TAE tests and apply it to the short checkpoint helpers and nearby direct call sites. - Make the user-level-lock test double observe recovery while an unlock is in flight. - Add a deterministic regression proving an already-blocked mock unlock resumes promptly. Production checkpoint and user-lock behavior is unchanged. ## Validation - `TestUserLevelLockCleanupTestServiceUnblocksInFlightUnlock|TestDetachedUserLevelLockCleanupQueueIsBoundedAndDeduped`: `-race -count=20` - `TestSnapshotGC|TestAppendAndGC2|TestCkpLeak|TestReplay2`: `-count=5` - Same TAE set: `-race -count=1` - Full `pkg/sql/plan/function`, `pkg/vm/engine/tae/db/test`, and `pkg/vm/engine/tae/db/testutil` tests - `go build` for affected production/test-support packages - `go vet` for affected test packages
1 parent 4c04a95 commit a55820b

4 files changed

Lines changed: 55 additions & 12 deletions

File tree

pkg/sql/plan/function/func_unary_test.go

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7441,7 +7441,8 @@ func (s *userLevelLockTestService) Unlock(ctx context.Context, txnID []byte, com
74417441
s.unlockMu.Lock()
74427442
s.unlockedTxnIDs = append(s.unlockedTxnIDs, append([]byte(nil), txnID...))
74437443
s.unlockMu.Unlock()
7444-
if s.blockUnlock.Load() {
7444+
waitForUnlock:
7445+
for s.blockUnlock.Load() {
74457446
if s.unlockStarted != nil {
74467447
select {
74477448
case s.unlockStarted <- struct{}{}:
@@ -7451,12 +7452,23 @@ func (s *userLevelLockTestService) Unlock(ctx context.Context, txnID []byte, com
74517452
if s.unlockResume != nil {
74527453
select {
74537454
case <-s.unlockResume:
7455+
break waitForUnlock
74547456
case <-ctx.Done():
74557457
return ctx.Err()
74567458
}
74577459
} else {
7458-
<-ctx.Done()
7459-
return ctx.Err()
7460+
timer := time.NewTimer(time.Millisecond)
7461+
select {
7462+
case <-timer.C:
7463+
case <-ctx.Done():
7464+
if !timer.Stop() {
7465+
select {
7466+
case <-timer.C:
7467+
default:
7468+
}
7469+
}
7470+
return ctx.Err()
7471+
}
74607472
}
74617473
}
74627474
if s.unlockErr != nil {
@@ -7532,6 +7544,35 @@ func runUserLevelLockTest(t *testing.T, fn func([]lockservice.LockService)) {
75327544
resetUserLevelLocksForTest(t)
75337545
}
75347546

7547+
func TestUserLevelLockCleanupTestServiceUnblocksInFlightUnlock(t *testing.T) {
7548+
service := &userLevelLockTestService{
7549+
id: "user-level-lock-unblock",
7550+
state: &userLevelLockTestState{locks: make(map[string]string)},
7551+
unlockStarted: make(chan struct{}, 1),
7552+
}
7553+
service.blockUnlock.Store(true)
7554+
7555+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
7556+
defer cancel()
7557+
done := make(chan error, 1)
7558+
go func() {
7559+
done <- service.Unlock(ctx, []byte("txn"), timestamp.Timestamp{})
7560+
}()
7561+
7562+
select {
7563+
case <-service.unlockStarted:
7564+
case <-time.After(time.Second):
7565+
t.Fatal("unlock did not reach the blocked state")
7566+
}
7567+
service.blockUnlock.Store(false)
7568+
select {
7569+
case err := <-done:
7570+
require.NoError(t, err)
7571+
case <-time.After(500 * time.Millisecond):
7572+
t.Fatal("in-flight unlock did not observe the test service recovery")
7573+
}
7574+
}
7575+
75357576
func requireUserLevelLockTxnUnlocked(t *testing.T, service *userLevelLockTestService, txnID []byte) {
75367577
t.Helper()
75377578
requireUserLevelLockTxnUnlockedFunc(t, service, func(unlocked []byte) bool {

pkg/vm/engine/tae/db/test/db_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7609,7 +7609,7 @@ func TestAppendAndGC2(t *testing.T) {
76097609
assert.Nil(t, err)
76107610
}
76117611
wg.Wait()
7612-
ckpCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
7612+
ckpCtx, cancel := context.WithTimeout(ctx, testutil.TestCheckpointTimeout)
76137613
err = db.ForceCheckpoint(ckpCtx, db.TxnMgr.Now())
76147614
cancel()
76157615
require.NoError(t, err)
@@ -7836,7 +7836,7 @@ func TestSnapshotGC(t *testing.T) {
78367836
snapWG.Wait()
78377837
wg.Wait()
78387838
t.Log(tae.Catalog.SimplePPString(common.PPL1))
7839-
ckpCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
7839+
ckpCtx, cancel := context.WithTimeout(ctx, testutil.TestCheckpointTimeout)
78407840
err = db.ForceCheckpoint(ckpCtx, db.TxnMgr.Now())
78417841
cancel()
78427842
require.NoError(t, err)
@@ -8847,7 +8847,7 @@ func TestCkpLeak(t *testing.T) {
88478847
assert.Nil(t, err)
88488848
}
88498849
wg.Wait()
8850-
ckpCtx, cancel := context.WithTimeout(ctx, time.Minute)
8850+
ckpCtx, cancel := context.WithTimeout(ctx, testutil.TestCheckpointTimeout)
88518851
defer cancel()
88528852
require.NoError(t, db.ForceCheckpoint(ckpCtx, db.TxnMgr.Now()))
88538853
testutil.WaitAllCheckpointsFinished(t, db)

pkg/vm/engine/tae/db/test/replay_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ func TestReplay2(t *testing.T) {
463463
assert.NotNil(t, err)
464464
assert.Nil(t, txn.Commit(context.Background()))
465465

466-
ctx2, cancel := context.WithTimeout(context.Background(), time.Second*10)
466+
ctx2, cancel := context.WithTimeout(context.Background(), testutil.TestCheckpointTimeout)
467467
defer cancel()
468468
err = tae2.ForceFlush(ctx2, tae2.TxnMgr.Now())
469469
assert.NoError(t, err)

pkg/vm/engine/tae/db/testutil/engine.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ import (
4141
"github.com/stretchr/testify/require"
4242
)
4343

44-
const (
45-
DefaultTestDB = "db"
46-
)
44+
const DefaultTestDB = "db"
45+
46+
// TestCheckpointTimeout leaves room for storage syncs under parallel CI while
47+
// still bounding a genuinely stuck checkpoint or flush.
48+
const TestCheckpointTimeout = time.Minute
4749

4850
type CtxOldVersion struct{}
4951

@@ -160,14 +162,14 @@ func (e *TestEngine) CheckRowsByScan(exp int, applyDelete bool) {
160162
assert.NoError(e.T, txn.Commit(context.Background()))
161163
}
162164
func (e *TestEngine) ForceCheckpoint() {
163-
ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
165+
ctx, cancel := context.WithTimeout(context.Background(), TestCheckpointTimeout)
164166
defer cancel()
165167
err := e.DB.ForceCheckpoint(ctx, e.TxnMgr.Now())
166168
assert.NoError(e.T, err)
167169
}
168170

169171
func (e *TestEngine) ForceLongCheckpoint() {
170-
ctx, cancel := context.WithTimeout(context.Background(), time.Second*20)
172+
ctx, cancel := context.WithTimeout(context.Background(), TestCheckpointTimeout)
171173
defer cancel()
172174
err := e.DB.ForceCheckpoint(ctx, e.TxnMgr.Now())
173175
assert.NoError(e.T, err)

0 commit comments

Comments
 (0)