SessionFsm Hardening

[kevinherron]

Summary

Corrects client-side Session establishment, reactivation, keep-alive accounting, and teardown in SessionFsm and SessionFsmFactory, under entry actions that throw, a Server that revises the Session timeout below the configured keep-alive interval, a channel lost before the client is watching for it, a definitive session-level status delivered as a channel-level Error, and a transport deliberately disconnected out from under an Active Session.

This began as a review of the two files. Twelve findings were raised; each was verified against the source and against the strict-machine and netty-channel-fsm sources before anything was changed. Eight were real and are fixed here, every one with a regression test that fails without the fix. The other four did not survive verification and are deliberately not addressed.

FSM totality

StrictMachine sets the next state before it runs the matching transition actions, and it catches and merely logs whatever those actions throw. An entry action that throws before attaching the whenComplete that fires the corresponding Success/Failure event therefore leaves the FSM parked in the new state with no event pending, and no Session state has a timeout to kick it out again. CloseSession is shelved rather than acted on in Creating, Activating, Transferring, Initializing and Reactivating, and a shelved event's future completes only when a later transition drains the shelf, so a wedge takes disconnect() with it: disconnectAsync() has an exceptionally but no timeout, and a future that never completes is not an exceptional one.

activateSession and reactivateSession were already total for this reason. createSession and closeSession now are too. createSession had two synchronous throw sites reachable before the request is sent — endpoint.getServer().getGatewayServerUri() NPEs when the configured EndpointDescription carries no server, and getSessionName().get() invokes a user-supplied Supplier outside any guard — and its body is now wrapped the way the other two already wrap theirs, routing the original Throwable into the existing whenComplete and on to CreatingWait as any other CreateSession failure would.

Transferring was exposed by a different mechanism: transferSubscriptions returns transferFuture but completes it from inside a whenComplete whose own result future is discarded, so anything the callback throws is captured by a future nobody observes. The reachable throw is requireNonNull(tsr.getResults()) against a Server answering Good with no Results array. An exceptionally on that discarded chain now guarantees transferFuture is always completed.

Sessions abandoned on the Server

CloseSessionRequest was built in one place, whose only caller was the Closing entry action — and that action reads KEY_SESSION, a context key written only on entry to Active. Once CreateSession had succeeded, every establishment failure that followed dropped straight back to CreatingWait via handleFailureToOpenSession, which did nothing but fail the pending Session future. The Activating, Transferring and Initializing failure edges all abandoned a Session that exists on the Server with no way left to name it, since the failure events carry only a Throwable. The Server reaps such a Session only when its timeout elapses; nothing ties Session lifetime to the SecureChannel, and a rejected ActivateSession leaves the entry in createdSessions. With a 120s default revised timeout and the CreatingWait backoff capped at 16s, a persistent ActivateSession failure — wrong credentials, an untrusted X.509 identity — keeps a rolling set of roughly eight orphans alive indefinitely.

KEY_PENDING_SESSION now names the state the FSM had no word for: a Session that exists on the Server but is not yet reachable via KEY_SESSION. It holds the CreateSessionResponse, is set on the Creating --CreateSessionSuccess--> Activating edge where both the sessionId and the authentication token are in hand, and is removed on entry to Active beside KEY_SESSION.set(), so exactly one of the two keys ever refers to a live Session. handleFailureToOpenSession removes it and, when it is non-null, sends a best-effort CloseSession — one hook covering all three edges, and a no-op on the edges where nothing is pending. The Reactivating --ReactivateSessionFailure--> CreatingWait edge gets a close of its own, since what it abandons is the old Session rather than a pending one.

Both of these closes are new, and both send deleteSubscriptions false, so an abandoned Session's Subscriptions survive for the replacement Session's TransferSubscriptions to recover; an abandonSession overload names that intent beside closeSession. The Closing entry action — the explicit client-shutdown path, and the only CloseSession the base branch ever sent — still closes with deletion and is otherwise untouched.

Reactivation

Reactivating had one edge to CreatingWait, guarded by a predicate asking whether UaException.extract(failure) produced a UaServiceFaultException. Every other ReactivateSessionFailure took the catch-all edge back to ReactivatingWait, where the wait doubles and saturates at 16s. That made the decision to abandon a Session turn on how the Server chose to frame its answer rather than on what the answer said. A Server replying to ActivateSession with a channel-level Error message produces a plain UaException carrying the same StatusCode — InboundUascResponseHandler builds it and failAndClearPending completes the in-flight ActivateSession with it — so Bad_SessionIdInvalid, Bad_SessionClosed and Bad_SessionNotActivated delivered that way never escalated, and the client re-sent ActivateSession every lap for a Session the Server had already said it no longer has. Part 4 §6.7 requires a new Session when ActivateSession fails, and that obligation does not depend on the message type the status arrived in.

