From 02adf46ef7858683433b791b55b70b1c85474571 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 30 Dec 2025 11:40:21 +0300 Subject: [PATCH 1/3] Add network info file cleanup and missing file retry logic - Clean up network info files older than 1 hour when monitoring starts - Search for new SSH process after 5 consecutive file read failures --- src/remote/sshProcess.ts | 110 ++++++++++++++++++++++------ test/unit/remote/sshProcess.test.ts | 88 ++++++++++++++++++++++ 2 files changed, 177 insertions(+), 21 deletions(-) diff --git a/src/remote/sshProcess.ts b/src/remote/sshProcess.ts index b610d6e4..f4b31e00 100644 --- a/src/remote/sshProcess.ts +++ b/src/remote/sshProcess.ts @@ -39,6 +39,9 @@ export interface SshProcessMonitorOptions { remoteSshExtensionId: string; } +// Cleanup threshold for old network info files (1 hour) +const CLEANUP_MAX_AGE_MS = 60 * 60 * 1000; + /** * Monitors the SSH process for a Coder workspace connection and displays * network status in the VS Code status bar. @@ -70,6 +73,48 @@ export class SshProcessMonitor implements vscode.Disposable { private pendingTimeout: NodeJS.Timeout | undefined; private lastStaleSearchTime = 0; + /** + * Cleans up network info files older than the specified age. + */ + private static async cleanupOldNetworkFiles( + networkInfoPath: string, + maxAgeMs: number, + logger: Logger, + ): Promise { + const deletedFiles: string[] = []; + try { + const files = await fs.readdir(networkInfoPath); + const now = Date.now(); + + for (const file of files) { + if (!file.endsWith(".json")) { + continue; + } + + const filePath = path.join(networkInfoPath, file); + try { + const stats = await fs.stat(filePath); + const ageMs = now - stats.mtime.getTime(); + + if (ageMs > maxAgeMs) { + await fs.unlink(filePath); + deletedFiles.push(file); + } + } catch { + // File may have been deleted by another process or doesn't exist, ignore + } + } + } catch { + // Directory may not exist yet, that's fine + } + + if (deletedFiles.length > 0) { + logger.debug( + `Cleaned up ${deletedFiles.length} old network info file(s): ${deletedFiles.join(", ")}`, + ); + } + } + private constructor(options: SshProcessMonitorOptions) { this.options = { ...options, @@ -91,6 +136,16 @@ export class SshProcessMonitor implements vscode.Disposable { */ public static start(options: SshProcessMonitorOptions): SshProcessMonitor { const monitor = new SshProcessMonitor(options); + + // Clean up old network info files (non-blocking, fire-and-forget) + SshProcessMonitor.cleanupOldNetworkFiles( + options.networkInfoPath, + CLEANUP_MAX_AGE_MS, + options.logger, + ).catch(() => { + // Ignore cleanup errors - they shouldn't affect monitoring + }); + monitor.searchForProcess().catch((err) => { options.logger.error("Error in SSH process monitor", err); }); @@ -285,10 +340,13 @@ export class SshProcessMonitor implements vscode.Disposable { /** * Monitors network info and updates the status bar. * Checks file mtime to detect stale connections and trigger reconnection search. + * After consecutive failures to read the file, searches for a new process. */ private async monitorNetwork(): Promise { const { networkInfoPath, networkPollInterval, logger } = this.options; const staleThreshold = networkPollInterval * 5; + const maxReadFailures = 5; + let consecutiveReadFailures = 0; while (!this.disposed && this.currentPid !== undefined) { const networkInfoFile = path.join( @@ -296,36 +354,46 @@ export class SshProcessMonitor implements vscode.Disposable { `${this.currentPid}.json`, ); + let searchReason = ""; + try { const stats = await fs.stat(networkInfoFile); const ageMs = Date.now() - stats.mtime.getTime(); - if (ageMs > staleThreshold) { - // Prevent tight loop: if we just searched due to stale, wait before searching again - const timeSinceLastSearch = Date.now() - this.lastStaleSearchTime; - if (timeSinceLastSearch < staleThreshold) { - await this.delay(staleThreshold - timeSinceLastSearch); - continue; - } - - logger.debug( - `Network info stale (${Math.round(ageMs / 1000)}s old), searching for new SSH process`, - ); + consecutiveReadFailures = 0; - // searchForProcess will update PID if a different process is found - this.lastStaleSearchTime = Date.now(); - await this.searchForProcess(); - return; + if (ageMs > staleThreshold) { + searchReason = `Network info stale (${Math.round(ageMs / 1000)}s old)`; + } else { + const content = await fs.readFile(networkInfoFile, "utf8"); + const network = JSON.parse(content) as NetworkInfo; + const isStale = ageMs > networkPollInterval * 2; + this.updateStatusBar(network, isStale); } - - const content = await fs.readFile(networkInfoFile, "utf8"); - const network = JSON.parse(content) as NetworkInfo; - const isStale = ageMs > this.options.networkPollInterval * 2; - this.updateStatusBar(network, isStale); } catch (error) { + consecutiveReadFailures++; logger.debug( - `Failed to read network info: ${(error as Error).message}`, + `Failed to read network info (attempt ${consecutiveReadFailures}): ${(error as Error).message}`, ); + + if (consecutiveReadFailures >= maxReadFailures) { + searchReason = `Network info missing for ${consecutiveReadFailures} attempts`; + } + } + + if (searchReason) { + // Throttle: don't search too frequently + const timeSinceLastSearch = Date.now() - this.lastStaleSearchTime; + if (timeSinceLastSearch < staleThreshold) { + await this.delay(staleThreshold - timeSinceLastSearch); + continue; + } + + logger.debug(`${searchReason}, searching for new SSH process`); + // searchForProcess will update PID if a different process is found + this.lastStaleSearchTime = Date.now(); + await this.searchForProcess(); + return; } await this.delay(networkPollInterval); diff --git a/test/unit/remote/sshProcess.test.ts b/test/unit/remote/sshProcess.test.ts index befd068b..e1df1fd1 100644 --- a/test/unit/remote/sshProcess.test.ts +++ b/test/unit/remote/sshProcess.test.ts @@ -423,6 +423,94 @@ describe("SshProcessMonitor", () => { }); }); + describe("cleanup old network files", () => { + const TWO_HOURS_AGO = Date.now() - 2 * 60 * 60 * 1000; + + function setOldMtime(filePath: string): void { + // Default cleanup is 1 hour; set mtime to 2 hours ago to mark as old + vol.utimesSync(filePath, TWO_HOURS_AGO / 1000, TWO_HOURS_AGO / 1000); + } + + it("deletes old .json files but preserves recent and non-.json files", async () => { + vol.fromJSON({ + "/logs/ms-vscode-remote.remote-ssh/1-Remote - SSH.log": + "-> socksPort 12345 ->", + "/network/old.json": "{}", + "/network/recent.json": "{}", + "/network/old.log": "{}", + }); + setOldMtime("/network/old.json"); + setOldMtime("/network/old.log"); + + createMonitor({ + codeLogDir: "/logs/window1", + networkInfoPath: "/network", + }); + + await vi.waitFor(() => { + const files = vol.readdirSync("/network"); + expect(files).toHaveLength(2); + expect(files).toContain("old.log"); + expect(files).toContain("recent.json"); + }); + }); + + it("does not throw when network directory is missing or empty", () => { + vol.fromJSON({ + "/logs/ms-vscode-remote.remote-ssh/1-Remote - SSH.log": + "-> socksPort 12345 ->", + }); + vol.mkdirSync("/empty-network", { recursive: true }); + + expect(() => + createMonitor({ + codeLogDir: "/logs/window1", + networkInfoPath: "/nonexistent", + }), + ).not.toThrow(); + + expect(() => + createMonitor({ + codeLogDir: "/logs/window1", + networkInfoPath: "/empty-network", + }), + ).not.toThrow(); + }); + }); + + describe("missing file retry logic", () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it("searches for new process after consecutive file read failures", async () => { + vol.fromJSON({ + "/logs/ms-vscode-remote.remote-ssh/1-Remote - SSH.log": + "-> socksPort 12345 ->", + }); + + vi.mocked(find) + .mockResolvedValueOnce([{ pid: 999, ppid: 1, name: "ssh", cmd: "ssh" }]) + .mockResolvedValue([{ pid: 888, ppid: 1, name: "ssh", cmd: "ssh" }]); + + const pollInterval = 10; + const monitor = createMonitor({ + codeLogDir: "/logs/window1", + networkInfoPath: "/network", + networkPollInterval: pollInterval, + }); + + await vi.advanceTimersByTimeAsync(pollInterval); + + const pids: (number | undefined)[] = []; + monitor.onPidChange((pid) => pids.push(pid)); + + // 5 failures at pollInterval each, then staleThreshold wait before search + await vi.advanceTimersByTimeAsync(pollInterval * 5 * 2); + + expect(pids).toContain(888); + }); + }); + describe("dispose", () => { it("disposes status bar", () => { const monitor = createMonitor(); From dba73e176babd5b8d5d7cbfdb205215bec8c443e Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 30 Dec 2025 12:57:02 +0300 Subject: [PATCH 2/3] Self-review --- src/remote/sshProcess.ts | 63 +++++++++++++++-------------- test/unit/remote/sshProcess.test.ts | 30 +++++++++----- 2 files changed, 51 insertions(+), 42 deletions(-) diff --git a/src/remote/sshProcess.ts b/src/remote/sshProcess.ts index f4b31e00..22f27deb 100644 --- a/src/remote/sshProcess.ts +++ b/src/remote/sshProcess.ts @@ -39,7 +39,7 @@ export interface SshProcessMonitorOptions { remoteSshExtensionId: string; } -// Cleanup threshold for old network info files (1 hour) +// Cleanup threshold for old network info files const CLEANUP_MAX_AGE_MS = 60 * 60 * 1000; /** @@ -81,11 +81,11 @@ export class SshProcessMonitor implements vscode.Disposable { maxAgeMs: number, logger: Logger, ): Promise { - const deletedFiles: string[] = []; try { const files = await fs.readdir(networkInfoPath); const now = Date.now(); + const deletedFiles: string[] = []; for (const file of files) { if (!file.endsWith(".json")) { continue; @@ -104,14 +104,14 @@ export class SshProcessMonitor implements vscode.Disposable { // File may have been deleted by another process or doesn't exist, ignore } } - } catch { - // Directory may not exist yet, that's fine - } - if (deletedFiles.length > 0) { - logger.debug( - `Cleaned up ${deletedFiles.length} old network info file(s): ${deletedFiles.join(", ")}`, - ); + if (deletedFiles.length > 0) { + logger.debug( + `Cleaned up ${deletedFiles.length} old network info file(s): ${deletedFiles.join(", ")}`, + ); + } + } catch { + // Directory may not exist yet, ignore } } @@ -339,57 +339,58 @@ export class SshProcessMonitor implements vscode.Disposable { /** * Monitors network info and updates the status bar. - * Checks file mtime to detect stale connections and trigger reconnection search. - * After consecutive failures to read the file, searches for a new process. + * Searches for a new process if the file is stale or unreadable. */ private async monitorNetwork(): Promise { const { networkInfoPath, networkPollInterval, logger } = this.options; const staleThreshold = networkPollInterval * 5; const maxReadFailures = 5; - let consecutiveReadFailures = 0; + let readFailures = 0; while (!this.disposed && this.currentPid !== undefined) { - const networkInfoFile = path.join( - networkInfoPath, - `${this.currentPid}.json`, - ); - - let searchReason = ""; + const filePath = path.join(networkInfoPath, `${this.currentPid}.json`); + let search: { needed: true; reason: string } | { needed: false } = { + needed: false, + }; try { - const stats = await fs.stat(networkInfoFile); + const stats = await fs.stat(filePath); const ageMs = Date.now() - stats.mtime.getTime(); - - consecutiveReadFailures = 0; + readFailures = 0; if (ageMs > staleThreshold) { - searchReason = `Network info stale (${Math.round(ageMs / 1000)}s old)`; + search = { + needed: true, + reason: `Network info stale (${Math.round(ageMs / 1000)}s old)`, + }; } else { - const content = await fs.readFile(networkInfoFile, "utf8"); + const content = await fs.readFile(filePath, "utf8"); const network = JSON.parse(content) as NetworkInfo; const isStale = ageMs > networkPollInterval * 2; this.updateStatusBar(network, isStale); } } catch (error) { - consecutiveReadFailures++; + readFailures++; logger.debug( - `Failed to read network info (attempt ${consecutiveReadFailures}): ${(error as Error).message}`, + `Failed to read network info (attempt ${readFailures}): ${(error as Error).message}`, ); - - if (consecutiveReadFailures >= maxReadFailures) { - searchReason = `Network info missing for ${consecutiveReadFailures} attempts`; + if (readFailures >= maxReadFailures) { + search = { + needed: true, + reason: `Network info missing for ${readFailures} attempts`, + }; } } - if (searchReason) { - // Throttle: don't search too frequently + // Search for new process if needed (with throttling) + if (search.needed) { const timeSinceLastSearch = Date.now() - this.lastStaleSearchTime; if (timeSinceLastSearch < staleThreshold) { await this.delay(staleThreshold - timeSinceLastSearch); continue; } - logger.debug(`${searchReason}, searching for new SSH process`); + logger.debug(`${search.reason}, searching for new SSH process`); // searchForProcess will update PID if a different process is found this.lastStaleSearchTime = Date.now(); await this.searchForProcess(); diff --git a/test/unit/remote/sshProcess.test.ts b/test/unit/remote/sshProcess.test.ts index e1df1fd1..8b745d7a 100644 --- a/test/unit/remote/sshProcess.test.ts +++ b/test/unit/remote/sshProcess.test.ts @@ -424,12 +424,11 @@ describe("SshProcessMonitor", () => { }); describe("cleanup old network files", () => { - const TWO_HOURS_AGO = Date.now() - 2 * 60 * 60 * 1000; - - function setOldMtime(filePath: string): void { + const setOldMtime = (filePath: string) => { // Default cleanup is 1 hour; set mtime to 2 hours ago to mark as old + const TWO_HOURS_AGO = Date.now() - 2 * 60 * 60 * 1000; vol.utimesSync(filePath, TWO_HOURS_AGO / 1000, TWO_HOURS_AGO / 1000); - } + }; it("deletes old .json files but preserves recent and non-.json files", async () => { vol.fromJSON({ @@ -486,11 +485,22 @@ describe("SshProcessMonitor", () => { vol.fromJSON({ "/logs/ms-vscode-remote.remote-ssh/1-Remote - SSH.log": "-> socksPort 12345 ->", + "/network/789.json": "{}", }); + // Set mtime far into the future so 789.json is always considered fresh + const FRESH_MTIME = Date.now() + 1_000_000; + vol.utimesSync( + "/network/789.json", + FRESH_MTIME / 1000, + FRESH_MTIME / 1000, + ); vi.mocked(find) - .mockResolvedValueOnce([{ pid: 999, ppid: 1, name: "ssh", cmd: "ssh" }]) - .mockResolvedValue([{ pid: 888, ppid: 1, name: "ssh", cmd: "ssh" }]); + .mockResolvedValueOnce([{ pid: 123, ppid: 1, name: "ssh", cmd: "ssh" }]) + .mockResolvedValueOnce([{ pid: 456, ppid: 1, name: "ssh", cmd: "ssh" }]) + .mockResolvedValueOnce([{ pid: 789, ppid: 1, name: "ssh", cmd: "ssh" }]) + // This will not be found since `789.json` is found and is not stale! + .mockResolvedValue([{ pid: 999, ppid: 1, name: "ssh", cmd: "ssh" }]); const pollInterval = 10; const monitor = createMonitor({ @@ -499,15 +509,13 @@ describe("SshProcessMonitor", () => { networkPollInterval: pollInterval, }); - await vi.advanceTimersByTimeAsync(pollInterval); - const pids: (number | undefined)[] = []; monitor.onPidChange((pid) => pids.push(pid)); - // 5 failures at pollInterval each, then staleThreshold wait before search - await vi.advanceTimersByTimeAsync(pollInterval * 5 * 2); + // Advance enough time for the monitor to cycle through PIDs 123, 456, and find 789 + await vi.advanceTimersByTimeAsync(pollInterval * 100); - expect(pids).toContain(888); + expect(pids).toEqual([123, 456, 789]); }); }); From 3760f26e718405cdb044cf248c8e1989ce835967 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 30 Dec 2025 19:17:41 +0300 Subject: [PATCH 3/3] Review comments --- src/remote/sshProcess.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/remote/sshProcess.ts b/src/remote/sshProcess.ts index 22f27deb..f77a444d 100644 --- a/src/remote/sshProcess.ts +++ b/src/remote/sshProcess.ts @@ -39,7 +39,7 @@ export interface SshProcessMonitorOptions { remoteSshExtensionId: string; } -// Cleanup threshold for old network info files +// 1 hour cleanup threshold for old network info files const CLEANUP_MAX_AGE_MS = 60 * 60 * 1000; /** @@ -100,8 +100,10 @@ export class SshProcessMonitor implements vscode.Disposable { await fs.unlink(filePath); deletedFiles.push(file); } - } catch { - // File may have been deleted by another process or doesn't exist, ignore + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + logger.debug(`Failed to clean up network info file ${file}`, error); + } } }