Permanent hang when the adb server accepts no connections (missed by #74) #109

Hi,

FYI, this has been partly written with an AI agent (Claude Fable 5.1). Rephrasing is mine.

AdbServer.listDadbs reaches the adb server through two Socket(host, port) constructions. That constructor connects with no timeout, and a thread blocked in connect() does not respond to Thread.interrupt(). When the adb server is listening but not accepting, the calling thread is not slow, it is gone permanently, and no caller-side timeout can reclaim it.

This is the same class of fix as #99 and #100: connect is the one blocking operation on this path that is still unbounded. It has also been aimed at twice before — #74 ("add timeouts to all socket creation management", merged Sep 2024) and #57 ("Allow setting connect and socket timeouts when creating Dadb object", merged Jan 2024) — and both missed these two call sites, so Dadb.list() still hangs forever on current master and in the released 2.0.0.

The affected call sites are (on master):

dadb/adbserver/AdbServer.kt:71

val output = Socket(adbServerHost, adbServerPort).use { socket ->   // no connect timeout, no soTimeout

dadb/adbserver/AdbBinary.kt:50, reached from AdbServer.kt:68 via tryStartServer → ensureServerRunning

private fun isServerRunning(adbServerHost: String, adbServerPort: Int): Boolean {
    return try {
        Socket(adbServerHost, adbServerPort).close()   // a liveness probe that can block forever
        true
    } catch (e: Exception) {
        false
    }
}

A third, smaller gap in the same function: AdbServer.kt:85 builds each Dadb with

.map { createDadb(adbServerHost, adbServerPort, "host:transport:${it}") }

dropping the connectTimeout / socketTimeout parameters that #57 added, so every Dadb returned by listDadbs silently gets 0 (infinite) for both.

To see this, I made a thread dump from Maestro 2.7.0, which bundles dadb 2.0.0, hung with zero output for 240s. adb devices answered in under a second throughout, and the adb server pid never changed:

"main" #1 prio=5 tid=0x0000000104ddcd40 nid=9987 runnable
   java.lang.Thread.State: RUNNABLE
	at sun.nio.ch.Net.connect0(java.base@21.0.9/Native Method)
	at java.net.Socket.<init>(java.base@21.0.9/Socket.java:555)
	at java.net.Socket.<init>(java.base@21.0.9/Socket.java:324)
	at dadb.adbserver.AdbBinary.isServerRunning(AdbBinary.kt:50)
	at dadb.adbserver.AdbBinary.ensureServerRunning(AdbBinary.kt:23)
	at dadb.adbserver.AdbBinary.tryStartServer(AdbBinary.kt:15)
	at dadb.adbserver.AdbServer.listDadbs(AdbServer.kt:68)
	at dadb.adbserver.AdbServer.listDadbs$default(AdbServer.kt:64)
	at dadb.Dadb$Companion.list(Dadb.kt:256)
	at maestro.android.AndroidDeviceConnection$Companion.byId(AndroidDeviceConnection.kt:409)
	at maestro.cli.session.MaestroSessionManager.createAndroid(MaestroSessionManager.kt:338)
	at maestro.cli.command.PrintHierarchyCommand.run(PrintHierarchyCommand.kt:130)

A second application on the same machine (also using Dadb.list(), dadb 1.2.10) accumulated 44 permanently parked threads out of 88 total in the JVM, and 521 open fds, over about two days — one leaked per ~30s discovery poll, split across both call sites:

at dadb.adbserver.AdbServer.listDadbs(AdbServer.kt:71)      <- 38 of 44
at dadb.adbserver.AdbBinary.isServerRunning(AdbBinary.kt:50) <- the rest

That application wrapped the call in FutureTask.get(timeout) + cancel(true). The caller timed out correctly every time; the thread never came back, because interrupt does nothing to a blocking connect(). No caller-side timeout can fix this — it has to be the socket.

Why "forever" and not "eventually": on macOS a loopback SYN that is dropped is not retried to exhaustion the way a remote one is. Measured here: a single SYN_SENT to 127.0.0.1:5037 still open 13 minutes later against a healthy, idle, accepting server. On Linux the same call typically fails after ~2 minutes — still far too long for a liveness probe.

Reproduction

The adb server must not be running on the port. Bind a listener that never accepts, so connects queue and then stall:

fun main() {
    val blackhole = ServerSocket(5037, 1)                      // never call accept()
    repeat(2) {                                                 // fill the 1-slot backlog
        runCatching { Socket().connect(InetSocketAddress("127.0.0.1", 5037), 200) }
    }
    println("calling Dadb.list() ...")
    Dadb.list()                                                 // never returns
    println("unreachable")
}

Suggested patch

Backward compatible: new parameters are optional, @JvmOverloads keeps the existing Java overloads. Kotlin callers compiled against the old jar need a recompile, since the listDadbs$default arity changes — appropriate for a minor bump.

--- a/dadb/src/main/kotlin/dadb/adbserver/AdbServer.kt
+++ b/dadb/src/main/kotlin/dadb/adbserver/AdbServer.kt
@@
 object AdbServer {
 
+    /**
+     * A local adb server that has not accepted a connection within this long is not
+     * going to. Bounded so callers cannot be parked forever in an uninterruptible connect().
+     */
+    const val DEFAULT_CONNECT_TIMEOUT_MS = 5_000
+
     /**
      * Experimental API
@@
     fun listDadbs(
         adbServerHost: String = "localhost",
         adbServerPort: Int = 5037,
+        connectTimeout: Int = DEFAULT_CONNECT_TIMEOUT_MS,
+        socketTimeout: Int = 0,
     ): List<Dadb> {
-        if (!AdbBinary.tryStartServer(adbServerHost, adbServerPort)) {
+        if (!AdbBinary.tryStartServer(adbServerHost, adbServerPort, connectTimeout)) {
             return emptyList()
         }
-        val output = Socket(adbServerHost, adbServerPort).use { socket ->
+        val output = Socket().use { socket ->
+            socket.connect(InetSocketAddress(adbServerHost, adbServerPort), connectTimeout)
+            socket.soTimeout = socketTimeout
             send(socket, "host:devices")
             readString(DataInputStream(socket.getInputStream()))
         }
@@
-            .map { createDadb(adbServerHost, adbServerPort, "host:transport:${it}") }
+            .map {
+                createDadb(
+                    adbServerHost, adbServerPort, "host:transport:${it}",
+                    connectTimeout, socketTimeout,
+                )
+            }
     }

--- a/dadb/src/main/kotlin/dadb/adbserver/AdbBinary.kt
+++ b/dadb/src/main/kotlin/dadb/adbserver/AdbBinary.kt
@@
 import java.io.File
 import java.io.IOException
+import java.net.InetSocketAddress
 import java.net.Socket
@@
-    fun tryStartServer(adbServerHost: String, adbServerPort: Int): Boolean {
+    fun tryStartServer(
+        adbServerHost: String,
+        adbServerPort: Int,
+        connectTimeout: Int = AdbServer.DEFAULT_CONNECT_TIMEOUT_MS,
+    ): Boolean {
         return try {
-            ensureServerRunning(adbServerHost, adbServerPort)
+            ensureServerRunning(adbServerHost, adbServerPort, connectTimeout)
             true
         } catch (ignore: Exception) {
             false
         }
     }
 
-    fun ensureServerRunning(adbServerHost: String, adbServerPort: Int) {
-        if (!isServerRunning(adbServerHost, adbServerPort)) {
+    fun ensureServerRunning(
+        adbServerHost: String,
+        adbServerPort: Int,
+        connectTimeout: Int = AdbServer.DEFAULT_CONNECT_TIMEOUT_MS,
+    ) {
+        if (!isServerRunning(adbServerHost, adbServerPort, connectTimeout)) {
@@
-    private fun isServerRunning(adbServerHost: String, adbServerPort: Int): Boolean {
+    private fun isServerRunning(
+        adbServerHost: String,
+        adbServerPort: Int,
+        connectTimeout: Int,
+    ): Boolean {
         return try {
-            Socket(adbServerHost, adbServerPort).close()
+            Socket().use { socket ->
+                socket.connect(InetSocketAddress(adbServerHost, adbServerPort), connectTimeout)
+            }
             true
         } catch (e: Exception) {
             false
         }
     }

InetSocketAddress is already imported in AdbServer.kt; only AdbBinary.kt needs the new import.

Remarks

The patch changes the effective default from infinite to 5s. Defaulting the new parameters to 0 instead would preserve strict behavioural compatibility — but 0 is the hang, so the bug would survive for every caller that does not opt in. For isServerRunning in particular, an unbounded liveness probe has no legitimate use, so a bounded default seems right there regardless. Happy to follow whichever you prefer, and to open the PR with tests.

I can make it a PR if it is accepted.