fix(#746): signal-driven AgentLoopDriver pacing - #856
Open
bedaHovorka wants to merge 1 commit into
Open
Conversation
Replace AgentLoopDriver's wall-clock Thread.sleep(1) poll with a SnapshotSignal the sim thread fires once per tick, and drop the simTime==prevSimTime stale-tick guard in signal mode — the mechanism the issue's root-cause analysis called for. - SnapshotSignal / DefaultSnapshotSignal: semaphore-based, at-most-one- pending signal with a bounded await() timeout that acts purely as a shutdown safety net (lets a driver thread parked in await() notice ShuntingLoop.isSimActive() went false once the sim stops signaling, instead of leaking a permanently-blocked thread). - AgentLoopDriver.runCycle() now returns Boolean (true = a full cycle ran and posted decisions, false = no-op short-circuit) so callers that gate a lock-step barrier on cycle completion don't release it for a no-op cycle. - ExampleRegistry.wireDispatcherAgent wires the signal into production; the signal must fire from controlStepListener, not snapshotCaptureHook (ShuntingLoop.iteration() publishes the per-tick TickObservation between those two calls) — confirmed by reproducing a stale-read conflictEventCount regression when signalling too early. - RuleBasedDispatcherDeterminismTest moves to the same signal-based wiring (via the new shared RuleBasedDispatcherDeterminismRunner, also used by the new 1000-repetition RuleBasedDispatcherDeterminismHeavyTest) while keeping a minimal ordering barrier for its own unrelated zero-conflict-events invariant. - No :core changes were needed, so the traffic-simulation-expert gate was not invoked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5 tasks
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #746 by replacing AgentLoopDriver’s wall-clock busy-poll pacing with a sim-thread-driven SnapshotSignal, aiming to make headless runs deterministic and avoid missed decision windows.
Changes:
- Introduces
SnapshotSignal/DefaultSnapshotSignaland updatesAgentLoopDriver.runCycle()to use signal-based pacing (and returnBooleanto indicate whether work was done). - Wires the signal into production (
ExampleRegistry.wireDispatcherAgent) and updates determinism tests to use the same wiring via a shared runner. - Adds
:dispatcher-agent:heavyTestplus new/expanded unit + heavy stress test coverage around the pacing mechanism.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/SnapshotSignal.kt | Adds signal abstraction + semaphore-based implementation for per-tick driver wakeups. |
| dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/AgentLoopDriver.kt | Switches to optional signal-based pacing; makes runCycle() return Boolean. |
| desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/ExampleRegistry.kt | Wires DefaultSnapshotSignal into production loop; signals from controlStepListener. |
| dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/SnapshotSignalTest.kt | Adds unit tests for signal semantics (timeout + coalescing + concurrency). |
| dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/AgentLoopDriverTest.kt | Adds tests covering signal-mode behavior and new Boolean contract. |
| dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/testutil/RuleBasedDispatcherDeterminismRunner.kt | Centralizes determinism wiring to keep 10x and 1000x gates aligned. |
| dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/RuleBasedDispatcherDeterminismTest.kt | Refactors to use the shared runner and production-style signal wiring. |
| dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/RuleBasedDispatcherDeterminismHeavyTest.kt | Adds 1000-repetition heavy stress variant tagged heavy-test. |
| dispatcher-agent/build.gradle.kts | Adds heavyTest Gradle task for the dispatcher-agent module. |
Comment on lines
+21
to
+25
| * 1. Sim thread (inside [cz.vutbr.fit.interlockSim.sim.ShuntingLoop.snapshotCaptureHook]): | ||
| * `perceptionPort.captureSnapshot()` → [signal] | ||
| * 2. Driver thread (inside [AgentLoopDriver.runCycle]): [await] (blocks until step 1 | ||
| * above runs, or until the bounded timeout — see below) → `perceptionPort.snapshot()` | ||
| * → decide → post |
Comment on lines
+55
to
+58
| * [ShuntingLoop][cz.vutbr.fit.interlockSim.sim.ShuntingLoop] stops calling | ||
| * `snapshotCaptureHook` (and therefore [signal]) the moment the simulation ends — its | ||
| * `interLoopSleep` sets `isSimActive() == false` and simply never invokes another | ||
| * `iteration()`. [AgentLoopDriver]'s run loop is `while (isSimActive()) { runCycle() }` |
Comment on lines
+75
to
+78
| * guard is skipped too. The caller must arrange for [SnapshotSignal.signal] to be | ||
| * called on the sim thread immediately after each | ||
| * [cz.vutbr.fit.interlockSim.ports.NetworkPerceptionPort.captureSnapshot] (see | ||
| * [cz.vutbr.fit.interlockSim.sim.ShuntingLoop.snapshotCaptureHook]). |
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.



Summary
Closes #746
Replaces
AgentLoopDriver's wall-clockThread.sleep(1)busy-poll with aSnapshotSignalthat the sim thread fires exactly once per tick, and drops thesnapshot.simTime == prevSimTimestale-tick guard in signal-based mode — the exact mechanism the issue's root-cause analysis (and PR #809, see below) called for.Root-cause recap
AgentLoopDriver.runCycle()paced itself withThread.sleep(1)plus a guard that skipped re-deciding whenever the sensedsimTimematched the previous cycle's. UnderNoOpSimulationController(headless runs), that guard was the only pacing mechanism, and it could suppress a real decision opportunity — reported as an intermittenttrainsExited = 0admission stall inRuleBasedDispatcherDeterminismTest.What changed
SnapshotSignal/DefaultSnapshotSignal(new,:dispatcher-agent): semaphore-based, at-most-one-pending signal.await()is bounded — the timeout is a pure shutdown safety net, not the pacing mechanism: it lets a driver thread parked inawait()noticeShuntingLoop.isSimActive()has gone false once the sim stops signaling (which happens after the last real tick, with no further signal ever coming), instead of leaking a permanently-blocked thread on every completed run.AgentLoopDriver.runCycle()now returnsBoolean(true= a full cycle ran and posted a decision batch,false= a no-op short-circuit — signal timeout,EMPTYsnapshot in polling mode, or the polling-mode stale-tick guard). Callers that gate a strict per-tick barrier on the sim thread must not release it after afalseresult — see below for why that matters.ExampleRegistry.wireDispatcherAgentwires the signal into production. Important ordering detail found and fixed during development: the signal must fire fromcontrolStepListener, not fromsnapshotCaptureHook—ShuntingLoop.iteration()callssnapshotCaptureHook()before it publishes the per-tickTickObservation, but callscontrolStepListenerafter. Signalling from the hook let the driver wake and read a staleTickObservationfrom the previous tick; this was caught by reproducing a spuriousconflictEventCountregression while developing the fix, not by inspection.RuleBasedDispatcherDeterminismTestnow uses the same signal-based wiring as production (via a new sharedRuleBasedDispatcherDeterminismRunner), replacing its own hand-rolleddriverTurn/simTurnpolling handshake. It still keeps a minimaldecisionsAppliedordering barrier — that barrier protects a different, out-of-scope-for-SP0.11c: Signal-driven AgentLoopDriver tick pacing (replaces wall-clock poll; unblocks SP2c.5 P10 gate) #746 invariant this same test asserts (conflictEventCount == 0, a decide-time-vs-apply-time staleness race in the reservation layer thatShuntingLoopLiftedDriverIntegrationTest's KDoc documents separately). That barrier is only released whenrunCycle()returnstrue— releasing it unconditionally (including after a timeout no-op) was reproduced as a real bug during development (see "Debugging trail" below).RuleBasedDispatcherDeterminismHeavyTest(new, 1000-repetition,@Tag("heavy-test")) + a new:dispatcher-agent:heavyTestGradle task (mirroring the existing:core/:desktop-uipattern) — didn't exist for this module before.SnapshotSignalTest(signal/await protocol, bounded-timeout safety net, at-most-one-pending coalescing, concurrent round-trips) and a newAgentLoopDriverTestnested class for signal-based pacing (awaits before sensing, bypasses the stale-tick guard, short-circuits cleanly on timeout,Booleanreturn contract).LiftedStackFixture(shared byShuntingLoopLiftedDriverIntegrationTestandDispatcherCollisionValidationTest) was deliberately left untouched — itsdriverTurn/simTurnlock-step handshake protects the same conflict-avoidance invariant mentioned above, which is out of scope for SP0.11c: Signal-driven AgentLoopDriver tick pacing (replaces wall-clock poll; unblocks SP2c.5 P10 gate) #746; touching it risked scope creep the issue explicitly warned against.:corechangeNone. The fix is fully contained in
:dispatcher-agent(plus the:desktop-uiwiring call site). The traffic-simulation-expert sub-agent review gate mandated for any:corechange was therefore not invoked — there was nothing to review.Debugging trail (reproduce → hypothesize → verify → fix)
RuleBasedDispatcherDeterminismTestrepeatedly before any change — 40/40 reps clean on this dev machine (the ~4% flake described in the issue did not reproduce reliably here; noted honestly below).signal()fromsnapshotCaptureHook. Repeated runs: intermittentconflictEventCount == 1failures (expected 0).TickObservationis published lets the driver read stale block-input state. Verified by tracingShuntingLoop.iteration()'s call order and moving the signal tocontrolStepListener(after publication) in both production and test wiring. Re-ran: clean.conflictEventCount == 1again, plus anIllegalArgumentExceptioncascade from a run 1 failure. Hypothesis: the test'sdecisionsAppliedbarrier was released unconditionally after everyrunCycle()call, including ones that timed out onSnapshotSignal.await()doing no real work — "banking" a permit the sim thread'sacquireUninterruptibly()could consume before the real decision was actually posted, reintroducing the exact staleness race the barrier exists to prevent. Verified by tracing the semaphore state transitions round by round. Fixed by makingrunCycle()report whether it did real work (Booleanreturn) and only releasing the barrier ontrue.SnapshotSignalTest.concurrentSignalAwaitRoundTripsoriginally paced itself withThread.sleep(1)and asserted 1:1 signal delivery — which the class's own documented at-most-one-pending coalescing semantics don't actually guarantee under producer/consumer scheduling skew. Rewritten to use a consumer→producer handshake instead of wall-clock timing.Verification evidence (real repeated-run output, not one lucky run)
RuleBasedDispatcherDeterminismTest(10 reps/invocation), run as separate sequential invocations after the final fix:(130+ total sequential repetitions clean since the barrier fix in step 4 above — this includes the runs that caught bugs 3 and 4 during development, which are excluded from this tally since they predate the fix.)
:dispatcher-agent:heavyTest(1000 repetitions):1000/1000 passed, 0 failed — 100%, well above the ≥99.9% bar.
Production headless path (
java -jar interlockSim.jar example shuntingLoop 300, run 5x after building:desktop-ui:shadowJar):trainsExited=5on all 5 runs.Regression gates (all green, run individually to avoid unrelated CPU-contention artifacts — see note below):
:core:jvmTest— 2492/2492 passed:core:integrationTest— 761/761 passed (includesShuntingLoopControlStepListenerTest,Issue742RegressionTest— 8/8 passed, all Issue SP0.11b: PathReservationRegistry.mergePathInfo cannot distinguish fork-overlap from genuine circular-loop revisit #742 switch-configuration invariants intact):dispatcher-agent:test— all passed (includesDispatchDecisionApplierTest,SnapshotSignalTest,AgentLoopDriverTest):dispatcher-agent:integrationTest— all passed (includesShuntingLoopLiftedDriverIntegrationTest— still passing unmodified with its own lock-step mechanism, confirming the two pacing mechanisms coexist without interference):desktop-ui:test— 689/690 passed, 1 pre-existing skip (unrelated to this change):desktop-ui:integrationTest— 689/690 passed, 1 pre-existing skip:core:checkCoreCommonMainPurity— passed (no:corechanges were made; run anyway for completeness):dispatcher-agent:ktlintCheck,:desktop-ui:ktlintCheck,:dispatcher-agent:detekt,:desktop-ui:detekt— all clean (force-reran with--rerun-tasks, not relying on Gradle's UP-TO-DATE cache)Note on methodology: running
:core:jvmTest+:dispatcher-agent:test+:desktop-ui:testconcurrently in one Gradle invocation intermittently surfaced an unrelated, pre-existing flaky test (SimulationController -> SpeedControllable propagation > setSpeed is a safe no-op when main process is null, a GUI test with no connection to this change) timing out under the resulting CPU contention. Confirmed as a pre-existing artifact, not a regression, by running it in isolation (passed cleanly, repeatedly). All gates listed above were verified running individually.Hard rules compliance
:corechanges.[WIP]commits.🤖 Generated with Claude Code