Skip to content

Fix flaky TestReadersWritersStress: nil guards and longer WaitGroup timeouts#2073

Open
asmyasnikov with Copilot wants to merge 4 commits into
masterfrom
copilot/fix-flaky-test-testreaderswritersstress
Open

Fix flaky TestReadersWritersStress: nil guards and longer WaitGroup timeouts#2073
asmyasnikov with Copilot wants to merge 4 commits into
masterfrom
copilot/fix-flaky-test-testreaderswritersstress

Conversation

Copilot AI commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

TestReadersWritersStress fails intermittently due to two bugs in stressTestInATopic.

Nil pointer dereference on StartWriter/StartReader failure

writeToTopic and readFromTopic declared writer/reader as nil before the defer, then called Close unconditionally. Any transient error from StartWriter/StartReader would trigger a nil dereference panic in the deferred cleanup.

// Before: panics if StartWriter fails
defer func() {
    closeErr := writer.Close(context.Background()) // nil deref
    wg.Done()
}()

// After: guarded
defer func() {
    if writer != nil {
        closeErr := writer.Close(context.Background())
    }
    wg.Done()
}()

WaitGroup timeout too short for stress test cleanup

xtest.WaitGroup uses commonWaitTimeout = 1s. After stopping writers/readers, cleanup involves closing gRPC streams and flushing buffers—operations that routinely exceed 1s under CI load. Replaced both xtest.WaitGroup calls with xtest.WaitGroupWithTimeout(..., time.Minute), consistent with the time.Minute already used for SpinWaitProgressWithTimeout in the same function.

Copilot AI linked an issue Apr 1, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Fix flaky test TestReadersWritersStress Fix flaky TestReadersWritersStress: nil guards and longer WaitGroup timeouts Apr 1, 2026
Copilot AI requested a review from asmyasnikov April 1, 2026 10:33
@asmyasnikov
asmyasnikov marked this pull request as ready for review May 3, 2026 12:48
@asmyasnikov
asmyasnikov requested a review from Copilot May 3, 2026 12:48
@robot-vibe-db

robot-vibe-db Bot commented May 3, 2026

Copy link
Copy Markdown

AI Review Summary

Verdict: ✅ No critical issues found

Critical issues

No critical issues found.

Other findings

None.


This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes intermittent TestReadersWritersStress failures by preventing nil dereference panics in deferred cleanup and by increasing stress-test shutdown wait time to better match CI conditions.

Changes:

  • Add nil guards before calling Close() in writer/reader deferred cleanup to avoid panics when StartWriter/StartReader fails.
  • Replace short xtest.WaitGroup waits with xtest.WaitGroupWithTimeout(..., time.Minute) for more reliable stress-test teardown.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

summary

Inferred base version: v3.135.10
Suggested version: v3.135.11

@github-actions github-actions Bot removed the SLO label May 3, 2026
@codecov-commenter

codecov-commenter commented May 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.97%. Comparing base (9955e4c) to head (e6e59f0).
⚠️ Report is 91 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2073      +/-   ##
==========================================
- Coverage   75.16%   74.97%   -0.20%     
==========================================
  Files         427      427              
  Lines       36974    36974              
==========================================
- Hits        27792    27721      -71     
- Misses       7993     8057      +64     
- Partials     1189     1196       +7     
Flag Coverage Δ
experiment 74.76% <ø> (-0.19%) ⬇️
go-1.21.x 72.44% <ø> (-0.23%) ⬇️
go-1.26.x 74.91% <ø> (-0.21%) ⬇️
integration 57.03% <ø> (-0.34%) ⬇️
macOS 46.21% <ø> (-0.01%) ⬇️
ubuntu 74.97% <ø> (-0.20%) ⬇️
unit 46.21% <ø> (-0.03%) ⬇️
windows 46.19% <ø> (-0.03%) ⬇️
ydb-24.4 56.46% <ø> (-0.49%) ⬇️
ydb-edge 56.78% <ø> (-0.39%) ⬇️
ydb-latest 56.73% <ø> (-0.34%) ⬇️
ydb-nightly 74.76% <ø> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: flaky test TestReadersWritersStress

4 participants