diff --git a/package.json b/package.json index e37f1ef8..7d006937 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..99010e43 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -24,6 +24,7 @@ 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 } from "./remote/userSettings"; import { escapeCommandArg, toRemoteAuthority, toSafeHost } from "./util"; import { vscodeProposed } from "./vscodeProposed"; import { @@ -310,6 +311,25 @@ export class Commands { } } + /** + * Apply recommended SSH settings for reliable Coder workspace connections. + */ + public async applyRecommendedSettings(): Promise { + const config = vscode.workspace.getConfiguration(); + const entries = Object.entries(RECOMMENDED_SSH_SETTINGS); + for (const [key, setting] of entries) { + await config.update( + key, + setting.value, + vscode.ConfigurationTarget.Global, + ); + } + const summary = entries.map(([, s]) => s.label).join(", "); + vscode.window.showInformationMessage( + `Applied recommended SSH settings: ${summary}`, + ); + } + /** * 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/remote/remote.ts b/src/remote/remote.ts index ac7c99ca..ab53ea6e 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,85 +459,17 @@ 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( - 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, - {}, - ), - ); - 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); - } - } + const overrides = buildSshOverrides( + vscodeProposed.workspace.getConfiguration(), + parts.sshHost, + agent.operating_system, + ); + await applySettingOverrides( + this.pathResolver.getUserSettingsPath(), + overrides, + this.logger, + ); const logDir = this.getLogDir(featureSet); diff --git a/src/remote/userSettings.ts b/src/remote/userSettings.ts new file mode 100644 index 00000000..6defd7ef --- /dev/null +++ b/src/remote/userSettings.ts @@ -0,0 +1,124 @@ +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; +} + +export const RECOMMENDED_SSH_SETTINGS = { + "remote.SSH.connectTimeout": { + value: 1800, + label: "Connect Timeout: 1800s (30 min)", + }, + "remote.SSH.reconnectionGraceTime": { + value: 28800, + label: "Reconnection Grace Time: 28800s (8 hours)", + }, + "remote.SSH.serverShutdownTimeout": { + value: 28800, + label: "Server Shutdown Timeout: 28800s (8 hours)", + }, + "remote.SSH.maxReconnectionAttempts": { + value: null, + label: "Max Reconnection Attempts: max allowed", + }, +} as const satisfies Record; + +/** + * 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 minConnTimeout = + RECOMMENDED_SSH_SETTINGS["remote.SSH.connectTimeout"].value; + const connTimeout = config.get("remote.SSH.connectTimeout"); + if (!connTimeout || connTimeout < minConnTimeout) { + overrides.push({ + key: "remote.SSH.connectTimeout", + value: minConnTimeout, + }); + } + + // Set recommended defaults for settings the user hasn't configured. + const setIfUndefined = [ + "remote.SSH.reconnectionGraceTime", + "remote.SSH.serverShutdownTimeout", + "remote.SSH.maxReconnectionAttempts", + ] as const; + for (const key of setIfUndefined) { + if (config.get(key) === undefined) { + overrides.push({ key, value: RECOMMENDED_SSH_SETTINGS[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 false; + } + + 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/unit/remote/userSettings.test.ts b/test/unit/remote/userSettings.test.ts new file mode 100644 index 00000000..59192432 --- /dev/null +++ b/test/unit/remote/userSettings.test.ts @@ -0,0 +1,252 @@ +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 false when overrides list is empty", async () => { + expect(await applySettingOverrides(settingsPath, [], logger)).toBe(false); + }); + + 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("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(); + }); +});