fix(server): ensure using a free port for dev console (#5163)

* fix(server): ensure using a free port for dev console

* fix(server): don't swallow errors on server start

Also print the error message and exit.

* fix(server): better error handling on server start

* Fail if the explicitly specified port is empty
* Always retry on `EADDRINUSE` error if the port hasn't been specified explicitly
* Always fail on any errno error expect `EADDRINUSE`
* Log the port the server has started at

* fix(server): use correct default port value

* fix(server): correct port range

* fix: remove unnecessary port range

Alternative port range makes so sense when `port` is specified:
https://github.com/unjs/get-port-please

* test: add unit tests

core/package.json

@@ -71,7 +71,7 @@
     "fs-extra": "^11.1.0",
     "fsevents": "^2.3.3",
     "get-port": "^5.1.1",
-    "get-port-please": "^3.0.1",
+    "get-port-please": "^3.1.1",
     "glob": "^10.2.6",
     "global-agent": "^3.0.0",
     "got": "^11.8.6",
@@ -293,4 +293,4 @@
     "fsevents": "^2.3.3"
   },
   "gitHead": "b0647221a4d2ff06952bae58000b104215aed922"
-}
+}

core/src/server/server.ts

@@ -22,11 +22,18 @@ import { BaseServerRequest, resolveRequest, serverRequestSchema, shellCommandPar
 import { DEFAULT_GARDEN_DIR_NAME, gardenEnv } from "../constants"
 import { Log } from "../logger/log-entry"
 import { Command, CommandResult, PrepareParams } from "../commands/base"
-import { toGardenError, GardenError } from "../exceptions"
+import {
+  toGardenError,
+  GardenError,
+  isEAddrInUseException,
+  ParameterError,
+  isErrnoException,
+  CommandError,
+} from "../exceptions"
 import { EventName, Events, EventBus, shouldStreamWsEvent } from "../events/events"
 import type { ValueOf } from "../util/util"
 import { joi } from "../config/common"
-import { randomString } from "../util/string"
+import { dedent, randomString } from "../util/string"
 import { authTokenHeader } from "../cloud/auth"
 import { ApiEventBatch, BufferedEventStream, LogEntryEventPayload } from "../cloud/buffered-event-stream"
 import { eventLogLevel, LogLevel } from "../logger/logger"
@@ -46,13 +53,10 @@ import type { AutocompleteSuggestion } from "../cli/autocomplete"
 import { z } from "zod"
 import { omitUndefined } from "../util/objects"
 import { createServer } from "http"
+import { defaultServerPort } from "../commands/serve"
 
 const pty = require("node-pty-prebuilt-multiarch")
 
-// Note: This is different from the `garden serve` default port.
-// We may no longer embed servers in watch processes from 0.13 onwards.
-export const defaultWatchServerPort = 9777
-
 const skipLogsForCommands = ["autocomplete"]
 
 const skipAnalyticsForCommands = ["sync status"]
@@ -197,20 +201,51 @@ export class GardenServer extends EventEmitter {
       })
     }
 
-    if (this.port) {
-      await _start()
+    if (this.port !== undefined) {
+      try {
+        await _start()
+      } catch (err) {
+        // Fail if the explicitly specified port is already in use.
+        if (isEAddrInUseException(err)) {
+          throw new ParameterError({
+            message: dedent`
+            Port ${this.port} is already in use, possibly by another Garden server process.
+            Either terminate the other process, or choose another port using the --port parameter.
+          `,
+          })
+        } else if (isErrnoException(err)) {
+          throw new CommandError({
+            message: `Unable to start server: ${err.message}`,
+            code: err.code,
+          })
+        }
+        throw err
+      }
     } else {
+      let serverStarted = false
       do {
         try {
           this.port = await getPort({
-            port: defaultWatchServerPort,
-            portRange: [defaultWatchServerPort + 1, defaultWatchServerPort + 50],
-            alternativePortRange: [defaultWatchServerPort - 1, defaultWatchServerPort - 50],
+            host: hostname,
+            port: defaultServerPort,
+            portRange: [defaultServerPort + 1, defaultServerPort + 99],
           })
           await _start()
-        } catch {}
-      } while (!this.server)
+          serverStarted = true
+        } catch (err) {
+          if (isEAddrInUseException(err)) {
+            this.log.debug(`Unable to start server. Port ${this.port} is already in use. Retrying with another port...`)
+          } else if (isErrnoException(err)) {
+            throw new CommandError({
+              message: `Unable to start server: ${err.message}`,
+              code: err.code,
+            })
+          }
+          throw err
+        }
+      } while (!serverStarted)
     }
+    this.log.info(chalk.white(`Garden server has successfully started at port ${chalk.bold(this.port)}.\n`))
 
     const processRecord = await this.globalConfigStore.get("activeProcesses", String(process.pid))
 

core/test/unit/src/server/server.ts

@@ -6,7 +6,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-import { makeTestGardenA, taskResultOutputs, testPluginReferences } from "../../../helpers"
+import { expectError, makeTestGardenA, taskResultOutputs, testPluginReferences } from "../../../helpers"
 import { Server } from "http"
 import { GardenServer, startServer } from "../../../../src/server/server"
 import { Garden } from "../../../../src/garden"
@@ -117,6 +117,59 @@ describe("GardenServer", () => {
     })
   })
 
+  context("port conflicts", () => {
+    const serverPort = 9777
+
+    it("should throw an error if an explicitly defined port is already in use", async () => {
+      const gardenServer1 = new GardenServer({
+        log: garden.log,
+        port: serverPort,
+        manager,
+        defaultProjectRoot: garden.projectRoot,
+        serveCommand,
+      })
+      await gardenServer1.start()
+
+      const gardenServer2 = new GardenServer({
+        log: garden.log,
+        port: serverPort,
+        manager,
+        defaultProjectRoot: garden.projectRoot,
+        serveCommand,
+      })
+
+      expectError(() => gardenServer2.start(), {
+        contains: `Port ${serverPort} is already in use, possibly by another Garden server process`,
+      })
+
+      await gardenServer1.close()
+      await gardenServer2.close()
+    })
+
+    it("two servers should use different ports if no ports have been declared explicitly", async () => {
+      const gardenServer1 = new GardenServer({
+        log: garden.log,
+        manager,
+        defaultProjectRoot: garden.projectRoot,
+        serveCommand,
+      })
+      await gardenServer1.start()
+
+      const gardenServer2 = new GardenServer({
+        log: garden.log,
+        manager,
+        defaultProjectRoot: garden.projectRoot,
+        serveCommand,
+      })
+      await gardenServer2.start()
+
+      expect(gardenServer1.port).to.not.equal(gardenServer2.port)
+
+      await gardenServer1.close()
+      await gardenServer2.close()
+    })
+  })
+
   describe("POST /api", () => {
     it("returns 401 if missing auth header", async () => {
       await request(server).post("/api").send({}).expect(401)