Skip to content

[fix][test] Fix flaky TopicListSizeResultCacheTest concurrent requests test#25357

Merged
lhotari merged 2 commits intoapache:masterfrom
merlimat:fix/flaky-TopicListSizeResultCacheTest
Mar 19, 2026
Merged

[fix][test] Fix flaky TopicListSizeResultCacheTest concurrent requests test#25357
lhotari merged 2 commits intoapache:masterfrom
merlimat:fix/flaky-TopicListSizeResultCacheTest

Conversation

@merlimat
Copy link
Copy Markdown
Contributor

Flaky test failure

java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.assertTrue(Assert.java:56)
	at org.apache.pulsar.broker.topiclistlimit.TopicListSizeResultCacheTest.testResultHolder_concurrentRequestsWaitForFirstToComplete(TopicListSizeResultCacheTest.java:132)

Summary

Fix flaky TopicListSizeResultCacheTest.testResultHolder_concurrentRequestsWaitForFirstToComplete which had two bugs:

  1. Unsynchronized ArrayList: futures list was populated from multiple threads via futures.add() without synchronization, causing potential data corruption and lost adds.

  2. Wrong ordering assumption: The test assumed futures.get(0) was the "first" request (the one that wins the CAS in getSizeAsync() and gets back a completed future). But thread scheduling is nondeterministic — any thread could add its future to the list first regardless of which called getSizeAsync() first.

Fix

  • Use CopyOnWriteArrayList for thread-safe concurrent adds
  • Instead of assuming futures.get(0) is the CAS winner, count how many futures are done (should be exactly 1) and find it via stream().filter(isDone)
  • Verify all futures complete after updateSize() without assuming index-based ordering

Verified with invocationCount = 100 — 100/100 pass.

Documentation

  • doc-not-needed
    (Your PR doesn't need any doc update)

Matching PR in forked repository

No response

Tip

Add the labels ready-to-test and area/test to trigger the CI.

…s test

The test had two bugs:

1. futures ArrayList was populated from multiple threads without
   synchronization, causing potential data corruption and lost adds.

2. The test assumed futures.get(0) was the "first" request (the one
   that wins the CAS in getSizeAsync()), but thread scheduling order
   is nondeterministic - any thread could add its future to the list
   first regardless of which called getSizeAsync() first.

Fix by using CopyOnWriteArrayList for thread-safe adds, and checking
that exactly one future isDone (the CAS winner) without assuming
which list index it occupies.
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 18, 2026
@merlimat merlimat requested a review from lhotari March 18, 2026 22:06
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.72%. Comparing base (7f6bc23) to head (be63377).
⚠️ Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
...r/topiclistlimit/TopicListSizeResultCacheTest.java 0.00% 14 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #25357       +/-   ##
=============================================
+ Coverage     37.45%   72.72%   +35.26%     
- Complexity    13161    34253    +21092     
=============================================
  Files          1897     1954       +57     
  Lines        150557   154770     +4213     
  Branches      17215    17718      +503     
=============================================
+ Hits          56398   112550    +56152     
+ Misses        86428    33182    -53246     
- Partials       7731     9038     +1307     
Flag Coverage Δ
inttests 25.77% <0.00%> (-0.27%) ⬇️
systests 22.50% <0.00%> (+<0.01%) ⬆️
unittests 73.71% <ø> (+39.53%) ⬆️

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

Files with missing lines Coverage Δ
...r/topiclistlimit/TopicListSizeResultCacheTest.java 0.00% <0.00%> (ø)

... and 1419 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit 5d03ef3 into apache:master Mar 19, 2026
53 checks passed
@lhotari lhotari added this to the 4.2.0 milestone Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/test doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants