fix: [sc-106108] Bound the Windows service stop wait so a wedged service cannot hang an update

[mlataza]

Problem

windowsService.Stop() polled the service state every 250 ms in an unbounded loop — no deadline, no iteration cap, no error path for "took too long":

for {
    if status.State == svc.Stopped { return nil }
    time.Sleep(pollingInterval)
    status, err = winSvc.handle.Query()
    if err != nil { return err }
}

Every caller of Stop() is a blocking, user- or updater-initiated operation: runUpdate, runConfig, and runUninstall. So a service that never reaches Stopped — a wedged agent process, or Windows itself holding the service in StopPending behind a stuck operation — hung the caller forever.

The auto-updater runs unattended every 48 hours, which is what makes this customer-visible. If it stops a service that then fails to reach Stopped, the installer blocks before it logs anything further: the endpoint drops offline, the new binary is never written (update.go reaches the executable copy only downstream of the hung call), and the customer sees a device that went offline during a routine update with no error anywhere. Recovery needs hands on the endpoint. An operator running an interactive uninstall just sees the command hang with no output, and killing it can leave a partially removed installation.

Latent in all released versions through v1.5.0 and current main. Windows only — the Unix service implementations do not use this polling loop. No customer reports isolated to this cause; found via a reliability audit.

Fix

Stop() now waits at most serviceStopTimeout (5 minutes) and otherwise returns an error naming the service, the deadline, and the last observed state:

service RewstRemoteAgent_<org> did not stop within 5m0s: last observed state StopPending

The bound is deliberately generous, and the constant's doc comment says why: it exists to catch a wedge, not to race a normal shutdown. A healthy agent finishes teardown in seconds, but it first drains whatever commands are in flight, and an operator can set command_timeout_seconds high enough that a single command legitimately runs for minutes. Five minutes sits far above any healthy shutdown while still failing an unattended update the same day it runs.

The loop distinguishes "still stopping" from "will not stop". StopPending keeps being polled to the deadline; a state the service cannot reach Stopped from returns immediately rather than burning the full deadline. Running and StartPending count as still-stopping, because ControlService reports the status the service last published and can still show the pre-stop state before the service thread picks the control up — rejecting those would break healthy stops.

Callers

All three already returned on a Stop() error, so the fix there is about what they say and one real bug:

• runUpdate aborts before any write and logs that the executable and config were not modified. This matters beyond tidiness: the old process may still hold the executable open, so overwriting it would either fail or leave a half-updated install. Leaving everything in place keeps the endpoint recoverable.
• runUninstall aborts before deleting the registration or any files, and logs Uninstall aborted; nothing was removed with service_registration=intact installed_files=intact.
• runConfig had a latent bug of exactly the kind this ticket is about. It reassigned err with Close()'s result before wrapping it, so a stop failure returned an error formatted from a nil wrapped error and the actual reason vanished. Close errors are now logged separately and the stop error is returned intact.

Testing

Unit tests use a fake clock that advances only when the code under test sleeps, so bounded waits are deterministic and instant: a prompt stop (unchanged behavior), a service stuck in StopPending past the deadline, one that reaches Stopped on the last poll before the deadline (no spurious error), Query failing mid-poll, an unexpected terminal state failing with zero polls, Running being polled rather than rejected, and the timeout resolution order. Caller tests assert runUpdate writes no files and neither starts nor deletes the service after a failed stop, and that runUninstall removes no directories and leaves the registration.

golangci-lint run 0 issues. Coverage 88.4% locally (darwin, so the Windows-only lane is the one that counts — the PR's own coverage job covers it).

Integration test — a new Windows-only scenario, and the part worth a careful look.

The wedge is a purpose-built fixture, test/wedgedservice: a service that accepts the stop control, returns from its handler, reports StopPending, and never stops until a release file appears. It has to be purpose-built. Killing the agent makes the SCM report Stopped almost at once, which is the happy path; suspending its threads makes the SCM fail the control request outright rather than exercising the wait. Neither reaches the code under test. Which wedge it is does not matter — the ticket's own framing is that any wedge will do, because what is under test is the bound on the wait and not how the agent got stuck.

.github/actions/wedged-service registers the fixture under the agent's own service name by swapping that service's ImagePath, so the installed executable and config are untouched and can be hashed before and after. The swap goes through the registry rather than sc.exe, because the agent's command line carries quoted paths and pushing those back through sc.exe's keyword= value parsing is a needless way to corrupt the registration the scenario has to restore. mode=restore reads the value back afterwards — a silently failed restore would leave the fixture reaching Running, which no later step could tell apart from a recovered agent.

The invoker is the integration binary, whose stop deadline is overridden to 25s by a new ldflags-injected stopTimeoutOverrideStr, mirroring the existing sasTokenLifetimeOverrideStr. Each wedged stop then costs seconds instead of five minutes; the production default is unchanged and covered by unit tests (resolution order is per-instance seam → override → constant, and a garbage or non-positive override falls back rather than disabling the bound).

Measured on windows-latest (run 31451274395, green first attempt, ~95s for the whole scenario):

assertion	observed
--update aborts	25.1s, did not stop within 25s: last observed state StopPending
installation untouched	exe 5AC33FB4… and config 4B7354CA… unchanged
--uninstall aborts	25.1s, nothing was removed, registration + all 3 directories intact
recovery	ImagePath restored, resubscribed in ~2s
post-recovery update	Agent installed to + Service started — the bound only catches a wedge

The StopPending result is the one I could not predict from a macOS host: it confirms Query() observes StopPending, so the polling wait is what ran, not the fast-fail branch.

Docs

README gains a "Bounded Windows Service Stop (Windows only)" section covering the failure mode, the deadline and why it is sized as it is, the still-stopping vs will-not-stop distinction, what each caller does on abort, and the recovery procedure. CLAUDE.md's internal/service entry points at it.

Reviewer notes

• The re-wedge step in the middle of the scenario is load-bearing, and it exposes an adjacent gap. After a failed stop the service sits in StopPending, and IsActive() treats only Running as active — so an uninstall issued at that point skips the stop entirely and would delete files out from under a live process. The scenario starts a fresh wedged run so the uninstall meets a Running service, as an operator would. Fixing the underlying gap needs a state accessor on the cross-platform Service interface (and IsActive's current meaning is the right one for diagnostic mode), so I left it out of scope. Worth its own ticket.
• An aborted --update still exits 0. The abort is reported through the log, not the exit status, so an unattended caller can only tell from the log. Not in the ACs and not changed here, but it is the kind of thing that would make the updater's own error handling possible later.
• The 5-minute default is not exercised on a runner. Doing so would mean five idle minutes per wedged stop, twice. The constant is unit-tested and the integration lane proves the mechanism at 25s.
• The runConfig error-wrapping fix is technically beyond the ticket's ACs, but it is the same class of defect (the reason a stop failed becoming invisible) on the third caller, and QA step 7 covers that path.
• This ran on Windows only. The scenario steps are all if: matrix.os == 'windows-latest', and the ubuntu/macOS lanes are untouched by the agent-side change, but I have not confirmed a green all run if you would like one before merge.
• test/wedgedservice sits behind the integration build tag so its statements stay out of go test ./... and cannot count against the coverage threshold; it carries a non-Windows stub file so go build -tags integration ./... still works on Linux and macOS, where the package would otherwise have no buildable files.

🤖 Generated with Claude Code