test: stabilize flaky TestTableTimerStore - #69921
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/M
📝 WalkthroughWalkthroughThe timer store watch integration helper now supports optional readiness preparation. The table-backed test waits for an etcd watch-ready event before generating timer changes, while the memory-backed test passes ChangesTimer watch test synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/timer/store_intergartion_test.go`:
- Around line 109-145: Update the readiness retry loop in runTimerStoreWatchTest
to use an exponential backoff for attemptTimer instead of a fixed 100ms
interval. Increase the retry interval after each timeout, while preserving the
latest readyID matching and the overall readyDeadline timeout, so slow event
delivery can be observed without repeatedly enqueueing readiness events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: decb4025-3b96-4cc0-937b-d20bcd0330a3
📒 Files selected for processing (1)
pkg/timer/store_intergartion_test.go
| // Watch starts asynchronously in the etcd notifier, so synchronize before producing table events. | ||
| runTimerStoreWatchTest(t, timerStore, func(ch api.WatchTimerChan) { | ||
| readyDeadline := time.NewTimer(time.Minute) | ||
| defer readyDeadline.Stop() | ||
| retryReady: | ||
| for { | ||
| readyID := "watch-ready-" + uuid.NewString() | ||
| readyKey := fmt.Sprintf("/tidb/timer/cluster/%d/notify/%s", 1, readyID) | ||
| putCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| _, err := cli.Put(putCtx, readyKey, fmt.Sprintf( | ||
| `{"events":[{"tp":"create","timer_id":%q,"timestamp":%d}]}`, | ||
| readyID, | ||
| time.Now().Unix(), | ||
| )) | ||
| cancel() | ||
| require.NoError(t, err) | ||
|
|
||
| attemptTimer := time.NewTimer(100 * time.Millisecond) | ||
| for { | ||
| select { | ||
| case resp, ok := <-ch: | ||
| require.True(t, ok) | ||
| for _, event := range resp.Events { | ||
| if event.Tp == api.WatchTimerEventCreate && event.TimerID == readyID { | ||
| attemptTimer.Stop() | ||
| return | ||
| } | ||
| } | ||
| case <-attemptTimer.C: | ||
| continue retryReady | ||
| case <-readyDeadline.C: | ||
| attemptTimer.Stop() | ||
| require.FailNow(t, "watch ready timeout") | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent tail-chasing race condition on slow CI environments.
The fixed 100ms attemptTimer can cause this test to flake and eventually timeout if the etcd watch event delivery latency consistently exceeds 100ms.
Because event.TimerID is strictly matched against the latest readyID, receiving an older event (e.g., delayed by >100ms) causes it to be ignored. The inner loop then hits the 100ms timeout before the newly expected event can arrive, triggering a new Put and resetting the 100ms timer. This creates a continuous tail-chasing scenario where the timer always expires before the correctly matched event arrives.
Use an exponential backoff for the retry interval. This ensures the timeout eventually exceeds the delivery latency, allowing the matching event to be received without leaving stray events in the channel for subsequent assertions.
🐛 Proposed fix to add exponential backoff
- retryReady:
+ retryInterval := 100 * time.Millisecond
+retryReady:
for {
readyID := "watch-ready-" + uuid.NewString()
readyKey := fmt.Sprintf("/tidb/timer/cluster/%d/notify/%s", 1, readyID)
putCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
_, err := cli.Put(putCtx, readyKey, fmt.Sprintf(
`{"events":[{"tp":"create","timer_id":%q,"timestamp":%d}]}`,
readyID,
time.Now().Unix(),
))
cancel()
require.NoError(t, err)
- attemptTimer := time.NewTimer(100 * time.Millisecond)
+ attemptTimer := time.NewTimer(retryInterval)
for {
select {
case resp, ok := <-ch:
require.True(t, ok)
for _, event := range resp.Events {
if event.Tp == api.WatchTimerEventCreate && event.TimerID == readyID {
attemptTimer.Stop()
return
}
}
case <-attemptTimer.C:
+ if retryInterval < 2*time.Second {
+ retryInterval *= 2
+ }
continue retryReady
case <-readyDeadline.C:
attemptTimer.Stop()
require.FailNow(t, "watch ready timeout")
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Watch starts asynchronously in the etcd notifier, so synchronize before producing table events. | |
| runTimerStoreWatchTest(t, timerStore, func(ch api.WatchTimerChan) { | |
| readyDeadline := time.NewTimer(time.Minute) | |
| defer readyDeadline.Stop() | |
| retryReady: | |
| for { | |
| readyID := "watch-ready-" + uuid.NewString() | |
| readyKey := fmt.Sprintf("/tidb/timer/cluster/%d/notify/%s", 1, readyID) | |
| putCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| _, err := cli.Put(putCtx, readyKey, fmt.Sprintf( | |
| `{"events":[{"tp":"create","timer_id":%q,"timestamp":%d}]}`, | |
| readyID, | |
| time.Now().Unix(), | |
| )) | |
| cancel() | |
| require.NoError(t, err) | |
| attemptTimer := time.NewTimer(100 * time.Millisecond) | |
| for { | |
| select { | |
| case resp, ok := <-ch: | |
| require.True(t, ok) | |
| for _, event := range resp.Events { | |
| if event.Tp == api.WatchTimerEventCreate && event.TimerID == readyID { | |
| attemptTimer.Stop() | |
| return | |
| } | |
| } | |
| case <-attemptTimer.C: | |
| continue retryReady | |
| case <-readyDeadline.C: | |
| attemptTimer.Stop() | |
| require.FailNow(t, "watch ready timeout") | |
| } | |
| } | |
| } | |
| }) | |
| // Watch starts asynchronously in the etcd notifier, so synchronize before producing table events. | |
| runTimerStoreWatchTest(t, timerStore, func(ch api.WatchTimerChan) { | |
| readyDeadline := time.NewTimer(time.Minute) | |
| defer readyDeadline.Stop() | |
| retryInterval := 100 * time.Millisecond | |
| retryReady: | |
| for { | |
| readyID := "watch-ready-" + uuid.NewString() | |
| readyKey := fmt.Sprintf("/tidb/timer/cluster/%d/notify/%s", 1, readyID) | |
| putCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| _, err := cli.Put(putCtx, readyKey, fmt.Sprintf( | |
| `{"events":[{"tp":"create","timer_id":%q,"timestamp":%d}]}`, | |
| readyID, | |
| time.Now().Unix(), | |
| )) | |
| cancel() | |
| require.NoError(t, err) | |
| attemptTimer := time.NewTimer(retryInterval) | |
| for { | |
| select { | |
| case resp, ok := <-ch: | |
| require.True(t, ok) | |
| for _, event := range resp.Events { | |
| if event.Tp == api.WatchTimerEventCreate && event.TimerID == readyID { | |
| attemptTimer.Stop() | |
| return | |
| } | |
| } | |
| case <-attemptTimer.C: | |
| if retryInterval < 2*time.Second { | |
| retryInterval *= 2 | |
| } | |
| continue retryReady | |
| case <-readyDeadline.C: | |
| attemptTimer.Stop() | |
| require.FailNow(t, "watch ready timeout") | |
| } | |
| } | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/timer/store_intergartion_test.go` around lines 109 - 145, Update the
readiness retry loop in runTimerStoreWatchTest to use an exponential backoff for
attemptTimer instead of a fixed 100ms interval. Increase the retry interval
after each timeout, while preserving the latest readyID matching and the overall
readyDeadline timeout, so slow event delivery can be observed without repeatedly
enqueueing readiness events.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #69921 +/- ##
================================================
- Coverage 76.3193% 73.9502% -2.3691%
================================================
Files 2041 2058 +17
Lines 559929 578636 +18707
================================================
+ Hits 427334 427903 +569
- Misses 131694 150401 +18707
+ Partials 901 332 -569
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
2 participants
What problem does this PR solve?
Issue Number: close #69920
Problem Summary:
Flaky test
TestTableTimerStoreinpkg/timerintermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
The table-store watch test had a setup race between Watch returning and the etcd watch subscription becoming active.
Fix
The readiness event proves the same watch channel is subscribed before the original real timer create/update/delete assertions run.
Verification
Spec:
pkg/timer :: TestTableTimerStoretidb.go_flaky.defaultBASELINE_ONLYGO_TEST_WITH_TAGSintest, deadlockbaseline_onlyObserved result:
Required flaky case was not skipped.
target_flaky passed.
package_surface passed.
build passed.
lint passed.
Gate checklist:
Commands:
go test -json -tags=intest,deadlock ./pkg/timer -run '^TestTableTimerStore$' -count=1go test -json -tags=intest,deadlock ./pkg/timer -count=1make buildmake lintCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Fixes #69920
Summary by CodeRabbit