Repository navigation
fix(queue): resolve data races and shutdown issues in worker queue - #282
Merged
Merged
Conversation
- Guard the items slice and all QueueItem state mutations with a sync.RWMutex so concurrent AddToQueue/GetQueueItems calls and the worker's updates can't race - Return detached item snapshots from AddToQueue and GetQueueItems so API handlers can safely marshal them while the worker processes jobs - trimItems: drop the oldest items instead of the newest ones and trim on the queue-full path too so the history stays bounded - Work: make idempotent so a second call no longer replaces the job channel and leaks the previous worker goroutine - Dispose: close the job channel under a lock so in-flight queueJob sends can't panic, wait for the worker to drain buffered/in-flight jobs, and make it safe to call multiple times; AddToQueue after Dispose now returns a clean error instead of panicking Add regression tests under lib/queue that fail on the old code with -race and cover trimming, Work idempotency, Dispose draining and post-Dispose queuing.
sebastianrakel
approved these changes
Oct 6, 2026
Member
|
LGTM |
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
2 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.
A race condition was found in
queue.gohandling of queue items with concurrency. TheQueueItemcould be accessed by each goroutine at the same time leading to issues where new channels and queues would be created orphaning old queues and losing data. This bug was found during the run of some localgo testruns with the-raceflag passed. Tests were updated to account for these changes.This PR Does the following things to fix the issue:
itemsslice and allQueueItemstate mutations so the concurrentAddToQueueandGetQueueItemscalls don't enter into a race with the worker's updates.AddToQueueandGetQueueItemso API handlers can safely marshal them while the worker processes jobs.trimItemsto drop the oldest item(s) instead of the newest ones and trim the queue-full path too so the history is bounded. May need some discussion on this.Work()function to make it idempotent so a second call no longer replaces the existing job channel and leaks the previous worker goroutine.Dispose()function so that it closes the job channel under a lock so in-flightqueueJobsends can't panic. It waits for the worker to drain buffered/in-flight jobs. It also is made idemopotent so if it's called more than once, it's safe.AddToQueueafterDisposenow returns a clean error instead of just panicking.The code in this PR was built in conjunction with AI, but not exclusively by it. The PR is written entirely by hand.