diff --git a/evaluator.go b/evaluator.go index 5768b854..e9540485 100644 --- a/evaluator.go +++ b/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. // diff --git a/vhs.go b/vhs.go index 235e7b42..dd7091b1 100644 --- a/vhs.go +++ b/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,8 +211,20 @@ 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 @@ -204,8 +232,13 @@ func (vhs *VHS) terminate() error { 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. diff --git a/vhs_test.go b/vhs_test.go new file mode 100644 index 00000000..2f1256ae --- /dev/null +++ b/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) + } +}