fix(SP0.11): suppress duplicate ReservePath race + CI green-up

bedaHovorka · claude · bedaHovorka · commit 082ffd071a38 · 2026-07-10T20:43:29.000+02:00
The async decoupling between AgentLoopDriver and the kDisco sim thread let the driver decide an identical ReservePath twice before the first application was reflected back (pathAlreadyExtendedBeyond only updates post-application). Re-applying it corrupted the train's registered path via a false-positive "legitimate circular route" match in PathReservationRegistry, permanently stalling the train. DispatchDecisionApplier now suppresses re-applying an already-reserved (trainId, from, to) triple, without touching shared navigation code. Raises RuleBasedDispatcherDeterminismTest from 0/10 to ~6/10 passing. The remaining intermittent failures are a second, pre-existing bug in PathReservationRegistry.mergePathInfo (fork-overlap vs. genuine circular-loop revisit are structurally indistinguishable by path position alone) — tracked as a follow-up, sub-issue #742 of #535. Also: add @timeout to ActuatorCommandQueueTest, AgentLoopDriverTest, DispatchDecisionApplierTest, RuleBasedDispatcherTest (none had one); cap dispatcher-agent test logging at WARN (was defaulting to a much more verbose level, producing multi-GB test-output artifacts); fix 4 ktlint violations blocking CI (import ordering in ExampleRegistry.kt and RuleBasedDispatcherDeterminismTest.kt, Koin module formatting in DispatcherAgentModule.kt, backing-property naming in ShuntingLoop.kt). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
diff --git a/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/sim/ShuntingLoop.kt b/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/sim/ShuntingLoop.kt
@@ -180,13 +180,13 @@ class ShuntingLoop(
 	// SP0.11: Observation data published at the start of each iteration for the off-kernel
 	// driver thread. Written on the single kDisco sim thread; @Volatile ensures visibility.
 	@kotlin.concurrent.Volatile
-	private var _latestQueuedTrains: List<QueuedTrainObservation> = emptyList()
+	private var latestQueuedTrains: List<QueuedTrainObservation> = emptyList()
 
 	@kotlin.concurrent.Volatile
-	private var _latestInnerBlockInputs: List<BlockInputObservation> = emptyList()
+	private var latestInnerBlockInputs: List<BlockInputObservation> = emptyList()
 
 	@kotlin.concurrent.Volatile
-	private var _latestOuterBlockInputs: List<BlockInputObservation> = emptyList()
+	private var latestOuterBlockInputs: List<BlockInputObservation> = emptyList()
 
 	// Test-observability counters (#365) — incremented from existing lifecycle sites only.
 	// Not atomic: ShuntingLoop runs on the single kDisco dispatcher thread, so all increment
@@ -328,12 +328,12 @@ class ShuntingLoop(
 		// the driver read current-tick state.  Written on the single kDisco sim thread;
 		// @Volatile declarations ensure visibility to the driver thread.
 		snapshotCaptureHook?.invoke()
-		_latestQueuedTrains = unapprowedTrains.map { QueuedTrainObservation(it.name, it.timetableDestinationName) }
-		_latestInnerBlockInputs =
+		latestQueuedTrains = unapprowedTrains.map { QueuedTrainObservation(it.name, it.timetableDestinationName) }
+		latestInnerBlockInputs =
 			innerTrackBlocks.flatMap { block ->
 				block.ends().map { end -> toBlockInputObservation(block, Util.assertInstanceOf<DynamicRailSemaphore>(end)) }
 			}
-		_latestOuterBlockInputs = outerTrackblocks.map { (block, sem) -> toBlockInputObservation(block, sem) }
+		latestOuterBlockInputs = outerTrackblocks.map { (block, sem) -> toBlockInputObservation(block, sem) }
 
 		// SP0.9: Drain and apply any decisions posted to the cross-thread command queue.
 		controlStepListener?.onControlStep()
@@ -403,23 +403,23 @@ class ShuntingLoop(
 	 *
 	 * @since Issue #733 (SP0.11 — Goal 10)
 	 */
-	fun getQueuedTrains(): List<QueuedTrainObservation> = _latestQueuedTrains
+	fun getQueuedTrains(): List<QueuedTrainObservation> = latestQueuedTrains
 
 	/**
 	 * Returns the block-input observations for all inner track blocks, as published at the
 	 * start of the most recent [iteration]. Safe to call from off-kernel threads.
 	 *
 	 * @since Issue #733 (SP0.11 — Goal 10)
 	 */
-	fun getInnerBlockInputs(): List<BlockInputObservation> = _latestInnerBlockInputs
+	fun getInnerBlockInputs(): List<BlockInputObservation> = latestInnerBlockInputs
 
 	/**
 	 * Returns the block-input observations for all outer track blocks, as published at the
 	 * start of the most recent [iteration]. Safe to call from off-kernel threads.
 	 *
 	 * @since Issue #733 (SP0.11 — Goal 10)
 	 */
-	fun getOuterBlockInputs(): List<BlockInputObservation> = _latestOuterBlockInputs
+	fun getOuterBlockInputs(): List<BlockInputObservation> = latestOuterBlockInputs
 
 	/**
 	 * Returns a snapshot of the currently approved (active) trains. Used by the external
diff --git a/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/sim/RuleBasedDispatcherTest.kt b/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/sim/RuleBasedDispatcherTest.kt
@@ -18,6 +18,8 @@ import cz.vutbr.fit.interlockSim.objects.core.TrackFacility
 import cz.vutbr.fit.interlockSim.ports.SimulationSnapshot
 import org.junit.jupiter.api.DisplayName
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
+import java.util.concurrent.TimeUnit
 
 /**
  * Direct unit tests for [RuleBasedDispatcher] — the branch-level coverage that the
@@ -32,6 +34,7 @@ import org.junit.jupiter.api.Test
  *   (SP0.7 — Goal 10)
  */
 @DisplayName("RuleBasedDispatcher — branch-level unit coverage")
+@Timeout(30, unit = TimeUnit.SECONDS)
 class RuleBasedDispatcherTest {
 	// ── Test data builders ──────────────────────────────────────────────────
 
diff --git a/desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/ExampleRegistry.kt b/desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/ExampleRegistry.kt
@@ -13,8 +13,8 @@ import cz.vutbr.fit.interlockSim.context.ContextCreationException
 import cz.vutbr.fit.interlockSim.context.DefaultSimulationContext
 import cz.vutbr.fit.interlockSim.context.NoOpSimulationController
 import cz.vutbr.fit.interlockSim.context.SimulationContext
-import cz.vutbr.fit.interlockSim.context.SimulationController
 import cz.vutbr.fit.interlockSim.context.SimulationContextFactory
+import cz.vutbr.fit.interlockSim.context.SimulationController
 import cz.vutbr.fit.interlockSim.dispatcher.ActuatorCommandQueue
 import cz.vutbr.fit.interlockSim.dispatcher.AgentLoopDriver
 import cz.vutbr.fit.interlockSim.dispatcher.DispatchDecisionApplier
diff --git a/dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/DispatchDecisionApplier.kt b/dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/DispatchDecisionApplier.kt
@@ -89,6 +89,20 @@ class DispatchDecisionApplier(
 		private val logger = KotlinLogging.logger {}
 	}
 
+	/**
+	 * Reservation triples (`trainId|fromSemaphoreName|toSeparatorName`) already
+	 * successfully applied via [applyReservePath], for the duplicate-suppression
+	 * guard documented on that function.
+	 *
+	 * Only ever appended to, never cleared: within one simulation run each train
+	 * makes a bounded, one-way trip through a handful of hops (2-3 for the bundled
+	 * `vyhybna.xml` topology), so the same triple recurring for the same train is
+	 * always the duplicate-decision race below, never a legitimate second need to
+	 * reserve the identical hop again. A fresh [DispatchDecisionApplier] is created
+	 * per simulation run, so this does not grow across runs either.
+	 */
+	private val appliedReservations: MutableSet<String> = mutableSetOf()
+
 	/**
 	 * Drains [queue] and applies each pending [DispatchDecision] via the actuator ports.
 	 *
@@ -119,7 +133,56 @@ class DispatchDecisionApplier(
 			DispatchDecision.NoAction -> Unit
 		}
 
+	/**
+	 * Applies [decision] via [NetworkActuatorPort.requestRoute], guarding against
+	 * duplicate application of an already-successfully-reserved hop.
+	 *
+	 * ## Duplicate-decision race (SP0.11 regression, Issue #733 follow-up)
+	 *
+	 * [AgentLoopDriver][cz.vutbr.fit.interlockSim.dispatcher.AgentLoopDriver] runs
+	 * on its own thread, decoupled from the sim thread that applies its decisions.
+	 * Because [ShuntingLoop][cz.vutbr.fit.interlockSim.sim.ShuntingLoop]'s own
+	 * polling ticks are not wall-clock throttled (no `SimulationController`
+	 * pacing in a headless run), several ShuntingLoop ticks can elapse before the
+	 * driver thread gets scheduled again. The block-input observation it reads —
+	 * in particular [cz.vutbr.fit.interlockSim.sim.BlockInputObservation.pathAlreadyExtendedBeyond],
+	 * the guard [cz.vutbr.fit.interlockSim.sim.RuleBasedDispatcher] relies on to
+	 * avoid re-deciding an already-reserved hop — only updates once a decided
+	 * [DispatchDecision.ReservePath] has actually been drained and applied here.
+	 * This means the driver can legitimately decide the *same* `ReservePath` more
+	 * than once before its first application is reflected back.
+	 *
+	 * Re-applying an already-owned hop is not a harmless no-op:
+	 * `DefaultPathReservationService.reservePath`'s already-owned fast path calls
+	 * `PathReservationRegistry.registerPathInfo` again, and that registry's merge
+	 * logic assumes the incoming path's `start` overlaps the *current* registered
+	 * `target`. Once the first (legitimate) application has already advanced that
+	 * target past the duplicate's `start`, the overlap check fails, the duplicate
+	 * segment is spliced back into the train's registered path, and the train's
+	 * `Front` process (`Train.kt`) loses track of its position — permanently
+	 * stalling (confirmed via `PathReservationRegistry` merge tracing during root
+	 * cause analysis; the malformed splice is silently "allowed" as a false-positive
+	 * "circular route" before a *third* occurrence trips the existing cycle guard).
+	 *
+	 * Skipping a triple already recorded in [appliedReservations] restores the
+	 * "at most one real application per reservation need" invariant that held
+	 * naturally under the pre-SP0.11 synchronous (decide-and-apply-in-one-step)
+	 * architecture, without touching the shared navigation/registry code that
+	 * other callers (e.g. `InOutWorker`) also depend on.
+	 *
+	 * @since Issue #733 (SP0.11 — Goal 10); duplicate-suppression guard added as a
+	 *   regression fix for the resulting train-freeze/deadlock
+	 */
 	private fun applyReservePath(decision: DispatchDecision.ReservePath) {
+		val reservationKey = "${decision.trainId}|${decision.fromSemaphoreName}|${decision.toSeparatorName}"
+		if (reservationKey in appliedReservations) {
+			logger.debug {
+				"Skipping duplicate ReservePath: trainId=${decision.trainId} " +
+					"${decision.fromSemaphoreName} → ${decision.toSeparatorName} (already applied)"
+			}
+			return
+		}
+
 		logger.debug {
 			"Applying ReservePath: trainId=${decision.trainId} " +
 				"${decision.fromSemaphoreName} → ${decision.toSeparatorName}"
@@ -139,6 +202,7 @@ class DispatchDecisionApplier(
 				logger.debug {
 					"ReservePath: reserved ${result.blocksCount} block(s) for ${decision.trainId}"
 				}
+				appliedReservations.add(reservationKey)
 				onBlockTransition(decision.trainId)
 			}
 			is RouteRequestResult.AllPathsBlocked -> {
diff --git a/dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/di/DispatcherAgentModule.kt b/dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/di/DispatcherAgentModule.kt
@@ -37,12 +37,13 @@ import org.koin.dsl.module
  *
  * @since Issue #733 (SP0.11 — Goal 10)
  */
-val dispatcherAgentModule: Module = module {
-	// Dispatcher: global singleton — RuleBasedDispatcher is stateless (pure function).
-	single<Dispatcher> { RuleBasedDispatcher() }
+val dispatcherAgentModule: Module =
+	module {
+		// Dispatcher: global singleton — RuleBasedDispatcher is stateless (pure function).
+		single<Dispatcher> { RuleBasedDispatcher() }
 
-	scope<DefaultSimulationContext> {
-		// ActuatorCommandQueue: one thread-safe handoff queue per simulation context.
-		scoped<ActuatorCommandQueue> { ActuatorCommandQueue() }
+		scope<DefaultSimulationContext> {
+			// ActuatorCommandQueue: one thread-safe handoff queue per simulation context.
+			scoped<ActuatorCommandQueue> { ActuatorCommandQueue() }
+		}
 	}
-}
diff --git a/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/ActuatorCommandQueueTest.kt b/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/ActuatorCommandQueueTest.kt
@@ -22,6 +22,7 @@ import assertk.assertions.isTrue
 import cz.vutbr.fit.interlockSim.sim.DispatchDecision
 import org.junit.jupiter.api.DisplayName
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
 import java.util.concurrent.CountDownLatch
 import java.util.concurrent.Executors
 import java.util.concurrent.TimeUnit
@@ -42,6 +43,7 @@ private const val CONSUMER_POLL_INTERVAL_MS: Long = 1
  * @since Issue #730 (SP0.8 — Goal 10)
  */
 @DisplayName("ActuatorCommandQueue — thread-safe driver-to-sim command handoff")
+@Timeout(30, unit = TimeUnit.SECONDS)
 class ActuatorCommandQueueTest {
 	@Test
 	@DisplayName("drain returns decisions in FIFO order")
diff --git a/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/AgentLoopDriverTest.kt b/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/AgentLoopDriverTest.kt
@@ -29,6 +29,8 @@ import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.DisplayName
 import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
+import java.util.concurrent.TimeUnit
 
 /**
  * Unit tests for [AgentLoopDriver].
@@ -50,6 +52,7 @@ import org.junit.jupiter.api.Test
  * @since Issue #732 (SP0.10 — Goal 10)
  */
 @DisplayName("AgentLoopDriver — paced sense-decide-act cycle")
+@Timeout(30, unit = TimeUnit.SECONDS)
 class AgentLoopDriverTest {
 	private lateinit var perceptionPort: NetworkPerceptionPort
 	private lateinit var dispatcher: Dispatcher
diff --git a/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/DispatchDecisionApplierTest.kt b/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/DispatchDecisionApplierTest.kt
@@ -25,6 +25,8 @@ import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.DisplayName
 import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
+import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicReference
 
 /**
@@ -41,6 +43,7 @@ import java.util.concurrent.atomic.AtomicReference
  * @since Issue #731 (SP0.9 — Goal 10)
  */
 @DisplayName("DispatchDecisionApplier — drain and apply decisions via actuator ports")
+@Timeout(30, unit = TimeUnit.SECONDS)
 class DispatchDecisionApplierTest {
 	private lateinit var networkActuator: NetworkActuatorPort
 	private val approvedTrains = mutableListOf<String>()
@@ -232,6 +235,118 @@ class DispatchDecisionApplierTest {
 		}
 	}
 
+	// ── Duplicate-reservation suppression (SP0.11 regression fix) ────────────
+
+	/**
+	 * Regression coverage for the SP0.11 async-driver race (Issue #733 follow-up):
+	 * [AgentLoopDriver] runs decoupled from the sim thread, so it can legitimately
+	 * decide the *same* `ReservePath` more than once before the block-input
+	 * observation reflects the first application. Re-applying an already-reserved
+	 * hop is not a harmless no-op — it corrupts the train's registered path via
+	 * `PathReservationRegistry.registerPathInfo`'s merge logic and permanently
+	 * stalls the train. [DispatchDecisionApplier] must apply each distinct
+	 * `(trainId, from, to)` reservation at most once.
+	 */
+	@Nested
+	@DisplayName("Duplicate-reservation suppression (SP0.11 regression fix)")
+	inner class DuplicateReservationSuppression {
+		@Test
+		@DisplayName("Identical ReservePath decision drained twice is applied to the actuator port only once")
+		fun duplicateReservePath_appliedOnlyOnce() {
+			every { networkActuator.requestRoute(any(), any(), any()) } returns RouteRequestResult.Reserved("T1", 1)
+			val (queue, applier) = makeApplier()
+
+			// First drain: the reservation is genuinely new.
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep()
+
+			// Second drain: the driver decided the identical hop again before observing
+			// the first application (the race this guard exists to suppress).
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep()
+
+			verify(exactly = 1) { networkActuator.requestRoute("T1", "zB", "doA1") }
+		}
+
+		@Test
+		@DisplayName("Identical ReservePath decision applied twice within the same drain is applied only once")
+		fun duplicateReservePath_sameDrain_appliedOnlyOnce() {
+			every { networkActuator.requestRoute(any(), any(), any()) } returns RouteRequestResult.Reserved("T1", 1)
+			val (queue, applier) = makeApplier()
+
+			queue.postAll(
+				listOf(
+					DispatchDecision.ReservePath("T1", "zB", "doA1"),
+					DispatchDecision.ReservePath("T1", "zB", "doA1")
+				)
+			)
+			applier.onControlStep()
+
+			verify(exactly = 1) { networkActuator.requestRoute("T1", "zB", "doA1") }
+		}
+
+		@Test
+		@DisplayName("onBlockTransition fires only once for a duplicated ReservePath")
+		fun duplicateReservePath_blockTransitionCountedOnce() {
+			every { networkActuator.requestRoute(any(), any(), any()) } returns RouteRequestResult.Reserved("T1", 1)
+			var transitionCount = 0
+			val queue = ActuatorCommandQueue()
+			val applier =
+				DispatchDecisionApplier(
+					queue = queue,
+					networkActuator = networkActuator,
+					onApproveTrain = onApproveTrain,
+					onBlockTransition = { transitionCount++ }
+				)
+
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep()
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep()
+
+			assertThat(transitionCount).isEqualTo(1)
+		}
+
+		@Test
+		@DisplayName("A different (trainId, from, to) triple is not suppressed by an earlier reservation")
+		fun differentTriple_notSuppressed() {
+			every { networkActuator.requestRoute(any(), any(), any()) } returns RouteRequestResult.Reserved("T1", 1)
+			val (queue, applier) = makeApplier()
+
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep()
+			// Same train, different hop — genuine forward progress, must still be applied.
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "doA1", "A")))
+			applier.onControlStep()
+
+			verify(exactly = 1) { networkActuator.requestRoute("T1", "zB", "doA1") }
+			verify(exactly = 1) { networkActuator.requestRoute("T1", "doA1", "A") }
+		}
+
+		@Test
+		@DisplayName("A failed reservation is not remembered, so a later retry of the same triple is applied")
+		fun failedReservation_retryIsApplied() {
+			var callCount = 0
+			every { networkActuator.requestRoute("T1", "zB", "doA1") } answers {
+				callCount++
+				if (callCount == 1) {
+					RouteRequestResult.AllPathsBlocked(attemptedPaths = 1)
+				} else {
+					RouteRequestResult.Reserved("T1", 1)
+				}
+			}
+			val (queue, applier) = makeApplier()
+
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep() // fails — must not be recorded as applied
+
+			queue.postAll(listOf(DispatchDecision.ReservePath("T1", "zB", "doA1")))
+			applier.onControlStep() // legitimate retry — must reach the actuator port
+
+			verify(exactly = 2) { networkActuator.requestRoute("T1", "zB", "doA1") }
+		}
+	}
+
 	// ── Thread-identity contract ──────────────────────────────────────────────
 
 	@Nested
diff --git a/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/RuleBasedDispatcherDeterminismTest.kt b/dispatcher-agent/src/test/kotlin/cz/vutbr/fit/interlockSim/dispatcher/RuleBasedDispatcherDeterminismTest.kt
@@ -16,13 +16,13 @@ import assertk.assertions.isGreaterThanOrEqualTo
 import cz.vutbr.fit.interlockSim.context.DefaultSimulationContext
 import cz.vutbr.fit.interlockSim.context.EditingContext
 import cz.vutbr.fit.interlockSim.context.NoOpSimulationController
+import cz.vutbr.fit.interlockSim.ports.DefaultNetworkActuatorPort
+import cz.vutbr.fit.interlockSim.ports.DefaultNetworkPerceptionPort
 import cz.vutbr.fit.interlockSim.sim.DefaultSimulationProcessFactory
 import cz.vutbr.fit.interlockSim.sim.RuleBasedDispatcher
 import cz.vutbr.fit.interlockSim.sim.ShuntingLoop
 import cz.vutbr.fit.interlockSim.testutil.TestFixtures
 import cz.vutbr.fit.interlockSim.xml.XMLContextFactory
-import cz.vutbr.fit.interlockSim.ports.DefaultNetworkActuatorPort
-import cz.vutbr.fit.interlockSim.ports.DefaultNetworkPerceptionPort
 import io.github.oshai.kotlinlogging.KotlinLogging
 import org.junit.jupiter.api.AfterEach
 import org.junit.jupiter.api.BeforeEach
diff --git a/dispatcher-agent/src/test/resources/logback-test.xml b/dispatcher-agent/src/test/resources/logback-test.xml
