From 82717c225a3f50c43a5b885175793859992577b7 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 17 Jan 2023 12:48:46 -0700 Subject: [PATCH 01/11] refactor: remove version from binaryPath This removes the version from the binary path to ensure that the path always remains the same on OS's. This way, tools like Windows Firewall and macOS Firewill will only prompt for a security check on the first install but not subsequent installs. --- src/storage.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index fe3c67ea..3daf3cee 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -80,7 +80,7 @@ export class Storage { const baseURI = vscode.Uri.parse(baseURL) const buildInfo = await getBuildInfo() - const binPath = this.binaryPath(buildInfo.version) + const binPath = this.binaryPath() const exists = await fs .stat(binPath) .then(() => true) @@ -264,10 +264,10 @@ export class Storage { } } - private binaryPath(version: string): string { + private binaryPath(): string { const os = goos() const arch = goarch() - let binPath = path.join(this.getBinaryCachePath(), `coder-${os}-${arch}-${version}`) + let binPath = path.join(this.getBinaryCachePath(), `coder-${os}-${arch}`) if (os === "windows") { binPath += ".exe" } From 197385a67f3cea0ecfd177b5a9f34f0836edeaef Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 17 Jan 2023 14:02:32 -0700 Subject: [PATCH 02/11] refactor: update logic for downloading binary This makes significant changes to the `fetchBinary` logic. First, it refactors a couple pieces of logic into methods on the `Storage` class to make the code more readable. Then it modifies the flow to first check if the binary is outdated. If it is, then it downloads the latest version. --- src/storage.ts | 89 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 25 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index 3daf3cee..ba3b88f6 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -1,6 +1,7 @@ import axios from "axios" import { execFile } from "child_process" import { getBuildInfo } from "coder/site/src/api/api" +import * as crypto from "crypto" import { createWriteStream } from "fs" import { ensureDir } from "fs-extra" import fs from "fs/promises" @@ -81,31 +82,7 @@ export class Storage { const buildInfo = await getBuildInfo() const binPath = this.binaryPath() - const exists = await fs - .stat(binPath) - .then(() => true) - .catch(() => false) - if (exists) { - // Even if the file exists, it could be corrupted. - // We run `coder version` to ensure the binary can be executed. - this.output.appendLine(`Using cached binary: ${binPath}`) - const valid = await new Promise((resolve) => { - try { - execFile(binPath, ["version"], (err) => { - if (err) { - this.output.appendLine("Check for binary corruption: " + err) - } - resolve(err === null) - }) - } catch (ex) { - this.output.appendLine("The cached binary cannot be executed: " + ex) - resolve(false) - } - }) - if (valid) { - return binPath - } - } + const exists = await this.checkBinaryExists(binPath) const os = goos() const arch = goarch() let binName = `coder-${os}-${arch}` @@ -114,6 +91,23 @@ export class Storage { binName += ".exe" } const controller = new AbortController() + + if (exists) { + this.output.appendLine(`Checking if binary outdated...`) + const outdated = await this.checkBinaryOutdated(binName, baseURL, controller) + // If it's outdated, we fall through to the download logic. + if (outdated) { + this.output.appendLine(`Found outdated version.`) + } else { + // Even if the file exists, it could be corrupted. + // We run `coder version` to ensure the binary can be executed. + this.output.appendLine(`Using existing binary: ${binPath}`) + const valid = await this.checkBinaryValid(binPath) + if (valid) { + return binPath + } + } + } const resp = await axios.get("/bin/" + binName, { signal: controller.signal, baseURL: baseURL, @@ -240,6 +234,10 @@ export class Storage { return path.join(this.globalStorageUri.fsPath, "url") } + public getBinaryETag(): string { + return crypto.createHash("sha1").update(this.binaryPath()).digest("hex") + } + private appDataDir(): string { switch (process.platform) { case "darwin": @@ -274,6 +272,47 @@ export class Storage { return binPath } + private async checkBinaryExists(binPath: string): Promise { + return await fs + .stat(binPath) + .then(() => true) + .catch(() => false) + } + + private async checkBinaryValid(binPath: string): Promise { + return await new Promise((resolve) => { + try { + execFile(binPath, ["version"], (err) => { + if (err) { + this.output.appendLine("Check for binary corruption: " + err) + } + resolve(err === null) + }) + } catch (ex) { + this.output.appendLine("The cached binary cannot be executed: " + ex) + resolve(false) + } + }) + } + + private async checkBinaryOutdated(binName: string, baseURL: string, controller: AbortController): Promise { + const resp = await axios.get("/bin/" + binName, { + signal: controller.signal, + baseURL: baseURL, + headers: { + "If-None-Match": this.getBinaryETag(), + }, + }) + + switch (resp.status) { + case 200: + return true + case 304: + default: + return false + } + } + private async updateSessionToken() { const token = await this.getSessionToken() if (token) { From 400ba7a657790236451bea256158c04864d58421 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 17 Jan 2023 15:18:51 -0700 Subject: [PATCH 03/11] refactor: only make request once --- src/storage.ts | 64 ++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index ba3b88f6..ea096cc9 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -93,18 +93,13 @@ export class Storage { const controller = new AbortController() if (exists) { - this.output.appendLine(`Checking if binary outdated...`) - const outdated = await this.checkBinaryOutdated(binName, baseURL, controller) - // If it's outdated, we fall through to the download logic. - if (outdated) { - this.output.appendLine(`Found outdated version.`) - } else { - // Even if the file exists, it could be corrupted. - // We run `coder version` to ensure the binary can be executed. - this.output.appendLine(`Using existing binary: ${binPath}`) - const valid = await this.checkBinaryValid(binPath) - if (valid) { - return binPath + this.output.appendLine(`Found existing binary: ${binPath}`) + const valid = await this.checkBinaryValid(binPath) + if (!valid) { + const removed = await this.rmBinary(binPath) + if (!removed) { + vscode.window.showErrorMessage("Failed to remove existing binary!") + return undefined } } } @@ -138,15 +133,12 @@ export class Storage { }) return } - if (resp.status !== 200) { - vscode.window.showErrorMessage("Failed to fetch the Coder binary: " + resp.statusText) - return - } const contentLength = Number.parseInt(resp.headers["content-length"]) // Ensure the binary directory exists! await fs.mkdir(path.dirname(binPath), { recursive: true }) + const tempFile = binPath + ".temp-" + Math.random().toString(36).substring(8) const completed = await vscode.window.withProgress( { @@ -169,7 +161,7 @@ export class Storage { contentLengthPretty = " / " + prettyBytes(contentLength) } - const writeStream = createWriteStream(binPath, { + const writeStream = createWriteStream(tempFile, { autoClose: true, mode: 0o755, }) @@ -205,8 +197,19 @@ export class Storage { if (!completed) { return } + if (resp.status === 200) { + this.output.appendLine(`Downloaded binary: ${binPath}`) + await fs.rename(tempFile, binPath) + return binPath + } + await fs.rm(tempFile) + + if (resp.status !== 304) { + vscode.window.showErrorMessage("Failed to fetch the Coder binary: " + resp.statusText) + return undefined + } - this.output.appendLine(`Downloaded binary: ${binPath}`) + this.output.appendLine(`Using cached binary: ${binPath}`) return binPath } @@ -279,6 +282,13 @@ export class Storage { .catch(() => false) } + private async rmBinary(binPath: string): Promise { + return await fs + .rm(binPath, { force: true }) + .then(() => true) + .catch(() => false) + } + private async checkBinaryValid(binPath: string): Promise { return await new Promise((resolve) => { try { @@ -295,24 +305,6 @@ export class Storage { }) } - private async checkBinaryOutdated(binName: string, baseURL: string, controller: AbortController): Promise { - const resp = await axios.get("/bin/" + binName, { - signal: controller.signal, - baseURL: baseURL, - headers: { - "If-None-Match": this.getBinaryETag(), - }, - }) - - switch (resp.status) { - case 200: - return true - case 304: - default: - return false - } - } - private async updateSessionToken() { const token = await this.getSessionToken() if (token) { From 369a8236bdf5e1aa8711fb037c2f28b7709046bd Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 18 Jan 2023 09:07:13 -0700 Subject: [PATCH 04/11] refactor: switch on status to avoid piping --- src/storage.ts | 190 ++++++++++++++++++++++++++----------------------- 1 file changed, 102 insertions(+), 88 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index ea096cc9..880a4f04 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -114,103 +114,117 @@ export class Storage { // Ignore all errors so we can catch a 404! validateStatus: () => true, }) - if (resp.status === 404) { - vscode.window - .showErrorMessage( - "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", - "Open an Issue", - ) - .then((value) => { - if (!value) { - return - } - const params = new URLSearchParams({ - title: `Support the \`${os}-${arch}\` platform`, - body: `I'd like to use the \`${os}-${arch}\` architecture with the VS Code extension.`, - }) - const uri = vscode.Uri.parse(`https://github.com/coder/vscode-coder/issues/new?` + params.toString()) - vscode.env.openExternal(uri) - }) - return - } - - const contentLength = Number.parseInt(resp.headers["content-length"]) - // Ensure the binary directory exists! - await fs.mkdir(path.dirname(binPath), { recursive: true }) - const tempFile = binPath + ".temp-" + Math.random().toString(36).substring(8) + switch (resp.status) { + case 200: { + const contentLength = Number.parseInt(resp.headers["content-length"]) + + // Ensure the binary directory exists! + await fs.mkdir(path.dirname(binPath), { recursive: true }) + const tempFile = binPath + ".temp-" + Math.random().toString(36).substring(8) + + const completed = await vscode.window.withProgress( + { + location: vscode.ProgressLocation.Notification, + title: `Downloading the latest binary (${buildInfo.version} from ${baseURI.authority})`, + cancellable: true, + }, + async (progress, token) => { + const readStream = resp.data as IncomingMessage + let cancelled = false + token.onCancellationRequested(() => { + controller.abort() + readStream.destroy() + cancelled = true + }) - const completed = await vscode.window.withProgress( - { - location: vscode.ProgressLocation.Notification, - title: `Downloading the latest binary (${buildInfo.version} from ${baseURI.authority})`, - cancellable: true, - }, - async (progress, token) => { - const readStream = resp.data as IncomingMessage - let cancelled = false - token.onCancellationRequested(() => { - controller.abort() - readStream.destroy() - cancelled = true - }) + let contentLengthPretty = "" + // Reverse proxies might not always send a content length! + if (!Number.isNaN(contentLength)) { + contentLengthPretty = " / " + prettyBytes(contentLength) + } - let contentLengthPretty = "" - // Reverse proxies might not always send a content length! - if (!Number.isNaN(contentLength)) { - contentLengthPretty = " / " + prettyBytes(contentLength) + const writeStream = createWriteStream(tempFile, { + autoClose: true, + mode: 0o755, + }) + let written = 0 + readStream.on("data", (buffer: Buffer) => { + writeStream.write(buffer, () => { + written += buffer.byteLength + progress.report({ + message: `${prettyBytes(written)}${contentLengthPretty}`, + increment: (buffer.byteLength / contentLength) * 100, + }) + }) + }) + try { + await new Promise((resolve, reject) => { + readStream.on("error", (err) => { + reject(err) + }) + readStream.on("close", () => { + if (cancelled) { + return reject() + } + writeStream.close() + resolve() + }) + }) + return true + } catch (ex) { + return false + } + }, + ) + if (!completed) { + return } - - const writeStream = createWriteStream(tempFile, { - autoClose: true, - mode: 0o755, - }) - let written = 0 - readStream.on("data", (buffer: Buffer) => { - writeStream.write(buffer, () => { - written += buffer.byteLength - progress.report({ - message: `${prettyBytes(written)}${contentLengthPretty}`, - increment: (buffer.byteLength / contentLength) * 100, + this.output.appendLine(`Downloaded binary: ${binPath}`) + await fs.rename(tempFile, binPath) + await fs.rm(tempFile) + return binPath + } + case 304: { + this.output.appendLine(`Using cached binary: ${binPath}`) + return binPath + } + case 404: { + vscode.window + .showErrorMessage( + "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", + "Open an Issue", + ) + .then((value) => { + if (!value) { + return + } + const params = new URLSearchParams({ + title: `Support the \`${os}-${arch}\` platform`, + body: `I'd like to use the \`${os}-${arch}\` architecture with the VS Code extension.`, }) + const uri = vscode.Uri.parse(`https://github.com/coder/vscode-coder/issues/new?` + params.toString()) + vscode.env.openExternal(uri) }) - }) - try { - await new Promise((resolve, reject) => { - readStream.on("error", (err) => { - reject(err) - }) - readStream.on("close", () => { - if (cancelled) { - return reject() - } - writeStream.close() - resolve() + return undefined + } + default: { + vscode.window + .showErrorMessage("Failed to download binary. Please open an issue.", "Open an Issue") + .then((value) => { + if (!value) { + return + } + const params = new URLSearchParams({ + title: `Failed to download binary on \`${os}-${arch}\``, + body: `Received status code \`${resp.status}\` when downloading the binary.`, }) + const uri = vscode.Uri.parse(`https://github.com/coder/vscode-coder/issues/new?` + params.toString()) + vscode.env.openExternal(uri) }) - return true - } catch (ex) { - return false - } - }, - ) - if (!completed) { - return - } - if (resp.status === 200) { - this.output.appendLine(`Downloaded binary: ${binPath}`) - await fs.rename(tempFile, binPath) - return binPath - } - await fs.rm(tempFile) - - if (resp.status !== 304) { - vscode.window.showErrorMessage("Failed to fetch the Coder binary: " + resp.statusText) - return undefined + return undefined + } } - - this.output.appendLine(`Using cached binary: ${binPath}`) - return binPath } // getBinaryCachePath returns the path where binaries are cached. From 923225bf5baa409bd73ed6d6fdd32f5b9341c4c0 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 18 Jan 2023 10:42:29 -0700 Subject: [PATCH 05/11] refactor: delete rm(tempFile) --- src/storage.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/storage.ts b/src/storage.ts index 880a4f04..4ec4af3f 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -182,7 +182,6 @@ export class Storage { } this.output.appendLine(`Downloaded binary: ${binPath}`) await fs.rename(tempFile, binPath) - await fs.rm(tempFile) return binPath } case 304: { From 0a0b0414fd60f1b361d760a375123891070c0df9 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 18 Jan 2023 10:43:24 -0700 Subject: [PATCH 06/11] chore: add package:prerelease script This will build a `.vsix` file locally marked as a pre-release which is handy for testing. --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 964397e3..ecc4cd5c 100644 --- a/package.json +++ b/package.json @@ -85,6 +85,7 @@ "build": "webpack", "watch": "webpack --watch", "package": "webpack --mode production --devtool hidden-source-map", + "package:prerelease": "npx vsce package --pre-release", "lint": "eslint . --ext ts,md", "tsc:compile": "tsc", "tsc:watch": "tsc -w", From eafac660ceca1cda66692bc80d5abc02586516a0 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 19 Jan 2023 11:05:12 -0700 Subject: [PATCH 07/11] feat: mv binPath to old before downloading new This makes a couple new changes: - if 200, move old binPath to binPath.old- - try removing .old binary - on start, cleanUpOldBinaries --- src/storage.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/storage.ts b/src/storage.ts index 4ec4af3f..ae08a0f5 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -74,6 +74,7 @@ export class Storage { // fetchBinary returns the path to a Coder binary. // The binary will be cached if a matching server version already exists. public async fetchBinary(): Promise { + await this.cleanUpOldBinaries() const baseURL = this.getURL() if (!baseURL) { throw new Error("Must be logged in!") @@ -122,6 +123,10 @@ export class Storage { // Ensure the binary directory exists! await fs.mkdir(path.dirname(binPath), { recursive: true }) const tempFile = binPath + ".temp-" + Math.random().toString(36).substring(8) + const oldBinPath = binPath + ".old-" + Math.random().toString(36).substring(8) + await fs.rename(binPath, oldBinPath).catch(() => { + this.output.appendLine(`Warning: failed to rename ${binPath} to ${oldBinPath}`) + }) const completed = await vscode.window.withProgress( { @@ -182,6 +187,9 @@ export class Storage { } this.output.appendLine(`Downloaded binary: ${binPath}`) await fs.rename(tempFile, binPath) + await fs.rm(oldBinPath, { force: true }).catch((error) => { + this.output.appendLine(`Warning: failed to remove old binary: ${error}`) + }) return binPath } case 304: { @@ -278,6 +286,18 @@ export class Storage { } } + private async cleanUpOldBinaries(): Promise { + const binPath = this.binaryPath() + const binDir = path.dirname(binPath) + const files = await fs.readdir(binDir) + for (const file of files) { + const fileName = path.basename(file) + if (fileName.includes(".old-")) { + await fs.rm(path.join(binDir, file), { force: true }) + } + } + } + private binaryPath(): string { const os = goos() const arch = goarch() From 6f6adb7d7ba6859aa5b28e43715495c5554793fe Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 19 Jan 2023 11:49:04 -0700 Subject: [PATCH 08/11] refactor: move and catch things - catch all potential errors from `fs` - only rename binPath to old right before replacing with tempFile --- src/storage.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index ae08a0f5..181ee906 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -123,10 +123,6 @@ export class Storage { // Ensure the binary directory exists! await fs.mkdir(path.dirname(binPath), { recursive: true }) const tempFile = binPath + ".temp-" + Math.random().toString(36).substring(8) - const oldBinPath = binPath + ".old-" + Math.random().toString(36).substring(8) - await fs.rename(binPath, oldBinPath).catch(() => { - this.output.appendLine(`Warning: failed to rename ${binPath} to ${oldBinPath}`) - }) const completed = await vscode.window.withProgress( { @@ -186,6 +182,10 @@ export class Storage { return } this.output.appendLine(`Downloaded binary: ${binPath}`) + const oldBinPath = binPath + ".old-" + Math.random().toString(36).substring(8) + await fs.rename(binPath, oldBinPath).catch(() => { + this.output.appendLine(`Warning: failed to rename ${binPath} to ${oldBinPath}`) + }) await fs.rename(tempFile, binPath) await fs.rm(oldBinPath, { force: true }).catch((error) => { this.output.appendLine(`Warning: failed to remove old binary: ${error}`) @@ -293,7 +293,11 @@ export class Storage { for (const file of files) { const fileName = path.basename(file) if (fileName.includes(".old-")) { - await fs.rm(path.join(binDir, file), { force: true }) + try { + await fs.rm(path.join(binDir, file), { force: true }) + } catch (error) { + this.output.appendLine(`Warning: failed to remove ${fileName}. Error: ${error}`) + } } } } From 7a316631b6604f5985f38e45f3808bd4bd189554 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 20 Jan 2023 10:18:13 -0700 Subject: [PATCH 09/11] refactor: generate Etag correctly, add to request This generates the Etag correctly using a readableStream and the appends it to the headers. This also adds some logging around etag, binPath and status code. --- src/storage.ts | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index 181ee906..b11db17e 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -2,7 +2,7 @@ import axios from "axios" import { execFile } from "child_process" import { getBuildInfo } from "coder/site/src/api/api" import * as crypto from "crypto" -import { createWriteStream } from "fs" +import { createWriteStream, createReadStream } from "fs" import { ensureDir } from "fs-extra" import fs from "fs/promises" import { IncomingMessage } from "http" @@ -104,17 +104,23 @@ export class Storage { } } } + const etag = await this.getBinaryETag() + this.output.appendLine(`Using binPath: ${binPath}`) + this.output.appendLine(`Using ETag: ${etag}`) + const resp = await axios.get("/bin/" + binName, { signal: controller.signal, baseURL: baseURL, responseType: "stream", headers: { "Accept-Encoding": "gzip", + "If-None-Match": `"${await this.getBinaryETag()}"`, }, decompress: true, // Ignore all errors so we can catch a 404! validateStatus: () => true, }) + this.output.appendLine("Response status code: " + resp.status) switch (resp.status) { case 200: { @@ -258,8 +264,21 @@ export class Storage { return path.join(this.globalStorageUri.fsPath, "url") } - public getBinaryETag(): string { - return crypto.createHash("sha1").update(this.binaryPath()).digest("hex") + public getBinaryETag(): Promise { + const hash = crypto.createHash("sha1") + const stream = createReadStream(this.binaryPath()) + return new Promise((resolve, reject) => { + stream.on("end", () => { + hash.end() + resolve(hash.digest("hex")) + }) + stream.on("error", (err) => { + reject(err) + }) + stream.on("data", (chunk) => { + hash.update(chunk) + }) + }) } private appDataDir(): string { From 5528dc863cdafb465fe368a41beb75e8cfa7a79f Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 20 Jan 2023 11:01:05 -0700 Subject: [PATCH 10/11] Update src/storage.ts Co-authored-by: Dean Sheather --- src/storage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage.ts b/src/storage.ts index b11db17e..0373eac1 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -114,7 +114,7 @@ export class Storage { responseType: "stream", headers: { "Accept-Encoding": "gzip", - "If-None-Match": `"${await this.getBinaryETag()}"`, + "If-None-Match": `"${etag}"`, }, decompress: true, // Ignore all errors so we can catch a 404! From db53aaafb6bd0a5785cd41c1b0d6fa2f8d23f7f1 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 20 Jan 2023 11:01:17 -0700 Subject: [PATCH 11/11] Update src/storage.ts Co-authored-by: Dean Sheather --- src/storage.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage.ts b/src/storage.ts index 0373eac1..0e1a09e9 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -105,6 +105,7 @@ export class Storage { } } const etag = await this.getBinaryETag() + this.output.appendLine(`Using binName: ${binName}`) this.output.appendLine(`Using binPath: ${binPath}`) this.output.appendLine(`Using ETag: ${etag}`)