Simplify the dynamic-port announcement follow-up to #204 - #206
Merged
Conversation
The reader thread already ends when the engine dies (stdout closes), so a plain thread.join(timeout) covers everything the queue, the manual deadline loop and the process.poll() check were doing. Fewer error paths, same guarantees. Also reject a non-numeric VOLCA_PORT= value with the same clear RuntimeError instead of leaking a bare ValueError, and always pass text=True to Popen since it has no effect when stdout is DEVNULL.
Warp's openFreePort binds 127.0.0.1, unlike the fixed-port path which listens on all interfaces. Say so in the --port help text and the changelog so nobody expects an externally reachable server from --port 0.
str.isdigit accepts Unicode digits such as superscripts that int() rejects, so a line like VOLCA_PORT=² still leaked a bare ValueError — the exact failure this branch set out to close. Every character accepted by isdecimal is parseable by int(), so the RuntimeError guarantee now holds for any input.
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
1 participant
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.
Summary
Follow-up to the post-merge review of #204.
_await_bound_portnow lets the reader thread do the parsing and relies onthread.join(timeout): the engine's death closes stdout and ends the thread, so the queue, the manual deadline loop and theprocess.poll()check were three error paths for the same condition. Same guarantees, a third less code.VOLCA_PORT=value now raises the same clearRuntimeErroras an out-of-range one, instead of leaking a bareValueError. Covered by a new test parameter.--porthelp text and the changelog now say that port 0 binds the loopback interface only — Warp'sopenFreePortbinds 127.0.0.1, unlike the fixed-port path which listens on all interfaces. Without this note the narrower bind is a silent behavior difference.Verification
server.py: 0 errors, 0 warnings