op-challenger: cleanup service lifecycle, improve subscription ctx usage

op-batcher/batcher/service.go

@@ -312,6 +312,10 @@ func (bs *BatcherService) Stop(ctx context.Context) error {
 			result = errors.Join(result, fmt.Errorf("failed to close balance metricer: %w", err))
 		}
 	}
+	if bs.TxManager != nil {
+		bs.TxManager.Close()
+	}
+
 	if bs.metricsSrv != nil {
 		if err := bs.metricsSrv.Stop(ctx); err != nil {
 			result = errors.Join(result, fmt.Errorf("failed to stop metrics server: %w", err))

op-challenger/challenger.go

@@ -2,22 +2,19 @@ package op_challenger
 
 import (
 	"context"
-	"fmt"
+
+	"github.com/ethereum/go-ethereum/log"
 
 	"github.com/ethereum-optimism/optimism/op-challenger/config"
 	"github.com/ethereum-optimism/optimism/op-challenger/game"
-	"github.com/ethereum/go-ethereum/log"
+	"github.com/ethereum-optimism/optimism/op-service/cliapp"
 )
 
-// Main is the programmatic entry-point for running op-challenger
-func Main(ctx context.Context, logger log.Logger, cfg *config.Config) error {
+// Main is the programmatic entry-point for running op-challenger with a given configuration.
+func Main(ctx context.Context, logger log.Logger, cfg *config.Config) (cliapp.Lifecycle, error) {
 	if err := cfg.Check(); err != nil {
-		return err
+		return nil, err
 	}
-	service, err := game.NewService(ctx, logger, cfg)
-	if err != nil {
-		return fmt.Errorf("failed to create the fault service: %w", err)
-	}
-
-	return service.MonitorGame(ctx)
+	srv, err := game.NewService(ctx, logger, cfg)
+	return srv, err
 }

op-challenger/challenger_test.go

@@ -12,6 +12,7 @@ import (
 
 func TestMainShouldReturnErrorWhenConfigInvalid(t *testing.T) {
 	cfg := &config.Config{}
-	err := Main(context.Background(), testlog.Logger(t, log.LvlInfo), cfg)
+	app, err := Main(context.Background(), testlog.Logger(t, log.LvlInfo), cfg)
 	require.ErrorIs(t, err, cfg.Check())
+	require.Nil(t, app)
 }

op-challenger/cmd/main.go

@@ -4,16 +4,18 @@ import (
 	"context"
 	"os"
 
-	op_challenger "github.com/ethereum-optimism/optimism/op-challenger"
-	opservice "github.com/ethereum-optimism/optimism/op-service"
-	"github.com/ethereum/go-ethereum/log"
 	"github.com/urfave/cli/v2"
 
+	"github.com/ethereum/go-ethereum/log"
+
+	challenger "github.com/ethereum-optimism/optimism/op-challenger"
 	"github.com/ethereum-optimism/optimism/op-challenger/config"
 	"github.com/ethereum-optimism/optimism/op-challenger/flags"
 	"github.com/ethereum-optimism/optimism/op-challenger/version"
+	opservice "github.com/ethereum-optimism/optimism/op-service"
 	"github.com/ethereum-optimism/optimism/op-service/cliapp"
 	oplog "github.com/ethereum-optimism/optimism/op-service/log"
+	"github.com/ethereum-optimism/optimism/op-service/opio"
 )
 
 var (
@@ -26,14 +28,15 @@ var VersionWithMeta = opservice.FormatVersion(version.Version, GitCommit, GitDat
 
 func main() {
 	args := os.Args
-	if err := run(args, op_challenger.Main); err != nil {
+	ctx := opio.WithInterruptBlocker(context.Background())
+	if err := run(ctx, args, challenger.Main); err != nil {
 		log.Crit("Application failed", "err", err)
 	}
 }
 
-type ConfigAction func(ctx context.Context, log log.Logger, config *config.Config) error
+type ConfiguredLifecycle func(ctx context.Context, log log.Logger, config *config.Config) (cliapp.Lifecycle, error)
 
-func run(args []string, action ConfigAction) error {
+func run(ctx context.Context, args []string, action ConfiguredLifecycle) error {
 	oplog.SetupDefaults()
 
 	app := cli.NewApp()
@@ -42,20 +45,20 @@ func run(args []string, action ConfigAction) error {
 	app.Name = "op-challenger"
 	app.Usage = "Challenge outputs"
 	app.Description = "Ensures that on chain outputs are correct."
-	app.Action = func(ctx *cli.Context) error {
+	app.Action = cliapp.LifecycleCmd(func(ctx *cli.Context, close context.CancelCauseFunc) (cliapp.Lifecycle, error) {
 		logger, err := setupLogging(ctx)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		logger.Info("Starting op-challenger", "version", VersionWithMeta)
 
 		cfg, err := flags.NewConfigFromCLI(ctx)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		return action(ctx.Context, logger, cfg)
-	}
-	return app.Run(args)
+	})
+	return app.RunContext(ctx, args)
 }
 
 func setupLogging(ctx *cli.Context) (log.Logger, error) {

op-challenger/cmd/main_test.go

@@ -2,15 +2,19 @@ package main
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
 	"time"
 
-	"github.com/ethereum-optimism/optimism/op-challenger/config"
-	"github.com/ethereum-optimism/optimism/op-service/txmgr"
+	"github.com/stretchr/testify/require"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/stretchr/testify/require"
+
+	"github.com/ethereum-optimism/optimism/op-challenger/config"
+	"github.com/ethereum-optimism/optimism/op-service/cliapp"
+	"github.com/ethereum-optimism/optimism/op-service/txmgr"
 )
 
 var (
@@ -36,7 +40,7 @@ func TestLogLevel(t *testing.T) {
 	for _, lvl := range []string{"trace", "debug", "info", "error", "crit"} {
 		lvl := lvl
 		t.Run("AcceptValid_"+lvl, func(t *testing.T) {
-			logger, _, err := runWithArgs(addRequiredArgs(config.TraceTypeAlphabet, "--log.level", lvl))
+			logger, _, err := dryRunWithArgs(addRequiredArgs(config.TraceTypeAlphabet, "--log.level", lvl))
 			require.NoError(t, err)
 			require.NotNil(t, logger)
 		})
@@ -431,25 +435,29 @@ func TestCannonL2Genesis(t *testing.T) {
 }
 
 func verifyArgsInvalid(t *testing.T, messageContains string, cliArgs []string) {
-	_, _, err := runWithArgs(cliArgs)
+	_, _, err := dryRunWithArgs(cliArgs)
 	require.ErrorContains(t, err, messageContains)
 }
 
 func configForArgs(t *testing.T, cliArgs []string) config.Config {
-	_, cfg, err := runWithArgs(cliArgs)
+	_, cfg, err := dryRunWithArgs(cliArgs)
 	require.NoError(t, err)
 	return cfg
 }
 
-func runWithArgs(cliArgs []string) (log.Logger, config.Config, error) {
+func dryRunWithArgs(cliArgs []string) (log.Logger, config.Config, error) {
 	cfg := new(config.Config)
 	var logger log.Logger
 	fullArgs := append([]string{"op-challenger"}, cliArgs...)
-	err := run(fullArgs, func(ctx context.Context, log log.Logger, config *config.Config) error {
+	testErr := errors.New("dry-run")
+	err := run(context.Background(), fullArgs, func(ctx context.Context, log log.Logger, config *config.Config) (cliapp.Lifecycle, error) {
 		logger = log
 		cfg = config
-		return nil
+		return nil, testErr
 	})
+	if errors.Is(err, testErr) { // expected error
+		err = nil
+	}
 	return logger, *cfg, err
 }
 

op-challenger/game/fault/responder/responder_test.go

@@ -232,6 +232,9 @@ func (m *mockTxManager) From() common.Address {
 	return m.from
 }
 
+func (m *mockTxManager) Close() {
+}
+
 type mockContract struct {
 	calls            int
 	callFails        bool

op-challenger/game/monitor.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"sync"
 	"time"
 
 	"github.com/ethereum-optimism/optimism/op-challenger/game/scheduler"
@@ -39,6 +40,7 @@ type gameMonitor struct {
 	allowedGames     []common.Address
 	l1HeadsSub       ethereum.Subscription
 	l1Source         *headSource
+	runState         sync.Mutex
 }
 
 type MinimalSubscriber interface {
@@ -126,27 +128,32 @@ func (m *gameMonitor) onNewL1Head(ctx context.Context, sig eth.L1BlockRef) {
 	}
 }
 
-func (m *gameMonitor) resubscribeFunction(ctx context.Context) event.ResubscribeErrFunc {
-	return func(innerCtx context.Context, err error) (event.Subscription, error) {
+func (m *gameMonitor) resubscribeFunction() event.ResubscribeErrFunc {
+	// The ctx is cancelled as soon as the subscription is returned,
+	// but is only used to create the subscription, and does not affect the returned subscription.
+	return func(ctx context.Context, err error) (event.Subscription, error) {
 		if err != nil {
 			m.logger.Warn("resubscribing after failed L1 subscription", "err", err)
 		}
 		return eth.WatchHeadChanges(ctx, m.l1Source, m.onNewL1Head)
 	}
 }
 
-func (m *gameMonitor) MonitorGames(ctx context.Context) error {
-	m.l1HeadsSub = event.ResubscribeErr(time.Second*10, m.resubscribeFunction(ctx))
-	for {
-		select {
-		case <-ctx.Done():
-			m.l1HeadsSub.Unsubscribe()
-			return nil
-		case err, ok := <-m.l1HeadsSub.Err():
-			if !ok {
-				return err
-			}
-			m.logger.Error("L1 subscription error", "err", err)
-		}
+func (m *gameMonitor) StartMonitoring() {
+	m.runState.Lock()
+	defer m.runState.Unlock()
+	if m.l1HeadsSub != nil {
+		return // already started
+	}
+	m.l1HeadsSub = event.ResubscribeErr(time.Second*10, m.resubscribeFunction())
+}
+
+func (m *gameMonitor) StopMonitoring() {
+	m.runState.Lock()
+	defer m.runState.Unlock()
+	if m.l1HeadsSub == nil {
+		return // already stopped
 	}
+	m.l1HeadsSub.Unsubscribe()
+	m.l1HeadsSub = nil
 }

op-challenger/game/monitor_test.go

@@ -84,8 +84,9 @@ func TestMonitorGames(t *testing.T) {
 			cancel()
 		}()
 
-		err := monitor.MonitorGames(ctx)
-		require.NoError(t, err)
+		monitor.StartMonitoring()
+		<-ctx.Done()
+		monitor.StopMonitoring()
 		require.Len(t, sched.scheduled, 1)
 		require.Equal(t, []common.Address{addr1, addr2}, sched.scheduled[0])
 	})
@@ -129,8 +130,9 @@ func TestMonitorGames(t *testing.T) {
 			cancel()
 		}()
 
-		err := monitor.MonitorGames(ctx)
-		require.NoError(t, err)
+		monitor.StartMonitoring()
+		<-ctx.Done()
+		monitor.StopMonitoring()
 		require.NotEmpty(t, sched.scheduled) // We might get more than one update scheduled.
 		require.Equal(t, []common.Address{addr1, addr2}, sched.scheduled[0])
 	})