Give the dual-clock audit suite a monotonic allocator for fabricated origin log keys, fixing the shared-key lookup flake - #2530
Merged
Conversation
…origin log keys (harper#2412 flake) Every test fabricated local-log keys as Date.now() +/- n. Under load two tests could land on the same key, and auditStore.get stops scanning at the first entry of a different transaction sharing the key, so 'one applied transaction can carry writes at different record versions' intermittently failed with 'lookup scans every entry sharing the transaction log key'. Keys now come from one allocator that increases 100 s per anchor, matching the uniqueness an origin's own log keys have. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the dual-clock audit record unit tests by introducing an originClock helper function. This helper generates unique local log keys starting from three hours ago, spaced 100 seconds apart, replacing direct calls to Date.now() across multiple test cases to prevent key collisions. Additionally, a subscription in one test is updated to use { omitCurrent: true } to skip replay. The reviewer suggested replacing the magic numbers used for time intervals in originClock with explicit calculations to improve readability.
Contributor
|
Reviewed; no blockers found. |
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
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.
one applied transaction can carry writes at different record versionsinunitTests/resources/dualClockAuditRecord.test.jsfailed on the Node 26 leg of the Unit Test run for #2526 withlookup scans every entry sharing the transaction log key, passed on rerun, and reproduces locally at about 1 in 20 runs pinned to two cores with CPU burners. Test-only change; no product code.Mechanism
Every test in the suite fabricated its origin log keys as
Date.now() ± n. Under load, an earlier test's key can equal a later test's: on the failing run the invalidate test'sDate.now() + 20landed on the same millisecond as this test'sDate.now() + 1, nineteen milliseconds later.RocksTransactionLogStore.getSyncreads only the contiguous run of entries at a key, which is correct for a real log, where one key names one transaction. Here that run was the invalidate test's transaction, andbatched-newsat further down the log under the same key, never reached.The fix mirrors the production property the lookup relies on. All fabricated keys come from one suite-level allocator,
originClock, stepping 100 s per call from three hours in the past, so no two tests can share a key and the offsets a test subtracts (at most 60 s) never cross the previous anchor. The subscriber-delivery test now subscribes withomitCurrent, which leaves the subscription's high-water mark at zero, so it takes an allocator key like every other test and the suite writes no future-dated record.For the human reviewer
getSyncassumes an origin log never holds two transactions at one key. Origin keys come from a per-node monotonic clock, so that holds in production; this suite was the only place two transactions shared a key. If you read it as a production lookup gap worth hardening, that is a product change and a different PR.omitCurrent. Reversible in one line.omitCurrentit provesevent.versionandevent.localTimeon a live event only. The default-subscribe form needed a key ahead of the wall clock, which the earlier rounds of this PR tried at +1 s and +1 h and rejected as a landmine for later subscribers.Date.now()key re-arms the flake. Declined an assertion inauditEntriesForto keep the diff to the flake; cheap to add later.Verification
lookup scans every entry sharing the transaction log keyapplied-invalidate-head-1at the colliding key, then an entry at a different key, then stopped;batched-oldandbatched-newwere later in the logtest:unit:resources, RocksDBlint:required,format:checkFails-on-base is the contended-loop row: the test asserts the same things as before, only its keys changed.
Five pre-push review rounds (codex + gemini, cursor-composer on rounds 1 and 2, Harper domain adjudication on each). Round 1 widened the anchor budget and corrected the comment's claimed invariant. Round 2 pulled the subscriber key back from an hour ahead after Gemini pointed out a far-future record gates any later subscription on the table. Round 3 fixed the comment's description of the scan and a 120 s version offset. Round 4 converged and proposed
omitCurrent, adopted in round 5, which converged again.Complexity: easy
Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=5 @ 8f65302
Human-Review-Need: 3 (decisions: anchor-direction-past-vs-future, omit-current-vs-default-subscribe, shared-allocator-vs-per-test-constants, flake-fix-layer) @ 8f65302