Merge pull request 'Wait for a stopped bundle's exit instead of sleeping after it (issue 476)' (#30) from fix/476-wait-for-the-exit into main

node-tools/internal/launch/launch.go

@@ -107,6 +107,10 @@ var (
 	RestartSettle = time.Minute
 )
 
+// StopWait bounds how long Launched.Stop waits for its bundle's exit to be handled; a bundle that
+// ignores SIGTERM longer than this is left to exit on its own (novox/hq issue 476).
+var StopWait = 5 * time.Second
+
 // Launched is a running bundle: what it registered, and how to stop it.
 type Launched struct {
 	Registrations []Registration
@@ -143,6 +147,9 @@ type child struct {
 	pending map[int64]chan message
 	next    int64
 	dead    chan struct{}
+	// handled is closed once the child's exit has been handled whole — its state cleared and its
+	// restart decided — which Stop waits for (novox/hq issue 476).
+	handled chan struct{}
 	why     error
 	// watches are the state watches this child asked for, stopped when it exits (ADR 0201): a child
 	// that starts again watches again, as its code runs again.
@@ -195,7 +202,10 @@ func Start(module, entry string, env []string, mesh Bus, logf func(string, ...an
 	}
 	var restart func(after time.Duration)
 	var startedAt time.Time
-	pause := RestartFirst
+	// The restart pauses are read once, here: the exit handling runs on its own goroutine and must not
+	// read the package's settings while a caller (a test) changes them (novox/hq issue 476).
+	pauseFirst, pauseMost, settle := RestartFirst, RestartMost, RestartSettle
+	pause := pauseFirst
 
 	start := func() (*child, error) {
 		cmd := exec.Command(entry)
@@ -215,7 +225,8 @@ func Start(module, entry string, env []string, mesh Bus, logf func(string, ...an
 		if err := cmd.Start(); err != nil {
 			return nil, err
 		}
-		c := &child{cmd: cmd, stdin: stdin, pending: map[int64]chan message{}, next: 1, dead: make(chan struct{})}
+		c := &child{cmd: cmd, stdin: stdin, pending: map[int64]chan message{}, next: 1, dead: make(chan struct{}),
+			handled: make(chan struct{})}
 		var lastSaid string
 		var saidMu sync.Mutex
 		stderrDone := make(chan struct{})
@@ -237,6 +248,7 @@ func Start(module, entry string, env []string, mesh Bus, logf func(string, ...an
 			}
 		}()
 		go func() {
+			defer close(c.handled)
 			scan := bufio.NewScanner(stdout)
 			scan.Buffer(make([]byte, 64*1024), 16<<20)
 			for scan.Scan() {
@@ -387,14 +399,14 @@ func Start(module, entry string, env []string, mesh Bus, logf func(string, ...an
 			}
 			wasStopped := stopped
 			// Started again at once if it ran a while; after a growing pause while it keeps exiting.
-			wait := RestartFirst
-			if time.Since(startedAt) < RestartSettle {
+			wait := pauseFirst
+			if time.Since(startedAt) < settle {
 				wait = pause
-				if pause *= 2; pause > RestartMost {
-					pause = RestartMost
+				if pause *= 2; pause > pauseMost {
+					pause = pauseMost
 				}
 			} else {
-				pause = RestartFirst
+				pause = pauseFirst
 			}
 			mu.Unlock()
 			if !wasStopped && wasServing {
@@ -451,8 +463,8 @@ func Start(module, entry string, env []string, mesh Bus, logf func(string, ...an
 			if err != nil {
 				if !stopped {
 					wait := pause
-					if pause *= 2; pause > RestartMost {
-						pause = RestartMost
+					if pause *= 2; pause > pauseMost {
+						pause = pauseMost
 					}
 					logf("[mesh-tools] %s's bundle did not start again: %v; trying in %s", module, err, wait)
 					go restart(wait)
@@ -557,6 +569,12 @@ func Start(module, entry string, env []string, mesh Bus, logf func(string, ...an
 		mu.Unlock()
 		if c != nil && c.cmd.Process != nil {
 			_ = c.cmd.Process.Signal(syscall.SIGTERM)
+			// Returns once the bundle's exit is handled, so nothing of it runs on after Stop
+			// (novox/hq issue 476); bounded, for a bundle that does not exit on SIGTERM.
+			select {
+			case <-c.handled:
+			case <-time.After(StopWait):
+			}
 		}
 	}}
 	for _, under := range order {

node-tools/internal/runtime/caller_test.go

@@ -27,7 +27,7 @@ func TestACallOnTheCallerNamedSubjectReachesTheModuleWithItsCaller(t *testing.T)
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer settled(stop)
+	defer stop() // returns once the bundle's exit is handled (novox/hq issue 476)
 	asker, err := bus.Connect(bus.Credential{URL: mt.URL(t), Module: "node-tools", Node: "shanks",
 		User: "shanks.node-tools", Password: "unused"})
 	if err != nil {
@@ -65,7 +65,7 @@ func TestTheSubjectThatNamesNoCallerStillWorksAndNamesNone(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer settled(stop)
+	defer stop() // returns once the bundle's exit is handled (novox/hq issue 476)
 	old, err := nats.Connect(mt.URL(t))
 	if err != nil {
 		t.Fatal(err)
@@ -89,11 +89,3 @@ func TestTheSubjectThatNamesNoCallerStillWorksAndNamesNone(t *testing.T) {
 		}
 	}
 }
-
-// settled stops a runtime and waits for its bundle's exit to be handled: the launcher reads its restart pauses
-// (launch.RestartFirst) as a bundle exits, and a later test in this package sets them, so a test that leaves its
-// bundle exiting behind it races the next one.
-func settled(stop func()) {
-	stop()
-	time.Sleep(500 * time.Millisecond)
-}