fix(process): remove data race on cmd.ProcessState (was read across goroutines)
The race: os/exec writes cmd.ProcessState inside cmd.Wait(), which
runs in the waitExit goroutine. Start, Status, Subscribe, and
Restart all read cmd.ProcessState from other goroutines to decide
'still running?'. That's a textbook Go data race — fixed by
TestStartRejectsDuplicateRunningSession under -race.
Fix: port the same pattern the shell package uses for its multi-shell
refactor. The manager now owns an exited map[string]struct{} guarded
by exitedMu. waitExit is the ONLY place that calls cmd.Wait(); after
Wait() returns it records exited[workspaceID]. Everyone else checks
the exited map instead of reading cmd.ProcessState.
- internal/process/manager.go: add exited + exitedMu to LocalManager;
Start conflict check, Status, Subscribe, Restart, Stop all consult
the exited map; Start clears the entry on new process; Stop and
Restart delete the entry on stop; waitExit sets it after Wait().
- go.mod / go.sum: tidied (uuid v1.6.0 was already direct dep from
the shell refactor).
go test -race -count=3 ./... clean across the whole repo.
go test -race -count=5 ./internal/process/... clean.
This commit is contained in:
@@ -5,6 +5,7 @@ go 1.25.0
|
|||||||
require (
|
require (
|
||||||
github.com/creack/pty v1.1.24
|
github.com/creack/pty v1.1.24
|
||||||
github.com/gin-gonic/gin v1.12.0
|
github.com/gin-gonic/gin v1.12.0
|
||||||
|
github.com/google/uuid v1.6.0
|
||||||
github.com/gorilla/websocket v1.5.3
|
github.com/gorilla/websocket v1.5.3
|
||||||
gopkg.in/yaml.v3 v3.0.1
|
gopkg.in/yaml.v3 v3.0.1
|
||||||
)
|
)
|
||||||
@@ -21,7 +22,6 @@ require (
|
|||||||
github.com/go-playground/validator/v10 v10.30.3 // indirect
|
github.com/go-playground/validator/v10 v10.30.3 // indirect
|
||||||
github.com/goccy/go-json v0.10.6 // indirect
|
github.com/goccy/go-json v0.10.6 // indirect
|
||||||
github.com/goccy/go-yaml v1.19.2 // indirect
|
github.com/goccy/go-yaml v1.19.2 // indirect
|
||||||
github.com/google/uuid v1.6.0 // indirect
|
|
||||||
github.com/json-iterator/go v1.1.12 // indirect
|
github.com/json-iterator/go v1.1.12 // indirect
|
||||||
github.com/klauspost/cpuid/v2 v2.4.0 // indirect
|
github.com/klauspost/cpuid/v2 v2.4.0 // indirect
|
||||||
github.com/leodido/go-urn v1.4.0 // indirect
|
github.com/leodido/go-urn v1.4.0 // indirect
|
||||||
|
|||||||
+40
-10
@@ -32,6 +32,8 @@ type LocalManager struct {
|
|||||||
args []string
|
args []string
|
||||||
mu sync.Mutex
|
mu sync.Mutex
|
||||||
sessions map[string]*Session
|
sessions map[string]*Session
|
||||||
|
exitedMu sync.Mutex
|
||||||
|
exited map[string]struct{}
|
||||||
}
|
}
|
||||||
|
|
||||||
// NewManager creates a LocalManager with the given command.
|
// NewManager creates a LocalManager with the given command.
|
||||||
@@ -41,6 +43,7 @@ func NewManager(command string, args []string) *LocalManager {
|
|||||||
command: normalizeCommand(command),
|
command: normalizeCommand(command),
|
||||||
args: args,
|
args: args,
|
||||||
sessions: make(map[string]*Session),
|
sessions: make(map[string]*Session),
|
||||||
|
exited: make(map[string]struct{}),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -50,8 +53,11 @@ func (m *LocalManager) Start(workspaceID string, workspaceRoot string) error {
|
|||||||
m.mu.Lock()
|
m.mu.Lock()
|
||||||
defer m.mu.Unlock()
|
defer m.mu.Unlock()
|
||||||
|
|
||||||
if sess, ok := m.sessions[workspaceID]; ok && sess.Cmd.Process != nil {
|
if _, ok := m.sessions[workspaceID]; ok {
|
||||||
if sess.Cmd.ProcessState == nil {
|
m.exitedMu.Lock()
|
||||||
|
_, exited := m.exited[workspaceID]
|
||||||
|
m.exitedMu.Unlock()
|
||||||
|
if !exited {
|
||||||
return util.New(util.CodeConflict, "process already running")
|
return util.New(util.CodeConflict, "process already running")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -96,6 +102,10 @@ func (m *LocalManager) Start(workspaceID string, workspaceRoot string) error {
|
|||||||
}
|
}
|
||||||
m.sessions[workspaceID] = sess
|
m.sessions[workspaceID] = sess
|
||||||
|
|
||||||
|
m.exitedMu.Lock()
|
||||||
|
delete(m.exited, workspaceID)
|
||||||
|
m.exitedMu.Unlock()
|
||||||
|
|
||||||
outputDone := make(chan struct{})
|
outputDone := make(chan struct{})
|
||||||
go m.captureOutput(sess, stdoutR, outputDone)
|
go m.captureOutput(sess, stdoutR, outputDone)
|
||||||
go m.waitExit(sess, outputDone)
|
go m.waitExit(sess, outputDone)
|
||||||
@@ -138,6 +148,10 @@ func (m *LocalManager) waitExit(sess *Session, outputDone <-chan struct{}) {
|
|||||||
_ = sess.Cmd.Wait()
|
_ = sess.Cmd.Wait()
|
||||||
<-outputDone
|
<-outputDone
|
||||||
|
|
||||||
|
m.exitedMu.Lock()
|
||||||
|
m.exited[sess.WorkspaceID] = struct{}{}
|
||||||
|
m.exitedMu.Unlock()
|
||||||
|
|
||||||
sess.mu.Lock()
|
sess.mu.Lock()
|
||||||
defer sess.mu.Unlock()
|
defer sess.mu.Unlock()
|
||||||
|
|
||||||
@@ -173,17 +187,26 @@ func (m *LocalManager) Stop(workspaceID string) error {
|
|||||||
}
|
}
|
||||||
|
|
||||||
delete(m.sessions, workspaceID)
|
delete(m.sessions, workspaceID)
|
||||||
|
m.exitedMu.Lock()
|
||||||
|
delete(m.exited, workspaceID)
|
||||||
|
m.exitedMu.Unlock()
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// Restart stops (if running) then starts the process.
|
// Restart stops (if running) then starts the process.
|
||||||
func (m *LocalManager) Restart(workspaceID string, workspaceRoot string) error {
|
func (m *LocalManager) Restart(workspaceID string, workspaceRoot string) error {
|
||||||
m.mu.Lock()
|
m.mu.Lock()
|
||||||
if sess, ok := m.sessions[workspaceID]; ok && sess.Cmd.Process != nil {
|
if sess, ok := m.sessions[workspaceID]; ok {
|
||||||
if sess.Cmd.ProcessState == nil {
|
m.exitedMu.Lock()
|
||||||
|
_, exited := m.exited[workspaceID]
|
||||||
|
m.exitedMu.Unlock()
|
||||||
|
if !exited && sess.Cmd.Process != nil {
|
||||||
_ = sess.Cmd.Process.Kill()
|
_ = sess.Cmd.Process.Kill()
|
||||||
delete(m.sessions, workspaceID)
|
|
||||||
}
|
}
|
||||||
|
delete(m.sessions, workspaceID)
|
||||||
|
m.exitedMu.Lock()
|
||||||
|
delete(m.exited, workspaceID)
|
||||||
|
m.exitedMu.Unlock()
|
||||||
}
|
}
|
||||||
m.mu.Unlock()
|
m.mu.Unlock()
|
||||||
|
|
||||||
@@ -193,14 +216,17 @@ func (m *LocalManager) Restart(workspaceID string, workspaceRoot string) error {
|
|||||||
// Status returns the current process status for the workspace.
|
// Status returns the current process status for the workspace.
|
||||||
func (m *LocalManager) Status(workspaceID string) Status {
|
func (m *LocalManager) Status(workspaceID string) Status {
|
||||||
m.mu.Lock()
|
m.mu.Lock()
|
||||||
defer m.mu.Unlock()
|
|
||||||
|
|
||||||
sess, ok := m.sessions[workspaceID]
|
sess, ok := m.sessions[workspaceID]
|
||||||
|
m.mu.Unlock()
|
||||||
|
|
||||||
if !ok || sess.Cmd.Process == nil {
|
if !ok || sess.Cmd.Process == nil {
|
||||||
return Status{WorkspaceID: workspaceID, Running: false}
|
return Status{WorkspaceID: workspaceID, Running: false}
|
||||||
}
|
}
|
||||||
|
|
||||||
if sess.Cmd.ProcessState != nil {
|
m.exitedMu.Lock()
|
||||||
|
_, exited := m.exited[workspaceID]
|
||||||
|
m.exitedMu.Unlock()
|
||||||
|
if exited {
|
||||||
return Status{WorkspaceID: workspaceID, Running: false}
|
return Status{WorkspaceID: workspaceID, Running: false}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -225,10 +251,14 @@ func (m *LocalManager) Subscribe(workspaceID string) (Subscription, error) {
|
|||||||
sess.mu.Lock()
|
sess.mu.Lock()
|
||||||
sub := newSubscription(sess)
|
sub := newSubscription(sess)
|
||||||
sess.Subscribers[sub] = struct{}{}
|
sess.Subscribers[sub] = struct{}{}
|
||||||
if sess.Cmd.ProcessState != nil {
|
sess.mu.Unlock()
|
||||||
|
|
||||||
|
m.exitedMu.Lock()
|
||||||
|
_, exited := m.exited[workspaceID]
|
||||||
|
m.exitedMu.Unlock()
|
||||||
|
if exited {
|
||||||
sub.closeChan()
|
sub.closeChan()
|
||||||
}
|
}
|
||||||
sess.mu.Unlock()
|
|
||||||
|
|
||||||
return sub, nil
|
return sub, nil
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user