Skip to content

fix(#746): signal-driven AgentLoopDriver pacing - #856

Open
bedaHovorka wants to merge 1 commit into
goal-10from
sp0-11c-agentloopdriver-pacing
Open

fix(#746): signal-driven AgentLoopDriver pacing#856
bedaHovorka wants to merge 1 commit into
goal-10from
sp0-11c-agentloopdriver-pacing

Conversation

@bedaHovorka

Copy link
Copy Markdown
Owner

Summary

Closes #746

Replaces AgentLoopDriver's wall-clock Thread.sleep(1) busy-poll with a SnapshotSignal that the sim thread fires exactly once per tick, and drops the snapshot.simTime == prevSimTime stale-tick guard in signal-based mode — the exact mechanism the issue's root-cause analysis (and PR #809, see below) called for.

⚠️ This issue has prior-attempt history (#809, closed as BROKEN) — please review carefully before merging.

Root-cause recap

AgentLoopDriver.runCycle() paced itself with Thread.sleep(1) plus a guard that skipped re-deciding whenever the sensed simTime matched the previous cycle's. Under NoOpSimulationController (headless runs), that guard was the only pacing mechanism, and it could suppress a real decision opportunity — reported as an intermittent trainsExited = 0 admission stall in RuleBasedDispatcherDeterminismTest.

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 in await() notice ShuntingLoop.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 returns Boolean (true = a full cycle ran and posted a decision batch, false = a no-op short-circuit — signal timeout, EMPTY snapshot 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 a false result — see below for why that matters.
  • ExampleRegistry.wireDispatcherAgent wires the signal into production. Important ordering detail found and fixed during development: the signal must fire from controlStepListener, not from snapshotCaptureHookShuntingLoop.iteration() calls snapshotCaptureHook() before it publishes the per-tick TickObservation, but calls controlStepListener after. Signalling from the hook let the driver wake and read a stale TickObservation from the previous tick; this was caught by reproducing a spurious conflictEventCount regression while developing the fix, not by inspection.
  • RuleBasedDispatcherDeterminismTest now uses the same signal-based wiring as production (via a new shared RuleBasedDispatcherDeterminismRunner), replacing its own hand-rolled driverTurn/simTurn polling handshake. It still keeps a minimal decisionsApplied ordering 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 that ShuntingLoopLiftedDriverIntegrationTest's KDoc documents separately). That barrier is only released when runCycle() returns true — 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:heavyTest Gradle task (mirroring the existing :core/:desktop-ui pattern) — didn't exist for this module before.
  • New unit coverage: SnapshotSignalTest (signal/await protocol, bounded-timeout safety net, at-most-one-pending coalescing, concurrent round-trips) and a new AgentLoopDriverTest nested class for signal-based pacing (awaits before sensing, bypasses the stale-tick guard, short-circuits cleanly on timeout, Boolean return contract).
  • LiftedStackFixture (shared by ShuntingLoopLiftedDriverIntegrationTest and DispatcherCollisionValidationTest) was deliberately left untouched — its driverTurn/simTurn lock-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.

:core change

None. The fix is fully contained in :dispatcher-agent (plus the :desktop-ui wiring call site). The traffic-simulation-expert sub-agent review gate mandated for any :core change was therefore not invoked — there was nothing to review.

Debugging trail (reproduce → hypothesize → verify → fix)

  1. Reproduce baseline: ran the existing RuleBasedDispatcherDeterminismTest repeatedly 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).
  2. Implement the signal mechanism as scoped. First pass wired signal() from snapshotCaptureHook. Repeated runs: intermittent conflictEventCount == 1 failures (expected 0).
  3. Hypothesis: signalling before TickObservation is published lets the driver read stale block-input state. Verified by tracing ShuntingLoop.iteration()'s call order and moving the signal to controlStepListener (after publication) in both production and test wiring. Re-ran: clean.
  4. Second flake found under load: running the determinism test concurrently with other Gradle builds (and later, even sequentially under contention) reproduced conflictEventCount == 1 again, plus an IllegalArgumentException cascade from a run 1 failure. Hypothesis: the test's decisionsApplied barrier was released unconditionally after every runCycle() call, including ones that timed out on SnapshotSignal.await() doing no real work — "banking" a permit the sim thread's acquireUninterruptibly() 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 making runCycle() report whether it did real work (Boolean return) and only releasing the barrier on true.
  5. Also found and fixed a flaky test (not production code): SnapshotSignalTest.concurrentSignalAwaitRoundTrips originally paced itself with Thread.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:

  • Run 1: 10/10 PASSED
  • Run 2: 10/10 PASSED
  • Run 3: 10/10 PASSED
  • Run 4: 10/10 PASSED
  • Run 5: 10/10 PASSED
  • Run 6: 10/10 PASSED
  • Run 7: 10/10 PASSED
  • Run 8: 10/10 PASSED
  • Run 9: 10/10 PASSED
  • Run 10: 10/10 PASSED

(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):

> Task :dispatcher-agent:heavyTest
...
RuleBasedDispatcher determinism — 1000-repetition heavy stress (Issue #746) > vyhybna.xml run produces identical outcome (1000x heavy stress) > repetition 1000 of 1000 PASSED

BUILD SUCCESSFUL in 4m 10s

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):

--- Simulation complete: 5 trains, 187.6s sim time, 0.5s wall ---
--- Simulation complete: 5 trains, 203.4s sim time, 0.7s wall ---
--- Simulation complete: 5 trains, 187.6s sim time, 0.6s wall ---
--- Simulation complete: 5 trains, 187.6s sim time, 0.5s wall ---
--- Simulation complete: 5 trains, 187.6s sim time, 0.6s wall ---

trainsExited=5 on 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 (includes ShuntingLoopControlStepListenerTest, 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 (includes DispatchDecisionApplierTest, SnapshotSignalTest, AgentLoopDriverTest)
  • :dispatcher-agent:integrationTest — all passed (includes ShuntingLoopLiftedDriverIntegrationTest — 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 :core changes 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:test concurrently 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

  • No test was disabled at any point to make anything pass.
  • No :core changes.
  • English only throughout.
  • No [WIP] commits.

🤖 Generated with Claude Code

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>
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / DefaultSnapshotSignal and updates AgentLoopDriver.runCycle() to use signal-based pacing (and return Boolean to 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:heavyTest plus 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]).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

goal-10 LLM Dispatcher

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SP0.11c: Signal-driven AgentLoopDriver tick pacing (replaces wall-clock poll; unblocks SP2c.5 P10 gate)

2 participants