Do not block forever on read-your-own-commits #743
Conversation
WalkthroughThe updates focus on improving the robustness and clarity of the cursor tracking process in the payer API. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NodeCursorTracker
participant gRPC
Caller->>NodeCursorTracker: BlockUntilDesiredCursorReached(parentCtx, ...)
NodeCursorTracker->>NodeCursorTracker: Create 10s timeout context (ctx)
loop Wait for cursor or context done
NodeCursorTracker->>NodeCursorTracker: select { cursor reached | parentCtx.Done | ctx.Done | error }
alt parentCtx.Done
NodeCursorTracker-->>Caller: return nil (graceful cancel)
else ctx.Done (timeout)
NodeCursorTracker-->>Caller: return DeadlineExceeded error
else ct.ctx.Done (internal cancel)
NodeCursorTracker-->>Caller: return Aborted error
else error from stream
NodeCursorTracker-->>Caller: return Aborted error if canceled
end
end
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/api/payer/nodeCursorTracker.go (1)
52-54: Consider making the timeout duration configurable.The 10-second timeout prevents indefinite blocking, which is good. However, hardcoding this value might be inflexible for different operational environments.
+const defaultCursorReachTimeout = 10 * time.Second + func (ct *NodeCursorTracker) BlockUntilDesiredCursorReached( parentCtx context.Context, nodeId uint32, desiredOriginatorId uint32, desiredSequenceId uint64, ) error { // TODO(mkysel) ideally we wouldn't create and tear down the stream for every request - ctx, cancel := context.WithTimeout(parentCtx, 10*time.Second) + ctx, cancel := context.WithTimeout(parentCtx, defaultCursorReachTimeout) defer cancel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/api/payer/nodeCursorTracker.go(2 hunks)pkg/api/payer/service.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (5)
pkg/api/payer/nodeCursorTracker.go (4)
45-45: Good rename fromctxtoparentCtx.This improves clarity by distinguishing between the parent context and the derived timeout context that will be created.
87-87: Appropriate error code change fromInternaltoAborted.This provides a more accurate error code when node termination occurs, improving error clarity.
88-95: Good addition of timeout handling with detailed error message.The new timeout case appropriately returns a
DeadlineExceedederror code with the elapsed wait time, which will help with debugging timeout issues.
101-101: Consistent error code update for node termination.This matches the earlier error code change, maintaining consistency in error reporting for the same condition.
pkg/api/payer/service.go (1)
388-388: Improved error logging with actual error details.Including the actual error in the log message is crucial for debugging and monitoring, particularly since you've updated the error reporting in
BlockUntilDesiredCursorReachedto be more specific about timeouts and cancellations.
Add 10-second timeout to
BlockUntilDesiredCursorReachedfunction to prevent indefinite blocking on read-your-own-commitscontext.WithTimeoutto prevent indefinite blockingcodes.Abortedfor node termination,codes.DeadlineExceededfor timeouts)BlockUntilDesiredCursorReachedfails📍Where to Start
Start with the
BlockUntilDesiredCursorReachedfunction in nodeCursorTracker.go which contains the core timeout and error handling changes.Macroscope summarized 1bf5d81.
Summary by CodeRabbit