Skip to content

Keep monotonic_ulid() monotonic when the clock does not move forward - #1641

Open
dylanpulver wants to merge 1 commit into
simonw:mainfrom
dylanpulver:fix-monotonic-ulid-clock-regression
Open

Keep monotonic_ulid() monotonic when the clock does not move forward#1641
dylanpulver wants to merge 1 commit into
simonw:mainfrom
dylanpulver:fix-monotonic-ulid-clock-regression

Conversation

@dylanpulver

Copy link
Copy Markdown

monotonic_ulid() promises a ULID strictly larger than every other from the same process, and claims parity with the reference JavaScript monotonicFactory. The increment branch fires only on now_ms == last_ms (llm/utils.py:746), so a reading at or below the previous timestamp falls through to _fresh() and yields a smaller ULID.

Two ways that reading arrives:

  • The wall clock steps backwards: time.time_ns() is not monotonic, so an NTP step or VM resume moves it.
  • time.time_ns() is read outside _lock, so a thread holding the earlier millisecond can reach the lock after a thread holding the later one. The lock is there because concurrent callers are in scope.

These IDs become conversation and turn ids in logs.db, where the ULID sorts rows by creation.

The fix tests now_ms <= last_ms, matching ulid/javascript source/ulid.ts:161 (if (seed <= lastTime)), which likewise reuses the previous timestamp rather than encoding the regressed one. Moving time.time_ns() inside the lock closes the thread window but not clock regression.

Residual: the guarantee stays per-process, and a regressed clock now draws randomness from the pinned millisecond, so a long regression reaches the existing OverflowError.

1107 tests pass locally against 1105 on main. Both new tests fail on == and pass on <=; test_monotonic_ulids passes either way. Black, ruff, mypy and cog are clean. No changelog entry, since docs/contributing.md assigns that to the release process.

The increment branch only fired when now_ms == last_ms, so a timestamp
reading at or below the previous one produced a fresh, smaller ULID.
Match the reference JavaScript monotonicFactory, which tests seed <=
lastTime and reuses the previous timestamp.
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.

1 participant