Merge branch 'main' into renovate/azure-setup-helm-2.x

Author: jsjoeio

src/node/app.ts

@@ -94,7 +94,8 @@ export const ensureAddress = (server: http.Server, protocol: string): URL | stri
   }
 
   if (typeof addr !== "string") {
-    return new URL(`${protocol}://${addr.address}:${addr.port}`)
+    const host = addr.family === "IPv6" ? `[${addr.address}]` : addr.address
+    return new URL(`${protocol}://${host}:${addr.port}`)
   }
 
   // If this is a string then it is a pipe or Unix socket.

src/node/cli.ts

@@ -542,7 +542,7 @@ export async function setDefaults(cliArgs: UserProvidedArgs, configArgs?: Config
     args.password = process.env.PASSWORD
   }
 
-  if (process.env.CS_DISABLE_FILE_DOWNLOADS === "1") {
+  if (process.env.CS_DISABLE_FILE_DOWNLOADS?.match(/^(1|true)$/)) {
     args["disable-file-downloads"] = true
   }
 

src/node/heart.ts

@@ -33,17 +33,7 @@ export class Heart {
     if (typeof this.heartbeatTimer !== "undefined") {
       clearTimeout(this.heartbeatTimer)
     }
-    this.heartbeatTimer = setTimeout(() => {
-      this.isActive()
-        .then((active) => {
-          if (active) {
-            this.beat()
-          }
-        })
-        .catch((error) => {
-          logger.warn(error.message)
-        })
-    }, this.heartbeatInterval)
+    this.heartbeatTimer = setTimeout(() => heartbeatTimer(this.isActive, this.beat), this.heartbeatInterval)
   }
 
   /**
@@ -55,3 +45,20 @@ export class Heart {
     }
   }
 }
+
+/**
+ * Helper function for the heartbeatTimer.
+ *
+ * If heartbeat is active, call beat. Otherwise do nothing.
+ *
+ * Extracted to make it easier to test.
+ */
+export async function heartbeatTimer(isActive: Heart["isActive"], beat: Heart["beat"]) {
+  try {
+    if (await isActive()) {
+      beat()
+    }
+  } catch (error: unknown) {
+    logger.warn((error as Error).message)
+  }
+}

test/e2e/downloads.test.ts

@@ -1,5 +1,5 @@
-import * as path from "path"
 import { promises as fs } from "fs"
+import * as path from "path"
 import { clean } from "../utils/helpers"
 import { describe, test, expect } from "./baseFixture"
 

test/unit/node/app.test.ts

@@ -152,10 +152,20 @@ describe("ensureAddress", () => {
   it("should throw and error if no address", () => {
     expect(() => ensureAddress(mockServer, "http")).toThrow("Server has no address")
   })
-  it("should return the address if it exists", async () => {
-    mockServer.address = () => "http://localhost:8080/"
+  it("should return the address if it's a string", async () => {
+    mockServer.address = () => "/path/to/unix.sock"
     const address = ensureAddress(mockServer, "http")
-    expect(address.toString()).toBe(`http://localhost:8080/`)
+    expect(address.toString()).toBe(`/path/to/unix.sock`)
+  })
+  it("should construct URL with an IPv4 address", async () => {
+    mockServer.address = () => ({ address: "1.2.3.4", port: 5678, family: "IPv4" })
+    const address = ensureAddress(mockServer, "http")
+    expect(address.toString()).toBe(`http://1.2.3.4:5678/`)
+  })
+  it("should construct URL with an IPv6 address", async () => {
+    mockServer.address = () => ({ address: "a:b:c:d::1234", port: 5678, family: "IPv6" })
+    const address = ensureAddress(mockServer, "http")
+    expect(address.toString()).toBe(`http://[a:b:c:d::1234]:5678/`)
   })
 })
 

test/unit/node/cli.test.ts

@@ -362,6 +362,18 @@ describe("parser", () => {
     })
   })
 
+  it("should use env var CS_DISABLE_FILE_DOWNLOADS set to true", async () => {
+    process.env.CS_DISABLE_FILE_DOWNLOADS = "true"
+    const args = parse([])
+    expect(args).toEqual({})
+
+    const defaultArgs = await setDefaults(args)
+    expect(defaultArgs).toEqual({
+      ...defaults,
+      "disable-file-downloads": true,
+    })
+  })
+
   it("should error if password passed in", () => {
     expect(() => parse(["--password", "supersecret123"])).toThrowError(
       "--password can only be set in the config file or passed in via $PASSWORD",

test/unit/node/heart.test.ts

@@ -0,0 +1,108 @@
+import { logger } from "@coder/logger"
+import { readFile, writeFile, stat, utimes } from "fs/promises"
+import { Heart, heartbeatTimer } from "../../../src/node/heart"
+import { clean, mockLogger, tmpdir } from "../../utils/helpers"
+
+const mockIsActive = (resolveTo: boolean) => jest.fn().mockResolvedValue(resolveTo)
+
+describe("Heart", () => {
+  const testName = "heartTests"
+  let testDir = ""
+  let heart: Heart
+
+  beforeAll(async () => {
+    mockLogger()
+    await clean(testName)
+    testDir = await tmpdir(testName)
+  })
+  beforeEach(() => {
+    heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive(true))
+  })
+  afterAll(() => {
+    jest.restoreAllMocks()
+  })
+  afterEach(() => {
+    jest.resetAllMocks()
+    if (heart) {
+      heart.dispose()
+    }
+  })
+  it("should write to a file when given a valid file path", async () => {
+    // Set up heartbeat file with contents
+    const text = "test"
+    const pathToFile = `${testDir}/file.txt`
+    await writeFile(pathToFile, text)
+    const fileContents = await readFile(pathToFile, { encoding: "utf8" })
+    // Explicitly set the modified time to 0 so that we can check
+    // that the file was indeed modified after calling heart.beat().
+    // This works around any potential race conditions.
+    // Docs: https://nodejs.org/api/fs.html#fspromisesutimespath-atime-mtime
+    await utimes(pathToFile, 0, 0)
+
+    expect(fileContents).toBe(text)
+
+    heart = new Heart(pathToFile, mockIsActive(true))
+    heart.beat()
+    // HACK@jsjoeio - beat has some async logic but is not an async method
+    // Therefore, we have to create an artificial wait in order to make sure
+    // all async code has completed before asserting
+    await new Promise((r) => setTimeout(r, 100))
+    // Check that the heart wrote to the heartbeatFilePath and overwrote our text
+    const fileContentsAfterBeat = await readFile(pathToFile, { encoding: "utf8" })
+    expect(fileContentsAfterBeat).not.toBe(text)
+    // Make sure the modified timestamp was updated.
+    const fileStatusAfterEdit = await stat(pathToFile)
+    expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThan(0)
+  })
+  it("should log a warning when given an invalid file path", async () => {
+    heart = new Heart(`fakeDir/fake.txt`, mockIsActive(false))
+    heart.beat()
+    // HACK@jsjoeio - beat has some async logic but is not an async method
+    // Therefore, we have to create an artificial wait in order to make sure
+    // all async code has completed before asserting
+    await new Promise((r) => setTimeout(r, 100))
+    expect(logger.warn).toHaveBeenCalled()
+  })
+  it("should be active after calling beat", () => {
+    heart.beat()
+
+    const isAlive = heart.alive()
+    expect(isAlive).toBe(true)
+  })
+  it("should not be active after dispose is called", () => {
+    heart.dispose()
+
+    const isAlive = heart.alive()
+    expect(isAlive).toBe(false)
+  })
+})
+
+describe("heartbeatTimer", () => {
+  beforeAll(() => {
+    mockLogger()
+  })
+  afterAll(() => {
+    jest.restoreAllMocks()
+  })
+  afterEach(() => {
+    jest.resetAllMocks()
+  })
+  it("should call beat when isActive resolves to true", async () => {
+    const isActive = true
+    const mockIsActive = jest.fn().mockResolvedValue(isActive)
+    const mockBeatFn = jest.fn()
+    await heartbeatTimer(mockIsActive, mockBeatFn)
+    expect(mockIsActive).toHaveBeenCalled()
+    expect(mockBeatFn).toHaveBeenCalled()
+  })
+  it("should log a warning when isActive rejects", async () => {
+    const errorMsg = "oh no"
+    const error = new Error(errorMsg)
+    const mockIsActive = jest.fn().mockRejectedValue(error)
+    const mockBeatFn = jest.fn()
+    await heartbeatTimer(mockIsActive, mockBeatFn)
+    expect(mockIsActive).toHaveBeenCalled()
+    expect(mockBeatFn).not.toHaveBeenCalled()
+    expect(logger.warn).toHaveBeenCalledWith(errorMsg)
+  })
+})