The guard now keys on the StatusCode as well as the exception class. The SESSION_ERROR predicate naming the three definitive session-level statuses is hoisted out of SessionFaultListener to a top-level constant with its contents unchanged. Failures carrying no such status — Bad_Timeout or Bad_ConnectionClosed from a request timer or a dropped channel — are connectivity problems and deliberately keep the indefinite ReactivatingWait retry: Part 4 §5.7.2.1 has the client establish a new connection and call ActivateSession again, and dropping to CreateSession while the channel is merely down throws away a Session whose Subscriptions could still be transferred. Behavior against a conformant Server, Milo's own included, is unchanged.

Milo builds its ChannelFsm with setLazy(false) and setPersistent(true), so a channel that drops unexpectedly reconnects without the Session doing anything. There is exactly one state that does not self-heal: NotConnected, reachable only through an explicit Disconnect, whose sole outbound edge is Connect and whose GetChannel completes immediately with a plain Exception("not connected"). An application calling getTransport().disconnect() while a Session is Active parks the transport somewhere nothing will move it from, and the Session did not notice — it left Active for ReactivatingWait either at once via the transition listener or one keep-alive interval later, and every reactivation attempt then failed with a bare Exception that carries no StatusCode and is not a UaException, so no escalation predicate could match. The FSM cycled ReactivatingWait ⇄ Reactivating at the 16s cap for the life of the client with every caller blocked in getSession() left hanging.

Reactivating now has a terminal edge to Inactive, guarded by the ordinary reactivation-failure predicate plus a check that the transport is an OpcTcpClientTransport whose ChannelFsm reads NotConnected — false for any other transport, so websocket and https behavior is untouched. The edge clears the wait time so a later Session starts from a fresh backoff, drops KEY_SESSION, logs one warning, and calls handleFailureToOpenSession so pending futures fail with the "not connected" cause instead of waiting forever. Leaving the FSM in Inactive keeps both exits open: the existing onTransitionTo(Inactive) action drains the shelf, so a disconnect() whose CloseSession was shelved still completes, and Inactive --OpenSession--> Creating means a subsequent connect() builds a fresh Session. The new edge is declared before the escalation to CreatingWait because StrictMachine takes the first matching transition, and escalating over a dead transport would only relocate the identical endless loop.

Keep-alive

The Event.KeepAlive internal transition captured the machine-wide ActionContext, and its whenComplete read-modify-wrote a single machine-wide failure count with nothing tying the completion back to the Session epoch the Read was sent on. Leaving Active cancels the scheduled future, which stops new keep-alives but does not cancel one already sent, and the Active --ServiceFault--> ReactivatingWait edge never takes the channel down, so failAndClearPending never drains it. The Read stayed parked in the transport across the whole reactivation cycle and, when it finally failed, incremented the new epoch's count. With the default keepAliveFailuresAllowed of 1 that spends an interval of the new epoch's budget on an observation about a Session that is already gone; with two such stale failures it fires Event.KeepAliveFailure outright and closes a still-healthy channel on a Session the Server had just accepted an ActivateSession for. The read-modify-write was also non-atomic — a get under the read lock and a set under the write lock — so completions racing on the shared executor could lose an increment.

The count is now a per-epoch AtomicLong installed on entry to Active, so the instance itself identifies the epoch. The action captures that instance before sending, and the callback returns before touching shared state if the current state is not Active, if KEY_SESSION no longer holds the event's session, or if the key no longer holds the captured instance. The last check is load-bearing: on the ServiceFault route the session and the channel are the same and the next epoch is Active again, so only the per-epoch instance separates them.

The RevisedSessionTimeout the Server answers CreateSession with was stored on the OpcUaSession and exposed as UaSession.getSessionTimeout(), but nothing in sdk-client read it back. The keep-alive was scheduled straight from OpcUaClientConfig.getKeepAliveInterval(), so the one mechanism that keeps an otherwise idle Session alive was driven entirely by a number the client picked and never reconciled with the number the Server granted. Part 4 §5.7.2.2 makes that number binding. When the configured interval lands at or above the revised timeout the Session dies between keep-alives, and the recovery machinery hides it: the Bad_SessionIdInvalid on the next keep-alive becomes Event.ServiceFault, a new Session is created and activated, and all of it is at DEBUG — the WARN needs the failure count to exceed keepAliveFailuresAllowed and the ServiceFault edge pre-empts it. The backoff does not escalate either, since the wait time is cleared on entry to Active, so the client churns Active → ReactivatingWait → Creating → Active for as long as it stays connected, with fresh Subscription transfers each lap and nothing in the log saying why.

