[2.x] fix: Stop a displaced server (#9713)

**Problem**
If a client cannot reach the server, it deletes the portfile. Then it
starts a second server. The first server is displaced: its socket path
now belongs to the second server.

A displaced server keeps running. A dropIfIdle notification cannot
reach it, because the proc file it registered names the socket that
the second server now owns.

**Solution**
The portfile holds the serverId of whichever server wrote it. Watch
that file, and exit when the id is not this server's. Leave the
portfile to the server that owns it.

---------

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

main-command/src/main/scala/sbt/internal/client/NetworkClient.scala

@@ -221,61 +221,60 @@ class NetworkClient(
         .map(NetworkClient.sysPropName)
         .toSet
       val current = NetworkClient.serverSysProps(arguments.sbtArguments)
-      Try(ClientSocket.portFile(portfile)).toOption match
-        case Some(pf) =>
-          val (dropped, added, changed) =
-            NetworkClient.sysPropsDiff(pf.sysProps, current, deferred)
-          if (dropped ++ added ++ changed).nonEmpty then
-            // a server nothing recorded the options of may well have the ones this client
-            // carries already, and an editor's server is the usual one to be in that state,
-            // so it gets a word rather than a shutdown it never asked for
-            val known = pf.sysPropsRecorded.contains(true)
-            val restarts = known && serverAutoStart && serverAutoRestart
-            val level = if restarts then Level.Info else Level.Warn
-            // the values are what a credential would be hiding in, so only the names of
-            // the options are worth saying out loud
-            console.appendLog(
-              level,
-              if restarts then "sbt server is running with different JVM options; restarting it"
-              else if known then
-                "sbt server is running with different JVM options, which it cannot pick up"
-              else
-                "sbt server was started by something other than the thin client, so it may "
-                  + "not have these JVM options"
-            )
-            if dropped.nonEmpty then console.appendLog(level, s"dropped: ${dropped.mkString(" ")}")
-            if added.nonEmpty then console.appendLog(level, s"added: ${added.mkString(" ")}")
-            if changed.nonEmpty then console.appendLog(level, s"changed: ${changed.mkString(" ")}")
-            if !restarts then console.appendLog(level, "run 'sbt shutdown' for them to take effect")
+      ClientSocket.loadPortFile(portfile).foreach { pf =>
+        val (dropped, added, changed) =
+          NetworkClient.sysPropsDiff(pf.sysProps, current, deferred)
+        if (dropped ++ added ++ changed).nonEmpty then
+          // a server nothing recorded the options of may well have the ones this client
+          // carries already, and an editor's server is the usual one to be in that state,
+          // so it gets a word rather than a shutdown it never asked for
+          val known = pf.sysPropsRecorded.contains(true)
+          val restarts = known && serverAutoStart && serverAutoRestart
+          val level = if restarts then Level.Info else Level.Warn
+          // the values are what a credential would be hiding in, so only the names of
+          // the options are worth saying out loud
+          console.appendLog(
+            level,
+            if restarts then "sbt server is running with different JVM options; restarting it"
+            else if known then
+              "sbt server is running with different JVM options, which it cannot pick up"
             else
-              shutdownRunningServer(pf.uri) match
-                case Some(true)  => ()
-                case Some(false) =>
-                  console.appendLog(
-                    Level.Error,
-                    "the sbt server did not shut down, it is most likely busy with another client"
-                  )
-                  // the request is queued on it and stays there, so a second one buys nothing
-                  console.appendLog(
-                    Level.Error,
-                    "it has the request and takes it once that work is done, which ends that"
-                      + " client's session too"
-                  )
-                  console.appendLog(Level.Error, "run this command again once the server is gone")
-                  throw new ServerFailedException
-                case None =>
-                  // it answers no socket but is still there, which says nothing about how
-                  // busy it is, so leaving it alone beats failing an invocation over it
-                  console.appendLog(
-                    Level.Warn,
-                    "the sbt server could not be reached to restart it; it keeps the JVM"
-                      + " options it was started with"
-                  )
-                  console.appendLog(
-                    Level.Warn,
-                    "run 'sbt shutdown' for the ones passed here to take effect"
-                  )
-        case _ => ()
+              "sbt server was started by something other than the thin client, so it may "
+                + "not have these JVM options"
+          )
+          if dropped.nonEmpty then console.appendLog(level, s"dropped: ${dropped.mkString(" ")}")
+          if added.nonEmpty then console.appendLog(level, s"added: ${added.mkString(" ")}")
+          if changed.nonEmpty then console.appendLog(level, s"changed: ${changed.mkString(" ")}")
+          if !restarts then console.appendLog(level, "run 'sbt shutdown' for them to take effect")
+          else
+            shutdownRunningServer(pf.uri) match
+              case Some(true)  => ()
+              case Some(false) =>
+                console.appendLog(
+                  Level.Error,
+                  "the sbt server did not shut down, it is most likely busy with another client"
+                )
+                // the request is queued on it and stays there, so a second one buys nothing
+                console.appendLog(
+                  Level.Error,
+                  "it has the request and takes it once that work is done, which ends that"
+                    + " client's session too"
+                )
+                console.appendLog(Level.Error, "run this command again once the server is gone")
+                throw new ServerFailedException
+              case None =>
+                // it answers no socket but is still there, which says nothing about how
+                // busy it is, so leaving it alone beats failing an invocation over it
+                console.appendLog(
+                  Level.Warn,
+                  "the sbt server could not be reached to restart it; it keeps the JVM"
+                    + " options it was started with"
+                )
+                console.appendLog(
+                  Level.Warn,
+                  "run 'sbt shutdown' for the ones passed here to take effect"
+                )
+      }
 
   /**
    * Asks the running server to shut down and waits for it to let go of its socket, so that

main-command/src/main/scala/sbt/internal/server/Server.scala

@@ -28,10 +28,12 @@ import sbt.internal.util.ErrorHandling
 import sbt.internal.util.Util.isWindows
 import org.scalasbt.ipcsocket.*
 import sbt.internal.bsp.BuildServerConnection
+import sbt.protocol.ClientSocket
 import xsbti.AppConfiguration
 
 private[sbt] sealed trait ServerInstance {
   def shutdown(): Unit
+  def serverId: String
   def ready: Future[Unit]
   def authenticate(challenge: String): Boolean
 }
@@ -43,6 +45,10 @@ private[sbt] object Server {
       with TokenFileFormats
   object JsonProtocol extends JsonProtocol
 
+  /** The id the portfile names, None when it names none, and a failure when unreadable. */
+  private[sbt] def serverIdOf(portfile: File): Try[Option[String]] =
+    ClientSocket.loadPortFile(portfile).map(_.serverId)
+
   def start(
       connection: ServerConnection,
       onIncomingSocket: (AtomicReference[Socket], ServerInstance) => Unit,
@@ -56,6 +62,7 @@ private[sbt] object Server {
       private val rand = new SecureRandom
       private var token: String = nextToken
       private val serverSocketHolder = AtomicCloseable[ServerSocket]()
+      override val serverId: String = java.util.UUID.randomUUID().toString
 
       val serverThread = new Thread("sbt-socket-server") {
         override def run(): Unit = {
@@ -157,12 +164,8 @@ private[sbt] object Server {
       }
 
       override def shutdown(): Unit = {
-        if (portfile.exists) {
-          IO.delete(portfile)
-        }
-        if (tokenfile.exists) {
-          IO.delete(tokenfile)
-        }
+        if (serverIdOf(portfile).getOrElse(None).contains(serverId)) IO.delete(portfile)
+        IO.delete(tokenfile)
         running.set(false)
         serverSocketHolder.close()
         log.info("shutting down sbt server")
@@ -197,20 +200,16 @@ private[sbt] object Server {
         // which of the two this is: a client can restart a server over the first but has no
         // business taking down one whose options it never saw
         val sysPropsRecorded = Option(startedByThisBuild)
-        val p =
-          auth match {
-            case _ if auth(ServerAuthentication.Token) =>
-              writeTokenfile()
-              PortFile(
-                uri,
-                Option(tokenfile.toString),
-                Option(IO.toURI(tokenfile).toString),
-                sysProps,
-                sysPropsRecorded
-              )
-            case _ =>
-              PortFile(uri, None, None, sysProps, sysPropsRecorded)
-          }
+        val authOK = auth(ServerAuthentication.Token)
+        if (authOK) writeTokenfile()
+        val p = PortFile(
+          uri,
+          if (authOK) Some(tokenfile.toString) else None,
+          if (authOK) Some(IO.toURI(tokenfile).toString) else None,
+          sysProps,
+          sysPropsRecorded,
+          Some(serverId)
+        )
         val json = Converter.toJson(p).get
         IO.writeFileAtomically(portfile)(tmp => IO.write(tmp, CompactPrinter(json)))
       }

main-command/src/test/scala/sbt/internal/server/ServerIdSpec.scala

@@ -0,0 +1,45 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+package server
+
+import java.io.{ File, FileNotFoundException }
+import java.nio.file.Files
+
+import scala.util.Success
+
+import verify.BasicTestSuite
+
+// check() tells these outcomes apart, so each one has to stay distinguishable
+object ServerIdSpec extends BasicTestSuite:
+  private def withPortfile(content: Option[String])(f: File => Unit): Unit =
+    val dir = Files.createTempDirectory("portfile").toFile
+    val portfile = new File(dir, "active.json")
+    content.foreach(sbt.io.IO.write(portfile, _))
+    try f(portfile)
+    finally sbt.io.IO.delete(dir)
+
+  test("a portfile that is not there"):
+    withPortfile(None): portfile =>
+      val failure = Server.serverIdOf(portfile).failed.get
+      assert(failure.isInstanceOf[FileNotFoundException])
+
+  test("a portfile that is not json"):
+    withPortfile(Some("this is not json")): portfile =>
+      assert(Server.serverIdOf(portfile).isFailure)
+
+  test("a portfile that names no server"):
+    withPortfile(Some("""{"uri":"local:///sock"}""")): portfile =>
+      assert(Server.serverIdOf(portfile) == Success(None))
+
+  test("a portfile that names a server"):
+    withPortfile(Some("""{"uri":"local:///sock","serverId":"an-id"}""")): portfile =>
+      assert(Server.serverIdOf(portfile) == Success(Some("an-id")))
+end ServerIdSpec