Conversation
|
Review requested:
|
nodejs-github-bot
added
crypto
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62601 +/- ##
==========================================
- Coverage 89.70% 89.70% -0.01%
==========================================
Files 695 695
Lines 214524 214549 +25
Branches 41080 41082 +2
==========================================
+ Hits 192443 192461 +18
+ Misses 14121 14119 -2
- Partials 7960 7969 +9
🚀 New features to boost your workflow:
|
Follow up #62553