fix: don't log graceful watcher shutdown as error#2168
Conversation
endigma
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughIntroduces an env-var expansion for CDN AUTH_JWT_SECRET in docker-compose, refines router watcher error handling and logging, renames a local watcher variable, and enables file-based execution config with watch enabled in router debug config. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docker-compose.yml (1)
189-197: Consider parameterizing AUTH_ADMISSION_JWT_SECRET as well and documenting .env usageFor consistency and to make rotating local secrets easier, expose AUTH_ADMISSION_JWT_SECRET via env expansion too. Example:
NODE_ENV: development - AUTH_JWT_SECRET: ${CDN_AUTH_JWT_SECRET:-fkczyomvdprgvtmvkuhvprxuggkbgwld} - AUTH_ADMISSION_JWT_SECRET: uXDxJLEvrw4aafPfrf3rRotCoBzRfPEW + AUTH_JWT_SECRET: ${CDN_AUTH_JWT_SECRET:-fkczyomvdprgvtmvkuhvprxuggkbgwld} + AUTH_ADMISSION_JWT_SECRET: ${CDN_AUTH_ADMISSION_JWT_SECRET:-uXDxJLEvrw4aafPfrf3rRotCoBzRfPEW}Also consider adding CDN_AUTH_JWT_SECRET (and CDN_AUTH_ADMISSION_JWT_SECRET if you adopt the change) to an .env.example for discoverability.
I can add an .env.example snippet and a short README note if you want.
router/cmd/main.go (1)
206-211: Treat DeadlineExceeded like Canceled for graceful watcher shutdownIf a deadline is ever introduced on rootCtx, you’ll log an error on graceful exit. Consider suppressing context.DeadlineExceeded the same way you handle context.Canceled.
- if err := w(rootCtx); err != nil { - if !errors.Is(err, context.Canceled) { - ll.Error("Error watching router config", zap.Error(err)) - } else { - ll.Debug("Watcher context cancelled, shutting down") - } - } + if err := w(rootCtx); err != nil { + if !(errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) { + ll.Error("Error watching router config", zap.Error(err)) + } else { + ll.Debug("Watcher context cancelled, shutting down") + } + }router/core/router.go (2)
1204-1208: Mirror graceful-exit handling: include DeadlineExceededSame rationale as in cmd: treat DeadlineExceeded as a graceful shutdown to avoid spurious error logs.
- if !errors.Is(err, context.Canceled) { - ll.Error("Error watching execution config", zap.Error(err)) - } else { - ll.Debug("Watcher context cancelled, shutting down") - } + if !(errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) { + ll.Error("Error watching execution config", zap.Error(err)) + } else { + ll.Debug("Watcher context cancelled, shutting down") + }
1204-1208: Optional: dedupe context-exit checks with a tiny helperYou now have identical shutdown-logging branches in two places. A small helper (package-local) would keep them consistent:
// package-local helper somewhere appropriate func isContextExit(err error) bool { return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) }Then:
if err := w(ctx); err != nil { if !isContextExit(err) { ll.Error("Error watching execution config", zap.Error(err)) } else { ll.Debug("Watcher context cancelled, shutting down") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
docker-compose.yml(1 hunks)router/cmd/main.go(2 hunks)router/core/router.go(1 hunks)router/debug.config.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/cmd/main.go (1)
router/pkg/watcher/watcher.go (2)
New(20-101)Options(12-18)
⏰ 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). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
router/debug.config.yaml (1)
8-11: WatchInterval default is non-zero (1s); no action neededThe
ExecutionConfigFilestruct inrouter/pkg/config/config.godeclares a defaultWatchIntervalof 1 second via itsenvDefault:"1s"tag, and the JSON defaults file (router/pkg/config/testdata/config_defaults.json) sets"WatchInterval": 1000000000(nanoseconds) for the same field. Sincewatch_intervaldefaults to 1s,watcher.Newwill receive a positive interval and will not error out at startup—no explicit override is required inrouter/debug.config.yaml.• router/pkg/config/config.go:819–822 –
WatchInterval time.Duration \yaml:"watch_interval,omitempty" envDefault:"1s"`• router/pkg/config/testdata/config_defaults.json:438–441 –"WatchInterval": 1000000000`docker-compose.yml (1)
191-191: Nice: CDN auth JWT can now come from .env with a sane fallbackGood usability improvement for local/dev setups.
router/cmd/main.go (1)
186-195: Watcher construction/readability: LGTMVariable rename and logger scoping are clear. Callback logs and triggers rs.Reload() only once per stable tick via watcher package, which aligns with intended behavior.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Checklist