From f0f4c3c03069d6ac8a26eb07d5e335c111caf629 Mon Sep 17 00:00:00 2001 From: Dhruba Datta <74358627+dhruba-datta@users.noreply.github.com> Date: Sat, 16 May 2026 01:46:41 +0600 Subject: [PATCH] fix: clean up ttyd process on early return from Evaluate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes charmbracelet/vhs#738. When Evaluate() returns early — between Start() and the Record()/teardown() path — the ttyd process was left running indefinitely. The deferred cleanup at evaluator.go:45 only called v.close(), which is set to vhs.browser.Close. The terminate() method that kills both browser AND ttyd was only reachable via Record()'s own cleanup, which requires Record() to have started. Affected code paths from the issue: - Page.Wait fails (evaluator.go:50-53) - A SET command fails (evaluator.go:59-63) - Video dimensions too small (evaluator.go:78-91) - A Hide block command fails (evaluator.go:106-109) - Browser launch or page creation fails inside Start() itself, after ttyd has already started Three changes: 1. evaluator.go: change defer v.close() to defer v.terminate() so every early return kills ttyd as well as the browser. 2. vhs.go Start(): kill ttyd if browser launch or page creation fails. Without this, terminate() is never reached because Start() returns before assigning vhs.started = true. 3. vhs.go terminate(): make idempotent via a 'terminated' guard on the VHS struct, and tolerate nil browser/tty fields. The happy path now calls terminate() twice (Record()'s own cleanup plus Evaluate()'s defer) — the second call is a no-op. Added vhs_test.go with two regression tests covering both new invariants (idempotent on already-terminated, no-op before Start). Full test suite passes. --- evaluator.go | 8 +++++++- vhs.go | 37 +++++++++++++++++++++++++++++++++++-- vhs_test.go | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 vhs_test.go 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) + } +}