safety: supervise flashblock scheduler task#556
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens flashblock scheduling by running the scheduler via the task executor and supervising it for panics, improving observability and safety for a task that should never unwind unexpectedly.
Changes:
- Add a new
flashblock_scheduler_deathcounter metric to track scheduler panics. - Spawn the flashblock scheduler using the builder task executor instead of
tokio::spawn. - Catch scheduler panics (
catch_unwind) and emit an error + increment the new counter.
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 a counter to track abnormal (panic) termination of the flashblock scheduler task. |
| crates/op-rbuilder/src/builder/payload.rs | Runs the scheduler through the task executor and catches panics to increment the new metric and log an error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.executor.spawn_task(async move { | ||
| if std::panic::AssertUnwindSafe(scheduler_task) | ||
| .catch_unwind() | ||
| .await | ||
| .is_err() | ||
| { | ||
| scheduler_metrics.flashblock_scheduler_death.increment(1); | ||
| error!( | ||
| target: "payload_builder", | ||
| id = %scheduler_payload_id, | ||
| "Flashblock scheduler task panicked, no further flashblocks will be triggered for this payload", | ||
| ); | ||
| } | ||
| }); |
| self.executor.spawn_task(async move { | ||
| if std::panic::AssertUnwindSafe(scheduler_task) | ||
| .catch_unwind() | ||
| .await | ||
| .is_err() |
|
Why is this safety measure needed? I don't know of any source of panicking with that task |
It's not needed but nice to have; rust already enforce handling all cases including panics, but here the task is detached so the failure signal is the same as the success one (recv None). There's no per job overhead, and it's good practice to atleast catch panic on detached task. |
It's adding complexity. I understand what you're saying that in general it's good to be safe but this is a code path we should just guarantee never panics. |
I think the complexity is scoped but I understand, one improvement could be to separate metrics in a new OpRBuilderDebugMetrics. We can remove it I don't hold strong opinion, I just tried to pinpoint failure points as I would like to try some refactoring around flashblocks scheduling. Feel free to close. |
Spawns the flashblock scheduler through task executor.
Also catches panics (flashblock_scheduler_death counter). Previously dropped trigger channel silently and could look like a completed schedule. This is a safety measure as scheduler should never panic.