Skip to content

Conversation

@einrobin
Copy link
Contributor

Addresses #708

The certificates will get refreshed when the SIGUSR1 signal is sent. This adds the possibility to configure ex. systemd reload for updating certificates.

Tested on Linux manually and in a Kubernetes container using cert-manager to rotate certificates and simply running pkill -USR1 -f moq-relay.

I also tried a lot around watching the files for changes (polling, inotify on linux) but I think it doesn't make much sense, so I removed it again. I found it was just too unpredictable, my personal use-case in Kubernetes would require watching the parent directory and not the certificate file itself, and I guess that wouldn't stay the only exception to this. Also having two different triggers for reloading seems not very intuitive to me.

@einrobin einrobin marked this pull request as ready for review December 14, 2025 22:59
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 14, 2025

Walkthrough

Certificate management was refactored to use shared, mutable TLS state. Server now holds certs: Arc<ServeCerts> and exposes TLS data via tls_info(): Arc<RwLock<TlsInfo>> (containing certs and fingerprints). ServeCerts batch-loads and can generate certificates, updates TlsInfo, and supplies a resolver that reads from that shared state. Relay, web, and CLI components read fingerprints from tls_info. Unix-only signal handling was added to reload certificates at runtime.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.92% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Certificate reloading' directly and clearly describes the main objective of the PR, which is to implement certificate refresh functionality triggered by SIGUSR1 signal.
Description check ✅ Passed The description is fully related to the changeset, explaining the certificate reloading feature, implementation approach, testing methodology, and design decisions around signal-based vs file-watching approaches.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
rs/moq-relay/src/reload.rs (2)

24-29: Consider handling mutex poisoning gracefully.

Using .unwrap() on the mutex lock will panic if the mutex is poisoned (i.e., a thread panicked while holding the lock). For a reload mechanism that should be resilient, consider using .lock().expect("...") with a descriptive message or handling the PoisonError to continue with potentially stale data.

 	pub fn watch_changes<F>(&self, listener: F)
 	where
 		F: Fn() + Send + Sync + 'static,
 	{
-		self.listeners.lock().unwrap().push(Arc::new(listener));
+		self.listeners
+			.lock()
+			.expect("listener registry lock poisoned")
+			.push(Arc::new(listener));
 	}

44-63: Simplify the reload loop logic.

