Skip to content

Commit 44a567b

Browse files
committed
fix: reject nested vhs invocations instead of panicking
Running `vhs` inside a tape that is itself being recorded (e.g. a tape that types `vhs other.tape`) spins up a second ttyd/browser pair sharing the outer process's environment. Depending on timing, this races with the outer instance's own ttyd/browser teardown and can crash the whole process with `panic: write tcp ...: use of closed network connection`. Full nested-rendering support is out of scope, so instead fail fast: VHS now exports VHS_RECORDING=1 into the shell it drives, and VHS.Start checks for it before launching ttyd/the browser, returning a clear "vhs is already recording; nested rendering is not supported" error rather than letting the inner instance run and potentially panic. Fixes #761
1 parent f48d79b commit 44a567b

3 files changed

Lines changed: 51 additions & 3 deletions

File tree

tty.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ import (
1616
"os/exec"
1717
)
1818

19+
// recordingEnvVar is set in the environment of the shell that VHS drives so
20+
// that any `vhs` invocation typed into that shell (e.g. a tape that itself
21+
// runs `vhs`) can detect that it is nested inside an existing recording
22+
// session. See VHS.Start for where this is checked.
23+
const recordingEnvVar = "VHS_RECORDING"
24+
1925
// randomPort returns a random port number that is not in use.
2026
func randomPort() int {
2127
addr, _ := net.Listen("tcp", ":0") //nolint:gosec,noctx
@@ -39,8 +45,7 @@ func buildTtyCmd(port int, shell Shell) *exec.Cmd {
3945
args = append(args, shell.Command...)
4046

4147
cmd := exec.Command("ttyd", args...)
42-
if shell.Env != nil {
43-
cmd.Env = append(shell.Env, os.Environ()...)
44-
}
48+
env := append(shell.Env, os.Environ()...)
49+
cmd.Env = append(env, recordingEnvVar+"=1")
4550
return cmd
4651
}

vhs.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,16 @@ func (vhs *VHS) Start() error {
130130
return fmt.Errorf("vhs is already started")
131131
}
132132

133+
// Refuse to start a nested recording session. Running `vhs` inside a
134+
// tape that is itself being recorded (e.g. `Type "vhs other.tape"`)
135+
// causes the inner instance to spin up its own ttyd and browser while
136+
// sharing the outer process's environment, which leads to port and
137+
// connection conflicts and, previously, a panic. Nested rendering is
138+
// not supported, so fail fast with a clear error instead.
139+
if os.Getenv(recordingEnvVar) != "" {
140+
return errors.New("vhs is already recording; nested rendering is not supported")
141+
}
142+
133143
port := randomPort()
134144
vhs.tty = buildTtyCmd(port, vhs.Options.Shell)
135145
if err := vhs.tty.Start(); err != nil {

vhs_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package main
2+
3+
import (
4+
"strings"
5+
"sync"
6+
"testing"
7+
)
8+
9+
// TestStart_NestedRecordingRejected ensures that starting a VHS recording
10+
// while already inside a recording session (as signaled by recordingEnvVar,
11+
// which VHS exports into the shell it drives) fails fast with a clear error
12+
// instead of going on to spin up a second ttyd/browser pair, which is what
13+
// used to cause a panic. See charmbracelet/vhs#761.
14+
func TestStart_NestedRecordingRejected(t *testing.T) {
15+
t.Setenv(recordingEnvVar, "1")
16+
17+
opts := DefaultVHSOptions()
18+
vhs := VHS{
19+
Options: &opts,
20+
mutex: &sync.Mutex{},
21+
}
22+
23+
err := vhs.Start()
24+
if err == nil {
25+
t.Fatal("expected an error when starting a nested recording, got nil")
26+
}
27+
if !strings.Contains(err.Error(), "already recording") {
28+
t.Fatalf("expected error to mention that vhs is already recording, got: %v", err)
29+
}
30+
if vhs.started {
31+
t.Fatal("vhs.started should remain false when a nested recording is rejected")
32+
}
33+
}

0 commit comments

Comments
 (0)