Skip to content

Commit 7041d10

Browse files
bedaHovorkaclaude
andcommitted
fix(#742): reject reservation of physically impossible switch routes
Trace capture from failing RuleBasedDispatcherDeterminismTest runs showed the remaining ~40% failures were NOT the suspected mergePathInfo fork-overlap corruption (all 250 observed merges were clean continuations). The real defect: when a train's through-exit was momentarily blocked, findNextReservationTarget offered the sibling parallel branch's semaphore (e.g. doB1→doB2), reservePath reserved it, and configureSwitchesInPath swallowed the FATAL PathSeparatorChangeException ("switch doesn't join this segments") — returning Success for a route no configuration of switch vB can join. The train committed to an untraversable route, isPathExtendedBeyond then suppressed the corrective decision forever, and the network gridlocked (trainsExited 1 instead of 5). Fix (railway-conservative, service layer only): - configureSwitchesInPath returns Boolean: a genuinely traversed switch (both segments known) that no configuration joins now fails the candidate; null-segment cases keep the historical lenient skip (Issue #300) - configureAndRegisterSwitches orders configuration BEFORE registerSwitches - rollbackUnconfigurableCandidate undoes only the candidate's blocks and newly locked switches — the train's pre-existing path state survives, so it simply waits and reserves the through route at a future simulation time - registerPathInfo moved AFTER switch+signal configuration so no rollback can leave a poisoned PathInfo Circular-route merging is untouched: mergePathInfo/addElementWithCycleDetection unchanged; Issue316RegressionTest and PathReservationRegistryMergingTest pass unchanged. New Issue742RegressionTest (@timeout): impossible diversion rejected without leaking blocks/locks/PathInfo (failed pre-fix), wait-then-extend partial-path semantics, legitimate parallel alternate zB→doA2 still succeeds. Determinism evidence on the dev machine: pre-fix 55/100 failed repetitions (0/10 suites green); post-fix 8/210 failed repetitions, all matching the pre-existing headless driver-pacing race (trainsExited=0 occurred 22×/100 pre-fix too) — that residue is AgentLoopDriver pacing, tracked separately. Closes #742 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent fbf5c89 commit 7041d10

2 files changed

Lines changed: 392 additions & 41 deletions

File tree

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/context/navigation/DefaultPathReservationService.kt

Lines changed: 153 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -267,30 +267,11 @@ class DefaultPathReservationService(
267267
trackSections = path // path is List<TrackSection> here
268268
)
269269

270-
// Step 2f: Register PathInfo metadata (Issue #295/#296 Phase 4)
271-
registry.registerPathInfo(trainId, pathInfo)
272-
logger.debug {
273-
"reservePath: Registered PathInfo for $trainId with ${pathInfo.entryDirections.size} entry directions, " +
274-
"reserved path has ${pathInfo.reservedPath.length()} elements"
275-
}
276-
277-
// Step 2f.1: Register switches (Tier 2 - Issue #291)
278-
val switches = extractUniqueSwitches(pathInfo)
279-
if (switches.isNotEmpty()) {
280-
registry.registerSwitches(trainId, switches)
281-
logger.debug {
282-
"reservePath: Registered ${switches.size} switches for $trainId"
283-
}
284-
}
285-
286-
// Step 2f.2: Configure switches based on path topology (Issue #300)
287-
// Switches must be configured BEFORE semaphore signals are set up
288-
// This ensures switches are in correct position (MAIN/BRANCH) for the reserved route
289-
if (switches.isNotEmpty()) {
290-
val configuredCount = configureSwitchesInPath(trainId, pathInfo)
291-
logger.debug {
292-
"reservePath: Configured $configuredCount of ${switches.size} switches for $trainId"
293-
}
270+
// Step 2f: Configure and register switches (Issue #300, #291, #742).
271+
// A candidate whose switches cannot be configured is physically impossible
272+
// and must fail the reservation — see configureAndRegisterSwitches.
273+
if (!configureAndRegisterSwitches(trainId, pathInfo, forwardBlocks)) {
274+
return PathReservationService.ReservationResult.AllPathsBlocked(1)
294275
}
295276

296277
// Step 2g: Configure semaphore signal after successful reservation
@@ -363,6 +344,17 @@ class DefaultPathReservationService(
363344
// (signal=STOP) and wait forever.
364345
configureIntermediateSemaphores(blocks)
365346

347+
// Step 2i: Register PathInfo metadata (Issue #295/#296 Phase 4; moved here by
348+
// Issue #742). Registration happens only after switches AND signals configured
349+
// successfully, so no rollback path can leave a poisoned PathInfo behind —
350+
// a PathInfo pointing at an unusable route permanently stalls the train
351+
// (isPathExtendedBeyond suppresses the corrective re-reservation).
352+
registry.registerPathInfo(trainId, pathInfo)
353+
logger.debug {
354+
"reservePath: Registered PathInfo for $trainId with ${pathInfo.entryDirections.size} entry directions, " +
355+
"reserved path has ${pathInfo.reservedPath.length()} elements"
356+
}
357+
366358
// Emit BlockReserved for each successfully reserved block
367359
val simTime = currentSimulationTime()
368360
blocks.forEach { block ->
@@ -1580,22 +1572,30 @@ class DefaultPathReservationService(
15801572
* interlocking principles: switches must be positioned and locked before
15811573
* authorizing train movement.
15821574
*
1583-
* ## Error Handling
1575+
* ## Error Handling (Issue #742)
1576+
*
1577+
* Two failure classes are distinguished:
15841578
*
1585-
* Any internal [PathSeparatorChangeException] from switch configuration
1586-
* is handled via logging only and is not propagated to callers. Switches that
1587-
* cannot be configured are skipped (this may occur when path topology doesn't
1588-
* actually traverse the switch).
1579+
* - **Undeterminable segments** (`from`/`to` is null, or no next track): the path does not
1580+
* geometrically traverse the switch, so it is skipped leniently — the pre-#742 behavior.
1581+
* - **Genuinely traversed switch that no configuration joins**
1582+
* ([PathSeparatorChangeException] with both segments known): the candidate route is
1583+
* physically impossible (e.g. the sibling-branch "diversion" doB1→doB2 through vB, whose
1584+
* legs no configuration of vB connects). This now FAILS the whole configuration —
1585+
* returning `false` — instead of being silently skipped. Silently skipping reserved
1586+
* untraversable routes and permanently stalled trains (captured in failing
1587+
* `RuleBasedDispatcherDeterminismTest` runs).
15891588
*
15901589
* @param trainId Train identifier for logging and occupant creation
15911590
* @param pathInfo PathInfo containing the reserved path with switches
1592-
* @return Number of switches successfully configured
1591+
* @return `true` when every genuinely traversed switch was configured, `false` when the
1592+
* route is impossible and the caller must roll the candidate back (Issue #742)
15931593
* @since Issue #300 Fix switch animation regression
15941594
*/
15951595
private fun configureSwitchesInPath(
15961596
trainId: String,
15971597
pathInfo: cz.vutbr.fit.interlockSim.objects.paths.PathInfo
1598-
): Int {
1598+
): Boolean {
15991599
// Convert Path to list for indexed access
16001600
val pathElements = pathInfo.reservedPath.toList()
16011601

@@ -1648,9 +1648,17 @@ class DefaultPathReservationService(
16481648
val from = context.getSegment(element, previous, next)
16491649
val to = context.getSegment(element, next, previous)
16501650

1651-
// Try to configure the switch - if it fails, skip this switch
1652-
// Some switches in the path may not need configuration (e.g., already configured,
1653-
// or path doesn't actually traverse the switch in a way that changes its state)
1651+
// Lenient skip (pre-#742 behavior): segments undeterminable means the path does
1652+
// not geometrically traverse this switch, so there is nothing to configure.
1653+
if (from == null || to == null) {
1654+
logger.info {
1655+
"configureSwitchesInPath: Skipped switch ${element.staticRef.getName()} " +
1656+
"for train $trainId - segments undeterminable, path does not traverse it " +
1657+
"(from=${from?.hashCode()}, to=${to?.hashCode()})"
1658+
}
1659+
return@forEachIndexed
1660+
}
1661+
16541662
try {
16551663
// Get allowed speed for this switch
16561664
val allowedSpeed = element.allowedSpeed()
@@ -1671,19 +1679,26 @@ class DefaultPathReservationService(
16711679
"(from=${from?.hashCode()}, to=${to?.hashCode()})"
16721680
}
16731681
} catch (e: PathSeparatorChangeException) {
1674-
// Switch configuration failed - segments don't match any valid configuration
1675-
// This is expected for switches in path that aren't actually traversed (e.g., parallel routes)
1676-
logger.info {
1677-
"configureSwitchesInPath: Skipped switch ${element.staticRef.getName()} " +
1678-
"for train $trainId - path topology doesn't require configuration " +
1679-
"(from=${from?.hashCode()}, to=${to?.hashCode()})"
1682+
// Issue #742: the route genuinely traverses this switch (both segments known)
1683+
// but NO switch configuration joins them — the candidate route is physically
1684+
// impossible. Reject the configuration so reservePath rolls the candidate
1685+
// back; silently skipping here reserved untraversable routes and permanently
1686+
// stalled trains.
1687+
logger.warn {
1688+
"configureSwitchesInPath: Switch ${element.staticRef.getName()} cannot join " +
1689+
"the route's segments for train $trainId - rejecting candidate route " +
1690+
"(from=${from.hashCode()}, to=${to.hashCode()}, Issue #742)"
16801691
}
16811692
logger.debug(e) { "Exception details: ${e.message}" }
1693+
return false
16821694
}
16831695
}
16841696
}
16851697

1686-
return configuredCount
1698+
logger.debug {
1699+
"configureSwitchesInPath: Configured $configuredCount switch(es) for train $trainId"
1700+
}
1701+
return true
16871702
}
16881703

16891704
/**
@@ -1848,6 +1863,103 @@ class DefaultPathReservationService(
18481863
}
18491864
}
18501865

1866+
/**
1867+
* Configure the candidate path's switches and register them on success (Issue #742).
1868+
*
1869+
* ## Ordering (Issue #300, #291, #742)
1870+
*
1871+
* Switches must be configured BEFORE semaphore signals are set up, so they are in the
1872+
* correct position (MAIN/BRANCH) for the reserved route. Configuration also runs BEFORE
1873+
* [PathReservationRegistry.registerSwitches] and [PathReservationRegistry.registerPathInfo]
1874+
* so a rejected candidate leaves no registry state and the rollback only needs to undo
1875+
* this candidate's blocks and switch locks.
1876+
*
1877+
* ## Why an unconfigurable switch fails the reservation (Issue #742)
1878+
*
1879+
* A switch the route genuinely traverses that CANNOT be configured (no switch
1880+
* configuration joins the route's entry/exit segments — e.g. the physically impossible
1881+
* sibling-branch "diversion" doB1→doB2 through vB) makes the whole candidate unusable.
1882+
* Returning Success anyway committed trains to untraversable routes and gridlocked the
1883+
* network. Failing instead means the train keeps waiting at its current semaphore and
1884+
* the through route is reserved at a future simulation time once it frees.
1885+
*
1886+
* @param trainId The train identifier
1887+
* @param pathInfo The candidate's PathInfo (switches are extracted from its path)
1888+
* @param forwardBlocks The candidate's freshly reserved blocks (for rollback on failure)
1889+
* @return `true` when the candidate's switches are configured and registered (or the
1890+
* path has none), `false` when the candidate was rolled back and the reservation
1891+
* must fail
1892+
*/
1893+
private fun configureAndRegisterSwitches(
1894+
trainId: String,
1895+
pathInfo: cz.vutbr.fit.interlockSim.objects.paths.PathInfo,
1896+
forwardBlocks: List<DynamicTrackBlock>
1897+
): Boolean {
1898+
val switches = extractUniqueSwitches(pathInfo)
1899+
if (switches.isEmpty()) {
1900+
return true
1901+
}
1902+
if (!configureSwitchesInPath(trainId, pathInfo)) {
1903+
rollbackUnconfigurableCandidate(trainId, forwardBlocks, switches)
1904+
return false
1905+
}
1906+
registry.registerSwitches(trainId, switches)
1907+
logger.debug {
1908+
"reservePath: Registered ${switches.size} switches for $trainId"
1909+
}
1910+
return true
1911+
}
1912+
1913+
/**
1914+
* Roll back a candidate path whose switches cannot be configured (Issue #742).
1915+
*
1916+
* Scoped strictly to THIS candidate's mutations — unlike [rollbackCompleteReservation]
1917+
* (which unregisters the train entirely), it must not touch the train's pre-existing
1918+
* blocks, switches or PathInfo: an extension attempt can fail mid-journey while the
1919+
* train is still running on its earlier reserved path, and that path must survive so
1920+
* the train simply keeps waiting for its through route.
1921+
*
1922+
* - Cancels path setup and unregisters ONLY the freshly reserved [forwardBlocks]
1923+
* - Unlocks ONLY this candidate's switches that are not registered to the train
1924+
* (switches locked by the train's earlier hops stay locked)
1925+
*
1926+
* PathInfo needs no rollback: Issue #742 moved [PathReservationRegistry.registerPathInfo]
1927+
* after switch and signal configuration, so nothing has been registered yet.
1928+
*
1929+
* @param trainId The train identifier
1930+
* @param forwardBlocks The freshly reserved blocks of the rejected candidate
1931+
* @param switches The rejected candidate's switches (possibly locked by setUpPath)
1932+
*/
1933+
private fun rollbackUnconfigurableCandidate(
1934+
trainId: String,
1935+
forwardBlocks: List<DynamicTrackBlock>,
1936+
switches: List<DynamicRailSwitch>
1937+
) {
1938+
val priorSwitches = registry.getSwitches(trainId).toSet()
1939+
switches.filterNot { it in priorSwitches }.forEach { switch ->
1940+
try {
1941+
switch.unlock()
1942+
} catch (e: Exception) {
1943+
logger.warn(e) { "rollbackUnconfigurableCandidate: Failed to unlock switch $switch" }
1944+
}
1945+
}
1946+
for (block in forwardBlocks) {
1947+
try {
1948+
val reservedFrom = block.reservedFrom
1949+
if (reservedFrom != null) {
1950+
block.cancelPathSetup(reservedFrom)
1951+
}
1952+
registry.unregisterBlock(trainId, block)
1953+
} catch (e: Exception) {
1954+
logger.warn(e) { "rollbackUnconfigurableCandidate: Failed to release block $block" }
1955+
}
1956+
}
1957+
logger.debug {
1958+
"rollbackUnconfigurableCandidate: Rolled back unconfigurable candidate for $trainId " +
1959+
"(${forwardBlocks.size} block(s), ${switches.size} switch(es) checked)"
1960+
}
1961+
}
1962+
18511963
/**
18521964
* Unregister all block reservations for a train.
18531965
*

0 commit comments

Comments
 (0)