From 468b6104b777dd7966e357011a26f3abd15a749e Mon Sep 17 00:00:00 2001
From: Kevin Tang <73975146+vt128@users.noreply.github.com>
Date: Mon, 13 Jul 2026 15:57:34 +0800
Subject: [PATCH] [fix] hold the Machine lock for the whole REPL session

REPL() drove prepareThread and repl.REPLOptions against m.thread and
m.predeclared with no lock, while every other execution path (Run,
RunFile, RunWithTimeout, ...) holds m.mu for the whole run and Reset
holds it to swap the thread. A REPL session concurrent with a Run or
Reset on the same Machine was therefore a data race on m.thread.

REPL now takes the write lock for its whole session, matching
runInternal; String() already uses TryRLock and returns the running
snapshot rather than blocking, so it stays responsive during a REPL.

TestMachine_REPLIsRaceFree drives the REPL non-interactively (stdin
pointed at /dev/null so repl.REPLOptions returns on EOF) and runs it
concurrently with Run and Reset; it reports a data race under -race
before this change and is clean after.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 machine_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++
 run_repl.go     | 10 ++++++++++
 2 files changed, 54 insertions(+)

diff --git a/machine_test.go b/machine_test.go
index 6764423..a98317b 100644
--- a/machine_test.go
+++ b/machine_test.go
@@ -74,6 +74,50 @@ func TestMachine_StringInCallbackDoesNotDeadlock(t *testing.T) {
 	}
 }
 
+// TestMachine_REPLIsRaceFree: REPL() drove prepareThread and repl.REPLOptions
+// against m.thread/m.predeclared with NO lock, while every other execution path
+// (Run/RunFile/RunWithTimeout/...) holds m.mu for the whole run and Reset holds
+// it to swap the thread. So a REPL session concurrent with a Run or Reset on the
+// same Machine was a data race on m.thread. REPL now takes the write lock for its
+// whole session, matching runInternal. This bites only under -race.
+func TestMachine_REPLIsRaceFree(t *testing.T) {
+	// Drive the REPL non-interactively: point stdin at an always-EOF reader so
+	// repl.REPLOptions returns promptly instead of blocking on a terminal.
+	devnull, err := os.Open(os.DevNull)
+	if err != nil {
+		t.Fatalf("open %s: %v", os.DevNull, err)
+	}
+	defer devnull.Close()
+	oldStdin := os.Stdin
+	os.Stdin = devnull
+	defer func() { os.Stdin = oldStdin }()
+
+	m := starlet.NewDefault()
+	m.SetScript("main.star", []byte(`x = len([i for i in range(100)])`), nil)
+
+	var wg sync.WaitGroup
+	wg.Add(3)
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 30; i++ {
+			m.REPL() // stdin is EOF, so each session returns immediately
+		}
+	}()
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 30; i++ {
+			_, _ = m.Run()
+		}
+	}()
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 30; i++ {
+			m.Reset()
+		}
+	}()
+	wg.Wait()
+}
+
 func TestNewDefault(t *testing.T) {
 	m := starlet.NewDefault()
 	if m == nil {
diff --git a/run_repl.go b/run_repl.go
index 787fc48..73b4995 100644
--- a/run_repl.go
+++ b/run_repl.go
@@ -11,6 +11,16 @@ import "go.starlark.net/repl"
 // chzyer/readline, a terminal library that does not compile for browser
 // js/wasm or WASI; see run_repl_stub.go for the no-terminal stub.
 func (m *Machine) REPL() {
+	// Hold the write lock for the whole REPL session, matching runInternal:
+	// prepareThread and repl.REPLOptions read and mutate m.thread/m.predeclared,
+	// and every other execution path already serializes on m.mu (String() uses
+	// TryRLock and returns the running snapshot rather than blocking). A REPL
+	// owns the Machine exclusively for its duration, so holding the lock until it
+	// exits is the right scope — without it, a REPL concurrent with Run/Reset
+	// races on m.thread.
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
 	if err := m.prepareThread(nil); err != nil {
 		repl.PrintError(err)
 		return