Repository navigation
fix(tokio): reload TLS only when the watched files change - #5716
Conversation
The certificate watcher watches each file's parent directory and reloaded on any create or modify there. A relay logging into its certificate directory reloaded on every line, and each reload logged another line: about 29k reloads in 3s locally, and ~60k in a smoke run. Filter events to the watched file names, plus a change in where a watched path resolves through its symlinks, so the Kubernetes `..data` swap (which never names the file) still reloads. Also reload on notify's Rescan flag, which is how an inotify queue overflow arrives; it was being ignored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
✨ Finishing Touches
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 |
![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: 1f714415fe
ℹ️ 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 named = event | ||
| .paths | ||
| .iter() | ||
| .filter_map(|p| p.file_name()) | ||
| .any(|name| self.paths.iter().any(|p| p.file_name() == Some(name))); |
There was a problem hiding this comment.
When TLS material spans multiple directories, this compares each event basename against every configured basename without checking the parent. For example, with /a/cert.pem and /b/key.pem watched, writing an unrelated /b/cert.pem passes named and triggers a reload, potentially recreating the reload/log storm this change is intended to prevent. Match the normalized parent and filename together rather than matching filenames globally. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 396dad0. A name match now also requires the event's canonicalized directory to match the watched file's, with the name check first so unrelated churn still skips the syscall. same_name_in_another_watched_directory_does_not_trigger_reload covers /b/cert.pem with /a/cert.pem + /b/key.pem watched.
(Written by Claude Opus 5.5)
| let event = rx | ||
| .recv_timeout(Duration::from_secs(10)) | ||
| .expect("missing event") |
There was a problem hiding this comment.
On a loaded Linux CI host, correct inotify behavior can still fail this test if event delivery or test-thread scheduling exceeds the hard-coded ten-second deadline. This makes the unit test depend on wall-clock timing; block on recv and rely on nextest's suite timeout to diagnose a genuine hang instead. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 396dad0: the step now blocks on recv(), and a lost event hangs until nextest's suite timeout.
(Written by Claude Opus 5.5)
… wall-clock deadline Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 396dad070f
ℹ️ 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".
| && self.located.iter().any(|f| f.file_name() == name) | ||
| && locate(p).is_some_and(|p| self.located.contains(&p)) |
There was a problem hiding this comment.
When a configured TLS path is a symlink and its target is modified in place under the same watched directory, notify reports the target name (for example, actual.pem) rather than the configured link name (cert.pem). This predicate rejects that event, while resolve() still returns the same canonical target and therefore does not set moved; the process consequently keeps serving stale certificates or trusting stale root CAs. Compare events against the currently resolved targets too, and cover this symlink-target write with a regression test. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this was a regression from the old "any event reloads" behavior. Fixed in 9fbd5d6: an event now also counts as named when it lands on a watched path's current canonical target, so writing actual.pem behind cert.pem -> actual.pem reloads. Covered by write_to_symlink_target_triggers_reload. A target in a different, unwatched directory was never seen before this PR either, so that stays out of scope.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging. Summary:
(Written by Claude Opus 5.5) |
Problem
moq_tokio::watchwatches each certificate's parent directory (so atomic renames and the Kubernetes..datasymlink swap are seen) and reloaded on any create or modify in that directory. When the relay's log file sits in the same directory, every reload writes a log line, which triggers the next reload. Reproduced locally by runningmoq-relaywithcert.pem,key.pem, and its stdout redirected torelay.login one directory: about 29,000 reloads in 3 seconds. A moq-dev/smoke nightly run logged about 60k.Fix
watch::callbacknow passes events through aFilterthat fires only when:fs::canonicalize) somewhere different from before. This is how the kubelet's..data_tmp->..datarename gets caught, because no event in that rotation namestls.crt.The resolved targets are refreshed on every event, so one rotation fires once. This is the same approach viper and Istio's file watcher use.
It also reloads on notify's
Rescanflag. An inotify queue overflow arrives asOk(Other + Rescan), notErr, so the old code dropped it even though its comment said an overflow would reload.Server certs/keys (
Files) and reloadable root CAs (Reloading) both go throughcallback, so both get the fix.Tests
unrelated_files_do_not_trigger_reload: synthetic events onrelay.logare rejected; writes and renames ofcert.pem/key.pemand a Rescan are accepted.write_to_symlink_target_triggers_reload: withcert.pem -> actual.pem, a write toactual.pemreloads.same_name_in_another_watched_directory_does_not_trigger_reload: withcert.pemandkey.pemin different directories, acert.pemin the key's directory is rejected.kubernetes_rotation_reloads_and_log_writes_do_not(Linux, real inotify): sets up a kubelet-style mount, writes a log, does the..dataswap, then writes the log again. Each step blocks until its own last event arrives (no wall-clock deadline; nextest's timeout catches hangs), so the result doesn't depend on timing. Checks that log writes never reload and that the rotation reloads exactly once.Both tests fail when the filter goes back to accepting every event. The Kubernetes test also fails when the filter only checks file names.
After the fix, the local relay repro shows 1 reload (from an
mvrotation partway through the run) instead of a storm.just checkpasses.API / wire impact
None.
Filteris private;watch::Fileskeeps its signature and only its docs changed. The relay docs already say "Reloaded on change", which is still accurate, sodoc/bin/relay/is unchanged.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code