[ISSUE #11441] fix(broker): wake the held pull requests when an arrival drained one bucket

[iloveeyjafjalla]

What this changes

PullRequestHoldService#notifyMasterOnline() checked a bucket for emptiness and then iterated the result of the drain:

for (PullRequest request : mpr.cloneListAndClear()) {

ManyPullRequest#cloneListAndClear() answers null, not an empty list, when the bucket is empty at drain time. The buckets are drained by the message-arrival path on the store's dispatch thread, so an arrival that wins the race between the isEmpty() check and the drain makes that call answer null and the enhanced for throw a NullPointerException.

The exception escaped notifyMasterOnline() into BrokerController#onMinBrokerChange (including the UPDATE_MIN_BROKER admin path), abandoning the sweep half way: every bucket not visited yet kept its suspended pull requests until the client's own suspend timeout - the exact stall this wake-up exists to prevent, in the failover window it is meant for.

Fix: drain into a local variable and skip the bucket that lost the race. This mirrors the guard notifyMessageArriving already has around the same call in the same class, so the null contract is now handled consistently.

Evidence

Red run before the fix (broker, JDK 8), forced interleaving where the bucket is drained by a concurrent arrival right after the emptiness check:

java.lang.AssertionError:
Expecting code not to raise a throwable but caught
  "java.lang.NullPointerException
	at org.apache.rocketmq.broker.longpolling.PullRequestHoldService.notifyMasterOnline(PullRequestHoldService.java:191)
	at org.apache.rocketmq.broker.longpolling.PullRequestHoldServiceTest.notifyMasterOnlineTestWakesTheBucketsLeftAfterAConcurrentArrivalDrainedOneTest(PullRequestHoldServiceTest.java:145)

Green run after the fix, and the CI-shaped verification (checkstyle + spotbugs) on the module:

Tests run: 29, Failures: 0, Errors: 0, Skipped: 0   (PullRequestHoldServiceTest, PopLongPollingServiceTest, PopLiteLongPollingServiceTest)
[INFO] You have 0 Checkstyle violations.
[INFO] BugInstance size is 0
[INFO] BUILD SUCCESS

The new test notifyMasterOnlineTestWakesTheBucketsLeftAfterAConcurrentArrivalDrainedOneTest asserts both halves of the contract: the sweep does not throw, and the held requests of the other queues are still woken (their bucket is drained by the sweep) instead of being skipped by an aborted sweep. The broker module's full test suite is not run here - it does not complete on this machine for unrelated reasons - so only the affected package's test classes were executed, as reported above.

Fixes #11441

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes a NullPointerException in PullRequestHoldService.notifyMasterOnline() caused by a race condition where a concurrent message arrival drains a bucket between the isEmpty() check and cloneListAndClear(), making the latter return null instead of an empty list. The null result then crashes the enhanced for-each loop, aborting the sweep and leaving remaining buckets' pull requests suspended until client timeout.

Findings

• [Info] PullRequestHoldService.java:191 — The fix correctly drains into a local variable and adds a null guard with continue, mirroring the existing pattern in notifyMessageArriving(). This is the minimal correct fix for the race.
• [Info] PullRequestHoldServiceTest.java — The RacyBucket anonymous class is a clever deterministic reproduction of the race condition: overriding isEmpty() to self-drain simulates the exact interleaving. The test asserts both halves of the contract (no NPE + other buckets still processed).

Assessment

• Correctness: ✅ The null guard handles the race properly. The continue ensures only the drained bucket is skipped, not the entire sweep.
• Performance: ✅ No impact — just a null check on a local variable.
• Tests: ✅ Good coverage. The test deterministically reproduces the race and verifies both the fix and the sweep continuation.
• Compatibility: ✅ No API changes, purely internal fix.

LGTM. Clean fix with solid test coverage.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes NPE in when returns due to a concurrent arrival draining the bucket first.

Findings

• [Info] PullRequestHoldService.java:191 — Clean fix that mirrors the existing null guard in . The race condition is well-documented in the comment.
• [Info] PullRequestHoldServiceTest.java — The test subclass is a clever way to deterministically reproduce the race. Good coverage of both halves of the contract (no NPE + other buckets still woken).

Verdict

LGTM. Clean, minimal fix with a solid regression test.

---

Automated review by github-manager-bot