chore: pace the duplicate prune sweep by measured block time - #1427
Conversation
Time Submission Status
Submit or update total time with: Add time on top of previous submission with: See available commands to help comply with our Guidelines. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesBroadcast Pending Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Broadcast timeouts now stop duplicate retries while scheduled digest and pruning work ends the current firing cleanly; smaller prune batches pace work across transactions. No concrete current-head merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant BroadcastAutoPruneDuplicatesWithRetry
participant Mempool
Scheduler->>BroadcastAutoPruneDuplicatesWithRetry: start broadcast
BroadcastAutoPruneDuplicatesWithRetry->>Mempool: submit transaction
Mempool-->>BroadcastAutoPruneDuplicatesWithRetry: timeout while transaction remains pending
BroadcastAutoPruneDuplicatesWithRetry-->>Scheduler: ErrBroadcastPending
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
@holdex pr submit-time 4h |
Sizes the duplicate prune sweep against what a prune transaction actually costs on mainnet, and stops a broadcast timeout from re-running one.
Why
Pruning was enabled on mainnet for one firing on 08-Sep and turned off again. In that window the gateway returned 1,619 × 503 and 28 × 500 between 00:00 and 00:19 UTC, with no 5xx before or after. Block execution for the 22 blocks carrying an
auto_prune_duplicatestransaction, against their neighbours:A prune transaction held the consensus path for about 17 seconds. The gateway's read timeout is 20 s, so reads queued behind it and timed out — the 503s carry
rpc_code -32001(ErrorTimeout) at a duration of exactly 20,000 ms. Nothing was down; the node was busy.The knob that was tuned was the wrong one. Run 1 deleted 794 event times in 14.4 s; every later run deleted essentially nothing and still cost 16-18 s. The work is the per-stream history scan across
stream_batch_sizestreams, andPruneDeleteCapbounds deletions only, so it cannot bound the scan.What changes
A whole pass costs what it costs — roughly 8.6 hours of execution for 182k streams however it is sliced — so the shape is many cheap transactions at a low duty cycle rather than a few expensive ones.
PruneStreamBatchSize100 to 5, putting a transaction near 0.85 s instead of 85% of the read budget.PruneDrainRunDelay60 s to 10 s andPruneDrainMaxRuns100 to 1000: an ~8% duty cycle, 5,000 streams a firing, a full pass in about 37 firings.PruneDeleteCap5,000 to 1,000. Not the bottleneck, but at five streams a run the old cap could never bind.That last one is the defect rather than a tuning change.
ktypes.ErrTxTimeoutmeans the wait for inclusion elapsed, not that the transaction was rejected: it stays in the mempool and still executes. Retrying it with a fresh nonce therefore duplicates a 17-second scan instead of replacing it, and the two transactions race the same nonce sequence. On mainnetauto_prune_duplicatesran 22 times on chain while the scheduler recorded one success and five failures. The retry loop now returnsErrBroadcastPending, the drain ends that firing, and the cyclic cursor resumes next time.BroadcastAutoDigestWithArgsAndRetryhas the identical shape and left the same nonce gaps, so it gets the same treatment. Detection iserrors.Isagainst the exported sentinel rather than string matching.Also corrects the
DigestDeleteCapcomment, which attributed the testnet crash loop to this cap being too large. That was wrong: the loop was log volume — debug level on the awslogs driver, one line per decoded row, blowing the hardcoded 30-second precommit window — and 40 hours across six firings on the old 100,000 cap confirmed it after the log level changed.Tests
Three regression tests on the pending-broadcast path: a bare timeout, a wrapped one, and an ordinary network error that must still be retried. Each was checked by breaking the fix and watching it fail — without it both timeout cases retry until the context deadline, 30 s each, which is the amplification itself. All three
tn_digestpackages pass.What this does not fix
The per-stream scan is still unbounded, because the deletable set is a whole-stream property: one stream holding 1.3 M rows costs the same in a batch of 5 as in a batch of 100. This fixes the average and not that tail. Bounding scan work per transaction needs a design change — a resumable cursor within a stream, or a maintained duplicate index — and does not belong in a constants change.
Re-enabling on mainnet should not be gated on testnet, which holds 212k rows at 1.8% duplicates and never reaches this path. The next attempt wants a mainnet-shaped fixture through
internal/benchmark/digest, with per-transaction block time as the pass criterion.Context
Summary by CodeRabbit
Bug Fixes
Performance