Skip to content

Clone shared runOptions before appending per-run options - #555

Open
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:fix-agent-runoptions-shared-slice-race
Open

Clone shared runOptions before appending per-run options#555
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:fix-agent-runoptions-shared-slice-race

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

In prepareRun, the agent-configured run options were prepended with:

if len(a.runOptions) != 0 {
    options = append(a.runOptions, options...)
}

a.runOptions is a field shared across every run of the Agent, and New builds it with slices.Clone(cfg.RunOptions) followed by a per-tool append(..., WithTool(tool)), which routinely leaves the slice with spare capacity. Because of that spare capacity, append(a.runOptions, options...) writes each run’s per-run options — the synthesized WithSession(...)/noSessionProvided(true), or a caller’s WithServiceID — into a.runOptions[len:cap], memory shared across all runs.

When a single *Agent (with ≥1 configured tool, the normal case) serves concurrent runs — the natural usage for an agent framework — this is a data race, and one run can observe another run’s clobbered session/service ID reaching the provider. Sequential runs mask it because length-bounded reads never see runOptions[len:].

Every other analogous spot in the package already clones before appending a shared slice (New clones RunOptions, Middlewares, prov.Middlewares; invoke clones options); this prepend site missed it.

Fix

options = append(slices.Clone(a.runOptions), options...)

Test

TestAgent_ConcurrentRunsDoNotShareRunOptions builds an agent with tools (so runOptions has spare capacity) and issues 64 concurrent runs, each with its own WithServiceID/WithSession. Under -race it reports a DATA RACE in prepareRun on the old code and passes with the fix.

Copilot AI review requested due to automatic review settings July 19, 2026 11:16
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 19, 2026 11:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a concurrency/data-race bug where per-run options could be appended into the Agent’s shared runOptions backing array due to spare slice capacity, causing cross-run option corruption under concurrent runs.

Changes:

  • Clone a.runOptions in prepareRun before prepending run-scoped options to avoid mutating shared backing storage.
  • Add a regression test that runs many concurrent agent runs (with configured tools to ensure spare capacity) and relies on -race to detect the prior data race.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
agent/agent.go Clones shared runOptions before append to prevent concurrent mutation of the shared slice backing array.
agent/agent_test.go Adds a -race regression test covering concurrent runs to ensure options aren’t shared/mutated across runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PratikDhanave
PratikDhanave force-pushed the fix-agent-runoptions-shared-slice-race branch 2 times, most recently from ca4688e to 342317c Compare July 23, 2026 15:44
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 23, 2026
@PratikDhanave
PratikDhanave force-pushed the fix-agent-runoptions-shared-slice-race branch from 342317c to 60c9a6c Compare July 24, 2026 01:42
@github-actions

This comment has been minimized.

prepareRun prepended the agent-configured run options with
append(a.runOptions, options...). a.runOptions is shared across every run
of the Agent and is built in New with slices.Clone followed by a per-tool
append, so it routinely carries spare capacity. Appending onto it wrote
each run's per-run options (the synthesized WithSession/noSessionProvided,
or a caller's WithServiceID) into that shared backing array.

Under concurrent runs of one Agent this is a data race, and one run can
observe another run's clobbered session/service ID. Clone runOptions
before appending so each run gets a private slice.
@PratikDhanave
PratikDhanave force-pushed the fix-agent-runoptions-shared-slice-race branch from 60c9a6c to 69d4677 Compare July 24, 2026 09:37
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review: No Issues Found

This PR fixes an internal data race in prepareRun by cloning a.runOptions before appending per-run options (slices.Clone(a.runOptions)). No exported Go types, methods, functions, or observable user-facing behaviors are changed.

Scope: Internal unexported logic only — prepareRun, a.runOptions (unexported field), and concurrency safety of per-run option merging.

Cross-repo parity: Not applicable. This is a Go-internal concurrency correctness fix with no equivalent API surface in the upstream .NET or Python implementations to compare against.

Label actions: parity-approved label correctly remains; public-api-change label is not warranted.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 18.5 AIC · ⌖ 4.88 AIC · ⊞ 5.9K ·

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

Labels

parity-approved Go API consistency review found no parity issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants