vt: data race on Emulator.closed between Close and a parked Read

[cameronsjo]

Summary

vt.Emulator.closed is a plain bool that Close writes and Read reads with no synchronization between them, so -race flags every shutdown of an Emulator whose output is being consumed.

This is not avoidable by using the type carefully. Read blocks on an empty pipe, and Close is the only thing that unblocks it, so "one goroutine reads, another closes" is the only shape a correct shutdown can take.

Reproducer

Fails reliably under go test -race on vt v0.0.0-20260730164118-7e2d3e6c5238:

package vtrace

import (
	"testing"
	"time"

	"github.com/charmbracelet/x/vt"
)

func TestCloseRacesParkedRead(t *testing.T) {
	e := vt.NewEmulator(80, 24)

	done := make(chan struct{})
	go func() {
		defer close(done)
		buf := make([]byte, 64)
		for {
			if _, err := e.Read(buf); err != nil {
				return
			}
		}
	}()

	time.Sleep(100 * time.Millisecond) // let the reader park inside Read
	if err := e.Close(); err != nil {
		t.Fatalf("Close: %v", err)
	}
	<-done
}

WARNING: DATA RACE
Read at 0x00c000112160 by goroutine 8:
  github.com/charmbracelet/x/vt.(*Emulator).Read()
      vt/emulator.go:252

Previous write at 0x00c000112160 by goroutine 7:
  github.com/charmbracelet/x/vt.(*Emulator).Close()
      vt/emulator.go:265

Detail

vt/emulator.go:

// Read reads data from the terminal input buffer.
func (e *Emulator) Read(p []byte) (n int, err error) {
	if e.closed {          // :252 — read
		return 0, io.EOF
	}
	return e.pr.Read(p)
}

// Close closes the terminal.
func (e *Emulator) Close() error {
	if e.closed {          // :261 — read
		return nil
	}
	e.closed = true        // :265 — write
	return e.pw.CloseWithError(io.EOF)
}

SafeEmulator does not help here, which may be worth noting given its doc comment says it "adds concurrency safety":

• its Read deliberately takes no lock (vt/safe_emulator.go:33), which is presumably intentional so a parked reader can't hold mu against every writer, and
• it does not override Close at all, so the embedded *Emulator.Close is promoted unchanged.

So the one pair that actually races is the one pair the wrapper doesn't cover.

Suggested fix

The guard is redundant rather than load-bearing: the real unblocking is e.pw.CloseWithError(io.EOF) on an io.Pipe, which is internally synchronized and already idempotent. closed is only an early-out, so it doesn't need a mutex — making it an atomic.Bool is enough, and keeps Read lock-free:

closed atomic.Bool

with e.closed → e.closed.Load() at :252, :261 and :271, and e.closed = true → e.closed.Store(true) at :265.

Happy to send that as a PR if the approach looks right.

Impact

Downstream this forces a choice between dropping -race from CI and excluding every test that exercises emulator shutdown, since there is no usage pattern that avoids it.

[cameronsjo]

Duplicate of #879, which reports the same race and predates this by two months. Closing in favour of it — apologies, this is the second time I've filed this without searching properly.

The fix is already proposed in #881.