Skip to content

feat(l2): make monitor quit #3622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 16, 2025
Merged

feat(l2): make monitor quit #3622

merged 10 commits into from
Jul 16, 2025

Conversation

gianbelinche
Copy link
Contributor

Motivation
When the monitor is quitted with Shift + Q it closes the monitor but does not end the process

Description
Changed the L2 task initialization to use JoinSet instead of a TaskTracker, so it can be joined and end the process if it ended.

How to test

  • Add --monitor to the init-l2-no-metrics target in crates/l2/Makefile.
  • Run a Sequencer (I suggest make restart in crates/l2).
  • Run the prover with make init-prover in crates/l2.
  • Run make test in crates/l2.
  • Press Shift + Q to close the monitor

Closes #3512

@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 13:57
@gianbelinche gianbelinche requested a review from a team as a code owner July 14, 2025 13:57
@github-actions github-actions bot added the L2 Rollup client label Jul 14, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR switches the L2 monitor shutdown handling from using TaskTracker to JoinSet so that quitting the monitor (Shift + Q) ends the process cleanly.

  • Break out of the sequencer loop when the first L2 task completes successfully.
  • Replace top‐level TaskTracker in the CLI command with a JoinSet and add a select branch to handle monitor quit.
  • Persist node config and cancel tasks when the monitor is closed.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
crates/l2/sequencer/mod.rs Changed the Ok(Ok(_)) match arm to break out of the join loop.
cmd/ethrex/l2/command.rs Imported JoinSet, initialized it, replaced tracker.spawn with join_set.spawn, and added a new select branch on join_next().
Comments suppressed due to low confidence (1)

cmd/ethrex/l2/command.rs:231

  • There are no tests verifying that pressing Shift+Q triggers this branch, cancels tasks, and persists the node config. Consider adding an integration test for the monitor shutdown flow.
                    _ = join_set.join_next() => {

@@ -227,6 +228,16 @@ impl Command {
tokio::time::sleep(Duration::from_secs(1)).await;
info!("Server shutting down!");
}
_ = join_set.join_next() => {
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When handling the first completed child task via join_set.join_next(), you don’t abort or await the remaining tasks. Explicitly abort pending tasks or await their completion to avoid background leaks.

Copilot uses AI. Check for mistakes.

Copy link

github-actions bot commented Jul 14, 2025

Lines of code report

Total lines added: 6
Total lines removed: 0
Total lines changed: 6

Detailed view
+-----------------------------------+-------+------+
| File                              | Lines | Diff |
+-----------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/command.rs   | 461   | +5   |
+-----------------------------------+-------+------+
| ethrex/crates/l2/sequencer/mod.rs | 173   | +1   |
+-----------------------------------+-------+------+

@tomip01 tomip01 moved this to In Review in ethrex_l2 Jul 14, 2025
Copy link
Contributor

@tomip01 tomip01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one small comment

@gianbelinche gianbelinche enabled auto-merge July 16, 2025 19:32
@gianbelinche gianbelinche added this pull request to the merge queue Jul 16, 2025
Merged via the queue into main with commit 3cf9507 Jul 16, 2025
40 of 55 checks passed
@gianbelinche gianbelinche deleted the monitor-quit branch July 16, 2025 20:21
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l2 Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L2 Rollup client
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants