WIP: fix(node): bound derivation shutdown wait - #1047
Conversation
…ancellation RetryableClient shared one BackOff across all methods and passed it to backoff.Retry unbound, so a canceled context could not stop a retry loop; context errors were also classified retryable. A shutdown during an L2 outage therefore held Derivation.Stop for the full 30-minute budget. Bind the backoff to the caller's context (fresh instance per call, since BackOff is stateful and these methods run concurrently), treat context errors as permanent, and bound Stop's wait so shutdown cannot hang on a poll that is mid-RPC. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDerivation shutdown now has a bounded wait and uses its context for selected RPC calls. Retryable RPC operations now create per-call, context-bound backoff policies and do not retry canceled or expired contexts. Tests cover cancellation classification and immediate retry termination. ChangesDerivation context lifecycle
Context-bound retry policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to Derivation shutdown can no longer wait indefinitely during an L2 outage, while transaction lookups cancel with derivation shutdown and block-number retries retain their existing behavior. No actionable merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
Classifying context errors as permanent is enough to unblock a canceled caller: retryableError returning false makes the backoff operation return nil, so the call exits after one attempt. Binding the backoff to ctx was a separate robustness fix and is not needed here. Co-authored-by: Cursor <cursoragent@cursor.com>
curryxbo
changed the title
Revert the retryableError change; bounding Stop's wait is enough to keep shutdown from hanging, since the process exits once Stop returns. Document why derive's BlockNumber call must stay on context.Background(). Co-authored-by: Cursor <cursoragent@cursor.com>
curryxbo
changed the title
curryxbo
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
1 participant
Problem
Derivation.Stop()cancels the context, then blocks on<-d.stopwith no bound. The main loop only checksctx.Done()between polls, so shutdown waits for the in-flight poll to unwind. When L2 is unavailable that poll sits insideRetryableClient's backoff, which is not context-bound and treats a canceled context as retryable, so it runs forGethRetryMaxElapsedTime(30 min). ASIGTERMduring an L2 outage heldStopuntil the supervisor sentSIGKILL.Changes
Contained to
node/derivation/derivation.go:Stop()waits ond.stopwith a 30s bound and logs if it expires. Abandoning the wait is safe: the process exits onceStopreturns, and the L1 cursor is only persisted after a fully successful poll, so a partially derived batch is redone on restart.fetchRollupDataByTxHashpassesd.ctxinstead ofcontext.Background(). That client is a rawethclient.Clientwith no retry wrapper, so cancellation aborts the call immediately.derive'sBlockNumbercall keepscontext.Background(), now with a comment explaining why: it goes throughRetryableClient, where a canceled context is classified retryable, sod.ctxthere would spin for the full 30-minute budget instead of returning.The underlying issue —
retryableErrornot treating context errors as permanent, and the backoff not being bound to the caller's context — is left alone. Fixing it touches every sequencer RPC path and is not needed to bound shutdown.Test plan
go test ./types/... ./derivation/...SIGTERMa full node while the L2 EL is stopped, confirm the process exits within theStopbound instead of hangingSummary by CodeRabbit