Skip to content

Commit ad63eff

Browse files
EhabYEhabY
committed
feat: set reconnectionGraceTime to 8h, extract settings helper
VS Code's default reconnection grace time of 3 hours is too short for Coder workspaces that go offline overnight. Set 8 hours (28800s) when the user hasn't configured a value. Extract the inline jsonc settings manipulation from remote.ts into a reusable applySettingOverrides() function with dedicated tests.
1 parent 5fc228c commit ad63eff

File tree

3 files changed

+216
-64
lines changed

3 files changed

+216
-64
lines changed

‎src/remote/remote.ts‎

Lines changed: 34 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import {
44
type Workspace,
55
type WorkspaceAgent,
66
} from "coder/site/src/api/typesGenerated";
7-
import * as jsonc from "jsonc-parser";
87
import * as fs from "node:fs/promises";
98
import * as os from "node:os";
109
import * as path from "node:path";
@@ -60,6 +59,7 @@ import {
6059
} from "./sshConfig";
6160
import { SshProcessMonitor } from "./sshProcess";
6261
import { computeSSHProperties, sshSupportsSetEnv } from "./sshSupport";
62+
import { type SettingOverride, applySettingOverrides } from "./userSettings";
6363
import { WorkspaceStateMachine } from "./workspaceStateMachine";
6464

