Skip to content

Restart pimonitor.service on upgrade if already running#76

Merged
LarsLaskowski merged 1 commit into
mainfrom
claude/issue-59-rxf3jb
Jul 17, 2026
Merged

Restart pimonitor.service on upgrade if already running#76
LarsLaskowski merged 1 commit into
mainfrom
claude/issue-59-rxf3jb

Conversation

@LarsLaskowski

@LarsLaskowski LarsLaskowski commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

packaging/install.sh replaces the /usr/local/bin/pimonitor binary and the
systemd unit files on every run, then calls systemctl enable --now pimonitor.service. That command is a no-op when the unit is already active,
so re-running the installer to upgrade an already-installed PiMonitor left
the old binary running until an unrelated reboot/restart — the dashboard
footer version would silently disagree with what was just installed.

The script now records whether pimonitor.service was active before
daemon-reload/enable --now, and explicitly runs systemctl restart pimonitor.service afterwards if it was. This also picks up any changed
systemd unit settings for an already-running service, not just the binary.
The top-of-file usage comment was updated to describe this behavior instead
of claiming re-running is purely a no-op.

Related Issue

Closes #59

Checklist

  • Tests added/updated for the change (go test ./... passes locally)
  • go vet ./... and golangci-lint run are clean
  • Documentation updated if this changes the REST API (docs/API.md),
    configuration (README.md, packaging/pimonitor.example.yaml), or
    installation/packaging (packaging/install.sh, systemd units)
  • No breaking change to /api/v1/... response shapes, or a new API
    version was introduced instead

Note on verification: install.sh is a shell script with no Go unit
test coverage; there's no test harness in this repo for it. I verified the
change with bash -n (syntax) and by re-reading the systemctl
control flow. go build ./..., go vet ./..., go test ./..., and
golangci-lint run all pass, but they exercise the unrelated Go codebase,
not this script. Actually exercising an upgrade-of-a-running-service on a
real systemd host (Raspberry Pi or otherwise) hasn't been done and would be
good to confirm before/after a release.

'systemctl enable --now' is a no-op when the unit is already active, so
running install.sh to upgrade an already-installed PiMonitor replaced the
binary and unit files but left the old process running until an unrelated
reboot/restart. The dashboard footer version would then silently disagree
with what was just installed.

Detect whether the service was active before daemon-reload/enable, and
explicitly restart it afterwards if so.

Closes #59

@LarsLaskowski LarsLaskowski left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against the repo's review checklist. Checked out the branch and ran go build ./..., go vet ./..., go test ./... — all pass (as expected, the diff doesn't touch Go code). bash -n packaging/install.sh is clean.

The fix itself is correct:

  • systemctl is-active is wrapped in an if, so its non-zero exit on an inactive/failed service doesn't trip set -euo pipefail.
  • The state is captured before daemon-reload/enable --now and the restart runs after the new unit file is loaded, so both a new binary and changed unit settings take effect — exactly the failure mode described in #59.
  • Edge cases behave sensibly: a failed or inactive service reports not-active and gets a fresh start from enable --now with the new binary anyway, so nothing is missed.
  • The updated usage comment now matches actual behavior.

Two non-blocking observations, no change requested:

  1. An equivalent, slightly simpler shape would be systemctl enable pimonitor.service followed by an unconditional systemctl restart pimonitor.service (restart also starts an inactive unit), which drops the state variable. The explicit version here is arguably clearer about why, so fine either way.
  2. pimonitor-apt-update.timer has the same latent issue this PR fixes for the service: if a future release changes the timer's schedule, enable --now on the already-running timer won't apply it. Out of scope for #59, but worth remembering if the timer definition ever changes.

@LarsLaskowski
LarsLaskowski merged commit 8f67746 into main Jul 17, 2026
3 checks passed
@LarsLaskowski
LarsLaskowski deleted the claude/issue-59-rxf3jb branch July 17, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Robustness: install.sh never restarts a running service on upgrade (old binary keeps running)

2 participants