refactor(moq-native)!: name the endpoint roles connect and listen - #2750
refactor(moq-native)!: name the endpoint roles connect and listen#2750kixelated wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35ae5a018c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .expect("failed to install default crypto provider"); | ||
|
|
||
| let config = Config::load()?; | ||
| let endpoint = config.client.resolved(); |
There was a problem hiding this comment.
When moq-bench is launched through the retained --client-connect flag or MOQ_CLIENT_CONNECT, resolved() places the URL only in this temporary endpoint; the original config.client.connect remains None, so the unchanged unwrap at line 49 panics. The clock example has the same raw-field read at rs/moq-native/examples/clock.rs:54 and reports that --connect is missing. Use the resolved config for all subsequent URL reads so the advertised compatibility path continues to work.
AGENTS.md reference: AGENTS.md:L91-L98
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. parse_and_merge now folds the legacy spellings once via resolved(), so every later field read (the startup log line and the dial in connection.rs) sees the canonical values; the clock example resolves before reading connect too. A --client-connect-launched run no longer panics.
(Written by Fable 5)
| pub use client::Client; | ||
| pub use config::Config; |
There was a problem hiding this comment.
Removing the ClientConfig and ServerConfig exports leaves published examples uncompilable. For example, doc/lib/rs/index.md:278 and doc/lib/rs/env/native.md:21-34 still construct ClientConfig, while rs/moq-rtmp/README.md:87-95 still constructs tls::Server. Update every remaining reference to use Config::client, Config::server, or tls::Config::server_config as appropriate.
AGENTS.md reference: AGENTS.md:L91-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. doc/lib/rs/index.md and doc/lib/rs/env/native.md now use moq_native::Config::default().client() (docs.rs links repointed at Config), and rs/moq-rtmp/README.md builds its RTMPS config from moq_native::tls::Config.
(Written by Fable 5)
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| #[arg(id = "connect", long = "connect", env = "MOQ_CONNECT")] | ||
| pub connect: Option<Url>, |
There was a problem hiding this comment.
The canonical flag rename is incomplete across the repository: rs/moq-cli/README.md:22-52 still teaches --client-connect and --server-bind, and rs/moq-bench/README.md:51-78 still teaches --client-connect and other role-prefixed flags. These aliases are intentionally hidden and emit deprecation warnings, so the user-facing guides should use --connect, --bind, and the new unprefixed TLS/QUIC names.
AGENTS.md reference: AGENTS.md:L182-L182
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. rs/moq-cli/README.md and rs/moq-bench/README.md now teach --connect / --bind / --tls-*, and a repo-wide grep also updated doc/bin/cli.md plus stale flag mentions in comment prose (clock example header, smoke-test comments, moq-boy).
(Written by Fable 5)
35ae5a0 to
f16ba49
Compare
|
Took over this PR: ran a Codex adversarial review, rebased onto current [critical] "Outbound TLS roots become unrestricted inbound relay credentials" — partially agree.
[high] "This breaking dev stack must not target main" — artifact of the review setup: Codex diffed against One pre-existing issue surfaced while validating: (Written by Fable 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f16ba4953e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The delay always parses to a value, so `or` would never fall | ||
| // back; keep the primary side wholesale. | ||
| #[cfg(feature = "websocket")] | ||
| websocket: a.websocket, |
There was a problem hiding this comment.
When the relay loads a legacy [client.websocket] table, a is normally the default endpoint config, but this branch always keeps a.websocket because its delay is pre-populated with 200 ms. Consequently, fallback values such as enabled = false or a custom delay are discarded by merge_legacy_tables, despite Config::or promising to fill every unset knob. Keep these fields optional until after merging, then apply their defaults so legacy TOML remains effective. rs/CLAUDE.mdL116-L116
Useful? React with 👍 / 👎.
|
CI status: the (Written by Fable 5) |
f16ba49 to
6d8308c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d8308c61b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cert: concat(self.cert, fallback.cert), | ||
| key: concat(self.key, fallback.key), | ||
| generate: concat(self.generate, fallback.generate), | ||
| root: concat(self.root, fallback.root), |
There was a problem hiding this comment.
When a legacy relay config has [server].tls.root for inbound mTLS, concatenating it into the unified root also makes it the outbound server-verification store. A relay that previously dialed public certificates using system roots will now reject them, while valid combinations such as a server mTLS root plus [client].tls.disable_verify or fingerprint fail startup in Config::verification as mutually exclusive. Preserve the server root as accept-only trust instead of folding it into the dial policy. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
| } | ||
| client.tls.cert.clear(); | ||
| client.tls.key.clear(); | ||
| self.endpoint = std::mem::take(&mut self.endpoint).or(client); |
There was a problem hiding this comment.
For a retained relay TOML config with [client] version = [...], this merge puts the pin in endpoint.version and leaves cluster.version empty. Relay::load therefore applies the old outbound-only pin to both the cluster client and the public server, causing inbound peers that use any other otherwise-supported version to fail negotiation. Move the legacy client version into cluster.version before merging so existing role-specific configs retain their behavior. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
6d8308c to
d138db9
Compare
|
Rebased onto the new Two
(Written by Fable 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef2eaa1cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub client: moq_native::ClientConfig, | ||
| pub client: moq_native::Config, |
There was a problem hiding this comment.
When an existing moq-bench TOML contains [client] bind = "..." or [client.tls] cert/key, deserializing it as the unified Config places those values in the accept-side bind and served-certificate fields. The later resolved() call does not remap TOML fields, while Client reads connect.bind and tls.client_cert/client_key, so the source bind is silently ignored and mTLS authentication fails. Fresh evidence beyond the earlier URL issue is that moq-relay::merge_legacy_tables performs these exact client-only remappings, but this loader does not; apply the same migration before resolving. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
|
|
||
| # Start a Game Boy emulator publisher. | ||
| start rom url='http://localhost:4443': | ||
| cargo run --bin moq-boy -- --client-connect "{{ url }}" --rom "{{ rom }}" --location localhost |
| # nodes' server certs are signed by `ca.pem`, so no custom trust | ||
| # store is required beyond this one file. | ||
| tls.root = ["ca.pem"] | ||
| tls.client_cert = "leaf0.crt" |
There was a problem hiding this comment.
not a fan.
I think the existing server/client structs made sense here.
There was a problem hiding this comment.
Done in the re-cut: the merged [endpoint] table is gone. The relay's tables are [connect] and [listen] now (with client/server serde aliases so existing configs keep parsing), and the types are role modules, moq_native::connect::Config / moq_native::listen::Config, per your suggestion.
(Written by Fable 5)
| # only; a non-loopback bind logs a warning. Requires the `tcp` build feature. | ||
| [server.tcp] | ||
| [endpoint.tcp] | ||
| bind = "127.0.0.1:4444" |
There was a problem hiding this comment.
TCP can't use a single bind for both clients and servers (technically yes, but not if you want to dial multiple connections to the same server).
I think server. made more sense
There was a problem hiding this comment.
Confirmed by the spike, and it's now the documented reason: quiche binds a fresh socket per connection attempt, so a shared or pinned dial port costs it address racing entirely. The dial socket is --connect-bind (its own knob on the dial side), the listener is --listen, and they stay separate.
(Written by Fable 5)
| timeout = "30s" | ||
|
|
||
| # Disable TLS verification (development only!) | ||
| tls.disable_verify = true |
There was a problem hiding this comment.
While we're here, maybe rename it to insecure or something?
There was a problem hiding this comment.
Renamed: it's --connect-tls-insecure / connect.tls.insecure now, with disable_verify kept as a serde alias. The name should say what it does.
(Written by Fable 5)
| // Automatically reconnects if the connection drops. | ||
| async fn run_session(origin: moq_net::origin::Producer) -> anyhow::Result<()> { | ||
| // Optional: Use moq_native to make a QUIC client. | ||
| let client = moq_native::ClientConfig::default().init()?; |
There was a problem hiding this comment.
if we do go back to Client/Server structs, I tihnk a client/server module might be nicer? moq_native::client::Config
There was a problem hiding this comment.
Done in the re-cut: the merged [endpoint] table is gone. The relay's tables are [connect] and [listen] now (with client/server serde aliases so existing configs keep parsing), and the types are role modules, moq_native::connect::Config / moq_native::listen::Config, per your suggestion.
(Written by Fable 5)
| /// How [`crate::Client::connect`] reacts to a peer's GOAWAY (`--goaway-*`). | ||
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub goaway: crate::GoawayConfig, |
There was a problem hiding this comment.
This should go in connect
There was a problem hiding this comment.
Applied: backoff and goaway are in connect now, tcp/unix are on listen, and backend is per-role (connect.backend / listen.backend) rather than in the shared quic section, which turned out to matter: a quiche acceptor with a quinn dialer is a real setup that the merged config had quietly removed.
(Written by Fable 5)
| /// Retry pacing for [`crate::Client::connect`] (`--backoff-*`). | ||
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub backoff: crate::Backoff, |
There was a problem hiding this comment.
This should go in connect
There was a problem hiding this comment.
Applied: backoff and goaway are in connect now, tcp/unix are on listen, and backend is per-role (connect.backend / listen.backend) rather than in the shared quic section, which turned out to matter: a quiche acceptor with a quinn dialer is a real setup that the merged config had quietly removed.
(Written by Fable 5)
| /// Auto-detected from compiled features if not specified. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| #[arg(id = "backend", long = "backend", env = "MOQ_BACKEND")] | ||
| pub backend: Option<QuicBackend>, |
There was a problem hiding this comment.
This should go in quic
There was a problem hiding this comment.
Applied: backoff and goaway are in connect now, tcp/unix are on listen, and backend is per-role (connect.backend / listen.backend) rather than in the shared quic section, which turned out to matter: a quiche acceptor with a quinn dialer is a real setup that the merged config had quietly removed.
(Written by Fable 5)
| #[cfg(all(feature = "uds", unix))] | ||
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub unix: crate::unix::Config, |
There was a problem hiding this comment.
Maybe in server?
There was a problem hiding this comment.
Applied: backoff and goaway are in connect now, tcp/unix are on listen, and backend is per-role (connect.backend / listen.backend) rather than in the shared quic section, which turned out to matter: a quiche acceptor with a quinn dialer is a real setup that the merged config had quietly removed.
(Written by Fable 5)
| #[cfg(feature = "tcp")] | ||
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub tcp: crate::tcp::Config, |
There was a problem hiding this comment.
maybe in server?
ef2eaa1 to
0052ebe
Compare
kixelated
changed the title
|
Re-cut per your review: this keeps the client/server split instead of merging the two configs, so the diff is now a rename rather than a restructure (−5642/+2649 against the previous revision's shape). What changed from the merged design:
Two things worth your attention, both explained in the description:
Also added Verified: (Written by Fable 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0052ebe54b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// `tcp`/`unix` listener is configured to run a stream-only server with no | ||
| /// QUIC. | ||
| #[serde(alias = "listen")] | ||
| #[arg(id = "listen", long = "listen", alias = "server-bind", env = "MOQ_LISTEN")] |
There was a problem hiding this comment.
Existing deployments using MOQ_SERVER_BIND now leave bind unset because the repository no longer registers that environment variable anywhere; the server-bind option alias does not create an environment-variable alias, so the relay falls back to [::]:443 rather than its configured address. The same regression affects the renamed backend/version/TLS and client TLS/WebSocket environment variables, including MOQ_SERVER_TLS_CERT and MOQ_CLIENT_TLS_ROOT; retain hidden env-backed fields and fold them into the canonical values. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
| #[serde(alias = "server")] | ||
| pub listen: moq_native::listen::Config, |
There was a problem hiding this comment.
When an existing relay file contains [server.quic] or [client.quic], aliasing only the outer table deserializes it directly into listen::Config or connect::Config, both of which deny the now-unknown quic field. The new top-level quic field therefore does not preserve these configurations, and the relay refuses to start instead of folding their settings into it; migrate the nested legacy tables before deserialization into the canonical config. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
| if moq.client.url.is_some() | ||
| && let Some(reconnect) = net.client(moq.client.clone())?.consume(origin.clone()) |
There was a problem hiding this comment.
When moq is invoked with the retained --client-connect flag or MOQ_CLIENT_CONNECT, MoqSide::validate succeeds because it inspects resolved().url, but this raw field remains None, so neither the consume connection nor the analogous publish connection at line 271 is ever started. Fresh evidence beyond the earlier bench/clock report is that the final moq-cli flow still gates client construction on the unresolved field; resolve once before these checks. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
| pub fn init(self, quic: crate::quic::Config) -> crate::Result<Server> { | ||
| Server::new(self, quic) | ||
| } |
There was a problem hiding this comment.
For a server initialized from retained --server-quic-* flags or their old environment variables, the values exist only in quic.legacy, but this path passes the unresolved config to Server::new; consequently the listener silently uses defaults, while a client initialized from the same config does call quic.resolved() and may incorrectly receive the server-only legacy value. Resolve the QUIC config here just as connect::Config::init does. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L95
Useful? React with 👍 / 👎.
|
|
||
| ```rust | ||
| let client = moq_native::ClientConfig::default().init()?; | ||
| let client = moq_native::connect::Config::default().init()?; |
There was a problem hiding this comment.
The final connect::Config::init signature requires a quic::Config, so this newly updated example still does not compile with .init()?; the surrounding text also continues linking the removed ClientConfig type. Fresh evidence after the earlier resolution is the current two-argument API together with this one-argument invocation, which is also repeated in doc/lib/rs/index.md. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L91-L96
Useful? React with 👍 / 👎.
0052ebe to
500adbb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 500adbbc5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let client = config | ||
| .init()? | ||
| .init(Default::default())? |
There was a problem hiding this comment.
When --cluster-lan is combined with any --quic-* setting, each discovered peer is initialized with a fresh default config here, so outbound mesh sessions ignore the configured stream limit, idle timeout, MTU discovery, congestion control, qlog, and GSO settings even though the listener and ordinary relay dial receive Net::quic. Store or pass the shared QUIC config into Lan and use it for these peer clients.
Useful? React with 👍 / 👎.
| # Listen address for HTTP (TCP) | ||
| # Defaults to disabled if not specified | ||
| listen = "0.0.0.0:4443" | ||
| bind = "0.0.0.0:4443" |
There was a problem hiding this comment.
listen in the web HTTP TOML example
A user copying this [web.http] example gets an unknown field bind startup error because HttpConfig still defines listen at rs/moq-relay/src/web.rs:61 and denies unknown fields. This section is unrelated to the endpoint role rename, so the example must remain listen = "0.0.0.0:4443".
AGENTS.md reference: AGENTS.md:L179-L179
Useful? React with 👍 / 👎.
The config surface spelled every transport knob twice, `--client-*` and `--server-*`, backed by duplicated quic::Client/quic::Server and tls::Client/tls::Server type definitions. Keep the two roles apart, since most knobs really do belong to one of them, and fix the naming instead. moq_native::connect::Config (--connect-*) is the dial side and moq_native::listen::Config (--listen-*) the accept side, each owning short unprefixed names. The tuning that means the same thing whichever way a connection was opened moves to one shared quic::Config (--quic-*), handed to each role at init; clap cannot flatten one struct twice, so per-role tuning would mean duplicate definitions that drift. backend and version stay per-role: a quiche acceptor with a quinn dialer is a real setup, and a per-role version lets a relay pin its mesh wire version without narrowing what it accepts. Splitting tls is what separates the two trust stores again: connect.tls.root verifies servers you dial, listen.tls.root authenticates mTLS clients, and the dialing identity drops its client_ prefix since the module now supplies the role. The accept-only QUIC knobs (preferred address, QUIC-LB) move onto listen. Reconnecting is the default, so the flag names the exception: connect.once (--connect-once) replaces the released `reconnect` pair of flag and TOML key, which keep working inverted. Every released flag and env var still configures the same thing, which tests/released_cli.rs freezes: the published moq-relay surface, checked against the parser this build has. Writing it found the hole. A clap alias carries the flag name and not the env var, so 20 released variables parsed as before while being silently ignored in the environment, including the MOQ_SERVER_BIND and MOQ_SERVER_TLS_* that this repo's own nix module and fly.toml set. Each released spelling now lives on a hidden arg carrying its original env var, folded by resolved() with a deprecation warning. Three more bugs came out of the same audit. listen::Config::init never resolved the quic section, dropping --server-quic-max-streams entirely, so Client::new and Server::new now resolve their own inputs. Relay::load filled in DEFAULT_MAX_STREAMS before the fold ran, and the fold prefers the canonical field, so a relay still using the released spelling was pinned to 10,000 streams. And --websocket-enabled/--websocket-delay carried a clap default_value, which the post-file CLI re-parse materializes over whatever a config file said. Closes #2696 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
500adbb to
9de5aa9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9de5aa90d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (cmd_tx, cmd_rx) = tokio::sync::mpsc::channel::<input::Command>(64); | ||
| let url = config.client.connect.clone().context("--client-connect is required")?; | ||
| let client = config.client.clone().init()?; | ||
| let url = config.client.url.clone().context("--connect is required")?; |
There was a problem hiding this comment.
When moq-boy is launched with the retained --client-connect flag or MOQ_CLIENT_CONNECT, the URL exists only in client.legacy.url; init() would fold it, but this raw read happens first and returns the misleading --connect is required error. The test masks the regression by calling resolved() explicitly, so resolve the config before extracting the URL. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L102-L104
Useful? React with 👍 / 👎.
| pub fn server_config(&self) -> moq_native::listen::Config { | ||
| let mut config = self.server.resolved(); |
There was a problem hiding this comment.
When the retained --server-bind flag or MOQ_SERVER_BIND is used, this helper correctly builds the server from the resolved bind, but spawn_server later consults the unresolved moq.server.bind at lines 110 and 121. With --cluster-lan, that makes cluster::serve treat the explicitly configured listener as private and reject every non-mesh request; without the mesh, it skips the /certificate.sha256 endpoint, so http:// clients cannot bootstrap the generated certificate. Carry the resolved listener through all of these decisions. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L102-L104
Useful? React with 👍 / 👎.
| default_value = "[::]:0", | ||
| env = "MOQ_CONNECT_BIND" | ||
| )] | ||
| pub bind: net::SocketAddr, |
There was a problem hiding this comment.
When an old MOQ_CLIENT_BIND or --client-bind remains present during migration, an explicit canonical --connect-bind [::]:0 cannot override it: resolved() treats equality with default_bind() as proof that the canonical option was absent and selects the legacy address. Keep this TOML-loadable clap field optional through merging and apply [::]:0 afterward, so explicit canonical values remain distinguishable from defaults. (Written by GPT-5.6 Sol) rs/CLAUDE.mdL116-L117
Useful? React with 👍 / 👎.
1 participant
Closes #2696.
A QUIC endpoint dials and accepts, and the config surface spelled every transport knob twice:
--client-*and--server-*, backed by duplicatedquic::Client/quic::Serverandtls::Client/tls::Servertype definitions. This keeps the two roles apart, because most knobs really do belong to one of them, and fixes the naming instead: role modules with short names, and the genuinely shared tuning spelled once.The shape
An endpoint that does both flattens all three and hands the tuning to each role:
connect.init(quic)/listen.init(quic).connect.url--connectconnect.bind--connect-bindconnect.timeout--connect-timeoutconnect.race--connect-racefailover_delayconnect.once--connect-onceconnect.tls--connect-tls-*insecure, and the mTLS identity as plaincert/keyconnect.websocket--connect-websocket-*listen.bind--listenlisten.tls--listen-tls-*cert/key/generate, plusrootfor mTLS CAslisten.tcp,listen.unix--listen-tcp-*,--listen-unix-*listen.preferred_v4/v6,listen.lb_id/lb_nonce--listen-*quicquic.*--quic-*A flag is its dotted field path, and
--connect(forconnect.url) is the one exception, because it's the flag everyone types. That rule is what pulled in the last few stragglers: the stream listeners still said--server-tcp-bind/--server-unix-*, the served certificate was bare--tls-certnext to a prefixed--listen-tls-root, and the WebSocket fallback was--websocket-*while its env var had already moved toMOQ_CONNECT_WEBSOCKET_*. Every one of those released spellings still parses, and still reads its original env var.backendandversionstay per-role: aquicheacceptor with aquinndialer is a real configuration (and--client-backend/--server-backendalready allowed it), and a per-roleversionis what lets a relay pin its mesh wire version without narrowing what it accepts from external publishers.Why
tlssplits andquicdoesn'ttlsis two different things wearing one name:cert/key/generateare the served identity, whileroot/fingerprint/insecure/host_nameare how you verify a peer you dial. Splitting them means the mTLS identity drops itsclient_prefix (the module supplies the role), and the tworoots stop being one field:connect.tls.rootverifies servers you dial,listen.tls.rootauthenticates mTLS clients.quicis the opposite:max_streamsmeans the same thing whichever way the connection was opened. Sharing it is also forced, not just preferred, since clap can't flatten one struct twice in a parser, so per-role tuning would mean maintaining duplicate definitions that drift.Compatibility
Every released flag and every released environment variable still configures the same thing. That is checked rather than asserted:
rs/moq-relay/tests/released_cli.rsfreezes the publishedmoq-relaysurface (98 flag/env pairs, taken from the parser itself at the last release) and fails if a spelling stops parsing or an env var stops being read.Writing that test is what turned up the hole. The first pass renamed flags with clap
alias, which carries the flag name and not the env var, so 20 released variables parsed as before on the command line while being silently ignored in the environment: everyMOQ_CLIENT_TLS_*,MOQ_CLIENT_WEBSOCKET_*,MOQ_SERVER_TLS_*, and theMOQ_SERVER_BIND/BACKEND/VERSION/PREFERRED_V4/V6/QUIC_LB_*set. This repo's own deployments were in that blast radius:nix/modules/moq-relay.nixsetsMOQ_SERVER_TLS_CERT/KEY/GENERATEandMOQ_SERVER_BIND, andfly.tomlsetsMOQ_SERVER_BIND, so a relay would have come up with no certificate and on the wrong port. Each released spelling now lives on a hidden arg carrying its original env var, folded into the canonical field byresolved()with a one-line deprecation warning, which is what--client-*and--client-quic-*already did. (--client-tls-disable-verifyhad also been aliased to a name that never shipped,--client-tls-insecure; both parse now.)TOML keeps working through serde aliases:
[client]/[server]tables,connectforurl,failover_delayforrace,disable_verifyforinsecure, andreconnectfor the invertedonce.Deliberate breaks, both loud rather than silent:
[server.quic]/[client.quic]move to a top-level[quic]. A move across tables is the one thing serde aliases can't express, so an old config fails at startup withunknown field quicinstead of quietly ignoring the section.server.quic.congestion_control = "delay"andclient.quic.congestion_control = "loss"; now there is one value.backendandversionremain per-role.Bugs found on the way
listen::Config::initnever calledquic.resolved(), so--server-quic-max-streamswas parsed and dropped.Client::newandServer::newnow resolve their own inputs (idempotent), so no call path can skip it.Relay::loadfilled inDEFAULT_MAX_STREAMSand then handed the config on; since the fold prefers the canonical field, a deployment still passing the released spelling was pinned to 10,000 streams. NowConfig::resolvefolds first and defaults after, with a regression test.--websocket-enabled/--websocket-delayclobbered their own TOML. Both carried a clapdefault_value, which the post-file CLI re-parse materializes over whatever the file said, the exact pitfallrs/CLAUDE.mddocuments. They areOptionnow with the defaults resolved in code (resolved_enabled()/resolved_delay()), sodelay = Nonemeans 200ms rather than "no head start"; the callers that wanted no head start saySome(ZERO).--listen-quic-lb-nonceneeds an id, which clap'srequirescan no longer express now that each knob has two spellings. Checked after the fold instead (Error::LbNonceWithoutId), so any mix of the four is judged correctly.Notes
demo_configs_parsereads the realdemo/relay/*.tomlfiles, since a rename that lands in code but not in the configs people copy is invisible until a relay refuses to boot. It caught one such break while being written.--cluster-versionknob an earlier revision of this PR added is gone: per-roleversionsubsumes it.--client-resolution-delay(feat(native): start dialing before the AAAA answer lands #2749) landed onmainafter this branch's base and hasn't reacheddev, so it's the one released spelling not in the frozen list. Whoever mergesmainintodevshould fold it intoconnect::Configalongside--client-failover-delay.--help; nothing published teaches a dead spelling. Three error messages did too (moq-bench,moq-boy, andmoq transcodeall said "--client-connect is required").just fix,just check, andjust test.(Written by Opus 5)