From 64c3eac4f7853a20b3bae9d9f07210874255fb60 Mon Sep 17 00:00:00 2001 From: Mike Wills Date: Thu, 16 Jul 2026 00:17:45 -0500 Subject: [PATCH] Fix Update.AutoApply crash-looping forever on Linux without updating systemd's default KillMode=control-group kills every process in a service's cgroup on stop/restart -- including the update.ps1 child process UpdateCheckService spawns right before calling StopApplication() to let it swap the binary. That killed update.ps1 before it ever ran, and Restart=always 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 fetching alerts. Confirmed live on a production instance stuck in this loop for 20+ minutes. setup-service.ps1's generated unit (and the hand-maintained deploy.yml unit documented in CONTRIBUTING.md) now set KillMode=process, so only the app's own PID is killed on stop -- update.ps1 survives to finish the swap and its own systemctl restart. Existing installations need KillMode=process added to their unit file by hand (or re-run setup-service.ps1) since this only changes the generated template. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01CfWmHii9XYmPNBhz5dGDxZ --- CHANGELOG.md | 11 +++++++++++ CLAUDE.md | 9 ++++++++- CONTRIBUTING.md | 1 + docs/TECHNICAL.md | 10 ++++++++++ scripts/setup-service.ps1 | 10 ++++++++++ 5 files changed, 40 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bb89c4..d430fa5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CLAUDE.md b/CLAUDE.md index 2e4c5b8..d7ad516 100644 --- a/CLAUDE.md +++ b/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 `0.0.0` 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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fdc2ac7..91e7fa0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -160,6 +160,7 @@ ExecStart=/opt/nwsalertbot/NwsAlertBot Restart=always RestartSec=10 KillSignal=SIGINT +KillMode=process StandardOutput=journal StandardError=journal diff --git a/docs/TECHNICAL.md b/docs/TECHNICAL.md index 7c2ee45..096f4c8 100644 --- a/docs/TECHNICAL.md +++ b/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 diff --git a/scripts/setup-service.ps1 b/scripts/setup-service.ps1 index 3fdbcf5..ba76def 100644 --- a/scripts/setup-service.ps1 +++ b/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