fix ConnectStreams: break loop if resend message fails

[MrGuin]

Close https://github.com/eloqdata/project_tracker/issues/87.
Close https://github.com/eloqdata/project_tracker/issues/93.

Summary by CodeRabbit

• Bug Fixes
  • Added reconnection detection that pauses immediate resends and re-enqueues pending messages when a stream is reconnecting.
  • Improved handling during stream reconnects to reduce failed send attempts and increase overall message delivery reliability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Two CcStreamSender methods now accept an optional bool *need_reconnect out-parameter and propagate it through resend and reconnect logic to re-enqueue unsent messages and suspend further sends when a stream is connecting. Separately, RocksDBHandler::FetchTableCatalog log statements were reordered relative to completion calls.

Changes

Cohort / File(s)	Summary
CcStreamSender signatures tx_service/include/remote/cc_stream_sender.h	Added optional bool *need_reconnect = nullptr parameter to SendMessageToNode(...) and SendScanRespToNode(...) declarations.
CcStreamSender resend & reconnect logic tx_service/src/remote/cc_stream_sender.cpp	Added need_reconnect out-parameter usage and local flags. Set flag when stream is in connecting state (stream_version == -1). Modified regular and long-message resend loops to re-enqueue remaining messages and stop further sends while reconnect is required. Propagated the flag through ConnectStreams and resend call sites.
RocksDBHandler logging order store_handler/rocksdb_handler.cpp	Reordered completion (SetFinish) and logging calls in FetchTableCatalog: on not-found and on successful catalog construction, logs now emit before finalizing the fetch result. No API changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Inspect resend-loop changes that re-enqueue messages and break loops when need_reconnect becomes true.
• Verify correct initialization/clearing and propagation of local need_reconnect variables in ConnectStreams and across call sites.
• Confirm default nullptr preserves prior behavior and that no call sites require signature updates.
• Review RocksDBHandler reorder to ensure no observable change to completion semantics or race with logging.

Poem

🐰 I nudge the stream with a gentle hop,
A tiny flag whispers "pause, don't prop",
Unsung messages curl and queue,
Waiting patient till the link is new,
Then off they bound — hop, hop, hooray! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided by the author. The repository template requires sections for tests, documentation, issue references, RFC links, and test suite verification.	Add a comprehensive pull request description following the template: include test coverage details, documentation updates, issue/RFC references using the specified format, and confirmation of passing the required test suite.
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix ConnectStreams: break loop if resend message fails' is specific and directly related to the main changes: improving the ConnectStreams flow with reconnection logic when resending fails.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix_stream_reconnect

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5fb5c30 and f6f9800.

📒 Files selected for processing (1)

• store_handler/rocksdb_handler.cpp (2 hunks)

🔇 Additional comments (2)

store_handler/rocksdb_handler.cpp (2)

1003-1010: LGTM! Log-then-complete pattern improves safety.

Emitting the log before calling SetFinish is the correct ordering. This ensures the log statement completes before any potential callbacks or cleanup triggered by SetFinish, avoiding potential use-after-free issues.

---

1030-1035: LGTM! Consistent ordering with the error path.

The log-then-complete pattern is consistently applied here as well. Both the not-found case (lines 1005-1008) and the normal case follow the same safe pattern: emit diagnostics before signaling completion.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5e25a8 and 27e7d81.

📒 Files selected for processing (2)

• tx_service/include/remote/cc_stream_sender.h (1 hunks)
• tx_service/src/remote/cc_stream_sender.cpp (8 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tx_service/include/remote/cc_stream_sender.h (1)

tx_service/src/remote/cc_stream_sender.cpp (2)

• SendScanRespToNode (371-516)
• SendScanRespToNode (371-374)

🔇 Additional comments (4)

tx_service/include/remote/cc_stream_sender.h (2)

91-92: LGTM! Backward-compatible API extension.

The optional parameters with default values maintain backward compatibility while enabling callers to opt into reconnection signaling and verbose logging.

---

95-96: LGTM! Consistent with SendMessageToNode.

The optional need_reconnect parameter follows the same pattern as SendMessageToNode, providing a consistent API for reconnection signaling.

tx_service/src/remote/cc_stream_sender.cpp (2)

199-202: LGTM! Proper reconnection signaling.

When the stream is in connecting state (version == -1), the code correctly signals the need for reconnection via the out-parameter. The message is buffered below (lines 210-213) for later retry.

---

401-404: LGTM! Consistent reconnection signaling.

The logic mirrors SendMessageToNode (lines 199-202), ensuring consistent behavior for long message streams.