Skip to content

fix(core): lock CommandRegistry.agentDirs in SetAgentDirs and Resolve - #1662

Open
hi-neason wants to merge 1 commit into
chenhg5:mainfrom
hi-neason:fix/command-registry-locking
Open

fix(core): lock CommandRegistry.agentDirs in SetAgentDirs and Resolve#1662
hi-neason wants to merge 1 commit into
chenhg5:mainfrom
hi-neason:fix/command-registry-locking

Conversation

@hi-neason

Copy link
Copy Markdown

Summary

core/command.go guards the commands map with r.mu, but access to r.agentDirs was inconsistent: ListAll reads it under RLock, SetAgentDirs wrote it with no lock at all, and Resolve released the read lock before ranging over it. Today SetAgentDirs is only called at engine construction (engine.go:749), so there is no live crash, but the locking contract is misleading — any future runtime re-bind (or a go test -race run that overlaps setup with message handling) races on the slice header. ListAll in the same file already treats agentDirs as lock-guarded, so this just makes the other two methods follow that contract.

Change

  • SetAgentDirs: take r.mu.Lock() and copy the incoming slice before storing it, so later mutations by the caller do not leak into the registry under a reader.
  • Resolve: snapshot r.agentDirs under RLock before doing file IO, so the slice header is read safely without holding the lock across disk reads.
  • No behavior change for callers.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • Internal refactor / chore (no user-visible change)

Testing

Automated tests added in this PR

  • core/command_race_test.go
    • TestCommandRegistry_ConcurrentDirSwap — spawns writers that repeatedly swap agentDirs between two temp directories while readers call Resolve on a config command, agent commands, and ListAll. Fails under -race if either the write or the post-unlock read is unlocked.

For bug fixes only — regression test

  • Regression test name: TestCommandRegistry_ConcurrentDirSwap.
  • Manual verification this test catches the regression:
    • Reverted the production fix locally; go test -race -run TestCommandRegistry_ConcurrentDirSwap ./core/ failed with race detected during execution of test pointing at the unlocked SetAgentDirs write and the unlocked Resolve read.

Critical User Journeys (CUJ) impact

  • No CUJ touched (locking-only change; no behavior change).

Manual / user-visible behavior change

None.

Checklist (reviewer will verify)

  • go build ./... passes
  • go test ./... passes, including go test -race -run TestCommandRegistry_ConcurrentDirSwap ./core/
  • AGENTS.md Pre-Commit Checklist items are satisfied
  • No new hardcoded platform/agent names in core/
  • i18n strings have all-language translations (no user-facing strings added)
  • No secrets / credentials in source

Related

  • ListAll in the same file already reads agentDirs under RLock; this PR makes SetAgentDirs and Resolve follow that same contract.

SetAgentDirs wrote r.agentDirs with no lock held, while Resolve read the
same field after releasing r.mu.RUnlock(), even though ListAll accessed
it under RLock. Today the write happens at engine construction and does
not overlap with Resolve, but the inconsistent locking meant any future
runtime re-bind raced on the slice header under -race.

- Take r.mu.Lock in SetAgentDirs and copy the incoming slice so later
  caller mutations don't leak into the registry.
- Snapshot agentDirs under RLock in Resolve before doing file IO, so
  the lock is not held across disk reads but the slice header is read
  safely.
- Add a -race regression test that concurrently swaps agent dirs while
  other goroutines resolve config and agent commands.

Co-Authored-By: Claude <noreply@anthropic.com>
@hi-neason
hi-neason requested a review from chenhg5 as a code owner August 10, 2026 02:11

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结论: Approve

总体判断: 一个与 PR #1664 同模式的 concurrency fix——把 CommandRegistry.agentDirs 的访问统一到 r.mu 锁下,与既有 ListAll 的 RLock 约定对齐。修复面精准,race test 同样在 fix 前会红、fix 后绿。建议合入。

Review 范围:

  • 看了 core/command.goSetAgentDirs + Resolve 的锁添加 + slice copy 防御。
  • 看了新增 core/command_race_test.go (TestCommandRegistry_ConcurrentDirSwap)。
  • CI: run 31349096370 全绿。

✅ 做得好的地方:

  • #1664 模式:与 Discord race fix 一致的写法(Lock + defensive slice copy + snapshot-then-release),形成 codebase 内部统一的 concurrency pattern。
  • SetAgentDirs 主动 slice copy:即使 caller 后续修改原 slice,registry 内部 state 不受影响——避免「race-by-side-channel」类的隐蔽 bug。
  • Resolve snapshot 后释放锁再 IO:锁内只读 slice header,IO 在锁外,避免锁跨越 syscall / disk read。
  • Race test 覆盖多入口:writers 反复 swap + readers 从 config/agent commands + ListAll 三处并发读——验证「所有 reader 路径」都受保护,不是只测一个入口。

🟠 建议改进(不阻塞):

  • Resolve snapshot 后用 local slice range,但 agentDirs[i] 仍是 slice header 的间接引用:作者已经 copy 了 SetAgentDirs 入口的 slice,但 Resolve snapshot 的是 slice header 本身(slice 是 value type)。如果 writer 在 snapshot 后但 range 前修改了 underlying array 元素,会读到错的值。建议 Resolve 内对 snapshot slice 再 copy 一次,或确保 agentDirs 元素本身是 immutable(看现有实现是 string,应该 immutable,所以 OK——但加注释明确更稳)。
  • engine.go:749 是 SetAgentDirs 当前唯一调用点:作者指出 set happens only at engine construction,但 PR 没把这条调用注释加进去——建议在 SetAgentDirs 注释里写明「called once at engine construction; runtime re-bind is currently not supported」,避免 future 误用。

🔵 可选优化:

  • 无。

Testing / Risk:

  • 已看到的验证: -race 模式下 race test 通过;CI 全绿;fix revert 后 race test 失败(作者 self-verify)。
  • Blast radius: 仅 core/command.go,对调用者 no behavior change。

Next step:

  • 建议 owner 直接 merge。Concurrency fix 范围精准,与 #1664 同模式,race test 设计好。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants