Add context cancellation to SS DB layer - #3940
Conversation
PR SummaryMedium Risk Overview This PR threads the RPC/trace Reviewed by Cursor Bugbot for commit ee8aeff. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3940 +/- ##
==========================================
- Coverage 59.60% 58.57% -1.03%
==========================================
Files 2329 2231 -98
Lines 199860 188594 -11266
==========================================
- Hits 119117 110470 -8647
+ Misses 69339 67683 -1656
+ Partials 11404 10441 -963
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The context-threading design (ctxkv wrapper, optional ContextIterator/ContextIteratorStore interfaces, Pebble skip-loop polling) is coherent and the no-op-when-uncancellable guard keeps block execution off the new path. The blocking problem is that TraceBaker calls tracersAPI.TraceBlockByNumber directly with a cancellable timeout context, bypassing the new resultUnlessExpired guard, so a bake timeout can persist a synthesized error trace into the trace cache.
Findings: 1 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Every cancellation now flows through
x/evm/keeper/msg_server.go:84, which re-panics but first callsdebug.PrintStack(), logsEVM PANIC, and increments thepanicscounter /evm_panics_total. A routinetrace_timeoutwill therefore dump a stack per replayed tx and move a panic-alert metric. Consider special-casingcontext.Canceled/context.DeadlineExceededthere alongside the existingErrReadEstimateexemption. Context.GigaKVStore(sei-cosmos/types/context.go:575) is notctxkv-wrapped, unlikeKVStoreandTransientStore, so Giga-backed reads will not observe the trace deadline. Likely intentional (Giga is the block-execution path, not trace replay), but the asymmetry is worth a one-line comment.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // newContextCancelledRecoveryMiddleware recovers a store access that panicked | ||
| // because the caller's context was cancelled or exceeded its deadline. | ||
| func newContextCancelledRecoveryMiddleware(next recoveryMiddleware) recoveryMiddleware { |
There was a problem hiding this comment.
medium: newContextCancelledRecoveryMiddleware runs for every runTx mode, DeliverTx included, and it is checked first. Any panic whose value wraps context.Canceled/DeadlineExceeded — from any source, not only the new iterator — now returns code 1 / codespace undefined instead of ErrPanic
|
@seidroid review |
There was a problem hiding this comment.
The cancellation plumbing (ctxkv wrapper, optional ContextIterator/ContextIteratorStore, Pebble skip-loop polling, runTx recovery, resultUnlessExpired) is coherent and well tested, and the trace-baker cache-poisoning and finishMVCCIterator blast-radius issues from the last round are fixed. The remaining blocker is unchanged: the cancellable context still reaches legacyabci.BeginBlock with no recovery, so a trace/bake timeout during BeginBlock unwinds as a panic and leaks the memiavl snapshot lease.
Findings: 1 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Every cancellation panic raised during replay flows through
x/evm/keeper/msg_server.go:84, which callsdebug.PrintStack(), logsEVM PANIC, and increments thepanicscounter /evm_panics_totalbefore re-panicking (onlyocctypes.ErrReadEstimateis exempted). Because this PR makes trace timeouts a routine, expected event, a single timed-outdebug_traceBlockBy*on a busy block will dump one stack trace per replayed tx and move a panic-alert metric. Exemptingcontext.Canceled/context.DeadlineExceededthere alongside the existingErrReadEstimatecheck would keep the signal clean. (Also raised by Codex; flagged in the previous review and not yet addressed.) Context.GigaKVStore(sei-cosmos/types/context.go:575) is still notctxkv-wrapped whileKVStoreandTransientStorenow are. Likely intentional (Giga is the block-execution path, not trace replay), but the asymmetry deserves a one-line comment so the next reader does not assume it was missed.- The new helpers take
context.Contextas the second parameter (types.IteratorOn(store, ctx, ...),seidbtypes.IterateWithContext(store, ctx, ...),ctxkv.Wrap(parent, ctx)), against the Go convention of ctx first. These are new exported APIs, so it is cheap to fix now and awkward later. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The initializeBlock blocker from the previous round is genuinely fixed: the base/next snapshot leases are now released on a cancel/deadline panic, other panics are re-raised after release, and both behaviours are covered by tests. The cancellation plumbing (ctxkv wrapper, optional ContextIterator/ContextIteratorStore, Pebble skip-loop polling, runTx recovery, resultUnlessExpired including the trace baker) reads coherently and its blast radius stays confined to the debug_trace* / baker paths; what remains is non-blocking cleanup plus two nits carried over unaddressed.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
x/evm/keeper/msg_server.go:84still callsdebug.PrintStack(), logsEVM PANIC, and increments thepanicscounter /evm_panics_totalbefore re-panicking, with onlyocctypes.ErrReadEstimateexempted. This PR makes trace timeouts a routine event, so one timed-outdebug_traceBlockBy*on a busy block dumps a stack trace per replayed tx and moves a panic-alert metric — and a bake-enabled node behind the tip does this continuously. Exemptingcontext.Canceled/context.DeadlineExceededalongside the existing check keeps the alert signal clean. (Also raised by Codex; third round unaddressed.)- The profiled block-trace worker goroutines (
evmrpc/block_trace_profiled.go:197-220) havedefer pend.Done()but no panic recovery, andapi.backend.GetBlockContext(...)on line 208 runs outsideprofiledTraceTx's recover. Now that SS iteration panics on cancellation, an unrecovered panic in one of those goroutines would take the process down rather than fail the trace. Today those calls only do pointGets (no skip loop), so this is latent rather than live — but adefer func() { if r := recover(); r != nil { results[task.index] = &tracers.TxTraceResult{...} } }()in the worker would make it safe by construction. Context.GigaKVStore(sei-cosmos/types/context.go:575) is still notctxkv-wrapped whileKVStoreandTransientStorenow are. Likely intentional (Giga is the block-execution path, not trace replay), but the asymmetry still deserves a one-line comment so the next reader does not assume it was missed.- The new exported helpers still take
context.Contextas the second parameter (types.IteratorOn(store, ctx, …),seidbtypes.IterateWithContext(store, ctx, …),ctxkv.Wrap(parent, ctx)), against the Go convention of ctx first. These are new APIs, so it is cheap to fix now and awkward later. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return NewRocksDBIterator(itr, readOpts, prefix, start, end, version, db.earliestVersion, true), nil | ||
| } | ||
|
|
||
| func (db *Database) IteratorWithContext(_ context.Context, storeKey string, version int64, start, end []byte) (dbm.Iterator, error) { |
There was a problem hiding this comment.
[nit] Still open from the previous round. These two methods make *Database satisfy types.ContextIteratorStore while discarding ctx, so IterateWithContext will prefer them and cancellation silently no-ops on rocksdb. The fallback branch in IterateWithContext already produces identical behaviour without them, so either drop them (making the fallback explicit) or add a comment stating that the rocksdb iterator has no cancellable skip loop — otherwise the interface's "can observe a deadline" contract is misleading.
| // ReverseIterator dispatches between descending- and ascending-mode | ||
| // implementations depending on the on-disk encoding detected at open time. | ||
| func (db *Database) ReverseIterator(storeKey string, version int64, start, end []byte) (dbm.Iterator, error) { | ||
| return db.ReverseIteratorWithContext(context.Background(), storeKey, version, start, end) |
There was a problem hiding this comment.
[nit] Still open from the previous round. The existing godoc on ReverseIterator ("dispatches between descending- and ascending-mode implementations depending on the on-disk encoding detected at open time") was deleted rather than moved, and the four *WithContext methods added here have none. Iterator above (line 542) kept its doc, so the file now documents one half of the pair. Restoring the ReverseIterator comment and giving IteratorWithContext/ReverseIteratorWithContext a one-line "what" would match the convention in AGENTS.md.
| return db.ReverseIterator(EVMStoreKey, version, start, end) | ||
| } | ||
|
|
||
| func (s *EVMStateStore) IteratorWithContext(ctx context.Context, _ string, version int64, start, end []byte) (dbm.Iterator, error) { |
There was a problem hiding this comment.
[suggestion] The separateDBs / routeKey decision is now duplicated between Iterator/ReverseIterator (lines 108-128) and these two new methods — four copies of the same routing rule that can drift independently. CompositeStateStore has the same duplication of evmRouted.
Elsewhere in this PR the non-context entry point delegates instead (pebbledb.Database.Iterator → IteratorWithContext(context.Background(), …), and the same in cachekv.Store and storev2/state.Store). Doing that here keeps the routing decision at a single choke point:
func (s *EVMStateStore) Iterator(storeKey string, version int64, start, end []byte) (dbm.Iterator, error) {
return s.IteratorWithContext(context.Background(), storeKey, version, start, end)
}
Superseded: latest AI review found no blocking issues.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f90826. Configure here.
5 participants
Summary
debug_trace*can spend a long time inside a PebbleDB MVCC skip loop while still holding a trace semaphore slot.trace_timeoutdid not unwind that loop, so other traces failed immediately withserver busy.This change threads the RPC deadline into those skip loops so a timed-out trace aborts, releases its slot, and returns the deadline instead of a synthesized error trace.
ctxkvsoIterator/ReverseIteratorforward that context (no-op when the context cannot be cancelled, so block execution is unchanged).ContextIterator/ContextIteratorStoreinterfaces; Pebble skip loops pollctx.Err()between steps.runTxas errors, and surfacecontext.DeadlineExceededfromdebug_traceTransaction/TraceBlockBy*/TraceCall.A single in-flight Pebble
Seek/Nextstill runs to completion. Abort happens between skip steps.Test plan
Nextpanicctxkvforwards a cancellable context and is a no-op otherwiseresultUnlessExpiredreturns the deadline instead of a fake error traceTestTraceTransactionTimeoutReleasesSemaphore: firstdebug_traceTransactionblocked in an SS-shaped skip loop times out; a concurrent call is rejected as busy; the semaphore slot is free afterwards