fix(harness): prevent missed wakeups during startup#1848
Conversation
|
“caihr” seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Reopening to retrigger CI after an unrelated transient JUnit temp-directory cleanup failure. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, well-targeted bug fix addressing two real race conditions in WakeupDispatcher.start():
-
Subscribe-before-drain reorder: The old order (drain → subscribe) had a window where a wakeup enqueued after the drain but before the subscription could sit unprocessed until the next signal. The fix correctly subscribes first, then drains — the standard "subscribe-then-catchup" pattern.
-
Full queue drain loop: The old code drained only one batch of 64 entries on startup. The fix loops until
entries.size() < MAX_DRAIN_COUNT, ensuring backlogs larger than 64 are fully processed.
Both regression tests are well-designed: startup_subscribesBeforeInitialDrain uses a clever EnqueueOnSubscribeMessageBus to inject a wakeup at the exact moment of subscription, and initialDrain_processesMoreThanOneBatch pushes 65 entries to validate the loop fix. The code is correct, thread-safe (concurrent drain calls get disjoint entry sets; dispatch is idempotent via isSessionRunning guard), and introduces no new issues.
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, well-targeted bug fix addressing two real race conditions in WakeupDispatcher.start():
-
Subscribe-before-drain reorder: The old order (drain → subscribe) had a window where a wakeup enqueued after the drain but before the subscription could sit unprocessed until the next signal. The fix correctly subscribes first, then drains — the standard "subscribe-then-catchup" pattern.
-
Full queue drain loop: The old code drained only one batch of 64 entries on startup. The fix loops until
entries.size() < MAX_DRAIN_COUNT, ensuring backlogs larger than 64 are fully processed.
Both regression tests are well-designed: startup_subscribesBeforeInitialDrain uses a clever EnqueueOnSubscribeMessageBus to inject a wakeup at the exact moment of subscription, and initialDrain_processesMoreThanOneBatch pushes 65 entries to validate the loop fix. The code is correct, thread-safe (concurrent drain calls get disjoint entry sets; dispatch is idempotent via isSessionRunning guard), and introduces no new issues.
AgentScope-Java Version
2.0.0-SNAPSHOT
Description
WakeupDispatcherpreviously drained the durable wakeup queue before subscribing to the transient wakeup signal. A wakeup enqueued between those operations could remain queued until another signal arrived. Startup recovery also drained only one batch of 64 entries, so a larger backlog could remain unprocessed without a follow-up signal.This PR:
Tested with:
mvn -pl agentscope-harness -am -Dtest=WakeupDispatcherTest -Dsurefire.failIfNoSpecifiedTests=false testAll 6
WakeupDispatcherTesttests pass.Checklist
Please check the following items before code is ready to be reviewed.
mvn test) — targeted harness tests pass; the full suite was not run locally