mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(dashboard): harden PTY reconnect race, wedged-connect recovery, IME guard
Follow-up hardening on the salvaged NS-591 mobile-chat reconnect fix, from
review findings:
- Guard the page-resume reconnect against the async socket-open window:
a connectInFlightRef is set synchronously before the ticket-URL await so
a visibilitychange/focus fired during that gap (wsRef still null) can't
spawn a redundant second socket. Threaded through
shouldReconnectPtyOnPageResume as connectInFlight.
- Recover a socket wedged in WS_CONNECTING (half-open mobile socket after a
radio handoff — the NS-591 scenario) via a PTY_CONNECTING_TIMEOUT_MS
force-close so onclose routes into scheduleReconnect. Cleared on
open/close/effect-cleanup.
- Avoid collapsing legitimate single-letter reduplication ("a a") in the
mobile duplicate-final-word heuristic (>=2-char guard).
- Extract the 350ms replacement window and 1000ms resume throttle to named
exported consts; drop the dead WS_CONNECTING term from the resume
predicate's final expression.
Adds tests for the in-flight guard and the single-letter reduplication case.
This commit is contained in:
parent
3e88cae243
commit
6f42bf344c
5 changed files with 111 additions and 5 deletions
|
|
@ -59,6 +59,15 @@ describe("normalizePtyMobileInput", () => {
|
|||
expect(result.nextLine).toBe("");
|
||||
expect(result.data).toBe("\r");
|
||||
});
|
||||
|
||||
it("does not collapse legitimate single-letter reduplication", () => {
|
||||
// "a a" is a plausible thing to type; the >=2-char guard keeps the
|
||||
// duplicate-final-word collapse from eating it inside the window.
|
||||
const result = normalizePtyMobileInput("a a", "a", true);
|
||||
|
||||
expect(result.normalized).toBe(false);
|
||||
expect(result.data).toBe("a a");
|
||||
});
|
||||
});
|
||||
|
||||
describe("updatePtyInputLine", () => {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,10 @@
|
|||
const DELETE = "\x7f";
|
||||
|
||||
// How long (ms) after a mobile IME / replacement event we treat subsequent
|
||||
// terminal input as a candidate line-replacement rather than a plain append.
|
||||
// Exported so the ChatPage integration and tests share one tunable value.
|
||||
export const MOBILE_REPLACEMENT_WINDOW_MS = 350;
|
||||
|
||||
function chars(text: string): string[] {
|
||||
return Array.from(text);
|
||||
}
|
||||
|
|
@ -24,6 +29,11 @@ function collapseDuplicatedFinalWord(text: string, previousLine: string): string
|
|||
|
||||
const [, prefix, first, , second, trailing] = match;
|
||||
if (first.toLocaleLowerCase() !== second.toLocaleLowerCase()) return text;
|
||||
// Only collapse a duplication the tracked line already ended with — i.e.
|
||||
// Gboard re-emitted the final word. Requiring a >=2-char word avoids
|
||||
// eating legitimate single-letter reduplication ("a a", "i i") that a
|
||||
// user may genuinely type inside the replacement window.
|
||||
if (first.length < 2) return text;
|
||||
if (!previousLine.trimEnd().toLocaleLowerCase().endsWith(first.toLocaleLowerCase())) {
|
||||
return text;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -79,6 +79,37 @@ describe("shouldReconnectPtyOnPageResume", () => {
|
|||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("does not fire a redundant reconnect while a connect is in flight (wsRef not yet assigned)", () => {
|
||||
// The async socket-open IIFE has begun but not yet assigned wsRef, so
|
||||
// socketReadyState reads null. Without the connectInFlight guard this
|
||||
// would return true and double-connect.
|
||||
expect(
|
||||
shouldReconnectPtyOnPageResume({
|
||||
isActive: true,
|
||||
visibilityState: "visible",
|
||||
online: true,
|
||||
socketReadyState: null,
|
||||
ptyState: "connecting",
|
||||
connectInFlight: true,
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("still reconnects an in-flight connect when the page already believes it is closed", () => {
|
||||
// A stuck attempt the user is actively trying to recover (manual reconnect
|
||||
// or a closed state) must not be suppressed by the in-flight guard.
|
||||
expect(
|
||||
shouldReconnectPtyOnPageResume({
|
||||
isActive: true,
|
||||
visibilityState: "visible",
|
||||
online: true,
|
||||
socketReadyState: null,
|
||||
ptyState: "closed",
|
||||
connectInFlight: true,
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("shouldBlockPtyInput", () => {
|
||||
|
|
|
|||
|
|
@ -8,12 +8,23 @@ export type PtyConnectionState =
|
|||
export const PTY_RECONNECT_INPUT_MESSAGE =
|
||||
"Chat is reconnecting. Input will resume when connected.";
|
||||
|
||||
// Minimum gap (ms) between page-resume-triggered reconnect attempts, so a
|
||||
// burst of visibilitychange/pageshow/focus/online events on tab-return
|
||||
// collapses into a single reconnect.
|
||||
export const PTY_RESUME_RECONNECT_THROTTLE_MS = 1000;
|
||||
|
||||
// If a socket sits in WS_CONNECTING past this budget it is treated as wedged
|
||||
// (e.g. a half-open mobile socket after a radio handoff — the NS-591 case)
|
||||
// and force-closed so `onclose` → scheduleReconnect can recover it.
|
||||
export const PTY_CONNECTING_TIMEOUT_MS = 8000;
|
||||
|
||||
export interface PtyResumeReconnectInput {
|
||||
isActive: boolean;
|
||||
visibilityState?: DocumentVisibilityState;
|
||||
online: boolean;
|
||||
socketReadyState?: number | null;
|
||||
ptyState: PtyConnectionState;
|
||||
connectInFlight?: boolean;
|
||||
}
|
||||
|
||||
const WS_CONNECTING = 0;
|
||||
|
|
@ -27,6 +38,7 @@ export function shouldReconnectPtyOnPageResume({
|
|||
online,
|
||||
socketReadyState,
|
||||
ptyState,
|
||||
connectInFlight,
|
||||
}: PtyResumeReconnectInput): boolean {
|
||||
if (!isActive || !online || visibilityState === "hidden") {
|
||||
return false;
|
||||
|
|
@ -37,8 +49,13 @@ export function shouldReconnectPtyOnPageResume({
|
|||
if (socketReadyState === WS_OPEN) {
|
||||
return false;
|
||||
}
|
||||
// A connect is mid-flight (the async socket-open IIFE is awaiting its
|
||||
// ticket URL and hasn't assigned wsRef yet, or the socket is still
|
||||
// CONNECTING on a non-stuck attempt). Don't fire a redundant reconnect
|
||||
// into that window unless the tab already believes it is reconnecting or
|
||||
// closed and needs a fresh attempt.
|
||||
if (
|
||||
socketReadyState === WS_CONNECTING &&
|
||||
(connectInFlight || socketReadyState === WS_CONNECTING) &&
|
||||
ptyState !== "reconnecting" &&
|
||||
ptyState !== "closed"
|
||||
) {
|
||||
|
|
@ -47,7 +64,6 @@ export function shouldReconnectPtyOnPageResume({
|
|||
return (
|
||||
socketReadyState === null ||
|
||||
socketReadyState === undefined ||
|
||||
socketReadyState === WS_CONNECTING ||
|
||||
socketReadyState === WS_CLOSING ||
|
||||
socketReadyState === WS_CLOSED ||
|
||||
ptyState === "reconnecting" ||
|
||||
|
|
|
|||
|
|
@ -37,12 +37,15 @@ import { useI18n } from "@/i18n";
|
|||
import { api } from "@/lib/api";
|
||||
import { normalizeSessionTitle } from "@/lib/chat-title";
|
||||
import {
|
||||
PTY_CONNECTING_TIMEOUT_MS,
|
||||
PTY_RECONNECT_INPUT_MESSAGE,
|
||||
PTY_RESUME_RECONNECT_THROTTLE_MS,
|
||||
type PtyConnectionState,
|
||||
shouldBlockPtyInput,
|
||||
shouldReconnectPtyOnPageResume,
|
||||
} from "@/lib/pty-reconnect";
|
||||
import {
|
||||
MOBILE_REPLACEMENT_WINDOW_MS,
|
||||
normalizePtyMobileInput,
|
||||
shouldTreatInputAsMobileReplacement,
|
||||
} from "@/lib/pty-mobile-input";
|
||||
|
|
@ -172,6 +175,11 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
const forceFreshPtyRef = useRef(false);
|
||||
const blockedInputNoticeRef = useRef(false);
|
||||
const lastResumeReconnectAtRef = useRef(0);
|
||||
// True from the moment the connect effect begins until the socket resolves
|
||||
// (open or close). Guards the page-resume reconnect against firing during
|
||||
// the async ticket/URL await gap where wsRef.current is not yet assigned.
|
||||
const connectInFlightRef = useRef(false);
|
||||
const connectingTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||
const ptyInputLineRef = useRef("");
|
||||
const mobileReplacementInputUntilRef = useRef(0);
|
||||
const [ptyState, setPtyState] =
|
||||
|
|
@ -618,11 +626,11 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
isMobileLike,
|
||||
)
|
||||
) {
|
||||
mobileReplacementInputUntilRef.current = Date.now() + 350;
|
||||
mobileReplacementInputUntilRef.current = Date.now() + MOBILE_REPLACEMENT_WINDOW_MS;
|
||||
}
|
||||
};
|
||||
const markCompositionEnd = () => {
|
||||
mobileReplacementInputUntilRef.current = Date.now() + 350;
|
||||
mobileReplacementInputUntilRef.current = Date.now() + MOBILE_REPLACEMENT_WINDOW_MS;
|
||||
};
|
||||
|
||||
textarea.addEventListener("beforeinput", markReplacementInput, true);
|
||||
|
|
@ -761,6 +769,16 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
let onResizeDisposable: { dispose(): void } | null = null;
|
||||
const forceFresh = forceFreshPtyRef.current;
|
||||
forceFreshPtyRef.current = false;
|
||||
// A connect attempt is now in flight — set synchronously (before the async
|
||||
// socket-open IIFE below awaits its ticket URL) so a page-resume event in
|
||||
// that gap doesn't fire a redundant reconnect (wsRef isn't assigned yet).
|
||||
connectInFlightRef.current = true;
|
||||
const clearConnectingTimer = () => {
|
||||
if (connectingTimerRef.current) {
|
||||
clearTimeout(connectingTimerRef.current);
|
||||
connectingTimerRef.current = null;
|
||||
}
|
||||
};
|
||||
const scheduleReconnect = (code: number) => {
|
||||
if (reconnectTimerRef.current) {
|
||||
return;
|
||||
|
|
@ -793,9 +811,26 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
const ws = new WebSocket(url);
|
||||
ws.binaryType = "arraybuffer";
|
||||
wsRef.current = ws;
|
||||
// W2 (NS-591): a mobile socket can wedge in CONNECTING after a radio
|
||||
// handoff and never fire onclose, so neither the resume predicate nor
|
||||
// scheduleReconnect can recover it. Force-close if it hasn't opened
|
||||
// within the budget; the resulting onclose routes into scheduleReconnect.
|
||||
clearConnectingTimer();
|
||||
connectingTimerRef.current = setTimeout(() => {
|
||||
connectingTimerRef.current = null;
|
||||
if (wsRef.current === ws && ws.readyState === WebSocket.CONNECTING) {
|
||||
try {
|
||||
ws.close();
|
||||
} catch {
|
||||
/* already tearing down */
|
||||
}
|
||||
}
|
||||
}, PTY_CONNECTING_TIMEOUT_MS);
|
||||
|
||||
ws.onopen = () => {
|
||||
clearReconnectTimer();
|
||||
clearConnectingTimer();
|
||||
connectInFlightRef.current = false;
|
||||
reconnectAttemptRef.current = 0;
|
||||
setBanner(null);
|
||||
setLastCloseCode(null);
|
||||
|
|
@ -842,6 +877,8 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
|
||||
ws.onclose = (ev) => {
|
||||
wsRef.current = null;
|
||||
connectInFlightRef.current = false;
|
||||
clearConnectingTimer();
|
||||
if (unmounting) {
|
||||
return;
|
||||
}
|
||||
|
|
@ -998,6 +1035,8 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
if (settleRaf1) cancelAnimationFrame(settleRaf1);
|
||||
if (settleRaf2) cancelAnimationFrame(settleRaf2);
|
||||
clearReconnectTimer();
|
||||
clearConnectingTimer();
|
||||
connectInFlightRef.current = false;
|
||||
// Phase 5.3: ``ws`` is local to the IIFE that opens it (the gated-mode
|
||||
// ticket fetch makes the open async). The cleanup runs at the outer
|
||||
// effect's top level so it can't reach into that scope — close via
|
||||
|
|
@ -1082,10 +1121,11 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
|||
online,
|
||||
socketReadyState,
|
||||
ptyState: ptyStateRef.current,
|
||||
connectInFlight: connectInFlightRef.current,
|
||||
})
|
||||
) {
|
||||
const now = Date.now();
|
||||
if (now - lastResumeReconnectAtRef.current < 1000) {
|
||||
if (now - lastResumeReconnectAtRef.current < PTY_RESUME_RECONNECT_THROTTLE_MS) {
|
||||
return;
|
||||
}
|
||||
lastResumeReconnectAtRef.current = now;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue