[alex268]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 98.52941% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.67%. Comparing base (8a1fef6) to head (91d6332).

Files with missing lines	Patch %	Lines
...java/tech/ydb/topic/impl/TopicRetryableStream.java	97.36%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #771      +/-   ##
============================================
+ Coverage     75.63%   75.67%   +0.04%     
- Complexity     3780     3785       +5     
============================================
  Files           396      396              
  Lines         16923    16936      +13     
  Branches       1790     1789       -1     
============================================
+ Hits          12799    12817      +18     
+ Misses         3503     3501       -2     
+ Partials        621      618       -3     

☔ 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.

[robot-vibe-db]

Review comments on graceful shutdown changes.

[robot-vibe-db]

AI Review Summary

Verdict: ✅ No critical issues found

Critical issues

No critical issues found.

Other findings

• Major | Medium: graceful close now only half-closes the gRPC call and waits for the server to close the stream with no timeout — SyncReaderImpl.shutdown() can block indefinitely on shutdownFuture.join(), and reader metrics gauges are unregistered only when onClose finally fires — topic/src/main/java/tech/ydb/topic/impl/TopicStreamBase.java:71
• Minor | Medium: after close(), state stays non-null in the stream != null branch, so a concurrent fail() reports the application-fail status via onClose and invokes the user's error handler for a user-initiated close (old code suppressed this by nulling the state atomically) — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:120
• Minor | Medium: race in State.close() — if the creation future completes between callback registration and the CAS, the new session is started and then half-closed, close() returns false and skips its SUCCESS onStreamStop, so onClose fires later with the transport status — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:215
• Minor | Medium: session teardown (preDestroy: decoder stop, partitions closed) now runs before the user's errorHandler in onRetry/onClose, reversing the old order (error handler first, then closeAll) — handlers now see an already-torn-down session — topic/src/main/java/tech/ydb/topic/read/impl/ReaderImpl.java:75
• Minor | Medium: writer graceful-close window — writeQueue is closed only when the server closes the stream; until then SendTask keeps running and queued messages are silently dropped with a warn per message — topic/src/main/java/tech/ydb/topic/impl/TopicStreamBase.java:96
• Minor | Low: ReadSession.preDestroy() is now called repeatedly and concurrently; a late StartPartitionSessionRequest between forEach(close) and clear() adds a partition that is dropped without being closed (onStartPartition has no isClosed guard) — topic/src/main/java/tech/ydb/topic/read/impl/ReadSession.java:109
• Minor | Low: test gap — ReadStreamMock(autoClose) always completes with SUCCESS; no coverage of graceful close ending with a non-SUCCESS server status, nor the close()/fail() interleaves — topic/src/test/java/tech/ydb/topic/read/impl/ReadStreamMock.java:66
• Nit | High: typo "nothign" in preDestroy() comment — topic/src/main/java/tech/ydb/topic/impl/TopicStreamBase.java:75
• Nit | Medium: unconditional initFuture.completeExceptionally("Reader closed by user") in shutdown() makes the descriptive "Reader closed with …" message in close(Status) dead in the graceful path; two different messages for the same event depending on timing — topic/src/main/java/tech/ydb/topic/read/impl/AsyncReaderImpl.java:94
• Nit | Low: send() now merges "before stream is ready" and "after closing" into a single warn message, which makes debugging misuses harder — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:105

---

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, z-ai/glm-5.3-flash.

[pnv1]

[ИИ-ревью] Очистка сессии может заблокировать завершение потока

preDestroy() закрывает партиции и синхронно завершает ожидающие commit-future, пока состояние потока ещё активно. Если обработчик ошибки commit вызывает reader.shutdown().join(), возникает цикл: shutdown ждёт streamStatus, а streamStatus.complete(status) ждёт возвращения этого обработчика. Завершение зависает даже при уже закрытом транспорте.

Этот же порядок позволяет исключению из onPartitionSessionClosed перехватить завершение: при executor Runnable::run серверный UNAUTHORIZED заменяется на CLIENT_INTERNAL_ERROR, и вместо остановки запускается retry. Остановку и итоговый статус следует фиксировать до вызова пользовательских callback очистки.

[alex268]

Да, текущая реализация в целом чувствительна к блокировкам в обработке commit. Это не предмет текущего PR и будет решаться отдельно