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: Pro Plus Run ID: 📒 Files selected for processing (3)
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 Merge Risk: 🟡 Moderate · up to The PR makes RPC retries cancellation-aware and bounds shutdown, but Stop can return while an L2 block operation is still running because the operation may outlive the 30-second shutdown limit. A restart could therefore overlap with L2 state mutation; merge requires explicit owner acceptance or aligning the operation and shutdown lifetimes. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title clearly describes the bounded derivation shutdown wait, which is a real and important change in the pull request. It does not mention the related RPC retry and context-cancellation changes, but the title does not need to cover every change.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
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
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 hanging