pm: auto-prune dead winning tickets + stop retrying expired tickets - #3954
pm: auto-prune dead winning tickets + stop retrying expired tickets#3954Pon-node wants to merge 3 commits into
Conversation
Dead winning tickets accumulate in the SQLite ticketQueue table and operators end up deleting the whole database to recover. Prune them automatically from the LocalSenderMonitor cleanup ticker: - unredeemed tickets older than ticketValidityPeriod+1 rounds (the redemption loop can never select them again) - tickets marked redeemed with a zero txHash, i.e. permanently failed redemptions Live tickets within the validity window and successfully redeemed tickets (real txHash) are never touched. Controlled by a new -ticketPrune bool flag, default true; opt out with -ticketPrune=false. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The prune cutoff was ticketValidityPeriod+1 rounds behind the last initialized round, one round more conservative than the redemption selection window (ticketValidityPeriod rounds). As a result a ticket that fell out of the redemption window — and could never be selected again — lingered in the store for a full extra round before being pruned. Set the cutoff to ticketValidityPeriod so it matches the redemption window exactly. The prune condition is now the exact complement of the selection condition (creationRound >= LastInit-ticketValidityPeriod), so a dead ticket is removed on the next cleanup tick after it leaves the window while a still-redeemable ticket is never pruned. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The redemption loop selects winning tickets purely by the local round window (creationRound >= LastInit - ticketValidityPeriod) and never pre-checks on-chain expiry. When the TicketBroker reverts with "ticket is expired", isNonRetryableTicketErr did not match it, so the error was treated as retryable: the loop re-selected and re-attempted the same ticket every block until the local round window closed. The attempts revert at eth_estimateGas (no gas burned, no tx mined) but generate pointless RPC churn. Mark "ticket is expired" as non-retryable so the ticket is recorded as redeemed on the first revert and excluded from further selection, stopping the retries immediately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds automatic pruning of dead winning tickets (expired-unredeemed and permanently-failed-redeemed) from the ticket queue database. A new ChangesDead Ticket Pruning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/db.go (1)
391-393:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPre-existing bug: wrong statement closed.
Line 392 closes
db.deleteMiniHeaderinstead ofdb.deleteUnbondingLock. This is not introduced by the current PR but should be fixed.🐛 Proposed fix
if db.deleteUnbondingLock != nil { - db.deleteMiniHeader.Close() + db.deleteUnbondingLock.Close() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/db.go` around lines 391 - 393, In the conditional block that checks if db.deleteUnbondingLock is not nil, the Close method is being called on the wrong object. Change the call from db.deleteMiniHeader.Close() to db.deleteUnbondingLock.Close() so that the correct resource is properly closed when the unbonding lock exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@common/db.go`:
- Around line 391-393: In the conditional block that checks if
db.deleteUnbondingLock is not nil, the Close method is being called on the wrong
object. Change the call from db.deleteMiniHeader.Close() to
db.deleteUnbondingLock.Close() so that the correct resource is properly closed
when the unbonding lock exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd765834-680e-46e9-82f9-9abf902c47f5
📒 Files selected for processing (7)
cmd/livepeer/starter/flags.gocmd/livepeer/starter/starter.gocommon/db.gocommon/db_test.gopm/queue.gopm/queue_test.gopm/sendermonitor.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3954 +/- ##
===================================================
- Coverage 33.30014% 33.27964% -0.02050%
===================================================
Files 171 171
Lines 42174 42212 +38
===================================================
+ Hits 14044 14048 +4
- Misses 27077 27108 +31
- Partials 1053 1056 +3
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Thanks for the PR @Pon-node This is OK with me if it also works for others running an orchestrator. Maybe @rickstaa can check with some others. Re: the ticket skipping behavior, it almost seems preferable to embed ticket skipping into the DB retrieval query, rather than waiting for a runtime failure, but that might be a larger change and shouldn't block this PR. Smaller and more incremental changes are preferable for sensitive paths like these. |
2 participants
What
Three related improvements to the PM winning-ticket redemption path:
-ticketPruneflag — auto-prune dead winning tickets (8a4d8ae1)Adds an opt-in cleanup loop that removes unredeemable winning tickets from the ticket store. A ticket is dead if it is unredeemed and outside the redemption validity window (the redemption loop can never select it again) or if its redemption permanently failed. Without this, dead tickets accumulate in the DB indefinitely.
Prune cutoff matches the redemption window exactly (
b75a0fcf)The prune cutoff is the exact complement of the selection condition (
creationRound >= LastInit - ticketValidityPeriod), so a dead ticket is pruned as soon as it leaves the redemption window on the next cleanup tick, rather than a round later. Still-redeemable tickets are never pruned.Stop retrying tickets that are expired on-chain (
86ea742d)The redemption loop selects winning tickets purely by the local round window and never pre-checks on-chain expiry. When
TicketBrokerreverts withticket is expired,isNonRetryableTicketErrdid not match it, so the error was treated as retryable: the loop re-selected and re-attempted the same ticket every block until the local round window closed. The attempts revert ateth_estimateGas(no gas burned, no tx mined) but generate pointless RPC churn. This change marksticket is expiredas non-retryable so the ticket is recorded as redeemed on the first revert and excluded from further selection.Why
Observed on a live orchestrator: a winning ticket whose redemption window elapsed while the orch wallet was out of gas became permanently unredeemable, then the redeem loop hammered it with an expired-revert every ~15s, and the dead row lingered in the DB. These changes clean up dead tickets automatically and stop the wasteful retry churn.
Testing
TestTicketQueueLoop_IsNonRetryableTicketErr_MarkAsRedeemed(red → green).pmsuite,go vet, andgofmtclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests