From bf352c72086fb2144d8dd8ca53c14e6f523bc900 Mon Sep 17 00:00:00 2001 From: Rajendra kadam Date: Tue, 30 Jun 2026 18:58:20 +0530 Subject: [PATCH] Boot-test runServeCmd end to end (serve.go ~7% to ~64% coverage) (#48261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ## 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. --- cmd/fleet/serve.go | 5 + cmd/fleet/serve_boot_test.go | 181 +++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+) create mode 100644 cmd/fleet/serve_boot_test.go diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index 19045aaee3..bf42675a72 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -1037,6 +1037,11 @@ func runServeCmd(cmd *cobra.Command, configManager configpkg.Manager, debug, dev select { case <-sig: case <-dbFatalCh: + // cmd.Context() is context.Background() in production (the root command + // is run via Execute, not ExecuteContext), so this case never fires + // there. Tests run the command with a cancelable context to trigger a + // graceful shutdown without sending an OS signal. + case <-cmd.Context().Done(): } ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() diff --git a/cmd/fleet/serve_boot_test.go b/cmd/fleet/serve_boot_test.go new file mode 100644 index 0000000000..0c45ddf74b --- /dev/null +++ b/cmd/fleet/serve_boot_test.go @@ -0,0 +1,181 @@ +package main + +import ( + "context" + "net" + "net/http" + "os" + "strconv" + "strings" + "testing" + "time" + + "github.com/fleetdm/fleet/v4/pkg/fleethttp" + "github.com/fleetdm/fleet/v4/server/config" + testing_utils "github.com/fleetdm/fleet/v4/server/platform/mysql/testing_utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func freeLocalAddr(t *testing.T) string { + l, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := l.Addr().String() + require.NoError(t, l.Close()) + return addr +} + +// fatalRecorder swaps the package-level initFatal var to collect calls instead +// of terminating the test binary, restoring the original on cleanup. This is +// safe because the boot test does not run in parallel. +type fatalRecorder struct{ calls []string } + +func installFatalRecorder(t *testing.T) *fatalRecorder { + r := &fatalRecorder{} + orig := initFatal + initFatal = func(err error, msg string) { + r.calls = append(r.calls, msg+": "+err.Error()) + } + t.Cleanup(func() { initFatal = orig }) + return r +} + +func (r *fatalRecorder) contains(substr string) bool { + for _, c := range r.calls { + if strings.Contains(c, substr) { + return true + } + } + return false +} + +// configureBootEnv points the server's config at the given migrated test +// database and the test Redis (on the given Redis logical database), on a free +// local port, with TLS off. It returns the server's address. +func configureBootEnv(t *testing.T, dbName string, redisDB int) string { + serverAddr := freeLocalAddr(t) + redisAddr := os.Getenv("REDIS_TEST_ADDRESS") + if redisAddr == "" { + redisAddr = "localhost:6379" + } + + t.Setenv("FLEET_MYSQL_ADDRESS", testing_utils.TestAddress) + t.Setenv("FLEET_MYSQL_USERNAME", testing_utils.TestUsername) + t.Setenv("FLEET_MYSQL_PASSWORD", testing_utils.TestPassword) + t.Setenv("FLEET_MYSQL_DATABASE", dbName) + t.Setenv("FLEET_REDIS_ADDRESS", redisAddr) + t.Setenv("FLEET_REDIS_DATABASE", strconv.Itoa(redisDB)) + t.Setenv("FLEET_SERVER_ADDRESS", serverAddr) + t.Setenv("FLEET_SERVER_TLS", "false") + // The test schema is loaded from a dump that does not mark every data + // migration as applied, so allow the server to boot past the migration + // status check (the schema is functionally complete for a boot test). + t.Setenv("FLEET_UPGRADES_ALLOW_MISSING_MIGRATIONS", "1") + + return serverAddr +} + +// runServe runs the serve command with a cancelable context and any extra +// command-line flags, returning a channel that receives the command's exit +// error when runServeCmd returns. +func runServe(ctx context.Context, extraArgs ...string) <-chan error { + rootCmd := createRootCmd() + configManager := config.NewManager(rootCmd) + rootCmd.AddCommand(createServeCmd(configManager)) + rootCmd.SetArgs(append([]string{"serve", "--dev_license"}, extraArgs...)) + + done := make(chan error, 1) + go func() { done <- rootCmd.ExecuteContext(ctx) }() + return done +} + +func waitHealthy(t *testing.T, serverAddr string) bool { + client := fleethttp.NewClient(fleethttp.WithTimeout(2 * time.Second)) + return assert.Eventually(t, func() bool { + resp, err := client.Get("http://" + serverAddr + "/healthz") //nolint:gosec + if err != nil { + return false + } + defer resp.Body.Close() + return resp.StatusCode == http.StatusOK + }, 60*time.Second, 250*time.Millisecond) +} + +func waitShutdown(t *testing.T, done <-chan error) { + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(60 * time.Second): + t.Fatal("server did not shut down within 60s of context cancellation") + } +} + +// TestRunServeCmd boots the full server via runServeCmd against a real (migrated) +// test MySQL and Redis and exercises the entire startup path end to end. +// +// The test does not run in parallel: it swaps the package-global initFatal and +// sets process-wide config env, and the scenarios share one database via serial +// subtests. +func TestRunServeCmd(t *testing.T) { + if os.Getenv("MYSQL_TEST") == "" || os.Getenv("REDIS_TEST") == "" { + t.Skip("requires MYSQL_TEST=1 and REDIS_TEST=1") + } + + const dbName = "fleet_serve_boot_test" + // Load the schema directly (rather than via CreateMySQLDS) so this test is + // not marked parallel — it mutates process-global state (config env and the + // initFatal var) that must not race with other package tests. + testing_utils.LoadDefaultSchema(t, dbName, &testing_utils.DatastoreTestOptions{}) + + // Boots the full server and shuts it down gracefully on context + // cancellation. A server private key is set so the boot also brings up the + // Apple MDM protocol services and the host-identity / conditional-access + // SCEP setup, exercising the MDM-enabled startup path. + // + // NOTE: runServeCmd registers metrics collectors with the process-global + // Prometheus registry, which can only happen once per process, so this is + // the single full boot in this package. Error-path scenarios below must fail + // before that registration. + t.Run("boots with Apple MDM enabled and shuts down gracefully", func(t *testing.T) { + rec := installFatalRecorder(t) + serverAddr := configureBootEnv(t, dbName, 12) + t.Setenv("FLEET_SERVER_PRIVATE_KEY", strings.Repeat("a", 32)) + + ctx, cancel := context.WithCancel(context.Background()) + done := runServe(ctx) + // Always tear the server down, even if an assertion below aborts. + defer func() { + cancel() + waitShutdown(t, done) + }() + + healthy := waitHealthy(t, serverAddr) + require.Emptyf(t, rec.calls, "initFatal was called during boot: %v", rec.calls) + require.True(t, healthy, "server did not become healthy") + }) + + // An invalid Redis host-cache configuration (enabled with a non-positive + // TTL) must make the server fail fast through initFatal and return rather + // than start serving, exercising the Redis-init error path and the nil-pool + // guard in runServeCmd. + t.Run("refuses to boot on invalid host-cache config", func(t *testing.T) { + rec := installFatalRecorder(t) + configureBootEnv(t, dbName, 13) + t.Setenv("FLEET_REDIS_HOST_CACHE_ENABLED", "true") + t.Setenv("FLEET_REDIS_HOST_CACHE_TTL", "0") + + // The server fails fast on the invalid config and returns on its own, so + // no manual cancellation is needed; t.Context() is canceled at cleanup. + done := runServe(t.Context()) + + select { + case <-done: + case <-time.After(60 * time.Second): + t.Fatal("server did not return after invalid host-cache config") + } + + require.NotEmpty(t, rec.calls, "expected initFatal for invalid host-cache config") + assert.Truef(t, rec.contains("host_cache_ttl must be > 0"), + "expected a host-cache validation failure, got: %v", rec.calls) + }) +}