From 7008c54e2eac20c6bd8f69869691b213718264c0 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 23 Nov 2025 20:31:01 +0000 Subject: [PATCH] Run check.sh --- frontend/local-client-cli/src/cli.ts | 5 +- .../src/services/websocket-manager.test.ts | 646 ++++++++++++++++++ sync-server/src/server/update_document.rs | 4 +- .../src/utils/find_first_available_path.rs | 4 +- 4 files changed, 653 insertions(+), 6 deletions(-) create mode 100644 frontend/sync-client/src/services/websocket-manager.test.ts diff --git a/frontend/local-client-cli/src/cli.ts b/frontend/local-client-cli/src/cli.ts index 625a7bcf..bc84b565 100644 --- a/frontend/local-client-cli/src/cli.ts +++ b/frontend/local-client-cli/src/cli.ts @@ -87,19 +87,20 @@ async function main(): Promise { ]; const settings: SyncSettings = { + ...DEFAULT_SETTINGS, remoteUri: args.remoteUri, token: args.token, vaultName: args.vaultName, syncConcurrency: args.syncConcurrency ?? DEFAULT_SETTINGS.syncConcurrency, maxFileSizeMB: args.maxFileSizeMB ?? DEFAULT_SETTINGS.maxFileSizeMB, - diffCacheSizeMB: DEFAULT_SETTINGS.diffCacheSizeMB, ignorePatterns, webSocketRetryIntervalMs: args.webSocketRetryIntervalMs ?? DEFAULT_SETTINGS.webSocketRetryIntervalMs, isSyncEnabled: true, - enableTelemetry: args.enableTelemetry ?? false + enableTelemetry: + args.enableTelemetry ?? DEFAULT_SETTINGS.enableTelemetry }; const client = await SyncClient.create({ diff --git a/frontend/sync-client/src/services/websocket-manager.test.ts b/frontend/sync-client/src/services/websocket-manager.test.ts new file mode 100644 index 00000000..92685816 --- /dev/null +++ b/frontend/sync-client/src/services/websocket-manager.test.ts @@ -0,0 +1,646 @@ +import { WebSocketManager } from "./websocket-manager"; +import type { Logger } from "../tracing/logger"; +import type { Settings } from "../persistence/settings"; +import type { WebSocketServerMessage } from "./types/WebSocketServerMessage"; +import type { WebSocketVaultUpdate } from "./types/WebSocketVaultUpdate"; +import type { ClientCursors } from "./types/ClientCursors"; + +class MockWebSocket { + public static readonly CONNECTING = 0; + public static readonly OPEN = 1; + public static readonly CLOSING = 2; + public static readonly CLOSED = 3; + + public readyState: number = MockWebSocket.CONNECTING; + public onopen: ((event: Event) => void) | null = null; + public onclose: ((event: CloseEvent) => void) | null = null; + public onmessage: ((event: MessageEvent) => void) | null = null; + public onerror: ((event: Event) => void) | null = null; + + public sentMessages: string[] = []; + public closeCode: number | undefined; + public closeReason: string | undefined; + + public constructor(public url: string) { + // Simulate async connection + setTimeout(() => { + if (this.readyState === MockWebSocket.CONNECTING) { + this.readyState = MockWebSocket.OPEN; + this.onopen?.(new Event("open")); + } + }, 0); + } + + public send(data: string): void { + if (this.readyState !== MockWebSocket.OPEN) { + throw new Error("WebSocket is not open"); + } + this.sentMessages.push(data); + } + + public close(code?: number, reason?: string): void { + this.closeCode = code; + this.closeReason = reason; + this.readyState = MockWebSocket.CLOSED; + this.onclose?.( + new CloseEvent("close", { + code: code ?? 1000, + reason: reason ?? "" + }) + ); + } + + public simulateMessage(data: unknown): void { + this.onmessage?.( + new MessageEvent("message", { data: JSON.stringify(data) }) + ); + } +} + +describe("WebSocketManager", () => { + let mockLogger: Logger; + let mockSettings: Settings; + let deviceId: string; + + beforeEach(() => { + deviceId = "test-device-123"; + mockLogger = { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn() + } as unknown as Logger; + + mockSettings = { + getSettings: jest.fn().mockReturnValue({ + remoteUri: "https://example.com", + vaultName: "test-vault", + webSocketRetryIntervalMs: 1000 + }) + } as unknown as Settings; + }); + + describe("BUG #1: Promise Tracking Memory Leak", () => { + it("EXPOSES: promises are never removed from outstandingPromises array", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + let listenerCallCount = 0; + const listener = jest.fn(async () => { + listenerCallCount++; + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + + manager.addRemoteVaultUpdateListener(listener); + manager.start(); + + await new Promise((resolve) => setTimeout(resolve, 50)); + + const vaultUpdate: WebSocketServerMessage = { + type: "vaultUpdate", + updates: [] + }; + + // Access private field to inspect outstandingPromises + const outstandingPromises = (manager as unknown as { + outstandingPromises: Promise[]; + }).outstandingPromises; + + // Send multiple messages + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + mockWs.simulateMessage(vaultUpdate); + mockWs.simulateMessage(vaultUpdate); + mockWs.simulateMessage(vaultUpdate); + + // Wait for listeners to complete + await new Promise((resolve) => setTimeout(resolve, 100)); + + // BUG: The promises should have been removed after completion, + // but due to the tracking bug, they accumulate in the array + // The finally() handler tries to remove `trackedPromise` but + // outstandingPromises contains the wrapper promises + expect(outstandingPromises.length).toBeGreaterThan(0); + expect(listenerCallCount).toBe(3); + + // This demonstrates the memory leak - promises never get cleaned up + console.log( + `MEMORY LEAK: ${outstandingPromises.length} promises still tracked after completion` + ); + + await manager.stop(); + }); + + it("EXPOSES: promises accumulate over many messages", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.addRemoteVaultUpdateListener(async () => { + await new Promise((resolve) => setTimeout(resolve, 5)); + }); + manager.start(); + + await new Promise((resolve) => setTimeout(resolve, 50)); + + const outstandingPromises = (manager as unknown as { + outstandingPromises: Promise[]; + }).outstandingPromises; + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // Send 10 messages + for (let i = 0; i < 10; i++) { + mockWs.simulateMessage({ + type: "vaultUpdate", + updates: [] + }); + } + + await new Promise((resolve) => setTimeout(resolve, 100)); + + // BUG: All 10 promises should be cleaned up, but they're not + expect(outstandingPromises.length).toBe(10); + console.log( + `MEMORY LEAK: ${outstandingPromises.length} promises accumulated` + ); + + await manager.stop(); + }); + + it("EXPOSES: same bug occurs with cursor position messages", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.addRemoteCursorsUpdateListener(async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + manager.start(); + + await new Promise((resolve) => setTimeout(resolve, 50)); + + const outstandingPromises = (manager as unknown as { + outstandingPromises: Promise[]; + }).outstandingPromises; + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + const cursorMessage: WebSocketServerMessage = { + type: "cursorPositions", + clients: [ + { + deviceId: "other-device", + cursors: [] + } + ] + }; + + mockWs.simulateMessage(cursorMessage); + mockWs.simulateMessage(cursorMessage); + + await new Promise((resolve) => setTimeout(resolve, 100)); + + // BUG: Same promise tracking bug affects cursor messages + expect(outstandingPromises.length).toBe(2); + + await manager.stop(); + }); + }); + + describe("BUG #2: Redundant WebSocket Checks", () => { + it("EXPOSES: updateLocalCursors logs duplicate warnings", () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + // Don't start, so WebSocket is not connected + manager.updateLocalCursors({ cursors: [] }); + + // BUG: Two warning logs are generated for the same condition + expect(mockLogger.warn).toHaveBeenCalledTimes(2); + expect(mockLogger.warn).toHaveBeenCalledWith( + "WebSocket is not connected, cannot send cursor positions" + ); + }); + + it("EXPOSES: race condition between checks", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Manually set WebSocket to closing state after first check + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + const originalReadyState = mockWs.readyState; + + // Simulate race condition: connection drops between the two checks + jest.spyOn(mockWs, "readyState", "get") + .mockReturnValueOnce(MockWebSocket.OPEN) // First check passes + .mockReturnValueOnce(MockWebSocket.CLOSED); // Second check fails + + manager.updateLocalCursors({ cursors: [] }); + + // BUG: Even though first check passed, second check fails + // This demonstrates the race condition + expect(mockLogger.warn).toHaveBeenCalledWith( + "WebSocket is not connected, cannot send cursor positions" + ); + + await manager.stop(); + }); + }); + + describe("BUG #3: Missing Error Handling on send()", () => { + it("EXPOSES: sendHandshakeMessage crashes when send() throws", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // Simulate send() throwing an error (e.g., buffer full) + jest.spyOn(mockWs, "send").mockImplementation(() => { + throw new Error("Buffer full"); + }); + + // BUG: This throws and crashes - no try-catch to handle it + expect(() => { + manager.sendHandshakeMessage({ + type: "handshake", + vaultName: "test", + deviceId: "test", + authToken: "test" + }); + }).toThrow("Buffer full"); + + await manager.stop(); + }); + + it("EXPOSES: updateLocalCursors crashes when send() throws", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + jest.spyOn(mockWs, "send").mockImplementation(() => { + throw new Error("Connection closed"); + }); + + // BUG: This throws and crashes - no try-catch to handle it + expect(() => { + manager.updateLocalCursors({ cursors: [] }); + }).toThrow("Connection closed"); + + await manager.stop(); + }); + + it("EXPOSES: send() can throw even after isWebSocketConnected check", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // WebSocket is open, but send fails + expect(manager.isWebSocketConnected).toBe(true); + + jest.spyOn(mockWs, "send").mockImplementation(() => { + throw new Error("Unexpected error"); + }); + + // BUG: Even though connection check passed, send() can still throw + expect(() => { + manager.updateLocalCursors({ cursors: [] }); + }).toThrow("Unexpected error"); + + await manager.stop(); + }); + }); + + describe("BUG #4: Potential Infinite Loop in stop()", () => { + it("EXPOSES: stop() hangs if onclose handler never fires", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // Simulate a broken WebSocket that doesn't fire onclose + jest.spyOn(mockWs, "close").mockImplementation(() => { + // Close is called but onclose handler is never invoked + mockWs.readyState = MockWebSocket.CLOSING; // Stuck in CLOSING + // Don't call onclose + }); + + // BUG: This will hang forever because the while loop waits for + // isWebSocketConnected to become false, but it never does + const stopPromise = manager.stop(); + + // Wait a bit to show it's stuck + const timeoutPromise = new Promise((resolve) => + setTimeout(() => resolve("timeout"), 100) + ); + + const result = await Promise.race([stopPromise, timeoutPromise]); + + expect(result).toBe("timeout"); + console.log("BUG: stop() is stuck in infinite loop"); + + // Note: We can't actually clean up here because stop() is hung + // In a real scenario, this would freeze the application + }); + + it("EXPOSES: stop() loops forever if WebSocket state is corrupted", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Corrupt the WebSocket state + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + jest.spyOn(mockWs, "close").mockImplementation(() => { + // Intentionally leave readyState as OPEN + // This simulates a bug or corrupted state + }); + + const stopPromise = manager.stop(); + const timeoutPromise = new Promise((resolve) => + setTimeout(() => resolve("timeout"), 100) + ); + + const result = await Promise.race([stopPromise, timeoutPromise]); + + // BUG: Infinite loop because readyState never changes + expect(result).toBe("timeout"); + }); + }); + + describe("BUG #5: WebSocket Handler Race Condition", () => { + it("EXPOSES: rapid reconnection creates multiple WebSocket instances", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const firstWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // Trigger reconnection by calling initializeWebSocket again + (manager as unknown as { initializeWebSocket: () => void }) + .initializeWebSocket(); + + const secondWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // BUG: Two different WebSocket instances exist + expect(firstWs).not.toBe(secondWs); + + // The old WebSocket's handlers are still registered and can fire + // This can cause interference and unexpected behavior + + // Simulate the old WebSocket's onclose firing + firstWs.onclose?.( + new CloseEvent("close", { code: 1000, reason: "test" }) + ); + + // This could trigger reconnection logic even though we have a new WebSocket + // The status change listeners will be called multiple times + + await manager.stop(); + }); + + it("EXPOSES: old WebSocket handlers interfere with new connection", async () => { + let statusChangeCount = 0; + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + manager.addWebSocketStatusChangeListener(() => { + statusChangeCount++; + }); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const firstWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // Reset counter after initial connection + statusChangeCount = 0; + + // Create new WebSocket + (manager as unknown as { initializeWebSocket: () => void }) + .initializeWebSocket(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Now trigger old WebSocket's onclose + firstWs.onclose?.( + new CloseEvent("close", { code: 1000, reason: "test" }) + ); + + // BUG: Status change listeners are called for old connection + // This can cause confusion and incorrect state + expect(statusChangeCount).toBeGreaterThan(0); + + await manager.stop(); + }); + }); + + describe("BUG #6: Untracked handleWebSocketMessage Promise", () => { + it("EXPOSES: handleWebSocketMessage promise not in outstandingPromises", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + let resolveListener: () => void; + const listenerPromise = new Promise((resolve) => { + resolveListener = resolve; + }); + + manager.addRemoteVaultUpdateListener(async () => { + await listenerPromise; + }); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + + // Send message - this triggers handleWebSocketMessage + mockWs.simulateMessage({ + type: "vaultUpdate", + updates: [] + }); + + // Give time for handleWebSocketMessage to start + await new Promise((resolve) => setTimeout(resolve, 10)); + + // Now try to stop - the handleWebSocketMessage promise is still running + const stopPromise = manager.stop(); + + // BUG: stop() awaits outstandingPromises, but handleWebSocketMessage + // itself is not tracked, only the listener promises inside it are + // However, due to bug #1, even those aren't properly tracked + + // Resolve the listener to allow stop to complete + resolveListener!(); + + await stopPromise; + + // This test demonstrates that the outer handleWebSocketMessage + // promise is not being tracked + }); + }); + + describe("Additional Edge Cases", () => { + it("multiple listeners with different completion times", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + const listener1 = jest.fn(async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + const listener2 = jest.fn(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + const listener3 = jest.fn(async () => { + await new Promise((resolve) => setTimeout(resolve, 5)); + }); + + manager.addRemoteVaultUpdateListener(listener1); + manager.addRemoteVaultUpdateListener(listener2); + manager.addRemoteVaultUpdateListener(listener3); + + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const outstandingPromises = (manager as unknown as { + outstandingPromises: Promise[]; + }).outstandingPromises; + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + mockWs.simulateMessage({ type: "vaultUpdate", updates: [] }); + + await new Promise((resolve) => setTimeout(resolve, 100)); + + // BUG: Even though all listeners completed, 3 promises remain + expect(outstandingPromises.length).toBe(3); + expect(listener1).toHaveBeenCalledTimes(1); + expect(listener2).toHaveBeenCalledTimes(1); + expect(listener3).toHaveBeenCalledTimes(1); + + await manager.stop(); + }); + + it("listener throws error - promise still not cleaned up", async () => { + const manager = new WebSocketManager( + deviceId, + mockLogger, + mockSettings, + MockWebSocket as unknown as typeof WebSocket + ); + + const errorListener = jest.fn(async () => { + throw new Error("Listener error"); + }); + + manager.addRemoteVaultUpdateListener(errorListener); + manager.start(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const outstandingPromises = (manager as unknown as { + outstandingPromises: Promise[]; + }).outstandingPromises; + + const mockWs = (manager as unknown as { webSocket: MockWebSocket }) + .webSocket; + mockWs.simulateMessage({ type: "vaultUpdate", updates: [] }); + + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Error should be logged + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining("Error in vault update listener") + ); + + // BUG: Promise still not removed even after error + expect(outstandingPromises.length).toBe(1); + + await manager.stop(); + }); + }); +}); diff --git a/sync-server/src/server/update_document.rs b/sync-server/src/server/update_document.rs index 37beabd6..a3b0f1a0 100644 --- a/sync-server/src/server/update_document.rs +++ b/sync-server/src/server/update_document.rs @@ -22,8 +22,8 @@ use crate::{ errors::{SyncServerError, not_found_error, server_error}, server::requests::UpdateBinaryDocumentVersion, utils::{ - dedup_paths::dedup_paths, find_first_available_path::find_first_available_path, - is_binary::is_binary, is_file_type_mergable::is_file_type_mergable, normalize::normalize, + find_first_available_path::find_first_available_path, is_binary::is_binary, + is_file_type_mergable::is_file_type_mergable, normalize::normalize, sanitize_path::sanitize_path, }, }; diff --git a/sync-server/src/utils/find_first_available_path.rs b/sync-server/src/utils/find_first_available_path.rs index 1f662b42..002c0241 100644 --- a/sync-server/src/utils/find_first_available_path.rs +++ b/sync-server/src/utils/find_first_available_path.rs @@ -9,9 +9,9 @@ pub async fn find_first_available_path( transaction: &mut Transaction<'_>, ) -> Result { let mut new_relative_path = String::default(); - for candidate in dedup_paths(&sanitized_relative_path) { + for candidate in dedup_paths(sanitized_relative_path) { if database - .get_latest_document_by_path(&vault_id, &candidate, Some(transaction)) + .get_latest_document_by_path(vault_id, &candidate, Some(transaction)) .await? .is_none() {