fix(coderd/util/syncmap): match sync.Map semantics in the typed wrapper - #27582
Merged
Conversation
sync.Map.LoadOrStore guarantees that actual is usable whether the value was loaded or stored, which is what makes the load-or-create pattern work. The wrapper discarded the value on the store path and returned the zero V, so a caller doing `v, _ := m.LoadOrStore(k, new(T)); v.Use()` dereferenced nil on the first call for every pointer value type. No caller used the method yet, so nothing else changes. The new tests pin every wrapper method to the stdlib contract; three of them fail against the old LoadOrStore. Fixes CODAGT-869
The wrapper exists to keep sync.Map's untyped values off callers, and Swap leaked one back: it declared previous as any, so a caller had to type-assert the value it just handed in. It has no callers, so this breaks nothing.
mafredri
force-pushed
the
mathias/codagt-869-syncmap-loadorstore
branch
from
July 28, 2026 11:45
a82d82b to
21d288e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the coderd/util/syncmap typed wrapper to better match sync.Map semantics, specifically ensuring LoadOrStore returns the stored value (not V’s zero) and making Swap return the correctly typed previous value.
Changes:
- Fix
LoadOrStoreto return the actual stored value, matching the stdlib load-or-create contract. - Change
Swap’s return type fromanytoV, removing the need for redundant type assertions by callers. - Add a new test suite that pins wrapper behavior to
sync.Mapsemantics across all methods.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| coderd/util/syncmap/map.go | Adjusts LoadOrStore and Swap behavior/signatures to match sync.Map semantics. |
| coderd/util/syncmap/map_test.go | Adds coverage asserting the wrapper matches stdlib sync.Map behavior, including concurrency and typed returns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sync.Map stores a nil interface as a nil `any`, and a nil `any` cannot be type-asserted, so every read path panicked with "interface conversion: interface is nil" when V was an interface type holding nil. Copilot flagged LoadOrStore and Swap on this PR; Load, LoadAndDelete and Range had the same hole, so all five now go through one cast helper that maps nil to the zero V, which is what sync.Map handed back. The helper subsumes the not-found early returns, since a miss also yields nil. TestLoadOrStoreConcurrent now asserts exactly one goroutine stores, an invariant sequential execution cannot satisfy.
mafredri
marked this pull request as ready for review
July 28, 2026 12:43
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Emyrk
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CODAGT-869.
syncmap.Mapwrapssync.Mapto keep untyped values off callers, and three defects broke that contract in different directions.LoadOrStorethrew away the value it stored and returned the zeroV, so the load-or-create pattern that the stdlib contract guarantees (v, _ := m.LoadOrStore(k, new(T)); v.Use()) nil-dereferenced on the first call.Swapdeclaredprevious any, making callers assert a type they had already supplied. And every read path panicked withinterface conversion: interface is nilwhenVwas an interface type holding nil, becausesync.Mapreturns that as a nilany, which cannot be asserted. All five read paths now share onecast[T]helper that maps nil to the zeroT, matching whatsync.Mapreturned; that also subsumes the not-found early returns, since a miss yields nil too.Neither
LoadOrStorenorSwaphas an in-tree caller today (theLoadOrStorecall sites inchatdebug,reconnectingpty, andaibridge/circuitbreakerare all on stdlibsync.Map), so nothing behavioural changes elsewhere and the signature change breaks nothing. The bug surfaced while writing a test in #27547, where the workaround was load-then-store under a mutex.The package had no tests. The new ones pin every method to the stdlib contract, including the
Swapreturn type, which only compiles asV. They were checked against the old implementation: three fail onLoadOrStore,TestSwapTypedfails to compile, and the nil-interface subtests panic.Clearis still missing versus Go 1.23sync.Map; left alone since nothing needs it.