Skip to content

crypto: add uuidv7 monotonic counter - #62601

Open
araujogui wants to merge 6 commits into
nodejs:mainfrom
araujogui:uuidv7-counter
Open

crypto: add uuidv7 monotonic counter#62601
araujogui wants to merge 6 commits into
nodejs:mainfrom
araujogui:uuidv7-counter

Conversation

@araujogui

Copy link
Copy Markdown
Member

Follow up #62553

CopilotAI review requested due to automatic review settings April 5, 2026 16:15
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 5, 2026

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_a counter 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.

FileDescription
test/parallel/test-crypto-randomuuidv7.jsUpdates ordering assertions to require strict monotonic UUID string ordering and adds a burst test.
lib/internal/crypto/random.jsAdds 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.

Comment threadtest/parallel/test-crypto-randomuuidv7.js
@harimm

Copy link
Copy Markdown

Can you confirm that it’s intentional for advanceV7() to let the encoded timestamp move ahead of current clock time in order to preserve strict monotonic ordering once the 12-bit space is exhausted within a millisecond? Relatedly, under sustained high-throughput generation across multiple consecutive milliseconds, should we expect that drift from wall clock to continue accumulating until the burst subsides? Out of curiosity, is the 12-bit limit here driven strictly by any spec, or was there any consideration of alternative approaches once that per-millisecond space is exhausted?

@codecov

codecovBot commented Apr 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.61%. Comparing base (dec5973) to head (e60619e).
⚠️ Report is 163 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/crypto/random.js92.98%4 Missing ⚠️
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 
Files with missing linesCoverage Δ
lib/internal/crypto/random.js95.73% <92.98%> (-0.34%)⬇️

... and 136 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexsch01

Copy link
Copy Markdown
Contributor

If this gets merged, #62600 should get reversed

panva
panva previously requested changes Apr 8, 2026

@panvapanva left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

@araujogui

Copy link
Copy Markdown
MemberAuthor

#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?

@panva

panva commented Apr 8, 2026

Copy link
Copy Markdown
Member

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

It would reduce the UUID entropy so would probably need to be optional.

@panva
panva dismissed their stale reviewApril 8, 2026 16:35

¯_(ツ)_/¯

Comment threadlib/internal/crypto/random.js Outdated
@araujogui

Copy link
Copy Markdown
MemberAuthor

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

It would reduce the UUID entropy so would probably need to be optional.

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

@panva
panva requested a review from tniessenApril 19, 2026 11:58
@panva

Copy link
Copy Markdown
Member

Is there a reason to not keep reusing uuidData/uuidBatch?

Comment on lines +425 to +434
if (now > v7LastTimestamp) {
v7LastTimestamp = now;
v7Counter = seed & 0xFFF;
} else {
v7Counter++;
if (v7Counter > 0xFFF) {
v7LastTimestamp++;
v7Counter = 0;
}
}

@Renegade334Renegade334Apr 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@araujoguiaraujoguiApr 22, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +430 to +433
if (v7Counter > 0xFFF) {
v7LastTimestamp++;
v7Counter = 0;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@araujogui

araujogui commented Apr 22, 2026

Copy link
Copy Markdown
MemberAuthor

Is there a reason to not keep reusing uuidData/uuidBatch?

Is there a reason to not keep reusing uuidData/uuidBatch?

Fixed!

@panva

Copy link
Copy Markdown
Member

Is there a reason to not keep reusing uuidData/uuidBatch?

getBufferedUUIDv7 writes back into the buffer slot before serializing, getBufferedUUID is read-only.

Is there a reason to not keep reusing uuidData/uuidBatch?

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

@araujogui

Copy link
Copy Markdown
MemberAuthor

Is there a reason to not keep reusing uuidData/uuidBatch?

getBufferedUUIDv7 writes back into the buffer slot before serializing, getBufferedUUID is read-only.

Is there a reason to not keep reusing uuidData/uuidBatch?

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

My bad, it was not needed, that's why I reverted.

@zdm

zdm commented May 30, 2026

Copy link
Copy Markdown

Will this PR be merged?
What is the problem?

@bricss

bricss commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

That would be great ✨ to make it move ✈️ towards the landing 🛬

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@araujogui@nodejs-github-bot@harimm@alexsch01@panva@zdm@bricss@Renegade334