fix: reject nested vhs invocations instead of panicking#768
Open
arimu1 wants to merge 1 commit into
Open
Conversation
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 charmbracelet#761
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #761.
Running
vhsinside a tape that is itself being recorded (e.g. a tape that typesvhs other.tapeand hits Enter) starts a second ttyd + browser pair inside the first recording session. The inner instance shares the outer process's environment and races with the outer instance's own ttyd/browser lifecycle (port selection, process teardown, etc.). Depending on timing this crashes the whole program:Repro from the issue:
inner.tape: any minimal tape (Output inner.gif, oneType/Enter).outer.tape:Output outer.gif,Type "vhs inner.tape",Enter,Sleep 10s.vhs outer.tape→ panic.I reproduced this locally (macOS, Chrome + ttyd 1.7.7) — nested rendering sometimes succeeds and sometimes fails, confirming it's a race rather than a deterministic error, consistent with the reporter's "reproduced twice" note on Windows/WSL.
Why full nested-rendering support is out of scope
Making nested rendering actually work would require isolating ttyd's port allocation, the browser's remote-debugging connection, and process teardown ordering between the outer and inner
vhsinstances — effectively giving each recording session its own sandboxed environment. That's a much larger change than a bug fix, and the issue reporter explicitly said a graceful error is an acceptable resolution.Fix
VHS.Startnow exportsVHS_RECORDING=1into the environment of the shell/ttyd process it drives (tty.go), so anyvhsinvocation typed into that shell inherits it.Before launching ttyd/the browser,
VHS.Start(vhs.go) checks forVHS_RECORDINGand, if set, returns immediately with a clear error instead of proceeding:This fails fast before any ttyd/browser process is spawned for the inner instance, so there's nothing left to race or panic.
Testing
go build ./...go vet ./...go test ./...TestStart_NestedRecordingRejectedinvhs_test.go, which assertsVHS.Startreturns the nested-recording error (and leavesvhs.startedfalse) whenVHS_RECORDINGis set, without needing ttyd/a browser.ttyd1.7.7 and Chrome installed): the outer tape now renders successfully and the recorded terminal shows the innervhsinvocation printingvhs is already recording; nested rendering is not supportedand exiting with a non-zero status, instead of panicking.