* fix(ws): align RPC timeout to 120s + rejectAllPending on disconnect (fixes #526) Mobile's pending-request handling diverged from the desktop contract (apps/shared JsonRpcGatewayClient) in two ways that caused silent hangs: 1. Timeout was 30s while the desktop uses 120s (DEFAULT_REQUEST_TIMEOUT_MS), so legitimately long agent turns were pruned early. Align to 120s and use it for both the cleanup sweep and sendRpcAndAwait's per-call withTimeout. 2. The cleanup sweep only Log.w'd stale requests and dropped them — it never failed the CompletableDeferred, so sendRpcAndAwait callers awaiting a server that never answered HUNG FOREVER. And on disconnect/reconnect there was NO rejectAllPending, so in-flight awaited RPCs also hung until their own timeout. Changes (all in ChatViewModel.kt): - cleanup sweep now completeExceptionally()s the deferred AND surfaces a 'Request timed out' error for fire-and-forget RPCs (was silently dropped) - new rejectAllPending() fails + clears every in-flight pending deferred; wired into the disconnect-status collector and reconnect() - PENDING_REQUEST_TIMEOUT_MS = 120_000 (matches desktop); sendRpcAndAwait uses it instead of a hardcoded 30s Tests: 2 new ChatViewModelTest cases cover rejectAllPending on disconnect and on reconnect (no hang, VM stays usable). (Timeout-surfacing path shares the deferred-fail logic with handleRpcError; the 120s sweep can't run under the unit-test dispatcher's periodic-timer limitation, noted in PR.) Mirrors the desktop-app audit in /opt/hermes-agent. * fix(ws): aggregate pending-request cleanup into a single UI update (PR #541) Sourcery review (PR #541): startPendingRequestCleanup emitted one _uiState.update{} per stale request inside the loop, causing N separate emissions / redundant recompositions when many RPCs time out at once. Collect fire-and-forget methods and emit a single combined errorMessage update after the loop. rejectAllPending is intentionally left silent on disconnect/reconnect: those are transient socket events (the RPC often completes on the new socket), so surfacing a scary error banner is UX noise. Only the timeout path surfaces errors — that is a genuine failure. This matches the desktop JsonRpcGatewayClient.rejectAllPending semantics. * refactor(ws): move awaited-RPC pending/timeout layer into HermesWsClient (closes #526) Issue #526: port the JsonRpcGatewayClient.request() pattern into HermesWsClient. HermesWsClient now owns the in-flight map, per-call 120s timeout, resolve-on-RpcResult/RpcError, and rejectAllPending on disconnect/reconnect. ChatViewModel.sendRpcAndAwait simply delegates to wsClient.request(...).await(); its local pendingRequests map, PendingRequest, startPendingRequestCleanup loop and rejectAllPending are removed. Fire-and-forget send() is unchanged. --------- Co-authored-by: M57 <m57@hermes-control.app>
This commit is contained in:
M57andM57 authored and GitHub committed
1 parent
c5d1ef1c42
commit
def32f8f27
3 files changed
+199
-84
No files matched your search
@@ -20,7 +20,6 @@ import com.m57.hermescontrol.data.ws.HermesWsClient
|
||||
import com.m57.hermescontrol.data.ws.WsEvent
|
||||
import com.m57.hermescontrol.data.ws.WsMethods
|
||||
import com.m57.hermescontrol.data.ws.toJsonElement
|
||||
import kotlinx.coroutines.CompletableDeferred
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.Job
|
||||
import kotlinx.coroutines.delay
|
||||
@@ -33,7 +32,6 @@ import kotlinx.coroutines.flow.stateIn
|
||||
import kotlinx.coroutines.flow.update
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.withContext
|
||||
import kotlinx.coroutines.withTimeout
|
||||
import kotlinx.coroutines.yield
|
||||
import kotlinx.serialization.json.decodeFromJsonElement
|
||||
import okhttp3.MediaType.Companion.toMediaType
|
||||
@@ -45,17 +43,6 @@ import java.util.concurrent.ConcurrentHashMap
|
||||
|
||||
private const val TAG = "ChatViewModel"
|
||||
|
||||
/**
|
||||
* Pending request timeout — if the server doesn't respond within this
|
||||
* window, the request is pruned and an error is surfaced.
|
||||
*/
|
||||
private const val PENDING_REQUEST_TIMEOUT_MS = 30_000L
|
||||
|
||||
/** Thrown when an awaited RPC returns an error response. */
|
||||
private class RpcCallException(
|
||||
message: String,
|
||||
) : Exception(message)
|
||||
|
||||
data class ChatUiState(
|
||||
val messages: List<ChatMessage> = emptyList(),
|
||||
val currentSessionId: String? = null,
|
||||
@@ -132,7 +119,6 @@ class ChatViewModel(
|
||||
val streamingState: StateFlow<StreamingState> = _streamingState.asStateFlow()
|
||||
|
||||
private val wsClient = HermesWsClient
|
||||
private val pendingRequests = ConcurrentHashMap<String, PendingRequest>()
|
||||
|
||||
private val slashDispatcher = SlashCommandDispatcher()
|
||||
private val searchDelegate =
|
||||
@@ -151,8 +137,6 @@ class ChatViewModel(
|
||||
isTestEnvironment = { isTestEnvironment() },
|
||||
)
|
||||
|
||||
private var pendingCleanupJob: Job? = null
|
||||
|
||||
// ── Public state ─────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
@@ -199,14 +183,12 @@ class ChatViewModel(
|
||||
status == ConnectionStatus.AUTH_EXPIRED
|
||||
) {
|
||||
_uiState.update { it.copy(isLoading = false) }
|
||||
// Fail any in-flight awaited RPCs so callers don't hang
|
||||
// across the disconnect (delegated to HermesWsClient, issue #526).
|
||||
wsClient.rejectAllPending()
|
||||
}
|
||||
}
|
||||
}
|
||||
if (startCleanup) {
|
||||
startPendingRequestCleanup()
|
||||
}
|
||||
|
||||
// B7 (Jun 30 2026, kanban t_connection_loading): if already connected on launch, trigger setup immediately
|
||||
if (wsClient.connectionStatus.value == ConnectionStatus.CONNECTED) {
|
||||
handleGatewayReady()
|
||||
}
|
||||
@@ -404,12 +386,7 @@ class ChatViewModel(
|
||||
id: String,
|
||||
result: Any?,
|
||||
) {
|
||||
val pending = pendingRequests.remove(id) ?: return
|
||||
val method = pending.method
|
||||
|
||||
// Complete the deferred so sendRpcAndAwait() callers can resume
|
||||
pending.deferred?.complete(result)
|
||||
|
||||
val method = idToMethod.remove(id) ?: return
|
||||
when (method) {
|
||||
WsMethods.SESSION_CREATE -> {
|
||||
val resultMap = result as? Map<String, Any?> ?: return
|
||||
@@ -541,25 +518,21 @@ class ChatViewModel(
|
||||
id: String,
|
||||
error: Any?,
|
||||
) {
|
||||
val pending = pendingRequests.remove(id)
|
||||
val method = pending?.method
|
||||
val method = idToMethod.remove(id) ?: return
|
||||
val errorMsg =
|
||||
when (error) {
|
||||
is Map<*, *> -> error["message"] as? String ?: error.toString()
|
||||
else -> error.toString()
|
||||
}
|
||||
|
||||
// Fail the deferred so sendRpcAndAwait() callers see the error
|
||||
pending?.deferred?.completeExceptionally(RpcCallException(errorMsg))
|
||||
|
||||
// Only surface error in UI for non-awaited RPCs
|
||||
if (pending?.deferred == null) {
|
||||
_uiState.update {
|
||||
it.copy(
|
||||
isLoading = false,
|
||||
errorMessage = "Error${if (method != null) " ($method)" else ""}: $errorMsg",
|
||||
)
|
||||
}
|
||||
// Surface error in UI (these are server-pushed RpcError for
|
||||
// fire-and-forget RPCs — awaited RPCs handle their own failure
|
||||
// via the HermesWsClient.request() deferred).
|
||||
_uiState.update {
|
||||
it.copy(
|
||||
isLoading = false,
|
||||
errorMessage = "Error${if (method != null) " ($method)" else ""}: $errorMsg",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -700,22 +673,16 @@ class ChatViewModel(
|
||||
}
|
||||
|
||||
/**
|
||||
* Send a JSON-RPC call and suspend until the response arrives.
|
||||
* Throws [RpcCallException] on error, [kotlinx.coroutines.TimeoutCancellationException] on timeout.
|
||||
* Send a JSON-RPC call and suspend until the response arrives, delegating
|
||||
* the deferred + 120s timeout to [HermesWsClient.request] (issue #526).
|
||||
* Throws [HermesWsClient.HermesRpcException] on RPC error, or
|
||||
* [kotlinx.coroutines.TimeoutCancellationException] if the server never
|
||||
* answers within the timeout.
|
||||
*/
|
||||
private suspend fun sendRpcAndAwait(
|
||||
method: String,
|
||||
params: Map<String, Any>,
|
||||
timeoutMs: Long = 30_000,
|
||||
): Any? {
|
||||
val deferred = CompletableDeferred<Any?>()
|
||||
wsClient.send(
|
||||
method = method,
|
||||
params = params,
|
||||
onSent = { id -> trackRequest(id, method, deferred) },
|
||||
)
|
||||
return withTimeout(timeoutMs) { deferred.await() }
|
||||
}
|
||||
): Any? = HermesWsClient.request(method, params).await()
|
||||
|
||||
// ── Attachment management ─────────────────────────────────────────────
|
||||
|
||||
@@ -1215,6 +1182,7 @@ class ChatViewModel(
|
||||
)
|
||||
}
|
||||
viewModelScope.launch(Dispatchers.IO) {
|
||||
wsClient.rejectAllPending()
|
||||
wsClient.disconnect()
|
||||
}
|
||||
viewModelScope.launch {
|
||||
@@ -1341,40 +1309,14 @@ class ChatViewModel(
|
||||
|
||||
// ── Pending request tracking ─────────────────────────────────────────
|
||||
|
||||
private data class PendingRequest(
|
||||
val method: String,
|
||||
val createdAt: Long = System.currentTimeMillis(),
|
||||
val deferred: CompletableDeferred<Any?>? = null,
|
||||
)
|
||||
/** Maps an in-flight RPC id → its method, purely for UI error labeling. The deferred/timeout lives in [HermesWsClient.request] (issue #526). */
|
||||
private val idToMethod = ConcurrentHashMap<String, String>()
|
||||
|
||||
private fun trackRequest(
|
||||
id: String,
|
||||
method: String,
|
||||
deferred: CompletableDeferred<Any?>? = null,
|
||||
) {
|
||||
pendingRequests[id] = PendingRequest(method, deferred = deferred)
|
||||
}
|
||||
|
||||
/**
|
||||
* Periodically prunes stale pending requests that the server never
|
||||
* responded to. Surfaces a timeout error for the user.
|
||||
*/
|
||||
private fun startPendingRequestCleanup() {
|
||||
pendingCleanupJob =
|
||||
viewModelScope.launch {
|
||||
while (true) {
|
||||
delay(PENDING_REQUEST_TIMEOUT_MS)
|
||||
val now = System.currentTimeMillis()
|
||||
val stale =
|
||||
pendingRequests.entries.filter {
|
||||
now - it.value.createdAt > PENDING_REQUEST_TIMEOUT_MS
|
||||
}
|
||||
for (entry in stale) {
|
||||
pendingRequests.remove(entry.key)
|
||||
Log.w(TAG, "Request timed out: ${entry.value.method} (id=${entry.key})")
|
||||
}
|
||||
}
|
||||
}
|
||||
idToMethod[id] = method
|
||||
}
|
||||
|
||||
// ── Search ────────────────────────────────────────────────────────────
|
||||
@@ -1410,7 +1352,6 @@ class ChatViewModel(
|
||||
|
||||
override fun onCleared() {
|
||||
super.onCleared()
|
||||
pendingCleanupJob?.cancel()
|
||||
// PERF-16: Don't disconnect the global HermesWsClient singleton when
|
||||
// leaving the Chat screen — it's used by background notification reply.
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user