Fix lost pre-handshake responses causing testAsynchronousBatch to hang

[vharseko]

Fixes #107

AsyncRemoteSecureConnectorInfoManagerTest.testAsynchronousBatch intermittently fails in CI after exactly 540 s with ConnectorException: Operation finished on remote server with unknown result (example run). This still reproduced after #105, which fixed a different race.

Root cause

1. OpenICFServerAdapter.onMessage dispatches every WebSocket message to a cached thread pool, so two messages delivered in order on the same socket can be processed out of order. On a freshly opened socket the batch-token operation response overtook the handshake response, and the pre-handshake branch of handleResponseMessage silently discarded it (Client received response('2') before handshake via socket:...).
2. With the token lost, BatchApiOpImpl$InternalRequest.CompletionListener timed out with returnToken == null and exited without completing the promise, leaving the caller blocked forever in promise.getOrThrowUninterruptibly() inside executeBatch().
3. The connection group checker (1 min + every 4 min, 3 inconsistent() strikes) finally failed the operation at the 9-minute mark — hence exactly 540 s.

Fix

• OpenICFServerAdapter: before dispatching a non-handshake message on a socket that is not yet handhooked, briefly wait (poll up to 10 s) for the concurrently-processing handshake to land instead of discarding the message. In the race this resolves in tens of milliseconds; if no handshake ever arrives, behavior falls through to the existing drop-with-error path.
• BatchApiOpImpl: when CompletionListener times out with no batch token, fail the promise with a ConnectorException instead of returning silently, so any future message loss surfaces as an error in ~5 s instead of a 9-minute hang. observer.onCompleted() is now only called when a token was actually delivered; the failure path signals observer.onError via the existing thenOnException chain.

Testing

• AsyncRemoteSecureConnectorInfoManagerTest (the flaky class): 14/14 pass
• Full connector-server-grizzly suite: 32/32 pass
• Full connector-framework-server suite: 24/24 pass

The race itself is timing-dependent (not reproducible locally on demand), so the real proof will be Windows CI over time.

[maximthomas]

Diagnosis is right and both fixes improve on the status quo. Three things before merge:

1. Missing tryCancelRemote on the new failure path. The sibling error path at BatchApiOpImpl.java:429-434 pairs the two; without it the remote batch runs to completion with nobody listening.

getExceptionHandler().handleException(new ConnectorException(
        "Batch operation failed to return a batch token"));
tryCancelRemote(getConnectionContext(), getRequestId());

2. volatile on the fields the new code depends on. awaitHandshake polls isHandHooked() → getRemoteConnectionContext(), which is a non-volatile field in all three holders (OpenICFWebSocketApplication.java:172, ClientRemoteConnectorInfoManager.java:506, jetty SinglePrincipal). Thread.sleep keeps the loop from being hoisted, but the RemoteOperationContext itself isn't safely published. Same for BatchApiOpImpl.java:293 returnToken — written on a message thread, read by CompletionListener. A stale read there now produces a spurious ConnectorException instead of a hang.

3. Shorter timeout, and exempt error responses. The real race resolves in tens of milliseconds, so 10 s buys nothing but costs a pool thread per message on sockets that will never hand-hook (messageExecutor is an unbounded cached pool — ConnectorFramework.java:83). It also delays legitimate "received request before handshake" feedback from immediate to 10 s. Error responses are handled at OpenICFServerAdapter.java:177, before the isHandHooked() guard, so they never needed the handshake:

private static final long HANDSHAKE_WAIT_MS = 500;

if (!isHandshakeMessage(message) && !message.getResponse().hasError()) {
    awaitHandshake(socket, message.getMessageId());
}

Minor: the give-up branch logs at info; warn matches the neighbouring drop paths at lines 215/221.

On tests — the adapter race isn't reproducible on demand, agreed. But the BatchApiOpImpl half needs no race: start CompletionListener, never deliver a token, assert executeBatch throws within ~5 s and that onError fired while onCompleted did not.

Bigger picture: this synchronizes only against the handshake boundary — two operation responses on the same socket can still be reordered. Serializing dispatch per socket would kill the bug class and make awaitHandshake unnecessary. Follow-up, not a blocker.

[vharseko]

Thanks for the thorough review — all points addressed in 2efeda5:

1. tryCancelRemote on the failure path — added, mirroring the sendResponse sibling including its !getPromise().isDone() guard. It is additionally wrapped in try/catch: in the lost-token scenario the transport may already be down, and trySendBytes would otherwise throw straight out of the CompletionListener thread.

2. volatile — the context fields in all three holders (ClientRemoteConnectorInfoManager, grizzly OpenICFWebSocketApplication, jetty SinglePrincipal) are now volatile. returnToken needed no change — it has been volatile since #106 (b47f778), which is part of this branch's base; the token is also fully populated before being published to the field.

3. Timeout / error responses — wait shortened to HANDSHAKE_WAIT_MS = 500, and error responses are exempted via an explicit isErrorResponse() check (message.hasResponse() && message.getResponse().hasError()) since they are handled before the isHandHooked() guards. The give-up branch now logs at warn.

Test — added BatchApiOpImplTest: a fake RequestDistributor delivers a "complete" BatchOpResult without a token, and the test asserts executeBatch fails with ConnectorException in ~5s, observer.onError fires and onCompleted does not.

Bigger picture — agreed that serializing dispatch per socket would eliminate the whole reordering class and make awaitHandshake unnecessary; I'd track that as a separate follow-up issue.

Test results: connector-framework-server 25/25 (incl. the new test), connector-server-grizzly 32/32, full upstream reactor green.