The interval is now derived from the Session actually established: the configured value is bounded at half the revised timeout, with a 1000 ms floor on the derived value so a Server revising to something very small cannot turn the keep-alive into a request flood. Half leaves room for exactly one keep-alive round trip to be lost before the Session expires, the same tolerance keepAliveFailuresAllowed already assumes. The outer Math.min means the clamp only ever lowers the interval, so it can only send more keep-alives than before, never fewer, and cannot break a configuration that was already working. A WARN naming all three numbers is emitted only when the clamp changes the value, once per Session epoch. With Milo defaults nothing changes at all.

Connection loss during establishment

The ChannelFsm.TransitionListener that turns a secure channel drop into Event.ConnectionLost is constructed and registered inside the Initializing --> Active transition action, and it reacts only to the edge leaving Connected. netty-channel-fsm dispatches listeners synchronously over the snapshot taken while the event is being evaluated, so a Connected --> ReconnectWait edge evaluated before the listener was added is lost permanently: the rest of the reconnect cycle contains no further transition originating from Connected, and that is the only shape a drop takes for a non-lazy persistent FSM. The listener is also removed on every Active --> non-Active transition, so nothing is watching the channel while the Session is being established.

The window is wide rather than a narrow race. runSequentially wraps each initializer with .exceptionally(ex -> Unit.VALUE), so a request failed with Bad_ConnectionClosed is swallowed and Event.InitializeSuccess fires anyway; the ChannelFsm learns of the loss from a pipeline handler on the netty thread, whereas the SessionFsm can only enqueue InitializeSuccess after the failed request's callback runs on the transport executor, so the ChannelFsm edge is evaluated first essentially every time. The result is a SessionFsm parked in Active holding a Session bound to a secure channel that no longer exists, with no onSessionInactive reported and application requests failing until the first request on the new channel draws Bad_SecureChannelIdInvalid. That fallback makes this delayed recovery rather than a permanently dead Session, but the delay is a keep-alive interval plus reconnect backoff, and it is paid in failed requests.

The action now re-reads channelFsm.getState() immediately after addTransitionListener and, if the ChannelFsm is no longer Connected, fires Event.ConnectionLost itself. The Session can only have been created and activated over a Connected channel, so a non-Connected state at that point means the channel was lost after the Session was established. The synthesized event is evaluated after the transition into Active completes, so it is consumed by the Active --> ReactivatingWait edge; a later duplicate from the listener is harmless, since ConnectionLost is consumed only in Active.

If the drop and the subsequent reconnect both complete before the action runs, the ChannelFsm is back in Connected and the state check cannot see it. Recognizing that would mean recording which channel instance the Session was bound to and comparing identities, a larger change than this is scoped for; that case remains covered by the Bad_SecureChannelIdInvalid path.

TransferSubscriptions indexing

#1823 independently fixed the misindexing this branch was originally written for, capturing the ids sent and the Subscriptions they came from in one pass and bounding both result loops against that aligned list. The two fixes conflicted, and the pairing is kept here in record form rather than as two parallel ArrayLists: the failure mode both address is a pairing that has silently shifted, and a single list of (subscription, id) cannot shift against itself, whereas two lists have to be maintained in lockstep to stay aligned. The ids array is derived from that list, so the request and the results are indexed against the same thing by construction, and the debug-logging zip streams that same array rather than deriving the ids from the snapshot a third time — which was a third independently racy read. The failure loop keeps #1823's double bound and its synchronous handleTransferFailure cleanup.

What is new beyond #1823 is the early return on the filtered list: a snapshot of entirely id-less Subscriptions previously still sent a TransferSubscriptionsRequest carrying no SubscriptionIds.

Behavior changes

• The keep-alive interval is bounded by the revised Session timeout — half of it, with a 1000 ms floor, and a WARN when the clamp changes the configured value. The clamp only ever lowers the interval. getSessionTimeout() keeps its current meaning as the raw revised value, and the configured value is not mutated.
• A Session abandoned during establishment, or abandoned in favor of a replacement, is closed on the Server rather than left to expire on its Session timeout. Both closes send deleteSubscriptions false, so the abandoned Session's Subscriptions remain transferable. The explicit client-shutdown close is unchanged and still deletes.
• Reactivation escalates on the StatusCode, so a definitive session-level status delivered as a channel-level Error now creates a new Session instead of retrying indefinitely.
• Reactivation over a deliberately disconnected transport ends in Inactive and fails pending getSession()/openSession() futures with the "not connected" cause, instead of cycling forever. A subsequent connect() builds a fresh Session.
• A synchronous throw from createSession completes the caller's openSession() future with that cause rather than wedging the FSM.
• No TransferSubscriptions request is sent when no Subscription in the snapshot holds an id.

🤖 Originally generated with Claude Code; updated after subsequent code review.