fix: clean up ttyd process on early return from Evaluate (#738)

evaluator.go

@@ -42,7 +42,13 @@ func Evaluate(ctx context.Context, tape string, out io.Writer, opts ...Evaluator
 	if err := v.Start(); err != nil {
 		return []error{err}
 	}
-	defer func() { _ = v.close() }()
+	// Use terminate() instead of close() so that every early return below
+	// (Page.Wait error, failed SET, video-dimension error, failed Hide block,
+	// etc.) cleans up the ttyd process as well as the browser. close() only
+	// closes the browser, which left ttyd orphaned before issue #738.
+	// terminate() is idempotent — Record()'s own cleanup path inside vhs.go
+	// also calls it, but the second call is a no-op.
+	defer func() { _ = v.terminate() }()
 
 	// Let's wait until we can access the window.term variable.
 	//

vhs.go

@@ -30,6 +30,7 @@ type VHS struct {
 	mutex        *sync.Mutex
 	started      bool
 	recording    bool
+	terminated   bool
 	tty          *exec.Cmd
 	totalFrames  int
 	close        func() error
@@ -122,6 +123,11 @@ func New() VHS {
 }
 
 // Start starts ttyd, browser and everything else needed to create the gif.
+//
+// If browser launch or page creation fails after ttyd has already started,
+// the ttyd process is killed before the error is returned. Without this
+// cleanup, callers cannot reach `terminate()` and the ttyd process is
+// orphaned (issue #738).
 func (vhs *VHS) Start() error {
 	vhs.mutex.Lock()
 	defer vhs.mutex.Unlock()
@@ -136,15 +142,25 @@ func (vhs *VHS) Start() error {
 		return fmt.Errorf("could not start tty: %w", err)
 	}
 
+	// killTty kills the spawned ttyd if a later step in Start() fails.
+	killTty := func() {
+		if vhs.tty != nil && vhs.tty.Process != nil {
+			_ = vhs.tty.Process.Kill()
+		}
+	}
+
 	path, _ := launcher.LookPath()
 	enableNoSandbox := os.Getenv("VHS_NO_SANDBOX") != ""
 	u, err := launcher.New().Leakless(false).Bin(path).NoSandbox(enableNoSandbox).Launch()
 	if err != nil {
+		killTty()
 		return fmt.Errorf("could not launch browser: %w", err)
 	}
 	browser := rod.New().ControlURL(u).MustConnect()
 	page, err := browser.Page(proto.TargetCreateTarget{URL: fmt.Sprintf("http://localhost:%d", port)})
 	if err != nil {
+		_ = browser.Close()
+		killTty()
 		return fmt.Errorf("could not open ttyd: %w", err)
 	}
 
@@ -195,17 +211,34 @@ const cleanupWaitTime = 100 * time.Millisecond
 // Terminate cleans up a VHS instance and terminates the go-rod browser and ttyd
 // processes.
 //
+// Idempotent: safe to call from both the post-Record cleanup path inside
+// Record() and the Evaluate() deferred cleanup that fires on early returns
+// (issue #738). The second call is a no-op.
+//
 //nolint:wrapcheck
 func (vhs *VHS) terminate() error {
+	vhs.mutex.Lock()
+	if vhs.terminated || !vhs.started {
+		vhs.mutex.Unlock()
+		return nil
+	}
+	vhs.terminated = true
+	vhs.mutex.Unlock()
+
 	// Give some time for any commands executed (such as `rm`) to finish.
 	//
 	// If a user runs a long running command, they must sleep for the required time
 	// to finish.
 	time.Sleep(cleanupWaitTime)
 
 	// Tear down the processes we started.
-	vhs.browser.MustClose()
-	return vhs.tty.Process.Kill()
+	if vhs.browser != nil {
+		vhs.browser.MustClose()
+	}
+	if vhs.tty != nil && vhs.tty.Process != nil {
+		return vhs.tty.Process.Kill()
+	}
+	return nil
 }
 
 // Cleanup individual frames.

vhs_test.go

@@ -0,0 +1,37 @@
+package main
+
+import (
+	"sync"
+	"testing"
+)
+
+// TestTerminateIdempotent guards the fix for issue #738 — Evaluate()
+// defers terminate() on every early return, and Record() also calls
+// terminate() once context is done. Both paths run on the normal happy
+// path, so terminate() must be safe to call twice.
+func TestTerminateIdempotent(t *testing.T) {
+	v := VHS{
+		mutex:      &sync.Mutex{},
+		started:    true,
+		terminated: true, // simulate post-Record state: terminate() already ran
+	}
+
+	// Second call must be a no-op and must not deref nil browser/tty fields.
+	if err := v.terminate(); err != nil {
+		t.Errorf("second terminate() returned error: %v", err)
+	}
+}
+
+// TestTerminateBeforeStartIsNoOp guards the fix for issue #738 — if
+// Evaluate() reaches the deferred terminate() without Start() having
+// succeeded (e.g. parser error path), terminate() must not panic
+// dereferencing nil browser / tty fields.
+func TestTerminateBeforeStartIsNoOp(t *testing.T) {
+	v := VHS{
+		mutex:   &sync.Mutex{},
+		started: false,
+	}
+	if err := v.terminate(); err != nil {
+		t.Errorf("terminate() before Start() returned error: %v", err)
+	}
+}