Inject initFatal as a package-level var (#33370) - #45343
Merged
Merged
Conversation
Replace the initFatal function with a package-level var so tests can swap it without terminating the test binary via os.Exit. Add a test for getTLSConfig's default case to exercise the new seam.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThis PR makes 🚥 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 |
Contributor
Author
|
Hello @getvictor - Do you mind taking a look, please? Thanks! 🙏 🙇 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45343 +/- ##
==========================================
- Coverage 66.81% 66.70% -0.12%
==========================================
Files 2724 2728 +4
Lines 219027 218328 -699
Branches 10590 10590
==========================================
- Hits 146342 145631 -711
+ Misses 59521 59483 -38
- Partials 13164 13214 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 tasks
getvictor
pushed a commit
that referenced
this pull request
May 21, 2026
#45583) Extracts early config-validation logic out of `runServeCmd` and puts it on the relevant config types in `server/config/`, following the existing pattern used by `ConditionalAccessConfig.Validate(initFatal)` and `AndroidAgentConfig.Validate(initFatal)`. (First commit on this branch did the extraction into a separate file in `cmd/fleet/`; reshaped per review.) `runServeCmd` is now a series of `config.X.Validate(initFatal)` calls: - `config.Logging.Validate(initFatal)` — OTEL logs requires tracing enabled - `config.Osquery.Validate(initFatal)` — `host_identifier` must be one of `provided`, `instance`, `uuid`, `hostname` - `config.Server.NormalizeURLPrefix()` + `config.Server.ValidateURLPrefix(initFatal)` — Normalize mutates, ValidateURLPrefix is pure - `config.Server.Validate(initFatal)` — `private_key` vs `private_key_arn` mutex check (called before Secrets Manager retrieval so a misconfig fails fast without paying for an external lookup) - `config.Server.ValidatePrivateKeyLength(initFatal)` — minimum 32 bytes (called after Secrets Manager retrieval so an SM-provided short key is also caught) The private-key checks are split into two methods rather than folded into one because the XOR check has to fire before the SM call, and SM retrieval populates `PrivateKey` — so a single Validate called twice would false-positive the XOR check post-SM whenever the user originally configured only `private_key_arn`. Open to feedback if a different split is preferred. Tests live in `server/config/config_test.go` next to the existing config Validate tests, structured as one smoke case plus error branches per the existing convention. Behavior is preserved: `runServeCmd` still calls `initFatal` at the same points with the same descriptions. ## Broader plan Issue #33370 calls for moving logic out of `serve.go` ("should only contain critical config and dependency injection logic"). This PR is one slice. Follow-ups, each in their own small PR: - Extract more config validations (Apple APNs/SCEP both-or-neither, etc.) - Use the `initFatal` injection from #45343 to cover runtime failure paths (datastore init, Redis init, MDM init) - Larger extractions (license init, MDM wiring, mailer init) **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] Input data is properly validated (validators added, no SQL/JS/shell paths involved) - 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 * **Bug Fixes & Improvements** * Centralized and strengthened startup configuration validation. * Enforced mutual exclusivity for private key sources and minimum private-key length. * Added URL-prefix normalization (ensure leading slash, trim trailing slash) and validation. * Ensured OTEL logging requires tracing when enabled. * Restricted osquery host identifier to supported values. * **Tests** * Added tests covering validation rules and URL-prefix normalization/validation. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/45583?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/45583?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
2 tasks
MagnusHJensen
pushed a commit
that referenced
this pull request
May 25, 2026
Extracts the Apple APNs/SCEP both-or-neither check out of `runServeCmd` and puts it on `MDMConfig` as `ValidateAppleAPNSAndSCEPPair(initFatal)`. Same pattern as `ConditionalAccessConfig.Validate`, `AndroidAgentConfig.Validate`, and the validators added in #45583. The call site (inside the existing `if len(toInsert) > 0` gate) goes from six lines of inline conditional `initFatal` calls to one method call. Behavior, error messages, and gating are unchanged. Tests live in `server/config/config_test.go`: one smoke case plus two error branches (APNs-only and SCEP-only). Skipped the "neither set" case on purpose — the outer `if config.MDM.IsAppleAPNsSet() || config.MDM.IsAppleSCEPSet()` gate in `runServeCmd` guarantees at least one is set before the validator is ever reached. This is the last pure config validation left in `runServeCmd` per the broader-plan note on #45583. Remaining `initFatal` sites are runtime failure paths (datastore init, Redis init, MDM init wiring) which need the injection from #45343 — those would be the next slice. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] Input validation (validator method plus tests; no SQL/JS/shell paths involved) - 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 * **Bug Fixes** * Improved Apple MDM configuration validation to ensure APNs and SCEP certificates are properly paired during setup. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46166?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46166?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This was referenced May 28, 2026
getvictor
pushed a commit
that referenced
this pull request
May 29, 2026
Extracts the OTEL trace, metric, and log provider setup out of `runServeCmd` and into `initOTELProviders` in a new `cmd/fleet/otel.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166). Side effects (`otel.SetTracerProvider`, `otel.SetMeterProvider`) are preserved inside the extracted function, so runtime behavior is identical. Three unit tests in `cmd/fleet/otel_test.go`: - OTEL disabled (the common production path) returns `(nil, nil, nil)` and never calls `initFatal`. - OTEL enabled without log export returns non-nil trace and meter providers; logger provider stays nil. - Log export enabled returns all three providers non-nil. One honest note on coverage: the four `initFatal` sites inside the function are paranoid wrapping for OTEL SDK constructors that don't dial at construction time, so the error paths are hard to drive in tests without mocking the SDK. The tests above exercise the success paths and the disabled gate, which is the bulk of the realistic flow. This continues the path toward `serve.go` >60% coverage per the discussion on #33370 — `serve.go` is now ~100 lines shorter and the OTEL phase is testable as a unit. Remaining slices per the broader plan: MDM Apple init, datastore init, Redis init. **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 * **Refactor** * Centralized OpenTelemetry provider initialization into a single setup path, simplifying startup and shutdown behavior and making observability configuration clearer. * **Tests** * Added unit tests covering disabled/enabled telemetry paths and optional log export, plus cleanup logic to ensure providers are shut down correctly. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46421?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46421?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 3, 2026
Extracts the Apple MDM initialization out of `runServeCmd` into testable functions in a new `cmd/fleet/mdm_apple.go`. Continues the chain of extractions on this issue (#44929, #45343, #45583, #46166, #46421) toward the `serve.go` >60% coverage target discussed on #33370. Five functions come out of the inline block: - `initAppleMDMStorages` — constructs the MDM, DEP, and SCEP storages. - `initAppleMDMPushService` — picks the no-op pusher under `FLEET_DEV_MDM_APPLE_DISABLE_PUSH=1`, otherwise the real APNs pusher. - `checkMDMAssetsExist` — promotes the inline `checkMDMAssets` closure to a package function. It was already used at several call sites; they now all share this one. - `reconcileAppleMDMAPNsAndSCEPAssets` / `reconcileAppleMDMABMAssets` — the APNs/SCEP and ABM asset reconciliation blocks. Behavior is preserved — `runServeCmd` calls these in the same order with the same arguments, and the full `cmd/fleet` suite passes unchanged against MySQL + Redis. Each function returns early after `initFatal` so it's also safe when the caller's `initFatal` doesn't terminate (the case in tests). On test scope: the new unit tests cover the dev-mode push gate, all four branches of `checkMDMAssetsExist`, and the no-op and missing-private-key paths of both reconcilers. The storage construction and the actual asset-insert paths need a real datastore, so those stay covered by the existing integration tests rather than new unit tests — I didn't want to stand up a full datastore mock for paths that are already exercised end-to-end. **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** * Added Apple MDM initialization and configuration management for APNs, SCEP, and Apple Business Manager with automatic reconciliation of missing assets and a dev-mode option to disable push. * **Tests** * Added unit tests covering push-service behavior, asset-existence checks, reconciliation logic, and fail-fast handling when required key material is missing. * **Refactor** * Simplified Apple MDM initialization flow by extracting initialization, push-service, and reconciliation logic into helpers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 4, 2026
Extracts the MySQL datastore initialization out of `runServeCmd` and into a new `cmd/fleet/datastore.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517). Continues the path toward `serve.go` >60% coverage per the discussion on #33370. Three functions come out of the inline block: - `initDatastore` — builds the shared DB connections, the datastore, and the carve store (S3-backed when configured, otherwise the datastore itself). - `buildMySQLOpts` — assembles the DB options: base logger and config, plus the optional read replica, dev SQL interceptor, and tracing. - `evalMigrationStatus` — prints any operator guidance for the migration status and returns whether `runServeCmd` should exit. The `os.Exit` stays in `runServeCmd`, so the boot/refuse-to-boot decision becomes unit-testable without the function terminating the test binary. Behavior is preserved — `runServeCmd` calls these in the same order with the same arguments, the migration-exit conditions are unchanged, and the full `cmd/fleet` suite passes against MySQL + Redis. `initDatastore` returns early after `initFatal` so it's safe when the caller's `initFatal` doesn't terminate (the case in tests). On test scope: `TestEvalMigrationStatus` covers every migration status code across the dev-mode and allow-missing-migrations combinations — that's the real decision logic. I deliberately didn't add unit tests for `initDatastore`/`buildMySQLOpts`: their only failure paths are paranoid `initFatal` wrapping around constructors that don't dial at construction time, and the option builder returns opaque option closures. Those success paths are already exercised by booting the server, so a full datastore mock wasn't worth it for coverage's sake. Remaining slice per the broader plan: Redis init. **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 * **Refactor** * Reorganized database startup initialization and migration status evaluation for improved maintainability. * **Tests** * Added comprehensive test coverage for database migration status handling across various scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
MagnusHJensen
pushed a commit
that referenced
this pull request
Jun 5, 2026
Extracts the Redis pool and the cached_mysql / mysqlredis datastore wrappers out of `runServeCmd` and into a new `cmd/fleet/redis.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742). Continues the path toward `serve.go` >60% coverage per the discussion on #33370. Three functions come out of the inline block: - `initRedis` — builds the Redis pool, wraps the datastore with `cached_mysql.New`, and applies `mysqlredis.New` with the license-enforced host limit and host-cache options. Returns the pool, the fully wrapped `fleet.Datastore`, and the outermost `*mysqlredis.Datastore` (a few callers need the concrete type). - `buildRedisPoolConfig` — translates `config.RedisConfig` into the `redis.PoolConfig`, including the `redis://` scheme strip. - `validateRedisConfig` — encodes the host-cache invariant: `HostCacheEnabled` requires `HostCacheTTL > 0`. Returns an error so the caller (or in this case `initRedis` via `initFatal`) can refuse boot without that decision being buried inside a pure builder. Behavior is preserved — `runServeCmd` calls these in the same order with the same arguments, the host-cache validation still aborts startup when violated, and the full `cmd/fleet` suite passes against MySQL + Redis. `initRedis` returns early after `initFatal` so it's safe when the caller's `initFatal` doesn't terminate (the case in tests). Following the precedent established on #46742, the caller also has a loud `initFatal` + `return` guard against a nil pool (covers the same nilaway flow we hit on the datastore slice). On test scope: `TestValidateRedisConfig` covers all four combinations of `HostCacheEnabled` and `HostCacheTTL` — that's the real boot/refuse-to-boot decision. `TestBuildRedisPoolConfigStripsScheme` pins the `redis://` scheme-strip contract for Render-style URIs. I didn't add a `buildRedisPoolConfig` field-mapping matrix or an `initRedis` happy-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 `runServeCmd` directly via the injected `initFatal`. **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 * **Refactor** * Consolidated Redis initialization and datastore wrapping into a dedicated helper; startup now validates the Redis pool and handles initialization failures explicitly. * **Tests** * Added unit tests for Redis address handling and host-cache TTL validation to ensure config behavior is enforced. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Second PR in the staged plan from #33370. Per @getvictor's confirmation, takes the package-level var approach so tests can swap
initFatalwithout terminating the test binary.The new
TestGetTLSConfigInvalidProfilecoversgetTLSConfig's default case (previously unreachable in tests because it callsinitFatal)Related issue: Part of #33370.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Testing
Database migrations
N/A — no database migrations in this PR.
New Fleet configuration settings
N/A — no new configuration settings.
fleetd/orbit/Fleet Desktop
N/A — no agent code changes.
Summary by CodeRabbit