Skip to content

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

Merged
lhotari merged 2 commits into
apache:masterfrom
void-ptr974:codex/fix-draining-hash-removal
Sep 10, 2026
Merged

lhotari merged 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

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reference-count changes and entry-identity check handle the tracker lifecycle races consistently. No issues found.

Validation: DrainingHashesTrackerTest, DrainingHashesTrackerConcurrencyTest, and PendingAcksMapTest passed with retries disabled (57 tests, no failures).

@lhotari lhotari added this to the 5.0.0-M2 milestone Sep 10, 2026
@lhotari
lhotari merged commit 64d1045 into apache:master Sep 10, 2026
43 checks passed
lhotari pushed a commit that referenced this pull request Sep 10, 2026
lhotari pushed a commit that referenced this pull request Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants