From ea7ac6e015d2162c5cb6ecf8b4bb81351cffcf43 Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 26 Jan 2024 13:45:56 -0900 Subject: [PATCH 1/4] Override connect timeout `parsed` is not used anywhere so I deleted it. --- src/remote.ts | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/remote.ts b/src/remote.ts index 5561aca3..e6030db2 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -273,17 +273,30 @@ export class Remote { [`${authorityParts[1]}`]: agent.operating_system, } + // 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. + const connectTimeout = 1800 + let settingsContent = "{}" try { settingsContent = await fs.readFile(this.storage.getUserSettingsPath(), "utf8") } catch (ex) { // Ignore! It's probably because the file doesn't exist. } - const parsed = jsonc.parse(settingsContent) - parsed["remote.SSH.remotePlatform"] = remotePlatforms - const edits = jsonc.modify(settingsContent, ["remote.SSH.remotePlatform"], remotePlatforms, {}) + + settingsContent = jsonc.applyEdits( + settingsContent, + jsonc.modify(settingsContent, ["remote.SSH.remotePlatform"], remotePlatforms, {}), + ) + + settingsContent = jsonc.applyEdits( + settingsContent, + jsonc.modify(settingsContent, ["remote.SSH.connectTimeout"], connectTimeout, {}), + ) + try { - await fs.writeFile(this.storage.getUserSettingsPath(), jsonc.applyEdits(settingsContent, edits)) + await fs.writeFile(this.storage.getUserSettingsPath(), settingsContent) } catch (ex) { // The user will just be prompted instead, which is fine! // If a user's settings.json is read-only, then we can't write to it. From 7b8e0c700da08230871cdcc5175f07c29d3ee1ae Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 30 Jan 2024 08:10:13 -0900 Subject: [PATCH 2/4] Add upstream issue link --- src/remote.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/remote.ts b/src/remote.ts index e6030db2..3e9dc840 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -276,6 +276,7 @@ export class Remote { // 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. + // If microsoft/vscode-remote-release#8519 is resolved we can remove this. const connectTimeout = 1800 let settingsContent = "{}" From 30620abd2fd4fc0b890205d507d945467a03dc7c Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 30 Jan 2024 10:25:40 -0900 Subject: [PATCH 3/4] Reset connect timeout after connecting --- src/remote.ts | 93 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 66 insertions(+), 27 deletions(-) diff --git a/src/remote.ts b/src/remote.ts index 3e9dc840..7a27b2c8 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -265,20 +265,17 @@ export class Remote { agent = matchingAgents[0] } - let remotePlatforms = this.vscodeProposed.workspace + const hostname = authorityParts[1] + const remotePlatforms = this.vscodeProposed.workspace .getConfiguration() - .get>("remote.SSH.remotePlatform") - remotePlatforms = { - ...remotePlatforms, - [`${authorityParts[1]}`]: agent.operating_system, - } - - // 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. - // If microsoft/vscode-remote-release#8519 is resolved we can remove this. - const connectTimeout = 1800 - + .get>("remote.SSH.remotePlatform", {}) + const connTimeout = this.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.storage.getUserSettingsPath(), "utf8") @@ -286,22 +283,44 @@ export class Remote { // Ignore! It's probably because the file doesn't exist. } - settingsContent = jsonc.applyEdits( - settingsContent, - jsonc.modify(settingsContent, ["remote.SSH.remotePlatform"], remotePlatforms, {}), - ) + // 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[hostname] || remotePlatforms[hostname] !== agent.operating_system) { + remotePlatforms[hostname] = agent.operating_system + settingsContent = jsonc.applyEdits( + settingsContent, + jsonc.modify(settingsContent, ["remote.SSH.remotePlatform"], remotePlatforms, {}), + ) + mungedPlatforms = true + } - settingsContent = jsonc.applyEdits( - settingsContent, - jsonc.modify(settingsContent, ["remote.SSH.connectTimeout"], connectTimeout, {}), - ) + // 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 but + // for now to mitigate the sketchiness we will reset it after connecting. + const minConnTimeout = 1800 + let mungedConnTimeout = false + if (!connTimeout || connTimeout < minConnTimeout) { + settingsContent = jsonc.applyEdits( + settingsContent, + jsonc.modify(settingsContent, ["remote.SSH.connectTimeout"], minConnTimeout, {}), + ) + mungedConnTimeout = true + } - try { - await fs.writeFile(this.storage.getUserSettingsPath(), settingsContent) - } catch (ex) { - // The user will just be prompted instead, which is fine! - // If a user's settings.json is read-only, then we can't write to it. - // This is the case when using home-manager on NixOS. + if (mungedPlatforms || mungedConnTimeout) { + try { + await fs.writeFile(this.storage.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 + } } const workspaceUpdate = new vscode.EventEmitter() @@ -445,6 +464,26 @@ export class Remote { await this.updateSSHConfig(authorityParts[1], hasCoderLogs) this.findSSHProcessID().then((pid) => { + // Once the SSH process has spawned we can reset the timeout. + if (mungedConnTimeout) { + // Re-read settings in case they changed. + fs.readFile(this.storage.getUserSettingsPath(), "utf8").then(async (rawSettings) => { + try { + await fs.writeFile( + this.storage.getUserSettingsPath(), + jsonc.applyEdits( + rawSettings, + jsonc.modify(rawSettings, ["remote.SSH.connectTimeout"], connTimeout, {}), + ), + ) + } catch (error) { + this.storage.writeToCoderOutputChannel( + `Failed to reset remote.SSH.connectTimeout back to ${connTimeout}: ${error}`, + ) + } + }) + } + if (!pid) { // TODO: Show an error here! return From 8ff655c9d87d20c860e681e9b75db94aed6220ac Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 30 Jan 2024 12:39:35 -0900 Subject: [PATCH 4/4] lint Could have sworn I ran this. --- src/remote.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/remote.ts b/src/remote.ts index 7a27b2c8..dcc19ad1 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -269,7 +269,9 @@ export class Remote { const remotePlatforms = this.vscodeProposed.workspace .getConfiguration() .get>("remote.SSH.remotePlatform", {}) - const connTimeout = this.vscodeProposed.workspace.getConfiguration().get("remote.SSH.connectTimeout") + const connTimeout = this.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 @@ -471,10 +473,7 @@ export class Remote { try { await fs.writeFile( this.storage.getUserSettingsPath(), - jsonc.applyEdits( - rawSettings, - jsonc.modify(rawSettings, ["remote.SSH.connectTimeout"], connTimeout, {}), - ), + jsonc.applyEdits(rawSettings, jsonc.modify(rawSettings, ["remote.SSH.connectTimeout"], connTimeout, {})), ) } catch (error) { this.storage.writeToCoderOutputChannel(