[I] [Bug] LMQ empty-bucket cleanup can drop concurrently suspended pull requests [rocketmq]

["ai-yang (via GitHub)" <gi...@apache.org>]

ai-yang opened a new issue, #10754:
URL: https://github.com/apache/rocketmq/issues/10754

   ## Affected branch
   
   `develop` at `a06836dd564e5e43115493f775626cf98d51d10e`.
   
   ## Problem
   
   `LmqPullRequestHoldService.checkHoldRequest()` removes an LMQ `ManyPullRequest` bucket after observing it as empty. The empty check and `pullRequestTable.remove(key)` are not atomic with `PullRequestHoldService.suspendPullRequest()` adding a request to the same bucket.
   
   The following interleaving can occur:
   
   1. the periodic check observes the current bucket as empty;
   2. a concurrent request obtains that bucket from the map and appends a new suspended pull request;
   3. the periodic check unconditionally removes the bucket based on its earlier empty observation.
   
   The new request remains in the detached `ManyPullRequest` object, but the hold table no longer contains that object.
   
   ## Deterministic reproduction
   
   A unit test pre-populates an empty bucket and uses a `ConcurrentHashMap` test double whose `remove(key)` is paused by two latches. This pauses the periodic check only after its empty condition has already selected the removal path. The test then calls the real `suspendPullRequest()` method, verifies that the existing bucket now contains the request, lets removal continue, and requires the same bucket to remain reachable from the table.
   
   The unmodified branch failed identically in 5/5 isolated JDK 8 Maven processes:
   
   ```text
   Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
   java.lang.AssertionError: concurrently suspended request should remain reachable expected same:<...> was not:<null>
   ```
   
   The reproduction has no sleep, network, timer, or random scheduling dependency. All latch and future waits are bounded only to turn a broken test into a prompt failure instead of a hang.
   
   ## Impact
   
   A concurrently suspended LMQ pull request can disappear from `pullRequestTable`. Later message-arrival and timeout scans cannot discover or wake it, so the broker-side long-poll request remains stranded until external connection or client timeout handling intervenes.
   
   ## Expected behavior
   
   An LMQ bucket must not be removed once a concurrent suspend or replay has made it non-empty. Every request that remains suspended must be reachable through `pullRequestTable` for later message-arrival and timeout processing.
   
   ## Suggested direction
   
   Make bucket insertion/replay and empty cleanup atomic per map key:
   
   - add newly suspended requests through `ConcurrentMap.compute`, creating or reusing the mapped bucket and appending inside the remapping function;
   - remove an LMQ bucket through `computeIfPresent`, using synchronized `ManyPullRequest.isEmpty()` as the authoritative check;
   - reinsert `notifyMessageArriving()` replay requests through the same map-aware helper, rather than appending them to a bucket that may have been detached while requests were evaluated.
   
   Changing only the final remove to a conditional remove is insufficient: a concurrent writer or replay can still retain and append to an object that cleanup has just detached from the map.
   
   ## Related work checked
   
   Searches covered open and closed issues and pull requests using `LmqPullRequestHoldService`, `ManyPullRequest`, `pullRequestTable`, `suspendPullRequest`, empty-bucket cleanup, and concurrent removal terms. No equivalent report, implementation, assignee, or maintainer handoff was found.
   
   #8341 and its unmerged PR #8342 only proposed replacing the redundant `getPullRequestList() == null` check with `isEmpty()`; they did not make the check/removal atomic or address concurrent suspend/replay.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Bug] LMQ empty-bucket cleanup can drop concurrently suspended pull requests [rocketmq]

["RockteMQ-AI (via GitHub)" <gi...@apache.org>]

RockteMQ-AI commented on issue #10754:
URL: https://github.com/apache/rocketmq/issues/10754#issuecomment-5158444694

   **Issue Evaluation**
   
   Category: `type/bug` | Status: **Confirmed**
   
   The reported issue has been verified against the current codebase at `a06836d`.
   
   **Root Cause:** `LmqPullRequestHoldService.checkHoldRequest()` has a TOCTOU race between the `isEmpty()` check and `pullRequestTable.remove(key)`. After `notifyMessageArriving` drains the bucket via `cloneListAndClear()`, a concurrent `suspendPullRequest` can insert a new request into the same bucket. The subsequent `remove(key)` then silently drops the newly inserted request. Additionally, `getPullRequestList().isEmpty()` is called without synchronization on the raw `ArrayList`, creating a data race.
   
   **Impact:** Pull requests can be silently lost during concurrent LMQ consumption, leading to message delivery delays or stalls for affected queues.
   
   **Severity:** High — silent request loss directly affects message delivery reliability.
   
   **Suggested fix direction:** Use `pullRequestTable.remove(key, mpr)` (conditional remove) to ensure only the same `mpr` instance is removed, or synchronize the isEmpty+remove sequence.
   
   An automated fix proposal can be generated. Reply `/approve` to proceed with PR generation.
   
   ---
   *Automated evaluation by RockteMQ-AI*


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org