From f391318ebca1576755f8a8c7c4e0334662d9ac31 Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Thu, 8 Oct 2026 04:03:43 -0400 Subject: [PATCH] fix(cli): roll back an upgrade whose new build never becomes ready - Stop closes the control server right after the deactivation-pin check, so a stopping process no longer answers /started for its successor. releaseResources still closes it for stop paths that skip Stop; a sync.Once makes the double close safe. - upgrade and restart wait for a 200 from /started via newReadySocketControlClient; a 408 (start never finished) now rolls the upgrade back and makes restart warn instead of reporting success. - The plain socket client still accepts any answer, for the deactivation-pin path. --- cmd/cli/cli.go | 41 +++++++- cmd/cli/commands_service_restart.go | 10 +- cmd/cli/commands_upgrade.go | 4 +- cmd/cli/prog.go | 31 +++++- cmd/cli/stop_control_server_test.go | 150 ++++++++++++++++++++++++++++ 5 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 cmd/cli/stop_control_server_test.go diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index 58bb121..41e3999 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -2355,7 +2355,37 @@ func newSocketControlClient(ctx context.Context, s service.Service, dir string) // newSocketControlClientWithTimeout returns new control client after control server was started. // The timeoutDuration controls how long to wait for the server. +// +// Any answer to /started counts, including the 408 of a service whose start +// never finished: callers such as the deactivation pin check need to reach a +// running service whether or not it is ready. A caller that needs it ready uses +// newReadySocketControlClient. func newSocketControlClientWithTimeout(ctx context.Context, s service.Service, dir string, timeoutDuration time.Duration) *controlClient { + return dialSocketControlServer(ctx, s, dir, timeoutDuration, false) +} + +// newReadySocketControlClient is newSocketControlClient for a caller that needs +// the service ready, not just reachable: only a 200 from /started counts. +// +// A service whose start never finishes still runs its control server, and +// answers /started with 408 once its onStarted hooks have not completed in +// time. `ctrld upgrade` must read that as "the new binary did not become +// ready" and roll back, rather than report the upgrade successful. +func newReadySocketControlClient(ctx context.Context, s service.Service, dir string) *controlClient { + return newReadySocketControlClientWithTimeout(ctx, s, dir, dialSocketControlServerTimeout) +} + +// newReadySocketControlClientWithTimeout is newReadySocketControlClient with +// an explicit deadline. +func newReadySocketControlClientWithTimeout(ctx context.Context, s service.Service, dir string, timeoutDuration time.Duration) *controlClient { + return dialSocketControlServer(ctx, s, dir, timeoutDuration, true) +} + +// dialSocketControlServer polls /started until the control server answers, or +// with requireReady until it answers 200, and returns the client. It returns +// nil if the service is not running, or the answer does not come within +// timeoutDuration. +func dialSocketControlServer(ctx context.Context, s service.Service, dir string, timeoutDuration time.Duration, requireReady bool) *controlClient { // Return early if service is not running. if status, err := s.Status(); err != nil || status != service.StatusRunning { return nil @@ -2370,10 +2400,15 @@ func newSocketControlClientWithTimeout(ctx context.Context, s service.Service, d // The socket control server may not start yet, so attempt to ping // it until we got a response. for { - _, err := cc.post(startedPath, nil) + resp, err := cc.post(startedPath, nil) if err == nil { - // Server was started, stop pinging. - break + code := resp.StatusCode + resp.Body.Close() + if !requireReady || code == http.StatusOK { + // Server was started, stop pinging. + break + } + err = fmt.Errorf("control server reported the service not ready: HTTP %d", code) } // The socket control server is not ready yet, backoff for waiting it to be ready. bo.BackOff(ctx, err) diff --git a/cmd/cli/commands_service_restart.go b/cmd/cli/commands_service_restart.go index dac1550..d287657 100644 --- a/cmd/cli/commands_service_restart.go +++ b/cmd/cli/commands_service_restart.go @@ -92,14 +92,20 @@ func (sc *ServiceCommand) Restart(cmd *cobra.Command, args []string) error { if validateConfigErr != nil { timeout = 5 * time.Second } - if cc := newSocketControlClientWithTimeout(context.TODO(), s, dir, timeout); cc != nil { + // Only a ready service counts. Restart replaces nothing, so + // there is nothing to roll back, but a service that came + // back without finishing its start must not be reported as + // restarted cleanly. + if cc := newReadySocketControlClientWithTimeout(context.TODO(), s, dir, timeout); cc != nil { _, _ = cc.post(ifacePath, nil) logger.Debug().Msg("Control server ping successful") } else { - logger.Warn().Err(err).Msg("Service was restarted, but ctrld process may not be ready yet") + logger.Warn().Msgf("Service was restarted, but ctrld did not report ready within %s; check `ctrld status`", timeout) + return nil } } else { logger.Warn().Err(err).Msg("Service was restarted, but could not ping the control server") + return nil } logger.Notice().Msg("Service restarted") } else { diff --git a/cmd/cli/commands_upgrade.go b/cmd/cli/commands_upgrade.go index 19171df..067d9da 100644 --- a/cmd/cli/commands_upgrade.go +++ b/cmd/cli/commands_upgrade.go @@ -125,7 +125,9 @@ func (uc *UpgradeCommand) Upgrade(cmd *cobra.Command, args []string) error { } if doTasks(tasks) { if dir, err := socketDir(); err == nil { - if cc := newSocketControlClient(context.TODO(), s, dir); cc != nil { + // Only a ready service counts: a build that starts but + // never becomes ready must be rolled back. + if cc := newReadySocketControlClient(context.TODO(), s, dir); cc != nil { _, _ = cc.post(ifacePath, nil) return true } diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index a9e1e58..e2d59de 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -389,6 +389,7 @@ type prog struct { restoreOnce sync.Once restoreErr error releaseOnce sync.Once + csStopOnce sync.Once runDone chan struct{} runAbortCh chan struct{} listenerWg sync.WaitGroup @@ -1407,11 +1408,7 @@ func (p *prog) restoreOSState() error { // queries on would fail those queries. Safe to call multiple times. func (p *prog) releaseResources() { p.releaseOnce.Do(func() { - if p.cs != nil { - if cerr := p.cs.stop(); cerr != nil { - p.Warn().Err(cerr).Msg("could not stop control server") - } - } + p.stopControlServer() p.closeLogConn() p.mu.Lock() upstreams := p.cfg.Upstream @@ -1420,6 +1417,27 @@ func (p *prog) releaseResources() { }) } +// stopControlServer closes the control socket. Stop calls it as soon as the stop +// is accepted, and releaseResources again for the stop paths that never call +// Stop. Safe to call multiple times. +// +// It must not wait for the run to finish. A stopping process that still answers +// on the socket answers for the service that replaces it: `ctrld upgrade` and +// restart take any reply to /started as "the new binary is up", so an upgrade +// to a build that never became ready was reported successful instead of rolled +// back. Closing late also unlinks the socket path after the new process has +// bound its own there. +func (p *prog) stopControlServer() { + p.csStopOnce.Do(func() { + if p.cs == nil { + return + } + if err := p.cs.stop(); err != nil { + p.Warn().Err(err).Msg("could not stop control server") + } + }) +} + // shutdown runs the full teardown, in the order both halves require. It is // called once the listeners stopped, on every stop path including the mobile // one where the OS never terminates the process. @@ -1479,6 +1497,9 @@ func (p *prog) Stop(_ service.Service) error { os.Exit(deactivationPinInvalidExitCode) } } + // The pin check above is the last use of the control server. From here on + // this process is going away and must not answer for its successor. + p.stopControlServer() close(p.stopCh) return err } diff --git a/cmd/cli/stop_control_server_test.go b/cmd/cli/stop_control_server_test.go new file mode 100644 index 0000000..01816ff --- /dev/null +++ b/cmd/cli/stop_control_server_test.go @@ -0,0 +1,150 @@ +package cli + +import ( + "context" + "net/http" + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/kardianos/service" + "github.com/stretchr/testify/require" + + "github.com/Control-D-Inc/ctrld" +) + +// A stopping process must not answer /started for the service that replaces +// it. `ctrld upgrade` treats any reply as "the new binary is up", so a reply +// from the old process, while it waits for its components to stop, turned an +// upgrade to a never-ready build into "Upgrade successful" instead of a +// rollback (ctrld-qa upgrade-readiness-rollback on v1.0). +func TestStopClosesControlServerBeforeRunFinishes(t *testing.T) { + // A short path, as the other control server tests use: t.TempDir() is + // longer than a Unix socket path may be. + f, err := os.CreateTemp("", "") + require.NoError(t, err) + sock := f.Name() + f.Close() + t.Cleanup(func() { os.Remove(sock) }) + cs, err := newControlServer(sock) + require.NoError(t, err) + cs.register(startedPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) + require.NoError(t, cs.start()) + + p := &prog{ + cfg: &ctrld.Config{}, + cs: cs, + waitCh: make(chan struct{}), stopCh: make(chan struct{}), runDone: make(chan struct{}), + runAbortCh: make(chan struct{}), dnsWatcherStopCh: make(chan struct{}), + pinCodeValidCh: make(chan struct{}, 1), + } + p.logger.Store(mainLog.Load()) + p.pinCodeValidCh <- struct{}{} // accept the stop even if a deactivation pin is configured + + cc := newControlClient(sock) + resp, err := cc.post(startedPath, nil) + require.NoError(t, err, "the running service answers") + resp.Body.Close() + + require.NoError(t, p.Stop(nil)) + // runDone is still open: the run has not finished, as when a component is + // slow to stop. The socket must already be closed. + _, err = cc.post(startedPath, nil) + require.Error(t, err, "a stopped service answered /started") + + p.stopControlServer() // the later releaseResources call is harmless +} + +// startTestControlServer starts a control server on a short socket path in a +// directory of its own, as the socket client expects, answering /started with +// whatever started returns. +func startTestControlServer(t *testing.T, started func() int) (dir, sock string, cs *controlServer) { + t.Helper() + // A short path, as the other control server tests use: t.TempDir() is + // longer than a Unix socket path may be. + dir, err := os.MkdirTemp("", "cs") + require.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(dir) }) + sock = filepath.Join(dir, ctrldControlUnixSock) + cs, err = newControlServer(sock) + require.NoError(t, err) + cs.register(startedPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(started()) })) + cs.register(listClientsPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) + require.NoError(t, cs.start()) + t.Cleanup(func() { _ = cs.stop() }) + // start serves on a goroutine, and the server tracks the listener only + // once that runs: a Shutdown before then leaves the listener, and its + // unlink, to Serve's own deferred close. Wait until it answers, as a + // service that has been running does. A path other than /started keeps + // the started func's call count to the test's own requests. + require.Eventually(t, func() bool { + resp, err := newControlClient(sock).post(listClientsPath, nil) + if err != nil { + return false + } + resp.Body.Close() + return true + }, 5*time.Second, 10*time.Millisecond, "control server did not start serving") + return dir, sock, cs +} + +// Stop unlinks the socket path as it closes the socket, before the successor +// starts: unlinking it later removed the path the successor had bound. +func TestStopRemovesControlSocketPath(t *testing.T) { + _, sock, cs := startTestControlServer(t, func() int { return http.StatusOK }) + p := &prog{ + cfg: &ctrld.Config{}, + cs: cs, + waitCh: make(chan struct{}), stopCh: make(chan struct{}), runDone: make(chan struct{}), + runAbortCh: make(chan struct{}), dnsWatcherStopCh: make(chan struct{}), + pinCodeValidCh: make(chan struct{}, 1), + } + p.logger.Store(mainLog.Load()) + p.pinCodeValidCh <- struct{}{} + + require.NoError(t, p.Stop(nil)) + _, err := os.Stat(sock) + require.True(t, os.IsNotExist(err), "socket path still present after Stop: %v", err) +} + +// A stop path that never calls Stop still closes the control server. +func TestReleaseResourcesClosesControlServer(t *testing.T) { + _, sock, cs := startTestControlServer(t, func() int { return http.StatusOK }) + p := &prog{cfg: &ctrld.Config{}, cs: cs} + p.logger.Store(mainLog.Load()) + + p.releaseResources() + _, err := newControlClient(sock).post(startedPath, nil) + require.Error(t, err, "the control server answered after releaseResources") +} + +// `ctrld upgrade` must not count a successor that started but never became +// ready: it answers /started with 408, and only a 200 means ready. The plain +// socket client keeps accepting any answer, because the deactivation pin check +// must reach a running service whether or not it is ready. +func TestReadySocketControlClientRequiresReady(t *testing.T) { + running := &fakeService{statuses: []service.Status{service.StatusRunning}} + + t.Run("never ready", func(t *testing.T) { + dir, _, _ := startTestControlServer(t, func() int { return http.StatusRequestTimeout }) + require.Nil(t, newReadySocketControlClientWithTimeout(context.Background(), running, dir, 2*time.Second), + "a service answering 408 counted as ready") + require.NotNil(t, newSocketControlClientWithTimeout(context.Background(), running, dir, 2*time.Second), + "the plain client must still reach a running service that is not ready") + }) + + t.Run("becomes ready", func(t *testing.T) { + var calls atomic.Int32 + dir, _, _ := startTestControlServer(t, func() int { + if calls.Add(1) == 1 { + return http.StatusRequestTimeout + } + return http.StatusOK + }) + require.NotNil(t, dialSocketControlServer(context.Background(), running, dir, 10*time.Second, true), + "the poll gave up on a service that became ready") + require.GreaterOrEqual(t, calls.Load(), int32(2), "the poll stopped at the 408") + }) +}