Fix Update.AutoApply crash-looping forever on Linux

CHANGELOG.md

@@ -3,6 +3,17 @@
 Notable changes to NwsAlertBot, most recent first. For setup and usage, see
 [README.md](README.md); for architecture and internals, see [docs/TECHNICAL.md](docs/TECHNICAL.md).
 
+- **Fix: `Update.AutoApply` could crash-loop forever on Linux, never actually applying the
+  update.** `setup-service.ps1`'s generated systemd unit now sets `KillMode=process`. Previously,
+  the default `KillMode=control-group` meant that when `UpdateCheckService` spawned `update.ps1`
+  as a child process and then stopped the app so it could swap the binary, systemd killed the
+  *entire cgroup* — including the just-spawned `update.ps1` — before it ever ran. `Restart=always`
+  then brought the old binary straight back up, which immediately re-detected the same newer
+  release and repeated, forever, ~10-13s apart, never once reaching the point of actually
+  fetching alerts. Confirmed live on a production instance. Existing installations must add
+  `KillMode=process` to their unit file by hand (or re-run `setup-service.ps1`) — this only
+  changes the generated template, not units already written to disk. See docs/TECHNICAL.md
+  "Known Limitations" (under Auto-Update).
 - **Fix: `Map.Enabled: false` (the shipped default) silently disabled free IEM map images too,
   not just Mapbox.** `MapService.GetMapUrlAsync` gated its entire body — including both
   no-account-needed IEM autoplot paths (#208 for VTEC alerts, #217 for SPS) — behind

CLAUDE.md

@@ -213,7 +213,14 @@ bypassing it skips CI entirely — a PR is what actually runs `dotnet test` befo
 - **`scripts/update.ps1` must never overwrite `appsettings.json`/`appsettings.Local.json` or any runtime state file** — it only replaces the executable and itself. It's tempting to simplify the script by extracting the whole downloaded release archive over the install directory, but that archive's `appsettings.json` is a blank template — doing so would silently destroy the user's real configuration. If you touch the "what gets replaced" logic in `update.ps1`, keep this constraint.
 - **A Windows Service wrapping this exe needs both `.UseWindowsService()` and a pinned working directory, or it won't start at all.** A bare Generic Host console app doesn't respond to the Windows Service Control Manager's start/stop handshake — Windows kills it almost immediately (error 1053) without `Microsoft.Extensions.Hosting.WindowsServices`' `.UseWindowsService()` in the host builder (harmless no-op everywhere else: interactive runs, systemd). Separately, Windows Services default their working directory to `C:\Windows\System32`, not the executable's folder, and there's no service-creation parameter that overrides this — every relative path in this app (`appsettings.json`, `posted_alerts.txt`, `logs/`, etc.) would resolve against the wrong directory. Fixed by `Directory.SetCurrentDirectory(AppContext.BaseDirectory)` as the very first line in `Program.cs`, before anything touches the filesystem. Both fixes are required together; either alone still leaves the service broken.
 - **`Update.ServiceName` in `appsettings.json` is the single source of truth for the service name — don't reintroduce a second place to type it.** Early in this feature's design, `setup-service.ps1 -ServiceName` and `UpdateSettings.ServiceName` were two independently-typed values that merely had to happen to match; a typo in either one would silently break `AutoApply`'s restart-after-update (or restart a different instance's service). Fixed: both `scripts/setup-service.ps1` and `scripts/update.ps1` now default `-ServiceName` to `Resolve-ServiceName` — a small function (intentionally duplicated identically in both scripts, since each is meant to run standalone) that reads `Update.ServiceName` out of `appsettings.Local.json` (checked first) or `appsettings.json` in `-InstallDir`, falling back to `"nwsalertbot"` only if neither sets it. `UpdateCheckService.LaunchUpdater` still passes `-ServiceName` explicitly from the C# side (it already has the value from config binding, so passing it is free and avoids any ambiguity) — but a human only ever types the name in one place: `appsettings.json`. If you touch either script's `-ServiceName` handling, preserve this — don't go back to requiring it as a required/duplicated CLI argument. `scripts/uninstall-service.ps1` breaks this "duplicated for standalone-ness" pattern deliberately: it's a thin wrapper that just forwards `-ServiceName`/`-InstallDir`/`-DryRun` to `setup-service.ps1 -Uninstall` rather than re-implementing `Resolve-ServiceName` and the actual removal logic a third time — it only exists for discoverability (an obviously-named file beats a hidden flag), not to run standalone, so it's fine for it to depend on `setup-service.ps1` being present alongside it.
