[BUG] Clearing one topic's forbidden bits wipes the whole group's forbidden table and skips persist

[zjncs]

Motivation

SubscriptionGroupManager.updateForbiddenValue handles a forbidden value of 0 (i.e. clearForbidden released the last forbidden bit of a topic) by removing the whole group from forbiddenTable:

if (forbidden == null || forbidden <= 0) {
    this.forbiddenTable.remove(group);
    log.info("clear group forbidden, {}@{} ", group, topic);
    return;
}

Two problems:

1. Cross-topic state loss. When a group is forbidden on multiple topics and one topic's permission is restored, the other topics' forbidden bits are silently dropped — the admin only intended to un-forbid one topic. (AdminBrokerProcessor.updateForbidden / the dashboard's per-topic perm toggle hit exactly this path.)
2. No persist / no data-version bump. The removal neither calls updateDataVersion() nor persist(), so after a broker restart the stale forbidden state comes back from disk, and peers syncing via GET_ALL_SUBSCRIPTIONGROUP_CONFIG see an unchanged data version and keep the old bits.

Changes

In the forbidden <= 0 branch: remove only the given topic's entry from the group's map (dropping the now-empty group map when the last topic is gone), then updateDataVersion() + persist(), mirroring the set path. A clear of a topic that was never forbidden stays a no-op (no spurious version bump).

Verification

New regression test ForbiddenTest#testClearOneTopicForbiddenKeepsOtherTopicsOfTheSameGroup: forbids t1/t2 of one group, clears t1, and asserts t2 keeps its forbidden bit, the table shrinks to the single remaining entry, the data version counter is bumped, and clearing the last topic removes the group entry.

$ mvn -pl broker test -Dtest='ForbiddenTest'
(before) Tests run: 2, Failures: 1, Errors: 0   (t2's state lost + version not bumped)
(after)  Tests run: 2, Failures: 0, Errors: 0

[Copilot]

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

[RockteMQ-AI]

Summary

Correct fix for a data-loss bug in updateForbiddenValue. The original code removed the entire group entry when clearing any single topic's forbidden bit, silently dropping forbidden state for all other topics in the same group. The fix is precise and minimal.

Findings

• [Info] SubscriptionGroupManager.java:229 — Good use of the two-arg ConcurrentMap.remove(group, topicForbiddens) for atomic conditional removal. This avoids a TOCTOU race where another thread could have already replaced the map.
• [Info] The no-op guard (topicForbiddens.remove(topic) != null) correctly prevents spurious data-version bumps and persists when the topic was never forbidden. Clean.
• [Info] The regression test is thorough — it verifies cross-topic isolation, data-version bump, and full cleanup when the last topic is cleared.

Verdict

LGTM. The fix correctly scopes the removal to a single topic, ensures durability via updateDataVersion() + persist(), and the test covers the exact failure mode described in the issue.

---

Automated review by github-manager-bot