fix(extensions): close ext.commands / ext.tools / lastFrameTime races
CI's `go test -race` flagged two races introduced over the recent extension work: 1. ext.commands / ext.tools were appended to from the read-loop goroutine without a lock, while Commands() / Tools() / HasCommand read them under m.mu. Same for the toolIndex pre-check. Fix: take m.mu around the appends and the index dedup so writers and readers serialise on the same lock. 2. assumeReadyAfterIdle read ext.lastFrameTime once on entry without the per-extension lock (the read inside the loop already had it). Fix: take ext.mu for the initial snapshot too. Verified locally with `go test -race ./...`; all packages pass. The corresponding CI run for the scratchpad-persistence commit failed for exactly these two races on linux + macos.
- commit
- 744685d68da4f7d535ea976391b4efa9db92a774
- author
- patriceckhart · · 2026-04-19 13:27 UTC
- parent
- 463c62d
- signature
- unsigned
- refs
- tag: v0.0.26
1 file · +8 -2
M internal/agent/extensions/manager.go +8 -2
| @@ -448,7 +448,9 @@ func (m *Manager) spawn(ctx context.Context, ext *Extension) error { | |||
| 448 | 448 | const readyIdleWindow = 250 * time.Millisecond | |
| 449 | 449 | ||
| 450 | 450 | func (m *Manager) assumeReadyAfterIdle(ext *Extension) { | |
| 451 | + | ext.mu.Lock() | |
| 451 | 452 | last := ext.lastFrameTime | |
| 453 | + | ext.mu.Unlock() | |
| 452 | 454 | for { | |
| 453 | 455 | select { | |
| 454 | 456 | case <-ext.readyCh: | |
| @@ -507,8 +509,12 @@ func (m *Manager) readLoop(ext *Extension, scanner *bufio.Scanner) { | |||
| 507 | 509 | case "register_command": | |
| 508 | 510 | var rc extproto.RegisterCommandFromExt | |
| 509 | 511 | if err := json.Unmarshal(line, &rc); err == nil { | |
| 510 | - | ext.commands = append(ext.commands, rc) | |
| 512 | + | // Both ext.commands and m.commandIndex are read by | |
| 513 | + | // the public Commands() / HasCommand() helpers under | |
| 514 | + | // m.mu, so the writes have to take the same lock to | |
| 515 | + | // keep the race detector happy. | |
| 511 | 516 | m.mu.Lock() | |
| 517 | + | ext.commands = append(ext.commands, rc) | |
| 512 | 518 | if _, exists := m.commandIndex[rc.Name]; !exists { | |
| 513 | 519 | m.commandIndex[rc.Name] = ext | |
| 514 | 520 | } | |
| @@ -529,8 +535,8 @@ func (m *Manager) readLoop(ext *Extension, scanner *bufio.Scanner) { | |||
| 529 | 535 | continue | |
| 530 | 536 | } | |
| 531 | 537 | } | |
| 532 | - | ext.tools = append(ext.tools, rt) | |
| 533 | 538 | m.mu.Lock() | |
| 539 | + | ext.tools = append(ext.tools, rt) | |
| 534 | 540 | if _, exists := m.toolIndex[rt.Name]; !exists { | |
| 535 | 541 | m.toolIndex[rt.Name] = ext | |
| 536 | 542 | } | |
| @@ -448,7 +448,9 @@ func (m *Manager) spawn(ctx context.Context, ext *Extension) error { | |||||
| 448 | const readyIdleWindow = 250 * time.Millisecond | 448 | const readyIdleWindow = 250 * time.Millisecond | ||
| 449 | 449 | ||||
| 450 | func (m *Manager) assumeReadyAfterIdle(ext *Extension) { | 450 | func (m *Manager) assumeReadyAfterIdle(ext *Extension) { | ||
| 451 | + | ext.mu.Lock() | |||
| 451 | last := ext.lastFrameTime | 452 | last := ext.lastFrameTime | ||
| 453 | + | ext.mu.Unlock() | |||
| 452 | for { | 454 | for { | ||
| 453 | select { | 455 | select { | ||
| 454 | case <-ext.readyCh: | 456 | case <-ext.readyCh: | ||
| @@ -507,8 +509,12 @@ func (m *Manager) readLoop(ext *Extension, scanner *bufio.Scanner) { | |||||
| 507 | case "register_command": | 509 | case "register_command": | ||
| 508 | var rc extproto.RegisterCommandFromExt | 510 | var rc extproto.RegisterCommandFromExt | ||
| 509 | if err := json.Unmarshal(line, &rc); err == nil { | 511 | if err := json.Unmarshal(line, &rc); err == nil { | ||
| 510 | - | ext.commands = append(ext.commands, rc) | 512 | + | // Both ext.commands and m.commandIndex are read by |
| 513 | + | // the public Commands() / HasCommand() helpers under | |||
| 514 | + | // m.mu, so the writes have to take the same lock to | |||
| 515 | + | // keep the race detector happy. | |||
| 511 | m.mu.Lock() | 516 | m.mu.Lock() | ||
| 517 | + | ext.commands = append(ext.commands, rc) | |||
| 512 | if _, exists := m.commandIndex[rc.Name]; !exists { | 518 | if _, exists := m.commandIndex[rc.Name]; !exists { | ||
| 513 | m.commandIndex[rc.Name] = ext | 519 | m.commandIndex[rc.Name] = ext | ||
| 514 | } | 520 | } | ||
| @@ -529,8 +535,8 @@ func (m *Manager) readLoop(ext *Extension, scanner *bufio.Scanner) { | |||||
| 529 | continue | 535 | continue | ||
| 530 | } | 536 | } | ||
| 531 | } | 537 | } | ||
| 532 | - | ext.tools = append(ext.tools, rt) | |||
| 533 | m.mu.Lock() | 538 | m.mu.Lock() | ||
| 539 | + | ext.tools = append(ext.tools, rt) | |||
| 534 | if _, exists := m.toolIndex[rt.Name]; !exists { | 540 | if _, exists := m.toolIndex[rt.Name]; !exists { | ||
| 535 | m.toolIndex[rt.Name] = ext | 541 | m.toolIndex[rt.Name] = ext | ||
| 536 | } | 542 | } | ||