-- **Auto-Update has known unattended-operation gaps, documented in docs/TECHNICAL.md "Known Limitations" (under Auto-Update — Full Reference) — read that before touching `UpdateCheckService`/`update.ps1`/`setup-service.ps1`.** Remaining: the Windows-Service self-stop-during-update path is theoretically safe but has never been exercised against a real service (possible race between SCM failure-recovery and the file swap). See `docs/plans/auto-update-remaining-limitations.md` for how to actually verify this live — don't reinvent it from scratch. Fixed since first written: Linux passwordless-sudo is now opt-in via `setup-service.ps1 -ConfigurePasswordlessSudo` (a scoped `/etc/sudoers.d/` rule, `visudo -c`-validated before install); `AlertPollingService` now logs the running version at startup (closes "no confirmation an update succeeded"); `setup-service.ps1` pre-checks `appsettings.json` exists; `UpdateCheckService` skips the check entirely on an unversioned (`0.0.0`) dev build (`UnversionedDevBuild` constant); `update.ps1` now health-checks the new version after restarting (`Start-BotService`/`Test-BotIsRunning`, `-RollbackCheckDelaySeconds` default 15) and automatically restores `.bak` + restarts if it didn't stay running — closes "no automatic rollback if a bad release is applied"; `release.yml` now publishes a `checksums.txt` (SHA256 per asset) and `update.ps1` verifies the downloaded archive against it before extracting, aborting on a missing/mismatched entry — closes "no integrity check on downloaded releases" (this protects against corrupted uploads/transport tampering only, not a compromised `release.yml`/repo/token — see docs/TECHNICAL.md for the honest scope). The health check is process/service-alive only, not a real app health check (no endpoint exists to call) — it catches "didn't start at all," not "started but is subtly broken." None of the remaining gaps are regressions — they're accepted-for-now, called out explicitly so they don't get rediscovered as surprises.
+- **Auto-Update has known unattended-operation gaps, documented in docs/TECHNICAL.md "Known Limitations" (under Auto-Update — Full Reference) — read that before touching `UpdateCheckService`/`update.ps1`/`setup-service.ps1`.** Remaining: the Windows-Service self-stop-during-update path is theoretically safe but has never been exercised against a real service (possible race between SCM failure-recovery and the file swap). See `docs/plans/auto-update-remaining-limitations.md` for how to actually verify this live — don't reinvent it from scratch. Fixed since first written: `setup-service.ps1`'s systemd unit now sets `KillMode=process` — the
+default `KillMode=control-group` meant that when `UpdateCheckService` spawned `update.ps1` as a
+child process and then stopped the app to let it swap the binary, systemd killed the entire
+cgroup (including the just-spawned `update.ps1`) before it ever ran; `Restart=always` then
+brought the old binary straight back up, which immediately re-detected the same newer release and
+repeated forever — a confirmed-live crash loop that never actually updated. Existing installs
+need `KillMode=process` added to their unit file by hand (or re-run `setup-service.ps1`) since
+this only changes the generated template. Linux passwordless-sudo is now opt-in via `setup-service.ps1 -ConfigurePasswordlessSudo` (a scoped `/etc/sudoers.d/` rule, `visudo -c`-validated before install); `AlertPollingService` now logs the running version at startup (closes "no confirmation an update succeeded"); `setup-service.ps1` pre-checks `appsettings.json` exists; `UpdateCheckService` skips the check entirely on an unversioned (`0.0.0`) dev build (`UnversionedDevBuild` constant); `update.ps1` now health-checks the new version after restarting (`Start-BotService`/`Test-BotIsRunning`, `-RollbackCheckDelaySeconds` default 15) and automatically restores `.bak` + restarts if it didn't stay running — closes "no automatic rollback if a bad release is applied"; `release.yml` now publishes a `checksums.txt` (SHA256 per asset) and `update.ps1` verifies the downloaded archive against it before extracting, aborting on a missing/mismatched entry — closes "no integrity check on downloaded releases" (this protects against corrupted uploads/transport tampering only, not a compromised `release.yml`/repo/token — see docs/TECHNICAL.md for the honest scope). The health check is process/service-alive only, not a real app health check (no endpoint exists to call) — it catches "didn't start at all," not "started but is subtly broken." None of the remaining gaps are regressions — they're accepted-for-now, called out explicitly so they don't get rediscovered as surprises.
 - **`UpdateCheckService` version comparison depends on `-p:Version=` being injected at publish time** — `release.yml`'s publish step strips the leading `v` from the pushed tag and passes it as `-p:Version=X.Y.Z`; `NwsAlertBot.csproj`'s default `<Version>0.0.0</Version>` only applies to local/dev builds (`deploy.yml` doesn't inject a version either, since it deploys from master, not tags — self-update isn't relevant to that continuously-deployed instance). If `release.yml`'s version injection is ever removed, every future release will compare as version 0.0.0 and `UpdateCheckService` will never detect it as newer.
 - **`RateLimitTracker` is hand-rolled, not `System.Threading.RateLimiting`** — `XService`/`TwilioService` use it to guard `MaxPostsPerMonth`/`MaxSmsPerDay`. The BCL's rate limiters (e.g. `FixedWindowRateLimiter`) are in-memory only with no way to reconstruct state from external storage, and this bot redeploys on every push to master (`deploy.yml`) — an in-memory-only counter would reset on every deploy, making a monthly/daily cap nearly meaningless. `RateLimitTracker` persists window-start + count to a small state file instead (`x_post_count.txt`, `twilio_sms_count.txt`). Don't swap it for a BCL limiter without solving the persistence problem first.
 - **Pushover priority 2 (emergency) requires `retry` + `expire`** — omitting causes a 400 error. Always include them when priority == 2.

