[improve][broker] Fix thread safety issue in ManagedCursorImpl.removeProperty

[3pacccccc]

Fixes #25103

Motivation

The ManagedCursorImpl.removeProperty method contains a thread safety vulnerability where it directly modifies the shared properties Map without proper synchronization. This can lead to:

1. ConcurrentModificationException when putProperty creates a new HashMap copy while removeProperty modifies the original Map
2. Data corruption in the internal HashMap structure under concurrent access
3. Inconsistent state where one thread's modifications may not be visible to others

Modifications

1. Fixed removeProperty method in ManagedCursorImpl.java:

   • Changed from direct Map modification (properties.remove(key)) to creating a new Map copy
   • Updated implementation to match the thread-safe pattern already used in putProperty
   • Maintains atomicity through LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet()

2. Added concurrency test in ManagedCursorTest.java:

   • 100-thread stress test with mixed put/remove/get operations
   • 5-second duration to simulate sustained concurrent access
   • Verifies no ConcurrentModificationException or data inconsistencies
   • Validates final state matches expected results

Verifying this change

• Make sure that the change passes the CI checks.

If the box was checked, please highlight the changes

• 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

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: 3pacccccc#37

[lhotari]

Please check the comment about the test

[Copilot]

Pull request overview

This PR fixes a critical thread safety issue in the ManagedCursorImpl.removeProperty method where direct modification of a shared Map could lead to ConcurrentModificationException and data corruption. The fix aligns the implementation with the existing thread-safe pattern used in putProperty by creating a new Map copy instead of modifying the original.

Key Changes:

• Modified removeProperty to create a new HashMap copy and return a new MarkDeleteEntry instance, ensuring thread-safe updates through the atomic field updater
• Added a concurrent stress test with 100 threads performing mixed put/remove/get operations to validate the fix

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java	Fixed removeProperty method to use copy-on-write pattern instead of direct Map modification
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java	Added concurrency test testConcurrentPropertyOperationsThreadSafety with new imports for ConcurrentModificationException and Random

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lhotari]

LGTM

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.44%. Comparing base (df89b60) to head (4e38e88).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #25104      +/-   ##
============================================
+ Coverage     74.42%   74.44%   +0.01%     
+ Complexity    34053    33655     -398     
============================================
  Files          1899     1899              
  Lines        149659   149663       +4     
  Branches      17394    17394              
============================================
+ Hits         111389   111410      +21     
+ Misses        29396    29381      -15     
+ Partials       8874     8872       -2     

Flag	Coverage Δ
inttests	26.36% <0.00%> (?)
systests	23.03% <0.00%> (-0.01%)	⬇️
unittests	73.98% <100.00%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	79.11% <100.00%> (-0.16%)	⬇️

... and 99 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.