feat(cli): auto-retry next port when serve port is in use - #6513
Merged
Conversation
When `qwen serve` or `npm run dev:daemon` encounters EADDRINUSE on the default port (4170), automatically try the next available port (up to 10 attempts) instead of failing immediately. This allows running multiple daemon instances side-by-side without manual port management. - run-qwen-serve.ts: replace single listen() with recursive tryListen() that retries on EADDRINUSE; --port 0 (ephemeral) skips retry - daemon-dev.js: pre-scan for an available port via net probe before spawning the daemon child, ensuring health-poll and web-shell target the correct URL - Tests: retry-then-succeed, non-EADDRINUSE immediate-fail, and existing all-ports-exhausted test all pass (138 tests)
Collaborator
|
Thanks for the PR, @wenshao! Template: has Motivation, Changes, and Reviewer Test Plan with clear verification steps. Missing Evidence (Before & After), Tested on table, Risk & Scope, and Linked Issues — but the essential review info is present. Not blocking on this. Problem: real and observable — starting a second Direction: aligned. Auto-port-retry is standard developer ergonomics for long-running dev servers (Vite, Next.js, webpack-dev-server all do this). Clearly "should do." Size: not applicable — changes are in Approach: scope feels right — focused, minimal, no drive-by refactors. The 中文说明感谢贡献，@wenshao！ 模板：有 Motivation、Changes 和 Reviewer Test Plan（含验证步骤）。缺少 Evidence（Before & After）、Tested on 表格、Risk & Scope 和 Linked Issues，但审查所需的关键信息都在，不因此阻塞。 问题：真实可观测——在同一端口启动第二个 方向：对齐。自动端口重试是长驻开发服务器的标准人体工学改进（Vite、Next.js、webpack-dev-server 都有此功能），明确属于"应该做"。 规模：不适用——改动在 方案：范围合理——聚焦、最小化、无顺手重构。serve 路径的 — Qwen Code · qwen3.7-max |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Code ReviewThe implementation is clean and correct. The The permanent runtime error handler wired via The existing test mock ("removes the channel pidfile reservation when listener startup fails") was updated to create a fresh server per One pre-existing observation: the TLS catch block inside Reuse check: no existing port-finding utility in the codebase. The two implementations (inline retry in serve, pre-probe in daemon-dev) are appropriate for their different contexts. Real-Scenario TestingBlocked port 4170 with a Python socket ( Post-retry health check on 4171: Note: the initial "listening on 4170" message fires before the EADDRINUSE error is detected — this is Node.js's SO_REUSEADDR behavior allowing the initial bind. The retry correctly fires afterward and the final state is correct (serve on 4171, health check passes). Cosmetic only. Unit tests: all 143 pass, including 4 new retry tests:
中文说明代码审查实现干净且正确。 一个已有问题： 真实场景测试用 Python socket（无 SO_REUSEADDR）阻塞端口 4170，然后启动 单元测试：143 个全部通过，包括 4 个新的重试测试。 — Qwen Code · qwen3.7-max |
Collaborator
|
Straightforward, well-executed fix for a real developer pain point. The No correctness issues, no security concerns, no scope creep. The kind of PR you want to see — small, focused, solves the stated problem, ships. The previous review feedback (IPv6 bracket handling, parseInt validation, exhaustion/port-0 tests, stderr message assertion) has all been addressed in the latest commits. Approving. ✅ 中文说明直接、执行良好的修复，解决了一个真实的开发者痛点。 无正确性问题，无安全隐患，无范围蔓延。正是你想看到的 PR——小、聚焦、解决所述问题、可发布。之前的 review 反馈（IPv6 括号处理、parseInt 校验、耗尽/端口0 测试、stderr 消息断言）都已在最新提交中解决。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
commented
Jul 8, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR; CI still running.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] Stale comment: the onListening callback at ~line 3471 references server.once('error', reject) — this pattern was replaced by tryListen's server.once('error', ...) handler. The quoted code fragment no longer exists in the file. Consider updating the comment to reference the current mechanism.
— qwen3.7-max via Qwen Code /review
The existing test reused a single fakeServer across all retry attempts,
accumulating 10+ once('listening') listeners and triggering a
MaxListenersExceededWarning. Create a new server per call to match
production behavior where each tryListen creates a fresh server.
- daemon-dev.js: strip IPv6 brackets before probe (ENOTFOUND fix), add port range/NaN validation - run-qwen-serve.ts: remove duplicate runtime error listener (onListening already installs one via removeAllListeners + on) - tests: add exhaustion (all 10 ports), port 0 EADDRINUSE no-retry, and stderr retry message assertion (140 tests pass)
Collaborator
Author
|
Review feedback addressed in d4e6d6c:
140 tests pass, lint clean, typecheck clean. |
wenshao
commented
Jul 8, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR; CI still running.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/cli/src/serve/run-qwen-serve.test.ts |
TLS + retry path has zero test coverage. All 4 new retry tests exercise only the non-TLS app.listen path — the httpsServer.listen() branch inside tryListen is never tested with EADDRINUSE. This gap allowed the Critical httpsServer-reuse bug to ship. |
Add one test that passes tlsCert/tlsKey options (or mocks https.createServer) and verifies EADDRINUSE retry works through the HTTPS path. |
packages/cli/src/serve/run-qwen-serve.test.ts:4065 |
Existing pidfile test ("removes the channel pidfile reservation when listener startup fails") uses EADDRINUSE. With the new retry logic, it now silently exercises all 10 retry attempts before rejecting, muddling its original intent. |
Switch to a non-retryable error code (e.g., EACCES) to preserve the test's narrow scope of immediate listen failure cleanup. |
scripts/daemon-dev.js:259-261 |
await findAvailablePort(...) has no try/catch. Rejection propagates as an unhandled promise rejection with a raw stack trace, inconsistent with the clean console.error(...); process.exit(1) pattern used everywhere else in the file. |
Wrap in try/catch: try { port = String(await findAvailablePort(...)); } catch (err) { console.error(\daemon-dev: ${err.message}`); process.exit(1); }` |
packages/cli/src/serve/run-qwen-serve.test.ts |
Port 65535 boundary condition (nextPort <= 65535) is untested. No test starts near 65535 to verify the guard stops retries at the port ceiling independently of MAX_PORT_ATTEMPTS. |
Add a test with port: 65534 that always gets EADDRINUSE, asserting portsAttempted equals [65534, 65535]. |
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 8, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
Author
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/cli/src/serve/run-qwen-serve.ts:3559 |
TLS + retry path has no test coverage. The httpsServer.removeAllListeners('listening') branch in tryListen is never exercised — all 4 retry tests mock only the HTTP path. |
Add a test mirroring "retries the next port on EADDRINUSE and succeeds" with tlsCert/tlsKey set. |
packages/cli/src/serve/run-qwen-serve.ts:3565 |
Synchronous .listen() throw (the catch block in tryListen) is untested. All tests emit errors asynchronously. |
Add a test where app.listen throws synchronously, asserting the promise rejects and pidfile cleanup runs. |
packages/cli/src/serve/run-qwen-serve.ts:3579 |
Port retry messages go only to stderr via writeStderrLine, not to daemonLog. When the daemon runs detached, the structured log won't show retry attempts. |
Add daemonLog.warn(...) calls alongside each writeStderrLine in the retry path. |
scripts/daemon-dev.js:261 |
Port coordination gap — launcher constructs health-check URL from the original port, but child's tryListen can bind to a different port on EADDRINUSE, causing a 30s timeout. |
Have the launcher read the actual bound port from the pidfile/service-info file instead of assuming the probed port. |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
ytahdn
reviewed
Jul 8, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no new blockers. The tryListen retry pattern and test coverage are solid. Two Critical items from prior reviews (port 65535 boundary overflow, daemon-dev health check port mismatch) remain open. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
scripts/daemon-dev.js:259,283 |
findAvailablePort runs unconditionally even when user specifies --port. The QWEN_DAEMON_URL always uses the probed port, but the daemon uses the user-specified port. When --port is occupied, health check targets the wrong address. |
Move findAvailablePort inside if (!hasOption('--port')) guard, or have daemon-dev parse the actual bound port from daemon stdout. |
scripts/daemon-dev.js:19-28 |
serveOptionNames whitelist is missing --compacted-replay-max-bytes (added by merged #6482). Developers using daemon-dev.js --compacted-replay-max-bytes N get rejected by validateLauncherArgs(). |
Add '--compacted-replay-max-bytes' to the serveOptionNames Set. |
packages/cli/src/serve/run-qwen-serve.ts:3581-3583 |
When all 10 port retries are exhausted, the else branch rejects with the raw EADDRINUSE error. The last stderr message says "trying 4180..." with no summary that all ports failed. | Add a summary stderr line before reject: qwen serve: all ports ${opts.port}–${opts.port + MAX_PORT_ATTEMPTS - 1} are in use. |
packages/cli/src/serve/run-qwen-serve.ts:3539-3557 |
https.createServer(tlsOptions, app) is inside tryListen and recreates TLS context on every retry. TLS setup is port-independent. |
Move https.createServer before tryListen and reuse the same server instance across retries. |
— qwen3.7-max via Qwen Code /review
- daemon-dev.js: cap probe at port 65535, skip probe when user specifies --port, add --compacted-replay-max-bytes to whitelist - run-qwen-serve.ts: move https.createServer before tryListen (avoid recreating TLS context per retry), cap retry at 65535, log summary "all ports X–Y are in use" on exhaustion - tests: verify exhaustion summary stderr message
Collaborator
Author
|
R2 feedback addressed in f7bd73d:
140 tests pass, lint clean, typecheck clean. All threads resolved. |
qwen-code-ci-bot
requested changes
Jul 8, 2026
wenshao
commented
Jul 8, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jul 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
ytahdn
approved these changes
Jul 8, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Incremental review: the latest commit (5a89cb9) correctly fixes the httpsServer listening listener accumulation issue identified in the previous review round. httpsServer.removeAllListeners('listening') before each retry ensures only the current onListening callback fires — no duplicate channel worker init, pidfile writes, or log output.
The non-TLS path (app.listen) is unaffected since Express creates a fresh http.Server per call. Port boundary guard (nextPort <= 65535), exhaustion summary message, and --port 0 skip-retry logic are all correct.
No new findings. LGTM! ✅
— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
qwen serveandnpm run dev:daemonboth hardcode port 4170. Starting a second instance fails withEADDRINUSE, forcing manual--portmanagement. This PR makes the daemon automatically find the next available port.Changes
packages/cli/src/serve/run-qwen-serve.ts— Replace the singleapp.listen()call with a recursivetryListen()that retries the next port onEADDRINUSE(up to 10 attempts). Each retry logsqwen serve: port X is in use, trying X+1.... The--port 0(ephemeral) path skips retry since the OS always finds a free port.scripts/daemon-dev.js— Pre-scan for an available port using anet.createServer()probe before spawning the daemon child. The resolved port is passed explicitly via--portso the health-poll and web-shellQWEN_DAEMON_URLtarget the correct URL.Reviewer Test Plan
How to verify:
qwen servein one terminal (binds 4170)qwen servein a second terminal — should seeport 4170 is in use, trying 4171...and bind successfully on 4171npm run dev:daemonin a third terminal — should auto-skip to the next free port and launch web-shell pointing at the correct URLqwen serve --port 0still works (ephemeral, no retry)qwen serve --port 99999fails immediately with a clear error (no retry on non-EADDRINUSE)