From f398ce414ebde61fb43d7f6e61d6ed3f3dff27e1 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 21:29:26 -0700 Subject: [PATCH 1/8] ADFA-5242: Retry an accept() failure instead of shutting the server down ServerSocket.accept() is declared to throw IOException, of which SocketException is one subtype. The accept loop caught only that subtype, and the enclosing try has a finally but no catch, so any other IOException unwound past the loop to start()'s outermost handler -- whose finally closes the listening socket *and* the database. Every later documentation request then failed until the app restarted, with a single "Error: ..." line as the only trace. The realistic trigger is descriptor exhaustion, which is self-limiting in the worst way: the descriptors accept() is waiting for are held by this server's own in-flight connections, so the condition clears moments later -- by which point the server has already shut itself down. Now only the listening socket closing ends the loop, as its own named predicate: getting this wrong fails differently in each direction, and treating a transient failure as terminal is exactly the bug being fixed. Non-fatal failures log their exception type and retry after 50 ms, so a persistent failure cannot spin the loop at full tilt, flooding the log and competing with the connection closes that would fix it. A successful accept never waits. Found by CodeRabbit on PR #1688, whose ADFA-5172 instrumentation is abandoned; the defect it pointed at is in stage regardless, which is why this is a separate change against stage's own loop rather than a rescue of that branch. Three tests on the predicate: both spellings of a closed socket, a reset connection, a SocketTimeoutException, descriptor exhaustion, and -- since the close is identified only by its message -- exceptions with no message at all. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/localWebServer/WebServer.kt | 35 ++++++++++++ .../localWebServer/AcceptFailureTest.kt | 54 +++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index a978a8f286..49cadf4093 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -158,6 +158,12 @@ class WebServer( .create() private val dbContextType = object : TypeToken>() {}.type private var bookshelfTemplateId: Int = -1 + + // Long enough that a descriptor-exhaustion spin cannot flood the log or starve the connections + // whose closing would fix it; short enough to be invisible to a user, and never paid on a + // successful accept. + private val failedAcceptBackoffMs = 50L + private val httpInternalServerError = 500 private val httpNotFound = 404 @@ -462,6 +468,35 @@ class WebServer( } } + /** + * Whether an accept failure means the server is shutting down rather than having hit something + * transient. `ServerSocket.accept()` is declared to throw `IOException`, of which + * `SocketException` is one subtype, so only the listening socket closing ends the loop -- + * everything else, a `SocketTimeoutException` or a descriptor-exhaustion `IOException` included, + * is retried. Getting this wrong is bad in a different way each way round: treating a transient + * failure as terminal stops serving documentation until the app restarts, and treating the close + * as transient spins the loop against a dead socket. + * + * A closed socket reports itself only in the exception's message, hence the string test. + */ + internal fun shouldStopAccepting(e: IOException): Boolean = + e is java.net.SocketException && e.message?.contains("Closed", ignoreCase = true) == true + + /** + * Brief pause after an accept failure that is not the socket closing, so a *persistent* failure + * cannot spin this loop at full tilt while it clears. "Too many open files" is the realistic one, + * and it is self-inflicted in the worst way: the descriptors this server is waiting to reuse are + * the ones its own in-flight connections hold, so retrying flat out both floods the log and + * competes with the work that would free them. Only the failure path ever waits. + */ + private fun pauseAfterFailedAccept() { + try { + Thread.sleep(failedAcceptBackoffMs) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + } + } + /** * Reads a single line from the stream (bytes until newline). Same stream is used for headers * and body so POST body bytes are not lost to a separate buffered reader. HTTP header lines are ASCII. diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt new file mode 100644 index 0000000000..bd6d4fa84c --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -0,0 +1,54 @@ +package com.itsaky.androidide.localWebServer + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import java.io.IOException +import java.net.SocketException +import java.net.SocketTimeoutException + +/** + * ADFA-5242: one failed accept() must not take documentation down for the rest of the session. + * + * `ServerSocket.accept()` is declared to throw `IOException`; `SocketException` is one subtype. The + * loop used to catch only that subtype, and the enclosing try has a finally but no catch, so any + * other `IOException` unwound to `start()`'s outermost handler -- whose finally closes the listening + * socket *and* the database. Every later request then failed until the app restarted, with one log + * line as the only trace. + */ +class AcceptFailureTest { + // Every path is given explicitly: ServerConfig's defaults reach for external storage, which a + // JVM test has no stub for. + private fun server() = + WebServer( + ServerConfig( + port = 0, + databasePath = "/nonexistent/test.db", + fileDirPath = "/tmp", + debugDatabasePath = "/nonexistent/debug.db", + debugEnablePath = "/nonexistent/debug-flag", + experimentsEnablePath = "/nonexistent/exp-flag", + clearCacheEnablePath = "/nonexistent/cs0-flag", + projectDatabasePath = "/nonexistent/recent-projects.db", + ), + ) + + @Test + fun `only the listening socket closing stops the accept loop`() { + assertThat(server().shouldStopAccepting(SocketException("Socket closed"))).isTrue() + assertThat(server().shouldStopAccepting(SocketException("socket is CLOSED"))).isTrue() + } + + @Test + fun `a transient accept failure is retried, whatever its type`() { + assertThat(server().shouldStopAccepting(SocketException("Connection reset by peer"))).isFalse() + assertThat(server().shouldStopAccepting(SocketTimeoutException("Accept timed out"))).isFalse() + assertThat(server().shouldStopAccepting(IOException("Too many open files"))).isFalse() + } + + // A message-less exception must not be mistaken for the close, whose only marker is its message. + @Test + fun `an accept failure with no message is retried`() { + assertThat(server().shouldStopAccepting(IOException())).isFalse() + assertThat(server().shouldStopAccepting(SocketException())).isFalse() + } +} From 8bd64491b10b3f6b5d35c6621f792f620e202988 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 23:04:43 -0700 Subject: [PATCH 2/8] ADFA-5242: Wire the accept-failure helpers into the loop they were written for The review is right: shouldStopAccepting and pauseAfterFailedAccept were reachable only from their unit tests. start() still caught SocketException and still retried without a backoff, so the fix this branch claims to make did not exist in the running server -- a bare IOException such as "Too many open files" went on unwinding to start()'s outermost handler, whose finally closes the listening socket and the database. The loop now catches IOException, breaks only when shouldStopAccepting says the socket closed, and pauses before retrying anything else. The accept loop moves out of start() into an internal acceptLoop(ServerSocket). That is what makes the behaviour testable: the rest of start() needs a live Android runtime -- TrafficStats, SQLite -- while the loop needs neither, which is why nothing exercised it before. Two tests now drive it through a ServerSocket whose accept() fails on demand; the retry test fails against the previous loop with the IOException escaping, which is the defect itself. Co-Authored-By: Claude Opus 5 --- .../androidide/localWebServer/WebServer.kt | 110 ++++++++++-------- .../localWebServer/AcceptFailureTest.kt | 37 ++++++ 2 files changed, 100 insertions(+), 47 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 49cadf4093..2b526e4ffa 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -364,6 +364,68 @@ class WebServer( } } + /** + * Accepts connections on [socket] until it closes. + * + * Extracted from [start] so ADFA-5242's retry path can be driven by a socket whose accept() + * fails: the rest of start() needs a live Android runtime -- TrafficStats, SQLite -- and this + * loop needs neither. The bug it fixes was invisible precisely because nothing could reach here. + */ + internal fun acceptLoop(socket: ServerSocket) { + while (true) { + var clientSocket: Socket? = null + try { + try { + if (debugEnabled) log.debug("About to call accept() on the server socket, {}.", socket) + clientSocket = socket.accept() + + if (debugEnabled) log.debug("Returned from socket accept(), clientSocket is {}.", clientSocket) + } catch (e: IOException) { + // IOException, not SocketException: accept() is declared to throw the wider + // type, and "Too many open files" arrives as a bare IOException. Catching only + // the subtype let that one unwind to start()'s outermost handler, whose finally + // closes the listening socket and the database (ADFA-5242). + // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 + if (debugEnabled) log.debug("Caught IOException '$e'.") + + if (shouldStopAccepting(e)) { + if (debugEnabled) log.debug("WebServer socket closed, shutting down.") + break + } + log.error("Accept() failed: {}", e.message) + pauseAfterFailedAccept() + continue + } + try { + clientSocket?.let { handleClient(it) } + } catch (e: Exception) { + // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 + if (debugEnabled) log.debug("Caught exception '$e'.") + + if (e is java.net.SocketException && e.message?.contains("Closed", ignoreCase = true) == true) { + if (debugEnabled) log.debug("Client disconnected: {}", e.message) + } else { + log.error("Error handling client: {}", e.message) + clientSocket?.let { client -> + try { + val output = client.outputStream + + sendError(PrintWriter(output, true), output, httpInternalServerError, "Internal Server Error 1") + } catch (e2: Exception) { + log.error("Error sending error response: {}", e2.message) + } + } + } + } + } finally { + clientSocket?.close() + + // CodeRabbit objects to the following line because clientSocket may print out as "null." This is intentional. --DS + if (debugEnabled) log.debug("clientSocket was {}.", clientSocket) + } + } + } + fun start() { // Hal Eisen: Required to fix StrictMode.VmPolicy.Builder.detectUntaggedSockets() TrafficStats.setThreadStatsTag(0xC0DE) @@ -400,53 +462,7 @@ class WebServer( } log.info("WebServer started successfully on '{}', port {}.", config.bindName, config.port) - while (true) { - var clientSocket: Socket? = null - try { - try { - if (debugEnabled) log.debug("About to call accept() on the server socket, {}.", serverSocket) - clientSocket = serverSocket.accept() - - if (debugEnabled) log.debug("Returned from socket accept(), clientSocket is {}.", clientSocket) - } catch (e: java.net.SocketException) { - // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 - if (debugEnabled) log.debug("Caught java.net.SocketException '$e'.") - - if (e.message?.contains("Closed", ignoreCase = true) == true) { - if (debugEnabled) log.debug("WebServer socket closed, shutting down.") - break - } - log.error("Accept() failed: {}", e.message) - continue - } - try { - clientSocket?.let { handleClient(it) } - } catch (e: Exception) { - // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 - if (debugEnabled) log.debug("Caught exception '$e'.") - - if (e is java.net.SocketException && e.message?.contains("Closed", ignoreCase = true) == true) { - if (debugEnabled) log.debug("Client disconnected: {}", e.message) - } else { - log.error("Error handling client: {}", e.message) - clientSocket?.let { socket -> - try { - val output = socket.outputStream - - sendError(PrintWriter(output, true), output, httpInternalServerError, "Internal Server Error 1") - } catch (e2: Exception) { - log.error("Error sending error response: {}", e2.message) - } - } - } - } - } finally { - clientSocket?.close() - - // CodeRabbit objects to the following line because clientSocket may print out as "null." This is intentional. --DS - if (debugEnabled) log.debug("clientSocket was {}.", clientSocket) - } - } + acceptLoop(serverSocket) } catch (e: Exception) { log.error("Error: {}", e.message) } finally { diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt index bd6d4fa84c..5164cd8b67 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -3,6 +3,8 @@ package com.itsaky.androidide.localWebServer import com.google.common.truth.Truth.assertThat import org.junit.Test import java.io.IOException +import java.net.ServerSocket +import java.net.Socket import java.net.SocketException import java.net.SocketTimeoutException @@ -51,4 +53,39 @@ class AcceptFailureTest { assertThat(server().shouldStopAccepting(IOException())).isFalse() assertThat(server().shouldStopAccepting(SocketException())).isFalse() } + + // The helpers above are only worth having if the loop calls them. It did not: both were dead + // code reachable from tests alone, so the fix this PR claimed to make did not exist. These two + // drive the real loop instead of the predicate. + @Test + fun `a retryable failure does not end the accept loop`() { + val socket = ScriptedServerSocket(failures = 2) + socket.use { server().acceptLoop(it) } + + // Three: two failures retried, then the close that ends it. One would mean the first + // failure escaped the loop -- the ADFA-5242 bug. + assertThat(socket.acceptCalls).isEqualTo(3) + } + + @Test + fun `a closed socket ends the accept loop at once`() { + val socket = ScriptedServerSocket(failures = 0) + socket.use { server().acceptLoop(it) } + + assertThat(socket.acceptCalls).isEqualTo(1) + } + + /** Fails accept() [failures] times with a retryable error, then reports the socket closed. */ + private class ScriptedServerSocket( + private val failures: Int, + ) : ServerSocket() { + var acceptCalls = 0 + private set + + override fun accept(): Socket { + acceptCalls++ + if (acceptCalls <= failures) throw IOException("Too many open files") + throw SocketException("Socket closed") + } + } } From a797fcb66104ba431c99c57f31219f17b6987bce Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 22 Aug 2026 00:46:04 -0700 Subject: [PATCH 3/8] ADFA-5242: Bound the accept retry, and trust the stop flag over the message text Three defects from reviewing my own PR. A 50 ms backoff bounds CPU, not volume: a permanent failure retried forever at 20 log lines a second, and the comment claimed the backoff prevented flooding the log. On this phone's 5 MiB logcat buffer that one line displaces every other diagnostic within the hour. After 20 consecutive failures the loop now gives up, having said so once; any successful accept resets the count, so unrelated failures over a long session cannot accumulate into a shutdown. shouldStopAccepting matches the exception's message. stopRequested is authoritative and message-independent, and is now checked first: if a platform ever words a closed socket differently, matching text alone would spin until the cap instead of exiting, leaving start()'s finally unrun -- the database open and the port held, which is worse than the failure this method exists to survive. stopRequested is @Volatile now that the accept loop reads it without the lock. Three tests: the cap, the reset, and the stop flag. The last fails at 20 instead of 1 without its fix; a negative test for the cap would hang the build, which is the defect it prevents. Co-Authored-By: Claude Opus 5 --- .../androidide/localWebServer/WebServer.kt | 33 ++++++++++++-- .../localWebServer/AcceptFailureTest.kt | 45 ++++++++++++++++++- 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 2b526e4ffa..e66d249995 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -116,6 +116,9 @@ class WebServer( // socket then binds anyway a moment later, orphaned, and holds the port until the process // dies. The next start() attempt on that port then fails with "Address already in use." private val lifecycleLock = Any() + // @Volatile: written under lifecycleLock by stop(), but read by the accept loop without it -- + // see acceptLoop, which has to see a stop that happened on another thread. + @Volatile private var stopRequested = false private lateinit var serverSocket: ServerSocket private lateinit var database: SQLiteDatabase @@ -159,11 +162,17 @@ class WebServer( private val dbContextType = object : TypeToken>() {}.type private var bookshelfTemplateId: Int = -1 - // Long enough that a descriptor-exhaustion spin cannot flood the log or starve the connections - // whose closing would fix it; short enough to be invisible to a user, and never paid on a - // successful accept. + // Long enough to stop a descriptor-exhaustion spin starving the connections whose closing would + // fix it; short enough to be invisible to a user, and never paid on a successful accept. private val failedAcceptBackoffMs = 50L + // A backoff alone does not bound anything: 50 ms between attempts is 20 log lines a second, for + // as long as the failure lasts, which on a 5 MiB logcat buffer is every other diagnostic on the + // device gone within the hour. After this many consecutive failures the loop gives up, having + // said so once -- a listener that cannot accept is not serving anyway, and stop()/start() is the + // recovery. Reset by any successful accept. + private val maxConsecutiveAcceptFailures = 20 + private val httpInternalServerError = 500 private val httpNotFound = 404 @@ -372,12 +381,14 @@ class WebServer( * loop needs neither. The bug it fixes was invisible precisely because nothing could reach here. */ internal fun acceptLoop(socket: ServerSocket) { + var consecutiveFailures = 0 while (true) { var clientSocket: Socket? = null try { try { if (debugEnabled) log.debug("About to call accept() on the server socket, {}.", socket) clientSocket = socket.accept() + consecutiveFailures = 0 if (debugEnabled) log.debug("Returned from socket accept(), clientSocket is {}.", clientSocket) } catch (e: IOException) { @@ -388,10 +399,24 @@ class WebServer( // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 if (debugEnabled) log.debug("Caught IOException '$e'.") - if (shouldStopAccepting(e)) { + // stopRequested first: it is authoritative and message-independent, where + // shouldStopAccepting reads the exception text. If a platform ever words a closed + // socket differently, matching on the text alone would spin here forever -- the + // thread never exiting, so start()'s finally never closing the database or freeing + // the port, which is worse than the failure this method exists to survive. + if (stopRequested || shouldStopAccepting(e)) { if (debugEnabled) log.debug("WebServer socket closed, shutting down.") break } + consecutiveFailures++ + if (consecutiveFailures >= maxConsecutiveAcceptFailures) { + log.error( + "Accept() failed {} times in a row, last error '{}'; giving up on this listener.", + consecutiveFailures, + e.message, + ) + break + } log.error("Accept() failed: {}", e.message) pauseAfterFailedAccept() continue diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt index 5164cd8b67..e13a8f7433 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -75,16 +75,57 @@ class AcceptFailureTest { assertThat(socket.acceptCalls).isEqualTo(1) } - /** Fails accept() [failures] times with a retryable error, then reports the socket closed. */ + // stop() is authoritative where the exception text is not. If a platform words a closed socket + // differently, matching on the message alone would spin here until the cap instead of exiting, + // leaving start()'s finally unrun -- the database open and the port held. + @Test + fun `a requested stop ends the loop whatever the exception says`() { + val server = server() + // No socket is bound yet, so this only records that a stop was asked for. + server.stop() + + val socket = ScriptedServerSocket(failures = Int.MAX_VALUE, message = "unexpected wording") + socket.use { server.acceptLoop(it) } + + assertThat(socket.acceptCalls).isEqualTo(1) + } + + // A backoff bounds CPU, not volume: without a cap this loop retries a permanent failure forever, + // logging every 50 ms. Giving up leaves a listener that was not serving anyway, and says so once. + @Test + fun `a permanent failure is abandoned rather than retried forever`() { + val socket = ScriptedServerSocket(failures = Int.MAX_VALUE) + socket.use { server().acceptLoop(it) } + + assertThat(socket.acceptCalls).isEqualTo(20) + } + + // A successful accept means the condition cleared, so the count towards the cap starts over -- + // otherwise a server up for long enough accumulates unrelated failures and stops on the twentieth. + @Test + fun `a success between failures resets the count`() { + val socket = ScriptedServerSocket(failures = Int.MAX_VALUE, succeedAt = setOf(5, 10)) + socket.use { server().acceptLoop(it) } + + // 4 failures, a success, 4 more, a success, then a full run of 20 to the cap. + assertThat(socket.acceptCalls).isEqualTo(30) + } + + /** Fails accept() [failures] times with a retryable error, then reports the socket closed. */ private class ScriptedServerSocket( private val failures: Int, + private val succeedAt: Set = emptySet(), + private val message: String = "Too many open files", ) : ServerSocket() { var acceptCalls = 0 private set override fun accept(): Socket { acceptCalls++ - if (acceptCalls <= failures) throw IOException("Too many open files") + // A returned socket would send the loop into handleClient, which needs a database; the + // loop only reads it for null, so an unconnected one is enough to count as a success. + if (acceptCalls in succeedAt) return Socket() + if (acceptCalls <= failures) throw IOException(message) throw SocketException("Socket closed") } } From 30932c19ac2f713d9f12ee2ee5bf1f8af0a7e83d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 22 Aug 2026 00:47:26 -0700 Subject: [PATCH 4/8] ADFA-5242: Apply Spotless formatting A blank line before the @Volatile comment, and the indentation of a KDoc the pre-push hook's spotlessApply corrected. Co-Authored-By: Claude Opus 5 --- .../main/java/com/itsaky/androidide/localWebServer/WebServer.kt | 1 + .../com/itsaky/androidide/localWebServer/AcceptFailureTest.kt | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index e66d249995..f04ac1a3ce 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -116,6 +116,7 @@ class WebServer( // socket then binds anyway a moment later, orphaned, and holds the port until the process // dies. The next start() attempt on that port then fails with "Address already in use." private val lifecycleLock = Any() + // @Volatile: written under lifecycleLock by stop(), but read by the accept loop without it -- // see acceptLoop, which has to see a stop that happened on another thread. @Volatile diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt index e13a8f7433..e3cf8e0efb 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -111,7 +111,7 @@ class AcceptFailureTest { assertThat(socket.acceptCalls).isEqualTo(30) } - /** Fails accept() [failures] times with a retryable error, then reports the socket closed. */ + /** Fails accept() [failures] times with a retryable error, then reports the socket closed. */ private class ScriptedServerSocket( private val failures: Int, private val succeedAt: Set = emptySet(), From 42e6dcfb4c53c87b83124d5fe039ad156d89bbb5 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 14:33:37 -0700 Subject: [PATCH 5/8] ADFA-5242: Flatten the accept loop, and decide stopping from socket state jatezzz: the loop was a try inside a try inside a while, with a finally around both. It is now three functions doing one thing each -- acceptLoop, serveThenClose, reportClientFailure -- with a single level of try in each and the accept result read as an expression. CodeRabbit: shouldStopAccepting matched the exception's message. It now reads stopRequested and socket.isClosed. ServerSocket.close() sets that flag before accept() unblocks, so the state is both authoritative and available, where the message was a guess about wording. A closed-sounding message from a socket that is still open is now retried like any other fault, which has a test. CodeRabbit: the throwable is passed to SLF4J rather than interpolated, so an unexpected accept failure carries its stack trace. The --DS note about placeholders concerned interpolation into the message; a trailing throwable argument is the idiom SLF4J is asking for, and I was wrong to decline this earlier for consistency with the interpolated lines. Eight tests, all driving the real loop: the cap, the reset, a closed socket, a stop before the socket closes, a closed-sounding message retried, and every IOException subtype retried. Co-Authored-By: Claude Opus 5 --- .../androidide/localWebServer/WebServer.kt | 106 +++++++++--------- .../localWebServer/AcceptFailureTest.kt | 90 ++++++++------- 2 files changed, 104 insertions(+), 92 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index f04ac1a3ce..a51d9f6c07 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -384,71 +384,68 @@ class WebServer( internal fun acceptLoop(socket: ServerSocket) { var consecutiveFailures = 0 while (true) { - var clientSocket: Socket? = null - try { + val client = try { if (debugEnabled) log.debug("About to call accept() on the server socket, {}.", socket) - clientSocket = socket.accept() - consecutiveFailures = 0 - - if (debugEnabled) log.debug("Returned from socket accept(), clientSocket is {}.", clientSocket) + socket.accept().also { consecutiveFailures = 0 } } catch (e: IOException) { - // IOException, not SocketException: accept() is declared to throw the wider - // type, and "Too many open files" arrives as a bare IOException. Catching only - // the subtype let that one unwind to start()'s outermost handler, whose finally - // closes the listening socket and the database (ADFA-5242). - // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 - if (debugEnabled) log.debug("Caught IOException '$e'.") - - // stopRequested first: it is authoritative and message-independent, where - // shouldStopAccepting reads the exception text. If a platform ever words a closed - // socket differently, matching on the text alone would spin here forever -- the - // thread never exiting, so start()'s finally never closing the database or freeing - // the port, which is worse than the failure this method exists to survive. - if (stopRequested || shouldStopAccepting(e)) { + // IOException, not SocketException: accept() is declared to throw the wider type, and + // "Too many open files" arrives as a bare IOException. Catching only the subtype let + // that one unwind to start()'s outermost handler, whose finally closes the listening + // socket and the database (ADFA-5242). + if (debugEnabled) log.debug("Caught IOException from accept().", e) + + if (shouldStopAccepting(socket)) { if (debugEnabled) log.debug("WebServer socket closed, shutting down.") break } - consecutiveFailures++ - if (consecutiveFailures >= maxConsecutiveAcceptFailures) { + if (++consecutiveFailures >= maxConsecutiveAcceptFailures) { log.error( - "Accept() failed {} times in a row, last error '{}'; giving up on this listener.", + "Accept() failed {} times in a row; giving up on this listener.", consecutiveFailures, - e.message, + e, ) break } - log.error("Accept() failed: {}", e.message) + log.error("Accept() failed: {}", e.message, e) pauseAfterFailedAccept() continue } - try { - clientSocket?.let { handleClient(it) } - } catch (e: Exception) { - // SLF4J placeholders produce wrong formatting here. --DS, 23-Feb-2026 - if (debugEnabled) log.debug("Caught exception '$e'.") - - if (e is java.net.SocketException && e.message?.contains("Closed", ignoreCase = true) == true) { - if (debugEnabled) log.debug("Client disconnected: {}", e.message) - } else { - log.error("Error handling client: {}", e.message) - clientSocket?.let { client -> - try { - val output = client.outputStream - - sendError(PrintWriter(output, true), output, httpInternalServerError, "Internal Server Error 1") - } catch (e2: Exception) { - log.error("Error sending error response: {}", e2.message) - } - } - } - } - } finally { - clientSocket?.close() - // CodeRabbit objects to the following line because clientSocket may print out as "null." This is intentional. --DS - if (debugEnabled) log.debug("clientSocket was {}.", clientSocket) - } + serveThenClose(client) + } + } + + /** Serves one connection and closes it, whatever happened. */ + private fun serveThenClose(client: Socket) { + try { + handleClient(client) + } catch (e: Exception) { + reportClientFailure(client, e) + } finally { + client.close() + if (debugEnabled) log.debug("clientSocket was {}.", client) + } + } + + /** A client that went wrong: a disconnect is unremarkable, anything else earns a 500 if it can. */ + private fun reportClientFailure( + client: Socket, + e: Exception, + ) { + if (debugEnabled) log.debug("Caught exception while handling a client.", e) + + if (e is java.net.SocketException && e.message?.contains("Closed", ignoreCase = true) == true) { + if (debugEnabled) log.debug("Client disconnected: {}", e.message) + return + } + log.error("Error handling client: {}", e.message, e) + try { + val output = client.outputStream + + sendError(PrintWriter(output, true), output, httpInternalServerError, "Internal Server Error 1") + } catch (e2: Exception) { + log.error("Error sending error response: {}", e2.message, e2) } } @@ -519,10 +516,13 @@ class WebServer( * failure as terminal stops serving documentation until the app restarts, and treating the close * as transient spins the loop against a dead socket. * - * A closed socket reports itself only in the exception's message, hence the string test. + * Decided from state, not from the exception's message: [ServerSocket.close] sets the closed flag + * before accept() unblocks, and [stopRequested] records a stop that arrived before the socket was + * even bound. Matching the message instead -- as this used to -- would spin forever against a + * platform that worded a closed socket differently, never exiting the thread, so start()'s finally + * would never close the database or free the port. */ - internal fun shouldStopAccepting(e: IOException): Boolean = - e is java.net.SocketException && e.message?.contains("Closed", ignoreCase = true) == true + internal fun shouldStopAccepting(socket: ServerSocket): Boolean = stopRequested || socket.isClosed /** * Brief pause after an accept failure that is not the socket closing, so a *persistent* failure diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt index e3cf8e0efb..a1fa70fc17 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -6,16 +6,19 @@ import java.io.IOException import java.net.ServerSocket import java.net.Socket import java.net.SocketException -import java.net.SocketTimeoutException /** * ADFA-5242: one failed accept() must not take documentation down for the rest of the session. * * `ServerSocket.accept()` is declared to throw `IOException`; `SocketException` is one subtype. The - * loop used to catch only that subtype, and the enclosing try has a finally but no catch, so any - * other `IOException` unwound to `start()`'s outermost handler -- whose finally closes the listening - * socket *and* the database. Every later request then failed until the app restarted, with one log - * line as the only trace. + * loop used to catch only that subtype, and the enclosing try had a finally but no catch, so any + * other `IOException` -- a descriptor-exhaustion "Too many open files", say -- unwound to + * `start()`'s outermost handler, whose finally closes the listening socket *and* the database. Every + * later request then failed until the app restarted, with one log line as the only trace. + * + * These drive the real loop through a socket whose accept() fails on demand. Stopping is decided + * from socket state rather than the exception's message, so the scripted socket closes itself when it + * means "stop" -- which is what a real `ServerSocket` does before accept() unblocks. */ class AcceptFailureTest { // Every path is given explicitly: ServerConfig's defaults reach for external storage, which a @@ -35,59 +38,64 @@ class AcceptFailureTest { ) @Test - fun `only the listening socket closing stops the accept loop`() { - assertThat(server().shouldStopAccepting(SocketException("Socket closed"))).isTrue() - assertThat(server().shouldStopAccepting(SocketException("socket is CLOSED"))).isTrue() + fun `a closed socket is what stops accepting`() { + val server = server() + ServerSocket().use { open -> + assertThat(server.shouldStopAccepting(open)).isFalse() + open.close() + assertThat(server.shouldStopAccepting(open)).isTrue() + } } + // stop() can arrive before start() has bound anything, so it records the intent and the loop has + // to honour it even while the socket it was handed is still open. @Test - fun `a transient accept failure is retried, whatever its type`() { - assertThat(server().shouldStopAccepting(SocketException("Connection reset by peer"))).isFalse() - assertThat(server().shouldStopAccepting(SocketTimeoutException("Accept timed out"))).isFalse() - assertThat(server().shouldStopAccepting(IOException("Too many open files"))).isFalse() + fun `a requested stop is honoured before the socket closes`() { + val server = server() + server.stop() + + ServerSocket().use { open -> + assertThat(server.shouldStopAccepting(open)).isTrue() + } } - // A message-less exception must not be mistaken for the close, whose only marker is its message. @Test - fun `an accept failure with no message is retried`() { - assertThat(server().shouldStopAccepting(IOException())).isFalse() - assertThat(server().shouldStopAccepting(SocketException())).isFalse() + fun `a closed socket ends the accept loop at once`() { + val socket = ScriptedServerSocket(failures = 0) + socket.use { server().acceptLoop(it) } + + assertThat(socket.acceptCalls).isEqualTo(1) } - // The helpers above are only worth having if the loop calls them. It did not: both were dead - // code reachable from tests alone, so the fix this PR claimed to make did not exist. These two - // drive the real loop instead of the predicate. @Test fun `a retryable failure does not end the accept loop`() { val socket = ScriptedServerSocket(failures = 2) socket.use { server().acceptLoop(it) } - // Three: two failures retried, then the close that ends it. One would mean the first - // failure escaped the loop -- the ADFA-5242 bug. + // Three: two failures retried, then the close that ends it. One would mean the first failure + // escaped the loop -- the ADFA-5242 bug. assertThat(socket.acceptCalls).isEqualTo(3) } + // A message is not evidence. An exception *saying* the socket closed, from a socket that is still + // open, is some other fault and gets retried like any other. @Test - fun `a closed socket ends the accept loop at once`() { - val socket = ScriptedServerSocket(failures = 0) + fun `a closed-sounding message from a live socket is retried`() { + val socket = ScriptedServerSocket(failures = Int.MAX_VALUE, error = { SocketException("Socket closed") }) socket.use { server().acceptLoop(it) } - assertThat(socket.acceptCalls).isEqualTo(1) + assertThat(socket.acceptCalls).isEqualTo(20) } - // stop() is authoritative where the exception text is not. If a platform words a closed socket - // differently, matching on the message alone would spin here until the cap instead of exiting, - // leaving start()'s finally unrun -- the database open and the port held. + // Every IOException subtype is retried, not just the ones named in the original bug report: a + // SocketTimeoutException reaching start()'s handler would have killed the server just as surely. @Test - fun `a requested stop ends the loop whatever the exception says`() { - val server = server() - // No socket is bound yet, so this only records that a stop was asked for. - server.stop() - - val socket = ScriptedServerSocket(failures = Int.MAX_VALUE, message = "unexpected wording") - socket.use { server.acceptLoop(it) } + fun `a transient failure is retried whatever its type`() { + val socket = + ScriptedServerSocket(failures = Int.MAX_VALUE, error = { java.net.SocketTimeoutException("Accept timed out") }) + socket.use { server().acceptLoop(it) } - assertThat(socket.acceptCalls).isEqualTo(1) + assertThat(socket.acceptCalls).isEqualTo(20) } // A backoff bounds CPU, not volume: without a cap this loop retries a permanent failure forever, @@ -111,11 +119,14 @@ class AcceptFailureTest { assertThat(socket.acceptCalls).isEqualTo(30) } - /** Fails accept() [failures] times with a retryable error, then reports the socket closed. */ + /** + * Fails accept() [failures] times, then closes itself and reports it -- the order a real + * `ServerSocket` uses, since [ServerSocket.close] sets the closed flag before accept() unblocks. + */ private class ScriptedServerSocket( private val failures: Int, private val succeedAt: Set = emptySet(), - private val message: String = "Too many open files", + private val error: () -> IOException = { IOException("Too many open files") }, ) : ServerSocket() { var acceptCalls = 0 private set @@ -123,9 +134,10 @@ class AcceptFailureTest { override fun accept(): Socket { acceptCalls++ // A returned socket would send the loop into handleClient, which needs a database; the - // loop only reads it for null, so an unconnected one is enough to count as a success. + // loop only needs one to hand on, so an unconnected one counts as a success. if (acceptCalls in succeedAt) return Socket() - if (acceptCalls <= failures) throw IOException(message) + if (acceptCalls <= failures) throw error() + close() throw SocketException("Socket closed") } } From 2885a57294e936b38480c9610750d81d141a9f1c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 25 Aug 2026 15:19:01 -0700 Subject: [PATCH 6/8] ADFA-5242: Back off instead of giving up, and stop a client killing the loop Review of #1728 found the retry could still end in the outcome this ticket exists to prevent, by three routes. The 20-failure cap was the main one. Hitting it broke out of the accept loop, which returns to start(), whose finally closes the listening socket AND the database -- documentation dead for the rest of the process. That is the ADFA-5242 symptom, delayed by a second. The cap existed to bound log volume, so the interval now does that job instead: 50 ms doubling to a 2 s ceiling, reached in eight failures, which is well under a line a second against the twenty a fixed 50 ms retry produced. The stack trace goes out once per burst rather than nineteen times, and repeats only when the interval escalates. Nothing ends the loop now except the socket closing or an interrupt. Second, client.close() sat unguarded in serveThenClose's finally, and the call was outside the loop's try. Socket.close() is declared to throw, and a client that resets mid-response makes it do so -- from a finally, so it replaced any in-flight exception and unwound into start(). One bad client took documentation down by the same route as the accept failure. It is caught now, and the loop guards the call as well. Third, an interrupt during the backoff re-armed the flag and continued, so every later sleep threw immediately and the loop hot-spun through its retries at full CPU -- the opposite of the backoff's purpose. An interrupt now ends the loop. Three comments claimed things that were not true and are corrected: stop() then start() was never a recovery path (stopRequested is one-way by design, so a stop before bind cannot leave an orphaned listener; MainActivity constructs a fresh WebServer per start, which is the actual restart), libcore's ServerSocket.close() sets its closed flag AFTER impl.close() so isClosed can still be false when accept() unblocks -- stopRequested is what carries the decision -- and the descriptor pressure is not self-inflicted, since handleClient runs inline and the server holds two descriptors at most. The backoff sleep is now injectable. The four tests that drove the cap spent 4.25 s in real Thread.sleep and asserted a bare 20; the suite now records the intervals instead, so it asserts the escalation and the reset by name and runs in 0.2 s. Six of the eleven cases fail against the unfixed code, each for the reason it is named for. Found in review of PR #1728. --- .../androidide/localWebServer/WebServer.kt | 113 ++++++++++++----- .../localWebServer/AcceptFailureTest.kt | 119 +++++++++++++----- 2 files changed, 171 insertions(+), 61 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index a51d9f6c07..ae18d36e61 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -108,6 +108,9 @@ internal fun joinChunks(chunks: List): ByteArray { class WebServer( private val config: ServerConfig, + // Seam for the accept-retry tests, which drive thousands of simulated failures and must not + // actually sleep for them. Production always gets Thread.sleep. + internal val sleepMs: (Long) -> Unit = { Thread.sleep(it) }, ) { // Guards serverSocket's creation/bind (in start(), on a background thread) against a // concurrent close (in stop(), typically from the main thread on Activity#onDestroy()). @@ -119,6 +122,12 @@ class WebServer( // @Volatile: written under lifecycleLock by stop(), but read by the accept loop without it -- // see acceptLoop, which has to see a stop that happened on another thread. + // + // Never cleared, deliberately: a stop that arrives before the socket is bound has to keep + // start() from binding an orphaned listener, so this is one-way and a stopped instance is + // finished. Restarting means a new WebServer, which is what MainActivity.startWebServer does + // -- it constructs one per start. stop() then start() on the same instance is not a recovery + // path and never was. @Volatile private var stopRequested = false private lateinit var serverSocket: ServerSocket @@ -165,14 +174,16 @@ class WebServer( // Long enough to stop a descriptor-exhaustion spin starving the connections whose closing would // fix it; short enough to be invisible to a user, and never paid on a successful accept. - private val failedAcceptBackoffMs = 50L + private val initialAcceptBackoffMs = 50L - // A backoff alone does not bound anything: 50 ms between attempts is 20 log lines a second, for - // as long as the failure lasts, which on a 5 MiB logcat buffer is every other diagnostic on the - // device gone within the hour. After this many consecutive failures the loop gives up, having - // said so once -- a listener that cannot accept is not serving anyway, and stop()/start() is the - // recovery. Reset by any successful accept. - private val maxConsecutiveAcceptFailures = 20 + // Doubling from 50 ms, the interval reaches this in eight failures, so a failure that persists + // costs well under a line a second instead of twenty. That is what bounds the log volume; an + // earlier version capped the retries instead and gave up after twenty, which closed the listener + // and the database and left documentation dead for the rest of the process -- the ADFA-5242 + // symptom, delayed by a second. A listener that cannot accept now keeps trying: the descriptor + // pressure that causes this comes from the rest of the process (Gradle, Termux, the editor) and + // clears on its own timescale, not ours. + private val maxAcceptBackoffMs = 2_000L private val httpInternalServerError = 500 private val httpNotFound = 404 @@ -382,12 +393,16 @@ class WebServer( * loop needs neither. The bug it fixes was invisible precisely because nothing could reach here. */ internal fun acceptLoop(socket: ServerSocket) { - var consecutiveFailures = 0 + // 0 means the last accept() succeeded; any other value is the interval the next retry waits. + var backoffMs = 0L while (true) { val client = try { if (debugEnabled) log.debug("About to call accept() on the server socket, {}.", socket) - socket.accept().also { consecutiveFailures = 0 } + socket.accept().also { + backoffMs = 0L + if (debugEnabled) log.debug("Returned from accept(), clientSocket is {}.", it) + } } catch (e: IOException) { // IOException, not SocketException: accept() is declared to throw the wider type, and // "Too many open files" arrives as a bare IOException. Catching only the subtype let @@ -399,20 +414,37 @@ class WebServer( if (debugEnabled) log.debug("WebServer socket closed, shutting down.") break } - if (++consecutiveFailures >= maxConsecutiveAcceptFailures) { - log.error( - "Accept() failed {} times in a row; giving up on this listener.", - consecutiveFailures, - e, - ) + + val previous = backoffMs + backoffMs = + if (previous == 0L) { + initialAcceptBackoffMs + } else { + minOf(previous * 2, maxAcceptBackoffMs) + } + // The stack trace goes out once per burst, on the first failure. Repeats say only + // that it is still failing, and only when the interval changes: nineteen identical + // traces told nobody anything the first one had not. + if (previous == 0L) { + log.error("Accept() failed, retrying in {} ms: {}", backoffMs, e.message, e) + } else if (backoffMs != previous) { + log.error("Accept() still failing, backing off to {} ms: {}", backoffMs, e.message) + } + + if (!pauseAfterFailedAccept(backoffMs)) { + log.info("Accept loop interrupted while backing off; shutting down.") break } - log.error("Accept() failed: {}", e.message, e) - pauseAfterFailedAccept() continue } - serveThenClose(client) + // A client cannot be allowed to end the loop: anything escaping here reaches start()'s + // handler, whose finally closes the listener and the database for everyone. + try { + serveThenClose(client) + } catch (e: Exception) { + log.error("Serving a client threw past its own handler; the listener stays up: {}", e.message, e) + } } } @@ -423,7 +455,15 @@ class WebServer( } catch (e: Exception) { reportClientFailure(client, e) } finally { - client.close() + // close() is declared to throw, and a client that reset mid-response makes it do so. That + // exception used to leave this function -- from a finally, so it replaced any in-flight one + // -- and unwound past the accept loop into start(), taking the listener and the database + // down with it. "Whatever happened" includes this. + try { + client.close() + } catch (e: IOException) { + if (debugEnabled) log.debug("Cannot close the client socket; it is being discarded anyway.", e) + } if (debugEnabled) log.debug("clientSocket was {}.", client) } } @@ -516,28 +556,37 @@ class WebServer( * failure as terminal stops serving documentation until the app restarts, and treating the close * as transient spins the loop against a dead socket. * - * Decided from state, not from the exception's message: [ServerSocket.close] sets the closed flag - * before accept() unblocks, and [stopRequested] records a stop that arrived before the socket was - * even bound. Matching the message instead -- as this used to -- would spin forever against a - * platform that worded a closed socket differently, never exiting the thread, so start()'s finally - * would never close the database or free the port. + * Decided from state, not from the exception's message, which would spin forever against a + * platform that worded a closed socket differently. [stopRequested] is what carries the decision: + * libcore's [ServerSocket.close] calls `impl.close()` *before* setting its closed flag, so + * accept() can unblock while [ServerSocket.isClosed] is still false, and [stop] sets + * [stopRequested] before closing for exactly that reason. `isClosed` is the belt to that braces + * -- it also covers a close that did not come through [stop] at all. */ internal fun shouldStopAccepting(socket: ServerSocket): Boolean = stopRequested || socket.isClosed /** - * Brief pause after an accept failure that is not the socket closing, so a *persistent* failure - * cannot spin this loop at full tilt while it clears. "Too many open files" is the realistic one, - * and it is self-inflicted in the worst way: the descriptors this server is waiting to reuse are - * the ones its own in-flight connections hold, so retrying flat out both floods the log and - * competes with the work that would free them. Only the failure path ever waits. + * Waits [delayMs] before the next accept() attempt, so a persistent failure cannot spin this loop + * at full tilt while it clears. Only the failure path ever waits. + * + * "Too many open files" is the realistic cause, and it is not this server's own doing: + * [handleClient] runs inline on this thread, so exactly one client socket is ever open and the + * listener holds two descriptors in total. The pressure comes from the rest of the process -- + * Gradle, Termux, the editor -- and clears on its timescale, which is why the interval escalates + * rather than the retries running out. + * + * Returns false if the wait was interrupted, which the caller must treat as shutdown: re-arming + * the flag and carrying on made every later sleep throw at once, turning the backoff into a hot + * spin -- the opposite of its purpose. */ - private fun pauseAfterFailedAccept() { + private fun pauseAfterFailedAccept(delayMs: Long): Boolean = try { - Thread.sleep(failedAcceptBackoffMs) + sleepMs(delayMs) + true } catch (e: InterruptedException) { Thread.currentThread().interrupt() + false } - } /** * Reads a single line from the stream (bytes until newline). Same stream is used for headers diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt index a1fa70fc17..cc68ed2be0 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -23,18 +23,27 @@ import java.net.SocketException class AcceptFailureTest { // Every path is given explicitly: ServerConfig's defaults reach for external storage, which a // JVM test has no stub for. - private fun server() = + // Every delay the loop asked for, in order. Recording instead of sleeping keeps a test that drives + // hundreds of failures instant, and makes the backoff itself assertable. + private val delays = mutableListOf() + + private fun server(onSleep: (Long) -> Unit = {}) = WebServer( - ServerConfig( - port = 0, - databasePath = "/nonexistent/test.db", - fileDirPath = "/tmp", - debugDatabasePath = "/nonexistent/debug.db", - debugEnablePath = "/nonexistent/debug-flag", - experimentsEnablePath = "/nonexistent/exp-flag", - clearCacheEnablePath = "/nonexistent/cs0-flag", - projectDatabasePath = "/nonexistent/recent-projects.db", - ), + sleepMs = { + delays += it + onSleep(it) + }, + config = + ServerConfig( + port = 0, + databasePath = "/nonexistent/test.db", + fileDirPath = "/tmp", + debugDatabasePath = "/nonexistent/debug.db", + debugEnablePath = "/nonexistent/debug-flag", + experimentsEnablePath = "/nonexistent/exp-flag", + clearCacheEnablePath = "/nonexistent/cs0-flag", + projectDatabasePath = "/nonexistent/recent-projects.db", + ), ) @Test @@ -81,10 +90,10 @@ class AcceptFailureTest { // open, is some other fault and gets retried like any other. @Test fun `a closed-sounding message from a live socket is retried`() { - val socket = ScriptedServerSocket(failures = Int.MAX_VALUE, error = { SocketException("Socket closed") }) + val socket = ScriptedServerSocket(failures = 30, error = { SocketException("Socket closed") }) socket.use { server().acceptLoop(it) } - assertThat(socket.acceptCalls).isEqualTo(20) + assertThat(socket.acceptCalls).isEqualTo(31) } // Every IOException subtype is retried, not just the ones named in the original bug report: a @@ -92,41 +101,93 @@ class AcceptFailureTest { @Test fun `a transient failure is retried whatever its type`() { val socket = - ScriptedServerSocket(failures = Int.MAX_VALUE, error = { java.net.SocketTimeoutException("Accept timed out") }) + ScriptedServerSocket(failures = 5, error = { java.net.SocketTimeoutException("Accept timed out") }) socket.use { server().acceptLoop(it) } - assertThat(socket.acceptCalls).isEqualTo(20) + assertThat(socket.acceptCalls).isEqualTo(6) } - // A backoff bounds CPU, not volume: without a cap this loop retries a permanent failure forever, - // logging every 50 ms. Giving up leaves a listener that was not serving anyway, and says so once. + // The loop used to give up after twenty consecutive failures, which returned to start(), whose + // finally closes the listening socket *and* the database -- documentation dead for the rest of the + // process. That is the ADFA-5242 symptom the retry exists to prevent, so there is no giving up: + // only the socket closing ends the loop. @Test - fun `a permanent failure is abandoned rather than retried forever`() { - val socket = ScriptedServerSocket(failures = Int.MAX_VALUE) + fun `a persistent failure is retried past any cap, until the socket closes`() { + val socket = ScriptedServerSocket(failures = 500) socket.use { server().acceptLoop(it) } - assertThat(socket.acceptCalls).isEqualTo(20) + assertThat(socket.acceptCalls).isEqualTo(501) } - // A successful accept means the condition cleared, so the count towards the cap starts over -- - // otherwise a server up for long enough accumulates unrelated failures and stops on the twentieth. + // What bounds the cost of a persistent failure is the interval, not a cap on attempts. @Test - fun `a success between failures resets the count`() { - val socket = ScriptedServerSocket(failures = Int.MAX_VALUE, succeedAt = setOf(5, 10)) + fun `the retry interval doubles from 50 ms and stops at 2 seconds`() { + val socket = ScriptedServerSocket(failures = 20) + socket.use { server().acceptLoop(it) } + + assertThat(delays.take(7)).containsExactly(50L, 100L, 200L, 400L, 800L, 1600L, 2000L).inOrder() + assertThat(delays.distinct().max()).isEqualTo(2000L) + } + + // A successful accept means the condition cleared, so the next failure starts over at 50 ms rather + // than inheriting an interval the server has already recovered from. + @Test + fun `a success resets the retry interval`() { + val socket = ScriptedServerSocket(failures = 12, succeedAt = setOf(5)) + socket.use { server().acceptLoop(it) } + + // Four failures before the success, so the fifth delay is the one after it. + assertThat(delays.take(4)).containsExactly(50L, 100L, 200L, 400L).inOrder() + assertThat(delays[4]).isEqualTo(50L) + } + + // Re-arming the interrupt flag and carrying on made every later sleep throw immediately, so the + // backoff became a hot spin -- the opposite of its purpose. An interrupt ends the loop. + @Test + fun `an interrupt ends the accept loop instead of spinning`() { + val socket = ScriptedServerSocket(failures = 500) + socket.use { server(onSleep = { throw InterruptedException("shutting down") }).acceptLoop(it) } + + assertThat(socket.acceptCalls).isEqualTo(1) + assertThat(Thread.interrupted()).isTrue() + } + + // Socket.close() is declared to throw and a reset client makes it do so. It ran in a finally, so + // the exception replaced whatever was in flight and unwound past this loop into start(), whose + // own finally closes the listener and the database -- one bad client killing documentation for + // the session, by the same route as the accept failure this ticket is about. + @Test + fun `a client whose close fails does not end the accept loop`() { + val socket = + ScriptedServerSocket( + failures = 1, + succeedAt = setOf(1), + client = { + object : Socket() { + override fun close() = throw IOException("Broken pipe") + } + }, + ) socket.use { server().acceptLoop(it) } - // 4 failures, a success, 4 more, a success, then a full run of 20 to the cap. - assertThat(socket.acceptCalls).isEqualTo(30) + // Two: the client whose close() threw, then the close that ends the loop. One would mean the + // throw escaped. + assertThat(socket.acceptCalls).isEqualTo(2) } /** - * Fails accept() [failures] times, then closes itself and reports it -- the order a real - * `ServerSocket` uses, since [ServerSocket.close] sets the closed flag before accept() unblocks. + * Fails accept() [failures] times, then closes itself and reports it. + * + * Closing before throwing is what makes [WebServer.shouldStopAccepting]'s `isClosed` arm fire + * here. A real libcore [ServerSocket.close] is the other way round -- `impl.close()` runs before + * the closed flag is set, so accept() can unblock while `isClosed()` is still false -- which is + * why the production path leans on `stopRequested`, set by [WebServer.stop] before it closes. */ private class ScriptedServerSocket( private val failures: Int, private val succeedAt: Set = emptySet(), private val error: () -> IOException = { IOException("Too many open files") }, + private val client: () -> Socket = { Socket() }, ) : ServerSocket() { var acceptCalls = 0 private set @@ -135,7 +196,7 @@ class AcceptFailureTest { acceptCalls++ // A returned socket would send the loop into handleClient, which needs a database; the // loop only needs one to hand on, so an unconnected one counts as a success. - if (acceptCalls in succeedAt) return Socket() + if (acceptCalls in succeedAt) return client() if (acceptCalls <= failures) throw error() close() throw SocketException("Socket closed") From e81a3e22793d3107c1f396893fe7a5da0d78d582 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 15:44:09 -0700 Subject: [PATCH 7/8] ADFA-5242: Decay the backoff, keep an Error off the listener, keep the log alive Three gaps in the loop I rewrote last round. Zeroing the backoff on every success defeated it for the common case. An intermittent accept failure -- a client RSTing between SYN and accept(), which a WebView cancelling a documentation request produces routinely -- was "first failure of a burst" every time, so each one logged a full stack trace and stalled the listener 50 ms. That is the flood the backoff was introduced to stop, and the test asserted it. A success now halves the interval instead: a flapping listener keeps most of its backoff, a recovered one is back to zero within a few clean accepts, and both directions are pinned by tests. All three guard layers caught Exception, so an Error still killed the server through the very path this ticket closed. joinChunks allocates a whole row in one array at 1 MB per chunk and Pebble renders recursively, so one large row can raise OutOfMemoryError and a bad template a StackOverflowError; either reached start()'s finally and closed the listening socket and the database. The per-client guard catches Throwable now. At the ceiling the interval stops changing, so neither log branch fired again: a permanent failure produced seven lines and then silence, while the loop by design never gives up and the open backlog leaves clients hanging rather than failing fast. A heartbeat every fifteen retries -- about one line per 30 s -- keeps it visible. The comment claiming the cap bounded the log to "well under a line a second" had drifted from what the code did. Also: the loop head tests shouldStopAccepting rather than `while (true)`, because stop() logs and swallows a throwing serverSocket.close(), which leaves closed == false and had the loop serving on past a requested shutdown; start()'s finally guards serverSocket.close() so a throw there no longer skips database.close(); its handler logs the throwable rather than only the message, which is the diagnosability gap this ticket's own description cites; and the interrupt test's flag is cleared in @After so an assertion failure cannot leak it onto the JUnit worker. 312 app tests pass. Found in review of PR #1728. --- .../androidide/localWebServer/WebServer.kt | 56 +++++++++++++++++-- .../localWebServer/AcceptFailureTest.kt | 36 ++++++++++-- 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index ae18d36e61..8e859ce77d 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -185,6 +185,10 @@ class WebServer( // clears on its own timescale, not ours. private val maxAcceptBackoffMs = 2_000L + // Retries between heartbeat lines once the interval stops growing: 15 x 2 s is one line every + // 30 seconds while a failure persists. + private val acceptHeartbeatRetries = 15L + private val httpInternalServerError = 500 private val httpNotFound = 404 @@ -393,14 +397,27 @@ class WebServer( * loop needs neither. The bug it fixes was invisible precisely because nothing could reach here. */ internal fun acceptLoop(socket: ServerSocket) { - // 0 means the last accept() succeeded; any other value is the interval the next retry waits. + // 0 means accept() has been succeeding; any other value is the interval the next retry waits. var backoffMs = 0L - while (true) { + // Retries spent at the ceiling, so a failure that never clears keeps saying so. Without this + // the escalation log went silent for good once the interval stopped changing. + var retriesAtCeiling = 0L + // Checked in the loop head, not only in the catch: stop() logs and swallows a throwing + // serverSocket.close(), which leaves closed == false, so accept() kept succeeding and the loop + // served on past a requested shutdown, holding the database open (ADFA-5242 review). + while (!shouldStopAccepting(socket)) { val client = try { if (debugEnabled) log.debug("About to call accept() on the server socket, {}.", socket) socket.accept().also { - backoffMs = 0L + // Halved, not zeroed. Zeroing made every failure "the first of a burst", so an + // intermittent one -- a client that RSTs between SYN and accept(), which a WebView + // cancelling a request produces routinely -- logged a full stack trace and stalled + // the listener 50 ms every single time. That is the flood the backoff exists to + // stop. Decaying means a flapping listener keeps most of its interval and a + // genuinely recovered one is back to zero within a few accepts. + backoffMs = if (backoffMs <= initialAcceptBackoffMs) 0L else backoffMs / 2 + if (backoffMs == 0L) retriesAtCeiling = 0L if (debugEnabled) log.debug("Returned from accept(), clientSocket is {}.", it) } } catch (e: IOException) { @@ -427,8 +444,23 @@ class WebServer( // traces told nobody anything the first one had not. if (previous == 0L) { log.error("Accept() failed, retrying in {} ms: {}", backoffMs, e.message, e) + retriesAtCeiling = 0L } else if (backoffMs != previous) { log.error("Accept() still failing, backing off to {} ms: {}", backoffMs, e.message) + } else { + // At the ceiling the interval stops changing, so neither branch above fires again. + // A heartbeat roughly every 30 s keeps a permanent failure visible without + // returning to a line per retry -- the loop never gives up, so the log must not + // either. + retriesAtCeiling++ + if (retriesAtCeiling % acceptHeartbeatRetries == 0L) { + log.error( + "Accept() still failing after {} retries at {} ms: {}", + retriesAtCeiling, + backoffMs, + e.message, + ) + } } if (!pauseAfterFailedAccept(backoffMs)) { @@ -440,9 +472,14 @@ class WebServer( // A client cannot be allowed to end the loop: anything escaping here reaches start()'s // handler, whose finally closes the listener and the database for everyone. + // + // Throwable, not Exception. joinChunks allocates the whole row in one array (1 MB per + // chunk) and Pebble renders recursively, so one large row can raise OutOfMemoryError and a + // pathological template a StackOverflowError -- neither an Exception, both fatal to the + // listener through exactly the path this ticket exists to close. try { serveThenClose(client) - } catch (e: Exception) { + } catch (e: Throwable) { log.error("Serving a client threw past its own handler; the listener stays up: {}", e.message, e) } } @@ -527,10 +564,17 @@ class WebServer( acceptLoop(serverSocket) } catch (e: Exception) { - log.error("Error: {}", e.message) + log.error("WebServer stopped on an unhandled exception: {}", e.message, e) } finally { if (::serverSocket.isInitialized) { - serverSocket.close() + // Guarded for the same reason serveThenClose guards the client socket: close() is + // declared to throw, and a throw here skipped database.close() and the traffic-stats + // tag below, leaving the SQLite handle open for the life of the process. + try { + serverSocket.close() + } catch (e: IOException) { + log.error("Cannot close the server socket: {}", e.message, e) + } } // database is opened before the stopRequested check that can abort start() // early (and before the accept loop on every other exit path), so it must be diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt index cc68ed2be0..bccccfc943 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/AcceptFailureTest.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.localWebServer import com.google.common.truth.Truth.assertThat +import org.junit.After import org.junit.Test import java.io.IOException import java.net.ServerSocket @@ -21,6 +22,14 @@ import java.net.SocketException * means "stop" -- which is what a real `ServerSocket` does before accept() unblocks. */ class AcceptFailureTest { + // The interrupt test sets the thread's interrupt flag; clearing it only via that test's own + // assertion means a failure earlier in the test leaks the flag onto the JUnit worker, where the + // next test that blocks fails for reasons that have nothing to do with it. + @After + fun clearInterrupt() { + Thread.interrupted() + } + // Every path is given explicitly: ServerConfig's defaults reach for external storage, which a // JVM test has no stub for. // Every delay the loop asked for, in order. Recording instead of sleeping keeps a test that drives @@ -129,16 +138,33 @@ class AcceptFailureTest { assertThat(delays.distinct().max()).isEqualTo(2000L) } - // A successful accept means the condition cleared, so the next failure starts over at 50 ms rather - // than inheriting an interval the server has already recovered from. + // A success halves the interval rather than zeroing it. + // + // This asserted a reset to 50 ms until the review pointed out what that costs: a listener that + // fails intermittently -- a client RSTing between SYN and accept(), which a WebView cancelling a + // documentation request produces routinely -- was treated as "first failure of a burst" every + // time, so every occurrence logged a full stack trace and stalled the listener 50 ms. Decay keeps + // most of the interval while a listener is flapping, and still returns to zero within a few clean + // accepts once it has genuinely recovered. @Test - fun `a success resets the retry interval`() { + fun `a success decays the retry interval instead of clearing it`() { val socket = ScriptedServerSocket(failures = 12, succeedAt = setOf(5)) socket.use { server().acceptLoop(it) } - // Four failures before the success, so the fifth delay is the one after it. + // Four failures walk 50..400; the success halves 400 to 200, so the next failure doubles to 400. assertThat(delays.take(4)).containsExactly(50L, 100L, 200L, 400L).inOrder() - assertThat(delays[4]).isEqualTo(50L) + assertThat(delays[4]).isEqualTo(400L) + } + + // Recovery still gets all the way back to zero, so a later isolated failure starts cheap. + @Test + fun `enough clean accepts return the interval to its starting point`() { + val socket = ScriptedServerSocket(failures = 20, succeedAt = setOf(2, 3, 4, 5)) + socket.use { server().acceptLoop(it) } + + // One failure (50), then four successes decay 50 -> 0, so the next failure starts at 50 again. + assertThat(delays.take(1)).containsExactly(50L) + assertThat(delays[1]).isEqualTo(50L) } // Re-arming the interrupt flag and carrying on made every later sleep throw immediately, so the From 661e02688dae1200da69905ddf29ed98667e990b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 18:11:58 -0700 Subject: [PATCH 8/8] ADFA-5242: Clear the ceiling counter on any success, and keep the type on repeats Two CodeRabbit findings on the accept loop. retriesAtCeiling was cleared only when a success brought backoffMs all the way to zero. The counter means "consecutive retries at the ceiling", and a successful accept ends that run whatever interval is left, so the old rule banked a stale count: at the ceiling with 14 retries recorded, one success followed by a return to the ceiling fired the 30-second heartbeat on the very next retry instead of the fifteenth. The repeat log lines carried e.message only. That was deliberate -- the stack trace goes out once per burst and repeats stay terse -- but the type went with it. A burst can change cause mid-flight (EMFILE giving way to ECONNABORTED) and the two lines would read identically, and message is null for some IOExceptions, which logged a bare "null". They now carry e.toString(), which keeps the type without the trace. Not covered by a test: both are logging cadence, and the app module's test classpath has slf4j-api with no provider, so nothing observes a log line. Adding a backend is a new dependency. The 12 existing AcceptFailureTest cases still pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU --- .../androidide/localWebServer/WebServer.kt | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 8e859ce77d..ee4577c553 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -417,7 +417,12 @@ class WebServer( // stop. Decaying means a flapping listener keeps most of its interval and a // genuinely recovered one is back to zero within a few accepts. backoffMs = if (backoffMs <= initialAcceptBackoffMs) 0L else backoffMs / 2 - if (backoffMs == 0L) retriesAtCeiling = 0L + // Reset on every success, not only once the interval reaches zero. The counter + // means "consecutive retries at the ceiling", and an accept that succeeds ends + // that run whatever the interval still is. Clearing it only at zero left a stale + // count behind: at the ceiling with 14 retries banked, one success then a return + // to the ceiling fired the heartbeat on the next retry instead of the fifteenth. + retriesAtCeiling = 0L if (debugEnabled) log.debug("Returned from accept(), clientSocket is {}.", it) } } catch (e: IOException) { @@ -441,12 +446,15 @@ class WebServer( } // The stack trace goes out once per burst, on the first failure. Repeats say only // that it is still failing, and only when the interval changes: nineteen identical - // traces told nobody anything the first one had not. + // traces told nobody anything the first one had not. They do carry e.toString() + // rather than e.message, so the type is still there -- a burst can change cause + // mid-flight (EMFILE giving way to ECONNABORTED), and message alone is null for + // some IOExceptions, which logged a bare "null". if (previous == 0L) { log.error("Accept() failed, retrying in {} ms: {}", backoffMs, e.message, e) retriesAtCeiling = 0L } else if (backoffMs != previous) { - log.error("Accept() still failing, backing off to {} ms: {}", backoffMs, e.message) + log.error("Accept() still failing, backing off to {} ms: {}", backoffMs, e.toString()) } else { // At the ceiling the interval stops changing, so neither branch above fires again. // A heartbeat roughly every 30 s keeps a permanent failure visible without @@ -458,7 +466,7 @@ class WebServer( "Accept() still failing after {} retries at {} ms: {}", retriesAtCeiling, backoffMs, - e.message, + e.toString(), ) } }