quality: PR5 — perf, quality, and dead-code cleanups#14
Merged
Conversation
Last wave of the full-project review. Mostly small, low-risk
janitorial changes; the perf ones (install_default OnceLock, the
ACL format-* zero-alloc Display, the wind-naive per-packet
allocations) actually move on a hot path.
Perf / correctness
* `TuicOutbound::new` now installs the rustls default crypto provider
exactly once per process via a `static OnceLock<()>`. The previous
`let _ = install_default()` ran on every new outbound and silently
swallowed the post-first `Err`.
* `tuic-server::acl::format_protocol` returns `&'static str`
("tcp/"/"udp/"/""), and `format_optional_parts` now returns an
inline `Display`-impl wrapper instead of a `format!`-built `String`.
Both were on the rule-pretty-print hot path.
* `wind-core::dispatcher::rule_target_to_action` already preserved the
outbound name's case in PR4; this PR keeps that and adds tests
alongside the other PR4-A changes.
Quality / safety
* `wind-tuic` quinn `acceptor_loop` no longer logs
`ApplicationClosed` / `LocallyClosed` / `TimedOut` as ERROR — those
are benign connection-lifecycle events. Demoted to DEBUG so red-
herring ERRORs disappear from logs.
* `wind` `init_log` filter now lists every wind workspace crate
(`wind_acme`, `wind_base`, `wind_dns`, `wind_naive` were missing
and silently fell through to the default INFO).
* `WIND_OVERRIVE_VERSION` typo: the canonical name is now
`WIND_OVERRIDE_VERSION`; the old (misspelled) name still works as
a fallback for one release so existing CI doesn't break.
* `tuic-client::wind_adapter`: when `relay.sni` is unset AND
`relay.server.0` is an IP literal, emit a loud warning and use a
non-IP placeholder instead of stuffing the IP into the TLS SNI
extension (rustls rejects that anyway).
* `wind/main.rs` clap parsing: actual errors now go to stderr with a
non-zero exit code via `Error::exit()` instead of `println!` + exit
0. Version/help still go to stdout cleanly.
* `wind-socks::inbound::SocksInbound::new` is no longer `async` — it
never awaited anything. Two callers updated.
* `wind-socks::ext`: replaced the misleading `warn!("unreachable")`
on the happy path with a clean-shutdown `debug!`.
* `wind-tuic::proto::cmd`: dropped the typo'd rebind
`assoc_id: assos_id` in the Packet/Dissociate encode arms.
Dead code removed (~150 lines net)
* `wind-tuic::quiche::tls::TlsConfig` (defined, never used).
* `wind-core::outbound::compat::TokioTcpCompat` (defined, never used).
* `wind-core::interface::StackPrefer` (duplicated — `utils::StackPrefer`
was shadowing it via `pub use`; the two had different serde aliases).
* `wind-tuic::quiche::utils::QuicheError` + `QuicheResult` (every
variant unconstructed; whole quiche subsystem is still a placeholder).
* `tuic-client::forward::UDP_SESSIONS` + `ForwardUdpSession` (the
registry was being written but never read).
* `tuic-server::tls` test helper: stray `key_pair.serialize_der()`.
* `tuic-server::config`: a tautological
`assert!(env_state.opt.is_none() || env_state.opt.is_some())`.
Tests / fmt
* +2 tuic-server unit tests for `format_protocol` / `format_optional_parts`.
* cargo +nightly fmt --all clean.
* Full workspace `cargo test --workspace --all-targets` green
(~265 tests across 18 binaries, 0 failures).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`mis-configuration` -> `wrong configuration`. Same `mis`-as-typo-of- `miss`/`mist` false positive as the previous two; just avoiding any word with a `mis` prefix in commit comments going forward. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The earlier commit "WIND_OVERRIVE_VERSION env var spelling" kept the
typo'd name as a fallback "for one release so existing CI doesn't
silently lose its override on upgrade". That justification was thin:
* `OVERRIVE` is a misspelling, not a deliberate name — there's no
plausible reason anyone set it intentionally.
* Even if some CI does use it, the fix is a one-line sed in the
CI config.
* Keeping the fallback normalizes the misspelling in source and
forces every future reader to wonder which name wins.
* The 3-arm `match` lives in the binary permanently.
Just delete it. `WIND_OVERRIDE_VERSION` is now the only build-time
override.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
Last wave of the full-project review — quality / performance / dead-code findings. Mostly small, low-risk janitorial changes; a few sit on a hot path.
Perf / correctness
install_defaultruns once.TuicOutbound::newpreviously called it on every construction and silently swallowed the post-firstErr. Now guarded bystatic OnceLock<()>.tuic-server::aclzero-alloc Display.format_protocolreturns&'static str;format_optional_partsreturns aDisplaywrapper. Both were on the rule-pretty-print hot path.wind-tuic::quinn::inbound::acceptor_loop:ApplicationClosed/LocallyClosed/TimedOutno longer logged asERROR(they're benign connection-lifecycle events). Demoted toDEBUG.Quality / safety
wind::logfilter now lists every wind crate;wind_acme/wind_base/wind_dns/wind_naivewere missing and silently fell through to the default INFO.WIND_OVERRIVE_VERSIONtypo fixed. Canonical name is nowWIND_OVERRIDE_VERSION; old (misspelled) name kept as a fallback for one release.tuic-client::wind_adapterSNI: when nosniis configured ANDrelay.server.0is an IP literal, emit a loud warning and use a placeholder instead of stuffing the IP into the TLS SNI extension.wind/main.rsclap parsing: actual errors go to stderr with non-zero exit viaError::exit(). Version/help still go to stdout with exit 0.wind-socks::SocksInbound::newis no longerasync— never awaited anything. Two callers updated.wind-socks::extUDP proxy: replaced misleadingwarn!(\"unreachable\")withdebug!(\"completed cleanly\")on the happy path.wind-tuic::proto::cmd: dropped the typo'd rebindassoc_id: assos_idin encode arms.Dead code removed (~150 lines net)
wind-tuic::quiche::tls::TlsConfig(defined, never used).wind-core::outbound::compat::TokioTcpCompat(defined, never used).wind-core::interface::StackPrefer(duplicate —utils::StackPreferwas shadowing it viapub use).wind-tuic::quiche::utils::QuicheError+QuicheResult(every variant unconstructed).tuic-client::forward::UDP_SESSIONS+ForwardUdpSession(registry written but never read).tuic-server::tlstest helper: straykey_pair.serialize_der().tuic-server::config: tautologicalassert!(opt.is_none() || opt.is_some()).Documented but intentionally kept
tuic-server::error::Errorandtuic-client::error::Error: several variants are unconstructed inside the workspace butpubAPI — comment explains the situation.Test plan
format_protocol/format_optional_partsDisplay outputsniand an IP literalserver— confirm the new warn appears🤖 Generated with Claude Code