fix(cli): wait for the service manager before exiting after stop

On Windows, `ctrld stop` in Intercept mode sometimes ended with SCM event
7031 and an automatic restart, which put the NRPT catch-all back (#652).

Service Stop closes stopCh and returns; kardianos/service then reports
SERVICE_STOPPED from the s.Run goroutine. run() returned as soon as
finishRun completed, and main called os.Exit(0) right away. If that beat
s.Run, the process was gone while the service was still StopPending, and
SCM treated it as a crash.

The ordering race is older, which fits #492. 9ad42622 (#627) moved the
control-server Shutdown from finishRun into Stop, so finishRun no longer
spends that time before the process exits, and the exit wins far more
often. Lab, service-stop-start Intercept: 1.5.8 0/21, 633689c 13/23,
633689c without the early call 0/15.

Keep the early control-server stop that #627 needs, and make run() wait,
bounded at 5 s, for s.Run to return before the process exits.

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:

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")
+	}
+}