fix: [sc-106109] Wait for actual process exit before replacing or deleting agent files (#100)

Install, update and uninstall waited for the old agent process to exit by sleeping
a fixed 5 seconds and then acting regardless: runUpdate overwrote the agent
executable in place, runUninstall deleted the installation directory. On a loaded
endpoint the old process is frequently still alive at the 5 second mark - its
shutdown legitimately drains in-flight commands, tears down MQTT and kills its
plugin subprocesses one at a time - and Windows refuses to replace a running
image, so the update failed with a sharing violation after the service was
already stopped, leaving the device offline with nothing to retry. Uninstall had
the mirror-image problem: files removed out from under a live process, leaving an
installation that neither ran nor reinstalled.

The four sleeps are replaced with a wait on real exit signals, all of which must
clear: the service manager no longer reporting the service active, no process
still executing the agent binary, and the executable no longer held open (a
sharing violation on Windows, ETXTBSY on Linux). The three overlap on purpose -
the file signal is what actually blocks the write on Windows, but macOS permits
writing to a running image, and a service manager can report a service stopped
while its process is still winding down. The wait returns on the first round of
probes when the process is already gone, so a healthy update is faster than the
unconditional sleep it replaces, and is bounded by a documented 2 minute deadline
sized for a slow but legitimate shutdown. An elapsed poll interval is never by
itself treated as evidence of an exit; a probe that cannot run at all is logged
once at Warn and the remaining signals are used.

Overrunning the deadline aborts before anything is written or deleted, logging
what was still outstanding and for how long. Update then restarts the service it
stopped - on any failure after the stop, not just this one - so a failed update no
longer leaves an endpoint silently offline; install does the same; uninstall logs
plainly that nothing was removed. The agent executable and config file are now
written to a temp file in the destination directory and atomically renamed into
place, mirroring the postback spool, so a failed or interrupted write leaves the
previous file byte-identical instead of truncated. The re-registration path's
sleep is replaced by polling until the deleted registration is actually reaped.

exitTimeoutOverrideStr is overridable via -ldflags (25s in the integration build)
so the deadline case can be observed in seconds, mirroring stopTimeoutOverrideStr.

Author: mlataza

CLAUDE.md

@@ -51,6 +51,18 @@ Required tools:
   - Service mode: `--config-file --log-file --org-id` 
   - Uninstall mode: `--uninstall --org-id`
 
+  The install, update and uninstall paths never assume the old agent process has
+  exited: after stopping the service they wait (bounded, 2 minutes, documented)
+  for real exit signals — the service manager no longer reporting the service
+  active, no process still executing the agent binary, and the executable no
+  longer held open — and return as soon as the process is gone. Overrunning the
+  deadline aborts before writing or deleting anything, leaves the installation
+  intact, and restarts the service that was stopped so a failed update cannot
+  leave an endpoint offline. The agent executable and config file are written to a
+  temp file and atomically renamed into place, so a failed write leaves the
+  previous file byte-identical. See the README's "Waiting for the Old Agent
+  Process to Exit" section.
+
 - **internal/agent/**: Device configuration, installation paths, and OS-specific host information
 - **internal/interpreter/**: Command execution engine supporting both PowerShell and Bash interpreters  
 - **internal/mqtt/**: Azure IoT Hub MQTT client implementation with auto-reconnection

README.md

@@ -448,6 +448,53 @@ Recovery is the ordinary one: end the wedged agent process, start the service, a
 re-run the update or uninstall. Linux and macOS are unaffected — their service
 implementations do not use this polling loop.
 
+### Waiting for the Old Agent Process to Exit
+
+Stopping the service is not the same as the old agent process being gone. Install,
+update and uninstall used to bridge that gap with a fixed `time.Sleep` of five
+seconds and then act regardless — overwriting the agent executable, or deleting
+the installation directory. On a loaded endpoint the old process is frequently
+still alive at the five second mark, because its shutdown legitimately drains the
+commands in flight, tears down MQTT and kills its plugin subprocesses one at a
+time. Windows then refuses to replace a running image, so the update failed with
+a sharing violation *after* the service was already stopped, leaving the device
+offline until someone intervened. Uninstall had the mirror-image problem: files
+deleted out from under a live process, leaving an installation that neither ran
+nor reinstalled.
+
+Nothing is written or deleted now until the process is observed to be gone:
+
+- The wait polls three **real signals**, all of which must clear: the service
+  manager no longer reports the service active, no running process is executing
+  the agent binary, and the executable is no longer held open as a running image
+  (a sharing violation on Windows, `ETXTBSY` on Linux). The three overlap on
+  purpose — the file signal is what actually blocks the write on Windows, but
+  macOS permits writing to a running image, and a service manager can report a
+  service stopped while its process is still winding down. An elapsed poll
+  interval is never by itself treated as evidence of anything.
+- It **returns as soon as the process is gone**, so a healthy update is faster
+  than the unconditional five second sleep it replaces, not slower.
+- It is bounded by a documented **2 minute** deadline, sized for a slow but
+  legitimate shutdown (many workers, long-running commands, several plugin
+  subprocesses). Overrunning it logs at `Error` what was still outstanding and for
+  how long, and the caller aborts rather than proceeding.
+- **Update** aborts before writing anything and leaves the installation fully
+  intact, then **restarts the service it stopped** so a failed update never leaves
+  the endpoint silently offline. **Uninstall** aborts before deleting the
+  registration or any files, and logs that nothing was removed. **Install**
+  (`--config`) aborts before deleting the existing registration and restarts the
+  service it stopped.
+- A probe that cannot run at all (a restrictive ACL, a process table that cannot
+  be enumerated) is logged once at `Warn` and the remaining signals are used. A
+  probe failure is not evidence a process is alive, and must not wedge every
+  update on an endpoint where it can never succeed.
+
+The agent executable and the config file are also written **atomically** — to a
+temporary file in the destination directory, then renamed into place, the same
+pattern the postback spool uses. An interrupted or failed write therefore leaves
+the previous file byte-identical rather than truncated: the endpoint keeps running
+the old agent instead of a binary that cannot start.
+
 ### Notification Plugin Supervision
 
 Notification plugins run as separate subprocesses reached over RPC, and every

cmd/agent_smith/config.go

@@ -9,7 +9,6 @@ import (
 	"net/http"
 	"os"
 	"runtime"
-	"time"
 
 	"github.com/RewstApp/agent-smith-go/internal/agent"
 	"github.com/RewstApp/agent-smith-go/internal/service"
@@ -153,16 +152,20 @@ func runConfig(params *configContext) error {
 	// Got configuration
 	logger.Info("Received configuration", "configuration", string(configBytes))
 
-	err = params.FS.WriteFile(configFilePath, configBytes, utils.DefaultFileMod)
+	// Written atomically so a failed write cannot leave a truncated config file
+	// that the service is then unable to start from.
+	err = writeFileAtomic(params.FS, configFilePath, configBytes, utils.DefaultFileMod)
 	if err != nil {
 		return fmt.Errorf("failed to save config: %w", err)
 	}
 
 	name := agent.GetServiceName(params.OrgId)
+	agentExecutablePath := agent.GetAgentExecutablePath(params.OrgId)
 
 	// Stop and delete the service if it already exists
 	existingService, err := params.ServiceManager.Open(name)
 	if err == nil {
+		stopped := false
 		if existingService.IsActive() {
 			logger.Info("Stopping service", "service", name)
 			// Abort before deleting the registration or overwriting the
@@ -179,6 +182,56 @@ func runConfig(params *configContext) error {
 				}
 				return fmt.Errorf("failed to stop service %s: %w", name, stopErr)
 			}
+			stopped = true
+		}
+
+		// Wait for the old process to actually exit before the registration is
+		// deleted and the executable is replaced. A process that is still running
+		// holds its own image open, so proceeding here is what fails the install
+		// with a sharing violation on Windows.
+		logger.Info(
+			"Waiting for the agent process to exit",
+			"service", name,
+			"agent_executable", agentExecutablePath,
+		)
+		if waitErr := waitForAgentProcessExit(
+			logger,
+			existingService,
+			params.FS,
+			agentExecutablePath,
+			params.exitWait,
+		); waitErr != nil {
+			// The config file was already refreshed above, so say so rather than
+			// claiming nothing changed: the installed agent and its registration are
+			// what had to be left alone while the old process is alive.
+			logger.Error(
+				"Install aborted; the existing agent and its service registration were left untouched",
+				"service",
+				name,
+				"agent_executable",
+				"not modified",
+				"service_registration",
+				"intact",
+				"config_file",
+				"updated",
+				"error",
+				waitErr,
+			)
+			// Put the endpoint back the way it was found rather than leaving a
+			// stopped service behind.
+			if stopped {
+				if startErr := existingService.Start(); startErr != nil {
+					logger.Error(
+						"Failed to restart service after aborted install; the endpoint is offline",
+						"service", name,
+						"error", startErr,
+					)
+				}
+			}
+			if closeErr := existingService.Close(); closeErr != nil {
+				logger.Error("Failed to close service handle", "service", name, "error", closeErr)
+			}
+			return fmt.Errorf("failed to wait for agent process to exit: %w", waitErr)
 		}
 
 		// Delete the service
@@ -188,13 +241,26 @@ func runConfig(params *configContext) error {
 		}
 		logger.Info("Service deleted", "service", name)
 
-		// Wait for some time for the service executable to clean up
 		err = existingService.Close()
 		if err != nil {
 			return fmt.Errorf("failed to close service %s: %w", name, err)
 		}
-		logger.Info("Waiting for service executable to stop")
-		time.Sleep(serviceExecutableTimeout)
+
+		// Wait for the deleted registration to be reaped so the name is free to
+		// register again. A registration that outlives the deadline is reported and
+		// creation is attempted anyway, which surfaces the real conflict.
+		logger.Info("Waiting for the service registration to be removed", "service", name)
+		if deregErr := waitForServiceDeregistration(
+			params.ServiceManager,
+			name,
+			params.exitWait,
+		); deregErr != nil {
+			logger.Error(
+				"Service registration outlived its deletion; registering anyway",
+				"service", name,
+				"error", deregErr,
+			)
+		}
 	}
 
 	logger.Info("Configuration saved to", "path", configFilePath)
@@ -218,8 +284,14 @@ func runConfig(params *configContext) error {
 		return fmt.Errorf("failed to read executable file: %w", err)
 	}
 
-	agentExecutablePath := agent.GetAgentExecutablePath(params.OrgId)
-	err = params.FS.WriteFile(agentExecutablePath, execFileBytes, utils.DefaultExecutableFileMod)
+	// Written atomically so a failure here leaves any previously installed binary
+	// byte-identical instead of truncated.
+	err = writeFileAtomic(
+		params.FS,
+		agentExecutablePath,
+		execFileBytes,
+		utils.DefaultExecutableFileMod,
+	)
 	if err != nil {
 		return fmt.Errorf("failed to create agent executable: %w", err)
 	}

cmd/agent_smith/config_context.go

@@ -185,6 +185,11 @@ type configContext struct {
 	FS             utils.FileSystem
 	ServiceManager service.ServiceManager
 	HTTPClient     *http.Client
+
+	// exitWait holds the test seams for the bounded wait for the old agent
+	// process to exit. Zero values select the documented defaults, so nothing
+	// outside tests ever sets it.
+	exitWait exitWaitOptions
 }
 
 // newConfigFlagSet builds the flag set for config mode, binding flags to the

cmd/agent_smith/config_test.go

@@ -74,6 +74,7 @@ func newBaseConfigParams(configURL string) *configContext {
 		Domain:         &mockDomainInfoProvider{},
 		FS:             newConfigTestFS(),
 		ServiceManager: newConfigTestServiceManager(),
+		exitWait:       stubExitWait(),
 	}
 }
 
@@ -273,6 +274,55 @@ func TestRunConfig_ExistingService_StopFails(t *testing.T) {
 	}
 }
 
+// A pre-existing agent whose process never exits must abort the install before
+// the registration is deleted or the executable replaced, and the service it
+// stopped must be brought back up.
+func TestRunConfig_ExistingService_ProcessNeverExits(t *testing.T) {
+	srv := newConfigServer(t, http.StatusOK, validConfigResponseBody("test-org"))
+	defer srv.Close()
+
+	clock := newFakeClock()
+	params := newBaseConfigParams(srv.URL)
+	params.exitWait = clock.options(2*time.Minute, 250*time.Millisecond)
+	var renames [][2]string
+	params.FS = &mockFileSystem{
+		executableFunc: func() (string, error) { return "/fake/agent", nil },
+		readFileFunc:   func(string) ([]byte, error) { return []byte("binary"), nil },
+		writeFileFunc:  func(string, []byte, os.FileMode) error { return nil },
+		mkdirAllFunc:   func(string) error { return nil },
+		removeAllFunc:  func(string) error { return nil },
+		renameFunc: func(oldPath string, newPath string) error {
+			renames = append(renames, [2]string{oldPath, newPath})
+			return nil
+		},
+		// The old process holds its image open for as long as it lives.
+		executableInUseFunc: func(string) (bool, error) { return true, nil },
+	}
+	existing := &mockService{isActive: true}
+	params.ServiceManager = &mockServiceManager{
+		openService:   existing,
+		createService: &mockService{},
+	}
+
+	err := runConfig(params)
+
+	if err == nil || !strings.Contains(err.Error(), "failed to wait for agent process to exit") {
+		t.Errorf("expected the install to abort on the exit wait, got %v", err)
+	}
+	if existing.deleteCalled {
+		t.Error("expected the existing service registration to be left alone")
+	}
+	if !existing.startCalled {
+		t.Error("expected the existing service to be restarted rather than left stopped")
+	}
+	agentExecutablePath := agent.GetAgentExecutablePath("test-org")
+	for _, rename := range renames {
+		if rename[1] == agentExecutablePath {
+			t.Error("expected the agent executable never to be replaced while the process is alive")
+		}
+	}
+}
+
 func TestRunConfig_ExistingService_DeleteFails(t *testing.T) {
 	srv := newConfigServer(t, http.StatusOK, validConfigResponseBody("test-org"))
 	defer srv.Close()

cmd/agent_smith/main_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"os"
 
 	"github.com/RewstApp/agent-smith-go/internal/service"
@@ -68,11 +69,14 @@ func (mock *mockDomainInfoProvider) EntraDomain(context.Context) (*string, error
 }
 
 type mockFileSystem struct {
-	executableFunc func() (string, error)
-	readFileFunc   func(name string) ([]byte, error)
-	writeFileFunc  func(name string, data []byte, perm os.FileMode) error
-	mkdirAllFunc   func(path string) error
-	removeAllFunc  func(path string) error
+	executableFunc      func() (string, error)
+	readFileFunc        func(name string) ([]byte, error)
+	writeFileFunc       func(name string, data []byte, perm os.FileMode) error
+	mkdirAllFunc        func(path string) error
+	removeAllFunc       func(path string) error
+	renameFunc          func(oldPath string, newPath string) error
+	removeFunc          func(name string) error
+	executableInUseFunc func(name string) (bool, error)
 }
 
 func (m *mockFileSystem) Executable() (string, error) {
@@ -95,21 +99,59 @@ func (m *mockFileSystem) RemoveAll(path string) error {
 	return m.removeAllFunc(path)
 }
 
+// Rename, Remove and ExecutableInUse default to the behaviour of a filesystem
+// where the write commits and no process holds the executable, so tests only
+// override them when the case under test is about those.
+func (m *mockFileSystem) Rename(oldPath string, newPath string) error {
+	if m.renameFunc == nil {
+		return nil
+	}
+	return m.renameFunc(oldPath, newPath)
+}
+
+func (m *mockFileSystem) Remove(name string) error {
+	if m.removeFunc == nil {
+		return nil
+	}
+	return m.removeFunc(name)
+}
+
+func (m *mockFileSystem) ExecutableInUse(name string) (bool, error) {
+	if m.executableInUseFunc == nil {
+		return false, nil
+	}
+	return m.executableInUseFunc(name)
+}
+
 type mockService struct {
 	isActive  bool
 	stopErr   error
 	deleteErr error
 	startErr  error
+	// isActiveFunc overrides isActive when a test needs the reported state to
+	// change from one observation to the next, as a service that is still
+	// shutting down does.
+	isActiveFunc func() bool
 
 	stopCalled   bool
 	deleteCalled bool
 	startCalled  bool
 }
 
-func (m *mockService) IsActive() bool { return m.isActive }
+func (m *mockService) IsActive() bool {
+	if m.isActiveFunc != nil {
+		return m.isActiveFunc()
+	}
+	return m.isActive
+}
 
+// Stop mirrors a real service manager: a stop that reports success means the
+// service reached Stopped, so it no longer reports itself active.
 func (m *mockService) Stop() error {
 	m.stopCalled = true
+	if m.stopErr == nil {
+		m.isActive = false
+	}
 	return m.stopErr
 }
 
@@ -131,10 +173,18 @@ type mockServiceManager struct {
 	createErr     error
 	createService service.Service
 
+	openCalls   int
 	createCalls []service.AgentParams
 }
 
 func (m *mockServiceManager) Open(name string) (service.Service, error) {
+	m.openCalls++
+	if svc, ok := m.openService.(*mockService); ok && svc.deleteCalled {
+		// A deleted registration is no longer visible to the manager. That is the
+		// signal the deregistration wait polls for, so the mock has to reproduce it
+		// or the wait would have nothing to observe.
+		return nil, errors.New("service does not exist")
+	}
 	return m.openService, m.openErr
 }