diff --git a/internal/knight/scheduler.go b/internal/knight/scheduler.go index 601018e53..eb0fe3683 100644 --- a/internal/knight/scheduler.go +++ b/internal/knight/scheduler.go @@ -496,14 +496,21 @@ func (k *Knight) Queue() *CandidateQueue { // Status returns a human-readable status string. func (k *Knight) Status() string { - if !k.cfg.Enabled { + // #2757: cfg.Enabled/running/lock are written under k.mu by + // Start/Stop/Enable; snapshot them under the same lock (Running() is the + // reference pattern). Unlocked reads were a Go memory model data race + // (WebUI Status polling vs daemon control commands). + k.mu.Lock() + enabled, running, lock := k.cfg.Enabled, k.running, k.lock + k.mu.Unlock() + if !enabled { return "disabled" } - if !k.running { - if k.lock == nil { + if !running { + if lock == nil { pid, _ := LockHeldBy(k.projDir) if pid > 0 { - return fmt.Sprintf("deferred — instance PID %d holds lock", pid) + return fmt.Sprintf("deferred - instance PID %d holds lock", pid) } } return "stopped" @@ -525,7 +532,11 @@ func (k *Knight) NotifyActivity() { // CanPerformTask checks if Knight has budget and is allowed to run. func (k *Knight) CanPerformTask() bool { - if !k.cfg.Enabled || !k.running { + // #2757: same locked snapshot as Status(). + k.mu.Lock() + enabled, running := k.cfg.Enabled, k.running + k.mu.Unlock() + if !enabled || !running { return false } return k.budget.CanSpend() diff --git a/internal/knight/zz_issue2757_test.go b/internal/knight/zz_issue2757_test.go new file mode 100644 index 000000000..b23c3c532 --- /dev/null +++ b/internal/knight/zz_issue2757_test.go @@ -0,0 +1,55 @@ +package knight + +// Issue #2757 probe: Status() and CanPerformTask() read k.cfg.Enabled / +// k.running / k.lock without holding k.mu while Start/Stop/Enable write +// them under the lock (Running() is the correct pattern). A -race build on +// concurrent Status-vs-Stop flags the race; after the fix both readers +// snapshot under k.mu. + +import ( + "github.com/topcheer/ggcode/internal/config" + "sync" + "testing" + "time" +) + +func TestIssue2757StatusConcurrentWithStopNoRace(t *testing.T) { + k := &Knight{cfg: config.KnightConfig{Enabled: true}, running: true, budget: &Budget{}} + var wg sync.WaitGroup + stop := make(chan struct{}) + // Readers hammer Status/CanPerformTask while the writer flips running + // under k.mu - with the unlocked readers, -race flags the data race; + // after the fix (snapshot under k.mu) the same loop is clean. + for i := 0; i < 4; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + _ = k.Status() + _ = k.CanPerformTask() + } + } + }() + } + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + k.mu.Lock() + k.running = !k.running + k.mu.Unlock() + } + } + }() + time.Sleep(150 * time.Millisecond) + close(stop) + wg.Wait() +}