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

[mlataza]

Problem

Install, update and uninstall all waited for the old agent process to exit by sleeping a fixed five seconds and then acting regardless:

// Wait for some time for the service executable to clean up
logger.Info("Waiting for service executable to stop")
time.Sleep(serviceExecutableTimeout) // 5s

Nothing checked whether the process had exited. After the sleep, runUpdate overwrote the agent executable in place and runUninstall deleted the installation directory.

On a loaded endpoint the old process is frequently still alive at the five second mark — its shutdown legitimately drains the commands in flight, tears down MQTT, and kills its plugin subprocesses one at a time, each waiting out go-plugin's graceful-exit grace period. Windows will not replace a running image, so the write failed with a sharing violation after the service was already stopped, and the updater returned. The customer sees devices silently dropping offline after a release, disproportionately their busiest servers, with nothing retrying. Because it is timing- and host-dependent it looks intermittent and reads as a network problem. Uninstall had the mirror-image failure: files removed out from under a live process, leaving an installation that neither ran nor reinstalled.

This is a fixed-deadline race, not a rare one — the same slow hosts fail every time. Latent in all released versions through v1.5.0 and current main. Hard failure on Windows (mandatory file locking); a subtler race on Linux and macOS.

Fix

The four sleeps (update.go ×2, config.go, uninstall.go) are replaced with a wait on signals that are actual observations of the process. Three of them, all of which must clear:

signal	what it observes
service manager	no longer reports the service active
process table	no process is executing the agent binary
executable	no longer held open as a running image

The ACs name the first and third. The process scan is there because the file probe is a no-op on macOS — I verified on darwin that opening a running executable for writing succeeds, where Linux returns ETXTBSY and Windows a sharing violation. Without the scan, macOS would fall back to the service-manager signal alone, and a service manager can report a service stopped while its process is still winding down. The three overlap deliberately: the file signal is what actually blocks the write on Windows, the scan is what covers macOS and the wind-down window everywhere.

An elapsed poll interval is never by itself treated as evidence of an exit.

• Fast path costs nothing. The wait probes before it ever sleeps, so a healthy endpoint pays one round of probes — the update gets faster than the unconditional five second sleep, not slower.

• Bounded at 2 minutes, and the constant's doc comment says why: it exists to catch a process that never exits, not to race a slow one. Sized for many workers, in-flight commands, and several plugin subprocesses. Overrun logs at Error what was still outstanding and for how long:

  agent process did not exit within 2m0s: the service manager still reports the
  service active; the agent executable C:\...\agent_smith.win.exe is still held open
  

• A probe that cannot run is not evidence. A restrictive ACL or an unenumerable process table is logged once at Warn and the remaining signals are used, rather than wedging every update on an endpoint where that probe can never succeed.

Callers

• runUpdate aborts before writing anything and leaves the installation fully intact. It now also restarts the service it stopped — on any failure after the stop, via a deferred recovery, not just this one — so a failed update no longer leaves the endpoint silently offline. If the failure comes after the registration was deleted (the --service-username path) there is nothing to start, and it says so plainly instead of failing quietly.
• runUninstall waits before deleting the registration or any files, and on overrun logs Uninstall aborted; nothing was removed. The delete/wait order is swapped so the service handle still exists to be observed.
• runConfig waits before deleting the existing registration and replacing the executable, and restarts the service it stopped. Its abort log is honest that the config file was already refreshed at that point — the installed agent and its registration are what had to be left alone.

Non-destructive writes

The agent executable and the config file are written to a temp file in the destination directory and atomically renamed into place, mirroring postback_spool.go:109-119. A failed or interrupted write leaves the previous file byte-identical rather than truncated: the endpoint keeps running the old agent instead of a binary that cannot start. FileSystem gains Rename, Remove and ExecutableInUse for this.

The re-registration path's second sleep is replaced by polling until the deleted registration is actually reaped. If it outlives its deadline the failure is logged and Create is attempted anyway — aborting there would leave the endpoint with no registration at all, and Create surfaces the real conflict if there is one.

Testing

Unit tests use a fake clock that advances only when the code under test sleeps, so a two minute deadline is burned instantly and deterministically. Covering exactly the ACs' four cases and then some: a process already gone (asserts zero waiting and a single probe), one that exits partway through, one that never exits (bounded, descriptive error, no destructive write), a failed commit leaving the original byte-identical, plus each probe failing independently, the nil-service handle case, deregistration, and the timeout resolution order. Caller-level tests assert runUpdate writes nothing and restarts the service on overrun, that the executable is only ever committed by renaming *.new into place, and that runUninstall starts removing only after the process is observed gone.

ProcessRunningFromExecutable is tested for real against a re-executed child process — it has to be able to say both "running" and "gone", since a scan stuck on either answer would wedge every update or be the fixed sleep again.

go test ./... green, golangci-lint run 0 issues, GOOS=windows|linux go vet clean, coverage 88.6% (threshold 80).

Integration — run 31501216206 on the head commit, os=all. Every job green across Windows, Linux and macOS: build, build-integration-test, test, and test-service-user — including the Windows wedged-service abort scenarios and the --service-username re-registration path that uses the new deregistration wait. (The set-matrix job's status is stuck in_progress in the API with all of its own steps completed successfully; every job downstream of it consumed its output and passed.)

I did not add a new integration scenario for the deadline case. Reproducing "service reports Stopped but the process lingers holding the image" needs a fixture distinct from test/wedgedservice (which never reports Stopped) plus a new composite action. The ACs call for unit tests; the QA steps call for running the existing workflows, which pass unchanged. Happy to build it out if you want it before merge.

exitTimeoutOverrideStr is overridable via -ldflags and set to 25s in the integration build, mirroring the existing stopTimeoutOverrideStr, so QA step 4/7 ("force the deadline case") costs seconds rather than two minutes. A garbage or non-positive override falls back to the constant rather than disabling the bound.

Docs

README gains "Waiting for the Old Agent Process to Exit" — the failure mode, the three signals and why three, the deadline and its sizing, what each caller does on abort, and the atomic-write guarantee. CLAUDE.md's cmd/agent_smith entry points at it.

Reviewer notes

• The macOS finding is the one worth checking. I tested it directly rather than assuming ETXTBSY is portable: a running binary on darwin opens for writing without error. If that matches your experience, the process scan is load-bearing on that platform; if not, it is redundant defence and still cheap.
• gopsutil is a new dependency for this path but already a direct dependency of the module (internal/agent host info), so no go.mod change. The scan runs once per 250ms poll; on Windows enumerating processes costs ~100-300ms, which is fine for an operation that already stops a service.
• The scan excludes the caller's own PID, so an operator running the installed binary in place is not mistaken for the service it is updating. That case still cannot succeed on Windows (the file is held by us), but it now fails with a clear wait error rather than a raw sharing violation.
• agent.NewPathsData moved above the stop in runUpdate so the wait knows which executable to watch. It only reads paths and host tags; no behavioural coupling to the service.
• runUpdate still exits 0 on an aborted update — same as before this change, the abort is reported through the log. Called out again because the new restart makes the endpoint recover, so the exit status is now the only thing that still misreports.
• The 2 minute default is not exercised on a runner, for the same reason the 5 minute stop deadline is not: it is unit-tested, and the ldflags override exists for the lane.

🤖 Generated with Claude Code