Fix data race on shared *Agent.historyProvider by not mutating the interface field mid-run - #722
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes a data race in Agent when a shared *Agent is run concurrently by preventing mid-run mutation of the historyProvider interface field. Instead, it preserves the existing “clear-on-conflict” behavior by recording the global clear using an atomic.Bool flag that is safe to read/write concurrently.
Changes:
- Added
historyCleared atomic.BooltoAgentand stopped mutatingAgent.historyProviderduring runs. - Updated
historyProviderForSessionto disable history provider usage oncehistoryClearedis set. - Added a concurrent
-raceregression test covering the clear-on-conflict path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| agent/agent.go | Replaces racy historyProvider field mutation with an atomic “cleared” flag and gates provider usage on that flag. |
| agent/agent_test.go | Adds a concurrency test intended to exercise the previous race pattern under go test -race. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
handleHistoryProviderConflict cleared the configured provider by writing a.historyProvider = nil during a run, while historyProviderForSession read the same interface field unsynchronized on every run. A shared *Agent run concurrently (each with its own service-managed session) therefore raced on the two-word interface value, which go test -race flags with a possible torn read. Keep historyProvider immutable and track the global clear with an atomic.Bool (historyCleared). handleHistoryProviderConflict now stores true instead of niling the field, and historyProviderForSession returns nil early when the flag is set. This preserves the .NET clear-on-conflict semantics (the provider is cleared globally after a conflict) while removing the race.
56c6490 to
2cb8f9e
Compare
Parity Review: ✅ No cross-repo consistency issuesThis PR fixes a data race in the Go Scope assessment: Internal-only change.
Labels: No parity issues found. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
handleHistoryProviderConflictcleared the configured history provider by writinga.historyProvider = nilin the middle of a run, whilehistoryProviderForSessionreads that same interface field unsynchronized on every run. TheAgentstruct has no mutex andhistoryProvideris a plain interface value, so a shared*Agentrun concurrently (each call with its own service-managed session) races the write against the reads — a two-word interface value written and read without synchronization, whichgo test -raceflags and which can produce a torn read.A shared
*Agentis the intended concurrency unit here (sessions are per-conversation and the repo ships a concurrent agent-workflow example), so this is a real data race.Fix
historyProviderimmutable after construction.historyCleared atomic.BooltoAgent.handleHistoryProviderConflictnow callsa.historyCleared.Store(true)instead of niling the field.historyProviderForSessionreturnsnilearly whena.historyCleared.Load()is set.This preserves the existing .NET/Python clear-on-conflict semantics: once a provider promotes a session to service-managed and the conflict policy allows clearing (throw disabled, keep disabled), the configured history provider is cleared globally for subsequent runs. The behavior is identical; only the mechanism changes from mutating a shared interface field to setting an atomic flag, which removes the race.
Testing
go build ./...,go vet ./agent/...,go test -race ./agent/...all pass.TestAgent_Run_HistoryProvider_ConcurrentConflictClearIsRaceFreeinagent_test.go: it configures an agent with an explicitHistoryProvider,AllowHistoryProviderConflict=true,KeepHistoryProviderOnConflict=false, and a provider whoseRunpromotes the session to service-managed mid-run, then launches many goroutines callingRunconcurrently and drains each stream. Under-raceit reports a data race against the pre-fix code and passes after the fix. The existing clear-on-conflict test remains green.