Skip to content

Add UUIDv7 node partitioning#78

Merged
dexcompiler merged 5 commits into
mainfrom
codex/68-node-partitioned-uuidv7
May 15, 2026
Merged

Add UUIDv7 node partitioning#78
dexcompiler merged 5 commits into
mainfrom
codex/68-node-partitioned-uuidv7

Conversation

@dexcompiler
Copy link
Copy Markdown
Owner

Summary

  • add UuidV7NodePartition for opt-in 1-16 bit rand_b node/shard/process/deployment discriminators
  • wire node partitioning through UuidV7Factory, Guid extraction helpers, and DI registration
  • add deterministic C# tests, F# properties, benchmark demo coverage, and a design note comparing alternatives

Validation

  • dotnet test tests/Clockworks.Tests.csproj --filter UuidV7NodePartitionTests -v minimal
  • dotnet test property-tests/Clockworks.PropertyTests.fsproj --filter "FullyQualifiedName~UuidV7FactoryProperties" -v minimal
  • dotnet test Clockworks.sln -v minimal -m:1
  • DOCS_VERSION=dev npm run docs:build (from docs/)
  • dotnet pack src/Clockworks.csproj -c Release -v minimal
  • dotnet run --project demo/Clockworks.Demo -- uuidv7 --bench

Benchmark sample from this machine:

  • default NewGuid: 2,392,182 ops/sec (418.0 ns/op)
  • node-partitioned NewGuid: 2,532,274 ops/sec (394.9 ns/op, -5.5% vs default sample)
  • default NewGuids batch: 2,579,133 ops/sec (387.7 ns/op)
  • node-partitioned NewGuids batch: 2,504,980 ops/sec (399.2 ns/op, 3.0% vs batch default)

Fixes #68

@dexcompiler
Copy link
Copy Markdown
Owner Author

@copilot review this pr

@dexcompiler
Copy link
Copy Markdown
Owner Author

@codex[agent] review this pr

@chatgpt-codex-connector
Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 15, 2026

@copilot review this pr

Code Review — Add UUIDv7 Node Partitioning

Overall: Solid implementation. The bit-layout math is correct across all three width ranges (≤6, 7–14, 15–16 bits), RFC variant bits are preserved correctly, and the lock-free path is unchanged. All 211 tests pass.


Issues

1. GetNodePartitionId throws instead of returning null on invalid width
All the other Get* helpers (e.g. GetTimestampMs, GetCounter) return null for bad input — GetNodePartitionId throws ArgumentOutOfRangeException if nodeIdBitWidth is outside 1–16. This breaks the pattern.

// Current — throws for bad width
public ushort? GetNodePartitionId(byte nodeIdBitWidth)
{
    UuidV7NodePartition.ValidateBitWidth(nodeIdBitWidth);  // throws
    ...
}

Callers expecting the same null-return contract will be surprised. Consider returning null for an out-of-range width.

2. DI statistics inconsistency in the full AddNodePartitionedGuidFactory overload
AddLockFreeGuidFactory resolves statistics from the service locator (sp.GetRequiredService<UuidV7FactoryStatistics>()), but the new AddNodePartitionedGuidFactory captures statistics as a closure:

// New overload — closure capture (different from existing pattern)
services.AddSingleton<IUuidV7Factory>(sp => new UuidV7Factory(
    sp.GetRequiredService<TimeProvider>(),
    nodePartition, rng, overflowBehavior,
    statistics));   // ← closure, not sp.GetRequiredService

Functionally identical since the same instance is registered, but it diverges from the established pattern and can confuse future maintainers.

3. Missing test for the simple AddNodePartitionedGuidFactory overload
AddNodePartitionedGuidFactory(UuidV7NodePartition, CounterOverflowBehavior) (the TimeProvider.System variant) has no test. The existing test only exercises the full overload that accepts TimeProvider.

4. ValidateBitWidth is internal — leaks into the assembly surface
It's called from the GuidExtensions helper and tests, but marking it internal exposes it in the assembly API. Since the extensions file is in the same assembly, a private static helper in GuidExtensions or a simple in-line guard would keep the surface cleaner.


