Fix multinode_Sim and simulation tests

Server2 multinode_Sim had unexpected transitions caused by a race with
withConnectionManager. So

ConnectionManager simulation had weird PopScheduleOutboundError and
leaked connections problems.

Both problems sources was due to weird race conditions caused to a wrong
assumption that the connVar could be reused if in the Terminating or
TerminatedState.

We also log TerminatingSt->TerminatedSt in the right place

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs

@@ -644,7 +644,6 @@ withConnectionManager ConnectionManagerArguments {
                           writeTVar connVar connState'
                           return $ There (Just transition)
                         TerminatingState {} -> do
-                          writeTVar connVar connState'
                           return $ Here (connVar, transition)
                         TerminatedState {} ->
                           return $ There Nothing
@@ -661,11 +660,10 @@ withConnectionManager ConnectionManagerArguments {
                   return ( Map.delete peerAddr state
                          , Left (Known (TerminatedState Nothing))
                          )
-                Here (connVar, transition) -> do
-                  traceWith trTracer transition
+                Here connVarAndTransition -> do
                   close cmSnocket socket
                   return ( state
-                         , Right connVar
+                         , Right connVarAndTransition
                          )
 
             case mConnVar of
@@ -682,7 +680,7 @@ withConnectionManager ConnectionManagerArguments {
               -- This case is impossible to reach since the previous atomically block
               -- does not return the 'Race' constructor.
               Left _ -> error "connection cleanup handler: impossible happened"
-              Right connVar ->
+              Right (connVar, transition) ->
                 do traceWith tracer (TrConnectionTimeWait connId)
                    when (cmTimeWaitTimeout > 0) $
                      unmask (threadDelay cmTimeWaitTimeout)
@@ -692,7 +690,11 @@ withConnectionManager ConnectionManagerArguments {
                   -- - handshake negotiation; or
                   -- - `Terminate: TerminatingState → TerminatedState` transition.
                   traceWith tracer (TrConnectionTimeWaitDone connId)
+
+                  -- TerminatingState -> TerminatedState transition
+                  traceWith trTracer transition
                   trs <- atomically $ do
+                    writeTVar connVar (TerminatedState Nothing)
                     --  We have to be careful when deleting it from
                     --  'ConnectionManagerState'.
                     updated <-
@@ -800,7 +802,7 @@ withConnectionManager ConnectionManagerArguments {
 
           Right (handle, version) -> do
             let dataFlow = connectionDataFlow version
-            transition <- atomically $ do
+            mbTransition <- atomically $ do
               connState <- readTVar connVar
               case connState of
                 -- Inbound connections cannot be found in this state at this
@@ -820,7 +822,7 @@ withConnectionManager ConnectionManagerArguments {
                                      connId connThread handle
                                      (connectionDataFlow version)
                   writeTVar connVar connState'
-                  return (mkTransition connState connState')
+                  return (Just $ mkTransition connState connState')
 
                 -- It is impossible to find a connection in 'OutboundUniState'
                 -- or 'OutboundDupState', since 'includeInboundConnection'
@@ -837,7 +839,7 @@ withConnectionManager ConnectionManagerArguments {
                                      connId connThread handle
                                      dataFlow'
                   writeTVar connVar connState'
-                  return (mkTransition connState connState')
+                  return (Just $ mkTransition connState connState')
 
                 InboundIdleState {} ->
                   throwSTM (withCallStack (ImpossibleState peerAddr))
@@ -852,19 +854,9 @@ withConnectionManager ConnectionManagerArguments {
                 DuplexState {} ->
                   throwSTM (withCallStack (ImpossibleState peerAddr))
 
-                TerminatingState {} -> do
-                  let connState' = InboundIdleState
-                                     connId connThread handle
-                                     (connectionDataFlow version)
-                  writeTVar connVar connState'
-                  return (mkTransition connState connState')
+                TerminatingState {} -> return Nothing
 
-                TerminatedState {} -> do
-                  let connState' = InboundIdleState
-                                     connId connThread handle
-                                     (connectionDataFlow version)
-                  writeTVar connVar connState'
-                  return (mkTransition connState connState')
+                TerminatedState {} -> return Nothing
             traceCounters stateVar
             -- Note that we don't set a timeout thread here which would perform
             -- @
@@ -877,8 +869,14 @@ withConnectionManager ConnectionManagerArguments {
             -- idle, it will call 'unregisterInboundConnection' which will
             -- perform the aforementioned @Commit@ transition.
 
-            traceWith trTracer (TransitionTrace peerAddr transition)
-            return (Connected connId dataFlow handle)
+            traverse_ (traceWith trTracer . TransitionTrace peerAddr) mbTransition
+
+            -- If mbTransition is Nothing, it means that the connVar was read
+            -- either in Terminating or TerminatedState. Either case we should
+            -- return Disconnected instead of Connected.
+            return (maybe (Disconnected connId Nothing)
+                          (const $ Connected connId dataFlow handle)
+                          mbTransition)
 
 
     unregisterInboundConnectionImpl
@@ -1112,16 +1110,10 @@ withConnectionManager ConnectionManagerArguments {
                   retry
 
                 TerminatedState _handleError -> do
-                  -- the connection terminated; we can reset 'connVar' and
-                  -- start afresh.
-                  let connState' = ReservedOutboundState
-                  writeTVar connVar connState'
-                  return ( Just (Left (TransitionTrace
-                                        peerAddr
-                                        (mkTransition connState connState')))
-                         , connVar
-                         , Right Nowhere
-                         )
+                  -- the connection terminated; we can not reset 'connVar' and
+                  -- start afresh. We should wait for the removal of the
+                  -- connection from the state.
+                  retry
 
             Nothing -> do
               let connState' = ReservedOutboundState
@@ -1455,7 +1447,7 @@ withConnectionManager ConnectionManagerArguments {
                 -- @
                 --   DemotedToCold^{Unidirectional}_{Local}
                 --     : OutboundState Unidirectional
-                --     → TerminatingState
+                --     → OutboundIdleState Unidirectional
                 -- @
                 let connState' = OutboundIdleState connId connThread handle
                                                    Unidirectional

ouroboros-network-framework/test/Test/Ouroboros/Network/ConnectionManager.hs

@@ -80,8 +80,6 @@ import qualified Ouroboros.Network.InboundGovernor.ControlChannel as ControlChan
 
 import           Ouroboros.Network.Testing.Utils (Delay (..),
                    genDelayWithPrecision)
--- import qualified Debug.Trace as Debug
-
 
 tests :: TestTree
 tests =
@@ -110,7 +108,6 @@ tests =
     ]
   ]
 
-
 -- | Addresss type.  '0' indicates local address, the 'Arbitrary' generator only
 -- returns (strictly) positive addresses.
 --