fix: clean up ttyd process on early return from Evaluate (#738)#751
Open
dhruba-datta wants to merge 1 commit into
Open
fix: clean up ttyd process on early return from Evaluate (#738)#751dhruba-datta wants to merge 1 commit into
dhruba-datta wants to merge 1 commit into
Conversation
Fixes charmbracelet#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.
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.
Fixes #738.
When
Evaluate()returns early — betweenStart()and theRecord()/teardown()path — thettydprocess was left running indefinitely. The deferred cleanup atevaluator.go:45only calledv.close(), which is set tovhs.browser.Close. Theterminate()method that kills both browser and ttyd was only reachable viaRecord()'s own cleanup path, which requiresRecord()to have started.Reproduction (from the issue)
Affected code paths
All early returns after
Start()succeeds but beforeRecord()begins:Page.Waitfails (evaluator.go:50-53)SETcommand fails (evaluator.go:59-63)evaluator.go:78-91)Hideblock command fails (evaluator.go:106-109)Start()itself, afterttydhas already started — the originalStart()returned the error without killing the spawnedttyd.Changes
Three small changes across two files:
evaluator.go: changedefer v.close()todefer v.terminate()so every early return cleans up ttyd as well as the browser.vhs.goStart(): kill the spawnedttydif browser launch or page creation fails. Without this, callers cannot reachterminate()becauseStart()returns before assigningvhs.started = true.vhs.goterminate(): make idempotent via aterminatedflag on theVHSstruct, and tolerate nilbrowser/ttyfields. The happy path now callsterminate()twice (once fromRecord()'s own cleanup goroutine and once fromEvaluate()'s deferred call) — the second call is a no-op.Test coverage
Added
vhs_test.gowith two regression tests:TestTerminateIdempotent— guards the double-call path.TestTerminateBeforeStartIsNoOp— guards the path whereEvaluate()defersterminate()butStart()never succeeded (parser-only error path).Full
go test ./...passes locally on macOS / arm64 (Go 1.26.3).