CONTRIBUTING.md

@@ -160,6 +160,7 @@ ExecStart=/opt/nwsalertbot/NwsAlertBot
 Restart=always
 RestartSec=10
 KillSignal=SIGINT
+KillMode=process
 
 StandardOutput=journal
 StandardError=journal

docs/TECHNICAL.md

@@ -204,6 +204,16 @@ This feature is aimed at unattended, "set and forget" use — which is also exac
 weakest points matter most, since nobody's watching. Worth understanding before turning on
 `AutoApply` unattended:
 
+- ~~**Linux `Update.AutoApply` could crash-loop forever without ever actually updating.**~~
+  Fixed: `setup-service.ps1`'s systemd unit now sets `KillMode=process`. Previously, the default
+  `KillMode=control-group` meant that when `UpdateCheckService` spawned `update.ps1` as a child
+  process and then stopped the app so it could swap the binary, systemd killed the *entire
+  cgroup* — including the just-spawned `update.ps1` — before it ever ran. `Restart=always` then
+  brought the old binary straight back up, which immediately re-detected the same newer release
+  and repeated, forever, ~10-13s apart (`RestartSec=10` plus startup overhead), never once
+  reaching the point of actually fetching alerts. Confirmed live. Existing installations must
+  add `KillMode=process` to their unit file by hand (or re-run `setup-service.ps1`) — this fix
+  only changes the generated template, not units already written to disk.
 - **Linux auto-restart requires passwordless `sudo`.** After swapping the executable,
   `update.ps1` runs `sudo systemctl restart $ServiceName` non-interactively. If the account
   running the bot doesn't have a `NOPASSWD` sudoers rule for that command, this silently fails

scripts/setup-service.ps1

@@ -250,6 +250,15 @@ elseif ($IsLinux) {
         }
     }
 
+    # KillMode=process (systemd default is "control-group") matters specifically for
+    # Update.AutoApply: UpdateCheckService spawns update.ps1 as a child process, then calls
+    # StopApplication() to exit so update.ps1 can swap the binary. Under the default
+    # control-group KillMode, stopping the service kills every process in its cgroup --
+    # including that just-spawned update.ps1 -- before it ever gets to run, so Restart=always
+    # just relaunches the same old binary, which immediately detects the same "newer version
+    # available" and repeats forever (confirmed live: a ~10-13s crash loop that never actually
+    # updates, RestartSec apart). KillMode=process limits the kill signal to the unit's main
+    # PID, letting update.ps1 survive to finish the swap and its own `systemctl restart`.
     $unitContent = @"
 [Unit]
 Description=$Description
@@ -262,6 +271,7 @@ WorkingDirectory=$InstallDir
 User=$serviceUser
 Restart=always
 RestartSec=10
+KillMode=process
 
 [Install]
 WantedBy=multi-user.target