From 5a7a12696023d5444fec08ceb86829b595549f5f Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 28 Aug 2026 17:02:48 -0700 Subject: [PATCH] Make how the target version is started a seam Both phases that bring the target version up against a not-yet-migrated store -- the recovery cycle, and the ordinary boot validate does after it -- constructed a child process from a path on this host directly. That is the one thing about them packaging changes: a container runs an image against the data volume instead. Everything either of them is started *for* is identical afterwards. So starting it is now a Launcher, returning a Supervised the callers stop and read output from. BinaryLauncher is today's behaviour and the default when Options.Launcher is nil, so every existing caller is unchanged -- no test needed editing, which is the evidence for that rather than a claim about it. Deliberately narrower than the interface sketched in issue #3. Stage and cutover also differ by packaging, but designing their interfaces now would be designing against a guess: there is no second implementation yet to shape them, and the shape a container needs is what PR 3 and PR 4 find out. This seam is different because it already had two callers doing the same thing for the same reason, so extracting it describes the code rather than predicting it. outputSuffix now takes Supervised. It only ever needed Output(), and the diagnosis it exists to preserve -- the server's own words about a bind conflict or a rejected config value, which a bare timeout loses -- matters whatever started the process. --- internal/recovery/launcher.go | 89 +++++++++++++++++++++ internal/recovery/launcher_test.go | 123 +++++++++++++++++++++++++++++ internal/recovery/recovery.go | 19 +++-- internal/validate/bootcheck.go | 16 +++- 4 files changed, 238 insertions(+), 9 deletions(-) create mode 100644 internal/recovery/launcher.go create mode 100644 internal/recovery/launcher_test.go diff --git a/internal/recovery/launcher.go b/internal/recovery/launcher.go new file mode 100644 index 0000000..1876ae1 --- /dev/null +++ b/internal/recovery/launcher.go @@ -0,0 +1,89 @@ +// SPDX-FileCopyrightText: 2026 LINUXexpert-org +// SPDX-License-Identifier: GPL-3.0-or-later + +package recovery + +import ( + "context" + "time" +) + +// Supervised is one running instance of the target Stalwart version, +// however it was started. The two phases that bring the target version up +// against a not-yet-migrated store - the recovery cycle here, and +// validate.BootCheck's ordinary boot afterwards - need exactly three things +// from it: to stop it, to read what it printed, and (via WaitForHealthy, +// which only needs a URL) to know when it is ready. +// +// Output is not a nicety. A recovery boot that failed because port 8080 was +// already in use reported only "connection refused" after a 60-second +// timeout, while Stalwart had printed "Address already in use" immediately +// to a pipe nothing was reading. Anything reporting a supervised process +// failing has to be able to say why, whatever started it. +type Supervised interface { + Stop(gracePeriod time.Duration) error + Output() string +} + +// LaunchOptions is everything about running the target version that does +// not depend on how it is packaged. What differs by packaging - a path on +// this host, or an image and the mounts to run it against - belongs to the +// Launcher, which was constructed knowing it. +type LaunchOptions struct { + ConfigPath string + + // RecoveryMode sets STALWART_RECOVERY_MODE=1 and + // STALWART_RECOVERY_ADMIN=:, the environment + // Stalwart's own upgrade guide uses to bring the new version up against + // an unmigrated store. False means an ordinary boot. + RecoveryMode bool + AdminUser string + AdminPassword string + + // ExtraEnv is appended after any recovery-mode variables, so a + // rehearsal can point ports and paths at a sandbox without those two + // becoming the only way to parameterize the launch. + ExtraEnv []string +} + +// Launcher starts the staged target version. It exists because that is the +// one thing in this phase that packaging changes: a systemd install runs a +// binary this tool downloaded, a container runs an image against the data +// volume, and everything either of them is started *for* - the recovery +// cycle, the settings apply, the boot check - is identical afterwards. +// +// See ARCHITECTURE.md ยง4.4. The container implementation is issue #3. +type Launcher interface { + // Launch starts the instance and returns once the OS reports it + // started. It does not wait for Stalwart to be ready: callers use + // WaitForHealthy for that, because readiness is an HTTP question and + // the same one either way. + Launch(ctx context.Context, o LaunchOptions) (Supervised, error) +} + +// BinaryLauncher runs the target version as a child process from a path on +// this host - the systemd deployment's answer, and the only one until +// container support lands. +type BinaryLauncher struct { + BinaryPath string +} + +func (b BinaryLauncher) Launch(ctx context.Context, o LaunchOptions) (Supervised, error) { + p := &Process{} + if err := p.Start(ctx, ProcessOptions{ + BinaryPath: b.BinaryPath, + ConfigPath: o.ConfigPath, + RecoveryMode: o.RecoveryMode, + AdminUser: o.AdminUser, + AdminPassword: o.AdminPassword, + ExtraEnv: o.ExtraEnv, + }); err != nil { + return nil, err + } + return p, nil +} + +// Process satisfies Supervised. Asserted here rather than left to the call +// sites so the compiler catches it if either side drifts. +var _ Supervised = (*Process)(nil) +var _ Launcher = BinaryLauncher{} diff --git a/internal/recovery/launcher_test.go b/internal/recovery/launcher_test.go new file mode 100644 index 0000000..b557f51 --- /dev/null +++ b/internal/recovery/launcher_test.go @@ -0,0 +1,123 @@ +// SPDX-FileCopyrightText: 2026 LINUXexpert-org +// SPDX-License-Identifier: GPL-3.0-or-later + +package recovery + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/LINUXexpert-org/stalwart-migrator/internal/checkpoint" +) + +// recordingLauncher stands in for a deployment that is not a local binary - +// a container, when that lands. It records what it was asked for and +// delegates the actual process to BinaryLauncher, so the recovery cycle +// still has something real to talk to. +type recordingLauncher struct { + inner Launcher + calls int + lastOp LaunchOptions +} + +func (r *recordingLauncher) Launch(ctx context.Context, o LaunchOptions) (Supervised, error) { + r.calls++ + r.lastOp = o + return r.inner.Launch(ctx, o) +} + +// The whole point of the seam: a deployment that is not a host binary can +// supply its own way of starting the target version, and everything the +// recovery cycle does afterwards is unchanged. +func TestRunUsesTheSuppliedLauncher(t *testing.T) { + port := freePort(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + applyDir := t.TempDir() + withFakeExecutable(t, "stalwart-cli", fmt.Sprintf("#!/bin/sh\necho \"$@\" >> %q\nexit 0\n", argsFile(t, applyDir))) + exportFile := filepath.Join(t.TempDir(), "export.json") + if err := os.WriteFile(exportFile, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + store := checkpoint.NewStore(t.TempDir()) + rs, err := store.Create("0.15.5", "0.16.14") + if err != nil { + t.Fatal(err) + } + + launcher := &recordingLauncher{inner: BinaryLauncher{BinaryPath: testBinaryPath(t)}} + _, err = Run(context.Background(), store, rs, Options{ + // Deliberately empty: a supplied Launcher owns how the target + // version is started, so BinaryPath must not be consulted at all. + BinaryPath: "", + ConfigPath: configPath, + ListenURL: fmt.Sprintf("http://127.0.0.1:%d/", port), + AdminUser: "admin", + ApplyFiles: []string{exportFile}, + ExtraEnv: helperProcessEnv(port), + StartupTimeout: 5 * time.Second, + StopGrace: 5 * time.Second, + Launcher: launcher, + }) + if err != nil { + t.Fatalf("Run with a supplied launcher: %v", err) + } + if launcher.calls != 1 { + t.Errorf("launcher called %d times, want 1", launcher.calls) + } + if !launcher.lastOp.RecoveryMode { + t.Error("recovery cycle launched without RecoveryMode set") + } + if launcher.lastOp.AdminPassword == "" { + t.Error("recovery cycle launched without a generated admin password") + } + if launcher.lastOp.ConfigPath != configPath { + t.Errorf("ConfigPath = %q, want %q", launcher.lastOp.ConfigPath, configPath) + } +} + +// A nil Launcher has to keep meaning exactly what it meant before this +// seam existed, or every existing caller changes behaviour silently. +func TestNilLauncherStillRunsTheBinaryPath(t *testing.T) { + port := freePort(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + applyDir := t.TempDir() + applyLog := argsFile(t, applyDir) + withFakeExecutable(t, "stalwart-cli", fmt.Sprintf("#!/bin/sh\necho \"$@\" >> %q\nexit 0\n", applyLog)) + exportFile := filepath.Join(t.TempDir(), "export.json") + if err := os.WriteFile(exportFile, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + store := checkpoint.NewStore(t.TempDir()) + rs, err := store.Create("0.15.5", "0.16.14") + if err != nil { + t.Fatal(err) + } + if _, err := Run(context.Background(), store, rs, Options{ + BinaryPath: testBinaryPath(t), + ConfigPath: configPath, + ListenURL: fmt.Sprintf("http://127.0.0.1:%d/", port), + AdminUser: "admin", + ApplyFiles: []string{exportFile}, + ExtraEnv: helperProcessEnv(port), + StartupTimeout: 5 * time.Second, + StopGrace: 5 * time.Second, + // Launcher deliberately unset. + }); err != nil { + t.Fatalf("Run with a nil launcher: %v", err) + } + if got := readArgsFile(t, applyLog); got == "" { + t.Fatal("stalwart-cli apply was never invoked, so the cycle did not run") + } +} diff --git a/internal/recovery/recovery.go b/internal/recovery/recovery.go index 63720d8..57c8ef4 100644 --- a/internal/recovery/recovery.go +++ b/internal/recovery/recovery.go @@ -29,6 +29,11 @@ type Options struct { StartupTimeout time.Duration StopGrace time.Duration HTTPClient *http.Client + + // Launcher starts the target version. Nil means BinaryPath as a child + // process, which is what every caller wants today; a container + // deployment supplies its own (issue #3). + Launcher Launcher } // GenerateRecoveryPassword returns a fresh random one-time password for @@ -64,12 +69,16 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, return checkpoint.StepOutcome{}, err } - proc := &Process{} - if startErr := proc.Start(ctx, ProcessOptions{ - BinaryPath: opts.BinaryPath, ConfigPath: opts.ConfigPath, + launcher := opts.Launcher + if launcher == nil { + launcher = BinaryLauncher{BinaryPath: opts.BinaryPath} + } + proc, startErr := launcher.Launch(ctx, LaunchOptions{ + ConfigPath: opts.ConfigPath, RecoveryMode: true, AdminUser: opts.AdminUser, AdminPassword: password, ExtraEnv: opts.ExtraEnv, - }); startErr != nil { + }) + if startErr != nil { return checkpoint.StepOutcome{}, startErr } @@ -115,7 +124,7 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, // words are usually the whole diagnosis - a bind conflict, a rejected // config value - and without them the caller is left guessing at a // timeout. -func outputSuffix(proc *Process) string { +func outputSuffix(proc Supervised) string { out := strings.TrimSpace(proc.Output()) if out == "" { return " (the process produced no output)" diff --git a/internal/validate/bootcheck.go b/internal/validate/bootcheck.go index 2d54e39..9d322a9 100644 --- a/internal/validate/bootcheck.go +++ b/internal/validate/bootcheck.go @@ -29,6 +29,10 @@ type BootCheckOptions struct { StopGrace time.Duration HTTPClient *http.Client + // Launcher starts the instance this boots. Nil means BinaryPath as a + // child process - see recovery.Launcher. + Launcher recovery.Launcher + // ContentIntegrityBefore, if non-nil, is the pre-migration snapshot // preflight captured (checkpoint.RunState.PreflightSnapshot). When set, // BootCheck captures a fresh snapshot from the instance it just booted @@ -59,10 +63,14 @@ type BootCheckOptions struct { // so a retry just redoes the whole cycle - see recovery.Run's doc comment // for the full reasoning, which applies identically here. func BootCheck(ctx context.Context, o BootCheckOptions) (detail string, result *ContentIntegrityResult, err error) { - proc := &recovery.Process{} - if startErr := proc.Start(ctx, recovery.ProcessOptions{ - BinaryPath: o.BinaryPath, ConfigPath: o.ConfigPath, RecoveryMode: false, ExtraEnv: o.ExtraEnv, - }); startErr != nil { + launcher := o.Launcher + if launcher == nil { + launcher = recovery.BinaryLauncher{BinaryPath: o.BinaryPath} + } + proc, startErr := launcher.Launch(ctx, recovery.LaunchOptions{ + ConfigPath: o.ConfigPath, RecoveryMode: false, ExtraEnv: o.ExtraEnv, + }) + if startErr != nil { return "", nil, fmt.Errorf("validate: start normal boot: %w", startErr) }