diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index fea6155..edd5837 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -51,6 +51,9 @@ const ( oldBinSuffix = "_previous" msgExit = "$$EXIT$$" shutdownTimeout = 10 * time.Second + // serviceRunExitTimeout bounds how long a finished run waits for s.Run + // to report the stop to the service manager. + serviceRunExitTimeout = 5 * time.Second ) var ( @@ -281,6 +284,9 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { return } + // serviceRunDone closes when s.Run returns, so the process exits only after + // the service manager has the final status. Nil when there is no s.Run. + var serviceRunDone chan struct{} switch { case isMobile(): defer p.finishRun() @@ -290,9 +296,11 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { mainLog.Load().Fatal().Err(err).Msg("failed to start ctrld") } case !daemon: + serviceRunDone = make(chan struct{}) // We need to call s.Run() as soon as possible to response to the OS manager, so it // can see ctrld is running and don't mark ctrld as failed service. go func() { + defer close(serviceRunDone) s, err := newService(p, svcConfig) if err != nil { failRunUnclassified(mainLog.Load().Error().Err(err), fmt.Sprintf("failed to create new service: %v", err), p.notifyExitToLogServer) @@ -534,6 +542,27 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { if !isMobile() { p.finishRun() } + waitServiceRun(serviceRunDone, serviceRunExitTimeout) +} + +// waitServiceRun waits for s.Run to return before run lets the process exit. +// +// Service Stop closes stopCh before it returns, and on Windows the service +// manager records the service as stopped only after Stop returns and s.Run +// reports it. A process that exits in between is a crash to the service +// manager: it logs event 7031 and restarts the service, which applies its DNS +// rules again after `ctrld stop` (#652). done is nil when nothing runs s.Run. +// The wait is bounded so a service manager that never answers cannot keep a +// finished run alive. +func waitServiceRun(done <-chan struct{}, timeout time.Duration) { + if done == nil { + return + } + select { + case <-done: + case <-time.After(timeout): + mainLog.Load().Warn().Msgf("service manager did not finish the stop within %s", timeout) + } } // configFileForWrite returns the config file that writeConfigFile writes: diff --git a/cmd/cli/wait_service_run_test.go b/cmd/cli/wait_service_run_test.go new file mode 100644 index 0000000..4de9225 --- /dev/null +++ b/cmd/cli/wait_service_run_test.go @@ -0,0 +1,51 @@ +package cli + +import ( + "testing" + "time" +) + +// A run must not return, and so let the process exit, while s.Run is still +// reporting the stop to the service manager (#652). +func TestWaitServiceRunWaitsForServiceRun(t *testing.T) { + done := make(chan struct{}) + returned := make(chan struct{}) + go func() { + waitServiceRun(done, time.Minute) + close(returned) + }() + + select { + case <-returned: + t.Fatal("waitServiceRun returned before s.Run finished") + case <-time.After(100 * time.Millisecond): + } + + close(done) + select { + case <-returned: + case <-time.After(5 * time.Second): + t.Fatal("waitServiceRun did not return after s.Run finished") + } +} + +func TestWaitServiceRunGivesUpAfterTimeout(t *testing.T) { + start := time.Now() + waitServiceRun(make(chan struct{}), 50*time.Millisecond) + if elapsed := time.Since(start); elapsed < 50*time.Millisecond { + t.Fatalf("waitServiceRun returned after %s, before its timeout", elapsed) + } +} + +func TestWaitServiceRunWithoutServiceRun(t *testing.T) { + returned := make(chan struct{}) + go func() { + waitServiceRun(nil, time.Minute) + close(returned) + }() + select { + case <-returned: + case <-time.After(5 * time.Second): + t.Fatal("waitServiceRun waited although nothing runs s.Run") + } +}