op-batcher: enable graceful shutdown, closing current channel

[BrianBland]

Description

This change is intended to enable graceful shutdowns for the BatchSubmitter when the Stop method is called.

In order to shut down safely, this first closes any current pending channel, then publishes any remaining frames on the channel back to the L1. Once the channel has been completely closed and submitted, the submitter loop should exit without loading any new L2 blocks into its local state.

One caveat is that a repeated failure to submit frames back to the L1 may cause this loop to hang even when a shutdown is desired, as this prioritizes channel completion over shutdown speed. This can be worked around by passing a channel into the Stop method with a reasonable deadline, as is now the case in at least one e2e test.

Tests

Added tests for various channel manager Close() behaviors, including "close before first use", "close with no pending channel", "close with pending channel", and "close when all transactions are failed"

Invariants

For changes to critical code paths, please list and describe the invariants or key security properties of your new or changed code.

Additional context

Add any other context about the problem you're solving.

Metadata

• Fixes CLI-3244

[changeset-bot]

⚠️ No Changeset found

Latest commit: acdd051

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[trianglesphere]

Thanks for implementing this. I'd love to see some tests around this. Unit tests would be ideal, but we also have some frameworks which can handle more complex system setups.

[BrianBland]

Thanks for implementing this. I'd love to see some tests around this. Unit tests would be ideal, but we also have some frameworks which can handle more complex system setups.

Agreed about the need for testing. I'll see what I can put together here.

[BrianBland]

I believe that this should be technically tested by some of the e2e tests, such as TestMigration:

optimism/op-e2e/migration_test.go

Line 121 in 68c357c

func TestMigration(t *testing.T) {

This is proving to be fairly difficult to unit test due to the dependency on the L1Client and L2Client *ethclient.Client fields. Is there a preferred framework we can use to simulate these blockchain dependencies?

[mergify]

Hey @BrianBland! This PR has merge conflicts. Please fix them before continuing review.

[netlify]

✅ Deploy Preview for opstack-docs canceled.

Name	Link
🔨 Latest commit	acdd051
🔍 Latest deploy log	https://app.netlify.com/sites/opstack-docs/deploys/6421ea489e7b5a00085695e5

[mergify]

Hey @BrianBland! This PR has merge conflicts. Please fix them before continuing review.

[sebastianst]

Thanks for adding this!

We should think about a way to not only test the channelManager but also the BatchSubmitter itself, because there's a lot going on now with different contexts, closing done channels etc.

[tynes]

Great work @BrianBland this is a super high leverage fix

[sebastianst]

Looks good now!

It seems one fix got reverted, see comment.

[mergify]

This PR has been added to the merge queue, and will be merged soon.

[mergify]

Hey @BrianBland, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with @mergifyio requeue.
More details can be found on the Queue: Embarked in merge train check-run.

[BrianBland]

@Mergifyio requeue

[mergify]

requeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

Details

• sender-permission>=write