Codecov Report

❌ Patch coverage is 92.07921% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.53%. Comparing base (f3fcaec) to head (148c03c).

@@             Coverage Diff              @@
##             master     #726      +/-   ##
============================================
- Coverage     74.54%   74.53%   -0.01%     
+ Complexity     3637     3632       -5     
============================================
  Files           392      391       -1     
  Lines         16493    16523      +30     
  Branches       1738     1745       +7     
============================================
+ Hits          12294    12316      +22     
- Misses         3591     3593       +2     
- Partials        608      614       +6     

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

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

AI Review Summary

Verdict: ✅ No critical issues found

Critical issues

No critical issues found.

Other findings

• Major | Medium: fail()/close() split the teardown into realStreamId.getAndSet(null) plus a separate read of realStream; a concurrently completing createNewStream future in between makes them close the newly-installed stream and abandon the previously active one unmanaged (its termination is never processed). The old code swapped the stream in a single atomic step — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:140
• Major | High: tryStartStream discards an already-created stream on both early-return paths (isClosed while creation is in flight, "double start" CAS failure) without closing it, leaking the eagerly-created gRPC call; reachable e.g. via writer.shutdown() during a direct-write (re)connect — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:78
• Minor | Low: exceptions thrown inside tryStartStream (including user callbacks reached via onRetry/onClose) are swallowed by the unobserved future returned from whenComplete, silently killing retry/stop handling — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:72
• Minor | High: source-incompatible signature change on the public, user-extensible WriteStreamFactory.createNewStream (and @Nullable stream params on TopicRetryableStream.onRetry/onClose); custom subclasses stop compiling — topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamFactory.java:63
• Nit | High: typo "exeption" in the creation-failure log message — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:98
• Nit | High: buildDirectStream parameter partitionId shadows the partitionId field of WriteStreamFactory — topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamDirectFactory.java:50

The core change itself (non-blocking stream creation, Result-based factory contract, @Nullable handling in ReaderImpl) looks sound, the new async/close/creation-failure scenarios are covered by tests, and the full topic module test suite passes.

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.

Full analysis log

Analysis performed by claude, z-ai/glm-5.3-flash.

AI Review Summary

Verdict: ❌ 1 critical issue(s) found

Critical issues

• Major | High: close()/fail() call local.close() before local.getStream(), so onStreamStop() is always passed a null stream — ReaderImpl.onRetry/onClose then skip closeAll(), and users no longer receive partition-session-closed notifications when the reader is closed or failed by the application (regression vs master) — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:116

Other findings

• Major | Medium: State.startStream()/State.start() never verify state.get() == this, so a State that lost ownership (e.g. fail() during a pending creation) can start an orphan duplicate stream whose termination is then silently ignored — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:196
• Minor | High: when the retryable stream is closed while creation is pending, the successfully created stream is dropped without being started or closed, leaking the underlying gRPC call (with disableDeadline() nothing reaps it); the new test codifies never().close() instead of closing it — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:177
• Minor | Medium: no test coverage for a creation future completing with a failed Result (retryable reconnect and non-retryable stop paths of onStreamStop(null, res.getStatus(), ...) are untested), nor for close()/fail() racing pending creation — topic/src/test/java/tech/ydb/topic/impl/TopicRetryableStreamTest.java:250
• Nit | High: send() allocates an Optional (plus lambda capture) on every call on the writer's hot path; a plain null check on state.get() is cheaper and clearer — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:100

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.

Full analysis log

Analysis performed by claude, z-ai/glm-5.3-flash.

Full analysis log

Analysis performed by claude, z-ai/glm-5.3-flash.

AI Review Summary

Verdict: ✅ No critical issues found

Critical issues

No critical issues found.

Other findings

• Major | Medium: lookupPartitionId early-return uses streamFuture.join(), which throws CompletionException if the probe stream future completed exceptionally; the exception escapes createNewStream() before the state CAS, leaving the stream dead with no onClose and no retry (writer/reader silently hangs when this happens on the reconnect scheduler) — topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamDirectFactory.java:145
• Minor | Medium: race in State.startStream() — close()/fail() can null the state between the entry check and the post-start check, so local.start(...) is invoked (gRPC init request sent) after the application was already notified via onClose — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:223
• Minor | High: source-incompatible public API change — WriteStreamFactory.createNewStream() return type changed and public WriteStream.Fail/TopicStreamFail removed; custom factory subclasses will stop compiling; should be noted in CHANGELOG.md — topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamFactory.java:63
• Minor | Medium: test gap — no test for a retryable creation failure scheduling a reconnect and recovering (only the noRetries close case is covered), nor for the non-blocking createNewStream contract — topic/src/test/java/tech/ydb/topic/impl/TopicRetryableStreamTest.java:286
• Minor | Low: probe stream is now half-closed from pidFuture.whenComplete(...) (gRPC callback thread on success); the !streamFuture.isDone() check races with call completion and halfClose() on a finished call can throw — topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamDirectFactory.java:159
• Minor | Low: send() silently drops messages during the creation window, which is now much longer for direct writers (probe + describe, up to ~2 min with deadlines); writers are currently protected by the isReady gate, but the contract is fragile — topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java:105
• Nit | High: typo in test comment "closed byt not started" — topic/src/test/java/tech/ydb/topic/impl/TopicRetryableStreamTest.java:177

The core state-machine rework (State with creation future + stream reference) was traced through the close/fail/retry/async-creation interleavings and reviewed against WriterImpl, ReaderImpl, WriteSession, and both stream factories; the topic module test suite passes on the merge commit.

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.

Full analysis log

Analysis performed by claude, z-ai/glm-5.3-flash.