fix: clock sequence race and add stress test;#219
fix: clock sequence race and add stress test;#219atlet99 wants to merge 14 commits intogofrs:masterfrom
Conversation
Signed-off-by: Abdurakhman R. <joha.shadibekov@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #219 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 457 467 +10
=========================================
+ Hits 457 467 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a race condition in Version-1 UUID generation's clock sequence handling and adds comprehensive stress testing to verify the fix. The issue occurred when the 14-bit clock sequence counter overflowed without proper handling, potentially causing UUID collisions.
- Implements proper 14-bit clock sequence wrapping with bitmask operation
- Adds mandatory timestamp advancement when sequence wraps to zero
- Introduces stress test with table-driven concurrent scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| generator.go | Fixed clock sequence race condition by adding proper 14-bit wrapping and timestamp advancement |
| race_v1_test.go | Added comprehensive concurrent stress test to verify UUID uniqueness under high contention |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Hi, I'd still like to see about getting this merged in :) I dont want to obligate you to any contribution you don't have time for - would it be ok if I picked this up and finished it? |
…ected high CPU usage Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| break | ||
| } | ||
| // Sleep briefly to avoid busy-waiting and reduce CPU usage. | ||
| time.Sleep(time.Microsecond) |
There was a problem hiding this comment.
worst case this has a repeating wait of 10 microseconds.
There was a problem hiding this comment.
Yep, that makes sense!
There was a problem hiding this comment.
I found some references that say that the minimum OS timer resolution on Linux can be 50+ microseconds and as much as 15ms on WIndows. That could make this loop VERY slow. 😬
This fixes the clock sequence overflow issue, but what does it do to the benchmarks for V1, V6, and V7 values? Since we want to wait a tiny amount of time this seems like a good place for runtime.Gosched() (so we only yield the current time slice on the scheduler).
There was a problem hiding this comment.
@dylan-bourque I'm not sure this solution will give us much of a gain. This loop with time.Sleep(time.Microsecond) is called only when the clockSequence wraps (once every 16,384 UUIDs with the same timestamp), not on every NewV1/NewV6/NewV7.
I compared Sleep vs. runtime.Gosched() locally on the BenchmarkGenerator/NewV1|NewV6|NewV7 benchmarks (3 runs each, Apple M3 Pro).
Here are the benchmarks:
NewV1: Sleep ~44-45 ns/op, Gosched ~45 ns/op
NewV6: Sleep ~119 ns/op, Gosched ~119-120 ns/op
NewV7: Sleep ~145 ns/op, Gosched ~145 ns/op
|
Hi @atlet99 thank you very much for contributing this change. I plan to get this merged as soon as I can get another reviewer to look. I took the liberty to change the proposed logic to use a microsecond sleep instead of relying on GoSched(). Documentation suggested this was the safer approach. |
|
@cameracker I'm glad my changes are helping. Unfortunately, I was busy for a while and couldn't respond to your message. In the future, I'll be happy to provide my assistance if required. |
generator.go
Outdated
| // If the sequence wrapped (back to zero) we MUST wait for the | ||
| // timestamp to advance to preserve uniqueness (see RFC-9562 §6.1). | ||
| if g.clockSequence == 0 { | ||
| for { |
There was a problem hiding this comment.
I think this loop code be tidied up a bit, but the logic looks correct.
There was a problem hiding this comment.
What change do you propose? @dylan-bourque
There was a problem hiding this comment.
I was thinking of something like this
for ; timeNow <= g.lastTime; timeNow = now() {
time.Sleep(time.Microsecond) // or runtime.Gosched(), see my comment below
}
where now() is a locally defined helper that wraps the if/else timestamp logic
now := func() uint64 {
epoch := g.epochFunc()
if useUnixTSMs {
return uint64(epoch.UnixMilli())
}
return g.getEpoch(epoch)
}
I'm not 100% convinced I like that better, though.
|
On mobile right now but I'll follow up later @atlett99
…On Mon, Mar 9, 2026, 4:57 PM Abdurakhman Rakhmankulov < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In generator.go
<#219 (comment)>:
> @@ -434,7 +434,27 @@ func (g *Gen) getClockSequence(useUnixTSMs bool, atTime time.Time) (uint64, uint
// Clock didn't change since last UUID generation.
// Should increase clock sequence.
if timeNow <= g.lastTime {
- g.clockSequence++
+ // Increment the 14-bit clock sequence (RFC-9562 §6.1).
+ // Only the lower 14 bits are encoded in the UUID; the upper two
+ // bits are overridden by the Variant in SetVariant().
+ g.clockSequence = (g.clockSequence + 1) & 0x3fff
+
+ // If the sequence wrapped (back to zero) we MUST wait for the
+ // timestamp to advance to preserve uniqueness (see RFC-9562 §6.1).
+ if g.clockSequence == 0 {
+ for {
What change do you propose? @dylan-bourque
<https://github.com/dylan-bourque>
—
Reply to this email directly, view it on GitHub
<#219 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEHWW64G5KHQLCYPYPA2M6D4P442ZAVCNFSM6AAAAACAZTODL2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTSMJYGQ2DONJRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
LGTM, but wait for @dylan-bourque too since he said he's going to look at it.
|
I've been playing with this locally to answer my own question about benchmarks and I'm still able to generate duplicate V1 values with this change at 2000 goroutines X 1000 values. 🤔 FWIW, I don't actually see much difference in the benchmarks (I had to write a new one), but the fix is not complete. |
I think I also need to conduct some tests and find out again what can come out of this; |
|
@dylan-bourque u were right about your doubts, I did make mistakes.
So, guys, im providing the changes for analysis and verification, IMHO, stress test with |
|
UPD: some changes for testing Added follow-up fixes and test coverage to close the remaining V1 duplication gap under heavy concurrency.
P.S. if u wanna run test |
| } | ||
| mu.Lock() | ||
| if _, exists := seen[u]; exists { | ||
| dupCount++ |
There was a problem hiding this comment.
nit: should make this atomic.AddUint32() as well, if only for consistency and to avoid someone coming along later and asking why it isn't.
| var ( | ||
| wg sync.WaitGroup | ||
| mu sync.Mutex | ||
| seen = make(map[UUID]struct{}, goroutines*uuidsPerGor) |
There was a problem hiding this comment.
nit: in my testing, I made this map[UUID]int32 so that the failure output could report the actual duplicate values and how many of each were generated.
mu.Lock()
if cnt, exists := seen[u]; exists {
seen[u] = cnt + 1
} else {
seen[u] = 1
}
mu.Unlock()
and
for v, n := range seen {
if n > 1 {
t.Errorf("duplicate V1 UUID: %s appeared %d time(s)", v, n)
}
}
definitely not necessary, but it makes the failure output more useful
| // Calls can arrive with stale atTime values (captured before acquiring the | ||
| // lock). Clamp backwards timestamps to the latest emitted one to avoid | ||
| // reusing older timestamp + clock-sequence pairs after sequence wrap. | ||
| if timeNow < g.lastTime { |
There was a problem hiding this comment.
could move this inside of the if timeNow <= g.lastTime block below since that check will always also be true. eliminating the branch might also make it slightly faster.
if timeNow <= g.lastTime {
// Calls can arrive with stale atTime values (captured before acquiring the
// lock). Clamp backwards timestamps to the latest emitted one to avoid
// reusing older timestamp + clock-sequence pairs after sequence wrap.
timeNow = g.lastTime
...
}
Issue - #216