Fix flaky remote-client tests: surface admin server bind errors and avoid ephemeral ports - #214
Conversation
Express 5 passes listen errors to the listen callback, so a failed bind (e.g. EADDRINUSE) resolved start() as if the server were running. Later stop() then failed with 'Server is not running', and every client got ECONNREFUSED. Now start() rejects with the bind error and leaves the admin server stopped, so it can be stopped or started again.
The node tests started every admin server on the default port 45454, which is inside the default Linux ephemeral port range (32768-60999). An outgoing connection from another test can take that port, so the admin server fails to bind and every remote-client test after it fails with ECONNREFUSED. Tests now use port 30454, which isn't in the default ephemeral range on Linux, macOS or Windows.
|
Thanks @philipp-winterle, merged. Looks like this came in in the recent upgrade to Express v5, good catch 👍 I appreciate the engagement with the project, but just as feedback: opening a blast of 5 AI generated PRs like this as an opening move is a bit much. If possible please open issues before PRs for debatable cases (not bug fixes) like httptoolkit/httptoolkit-server#246 and httptoolkit/httptoolkit-ui#271 so they can be discussed, and I'd recommend in general not opening loads of PRs at once in projects like this. I'd also really strongly recommend writing the text yourself in PR descriptions. The massive wall of text here is clearly straight from an LLM, it has very little information in a lot of words, which is just outsourcing the work of extracting the actual meaning there (5 times) to maintainers, instead of writing a clear description yourself. An example to illustrate:
Anyway, regardless of that, thank you for contributing! In case you're not aware HTTP Toolkit Pro is free for all contributors to all https://github.com/httptoolkit repos. I've just set up an account with a year free linked to your git commit email, just click 'Get Pro' then 'Log into existing account' to get started. |
|
You are absolutely right. Thanks for the feedback. I stumbled over the issue and fixed it and already reviewed it. But I was too lazy to setup the pr so I gave the job to the AI. But I do not like the ai text as well. Will write the desc on my own next time. Sry. Thanks for the pro <3 |
The CI run for #213 (run 35230056297) failed with 30 failures, all of them
Failed to connect to admin server at http://127.0.0.1:45454(plusServer is not runningfrom the teardown hooks), starting atplugins.spec.ts. The full node suite passes locally on the same code, and main on the same base was green.What the log shows
Every admin server start from
plugins.spec.tsonwards failed to bind its port, butstart()still resolved. Express 5 passes listen errors to theapp.listen()callback, andAdminServer.start()resolved from that callback unconditionally, so a failed bind looked like a running server. Clients then gotECONNREFUSED, andstop()threwServer is not running.Likely cause (inferred, not proven)
45454 is inside the default Linux ephemeral port range (32768-60999). An outgoing connection from another test can take it as its local port, and then the admin server can't bind it. That fits a one-off failure that later passes, but I can't prove it from the log.
Changes
start()now rejects with the bind error (e.g.EADDRINUSE) and leaves the server stopped, sostop()works and a laterstart()on another port succeeds. Covered by three new tests that fail without the fix.with-admin) are untouched.Even if the cause is something else, (1) means the next failure of this kind reports one clear
EADDRINUSEinstead of 30 unrelated-looking connection errors.Testing
🤖 Generated with Claude Code
https://claude.ai/code/session_01SzQcMX9TVncn4dz6PhGuWn