[adapters] Fix the tick-accounting race in test_clock - #6740
Merged
Conversation
mythical-fred
approved these changes
Jul 28, 2026
mythical-fred
left a comment
There was a problem hiding this comment.
Solid diagnosis and a well-scoped fix.
The two off-by-ones are correctly identified: pause is asynchronous so the baseline was one short, and input_step unconditionally polls the clock connector on the first live step after replay, which spends the + 1 allowance systematically. Moving the baseline read after controller.stop() (which joins the circuit thread) and gating Run 2's read on settled_ticks(&stats, expected) closes both directions of the race without weakening the assertion.
A few small notes, none blocking:
settled_tickspolls untilticks >= expected, then sleepsSETTLE = 2sand returns. That is exactly what you want for detecting the +1 overshoot without accepting a stalled replay. Worth adding a comment that the 2s settle is intentionally chosen to be larger than one clock resolution so an overshoot cannot hide behind the deadline.- The failure message now reports both
ticksandticks_after_checkpoint— good. Consider also printing the failure-mode class ("replay stalled" vs "extra live tick") if this ever flakes again, but only if it flakes again. - The
Note on the live tick after replayin the PR body is the interesting bit for future readers. Worth mirroring one sentence of it into a// TODO/// NOTEin the source next to the+ 1allowance so the next person who touches this doesn't retune the tolerance for the fifth time.
Approving.
blp
approved these changes
Jul 28, 2026
ryzhyk
force-pushed
the
fix-clock-test-tick-accounting
branch
from
July 28, 2026 18:09
dc0359f to
1b148a8
Compare
The test sampled Run 1's post-checkpoint tick count right after `controller.pause()`. Pause is asynchronous, so a step that raced it was still journaled, and hence still replayed, while its tick went uncounted: the baseline came out one short of what Run 2 replays. The assertion allows one extra replayed tick, but that allowance is already spent. Once replay ends, the first step commits the last replayed transaction, and `input_step` queues every connector on that step regardless of the paused state, so the clock connector answers with a live tick. Run 2 therefore replays the journaled ticks plus one every time, which leaves no room for a miscounted tick in Run 1. Take the baseline after `stop`, which joins the circuit thread and so counts every journaled step, and wait for the replayed ticks instead of sleeping a fixed five seconds, so a slow replay cannot be sampled halfway through. Failure: https://github.com/feldera/feldera/actions/runs/30334386760/job/90198147938 Previously reported as #4516.
ryzhyk
force-pushed
the
fix-clock-test-tick-accounting
branch
from
July 28, 2026 18:10
1b148a8 to
1834663
Compare
ryzhyk
enabled auto-merge
July 28, 2026 18:10
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
3 participants
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.
Makes
transport::clock::test::test_clockcount replayed clock ticks against a baseline that cannot race the pipeline, which is what failed in the merge queue (assertion failed: ticks >= ticks_after_checkpoint && ticks <= ticks_after_checkpoint + 1). The failure cancelled every other job in that run; nothing was wrong with the change under test (#6718).Why it failed
Both sides of the assertion were off by one, in opposite directions. The job's own replay log names the journaled steps (7 through 12, so six of them):
ticks_after_checkpointcontroller.pause(). Pause is asynchronous, so a step that races it is still journaled, and hence still replayed, while its tick goes uncounted.ticksinput_stepqueues every connector on that step regardless of the paused state, so the clock connector answers with a live tick.The second row is systematic: locally the test reports 6 journaled and 7 replayed on every run, landing exactly on the assertion's upper bound. The
+ 1allowance is therefore already spent, and a single miscounted tick in Run 1 fails the test. That is why the tolerance has been retuned four times (± 2, then exact, then+ 1) and why #4516 came back.The change
stop, which joins the circuit thread and so counts every journaled step.settled_ticks) instead of sleeping a fixed five seconds, then read the count after a short settle, so neither a slow replay nor an overshoot can hide.Validation
Replaystops flushing its recordtransport::clockmoduleA duplicate replay push stays invisible here because same-value records coalesce in the zset. That is harmless in production: the compiler consumes
nowasmap_index -> chain_aggregate(max) -> deindex, so a repeated timestamp is idempotent.Note on the live tick after replay
A pipeline resumed from a checkpoint and left paused still runs one step and advances
NOW()by up to one clock resolution.NOW()only moves forward and the aggregate is idempotent, so there is no data impact, and this PR leaves the behaviour alone rather than changing the controller's step logic to fix a test.🤖 Generated with Claude Code