Repository navigation
Conversation
The background worker decided 'queue empty, I am done' under the lock but only marked itself idle in a second lock acquisition (its finally block). An Enqueue landing between the two saw a worker still running and started none, and the worker then exited without the new message. In a real session that is an EDDN message left unsent until the next upload, and lost if the app closes first. The empty-queue decision and the idle flag are now one step under one lock. The finally block only clears the flag for an exit that did not go through that branch (cancellation or a fault): after a normal exit the flag already belongs to whichever worker an Enqueue has since started, and clearing it there would let a second worker send concurrently. This is what made EddnBridgeBatchingTests.A_run_of_signals_is_sent_as_one_message_ when_the_next_event_arrives flaky (one upload where two were expected, about 1 in 1500 under load): not a timing problem in the test but a real race in production code. Reproduced before the fix, then 60,000 iterations of the same scenario on the fixed code with no failures. Adds EddnUploaderHandOffTests: a stress test that fails on the original code 4/4 runs, and a concurrency test that fails on the original code and on the obvious naive fix (clear the flag in finally) about half the time, and passes on the fix (25 consecutive runs). EDNexus.Tests 1110 passed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 46 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This branch has not been deployed
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.
What was flaky, and what it really was
EddnBridgeBatchingTests.A_run_of_signals_is_sent_as_one_message_when_the_next_event_arrivesfailed occasionally (one upload where two were expected). I first suspected the fixedTask.Delaywindows elsewhere in that file, but this test contains no sleeps at all, and it was not a test-timing problem. It is a real race in production code, inEddnUploader's background worker.The race
WorkerAsyncdecided "queue empty, I'm done" under the lock, but only marked itself idle (_running = false) in a second lock acquisition, in itsfinallyblock. AnEnqueuethat landed between the two saw a worker still running, started none, and the worker then exited without the new message. In a real session that is an EDDN upload left unsent until the next upload happens, and lost outright if the app closes first. The bridge test just happens to hit the window: the first message is finishing as the second is queued.Reproduction (before changing anything)
The same scenario run 3000 times in-process on 16 threads: 2 failures in 3000, always
CallCount == 1(the signals message lost), reproduced in 2 of 3 runs.The fix
"queue empty"and"I am idle"are now one decision under one lock. Thefinallyonly clears the flag for an exit that did not go through that branch (cancellation mid-send, or a fault). After a normal exit the flag already belongs to whichever worker anEnqueuehas since started; clearing it there (the obvious fix) would let a second worker send concurrently, which would break upload ordering.DisposeAsync's snapshot of the worker is unaffected: any message enqueued before disposal is either picked up by the running worker or starts a new one whose task is stored under the same lock.Tests (new:
EddnUploaderHandOffTests)A race this narrow cannot be pinned by one run, so these make the window busy: thousands of uploaders on 16 threads.
A_message_enqueued_as_the_worker_goes_idle_is_never_stranded: fails on the original code 4/4 runs.Only_one_worker_sends_at_a_time: fails on the original code and on the naive fix (flag cleared unconditionally infinally) about half the runs, which is why it is a guard and not the primary test. (I first wrote a version of this that did not catch the naive fix and said nothing; that one was reworked and re-checked against the mutation.)Messages_queued_after_the_worker_has_gone_idle_start_it_again: a plain baseline.Verified
Eddntests 10 runs, 0 failures.Limits
Task.Delaywindows in the other tests in that file; they are not what failed.🤖 Generated with Claude Code