apache / apache/rocketmq

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

Open
#10,754 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
22.6k
Forks
12k
Avg merge
3d 1h
Merged PRs (30d)
27

Description

## 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:
```

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.

Contributor guide

Open the contributing guide

Research direction

Start by reading LmqPullRequestHoldService.checkHoldRequest(), PullRequestHoldService.suspendPullRequest(), and ManyPullRequest, then run the deterministic unit test described in the issue. The fix is done when suspend and replay operations remain reachable through pullRequestTable during empty-bucket cleanup, and the reproduction passes reliably on the affected branch.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.