fix(start-os): bound service-side TLS establishment - #3869
Merged
Conversation
helix-nine
force-pushed
the
fix/tls-config-resolution-timeout
branch
from
September 2, 2026 22:02
5177680 to
74bab80
Compare
helix-nine
changed the title
helix-nine
marked this pull request as ready for review
September 2, 2026 22:03
dr-bonez
reviewed
Sep 2, 2026
Comment on lines
+1272
to
+1276
| let deadline = tokio::time::Instant::now() + BACKEND_DIAL_TIMEOUT; | ||
| let stream = with_backend_deadline(deadline, self.addr, TcpStream::connect(self.addr)) | ||
| .await? | ||
| .with_ctx(|_| (ErrorKind::Network, self.addr)) | ||
| .log_err() | ||
| .log_err()?; |
Member
There was a problem hiding this comment.
why not just tokio::time::timeout??
Contributor
Author
There was a problem hiding this comment.
No good reason for the wrapper. I removed it and now use Tokio’s timeout API directly at both awaits. I kept timeout_at rather than two independent timeout calls so the ordinary TCP connect and optional TLS handshake share one 15-second budget instead of each receiving 15 seconds. Rebased and re-ran the ALPN tests plus the start-core all-targets check in ab81885.
Apply one 15-second deadline to the backend TCP connect and optional service-side TLS handshake so a service that accepts TCP without speaking TLS cannot park the client indefinitely. Preserve transparent-connect fallback behavior outside the new deadline.
helix-nine
force-pushed
the
fix/tls-config-resolution-timeout
branch
from
September 2, 2026 23:40
74bab80 to
ab81885
Compare
dr-bonez
approved these changes
Sep 2, 2026
Contributor
Author
|
Thanks. The approved head is ab81885; CI is still running. |
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
2 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.
Summary
Why
TLS listener phase 2 resolves the service-side connection before completing the client handshake. A service could accept TCP and then never answer its own TLS handshake, leaving the client parked indefinitely and retaining both sockets in the listener's in-progress set.
The deadline belongs inside
ProxyTarget::preprocess, around backend establishment. Bounding all TLS config resolution would also cancel ACME certificate work and root-CA state updates, so those remain outside it.Verification
cargo test -p start-core upstream_alpn_tests --features=testcargo test -p start-core --features=test --lib— 750 passed, 3 ignoredcargo check -p start-core --features=test --all-targetsmake start-core-format-checkshared-libs/crates/start-core/Cargo.tomlprojects/start-docs/build.shcargo check -p start-os --all-targetsreaches the existing embedded-UI prerequisite and cannot run in this checkout becauseprojects/start-os/web/dist/static/{ui,setup-wizard}has not been built.Closes #3776