From 3d719302a05abc9259a5aa2805fe1c5e1c726da3 Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 24 Aug 2026 22:24:14 -0700 Subject: [PATCH 1/3] code review updates --- cmd/lanternd/lanternd.go | 10 +- cmd/lanternd/lanternd_windows.go | 129 ++++++++++++-- cmd/lanternd/lanternd_windows_test.go | 241 ++++++++++++++++++++++++++ 3 files changed, 359 insertions(+), 21 deletions(-) create mode 100644 cmd/lanternd/lanternd_windows_test.go diff --git a/cmd/lanternd/lanternd.go b/cmd/lanternd/lanternd.go index 5b058515..4d2002a0 100644 --- a/cmd/lanternd/lanternd.go +++ b/cmd/lanternd/lanternd.go @@ -240,6 +240,11 @@ type childProcess struct { logger *slog.Logger } +const ( + daemonRestartBackoffMax = 60 * time.Second + daemonRestartBackoffResetAfter = 2 * time.Minute +) + // spawnChild creates and starts a daemon child process with piped I/O. The child's stdout and // stderr are merged and drained through the provided logger (or os.Stdout as fallback). func spawnChild(args []string, dataPath, logPath, logLevel string) (*childProcess, error) { @@ -345,8 +350,7 @@ func babysit(args []string, dataPath, logPath, logLevel string) error { signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) stopping := false - const resetAfter = 2 * time.Minute // reset backoff if child ran longer than this - bo := common.NewBackoff(60 * time.Second) + bo := common.NewBackoff(daemonRestartBackoffMax) for { child, err := spawnChild(args, dataPath, logPath, logLevel) @@ -384,7 +388,7 @@ func babysit(args []string, dataPath, logPath, logLevel string) error { } // Reset backoff if the child ran for a while (i.e. it wasn't a fast crash loop). - if time.Since(startedAt) > resetAfter { + if time.Since(startedAt) > daemonRestartBackoffResetAfter { bo.Reset() } diff --git a/cmd/lanternd/lanternd_windows.go b/cmd/lanternd/lanternd_windows.go index f5a33219..2ae06514 100644 --- a/cmd/lanternd/lanternd_windows.go +++ b/cmd/lanternd/lanternd_windows.go @@ -75,7 +75,24 @@ func install(dataPath, logPath, logLevel string, environment daemonEnvironment) } defer service.Close() - err = service.SetRecoveryActions([]mgr.RecoveryAction{ + if err := configureWindowsServiceRecovery(service); err != nil { + return err + } + if err := service.Start(); err != nil { + return fmt.Errorf("failed to start service: %w", err) + } + + slog.Info("Windows service installed successfully") + return nil +} + +type windowsServiceRecoveryConfigurer interface { + SetRecoveryActions([]mgr.RecoveryAction, uint32) error + SetRecoveryActionsOnNonCrashFailures(bool) error +} + +func configureWindowsServiceRecovery(service windowsServiceRecoveryConfigurer) error { + if err := service.SetRecoveryActions([]mgr.RecoveryAction{ {Type: mgr.ServiceRestart, Delay: 1 * time.Second}, {Type: mgr.ServiceRestart, Delay: 2 * time.Second}, {Type: mgr.ServiceRestart, Delay: 4 * time.Second}, @@ -83,15 +100,12 @@ func install(dataPath, logPath, logLevel string, environment daemonEnvironment) {Type: mgr.ServiceRestart, Delay: 16 * time.Second}, {Type: mgr.ServiceRestart, Delay: 32 * time.Second}, {Type: mgr.ServiceRestart, Delay: 64 * time.Second}, - }, 60) - if err != nil { + }, 60); err != nil { return fmt.Errorf("failed to set service recovery actions: %w", err) } - if err := service.Start(); err != nil { - return fmt.Errorf("failed to start service: %w", err) + if err := service.SetRecoveryActionsOnNonCrashFailures(true); err != nil { + return fmt.Errorf("failed to enable recovery actions for non-crash failures: %w", err) } - - slog.Info("Windows service installed successfully") return nil } @@ -152,10 +166,51 @@ func maybePlatformService() bool { return true } -type service struct{} +type windowsServiceChild interface { + Done() <-chan error + RequestShutdown() + WaitOrKill(time.Duration) error + HandleCrash(error) + info(string, ...any) +} + +type windowsServiceChildProcess struct { + *childProcess +} + +func (c *windowsServiceChildProcess) info(message string, args ...any) { + c.logger.Info(message, args...) +} + +type windowsServiceBackoff interface { + Wait(context.Context) + Reset() +} + +type service struct { + spawnChild func([]string, string, string, string) (windowsServiceChild, error) + newBackoff func() windowsServiceBackoff +} + +// newWindowsService returns a production service handler. Its injected process and backoff +// functions keep supervision tests independent of OS processes and wall-clock delays. +func newWindowsService() *service { + return &service{ + spawnChild: func(args []string, dataPath, logPath, logLevel string) (windowsServiceChild, error) { + child, err := spawnChild(args, dataPath, logPath, logLevel) + if err != nil { + return nil, err + } + return &windowsServiceChildProcess{childProcess: child}, nil + }, + newBackoff: func() windowsServiceBackoff { + return common.NewBackoff(daemonRestartBackoffMax) + }, + } +} func startWindowsService() error { - return svc.Run(serviceName, &service{}) + return svc.Run(serviceName, newWindowsService()) } func (s *service) Execute(args []string, r <-chan svc.ChangeRequest, status chan<- svc.Status) (bool, uint32) { @@ -170,33 +225,71 @@ func (s *service) Execute(args []string, r <-chan svc.ChangeRequest, status chan slog.Error("Failed to parse service arguments", "error", err) return true, 1 } + return s.run(config, r, status) +} - // Run the daemon as a child process so we can clean up network state if it crashes, - // regardless of whether the SCM is configured to restart the service. +// run supervises the daemon child so crash cleanup and restart do not depend on SCM recovery. +func (s *service) run(config serviceRunConfig, r <-chan svc.ChangeRequest, status chan<- svc.Status) (bool, uint32) { childArgs := config.args() - child, err := spawnChild(childArgs, config.dataPath, config.logPath, config.logLevel) + child, err := s.spawnChild(childArgs, config.dataPath, config.logPath, config.logLevel) if err != nil { slog.Error("Failed to start daemon", "error", err) return true, 1 } status <- svc.Status{State: svc.Running, Accepts: svc.AcceptStop | svc.AcceptShutdown} - child.logger.Info("Running as Windows service") + child.info("Running as Windows service") + + backoff := s.newBackoff() + startedAt := time.Now() + childDone := child.Done() + var restartReady <-chan struct{} + serviceContext, cancelService := context.WithCancel(context.Background()) + defer cancelService() for { select { - case err := <-child.Done(): + case err := <-childDone: if err != nil { child.HandleCrash(err) } - return true, 1 + if time.Since(startedAt) > daemonRestartBackoffResetAfter { + backoff.Reset() + } + child.info("Restarting daemon process") + child = nil + childDone = nil + + restartDone := make(chan struct{}) + restartReady = restartDone + go func() { + backoff.Wait(serviceContext) + close(restartDone) + }() + case <-restartReady: + restartReady = nil + + child, err = s.spawnChild(childArgs, config.dataPath, config.logPath, config.logLevel) + if err != nil { + slog.Error("Failed to restart daemon", "error", err) + return true, 1 + } + startedAt = time.Now() + childDone = child.Done() + child.info("Running as Windows service") case change := <-r: switch change.Cmd { case svc.Stop, svc.Shutdown: status <- svc.Status{State: svc.StopPending} - child.logger.Info("Service stop requested") - child.RequestShutdown() - child.WaitOrKill(15 * time.Second) + cancelService() + if restartReady != nil { + <-restartReady + } + if child != nil { + child.info("Service stop requested") + child.RequestShutdown() + child.WaitOrKill(15 * time.Second) + } return false, windows.NO_ERROR case svc.Interrogate: status <- change.CurrentStatus diff --git a/cmd/lanternd/lanternd_windows_test.go b/cmd/lanternd/lanternd_windows_test.go new file mode 100644 index 00000000..5f2cfcb6 --- /dev/null +++ b/cmd/lanternd/lanternd_windows_test.go @@ -0,0 +1,241 @@ +package main + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" + "golang.org/x/sys/windows/svc" + "golang.org/x/sys/windows/svc/mgr" +) + +type fakeWindowsServiceChild struct { + done chan error + crashes chan error + shutdowns chan struct{} + waits chan time.Duration +} + +func newFakeWindowsServiceChild() *fakeWindowsServiceChild { + return &fakeWindowsServiceChild{ + done: make(chan error, 1), + crashes: make(chan error, 1), + shutdowns: make(chan struct{}, 1), + waits: make(chan time.Duration, 1), + } +} + +func (c *fakeWindowsServiceChild) Done() <-chan error { + return c.done +} + +func (c *fakeWindowsServiceChild) RequestShutdown() { + c.shutdowns <- struct{}{} +} + +func (c *fakeWindowsServiceChild) WaitOrKill(timeout time.Duration) error { + c.waits <- timeout + return nil +} + +func (c *fakeWindowsServiceChild) HandleCrash(err error) { + c.crashes <- err +} + +func (c *fakeWindowsServiceChild) info(string, ...any) {} + +type immediateWindowsServiceBackoff struct{} + +func (immediateWindowsServiceBackoff) Wait(context.Context) {} +func (immediateWindowsServiceBackoff) Reset() {} + +type blockingWindowsServiceBackoff struct { + started chan struct{} + stopped chan struct{} + resets chan struct{} +} + +func (b *blockingWindowsServiceBackoff) Wait(ctx context.Context) { + b.started <- struct{}{} + <-ctx.Done() + b.stopped <- struct{}{} +} + +func (b *blockingWindowsServiceBackoff) Reset() { + b.resets <- struct{}{} +} + +type windowsServiceResult struct { + serviceSpecificExitCode bool + exitCode uint32 +} + +func TestWindowsServiceRestartsExitedChildren(t *testing.T) { + children := []*fakeWindowsServiceChild{ + newFakeWindowsServiceChild(), + newFakeWindowsServiceChild(), + newFakeWindowsServiceChild(), + } + spawned := make(chan *fakeWindowsServiceChild, len(children)) + spawnIndex := 0 + windowsService := &service{ + spawnChild: func([]string, string, string, string) (windowsServiceChild, error) { + child := children[spawnIndex] + spawnIndex++ + spawned <- child + return child, nil + }, + newBackoff: func() windowsServiceBackoff { return immediateWindowsServiceBackoff{} }, + } + + requests := make(chan svc.ChangeRequest, 1) + statuses := make(chan svc.Status, 2) + result := executeWindowsService(windowsService, requests, statuses) + + require.Same(t, children[0], receive(t, spawned)) + require.Equal(t, svc.Running, receive(t, statuses).State) + + crashErr := errors.New("daemon crashed") + children[0].done <- crashErr + require.ErrorIs(t, receive(t, children[0].crashes), crashErr) + require.Same(t, children[1], receive(t, spawned)) + + children[1].done <- nil + require.Same(t, children[2], receive(t, spawned)) + select { + case err := <-children[1].crashes: + t.Fatalf("clean child exit handled as crash: %v", err) + default: + } + + requests <- svc.ChangeRequest{Cmd: svc.Stop} + require.Equal(t, svc.StopPending, receive(t, statuses).State) + receive(t, children[2].shutdowns) + require.Equal(t, 15*time.Second, receive(t, children[2].waits)) + require.Equal(t, windowsServiceResult{false, windows.NO_ERROR}, receive(t, result)) +} + +func TestWindowsServiceStopsDuringRestartBackoff(t *testing.T) { + child := newFakeWindowsServiceChild() + spawned := make(chan *fakeWindowsServiceChild, 2) + backoff := &blockingWindowsServiceBackoff{ + started: make(chan struct{}, 1), + stopped: make(chan struct{}, 1), + resets: make(chan struct{}, 1), + } + windowsService := &service{ + spawnChild: func([]string, string, string, string) (windowsServiceChild, error) { + spawned <- child + return child, nil + }, + newBackoff: func() windowsServiceBackoff { return backoff }, + } + + requests := make(chan svc.ChangeRequest, 1) + statuses := make(chan svc.Status, 2) + result := executeWindowsService(windowsService, requests, statuses) + + require.Same(t, child, receive(t, spawned)) + require.Equal(t, svc.Running, receive(t, statuses).State) + + child.done <- errors.New("daemon crashed") + receive(t, child.crashes) + receive(t, backoff.started) + requests <- svc.ChangeRequest{Cmd: svc.Stop} + require.Equal(t, svc.StopPending, receive(t, statuses).State) + receive(t, backoff.stopped) + require.Equal(t, windowsServiceResult{false, windows.NO_ERROR}, receive(t, result)) + select { + case unexpected := <-spawned: + t.Fatalf("spawned child during service stop: %p", unexpected) + default: + } +} + +func TestWindowsServiceReturnsFailureWhenChildCannotRestart(t *testing.T) { + child := newFakeWindowsServiceChild() + spawnCalls := 0 + restartErr := errors.New("restart failed") + windowsService := &service{ + spawnChild: func([]string, string, string, string) (windowsServiceChild, error) { + spawnCalls++ + if spawnCalls == 1 { + return child, nil + } + return nil, restartErr + }, + newBackoff: func() windowsServiceBackoff { return immediateWindowsServiceBackoff{} }, + } + + requests := make(chan svc.ChangeRequest, 1) + statuses := make(chan svc.Status, 1) + result := executeWindowsService(windowsService, requests, statuses) + + require.Equal(t, svc.Running, receive(t, statuses).State) + child.done <- restartErr + require.ErrorIs(t, receive(t, child.crashes), restartErr) + require.Equal(t, windowsServiceResult{true, 1}, receive(t, result)) + require.Equal(t, 2, spawnCalls) +} + +type fakeWindowsServiceRecoveryConfigurer struct { + actions []mgr.RecoveryAction + resetPeriod uint32 + nonCrashFailure bool +} + +func (f *fakeWindowsServiceRecoveryConfigurer) SetRecoveryActions(actions []mgr.RecoveryAction, resetPeriod uint32) error { + f.actions = actions + f.resetPeriod = resetPeriod + return nil +} + +func (f *fakeWindowsServiceRecoveryConfigurer) SetRecoveryActionsOnNonCrashFailures(flag bool) error { + f.nonCrashFailure = flag + return nil +} + +func TestConfigureWindowsServiceRecovery(t *testing.T) { + configured := &fakeWindowsServiceRecoveryConfigurer{} + require.NoError(t, configureWindowsServiceRecovery(configured)) + require.Equal(t, uint32(60), configured.resetPeriod) + require.Equal(t, []mgr.RecoveryAction{ + {Type: mgr.ServiceRestart, Delay: 1 * time.Second}, + {Type: mgr.ServiceRestart, Delay: 2 * time.Second}, + {Type: mgr.ServiceRestart, Delay: 4 * time.Second}, + {Type: mgr.ServiceRestart, Delay: 8 * time.Second}, + {Type: mgr.ServiceRestart, Delay: 16 * time.Second}, + {Type: mgr.ServiceRestart, Delay: 32 * time.Second}, + {Type: mgr.ServiceRestart, Delay: 64 * time.Second}, + }, configured.actions) + require.True(t, configured.nonCrashFailure) +} + +func executeWindowsService(windowsService *service, requests chan svc.ChangeRequest, statuses chan svc.Status) <-chan windowsServiceResult { + result := make(chan windowsServiceResult, 1) + go func() { + serviceSpecificExitCode, exitCode := windowsService.run(serviceRunConfig{ + dataPath: `C:\ProgramData\Lantern`, + logPath: `C:\ProgramData\Lantern`, + logLevel: "debug", + environment: daemonEnvironmentProd, + }, requests, statuses) + result <- windowsServiceResult{serviceSpecificExitCode, exitCode} + }() + return result +} + +func receive[T any](t *testing.T, values <-chan T) T { + t.Helper() + select { + case value := <-values: + return value + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for value") + var zero T + return zero + } +} From 020492bb11893594f035ed191eada48d605e7abf Mon Sep 17 00:00:00 2001 From: atavism Date: Tue, 25 Aug 2026 13:36:26 -0700 Subject: [PATCH 2/3] code review updates --- cmd/lanternd/lanternd.go | 2 +- cmd/lanternd/lanternd_windows.go | 4 +++- cmd/lanternd/lanternd_windows_test.go | 34 ++++++++++++++++++++++++++- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/cmd/lanternd/lanternd.go b/cmd/lanternd/lanternd.go index 4d2002a0..2c557a00 100644 --- a/cmd/lanternd/lanternd.go +++ b/cmd/lanternd/lanternd.go @@ -340,7 +340,7 @@ func (c *childProcess) HandleCrash(err error) { // babysit runs the daemon as a child process and monitors it. If the child exits unexpectedly // (crash, panic, etc.), the parent immediately cleans up any stale VPN network state and -// automatically restarts the child process with exponential backoff. +// automatically restarts the child process with quadratic backoff and jitter. // // Graceful shutdown is signaled by closing the child's stdin pipe — this works cross-platform, // including inside a Windows service where there is no console for signal delivery. diff --git a/cmd/lanternd/lanternd_windows.go b/cmd/lanternd/lanternd_windows.go index 2ae06514..8370921a 100644 --- a/cmd/lanternd/lanternd_windows.go +++ b/cmd/lanternd/lanternd_windows.go @@ -288,7 +288,9 @@ func (s *service) run(config serviceRunConfig, r <-chan svc.ChangeRequest, statu if child != nil { child.info("Service stop requested") child.RequestShutdown() - child.WaitOrKill(15 * time.Second) + if err := child.WaitOrKill(15 * time.Second); err != nil { + slog.Warn("Daemon process did not stop cleanly", "error", err) + } } return false, windows.NO_ERROR case svc.Interrogate: diff --git a/cmd/lanternd/lanternd_windows_test.go b/cmd/lanternd/lanternd_windows_test.go index 5f2cfcb6..d5a6d5d4 100644 --- a/cmd/lanternd/lanternd_windows_test.go +++ b/cmd/lanternd/lanternd_windows_test.go @@ -1,8 +1,10 @@ package main import ( + "bytes" "context" "errors" + "log/slog" "testing" "time" @@ -17,6 +19,7 @@ type fakeWindowsServiceChild struct { crashes chan error shutdowns chan struct{} waits chan time.Duration + waitErr error } func newFakeWindowsServiceChild() *fakeWindowsServiceChild { @@ -38,7 +41,7 @@ func (c *fakeWindowsServiceChild) RequestShutdown() { func (c *fakeWindowsServiceChild) WaitOrKill(timeout time.Duration) error { c.waits <- timeout - return nil + return c.waitErr } func (c *fakeWindowsServiceChild) HandleCrash(err error) { @@ -155,6 +158,35 @@ func TestWindowsServiceStopsDuringRestartBackoff(t *testing.T) { } } +func TestWindowsServiceLogsChildShutdownError(t *testing.T) { + var logs bytes.Buffer + previousLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previousLogger) }) + + shutdownErr := errors.New("child exited unsuccessfully") + child := newFakeWindowsServiceChild() + child.waitErr = shutdownErr + windowsService := &service{ + spawnChild: func([]string, string, string, string) (windowsServiceChild, error) { + return child, nil + }, + newBackoff: func() windowsServiceBackoff { return immediateWindowsServiceBackoff{} }, + } + + requests := make(chan svc.ChangeRequest, 1) + statuses := make(chan svc.Status, 2) + result := executeWindowsService(windowsService, requests, statuses) + + require.Equal(t, svc.Running, receive(t, statuses).State) + requests <- svc.ChangeRequest{Cmd: svc.Stop} + require.Equal(t, svc.StopPending, receive(t, statuses).State) + require.Equal(t, 15*time.Second, receive(t, child.waits)) + require.Equal(t, windowsServiceResult{false, windows.NO_ERROR}, receive(t, result)) + require.Contains(t, logs.String(), "Daemon process did not stop cleanly") + require.Contains(t, logs.String(), shutdownErr.Error()) +} + func TestWindowsServiceReturnsFailureWhenChildCannotRestart(t *testing.T) { child := newFakeWindowsServiceChild() spawnCalls := 0 From 219be6e5f26c5cb1df231e574f259b0634f2172b Mon Sep 17 00:00:00 2001 From: atavism Date: Tue, 25 Aug 2026 13:45:22 -0700 Subject: [PATCH 3/3] windows: report service stop wait hint --- cmd/lanternd/lanternd_windows.go | 13 +++++++++++-- cmd/lanternd/lanternd_windows_test.go | 9 ++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/cmd/lanternd/lanternd_windows.go b/cmd/lanternd/lanternd_windows.go index 8370921a..935303b3 100644 --- a/cmd/lanternd/lanternd_windows.go +++ b/cmd/lanternd/lanternd_windows.go @@ -18,6 +18,11 @@ import ( const ( serviceName = "LanternSvc" binPath = "C:\\Program Files\\Lantern\\" + serviceName + ".exe" + + // windowsServiceChildShutdownTimeout bounds the child's graceful shutdown period. + windowsServiceChildShutdownTimeout = 15 * time.Second + // windowsServiceStopWaitHint includes headroom for forced termination after that period. + windowsServiceStopWaitHint = 20 * time.Second ) var isWindowsService bool @@ -280,7 +285,11 @@ func (s *service) run(config serviceRunConfig, r <-chan svc.ChangeRequest, statu case change := <-r: switch change.Cmd { case svc.Stop, svc.Shutdown: - status <- svc.Status{State: svc.StopPending} + status <- svc.Status{ + State: svc.StopPending, + CheckPoint: 1, + WaitHint: uint32(windowsServiceStopWaitHint / time.Millisecond), + } cancelService() if restartReady != nil { <-restartReady @@ -288,7 +297,7 @@ func (s *service) run(config serviceRunConfig, r <-chan svc.ChangeRequest, statu if child != nil { child.info("Service stop requested") child.RequestShutdown() - if err := child.WaitOrKill(15 * time.Second); err != nil { + if err := child.WaitOrKill(windowsServiceChildShutdownTimeout); err != nil { slog.Warn("Daemon process did not stop cleanly", "error", err) } } diff --git a/cmd/lanternd/lanternd_windows_test.go b/cmd/lanternd/lanternd_windows_test.go index d5a6d5d4..f5e04f55 100644 --- a/cmd/lanternd/lanternd_windows_test.go +++ b/cmd/lanternd/lanternd_windows_test.go @@ -115,9 +115,12 @@ func TestWindowsServiceRestartsExitedChildren(t *testing.T) { } requests <- svc.ChangeRequest{Cmd: svc.Stop} - require.Equal(t, svc.StopPending, receive(t, statuses).State) + stopStatus := receive(t, statuses) + require.Equal(t, svc.StopPending, stopStatus.State) + require.Equal(t, uint32(1), stopStatus.CheckPoint) + require.Equal(t, uint32(windowsServiceStopWaitHint/time.Millisecond), stopStatus.WaitHint) receive(t, children[2].shutdowns) - require.Equal(t, 15*time.Second, receive(t, children[2].waits)) + require.Equal(t, windowsServiceChildShutdownTimeout, receive(t, children[2].waits)) require.Equal(t, windowsServiceResult{false, windows.NO_ERROR}, receive(t, result)) } @@ -181,7 +184,7 @@ func TestWindowsServiceLogsChildShutdownError(t *testing.T) { require.Equal(t, svc.Running, receive(t, statuses).State) requests <- svc.ChangeRequest{Cmd: svc.Stop} require.Equal(t, svc.StopPending, receive(t, statuses).State) - require.Equal(t, 15*time.Second, receive(t, child.waits)) + require.Equal(t, windowsServiceChildShutdownTimeout, receive(t, child.waits)) require.Equal(t, windowsServiceResult{false, windows.NO_ERROR}, receive(t, result)) require.Contains(t, logs.String(), "Daemon process did not stop cleanly") require.Contains(t, logs.String(), shutdownErr.Error())