What's correct

  • Bit encoding/decoding is verified correct for all width ranges: the read/write round-trip is exact.
  • ApplyTo sets variant bits (10xxxxxx) before WriteNodeId, preserving RFC 9562 compliance.
  • NodePartition property returns null (not the default struct) when partitioning is disabled — good discriminator.
  • _nodePartition is readonly; no locking needed in CreateGuidFromState.
  • FsCheck properties cover round-trip and stream separation — good coverage for a stateful generator.
  • MaxNodeIdBitWidth = 16 is safe: (ushort)((1 << 16) - 1) evaluates correctly in C# int arithmetic before the cast.

Co-authored-by: dexcompiler <115876036+dexcompiler@users.noreply.github.com>
@Codex
Copy link
Copy Markdown
Contributor

Codex AI commented May 15, 2026

@codex[agent] review this pr

Pushed follow-ups in 19b8642: Guid.GetNodePartitionId now returns null for invalid widths (no throw), AddNodePartitionedGuidFactory resolves UuidV7FactoryStatistics from DI when provided, and tests now cover the system-time overload + invalid-width extraction.

@chatgpt-codex-connector
Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@dexcompiler
Copy link
Copy Markdown
Owner Author

Verification follow-up after the Copilot/Codex review pass:

  • Verified remote commit 19b8642. The changes are correct and in-scope: invalid node-partition widths now return null from Guid.GetNodePartitionId, the full DI overload resolves UuidV7FactoryStatistics from the container when supplied, ValidateBitWidth is private again, and tests now cover invalid-width extraction plus the system-time AddNodePartitionedGuidFactory overload.
  • I looked for a lower-overhead partitioned hot path and added a follow-up commit a7c2465. UuidV7NodePartition now precomputes byte masks/OR values once at construction, so each generated UUID applies the partition with fixed byte operations instead of recomputing width-dependent masks and shifts.
  • No adjacent issue found that needs a separate GitHub issue. The remaining operational caveat is already documented: node partitioning depends on correct non-overlapping node/shard assignment and still benefits from storage uniqueness constraints in high-assurance domains.

Validation after the follow-up:

  • dotnet test tests/Clockworks.Tests.csproj --filter UuidV7NodePartitionTests -v minimal
  • dotnet test property-tests/Clockworks.PropertyTests.fsproj --filter "FullyQualifiedName~UuidV7FactoryProperties" -v minimal
  • dotnet test Clockworks.sln -v minimal -m:1
  • DOCS_VERSION=dev npm run docs:build
  • dotnet pack src/Clockworks.csproj -c Release -v minimal
  • dotnet run --project demo/Clockworks.Demo -- uuidv7 --bench

Benchmark sample after precomputing masks:

  • default NewGuid: 2,434,158 ops/sec (410.8 ns/op)
  • node-partitioned NewGuid: 2,422,206 ops/sec (412.8 ns/op, 0.5% vs default)
  • default NewGuids batch: 2,618,720 ops/sec (381.9 ns/op)
  • node-partitioned NewGuids batch: 2,530,984 ops/sec (395.1 ns/op, 3.5% vs batch default)

Refreshed GitHub checks are green.

@dexcompiler
Copy link
Copy Markdown
Owner Author

One benchmark nuance worth calling out explicitly: the slightly worse node-partitioned batch result is expected. Batch generation amortizes the timestamp/counter CAS reservation, but node partitioning still has to overlay the configured partition bits into each UUID's rand_b bytes. After a batch reserves counters, the default path mostly benefits from amortized allocation; the partitioned path still pays a tiny fixed per-UUID byte mask/or cost.

After a7c2465, the width-dependent mask/shift math is precomputed once in UuidV7NodePartition, so the remaining batch delta is the unavoidable per-UUID byte overlay plus normal benchmark noise. A further specialized partitioned batch writer might shave a little more, but I would only add that complexity with BenchmarkDotNet evidence showing a meaningful and stable win.

@dexcompiler dexcompiler merged commit f2fdf0e into main May 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: add node-partitioned UUIDv7 generation option for distributed fleets

3 participants