fix(app, mcp): PersistGate recovery + MCP handler drain on disconnect (#857)

Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
This commit is contained in:
Jwalin Shah
2026-04-29 12:52:15 -07:00
committed by GitHub
co-authored by Steven Enamakel
parent b2cfcd50e3
commit 394bfcd0fa
10 changed files with 410 additions and 10 deletions
+5
View File
@@ -49,6 +49,11 @@ VITE_DEV_FORCE_ONBOARDING=false
# Should match OPENHUMAN_TOOL_TIMEOUT_SECS on the core when set.
# VITE_TOOL_TIMEOUT_SECS=
# [optional] Per-request timeout for Core JSON-RPC `fetch()` calls, in milliseconds.
# Guards the UI against a hung sidecar by rejecting in-flight requests when the
# core stops responding. Bounded [1000, 600000]; default 30000.
# VITE_CORE_RPC_TIMEOUT_MS=30000
# [optional] Minimum desktop app semver to complete OAuth deep links (openhuman://oauth/success). Leave unset in dev.
# VITE_MINIMUM_SUPPORTED_APP_VERSION=0.51.0
# [optional] Download page when OAuth is blocked due to an outdated build (default: GitHub releases/latest).
+2 -2
View File
@@ -13,7 +13,7 @@ import ErrorFallbackScreen from './components/ErrorFallbackScreen';
import LocalAIDownloadSnackbar from './components/LocalAIDownloadSnackbar';
import MeshGradient from './components/MeshGradient';
import OpenhumanLinkModal from './components/OpenhumanLinkModal';
import RouteLoadingScreen from './components/RouteLoadingScreen';
import PersistRehydrationScreen from './components/PersistRehydrationScreen';
import GlobalUpsellBanner from './components/upsell/GlobalUpsellBanner';
import { isWelcomeLocked } from './lib/coreState/store';
import { startNativeNotificationsService } from './lib/nativeNotifications';
@@ -47,7 +47,7 @@ function App() {
tagErrorSource(eventId, 'react', componentStack);
}}>
<Provider store={store}>
<PersistGate loading={<RouteLoadingScreen />} persistor={persistor}>
<PersistGate loading={<PersistRehydrationScreen />} persistor={persistor}>
<CoreStateProvider>
<SocketProvider>
<ChatRuntimeProvider>
@@ -0,0 +1,77 @@
import debug from 'debug';
import { useEffect, useState } from 'react';
import { persistor } from '../store';
import RouteLoadingScreen from './RouteLoadingScreen';
const persistWarn = debug('persist:warn');
/**
* If rehydration has not completed by this cap we surface a recovery CTA.
* Chosen to be long enough that slow disks / antivirus scans don't flap
* users into it, but short enough that a stuck splash screen is noticeable.
*/
const REHYDRATION_WARN_TIMEOUT_MS = 10_000;
/**
* Loading surface used as the `loading` prop for `<PersistGate>`.
*
* PersistGate alone has no deadline: if rehydration stalls (corrupt
* `localStorage`, disk stalls, a storage adapter that never resolves) the
* user sees a permanent splash with no way out. After `REHYDRATION_WARN_TIMEOUT_MS`
* we swap in a recovery panel that lets the user purge persisted state and
* reload. PersistGate still tears down this component the moment rehydration
* finishes, so a slow-but-eventual boot behaves identically to today.
*/
function PersistRehydrationScreen() {
const [timedOut, setTimedOut] = useState(false);
const [resetting, setResetting] = useState(false);
useEffect(() => {
const timer = setTimeout(() => {
persistWarn(
'redux-persist rehydration exceeded %dms — surfacing recovery CTA',
REHYDRATION_WARN_TIMEOUT_MS
);
setTimedOut(true);
}, REHYDRATION_WARN_TIMEOUT_MS);
return () => clearTimeout(timer);
}, []);
if (!timedOut) return <RouteLoadingScreen />;
const handleReset = async () => {
if (resetting) return;
setResetting(true);
try {
await persistor.purge();
} catch (err) {
persistWarn('persistor.purge() failed: %o', err);
}
window.location.reload();
};
return (
<div className="fixed inset-0 flex items-center justify-center bg-canvas-50 p-6">
<div className="max-w-sm w-full space-y-4 rounded-xl border border-stone-200 bg-white p-6 shadow-soft text-center">
<p className="text-sm font-medium text-stone-900">
OpenHuman is taking longer than usual to load.
</p>
<p className="text-xs text-stone-500 leading-relaxed">
Local app state may be corrupted. Resetting will sign you out and clear cached data on
this device; you&apos;ll reconnect your accounts on next launch. Your data on the server
is not affected.
</p>
<button
type="button"
onClick={handleReset}
disabled={resetting}
className="w-full rounded-xl bg-primary-500 px-4 py-2.5 text-sm font-medium text-white transition-colors hover:bg-primary-600 disabled:cursor-not-allowed disabled:opacity-60">
{resetting ? 'Resetting…' : 'Reset local state'}
</button>
</div>
</div>
);
}
export default PersistRehydrationScreen;
+107
View File
@@ -0,0 +1,107 @@
import type { Socket } from 'socket.io-client';
import { beforeEach, describe, expect, test } from 'vitest';
import { SocketIOMCPTransportImpl } from '../transport';
import type { MCPRequest } from '../types';
/**
* Minimal stand-in for a `socket.io-client` Socket — just enough for the
* transport to register/unregister listeners, emit events, and simulate
* connect/disconnect transitions.
*/
class FakeSocket {
connected = true;
private listeners = new Map<string, Set<(...args: unknown[]) => void>>();
emitted: Array<{ event: string; data: unknown }> = [];
on(event: string, handler: (...args: unknown[]) => void) {
let set = this.listeners.get(event);
if (!set) {
set = new Set();
this.listeners.set(event, set);
}
set.add(handler);
}
off(event: string, handler: (...args: unknown[]) => void) {
this.listeners.get(event)?.delete(handler);
}
emit(event: string, data: unknown) {
this.emitted.push({ event, data });
}
/** Fire a socket-side event (simulating the server sending something). */
trigger(event: string, ...args: unknown[]) {
const set = this.listeners.get(event);
if (!set) return;
for (const handler of Array.from(set)) handler(...args);
}
asSocket(): Socket {
return this as unknown as Socket;
}
}
function baseRequest(id: number, method = 'tools/list'): MCPRequest {
return { jsonrpc: '2.0', id, method };
}
describe('SocketIOMCPTransportImpl', () => {
let socket: FakeSocket;
let transport: SocketIOMCPTransportImpl;
beforeEach(() => {
socket = new FakeSocket();
transport = new SocketIOMCPTransportImpl(socket.asSocket());
});
test('delivers a matching response to the in-flight request', async () => {
const pending = transport.request(baseRequest(1));
socket.trigger('mcp:response', { jsonrpc: '2.0', id: 1, result: { ok: true } });
await expect(pending).resolves.toMatchObject({ id: 1, result: { ok: true } });
});
test('rejects pending requests when the socket emits disconnect', async () => {
const pending = transport.request(baseRequest(42, 'tools/call'));
// The promise is pending; simulate a socket drop.
socket.connected = false;
socket.trigger('disconnect', 'transport close');
await expect(pending).rejects.toThrow(/Socket disconnected: transport close/);
});
test('rejects all pending requests, not just the first', async () => {
const p1 = transport.request(baseRequest(1));
const p2 = transport.request(baseRequest(2));
const p3 = transport.request(baseRequest(3));
socket.trigger('disconnect');
await expect(p1).rejects.toThrow(/Socket disconnected/);
await expect(p2).rejects.toThrow(/Socket disconnected/);
await expect(p3).rejects.toThrow(/Socket disconnected/);
});
test('updateSocket rejects pending requests emitted on the old socket', async () => {
const pending = transport.request(baseRequest(7));
const replacement = new FakeSocket();
transport.updateSocket(replacement.asSocket());
await expect(pending).rejects.toThrow(/Socket replaced/);
});
test('after disconnect the handler map is empty and a replayed response is a noop', async () => {
const pending = transport.request(baseRequest(99));
socket.trigger('disconnect');
await expect(pending).rejects.toThrow(/Socket disconnected/);
// A late-arriving response for the same id must not blow up or resolve
// anything — it should just be logged as unhandled.
expect(() =>
socket.trigger('mcp:response', { jsonrpc: '2.0', id: 99, result: { late: true } })
).not.toThrow();
});
});
+31
View File
@@ -41,6 +41,32 @@ export class SocketIOMCPTransportImpl implements SocketIOMCPTransport {
private setupEventHandlers(): void {
if (!this.socket) return;
this.socket.on(`${this.eventPrefix}response`, this.responseHandler);
// If the socket drops while a request is in flight, the response will
// never arrive and the caller would otherwise block until the 30s
// request timeout. Drain pending handlers immediately so callers see
// a clear `Socket disconnected` error and can recover / retry.
this.socket.on('disconnect', this.disconnectHandler);
}
private disconnectHandler = (reason?: string): void => {
this.rejectAllPending(`Socket disconnected${reason ? `: ${reason}` : ''}`);
};
/**
* Fail every in-flight request with a synthetic JSON-RPC error so its
* promise rejects instead of leaking into the 30s request timeout.
* Used on socket `disconnect` and when `updateSocket` replaces the
* underlying transport (since old in-flight requests were emitted on the
* previous socket and can never receive a response on the new one).
*/
private rejectAllPending(reason: string): void {
if (this.requestHandlers.size === 0) return;
const handlers = Array.from(this.requestHandlers.entries());
this.requestHandlers.clear();
mcpWarn('Rejecting pending MCP requests', { count: handlers.length, reason });
for (const [id, handler] of handlers) {
handler({ jsonrpc: '2.0', id, error: { code: -32000, message: reason } });
}
}
emit(event: string, data: unknown): void {
@@ -104,7 +130,12 @@ export class SocketIOMCPTransportImpl implements SocketIOMCPTransport {
updateSocket(socket: Socket | null | undefined): void {
if (this.socket) {
this.socket.off(`${this.eventPrefix}response`, this.responseHandler);
this.socket.off('disconnect', this.disconnectHandler);
}
// Pending handlers were emitted on the old socket; the new socket will
// never deliver their responses, so reject them now rather than letting
// them hang until the per-request timeout fires.
this.rejectAllPending('Socket replaced');
this.socket = socket ?? undefined;
this.setupEventHandlers();
}
@@ -1,6 +1,7 @@
import { beforeEach, describe, expect, test, vi } from 'vitest';
import { dispatchLocalAiMethod } from '../../lib/ai/localCoreAiMemory';
import { CORE_RPC_TIMEOUT_MS } from '../../utils/config';
import type { AccessibilityStatus, CommandResponse } from '../../utils/tauriCommands';
import { callCoreRpc } from '../coreRpcClient';
@@ -292,6 +293,58 @@ describe('coreRpcClient', () => {
expect(body.method).toBe('openhuman.auth_sub_segment');
});
test('rejects with a timeout error when fetch does not resolve within CORE_RPC_TIMEOUT_MS', async () => {
vi.useFakeTimers();
try {
const fetchMock = vi.mocked(fetch);
// Simulate a hung core: the fetch never resolves, but we honor the
// AbortSignal so the client's timeout can tear us down.
fetchMock.mockImplementationOnce(
(_url, init) =>
new Promise<Response>((_resolve, reject) => {
const signal = (init as RequestInit).signal as AbortSignal | undefined;
if (!signal) return;
const onAbort = () => {
const err = new Error('The operation was aborted');
err.name = 'AbortError';
reject(err);
};
if (signal.aborted) onAbort();
else signal.addEventListener('abort', onAbort, { once: true });
})
);
const pending = callCoreRpc({ method: 'openhuman.threads_list' });
// Swallow the unhandled rejection that would otherwise be raised when
// advancing timers triggers the abort before the `await expect` below.
pending.catch(() => {});
await vi.advanceTimersByTimeAsync(CORE_RPC_TIMEOUT_MS + 1);
await expect(pending).rejects.toThrow(
`Core RPC openhuman.threads_list timed out after ${CORE_RPC_TIMEOUT_MS}ms`
);
} finally {
vi.useRealTimers();
}
});
test('does not trigger the timeout path when fetch resolves promptly', async () => {
const fetchMock = vi.mocked(fetch);
fetchMock.mockResolvedValueOnce({
ok: true,
json: async () => ({ jsonrpc: '2.0', id: 1, result: { ok: true } }),
} as Response);
const result = await callCoreRpc<{ ok: boolean }>({ method: 'openhuman.threads_list' });
expect(result).toEqual({ ok: true });
// Signal on the request init must be populated so the timeout path
// can tear down a real hung call.
const init = fetchMock.mock.calls[0][1] as RequestInit;
expect(init.signal).toBeInstanceOf(AbortSignal);
});
test('sends content-type json header and POST method', async () => {
const fetchMock = vi.mocked(fetch);
fetchMock.mockResolvedValueOnce({
+40 -8
View File
@@ -2,7 +2,7 @@ import { isTauri as coreIsTauri, invoke } from '@tauri-apps/api/core';
import debug from 'debug';
import { dispatchLocalAiMethod } from '../lib/ai/localCoreAiMemory';
import { CORE_RPC_URL } from '../utils/config';
import { CORE_RPC_TIMEOUT_MS, CORE_RPC_URL } from '../utils/config';
import { getStoredRpcUrl } from '../utils/configPersistence';
import { sanitizeError } from '../utils/sanitize';
@@ -129,12 +129,27 @@ export async function getCoreRpcUrl(): Promise<string> {
const url = await invoke<string>('core_rpc_url');
const trimmed = String(url || '').trim();
if (!trimmed) {
// The Tauri command succeeded but returned an empty string. That's
// almost certainly a shell misconfiguration — prefer the build-time
// default but make the fallback visible rather than silent.
coreRpcError('core_rpc_url returned empty; using build-time default', {
fallback: CORE_RPC_URL,
});
}
resolvedCoreRpcUrl = trimmed || CORE_RPC_URL;
return resolvedCoreRpcUrl || CORE_RPC_URL;
} catch {
// Fallback to stored or default on error
} catch (err) {
// Fallback to a stored override first, then the build-time default.
// Keep the underlying invoke failure visible so port mismatches and
// shell misconfiguration are diagnosable in dev logs.
const storedUrl = getStoredRpcUrl();
resolvedCoreRpcUrl = storedUrl || CORE_RPC_URL;
coreRpcError('core_rpc_url invoke failed; using fallback RPC URL', {
fallback: resolvedCoreRpcUrl,
usedStoredUrl: Boolean(storedUrl),
error: sanitizeError(err),
});
return resolvedCoreRpcUrl;
} finally {
resolvingCoreRpcUrl = null;
@@ -177,11 +192,28 @@ export async function callCoreRpc<T>({
const rpcUrl = await getCoreRpcUrl();
coreRpcLog('HTTP request', { id: payload.id, method: payload.method });
const response = await fetch(rpcUrl, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
// Bound the fetch to CORE_RPC_TIMEOUT_MS. Without this a hung core
// sidecar will block every caller (and the UI) forever. We use a
// manual AbortController + setTimeout rather than AbortSignal.timeout()
// so test fake timers can drive the abort deterministically.
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), CORE_RPC_TIMEOUT_MS);
let response: Response;
try {
response = await fetch(rpcUrl, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
signal: controller.signal,
});
} catch (fetchErr) {
if (controller.signal.aborted) {
throw new Error(`Core RPC ${payload.method} timed out after ${CORE_RPC_TIMEOUT_MS}ms`);
}
throw fetchErr;
} finally {
clearTimeout(timeoutId);
}
if (!response.ok) {
const text = await response.text();
+1
View File
@@ -72,6 +72,7 @@ vi.mock('../utils/tauriCommands', () => ({
// Mock the config module
vi.mock('../utils/config', () => ({
CORE_RPC_URL: 'http://127.0.0.1:7788/rpc',
CORE_RPC_TIMEOUT_MS: 30_000,
IS_DEV: true,
IS_PROD: false,
DEV_FORCE_ONBOARDING: false,
+22
View File
@@ -26,6 +26,28 @@ function parseToolTimeoutSecs(): number {
export const TOOL_TIMEOUT_SECS = parseToolTimeoutSecs();
/**
* Per-request timeout for Core JSON-RPC `fetch()` calls, in milliseconds.
* Without this the UI can hang indefinitely if the core sidecar stops
* responding mid-flight. Bounded to [1s, 10min]; default 30s. Override with
* `VITE_CORE_RPC_TIMEOUT_MS`.
*/
const DEFAULT_CORE_RPC_TIMEOUT_MS = 30_000;
const MIN_CORE_RPC_TIMEOUT_MS = 1_000;
const MAX_CORE_RPC_TIMEOUT_MS = 10 * 60 * 1_000;
function parseCoreRpcTimeoutMs(): number {
const raw = import.meta.env.VITE_CORE_RPC_TIMEOUT_MS as string | undefined;
if (raw === undefined || raw === '') return DEFAULT_CORE_RPC_TIMEOUT_MS;
const n = Number(raw);
if (!Number.isFinite(n) || n < MIN_CORE_RPC_TIMEOUT_MS || n > MAX_CORE_RPC_TIMEOUT_MS) {
return DEFAULT_CORE_RPC_TIMEOUT_MS;
}
return Math.round(n);
}
export const CORE_RPC_TIMEOUT_MS = parseCoreRpcTimeoutMs();
export const IS_DEV = import.meta.env.DEV;
export const IS_PROD = import.meta.env.PROD;
@@ -0,0 +1,72 @@
# Adversarial Review — Fix Plan (2026-04-23)
Working branch: `claude/review-openhuman-issues-XG0SR`.
Upstream: `tinyhumansai/openhuman` (fork `jwalin-shah/openhuman` has no open PRs/issues at time of review).
This plan is the concrete, verified follow-up to the adversarial review run on 2026-04-23. It only lists findings that were spot-checked against the actual code. Findings that the initial sweep surfaced but that turned out to be already fixed or too theoretical to action are explicitly excluded below.
## Verification vs. initial sweep
| Item | Status after verification |
| --- | --- |
| RPC `fetch()` has no timeout (`coreRpcClient.ts:154`) | Confirmed |
| Core spawn does not check `try_wait()` before polling (`core_process.rs:161164`) | **Rejected after re-read** — the poll loop at lines 209254 runs `is_rpc_port_open()` then `try_wait()` *before* the 100 ms sleep, so an instant-exit child is caught within ~250 ms. Not worth changing. |
| `Arc<Mutex>` on child handle held across `.await` in spawn (`core_process.rs:168205`) | **Rejected after re-read**`tokio::process::Command::spawn()` is synchronous; no `.await` happens between lock acquire and release in either the spawn block or the poll iteration. Not a real issue. |
| `core_rpc_url` silent fallback (`coreRpcClient.ts:105112`) | Confirmed |
| CSP wildcard port for localhost in `tauri.conf.json:26` | Confirmed — directly enables the upstream **#812** class of bug |
| MCP transport leaks request handlers on disconnect (`lib/mcp/transport.ts`) | Confirmed (to re-verify when patch is written) |
| Event-bus subscriber panic poisons the loop (`core/event_bus/bus.rs:147`) | **Already fixed** — code at lines 163180 wraps the handler call in `AssertUnwindSafe(...).catch_unwind()` and logs+continues on panic. Removed from queue. |
| Request ID collision after `Number.MAX_SAFE_INTEGER` calls | Excluded — 2^53 RPC calls per process lifetime is not a realistic failure mode |
| `.unwrap()` in `src/rpc/dispatch.rs:61` | Excluded — it is test code |
| Centralized cancellation for all 97 `tokio::spawn` sites | Deferred — too big for this branch; tracked as follow-up |
| Stale `.claude/rules/*` (upstream **#805**) | Confirmed — `08-frontend-guide.md`, `11-tech-stack-detailed.md`, `16-macos-background-execution.md` contradict CLAUDE.md (e.g. Zustand vs. Redux Toolkit; Telegram MTProto/tray/ticker features that don't exist in code). |
## Queue
Items are ordered by: blast radius × concreteness × low risk of regression. I aim to land them in a single branch, one commit per fix, so each is easy to bisect or revert.
### Security / hardening
1. **[~] Tighten `tauri.conf.json` CSP** — deferred. Pinning the core port in static CSP is incompatible with `OPENHUMAN_CORE_PORT` runtime override, and #812 is a capability-level issue (command reachability from 3rd-party origins) that CSP alone wouldn't fix. Needs upstream design.
### Reliability / IPC
2. **[x] RPC fetch timeout** — `callCoreRpc()` uses `AbortController` with a bounded timeout (default 30 s; configurable via `VITE_CORE_RPC_TIMEOUT_MS`) and surfaces a distinct error message. Landed: `app/src/services/coreRpcClient.ts`, `app/src/utils/config.ts`, tests at `app/src/services/__tests__/coreRpcClient.test.ts`, env example updated.
3. ~~Core spawn: immediate `try_wait()` after `spawn()`~~ — rejected on re-read (see table above).
4. ~~Core spawn: drop child lock before awaits~~ — rejected on re-read (see table above).
5. **[x] MCP transport: drain pending handlers on disconnect** — rejects all in-flight `requestHandlers` with `Socket disconnected` (via `disconnect` listener) or `Socket replaced` (via `updateSocket`). Landed: `app/src/lib/mcp/transport.ts` + new test suite `app/src/lib/mcp/__tests__/transport.test.ts`.
6. **[x] `core_rpc_url` resolution: stop swallowing the failure** — both the throw path and the empty-string path now log the underlying misconfig via `coreRpcError`. Landed: `app/src/services/coreRpcClient.ts`.
### UX / observability
7. **[x] PersistGate timeout** — new `PersistRehydrationScreen` preserves the normal splash for the first 10 s, then swaps in a recovery panel that calls `persistor.purge()` + `window.location.reload()`. Landed: `app/src/App.tsx`, `app/src/components/PersistRehydrationScreen.tsx`.
8. **[ ] RPC errors carry a structured code** — deferred to a follow-up. `coreRpcClient.ts` still throws `Error(message)`; richer error codes would churn every call site. Tracked as next-branch work.
### Repo hygiene
9. **[x] Stale `.claude/rules/` pass (upstream #805)** — deleted 7 files that actively contradicted CLAUDE.md (they auto-load into every Claude Code session, so their misinformation is load-bearing context, not just docs):
- `00-project-vision.md`, `01-project-overview.md` — "crypto community platform, poke.com" framing, mobile targets.
- `05-platform-setup-android.md`, `06-platform-setup-ios.md` — non-desktop targets are rejected by `compile_error!`.
- `08-frontend-guide.md` — fabricated features (Telegram MTProto, crypto price ticker, chat with crypto addresses).
- `11-tech-stack-detailed.md` — claimed Zustand for state mgmt (repo uses Redux Toolkit); wrong directory paths.
- `16-macos-background-execution.md` — "Outsourced" product name, tray config that isn't in `tauri.conf.json`.
Rewrote `02-development-commands.md` to match CLAUDE.md (desktop-only, workspace paths, real script names). Trimmed Android/iOS sections out of `10-troubleshooting.md`, fixed `src-tauri/` paths.
### Explicitly out of scope for this branch
- Cancellation scaffolding for every `tokio::spawn`. Needs a design pass first; separate branch/issue.
- AI config signature verification for remote refresh. Needs product decision on whether remote refresh stays.
- Relay ACL / allowlist on `core_rpc_relay`. Needs explicit scope decision — item (1) closes the main externally reachable gap.
- Deep-link `nonce`/`state` hardening (upstream **#829**). Larger auth-flow change; track upstream.
- Release pipeline reliability (upstream **#828**, **#826**, **#823**, **#843**, **#840**, **#785**). Owned upstream.
## Validation
Before each commit:
- `yarn typecheck` and relevant `yarn test:unit` subset in `app/`.
- `cargo check --manifest-path app/src-tauri/Cargo.toml` for Rust shell changes.
- `cargo check --manifest-path Cargo.toml` for Rust core changes.
- `yarn lint` / `yarn format:check` at the end of the batch.
Final: `yarn typecheck && yarn lint && yarn test:unit && cargo check` across both manifests before pushing.