Improve concurrent package#2810
Merged
Merged
Conversation
- Buffer: add Len() and Bytes() methods with proper locking - Map: lazy-initialize values; Range() uses snapshot to allow safe callback mutations - Slice: initialize values to empty slice for consistency; document lock guarantees in Range, Find, Update - Add comprehensive tests for Buffer, Map; add AllEmpty test for Slice
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR improves the concurrent package and migrates callers to use it. The primitives themselves (Buffer, Map, Slice) look correct. However, two issues in the callers introduce real data races.
Add LoadOrStore method to concurrent.Map with double-checked locking pattern for efficient concurrent access. Simplifies snapshot.Manager.lock() by replacing manual sync.Mutex map management with the new LoadOrStore helper.
exec.Cmd spawns separate goroutines for Stdout and Stderr, both writing to the same buffer. bytes.Buffer is not safe for concurrent writes, so use concurrent.Buffer instead. Rename boundedBuffer to boundedWriter to reflect the actual interface.
dgageot
force-pushed
the
improve-concurrent
branch
from
dfa5d0a to
9153372
Compare
Member
Author
|
/review |
aheritier
approved these changes
May 18, 2026
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Reviewed 1082 lines across 16 files. The concurrent package improvements look well-implemented:
concurrent.Map:LoadOrStorecorrectly uses double-checked locking (read lock → re-check under write lock).Rangeclones the map under a read lock before iterating, so callbacks can safely mutate the map without deadlocking.ClearandDeletehold the write lock as expected.concurrent.Slice:All()returns aslices.Clonefor snapshot isolation.RangeandFinddocument the constraint that callbacks must not acquire the write lock.concurrent.Buffer: All operations (Write,String,Bytes,Drain) correctly hold the mutex.Bytes()returns aslices.Cloneto prevent external mutation of the internal buffer.- Callers (
fast_renderer.go,editfile/render.go,styles/composite.go,hooks/handler.go,hooks/model_handler.go,snapshot/snapshot.go,evaluation/progress.go,chatserver/conversation_lock.go): All refactored usages correctly replace manual mutex+map patterns with the new primitives. No races or logic errors introduced.
No bugs were found in the changed code.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Improve concurrent package and use it in more places