crypto: add uuidv7 monotonic counter#62601
Conversation
|
Review requested:
|
There was a problem hiding this comment.
Pull request overview
Adds a monotonic counter to crypto.randomUUIDv7() so UUIDv7 values are strictly increasing even when multiple UUIDs are generated within the same millisecond.
Changes:
- Implement monotonic
rand_acounter state for UUIDv7 generation (buffered + unbuffered paths). - Write timestamp + counter into UUIDv7 bytes before serialization.
- Tighten/extend tests to assert strict ordering, including burst generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/parallel/test-crypto-randomuuidv7.js | Updates ordering assertions to require strict monotonic UUID string ordering and adds a burst test. |
| lib/internal/crypto/random.js | Adds UUIDv7-specific buffered state and monotonic timestamp/counter logic used by randomUUIDv7(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| let prev = randomUUIDv7(); | ||
| for (let i = 0; i < 100; i++) { | ||
| const curr = randomUUIDv7(); | ||
| // UUIDs with later timestamps must sort after earlier ones. | ||
| // Within the same millisecond, ordering depends on random bits, | ||
| // so we only assert >= on the timestamp portion. | ||
| const prevTs = parseInt(prev.replace(/-/g, '').slice(0, 12), 16); | ||
| const currTs = parseInt(curr.replace(/-/g, '').slice(0, 12), 16); | ||
| assert(currTs >= prevTs, | ||
| `Timestamp went backwards: ${currTs} < ${prevTs}`); | ||
| // With a monotonic counter in rand_a, each UUID must be strictly greater | ||
| // than the previous regardless of whether the timestamp changed. | ||
| assert(curr > prev, | ||
| `UUID ordering violated: ${curr} <= ${prev}`); | ||
| prev = curr; | ||
| } | ||
| } | ||
|
|
||
| // Sub-millisecond ordering: a tight synchronous burst exercises the counter | ||
| // increment path and must also produce strictly increasing UUIDs. | ||
| { | ||
| const burst = []; | ||
| for (let i = 0; i < 500; i++) { | ||
| burst.push(randomUUIDv7()); | ||
| } | ||
| for (let i = 1; i < burst.length; i++) { | ||
| assert(burst[i] > burst[i - 1], | ||
| `Sub-millisecond ordering violated at index ${i}: ` + | ||
| `${burst[i]} <= ${burst[i - 1]}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new monotonic-ordering assertions only exercise the default (buffered) code path. Since this PR changes both buffered and disableEntropyCache: true (unbuffered) implementations, it would be good to add at least one ordering check for randomUUIDv7({ disableEntropyCache: true }) as well, to ensure the unbuffered counter/timestamp logic stays monotonic too.
|
Can you confirm that it’s intentional for |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62601 +/- ##
==========================================
- Coverage 89.70% 89.61% -0.10%
==========================================
Files 695 706 +11
Lines 214524 219238 +4714
Branches 41080 41999 +919
==========================================
+ Hits 192443 196468 +4025
- Misses 14121 14672 +551
- Partials 7960 8098 +138
🚀 New features to boost your workflow:
|
|
If this gets merged, #62600 should get reversed |
There was a problem hiding this comment.
#62553 (comment)
That can be a follow-up. It would reduce the UUID entropy so would probably need to be optional.
If this gets merged, #62600 should get reversed
In making this optional and documenting it it can also describe the nuance in the docs.
Why do you think it should be optional? |
First of all I don't think this is needed in the first place. Second, as @Renegade334's original comment that I've quoted says
|
I agree and made it optional via an argument, but I don't agree it's not needed, there's a lot of use cases where a counter is critical. The most used UUID v7 library does this: https://github.com/LiosK/uuidv7/blob/main/src/index.ts#L263 |
|
Is there a reason to not keep reusing uuidData/uuidBatch? |
| if (now > v7LastTimestamp) { | ||
| v7LastTimestamp = now; | ||
| v7Counter = seed & 0xFFF; | ||
| } else { | ||
| v7Counter++; | ||
| if (v7Counter > 0xFFF) { | ||
| v7LastTimestamp++; | ||
| v7Counter = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is problematic if the clock moves backwards significantly, as the timestamp ends up frozen until such a time as the clock catches up. I don't know how other implementations handle this possibility.
There was a problem hiding this comment.
From postgresql: https://raw.githubusercontent.com/postgres/postgres/dbf217c1c7c2744a18db489c255255e07cfbb110/src/backend/utils/adt/uuid.c
If the wall clock returns a value that isn't at least SUBMS_MINIMAL_STEP_NS ahead of the last call, the returned timestamp is synthetically bumped forward
There was a problem hiding this comment.
from uuidjs https://github.com/uuidjs/uuid/blob/main/src/v7.ts#L65 just ignored
| if (v7Counter > 0xFFF) { | ||
| v7LastTimestamp++; | ||
| v7Counter = 0; | ||
| } |
There was a problem hiding this comment.
Prematurely incrementing the timestamp is a permissible way to handle counter overflow in the RFC, but I think we need to be really clear in the documentation that the potential timestamp drift in the generated UUIDs is effectively unbounded, and could diverge ad infinitum if generating UUIDs at very high frequency.
Fixed! |
I'm confused. Is the reason sound enough so that the split is needed or not? presumably not if you reverted to use the existing batches |
Follow up #62553