The tokio::select! with a single branch and the reload variable that's always true add unnecessary complexity. The code can be simplified to directly await the signal and invoke listeners.

 		#[cfg(unix)]
 		loop {
-			let reload = tokio::select! {
-				_ = async {
-					sigusr1.recv().await;
-				} => true,
-			};
-
-			if reload {
-				tracing::info!("reloading configuration");
-				let listeners = {
-					let lock = self.listeners.lock().unwrap();
-					lock.clone()
-				};
-
-				for listener in listeners {
-					listener();
-				}
+			sigusr1.recv().await;
+			tracing::info!("reloading configuration");
+
+			let listeners = {
+				let lock = self.listeners.lock().expect("listener registry lock poisoned");
+				lock.clone()
+			};
+
+			for listener in listeners {
+				listener();
 			}
 		}
rs/moq-native/src/server.rs (1)

405-413: Consider using expect() with descriptive messages for lock operations.

The .unwrap() calls on RwLock operations will panic if the lock is poisoned. While lock poisoning is rare, using expect() with descriptive messages would make debugging easier if it ever occurs.

 	pub fn set(&self, certs: Vec<Arc<CertifiedKey>>) {
-		*self.certs.write().unwrap() = certs;
+		*self.certs.write().expect("certs write lock poisoned") = certs;
 	}

 	pub fn add(&self, cert: Arc<CertifiedKey>) {
-		self.certs.write().unwrap().push(cert);
+		self.certs.write().expect("certs write lock poisoned").push(cert);
 	}
rs/moq-relay/src/main.rs (1)

36-42: Certificate reload performs blocking file I/O in the signal handler callback.

The server_reloader.reload(&tls_config) call performs synchronous file I/O (reading certificate and key files) within the signal handler callback. While this runs in a Tokio task context, it blocks the async runtime thread.

Consider spawning a blocking task for the file I/O operations:

 		reloader_arc.clone().watch_changes(move || {
 			let server_reloader = server_reloader.clone();
 			let tls_config = tls_config.clone();
-			if let Err(err) = server_reloader.reload(&tls_config) {
-				tracing::warn!(%err, "failed to reload server certificate");
-			}
+			tokio::spawn(async move {
+				// Use spawn_blocking for file I/O operations
+				let result = tokio::task::spawn_blocking(move || {
+					server_reloader.reload(&tls_config)
+				}).await;
+				
+				match result {
+					Ok(Err(err)) => tracing::warn!(%err, "failed to reload server certificate"),
+					Err(err) => tracing::warn!(%err, "reload task panicked"),
+					Ok(Ok(())) => {}
+				}
+			});
 		});

Alternatively, making ServerReloader::reload async with tokio::fs operations would be cleaner.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a66405f and ed8b5d1.

📒 Files selected for processing (4)
  • rs/moq-native/src/server.rs (10 hunks)
  • rs/moq-relay/src/main.rs (2 hunks)
  • rs/moq-relay/src/reload.rs (1 hunks)
  • rs/moq-relay/src/web.rs (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

In Rust crates, tests should be integrated within source files using inline test modules

Files:

  • rs/moq-relay/src/reload.rs
  • rs/moq-relay/src/main.rs
  • rs/moq-relay/src/web.rs
  • rs/moq-native/src/server.rs
🧬 Code graph analysis (3)
rs/moq-relay/src/reload.rs (1)
rs/moq-relay/src/web.rs (2)
  • new (90-96)
  • new (290-292)
rs/moq-relay/src/main.rs (2)
rs/moq-relay/src/reload.rs (2)
  • new (18-22)
  • default (12-14)
rs/moq-relay/src/web.rs (2)
  • new (90-96)
  • new (290-292)
rs/moq-relay/src/web.rs (1)
rs/moq-relay/src/reload.rs (1)
  • new (18-22)
🔇 Additional comments (7)
rs/moq-relay/src/reload.rs (1)

31-38: Background task silently swallows errors after logging.

The start_background_task method logs a warning if start_watching fails but doesn't provide any mechanism for the caller to know the watcher failed to start. This is acceptable for signal registration failures, but consider documenting this behavior.

rs/moq-native/src/server.rs (2)

82-105: ServerReloader implementation looks correct for atomic certificate replacement.

The reload logic properly:

  1. Validates cert/key count match
  2. Loads all certificates first before modifying state
  3. Uses set() to atomically replace all certificates

This ensures that if any certificate fails to load, the existing certificates remain intact.


374-380: Good addition of certificate/key validation.

Adding keys_match() validation ensures that mismatched certificate and key files are caught early during load, preventing cryptic TLS errors at runtime.

rs/moq-relay/src/web.rs (2)

125-147: HTTPS certificate reload integration looks correct.

The implementation properly:

  1. Clones the cert/key paths for the 'static closure
  2. Uses RustlsConfig::reload_from_pem_file for live TLS config updates
  3. Logs errors without crashing on reload failures

131-141: No changes needed. The implementation is safe by design—tokio::spawn is called within the async start_watching() context, which runs in a tokio task spawned by start_background_task(). Using Handle::current().spawn() would be unnecessary and add complexity without benefit.

rs/moq-relay/src/main.rs (2)

21-30: Reloader initialization and background task setup looks correct.

The ordering is appropriate:

  1. Create ConfigReloader
  2. Initialize server (which creates the initial certificates)
  3. Start background task (begins listening for signals)
  4. Register watchers

This ensures all components are ready before signal handling begins.


32-43: Guard condition correctly prevents reload registration when no certificates are configured.

The check !tls_config.cert.is_empty() ensures that the reload watcher is only registered when TLS certificates are actually configured, avoiding unnecessary overhead for non-TLS configurations.

@einrobin einrobin marked this pull request as draft December 14, 2025 23:38

// Load a certificate and corresponding key from a file
pub fn load(&mut self, chain: &PathBuf, key: &PathBuf) -> anyhow::Result<()> {
// Load a certificate and corresponding key from a file, but don't add it to the certs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? Why is this is &mut self if it doesn't even mutate ServeCerts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was &mut self but I've removed that because on reload it first loads all the certificates/keys and then replaces them. This prevents that certificates get unloaded when there is an issue with the new certificates like a key that doesn't match the certificate.

@kixelated
Copy link
Collaborator

Hit the ready for review button when you're ready. So AI SLOP can also review.

@einrobin einrobin marked this pull request as ready for review December 20, 2025 14:19
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
rs/moq-relay/src/web.rs (1)

160-170: Consider graceful handling of missing fingerprints.

The function panics with .expect("missing certificate") when no fingerprints are available, which could crash the server if certificates fail to load. This is inconsistent with rs/hang-cli/src/server.rs (lines 98-107) which returns NOT_FOUND. Consider returning an error response instead of panicking, or ensure certificates are always loaded at startup.

🔎 Proposed graceful error handling
 async fn serve_fingerprint(State(state): State<Arc<WebState>>) -> String {
 	// Get the first certificate's fingerprint.
 	// TODO serve all of them so we can support multiple signature algorithms.
 	state
 		.fingerprints
 		.read()
 		.expect("fingerprints lock poisoned")
 		.first()
-		.expect("missing certificate")
-		.clone()
+		.cloned()
+		.unwrap_or_else(|| String::from("unavailable"))
 }

Or return a Result:

-async fn serve_fingerprint(State(state): State<Arc<WebState>>) -> String {
+async fn serve_fingerprint(State(state): State<Arc<WebState>>) -> Result<String, StatusCode> {
 	// Get the first certificate's fingerprint.
 	// TODO serve all of them so we can support multiple signature algorithms.
-	state
+	Ok(state
 		.fingerprints
 		.read()
 		.expect("fingerprints lock poisoned")
 		.first()
-		.expect("missing certificate")
-		.clone()
+		.cloned()
+		.ok_or(StatusCode::NOT_FOUND)?)
 }
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed8b5d1 and 6a0fee9.

📒 Files selected for processing (4)
  • rs/hang-cli/src/server.rs (3 hunks)
  • rs/moq-native/src/server.rs (10 hunks)
  • rs/moq-relay/src/main.rs (1 hunks)
  • rs/moq-relay/src/web.rs (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • rs/moq-relay/src/main.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

In Rust crates, tests should be integrated within source files using inline test modules

Files:

  • rs/moq-relay/src/web.rs
  • rs/hang-cli/src/server.rs
  • rs/moq-native/src/server.rs
🧠 Learnings (1)
📚 Learning: 2025-12-20T13:14:01.334Z
Learnt from: Frando
Repo: moq-dev/moq PR: 794
File: rs/moq-native/src/iroh/client.rs:40-42
Timestamp: 2025-12-20T13:14:01.334Z
Learning: In rs/moq-native/src/iroh/server.rs, the Server::close method uses `&mut self` instead of `&self` (unlike Client::close) because the Server struct contains `FuturesUnordered<BoxFuture<...>>` which is Send but not Sync, and using `&self` would require Self: Sync. This is documented in the code and is an acceptable trade-off.

Applied to files:

  • rs/hang-cli/src/server.rs
  • rs/moq-native/src/server.rs
🧬 Code graph analysis (2)
rs/hang-cli/src/server.rs (1)
rs/moq-native/src/server.rs (3)
  • fingerprints (165-167)
  • new (91-162)
  • new (323-329)
rs/moq-native/src/server.rs (2)
rs/moq-native/src/client.rs (2)
  • new (73-139)
  • new (254-256)
rs/moq-native/src/crypto.rs (2)
  • provider (6-18)
  • sha256 (25-42)
🔇 Additional comments (12)
rs/hang-cli/src/server.rs (1)

33-38: LGTM: Fingerprints retrieval and web server integration.

The fingerprints are correctly retrieved from the server and passed to the web function within the tokio::select! block, enabling concurrent access to the shared fingerprint store.

rs/moq-relay/src/web.rs (4)

27-28: LGTM: Conditional Unix imports.

The signal imports are correctly guarded with #[cfg(unix)], ensuring the code compiles on non-Unix platforms.


80-80: LGTM: Thread-safe fingerprint storage.

The change from Vec<String> to Arc<RwLock<Vec<String>>> enables safe concurrent access and updates to the fingerprints, supporting the certificate reload feature.


118-124: LGTM: TLS configuration with reload support.

The HTTPS setup correctly initializes the RustlsConfig from PEM files and triggers reload setup on Unix. The .expect() calls are safe since clap's requires_all attribute ensures cert and key are present when HTTPS is configured. The PathBuf clones are inexpensive.


142-158: LGTM: Signal-based certificate reload.

The setup_reload function correctly implements the signal-based reload pattern suggested in previous reviews. It spawns a task that listens for SIGUSR1 and reloads certificates, with appropriate error logging.

rs/moq-native/src/server.rs (7)

103-128: LGTM: Certificate initialization and reload setup.

The ServeCerts initialization and Unix signal-based reload logic is well-structured. The reload task correctly clones the Arc and config for owned access, and error handling appropriately logs warnings without crashing the server.


165-167: LGTM: Dynamic fingerprint access.

The fingerprints() method now returns a reference to the shared RwLock container that is updated dynamically when certificates reload, addressing the previous concern about stale fingerprints after reload.


316-329: LGTM: ServeCerts structure with interior mutability.

The use of RwLock for both certs and fingerprints enables in-place updates during reload while allowing concurrent read access. The nested Arc<CertifiedKey> allows individual certificates to be shared efficiently.


331-348: LGTM: Atomic certificate loading.

The load_certs method correctly collects all certificates before calling set_certs, ensuring atomic updates. If any certificate fails to load, the method returns early without partially updating the certificate store, maintaining consistency.


350-380: LGTM: Certificate loading with key validation.

The load method properly validates that the private key matches the certificate using keys_match(), which is a critical security check. The error messages include file paths for easier debugging.


382-402: LGTM: Self-signed certificate generation.

The generate method correctly creates self-signed certificates with a two-week validity period as required by WebTransport specifications.


426-458: LGTM: SNI-based certificate selection.

The best_certificate method correctly uses webpki to validate certificates against the requested server name, and the ResolvesServerCert implementation provides appropriate fallback behavior with warning logging when no matching certificate is found.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
rs/moq-relay/src/web.rs (1)

119-121: Consider more graceful error handling for missing cert/key.

While the clap requires_all constraint ensures cert and key are present when listen is set via CLI, programmatic config construction could bypass this validation and trigger panics. Consider returning an error instead:

let cert = self.config.https.cert.ok_or_else(|| anyhow::anyhow!("https.cert is required when https.listen is set"))?;
let key = self.config.https.key.ok_or_else(|| anyhow::anyhow!("https.key is required when https.listen is set"))?;
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a0fee9 and d6bbcb2.

📒 Files selected for processing (4)
  • rs/hang-cli/src/server.rs (3 hunks)
  • rs/moq-native/src/server.rs (10 hunks)
  • rs/moq-relay/src/main.rs (1 hunks)
  • rs/moq-relay/src/web.rs (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • rs/hang-cli/src/server.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

In Rust crates, tests should be integrated within source files using inline test modules

Files:

  • rs/moq-relay/src/web.rs
  • rs/moq-relay/src/main.rs
  • rs/moq-native/src/server.rs
🧠 Learnings (1)
📚 Learning: 2025-12-20T13:14:01.334Z
Learnt from: Frando
Repo: moq-dev/moq PR: 794
File: rs/moq-native/src/iroh/client.rs:40-42
Timestamp: 2025-12-20T13:14:01.334Z
Learning: In rs/moq-native/src/iroh/server.rs, the Server::close method uses `&mut self` instead of `&self` (unlike Client::close) because the Server struct contains `FuturesUnordered<BoxFuture<...>>` which is Send but not Sync, and using `&self` would require Self: Sync. This is documented in the code and is an acceptable trade-off.

Applied to files:

  • rs/moq-native/src/server.rs
🧬 Code graph analysis (2)
rs/moq-relay/src/main.rs (1)
rs/hang-cli/src/server.rs (1)
  • server (15-40)
rs/moq-native/src/server.rs (1)
rs/moq-native/src/crypto.rs (2)
  • provider (6-18)
  • sha256 (25-42)
🔇 Additional comments (19)
rs/moq-relay/src/main.rs (1)

31-31: LGTM: TLS info wiring updated correctly.

The change from a static fingerprints vector to the shared tls_info state enables the web server to access dynamically updated certificate fingerprints after runtime reloads.

rs/moq-relay/src/web.rs (5)

27-28: LGTM: Platform-specific imports correctly guarded.


80-80: LGTM: WebState updated to use shared TLS info.

The Arc<RwLock<TlsInfo>> enables thread-safe access to dynamically updated certificate information after runtime reloads.


97-97: LGTM: Fingerprint endpoint correctly routed.


142-158: LGTM: Signal-based certificate reloading implemented correctly.

The implementation properly:

  • Handles SIGUSR1 to trigger web certificate reloads
  • Logs reload attempts and failures
  • Runs asynchronously without blocking the main server
  • Gracefully handles signal setup failures

The spawned task runs indefinitely, which is acceptable for this use case.


160-171: LGTM: Fingerprint serving correctly accesses shared TLS state.

The handler properly acquires a read lock and returns the first certificate's fingerprint. The expect calls are reasonable given that:

  • Lock poisoning requires a panic while holding the lock (rare)
  • Certificates should always be present after server initialization
rs/moq-native/src/server.rs (13)

2-2: LGTM: Imports correctly updated for new functionality.

Also applies to: 11-13


87-87: LGTM: Server field updated to enable dynamic certificate management.

Storing Arc<ServeCerts> instead of static fingerprints enables runtime certificate reloading while maintaining shared access across the resolver and signal handler.


103-128: LGTM: Certificate initialization and signal-based reloading implemented correctly.

The implementation properly:

  • Loads certificates at startup with early failure if invalid
  • Wraps ServeCerts in Arc for shared ownership
  • Sets up Unix signal handling for runtime reloads via SIGUSR1
  • Logs reload attempts and failures without crashing the server

The ServerTlsConfig clone on line 113 occurs once at startup (not per reload), making the overhead acceptable.


133-133: LGTM: Certificate resolver correctly wired to shared ServeCerts.


165-167: LGTM: TLS info accessor correctly exposes shared state.

The method provides external access to the shared TlsInfo state, enabling consumers to read current certificates and fingerprints after runtime reloads.


315-319: LGTM: TlsInfo struct correctly encapsulates certificate state.

The visibility modifiers are appropriate:

  • certs is pub(crate) for internal resolver usage
  • fingerprints is pub for external access by web endpoints

321-336: LGTM: ServeCerts correctly uses RwLock for shared mutable TLS state.

The Arc<RwLock<TlsInfo>> pattern is appropriate because:

  • Multiple readers need concurrent access (certificate resolver, fingerprint endpoints)
  • Certificate reloads require exclusive write access to update both certs and fingerprints atomically
  • The Arc itself is never replaced; instead, the TlsInfo contents are mutated in place

338-355: LGTM: Certificate loading implements atomic all-or-nothing updates.

The method correctly:

  • Validates that certificate and key counts match
  • Loads all certificates first (failing early if any are invalid)
  • Applies the complete set atomically via set_certs

This ensures partial failures don't leave the server with mismatched certificates, as noted in past discussions.


357-387: LGTM: Certificate loading validates key/cert pairing.

The refactored signature (returning CertifiedKey instead of mutating state) enables the all-or-nothing atomic update pattern. The keys_match() validation on lines 380-384 prevents loading mismatched key/certificate pairs.


389-409: LGTM: Self-signed certificate generation follows WebTransport requirements.

The method correctly:

  • Generates certificates valid for 14 days (WebTransport maximum)
  • Starts validity yesterday to account for clock drift
  • Returns CertifiedKey without mutating state (enabling atomic updates)

411-424: LGTM: Atomic certificate and fingerprint update addresses race condition concerns.

The implementation correctly computes fingerprints before acquiring any locks (lines 413-419), then updates both certs and fingerprints under a single write lock (lines 421-423). This ensures readers never observe updated certificates with stale fingerprints, addressing the race condition flagged in previous reviews.


427-444: LGTM: Certificate selection correctly uses SNI matching.

The method properly acquires a read lock and validates certificates against the requested server name using webpki's subject name verification.


447-463: LGTM: Certificate resolver implements graceful fallback.

The implementation correctly attempts SNI matching first, then falls back to the first certificate with appropriate warning logging when no match is found (e.g., missing or unknown SNI).

@kixelated kixelated enabled auto-merge (squash) December 22, 2025 03:04
@kixelated
Copy link
Collaborator

Thanks @einrobin ! I'll see about setting this up for the terraform setup in cdn

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
rs/moq-relay/src/web.rs (1)

156-167: Consider returning an HTTP error instead of panicking when no certificate is available.

The .expect("missing certificate") on line 165 will panic if the fingerprints vector is empty. While this shouldn't happen in normal operation (since Server::new validates certificates at startup), returning a proper HTTP error would be more defensive.

🔎 Proposed fix
 async fn serve_fingerprint(State(state): State<Arc<WebState>>) -> impl IntoResponse {
 	// Get the first certificate's fingerprint.
 	// TODO serve all of them so we can support multiple signature algorithms.
-	state
+	match state
 		.tls_info
 		.read()
 		.expect("tls_info lock poisoned")
 		.fingerprints
 		.first()
-		.expect("missing certificate")
 		.clone()
+	{
+		Some(fp) => Ok(fp),
+		None => Err(StatusCode::SERVICE_UNAVAILABLE),
+	}
 }
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6bbcb2 and 7280fb2.

📒 Files selected for processing (2)
  • rs/moq-native/src/server.rs
  • rs/moq-relay/src/web.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

In Rust crates, tests should be integrated within source files using inline test modules

Files:

  • rs/moq-relay/src/web.rs
  • rs/moq-native/src/server.rs
🧠 Learnings (1)
📚 Learning: 2025-12-20T13:14:01.334Z
Learnt from: Frando
Repo: moq-dev/moq PR: 794
File: rs/moq-native/src/iroh/client.rs:40-42
Timestamp: 2025-12-20T13:14:01.334Z
Learning: In rs/moq-native/src/iroh/server.rs, the Server::close method uses `&mut self` instead of `&self` (unlike Client::close) because the Server struct contains `FuturesUnordered<BoxFuture<...>>` which is Send but not Sync, and using `&self` would require Self: Sync. This is documented in the code and is an acceptable trade-off.

Applied to files:

  • rs/moq-native/src/server.rs
🧬 Code graph analysis (1)
rs/moq-relay/src/web.rs (1)
rs/moq-native/src/server.rs (1)
  • reload_certs (145-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Check
🔇 Additional comments (11)
rs/moq-native/src/server.rs (8)

101-108: LGTM! Certificate initialization and reload setup is well-structured.

The sequence of creating ServeCerts, loading certificates, wrapping in Arc, and then spawning the reload task ensures that certificates are validated before the server starts accepting connections.


144-158: LGTM! Signal-based certificate reload implementation.

The reload logic correctly:

  • Runs in a spawned task without blocking server operation
  • Logs both reload attempts and failures
  • Continues serving with existing certificates if reload fails

The .expect() on signal registration at line 149 will panic if signal handling can't be set up, but this is an acceptable startup-time failure since the server requires this capability.


160-163: LGTM! TLS info exposure enables dynamic fingerprint access.

Returning Arc<RwLock<TlsInfo>> allows consumers to always read the current certificates and fingerprints, even after a reload. This correctly addresses the previously raised concern about stale fingerprints.

Also applies to: 311-315


323-332: LGTM!

Initialization creates an empty TlsInfo state that will be populated by load_certs.


334-351: LGTM! Atomic batch certificate loading.

The implementation correctly loads and validates all certificates before calling set_certs, ensuring that partial failures don't corrupt the running state. This aligns with the PR discussion about preventing certificate unloading on errors.


353-383: LGTM! Certificate loading with proper validation.

The method correctly validates that the private key matches the certificate before returning, providing clear error messages with file paths for easier debugging.


407-420: LGTM! Atomic certificate and fingerprint updates.

This correctly addresses the previously raised race condition concern. Fingerprints are computed before acquiring the lock, then both certs and fingerprints are updated under a single write lock scope, ensuring readers always see consistent state.


423-459: LGTM! Certificate resolution with SNI matching.

The implementation correctly matches certificates to client SNI and falls back to the first certificate with a warning when no match is found. The read lock scope is appropriate for the iteration.

rs/moq-relay/src/web.rs (3)

75-80: LGTM!

The tls_info field correctly uses the shared TLS state, enabling dynamic access to updated fingerprints after certificate reloads.


116-128: LGTM! HTTPS configuration with reload support.

The cert.clone() and key.clone() calls are necessary because:

  1. The original values are needed for initial config creation
  2. The clones are moved into the spawned reload task

The .expect() calls are safe since clap's requires_all ensures these values are present when listen is set.


140-154: LGTM! Web certificate reload handler.

The implementation correctly uses hyper_serve::tls_rustls::RustlsConfig::reload_from_pem_file for hot-reloading web HTTPS certificates, mirroring the QUIC certificate reload pattern.

@kixelated kixelated merged commit 200693d into moq-dev:main Dec 22, 2025
1 check passed
@moq-bot moq-bot bot mentioned this pull request Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants