Skip to content

fix(teamloader): LoadWithConfig races on shared RuntimeConfig #4239

Description

@aheritier

Summary

The ci / test-race job on main is intermittently failing due to a real data race in pkg/teamloader.LoadWithConfig. The behavior is schedule-dependent, but the underlying concurrent access is deterministic—not runner or network flakiness.

Observed CI behavior

Recent main runs have alternated between pass and failure at effectively the same head, including failures in runs 34603115677, 34601273521, and 34579008635, followed by a passing run 34603490548.

The race report consistently points to concurrent access in LoadWithConfig:

  • Read: pkg/teamloader/teamloader.go:297 (config.MergeGlobalProviders reading runConfig.Providers)
  • Write: pkg/teamloader/teamloader.go:378 (mutating runConfig.Providers; the function also mutates Models and ProviderRegistry)

A representative race trace is:

WARNING: DATA RACE
Read at ... by goroutine 473:
  LoadWithConfig() teamloader.go:297
  strict_test.go:185

Previous write at ... by goroutine 472:
  LoadWithConfig() teamloader.go:378
  strict_test.go:178

Trigger

pkg/teamloader/strict_test.go's TestLoadExternalAgent_InheritsLoaderOptions creates one shared *config.RuntimeConfig and passes it into two t.Parallel() subtests. Each calls teamloader.Load, with recursive loading through loadExternalAgent.

LoadWithConfig mutates the supplied RuntimeConfig. It has a partial clone safeguard when a caller provides a working directory different from runConfig.WorkingDir, but does not clone when concurrent callers share the same config and working directory.

The race detector may attribute the failure to different concurrently-running tests in the package; those varying test names are collateral, rather than separate root causes.

Impact

  • ci / test-race on main is unreliable.
  • The current implementation may have a production thread-safety gap if API-server sessions or other callers can invoke LoadWithConfig concurrently with the same RuntimeConfig and working directory.

Proposed remediation

  1. Immediate test isolation: update TestLoadExternalAgent_InheritsLoaderOptions so each parallel subtest creates or receives its own RuntimeConfig; audit nearby teamloader tests for equivalent shared-config patterns.
  2. Decide and enforce the API contract: if LoadWithConfig is expected to support concurrent callers sharing one config, defensively clone the RuntimeConfig on every call (or otherwise stop mutating the caller-owned object). If not, document and enforce the ownership/concurrency requirement at call sites.
  3. Re-run and monitor ci / test-race after this repair. Track the independent E2E chat-server timeout and historical Docker-cancel race separately if they persist.

Context

PR #4228 addressed earlier pkg/evaluation and pkg/tui race findings, but did not modify pkg/teamloader or the relevant E2E tests; the teamloader race continued afterward.

Activity

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

Metadata

Metadata

Assignees

Labels

area/ciCI/CD workflows and pipelinearea/testingTest infrastructure, CI/CD, test runners, evaluation

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions