fix: prevent UUIDv7 counter wrap after 4096 ids - #253
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes UUIDv7 monotonic ordering issues caused by counter wrap and shared state with other UUID versions, ensuring generated V7 UUIDs remain strictly increasing under high-throughput and mixed-version usage.
Changes:
- Introduces dedicated V7 counter/timestamp state and sequence logic, removing V7’s dependency on the V1/V6 clock sequence path.
- Implements 12-bit dedicated counter seeding/reseeding and rollover handling per RFC 9562 guidance, including bounded “timestamp borrow” behavior.
- Expands test coverage substantially to validate rollover, bounded borrow, backwards clock behavior, mixed V1/V7 usage, concurrency, and RNG failure paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| generator.go | Splits V7 sequencing from V1/V6 clock sequence logic; adds dedicated V7 counter/timestamp state and rollover handling. |
| generator_test.go | Adds targeted regression and stress tests for V7 ordering, rollover behavior, concurrency, and error paths; updates existing assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #253 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 457 480 +23
=========================================
+ Hits 457 480 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kohenkatz
left a comment
There was a problem hiding this comment.
Looks good to me, other than the one documentation adjustment suggested.
Thank you for working on this!
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Thanks for the review @kohenkatz! i accepted copilot's suggestions to fix the comments. Should be good to merge if everything looks ✅ to you. |
dylan-bourque
left a comment
There was a problem hiding this comment.
LGTM as well.
nit: I'd like to see a "load test" that generates a very large number of UUIDs (much larger than 50K, maybe 1M) and validates that they are sequential.
|
great idea @dylan-bourque. Just added |
|
@kohenkatz @dylan-bourque let me know if there's anything else y'all want added to get this over the line |
|
@mistermoe thank you very much for this contribution :) It's great that you nailed down a problem that was messing up our sorting guarantees, and the regression test will help us make sure our sorting continues to work going forward. Really appreciate your effort here. I'll release this shortly. |
Why
NewV7documents support for single-node batch generation with a monotonic counter, but roughly one id in every 4096 sorts below its predecessor. In such a pair the 48-bit timestamp and the version nibble are identical while the counter inrand_arolls fromfffback to000.Three things combined to cause it.
g.clockSequencewas seeded once from random bytes and then only ever incremented, never reset when the timestamp advanced, so its wrap point had no relationship to millisecond boundaries.NewV7AtTimewrote the fulluint16to bytes 6 and 7 and then calledSetVersion(V7), which overwrites the high 4 bits, so only the low 12 bits reached the id and the encoded counter wrapped every 4096 generations. Finally,lastTimewas shared between V1 and V6, which count 100-nanosecond intervals since 1582, and V7, which counts milliseconds since 1970. On a generator producing both, the V1 value dwarfs any V7 millisecond, so every V7 call took the "clock didn't change" branch and incremented the counter regardless of the millisecond, reaching the wrap sooner.Because the counter was seeded randomly, the first wrap landed after an arbitrary 0 to 4095 ids and in a different place on every process start, which is why this surfaces as intermittent misordering in downstream systems rather than as an obvious failure. The existing
ClockSequencetest asserted ordering over 10 ids, which straddles the 12-bit boundary with probability near 0.24% and so passed essentially always.What
V7 now has its own counter state and its own code path, and no longer calls
getClockSequence. Separate state is the smallest correct fix because one counter cannot serve both callers: V1 writes to bytes 8 and 9 whereSetVariantclaims 2 bits, leaving 14 usable, while V7 writes to bytes 6 and 7 whereSetVersionclaims 4, leaving 12. Any single mask is wrong for one of them. Splitting the state also resolves the unit mismatch described above, so V1 and V7 no longer interfere on a shared generator.The counter is a dedicated 12-bit field as described by RFC 9562 section 6.2, Method 1. It is reseeded from 11 random bits at the start of each millisecond, with the leading bit left zero as a rollover guard, following the "Fixed Bit-Length Dedicated Counter Seeding" guidance. Seeding randomly rather than from zero keeps the entropy the previous implementation had in
rand_a, and the guard bit guarantees at least 2048 increments within a tick regardless of the seed drawn.When the counter is exhausted within a tick, the embedded timestamp is incremented ahead of the actual time and the counter reseeded. RFC 9562 section 6.2 permits either this or freezing the counter until the clock advances. Freezing is not available here, because the timestamp can be supplied by the caller through
NewV7AtTimeor by a fixedEpochFunc, and is then under no obligation to ever advance; waiting on it would hang. The cost is bounded and observable: the timestamp runs ahead by one millisecond per rollover, so it stays accurate below roughly two million ids per second.NewV7andNewV7AtTimenow differ in how they treat a timestamp that moves backwards, and both call a sharednewV7with a flag that selects the behavior.NewV7reads the clock itself, so a timestamp below the last one used means the clock was corrected backwards, and it is held at the last value to keep ordering intact, per the "Monotonic Error Checking" guidance in the same section.NewV7AtTimereceives its timestamp from the caller, so an older timestamp is a deliberate request for an older id and is encoded as given, with the counter restarting there. Clamping in that case would meanuuid.NewV7AtTime(historicalTime)silently returning the current time on the package level default generator, which is a worse outcome than the caller getting the timestamp they asked for. The consequence, that ordering underNewV7AtTimeonly holds for timestamps that do not move backwards, is now stated in its documentation, along with the fact that a timestamp ahead of the clock holds subsequentNewV7ids at that timestamp.Timestamps outside the range the 48-bit millisecond field can represent are pinned to the nearest end of that range before they reach the counter state. Without this, the negative
UnixMilliof a zerotime.Timewould convert to a value near 1.8e19, enter the state as a timestamp far in the future, and hold every subsequent id from that generator there.Two smaller changes come with it. The
useUnixTSMsparameter ongetClockSequenceis dead once V7 stops calling it, so it and its branch are removed in a separate commit. TheAtSpecificTimetest compared the first 7 bytes of two ids sharing a timestamp; byte 6 now carries counter bits by design, so it compares the 6 timestamp bytes and asserts ordering instead.This does not conflict with #219. Its 14-bit mask is correct for V1 and wrong for V7, and removing V7 from the shared helper leaves that patch correct as written for the only versions still using it.
Verification
The reproduction from the issue goes from 3 inversions in 10,000 pinned-epoch ids and 5 in 20,000 real-clock ids to 0 in both. Ten new subtests cover rollover across 50,000 ids in one millisecond, the bound on how far the timestamp runs ahead, reseeding on a new tick, interleaving with V1, a backwards clock, historical and unrepresentable timestamps, concurrency, and both random-source failure paths.
CounterRolloverandMixedWithV1were confirmed to fail against unpatched master. Statement coverage stays at 100%, andBenchmarkGenerator/NewV7is unchanged at about 92 ns/op with one allocation, even though the benchmark generates around 10,000 ids per millisecond and therefore rolls the counter over continuously.