fix: harden web terminal paste and reconnect
- Queue bounded ordered terminal input so acknowledgements remain responsive - Prevent stale sockets and retries from replacing healthy connections - Preserve desktop scrollback behavior and add terminal regression coverage Quality gates: frontend tests (91 passed), typecheck, lint, build, Python compilation, LSP diagnostics. Backend pytest skipped by user request.
This commit is contained in:
@@ -3,6 +3,8 @@
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
from asyncio import QueueFull
|
||||
from json import JSONDecodeError
|
||||
import uuid
|
||||
from contextlib import suppress
|
||||
|
||||
@@ -31,6 +33,9 @@ from src.services.terminal.terminal_manager import (
|
||||
router = APIRouter()
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
MAX_PENDING_INPUT_MESSAGES = 64
|
||||
MAX_TERMINAL_INPUT_BYTES = 1024 * 1024
|
||||
|
||||
|
||||
class SessionRef:
|
||||
"""Mutable reference to a terminal session, allowing updates during reset."""
|
||||
@@ -320,8 +325,38 @@ async def _handle_terminal_websocket(
|
||||
)
|
||||
|
||||
|
||||
async def _input_write_loop(input_queue: asyncio.Queue[tuple[object, bytes]]) -> None:
|
||||
"""Serialize PTY writes without blocking terminal control messages."""
|
||||
while True:
|
||||
session, data = await input_queue.get()
|
||||
try:
|
||||
await session.write_input(data) # type: ignore[attr-defined]
|
||||
except Exception:
|
||||
logger.debug("Terminal input write failed", exc_info=True)
|
||||
finally:
|
||||
input_queue.task_done()
|
||||
|
||||
|
||||
def _queue_terminal_input(
|
||||
input_queue: asyncio.Queue[tuple[object, bytes]], session: object, data: bytes
|
||||
) -> bool:
|
||||
"""Queue bounded terminal input without blocking control-message processing."""
|
||||
if len(data) > MAX_TERMINAL_INPUT_BYTES:
|
||||
return False
|
||||
try:
|
||||
input_queue.put_nowait((session, data))
|
||||
except QueueFull:
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
async def _write_loop(session_ref: SessionRef, websocket, instance_id: str) -> None:
|
||||
"""Read input from WebSocket and send to container."""
|
||||
"""Receive terminal messages while a dedicated worker serializes PTY input."""
|
||||
input_queue: asyncio.Queue[tuple[object, bytes]] = asyncio.Queue(
|
||||
maxsize=MAX_PENDING_INPUT_MESSAGES
|
||||
)
|
||||
input_writer = asyncio.create_task(_input_write_loop(input_queue))
|
||||
|
||||
try:
|
||||
while True:
|
||||
session = session_ref.session
|
||||
@@ -331,7 +366,12 @@ async def _write_loop(session_ref: SessionRef, websocket, instance_id: str) -> N
|
||||
message = await websocket.receive()
|
||||
if message["type"] == "websocket.receive":
|
||||
if "bytes" in message:
|
||||
await session.write_input(message["bytes"])
|
||||
if not _queue_terminal_input(
|
||||
input_queue, session, message["bytes"]
|
||||
):
|
||||
logger.warning("Terminal input buffer exceeded for %s", instance_id)
|
||||
await websocket.close(code=1009, reason="Terminal input buffer full")
|
||||
break
|
||||
elif "text" in message:
|
||||
text = message["text"]
|
||||
# A text frame that parses to a JSON object with a
|
||||
@@ -345,13 +385,22 @@ async def _write_loop(session_ref: SessionRef, websocket, instance_id: str) -> N
|
||||
if text.startswith("{"):
|
||||
try:
|
||||
parsed = json.loads(text)
|
||||
except json.JSONDecodeError:
|
||||
except JSONDecodeError:
|
||||
parsed = None
|
||||
if isinstance(parsed, dict) and "type" in parsed:
|
||||
ctrl = parsed
|
||||
|
||||
if ctrl is None:
|
||||
await session.write_input(text.encode("utf-8"))
|
||||
if not _queue_terminal_input(
|
||||
input_queue, session, text.encode("utf-8")
|
||||
):
|
||||
logger.warning(
|
||||
"Terminal input buffer exceeded for %s", instance_id
|
||||
)
|
||||
await websocket.close(
|
||||
code=1009, reason="Terminal input buffer full"
|
||||
)
|
||||
break
|
||||
continue
|
||||
|
||||
msg_type = ctrl["type"]
|
||||
@@ -403,8 +452,14 @@ async def _write_loop(session_ref: SessionRef, websocket, instance_id: str) -> N
|
||||
continue
|
||||
elif message["type"] == "websocket.disconnect":
|
||||
break
|
||||
except Exception:
|
||||
except WebSocketDisconnect:
|
||||
pass
|
||||
except (RuntimeError, TypeError, ValueError) as exc:
|
||||
logger.debug("Terminal WebSocket receive loop ended: %s", exc)
|
||||
finally:
|
||||
input_writer.cancel()
|
||||
with suppress(asyncio.CancelledError):
|
||||
await input_writer
|
||||
|
||||
|
||||
async def _heartbeat_loop(websocket: WebSocket) -> None:
|
||||
|
||||
@@ -1,7 +1,15 @@
|
||||
"""Integration tests for multi-session terminal WebSocket and REST API."""
|
||||
|
||||
import asyncio
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from src.api.system.terminal import (
|
||||
MAX_TERMINAL_INPUT_BYTES,
|
||||
SessionRef,
|
||||
_queue_terminal_input,
|
||||
_write_loop,
|
||||
)
|
||||
from src.main import app
|
||||
|
||||
|
||||
@@ -19,12 +27,79 @@ class TestTerminalWebSocketMultiSession:
|
||||
# the route exists by checking for a 403 (no auth cookie)
|
||||
response = client.get("/ws/tool-instances/test-instance/terminal/test-session")
|
||||
# WebSocket endpoint returns 403 when accessed via HTTP GET
|
||||
assert response.status_code in (403, 404)
|
||||
assert response.status_code == 403 or response.status_code == 404
|
||||
|
||||
def test_default_session_alias_route_exists(self, client):
|
||||
"""The default session alias route should still exist."""
|
||||
response = client.get("/ws/tool-instances/test-instance/terminal")
|
||||
assert response.status_code in (403, 404)
|
||||
assert response.status_code == 403 or response.status_code == 404
|
||||
|
||||
|
||||
def test_terminal_input_queue_rejects_excess_input_without_blocking() -> None:
|
||||
"""A stalled PTY writer cannot make the input queue grow without limit."""
|
||||
input_queue: asyncio.Queue[tuple[object, bytes]] = asyncio.Queue(maxsize=1)
|
||||
session = object()
|
||||
|
||||
assert _queue_terminal_input(input_queue, session, b"first")
|
||||
assert not _queue_terminal_input(input_queue, session, b"second")
|
||||
assert not _queue_terminal_input(
|
||||
asyncio.Queue(), session, b"x" * (MAX_TERMINAL_INPUT_BYTES + 1)
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ack_is_processed_while_a_pty_write_is_waiting() -> None:
|
||||
"""A blocked paste writer must not block flow-control acknowledgements."""
|
||||
|
||||
write_started = asyncio.Event()
|
||||
|
||||
class Session:
|
||||
_closed = False
|
||||
|
||||
def __init__(self) -> None:
|
||||
self.acks: list[int] = []
|
||||
self.write_finished = False
|
||||
|
||||
def is_alive(self) -> bool:
|
||||
return True
|
||||
|
||||
async def write_input(self, _data: bytes) -> None:
|
||||
write_started.set()
|
||||
try:
|
||||
await asyncio.Event().wait()
|
||||
finally:
|
||||
self.write_finished = True
|
||||
|
||||
def acknowledge_data(self, char_count: int) -> None:
|
||||
self.acks.append(char_count)
|
||||
|
||||
class WebSocket:
|
||||
def __init__(self) -> None:
|
||||
self.messages = iter(
|
||||
[
|
||||
{"type": "websocket.receive", "bytes": b"large paste"},
|
||||
{"type": "websocket.receive", "text": '{"type":"ack","chars":4096}'},
|
||||
{"type": "websocket.disconnect"},
|
||||
]
|
||||
)
|
||||
self.receive_count = 0
|
||||
|
||||
async def receive(self):
|
||||
self.receive_count += 1
|
||||
if self.receive_count > 1:
|
||||
await write_started.wait()
|
||||
return next(self.messages)
|
||||
|
||||
session = Session()
|
||||
websocket = WebSocket()
|
||||
task = asyncio.create_task(_write_loop(SessionRef(session), websocket, "instance"))
|
||||
|
||||
await asyncio.wait_for(write_started.wait(), timeout=0.1)
|
||||
await asyncio.sleep(0)
|
||||
assert session.acks == [4096]
|
||||
|
||||
await task
|
||||
assert session.write_finished
|
||||
|
||||
|
||||
class TestTerminalRestApi:
|
||||
|
||||
@@ -1,6 +1,10 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import { getTerminalScrollbackLimit } from "./terminal.tsx";
|
||||
import {
|
||||
getTerminalScrollbackLimit,
|
||||
isCurrentWebSocket,
|
||||
shouldRetryWebSocketClose,
|
||||
} from "./terminal.tsx";
|
||||
|
||||
describe("getTerminalScrollbackLimit", () => {
|
||||
it("retains normal-buffer history for custom mobile swipe scrolling", () => {
|
||||
@@ -10,4 +14,19 @@ describe("getTerminalScrollbackLimit", () => {
|
||||
it("keeps desktop scrollback disabled to prevent stale-frame wheel scrolling", () => {
|
||||
expect(getTerminalScrollbackLimit(false)).toBe(0);
|
||||
});
|
||||
|
||||
it("rejects stale WebSocket callbacks after a replacement connection", () => {
|
||||
const current = {} as WebSocket;
|
||||
const stale = {} as WebSocket;
|
||||
|
||||
expect(isCurrentWebSocket(current, current)).toBe(true);
|
||||
expect(isCurrentWebSocket(current, stale)).toBe(false);
|
||||
});
|
||||
|
||||
it("retries a heartbeat timeout but not a server socket replacement", () => {
|
||||
expect(shouldRetryWebSocketClose(4000, "Heartbeat timeout")).toBe(true);
|
||||
expect(shouldRetryWebSocketClose(4000, "New connection established")).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -57,6 +57,17 @@ export function getTerminalScrollbackLimit(isMobile: boolean): number {
|
||||
return isMobile ? 10_000 : 0;
|
||||
}
|
||||
|
||||
export function isCurrentWebSocket(
|
||||
current: WebSocket | null,
|
||||
candidate: WebSocket,
|
||||
): boolean {
|
||||
return current === candidate;
|
||||
}
|
||||
|
||||
export function shouldRetryWebSocketClose(code: number, reason: string): boolean {
|
||||
return code !== 1000 && !(code === 4000 && reason === "New connection established");
|
||||
}
|
||||
|
||||
function matchesByteSequence(
|
||||
data: Uint8Array,
|
||||
start: number,
|
||||
@@ -87,6 +98,7 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
const bracketedPasteEnabledRef = useRef(false);
|
||||
const pasteTextRef = useRef<(text: string) => void>(() => {});
|
||||
const reconnectAttemptsRef = useRef(0);
|
||||
const reconnectTimerRef = useRef<number | null>(null);
|
||||
const onTerminalReadyRef = useRef(onTerminalReady);
|
||||
onTerminalReadyRef.current = onTerminalReady;
|
||||
const handleFontSizeChangeRef = useRef<(delta: number) => void>(() => {});
|
||||
@@ -115,7 +127,23 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
return fontSize;
|
||||
}, [fontSize]);
|
||||
|
||||
const clearReconnectTimer = useCallback(() => {
|
||||
if (reconnectTimerRef.current !== null) {
|
||||
window.clearTimeout(reconnectTimerRef.current);
|
||||
reconnectTimerRef.current = null;
|
||||
}
|
||||
}, []);
|
||||
|
||||
const connectWebSocket = useCallback(() => {
|
||||
const currentWs = wsRef.current;
|
||||
if (
|
||||
currentWs?.readyState === WebSocket.CONNECTING ||
|
||||
currentWs?.readyState === WebSocket.OPEN
|
||||
) {
|
||||
return currentWs;
|
||||
}
|
||||
clearReconnectTimer();
|
||||
|
||||
const apiUrl = import.meta.env.VITE_API_BASE_URL || "";
|
||||
const wsProtocol = window.location.protocol === "https:" ? "wss:" : "ws:";
|
||||
const wsHost = apiUrl.replace(/^https?:\/\//, "").replace(/\/+$/, "");
|
||||
@@ -165,6 +193,11 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
};
|
||||
|
||||
ws.onopen = () => {
|
||||
if (!isCurrentWebSocket(wsRef.current, ws)) {
|
||||
ws.close(1000, "Superseded connection");
|
||||
return;
|
||||
}
|
||||
|
||||
setStatus("connected");
|
||||
setError(null);
|
||||
reconnectAttemptsRef.current = 0;
|
||||
@@ -205,7 +238,7 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
};
|
||||
|
||||
ws.onmessage = (event) => {
|
||||
if (!termRef.current) return;
|
||||
if (!isCurrentWebSocket(wsRef.current, ws) || !termRef.current) return;
|
||||
|
||||
if (event.data instanceof ArrayBuffer) {
|
||||
const data = new Uint8Array(event.data);
|
||||
@@ -264,6 +297,10 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
};
|
||||
|
||||
ws.onclose = (event) => {
|
||||
if (!isCurrentWebSocket(wsRef.current, ws)) return;
|
||||
wsRef.current = null;
|
||||
if (ackTimeout) window.clearTimeout(ackTimeout);
|
||||
|
||||
// Clean up heartbeat check
|
||||
if (heartbeatCheckRef.current) {
|
||||
window.clearInterval(heartbeatCheckRef.current);
|
||||
@@ -279,18 +316,12 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
return;
|
||||
}
|
||||
|
||||
if (event.code === 1000) {
|
||||
if (!shouldRetryWebSocketClose(event.code, event.reason)) {
|
||||
setStatus("disconnected");
|
||||
return;
|
||||
}
|
||||
|
||||
if (event.code === 4000) {
|
||||
// Server closed old connection for concurrent connection - don't reconnect
|
||||
// The new connection is already established
|
||||
return;
|
||||
}
|
||||
|
||||
// Transient errors: attempt reconnection
|
||||
// Transient errors: attempt reconnection.
|
||||
setStatus("disconnected");
|
||||
setError(`Connection closed (code: ${event.code})`);
|
||||
|
||||
@@ -299,11 +330,14 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
const delay =
|
||||
RECONNECT_DELAY_BASE *
|
||||
Math.pow(2, reconnectAttemptsRef.current - 1);
|
||||
setTimeout(() => {
|
||||
if (isUnmountingRef.current) {
|
||||
return;
|
||||
}
|
||||
if (document.visibilityState !== "hidden") {
|
||||
clearReconnectTimer();
|
||||
reconnectTimerRef.current = window.setTimeout(() => {
|
||||
reconnectTimerRef.current = null;
|
||||
if (
|
||||
!isUnmountingRef.current &&
|
||||
document.visibilityState !== "hidden" &&
|
||||
wsRef.current === null
|
||||
) {
|
||||
connectWebSocket();
|
||||
}
|
||||
}, delay);
|
||||
@@ -311,15 +345,18 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
};
|
||||
|
||||
ws.onerror = () => {
|
||||
if (!isCurrentWebSocket(wsRef.current, ws)) return;
|
||||
setStatus("error");
|
||||
setError("WebSocket error");
|
||||
};
|
||||
|
||||
return ws;
|
||||
}, [instanceId, sessionId]);
|
||||
}, [clearReconnectTimer, instanceId, sessionId]);
|
||||
|
||||
useEffect(() => {
|
||||
if (!terminalRef.current) return;
|
||||
isUnmountingRef.current = false;
|
||||
permanentErrorRef.current = null;
|
||||
|
||||
// Initialize terminal
|
||||
const currentFontSize = calculateFontSize();
|
||||
@@ -410,7 +447,7 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
// Open xterm first (must happen before fit)
|
||||
term.open(container);
|
||||
term.focus();
|
||||
const ws = connectWebSocket();
|
||||
connectWebSocket();
|
||||
|
||||
pasteTextRef.current = (text: string) => {
|
||||
const currentWs = wsRef.current;
|
||||
@@ -658,14 +695,12 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
|
||||
// Visibility API for reconnection
|
||||
const handleVisibilityChange = () => {
|
||||
const currentWs = wsRef.current;
|
||||
if (
|
||||
document.visibilityState === "visible" &&
|
||||
ws &&
|
||||
ws.readyState !== WebSocket.OPEN
|
||||
!permanentErrorRef.current &&
|
||||
(currentWs === null || currentWs.readyState === WebSocket.CLOSED)
|
||||
) {
|
||||
if (permanentErrorRef.current) {
|
||||
return;
|
||||
}
|
||||
reconnectAttemptsRef.current = 0;
|
||||
connectWebSocket();
|
||||
}
|
||||
@@ -674,6 +709,7 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
|
||||
return () => {
|
||||
isUnmountingRef.current = true;
|
||||
clearReconnectTimer();
|
||||
clearTimeout(resizeTimeout);
|
||||
clearTimeout(windowResizeTimeout);
|
||||
clearTimeout(headerHideTimeout);
|
||||
@@ -687,8 +723,10 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
container.removeEventListener("paste", handleBrowserPaste, true);
|
||||
pasteTextRef.current = () => {};
|
||||
bracketedPasteEnabledRef.current = false;
|
||||
if (ws) {
|
||||
ws.close(1000, "Component unmounting");
|
||||
const currentWs = wsRef.current;
|
||||
wsRef.current = null;
|
||||
if (currentWs) {
|
||||
currentWs.close(1000, "Component unmounting");
|
||||
}
|
||||
if (heartbeatCheckRef.current) {
|
||||
window.clearInterval(heartbeatCheckRef.current);
|
||||
|
||||
Reference in New Issue
Block a user