fix(moq-relay,moq-native): stop the cert-reload busy loop, then dedupe FileWatcher#1773
Conversation
The TLS cert reload path opens and reads the watched cert/key files, and notify's inotify backend reports IN_OPEN/IN_ACCESS for those reads. The FileWatcher treated every event as a change, so a reload's own reads fired events that re-triggered it: a self-sustaining busy loop (~400 reloads/sec) that pinned a core and starved TLS handshakes, taking down QUIC and the WSS fallback alike. Anything that opens a file in the watched directory once (e.g. moq-health's hourly cert read) kicks it off. Filter the notify callback to an explicit allowlist of events that can mean new cert bytes: a create, a modify/rename, or a finished write (IN_CLOSE_WRITE). Read-only access events (open/read/close-without-write) and bare removals no longer reload. A watcher error (e.g. inotify queue overflow) still reloads, since it may have dropped a real change. We keep watching the parent directory rather than the files themselves: rotations replace the file by atomic rename or symlink swap, which changes the inode, so a path-level watch would go deaf after the first rotation (and the K8s `..data` swap never names the cert file at all). Both moq-relay (web + cluster) and moq-native (QUIC) carry a copy of FileWatcher; fix both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ating it moq-relay and moq-native each carried a near-identical copy of FileWatcher (the notify-based cert/key watcher), so the recent read-event filtering fix had to land in both places. Consolidate on moq-native's copy (the superset, with empty-parent handling) and have moq-relay use it. - Expose moq-native's watcher as `moq_native::watch::FileWatcher` behind a new `watch` feature (implied by the quinn/noq backends). Re-export `notify`, since `FileWatcher::new` surfaces `notify::Result` in its signature. - moq-relay enables `moq-native/watch` unconditionally (its web and cluster cert-reload paths need the watcher regardless of QUIC backend) and drops its own `notify` dependency and copy of watch.rs. No behavior change. is_reload_trigger and its tests now live only in moq-native. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-native/src/lib.rs`:
- Around line 46-49: The re-export of the notify crate is currently documented
with regular comments (`//`) instead of rustdoc comments (`///`), which means
the explanation for the re-export will not appear in generated documentation.
Convert the two comment lines explaining why notify is re-exported (the lines
before `pub use notify;` in the watch feature block) to use `///` syntax instead
of `//` so that the documentation is included in the generated rustdoc output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa2af580-7f4f-47ba-9773-033f646bbabf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
rs/moq-native/Cargo.tomlrs/moq-native/src/lib.rsrs/moq-native/src/watch.rsrs/moq-relay/Cargo.tomlrs/moq-relay/src/cluster.rsrs/moq-relay/src/lib.rsrs/moq-relay/src/watch.rsrs/moq-relay/src/web.rs
💤 Files with no reviewable changes (2)
- rs/moq-relay/src/watch.rs
- rs/moq-relay/src/lib.rs
The notify re-export is public API, so its rationale belongs in a /// doc comment that renders on docs.rs, not a // implementation comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The quinn/noq feature arrays got wrapped to multi-line, but .taplo.toml sets column_width = 150, so taplo wants them on one line. Collapse them back to match the repo style and pass 'taplo format --check'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes a self-sustaining TLS cert-reload loop that took a production relay fully offline, then removes the duplication that made the fix land in two places.
Two commits:
fix: only reload certs on real change events — the bug.refactor: share moq-native'sFileWatcher— cleanup so the next fix lands once.The bug
FileWatcherwatches the parent directory of the cert/key files (correct: rotations swap the inode via atomic rename or symlink, so a path-level watch goes deaf after the first rotation, and the K8s..dataswap never names the cert file). Butnotify's inotify backend reportsIN_OPEN/IN_ACCESS, and the callback treated every event as a change. The reload path opens and reads those same files, so each reload fired events that re-triggered it:ServeCertstakes theRwLockwrite lock on every reload; inbound handshakes take the read lock. The write-lock churn + CPU burn starved all TLS handshakes, so QUIC/WebTransport and thewss://TCP fallback failed.The fix
Filter the
notifycallback through an explicitmatchallowlist of events that can mean new cert bytes:Create,Modify(data/rename), and a finished write (IN_CLOSE_WRITE→Access(Close(Write))).Remove(nothing to load until the replacement's create).Behavior change to note: a bare
Removeno longer triggers a reload. Real rotations (cert-manager,mv-into-place, K8s..data) are unaffected — they arrive as create/modify/rename.Added a
is_reload_triggerpredicate with unit tests that don't depend on real inotify timing.The dedupe
moq-relay and moq-native each carried a near-identical
FileWatcher, so the fix above had to land twice. Consolidated onto moq-native's copy (the superset, with empty-parent handling):moq_native::watch::FileWatcherbehind a newwatchfeature (implied by thequinn/noqbackends, so default builds are unchanged). Minimal surface:new+changedpublic,is_reload_triggerstays private. Re-exportednotify(sincenewreturnsnotify::Result), consistent with the crate's existingpub use rustls; pub use moq_net;.moq-native/watchunconditionally (web + cluster reload paths need it regardless of QUIC backend), drops its ownnotifydep andwatch.rs, and pointsweb.rs/cluster.rsat the shared type.Public API changes
pub mod watchwithpub struct FileWatcher(new,changed), newwatchfeature, andpub use notify(both feature-gated). A majornotifybump becomes a breaking change for moq-native, documented at the re-export. Not in the dev-gated list and purely additive, so targetingmain; reviewer may want a minor version bump for the new surface.mod watchand thenotifydependency. No public API change.Branch targeting
main: a bug fix plus an additive, non-breaking library change. No wire/protocol or catalog/container changes.Cross-package sync
rs/moq-netwire/API untouched, so nojs/netordoc/conceptupdates needed. Relay config/behavior unchanged (hot-reload still works; it just no longer self-triggers), so nodoc/bin/relay/change.Test plan
All via
nix develop(CI's pinned toolchain):cargo test -p moq-native— watch tests pass (read/remove ignored, write/create/modify trigger)cargo clippy -p moq-native -p moq-relay --all-targets— cleanwatchcargo-shear— clean (confirms thenotifyremoval)taplo format --check— moq-native/moq-relay Cargo.toml canonicalDeploy note
The live incident still needs a
moq-relayrelease + fleet redeploy; immediate per-node mitigation isstop moq-health(removes the trigger) thenrestart moq-relay.(Written by Claude)