Fixed deadlock between MessageCommitter/SessionBase

[alex268]

No description provided.

[robot-vibe-db]

@alex268 Please set up your personal token, see instructions: https://nda.ya.ru/t/dur-L1y77biDC2

[codecov-commenter]

Codecov Report

❌ Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.59%. Comparing base (a402f6f) to head (e7f2a87).

Files with missing lines	Patch %	Lines
...tech/ydb/topic/read/impl/MessageCommitterImpl.java	22.22%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #666      +/-   ##
============================================
- Coverage     71.61%   71.59%   -0.03%     
  Complexity     3426     3426              
============================================
  Files           386      386              
  Lines         16066    16071       +5     
  Branches       1687     1688       +1     
============================================
  Hits          11506    11506              
- Misses         3904     3909       +5     
  Partials        656      656              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR aims to eliminate a deadlock between MessageCommitterImpl and the underlying session (via SessionBase) by changing the ordering of the commit request vs. internal commit-future bookkeeping.

Changes:

• Moves commitOffsets(...) invocation outside of commitFuturesLock in MessageCommitterImpl.commit(...).
• Simplifies the commit path by removing the conditional insertion into commitFutures under the lock.

---

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

[robot-vibe-db]

AI Review Summary

Verdict: ✅ No critical issues found

Critical issues

No critical issues found.

Other findings

• Minor | Medium: Unconditional commitFutures.remove(range.getEnd()) in the failure path may remove a future belonging to a different concurrent commit() call that reused the same key — MessageCommitterImpl.java:87
• Minor | Low: future.completeExceptionally() outside the lock (line 83) races with confirmCommit completing the same future via future.complete(null) — both are safe individually (CompletableFuture is thread-safe), but the caller could observe success or failure non-deterministically if commitOffsets returns false while a confirmation for the same offset is in-flight — MessageCommitterImpl.java:83
• Nit | Medium: No unit test covering the deadlock or the data-race scenario — the fix is correct but regressions would be hard to catch — MessageCommitterImpl.java:64

---

This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.

[robot-vibe-db]

Full analysis log

Analysis performed by claude, claude-opus-4-6.

[Copilot]

Pull request overview

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