6565
export interface RemoteDetails extends vscode.Disposable {
@@ -474,86 +474,56 @@ export class Remote {
474474
const inbox = await Inbox.create(workspace, workspaceClient, this.logger);
475475
disposables.push(inbox);
476476

477-
// Do some janky setting manipulation.
478477
this.logger.info("Modifying settings...");
479478
const remotePlatforms = vscodeProposed.workspace
480479
.getConfiguration()
481480
.get<Record<string, string>>("remote.SSH.remotePlatform", {});
482481
const connTimeout = vscodeProposed.workspace
483482
.getConfiguration()
484483
.get<number | undefined>("remote.SSH.connectTimeout");
484+
const reconnGraceTime = vscodeProposed.workspace
485+
.getConfiguration()
486+
.get<number | undefined>("remote.SSH.reconnectionGraceTime");
485487

486-
// We have to directly munge the settings file with jsonc because trying to
487-
// update properly through the extension API hangs indefinitely. Possibly
488-
// VS Code is trying to update configuration on the remote, which cannot
489-
// connect until we finish here leading to a deadlock. We need to update it
490-
// locally, anyway, and it does not seem possible to force that via API.
491-
let settingsContent = "{}";
492-
try {
493-
settingsContent = await fs.readFile(
494-
this.pathResolver.getUserSettingsPath(),
495-
"utf8",
496-
);
497-
} catch {
498-
// Ignore! It's probably because the file doesn't exist.
499-
}
488+
const overrides: SettingOverride[] = [];
500489

501-
// Add the remote platform for this host to bypass a step where VS Code asks
502-
// the user for the platform.
503-
let mungedPlatforms = false;
504-
if (
505-
!remotePlatforms[parts.sshHost] ||
506-
remotePlatforms[parts.sshHost] !== agent.operating_system
507-
) {
508-
remotePlatforms[parts.sshHost] = agent.operating_system;
509-
settingsContent = jsonc.applyEdits(
510-
settingsContent,
511-
jsonc.modify(
512-
settingsContent,
513-
["remote.SSH.remotePlatform"],
514-
remotePlatforms,
515-
{},
516-
),
517-
);
518-
mungedPlatforms = true;
490+
// Bypass the platform prompt by setting the remote platform for this host.
491+
if (remotePlatforms[parts.sshHost] !== agent.operating_system) {
492+
overrides.push({
493+
key: "remote.SSH.remotePlatform",
494+
value: {
495+
...remotePlatforms,
496+
[parts.sshHost]: agent.operating_system,
497+
},
498+
});
519499
}
520500

521-
// VS Code ignores the connect timeout in the SSH config and uses a default
522-
// of 15 seconds, which can be too short in the case where we wait for
523-
// startup scripts. For now we hardcode a longer value. Because this is
524-
// potentially overwriting user configuration, it feels a bit sketchy. If
525-
// microsoft/vscode-remote-release#8519 is resolved we can remove this.
501+
// VS Code's default connect timeout of 15s is too short when waiting for
502+
// startup scripts. Enforce a minimum.
526503
const minConnTimeout = 1800;
527-
let mungedConnTimeout = false;
528504
if (!connTimeout || connTimeout < minConnTimeout) {
529-
settingsContent = jsonc.applyEdits(
530-
settingsContent,
531-
jsonc.modify(
532-
settingsContent,
533-
["remote.SSH.connectTimeout"],
534-
minConnTimeout,
535-
{},
536-
),
537-
);
538-
mungedConnTimeout = true;
505+
overrides.push({
506+
key: "remote.SSH.connectTimeout",
507+
value: minConnTimeout,
508+
});
539509
}
540510

541-
if (mungedPlatforms || mungedConnTimeout) {
542-
try {
543-
await fs.writeFile(
544-
this.pathResolver.getUserSettingsPath(),
545-
settingsContent,
546-
);
547-
} catch (ex) {
548-
// This could be because the user's settings.json is read-only. This is
549-
// the case when using home-manager on NixOS, for example. Failure to
550-
// write here is not necessarily catastrophic since the user will be
551-
// asked for the platform and the default timeout might be sufficient.
552-
mungedPlatforms = mungedConnTimeout = false;
553-
this.logger.warn("Failed to configure settings", ex);
554-
}
511+
// VS Code's default reconnection grace time (ProtocolConstants.ReconnectionGraceTime)
512+
// is 3 hours (10800s). Coder workspaces commonly go offline overnight, so we
513+
// bump to 8 hours. See https://github.com/microsoft/vscode/blob/main/src/vs/base/parts/ipc/common/ipc.net.ts
514+
if (reconnGraceTime === undefined) {
515+
overrides.push({
516+
key: "remote.SSH.reconnectionGraceTime",
517+
value: 28800, // 8 hours in seconds
518+
});
555519
}
556520

521+
await applySettingOverrides(
522+
this.pathResolver.getUserSettingsPath(),
523+
overrides,
524+
this.logger,
525+
);
526+
557527
const logDir = this.getLogDir(featureSet);
558528

559529
// This ensures the Remote SSH extension resolves the host to execute the

‎src/remote/userSettings.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import * as jsonc from "jsonc-parser";
2+
import * as fs from "node:fs/promises";
3+
4+
import { type Logger } from "../logging/logger";
5+
6+
export interface SettingOverride {
7+
key: string;
8+
value: unknown;
9+
}
10+
11+
/**
12+
* Apply setting overrides to the user's settings.json file.
13+
*
14+
* We munge the file directly with jsonc instead of using the VS Code API
15+
* because the API hangs indefinitely during remote connection setup (likely
16+
* a deadlock from trying to update config on the not-yet-connected remote).
17+
*/
18+
export async function applySettingOverrides(
19+
settingsFilePath: string,
20+
overrides: SettingOverride[],
21+
logger: Logger,
22+
): Promise<boolean> {
23+
if (overrides.length === 0) {
24+
return false;
25+
}
26+
27+
let settingsContent = "{}";
28+
try {
29+
settingsContent = await fs.readFile(settingsFilePath, "utf8");
30+
} catch {
31+
// File probably doesn't exist yet.
32+
}
33+
34+
for (const { key, value } of overrides) {
35+
settingsContent = jsonc.applyEdits(
36+
settingsContent,
37+
jsonc.modify(settingsContent, [key], value, {}),
38+
);
39+
}
40+
41+
try {
42+
await fs.writeFile(settingsFilePath, settingsContent);
43+
return true;
44+
} catch (ex) {
45+
// Could be read-only (e.g. home-manager on NixOS). Not catastrophic.
46+
logger.warn("Failed to configure settings", ex);
47+
return false;
48+
}
49+
}

‎test/unit/remote/userSettings.test.ts‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
import * as fs from "node:fs/promises";
2+
import * as os from "node:os";
3+
import * as path from "node:path";
4+
import { afterEach, beforeEach, describe, expect, it } from "vitest";
5+
6+
import { applySettingOverrides } from "@/remote/userSettings";
7+
8+
import { createMockLogger } from "../../mocks/testHelpers";
9+
10+
describe("applySettingOverrides", () => {
11+
let tmpDir: string;
12+
let settingsPath: string;
13+
let logger: ReturnType<typeof createMockLogger>;
14+
15+
beforeEach(async () => {
16+
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "userSettings-test-"));
17+
settingsPath = path.join(tmpDir, "settings.json");
18+
logger = createMockLogger();
19+
});
20+
21+
afterEach(async () => {
22+
await fs.rm(tmpDir, { recursive: true, force: true });
23+
});
24+
25+
async function readSettings(): Promise<Record<string, unknown>> {
26+
return JSON.parse(await fs.readFile(settingsPath, "utf8")) as Record<
27+
string,
28+
unknown
29+
>;
30+
}
31+
32+
async function apply(
33+
overrides: Array<{ key: string; value: unknown }>,
34+
opts?: { initialContent?: string; readOnly?: boolean },
35+
): Promise<boolean> {
36+
if (opts?.initialContent !== undefined) {
37+
await fs.writeFile(settingsPath, opts.initialContent);
38+
}
39+
if (opts?.readOnly) {
40+
await fs.chmod(settingsPath, 0o444);
41+
}
42+
return applySettingOverrides(settingsPath, overrides, logger);
43+
}
44+
45+
it("returns false when overrides list is empty", async () => {
46+
expect(await apply([])).toBe(false);
47+
});
48+
49+
it("creates file and applies overrides when file does not exist", async () => {
50+
expect(
51+
await apply([
52+
{ key: "editor.fontSize", value: 14 },
53+
{ key: "editor.tabSize", value: 2 },
54+
]),
55+
).toBe(true);
56+
57+
expect(await readSettings()).toMatchObject({
58+
"editor.fontSize": 14,
59+
"editor.tabSize": 2,
60+
});
61+
});
62+
63+
it("preserves existing settings when applying overrides", async () => {
64+
expect(
65+
await apply([{ key: "editor.fontSize", value: 16 }], {
66+
initialContent: JSON.stringify({
67+
"editor.wordWrap": "on",
68+
"editor.fontSize": 12,
69+
}),
70+
}),
71+
).toBe(true);
72+
73+
expect(await readSettings()).toMatchObject({
74+
"editor.wordWrap": "on",
75+
"editor.fontSize": 16,
76+
});
77+
});
78+
79+
it("handles JSONC with comments", async () => {
80+
const jsonc = [
81+
"{",
82+
" // This is a comment",
83+
' "editor.fontSize": 12,',
84+
' "editor.tabSize": 4',
85+
"}",
86+
].join("\n");
87+
88+
await apply([{ key: "editor.fontSize", value: 18 }], {
89+
initialContent: jsonc,
90+
});
91+
92+
const raw = await fs.readFile(settingsPath, "utf8");
93+
expect(raw).toContain("// This is a comment");
94+
expect(raw).toContain("18");
95+
expect(raw).toContain('"editor.tabSize": 4');
96+
});
97+
98+
it("applies multiple overrides at once", async () => {
99+
expect(
100+
await apply(
101+
[
102+
{ key: "remote.SSH.remotePlatform", value: { myhost: "linux" } },
103+
{ key: "remote.SSH.connectTimeout", value: 1800 },
104+
{ key: "remote.SSH.reconnectionGraceTime", value: 28800 },
105+
],
106+
{ initialContent: "{}" },
107+
),
108+
).toBe(true);
109+
110+
expect(await readSettings()).toMatchObject({
111+
"remote.SSH.remotePlatform": { myhost: "linux" },
112+
"remote.SSH.connectTimeout": 1800,
113+
"remote.SSH.reconnectionGraceTime": 28800,
114+
});
115+
});
116+
117+
it("returns false and logs warning when file is read-only", async () => {
118+
expect(
119+
await apply([{ key: "editor.fontSize", value: 14 }], {
120+
initialContent: "{}",
121+
readOnly: true,
122+
}),
123+
).toBe(false);
124+
125+
expect(logger.warn).toHaveBeenCalledWith(
126+
"Failed to configure settings",
127+
expect.anything(),
128+
);
129+
130+
// Restore permissions for cleanup.
131+
await fs.chmod(settingsPath, 0o644);
132+
});
133+
});

0 commit comments

Comments
 (0)