fix(vt): use atomic.Bool for Emulator.closed to prevent data race#881
Open
toller892 wants to merge 1 commit into
Open
fix(vt): use atomic.Bool for Emulator.closed to prevent data race#881toller892 wants to merge 1 commit into
toller892 wants to merge 1 commit into
Conversation
The closed field in Emulator was accessed without synchronization between Read() and Close(), causing a data race when one goroutine drains Read() while another calls Close() to unblock it. Replace the plain bool with sync/atomic.Bool: - Read(): closed.Load() - Close(): closed.Swap(true) (atomic check-and-set) - Write(): closed.Load() SafeEmulator.Read() deliberately holds no lock (so it can block without holding the mutex), making atomic access the right fix rather than adding a mutex override. Fixes charmbracelet#879
paultyng
added a commit
to paultyng/ideate
that referenced
this pull request
Jun 2, 2026
The vt.Emulator stores its `closed` state as a plain bool with no
synchronization. Buffer.Close (in internal/agent/vscreen) races the
drain goroutine's io.Copy(io.Discard, emu), tripping `go test -race`:
WARNING: DATA RACE
Read at 0x... by goroutine N:
vt.(*Emulator).Read emulator.go:252 // if e.closed
Previous write at 0x... by goroutine M:
vt.(*Emulator).Close emulator.go:265 // e.closed = true
Reproduces in TestSnapshotSurvivesByteRingWraparound,
TestConcurrentFeedAndSnapshot, and TestBuffer_Snapshot_PersistOnStop.
Filed upstream as charmbracelet/x#879; fix is up as
charmbracelet/x#881 (atomic.Bool), unmerged.
Replace pin: github.com/toller892/x/vt@4fbcadcb (PR #881 head). The
delta vs our prior pin is ahead-only (ahead_by=2, behind_by=0); the
one ahead-commit outside vt is an unrelated charmtone change we
don't import. Full -race ./... passes locally after the pin.
Drop this replace when upstream merges; tracked at backlog item
e915f41c.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
paultyng
added a commit
to paultyng/ideate
that referenced
this pull request
Jun 3, 2026
… froze resumed terminals (#11) * fix(frontend): vite target esnext; bypass esbuild ||= miscompile that froze resumed terminals esbuild 0.25.9+ (shipped via Vite 6.4.3) miscompiles the `||=` operator lowering inside xterm.js's `requestMode` handler. Source: requestMode(e, t) { let r; ((te) => /*…*/)(r ||= {}); ... } Lowered (broken): requestMode(t, e) { var g; ((S) => /*…*/)(void 0 || (n = {})); ... } The lowering renames `let r` to `var g` but the assignment in the lowered `||=` references `n` — a different, undeclared name. Since the bundle loads as a module (strict), assigning to undeclared `n` throws `ReferenceError: Can't find variable: n` in WebKit. Trigger in Ideate: session resume calls `terminal.write(snapshot)` to replay the prior incarnation's screen state, hitting Claude Code's boot-time DECRQM (`CSI ? n $p`) capability probes early in the burst. First DECRQM lands `requestMode`, which throws, corrupting parser state for that Terminal instance. Subsequent writes also throw — typing visibly stops rendering until React remounts the component. Fix: target=esnext tells esbuild the runtime (WKWebView via Wails = Safari 15+) supports `||=` natively, so no lowering pass runs. The miscompile can't fire. No xterm version pin or chunking workaround needed. Verified in the built bundle: `requestMode(t,e){let n;...(n||={...}` — `n` is declared, `||=` preserved, assignment lands on the declared variable. Related: evanw/esbuild#4297 (`global scope pollution since 0.25.9`). Revisit when esbuild ships a fix; can drop `target: esnext` then. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(claudecode): inject BinaryPath to decouple BuildCommand tests from $PATH CommandConfig now carries an optional BinaryPath. When empty, BuildCommand falls back to exec.LookPath("claude") — the production path. Tests set it to a sentinel ("/path/to/claude-test-stub") so they exercise BuildCommand's real logic (settings file, header composition, argv ordering) without needing claude on the CI runner's $PATH. Before this change, the lint-test-build job failed on every CI run that didn't pre-install claude: config_test.go:92: BuildCommand: claude CLI not found: exec: "claude": executable file not found in $PATH The bindisco rework (backlog 3c74c629) will replace the LookPath fallback with bindisco.Resolve and route discovery results through this same BinaryPath field — the API stays compatible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(deps): replace charmbracelet/x/vt to pull in atomic.Bool race fix The vt.Emulator stores its `closed` state as a plain bool with no synchronization. Buffer.Close (in internal/agent/vscreen) races the drain goroutine's io.Copy(io.Discard, emu), tripping `go test -race`: WARNING: DATA RACE Read at 0x... by goroutine N: vt.(*Emulator).Read emulator.go:252 // if e.closed Previous write at 0x... by goroutine M: vt.(*Emulator).Close emulator.go:265 // e.closed = true Reproduces in TestSnapshotSurvivesByteRingWraparound, TestConcurrentFeedAndSnapshot, and TestBuffer_Snapshot_PersistOnStop. Filed upstream as charmbracelet/x#879; fix is up as charmbracelet/x#881 (atomic.Bool), unmerged. Replace pin: github.com/toller892/x/vt@4fbcadcb (PR #881 head). The delta vs our prior pin is ahead-only (ahead_by=2, behind_by=0); the one ahead-commit outside vt is an unrelated charmtone change we don't import. Full -race ./... passes locally after the pin. Drop this replace when upstream merges; tracked at backlog item e915f41c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(headless): exec the sleep to keep ctx-cancel test on a single PID TestClaudeRunner_CtxCancellationKillsSubprocess flaked on CI because the inline /bin/sh script ran `sleep 30` as sh's child. exec.CommandContext SIGKILL'd sh, but the orphaned sleep was reparented to init and kept the stdout pipe's write end open for its full 30 s, so the test's rc.Read() didn't observe EOF inside the 5 s deadline. `exec sleep 30` replaces sh in the same PID, so SIGKILL kills sleep directly and the pipe closes immediately — same teardown behavior as the production claude binary the runner targets. Considered swapping to a real testagent invocation via /think dispatch (v0.6+) so the test exercises a real binary instead of a shell-script fake; deferred because the v0.6 TUI overhaul breaks 33 unrelated playwright tests. Tracked at backlog item f550ecd1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Description
Fixes a data race on
Emulator.closedbetweenRead()andClose().Problem:
Emulator.Readreadsclosed(line 252) then blocks on the pipe.Emulator.Closewritesclosed = true(line 265) to unblock it. Both accesses are unsynchronized —go test -raceflags this as a data race.SafeEmulator.Readdeliberately holds no lock (so it can block without holding the mutex), and there's noSafeEmulator.Closeoverride, so the race also exists through the safe wrapper.Fix: Replace
closed boolwithsync/atomic.Bool:Read():closed.Load()— lock-free check before blocking on the pipeClose():closed.Swap(true)— atomic check-and-set (returns early if already closed)Write():closed.Load()— lock-free check before writingNo mutex needs to be held across the blocking read, which is the whole point of
SafeEmulator.Readnot taking the lock.Testing: Added
TestEmulatorCloseDataRaceandTestSafeEmulatorCloseDataRacewhich exercise the concurrent Read+Close pattern withgo test -race. All existing tests pass.Fixes #879