fix(FN-958): fix terminal idle disconnect with heartbeat tolerance
- Add heartbeat tolerance window to prevent false idle disconnects on terminal connections - Increase client heartbeat interval from 30s to 45s for better idle tolerance - Add session staleness detection on reconnect with idle state logging - Add tests for heartbeat tolerance and client interval configuration - Add changeset for patch release
This commit is contained in:
@@ -1,10 +1,34 @@
|
||||
// @vitest-environment node
|
||||
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { createServer } from "./server.js";
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import http from "node:http";
|
||||
import express from "express";
|
||||
import { createServer, setupTerminalWebSocket } from "./server.js";
|
||||
import type { TaskStore } from "@fusion/core";
|
||||
import { get as performGet, request as performRequest } from "./test-request.js";
|
||||
|
||||
// Mock terminal-service before any imports that use it
|
||||
vi.mock("./terminal-service.js", () => {
|
||||
const mockTerminalService = {
|
||||
getSession: vi.fn(),
|
||||
getScrollbackAndClearPending: vi.fn().mockReturnValue(null),
|
||||
onData: vi.fn().mockReturnValue(() => {}),
|
||||
onExit: vi.fn().mockReturnValue(() => {}),
|
||||
write: vi.fn(),
|
||||
resize: vi.fn(),
|
||||
evictStaleSessions: vi.fn().mockReturnValue(0),
|
||||
};
|
||||
|
||||
return {
|
||||
getTerminalService: vi.fn(() => mockTerminalService),
|
||||
STALE_SESSION_THRESHOLD_MS: 300_000,
|
||||
__mockTerminalService: mockTerminalService,
|
||||
};
|
||||
});
|
||||
|
||||
// Access the mock terminal service
|
||||
const { __mockTerminalService: mockTerminalService } = await import("./terminal-service.js") as any;
|
||||
|
||||
function createMockStore(overrides: Partial<TaskStore> = {}): TaskStore {
|
||||
return {
|
||||
getTask: vi.fn(),
|
||||
@@ -248,3 +272,193 @@ describe("API Error Handling Middleware", () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("Terminal WebSocket heartbeat", () => {
|
||||
let app: ReturnType<typeof express>;
|
||||
let server: http.Server;
|
||||
|
||||
beforeEach(() => {
|
||||
app = express();
|
||||
server = http.createServer(app);
|
||||
vi.useFakeTimers();
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
vi.spyOn(console, "info").mockImplementation(() => {});
|
||||
vi.spyOn(console, "log").mockImplementation(() => {});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
vi.restoreAllMocks();
|
||||
server.close();
|
||||
});
|
||||
|
||||
/** Create a mock WebSocket that simulates the ws library's WebSocket */
|
||||
function createMockWs(): any {
|
||||
const listeners: Record<string, Function[]> = {};
|
||||
return {
|
||||
readyState: 1, // OPEN
|
||||
_listeners: listeners,
|
||||
on(event: string, handler: Function) {
|
||||
if (!listeners[event]) listeners[event] = [];
|
||||
listeners[event].push(handler);
|
||||
},
|
||||
emit(event: string, ...args: any[]) {
|
||||
(listeners[event] || []).forEach((h) => h(...args));
|
||||
},
|
||||
send: vi.fn(),
|
||||
close: vi.fn(),
|
||||
terminate: vi.fn(),
|
||||
};
|
||||
}
|
||||
|
||||
/** Create a mock HTTP request with sessionId */
|
||||
function createMockReq(sessionId: string): any {
|
||||
return {
|
||||
url: `/api/terminal/ws?sessionId=${sessionId}`,
|
||||
headers: { host: "localhost:3000" },
|
||||
};
|
||||
}
|
||||
|
||||
/** Setup terminal WebSocket and trigger a connection */
|
||||
function setupAndConnect(ws: any, req: any): void {
|
||||
const wss = setupTerminalWebSocket(app, server);
|
||||
|
||||
// The function sets up wss on the server's upgrade event.
|
||||
// We need to access the WebSocketServer directly to emit a connection.
|
||||
// setupTerminalWebSocket stores wss on the app
|
||||
const storedWss = (app as any).terminalWsServer;
|
||||
if (storedWss) {
|
||||
storedWss.emit("connection", ws, req);
|
||||
}
|
||||
}
|
||||
|
||||
it("does NOT terminate connection after 1 missed pong", () => {
|
||||
const ws = createMockWs();
|
||||
const req = createMockReq("session-1");
|
||||
|
||||
// Setup a mock session
|
||||
mockTerminalService.getSession.mockReturnValue({
|
||||
id: "session-1",
|
||||
shell: "/bin/bash",
|
||||
cwd: "/project",
|
||||
lastActivityAt: new Date(),
|
||||
});
|
||||
|
||||
setupAndConnect(ws, req);
|
||||
|
||||
// First ping interval: mark as not alive
|
||||
vi.advanceTimersByTime(30000);
|
||||
// The server sends a ping, ws is marked as not alive
|
||||
expect(ws.send).toHaveBeenCalled();
|
||||
|
||||
// Don't send a pong response — simulate missed pong
|
||||
// Second ping interval: first missed pong — should NOT terminate
|
||||
vi.advanceTimersByTime(30000);
|
||||
|
||||
// Connection should still be alive after 1 missed pong
|
||||
expect(ws.terminate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("terminates connection after 2 consecutive missed pongs", () => {
|
||||
const ws = createMockWs();
|
||||
const req = createMockReq("session-2");
|
||||
|
||||
mockTerminalService.getSession.mockReturnValue({
|
||||
id: "session-2",
|
||||
shell: "/bin/bash",
|
||||
cwd: "/project",
|
||||
lastActivityAt: new Date(),
|
||||
});
|
||||
|
||||
setupAndConnect(ws, req);
|
||||
|
||||
// First ping interval: mark as not alive (isAlive = false)
|
||||
vi.advanceTimersByTime(30000);
|
||||
expect(ws.send).toHaveBeenCalled();
|
||||
|
||||
// Don't send pong — missed pong #1
|
||||
vi.advanceTimersByTime(30000);
|
||||
expect(ws.terminate).not.toHaveBeenCalled();
|
||||
|
||||
// Don't send pong — missed pong #2: should terminate
|
||||
vi.advanceTimersByTime(30000);
|
||||
expect(ws.terminate).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("resets missed pong counter on successful pong", () => {
|
||||
const ws = createMockWs();
|
||||
const req = createMockReq("session-3");
|
||||
|
||||
mockTerminalService.getSession.mockReturnValue({
|
||||
id: "session-3",
|
||||
shell: "/bin/bash",
|
||||
cwd: "/project",
|
||||
lastActivityAt: new Date(),
|
||||
});
|
||||
|
||||
setupAndConnect(ws, req);
|
||||
|
||||
// First interval: mark as not alive
|
||||
vi.advanceTimersByTime(30000);
|
||||
|
||||
// Miss first pong — interval 2: missedPongs = 1
|
||||
vi.advanceTimersByTime(30000);
|
||||
expect(ws.terminate).not.toHaveBeenCalled();
|
||||
|
||||
// Now respond with pong (application-level "pong" message)
|
||||
const msgHandler = ws._listeners["message"]?.[0];
|
||||
expect(msgHandler).toBeDefined();
|
||||
msgHandler!(Buffer.from(JSON.stringify({ type: "pong" })));
|
||||
|
||||
// Interval 3: isAlive is true again, missedPongs is 0
|
||||
vi.advanceTimersByTime(30000);
|
||||
// Still alive — missed pong counter was reset
|
||||
expect(ws.terminate).not.toHaveBeenCalled();
|
||||
|
||||
// Miss 2 more pongs — should still be alive after just 1 more miss
|
||||
vi.advanceTimersByTime(30000);
|
||||
expect(ws.terminate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("logs warning for stale session reconnect", () => {
|
||||
const ws = createMockWs();
|
||||
const req = createMockReq("stale-session");
|
||||
|
||||
// Session last active 10 minutes ago (past the 5-minute threshold)
|
||||
const tenMinutesAgo = new Date(Date.now() - 600_000);
|
||||
mockTerminalService.getSession.mockReturnValue({
|
||||
id: "stale-session",
|
||||
shell: "/bin/bash",
|
||||
cwd: "/project",
|
||||
lastActivityAt: tenMinutesAgo,
|
||||
});
|
||||
|
||||
setupAndConnect(ws, req);
|
||||
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining("stale-session"),
|
||||
);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining("PTY may be stale"),
|
||||
);
|
||||
});
|
||||
|
||||
it("does not warn for fresh session reconnect", () => {
|
||||
const ws = createMockWs();
|
||||
const req = createMockReq("fresh-session");
|
||||
|
||||
// Session last active 1 minute ago (under the 5-minute threshold)
|
||||
mockTerminalService.getSession.mockReturnValue({
|
||||
id: "fresh-session",
|
||||
shell: "/bin/bash",
|
||||
cwd: "/project",
|
||||
lastActivityAt: new Date(Date.now() - 60_000),
|
||||
});
|
||||
|
||||
setupAndConnect(ws, req);
|
||||
|
||||
expect(console.warn).not.toHaveBeenCalledWith(
|
||||
expect.stringContaining("PTY may be stale"),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -9,7 +9,7 @@ import { createApiRoutes } from "./routes.js";
|
||||
import { createSSE } from "./sse.js";
|
||||
import { rateLimit, RATE_LIMITS } from "./rate-limit.js";
|
||||
import { getOrCreateProjectStore, evictAllProjectStores } from "./project-store-resolver.js";
|
||||
import { getTerminalService, type TerminalSession } from "./terminal-service.js";
|
||||
import { getTerminalService, type TerminalSession, STALE_SESSION_THRESHOLD_MS } from "./terminal-service.js";
|
||||
import { WebSocketServer, type WebSocket } from "ws";
|
||||
import { terminalSessionManager } from "./terminal.js";
|
||||
import { getCurrentGitHubRepo, parseBadgeUrl } from "./github.js";
|
||||
@@ -379,11 +379,22 @@ export function setupTerminalWebSocket(
|
||||
return;
|
||||
}
|
||||
|
||||
const MAX_MISSED_PONGS = 2; // Allow 2 missed pongs (~90s) before terminating
|
||||
|
||||
// Track if connection is alive
|
||||
let isAlive = true;
|
||||
let missedPongs = 0; // Track consecutive missed pongs
|
||||
let dataUnsub: (() => void) | null = null;
|
||||
let exitUnsub: (() => void) | null = null;
|
||||
|
||||
// Detect potentially stale sessions on reconnect
|
||||
const idleMs = Date.now() - session.lastActivityAt.getTime();
|
||||
if (idleMs > STALE_SESSION_THRESHOLD_MS) {
|
||||
console.warn(
|
||||
`[terminal] Session ${sessionId} reconnect after ${Math.round(idleMs / 1000)}s idle — PTY may be stale`
|
||||
);
|
||||
}
|
||||
|
||||
// Send scrollback buffer first
|
||||
const scrollback = terminalService.getScrollbackAndClearPending(sessionId);
|
||||
if (scrollback) {
|
||||
@@ -413,6 +424,8 @@ export function setupTerminalWebSocket(
|
||||
if (id === sessionId && isAlive) {
|
||||
try {
|
||||
ws.send(JSON.stringify({ type: "exit", exitCode }));
|
||||
const idleSec = id ? Math.round((Date.now() - (terminalService.getSession(id)?.lastActivityAt?.getTime() ?? Date.now())) / 1000) : 0;
|
||||
console.info(`[terminal] Session ${id} exited with code ${exitCode} (was ${idleSec}s idle)`);
|
||||
} catch {
|
||||
// WebSocket might be closing
|
||||
}
|
||||
@@ -422,7 +435,13 @@ export function setupTerminalWebSocket(
|
||||
// Heartbeat ping/pong
|
||||
const pingInterval = setInterval(() => {
|
||||
if (!isAlive) {
|
||||
ws.terminate();
|
||||
missedPongs++;
|
||||
if (missedPongs >= MAX_MISSED_PONGS) {
|
||||
console.warn(`[terminal] Connection dead after ${missedPongs} missed pongs, terminating`);
|
||||
ws.terminate();
|
||||
return;
|
||||
}
|
||||
console.info(`[terminal] Missed pong #${missedPongs}, waiting for response...`);
|
||||
return;
|
||||
}
|
||||
isAlive = false;
|
||||
@@ -435,6 +454,7 @@ export function setupTerminalWebSocket(
|
||||
|
||||
ws.on("pong", () => {
|
||||
isAlive = true;
|
||||
missedPongs = 0; // Reset on successful pong
|
||||
});
|
||||
|
||||
ws.on("message", (message: Buffer) => {
|
||||
@@ -457,6 +477,7 @@ export function setupTerminalWebSocket(
|
||||
break;
|
||||
case "pong":
|
||||
isAlive = true;
|
||||
missedPongs = 0; // Reset on successful pong
|
||||
break;
|
||||
}
|
||||
} catch {
|
||||
|
||||
Reference in New Issue
Block a user