safety(continuous): pool-epoch listener liveness metrics#557
Conversation
There was a problem hiding this comment.
Pull request overview
Adds liveness-oriented metrics around the continuous-build pool-change listener so operators can distinguish “no new pending txs” from “listener is no longer running,” which would otherwise silently freeze continuous candidate gating behavior.
Changes:
- Added
pool_change_epoch(gauge) andpool_epoch_listener_exit(counter) toOpRBuilderMetrics. - Updated the pending-tx listener task to publish the current epoch to the gauge and increment/log on listener exit (including panics via
catch_unwind).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/op-rbuilder/src/metrics.rs | Adds new gauge/counter fields for pool-epoch listener liveness/exit detection. |
| crates/op-rbuilder/src/builder/service.rs | Instruments the pool pending-tx listener to update the epoch gauge and record exits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Current value of the continuous-build pool-change epoch counter. | ||
| /// A flatlined gauge on a dashboard means the pool listener died, not | ||
| /// that the pool is idle. |
| let _ = std::panic::AssertUnwindSafe(listen).catch_unwind().await; | ||
| listener_metrics.pool_epoch_listener_exit.increment(1); | ||
| warn!( | ||
| target: "payload_builder", | ||
| "Pool epoch listener exited (channel closed or task panicked); \ | ||
| continuous candidate gating is now frozen and will degrade to idle-backoff" | ||
| ); |
| listener_metrics.pool_change_epoch.set(epoch as f64); | ||
| } | ||
| }; | ||
| let _ = std::panic::AssertUnwindSafe(listen).catch_unwind().await; |
There was a problem hiding this comment.
Why do you need to catch panics for this?
There was a problem hiding this comment.
The counter is an alert signal, without catching panic we would skip it and miss alert.
There was a problem hiding this comment.
I mean have you been seeing anything to motivate tracking this? This closure is very simple so I don't see the need to catch a panic here
There was a problem hiding this comment.
I would like to move all this pool logic somewhere else, so from the builder service I consider this closure like a black box even if it is very simple (at the moment).
Now if the pool listener panic for whatever reason we can't tell anything from the builder service, and can't make the difference between an empty pool or a panicked one. But totally agree that in the current shape this seems useless, I can always add this later with more changes that justify this if needed.
Adds pool_change_epoch gauge + pool_epoch_listener_exit counter so we can detect dead pool-change listener from a genuinely idle pool. A dead listener could silently degrades continuous candidate gating.