diff --git a/CHANGELOG.md b/CHANGELOG.md index e9f3e218..6dcb88c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ ### Added +- Automatically set `reconnectionGraceTime`, `serverShutdownTimeout`, and `maxReconnectionAttempts` + on first connection to prevent disconnects during overnight workspace sleep. +- New **Coder: Apply Recommended SSH Settings** command to overwrite all recommended SSH settings at once. - Proxy log directory now defaults to the extension's global storage when `coder.proxyLogDirectory` is not set, so SSH connection logs are always captured without manual configuration. Also respects the `CODER_SSH_LOG_DIR` environment variable as a fallback. diff --git a/package.json b/package.json index bfe0dabf..02c49b40 100644 --- a/package.json +++ b/package.json @@ -320,6 +320,11 @@ "title": "Refresh Tasks", "category": "Coder", "icon": "$(refresh)" + }, + { + "command": "coder.applyRecommendedSettings", + "title": "Apply Recommended SSH Settings", + "category": "Coder" } ], "menus": { @@ -386,6 +391,9 @@ { "command": "coder.tasks.refresh", "when": "false" + }, + { + "command": "coder.applyRecommendedSettings" } ], "view/title": [ diff --git a/src/commands.ts b/src/commands.ts index 3a9ad5f1..3357f456 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -24,6 +24,10 @@ import { type Logger } from "./logging/logger"; import { type LoginCoordinator } from "./login/loginCoordinator"; import { withProgress } from "./progress"; import { maybeAskAgent, maybeAskUrl } from "./promptUtils"; +import { + RECOMMENDED_SSH_SETTINGS, + applySettingOverrides, +} from "./remote/userSettings"; import { escapeCommandArg, toRemoteAuthority, toSafeHost } from "./util"; import { vscodeProposed } from "./vscodeProposed"; import { @@ -310,6 +314,55 @@ export class Commands { } } + /** + * Apply recommended SSH settings for reliable Coder workspace connections. + */ + public async applyRecommendedSettings(): Promise { + const entries = Object.entries(RECOMMENDED_SSH_SETTINGS); + const summary = entries.map(([, s]) => s.label).join("\n"); + const confirm = await vscodeProposed.window.showWarningMessage( + "Apply Recommended SSH Settings", + { + useCustom: true, + modal: true, + detail: summary, + }, + "Apply", + ); + if (confirm !== "Apply") { + return; + } + + const overrides = entries.map(([key, setting]) => ({ + key, + value: setting.value, + })); + const ok = await applySettingOverrides( + this.pathResolver.getUserSettingsPath(), + overrides, + this.logger, + ); + if (!ok) { + const action = await vscode.window.showErrorMessage( + "Failed to write SSH settings. Check the Coder output for details.", + "Show Output", + ); + if (action === "Show Output") { + this.logger.show(); + } + } else if (this.remoteWorkspaceClient) { + const action = await vscode.window.showInformationMessage( + "Applied recommended SSH settings. Reload the window for changes to take effect.", + "Reload Window", + ); + if (action === "Reload Window") { + await vscode.commands.executeCommand("workbench.action.reloadWindow"); + } + } else { + vscode.window.showInformationMessage("Applied recommended SSH settings."); + } + } + /** * Create a new workspace for the currently logged-in deployment. * diff --git a/src/extension.ts b/src/extension.ts index 9266bf54..4a0db0ad 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -271,6 +271,10 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { "coder.manageCredentials", commands.manageCredentials.bind(commands), ), + vscode.commands.registerCommand( + "coder.applyRecommendedSettings", + commands.applyRecommendedSettings.bind(commands), + ), ); const remote = new Remote(serviceContainer, commands, ctx); diff --git a/src/logging/logger.ts b/src/logging/logger.ts index 30bf0ec6..0010d758 100644 --- a/src/logging/logger.ts +++ b/src/logging/logger.ts @@ -4,4 +4,5 @@ export interface Logger { info(message: string, ...args: unknown[]): void; warn(message: string, ...args: unknown[]): void; error(message: string, ...args: unknown[]): void; + show(): void; } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index ac7c99ca..d661a013 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -4,7 +4,6 @@ import { type Workspace, type WorkspaceAgent, } from "coder/site/src/api/typesGenerated"; -import * as jsonc from "jsonc-parser"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -60,6 +59,7 @@ import { } from "./sshConfig"; import { SshProcessMonitor } from "./sshProcess"; import { computeSshProperties, sshSupportsSetEnv } from "./sshSupport"; +import { applySettingOverrides, buildSshOverrides } from "./userSettings"; import { WorkspaceStateMachine } from "./workspaceStateMachine"; export interface RemoteDetails extends vscode.Disposable { @@ -459,83 +459,20 @@ export class Remote { const inbox = await Inbox.create(workspace, workspaceClient, this.logger); disposables.push(inbox); - // Do some janky setting manipulation. this.logger.info("Modifying settings..."); - const remotePlatforms = vscodeProposed.workspace - .getConfiguration() - .get>("remote.SSH.remotePlatform", {}); - const connTimeout = vscodeProposed.workspace - .getConfiguration() - .get("remote.SSH.connectTimeout"); - - // We have to directly munge the settings file with jsonc because trying to - // update properly through the extension API hangs indefinitely. Possibly - // VS Code is trying to update configuration on the remote, which cannot - // connect until we finish here leading to a deadlock. We need to update it - // locally, anyway, and it does not seem possible to force that via API. - let settingsContent = "{}"; - try { - settingsContent = await fs.readFile( + const overrides = buildSshOverrides( + vscodeProposed.workspace.getConfiguration(), + parts.sshHost, + agent.operating_system, + ); + if (overrides.length > 0) { + const ok = await applySettingOverrides( this.pathResolver.getUserSettingsPath(), - "utf8", - ); - } catch { - // Ignore! It's probably because the file doesn't exist. - } - - // Add the remote platform for this host to bypass a step where VS Code asks - // the user for the platform. - let mungedPlatforms = false; - if ( - !remotePlatforms[parts.sshHost] || - remotePlatforms[parts.sshHost] !== agent.operating_system - ) { - remotePlatforms[parts.sshHost] = agent.operating_system; - settingsContent = jsonc.applyEdits( - settingsContent, - jsonc.modify( - settingsContent, - ["remote.SSH.remotePlatform"], - remotePlatforms, - {}, - ), + overrides, + this.logger, ); - mungedPlatforms = true; - } - - // VS Code ignores the connect timeout in the SSH config and uses a default - // of 15 seconds, which can be too short in the case where we wait for - // startup scripts. For now we hardcode a longer value. Because this is - // potentially overwriting user configuration, it feels a bit sketchy. If - // microsoft/vscode-remote-release#8519 is resolved we can remove this. - const minConnTimeout = 1800; - let mungedConnTimeout = false; - if (!connTimeout || connTimeout < minConnTimeout) { - settingsContent = jsonc.applyEdits( - settingsContent, - jsonc.modify( - settingsContent, - ["remote.SSH.connectTimeout"], - minConnTimeout, - {}, - ), - ); - mungedConnTimeout = true; - } - - if (mungedPlatforms || mungedConnTimeout) { - try { - await fs.writeFile( - this.pathResolver.getUserSettingsPath(), - settingsContent, - ); - } catch (ex) { - // This could be because the user's settings.json is read-only. This is - // the case when using home-manager on NixOS, for example. Failure to - // write here is not necessarily catastrophic since the user will be - // asked for the platform and the default timeout might be sufficient. - mungedPlatforms = mungedConnTimeout = false; - this.logger.warn("Failed to configure settings", ex); + if (ok) { + this.logger.info("Settings modified successfully"); } } diff --git a/src/remote/userSettings.ts b/src/remote/userSettings.ts new file mode 100644 index 00000000..25f11202 --- /dev/null +++ b/src/remote/userSettings.ts @@ -0,0 +1,141 @@ +import { formatDuration, intervalToDuration } from "date-fns"; +import * as jsonc from "jsonc-parser"; +import * as fs from "node:fs/promises"; + +import type { WorkspaceConfiguration } from "vscode"; + +import type { Logger } from "../logging/logger"; + +export interface SettingOverride { + key: string; + value: unknown; +} + +interface RecommendedSetting { + readonly value: number | null; + readonly label: string; +} + +function recommended( + shortName: string, + value: number | null, +): RecommendedSetting { + if (value === null) { + return { value, label: `${shortName}: max allowed` }; + } + const humanized = formatDuration( + intervalToDuration({ start: 0, end: value * 1000 }), + ); + return { value, label: `${shortName}: ${humanized}` }; +} + +/** + * Applied by the "Apply Recommended SSH Settings" command. + * These are more aggressive (24h) than AUTO_SETUP_DEFAULTS (8h) because the + * user is explicitly opting in via the command palette. + */ +export const RECOMMENDED_SSH_SETTINGS = { + "remote.SSH.connectTimeout": recommended("Connect Timeout", 1800), + "remote.SSH.reconnectionGraceTime": recommended( + "Reconnection Grace Time", + 86400, + ), + "remote.SSH.serverShutdownTimeout": recommended( + "Server Shutdown Timeout", + 86400, + ), + "remote.SSH.maxReconnectionAttempts": recommended( + "Max Reconnection Attempts", + null, + ), +} as const satisfies Record; + +type SshSettingKey = keyof typeof RECOMMENDED_SSH_SETTINGS; + +/** Defaults set during connection when the user hasn't configured a value. */ +const AUTO_SETUP_DEFAULTS = { + "remote.SSH.reconnectionGraceTime": 28800, // 8h + "remote.SSH.serverShutdownTimeout": 28800, // 8h + "remote.SSH.maxReconnectionAttempts": null, // max allowed +} as const satisfies Partial>; + +/** + * Build the list of VS Code setting overrides needed for a remote SSH + * connection to a Coder workspace. + */ +export function buildSshOverrides( + config: Pick, + sshHost: string, + agentOS: string, +): SettingOverride[] { + const overrides: SettingOverride[] = []; + + // Set the remote platform for this host to bypass the platform prompt. + const remotePlatforms = config.get>( + "remote.SSH.remotePlatform", + {}, + ); + if (remotePlatforms[sshHost] !== agentOS) { + overrides.push({ + key: "remote.SSH.remotePlatform", + value: { ...remotePlatforms, [sshHost]: agentOS }, + }); + } + + // Default 15s is too short for startup scripts; enforce a minimum. + const connTimeoutKey: SshSettingKey = "remote.SSH.connectTimeout"; + const { value: minConnTimeout } = RECOMMENDED_SSH_SETTINGS[connTimeoutKey]; + const connTimeout = config.get(connTimeoutKey); + if (minConnTimeout && (!connTimeout || connTimeout < minConnTimeout)) { + overrides.push({ key: connTimeoutKey, value: minConnTimeout }); + } + + // Set conservative defaults for settings the user hasn't configured. + for (const [key, value] of Object.entries(AUTO_SETUP_DEFAULTS)) { + if (config.get(key) === undefined) { + overrides.push({ key, value }); + } + } + + return overrides; +} + +/** + * Apply setting overrides to the user's settings.json file. + * + * We munge the file directly with jsonc instead of using the VS Code API + * because the API hangs indefinitely during remote connection setup (likely + * a deadlock from trying to update config on the not-yet-connected remote). + */ +export async function applySettingOverrides( + settingsFilePath: string, + overrides: SettingOverride[], + logger: Logger, +): Promise { + if (overrides.length === 0) { + return true; + } + + let settingsContent = "{}"; + try { + settingsContent = await fs.readFile(settingsFilePath, "utf8"); + } catch { + // File probably doesn't exist yet. + } + + for (const { key, value } of overrides) { + settingsContent = jsonc.applyEdits( + settingsContent, + jsonc.modify(settingsContent, [key], value, {}), + ); + } + + try { + await fs.writeFile(settingsFilePath, settingsContent); + return true; + } catch (ex) { + // Could be read-only (e.g. home-manager on NixOS). Not catastrophic. + logger.warn("Failed to configure settings", ex); + return false; + } +} diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 631a1282..9cc093f4 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -391,6 +391,7 @@ export function createMockLogger(): Logger { info: vi.fn(), warn: vi.fn(), error: vi.fn(), + show: vi.fn(), }; } diff --git a/test/unit/error/serverCertificateError.test.ts b/test/unit/error/serverCertificateError.test.ts index 27710d4b..622ab405 100644 --- a/test/unit/error/serverCertificateError.test.ts +++ b/test/unit/error/serverCertificateError.test.ts @@ -40,6 +40,7 @@ describe("Certificate errors", () => { info: throwingLog, warn: throwingLog, error: throwingLog, + show: () => {}, }; const disposers: Array<() => void> = []; diff --git a/test/unit/remote/userSettings.test.ts b/test/unit/remote/userSettings.test.ts new file mode 100644 index 00000000..60761c3d --- /dev/null +++ b/test/unit/remote/userSettings.test.ts @@ -0,0 +1,264 @@ +import { vol } from "memfs"; +import * as fsPromises from "node:fs/promises"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { + applySettingOverrides, + buildSshOverrides, +} from "@/remote/userSettings"; + +import { + MockConfigurationProvider, + createMockLogger, +} from "../../mocks/testHelpers"; + +import type * as fs from "node:fs"; + +vi.mock("node:fs/promises", async () => { + const memfs: { fs: typeof fs } = await vi.importActual("memfs"); + return memfs.fs.promises; +}); + +/** Helper to extract a single override by key from the result. */ +function findOverride( + overrides: Array<{ key: string; value: unknown }>, + key: string, +): unknown { + return overrides.find((o) => o.key === key)?.value; +} + +interface TimeoutCase { + timeout: number | undefined; + label: string; +} + +describe("buildSshOverrides", () => { + describe("remote platform", () => { + it("adds host when missing or OS differs", () => { + const config = new MockConfigurationProvider(); + + // New host is added alongside existing entries. + config.set("remote.SSH.remotePlatform", { "other-host": "darwin" }); + expect( + findOverride( + buildSshOverrides(config, "new-host", "linux"), + "remote.SSH.remotePlatform", + ), + ).toEqual({ "other-host": "darwin", "new-host": "linux" }); + + // Existing host with wrong OS gets corrected. + config.set("remote.SSH.remotePlatform", { "my-host": "windows" }); + expect( + findOverride( + buildSshOverrides(config, "my-host", "linux"), + "remote.SSH.remotePlatform", + ), + ).toEqual({ "my-host": "linux" }); + }); + + it("skips override when host already matches", () => { + const config = new MockConfigurationProvider(); + config.set("remote.SSH.remotePlatform", { "my-host": "linux" }); + expect( + findOverride( + buildSshOverrides(config, "my-host", "linux"), + "remote.SSH.remotePlatform", + ), + ).toBeUndefined(); + }); + }); + + describe("connect timeout", () => { + it.each([ + { timeout: undefined, label: "unset" }, + { timeout: 0, label: "zero" }, + { timeout: 15, label: "below minimum" }, + { timeout: 1799, label: "just under minimum" }, + ])("enforces minimum of 1800 when $label", ({ timeout }) => { + const config = new MockConfigurationProvider(); + if (timeout !== undefined) { + config.set("remote.SSH.connectTimeout", timeout); + } + expect( + findOverride( + buildSshOverrides(config, "host", "linux"), + "remote.SSH.connectTimeout", + ), + ).toBe(1800); + }); + + it.each([ + { timeout: 1800, label: "exactly minimum" }, + { timeout: 3600, label: "above minimum" }, + ])("preserves timeout when $label", ({ timeout }) => { + const config = new MockConfigurationProvider(); + config.set("remote.SSH.connectTimeout", timeout); + expect( + findOverride( + buildSshOverrides(config, "host", "linux"), + "remote.SSH.connectTimeout", + ), + ).toBeUndefined(); + }); + }); + + describe("reconnection grace time", () => { + it("defaults to 8 hours when not configured", () => { + expect( + findOverride( + buildSshOverrides(new MockConfigurationProvider(), "host", "linux"), + "remote.SSH.reconnectionGraceTime", + ), + ).toBe(28800); + }); + + it("preserves any user-configured value", () => { + const config = new MockConfigurationProvider(); + config.set("remote.SSH.reconnectionGraceTime", 3600); + expect( + findOverride( + buildSshOverrides(config, "host", "linux"), + "remote.SSH.reconnectionGraceTime", + ), + ).toBeUndefined(); + }); + }); + + it.each([ + { key: "remote.SSH.serverShutdownTimeout", expected: 28800 }, + { key: "remote.SSH.maxReconnectionAttempts", expected: null }, + ])("defaults $key when not configured", ({ key, expected }) => { + const overrides = buildSshOverrides( + new MockConfigurationProvider(), + "host", + "linux", + ); + expect(findOverride(overrides, key)).toBe(expected); + }); + + it("produces no overrides when all settings are already correct", () => { + const config = new MockConfigurationProvider(); + config.set("remote.SSH.remotePlatform", { "my-host": "linux" }); + config.set("remote.SSH.connectTimeout", 3600); + config.set("remote.SSH.reconnectionGraceTime", 7200); + config.set("remote.SSH.serverShutdownTimeout", 600); + config.set("remote.SSH.maxReconnectionAttempts", 4); + expect(buildSshOverrides(config, "my-host", "linux")).toHaveLength(0); + }); +}); + +describe("applySettingOverrides", () => { + const settingsPath = "/settings.json"; + const logger = createMockLogger(); + + beforeEach(() => { + vol.reset(); + }); + + async function readSettings(): Promise> { + const raw = await fsPromises.readFile(settingsPath, "utf8"); + return JSON.parse(raw) as Record; + } + + it("returns true when overrides list is empty", async () => { + expect(await applySettingOverrides(settingsPath, [], logger)).toBe(true); + }); + + it("creates file and applies overrides when file does not exist", async () => { + const ok = await applySettingOverrides( + settingsPath, + [ + { + key: "remote.SSH.remotePlatform", + value: { "coder-gke--main": "linux" }, + }, + { key: "remote.SSH.connectTimeout", value: 1800 }, + { key: "remote.SSH.reconnectionGraceTime", value: 28800 }, + ], + logger, + ); + + expect(ok).toBe(true); + expect(await readSettings()).toMatchObject({ + "remote.SSH.remotePlatform": { "coder-gke--main": "linux" }, + "remote.SSH.connectTimeout": 1800, + "remote.SSH.reconnectionGraceTime": 28800, + }); + }); + + it("preserves existing settings when applying overrides", async () => { + vol.fromJSON({ + [settingsPath]: JSON.stringify({ + "remote.SSH.remotePlatform": { "coder-gke--main": "linux" }, + "remote.SSH.connectTimeout": 15, + }), + }); + + await applySettingOverrides( + settingsPath, + [{ key: "remote.SSH.connectTimeout", value: 1800 }], + logger, + ); + + expect(await readSettings()).toMatchObject({ + "remote.SSH.remotePlatform": { "coder-gke--main": "linux" }, + "remote.SSH.connectTimeout": 1800, + }); + }); + + it("handles JSONC with comments", async () => { + vol.fromJSON({ + [settingsPath]: [ + "{", + " // Platform overrides for remote SSH hosts", + ' "remote.SSH.remotePlatform": { "coder-gke--main": "linux" },', + ' "remote.SSH.connectTimeout": 15', + "}", + ].join("\n"), + }); + + await applySettingOverrides( + settingsPath, + [{ key: "remote.SSH.connectTimeout", value: 1800 }], + logger, + ); + + const raw = await fsPromises.readFile(settingsPath, "utf8"); + expect(raw).toContain("// Platform overrides for remote SSH hosts"); + expect(raw).toContain("1800"); + expect(raw).toContain('"remote.SSH.remotePlatform"'); + }); + + it("writes null values literally instead of deleting the key", async () => { + const ok = await applySettingOverrides( + settingsPath, + [{ key: "remote.SSH.maxReconnectionAttempts", value: null }], + logger, + ); + + expect(ok).toBe(true); + const raw = await fsPromises.readFile(settingsPath, "utf8"); + expect(raw).toContain('"remote.SSH.maxReconnectionAttempts": null'); + }); + + it("returns false and logs warning when write fails", async () => { + vol.fromJSON({ [settingsPath]: "{}" }); + const writeSpy = vi + .spyOn(fsPromises, "writeFile") + .mockRejectedValueOnce(new Error("EACCES: permission denied")); + + const ok = await applySettingOverrides( + settingsPath, + [{ key: "remote.SSH.connectTimeout", value: 1800 }], + logger, + ); + + expect(ok).toBe(false); + expect(logger.warn).toHaveBeenCalledWith( + "Failed to configure settings", + expect.anything(), + ); + + writeSpy.mockRestore(); + }); +});