Conversation
WalkthroughA local helper function Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
bitcoin-coder-bob
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/client-lib/client/grpc/client.go (1)
299-300: Log when terminal batch error delivery times out.These branches ignore
send(...)'s boolean result. If the 5-second terminal send times out, the error reason is silently lost; adding a warning here would make failures diagnosable.Also applies to: 322-323, 350-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/client/grpc/client.go` around lines 299 - 300, The terminal send calls like send(client.BatchEventChannel{Err: err}) currently ignore send(...)'s boolean result so timed-out deliveries are dropped silently; update these branches (the occurrences using send with client.BatchEventChannel{Err: err} at the shown location and the similar spots around the other two occurrences) to capture the bool result and, when false, emit a warning log that includes context and the err value (e.g., "terminal batch error delivery timed out" + err) so failed/timeouts are diagnosable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client-lib/client/grpc/client.go`:
- Around line 456-463: The send helper currently returns false immediately on
ctx.Done(), which can drop terminal TransactionEvent errors; change send (used
to push to eventsCh) so that for terminal events (the error/terminal kinds sent
on the branches at the locations around lines 474, 503, 544) it will perform a
blocking send to eventsCh (ignoring ctx.Done) to guarantee delivery, while
non-terminal events can keep the non-blocking/select behavior; also add a small
buffer when creating eventsCh to reduce brief consumer stalls. Update the send
function and the eventsCh creation accordingly (referencing send, eventsCh,
ctx.Done and the terminal event sends).
---
Nitpick comments:
In `@pkg/client-lib/client/grpc/client.go`:
- Around line 299-300: The terminal send calls like
send(client.BatchEventChannel{Err: err}) currently ignore send(...)'s boolean
result so timed-out deliveries are dropped silently; update these branches (the
occurrences using send with client.BatchEventChannel{Err: err} at the shown
location and the similar spots around the other two occurrences) to capture the
bool result and, when false, emit a warning log that includes context and the
err value (e.g., "terminal batch error delivery timed out" + err) so
failed/timeouts are diagnosable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d7733d0-1dc5-4a3b-92c8-2640f49bcd1b
📒 Files selected for processing (1)
pkg/client-lib/client/grpc/client.go
| send := func(ev client.TransactionEvent) bool { | ||
| select { | ||
| case eventsCh <- ev: | ||
| return true | ||
| case <-ctx.Done(): | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid dropping terminal TransactionEvent errors on cancellation.
On Line 460, send returns false immediately on ctx.Done(), so terminal errors sent on Lines 474, 503, and 544 can still be lost. That weakens the transaction-event delivery guarantee.
Suggested adjustment
- send := func(ev client.TransactionEvent) bool {
- select {
- case eventsCh <- ev:
- return true
- case <-ctx.Done():
- return false
- }
- }
+ send := func(ev client.TransactionEvent) bool {
+ if ev.Err != nil {
+ timer := time.NewTimer(5 * time.Second)
+ defer timer.Stop()
+ select {
+ case eventsCh <- ev:
+ return true
+ case <-timer.C:
+ return false
+ }
+ }
+ select {
+ case eventsCh <- ev:
+ return true
+ case <-ctx.Done():
+ return false
+ }
+ }Also consider a small buffer on eventsCh at creation time to improve terminal-event delivery under brief consumer stalls.
Based on learnings: "In the arkd codebase, transaction events should never be dropped when propagating to channels. The interface layer is designed to read events immediately, so buffering channels is preferred over non-blocking sends with drop semantics to maintain delivery guarantees."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| send := func(ev client.TransactionEvent) bool { | |
| select { | |
| case eventsCh <- ev: | |
| return true | |
| case <-ctx.Done(): | |
| return false | |
| } | |
| } | |
| send := func(ev client.TransactionEvent) bool { | |
| if ev.Err != nil { | |
| timer := time.NewTimer(5 * time.Second) | |
| defer timer.Stop() | |
| select { | |
| case eventsCh <- ev: | |
| return true | |
| case <-timer.C: | |
| return false | |
| } | |
| } | |
| select { | |
| case eventsCh <- ev: | |
| return true | |
| case <-ctx.Done(): | |
| return false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client-lib/client/grpc/client.go` around lines 456 - 463, The send helper
currently returns false immediately on ctx.Done(), which can drop terminal
TransactionEvent errors; change send (used to push to eventsCh) so that for
terminal events (the error/terminal kinds sent on the branches at the locations
around lines 474, 503, 544) it will perform a blocking send to eventsCh
(ignoring ctx.Done) to guarantee delivery, while non-terminal events can keep
the non-blocking/select behavior; also add a small buffer when creating eventsCh
to reduce brief consumer stalls. Update the send function and the eventsCh
creation accordingly (referencing send, eventsCh, ctx.Done and the terminal
event sends).
1 participant
requested via: #933 (comment)
Changes in pkg/client-lib/client/grpc/client.go:
GetEventStream- send helper forBatchEventChannel:Consolidates all channel-send-or-ctx-cancel patterns into a singlesend()function. For terminal errors (ev.Err != nil), it uses a 5-secondtimeout instead of
ctx.Done(), ensuring consumers always learn why the stream ended even if the context was already cancelled.GetTransactionsStream- send helper forTransactionEvent: Same consolidation pattern, replacing 5 separateselect { case <-ctx.Done(): return; case eventsCh <- ... }blocks with calls tosend().Summary by CodeRabbit