fix: stop the quantized revision tests racing the database clock - #3326
Draft
vroldanbet wants to merge 1 commit into
Draft
vroldanbet wants to merge 1 commit into
vroldanbet wants to merge 1 commit into
Conversation
…lock
TestPostgresDatastore/QuantizedRevisions failed a real `mage testds:postgres`
run:
postgres_shared_test.go:1068: Not equal: expected: 0x2, actual: 0x3
incorrect number of revisions visible to snapshot, expected 2, got 3
The table positions its fixture rows relative to a database clock read once, up
front, while querySelectRevision recomputes its cutoff from the database's NOW()
at the moment it runs. Both are floored into quantization buckets, so a boundary
falling between the two reads moves the cutoff, selects a different row as the
revision, and leaves the expected visibility counts describing a fixture that is
no longer the one being measured. The slack is whatever remains of the current
bucket - uniform in [0, quantization), mean 250ms for the 500ms cases - against
several container round-trips, so on a loaded machine it is close to a coin flip.
A second case of the same table, OldestInWindowIsSelected, failed under
repetition, which is what makes this structural rather than one bad row.
Each attempt now positions the fixture just after a boundary, leaving a full
quantization period, and then checks whether one was crossed anyway; an attempt
that lost the race is discarded rather than reported. The final attempt asserts
whatever it finds, so a genuine regression still fails rather than being retried
until it passes and is mistaken for flakiness.
The first version of this retried on every run without saying so usefully: the
QuantizationDisabled case sets quantization to 1ns, where every instant is its
own bucket, so the boundary check was always true and the case only ever passed
on the forced final attempt - reintroducing the very thing being fixed. Bucket
logic is now skipped below a millisecond, where there is no boundary to align to
and crossing one says nothing, and the retry log names the case so that a
recurrence is diagnosable rather than mysterious.
MySQL's copy of this test has the same defect - it pins the clock through
mgg.Now and revisions.go floors a live UTC_TIMESTAMP(6) - and gets the same
treatment. It was not observed failing, unlike postgres, so this is closing a
latent hole rather than a reported one.
Verified with -count=3 -race: postgres 27/27 subtests and mysql 21/21, with zero
boundary-crossing retries logged on either.
vroldanbet
added this pull request to stack #3327
September 19, 2026 01:20
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 0bfd60e | Previous: f7620a5 | Ratio |
|---|---|---|---|
BenchmarkCheck/DoubleWideArrow/advised (github.com/authzed/spicedb/pkg/query/benchmarks) |
1572831 ns/op 8120 B/op 180 allocs/op |
619877 ns/op 8120 B/op 180 allocs/op |
2.54 |
BenchmarkCheck/DoubleWideArrow/advised (github.com/authzed/spicedb/pkg/query/benchmarks) - ns/op |
1572831 ns/op |
619877 ns/op |
2.54 |
This comment was automatically generated by workflow using github-action-benchmark.
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.
TestPostgresDatastore/QuantizedRevisionsfailed a realmage testds:postgresrun:What was wrong
These tests check which revisions a snapshot can see. To set that up, the test asks the database for the current time, then inserts rows at offsets from it — two seconds ago, four seconds ago, and so on.
The problem is that the code under test asks the database for the time again when it runs, and rounds that second reading down to the nearest half-second or second. If the clock ticks past one of those rounding points between the two readings, the code picks a different starting row than the test set up for, and the expected count no longer matches.
How much slack there is depends on where in the current rounding interval the test happens to start — anywhere from zero to a full interval, averaging half. Against several round-trips to a database container, that is close to a coin flip on a loaded machine.
This is entirely a test problem. The query itself is correct.
A second case from the same table,
OldestInWindowIsSelected, also failed when run repeatedly, which is what makes this a problem with the whole table rather than one bad row.The fix
Each attempt now starts the setup just after a rounding point, which gives it a full interval of slack instead of half an interval on average. It then checks whether the clock crossed a rounding point anyway, and if it did, throws that attempt away and starts over rather than reporting a failure about data that no longer matches.
The final attempt always checks its result, so a real regression still fails rather than being retried until it passes.
One thing worth knowing
The first version of this retried on every single run without saying why. One case switches rounding off entirely by setting the interval to one nanosecond, so every instant counts as its own interval and the "did we cross a boundary" check was always true. That case was only ever passing on the forced final attempt — quietly reintroducing the flakiness this was meant to remove.
The rounding logic is now skipped when the interval is under a millisecond, where there is no boundary to align to and crossing one means nothing. The retry message now names the case, so if this ever happens again it is obvious rather than mysterious.
MySQL
MySQL's copy of this test has the same flaw and gets the same fix. It was never actually seen failing, unlike postgres, so that half closes a gap rather than a reported bug.
Verification
With 3 repeated runs and the race detector on: postgres 27/27 subtests, mysql 21/21, and zero boundary-crossing retries on either.