fix(devtools): refuse a webserver port the dev server does not own

.cspell-project-words.txt

@@ -1,3 +1,7 @@
+Landofile
+Olivero
+PKCE
+Stanislav
 appserver
 behaviour
 bmewburn
@@ -25,7 +29,6 @@ intelephense
 interruptible
 knip
 lando
-Landofile
 libasound
 libfreetype
 libgbm
@@ -46,21 +49,19 @@ nohup
 nuxt
 nuxtjs
 nvmrc
-Olivero
 opcache
 openssl
 openvscode
 phpsab
 phpstan
 phpunit
-PKCE
+procps
 recognise
 recognised
 recognises
 repository
 setuptools
 shivammathur
-Stanislav
 starterkit
 startswith
 tokenless

.github/workflows/ci.yml

@@ -1,4 +1,4 @@
 name: CI
 
 on:
   push:
@@ -334,6 +334,89 @@
         working-directory: /tmp/site/nuxt
         run: npm run build
 
+  # `.devtools/start` must never report a server it did not start. Needs
+  # PHP and the scripts, nothing else: both cases fail before Drupal is
+  # touched, so this job skips assemble and provision entirely.
+  test_start_guardrails:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v7
+        with:
+          persist-credentials: false
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '8.4'
+
+      # The readiness probe used to connect to the port and accept any
+      # answer, so a stranger holding it read as a healthy start: BASE_URL
+      # was written and a login link printed for a site never started.
+      - name: A port held by another process is refused
+        working-directory: drupal
+        run: |
+          python3 -m http.server 8899 --bind 127.0.0.1 >/dev/null 2>&1 &
+          squatter=$!
+          trap 'kill "$squatter" 2>/dev/null || true' EXIT
+          until curl -sf -o /dev/null http://127.0.0.1:8899/; do sleep 1; done
+          if WEBSERVER_PORT=8899 .devtools/start > /tmp/squatted.log 2>&1; then
+            echo 'start reported success against a port it never bound' >&2
+            cat /tmp/squatted.log >&2
+            exit 1
+          fi
+          grep -q 'already in use by another process' /tmp/squatted.log
+          test ! -f ../.env
+          # A guard that clears the port by killing whatever holds it is
+          # not a guard. stop_webserver only targets `php -S`, and this
+          # is what pins that.
+          kill -0 "$squatter" 2>/dev/null || {
+            echo 'start killed a process it did not start' >&2
+            exit 1
+          }
+
+      # The gitlab CI image has no procps, and the first version of this
+      # guard shelled out to `ps` for both liveness and identity: it reported
+      # a healthy server as exited and could not recognise its own process to
+      # stop it. This runner does have `ps`, so the guard passed here while
+      # failing there. Hiding `ps` is what makes that case reachable.
+      - name: The guard works on an image without procps
+        working-directory: drupal
+        run: |
+          shim=$(mktemp -d)
+          printf '#!/bin/sh\nexit 127\n' > "$shim/ps"
+          chmod +x "$shim/ps"
+          python3 -m http.server 8908 --bind 127.0.0.1 >/dev/null 2>&1 &
+          squatter=$!
+          trap 'kill "$squatter" 2>/dev/null || true' EXIT
+          until curl -sf -o /dev/null http://127.0.0.1:8908/; do sleep 1; done
+          if PATH="$shim:$PATH" WEBSERVER_PORT=8908 .devtools/start > /tmp/no-procps.log 2>&1; then
+            echo 'start reported success against a squatted port with no ps' >&2
+            cat /tmp/no-procps.log >&2
+            exit 1
+          fi
+          grep -q 'already in use by another process' /tmp/no-procps.log
+          # /proc gives the command line, so the holder is still named.
+          grep -q 'http.server 8908' /tmp/no-procps.log
+          kill -0 "$squatter" 2>/dev/null || {
+            echo 'start killed a process it did not start' >&2
+            exit 1
+          }
+
+      # Second net, for a bind that fails with the port free: the wait
+      # watches the process it launched, not the port, so it reports the
+      # reason instead of timing out on a server that is already gone.
+      - name: A failed bind is reported, not waited out
+        working-directory: drupal
+        run: |
+          if WEBSERVER_HOST=203.0.113.1 WEBSERVER_PORT=8901 WEBSERVER_WAIT_TIMEOUT=20 \
+            .devtools/start > /tmp/bind-failure.log 2>&1; then
+            echo 'start reported success for a server that never bound' >&2
+            cat /tmp/bind-failure.log >&2
+            exit 1
+          fi
+          grep -q 'exited before it accepted a connection' /tmp/bind-failure.log
+          grep -q 'Cannot assign requested address' /tmp/bind-failure.log
+
   # What the root install promises on machines without a working PHP: a
   # consumer's `npm install` must never fail, `npm run setup` must
   # fail loudly, and a too-old PHP must be rejected by the preflight.

