-
Notifications
You must be signed in to change notification settings - Fork 8
fix: Failure to fetch information from Kupo prevents output #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…kups fail Signed-off-by: Akhil Repala <[email protected]>
📝 WalkthroughWalkthroughThis change modifies error handling in transaction input resolution within the chain synchronization process. When Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
input/chainsync/chainsync.go (2)
410-418: Kupo resolution failures now logged and tolerated (matches PR intent)Turning
resolveTransactionInputsfailures into a logged error while emitting the transaction withresolvedInputs = nilachieves the goal of not blocking chainsync/blockfetch when Kupo is unavailable or misbehaving, while still surfacing the problem via logs. Semantically this is consistent in bothhandleRollForwardandhandleBlockFetchBlock.As a small polish, you might want to add the transaction hash/index as structured fields in this log to make it easier to correlate problematic inputs in production.
Also applies to: 497-505
664-668: Avoiddefer cancel()inside the per-input loop
defer cancel()inside thefor _, input := range transaction.Inputs()loop means earlier contexts aren’t canceled untilresolveTransactionInputsreturns, which keeps more timers/goroutines alive than necessary. It’s more efficient and idiomatic to callcancel()immediately afterk.Matchesreturns:- ctx, cancel := context.WithTimeout( - context.Background(), - defaultKupoTimeout, - ) - defer cancel() - - // Create a simple transaction identifier - txID := fmt.Sprintf("%d@%s", txIndex, txId) - matches, err := k.Matches(ctx, kugo.Transaction(txID)) + ctx, cancel := context.WithTimeout( + context.Background(), + defaultKupoTimeout, + ) + + // Create a simple transaction identifier + txID := fmt.Sprintf("%d@%s", txIndex, txId) + matches, err := k.Matches(ctx, kugo.Transaction(txID)) + cancel()This keeps behavior the same while tightening resource usage per input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
input/chainsync/chainsync.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
Closes #400
Summary by cubic
Emit input events even when Kupo fails to resolve transaction inputs, logging the error and sending events without resolved inputs. Fixes #400.
Written for commit 45394a1. Summary will update automatically on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.