Skip to content

[fix][broker] Prevent NPE when the last ACK races with sticky hash reassignment - #26471

Open
void-ptr974 wants to merge 2 commits into
apache:masterfrom
void-ptr974:codex/fix-draining-hash-removal
Open

[fix][broker] Prevent NPE when the last ACK races with sticky hash reassignment#26471
void-ptr974 wants to merge 2 commits into
apache:masterfrom
void-ptr974:codex/fix-draining-hash-removal

Conversation

@void-ptr974

Copy link
Copy Markdown
Contributor

Motivation

DrainingHashesTracker.reduceRefCount previously looked up an entry, decremented its reference count,
and removed it from the map as separate operations. If the sticky hash was reassigned to its original
consumer in between, shouldBlockStickyKeyHash could remove the entry first. The final ACK then received
null from drainingHashes.remove and dereferenced it while checking isBlocking, causing an NPE.

The same key-only removal could also let a stale ACK remove a replacement entry or repeat stats cleanup.

Modifications

  • Keep entry publication and reference increments atomic with map removal.
  • Use a CAS fast path for non-final ACKs, while handling the final decrement and removal under the tracker
    write lock.
  • Recheck entry identity before the final decrement so a stale ACK cannot affect a replacement generation.
  • Make owner-reassignment removal conditional and let only the successful remover clear stats.
  • Add deterministic concurrency tests for both race orderings, replacement generations, duplicate final
    ACKs, and concurrent reference reductions.

Verifying this change

  • Added deterministic concurrency coverage in DrainingHashesTrackerConcurrencyTest.
  • Ran DrainingHashesTrackerTest, DrainingHashesTrackerConcurrencyTest, and PendingAcksMapTest
    (48 test invocations, no failures).
  • Ran ./gradlew --offline quickCheck.

An exploratory tracker-level JMH comparison against the exact base revision found no material regression
from the CAS version. The one-thread tracked-ACK result changed by -0.9%; the four-thread mixed result by
-2.7%; sampled mixed p99 remained approximately 3.3-3.6 microseconds. These figures exclude the rest of
broker ACK processing.

Does this pull request potentially affect one of the following parts:

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

The change narrows write-lock use to entry lifecycle transitions and uses CAS for non-final reference
decrements.

Add deterministic coverage for entry publication, concurrent reference additions, and final ACK slow-path rechecks. Exercise nested batching and closing during concurrent removals using an injectable test lock.

Validation: 57 tracker and pending-ack test invocations, 4 broker regression test invocations, and offline quickCheck passed. Targeted implementation mutations fail the three new tests as expected.

Assisted-by: Codex
@merlimat
merlimat requested a review from lhotari September 7, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant