Repository navigation
Fix absoluteTimeoutFiresEvenIfCallsKeepArriving flake: stop debouncing before the key is freed - #597
Merged
Merged
Conversation
…sKeepArriving The test kept calling debounce while callCount was 0. Once the debounce deadline passes, the delayed-workflow sweep moves the row to ENQUEUED and frees the key, but the workflow only runs when the queue poller dequeues it. A call in that gap started a second workflow. When getResult was slow to return, that second workflow ran before the assertion and callCount was 2. Stop at the first call whose own period would end past the deadline, which is still well before the deadline, so every call lands while the row holds the key. Assert every call bounced the first workflow, the workflow ran once with the last call's argument, and its delay ended at the deadline rather than the last call's period. Fixes #596
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The loop can still call debounce after the deadline because its cutoff is checked only afterward.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates the debouncer timeout test to avoid creating a second workflow after key release.
Changes:
- Bounds debounce calls by the stored deadline.
- Verifies workflow identity, final arguments, and deadline clamping.
| File | Description |
|---|---|
DebouncerTest.java |
Strengthens absolute-timeout behavior assertions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kraftp
approved these changes
Oct 1, 2026
…the key window The loop decided to stop only after each call, so a pause long enough to outlast the window in which the row holds its key (a GC pause or a starved runner overrunning the sleep) let the next call start a fresh workflow and fail the test. Before each call, check that it is still at least 200ms inside that window: the earlier of the previous call's period and the deadline. If it is not, abort the run, since it cannot test the scenario.
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.

Closes #596.
DebouncerTest.absoluteTimeoutFiresEvenIfCallsKeepArrivingflaked withexpected: <1> but was: <2>on the callCount assertion. The debouncer behaves as documented; the test raced against it.Cause
The test kept calling
debouncewhilecallCount == 0.When the debounce deadline passes, the delayed-workflow sweep moves the row to ENQUEUED and clears its deduplication ID, which frees the key. The workflow only runs later, when the queue poller dequeues it, so
callCountstays 0 in between. A call in that gap started a second workflow, as theDebouncerJavadoc says it should.Passing runs create that second workflow too. They pass only because the assertion runs before it does. This run's
getResulttook about a second to return, and the second workflow ran in that time.Fix
The test now stops at the first call whose own 800 ms period would end past the deadline:
delay_until_epoch_msequalsdebounce_deadline_epoch_ms. This checks the cap directly, with no assertion on timing.callCount == 2."result:" + lastArg). The old test checked onlystartsWith("result:").The test is the same on
release/v1.2, so this should be cherry-picked there.Testing
DebouncerTest(52 tests) passes on Postgres.absoluteTimeoutFiresEvenIfCallsKeepArrivingpassed 5 times in a row with--rerun.🤖 Generated with Claude Code