drupal/.devtools/helpers.php

@@ -244,6 +244,107 @@ function find_free_port(int $start = 8888, int $max_attempts = 100): int {
   // @codeCoverageIgnoreEnd
 }
 
+/**
+ * Whether something is already accepting connections on a host and port.
+ *
+ * Used as a pre-flight guard, not a readiness probe: a readiness probe
+ * that only asks "is the port answering" cannot tell our own server from
+ * a stranger's, which is how a failed bind used to read as a good start.
+ */
+function port_is_open(string $host, string $port, float $timeout = 0.5): bool {
+  $conn = @stream_socket_client(sprintf('tcp://%s:%s', $host, $port), $errno, $errstr, $timeout);
+  if ($conn === FALSE) {
+    return FALSE;
+  }
+  fclose($conn);
+
+  return TRUE;
+}
+
+/**
+ * Describe whatever holds a port, for an error message. '' when unknown.
+ */
+function port_holder(string $port): string {
+  $pids = [];
+  @exec(sprintf('lsof -ti:%s 2>/dev/null', escapeshellarg($port)), $pids);
+  foreach ($pids as $pid) {
+    $pid = trim((string) $pid);
+    if ($pid === '' || !ctype_digit($pid)) {
+      continue;
+    }
+    $command = process_command((int) $pid);
+    if ($command !== '') {
+      return sprintf('pid %d: %s', (int) $pid, $command);
+    }
+  }
+
+  return '';
+}
+
+/**
+ * The command line of a process, or '' when it cannot be read.
+ *
+ * `/proc` first so this works on images without procps. Note the asymmetry
+ * with pid_is_running(): an unknown liveness must not be read as "dead", but
+ * an unknown command line must not be read as "ours", because the caller uses
+ * it to decide what to kill. Uncertainty is safe in opposite directions.
+ */
+function process_command(int $pid): string {
+  if ($pid < 1) {
+    return '';
+  }
+
+  $cmdline = @file_get_contents(sprintf('/proc/%d/cmdline', $pid));
+  if ($cmdline !== FALSE && $cmdline !== '') {
+    return trim(str_replace("\0", ' ', $cmdline));
+  }
+
+  return trim((string) @shell_exec(sprintf('ps -p %d -o command= 2>/dev/null', $pid)));
+}
+
+/**
+ * Whether a process id is still running.
+ *
+ * Returns NULL when it cannot be determined, which callers must not read as
+ * "dead". The first version shelled out to `ps` and treated its absence as a
+ * dead process, so on an image without procps (the `php:8.3` CI image, for
+ * one) a perfectly healthy server was reported as having exited. A liveness
+ * check that cannot answer has to say so rather than guess the alarming
+ * answer.
+ *
+ * @return bool|null
+ *   TRUE if running, FALSE if definitely not, NULL if undeterminable.
+ */
+function pid_is_running(int $pid): ?bool {
+  if ($pid < 1) {
+    return FALSE;
+  }
+
+  // Signal 0 performs the permission and existence checks without sending
+  // anything. Native, no subprocess, and the usual answer for this.
+  if (function_exists('posix_kill')) {
+    if (posix_kill($pid, 0)) {
+      return TRUE;
+    }
+    // ESRCH means no such process; EPERM means it exists but is not ours.
+    return function_exists('posix_get_last_error')
+      && posix_get_last_error() === (defined('PCNTL_ESRCH') ? PCNTL_ESRCH : 3)
+      ? FALSE
+      : NULL;
+  }
+
+  // Linux without the posix extension.
+  if (is_dir('/proc')) {
+    return is_dir('/proc/' . $pid);
+  }
+
+  // Last resort. An empty result here is genuinely ambiguous, because it is
+  // also what a missing `ps` produces, so it reports unknown rather than dead.
+  $out = trim((string) @shell_exec(sprintf('ps -p %d -o pid= 2>/dev/null', $pid)));
+
+  return $out !== '' ? TRUE : NULL;
+}
+
 /**
  * Stop the dev webserver started by .devtools/start.
  *
@@ -277,7 +378,9 @@ function stop_webserver(string $port): void {
   }
 
   $targets = array_filter(array_unique($candidates), function (int $pid): bool {
-    $command = trim((string) @shell_exec(sprintf('ps -p %d -o command= 2>/dev/null', $pid)));
+    $command = process_command($pid);
+    // An unreadable command line means "do not touch": better to leave our
+    // own server running than to signal a process we could not identify.
     return $command !== '' && str_contains($command, 'php') && str_contains($command, '-S');
   });
   if ($targets === []) {

drupal/.devtools/start

@@ -55,6 +55,32 @@ if ($xdebug_enabled) {
 TASK('Stopping previously started services, if any.');
 stop_webserver($webserver_port);
 
+// stop_webserver only kills `php -S` processes, which is right: it must
+// not shoot at whatever else happens to hold the port. So check here,
+// after it has cleared our own server away, and refuse rather than start
+// a server that cannot bind. A killed server can take a moment to release
+// the port, so the port gets a short grace period before it counts as
+// somebody else's.
+TASK('Checking that the webserver port is free.');
+$free = FALSE;
+for ($i = 0; $i < 6; $i++) {
+  if (!port_is_open($webserver_host, $webserver_port)) {
+    $free = TRUE;
+    break;
+  }
+  usleep(500000);
+}
+if (!$free) {
+  $holder = port_holder($webserver_port);
+  FAIL(
+    'Port %s is already in use by another process%s.' . "\n"
+    . 'Stop it, or set WEBSERVER_PORT to a free port.',
+    $webserver_port,
+    $holder !== '' ? ' (' . $holder . ')' : ''
+  );
+}
+PASS('Port %s is free.', $webserver_port);
+
 TASK('Starting the PHP webserver (docroot: web/).');
 NOTE('Waiting up to %s seconds for the server to be ready.', (string) $webserver_wait_timeout);
 $cwd = (string) getcwd();
@@ -75,22 +101,39 @@ passthru(sprintf(
 ));
 
 $ready = FALSE;
+$died = FALSE;
 for ($i = 0; $i < $webserver_wait_timeout; $i++) {
   sleep(1);
   // Liveness: a cold first start (fresh site, no opcache) can take a
   // while - dots show the script is waiting, not hung.
   echo '.';
-  $fp = @fsockopen($webserver_host, (int) $webserver_port, $errno, $errstr, 1);
-  if ($fp !== FALSE) {
-    fclose($fp);
+
+  // The process we launched, not the port, is the thing being waited on.
+  // A dead pid means the bind failed (or PHP exited), and there is nothing
+  // left to wait for: stop now and print why. Only an explicit FALSE counts;
+  // NULL means the check could not tell, and an undeterminable liveness must
+  // fall through to the port probe rather than abort a healthy start.
+  $pid = (int) trim((string) @file_get_contents(server_pid_file()));
+  if ($pid > 0 && pid_is_running($pid) === FALSE) {
+    $died = TRUE;
+    break;
+  }
+
+  if (port_is_open($webserver_host, $webserver_port, 1)) {
     $ready = TRUE;
     break;
   }
 }
 echo PHP_EOL;
 if (!$ready) {
   $log = @file_get_contents('/tmp/quickstart-drupal-php-server.log') ?: '';
-  FAIL('Unable to start inbuilt PHP server' . ($log !== '' ? "\n" . $log : ''));
+  // '%s' as the format, not the message: a server log can contain a
+  // literal % and sprintf would choke on it.
+  FAIL(
+    '%s',
+    ($died ? 'The PHP server exited before it accepted a connection.' : 'Unable to start inbuilt PHP server')
+    . ($log !== '' ? "\n" . $log : '')
+  );
 }
 PASS('Server started successfully.');