fix: prevent non-monotonic ULIDs from timestamp/randomness clock skew#57
Open
gaoflow wants to merge 1 commit into
Open
fix: prevent non-monotonic ULIDs from timestamp/randomness clock skew#57gaoflow wants to merge 1 commit into
gaoflow wants to merge 1 commit into
Conversation
from_timestamp() sampled the clock twice: once for the timestamp value and again inside randomness() to decide whether to increment or generate fresh entropy. When a millisecond boundary fell between those two samples, the encoded ULID contained the old timestamp but fresh (potentially smaller) randomness, breaking lexicographic sort order within the same millisecond. Pass the resolved timestamp from from_timestamp() into randomness() so both the timestamp bytes and the monotonicity decision use the same clock sample. The randomness() parameter is optional so any existing callers outside from_timestamp() continue to work unchanged. Fixes mdomke#56 Fixes mdomke#45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
from_timestamp()samples the system clock twice: once for the timestamp value and again insiderandomness()to decide whether to increment or generate fresh entropy. When a millisecond boundary falls between these two samples, the encoded ULID carries the old timestamp but fresh (potentially much smaller) randomness, breaking the lexicographic sort-order guarantee within the same millisecond.Seen in the wild:
Both have timestamp
01KT6JE36Abut the randomness dropped fromY...to9....Fix
Pass the same resolved timestamp from
from_timestamp()intorandomness()as an optional parameter, so the timestamp used for the ULID bytes and the monotonicity decision are guaranteed identical. The parameter defaults toNone(backward-compatible), preserving existing callers.ulid/__init__.py:randomness()accepts optionalcurrent_timestamp;from_timestamp()captures the resolved value and passes it through (+6 / -4 lines).tests/test_ulid.py: addedtest_z_real_time_monotonic_sorting()generating 5000 ULIDs without frozen time (the exact scenario that triggers the bug), and made the overflow test self-contained.Tested with the reproduction from issue #56 — zero violations over 100000 iterations.
This pull request was prepared with the assistance of AI, under my direction and review.
Fixes #56
Fixes #45