ADFA-5035: Fix WebServer occasionally failing to start with EADDRINUSE - #1634
Conversation
start() binds serverSocket on a background thread (launched from MainActivity.startWebServer()); stop() (called from onDestroy(), main thread) only closes serverSocket if it's already initialized. If stop() runs before start() reaches bind(), it's a silent no-op -- start() then binds anyway a moment later, orphaned, holding the port until the process dies. The next start() attempt on that port fails with "Address already in use." Synchronize start()'s bind and stop()'s close on a shared lock, and have stop() record that a stop was requested so start() can abort before binding if one arrived first. Closes the race window instead of relying on timing. Also renamed HTTP_INTERNAL_SERVER_ERROR/HTTP_NOT_FOUND to camelCase (pre-existing ktlint property-naming violations, unrelated to this fix but required once this file falls under the Spotless ratchet).
WebServer.kt was space-indented and had several pre-existing ktlint violations (max-line-length, snake_case sql_query). Touching the file in the previous commit pulled it under the Spotless ratchet, so bring it into compliance: tabs, wrapped long lines/comments, and sql_query -> sqlQuery. No behavioral change.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
📝 Walkthrough
Walkthrough
ChangesWebServer lifecycle and request handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 639-645: The response-start state is updated too late, allowing
error handling to send a second response if a write fails. In WebServer.kt lines
639-645, update realHandleBsEndpoint(), and in lines 725-728, update
realHandlePrEndpoint(), to accept mutable response state and set it immediately
before the first header or body write, including before writeNormalToClient().
- Around line 149-184: Add lifecycle tests for WebServer.start and stop using a
controllable bind point: verify stop() invoked before binding prevents any
listener from remaining bound, and concurrently stopping during bind releases
the port so it can be reused. Keep the tests non-UI and deterministic by
synchronizing the bind/stop phases rather than relying on timing or sleeps.
- Around line 133-140: Update WebServer.stop() so calling it before start
remains a no-op as documented: check whether serverSocket is initialized before
setting stopRequested, and return without mutating lifecycle state when it is
not. Preserve the existing shutdown behavior for an initialized server.
- Around line 167-180: Update the startup cleanup surrounding the database
opened in the WebServer start flow to close the initialized database when
stopRequested causes the synchronized lifecycle check to return, including the
corresponding cleanup path noted near the later startup handling. Preserve
serverSocket cleanup and avoid closing an uninitialized database.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 666aac53-7fe4-4a29-b0ba-560524156e3f
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
- Close database in start()'s finally alongside serverSocket. It was
opened before the stopRequested check that can now abort start()
early, and was never closed on any other shutdown path either
(normal accept-loop exit, exception) -- isInitialized guards the
case where opening it failed and this finally still runs.
- Correct stop()'s doc comment: it's no longer a full no-op before
start() binds -- it still records the stop request so start() can
abort before binding, which is the fix itself. Only the socket-close
side stays a no-op in that case. (Reverting the behavior back to a
literal no-op, as literally suggested, would reopen the exact
EADDRINUSE race this ticket fixes.)
- Add WebServerTest: deterministic coverage for both lifecycle
orderings (stop-before-start aborts the bind; start-then-stop frees
the port for reuse), synchronized via the port's own bind/connect
behavior rather than fixed sleeps.
Skipped: the outputStarted-timing suggestion for realHandleBsEndpoint/
realHandlePrEndpoint is the same CodeRabbit finding already considered
and explicitly rejected in an existing code comment ("I disagree...
--DS, 23-Feb-2026"); out of scope to unilaterally revisit here.
outputStarted was only set after realHandleBsEndpoint/realHandlePrEndpoint returned, so if writeNormalToClient threw partway through (e.g. after the status line but mid-body), the catch block still saw outputStarted=false and sent a second, well-formed response on top of the already-partially-written one. Pass a markOutputStarted callback into both functions and invoke it right before the first write, so the caller's flag reflects reality even when the write itself then fails. Removes the "I disagree with CodeRabbit's message" comment that had left this finding unaddressed.
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc for the public test declarations.
The current block comments do not generate KDoc. Document
WebServerTest,setup(),tearDown(), and the public test methods with concise KDoc that states their lifecycle contract.As per coding guidelines, “Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units.”
Also applies to: 25-43, 66-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt` around lines 19 - 24, The public declarations in WebServerTest lack KDoc. Replace the class-level block comment and add concise KDoc to WebServerTest, setup(), tearDown(), and each public test method, documenting their lifecycle contracts and the relevant serialized start/stop behavior without changing test logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt`:
- Around line 95-102: Update the cleanup assertions around serverThread.join in
the WebServerTest case to verify serverThread.isAlive is false before asserting
assertPortIsFree(port). Keep the existing join and port-reuse assertion, but
explicitly fail the test if the server thread remains running after stop().
- Around line 9-13: Update WebServerTest to use JUnit Jupiter annotations and
Truth assertions instead of org.junit.After, org.junit.Before, org.junit.Test,
and org.junit.Assert methods. Add the libs.tests.junit.jupiter dependency in
app/build.gradle.kts if it is not already available, then replace the test
lifecycle annotations and assertions while preserving the existing test
behavior.
---
Nitpick comments:
In `@app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt`:
- Around line 19-24: The public declarations in WebServerTest lack KDoc. Replace
the class-level block comment and add concise KDoc to WebServerTest, setup(),
tearDown(), and each public test method, documenting their lifecycle contracts
and the relevant serialized start/stop behavior without changing test logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0117faf8-5a85-49bd-864f-adfb311af5d9
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.ktapp/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
|
The One gap, in private var webServer: WebServer? = null // :93
private fun startWebServer() { // :439
lifecycleScope.launch(Dispatchers.IO) {
val server = WebServer(ServerConfig(...)) // PebbleEngine build + 3x File.exists()
webServer = server
server.start()
}
}
override fun onDestroy() { webServer?.stop() } // :459 -- main threadTwo windows
Either path reproduces ADFA-5035's symptom identically: a listener holding 6174 with nothing able to stop it, then Suggested fix -- construct on the main thread, launch only private fun startWebServer() {
val server = try {
WebServer(ServerConfig(databasePath = Environment.DOC_DB.absolutePath, ...))
} catch (e: Exception) { log.error("Failed to create WebServer", e); return }
webServer = server // main thread, before any launch
lifecycleScope.launch(Dispatchers.IO) {
try { server.start() } finally { webServer = null }
}
}
Happy either way on sequencing -- fold it into this PR, or land this and follow up before ADFA-5035 moves to Done. Just shouldn't close the ticket on this alone. Non-blocking nits:
Pre-existing, out of scope, maybe worth a ticket: the debug-db hot-swap at |
|
Reviewed by checking out the branch in an isolated worktree and verifying, not just reading:
LGTM (self-review — GitHub won't let me approve my own PR). |
2 participants
Summary
WebServer.start()bindsserverSocketon a background thread (launched fromMainActivity.startWebServer());stop()(called fromonDestroy(), main thread) only closes it if already initialized -- otherwise it's a silent no-op.stop()runs beforestart()reachesbind(), that no-op letsstart()bind anyway a moment later, orphaned, holding the port until the process dies. The nextstart()attempt on that port then fails withBindException: Address already in use(EADDRINUSE) -- matching ADFA-5035's "occasionally fails to start" report.start()'s bind step andstop()'s close step on a shared lock, and havestop()record that a stop was requested sostart()can abort before binding if one arrived first. Closes the race window instead of relying on timing (reuseAddress = truealone doesn't help here -- it only helps rebind a socket lingering inTIME_WAIT, not one still actively held by an orphaned listener).sql_query->sqlQuery,HTTP_*-> camelCase) required once it was touched -- no behavioral change.Test plan
:app:compileV8DebugKotlinsucceeds.spotlessCheckpasses.