cli: refuse a --distro that conflicts with the installed engine

CHANGELOG.md

@@ -22,6 +22,12 @@ useful than saying where the real one is.
   process". It now retries the compaction itself until the disk is released (about a
   minute) or `--wait` runs out. `compact` also no longer boots an engine that was already
   stopped just to stop it, which with `--no-trim` saves that minute.
+- **`--distro` no longer acts on a different engine** ([#561](https://github.com/wslkit/skrog/issues/561)).
+  Wherever an install exists, its manifest decided the distro and the flag was silently
+  ignored: `skrog compact --distro X` compacted the installed engine instead. `compact`,
+  `enable-gpu`, `relocate`, `serve`, `proxy`, `supervise` and `uninstall` now refuse a
+  `--distro` that differs from the installed one (exit 2) and point at `--state-dir`, which
+  is how another install is reached. The installed name itself, in any case, is accepted.
 - **`wsl.auto-memory-reclaim` now takes effect** ([#550](https://github.com/wslkit/skrog/issues/550)).
   `skrog wsl-config apply` wrote `autoMemoryReclaim` under `[wsl2]`, but WSL reads it only
   as `experimental.autoMemoryReclaim`: it ignored the line and printed `Unknown key

cmd/skrog/compact.go

@@ -72,6 +72,9 @@ flags:
 
 	opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro})
 	p := &provision.Provisioner{Logger: cliLogger(false)}
+	if refuseDistroConflict(p, opts) {
+		return exitUsage
+	}
 	target, ok := requireDistroInstall(p, opts)
 	if !ok {
 		return exitNotFound

cmd/skrog/distroconflict_test.go

@@ -0,0 +1,91 @@
+package main
+
+import (
+	"io"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/wslkit/skrog/internal/provision"
+)
+
+// runStderr runs a command's entry point and returns its exit code and stderr.
+func runStderr(t *testing.T, run func([]string) int, args []string) (int, string) {
+	t.Helper()
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	old := os.Stderr
+	os.Stderr = w
+	done := make(chan string, 1)
+	go func() {
+		b, _ := io.ReadAll(r)
+		done <- string(b)
+	}()
+	code := run(args)
+	w.Close()
+	os.Stderr = old
+	return code, <-done
+}
+
+// `compact --distro X` on a machine whose engine is Y compacted Y (#561): the
+// manifest wins wherever a distro is resolved. Every command that takes
+// --distro must refuse the conflict before doing anything at all.
+//
+// Neither distro exists, so a command that ignored the conflict would fail
+// against nothing -- but each one also runs only as far as its own refusal
+// once fixed, which is what this asserts.
+func TestDistroConflictIsRefusedBeforeAnything(t *testing.T) {
+	cases := []struct {
+		name string
+		run  func([]string) int
+		args []string
+	}{
+		{"compact", runCompact, nil},
+		{"enable-gpu", runEnableGPU, nil},
+		{"relocate", runRelocate, []string{`Z:\skrog-561-nowhere`, "--dry-run"}},
+		{"serve", runServe, []string{"--tcp", "127.0.0.1:0"}},
+		{"proxy", runProxy, nil},
+		{"supervise", runSupervise, nil},
+		{"uninstall", runUninstall, []string{"--yes"}},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			opts := writeManifest(t, provision.Manifest{Distro: "skrog-561-recorded"})
+			before, _ := os.ReadDir(opts.StateDir)
+			args := append([]string{"--distro", "skrog-561-other", "--state-dir", opts.StateDir}, c.args...)
+
+			code, stderr := runStderr(t, c.run, args)
+			if code != exitUsage {
+				t.Errorf("exit = %d, want %d (usage)", code, exitUsage)
+			}
+			for _, want := range []string{"skrog-561-other", "skrog-561-recorded", "--state-dir"} {
+				if !strings.Contains(stderr, want) {
+					t.Errorf("stderr does not mention %q:\n%s", want, stderr)
+				}
+			}
+			after, _ := os.ReadDir(opts.StateDir)
+			if len(after) != len(before) {
+				t.Errorf("the refused command changed the state dir: %d entries, then %d", len(before), len(after))
+			}
+		})
+	}
+}
+
+// The same name, in any case, is not a conflict: WSL distro names are
+// case-insensitive, and the e2e suite passes --distro with the manifest's own.
+func TestDistroMatchingTheManifestIsNoConflict(t *testing.T) {
+	opts := writeManifest(t, provision.Manifest{Distro: "skrog-engine"})
+	for _, d := range []string{"", "skrog-engine", "Skrog-Engine"} {
+		opts.Distro = d
+		if msg := distroConflict(&provision.Provisioner{}, opts); msg != "" {
+			t.Errorf("--distro %q: %s", d, msg)
+		}
+	}
+	// With no install, --distro is how a distro is chosen at all.
+	none := provision.Options{StateDir: t.TempDir(), Distro: "hand-rolled"}
+	if msg := distroConflict(&provision.Provisioner{}, none); msg != "" {
+		t.Errorf("no manifest: %s", msg)
+	}
+}

cmd/skrog/distroinstall.go

@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/wslkit/skrog/internal/provision"
 	"github.com/wslkit/skrog/internal/selfexe"
@@ -47,6 +48,39 @@ const legacyBackendMessage = "skrog: this install uses the WSL container session
 	"  belong to the other engine. Save anything you need with `wslc` before\n" +
 	"  uninstalling.\n"
 
+// distroConflict is what to say when --distro names a different distro from
+// the one the install manifest records, or "" when there is no conflict.
+//
+// The manifest wins everywhere a distro is resolved, so `compact --distro X`
+// on a machine whose engine is Y used to compact Y (#561). Honouring the flag
+// instead would pair X with Y's state -- its disk path, its secret, its
+// supervisor -- so a conflict is refused. Another install is reached by its
+// --state-dir, which carries its own manifest.
+func distroConflict(p *provision.Provisioner, opts provision.Options) string {
+	if opts.Distro == "" {
+		return ""
+	}
+	opts = optsWithResolvedStateDir(opts)
+	m, err := p.ReadManifest(opts)
+	if err != nil || m.Distro == "" || strings.EqualFold(m.Distro, opts.Distro) {
+		return ""
+	}
+	return fmt.Sprintf("skrog: --distro %s does not match this install's engine, %s (recorded in %s).\n"+
+		"  --distro only chooses a distro where there is no install; to act on another\n"+
+		"  install, pass its --state-dir.\n", opts.Distro, m.Distro, opts.StateDir)
+}
+
+// refuseDistroConflict prints distroConflict's message, reporting whether
+// there was one.
+func refuseDistroConflict(p *provision.Provisioner, opts provision.Options) bool {
+	msg := distroConflict(p, opts)
+	if msg == "" {
+		return false
+	}
+	fmt.Fprint(os.Stderr, msg)
+	return true
+}
+
 // requireDistroInstall resolves the engine distro for a command that needs
 // one, and explains itself when there is not one.
 func requireDistroInstall(p *provision.Provisioner, opts provision.Options) (string, bool) {

cmd/skrog/gpu.go

@@ -74,6 +74,9 @@ flags:
 	opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro, GPUVendor: string(v)})
 	log := cliLogger(false)
 	p := &provision.Provisioner{Logger: log}
+	if refuseDistroConflict(p, opts) {
+		return exitUsage
+	}
 
 	targetDistro, ok := requireDistroInstall(p, opts)
 	if !ok {

cmd/skrog/install.go

@@ -326,6 +326,9 @@ flags:
 	opts := provision.Options{Distro: *distro, StateDir: *stateDir}
 	log := cliLogger(false)
 	p := &provision.Provisioner{Logger: log}
+	if refuseDistroConflict(p, opts) {
+		return exitUsage
+	}
 
 	// A named engine is removed with `skrog target remove`; uninstalling one as
 	// if it were an install would leave its context and logon entry behind.

cmd/skrog/proxy.go

@@ -67,6 +67,9 @@ flags:
 	// The manifest knows which distro this machine actually has, which matters
 	// when it was installed under a custom name.
 	p := &provision.Provisioner{Logger: log}
+	if refuseDistroConflict(p, opts) {
+		return exitUsage
+	}
 	targetDistro := opts.Distro
 	if m, err := p.ReadManifest(opts); err == nil && m.Distro != "" {
 		targetDistro = m.Distro

cmd/skrog/relocate.go

@@ -87,6 +87,9 @@ flags:
 
 	opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro})
 	p := &provision.Provisioner{Logger: cliLogger(false)}
+	if refuseDistroConflict(p, opts) {
+		return exitUsage
+	}
 	name, ok := requireDistroInstall(p, opts)
 	if !ok {
 		return exitNotFound

cmd/skrog/serve.go

@@ -72,14 +72,17 @@ flags:
 
 	opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro})
 	log := cliLogger(false)
+	p := &provision.Provisioner{Logger: log}
+	if refuseDistroConflict(p, opts) {
+		return exitUsage
+	}
 
 	cfg, err := serverTLSConfig(tlsDir(opts.StateDir))
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "skrog: %v\n", err)
 		return exitNotFound
 	}
 
-	p := &provision.Provisioner{Logger: log}
 	targetDistro, ok := resolveDistro(p, opts)
 	if !ok {
 		fmt.Fprintln(os.Stderr, "skrog: no install found. Run `skrog install` first.")

cmd/skrog/supervise.go

@@ -180,6 +180,11 @@ flags:
 	opts := provision.Options{Distro: *distro, StateDir: *stateDir}
 	opts = optsWithResolvedStateDir(opts)
 
+	// A refused command must not take the lock or clear the endpoint record.
+	if refuseDistroConflict(&provision.Provisioner{}, opts) {
+		return exitUsage
+	}
+
 	// Single instance before anything else: two supervisors would fight over
 	// the pipe and the engine.
 	lock, err := supervise.Acquire(opts.StateDir)