[fix] Resolve server.port=0 for Starlette/UvicornServer path#14521
[fix] Resolve server.port=0 for Starlette/UvicornServer path#14521sfc-gh-lwilby wants to merge 3 commits intodevelopfrom
Conversation
Made-with: Cursor Co-authored-by: lawilby <[email protected]>
sfc-gh-lwilby
added
impact:internal
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
| ) | ||
|
|
||
| # TODO(lukasmasuch): Print the URL with the selected port. | ||
| # Note: server.port=0 (ephemeral port) won't work here because |
There was a problem hiding this comment.
Let's remove this comment, it is out of scope.
There was a problem hiding this comment.
Removed, agreed it's out of scope.
sfc-gh-lwilby
added
the
ai-review
There was a problem hiding this comment.
Pull request overview
Fixes the Starlette/Uvicorn server path so that when server.port=0 is configured (ephemeral port), Streamlit updates server.port to the actual OS-assigned bound port (preventing URLs like http://localhost:0 from being displayed).
Changes:
- Read back the real bound port via
getsockname()[1]after_bind_socket()whenport == 0, then write it to Streamlit config. - Add unit tests covering
server.port=0behavior and ensuringgetsockname()isn’t called for non-zero ports. - Add an inline note in
UvicornRunner.run()documenting whyserver.port=0can’t be supported there without pre-binding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/streamlit/web/server/starlette/starlette_server.py |
Updates Starlette/Uvicorn startup to persist the actual ephemeral port into Streamlit config; adds documentation note for UvicornRunner. |
lib/tests/streamlit/web/server/starlette/starlette_server_test.py |
Adds coverage for the server.port=0 ephemeral port behavior and a non-zero regression check. |
| # Port 0 means the OS assigns an ephemeral port. Read it back | ||
| # so that config and displayed URLs reflect the real port. | ||
| if port == 0: | ||
| port = self._socket.getsockname()[1] |
There was a problem hiding this comment.
self._socket is typed as socket.socket | None, and mypy generally doesn't narrow attribute types after assignment. Accessing self._socket.getsockname() here can fail type-checking. Consider assigning to a local sock variable (with an assert self._socket is not None) before calling getsockname() so the type is non-optional.
| port = self._socket.getsockname()[1] | |
| assert self._socket is not None | |
| sock = self._socket | |
| port = sock.getsockname()[1] |
There was a problem hiding this comment.
Ruff's S101 rule disallows bare assert statements, and mypy doesn't flag this today (CI passes), so skipping the assert. The type narrowing concern is valid in general but not an issue here since self._socket is assigned by _bind_socket() in the try block immediately above.
github-actions
bot
removed
the
ai-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR fixes the server.port=0 (OS-assigned ephemeral port) behavior for the Starlette/UvicornServer code path. Previously, when port 0 was configured, UvicornServer.start() wrote 0 into the Streamlit config instead of the actual bound port, producing broken URLs like http://localhost:0. The fix reads the real port back from the socket via getsockname()[1] after successful binding and before writing to config. A TODO comment is also added to UvicornRunner.run() documenting why port=0 cannot work in that synchronous path. Two focused unit tests are included. This is a companion fix to #14372, which addresses the same bug on the Tornado path.
Reviewer consensus: All three reviewers (claude-4.6-opus-high-thinking, gemini-3.1-pro, gpt-5.3-codex-high) approved unanimously with no blocking issues or disagreements.
Code Quality
The implementation is minimal (3 lines of logic) and correctly placed — after _bind_socket() succeeds but before config.set_option() — ensuring the actual port propagates to config and all downstream URL generation. The getsockname()[1] approach correctly handles both IPv4 (2-tuple) and IPv6 (4-tuple) sockets since the port is at index 1 in both cases. The if port == 0 guard ensures the existing non-zero-port code path is completely unchanged.
One minor observation (noted by claude-4.6-opus-high-thinking): the uvicorn.Config object is created with port=0 before the real port is read back. Since the pre-bound socket is passed directly to uvicorn via startup(sockets=[self._socket]), uvicorn does not use its config's port for binding, so this has no functional impact — only uvicorn's own internal logging might show port 0. This is not a blocking concern.
The test class (TestDynamicPort) follows established patterns from neighboring test classes (TestSslConfiguration, TestStartStarletteServer) with consistent setUp/tearDown autouse fixtures, _create_server, and _run_async helpers.
Test Coverage
Two well-targeted unit tests provide solid coverage:
test_updates_config_port_when_binding_to_zero— Verifies the core fix: config is updated to the actual ephemeral port (54321) when binding to port 0.test_does_not_call_getsockname_for_nonzero_port— Anti-regression check ensuringgetsockname()is only called whenport == 0.
Both tests correctly use mocking to simulate socket behavior without requiring actual network binding. No E2E tests are needed — this is internal server binding logic that does not affect the public st.* API surface.
Backwards Compatibility
No breaking changes. This is a pure bug fix guarded by if port == 0. Users who don't use server.port=0 are completely unaffected. For users who do, the only behavior change is that config.get_option("server.port") now returns the actual bound port instead of 0.
Security & Risk
No security concerns. The change only adds a getsockname() call on an already-bound socket that Streamlit owns. No new network operations, dependencies, route changes, authentication modifications, CORS changes, or security header alterations. Regression risk is low and fully localized to startup port bookkeeping for Starlette mode.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
lib/streamlit/web/server/starlette/starlette_server.py: Change is limited to reading back the OS-assigned port after socket binding — purely internal server setup with no routing, auth, embedding, or header changes.lib/tests/streamlit/web/server/starlette/starlette_server_test.py: Adds unit-level regression coverage for dynamic port handling only.
- Suggested external_test focus areas: N/A
- Confidence: High
- Assumptions and gaps: None. The change is strictly internal port bookkeeping with no impact on request handling, WebSocket transport, embedding, or cross-origin behavior.
Accessibility
N/A — backend-only change with no frontend or UI modifications.
Recommendations
No changes requested. The implementation is clean, correctly scoped, and well-tested.
Verdict
APPROVED: Clean, minimal bug fix for ephemeral port support on the Starlette path with unanimous reviewer agreement, good test coverage, and no risk to existing behavior.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from claude-4.6-opus-high-thinking, gemini-3.1-pro, and gpt-5.3-codex-high.
[question] Is this something that could be improved in this PR as well? |
- Remove out-of-scope comment from UvicornRunner.run() - Update uvicorn_config.port after ephemeral port read-back Co-authored-by: lawilby <[email protected]>
|
@sfc-gh-lwilby Re: updating |
sfc-gh-lwilby
added
the
ai-review
github-actions
bot
removed
the
ai-review
There was a problem hiding this comment.
Summary
This PR fixes server.port=0 (OS-assigned ephemeral port) behavior for the Starlette/UvicornServer code path. When port 0 is configured, the OS assigns a random available port, but the config and displayed URLs previously showed http://localhost:0. The fix reads back the actual bound port via self._socket.getsockname()[1] after successful _bind_socket() and updates both the uvicorn config and the Streamlit config option. This is the companion fix to #14372 (which addresses the same bug on the Tornado path).
Reviewer consensus: All three reviewers (claude-4.6-opus-high-thinking, gemini-3.1-pro, gpt-5.3-codex-high) agree the fix is correct, minimal, and well-placed. Two approved outright; one requested minor non-blocking changes.
Code Quality
The implementation is clean and minimal — a 5-line addition between the successful _bind_socket() call and the uvicorn.Server() creation, which is the correct insertion point. The if port == 0 guard ensures no behavior change for non-zero ports. The getsockname()[1] approach correctly handles both IPv4 (host, port) and IPv6 (host, port, flowinfo, scope_id) tuples.
All three reviewers agreed the code quality is high and follows existing patterns.
PR description discrepancy (noted by 2/3 reviewers): The PR description mentions "Added TODO note on UvicornRunner.run() documenting that port=0 cannot work there without pre-binding," but no such TODO appears in the diff. This is a description-only issue — the author should either add the TODO or update the description. This is non-blocking.
Test Coverage
Two well-targeted unit tests are added in TestDynamicPort:
test_updates_config_port_when_binding_to_zero— Verifies that port 0 is resolved to the actual bound port (54321) andconfig.get_option("server.port")reflects it.test_does_not_call_getsockname_for_nonzero_port— Anti-regression test verifying thatgetsocknameis not called when a specific port is configured.
All three reviewers agreed the test coverage is adequate. No E2E tests are needed since this is internal server startup behavior with no user-facing UI impact.
Backwards Compatibility
No backwards compatibility concerns. The change only affects behavior when server.port=0 is explicitly configured, which was previously broken. Users with a specific non-zero port are unaffected — the if port == 0 guard skips the new code path entirely. All reviewers agreed.
Security & Risk
No security concerns. The change does not introduce new endpoints, modify authentication, alter header handling, or change any externally-facing behavior. Regression risk is very low due to the narrow if port == 0 guard. All reviewers agreed.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
lib/streamlit/web/server/starlette/starlette_server.py: 5-line port resolution fix within server startup, touching only config value assignment — no routes, auth, websocket, embedding, asset serving, or header changes.
- Suggested external_test focus areas: N/A
- Confidence: High (unanimous across all reviewers)
- Assumptions and gaps: None. The change is purely internal server configuration logic.
Accessibility
No frontend changes. Not applicable. All reviewers agreed.
Recommendations
- Update PR description or add TODO: The PR description mentions a TODO note on
UvicornRunner.run()that is missing from the diff. Either add it or update the description to match the actual changes. (Noted by claude-4.6-opus-high-thinking and gemini-3.1-pro; non-blocking)
Verdict
APPROVED: Clean, minimal fix for server.port=0 ephemeral port resolution on the Starlette/UvicornServer path, with well-targeted unit tests and no risk to existing behavior. The majority of reviewers (2/3) approved outright. The one reviewer requesting changes cited only non-blocking concerns (missing TODO note and a test style suggestion), neither of which represent code defects or correctness issues.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from claude-4.6-opus-high-thinking, gemini-3.1-pro, and gpt-5.3-codex-high.
This review also includes 2 inline comment(s) on specific code lines.
| """Set up test fixtures.""" | ||
| Runtime._instance = None | ||
| self.original_port = config.get_option("server.port") | ||
| config.set_option("server.port", 0) |
There was a problem hiding this comment.
suggestion: Consider using @patch_config_options({"server.port": 0}) (from tests.testutil) on individual test methods instead of manually saving/restoring the config in setUp/tearDown. This decorator is used extensively elsewhere in this file and handles cleanup automatically. That said, the current approach mirrors the neighboring TestStartStarletteServer class, so this is non-blocking.
There was a problem hiding this comment.
Investigated — @patch_config_options replaces config.get_option with a mock that returns frozen values, so config.set_option updates from the production code aren't visible through it. The manual setUp/tearDown is the correct pattern here since the test verifies config propagation. Keeping as-is.
|
|
||
| self._run_async(server._start_starlette()) | ||
|
|
||
| assert config.get_option("server.port") == 54321 |
There was a problem hiding this comment.
suggestion: Consider also asserting that uvicorn_config.port was updated to 54321 (via the mock's call_args). The production code sets uvicorn_config.port = port on line 311 of starlette_server.py, but this test only verifies the Streamlit config update — not the uvicorn config synchronization.
There was a problem hiding this comment.
Good catch — added assertion that uvicorn_config.port is also updated to the resolved ephemeral port (9e877d3).
Address review feedback — verify that the uvicorn Config object's port attribute is also set to the resolved ephemeral port, not just the Streamlit config option. Co-authored-by: lawilby <[email protected]>
Describe your changes
When
server.port=0is configured (requesting an OS-assigned ephemeral port),UvicornServer.start()wrote0into config instead of the actual bound port, causing URLs likehttp://localhost:0to be displayed._bind_socket()succeeds with port 0, read back the real port viagetsockname()[1]before writing to configCompanion fix to #14372 (which addresses the same bug on the Tornado path).
GitHub Issue Link (if applicable)
Closes #11308 (Starlette path only; Tornado path addressed in #14372)
Testing Plan
Two new tests in
TestDynamicPort: verifies config is updated with actual port when binding to 0, and verifiesgetsocknameis not called for non-zero ports.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.