Skip to content

refactor: replace single-variable mutexes with atomic types - #3330

Merged
Sayt-0 merged 3 commits into
docker:mainfrom
dgageot:refactor/mutex-to-atomic
Jun 30, 2026
Merged

Sayt-0 merged 3 commits into
docker:mainfrom
dgageot:refactor/mutex-to-atomic

Conversation

@dgageot

@dgageot dgageot commented Jun 30, 2026

Copy link
Copy Markdown
Member

Each of the three structs changed here used a sync.Mutex or sync.RWMutex to protect exactly one mutable field — all other fields were written once at construction and never changed afterward. When a mutex guards a single variable, the standard library's sync/atomic package offers a simpler and more self-documenting fit: the data and its synchronization are expressed as one value rather than two separate declarations.

lspSession in pkg/tools/builtin/lsp/lsp_lifecycle.go had a closed bool protected by a plain mutex. It is now an atomic.Bool, with CompareAndSwap in Close() to preserve idempotency and Load() in the wait goroutine to check the flag. Client in pkg/model/provider/rulebased/client.go had a lastSelectedID of type modelsdev.ID behind an RWMutex; it is now an atomic.Pointer[modelsdev.ID]. agentRouter in pkg/runtime/agent_router.go had a current string behind an RWMutex; it is now an atomic.Pointer[string].

All changes are behavior-preserving. The suite passes under go test -race and the linter reports no new issues.

@dgageot
dgageot requested a review from a team as a code owner June 30, 2026 15:18

@docker-agent docker-agent 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.

Assessment: 🟢 APPROVE

The refactor correctly replaces single-variable mutexes with atomic types across all three files:

  • **** → with in for idempotency and in the wait goroutine — both uses are correct.
  • **** → — nil checks are in place before dereferencing the loaded pointer.
  • **** → in — nil is handled correctly.

The removed mutexes each guarded exactly one mutable field (as described), so no other fields are left unprotected. The changes are behavior-preserving and idiomatic Go.

@docker-agent docker-agent 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.

Assessment: 🟢 APPROVE

The refactor correctly replaces single-variable mutexes with atomic types across all three files. No bugs were found in the changed code.

  • lspSession.closed → atomic.Bool: CompareAndSwap(false, true) in Close() correctly preserves idempotency; Load() in the wait goroutine is the right read primitive.
  • lastSelectedID → atomic.Pointer[modelsdev.ID]: nil is handled before dereferencing the loaded pointer.
  • current → atomic.Pointer[string] in agentRouter: nil is handled correctly on load.

The removed mutexes each guarded exactly one mutable field, so no other fields are left without synchronization. The changes are behavior-preserving and idiomatic Go.

@docker-agent docker-agent 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.

Assessment: 🟢 APPROVE

The refactor correctly replaces mutex-protected single fields with sync/atomic types across all three files.

lspSession.closed → atomic.Bool: The CompareAndSwap(false, true) in Close() correctly preserves idempotency. The happens-before ordering (CAS → processCancel → process exit → cmd.Wait()) ensures the wait goroutine observes closed=true after a signal-induced exit.

Client.lastSelectedID → atomic.Pointer[modelsdev.ID]: Taking the address of the local selectedID variable correctly heap-escapes it via the Go compiler. The nil-check on Load() properly mirrors the old zero-value behavior on first call.

agentRouter.current → atomic.Pointer[string]: newAgentRouter always calls Store before the struct is shared, and the nil guard in Name() is a correct and harmless safety net.

No bugs introduced. All changes are behavior-preserving.

@aheritier aheritier added area/providers For features/issues/fixes related to LLM providers (Bedrock, LiteLLM, Qwen, custom, etc.) area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/refactor PR refactors code without behavior change labels Jun 30, 2026
@Sayt-0
Sayt-0 merged commit b2c3df5 into docker:main Jun 30, 2026
10 checks passed
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
docker-agent 1.93.0

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>This release adds OpenRouter as a built-in provider, introduces per-model gateway bypass support, and allows a dedicated model for session compaction, alongside several internal refactoring improvements.

## What's New
- Adds OpenRouter as a first-class built-in provider; when `OPENROUTER_API_KEY` is set, the provider is auto-detected and its base URL resolved automatically
- Adds a `compaction_model` field to allow a dedicated model for session summary generation, separate from the primary agent model
- Adds a `bypass_models_gateway` boolean field on a per-model basis, allowing specific models to connect directly to their provider instead of routing through the configured models gateway

## Technical Changes
- Replaces mutex usage with atomic types (`atomic.Bool`, `atomic.Pointer`) for single mutable fields in several structs
- Replaces package-level global variables with explicit parameters or struct fields to enable parallel-safe testing across several packages (`toolinstall`, `runtime`, `mcp`, `notification`, `tui`, `cmd/root`)
- Adds a concurrency group to the PR review trigger CI workflow to prevent duplicate reviews for the same PR
---

## What's Changed
* docs: update CHANGELOG.md for v1.92.0 by @docker-read-write[bot] in docker/docker-agent#3331
* refactor: replace single-variable mutexes with atomic types by @dgageot in docker/docker-agent#3330
* ci: add concurrency group to pr-review-trigger to prevent duplicate reviews by @derekmisler in docker/docker-agent#3334
* refactor: replace test-overridden globals with parallel-safe injection by @dgageot in docker/docker-agent#3332
* feat(compaction): allow a dedicated model for session summary generation by @Sayt-0 in docker/docker-agent#3335
* feat: add OpenRouter provider by @dgageot in docker/docker-agent#3337
* feat: add per-model bypass_models_gateway option by @dgageot in docker/docker-agent#3338


**Full Changelog**: docker/docker-agent@v1.92.0...v1.93.0
</pre>
  <p>View the full release notes at <a href="https://github.com/docker/docker-agent/releases/tag/v1.93.0">https://github.com/docker/docker-agent/releases/tag/v1.93.0</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!13152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/providers For features/issues/fixes related to LLM providers (Bedrock, LiteLLM, Qwen, custom, etc.) area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/refactor PR refactors code without behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants