From 024e365ee03cbff951fe2ada37102a50b69074c2 Mon Sep 17 00:00:00 2001 From: importcpp Date: Tue, 22 Sep 2026 16:30:53 +0800 Subject: [PATCH] fix(mobile): avoid duplicate WebSocket recovery and reject invalid frames (#8690) --- .../data/realtime/realtime-provider.test.ts | 76 ++++++++++++ .../data/realtime/realtime-provider.tsx | 3 +- apps/mobile/data/realtime/ws-client.test.ts | 109 ++++++++++++++++++ apps/mobile/data/realtime/ws-client.ts | 27 ++--- 4 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 apps/mobile/data/realtime/realtime-provider.test.ts diff --git a/apps/mobile/data/realtime/realtime-provider.test.ts b/apps/mobile/data/realtime/realtime-provider.test.ts new file mode 100644 index 000000000..ab3ea9a0b --- /dev/null +++ b/apps/mobile/data/realtime/realtime-provider.test.ts @@ -0,0 +1,76 @@ +// @vitest-environment node +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const lifecycle = vi.hoisted(() => ({ + setup: undefined as (() => () => void) | undefined, + onAppState: undefined as ((state: string) => void) | undefined, +})); + +vi.mock("react", async (importOriginal) => ({ + ...await importOriginal(), + useEffect: (setup: () => () => void) => { lifecycle.setup = setup; }, + useRef: (current: unknown) => ({ current }), + useState: () => [null, vi.fn()], +})); +vi.mock("react-native", () => ({ + AppState: { + addEventListener: (_event: string, handler: (state: string) => void) => { + lifecycle.onAppState = handler; + return { remove: vi.fn() }; + }, + }, +})); +vi.mock("@react-native-community/netinfo", () => ({ + default: { addEventListener: () => vi.fn() }, +})); +vi.mock("@/data/auth-store", () => ({ useAuthStore: () => "user-1" })); +vi.mock("@/data/workspace-store", () => ({ useWorkspaceStore: () => "workspace" })); +vi.mock("@/data/secure-storage", () => ({ getToken: async () => "token" })); +vi.mock("@/data/api", () => ({ api: { getToken: () => "token" } })); + +// Run the provider's effect and its actual WSClient, without rendering RN. +class MockWebSocket { + static instances: MockWebSocket[] = []; + constructor() { MockWebSocket.instances.push(this); } + close() {} +} + +describe("RealtimeProvider foreground recovery", () => { + let cleanup: (() => void) | undefined; + + beforeEach(() => { + lifecycle.setup = undefined; + lifecycle.onAppState = undefined; + MockWebSocket.instances = []; + vi.stubGlobal("WebSocket", MockWebSocket); + vi.stubEnv("EXPO_PUBLIC_API_URL", "https://example.test"); + vi.spyOn(console, "info").mockImplementation(() => {}); + }); + + afterEach(() => { + cleanup?.(); + cleanup = undefined; + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + it("opens exactly one socket per background or inactive return", async () => { + const { RealtimeProvider } = await import("./realtime-provider"); + RealtimeProvider({ children: null }); + cleanup = lifecycle.setup!(); + // Let the effect finish reading the token and install native listeners. + await Promise.resolve(); + expect(MockWebSocket.instances).toHaveLength(1); + + lifecycle.onAppState!("background"); + expect(MockWebSocket.instances).toHaveLength(1); + lifecycle.onAppState!("active"); + expect(MockWebSocket.instances).toHaveLength(2); + + lifecycle.onAppState!("inactive"); + expect(MockWebSocket.instances).toHaveLength(2); + lifecycle.onAppState!("active"); + expect(MockWebSocket.instances).toHaveLength(3); + }); +}); diff --git a/apps/mobile/data/realtime/realtime-provider.tsx b/apps/mobile/data/realtime/realtime-provider.tsx index 70aa71451..9205ff243 100644 --- a/apps/mobile/data/realtime/realtime-provider.tsx +++ b/apps/mobile/data/realtime/realtime-provider.tsx @@ -108,9 +108,8 @@ export function RealtimeProvider({ children }: { children: React.ReactNode }) { if (status === "active") { // Foreground. The socket may have been paused (we put it // there on background) or it may be a zombie (iOS killed - // it silently). Either way: resume / force-reconnect. + // it silently). resume() handles both with one fresh socket. ws?.resume(); - ws?.forceReconnect(); } else if (status === "background") { ws?.pause(); } diff --git a/apps/mobile/data/realtime/ws-client.test.ts b/apps/mobile/data/realtime/ws-client.test.ts index 73ac99a91..3d121061b 100644 --- a/apps/mobile/data/realtime/ws-client.test.ts +++ b/apps/mobile/data/realtime/ws-client.test.ts @@ -1,3 +1,4 @@ +// @vitest-environment node import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { WSClient } from "./ws-client"; @@ -149,3 +150,111 @@ describe("WSClient session renewal", () => { }); }); }); + +describe("WSClient frame validation", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.stubGlobal("WebSocket", MockWebSocket); + MockWebSocket.instances = []; + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it.each([ + null, + false, + 42, + "event", + [], + {}, + { error: "unauthorized" }, + { type: null }, + { type: 42 }, + { type: {} }, + { type: "" }, + ])("ignores invalid envelopes and still delivers the next event: %j", (frame) => { + const { client, socket } = connectAuthenticatedClient(); + const onAny = vi.fn(); + const onRunning = vi.fn(); + client.onAny(onAny); + client.on("task:running", onRunning); + + expect(() => socket.receive(frame)).not.toThrow(); + expect(onAny).not.toHaveBeenCalled(); + expect(onRunning).not.toHaveBeenCalled(); + + const payload = { task_id: "task-1", agent_id: "agent-1", issue_id: "issue-1" }; + const message = { type: "task:running", payload, actor_id: "actor-1" }; + socket.receive(message); + + expect(onRunning).toHaveBeenCalledExactlyOnceWith(payload, "actor-1"); + expect(onAny).toHaveBeenCalledExactlyOnceWith(message); + client.disconnect(); + }); +}); + +describe("WSClient foreground recovery", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.stubGlobal("WebSocket", MockWebSocket); + MockWebSocket.instances = []; + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it.each(["paused", "active"])("opens one fresh socket on returning from %s", (state) => { + const { client, socket } = connectAuthenticatedClient(); + const onReconnect = vi.fn(); + client.onReconnect(onReconnect); + if (state === "paused") client.pause(); + + client.resume(); + + expect(MockWebSocket.instances).toHaveLength(2); + expect(socket.readyState).toBe(MockWebSocket.CLOSED); + expect(onReconnect).not.toHaveBeenCalled(); + const resumed = MockWebSocket.instances[1]; + resumed.open(); + resumed.receive({ type: "auth_ack" }); + resumed.receive({ type: "pong" }); + expect(onReconnect).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(10_001); + expect(MockWebSocket.instances).toHaveLength(2); + client.disconnect(); + }); + + it("cancels a pending reconnect when returning to the foreground", () => { + vi.spyOn(Math, "random").mockReturnValue(0.5); + const { client, socket } = connectAuthenticatedClient(); + socket.close(); + + client.resume(); + + expect(MockWebSocket.instances).toHaveLength(2); + vi.advanceTimersByTime(2_000); + expect(MockWebSocket.instances).toHaveLength(2); + client.disconnect(); + }); + + it("does not resume after disconnect or wake a paused client on a network edge", () => { + const { client } = connectAuthenticatedClient(); + client.pause(); + client.forceReconnect(); + vi.advanceTimersByTime(60_000); + expect(MockWebSocket.instances).toHaveLength(1); + + client.disconnect(); + client.resume(); + client.forceReconnect(); + vi.advanceTimersByTime(60_000); + expect(MockWebSocket.instances).toHaveLength(1); + }); +}); diff --git a/apps/mobile/data/realtime/ws-client.ts b/apps/mobile/data/realtime/ws-client.ts index 58386eaed..650dd8637 100644 --- a/apps/mobile/data/realtime/ws-client.ts +++ b/apps/mobile/data/realtime/ws-client.ts @@ -136,12 +136,12 @@ export class WSClient { this.teardownSocket(); } - /** Paused → active. Used by the provider when AppState=active. */ + /** Return to the foreground with one fresh socket. An active socket may + * also need replacing after iOS silently drops the connection. */ resume() { - if (this.state !== "paused") return; + if (this.state === "idle") return; this.state = "active"; - this.reconnectAttempt = 0; - this.openSocket(); + this.forceReconnect(); } /** Force a fresh socket without going through paused. Used when NetInfo @@ -232,7 +232,16 @@ export class WSClient { return; } - const type = (msg as { type?: string }).type; + // Validate the envelope before transport frames or business dispatch. + // JSON primitives (including null) and non-string types are not events. + const type = (msg as { type?: unknown } | null)?.type; + if (typeof type !== "string" || !type) { + // Server-side error frames have shape {error: "..."}; log and drop. + // Reconnect loop is bounded by auth-store's 401 handler eventually + // tearing this client down via disconnect(). + this.logger.warn("[ws] frame without a string type", event.data); + return; + } if (type === "auth_ack") { this.onAuthenticated(); return; @@ -241,14 +250,6 @@ export class WSClient { this.onPong(); return; } - if (!type) { - // Server-side error frames have shape {error: "..."}; log and drop. - // Reconnect loop is bounded by auth-store's 401 handler eventually - // tearing this client down via disconnect(). - this.logger.warn("[ws] frame without type", event.data); - return; - } - this.logger.debug("[ws] event", type); const set = this.handlers.get(msg.type); if (set) {