Extract Redis initialization out of runServeCmd - #46830
Merged
Merged
Conversation
Move the Redis pool, cached_mysql wrap, and mysqlredis wrap into a new cmd/fleet/redis.go. validateRedisConfig encodes the host-cache invariant (HostCacheEnabled requires HostCacheTTL > 0) so the boot/refuse decision is unit-testable. Refs fleetdm#33370
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExtracts Redis initialization from serve.go into cmd/fleet/redis.go. Adds buildRedisPoolConfig (strips Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/fleet/redis.go`:
- Around line 84-94: The code creates redisPool via
redis.NewPool(buildRedisPoolConfig(cfg.Redis)) before calling
validateRedisConfig(cfg.Redis), which can surface config errors as connectivity
failures and leak the pool when initFatal records failures; fix by calling
validateRedisConfig(cfg.Redis) before creating the pool (move the
validateRedisConfig call above redis.NewPool), so validation runs first and only
then call redis.NewPool(buildRedisPoolConfig(cfg.Redis)); keep existing
initFatal usage for errors and preserve the logger.InfoContext call (which
references redisPool.Mode()) after the pool is successfully created.
- Around line 20-30: In buildRedisPoolConfig, the Server value currently only
trims the "redis://" prefix; update the logic that sets Server (and adjust
UseTLS if appropriate) to also strip "rediss://" prefixes so URIs like
"rediss://host:6379" don't retain the scheme; locate the Server assignment in
buildRedisPoolConfig and ensure it removes both "redis://" and "rediss://", and
if a "rediss://" prefix was detected, set cfg.UseTLS (or the returned
PoolConfig.UseTLS) to true so TLS endpoints are handled correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab382919-6bb4-48bb-a7dd-e4c139057ded
📒 Files selected for processing (3)
cmd/fleet/redis.gocmd/fleet/redis_test.gocmd/fleet/serve.go
Comment on lines
+20
to
+30
| func buildRedisPoolConfig(cfg config.RedisConfig) redis.PoolConfig { | ||
| return redis.PoolConfig{ | ||
| // Strip the Redis URI scheme if it's present. Scheme docs are at: | ||
| // https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml | ||
| // In the future, we could support the full Redis URI if needed | ||
| // (including username, password, database, etc.) | ||
| Server: strings.TrimPrefix(cfg.Address, "redis://"), | ||
| Username: cfg.Username, | ||
| Password: cfg.Password, | ||
| Database: cfg.Database, | ||
| UseTLS: cfg.UseTLS, |
Contributor
There was a problem hiding this comment.
Handle rediss:// endpoints here too.
Line 26 only strips redis://. A TLS endpoint like rediss://host:6379 keeps the scheme in Server, so the helper still breaks on the same managed-Redis URI format this extraction is meant to support.
Suggested fix
func buildRedisPoolConfig(cfg config.RedisConfig) redis.PoolConfig {
+ server := cfg.Address
+ useTLS := cfg.UseTLS
+ switch {
+ case strings.HasPrefix(server, "rediss://"):
+ server = strings.TrimPrefix(server, "rediss://")
+ useTLS = true
+ case strings.HasPrefix(server, "redis://"):
+ server = strings.TrimPrefix(server, "redis://")
+ }
+
return redis.PoolConfig{
- Server: strings.TrimPrefix(cfg.Address, "redis://"),
+ Server: server,
Username: cfg.Username,
Password: cfg.Password,
Database: cfg.Database,
- UseTLS: cfg.UseTLS,
+ UseTLS: useTLS,🤖 Prompt for 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.
In `@cmd/fleet/redis.go` around lines 20 - 30, In buildRedisPoolConfig, the Server
value currently only trims the "redis://" prefix; update the logic that sets
Server (and adjust UseTLS if appropriate) to also strip "rediss://" prefixes so
URIs like "rediss://host:6379" don't retain the scheme; locate the Server
assignment in buildRedisPoolConfig and ensure it removes both "redis://" and
"rediss://", and if a "rediss://" prefix was detected, set cfg.UseTLS (or the
returned PoolConfig.UseTLS) to true so TLS endpoints are handled correctly.
Contributor
Author
There was a problem hiding this comment.
The original code only handled for redis:// but this is a good extension to add. Will raise another PR if required to support striping rediss:// endpoints too.
Move validateRedisConfig ahead of redis.NewPool so a bad host-cache TTL surfaces as the config error itself, not as a Redis connectivity failure, and avoid leaving an open pool dangling if initFatal is swapped (e.g., in tests where it does not terminate). Refs fleetdm#33370
Contributor
Author
|
Hello @MagnusHJensen - This is another installment in the serve.go extraction. Do you mind taking a look, please? |
initLicense returns nil on the failure path when initFatal is swapped out in tests, so license can reach initRedis as nil. Route through initFatal to be loud, then return cleanly. Resolves the nilaway flow the lint check identified. Refs fleetdm#33370
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46830 +/- ##
=======================================
Coverage 66.94% 66.95%
=======================================
Files 2840 2841 +1
Lines 225348 225356 +8
Branches 11537 11537
=======================================
+ Hits 150869 150881 +12
+ Misses 60787 60779 -8
- Partials 13692 13696 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 8, 2026
Extracts the osquery status, result, and audit JSON logger setup out of `runServeCmd` and into a new `cmd/fleet/logging.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830). Continues trimming `runServeCmd` toward the `serve.go` coverage goal on #33370 — this is the largest single slice so far (~100 lines out). Three functions come out of the inline block: - `initOsqueryLogging` — builds the status and result loggers, plus the audit logger when enabled. Mutates the shared `logging.Config` per logger in the same sequence as before, so the constructed loggers are identical. - `buildLoggingConfig` — maps `config.FleetConfig` into the common `logging.Config` shared by all three loggers. - `shouldEnableAuditLog` — the premium-and-enabled gate for the audit logger, pulled out so the decision is its own testable unit. Behavior is preserved — `runServeCmd` calls this in the same place with the same arguments, the per-logger config mutation order is unchanged, and the full `cmd/fleet` suite passes against MySQL + Redis. `initOsqueryLogging` returns early after `initFatal` so it's safe when the caller's `initFatal` doesn't terminate (the case in tests), and it guards a nil license up front since the audit gate dereferences it (matching the nil-guard precedent from #46742/#46830). On test scope: `TestShouldEnableAuditLog` covers all four combinations of license tier and the config flag — audit logging is a premium feature, so the gate is the meaningful decision here. `TestBuildLoggingConfigMapsConfig` is a light check that the config mapping is wired through. I didn't add a full `initOsqueryLogging` happy-path unit test: `logging.NewJSONLogger` constructs real log sinks, so that path is exercised by booting the server rather than by standing up logger backends in a unit test. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Audit logging support is now available for premium license holders. * **Refactor** * Improved logging initialization and configuration management. * **Tests** * Added test coverage for audit logging enablement and configuration mapping. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 9, 2026
) Extracts the geoIP provider and mail service setup out of `runServeCmd` and into new `cmd/fleet/geoip.go` and `cmd/fleet/mail.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893). Both are best-effort startup providers — they log and fall back rather than aborting boot — so they group naturally. Functions: - `initGeoIP` — returns the GeoIP provider. When no database path is configured, or the MaxMind database fails to load, it returns a no-op provider and logs rather than aborting startup. - `initMailService` — configures the mail service; a construction failure is logged and the (possibly nil) service is returned, matching the prior best-effort behavior. - `shouldForceSMTPBackend` — the SMTP-vs-custom-backend rule, pulled out so the decision is its own testable unit: SMTP and a custom email backend are mutually exclusive, and an already-enabled SMTP configuration wins. Behavior is preserved — `runServeCmd` calls these in the same place with the same arguments, and the full `cmd/fleet` suite passes against MySQL + Redis. The mail block's `config.Email.EmailBackend` reset is local to mail construction (nothing downstream reads it), so moving it into `initMailService` is behavior-identical. On test scope: `TestInitGeoIP` pins the not-fatal fallback for both the missing-path and invalid-path cases — GeoIP being best-effort is a real guarantee worth locking. `TestShouldForceSMTPBackend` covers the backend mutual-exclusion decision, including the nil app config / nil SMTP settings edges. I didn't add a full `initMailService` happy-path unit test: `mail.NewService` builds real SMTP/SES backends, so that path is exercised by booting the server. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Refactor** * Improved GeoIP initialization with automatic fallback when database configuration is unavailable * Enhanced mail service initialization with better error handling during startup * Refined SMTP backend precedence logic * **Tests** * Added comprehensive unit tests for GeoIP and mail service initialization scenarios <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 17, 2026
Extracts the cron schedule registration out of `runServeCmd` and into a new `cmd/fleet/cron_registration.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151). This is the largest slice so far — `runServeCmd` drops from ~1300 to ~1000 lines, and `serve.go` from 1776 to 1472. The 33 `StartCronSchedule` registrations move into one `startCronSchedules` entry point backed by a `cronSchedulesDeps` struct (the dependencies the closures previously captured from `runServeCmd`). Registration is grouped by domain: - `registerCleanupAndMaintenanceCrons` — chart data collection, the `cron_stats` cleanup goroutine, software migrations, frequent cleanups, cleanups-then-aggregation, query results cleanup, upcoming activities, usage statistics, batch activities. - `registerVulnerabilityCrons` — the vulnerabilities schedule, or the remote-trigger proxy when processing is disabled on this instance. - `registerWorkerCrons` — automations and worker integrations. - `registerMDMCrons` — Apple MDM worker, DEP profile assigner, service discovery, the Apple/Windows/Android profile managers, the Android device reconciler, the Android policy migrations, and the APNs pusher. - `registerPremiumCrons` — iPhone/iPad refetcher and reviver, maintained apps, VPP app version refresh (and the one-shot VPP country backfill), recovery lock passwords, managed local account rotation, activities streaming, and the calendar schedule. - `registerMiscCrons` — host vitals label membership and the batch activity completion checker. Behavior is preserved — the schedules register in the same order with the same arguments, the same conditionals gate them (premium, audit log, env vars, software store presence), and the `config` is threaded as a pointer so the `&config` and `config.Calendar` mutations inside the calendar closure keep their original semantics. `cmd/fleet/cron.go` (the schedule definitions) is intentionally untouched; only the wiring moved. One unit test added: `TestVulnerabilityProcessingDisabled` covers the vuln enable/disable predicate extracted into `vulnerabilityProcessingDisabled`, including the legacy `current_instance_checks` `"0"` value. The rest of the file is dependency-wiring relocation with no further decision logic to unit-test — those paths construct real schedules, so they stay covered by the existing suite and integration tests. The full `cmd/fleet` suite passes against MySQL + Redis, and a local server boot confirms the same 30 cron schedules start as before (verified against the "started cron schedules" log line). **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified via local server boot — same 30 cron schedules start) - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized background cron schedule startup and standardized job initialization sequencing for maintenance, vulnerability handling, integrations, MDM workflows, and premium tasks. * **New Features / Behavior** * Added config- and license-controlled enablement for vulnerability processing (local vs remote triggering), MDM automation (including APNs delivery and device reconciliation), and premium-only refresh/recovery behaviors. * Made chart data collection and optional activity streaming configurable, with safe fallbacks for scheduling periodicity. * **Tests** * Added coverage for vulnerability-schedule enable/disable decision logic. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
JordanMontgomery
pushed a commit
that referenced
this pull request
Jun 24, 2026
Extracts the `/api/` request timeout/body-size override middleware out of `runServeCmd` and into `apiTimeoutOverrideHandler` in a new `cmd/fleet/http_middleware.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562). `runServeCmd` drops from ~1000 to ~900 lines, and `serve.go` from 1475 to 1373. The middleware is the `~100`-line `rootMux.HandleFunc("/api/", ...)` closure that applies per-route read/write deadline overrides for endpoints that legitimately run long — synchronous script runs, large software-installer and bootstrap-package uploads, the Android enterprise signup SSE stream, and large MDM profile batch operations — and, for package-upload routes, caps the request body and threads the configured max installer size through the request context. Behavior is preserved — the handler is moved verbatim and wired into `rootMux` via a single `apiTimeoutOverrideHandler(apiHandler, config, logger)` call, so the same routes get the same overrides and every request still falls through to `apiHandler.ServeHTTP`. The now-unused `scripts` and `installersize` imports drop out of `serve.go`. On test scope: `TestAPITimeoutOverrideHandler` verifies the real decision in this middleware — that package-upload paths thread the configured max installer size into the request context (and non-upload requests keep the default) — and that the wrapped API handler is always invoked. The deadline overrides themselves go through `http.ResponseController`, which a unit-test `ResponseRecorder` doesn't support (the handler logs and proceeds, as in production), so those are exercised by booting the server rather than asserted in a unit test. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified via local server boot) - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved timeout handling for long-running operations across the API. Script execution, file uploads, Server-Sent Event streams, and batch operations now have optimized request timeouts and body size limits. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
juan-fdz-hawa
pushed a commit
that referenced
this pull request
Jun 30, 2026
) Adds an end-to-end boot test for `runServeCmd`, the main server entry point. This is the coverage milestone for #33370: `serve.go` goes from ~7% to ~64%, and `runServeCmd` itself from 0% to ~62%. The earlier PRs on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562, #47891) extracted testable pieces out of `runServeCmd`, but the function itself stayed at 0% — it blocks on an OS signal and wires the entire server together, so the only way to cover it is to actually boot it. This PR does that. `TestRunServeCmd` (gated behind `MYSQL_TEST` + `REDIS_TEST`) boots the full server against a real migrated test MySQL and Redis, waits for `/healthz`, then cancels the command context to trigger a graceful shutdown. It covers two paths: - **Full boot with Apple MDM enabled** — a 32-byte server private key brings up the Apple MDM protocol services and the host-identity / conditional-access SCEP setup, so the boot exercises the MDM startup path as well as the core wiring, cron schedules, and HTTP server. - **Fail-fast on bad config** — an invalid Redis host-cache configuration (enabled with a non-positive TTL) aborts startup through `initFatal` and returns rather than serving, covering the Redis-init error path and the nil-pool guard. Beyond coverage, this doubles as a regression net for the ongoing `runServeCmd` slicing: a future change that breaks startup now fails this test instead of reaching a release. **One production change**, in `runServeCmd`'s shutdown `select`: it now also watches `cmd.Context().Done()`. This is inert in production — the root command runs via `Execute()` (not `ExecuteContext()`), so `cmd.Context()` is `context.Background()` and never cancels. Only the test runs the command with a cancelable context, which is how it shuts the server down without sending a real signal (a `SIGTERM` would kill the test binary). A couple of notes for reviewers: - The test uses `os.Setenv` (not `t.Setenv`) because the MySQL test helper marks the test parallel; the boot scenarios run as serial subtests so the process-global config env doesn't race. - `runServeCmd` registers metrics with the process-global Prometheus registry, which can only happen once per process, so there is a single full boot here; the error-path scenario fails before that registration. - The test DB is loaded from a schema dump that doesn't mark every data migration as applied, so the boot runs with `FLEET_UPGRADES_ALLOW_MISSING_MIGRATIONS=1`. It adds ~2s to the `cmd/fleet` (`main`) test bundle, which is well off the CI critical path. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified locally: boots to /healthz, graceful shutdown, ~64% serve.go coverage) - Changes file: not applicable — internal test coverage with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved server shutdown handling to stop cleanly when the running command’s context is canceled, not only on OS signals. * Added stronger startup validation to fail fast for invalid Redis host-cache configuration (e.g., non-positive TTL). * **Tests** * Added an end-to-end test that boots the server against real MySQL/Redis, verifies graceful startup/shutdown, and confirms fast-fail behavior for misconfiguration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracts the Redis pool and the cached_mysql / mysqlredis datastore wrappers out of
runServeCmdand into a newcmd/fleet/redis.go. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742). Continues the path towardserve.go>60% coverage per the discussion on #33370.Three functions come out of the inline block:
initRedis— builds the Redis pool, wraps the datastore withcached_mysql.New, and appliesmysqlredis.Newwith the license-enforced host limit and host-cache options. Returns the pool, the fully wrappedfleet.Datastore, and the outermost*mysqlredis.Datastore(a few callers need the concrete type).buildRedisPoolConfig— translatesconfig.RedisConfiginto theredis.PoolConfig, including theredis://scheme strip.validateRedisConfig— encodes the host-cache invariant:HostCacheEnabledrequiresHostCacheTTL > 0. Returns an error so the caller (or in this caseinitRedisviainitFatal) can refuse boot without that decision being buried inside a pure builder.Behavior is preserved —
runServeCmdcalls these in the same order with the same arguments, the host-cache validation still aborts startup when violated, and the fullcmd/fleetsuite passes against MySQL + Redis.initRedisreturns early afterinitFatalso it's safe when the caller'sinitFataldoesn't terminate (the case in tests). Following the precedent established on #46742, the caller also has a loudinitFatal+returnguard against a nil pool (covers the same nilaway flow we hit on the datastore slice).On test scope:
TestValidateRedisConfigcovers all four combinations ofHostCacheEnabledandHostCacheTTL— that's the real boot/refuse-to-boot decision.TestBuildRedisPoolConfigStripsSchemepins theredis://scheme-strip contract for Render-style URIs. I didn't add abuildRedisPoolConfigfield-mapping matrix or aninitRedishappy-path unit test: the former would just re-state the struct literal, and the latter needs a real Redis pool (the smoke boot exercises it end-to-end instead).This completes the four named init-block extractions on this issue. If further coverage gains are needed beyond what these have already moved, the next conversation is whether to test
runServeCmddirectly via the injectedinitFatal.Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit
Refactor
Tests