fix: stop orphaned mock on healthcheck timeout

cmd/proxy_test.go

@@ -93,7 +93,7 @@ func Test_proxyUpstream(t *testing.T) {
 			go func() {
 				proxyUpstream(upstream, port, outputDir, tt.args.rewrite, false, tt.args.options)
 			}()
-			if up := engine.WaitUntilUp(port, nil); !up {
+			if up, _ := engine.WaitUntilUp(port, nil); !up {
 				t.Fatalf("proxy did not come up on port %d", port)
 			}
 
@@ -122,7 +122,7 @@ func Test_proxyUpstream(t *testing.T) {
 				indexFileName = "GET-index.txt"
 			}
 
-			if cfgExists := engine.WaitForOp(fmt.Sprintf("config file: %s", cfgFileName), 10*time.Second, nil, func() bool {
+			if cfgExists, _ := engine.WaitForOp(fmt.Sprintf("config file: %s", cfgFileName), 10*time.Second, nil, func() bool {
 				if _, err = os.Stat(path.Join(outputDir, cfgFileName)); err != nil {
 					return false
 				}
@@ -131,7 +131,7 @@ func Test_proxyUpstream(t *testing.T) {
 				t.Fatalf("config file not found")
 			}
 
-			if indexExists := engine.WaitForOp(fmt.Sprintf("index file: %s", indexFileName), 10*time.Second, nil, func() bool {
+			if indexExists, _ := engine.WaitForOp(fmt.Sprintf("index file: %s", indexFileName), 10*time.Second, nil, func() bool {
 				if _, err = os.Stat(path.Join(outputDir, indexFileName)); err != nil {
 					return false
 				}
@@ -155,7 +155,7 @@ func startUpstream() (server *http.Server, url string, port int, err error) {
 		server.ListenAndServe()
 	}()
 	url = fmt.Sprintf("http://localhost:%d", port)
-	if up := engine.WaitForUrl("upstream", url, nil); !up {
+	if up, _ := engine.WaitForUrl("upstream", url, nil); !up {
 		return nil, "", 0, fmt.Errorf("failed to start upstream on port %d", port)
 	}
 	return server, url, port, nil

cmd/up.go

@@ -30,6 +30,7 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"syscall"
 )
 
@@ -216,27 +217,44 @@ func start(lib *engine.EngineLibrary, startOptions engine.StartOptions, configDi
 	mockEngine := provider.Build(configDir, startOptions)
 
 	wg := &sync.WaitGroup{}
+	var interrupted atomic.Bool
 
 	if startOptions.IsDetached() {
 		// DetachHealthy still traps Ctrl+C so an abort during the
 		// healthcheck wait stops the mock; DetachNow returns immediately
 		// so there is nothing to interrupt.
 		if startOptions.Detach == engine.DetachHealthy {
-			trapExit(mockEngine, wg)
+			trapExit(mockEngine, wg, &interrupted)
 		}
 		if !mockEngine.Start(wg) {
-			// healthcheck timeout already calls logger.Fatalf; reaching
-			// here means the wait was aborted (e.g. Ctrl+C)
+			// on a healthcheck timeout the engine has already stopped the
+			// mock it started; exit non-zero so the failed start is
+			// visible. A false return with interrupted set means the wait
+			// was aborted (e.g. Ctrl+C), which is a clean shutdown.
+			if !interrupted.Load() {
+				os.Exit(1)
+			}
 			return
 		}
 		printDetachSummary(mockEngine, startOptions)
 		return
 	}
 
-	trapExit(mockEngine, wg)
+	trapExit(mockEngine, wg, &interrupted)
 	success := mockEngine.Start(wg)
 
-	if success && restartOnChange {
+	if !success {
+		// wait for any in-progress cleanup (the engine stops a mock that
+		// timed out; Ctrl+C triggers StopImmediately) to finish before
+		// returning, so we never leak a running mock.
+		wg.Wait()
+		if !interrupted.Load() {
+			os.Exit(1)
+		}
+		return
+	}
+
+	if restartOnChange {
 		dirUpdated := fileutil.WatchDir(configDir)
 		go func() {
 			for {
@@ -259,12 +277,15 @@ func printDetachSummary(mockEngine engine.MockEngine, startOptions engine.StartO
 	logger.Info("use 'imposter ls' to list running mocks, or 'imposter down' to stop them")
 }
 
-// listen for an interrupt from the OS, then attempt engine cleanup
-func trapExit(mockEngine engine.MockEngine, wg *sync.WaitGroup) {
-	c := make(chan os.Signal)
+// listen for an interrupt from the OS, then attempt engine cleanup.
+// interrupted is set before cleanup begins so the caller can tell an
+// abort apart from a healthcheck timeout once Start returns.
+func trapExit(mockEngine engine.MockEngine, wg *sync.WaitGroup, interrupted *atomic.Bool) {
+	c := make(chan os.Signal, 1)
 	signal.Notify(c, os.Interrupt, syscall.SIGTERM)
 	go func() {
 		<-c
+		interrupted.Store(true)
 		println()
 		mockEngine.StopImmediately(wg)
 	}()

internal/engine/docker/engine.go

@@ -107,20 +107,38 @@ func (d *DockerMockEngine) startWithOptions(wg *sync.WaitGroup, options engine.S
 	case engine.DetachHealthy:
 		// wait for health but don't stream logs or reap - the container
 		// keeps running in dockerd after the CLI exits
-		return engine.WaitUntilUp(options.Port, d.shutDownC)
+		up, timedOut := engine.WaitUntilUp(options.Port, d.shutDownC)
+		if !up && timedOut {
+			d.stopAfterFailedStart(wg)
+		}
+		return up
 	default:
 		if err = streamLogsToStdIo(cli, ctx, containerId); err != nil {
 			logger.Warn(err)
 		}
-		up := engine.WaitUntilUp(options.Port, d.shutDownC)
+		up, timedOut := engine.WaitUntilUp(options.Port, d.shutDownC)
+		if !up {
+			if timedOut {
+				d.stopAfterFailedStart(wg)
+			}
+			return false
+		}
 
 		// watch in case container stops
 		go notifyOnStopBlocking(d, wg, containerId, cli, ctx)
 
-		return up
+		return true
 	}
 }
 
+// stopAfterFailedStart removes the container that was started but never
+// became healthy, so a failed `up` does not leave an orphaned container
+// running in dockerd.
+func (d *DockerMockEngine) stopAfterFailedStart(wg *sync.WaitGroup) {
+	logger.Warnf("stopping mock that did not become healthy")
+	d.Stop(wg)
+}
+
 func (d *DockerMockEngine) GetID() string {
 	if len(d.containerId) > 12 {
 		return d.containerId[:12]

internal/engine/health.go

@@ -67,7 +67,12 @@ func CheckMockStatus(port int) error {
 	return fmt.Errorf("healthcheck status was %d for mock at %s: %s", resp.StatusCode, url, err)
 }
 
-func WaitUntilUp(port int, shutDownC chan bool) (success bool) {
+// WaitUntilUp blocks until the mock on the given port reports healthy.
+// It returns success=true once healthy. If it returns success=false,
+// timedOut distinguishes a healthcheck timeout (true) from an external
+// abort via shutDownC, e.g. Ctrl+C (false), so callers can clean up a
+// mock that never became healthy.
+func WaitUntilUp(port int, shutDownC chan bool) (success bool, timedOut bool) {
 	url := getStatusUrl(port)
 	return WaitForUrl(fmt.Sprintf("status endpoint to return HTTP 200 at %v", url), url, shutDownC)
 }
@@ -76,7 +81,7 @@ func getStatusUrl(port int) string {
 	return fmt.Sprintf("http://localhost:%d/system/status", port)
 }
 
-func WaitForUrl(desc string, url string, abortC chan bool) (success bool) {
+func WaitForUrl(desc string, url string, abortC chan bool) (success bool, timedOut bool) {
 	return WaitForOp(desc, getStartTimeout(), abortC, func() bool {
 		resp, err := http.Get(url)
 		if err != nil {
@@ -90,7 +95,11 @@ func WaitForUrl(desc string, url string, abortC chan bool) (success bool) {
 	})
 }
 
-func WaitForOp(desc string, timeoutDuration time.Duration, abortC chan bool, operation func() bool) (success bool) {
+// WaitForOp polls operation until it succeeds, the timeout elapses, or an
+// abort is received on abortC. On timeout it returns (false, true) rather
+// than terminating the process, so the caller can stop any mock it started
+// before exiting; an abort returns (false, false).
+func WaitForOp(desc string, timeoutDuration time.Duration, abortC chan bool, operation func() bool) (success bool, timedOut bool) {
 	logger.Tracef("waiting for %s", desc)
 
 	successC := make(chan bool)
@@ -107,21 +116,16 @@ func WaitForOp(desc string, timeoutDuration time.Duration, abortC chan bool, ope
 		}
 	}()
 
-	finished := false
 	select {
 	case <-timeout.C:
-		finished = true
-		logger.Fatalf("timed out waiting for %s", desc)
-		return false
+		logger.Errorf("timed out waiting for %s", desc)
+		return false, true
 	case <-successC:
-		finished = true
 		logger.Tracef("successfully waited for %s", desc)
-		return true
+		return true, false
 	case <-abortC:
-		if !finished {
-			logger.Debugf("aborted waiting for %s", desc)
-		}
-		return false
+		logger.Debugf("aborted waiting for %s", desc)
+		return false, false
 	}
 }
 

internal/engine/health_test.go

@@ -0,0 +1,52 @@
+/*
+Copyright © 2021 Pete Cornish <outofcoffee@gmail.com>
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package engine
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_WaitForOp(t *testing.T) {
+	t.Run("succeeds when the operation passes", func(t *testing.T) {
+		success, timedOut := WaitForOp("op", time.Second, nil, func() bool {
+			return true
+		})
+		assert.True(t, success)
+		assert.False(t, timedOut)
+	})
+
+	t.Run("reports timeout without terminating the process", func(t *testing.T) {
+		success, timedOut := WaitForOp("op", 50*time.Millisecond, nil, func() bool {
+			return false
+		})
+		assert.False(t, success, "operation that never passes must not report success")
+		assert.True(t, timedOut, "timeout must be distinguishable so callers can clean up")
+	})
+
+	t.Run("reports abort separately from timeout", func(t *testing.T) {
+		abortC := make(chan bool, 1)
+		abortC <- true
+		success, timedOut := WaitForOp("op", time.Minute, abortC, func() bool {
+			return false
+		})
+		assert.False(t, success)
+		assert.False(t, timedOut, "an external abort is not a timeout")
+	})
+}

internal/engine/jvm/engine.go

@@ -74,17 +74,34 @@ func (j *JvmMockEngine) startWithOptions(wg *sync.WaitGroup, options engine.Star
 		return true
 	case engine.DetachHealthy:
 		// wait for health but do not reap - the OS reparents the child
-		return engine.WaitUntilUp(options.Port, j.shutDownC)
+		up, timedOut := engine.WaitUntilUp(options.Port, j.shutDownC)
+		if !up && timedOut {
+			j.stopAfterFailedStart(wg)
+		}
+		return up
 	default:
-		up := engine.WaitUntilUp(options.Port, j.shutDownC)
+		up, timedOut := engine.WaitUntilUp(options.Port, j.shutDownC)
+		if !up {
+			if timedOut {
+				j.stopAfterFailedStart(wg)
+			}
+			return false
+		}
 
 		// watch in case process stops
 		go j.notifyOnStopBlocking(wg)
 
-		return up
+		return true
 	}
 }
 
+// stopAfterFailedStart kills the child process that was started but never
+// became healthy, so a failed `up` does not leave an orphaned mock behind.
+func (j *JvmMockEngine) stopAfterFailedStart(wg *sync.WaitGroup) {
+	logger.Warnf("stopping mock that did not become healthy")
+	j.Stop(wg)
+}
+
 func (j *JvmMockEngine) GetID() string {
 	if j.command == nil || j.command.Process == nil {
 		return ""

internal/engine/native/engine.go

@@ -74,16 +74,33 @@ func (g *NativeMockEngine) startWithOptions(wg *sync.WaitGroup, options engine.S
 		return true
 	case engine.DetachHealthy:
 		// wait for health but do not reap - the OS reparents the child
-		return engine.WaitUntilUp(options.Port, g.shutDownC)
+		up, timedOut := engine.WaitUntilUp(options.Port, g.shutDownC)
+		if !up && timedOut {
+			g.stopAfterFailedStart(wg)
+		}
+		return up
 	default:
-		// watch in case process stops
-		up := engine.WaitUntilUp(options.Port, g.shutDownC)
+		up, timedOut := engine.WaitUntilUp(options.Port, g.shutDownC)
+		if !up {
+			if timedOut {
+				g.stopAfterFailedStart(wg)
+			}
+			return false
+		}
 
+		// watch in case process stops
 		go g.notifyOnStopBlocking(wg)
-		return up
+		return true
 	}
 }
 
+// stopAfterFailedStart kills the child process that was started but never
+// became healthy, so a failed `up` does not leave an orphaned mock behind.
+func (g *NativeMockEngine) stopAfterFailedStart(wg *sync.WaitGroup) {
+	logger.Warnf("stopping mock that did not become healthy")
+	g.Stop(wg)
+}
+
 func (g *NativeMockEngine) GetID() string {
 	if g.cmd == nil || g.cmd.Process == nil {
 		return ""