From 062dcbfb1262788b4ea354f1192c748df3407660 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 30 Jul 2026 16:48:04 +0700 Subject: [PATCH] cmd/cli: report whether ctrld finished starting up, not just what SCM thinks "ctrld status" reported the service manager's view and nothing else, so it printed "Service is running" and exited 0 for the incident's process: alive, registered as started, stuck in API preflight, with no control socket, no DNS listener and no policy applied. The one command an operator reaches for first confirmed the service was fine while the host had no working DNS. Probe the control server's /started endpoint before reporting success. That endpoint only answers once the onStarted hooks have completed, which is after the listeners are up, so a successful probe means the process is serving rather than merely alive. A service that is registered as running but cannot confirm startup is now reported as such, with a pointer to the log, and exits 3 - distinct from stopped (1) and unknown (2), because it needs a different response. A probe blocked by permissions is not evidence of a broken service: an unprivileged caller still gets "Service is running", with a note that startup was not verified. The probe is bounded by a short timeout so status stays fast. Document the exit codes in the command's help, and cover the probe (ready, not finished starting, no socket, timed out) and the classification, including that an unreadable socket is not reported as a failure. The not-ready verdict is only reported when the probe could have found the daemon's socket. socketDir() is caller-relative on unix - the system directory when writable, the caller's home otherwise - so an unprivileged "ctrld status" looks somewhere the root-owned daemon never listened and gets ENOENT, which is "wrong path", not "not ready". Since only darwin has an elevation PreRun and the root-level alias has none, that is the normal invocation; reporting exit 3 there would have told a monitoring check to restart healthy daemons. Such a caller now gets the service manager's view with startup reported as unverified. Windows and mobile resolve the same directory for every caller, so the verdict stays fully available on the platform the hung start was seen on. A successful probe is still conclusive whoever ran it. --- cmd/cli/commands_service.go | 2 + cmd/cli/commands_service_status.go | 193 +++++++++++++++- cmd/cli/commands_service_status_test.go | 281 ++++++++++++++++++++++++ cmd/cli/service_image_path.go | 59 +++++ cmd/cli/service_image_path_others.go | 8 + cmd/cli/service_image_path_test.go | 103 +++++++++ cmd/cli/service_image_path_windows.go | 41 ++++ 7 files changed, 683 insertions(+), 4 deletions(-) create mode 100644 cmd/cli/commands_service_status_test.go create mode 100644 cmd/cli/service_image_path.go create mode 100644 cmd/cli/service_image_path_others.go create mode 100644 cmd/cli/service_image_path_test.go create mode 100644 cmd/cli/service_image_path_windows.go diff --git a/cmd/cli/commands_service.go b/cmd/cli/commands_service.go index fb5c661..212961e 100644 --- a/cmd/cli/commands_service.go +++ b/cmd/cli/commands_service.go @@ -114,6 +114,7 @@ func InitServiceCmd(rootCmd *cobra.Command) *cobra.Command { statusCmd := &cobra.Command{ Use: "status", Short: "Show status of the ctrld service", + Long: statusCmdLong, Args: cobra.NoArgs, RunE: sc.Status, } @@ -202,6 +203,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`, statusCmdAlias := &cobra.Command{ Use: "status", Short: "Show status of the ctrld service", + Long: statusCmdLong, Args: cobra.NoArgs, RunE: statusCmd.RunE, } diff --git a/cmd/cli/commands_service_status.go b/cmd/cli/commands_service_status.go index 270e0e0..6a5bf14 100644 --- a/cmd/cli/commands_service_status.go +++ b/cmd/cli/commands_service_status.go @@ -1,12 +1,57 @@ package cli import ( + "errors" + "fmt" + "io/fs" + "net/http" "os" + "path/filepath" + "runtime" + "time" "github.com/kardianos/service" "github.com/spf13/cobra" ) +// Exit codes reported by "ctrld status". +const ( + statusExitRunning = 0 + statusExitStopped = 1 + statusExitUnknown = 2 + // statusExitNotReady means the service manager considers the service running, + // but the process has not finished starting up, so it is not serving DNS or + // applying policy. This is a distinct code because it needs a distinct response: + // the process exists, so restarting the service is what recovers it, while a + // stopped service needs starting and an unknown state needs investigation. + statusExitNotReady = 3 +) + +// serviceReadinessTimeout bounds the control-socket probe. Status must answer +// quickly, and a service that cannot respond within this window is not usefully +// "running" from a caller's point of view either way. +const serviceReadinessTimeout = 3 * time.Second + +// statusCmdLong documents what the reported states mean, including that a service the +// OS calls running is not necessarily serving. +const statusCmdLong = `Show status of the ctrld service. + +Reports both what the OS service manager thinks and whether ctrld has finished +starting up, since a service can be registered as running while its process is +still in startup and serving nothing. + +Exit codes: + 0 running and serving, or running with startup not verified + 1 stopped + 2 status unknown + 3 registered as running, but startup has not completed + +Verifying startup requires reaching ctrld's control socket. On Linux, BSD and macOS +that socket lives in a directory only the privileged user resolves, so an +unprivileged "ctrld status" reports the service manager's view and says startup was +not verified rather than claiming the service is unhealthy. Exit 3 is only reported +when the check could actually be made.` + // Status implements the logic from cmdStatus.Run func (sc *ServiceCommand) Status(cmd *cobra.Command, args []string) error { logger := mainLog.Load() @@ -27,15 +72,155 @@ func (sc *ServiceCommand) Status(cmd *cobra.Command, args []string) error { switch status { case service.StatusUnknown: logger.Notice().Msg("Unknown status") - os.Exit(2) + os.Exit(statusExitUnknown) case service.StatusRunning: - logger.Notice().Msg("Service is running") - os.Exit(0) + // The service manager only knows a process was created. During the Windows + // Firewall Mode incident it reported "running" for a process stuck in API + // preflight that had never created its control socket, started a DNS + // listener, or applied any policy - and "Service is running" sent the + // investigation looking elsewhere. Probe readiness before claiming it. + ready, probeErr := serviceReady() + if probeErr != nil { + logger.Debug().Err(probeErr).Msg("Readiness probe did not confirm startup") + } + r := classifyReadiness(ready, probeErr, readinessVerifiable()) + for _, msg := range r.messages { + logger.Notice().Msg(msg) + } + os.Exit(r.exitCode) case service.StatusStopped: logger.Notice().Msg("Service is stopped") - os.Exit(1) + os.Exit(statusExitStopped) } logger.Debug().Msg("Service status command completed") return nil } + +// readiness is what "ctrld status" reports for a service the service manager +// considers running. +type readiness struct { + messages []string + exitCode int +} + +// readinessVerifiable reports whether a failed control-socket probe can be trusted to +// mean "the service has not finished starting up". +// +// It can only mean that if this process resolves the same socket path the daemon +// created, and socketDir() is caller-relative on unix: it returns the system directory +// only when that is writable, and the caller's home directory otherwise. So a +// root-owned daemon listens on /var/run/ctrld_control.sock while an unprivileged +// "ctrld status" looks under $HOME, finds nothing, and gets ENOENT - which means "wrong +// path", not "not ready". Reporting exit 3 there would tell a monitoring check to +// restart a perfectly healthy daemon. +// +// On Windows and mobile socketDir() is the install/home directory for every caller, so +// the probe is comparable - which matters because Windows is where the hung-start this +// exit code exists for was seen. On Windows that only holds while this binary is the +// installed one: a copy run from elsewhere resolves a different socket directory, so its +// failed probe would say nothing about the service. installedServiceDirMatches() checks +// that, and answers true when it cannot tell, preserving the previous behaviour. +func readinessVerifiable() bool { + if isMobile() { + return true + } + if runtime.GOOS == "windows" { + return installedServiceDirMatches() + } + elevated, err := hasElevatedPrivilege() + return err == nil && elevated +} + +// classifyReadiness turns a control-socket probe result into the report for a service +// the service manager calls running. +// +// verifiable comes from readinessVerifiable: when it is false a failed probe says +// nothing about the service, so the report falls back to the service manager's view. +// A *successful* probe is still conclusive either way - reaching the socket at all is +// positive evidence, whoever the caller is. +func classifyReadiness(ready bool, err error, verifiable bool) readiness { + switch { + case ready: + return readiness{ + messages: []string{"Service is running"}, + exitCode: statusExitRunning, + } + case !verifiable: + return readiness{ + messages: []string{"Service is running (startup not verified: re-run with elevated privileges to check readiness)"}, + exitCode: statusExitRunning, + } + case errors.Is(err, errReadinessNotReported): + // The service answered, just not with a verdict - an older daemon without the + // /started route. It is alive and reachable, so the service manager's view is + // the best available answer. + return readiness{ + messages: []string{"Service is running (startup not verified: this ctrld build does not report readiness)"}, + exitCode: statusExitRunning, + } + case errors.Is(err, fs.ErrPermission): + // Without access to the control socket there is nothing to report beyond the + // service manager's view. Do not call a service unhealthy because the caller + // lacks privilege. + return readiness{ + messages: []string{"Service is running (startup not verified: control socket requires elevated privileges)"}, + exitCode: statusExitRunning, + } + default: + return readiness{ + messages: []string{ + "Service is registered as running, but has not completed startup: it is not serving DNS", + "Check the ctrld log for why startup did not finish, then restart the service", + }, + exitCode: statusExitNotReady, + } + } +} + +// serviceReady reports whether a running ctrld has finished starting up, by asking +// its control server. The control server answers /started only once the onStarted +// hooks have completed, which is after the DNS listeners are up, so a successful +// probe means the process is actually serving rather than merely alive. +// +// An error means "could not confirm readiness" and is returned for the caller to +// classify: a refused connection or missing socket is a process that never got that +// far, while a permission error says nothing about the service's health. +func serviceReady() (bool, error) { + dir, err := socketDir() + if err != nil { + return false, err + } + return serviceReadyAt(filepath.Join(dir, ControlSocketName()), serviceReadinessTimeout) +} + +// errReadinessNotReported marks a control server that answered without a readiness +// verdict. +// +// http.Client.Post returns (resp, nil) for any status, so a daemon with no /started +// route answers 404 and an internal failure answers 5xx - neither says the service has +// not started. Reporting "not ready" there tells a monitoring check to restart a healthy +// service, and it happens in normal operation: after an upgrade replaces the binary on +// disk but before the service restarts, and throughout a mixed-version rollout. +var errReadinessNotReported = errors.New("control server did not report readiness") + +// serviceReadyAt is serviceReady against an explicit socket path and timeout. +func serviceReadyAt(sockPath string, timeout time.Duration) (bool, error) { + cc := newControlClient(sockPath) + cc.c.Timeout = timeout + resp, err := cc.post(startedPath, nil) + if err != nil { + return false, err + } + defer resp.Body.Close() + switch resp.StatusCode { + case http.StatusOK: + return true, nil + case http.StatusRequestTimeout: + // The daemon's own verdict: its onStarted hooks have not completed. This is the + // hung start statusExitNotReady exists for. + return false, nil + default: + return false, fmt.Errorf("%w: HTTP %d", errReadinessNotReported, resp.StatusCode) + } +} diff --git a/cmd/cli/commands_service_status_test.go b/cmd/cli/commands_service_status_test.go new file mode 100644 index 0000000..2d4d3b3 --- /dev/null +++ b/cmd/cli/commands_service_status_test.go @@ -0,0 +1,281 @@ +package cli + +import ( + "errors" + "io/fs" + "net" + "net/http" + "os" + "path/filepath" + "runtime" + "testing" + "time" +) + +// startControlSocket serves handler on a unix socket and returns its path. +func startControlSocket(t *testing.T, handler http.HandlerFunc) string { + t.Helper() + // Keep the path short: unix socket paths have a low length limit. + dir, err := os.MkdirTemp("", "ctrldsock") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + + sockPath := filepath.Join(dir, "s.sock") + ln, err := net.Listen("unix", sockPath) + if err != nil { + t.Skipf("cannot listen on a unix socket: %v", err) + } + mux := http.NewServeMux() + mux.Handle(startedPath, handler) + srv := &http.Server{Handler: mux} + go func() { _ = srv.Serve(ln) }() + t.Cleanup(func() { _ = srv.Close() }) + return sockPath +} + +func TestServiceReadyAt(t *testing.T) { + t.Run("ready when the control server reports started", func(t *testing.T) { + sock := startControlSocket(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + ready, err := serviceReadyAt(sock, time.Second) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ready { + t.Error("ready = false, want true") + } + }) + + t.Run("not ready when startup has not finished", func(t *testing.T) { + // What /started returns when the onStarted hooks have not completed. + sock := startControlSocket(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusRequestTimeout) + }) + ready, err := serviceReadyAt(sock, time.Second) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ready { + t.Error("ready = true for a control server that has not finished startup") + } + }) + + t.Run("not ready when there is no control socket", func(t *testing.T) { + // The incident: the process was alive but had never created the socket, so + // every control request was refused. + ready, err := serviceReadyAt(filepath.Join(t.TempDir(), "absent.sock"), time.Second) + if ready { + t.Error("ready = true with no control socket") + } + if err == nil { + t.Error("expected an error when the control socket does not exist") + } + }) + + t.Run("not ready when the probe times out", func(t *testing.T) { + sock := startControlSocket(t, func(w http.ResponseWriter, r *http.Request) { + time.Sleep(2 * time.Second) + w.WriteHeader(http.StatusOK) + }) + ready, err := serviceReadyAt(sock, 50*time.Millisecond) + if ready { + t.Error("ready = true for a probe that timed out") + } + if err == nil { + t.Error("expected an error when the probe times out") + } + }) +} + +func TestClassifyReadiness(t *testing.T) { + tests := []struct { + name string + ready bool + err error + verifiable bool + wantCode int + }{ + { + name: "ready", + ready: true, + verifiable: true, + wantCode: statusExitRunning, + }, + { + // The service manager says running, the process is not serving. This + // must not report success. + name: "running but never finished startup", + err: errors.New("connect: connection refused"), + verifiable: true, + wantCode: statusExitNotReady, + }, + { + // A caller without privilege cannot probe; that is not evidence of a + // broken service, so it must not be reported as one. + name: "probe not permitted", + err: fs.ErrPermission, + verifiable: true, + wantCode: statusExitRunning, + }, + { + name: "wrapped permission error", + err: &net.OpError{Op: "dial", Err: fs.ErrPermission}, + verifiable: true, + wantCode: statusExitRunning, + }, + { + // The P2: an unprivileged caller on unix resolves a socket path the + // daemon never used, so the probe fails with ENOENT rather than a + // permission error. That says nothing about the service and must not be + // reported as unhealthy - a monitoring check acting on exit 3 would + // restart a healthy daemon. + name: "missing socket at an unverifiable path", + err: &net.OpError{Op: "dial", Err: os.ErrNotExist}, + verifiable: false, + wantCode: statusExitRunning, + }, + { + name: "connection refused at an unverifiable path", + err: errors.New("connect: connection refused"), + verifiable: false, + wantCode: statusExitRunning, + }, + { + // A probe that actually reached the socket is conclusive whoever ran it. + name: "successful probe is trusted even when unverifiable", + ready: true, + verifiable: false, + wantCode: statusExitRunning, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := classifyReadiness(tc.ready, tc.err, tc.verifiable) + if got.exitCode != tc.wantCode { + t.Errorf("exitCode = %d, want %d", got.exitCode, tc.wantCode) + } + if len(got.messages) == 0 { + t.Error("no message to report") + } + }) + } +} + +// TestReadinessVerifiableMatchesSocketVisibility is the closure test for the P2: the +// not-ready verdict must only be reachable when this process resolves the same socket +// directory the daemon uses. +// +// On unix that is the privileged user's path, so an unprivileged run - which is how +// "ctrld status" is normally invoked, since only darwin has an elevation PreRun and the +// root-level alias has none - must not be able to reach exit 3. +func TestReadinessVerifiableMatchesSocketVisibility(t *testing.T) { + verifiable := readinessVerifiable() + + if runtime.GOOS == "windows" { + if !verifiable { + t.Error("on Windows every caller resolves the install directory, so the probe is always verifiable") + } + return + } + + elevated, err := hasElevatedPrivilege() + if err != nil { + t.Skipf("cannot determine privilege: %v", err) + } + if verifiable != elevated { + t.Errorf("readinessVerifiable() = %v, want %v (elevated)", verifiable, elevated) + } + + if !elevated { + // The shape the review asked to assert: unprivileged, healthy daemon, and a + // probe that cannot see its socket must still report running. + dir, err := socketDir() + if err != nil { + t.Fatalf("socketDir(): %v", err) + } + if dir == "/var/run" { + t.Skip("unprivileged but /var/run is writable, so the probe path does match") + } + r := classifyReadiness(false, &net.OpError{Op: "dial", Err: os.ErrNotExist}, verifiable) + if r.exitCode == statusExitNotReady { + t.Errorf("unprivileged status probing %q reported not-ready (exit %d) for a healthy service", dir, r.exitCode) + } + } +} + +// Every status must map to its own exit code: a caller that cannot tell a hung +// service from a healthy or a stopped one is back to the incident's diagnostics. +// +// The literal values are the contract. statusCmdLong documents them and monitoring +// scripts key off them, so asserting the constants against each other would let a +// renumbering keep the suite green while silently breaking every caller. +func TestStatusExitCodesAreDistinct(t *testing.T) { + for _, tc := range []struct { + name string + got int + want int + }{ + {"running", statusExitRunning, 0}, + {"stopped", statusExitStopped, 1}, + {"unknown", statusExitUnknown, 2}, + {"not ready", statusExitNotReady, 3}, + } { + if tc.got != tc.want { + t.Errorf("%s exit code = %d, want %d: statusCmdLong and monitoring scripts document this value", tc.name, tc.got, tc.want) + } + } + + codes := map[int]string{ + statusExitRunning: "running", + statusExitStopped: "stopped", + statusExitUnknown: "unknown", + statusExitNotReady: "not ready", + } + if len(codes) != 4 { + t.Errorf("status exit codes collide, only %d distinct: %v", len(codes), codes) + } +} + +// TestReadinessProbeStatusHandling covers what each control-server answer means. +// +// http.Client.Post returns (resp, nil) for any status code, so a daemon without the +// /started route answers 404 and the probe must report "cannot confirm" rather than "not +// started". That state is reached in normal operation - after an upgrade replaces the +// binary but before the service restarts, and throughout a mixed-version rollout - and +// reporting exit 3 there tells monitoring to restart a healthy service. +func TestReadinessProbeStatusHandling(t *testing.T) { + tests := []struct { + name string + status int + wantReady bool + wantReported bool // whether the answer carries a readiness verdict + wantExitCode int + }{ + {"started", http.StatusOK, true, true, statusExitRunning}, + {"still starting", http.StatusRequestTimeout, false, true, statusExitNotReady}, + {"no readiness route", http.StatusNotFound, false, false, statusExitRunning}, + {"control server error", http.StatusInternalServerError, false, false, statusExitRunning}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + status := tc.status + sock := startControlSocket(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + }) + + ready, err := serviceReadyAt(sock, time.Second) + if ready != tc.wantReady { + t.Errorf("ready = %v, want %v", ready, tc.wantReady) + } + if reported := !errors.Is(err, errReadinessNotReported); reported != tc.wantReported { + t.Errorf("readiness reported = %v, want %v (err: %v)", reported, tc.wantReported, err) + } + if got := classifyReadiness(ready, err, true).exitCode; got != tc.wantExitCode { + t.Errorf("exit code = %d, want %d", got, tc.wantExitCode) + } + }) + } +} diff --git a/cmd/cli/service_image_path.go b/cmd/cli/service_image_path.go new file mode 100644 index 0000000..6b2fc8c --- /dev/null +++ b/cmd/cli/service_image_path.go @@ -0,0 +1,59 @@ +package cli + +import "strings" + +// serviceBinaryFromImagePath extracts the executable path from a Windows service +// ImagePath value, which carries the command line rather than a bare path: it may be +// quoted and is usually followed by arguments, e.g. +// +// "C:\Program Files\Control D\ctrld.exe" run --config C:\...\ctrld.toml +// +// It returns "" when no path can be read, which callers must treat as "cannot tell" +// rather than "does not match". +func serviceBinaryFromImagePath(imagePath string) string { + imagePath = strings.TrimSpace(imagePath) + if imagePath == "" { + return "" + } + if imagePath[0] == '"' { + // Quoted form: everything up to the closing quote is the path, so a directory + // containing spaces stays intact. + if end := strings.IndexByte(imagePath[1:], '"'); end >= 0 { + return strings.TrimSpace(imagePath[1 : 1+end]) + } + return strings.TrimSpace(imagePath[1:]) + } + // Unquoted form: the path cannot contain spaces, so the first field is it. + if idx := strings.IndexByte(imagePath, ' '); idx >= 0 { + return strings.TrimSpace(imagePath[:idx]) + } + return imagePath +} + +// sameExecutableDir reports whether two Windows executable paths live in the same +// directory, compared case-insensitively because Windows paths are. +// +// The separator handling is explicit rather than filepath's, because filepath follows the +// *host* rules: off Windows it does not treat "\\" as a separator, so every backslash path +// would reduce to the same directory and any two paths would compare equal. Doing it here +// keeps the comparison correct and testable on any host. +// +// A path with no directory part answers false, which callers read as "cannot tell". +func sameExecutableDir(a, b string) bool { + dirA, dirB := windowsExecutableDir(a), windowsExecutableDir(b) + if dirA == "" || dirB == "" { + return false + } + return strings.EqualFold(dirA, dirB) +} + +// windowsExecutableDir returns the directory part of a Windows path, accepting either +// separator and normalising to a backslash. It returns "" when there is no directory part. +func windowsExecutableDir(path string) string { + path = strings.TrimSpace(path) + idx := strings.LastIndexAny(path, `\/`) + if idx <= 0 { + return "" + } + return strings.ReplaceAll(path[:idx], "/", `\`) +} diff --git a/cmd/cli/service_image_path_others.go b/cmd/cli/service_image_path_others.go new file mode 100644 index 0000000..866308b --- /dev/null +++ b/cmd/cli/service_image_path_others.go @@ -0,0 +1,8 @@ +//go:build !windows + +package cli + +// installedServiceDirMatches is Windows-only: it exists because socketDir() there is +// relative to the running executable. Other platforms answer this question through +// hasElevatedPrivilege in readinessVerifiable. +func installedServiceDirMatches() bool { return true } diff --git a/cmd/cli/service_image_path_test.go b/cmd/cli/service_image_path_test.go new file mode 100644 index 0000000..2fb050f --- /dev/null +++ b/cmd/cli/service_image_path_test.go @@ -0,0 +1,103 @@ +package cli + +import "testing" + +// TestServiceBinaryFromImagePath covers the ImagePath shapes Windows stores. Getting this +// wrong makes readinessVerifiable compare the wrong directories, and "ctrld status" would +// then report a healthy service as not-ready - the false positive the readiness exit code +// exists to avoid. +func TestServiceBinaryFromImagePath(t *testing.T) { + tests := []struct { + name string + imagePath string + want string + }{ + { + // The installed form: quoted because the directory contains a space, with the + // service arguments following it. + name: "quoted path with arguments", + imagePath: `"C:\Program Files\Control D\ctrld.exe" run --config "C:\ProgramData\Control D\ctrld.toml"`, + want: `C:\Program Files\Control D\ctrld.exe`, + }, + { + name: "quoted path without arguments", + imagePath: `"C:\Program Files\Control D\ctrld.exe"`, + want: `C:\Program Files\Control D\ctrld.exe`, + }, + { + name: "unquoted path with arguments", + imagePath: `C:\ctrld\ctrld.exe run --cd abc123`, + want: `C:\ctrld\ctrld.exe`, + }, + { + name: "unquoted path alone", + imagePath: `C:\ctrld\ctrld.exe`, + want: `C:\ctrld\ctrld.exe`, + }, + { + name: "surrounding whitespace", + imagePath: ` "C:\ctrld\ctrld.exe" run `, + want: `C:\ctrld\ctrld.exe`, + }, + { + // Unterminated quote: take what is there rather than returning nothing, since + // "" means "cannot tell" and would silently disable the check. + name: "unterminated quote", + imagePath: `"C:\ctrld\ctrld.exe run`, + want: `C:\ctrld\ctrld.exe run`, + }, + { + name: "empty", + imagePath: "", + want: "", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := serviceBinaryFromImagePath(tc.imagePath); got != tc.want { + t.Errorf("serviceBinaryFromImagePath(%q) = %q, want %q", tc.imagePath, got, tc.want) + } + }) + } +} + +// TestSameExecutableDir pins the comparison itself: Windows paths are case-insensitive, and +// an empty side means "cannot tell", which must never read as a match. +func TestSameExecutableDir(t *testing.T) { + tests := []struct { + name string + a string + b string + want bool + }{ + { + name: "same directory", + a: `C:\Program Files\Control D\ctrld.exe`, + b: `C:\Program Files\Control D\ctrld.exe`, + want: true, + }, + { + name: "same directory different case", + a: `C:\Program Files\Control D\ctrld.exe`, + b: `c:\program files\control d\ctrld.exe`, + want: true, + }, + { + // The case the check exists for: a copy run from a download directory + // resolves a different control socket than the installed service. + name: "different directory", + a: `C:\Program Files\Control D\ctrld.exe`, + b: `C:\Users\admin\Downloads\ctrld.exe`, + want: false, + }, + {name: "unknown installed path", a: "", b: `C:\ctrld\ctrld.exe`, want: false}, + {name: "unknown self path", a: `C:\ctrld\ctrld.exe`, b: "", want: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := sameExecutableDir(tc.a, tc.b); got != tc.want { + t.Errorf("sameExecutableDir(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) + } + }) + } +} diff --git a/cmd/cli/service_image_path_windows.go b/cmd/cli/service_image_path_windows.go new file mode 100644 index 0000000..f7c6ef8 --- /dev/null +++ b/cmd/cli/service_image_path_windows.go @@ -0,0 +1,41 @@ +//go:build windows + +package cli + +import ( + "os" + + "golang.org/x/sys/windows/registry" +) + +// installedServiceDirMatches reports whether this executable is the installed service +// binary, by comparing its directory with the one in the service's registered ImagePath. +// +// socketDir() on Windows is relative to the running executable, so a ctrld.exe run from +// somewhere else - a download directory, a build tree - looks for the control socket in +// its own directory and never finds the installed daemon's. A failed probe from there +// says nothing about the service's health, and reporting "not ready" for it would tell +// monitoring to restart a healthy service. +// +// Anything unreadable answers true, keeping the previous behaviour: readiness stays +// verifiable unless there is positive evidence of a different install. +func installedServiceDirMatches() bool { + self, err := os.Executable() + if err != nil { + return true + } + key, err := registry.OpenKey(registry.LOCAL_MACHINE, `SYSTEM\CurrentControlSet\Services\`+ctrldServiceName, registry.QUERY_VALUE) + if err != nil { + return true + } + defer key.Close() + imagePath, _, err := key.GetStringValue("ImagePath") + if err != nil { + return true + } + installed := serviceBinaryFromImagePath(imagePath) + if installed == "" { + return true + } + return sameExecutableDir(installed, self) +}