From 852b565a11fccc5ab8597995e799e7f16ffe4ea7 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 01:18:32 +0300 Subject: [PATCH 1/4] chore(deps): go 1.25.13, grpc v1.83.2 Raise the go directive from 1.25.12 to 1.25.13 so CI (setup-go reads go-version-file) builds with the standard-library security fixes (crypto/tls, net/http, net/url, encoding/asn1, ...). Bump the indirect google.golang.org/grpc from v1.82.1 to v1.83.2. It fixes GO-2026-6348, which fails the govulncheck job on every PR today. v1.83.2 rather than v1.83.1 (Dependabot #46) because v1.83.1 fails dependency review (GHSA-2v4p-qf9q-27wj). govulncheck now reports 0 reachable vulnerabilities. Co-Authored-By: Claude Opus 5.5 (1M context) --- go.mod | 12 ++++++------ go.sum | 20 ++++++++++---------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/go.mod b/go.mod index 202c4dc..8771d2c 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/pilot-protocol/updater -go 1.25.12 +go 1.25.13 require github.com/pilot-protocol/common v0.5.13 @@ -65,16 +65,16 @@ require ( go.opentelemetry.io/otel/metric v1.44.0 // indirect go.opentelemetry.io/otel/trace v1.44.0 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect - golang.org/x/crypto v0.54.0 // indirect + golang.org/x/crypto v0.55.0 // indirect golang.org/x/mod v0.38.0 // indirect - golang.org/x/net v0.57.0 // indirect + golang.org/x/net v0.58.0 // indirect golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/term v0.45.0 // indirect - golang.org/x/text v0.40.0 // indirect + golang.org/x/text v0.41.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect - google.golang.org/genproto/googleapis/rpc v0.0.0-20260523011958-0a33c5d7ca68 // indirect - google.golang.org/grpc v1.82.1 // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect + google.golang.org/grpc v1.83.2 // indirect google.golang.org/protobuf v1.36.11 // indirect k8s.io/klog/v2 v2.140.0 // indirect ) diff --git a/go.sum b/go.sum index 358a420..9b6926a 100644 --- a/go.sum +++ b/go.sum @@ -329,12 +329,12 @@ go.yaml.in/yaml/v2 v2.4.4 h1:tuyd0P+2Ont/d6e2rl3be67goVK4R6deVxCUX5vyPaQ= go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ= go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= -golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= -golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= +golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= golang.org/x/mod v0.38.0 h1:MECBjubtXD7yj4HrhIUcywNaGeNVUdfVnxmPajOk4yk= golang.org/x/mod v0.38.0/go.mod h1:V6Xz0pq8TQ3dGqVQ1FVHuelZpAL0uNhSkk9ogYP3c40= -golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= -golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= @@ -343,8 +343,8 @@ golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= -golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= -golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= @@ -355,10 +355,10 @@ google.golang.org/genproto v0.0.0-20260319201613-d00831a3d3e7 h1:XzmzkmB14QhVhgn google.golang.org/genproto v0.0.0-20260319201613-d00831a3d3e7/go.mod h1:L43LFes82YgSonw6iTXTxXUX1OlULt4AQtkik4ULL/I= google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa h1:Kjn0N0tCrDgiAFW+lGO4JZ3ck44CehvJQMAwj9QF0G8= google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:q4lMZS6kskjT5HvCPrnnypcDPVJqT/f4nfxmkE7gryY= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260523011958-0a33c5d7ca68 h1:PvEgGJf9C/1u5CHkInMg7UFYYUoiaQmW2LbtH0pjB78= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260523011958-0a33c5d7ca68/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= -google.golang.org/grpc v1.82.1 h1:NnAxzGRA0677vCa4BUkOAnO5+FfQqVl9iUXeD0IqcGE= -google.golang.org/grpc v1.82.1/go.mod h1:yzTZ1TB1Z3SG+LIYaI+WiE8D5+PZ3ArnrSp8zF3+/ZA= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa h1:mZHHdPZl0dbGHCflZgAq/Q468DWVFcU2whhB2KAo8fk= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= +google.golang.org/grpc v1.83.2 h1:EManeRomTObA0BU7I8vXgg/78uE5MJ9M8B39EX2WscU= +google.golang.org/grpc v1.83.2/go.mod h1:YPI1hK3kDked6iHvgX3tR0y+nX/qpMFKhPgFsokw1S8= google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 7458884acb903851c17b651c3c084b49aa2b4a62 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 01:18:32 +0300 Subject: [PATCH 2/4] fix: make update failures visible; resumable downloads; GITHUB_TOKEN Update failures were invisible. RunOnce returned nothing, so `pilotctl --json update` printed status ok and exited 0 on a GitHub 403. `update status` could only show the enabled flag, so a 16-day stall (307 hourly failures) went unnoticed. Errors and state - RunOnce() now returns error. Existing statement call sites still compile. Failed checks (fetch, bad tag, invalid pin, apply) are returned instead of only being logged. - New update-state.json, written atomically after every check. It holds last check time, trigger, result and error, consecutive failures, last success, installed and latest versions, last update, a daemon restart error, and a loop heartbeat (loop_pid, next_check_at). Config.StatusPath sets the path. By default the file goes next to StatePath, so cmd/updater writes ~/.pilot/update-state.json with no change on its side. Writers merge with the file on disk, so manual and loop runs share one record. ReadStatus and LastStatus expose it. - RunOnce no longer os.Exit()s the caller when a release replaces pilot-updater. Before, `pilotctl update` exited 0 silently and never restarted the daemon. The loop still exits for its service manager, after recording the update. - Linux: the daemon is now restarted after an update. /proc//exe reads " (deleted)" once the binary is renamed over, so the old exact match never found the daemon. Restart failures are returned and recorded as restart_error. Downloads (sweep item 45) - Downloads no longer have a fixed 30 s total timeout, which needed ~5 Mbit/s for a 19 MB archive. A transfer fails after 60 s with no data or 30 min in total. Dropped or stalled transfers resume with HTTP Range (up to 4 attempts, 5xx retried, restart when Range is ignored). Stop() aborts an in-flight download. TLS handshake timeout is 20 s. GitHub API - The releases API now sends GITHUB_TOKEN/GH_TOKEN, as the attestations API already did. Tokens go only to api.github.com, and a 401 falls back to an unauthenticated retry. Rate-limit errors say so, give the reset time and how to raise the limit. No gh dependency (sweep item 12) - TestNoExternalToolDependency parses the sources and fails on any exec other than runCommand (used only for launchctl), or any "gh" literal. - The live attestation test runs with an empty PATH. It passes in a container that has no gh. - README documents what nodes stranded on v1.12.2-v1.13.4 (updater v0.2.3, which needed gh) must do. Tests no longer run the real launchctl. TestMain stubs runCommand. On a developer Mac, the suite used to `launchctl kickstart -k` the running Pilot daemon. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/security.yml | 2 +- CHANGELOG.md | 39 +++ README.md | 160 ++++++++- attestation.go | 30 +- attestation_test.go | 3 + download.go | 265 ++++++++++++++ github.go | 91 +++++ status.go | 261 ++++++++++++++ updater.go | 355 ++++++++++++------- zz_checksums_required_bug_test.go | 10 +- zz_coverage_test.go | 8 +- zz_download_test.go | 380 ++++++++++++++++++++ zz_fuzz_evals_test.go | 2 +- zz_github_test.go | 189 ++++++++++ zz_no_gh_test.go | 108 ++++++ zz_restart_test.go | 139 ++++++++ zz_status_test.go | 565 ++++++++++++++++++++++++++++++ zz_test.go | 21 +- 18 files changed, 2470 insertions(+), 158 deletions(-) create mode 100644 download.go create mode 100644 github.go create mode 100644 status.go create mode 100644 zz_download_test.go create mode 100644 zz_github_test.go create mode 100644 zz_no_gh_test.go create mode 100644 zz_restart_test.go create mode 100644 zz_status_test.go diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 9c47c3f..7383950 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -29,7 +29,7 @@ jobs: # SAST. Accepted rule exclusions (documented, minimal — never blanket): # G104 — best-effort os.Remove/Close on error-cleanup paths. - # G204 — exec of gh / launchctl with internally-built args (no user input). + # G204 — exec of launchctl with internally-built args (no user input). # G301 — staging/install dirs are 0755 (operator-readable install tree). # G302 — extracted artifacts are executables and must be 0755. # G304 — file paths derive from InstallDir / temp dir, inherent to an updater. diff --git a/CHANGELOG.md b/CHANGELOG.md index e1da867..907dc3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,45 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- `update-state.json` status file written after every check (`Config.StatusPath`, + default: next to `StatePath`). It records the last check time, result and + error, the run of consecutive failures, the installed and latest versions, + the last update, any daemon restart failure, and a heartbeat for the loop + (`loop_pid`, `next_check_at`). Read it with `ReadStatus`. `LastStatus()` + returns the same record in-process. +- `GITHUB_TOKEN` / `GH_TOKEN` is now used for the releases API as well as for + attestations. It is sent only to `api.github.com`, and the call is retried + without it if GitHub rejects it with a 401. Rate-limit errors say when the + limit resets and how to raise it. +- Downloads resume with HTTP `Range` after a dropped or stalled transfer (up to + 4 attempts, 5xx responses retried) and are aborted by `Stop()`. + +### Changed + +- **`RunOnce()` now returns `error`.** Callers such as `pilotctl update` can + report a failed update and exit non-zero instead of printing "ok". Existing + call sites that ignore the result still compile. +- `RunOnce()` no longer exits the calling process when a release replaces + `pilot-updater`. It restarts the daemon and returns. The loop (`Start`) + still exits for its service manager, after recording the update. +- Asset downloads have no fixed 30 s total timeout. They fail after 60 s + without data, or after 30 minutes in total. The TLS handshake timeout is + now 20 s. +- `go` directive raised to 1.25.13 (standard-library security fixes). + +### Fixed + +- On Linux the daemon is now restarted after an update. The running daemon's + `/proc//exe` reads ` (deleted)` once its binary is renamed over, + so the old exact-path match never found it and daemons kept running the old + version. +- The test suite no longer runs the real `launchctl`. On a developer Mac with + Pilot installed, it used to restart the running daemon. + ## [v0.1.0] Initial release. diff --git a/README.md b/README.md index d75750c..de0e0cf 100644 --- a/README.md +++ b/README.md @@ -19,14 +19,34 @@ import "github.com/pilot-protocol/updater" ```go u := updater.New(updater.Config{ - Repo: "TeoSlayer/pilotprotocol", + Repo: "pilot-protocol/pilotprotocol", InstallDir: "/home/user/.pilot/bin", Version: "v1.10.5", CheckInterval: 1 * time.Hour, + StatePath: "/home/user/.pilot/auto-update.json", // {"enabled": bool} }) u.Start() ``` +One-shot check (what `pilotctl update` does): + +```go +u := updater.New(updater.Config{ + Repo: "pilot-protocol/pilotprotocol", + InstallDir: "/home/user/.pilot/bin", + StatusPath: "/home/user/.pilot/update-state.json", +}) +if err := u.RunOnce(); err != nil { + // The check failed: report it and exit non-zero. +} +st := u.LastStatus() // st.LastResult is "up_to_date" or "updated" +``` + +`RunOnce` returns the check's error. It never exits the calling process. If +the release replaced `pilot-updater`, the daemon is still restarted onto the +new binaries. A running updater service switches to its new binary the next +time its service manager restarts it. + ### Pinning a version Set `PinnedVersion` to lock the updater to a specific release tag. When @@ -37,7 +57,7 @@ install, then idles — it will **not** chase the latest release. Clear ```go u := updater.New(updater.Config{ - Repo: "TeoSlayer/pilotprotocol", + Repo: "pilot-protocol/pilotprotocol", InstallDir: "/home/user/.pilot/bin", PinnedVersion: "v1.10.5", // stay on this version }) @@ -48,11 +68,145 @@ The in-process `Service` adapter is used when embedding into the daemon; a standalone sidecar binary built from this package is also supported. +## Update status file + +After every check the updater writes `update-state.json` so that other tools +can tell whether updates actually work. Before this file existed, +`pilotctl update status` could only show whether auto-update was switched on, +and a node could fail every hourly check for weeks without anyone noticing. + +The file goes to `Config.StatusPath`. When that is empty and `StatePath` is +set, it goes next to the control file. For a standard install that is +`~/.pilot/auto-update.json` → `~/.pilot/update-state.json`. The long-running +updater and one-shot `pilotctl update` runs share the file: each writer +merges its fields into what is already there, and writes are atomic. + +```json +{ + "updater_version": "v1.13.10", + "repo": "pilot-protocol/pilotprotocol", + "last_check_at": "2026-09-24T10:00:12Z", + "last_check_trigger": "auto", + "last_result": "failed", + "last_error": "fetch latest release: GitHub API returned 403 (rate limit exceeded, resets at 2026-09-24T10:41:03Z; ...)", + "consecutive_failures": 3, + "last_success_at": "2026-09-24T07:00:09Z", + "current_version": "v1.13.9", + "latest_version": "v1.13.10", + "last_update_at": "2026-09-01T12:35:40Z", + "last_update_version": "v1.13.9", + "loop_pid": 812, + "loop_started_at": "2026-09-20T08:11:02Z", + "next_check_at": "2026-09-24T11:00:12Z" +} +``` + +| Field | Meaning | +|---|---| +| `last_result` | `up_to_date`, `updated` or `failed`. | +| `last_error` | Why the last check failed. Empty after a success. | +| `consecutive_failures` | Failed checks since the last success. | +| `last_success_at` | The last check that completed without error. | +| `current_version` / `latest_version` | The installed release, and the release it was compared against (the pin, when pinned). | +| `restart_error` | New binaries were installed but the daemon could not be restarted onto them. Cleared by the next successful restart. | +| `loop_pid`, `loop_started_at` | The long-running updater process. | +| `next_check_at` | When the loop wakes next. It is updated even while auto-update is disabled, so it doubles as a heartbeat. | + +Suggested readings for `pilotctl update status` and similar tools: + +- **Updates failing:** `consecutive_failures > 0`. Show `last_error`. +- **Updates stalled:** `last_success_at` is more than 48 hours old while auto-update is enabled. +- **No updater running:** auto-update is enabled, but the file is missing, `loop_pid` is not alive, or `next_check_at` is more than one interval in the past. This happens on Linux without systemd (containers, WSL, CI), where the installer cannot start the updater. +- **Daemon on the old version:** `restart_error` is set. Restart the daemon by hand. + +Use `updater.ReadStatus(path)` to read the file. A missing file returns an +error matching `errors.Is(err, fs.ErrNotExist)`. + +## GitHub API rate limits and `GITHUB_TOKEN` + +The updater calls the GitHub REST API for the release list and for the build +attestation. Without a token GitHub allows 60 requests per hour per IP, and a +shared NAT, VPN or CI egress can use that up. When that happens the +check fails with an error that says so, and it is recorded in the status file. + +If `GITHUB_TOKEN` (or `GH_TOKEN`) is set in the updater's environment, it is +sent to `api.github.com`, and only there, which raises the limit to 5,000 per +hour. The token needs no scopes because the repository is public: a classic +token with no scopes, or a fine-grained token with public read-only access, +is enough. If GitHub rejects the token (401), the updater retries without it, +so an expired token never stops updates. + +To set it for the service: + +- **systemd:** `sudo systemctl edit pilot-updater`, add + `[Service]` / `Environment=GITHUB_TOKEN=`, then + `sudo systemctl restart pilot-updater`. +- **launchd:** add an `EnvironmentVariables` dict with `GITHUB_TOKEN` to + `~/Library/LaunchAgents/network.pilotprotocol.pilot-updater.plist`, then + reload it with `launchctl bootout` / `launchctl bootstrap`. + +## Downloads + +Release archives are 17–20 MB. Downloads no longer have a fixed total +timeout (the old 30 s limit needed a sustained ~5 Mbit/s). Instead: + +- An attempt fails only when no data arrives for 60 s. +- A dropped or stalled transfer resumes with an HTTP `Range` request from the + bytes already on disk, or starts over if the server ignores `Range`. Up to + 4 attempts are made. 5xx responses are retried and 4xx responses are not. +- One file download is capped at 30 minutes in total. +- `Stop()` aborts an in-flight download. + +The SHA-256 match against the attested `checksums.txt` still gates every install. + +## No external tools (no `gh`) + +The updater never runs `gh` or any other external tool. It checks the SLSA +provenance of `checksums.txt` in-process with sigstore-go +(`attestation.go`). The only command it runs is `launchctl kickstart`, which +restarts the daemon under launchd on macOS. `TestNoExternalToolDependency` +fails the build if another exec is added. + +### Nodes stuck on v1.12.2–v1.13.4 (updater v0.2.3) + +pilotprotocol releases **v1.12.2 through v1.13.4** shipped updater v0.2.3, +which ran `gh attestation verify` and refused every update when `gh` was not +on its `PATH`. Service managers never put `gh` on the `PATH`: launchd's default is +`/usr/bin:/bin:/usr/sbin:/sbin`, and most Linux hosts have no `gh` at all. So +those nodes cannot update themselves. `~/.pilot/updater.log` shows an hourly +`gh CLI required for SLSA attestation verification` error. + +The broken updater cannot fix itself. On an affected node, do one of these: + +1. **Re-run the installer** (recommended). It installs the current binaries, + including a gh-free updater, and restarts the services: + + ```sh + curl -fsSL https://pilotprotocol.network/install.sh | sh + ``` + +2. **Or** run `pilotctl update` once from an interactive shell where `gh` is + installed and logged in (`gh auth status`). That one-shot update uses your + shell's `PATH`, installs the current release and replaces `pilot-updater` + with a gh-free version. The old `pilotctl update` exits silently after + doing so. Then restart both services so they run the new binaries: + `sudo systemctl restart pilot-updater pilot-daemon` on Linux, or on macOS + `launchctl kickstart -k gui/$(id -u)/network.pilotprotocol.pilot-updater` + and the same command for `network.pilotprotocol.pilot-daemon`. + +Check the result with `pilotctl version` and `~/.pilot/bin/pilot-updater --version`. +Do **not** work around the problem with `PILOT_UPDATER_SKIP_ATTESTATION`, +because that switches off provenance checks. + ## Layout | File | What it does | |---|---| -| `updater.go` | `Updater` — GitHub release polling, download, SHA verify, atomic rename. | +| `updater.go` | `Updater`: check loop, `RunOnce`, release polling, install, daemon restart. | +| `download.go` | Resumable asset downloads with idle and total-duration limits. | +| `github.go` | GitHub API calls: optional `GITHUB_TOKEN`, rate-limit errors. | +| `attestation.go` | In-process SLSA provenance verification of `checksums.txt` (sigstore-go). | +| `status.go` | `Status` / `update-state.json` persistence and `ReadStatus`. | | `version.go` | SemVer parsing and comparison. | | `service.go` | `*Service` — `coreapi.Service` adapter. Build tag `!no_updater`. | | `service_disabled.go` | Stub when build tag `no_updater` is set. | diff --git a/attestation.go b/attestation.go index 9548b4d..2edc109 100644 --- a/attestation.go +++ b/attestation.go @@ -14,7 +14,6 @@ import ( "regexp" "strings" "sync" - "time" sgbundle "github.com/sigstore/sigstore-go/pkg/bundle" "github.com/sigstore/sigstore-go/pkg/root" @@ -47,8 +46,9 @@ var ( ) // githubToken returns a GitHub API token from the environment, if any. Optional: -// used only to raise the API rate limit (see fetchAttestations). GITHUB_TOKEN is -// the conventional name; GH_TOKEN is gh's alias. +// used only to raise the API rate limit (see doGitHubAPI) for the releases and +// attestations endpoints. GITHUB_TOKEN is the conventional name; GH_TOKEN is +// gh's alias. The updater never runs the gh CLI itself. func githubToken() string { for _, k := range []string{"GITHUB_TOKEN", "GH_TOKEN"} { if v := strings.TrimSpace(os.Getenv(k)); v != "" { @@ -89,8 +89,8 @@ func sha256Hex(path string) (string, error) { } // fetchAttestations retrieves the SLSA attestation bundles GitHub holds for the -// given repository + artifact digest. Unauthenticated: the endpoint is public -// for public repositories and requires no gh CLI or token. +// given repository + artifact digest. The endpoint is public for public +// repositories and requires no gh CLI or token. func fetchAttestations(client *http.Client, repo, digestHex string) ([]json.RawMessage, error) { url := fmt.Sprintf("%s/repos/%s/attestations/sha256:%s", attestationAPIBase, repo, digestHex) req, err := http.NewRequest(http.MethodGet, url, nil) @@ -99,23 +99,19 @@ func fetchAttestations(client *http.Client, repo, digestHex string) ([]json.RawM } req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("X-GitHub-Api-Version", "2022-11-28") - // No token is required — the endpoint is public for public repositories. - // If one is present in the environment we send it purely to lift the - // 60-req/hr unauthenticated rate limit (5000/hr authenticated), so a busy - // fleet does not intermittently fail closed on a 403 rate-limit. Absence of - // a token never changes correctness, only the rate ceiling. - if tok := githubToken(); tok != "" { - req.Header.Set("Authorization", "Bearer "+tok) - } - resp, err := client.Do(req) + // No token is required. If GITHUB_TOKEN/GH_TOKEN is set, doGitHubAPI + // sends it purely to lift the 60-req/hr unauthenticated rate limit + // (5000/hr authenticated), so a busy fleet does not intermittently fail + // closed on a 403 rate-limit. A token never changes correctness, only the + // rate ceiling. + resp, err := doGitHubAPI(client, req) if err != nil { return nil, fmt.Errorf("fetch attestations: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) - return nil, fmt.Errorf("GitHub attestations API returned %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) + return nil, githubAPIError("GitHub attestations API", resp) } var parsed githubAttestationsResponse @@ -206,7 +202,7 @@ func realVerifyChecksumsAttestationFn(repo, tag, checksumsPath string) error { verify.WithCertificateIdentity(certID), ) - client := &http.Client{Timeout: 30 * time.Second} + client := &http.Client{Timeout: apiTimeout, Transport: newTransport()} rawBundles, err := fetchAttestations(client, repo, digestHex) if err != nil { return err diff --git a/attestation_test.go b/attestation_test.go index 93fd502..a0b0087 100644 --- a/attestation_test.go +++ b/attestation_test.go @@ -168,6 +168,9 @@ func TestRealVerify_EndToEnd_v1_12_5(t *testing.T) { if !networkUp() { t.Skip("network unreachable; skipping live attestation verification") } + // Run with an empty PATH, as a service manager effectively does for + // user-installed tools: verification must not need gh or anything else. + t.Setenv("PATH", t.TempDir()) dir := t.TempDir() cks := filepath.Join(dir, "checksums.txt") diff --git a/download.go b/download.go new file mode 100644 index 0000000..f1b4490 --- /dev/null +++ b/download.go @@ -0,0 +1,265 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "os" + "strconv" + "strings" + "time" +) + +// Release archives are 17-20 MB. A fixed total timeout (the old 30 s +// http.Client.Timeout) needs a sustained ~5 Mbit/s and restarts the whole +// download an hour later on anything slower. Downloads now fail only when +// they stop making progress, and resume where they stopped. +const ( + // defaultDownloadIdleTimeout aborts an attempt that receives no bytes for + // this long (also bounds connect + TLS + response headers). + defaultDownloadIdleTimeout = 60 * time.Second + // downloadMaxDuration caps one file download across all attempts, so a + // connection that trickles a byte a minute cannot pin the updater forever. + downloadMaxDuration = 30 * time.Minute + // downloadMaxAttempts is the number of tries per file (1 + resumes). + downloadMaxAttempts = 4 + // defaultDownloadRetryBackoff is the base pause between attempts; the + // n-th retry waits n times this. + defaultDownloadRetryBackoff = 2 * time.Second + // apiTimeout bounds a GitHub REST API call (small JSON responses). + apiTimeout = 30 * time.Second +) + +// errDownloadStalled is the cause recorded when an attempt receives no data +// for the idle timeout. +var errDownloadStalled = errors.New("download stalled") + +// newTransport returns the HTTP transport used for API calls and downloads: +// the default transport with a longer TLS handshake budget (the default 10 s +// tripped on slow links) and an explicit response-header timeout. +func newTransport() *http.Transport { + var t *http.Transport + if dt, ok := http.DefaultTransport.(*http.Transport); ok { + t = dt.Clone() + } else { + t = &http.Transport{Proxy: http.ProxyFromEnvironment} + } + t.TLSHandshakeTimeout = 20 * time.Second + t.ResponseHeaderTimeout = 30 * time.Second + return t +} + +// downloadClient returns the client used for release asset downloads. It +// must have no total Timeout: progress is enforced by the idle timeout and +// the overall cap in downloadFile instead. +func (u *Updater) downloadClient() *http.Client { + if u.dlClient != nil { + return u.dlClient + } + if u.client == nil { + return &http.Client{Transport: newTransport()} + } + c := *u.client + c.Timeout = 0 + return &c +} + +func (u *Updater) downloadIdleTimeout() time.Duration { + if u.dlIdleTimeout > 0 { + return u.dlIdleTimeout + } + return defaultDownloadIdleTimeout +} + +// downloadFile fetches url into dst. An attempt that stalls, drops the +// connection or gets a 5xx is retried, resuming with an HTTP Range request +// from the bytes already on disk (or starting over when the server ignores +// Range). Integrity is still enforced afterwards by the SHA256 check. +func (u *Updater) downloadFile(url, dst string) error { + f, err := os.Create(dst) + if err != nil { + return err + } + defer f.Close() + + ctx, cancel := context.WithTimeout(context.Background(), downloadMaxDuration) + defer cancel() + if u.stopCh != nil { + // Abort promptly when the updater is being stopped (SIGTERM from the + // service manager) instead of holding shutdown for up to 30 minutes. + go func() { + select { + case <-u.stopCh: + cancel() + case <-ctx.Done(): + } + }() + } + + client := u.downloadClient() + var have int64 + var lastErr error + for attempt := 1; attempt <= downloadMaxAttempts; attempt++ { + if attempt > 1 { + if err := sleepCtx(ctx, time.Duration(attempt-1)*u.dlRetryBackoff); err != nil { + break + } + slog.Warn("download interrupted, retrying", + "url", url, "attempt", attempt, "resume_from", have, "error", lastErr) + } + var retry bool + have, retry, err = u.downloadAttempt(ctx, client, url, f, have) + if err == nil { + return nil + } + lastErr = err + if !retry || ctx.Err() != nil { + break + } + } + if ctx.Err() != nil && !errors.Is(lastErr, errDownloadStalled) { + switch { + case errors.Is(ctx.Err(), context.DeadlineExceeded): + return fmt.Errorf("download did not finish within %s (%d bytes received): %w", downloadMaxDuration, have, lastErr) + default: + return fmt.Errorf("download aborted (updater stopping): %w", lastErr) + } + } + return lastErr +} + +// downloadAttempt makes one GET for url, resuming at offset when offset > 0, +// and appends the body to f. It returns the number of bytes now in f, +// whether a failure is worth retrying, and the error. +func (u *Updater) downloadAttempt(ctx context.Context, client *http.Client, url string, f *os.File, offset int64) (int64, bool, error) { + idle := u.downloadIdleTimeout() + actx, cancel := context.WithCancelCause(ctx) + defer cancel(nil) + timer := time.AfterFunc(idle, func() { cancel(errDownloadStalled) }) + defer timer.Stop() + + // explain turns a context cancellation caused by the idle timer into a + // clear message instead of a bare "context canceled". + explain := func(err error) error { + if errors.Is(context.Cause(actx), errDownloadStalled) { + return fmt.Errorf("no data received for %s (%d bytes so far): %w", idle, offset, errDownloadStalled) + } + return err + } + + req, err := http.NewRequestWithContext(actx, http.MethodGet, url, nil) + if err != nil { + return offset, false, err + } + if offset > 0 { + req.Header.Set("Range", fmt.Sprintf("bytes=%d-", offset)) + } + resp, err := client.Do(req) + if err != nil { + return offset, true, explain(err) + } + defer resp.Body.Close() + + switch { + case resp.StatusCode == http.StatusOK: + if offset > 0 { + // The server ignored the Range header: start over. + if offset, err = restartFile(f); err != nil { + return offset, false, err + } + } + case resp.StatusCode == http.StatusPartialContent: + start, ok := contentRangeStart(resp.Header.Get("Content-Range")) + if !ok || start != offset { + off, err := restartFile(f) + if err != nil { + return off, false, err + } + return off, true, fmt.Errorf("server resumed at an unexpected offset (Content-Range %q, wanted %d)", resp.Header.Get("Content-Range"), offset) + } + case resp.StatusCode == http.StatusRequestedRangeNotSatisfiable && offset > 0: + off, err := restartFile(f) + if err != nil { + return off, false, err + } + return off, true, fmt.Errorf("server refused to resume at byte %d (HTTP 416)", offset) + default: + return offset, resp.StatusCode >= 500, fmt.Errorf("HTTP %d for %s", resp.StatusCode, url) + } + + // Read one byte past the cap so we can distinguish "exactly at the limit" + // from "exceeded the limit". Silently truncating an oversize archive + // would surface as a confusing "checksum mismatch" instead. + body := &progressReader{r: resp.Body, onRead: func() { timer.Reset(idle) }} + n, err := io.Copy(f, io.LimitReader(body, maxDownloadBytes+1-offset)) + offset += n + if err != nil { + return offset, true, explain(err) + } + if offset > maxDownloadBytes { + return offset, false, fmt.Errorf("archive exceeds max download size %d bytes", maxDownloadBytes) + } + return offset, false, nil +} + +// restartFile truncates f so the next write starts at byte 0. +func restartFile(f *os.File) (int64, error) { + if err := f.Truncate(0); err != nil { + return 0, err + } + _, err := f.Seek(0, io.SeekStart) + return 0, err +} + +// contentRangeStart parses the first byte position of a +// "bytes -/" Content-Range header. +func contentRangeStart(h string) (int64, bool) { + rest, ok := strings.CutPrefix(strings.TrimSpace(h), "bytes ") + if !ok { + return 0, false + } + startStr, _, ok := strings.Cut(rest, "-") + if !ok { + return 0, false + } + n, err := strconv.ParseInt(strings.TrimSpace(startStr), 10, 64) + if err != nil || n < 0 { + return 0, false + } + return n, true +} + +// progressReader calls onRead after every read that returned data, which +// re-arms the idle timer. +type progressReader struct { + r io.Reader + onRead func() +} + +func (p *progressReader) Read(b []byte) (int, error) { + n, err := p.r.Read(b) + if n > 0 { + p.onRead() + } + return n, err +} + +// sleepCtx waits for d or until ctx is done. +func sleepCtx(ctx context.Context, d time.Duration) error { + if d <= 0 { + return ctx.Err() + } + t := time.NewTimer(d) + defer t.Stop() + select { + case <-t.C: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} diff --git a/github.go b/github.go new file mode 100644 index 0000000..dbace56 --- /dev/null +++ b/github.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "fmt" + "io" + "log/slog" + "net/http" + "strconv" + "strings" + "time" +) + +// githubAPIHost is the only host a GitHub token is ever sent to. Release +// assets are served from other hosts (objects.githubusercontent.com via +// redirects) and never see the token. +const githubAPIHost = "api.github.com" + +// doGitHubAPI sends req to the GitHub REST API. When GITHUB_TOKEN (or +// GH_TOKEN) is set and the request goes to api.github.com, the token is sent +// to lift the unauthenticated limit of 60 requests/hour per IP, which a +// shared NAT or VPN egress exhausts (the updater then stops updating). The +// releases and attestations endpoints are public, so a token is never +// required; if GitHub rejects the token (401: expired, revoked, mistyped) the +// request is retried once without it, so a stale token cannot stop updates. +func doGitHubAPI(client *http.Client, req *http.Request) (*http.Response, error) { + tok := githubToken() + if tok == "" || req.URL.Hostname() != githubAPIHost { + return client.Do(req) + } + authed := req.Clone(req.Context()) + authed.Header.Set("Authorization", "Bearer "+tok) + resp, err := client.Do(authed) + if err != nil || resp.StatusCode != http.StatusUnauthorized { + return resp, err + } + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) + resp.Body.Close() + slog.Warn("GitHub rejected GITHUB_TOKEN/GH_TOKEN (HTTP 401); retrying without it — fix or unset the token") + return client.Do(req) +} + +// githubAPIError builds the error for a non-200 GitHub API response. The +// message always starts with " returned ". A rate-limit +// response additionally says so plainly, when the limit resets and how to +// raise it, so the failure is actionable when it surfaces in +// `pilotctl update` or the status file. +func githubAPIError(what string, resp *http.Response) error { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) + msg := strings.TrimSpace(string(body)) + if isGitHubRateLimit(resp, msg) { + detail := "rate limit exceeded" + if reset := rateLimitReset(resp); !reset.IsZero() { + detail += ", resets at " + reset.UTC().Format(time.RFC3339) + } + if githubToken() == "" { + detail += "; unauthenticated requests are limited to 60/hour per IP — set GITHUB_TOKEN in the updater's environment to raise the limit" + } + return fmt.Errorf("%s returned %d (%s): %s", what, resp.StatusCode, detail, msg) + } + return fmt.Errorf("%s returned %d: %s", what, resp.StatusCode, msg) +} + +// isGitHubRateLimit reports whether resp is GitHub throttling the caller +// (primary limit: 403/429 with X-RateLimit-Remaining: 0; secondary limit: +// 403/429 whose body mentions a rate limit). +func isGitHubRateLimit(resp *http.Response, body string) bool { + if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusTooManyRequests { + return false + } + if resp.Header.Get("X-RateLimit-Remaining") == "0" { + return true + } + b := strings.ToLower(body) + return strings.Contains(b, "rate limit") || strings.Contains(b, "abuse detection") +} + +// rateLimitReset returns when the primary rate limit resets, or the zero time +// when GitHub did not say. +func rateLimitReset(resp *http.Response) time.Time { + v := resp.Header.Get("X-RateLimit-Reset") + if v == "" { + return time.Time{} + } + secs, err := strconv.ParseInt(v, 10, 64) + if err != nil || secs <= 0 { + return time.Time{} + } + return time.Unix(secs, 0) +} diff --git a/status.go b/status.go new file mode 100644 index 0000000..cb6a1a7 --- /dev/null +++ b/status.go @@ -0,0 +1,261 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "encoding/json" + "fmt" + "log/slog" + "os" + "path/filepath" + "time" +) + +// StatusFileName is the default name of the file the updater writes after +// every check. When Config.StatusPath is empty and Config.StatePath is set, +// the status file lives next to the control file (for the standard install: +// ~/.pilot/auto-update.json -> ~/.pilot/update-state.json). +const StatusFileName = "update-state.json" + +// Values of Status.LastResult. +const ( + // ResultUpToDate: the check succeeded and nothing needed installing + // (the installed version is the latest release, or the pinned one). + ResultUpToDate = "up_to_date" + // ResultUpdated: the check succeeded and a release was installed. + ResultUpdated = "updated" + // ResultFailed: the check failed; Status.LastError says why. + ResultFailed = "failed" +) + +// Values of Status.LastCheckTrigger. +const ( + // TriggerAuto marks a check run by the periodic loop (Start). + TriggerAuto = "auto" + // TriggerManual marks a one-shot check (RunOnce, e.g. `pilotctl update`). + TriggerManual = "manual" +) + +// Status is the machine-readable record of what the updater last did. It is +// persisted as JSON at Config.StatusPath after every check so that other +// tools (`pilotctl update status`, `daemon status`, support scripts) can tell +// whether updates are actually working, instead of only whether they are +// switched on. +// +// The file is shared by the long-running updater service and one-shot +// `pilotctl update` runs: each writer merges its fields into what is already +// on disk, so a manual run keeps the loop's LoopPID/NextCheckAt and the loop +// keeps the failure streak a manual run started. +type Status struct { + // UpdaterVersion is the version of the updater that wrote the record. + UpdaterVersion string `json:"updater_version,omitempty"` + // Repo is the GitHub owner/repo releases are read from. + Repo string `json:"repo,omitempty"` + // PinnedVersion is the pin in force for the last check ("" = latest). + PinnedVersion string `json:"pinned_version,omitempty"` + + // LastCheckAt is when the last check finished. + LastCheckAt time.Time `json:"last_check_at,omitzero"` + // LastCheckTrigger is TriggerAuto or TriggerManual. + LastCheckTrigger string `json:"last_check_trigger,omitempty"` + // LastResult is ResultUpToDate, ResultUpdated or ResultFailed. + LastResult string `json:"last_result,omitempty"` + // LastError is the error of the last check; empty when it succeeded. + LastError string `json:"last_error,omitempty"` + // ConsecutiveFailures counts failed checks since the last success. + ConsecutiveFailures int `json:"consecutive_failures"` + // LastSuccessAt is when a check last completed without error. + LastSuccessAt time.Time `json:"last_success_at,omitzero"` + + // CurrentVersion is the installed release after the last check. + CurrentVersion string `json:"current_version,omitempty"` + // LatestVersion is the release the last check compared against: the + // latest published release, or the pinned tag when pinned. + LatestVersion string `json:"latest_version,omitempty"` + + // LastUpdateAt / LastUpdateVersion record the last release installed. + LastUpdateAt time.Time `json:"last_update_at,omitzero"` + LastUpdateVersion string `json:"last_update_version,omitempty"` + // RestartError is set when new binaries were installed but the daemon + // could not be restarted onto them (it keeps running the old version + // until restarted by hand). Cleared by the next successful restart. + RestartError string `json:"restart_error,omitempty"` + + // LoopPID and LoopStartedAt identify the long-running updater process + // (the service started via Start). Absent when no loop ever ran. + LoopPID int `json:"loop_pid,omitempty"` + LoopStartedAt time.Time `json:"loop_started_at,omitzero"` + // NextCheckAt is when the loop will next wake up (plus up to 30s of + // jitter). A NextCheckAt well in the past means the loop is not running. + NextCheckAt time.Time `json:"next_check_at,omitzero"` +} + +// ReadStatus loads the status file written by the updater. A missing file +// returns an error satisfying errors.Is(err, fs.ErrNotExist), which callers +// should read as "the updater has never recorded a check here". +func ReadStatus(path string) (Status, error) { + var s Status + data, err := os.ReadFile(path) + if err != nil { + return s, err + } + if err := json.Unmarshal(data, &s); err != nil { + return Status{}, fmt.Errorf("parse %s: %w", path, err) + } + return s, nil +} + +// statusPath returns where this updater records its status, or "" when it +// records nothing (no StatusPath and no StatePath configured). +func (u *Updater) statusPath() string { + if u.config.StatusPath != "" { + return u.config.StatusPath + } + if u.config.StatePath != "" { + return filepath.Join(filepath.Dir(u.config.StatePath), StatusFileName) + } + return "" +} + +// LastStatus returns the status recorded by this Updater's most recent check +// (or loop event). It is available even when no status file is configured, +// so a caller of RunOnce can report the outcome without reading the file. +func (u *Updater) LastStatus() Status { + u.statusMu.Lock() + defer u.statusMu.Unlock() + return u.status +} + +// updateStatus applies mutate to the current status and persists it. The +// base is the file on disk (so fields written by another process survive); +// when the file is missing or unreadable the in-memory copy is used. Write +// failures are logged, never returned: recording the outcome must not change +// the outcome. +func (u *Updater) updateStatus(mutate func(*Status)) { + u.statusMu.Lock() + defer u.statusMu.Unlock() + + path := u.statusPath() + base := u.status + if path != "" { + if onDisk, err := ReadStatus(path); err == nil { + base = onDisk + } + } + mutate(&base) + base.UpdaterVersion = u.config.Version + base.Repo = u.config.Repo + u.status = base + + if path == "" { + return + } + if err := writeStatusFile(path, base); err != nil { + slog.Warn("failed to write update status file", "path", path, "error", err) + } +} + +// writeStatusFile writes s to path atomically (temp file + rename in the same +// directory) so a reader never sees a torn file. +func writeStatusFile(path string, s Status) error { + data, err := json.MarshalIndent(s, "", " ") + if err != nil { + return err + } + data = append(data, '\n') + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + tmp, err := os.CreateTemp(dir, ".update-state-*.tmp") + if err != nil { + return err + } + tmpName := tmp.Name() + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + os.Remove(tmpName) + return err + } + if err := os.Chmod(tmpName, 0o644); err != nil { + os.Remove(tmpName) + return err + } + if err := os.Rename(tmpName, path); err != nil { + os.Remove(tmpName) + return err + } + return nil +} + +// checkOutcome is what one update check found and did. +type checkOutcome struct { + current string // installed version before the check + latest string // release compared against (latest or pinned) + installed string // release installed by this check ("" = none) + // updaterReplaced reports that the pilot-updater binary itself was + // swapped, so the running updater process is now stale. + updaterReplaced bool + // restartErr is the error from restarting the daemon onto the new + // binaries, if that failed. + restartErr error +} + +// recordCheck persists the outcome of one check. +func (u *Updater) recordCheck(trigger string, out checkOutcome, checkErr error) { + now := time.Now().UTC() + u.updateStatus(func(s *Status) { + s.PinnedVersion = u.config.PinnedVersion + s.LastCheckAt = now + s.LastCheckTrigger = trigger + if out.latest != "" { + s.LatestVersion = out.latest + } + switch { + case out.installed != "": + s.CurrentVersion = out.installed + case out.current != "": + s.CurrentVersion = out.current + } + if checkErr != nil { + s.LastResult = ResultFailed + s.LastError = checkErr.Error() + s.ConsecutiveFailures++ + return + } + s.LastError = "" + s.ConsecutiveFailures = 0 + s.LastSuccessAt = now + if out.installed == "" { + s.LastResult = ResultUpToDate + return + } + s.LastResult = ResultUpdated + s.LastUpdateAt = now + s.LastUpdateVersion = out.installed + if out.restartErr != nil { + s.RestartError = out.restartErr.Error() + } else if !out.updaterReplaced { + // The daemon was restarted onto the new binaries. (When the + // updater replaced itself, the restart happens in the new + // process — see recoverPendingRestart — which records it.) + s.RestartError = "" + } + }) +} + +// recordRestart records the outcome of a daemon restart made outside a +// check (recoverPendingRestart). +func (u *Updater) recordRestart(err error) { + u.updateStatus(func(s *Status) { + if err != nil { + s.RestartError = err.Error() + } else { + s.RestartError = "" + } + }) +} diff --git a/updater.go b/updater.go index f7cf553..506a0d9 100644 --- a/updater.go +++ b/updater.go @@ -17,6 +17,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strconv" "strings" "sync" "syscall" @@ -72,15 +73,44 @@ type Config struct { // ignoring this gate. An empty StatePath preserves the legacy // always-on loop behaviour for backward compatibility. StatePath string + + // StatusPath is where the updater records the outcome of every check + // (see Status): last check time and result, last error, consecutive + // failures, installed and latest versions, and whether the loop is + // running. Empty means "update-state.json next to StatePath"; with + // neither set, nothing is written (LastStatus still reports it). + StatusPath string } // Updater periodically checks GitHub Releases for new versions and optionally applies them. type Updater struct { config Config - client *http.Client + client *http.Client // GitHub API calls (short total timeout) stopCh chan struct{} wg sync.WaitGroup exitFn func(int) // injectable for testing; defaults to os.Exit + + // dlClient downloads release assets. It has no total timeout: see + // downloadFile. Nil falls back to client without its total timeout. + dlClient *http.Client + dlIdleTimeout time.Duration // 0 = defaultDownloadIdleTimeout + dlRetryBackoff time.Duration // base pause between download attempts + + // Process hooks, injectable for tests. Nil means the real thing. + runCmd func(name string, args ...string) ([]byte, error) + killFn func(pid int, sig syscall.Signal) error + procRoot string // "" = /proc + + statusMu sync.Mutex + status Status +} + +// runCommand runs an external command and returns its combined output. The +// only command the updater ever runs is launchctl (daemon restart on macOS); +// it never runs gh or any other tool. Tests replace this so the suite cannot +// restart a real daemon on a developer machine. +var runCommand = func(name string, args ...string) ([]byte, error) { + return exec.Command(name, args...).CombinedOutput() } // GitHubRelease represents a subset of the GitHub release API response. @@ -98,10 +128,12 @@ type GitHubAsset struct { // New creates a new Updater. func New(cfg Config) *Updater { return &Updater{ - config: cfg, - client: &http.Client{Timeout: 30 * time.Second}, - stopCh: make(chan struct{}), - exitFn: os.Exit, + config: cfg, + client: &http.Client{Timeout: apiTimeout, Transport: newTransport()}, + dlClient: &http.Client{Transport: newTransport()}, + dlRetryBackoff: defaultDownloadRetryBackoff, + stopCh: make(chan struct{}), + exitFn: os.Exit, } } @@ -124,9 +156,19 @@ func (u *Updater) Stop() { // `pilotctl update` and similar CLI commands. It ALWAYS runs — the // StatePath enabled-gate applies only to the automatic loop, so a manual // `pilotctl update` works even when auto-update is disabled. -func (u *Updater) RunOnce() { +// +// It returns the check's error (nil when already up to date or when the +// update was installed), so callers can report failure and exit non-zero. +// LastStatus describes the outcome in detail, and the same record is written +// to the status file when one is configured. +// +// RunOnce never exits the calling process. When the release replaces the +// pilot-updater binary, the daemon is still restarted onto the new binaries; +// a separately running updater service picks up its new binary the next +// time its service manager restarts it. +func (u *Updater) RunOnce() error { u.recoverPendingRestart() - u.checkOnce() + return u.runCheck(TriggerManual) } // enabled reports whether the automatic update loop may apply updates. With @@ -153,16 +195,26 @@ func (u *Updater) enabled() bool { func (u *Updater) checkLoop() { defer u.wg.Done() + // Record that an updater loop is running, so `pilotctl update status` + // can tell "enabled and running" from "enabled but no updater process" + // (e.g. a container or WSL host without systemd). + started := time.Now().UTC() + u.updateStatus(func(s *Status) { + s.LoopPID = os.Getpid() + s.LoopStartedAt = started + }) + // On startup, catch any missed daemon restart from a previous update cycle // (e.g. old macOS updater replaced the binary but never called launchctl). u.recoverPendingRestart() // Run once immediately on start (only if auto-update is enabled). if u.enabled() { - u.checkOnce() + _ = u.runCheck(TriggerAuto) } else { slog.Info("auto-update disabled; loop idle until enabled", "state_path", u.config.StatePath) } + u.markNextCheck() ticker := time.NewTicker(u.config.CheckInterval) defer ticker.Stop() @@ -185,17 +237,73 @@ func (u *Updater) checkLoop() { // Re-read the gate each tick so `pilotctl update enable/disable` // takes effect without restarting the updater. if u.enabled() { - u.checkOnce() + _ = u.runCheck(TriggerAuto) } else { slog.Debug("auto-update disabled; skipping tick") } + u.markNextCheck() case <-u.stopCh: return } } } -func (u *Updater) checkOnce() { +// markNextCheck records when the loop wakes up next. It doubles as the +// loop's heartbeat: a NextCheckAt far in the past means no loop is running. +func (u *Updater) markNextCheck() { + next := time.Now().UTC().Add(u.config.CheckInterval) + u.updateStatus(func(s *Status) { s.NextCheckAt = next }) +} + +// checkOnce runs one automatic (loop) check. See runCheck. +func (u *Updater) checkOnce() error { + return u.runCheck(TriggerAuto) +} + +// runCheck performs one update check, restarts the daemon when an update was +// installed, records the outcome in the status file and returns the check's +// error. +func (u *Updater) runCheck(trigger string) error { + out, err := u.check() + if err != nil { + slog.Error("update check failed", "trigger", trigger, "error", err) + u.recordCheck(trigger, out, err) + return err + } + if out.installed == "" { + u.recordCheck(trigger, out, nil) + return nil + } + slog.Info("update applied successfully", "version", out.installed) + + if out.updaterReplaced && trigger == TriggerAuto { + // This process is now running a stale updater. Record the update, + // then exit so launchd/systemd restarts the process with the new + // binary. On startup the new process runs recoverPendingRestart(), + // which restarts the daemon (and records the result). + u.recordCheck(trigger, out, nil) + slog.Info("updater binary replaced — exiting for process manager to restart with new binary") + exit := u.exitFn + if exit == nil { + exit = os.Exit + } + exit(0) + return nil + } + if out.updaterReplaced { + slog.Info("pilot-updater binary replaced; a running updater service uses it after its next restart") + } + + // Restart the daemon onto the new binaries (SIGTERM / launchctl). + out.restartErr = u.signalDaemonRestart() + u.touchRestartRecord() + u.recordCheck(trigger, out, nil) + return nil +} + +// check fetches the target release (latest, or the pinned tag) and installs +// it when needed. It does not restart anything or exit. +func (u *Updater) check() (checkOutcome, error) { slog.Debug("checking for updates") // Pinned-version path: install a specific version regardless of @@ -203,45 +311,44 @@ func (u *Updater) checkOnce() { // pinned version is installed, subsequent ticks are no-ops until // the pin is changed or cleared. if u.config.PinnedVersion != "" { - u.checkPinnedVersion() - return + return u.checkPinnedVersion() } // Default path: follow the latest release. + var out checkOutcome release, err := u.fetchLatestRelease() if err != nil { - slog.Error("failed to fetch latest release", "error", err) - return + return out, fmt.Errorf("fetch latest release: %w", err) } latest, err := ParseSemver(release.TagName) if err != nil { - slog.Error("failed to parse release tag", "tag", release.TagName, "error", err) - return + return out, fmt.Errorf("parse release tag %q: %w", release.TagName, err) } + out.latest = release.TagName current, err := u.currentVersion() if err != nil { - slog.Error("failed to get current version", "error", err) - return + return out, fmt.Errorf("read installed version: %w", err) } + out.current = current.String() slog.Info("version check", "current", current.String(), "latest", latest.String()) if !latest.NewerThan(current) { slog.Debug("already up to date") - return + return out, nil } slog.Info("new version available, updating", "current", current.String(), "latest", latest.String()) - if err := u.applyUpdate(release); err != nil { - slog.Error("failed to apply update", "error", err) - return + replaced, err := u.applyUpdate(release) + if err != nil { + return out, fmt.Errorf("apply update %s: %w", release.TagName, err) } - - slog.Info("update applied successfully", "version", latest.String()) - u.touchRestartRecord() + out.installed = release.TagName + out.updaterReplaced = replaced + return out, nil } // checkPinnedVersion installs the exact release specified by @@ -249,22 +356,22 @@ func (u *Updater) checkOnce() { // default latest-following path, it does not compare versions — it // fetches the named release and applies it unconditionally when the // current install differs from the pin. -func (u *Updater) checkPinnedVersion() { +func (u *Updater) checkPinnedVersion() (checkOutcome, error) { + out := checkOutcome{latest: u.config.PinnedVersion} pinned, err := ParseSemver(u.config.PinnedVersion) if err != nil { - slog.Error("invalid pinned version", "version", u.config.PinnedVersion, "error", err) - return + return out, fmt.Errorf("invalid pinned version %q: %w", u.config.PinnedVersion, err) } current, err := u.currentVersion() if err != nil { - slog.Error("failed to get current version", "error", err) - return + return out, fmt.Errorf("read installed version: %w", err) } + out.current = current.String() if current == pinned { slog.Info("pinned version already installed", "version", pinned.String()) - return + return out, nil } slog.Info("pinned version requested, installing", @@ -274,17 +381,18 @@ func (u *Updater) checkPinnedVersion() { release, err := u.fetchReleaseByTag(u.config.PinnedVersion) if err != nil { - slog.Error("failed to fetch pinned release", "tag", u.config.PinnedVersion, "error", err) - return + return out, fmt.Errorf("fetch pinned release %s: %w", u.config.PinnedVersion, err) } - if err := u.applyUpdate(release); err != nil { - slog.Error("failed to apply pinned update", "error", err) - return + replaced, err := u.applyUpdate(release) + if err != nil { + return out, fmt.Errorf("apply pinned update %s: %w", u.config.PinnedVersion, err) } slog.Info("pinned version installed", "version", pinned.String()) - u.touchRestartRecord() + out.installed = release.TagName + out.updaterReplaced = replaced + return out, nil } func (u *Updater) fetchLatestRelease() (*GitHubRelease, error) { @@ -316,15 +424,15 @@ func (u *Updater) fetchRelease(tag string) (*GitHubRelease, error) { req.Header.Set("User-Agent", "pilot-updater/"+u.config.Version) } - resp, err := u.client.Do(req) + // Sends GITHUB_TOKEN/GH_TOKEN when set (rate limit only; never required). + resp, err := doGitHubAPI(u.client, req) if err != nil { return nil, fmt.Errorf("HTTP request: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) - return nil, fmt.Errorf("GitHub API returned %d: %s", resp.StatusCode, string(body)) + return nil, githubAPIError("GitHub API", resp) } var release GitHubRelease @@ -352,7 +460,11 @@ func (u *Updater) currentVersion() (Semver, error) { return ParseSemver(strings.TrimSpace(string(data))) } -func (u *Updater) applyUpdate(release *GitHubRelease) error { +// applyUpdate downloads, verifies and installs release into InstallDir. It +// reports whether the pilot-updater binary itself was replaced. It does not +// restart the daemon or exit: runCheck decides that, after recording the +// outcome. +func (u *Updater) applyUpdate(release *GitHubRelease) (updaterReplaced bool, err error) { archiveName := fmt.Sprintf("pilot-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH) var archiveURL, checksumsURL string @@ -366,19 +478,19 @@ func (u *Updater) applyUpdate(release *GitHubRelease) error { } if archiveURL == "" { - return fmt.Errorf("no asset %q in release %s", archiveName, release.TagName) + return false, fmt.Errorf("no asset %q in release %s", archiveName, release.TagName) } tmpDir, err := os.MkdirTemp("", "pilot-update-*") if err != nil { - return fmt.Errorf("create temp dir: %w", err) + return false, fmt.Errorf("create temp dir: %w", err) } defer os.RemoveAll(tmpDir) // Download archive. archivePath := filepath.Join(tmpDir, archiveName) if err := u.downloadFile(archiveURL, archivePath); err != nil { - return fmt.Errorf("download archive: %w", err) + return false, fmt.Errorf("download archive: %w", err) } // Verify checksums. Both the asset's presence in the release AND @@ -396,11 +508,11 @@ func (u *Updater) applyUpdate(release *GitHubRelease) error { // before trusting the checksums file, closing the "matched fake // binary + fake checksums" gap. if checksumsURL == "" { - return fmt.Errorf("release %s has no checksums.txt asset; refusing to install unverified binary", release.TagName) + return false, fmt.Errorf("release %s has no checksums.txt asset; refusing to install unverified binary", release.TagName) } checksumsPath := filepath.Join(tmpDir, "checksums.txt") if err := u.downloadFile(checksumsURL, checksumsPath); err != nil { - return fmt.Errorf("download checksums: %w", err) + return false, fmt.Errorf("download checksums: %w", err) } // Verify checksums.txt provenance via GitHub SLSA attestation. @@ -413,29 +525,28 @@ func (u *Updater) applyUpdate(release *GitHubRelease) error { // still-attested checksums.txt cannot be replayed under a new tag // (validated rollback). Fails closed unless SkipAttestation is set. if err := u.verifyChecksumsAttestation(release.TagName, checksumsPath); err != nil { - return fmt.Errorf("checksums attestation verification failed: %w", err) + return false, fmt.Errorf("checksums attestation verification failed: %w", err) } if err := VerifyChecksum(archivePath, archiveName, checksumsPath); err != nil { - return fmt.Errorf("checksum verification failed: %w", err) + return false, fmt.Errorf("checksum verification failed: %w", err) } slog.Info("checksum verified", "archive", archiveName) // Extract to staging directory. stagingDir := filepath.Join(tmpDir, "staging") if err := os.MkdirAll(stagingDir, 0755); err != nil { - return fmt.Errorf("create staging dir: %w", err) + return false, fmt.Errorf("create staging dir: %w", err) } if err := extractTarGz(archivePath, stagingDir); err != nil { - return fmt.Errorf("extract archive: %w", err) + return false, fmt.Errorf("extract archive: %w", err) } entries, err := os.ReadDir(stagingDir) if err != nil { - return fmt.Errorf("read staging dir: %w", err) + return false, fmt.Errorf("read staging dir: %w", err) } - updaterReplaced := false for _, entry := range entries { if entry.IsDir() { continue @@ -448,7 +559,7 @@ func (u *Updater) applyUpdate(release *GitHubRelease) error { src := filepath.Join(stagingDir, entry.Name()) dst := filepath.Join(u.config.InstallDir, installName) if err := replaceBinary(src, dst); err != nil { - return fmt.Errorf("replace %s: %w", installName, err) + return false, fmt.Errorf("replace %s: %w", installName, err) } slog.Info("replaced binary", "name", installName) if installName == "pilot-updater" { @@ -467,52 +578,7 @@ func (u *Updater) applyUpdate(release *GitHubRelease) error { slog.Warn("failed to write version file", "error", err) } - // If the updater binary itself was replaced, exit so launchd/systemd - // restarts the process with the new binary. On startup the new process - // runs recoverPendingRestart() which will handle the daemon restart. - // Explicitly clean up tmpDir first since defer won't run after os.Exit. - if updaterReplaced { - os.RemoveAll(tmpDir) - slog.Info("updater binary replaced — exiting for process manager to restart with new binary") - u.exitFn(0) - } - - // Signal daemon to restart (SIGTERM for graceful shutdown). - u.signalDaemonRestart() - - return nil -} - -func (u *Updater) downloadFile(url, dst string) error { - resp, err := u.client.Get(url) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("HTTP %d for %s", resp.StatusCode, url) - } - - f, err := os.Create(dst) - if err != nil { - return err - } - defer f.Close() - - // Read one byte past the cap so we can distinguish "exactly at the limit" - // from "exceeded the limit". A plain io.LimitReader(maxDownloadBytes) would - // silently truncate oversize archives — the SHA256 check would then fail - // with a confusing "checksum mismatch" instead of telling the operator - // the archive is too large. - n, err := io.Copy(f, io.LimitReader(resp.Body, maxDownloadBytes+1)) - if err != nil { - return err - } - if n > maxDownloadBytes { - return fmt.Errorf("archive exceeds max download size %d bytes", maxDownloadBytes) - } - return nil + return updaterReplaced, nil } // VerifyChecksum checks the SHA256 of archivePath against the checksums file. @@ -690,8 +756,9 @@ func (u *Updater) recoverPendingRestart() { recordStat, err := os.Stat(restartRecord) if err != nil || binStat.ModTime().After(recordStat.ModTime()) { slog.Info("daemon binary updated since last restart, triggering restart") - u.signalDaemonRestart() + err := u.signalDaemonRestart() u.touchRestartRecord() + u.recordRestart(err) } } @@ -720,59 +787,101 @@ func writeFileSync(path string, data []byte, perm os.FileMode) error { return f.Close() } -func (u *Updater) signalDaemonRestart() { +// signalDaemonRestart restarts the daemon so it runs the newly installed +// binaries. It returns an error when the daemon may still be running the old +// binary; a daemon that is not running at all is not an error (its next +// start uses the new binary). +func (u *Updater) signalDaemonRestart() error { if runtime.GOOS == "darwin" { - u.signalDaemonRestartDarwin() - return + return u.signalDaemonRestartDarwin() + } + return u.signalDaemonRestartLinux() +} + +// command runs an external command via the injectable hook. +func (u *Updater) command(name string, args ...string) ([]byte, error) { + if u.runCmd != nil { + return u.runCmd(name, args...) } - u.signalDaemonRestartLinux() + return runCommand(name, args...) } -func (u *Updater) signalDaemonRestartDarwin() { +func (u *Updater) signalDaemonRestartDarwin() error { // On macOS the daemon is managed by launchd. Use launchctl kickstart -k // to kill the running instance and restart it immediately. The label // matches the plist written by install.sh. uid := os.Getuid() label := "network.pilotprotocol.pilot-daemon" target := fmt.Sprintf("gui/%d/%s", uid, label) - out, err := exec.Command("launchctl", "kickstart", "-k", target).CombinedOutput() + out, err := u.command("launchctl", "kickstart", "-k", target) if err != nil { + output := strings.TrimSpace(string(out)) slog.Warn("launchctl kickstart failed — restart daemon manually", - "target", target, "err", err, "output", strings.TrimSpace(string(out))) - return + "target", target, "err", err, "output", output) + return fmt.Errorf("restart daemon (launchctl kickstart -k %s): %v %s", target, err, output) } slog.Info("daemon restarted via launchctl", "target", target) + return nil } -func (u *Updater) signalDaemonRestartLinux() { +func (u *Updater) signalDaemonRestartLinux() error { // On Linux, find the daemon process via /proc//exe and send SIGTERM. - // systemd Restart=on-failure will relaunch it automatically. + // systemd (Restart=always in the unit install.sh writes) relaunches it on + // the new binary. daemonPath := filepath.Join(u.config.InstallDir, "pilot-daemon") - entries, err := os.ReadDir("/proc") + procRoot := u.procRoot + if procRoot == "" { + procRoot = "/proc" + } + pid, err := findProcessByExe(procRoot, daemonPath) if err != nil { - slog.Warn("cannot read /proc — restart daemon manually") - return + slog.Warn("cannot read /proc — restart daemon manually", "error", err) + return fmt.Errorf("restart daemon: find process: %w", err) + } + if pid == 0 { + // Not running: its next start uses the new binary. + slog.Warn("daemon process not found — restart daemon manually if it is running", "path", daemonPath) + return nil + } + slog.Info("sending SIGTERM to daemon", "pid", pid) + kill := u.killFn + if kill == nil { + kill = syscall.Kill + } + if err := kill(pid, syscall.SIGTERM); err != nil { + return fmt.Errorf("restart daemon: signal pid %d: %w", pid, err) } + return nil +} +// findProcessByExe returns the pid of a process (other than this one) whose +// executable is exePath, or 0 when there is none. After an update the old +// binary has been renamed over, so the running daemon's /proc//exe +// reads " (deleted)". That must match too, or the daemon is never +// restarted onto the new version. +func findProcessByExe(procRoot, exePath string) (int, error) { + entries, err := os.ReadDir(procRoot) + if err != nil { + return 0, err + } + self := os.Getpid() for _, entry := range entries { if !entry.IsDir() { continue } - exe, err := os.Readlink(filepath.Join("/proc", entry.Name(), "exe")) + pid, err := strconv.Atoi(entry.Name()) + if err != nil || pid <= 0 || pid == self { + continue + } + exe, err := os.Readlink(filepath.Join(procRoot, entry.Name(), "exe")) if err != nil { continue } - if exe == daemonPath { - pid := 0 - fmt.Sscanf(entry.Name(), "%d", &pid) - if pid > 0 { - slog.Info("sending SIGTERM to daemon", "pid", pid) - syscall.Kill(pid, syscall.SIGTERM) - return - } + if exe == exePath || exe == exePath+" (deleted)" { + return pid, nil } } - slog.Warn("daemon process not found — restart daemon manually") + return 0, nil } // verifyChecksumsAttestation verifies the SLSA provenance of checksums.txt for diff --git a/zz_checksums_required_bug_test.go b/zz_checksums_required_bug_test.go index 712a762..6878fcc 100644 --- a/zz_checksums_required_bug_test.go +++ b/zz_checksums_required_bug_test.go @@ -63,7 +63,7 @@ func TestApplyUpdate_FailsWhenChecksumsAssetMissing(t *testing.T) { }, } - err := u.applyUpdate(release) + _, err := u.applyUpdate(release) if err == nil { // Confirm the malicious binary was installed unverified. got, _ := os.ReadFile(filepath.Join(installDir, "pilot-daemon")) @@ -129,7 +129,7 @@ func TestApplyUpdate_FailsWhenChecksumsDownloadFails(t *testing.T) { }, } - err := u.applyUpdate(release) + _, err := u.applyUpdate(release) if err == nil { got, _ := os.ReadFile(filepath.Join(installDir, "pilot-daemon")) t.Fatalf( @@ -192,7 +192,7 @@ func TestApplyUpdate_PassesWithValidChecksums(t *testing.T) { }, } - if err := u.applyUpdate(release); err != nil { + if _, err := u.applyUpdate(release); err != nil { t.Fatalf("green-path install failed: %v", err) } } @@ -254,7 +254,7 @@ func TestApplyUpdate_SkipAttestationStillRequiresChecksum(t *testing.T) { stopCh: make(chan struct{}), exitFn: func(int) {}, } - if err := u.applyUpdate(newRelease(srv.URL)); err != nil { + if _, err := u.applyUpdate(newRelease(srv.URL)); err != nil { t.Fatalf("SkipAttestation=true with matching checksum should install: %v", err) } }) @@ -280,7 +280,7 @@ func TestApplyUpdate_SkipAttestationStillRequiresChecksum(t *testing.T) { stopCh: make(chan struct{}), exitFn: func(int) {}, } - err := u.applyUpdate(newRelease(srv.URL)) + _, err := u.applyUpdate(newRelease(srv.URL)) if err == nil { t.Fatal("SkipAttestation=true must still enforce SHA256: mismatch should fail") } diff --git a/zz_coverage_test.go b/zz_coverage_test.go index 801bc4d..d7568e2 100644 --- a/zz_coverage_test.go +++ b/zz_coverage_test.go @@ -343,7 +343,7 @@ func TestApplyUpdate_ArchiveAssetMissing(t *testing.T) { client: http.DefaultClient, stopCh: make(chan struct{}), } - err := u.applyUpdate(&GitHubRelease{ + _, err := u.applyUpdate(&GitHubRelease{ TagName: "v1.0.0", Assets: []GitHubAsset{{Name: "checksums.txt", BrowserDownloadURL: "http://x"}}, }) @@ -367,7 +367,7 @@ func TestApplyUpdate_ArchiveDownloadFails(t *testing.T) { client: srv.Client(), stopCh: make(chan struct{}), } - err := u.applyUpdate(&GitHubRelease{ + _, err := u.applyUpdate(&GitHubRelease{ TagName: "v1.0.0", Assets: []GitHubAsset{ {Name: archiveName, BrowserDownloadURL: srv.URL + "/archive"}, @@ -405,7 +405,7 @@ func TestApplyUpdate_ChecksumMismatch(t *testing.T) { client: srv.Client(), stopCh: make(chan struct{}), } - err := u.applyUpdate(&GitHubRelease{ + _, err := u.applyUpdate(&GitHubRelease{ TagName: "v1.0.0", Assets: []GitHubAsset{ {Name: archiveName, BrowserDownloadURL: srv.URL + "/dl/" + archiveName}, @@ -444,7 +444,7 @@ func TestApplyUpdate_CorruptArchiveExtractFails(t *testing.T) { client: srv.Client(), stopCh: make(chan struct{}), } - err := u.applyUpdate(&GitHubRelease{ + _, err := u.applyUpdate(&GitHubRelease{ TagName: "v1.0.0", Assets: []GitHubAsset{ {Name: archiveName, BrowserDownloadURL: srv.URL + "/dl/" + archiveName}, diff --git a/zz_download_test.go b/zz_download_test.go new file mode 100644 index 0000000..036d89c --- /dev/null +++ b/zz_download_test.go @@ -0,0 +1,380 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "bytes" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" +) + +// payload returns n deterministic bytes. +func payload(n int) []byte { + b := make([]byte, n) + for i := range b { + b[i] = byte(i*7 + i/251) + } + return b +} + +// rangeStart parses "bytes=N-" and returns N (0 when absent). +func rangeStart(t *testing.T, r *http.Request) int { + t.Helper() + h := r.Header.Get("Range") + if h == "" { + return 0 + } + v := strings.TrimSuffix(strings.TrimPrefix(h, "bytes="), "-") + n, err := strconv.Atoi(v) + if err != nil { + t.Errorf("bad Range header %q", h) + } + return n +} + +// servePartial writes body[from:] as a 206 (or a 200 when from == 0). +func servePartial(w http.ResponseWriter, body []byte, from int) { + if from > 0 { + w.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", from, len(body)-1, len(body))) + w.Header().Set("Content-Length", strconv.Itoa(len(body)-from)) + w.WriteHeader(http.StatusPartialContent) + } else { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + } + _, _ = w.Write(body[from:]) +} + +// dropAfter sends the headers for the full body plus its first n bytes, then +// kills the connection, as a flaky link or CDN edge does. +func dropAfter(t *testing.T, w http.ResponseWriter, body []byte, n int) { + t.Helper() + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(body[:n]) + w.(http.Flusher).Flush() + conn, _, err := http.NewResponseController(w).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + conn.Close() +} + +func readFile(t *testing.T, path string) []byte { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return b +} + +// TestDownload_ResumesAfterDroppedConnection: a dropped connection used to +// fail the update and wait an hour. It now resumes with a Range request from +// the bytes already on disk. +func TestDownload_ResumesAfterDroppedConnection(t *testing.T) { + t.Parallel() + body := payload(256 * 1024) + var hits atomic.Int32 + var mu sync.Mutex + var ranges []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + ranges = append(ranges, r.Header.Get("Range")) + mu.Unlock() + if hits.Add(1) == 1 { + dropAfter(t, w, body, 100*1024) + return + } + servePartial(w, body, rangeStart(t, r)) + })) + defer srv.Close() + + u := &Updater{client: srv.Client()} + dst := filepath.Join(t.TempDir(), "archive") + if err := u.downloadFile(srv.URL, dst); err != nil { + t.Fatalf("downloadFile: %v", err) + } + if got := readFile(t, dst); !bytes.Equal(got, body) { + t.Fatalf("downloaded %d bytes, mismatch with %d-byte body", len(got), len(body)) + } + mu.Lock() + defer mu.Unlock() + if len(ranges) != 2 || ranges[0] != "" || ranges[1] != fmt.Sprintf("bytes=%d-", 100*1024) { + t.Errorf("Range headers = %q, want [\"\" \"bytes=102400-\"]", ranges) + } +} + +// TestDownload_ServerIgnoringRangeRestartsCleanly: when the server answers a +// resume with a full 200, the file must be rewritten, not appended to. +func TestDownload_ServerIgnoringRangeRestartsCleanly(t *testing.T) { + t.Parallel() + body := payload(64 * 1024) + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if hits.Add(1) == 1 { + dropAfter(t, w, body, 10*1024) + return + } + servePartial(w, body, 0) // ignores Range + })) + defer srv.Close() + + u := &Updater{client: srv.Client()} + dst := filepath.Join(t.TempDir(), "archive") + if err := u.downloadFile(srv.URL, dst); err != nil { + t.Fatalf("downloadFile: %v", err) + } + if got := readFile(t, dst); !bytes.Equal(got, body) { + t.Fatalf("got %d bytes, want exactly the %d-byte body", len(got), len(body)) + } +} + +// TestDownload_MismatchedContentRangeStartsOver: a 206 that does not start +// where we asked is discarded and the download restarts from byte 0. +func TestDownload_MismatchedContentRangeStartsOver(t *testing.T) { + t.Parallel() + body := payload(32 * 1024) + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch hits.Add(1) { + case 1: + dropAfter(t, w, body, 8*1024) + case 2: + servePartial(w, body, 4*1024) // wrong offset + default: + servePartial(w, body, rangeStart(t, r)) + } + })) + defer srv.Close() + + u := &Updater{client: srv.Client()} + dst := filepath.Join(t.TempDir(), "archive") + if err := u.downloadFile(srv.URL, dst); err != nil { + t.Fatalf("downloadFile: %v", err) + } + if got := readFile(t, dst); !bytes.Equal(got, body) { + t.Fatalf("got %d bytes, want exactly the %d-byte body", len(got), len(body)) + } +} + +// TestDownload_SlowButSteadyIsNotCutOff: the old client had a 30 s TOTAL +// timeout, so a 19 MB archive needed ~5 Mbit/s. Scaled down: the API client +// here times out after 300 ms, yet a download that keeps trickling for ~1 s +// must complete, because downloads are bounded by idleness, not duration. +func TestDownload_SlowButSteadyIsNotCutOff(t *testing.T) { + t.Parallel() + body := payload(10 * 1024) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + w.WriteHeader(http.StatusOK) + for i := 0; i < 10; i++ { + _, _ = w.Write(body[i*1024 : (i+1)*1024]) + w.(http.Flusher).Flush() + time.Sleep(100 * time.Millisecond) + } + })) + defer srv.Close() + + api := srv.Client() + api.Timeout = 300 * time.Millisecond + u := &Updater{client: api, dlIdleTimeout: 2 * time.Second} + dst := filepath.Join(t.TempDir(), "archive") + if err := u.downloadFile(srv.URL, dst); err != nil { + t.Fatalf("steady ~1s download failed under a 300ms API timeout: %v", err) + } + if got := readFile(t, dst); !bytes.Equal(got, body) { + t.Fatal("content mismatch") + } +} + +// TestDownload_StallIsRetriedThenResumed: a stream that stops sending is +// cut after the idle timeout and resumed. +func TestDownload_StallIsRetriedThenResumed(t *testing.T) { + t.Parallel() + body := payload(20 * 1024) + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if hits.Add(1) == 1 { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(body[:5*1024]) + w.(http.Flusher).Flush() + <-r.Context().Done() // stall until the client gives up + return + } + servePartial(w, body, rangeStart(t, r)) + })) + defer srv.Close() + + u := &Updater{client: srv.Client(), dlIdleTimeout: 200 * time.Millisecond} + dst := filepath.Join(t.TempDir(), "archive") + start := time.Now() + if err := u.downloadFile(srv.URL, dst); err != nil { + t.Fatalf("downloadFile: %v", err) + } + if el := time.Since(start); el > 5*time.Second { + t.Errorf("stall recovery took %v", el) + } + if got := readFile(t, dst); !bytes.Equal(got, body) { + t.Fatal("content mismatch after resume") + } + if hits.Load() != 2 { + t.Errorf("requests = %d, want 2", hits.Load()) + } +} + +// TestDownload_PersistentStallFailsWithClearError: after the last attempt +// the error says the download stalled, not "context canceled". +func TestDownload_PersistentStallFailsWithClearError(t *testing.T) { + t.Parallel() + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Length", "1000") + w.WriteHeader(http.StatusOK) + w.(http.Flusher).Flush() + <-r.Context().Done() + })) + defer srv.Close() + + u := &Updater{client: srv.Client(), dlIdleTimeout: 100 * time.Millisecond} + err := u.downloadFile(srv.URL, filepath.Join(t.TempDir(), "archive")) + if !errors.Is(err, errDownloadStalled) { + t.Fatalf("err = %v, want errDownloadStalled", err) + } + if !strings.Contains(err.Error(), "no data received for 100ms") { + t.Errorf("error %q does not explain the stall", err) + } + if hits.Load() != downloadMaxAttempts { + t.Errorf("attempts = %d, want %d", hits.Load(), downloadMaxAttempts) + } +} + +// TestDownload_ClientErrorsAreNotRetried: a 404 will not fix itself. +func TestDownload_ClientErrorsAreNotRetried(t *testing.T) { + t.Parallel() + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + http.NotFound(w, r) + })) + defer srv.Close() + + u := &Updater{client: srv.Client()} + err := u.downloadFile(srv.URL, filepath.Join(t.TempDir(), "archive")) + if err == nil || !strings.Contains(err.Error(), "HTTP 404") { + t.Fatalf("err = %v, want HTTP 404", err) + } + if hits.Load() != 1 { + t.Errorf("attempts = %d, want 1", hits.Load()) + } +} + +// TestDownload_ServerErrorsAreRetried: a 503 from a CDN edge is transient. +func TestDownload_ServerErrorsAreRetried(t *testing.T) { + t.Parallel() + body := payload(4096) + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if hits.Add(1) == 1 { + http.Error(w, "busy", http.StatusServiceUnavailable) + return + } + servePartial(w, body, 0) + })) + defer srv.Close() + + u := &Updater{client: srv.Client()} + dst := filepath.Join(t.TempDir(), "archive") + if err := u.downloadFile(srv.URL, dst); err != nil { + t.Fatalf("downloadFile: %v", err) + } + if got := readFile(t, dst); !bytes.Equal(got, body) { + t.Fatal("content mismatch") + } +} + +// TestDownload_StopAbortsPromptly: Stop (SIGTERM from the service manager) +// must not wait out a slow download. +func TestDownload_StopAbortsPromptly(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", "1000") + w.WriteHeader(http.StatusOK) + w.(http.Flusher).Flush() + <-r.Context().Done() + })) + defer srv.Close() + + u := &Updater{client: srv.Client(), stopCh: make(chan struct{}), dlIdleTimeout: time.Minute} + time.AfterFunc(100*time.Millisecond, func() { close(u.stopCh) }) + start := time.Now() + err := u.downloadFile(srv.URL, filepath.Join(t.TempDir(), "archive")) + if err == nil || !strings.Contains(err.Error(), "updater stopping") { + t.Fatalf("err = %v, want an abort because the updater is stopping", err) + } + if el := time.Since(start); el > 5*time.Second { + t.Errorf("stop took %v to abort the download", el) + } +} + +// TestNew_ClientsTimeouts pins the split: short total timeout for API calls, +// none for downloads (idle + max-duration bounds apply instead). +func TestNew_ClientsTimeouts(t *testing.T) { + t.Parallel() + u := New(Config{}) + if u.client.Timeout != apiTimeout { + t.Errorf("API client timeout = %v, want %v", u.client.Timeout, apiTimeout) + } + if u.dlClient == nil || u.dlClient.Timeout != 0 { + t.Errorf("download client must have no total timeout, got %+v", u.dlClient) + } + if u.downloadClient() != u.dlClient { + t.Error("downloadClient() should return dlClient when set") + } + tr, ok := u.dlClient.Transport.(*http.Transport) + if !ok || tr.TLSHandshakeTimeout < 20*time.Second || tr.ResponseHeaderTimeout == 0 { + t.Errorf("download transport timeouts not set: %+v", u.dlClient.Transport) + } + // A literal Updater (as tests build) derives a download client from + // client with the total timeout removed. + lit := &Updater{client: &http.Client{Timeout: time.Second}} + if lit.downloadClient().Timeout != 0 { + t.Error("derived download client kept the total timeout") + } + if (&Updater{}).downloadClient() == nil { + t.Error("nil client must still yield a download client") + } +} + +func TestContentRangeStart(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + in string + want int64 + ok bool + }{ + {"bytes 100-199/200", 100, true}, + {" bytes 0-9/10 ", 0, true}, + {"bytes */200", 0, false}, + {"items 1-2/3", 0, false}, + {"", 0, false}, + {"bytes -5-9/10", 0, false}, + } { + got, ok := contentRangeStart(tc.in) + if got != tc.want || ok != tc.ok { + t.Errorf("contentRangeStart(%q) = %d,%v want %d,%v", tc.in, got, ok, tc.want, tc.ok) + } + } +} diff --git a/zz_fuzz_evals_test.go b/zz_fuzz_evals_test.go index 23a38ed..960dffe 100644 --- a/zz_fuzz_evals_test.go +++ b/zz_fuzz_evals_test.go @@ -160,7 +160,7 @@ func TestApplyUpdate_TamperedArchiveRefused(t *testing.T) { }, } - err := u.applyUpdate(release) + _, err := u.applyUpdate(release) if err == nil { t.Fatal("tampered archive (bytes swapped, checksums valid for original) must be refused") } diff --git a/zz_github_test.go b/zz_github_test.go new file mode 100644 index 0000000..2d24180 --- /dev/null +++ b/zz_github_test.go @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync/atomic" + "testing" + "time" +) + +// These tests set GITHUB_TOKEN/GH_TOKEN with t.Setenv, so none of them may +// run in parallel. + +func newAPIUpdater(srv *httptest.Server) *Updater { + return &Updater{ + config: Config{Repo: "owner/repo", Version: "vTEST"}, + client: newRewriteClient(srv), // requests keep Host api.github.com + stopCh: make(chan struct{}), + } +} + +// TestFetchRelease_SendsGitHubToken: the releases call used to be +// unauthenticated even when a token was available, so a shared NAT/VPN +// egress hit the 60/hour limit and updates silently stopped. +func TestFetchRelease_SendsGitHubToken(t *testing.T) { + t.Setenv("GH_TOKEN", "") + t.Setenv("GITHUB_TOKEN", "tok-123") + var auth atomic.Value + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + auth.Store(r.Header.Get("Authorization")) + _ = json.NewEncoder(w).Encode(GitHubRelease{TagName: "v1.0.0"}) + })) + defer srv.Close() + + if _, err := newAPIUpdater(srv).fetchLatestRelease(); err != nil { + t.Fatalf("fetchLatestRelease: %v", err) + } + if got := auth.Load(); got != "Bearer tok-123" { + t.Errorf("Authorization = %q, want Bearer tok-123", got) + } +} + +// TestFetchRelease_GHTokenAlias: gh's GH_TOKEN works too. +func TestFetchRelease_GHTokenAlias(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "gh-tok") + var auth atomic.Value + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + auth.Store(r.Header.Get("Authorization")) + _ = json.NewEncoder(w).Encode(GitHubRelease{TagName: "v1.0.0"}) + })) + defer srv.Close() + + if _, err := newAPIUpdater(srv).fetchReleaseByTag("v1.0.0"); err != nil { + t.Fatalf("fetchReleaseByTag: %v", err) + } + if got := auth.Load(); got != "Bearer gh-tok" { + t.Errorf("Authorization = %q, want Bearer gh-tok", got) + } +} + +// TestFetchRelease_NoTokenNoHeader: without a token nothing is sent. +func TestFetchRelease_NoTokenNoHeader(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + var auth atomic.Value + auth.Store("unset") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + auth.Store(r.Header.Get("Authorization")) + _ = json.NewEncoder(w).Encode(GitHubRelease{TagName: "v1.0.0"}) + })) + defer srv.Close() + + if _, err := newAPIUpdater(srv).fetchLatestRelease(); err != nil { + t.Fatalf("fetchLatestRelease: %v", err) + } + if got := auth.Load(); got != "" { + t.Errorf("Authorization = %q, want none", got) + } +} + +// TestFetchRelease_RejectedTokenFallsBack: an expired or revoked token must +// not block updates of a public repo; the call is retried without it. +func TestFetchRelease_RejectedTokenFallsBack(t *testing.T) { + t.Setenv("GH_TOKEN", "") + t.Setenv("GITHUB_TOKEN", "revoked") + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + if r.Header.Get("Authorization") != "" { + http.Error(w, `{"message":"Bad credentials"}`, http.StatusUnauthorized) + return + } + _ = json.NewEncoder(w).Encode(GitHubRelease{TagName: "v1.2.3"}) + })) + defer srv.Close() + + rel, err := newAPIUpdater(srv).fetchLatestRelease() + if err != nil { + t.Fatalf("fetchLatestRelease with a rejected token: %v", err) + } + if rel.TagName != "v1.2.3" || hits.Load() != 2 { + t.Errorf("tag=%q hits=%d, want v1.2.3 after 2 requests", rel.TagName, hits.Load()) + } +} + +// TestGitHubToken_NeverSentToOtherHosts: the token goes to api.github.com +// only — not to a mirror, a test server or an asset host. +func TestGitHubToken_NeverSentToOtherHosts(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "secret") + var auth atomic.Value + auth.Store("unset") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + auth.Store(r.Header.Get("Authorization")) + _, _ = w.Write([]byte(`{"attestations":[]}`)) + })) + defer srv.Close() + + req, _ := http.NewRequest(http.MethodGet, srv.URL+"/repos/o/r/attestations/sha256:00", nil) + resp, err := doGitHubAPI(srv.Client(), req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if got := auth.Load(); got != "" { + t.Errorf("token leaked to %s: Authorization = %q", srv.URL, got) + } +} + +// TestFetchRelease_RateLimitErrorIsActionable: the 403 says it is a rate +// limit, when it resets, and how to raise it. +func TestFetchRelease_RateLimitErrorIsActionable(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + reset := time.Date(2026, 9, 24, 13, 0, 0, 0, time.UTC) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(reset.Unix(), 10)) + http.Error(w, `{"message":"API rate limit exceeded for 203.0.113.9."}`, http.StatusForbidden) + })) + defer srv.Close() + + _, err := newAPIUpdater(srv).fetchLatestRelease() + if err == nil { + t.Fatal("expected an error") + } + for _, want := range []string{"returned 403", "rate limit exceeded", "resets at 2026-09-24T13:00:00Z", "set GITHUB_TOKEN"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not contain %q", err, want) + } + } + + // With a token already set, don't tell the user to set one. + t.Setenv("GITHUB_TOKEN", "tok") + _, err = newAPIUpdater(srv).fetchLatestRelease() + if err == nil || strings.Contains(err.Error(), "set GITHUB_TOKEN") { + t.Errorf("with a token set, error = %v", err) + } +} + +// TestGitHubAPIError_SecondaryLimitAndPlainErrors covers the 429 "abuse +// detection" body and a non-rate-limit 403. +func TestGitHubAPIError_SecondaryLimitAndPlainErrors(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + mk := func(code int, body string) *http.Response { + rec := httptest.NewRecorder() + rec.WriteHeader(code) + _, _ = rec.WriteString(body) + return rec.Result() + } + err := githubAPIError("GitHub API", mk(http.StatusTooManyRequests, "You have triggered an abuse detection mechanism")) + if !strings.Contains(err.Error(), "returned 429 (rate limit exceeded") { + t.Errorf("429 secondary limit: %v", err) + } + err = githubAPIError("GitHub API", mk(http.StatusForbidden, "Resource not accessible")) + if strings.Contains(err.Error(), "rate limit") || !strings.Contains(err.Error(), "returned 403: Resource not accessible") { + t.Errorf("plain 403: %v", err) + } + err = githubAPIError("GitHub API", mk(http.StatusNotFound, "Not Found")) + if err.Error() != "GitHub API returned 404: Not Found" { + t.Errorf("404: %v", err) + } +} diff --git a/zz_no_gh_test.go b/zz_no_gh_test.go new file mode 100644 index 0000000..d070d22 --- /dev/null +++ b/zz_no_gh_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// TestNoExternalToolDependency guards against a repeat of updater v0.2.3, +// which required the `gh` CLI for attestation verification. Service managers +// never have gh on their PATH (launchd's default PATH is +// /usr/bin:/bin:/usr/sbin:/sbin), so every node on pilotprotocol +// v1.12.2–v1.13.4 refused every update for as long as it ran that updater. +// +// The updater must verify and apply releases with no external tools. The one +// allowed exec is `launchctl` (restarting the daemon under launchd on +// macOS), and it must go through runCommand so tests can stub it. This test +// parses the package's non-test sources and fails on any other exec. +func TestNoExternalToolDependency(t *testing.T) { + t.Parallel() + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + fset := token.NewFileSet() + var execCalls int + for _, name := range files { + if strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + ast.Inspect(f, func(n ast.Node) bool { + switch n := n.(type) { + case *ast.BasicLit: + if n.Kind == token.STRING { + if v, _ := strconv.Unquote(n.Value); v == "gh" { + t.Errorf("%s: string literal \"gh\" — the updater must not depend on the gh CLI", fset.Position(n.Pos())) + } + } + case *ast.CallExpr: + sel, ok := n.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok || pkg.Name != "exec" { + return true + } + execCalls++ + switch sel.Sel.Name { + case "Command", "CommandContext", "LookPath": + default: + return true + } + // The single allowed exec lives in runCommand and takes its + // program name from the caller. + if fn := enclosingVar(f, n); fn != "runCommand" { + t.Errorf("%s: exec.%s outside runCommand; the updater must not run external tools", + fset.Position(n.Pos()), sel.Sel.Name) + } + } + return true + }) + } + if execCalls != 1 { + t.Errorf("found %d exec.* calls, want exactly 1 (runCommand)", execCalls) + } + + // Every command actually run goes through Updater.command; the only + // program named anywhere is launchctl. + src, err := os.ReadFile("updater.go") + if err != nil { + t.Fatal(err) + } + if n := strings.Count(string(src), `u.command(`); n != 1 || !strings.Contains(string(src), `u.command("launchctl"`) { + t.Errorf("expected exactly one u.command(\"launchctl\", ...) call, found %d u.command calls", n) + } +} + +// enclosingVar returns the name of the package-level var whose initializer +// contains node, or "". +func enclosingVar(f *ast.File, node ast.Node) string { + for _, decl := range f.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.VAR { + continue + } + for _, spec := range gd.Specs { + vs := spec.(*ast.ValueSpec) + for i, v := range vs.Values { + if v.Pos() <= node.Pos() && node.End() <= v.End() && i < len(vs.Names) { + return vs.Names[i].Name + } + } + } + } + return "" +} diff --git a/zz_restart_test.go b/zz_restart_test.go new file mode 100644 index 0000000..6cc0eda --- /dev/null +++ b/zz_restart_test.go @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "errors" + "os" + "path/filepath" + "strings" + "syscall" + "testing" +) + +// fakeProc builds a /proc-like tree: pid -> exe symlink target. +func fakeProc(t *testing.T, procs map[string]string) string { + t.Helper() + root := t.TempDir() + for pid, exe := range procs { + dir := filepath.Join(root, pid) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if exe != "" { + if err := os.Symlink(exe, filepath.Join(dir, "exe")); err != nil { + t.Fatal(err) + } + } + } + // Non-pid noise that real /proc has. + if err := os.WriteFile(filepath.Join(root, "uptime"), []byte("1 1"), 0o644); err != nil { + t.Fatal(err) + } + return root +} + +// TestFindProcessByExe_MatchesReplacedBinary: right after an update the +// daemon's /proc//exe reads " (deleted)" (its inode was renamed +// over). The old exact-match check never found it, so Linux daemons kept +// running the old version after every auto-update. +func TestFindProcessByExe_MatchesReplacedBinary(t *testing.T) { + t.Parallel() + daemon := "/home/u/.pilot/bin/pilot-daemon" + root := fakeProc(t, map[string]string{ + "100": "/usr/bin/bash", + "200": daemon + " (deleted)", + "self": daemon, // non-numeric: ignored + "300": "", // no exe link (kernel thread) + }) + pid, err := findProcessByExe(root, daemon) + if err != nil || pid != 200 { + t.Fatalf("findProcessByExe = %d, %v; want 200", pid, err) + } +} + +func TestFindProcessByExe_ExactAndMissing(t *testing.T) { + t.Parallel() + daemon := "/opt/pilot/pilot-daemon" + root := fakeProc(t, map[string]string{"55": daemon, "56": "/opt/pilot/pilot-daemon-old"}) + if pid, err := findProcessByExe(root, daemon); err != nil || pid != 55 { + t.Errorf("exact match = %d, %v; want 55", pid, err) + } + if pid, err := findProcessByExe(root, "/nowhere/pilot-daemon"); err != nil || pid != 0 { + t.Errorf("no match = %d, %v; want 0, nil", pid, err) + } + if _, err := findProcessByExe(filepath.Join(root, "missing"), daemon); err == nil { + t.Error("unreadable proc root must be an error") + } +} + +// TestSignalDaemonRestartLinux_Paths covers signal success, signal failure, +// daemon not running (not an error) and unreadable /proc (an error). +func TestSignalDaemonRestartLinux_Paths(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + daemon := filepath.Join(installDir, "pilot-daemon") + root := fakeProc(t, map[string]string{"77": daemon + " (deleted)"}) + + var gotPid int + var gotSig syscall.Signal + u := &Updater{ + config: Config{InstallDir: installDir}, + procRoot: root, + killFn: func(pid int, sig syscall.Signal) error { + gotPid, gotSig = pid, sig + return nil + }, + } + if err := u.signalDaemonRestartLinux(); err != nil { + t.Fatalf("signalDaemonRestartLinux: %v", err) + } + if gotPid != 77 || gotSig != syscall.SIGTERM { + t.Errorf("signalled pid %d with %v, want 77 SIGTERM", gotPid, gotSig) + } + + u.killFn = func(int, syscall.Signal) error { return errors.New("operation not permitted") } + if err := u.signalDaemonRestartLinux(); err == nil || !strings.Contains(err.Error(), "operation not permitted") { + t.Errorf("kill failure = %v, want reported", err) + } + + u.procRoot = fakeProc(t, nil) + if err := u.signalDaemonRestartLinux(); err != nil { + t.Errorf("daemon not running must not be an error, got %v", err) + } + + u.procRoot = filepath.Join(root, "does-not-exist") + if err := u.signalDaemonRestartLinux(); err == nil { + t.Error("unreadable /proc must be reported") + } +} + +// TestSignalDaemonRestartDarwin_Paths: the launchctl invocation and its +// failure are reported (a daemon started by hand, not by launchd, keeps +// running the old binary). +func TestSignalDaemonRestartDarwin_Paths(t *testing.T) { + t.Parallel() + var got []string + u := &Updater{ + config: Config{InstallDir: t.TempDir()}, + runCmd: func(name string, args ...string) ([]byte, error) { + got = append([]string{name}, args...) + return nil, nil + }, + } + if err := u.signalDaemonRestartDarwin(); err != nil { + t.Fatalf("signalDaemonRestartDarwin: %v", err) + } + if len(got) != 4 || got[0] != "launchctl" || got[1] != "kickstart" || got[2] != "-k" || + !strings.HasSuffix(got[3], "/network.pilotprotocol.pilot-daemon") { + t.Errorf("command = %q", got) + } + + u.runCmd = func(string, ...string) ([]byte, error) { + return []byte("Could not find service"), errors.New("exit status 113") + } + err := u.signalDaemonRestartDarwin() + if err == nil || !strings.Contains(err.Error(), "Could not find service") || !strings.Contains(err.Error(), "exit status 113") { + t.Errorf("launchctl failure = %v, want reported with output", err) + } +} diff --git a/zz_status_test.go b/zz_status_test.go new file mode 100644 index 0000000..574aff7 --- /dev/null +++ b/zz_status_test.go @@ -0,0 +1,565 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "crypto/sha256" + "encoding/json" + "errors" + "fmt" + "io/fs" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" +) + +// restartSpy captures daemon-restart attempts on both platforms: launchctl +// on macOS (runCmd) and SIGTERM via a fake /proc on Linux (killFn). The fake +// /proc holds one daemon whose exe reads " (deleted)", which is what a +// real daemon looks like right after its binary was renamed over. +type restartSpy struct { + mu sync.Mutex + calls []string + fail error +} + +func (r *restartSpy) install(t *testing.T, u *Updater) { + t.Helper() + u.runCmd = func(name string, args ...string) ([]byte, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls = append(r.calls, name+" "+strings.Join(args, " ")) + if r.fail != nil { + return []byte("launchd says no"), r.fail + } + return nil, nil + } + proc := t.TempDir() + if err := os.MkdirAll(filepath.Join(proc, "3999999"), 0o755); err != nil { + t.Fatal(err) + } + exe := filepath.Join(u.config.InstallDir, "pilot-daemon") + " (deleted)" + if err := os.Symlink(exe, filepath.Join(proc, "3999999", "exe")); err != nil { + t.Fatal(err) + } + u.procRoot = proc + u.killFn = func(pid int, sig syscall.Signal) error { + r.mu.Lock() + defer r.mu.Unlock() + r.calls = append(r.calls, fmt.Sprintf("kill %d %d", pid, sig)) + return r.fail + } +} + +func (r *restartSpy) count() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.calls) +} + +// releaseServer serves a GitHub-like API for owner/repo whose latest release +// is tag, with an archive containing files (plus checksums.txt). apiStatus, +// when non-zero, makes the release endpoints fail with that status instead. +type releaseServer struct { + *httptest.Server + apiStatus atomic.Int32 + apiHits atomic.Int32 +} + +func newReleaseServer(t *testing.T, tag string, files map[string]string) *releaseServer { + t.Helper() + archiveName := fmt.Sprintf("pilot-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH) + archivePath := filepath.Join(t.TempDir(), archiveName) + createTestTarGz(t, archivePath, files) + archive, err := os.ReadFile(archivePath) + if err != nil { + t.Fatal(err) + } + checksums := fmt.Sprintf("%x %s\n", sha256.Sum256(archive), archiveName) + + rs := &releaseServer{} + rs.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/repos/owner/repo/releases/latest", "/repos/owner/repo/releases/tags/" + tag: + rs.apiHits.Add(1) + if st := rs.apiStatus.Load(); st != 0 { + w.Header().Set("X-RateLimit-Remaining", "0") + http.Error(w, `{"message":"API rate limit exceeded for 203.0.113.9."}`, int(st)) + return + } + _ = json.NewEncoder(w).Encode(GitHubRelease{ + TagName: tag, + Assets: []GitHubAsset{ + {Name: archiveName, BrowserDownloadURL: "https://api.github.com/dl/" + archiveName}, + {Name: "checksums.txt", BrowserDownloadURL: "https://api.github.com/dl/checksums.txt"}, + }, + }) + case "/dl/" + archiveName: + _, _ = w.Write(archive) + case "/dl/checksums.txt": + _, _ = w.Write([]byte(checksums)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(rs.Close) + return rs +} + +// newTestUpdater returns an Updater wired to srv with a status file in a +// temp dir and an install dir seeded at version. +func newTestUpdater(t *testing.T, srv *httptest.Server, version string) (*Updater, string) { + t.Helper() + installDir := t.TempDir() + seedInstallDir(t, installDir, version) + statusPath := filepath.Join(t.TempDir(), StatusFileName) + u := &Updater{ + config: Config{ + Repo: "owner/repo", + InstallDir: installDir, + Version: "v0.3.0-test", + StatusPath: statusPath, + CheckInterval: time.Hour, + }, + client: newRewriteClient(srv), + stopCh: make(chan struct{}), + exitFn: func(int) { t.Error("unexpected exit") }, + } + return u, statusPath +} + +func mustReadStatus(t *testing.T, path string) Status { + t.Helper() + s, err := ReadStatus(path) + if err != nil { + t.Fatalf("ReadStatus(%s): %v", path, err) + } + return s +} + +// TestRunOnce_FailureIsReturnedAndRecorded is the core of the "update +// failures are invisible" fix: a GitHub 403 used to be logged and swallowed +// (pilotctl printed status ok, exit 0). RunOnce now returns the error and the +// status file records it, with a failure streak that a success resets. +func TestRunOnce_FailureIsReturnedAndRecorded(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + srv := newReleaseServer(t, "v1.0.0", map[string]string{"daemon": "d"}) + srv.apiStatus.Store(http.StatusForbidden) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + + for i := 1; i <= 2; i++ { + err := u.RunOnce() + if err == nil { + t.Fatalf("run %d: RunOnce returned nil for a GitHub 403", i) + } + for _, want := range []string{"fetch latest release", "returned 403", "rate limit exceeded", "GITHUB_TOKEN"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("run %d: error %q does not mention %q", i, err, want) + } + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultFailed || st.ConsecutiveFailures != i { + t.Errorf("run %d: status = %s/%d failures, want failed/%d", i, st.LastResult, st.ConsecutiveFailures, i) + } + if st.LastError != err.Error() { + t.Errorf("run %d: last_error = %q, want %q", i, st.LastError, err) + } + if st.LastCheckTrigger != TriggerManual { + t.Errorf("run %d: trigger = %q, want manual", i, st.LastCheckTrigger) + } + if !st.LastSuccessAt.IsZero() { + t.Errorf("run %d: last_success_at set without a success: %v", i, st.LastSuccessAt) + } + } + + // GitHub recovers: the streak resets and the result is up to date. + srv.apiStatus.Store(0) + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce after recovery: %v", err) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultUpToDate || st.ConsecutiveFailures != 0 || st.LastError != "" { + t.Errorf("after recovery: %+v", st) + } + if st.LastSuccessAt.IsZero() || st.CurrentVersion != "v1.0.0" || st.LatestVersion != "v1.0.0" { + t.Errorf("after recovery: success/version fields not set: %+v", st) + } + if st.UpdaterVersion != "v0.3.0-test" || st.Repo != "owner/repo" { + t.Errorf("identity fields = %q/%q", st.UpdaterVersion, st.Repo) + } + if got := u.LastStatus(); got.LastResult != ResultUpToDate || !got.LastCheckAt.Equal(st.LastCheckAt) { + t.Errorf("LastStatus() = %+v, want the persisted record", got) + } +} + +// TestRunOnce_UpdateRecordsStatusAndRestartsDaemon checks the success path: +// the update is installed, the daemon restart is requested and the status +// file says what was installed. +func TestRunOnce_UpdateRecordsStatusAndRestartsDaemon(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{"daemon": "new-daemon"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + spy := &restartSpy{} + spy.install(t, u) + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if spy.count() != 1 { + t.Errorf("daemon restart requests = %d (%v), want 1", spy.count(), spy.calls) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultUpdated || st.CurrentVersion != "v2.0.0" || st.LatestVersion != "v2.0.0" { + t.Errorf("status = %+v", st) + } + if st.LastUpdateVersion != "v2.0.0" || st.LastUpdateAt.IsZero() || st.RestartError != "" { + t.Errorf("update fields = %q/%v/%q", st.LastUpdateVersion, st.LastUpdateAt, st.RestartError) + } +} + +// TestRunOnce_RestartFailureIsRecorded: the binaries were installed but the +// daemon could not be restarted onto them. RunOnce still succeeds (the +// update is on disk) but the status says the daemon is on the old binary. +func TestRunOnce_RestartFailureIsRecorded(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{"daemon": "new-daemon"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + spy := &restartSpy{fail: errors.New("permission denied")} + spy.install(t, u) + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultUpdated { + t.Errorf("last_result = %q, want updated", st.LastResult) + } + if !strings.Contains(st.RestartError, "permission denied") { + t.Errorf("restart_error = %q, want the restart failure", st.RestartError) + } +} + +// TestRunOnce_DoesNotExitWhenUpdaterReplaced: RunOnce is called in-process +// by `pilotctl update`. When the release replaced pilot-updater it used to +// os.Exit(0) the pilotctl process before it printed anything and without +// restarting the daemon. It must return normally and restart the daemon. +func TestRunOnce_DoesNotExitWhenUpdaterReplaced(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{ + "daemon": "new-daemon", + "updater": "new-updater", + }) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + spy := &restartSpy{} + spy.install(t, u) + var exited atomic.Bool + u.exitFn = func(int) { exited.Store(true) } + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if exited.Load() { + t.Fatal("RunOnce exited the calling process") + } + if spy.count() != 1 { + t.Errorf("daemon restart requests = %d, want 1", spy.count()) + } + if got, _ := os.ReadFile(filepath.Join(u.config.InstallDir, "pilot-updater")); string(got) != "new-updater" { + t.Errorf("pilot-updater = %q, want new-updater", got) + } + if st := mustReadStatus(t, statusPath); st.LastResult != ResultUpdated { + t.Errorf("last_result = %q, want updated", st.LastResult) + } +} + +// TestLoop_RecordsUpdateBeforeSelfExit: the loop still exits after replacing +// its own binary (so the service manager restarts it), but only after the +// update has been recorded, and it leaves the daemon restart to the new +// process (recoverPendingRestart). +func TestLoop_RecordsUpdateBeforeSelfExit(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{ + "daemon": "new-daemon", + "updater": "new-updater", + }) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + spy := &restartSpy{} + spy.install(t, u) + var atExit Status + var exits atomic.Int32 + u.exitFn = func(code int) { + exits.Add(1) + if code != 0 { + t.Errorf("exit code = %d, want 0", code) + } + atExit = mustReadStatus(t, statusPath) + } + + if err := u.checkOnce(); err != nil { + t.Fatalf("checkOnce: %v", err) + } + if exits.Load() != 1 { + t.Fatalf("exit calls = %d, want 1", exits.Load()) + } + if atExit.LastResult != ResultUpdated || atExit.LastCheckTrigger != TriggerAuto || atExit.CurrentVersion != "v2.0.0" { + t.Errorf("status at exit = %+v, want updated/auto/v2.0.0", atExit) + } + if spy.count() != 0 { + t.Errorf("stale updater restarted the daemon (%v); the new process must do it", spy.calls) + } +} + +// TestRecoverPendingRestart_RecordsRestartResult: the restart made by the +// new updater process after a self-update is reflected in restart_error. +func TestRecoverPendingRestart_RecordsRestartResult(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + statusPath := filepath.Join(t.TempDir(), StatusFileName) + if err := os.WriteFile(filepath.Join(installDir, ".pilot-version"), []byte("v2.0.0\n"), 0o644); err != nil { + t.Fatal(err) + } + // Daemon binary newer than the (absent) restart record → restart due. + if err := os.WriteFile(filepath.Join(installDir, "pilot-daemon"), []byte("d"), 0o755); err != nil { + t.Fatal(err) + } + u := &Updater{config: Config{InstallDir: installDir, StatusPath: statusPath}} + spy := &restartSpy{fail: errors.New("no such service")} + spy.install(t, u) + + u.recoverPendingRestart() + if st := mustReadStatus(t, statusPath); !strings.Contains(st.RestartError, "no such service") { + t.Fatalf("restart_error = %q, want recorded failure", st.RestartError) + } + + // Next binary update, restart succeeds → error cleared. + spy.fail = nil + future := time.Now().Add(time.Hour) + if err := os.Chtimes(filepath.Join(installDir, "pilot-daemon"), future, future); err != nil { + t.Fatal(err) + } + u.recoverPendingRestart() + if st := mustReadStatus(t, statusPath); st.RestartError != "" { + t.Errorf("restart_error = %q, want cleared after a successful restart", st.RestartError) + } +} + +// TestRunOnce_InvalidPinReturnsError: a bad --pin used to be logged only. +func TestRunOnce_InvalidPinReturnsError(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v1.0.0", map[string]string{"daemon": "d"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + u.config.PinnedVersion = "not-a-version" + + err := u.RunOnce() + if err == nil || !strings.Contains(err.Error(), "invalid pinned version") { + t.Fatalf("RunOnce = %v, want invalid pinned version error", err) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultFailed || st.PinnedVersion != "not-a-version" { + t.Errorf("status = %+v", st) + } +} + +// TestRunOnce_PinnedAlreadyInstalledIsUpToDate covers the pinned no-op path. +func TestRunOnce_PinnedAlreadyInstalledIsUpToDate(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v1.5.0", map[string]string{"daemon": "d"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.5.0") + u.config.PinnedVersion = "v1.5.0" + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if srv.apiHits.Load() != 0 { + t.Errorf("pinned+installed made %d API calls, want 0", srv.apiHits.Load()) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultUpToDate || st.LatestVersion != "v1.5.0" || st.PinnedVersion != "v1.5.0" { + t.Errorf("status = %+v", st) + } +} + +// TestRunOnce_ApplyFailureIsReturned: a release without an asset for this +// platform is a failed check, not a silent no-op. +func TestRunOnce_ApplyFailureIsReturned(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(GitHubRelease{TagName: "v2.0.0"}) + })) + defer srv.Close() + u, statusPath := newTestUpdater(t, srv, "v1.0.0") + + err := u.RunOnce() + if err == nil || !strings.Contains(err.Error(), "apply update v2.0.0") { + t.Fatalf("RunOnce = %v, want apply error", err) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultFailed || st.LatestVersion != "v2.0.0" || st.CurrentVersion != "v1.0.0" { + t.Errorf("status = %+v", st) + } +} + +// TestStatus_MergesWithFileOnDisk: a manual run must not wipe what the loop +// (another process) recorded, and must continue its failure streak. +func TestStatus_MergesWithFileOnDisk(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v1.0.0", map[string]string{"daemon": "d"}) + srv.apiStatus.Store(http.StatusBadGateway) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + + next := time.Now().Add(time.Hour).UTC().Truncate(time.Second) + started := time.Now().Add(-time.Hour).UTC().Truncate(time.Second) + if err := writeStatusFile(statusPath, Status{ + LoopPID: 4242, + LoopStartedAt: started, + NextCheckAt: next, + ConsecutiveFailures: 3, + LastResult: ResultFailed, + }); err != nil { + t.Fatal(err) + } + + if err := u.RunOnce(); err == nil { + t.Fatal("RunOnce returned nil for a 502") + } + st := mustReadStatus(t, statusPath) + if st.LoopPID != 4242 || !st.LoopStartedAt.Equal(started) || !st.NextCheckAt.Equal(next) { + t.Errorf("loop fields not preserved: %+v", st) + } + if st.ConsecutiveFailures != 4 { + t.Errorf("consecutive_failures = %d, want 4", st.ConsecutiveFailures) + } +} + +// TestStatusPath_Resolution pins where the status file goes. +func TestStatusPath_Resolution(t *testing.T) { + t.Parallel() + dir := t.TempDir() + if got := New(Config{StatePath: filepath.Join(dir, "auto-update.json")}).statusPath(); got != filepath.Join(dir, "update-state.json") { + t.Errorf("default status path = %q, want next to the control file", got) + } + if got := New(Config{StatePath: filepath.Join(dir, "a.json"), StatusPath: "/x/s.json"}).statusPath(); got != "/x/s.json" { + t.Errorf("explicit StatusPath = %q", got) + } + if got := New(Config{}).statusPath(); got != "" { + t.Errorf("no paths configured → %q, want none", got) + } +} + +// TestStatus_WriteFailureDoesNotFailCheck: an unwritable status path is +// logged, never turned into a failed update. +func TestStatus_WriteFailureDoesNotFailCheck(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v1.0.0", map[string]string{"daemon": "d"}) + u, _ := newTestUpdater(t, srv.Server, "v1.0.0") + blocker := filepath.Join(t.TempDir(), "file") + if err := os.WriteFile(blocker, nil, 0o644); err != nil { + t.Fatal(err) + } + u.config.StatusPath = filepath.Join(blocker, "sub", StatusFileName) // parent is a file + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if u.LastStatus().LastResult != ResultUpToDate { + t.Errorf("in-memory status = %+v", u.LastStatus()) + } +} + +// TestReadStatus_Missing lets callers distinguish "never checked". +func TestReadStatus_Missing(t *testing.T) { + t.Parallel() + _, err := ReadStatus(filepath.Join(t.TempDir(), "nope.json")) + if !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("ReadStatus(missing) = %v, want fs.ErrNotExist", err) + } + bad := filepath.Join(t.TempDir(), "bad.json") + if err := os.WriteFile(bad, []byte("{"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := ReadStatus(bad); err == nil { + t.Fatal("ReadStatus(corrupt) = nil error") + } +} + +// TestStatus_JSONShape pins the field names pilotctl reads. +func TestStatus_JSONShape(t *testing.T) { + t.Parallel() + now := time.Date(2026, 9, 24, 12, 0, 0, 0, time.UTC) + data, err := json.Marshal(Status{ + LastCheckAt: now, LastResult: ResultFailed, LastError: "boom", + ConsecutiveFailures: 2, CurrentVersion: "v1.0.0", LatestVersion: "v1.1.0", + LoopPID: 7, NextCheckAt: now, + }) + if err != nil { + t.Fatal(err) + } + for _, key := range []string{ + `"last_check_at":"2026-09-24T12:00:00Z"`, `"last_result":"failed"`, `"last_error":"boom"`, + `"consecutive_failures":2`, `"current_version":"v1.0.0"`, `"latest_version":"v1.1.0"`, + `"loop_pid":7`, `"next_check_at":"2026-09-24T12:00:00Z"`, + } { + if !strings.Contains(string(data), key) { + t.Errorf("JSON %s missing %s", data, key) + } + } + // Zero times are omitted rather than written as year 1. + if strings.Contains(string(data), "0001-01-01") || strings.Contains(string(data), "last_success_at") { + t.Errorf("zero time serialized: %s", data) + } +} + +// TestCheckLoop_RecordsLoopHeartbeat: the loop records its pid and next +// wake-up even while auto-update is disabled, so `update status` can tell a +// running-but-disabled updater from a missing one. +func TestCheckLoop_RecordsLoopHeartbeat(t *testing.T) { + t.Parallel() + dir := t.TempDir() + installDir := t.TempDir() + seedInstallDir(t, installDir, "v1.0.0") + u := &Updater{ + config: Config{ + Repo: "owner/repo", + InstallDir: installDir, + CheckInterval: time.Hour, + StatePath: filepath.Join(dir, "auto-update.json"), // absent → disabled + }, + client: http.DefaultClient, + stopCh: make(chan struct{}), + exitFn: func(int) { t.Error("unexpected exit") }, + } + before := time.Now() + u.Start() + defer u.Stop() + + statusPath := filepath.Join(dir, StatusFileName) + deadline := time.Now().Add(5 * time.Second) + for { + st, err := ReadStatus(statusPath) + if err == nil && !st.NextCheckAt.IsZero() { + if st.LoopPID != os.Getpid() || st.LoopStartedAt.Before(before.Add(-time.Second)) { + t.Errorf("loop fields = pid %d started %v", st.LoopPID, st.LoopStartedAt) + } + if d := st.NextCheckAt.Sub(before); d < 59*time.Minute || d > 61*time.Minute { + t.Errorf("next_check_at %v is not ~1h after start", st.NextCheckAt) + } + if st.LastResult != "" { + t.Errorf("disabled loop recorded a check: %+v", st) + } + return + } + if time.Now().After(deadline) { + t.Fatalf("loop never recorded its heartbeat (err=%v)", err) + } + time.Sleep(10 * time.Millisecond) + } +} diff --git a/zz_test.go b/zz_test.go index 8c350ac..ab35374 100644 --- a/zz_test.go +++ b/zz_test.go @@ -23,10 +23,18 @@ import ( // production fail-closed behaviour restore realVerifyChecksumsAttestationFn // explicitly (see TestVerifyChecksumsAttestation_* and // TestApplyUpdate_SkipAttestationStillRequiresChecksum). +// +// It also replaces runCommand so no test can run the real launchctl: on a +// developer Mac with Pilot installed, `launchctl kickstart -k +// gui//network.pilotprotocol.pilot-daemon` would restart the developer's +// running daemon. Tests that care about the command use Updater.runCmd. func TestMain(m *testing.M) { verifyChecksumsAttestationFn = func(repo, tag, checksumsPath string) error { return nil } + runCommand = func(name string, args ...string) ([]byte, error) { + return nil, nil + } os.Exit(m.Run()) } @@ -420,13 +428,18 @@ func TestApplyUpdate_SkipsServerBinaries(t *testing.T) { }, } - if err := u.applyUpdate(release); err != nil { + updaterReplaced, err := u.applyUpdate(release) + if err != nil { t.Fatalf("applyUpdate: %v", err) } - // updater binary replaced → self-exit should have been triggered. - if !exitCalled { - t.Error("expected exitFn to be called after updater binary replacement") + // applyUpdate reports the self-replacement; the exit decision belongs + // to runCheck (loop exits, RunOnce does not), so applyUpdate never exits. + if !updaterReplaced { + t.Error("expected applyUpdate to report the updater binary was replaced") + } + if exitCalled { + t.Error("applyUpdate must not exit; runCheck decides") } // Client binaries should be updated using installed names (pilot- prefix). From 845571b26a18fdd008bfdc8ea1fe35d753c9bbf9 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 01:21:27 +0300 Subject: [PATCH 3/4] ci: pin govulncheck to v1.7.0 (v1.8.0 requires Go 1.26) govulncheck@latest resolves to v1.8.0, which requires go >= 1.26.0. setup-go runs with GOTOOLCHAIN=local on the go.mod version (1.25.13), so the job failed before scanning anything, on every PR. v1.7.0 supports Go 1.25 and reports 0 reachable vulnerabilities on this branch. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/security.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 7383950..58fcdc8 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -57,7 +57,10 @@ jobs: # golang/govulncheck-action does its own git fetch and fails with # "fatal: unable to access ... error: 400" on org repos. Run the tool # directly after checkout + setup-go (rendezvous/common pattern). - run: go run golang.org/x/vuln/cmd/govulncheck@latest ./... + # Pinned: govulncheck v1.8.0+ requires Go 1.26, and setup-go runs + # with GOTOOLCHAIN=local on the go.mod version (1.25.x), so @latest + # fails before scanning anything. Bump together with the Go 1.26 move. + run: go run golang.org/x/vuln/cmd/govulncheck@v1.7.0 ./... # Secret scan over the full history on push, the PR range on PR. gitleaks: From 4474271dec3bce8614a6ffb156875b41178808ac Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 02:01:33 +0300 Subject: [PATCH 4/4] fix: restart the Linux daemon only when systemd will; lock the status file Review findings on #49. UPD49-1 (high): signalDaemonRestartLinux now matched the renamed-over daemon and SIGTERMed it on the assumption that systemd Restart=always would bring it back. Nothing restarts a daemon started with `pilotctl daemon start` (containers, WSL, CI), and units written by install.sh up to v1.9.0 say Restart=on-failure, under which systemd does not restart the clean exit pilot-daemon makes on SIGTERM. Those nodes went offline while the status file said last_result=updated with no restart_error. The updater now sends SIGTERM only when the daemon is the ExecStart= program of a system service that restarts after a clean exit (Restart=always/on-success, RestartForceExitStatus, RestartPreventExitStatus honoured). The service is found from /proc//cgroup (v1, v2, hybrid) and its unit file plus drop-ins are read from the systemd search path; no systemctl exec, so TestNoExternalToolDependency still holds. Otherwise the daemon keeps running and restart_error says why and gives the command that restarts it. After a SIGTERM the updater waits up to 30 s for a new daemon process on the new binary and records a failure if none appears. On Linux, restart_error is cleared by the next check that finds the daemon on the installed binary (e.g. after a manual restart). Verified against a real systemd (PID 1, privileged container): always -> restarted onto the new binary; on-failure -> left running, error reported; Setsid daemon -> left running, error reported (zz_systemd_e2e_test.go, env-gated). UPD49-2 (low): updateStatus's read-merge-write now holds an advisory flock on update-state.json.lock, so writers in different processes no longer drop each other's changes (the repro lost 203 of 400 increments without it). A writer that cannot get the lock within 5 s writes unlocked and logs; the lock file is opened read-only so one created by another user still works. UPD49-3 (low): README/CHANGELOG no longer claim `pilotctl update` writes or shares update-state.json: RunOnce writes it only when the caller sets StatusPath/StatePath, and web4's pilotctl does not yet (nor does it check RunOnce's error). The stranded-node section gives the Linux log location (journalctl -u pilot-updater). New "Restarting the daemon" section. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 18 +- README.md | 72 +++- status.go | 86 ++++- supervisor.go | 466 +++++++++++++++++++++++ updater.go | 76 ++-- zz_restart_test.go | 23 +- zz_status_lock_test.go | 197 ++++++++++ zz_status_test.go | 24 +- zz_supervisor_test.go | 817 +++++++++++++++++++++++++++++++++++++++++ zz_systemd_e2e_test.go | 230 ++++++++++++ 10 files changed, 1926 insertions(+), 83 deletions(-) create mode 100644 supervisor.go create mode 100644 zz_status_lock_test.go create mode 100644 zz_supervisor_test.go create mode 100644 zz_systemd_e2e_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 907dc3e..29014ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 error, the run of consecutive failures, the installed and latest versions, the last update, any daemon restart failure, and a heartbeat for the loop (`loop_pid`, `next_check_at`). Read it with `ReadStatus`. `LastStatus()` - returns the same record in-process. + returns the same record in-process. Writers in different processes + serialise on an advisory lock (`update-state.json.lock`), so none of them + lose each other's updates. A `RunOnce` caller writes the file only when it + sets `StatusPath` or `StatePath`. `pilotctl update` does not do that yet. - `GITHUB_TOKEN` / `GH_TOKEN` is now used for the releases API as well as for attestations. It is sent only to `api.github.com`, and the call is retried without it if GitHub rejects it with a 401. Rate-limit errors say when the @@ -41,6 +44,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `/proc//exe` reads ` (deleted)` once its binary is renamed over, so the old exact-path match never found it and daemons kept running the old version. +- The updater sends that SIGTERM only when systemd will start the daemon + again: the daemon runs as `ExecStart=` of a system service with + `Restart=always` or `on-success`, read from `/proc//cgroup` and the unit + files. Otherwise the daemon keeps running and `restart_error` says how to + restart it. This covers a daemon started with `pilotctl daemon start` + (containers, WSL, CI) and units from install.sh up to v1.9.0, which have + `Restart=on-failure`. Stopping those daemons would take the node offline + while the status file reported `updated`. After a SIGTERM the updater waits + up to 30 s for a new daemon on the new binary, and reports a failure if none + appears. +- On Linux, `restart_error` is cleared once a check finds the daemon running + the installed binary, for example after a manual restart. Before, it stayed + until the next release. - The test suite no longer runs the real `launchctl`. On a developer Mac with Pilot installed, it used to restart the running daemon. diff --git a/README.md b/README.md index de0e0cf..95aab45 100644 --- a/README.md +++ b/README.md @@ -28,18 +28,22 @@ u := updater.New(updater.Config{ u.Start() ``` -One-shot check (what `pilotctl update` does): +One-shot check, for `pilotctl update` and similar commands: ```go u := updater.New(updater.Config{ Repo: "pilot-protocol/pilotprotocol", InstallDir: "/home/user/.pilot/bin", - StatusPath: "/home/user/.pilot/update-state.json", + StatusPath: "/home/user/.pilot/update-state.json", // record the run }) if err := u.RunOnce(); err != nil { // The check failed: report it and exit non-zero. } st := u.LastStatus() // st.LastResult is "up_to_date" or "updated" +if st.RestartError != "" { + // Installed, but the daemon still runs the old version. The message + // says how to restart it. +} ``` `RunOnce` returns the check's error. It never exits the calling process. If @@ -47,6 +51,42 @@ the release replaced `pilot-updater`, the daemon is still restarted onto the new binaries. A running updater service switches to its new binary the next time its service manager restarts it. +A `RunOnce` caller writes the status file only when it sets `StatusPath` (or +`StatePath`); otherwise only `LastStatus()` has the result. **`pilotctl +update` does not do this yet.** As of web4 `main` on 2026-09-24, +`cmd/pilotctl/updates.go` sets neither path and ignores the error `RunOnce` +returns. So manual updates are not recorded in `update-state.json`, and a +failed `pilotctl update` still prints success and exits 0. That stays true +until pilotctl passes `StatusPath: ~/.pilot/update-state.json` and checks the +error, which needs a web4 change after this version is released. + +### Restarting the daemon + +After installing new binaries the updater restarts the daemon onto them: + +- **macOS:** `launchctl kickstart -k gui//network.pilotprotocol.pilot-daemon`. +- **Linux:** SIGTERM, but only when systemd will start the daemon again. The + daemon must run as the main program (`ExecStart=`) of a system service whose + unit restarts it after a clean exit. pilot-daemon exits 0 on SIGTERM, so + that means `Restart=always` or `on-success`. The updater finds the service + from `/proc//cgroup` and reads the unit file and its drop-ins. It does + not run `systemctl`. After the SIGTERM it waits up to 30 s for a new daemon + process on the new binary. + +On Linux the daemon is left running the old version, with `restart_error` +saying how to restart it, when: + +- it was started with `pilotctl daemon start`, which is how containers, WSL + and CI run it because install.sh sets up no service there; +- its unit has `Restart=on-failure`, as written by install.sh up to v1.9.0. + Updates never rewrite the unit, and systemd does not restart a clean exit + under that policy. Re-running install.sh rewrites the unit with + `Restart=always`; +- it runs under a systemd user service, or inside another service, such as a + CI runner. + +Stopping the daemon in any of those cases would take the node offline. + ### Pinning a version Set `PinnedVersion` to lock the updater to a specific release tag. When @@ -77,9 +117,16 @@ and a node could fail every hourly check for weeks without anyone noticing. The file goes to `Config.StatusPath`. When that is empty and `StatePath` is set, it goes next to the control file. For a standard install that is -`~/.pilot/auto-update.json` → `~/.pilot/update-state.json`. The long-running -updater and one-shot `pilotctl update` runs share the file: each writer -merges its fields into what is already there, and writes are atomic. +`~/.pilot/auto-update.json` → `~/.pilot/update-state.json`. With neither +set, nothing is written. + +The long-running updater and any one-shot `RunOnce` caller configured with +the same path share the file. `pilotctl update` does not pass one yet (see +above). Each writer takes an advisory lock on `update-state.json.lock` and +merges its fields into what is already there, so concurrent writers from +different processes do not lose each other's changes. Writes are atomic +(temp file + rename). A writer that cannot get the lock within 5 s writes +anyway and logs a warning. ```json { @@ -108,7 +155,7 @@ merges its fields into what is already there, and writes are atomic. | `consecutive_failures` | Failed checks since the last success. | | `last_success_at` | The last check that completed without error. | | `current_version` / `latest_version` | The installed release, and the release it was compared against (the pin, when pinned). | -| `restart_error` | New binaries were installed but the daemon could not be restarted onto them. Cleared by the next successful restart. | +| `restart_error` | New binaries were installed but the daemon is not running them. Either it was left on the old version because stopping it would have left it down (see [Restarting the daemon](#restarting-the-daemon)) or the restart failed, or it was stopped and did not come back. Says how to restart it. Cleared by the next successful restart, and on Linux by the next check that finds the daemon running the installed binary. | | `loop_pid`, `loop_started_at` | The long-running updater process. | | `next_check_at` | When the loop wakes next. It is updated even while auto-update is disabled, so it doubles as a heartbeat. | @@ -117,7 +164,7 @@ Suggested readings for `pilotctl update status` and similar tools: - **Updates failing:** `consecutive_failures > 0`. Show `last_error`. - **Updates stalled:** `last_success_at` is more than 48 hours old while auto-update is enabled. - **No updater running:** auto-update is enabled, but the file is missing, `loop_pid` is not alive, or `next_check_at` is more than one interval in the past. This happens on Linux without systemd (containers, WSL, CI), where the installer cannot start the updater. -- **Daemon on the old version:** `restart_error` is set. Restart the daemon by hand. +- **Daemon on the old version:** `restart_error` is set. Show it: it names the command that restarts the daemon. Use `updater.ReadStatus(path)` to read the file. A missing file returns an error matching `errors.Is(err, fs.ErrNotExist)`. @@ -164,8 +211,9 @@ The SHA-256 match against the attested `checksums.txt` still gates every install The updater never runs `gh` or any other external tool. It checks the SLSA provenance of `checksums.txt` in-process with sigstore-go (`attestation.go`). The only command it runs is `launchctl kickstart`, which -restarts the daemon under launchd on macOS. `TestNoExternalToolDependency` -fails the build if another exec is added. +restarts the daemon under launchd on macOS. On Linux it reads `/proc` and the +systemd unit files directly instead of running `systemctl`. +`TestNoExternalToolDependency` fails the build if another exec is added. ### Nodes stuck on v1.12.2–v1.13.4 (updater v0.2.3) @@ -173,8 +221,9 @@ pilotprotocol releases **v1.12.2 through v1.13.4** shipped updater v0.2.3, which ran `gh attestation verify` and refused every update when `gh` was not on its `PATH`. Service managers never put `gh` on the `PATH`: launchd's default is `/usr/bin:/bin:/usr/sbin:/sbin`, and most Linux hosts have no `gh` at all. So -those nodes cannot update themselves. `~/.pilot/updater.log` shows an hourly -`gh CLI required for SLSA attestation verification` error. +those nodes cannot update themselves. The updater log shows an hourly +`gh CLI required for SLSA attestation verification` error. On macOS the log +is `~/.pilot/updater.log`. On Linux, read it with `journalctl -u pilot-updater`. The broken updater cannot fix itself. On an affected node, do one of these: @@ -203,6 +252,7 @@ because that switches off provenance checks. | File | What it does | |---|---| | `updater.go` | `Updater`: check loop, `RunOnce`, release polling, install, daemon restart. | +| `supervisor.go` | Linux restart safety: finds the daemon's systemd service and checks its restart policy before any SIGTERM. | | `download.go` | Resumable asset downloads with idle and total-duration limits. | | `github.go` | GitHub API calls: optional `GITHUB_TOKEN`, rate-limit errors. | | `attestation.go` | In-process SLSA provenance verification of `checksums.txt` (sigstore-go). | diff --git a/status.go b/status.go index cb6a1a7..5ca8cb6 100644 --- a/status.go +++ b/status.go @@ -4,10 +4,12 @@ package updater import ( "encoding/json" + "errors" "fmt" "log/slog" "os" "path/filepath" + "syscall" "time" ) @@ -42,10 +44,12 @@ const ( // whether updates are actually working, instead of only whether they are // switched on. // -// The file is shared by the long-running updater service and one-shot -// `pilotctl update` runs: each writer merges its fields into what is already -// on disk, so a manual run keeps the loop's LoopPID/NextCheckAt and the loop -// keeps the failure streak a manual run started. +// The file can be shared by the long-running updater service and one-shot +// RunOnce callers configured with the same StatusPath (or StatePath). Each +// writer takes an advisory lock (StatusFileName + ".lock") and merges its +// fields into what is already on disk, so a manual run keeps the loop's +// LoopPID/NextCheckAt and the loop keeps the failure streak a manual run +// started. A RunOnce caller with neither path set writes nothing. type Status struct { // UpdaterVersion is the version of the updater that wrote the record. UpdaterVersion string `json:"updater_version,omitempty"` @@ -77,8 +81,13 @@ type Status struct { LastUpdateAt time.Time `json:"last_update_at,omitzero"` LastUpdateVersion string `json:"last_update_version,omitempty"` // RestartError is set when new binaries were installed but the daemon - // could not be restarted onto them (it keeps running the old version - // until restarted by hand). Cleared by the next successful restart. + // is not running them: it was left on the old version, or it was + // stopped and did not come back. It says how to restart it. On Linux the + // updater stops the daemon only when systemd will start it again, so a + // daemon without such a service (e.g. `pilotctl daemon start` in a + // container) is left running and reported here. Cleared by the next + // successful restart and, on Linux, by the next check that finds the + // daemon running the installed binary. RestartError string `json:"restart_error,omitempty"` // LoopPID and LoopStartedAt identify the long-running updater process @@ -128,9 +137,11 @@ func (u *Updater) LastStatus() Status { // updateStatus applies mutate to the current status and persists it. The // base is the file on disk (so fields written by another process survive); -// when the file is missing or unreadable the in-memory copy is used. Write -// failures are logged, never returned: recording the outcome must not change -// the outcome. +// when the file is missing or unreadable the in-memory copy is used. The +// read-merge-write runs under statusMu (this process) and an advisory file +// lock (other processes), so concurrent writers do not drop each other's +// changes. Write failures are logged, never returned: recording the outcome +// must not change the outcome. func (u *Updater) updateStatus(mutate func(*Status)) { u.statusMu.Lock() defer u.statusMu.Unlock() @@ -138,6 +149,12 @@ func (u *Updater) updateStatus(mutate func(*Status)) { path := u.statusPath() base := u.status if path != "" { + wait := u.statusLockWait + if wait <= 0 { + wait = defaultStatusLockWait + } + unlock := lockStatusFile(path, wait) + defer unlock() if onDisk, err := ReadStatus(path); err == nil { base = onDisk } @@ -155,6 +172,50 @@ func (u *Updater) updateStatus(mutate func(*Status)) { } } +// defaultStatusLockWait bounds how long a writer waits for another +// process's read-merge-write of the status file (each holds the lock for a +// few milliseconds). After it, the write goes ahead unlocked: recording the +// outcome must never block an update. +const defaultStatusLockWait = 5 * time.Second + +// lockStatusFile takes an exclusive advisory lock (flock) on path + ".lock", +// a sidecar file: path itself is replaced by rename on every write, so it +// cannot carry the lock. It returns the function that releases the lock. If +// the lock cannot be taken within wait it logs why and returns a no-op. +func lockStatusFile(path string, wait time.Duration) (unlock func()) { + noop := func() {} + lockPath := path + ".lock" + if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { + slog.Warn("cannot lock update status file; writing unlocked", "path", lockPath, "error", err) + return noop + } + // Read-only is enough for flock and works when another user (e.g. a + // `sudo pilotctl update`) created the lock file. + f, err := os.OpenFile(lockPath, os.O_RDONLY|os.O_CREATE, 0o644) + if err != nil { + slog.Warn("cannot lock update status file; writing unlocked", "path", lockPath, "error", err) + return noop + } + fd := int(f.Fd()) // #nosec G115 -- a file descriptor always fits in an int + deadline := time.Now().Add(wait) + for { + err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return func() { + _ = syscall.Flock(fd, syscall.LOCK_UN) + _ = f.Close() + } + } + retry := errors.Is(err, syscall.EWOULDBLOCK) || errors.Is(err, syscall.EINTR) + if !retry || !time.Now().Before(deadline) { + slog.Warn("cannot lock update status file; writing unlocked", "path", lockPath, "error", err) + _ = f.Close() + return noop + } + time.Sleep(5 * time.Millisecond) + } +} + // writeStatusFile writes s to path atomically (temp file + rename in the same // directory) so a reader never sees a torn file. func writeStatusFile(path string, s Status) error { @@ -203,6 +264,10 @@ type checkOutcome struct { // restartErr is the error from restarting the daemon onto the new // binaries, if that failed. restartErr error + // daemonCurrent reports that, with nothing installed by this check, the + // daemon was found running the installed binary (Linux only), so an + // earlier restart_error no longer applies. + daemonCurrent bool } // recordCheck persists the outcome of one check. @@ -212,6 +277,9 @@ func (u *Updater) recordCheck(trigger string, out checkOutcome, checkErr error) s.PinnedVersion = u.config.PinnedVersion s.LastCheckAt = now s.LastCheckTrigger = trigger + if out.installed == "" && out.daemonCurrent { + s.RestartError = "" + } if out.latest != "" { s.LatestVersion = out.latest } diff --git a/supervisor.go b/supervisor.go new file mode 100644 index 0000000..a9706a2 --- /dev/null +++ b/supervisor.go @@ -0,0 +1,466 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "sort" + "strconv" + "strings" + "time" +) + +// Restarting the daemon on Linux. +// +// The updater cannot run `systemctl restart`: it runs as the unprivileged +// install user, and it runs no external tools (TestNoExternalToolDependency). +// It restarts the daemon by sending it SIGTERM and letting systemd start it +// again on the new binary. That works only when systemd really will start it +// again, which needs two things: +// +// - The daemon is the process of a system service, not a login session, a +// container or another service's child. A daemon started with +// `pilotctl daemon start` (containers, WSL, CI, where install.sh sets up +// no service) has no supervisor at all. +// - The service restarts after a clean exit. pilot-daemon exits 0 on +// SIGTERM. systemd restarts that under Restart=always or on-success, but +// not under Restart=on-failure, which install.sh wrote up to v1.9.0 (web4 +// 1ee4db7d switched to Restart=always). Updates never rewrite the unit. +// +// Stopping a daemon that nothing restarts takes the node offline. So when +// either condition fails, the updater leaves the daemon running the old +// binary and reports why, with the command that restarts it (restart_error in +// the status file). When both hold, it sends SIGTERM and then waits for a new +// daemon process on the new binary; if none appears, that is reported too. +// +// The unit is read from the unit files, not from systemctl: the service is +// found from /proc//cgroup, and Restart=, ExecStart= and the exit-status +// lists are read from its unit file and drop-ins. + +const ( + // defaultRestartWait bounds how long the updater waits for systemd to + // start the daemon again after SIGTERM. The daemon's shutdown takes up + // to ~5 s and install.sh units use RestartSec=5. + defaultRestartWait = 30 * time.Second + // defaultRestartPoll is how often /proc is scanned while waiting. + defaultRestartPoll = 250 * time.Millisecond +) + +// systemdSystemUnitDirs is the systemd system unit search path, highest +// priority first (systemd.unit(5)). /lib/systemd/system is included for +// distributions without a merged /usr. +var systemdSystemUnitDirs = []string{ + "/etc/systemd/system.control", + "/run/systemd/system.control", + "/run/systemd/transient", + "/run/systemd/generator.early", + "/etc/systemd/system", + "/etc/systemd/system.attached", + "/run/systemd/system", + "/run/systemd/system.attached", + "/run/systemd/generator", + "/usr/local/lib/systemd/system", + "/usr/lib/systemd/system", + "/lib/systemd/system", + "/run/systemd/generator.late", +} + +// targetOS is the OS whose restart mechanism is used. Tests override it to +// drive the Linux path on macOS. +func (u *Updater) targetOS() string { + if u.goos != "" { + return u.goos + } + return runtime.GOOS +} + +// procDir is the /proc root to scan. +func (u *Updater) procDir() string { + if u.procRoot != "" { + return u.procRoot + } + return "/proc" +} + +// unitDirs is the systemd unit search path to read. +func (u *Updater) unitDirs() []string { + if u.systemdDirs != nil { + return u.systemdDirs + } + return systemdSystemUnitDirs +} + +// forEachProcExe calls fn with the pid and /proc//exe target of every +// process other than this one whose exe link is readable. fn returns false +// to stop the scan. +func forEachProcExe(procRoot string, fn func(pid int, exe string) bool) error { + entries, err := os.ReadDir(procRoot) + if err != nil { + return err + } + self := os.Getpid() + for _, entry := range entries { + if !entry.IsDir() { + continue + } + pid, err := strconv.Atoi(entry.Name()) + if err != nil || pid <= 0 || pid == self { + continue + } + exe, err := os.Readlink(filepath.Join(procRoot, entry.Name(), "exe")) + if err != nil { + continue + } + if !fn(pid, exe) { + return nil + } + } + return nil +} + +// scanDaemonProcs lists the processes running exePath. current holds those +// running the file now at exePath; replaced holds those still running a +// binary that has since been renamed over (" (deleted)"). +func scanDaemonProcs(procRoot, exePath string) (current, replaced []int, err error) { + err = forEachProcExe(procRoot, func(pid int, exe string) bool { + switch exe { + case exePath: + current = append(current, pid) + case exePath + " (deleted)": + replaced = append(replaced, pid) + } + return true + }) + return current, replaced, err +} + +// daemonOnInstalledBinary reports whether the daemon is known to be running +// the installed binary: a process runs daemonPath itself and none still runs +// a replaced copy. Only Linux can tell (from /proc); elsewhere it returns +// false. +func (u *Updater) daemonOnInstalledBinary() bool { + if u.targetOS() != "linux" { + return false + } + daemonPath := filepath.Join(u.config.InstallDir, "pilot-daemon") + current, replaced, err := scanDaemonProcs(u.procDir(), daemonPath) + return err == nil && len(current) > 0 && len(replaced) == 0 +} + +// serviceUnitOf returns the systemd service whose cgroup holds pid, read from +// //cgroup. unit is "" when the process is not in a service: +// it is in a login session or container scope, or systemd is not running. +// userManager reports that the service belongs to a per-user systemd +// instance (under user@.service) rather than the system manager. +func serviceUnitOf(procRoot string, pid int) (unit string, userManager bool, err error) { + data, err := os.ReadFile(filepath.Join(procRoot, strconv.Itoa(pid), "cgroup")) + if err != nil { + return "", false, err + } + // Lines are "hierarchy-ID:controller-list:cgroup-path". systemd's own + // hierarchy is "name=systemd" under cgroup v1 and the unified "0::" + // line under v2 (both are present, and agree, in hybrid mode). + var named, unified string + for _, line := range strings.Split(string(data), "\n") { + parts := strings.SplitN(strings.TrimSpace(line), ":", 3) + if len(parts) != 3 { + continue + } + switch { + case parts[1] == "name=systemd": + named = parts[2] + case parts[0] == "0" && parts[1] == "": + unified = parts[2] + } + } + path := named + if path == "" { + path = unified + } + // The process belongs to the deepest unit in the path. Slices only group + // units; sub-cgroups a service delegates have no unit suffix. + for _, comp := range strings.Split(path, "/") { + comp = strings.TrimPrefix(comp, "_") // systemd escapes some names with a leading "_" + if !strings.HasSuffix(comp, ".service") && !strings.HasSuffix(comp, ".scope") { + continue + } + if strings.HasPrefix(unit, "user@") && strings.HasSuffix(unit, ".service") { + userManager = true + } + unit = comp + } + if !strings.HasSuffix(unit, ".service") { + return "", false, nil + } + return unit, userManager, nil +} + +// serviceUnit is the part of a systemd service's configuration that decides +// whether stopping its process with SIGTERM makes systemd start it again. +type serviceUnit struct { + name string + fragment string // the unit file that was read + + execStart string // program of the first ExecStart= command ("" = none) + execSet bool + restart string // Restart=, lower-cased ("" = the default, "no") + forceExit []string // RestartForceExitStatus= + preventExit []string // RestartPreventExitStatus= +} + +// templateOf returns the template unit name of an instance unit +// ("pilot-daemon@x.service" -> "pilot-daemon@.service"), or "". +func templateOf(name string) string { + at := strings.IndexByte(name, '@') + dot := strings.LastIndexByte(name, '.') + if at < 0 || dot < at || at+1 == dot { + return "" + } + return name[:at+1] + name[dot:] +} + +// loadServiceUnit reads the unit file of service name (or its template) +// from dirs, highest priority first, and applies its drop-ins +// (.d/*.conf and the top-level service.d/*.conf), sorted by file name, +// the first of each name winning, as systemd does. +func loadServiceUnit(dirs []string, name string) (serviceUnit, error) { + su := serviceUnit{name: name} + names := []string{name} + if tmpl := templateOf(name); tmpl != "" { + names = append(names, tmpl) + } + for _, n := range names { + for _, dir := range dirs { + p := filepath.Join(dir, n) + data, err := os.ReadFile(p) + if err != nil { + continue + } + su.fragment = p + su.parse(data) + break + } + if su.fragment != "" { + break + } + } + if su.fragment == "" { + return su, fmt.Errorf("no unit file for %s", name) + } + + dropins := map[string]string{} // file name -> path + for _, n := range append(names, "service") { + for _, dir := range dirs { + matches, _ := filepath.Glob(filepath.Join(dir, n+".d", "*.conf")) + for _, m := range matches { + if _, seen := dropins[filepath.Base(m)]; !seen { + dropins[filepath.Base(m)] = m + } + } + } + } + order := make([]string, 0, len(dropins)) + for base := range dropins { + order = append(order, base) + } + sort.Strings(order) + for _, base := range order { + data, err := os.ReadFile(dropins[base]) + if err != nil { + continue + } + su.parse(data) + } + return su, nil +} + +// parse applies the [Service] settings in one unit file or drop-in. +func (su *serviceUnit) parse(data []byte) { + lines := strings.Split(string(data), "\n") + section := "" + for i := 0; i < len(lines); i++ { + line := strings.TrimSpace(lines[i]) + if line == "" || line[0] == '#' || line[0] == ';' { + continue + } + // A trailing backslash continues the line. Comment lines inside a + // continued line are skipped. + for strings.HasSuffix(line, `\`) && i+1 < len(lines) { + i++ + next := strings.TrimSpace(lines[i]) + if next != "" && (next[0] == '#' || next[0] == ';') { + continue + } + line = strings.TrimSuffix(line, `\`) + " " + next + } + line = strings.TrimSuffix(line, `\`) + if strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]") { + section = line[1 : len(line)-1] + continue + } + if section != "Service" { + continue + } + key, value, ok := strings.Cut(line, "=") + if !ok { + continue + } + value = strings.TrimSpace(value) + switch strings.TrimSpace(key) { + case "Restart": + su.restart = strings.ToLower(value) + case "ExecStart": + // An empty assignment resets the list (how drop-ins replace it). + if value == "" { + su.execStart, su.execSet = "", false + } else if !su.execSet { + su.execStart, su.execSet = execProgram(value), true + } + case "RestartForceExitStatus": + su.forceExit = appendStatusList(su.forceExit, value) + case "RestartPreventExitStatus": + su.preventExit = appendStatusList(su.preventExit, value) + } + } +} + +func appendStatusList(list []string, value string) []string { + if value == "" { + return nil + } + return append(list, strings.Fields(value)...) +} + +// execProgram returns the program of an ExecStart= command line, without +// systemd's special prefixes (@ - : + ! |) and quotes. +func execProgram(cmd string) string { + cmd = strings.TrimSpace(strings.TrimLeft(cmd, "@-:+!|")) + if cmd == "" { + return "" + } + if q := cmd[0]; q == '"' || q == '\'' { + if end := strings.IndexByte(cmd[1:], q); end >= 0 { + return cmd[1 : 1+end] + } + return cmd[1:] + } + if i := strings.IndexAny(cmd, " \t"); i >= 0 { + return cmd[:i] + } + return cmd +} + +// restartSetting is Restart= as systemd reads it. +func (su serviceUnit) restartSetting() string { + if su.restart == "" { + return "no" + } + return su.restart +} + +// restartsOnCleanExit reports whether systemd starts the service again after +// its main process exits cleanly, which is how pilot-daemon exits on SIGTERM +// (exit status 0; SIGTERM itself also counts as clean). +func (su serviceUnit) restartsOnCleanExit() bool { + clean := func(list []string) bool { + for _, s := range list { + switch strings.ToUpper(s) { + case "0", "SUCCESS", "SIGTERM", "TERM": + return true + } + } + return false + } + if clean(su.preventExit) { + return false + } + switch su.restart { + case "always", "on-success": + return true + } + return clean(su.forceExit) +} + +// sameFile reports whether the program path a names the file at b. +func sameFile(a, b string) bool { + if a == "" { + return false + } + if filepath.Clean(a) == filepath.Clean(b) { + return true + } + ra, errA := filepath.EvalSymlinks(a) + rb, errB := filepath.EvalSymlinks(b) + return errA == nil && errB == nil && ra == rb +} + +// unitShortName drops the ".service" suffix for systemctl commands. +func unitShortName(unit string) string { + return strings.TrimSuffix(unit, ".service") +} + +// daemonSupervisor decides whether the daemon (pid, running daemonPath) may +// be stopped with SIGTERM, i.e. whether systemd will start it again. It +// returns the service that will, or an error that says why not and how to +// restart the daemon by hand. The daemon is left running in that case. +func (u *Updater) daemonSupervisor(procRoot string, pid int, daemonPath string) (string, error) { + const manual = "pilotctl daemon stop && pilotctl daemon start" + unit, userManager, err := serviceUnitOf(procRoot, pid) + if err != nil { + return "", fmt.Errorf("daemon (pid %d) left running the old version: cannot tell whether a service manager would start it again (%v); restart it with: %s", pid, err, manual) + } + if unit == "" { + return "", fmt.Errorf("daemon (pid %d) left running the old version: it is not run by a systemd service, so nothing would start it again after a stop; restart it with: %s", pid, manual) + } + if userManager { + return "", fmt.Errorf("daemon (pid %d) left running the old version: it runs in the systemd user service %s, which the updater does not restart; restart it with: systemctl --user restart %s", pid, unit, unitShortName(unit)) + } + su, err := loadServiceUnit(u.unitDirs(), unit) + if err != nil { + return "", fmt.Errorf("daemon (pid %d) left running the old version: it runs in %s, but that unit could not be read (%v); restart it with: sudo systemctl restart %s", pid, unit, err, unitShortName(unit)) + } + if !sameFile(su.execStart, daemonPath) { + return "", fmt.Errorf("daemon (pid %d) left running the old version: it runs inside %s, which starts %q rather than %s, so systemd would not start the daemon again; restart it with: %s", pid, unit, su.execStart, daemonPath, manual) + } + if !su.restartsOnCleanExit() { + return "", fmt.Errorf("daemon (pid %d) left running the old version: %s has Restart=%s, so systemd would not start the daemon again after it stops cleanly; restart it with: sudo systemctl restart %s (re-running install.sh rewrites the unit with Restart=always)", pid, unit, su.restartSetting(), unitShortName(unit)) + } + return unit, nil +} + +// waitForDaemonRestart waits for systemd to start the daemon again after +// oldPid was sent SIGTERM: a new process running daemonPath itself, not a +// replaced binary. It returns the new pid. +func (u *Updater) waitForDaemonRestart(procRoot, daemonPath string, oldPid int, unit string) (int, error) { + wait, poll := u.restartWait, u.restartPoll + if wait <= 0 { + wait = defaultRestartWait + } + if poll <= 0 { + poll = defaultRestartPoll + } + deadline := time.Now().Add(wait) + for { + if current, _, err := scanDaemonProcs(procRoot, daemonPath); err == nil { + for _, pid := range current { + if pid != oldPid { + return pid, nil + } + } + } + if !time.Now().Before(deadline) { + return 0, fmt.Errorf("sent SIGTERM to the daemon (pid %d), but %s did not start it again on the new binary within %s; check `systemctl status %s` and start it with: sudo systemctl start %s", + oldPid, unit, wait, unitShortName(unit), unitShortName(unit)) + } + timer := time.NewTimer(poll) + select { + case <-timer.C: + case <-u.stopCh: + timer.Stop() + return 0, fmt.Errorf("updater stopped while waiting for %s to start the daemon again (pid %d was sent SIGTERM)", unit, oldPid) + } + } +} diff --git a/updater.go b/updater.go index 506a0d9..dcb9906 100644 --- a/updater.go +++ b/updater.go @@ -17,7 +17,6 @@ import ( "os/exec" "path/filepath" "runtime" - "strconv" "strings" "sync" "syscall" @@ -96,13 +95,18 @@ type Updater struct { dlIdleTimeout time.Duration // 0 = defaultDownloadIdleTimeout dlRetryBackoff time.Duration // base pause between download attempts - // Process hooks, injectable for tests. Nil means the real thing. - runCmd func(name string, args ...string) ([]byte, error) - killFn func(pid int, sig syscall.Signal) error - procRoot string // "" = /proc + // Process hooks, injectable for tests. Zero values mean the real thing. + runCmd func(name string, args ...string) ([]byte, error) + killFn func(pid int, sig syscall.Signal) error + procRoot string // "" = /proc + goos string // "" = runtime.GOOS; selects the restart mechanism + systemdDirs []string // nil = systemdSystemUnitDirs + restartWait time.Duration // 0 = defaultRestartWait + restartPoll time.Duration // 0 = defaultRestartPoll - statusMu sync.Mutex - status Status + statusMu sync.Mutex + status Status + statusLockWait time.Duration // 0 = defaultStatusLockWait } // runCommand runs an external command and returns its combined output. The @@ -265,6 +269,11 @@ func (u *Updater) checkOnce() error { // error. func (u *Updater) runCheck(trigger string) error { out, err := u.check() + if out.installed == "" { + // A daemon now running the installed binary (restarted by hand after + // an earlier restart failure) settles restart_error. + out.daemonCurrent = u.daemonOnInstalledBinary() + } if err != nil { slog.Error("update check failed", "trigger", trigger, "error", err) u.recordCheck(trigger, out, err) @@ -792,7 +801,7 @@ func writeFileSync(path string, data []byte, perm os.FileMode) error { // binary; a daemon that is not running at all is not an error (its next // start uses the new binary). func (u *Updater) signalDaemonRestart() error { - if runtime.GOOS == "darwin" { + if u.targetOS() == "darwin" { return u.signalDaemonRestartDarwin() } return u.signalDaemonRestartLinux() @@ -825,14 +834,11 @@ func (u *Updater) signalDaemonRestartDarwin() error { } func (u *Updater) signalDaemonRestartLinux() error { - // On Linux, find the daemon process via /proc//exe and send SIGTERM. - // systemd (Restart=always in the unit install.sh writes) relaunches it on - // the new binary. + // On Linux, find the daemon process via /proc//exe. When systemd + // will start it again (see supervisor.go), send SIGTERM and wait for the + // new process; otherwise leave it running and say how to restart it. daemonPath := filepath.Join(u.config.InstallDir, "pilot-daemon") - procRoot := u.procRoot - if procRoot == "" { - procRoot = "/proc" - } + procRoot := u.procDir() pid, err := findProcessByExe(procRoot, daemonPath) if err != nil { slog.Warn("cannot read /proc — restart daemon manually", "error", err) @@ -843,7 +849,12 @@ func (u *Updater) signalDaemonRestartLinux() error { slog.Warn("daemon process not found — restart daemon manually if it is running", "path", daemonPath) return nil } - slog.Info("sending SIGTERM to daemon", "pid", pid) + unit, err := u.daemonSupervisor(procRoot, pid, daemonPath) + if err != nil { + slog.Warn("daemon not restarted onto the new binary", "pid", pid, "reason", err) + return fmt.Errorf("restart daemon: %w", err) + } + slog.Info("sending SIGTERM to daemon; systemd starts it again", "pid", pid, "unit", unit) kill := u.killFn if kill == nil { kill = syscall.Kill @@ -851,6 +862,12 @@ func (u *Updater) signalDaemonRestartLinux() error { if err := kill(pid, syscall.SIGTERM); err != nil { return fmt.Errorf("restart daemon: signal pid %d: %w", pid, err) } + newPid, err := u.waitForDaemonRestart(procRoot, daemonPath, pid, unit) + if err != nil { + slog.Error("daemon did not come back after SIGTERM", "unit", unit, "error", err) + return fmt.Errorf("restart daemon: %w", err) + } + slog.Info("daemon restarted on the new binary", "unit", unit, "pid", newPid) return nil } @@ -860,28 +877,15 @@ func (u *Updater) signalDaemonRestartLinux() error { // reads " (deleted)". That must match too, or the daemon is never // restarted onto the new version. func findProcessByExe(procRoot, exePath string) (int, error) { - entries, err := os.ReadDir(procRoot) - if err != nil { - return 0, err - } - self := os.Getpid() - for _, entry := range entries { - if !entry.IsDir() { - continue - } - pid, err := strconv.Atoi(entry.Name()) - if err != nil || pid <= 0 || pid == self { - continue - } - exe, err := os.Readlink(filepath.Join(procRoot, entry.Name(), "exe")) - if err != nil { - continue - } + found := 0 + err := forEachProcExe(procRoot, func(pid int, exe string) bool { if exe == exePath || exe == exePath+" (deleted)" { - return pid, nil + found = pid + return false } - } - return 0, nil + return true + }) + return found, err } // verifyChecksumsAttestation verifies the SLSA provenance of checksums.txt for diff --git a/zz_restart_test.go b/zz_restart_test.go index 6cc0eda..2523547 100644 --- a/zz_restart_test.go +++ b/zz_restart_test.go @@ -67,23 +67,18 @@ func TestFindProcessByExe_ExactAndMissing(t *testing.T) { } } -// TestSignalDaemonRestartLinux_Paths covers signal success, signal failure, -// daemon not running (not an error) and unreadable /proc (an error). +// TestSignalDaemonRestartLinux_Paths covers a supervised daemon (SIGTERM, +// systemd starts it again), signal failure, daemon not running (not an +// error) and unreadable /proc (an error). func TestSignalDaemonRestartLinux_Paths(t *testing.T) { t.Parallel() - installDir := t.TempDir() - daemon := filepath.Join(installDir, "pilot-daemon") - root := fakeProc(t, map[string]string{"77": daemon + " (deleted)"}) + u, sys := supervisedLinuxUpdater(t, "always") var gotPid int var gotSig syscall.Signal - u := &Updater{ - config: Config{InstallDir: installDir}, - procRoot: root, - killFn: func(pid int, sig syscall.Signal) error { - gotPid, gotSig = pid, sig - return nil - }, + sys.onKill = func(pid int, sig syscall.Signal) error { + gotPid, gotSig = pid, sig + return nil } if err := u.signalDaemonRestartLinux(); err != nil { t.Fatalf("signalDaemonRestartLinux: %v", err) @@ -92,11 +87,13 @@ func TestSignalDaemonRestartLinux_Paths(t *testing.T) { t.Errorf("signalled pid %d with %v, want 77 SIGTERM", gotPid, gotSig) } - u.killFn = func(int, syscall.Signal) error { return errors.New("operation not permitted") } + // The restarted daemon (on the installed binary) is found next time. + sys.onKill = func(int, syscall.Signal) error { return errors.New("operation not permitted") } if err := u.signalDaemonRestartLinux(); err == nil || !strings.Contains(err.Error(), "operation not permitted") { t.Errorf("kill failure = %v, want reported", err) } + root := u.procRoot u.procRoot = fakeProc(t, nil) if err := u.signalDaemonRestartLinux(); err != nil { t.Errorf("daemon not running must not be an error, got %v", err) diff --git a/zz_status_lock_test.go b/zz_status_lock_test.go new file mode 100644 index 0000000..fae6c90 --- /dev/null +++ b/zz_status_lock_test.go @@ -0,0 +1,197 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strconv" + "sync" + "syscall" + "testing" + "time" +) + +func incrementFailures(u *Updater, n int) { + for i := 0; i < n; i++ { + u.updateStatus(func(s *Status) { s.ConsecutiveFailures++ }) + } +} + +// lockHeld reports whether another open file description holds the status +// lock for path. +func lockHeld(t *testing.T, path string) bool { + t.Helper() + f, err := os.Open(path + ".lock") + if err != nil { + t.Fatal(err) + } + defer f.Close() + err = syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + return false + } + if !errors.Is(err, syscall.EWOULDBLOCK) { + t.Fatalf("flock: %v", err) + } + return true +} + +// TestUpdateStatus_ConcurrentWritersDoNotLoseUpdates reproduces UPD49-2: +// two Updaters with separate mutexes (as two processes have) share one +// status file. Without a file lock their read-merge-write cycles interleaved +// and increments were lost (197 of 400 in the review's repro). +func TestUpdateStatus_ConcurrentWritersDoNotLoseUpdates(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), StatusFileName) + const n = 200 + var wg sync.WaitGroup + for i := 0; i < 2; i++ { + u := &Updater{config: Config{StatusPath: path}} + wg.Add(1) + go func() { + defer wg.Done() + incrementFailures(u, n) + }() + } + wg.Wait() + if got := mustReadStatus(t, path).ConsecutiveFailures; got != 2*n { + t.Fatalf("consecutive_failures = %d, want %d: concurrent writers lost updates", got, 2*n) + } +} + +// TestHelperStatusWriter is not a test: TestUpdateStatus_CrossProcessWriters +// runs it in child processes. +func TestHelperStatusWriter(t *testing.T) { + path := os.Getenv("UPDATER_TEST_STATUS_WRITER_PATH") + if path == "" { + t.Skip("helper process for TestUpdateStatus_CrossProcessWriters") + } + n, err := strconv.Atoi(os.Getenv("UPDATER_TEST_STATUS_WRITER_N")) + if err != nil { + t.Fatal(err) + } + incrementFailures(&Updater{config: Config{StatusPath: path}}, n) +} + +// TestUpdateStatus_CrossProcessWriters runs real separate processes (the +// updater service and `pilotctl update` are separate processes) against one +// status file, alongside this one. +func TestUpdateStatus_CrossProcessWriters(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), StatusFileName) + const n, procs = 100, 3 + cmds := make([]*exec.Cmd, procs) + outs := make([][]byte, procs) + var wg sync.WaitGroup + for i := range cmds { + cmd := exec.Command(os.Args[0], "-test.run=^TestHelperStatusWriter$", "-test.count=1") + cmd.Env = append(os.Environ(), + "UPDATER_TEST_STATUS_WRITER_PATH="+path, + "UPDATER_TEST_STATUS_WRITER_N="+strconv.Itoa(n), + ) + cmds[i] = cmd + wg.Add(1) + go func(i int) { + defer wg.Done() + outs[i], _ = cmd.CombinedOutput() + }(i) + } + incrementFailures(&Updater{config: Config{StatusPath: path}}, n) + wg.Wait() + for i, cmd := range cmds { + if !cmd.ProcessState.Success() { + t.Fatalf("writer %d failed: %s", i, outs[i]) + } + } + if got := mustReadStatus(t, path).ConsecutiveFailures; got != (procs+1)*n { + t.Fatalf("consecutive_failures = %d, want %d: writers in other processes lost updates", got, (procs+1)*n) + } +} + +// TestUpdateStatus_HoldsLockDuringReadModifyWrite: the lock is held while +// mutate runs and released afterwards. +func TestUpdateStatus_HoldsLockDuringReadModifyWrite(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), StatusFileName) + u := &Updater{config: Config{StatusPath: path}} + var heldInside bool + u.updateStatus(func(s *Status) { + heldInside = lockHeld(t, path) + s.LastResult = ResultUpToDate + }) + if !heldInside { + t.Error("status lock not held during read-merge-write") + } + if lockHeld(t, path) { + t.Error("status lock still held after the write") + } + if st := mustReadStatus(t, path); st.LastResult != ResultUpToDate { + t.Errorf("status = %+v", st) + } +} + +// TestUpdateStatus_LockTimeoutWritesUnlocked: a writer that cannot get the +// lock (another process wedged holding it) waits a bounded time, then +// records the outcome anyway. Recording must never block an update. +func TestUpdateStatus_LockTimeoutWritesUnlocked(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), StatusFileName) + release := lockStatusFile(path, time.Second) + defer release() + if !lockHeld(t, path) { + t.Fatal("setup: lock not taken") + } + + const wait = 50 * time.Millisecond + u := &Updater{config: Config{StatusPath: path}, statusLockWait: wait} + start := time.Now() + u.updateStatus(func(s *Status) { s.LastResult = ResultFailed }) + if d := time.Since(start); d < wait { + t.Errorf("returned after %v, before the %v lock wait", d, wait) + } + if st := mustReadStatus(t, path); st.LastResult != ResultFailed { + t.Errorf("status not written after lock timeout: %+v", st) + } +} + +// TestLockStatusFile_ReadOnlyLockFile: a lock file created by another user +// (e.g. `sudo pilotctl update`) is only readable; flock still works on it. +func TestLockStatusFile_ReadOnlyLockFile(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), StatusFileName) + if err := os.WriteFile(path+".lock", nil, 0o444); err != nil { + t.Fatal(err) + } + release := lockStatusFile(path, time.Second) + defer release() + if !lockHeld(t, path) { + t.Error("lock not taken on a read-only lock file") + } +} + +// TestLockStatusFile_Unusable: when the lock file cannot be opened or its +// directory created, the write goes ahead unlocked. +func TestLockStatusFile_Unusable(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := filepath.Join(dir, StatusFileName) + if err := os.Mkdir(path+".lock", 0o755); err != nil { // a directory, not a file + t.Fatal(err) + } + u := &Updater{config: Config{StatusPath: path}} + u.updateStatus(func(s *Status) { s.LastResult = ResultUpToDate }) + if st := mustReadStatus(t, path); st.LastResult != ResultUpToDate { + t.Errorf("status not written with an unusable lock file: %+v", st) + } + + blocker := filepath.Join(dir, "file") + if err := os.WriteFile(blocker, nil, 0o644); err != nil { + t.Fatal(err) + } + release := lockStatusFile(filepath.Join(blocker, "sub", StatusFileName), time.Second) + release() // a no-op, must not panic +} diff --git a/zz_status_test.go b/zz_status_test.go index 574aff7..b10a475 100644 --- a/zz_status_test.go +++ b/zz_status_test.go @@ -22,9 +22,11 @@ import ( ) // restartSpy captures daemon-restart attempts on both platforms: launchctl -// on macOS (runCmd) and SIGTERM via a fake /proc on Linux (killFn). The fake -// /proc holds one daemon whose exe reads " (deleted)", which is what a -// real daemon looks like right after its binary was renamed over. +// on macOS (runCmd) and SIGTERM on Linux (killFn). On Linux the daemon is a +// fake /proc entry whose exe reads " (deleted)", which is what a real +// daemon looks like right after its binary was renamed over. It runs in +// pilot-daemon.service with Restart=always, and after a successful SIGTERM +// the fake systemd starts it again on the installed binary. type restartSpy struct { mu sync.Mutex calls []string @@ -42,21 +44,17 @@ func (r *restartSpy) install(t *testing.T, u *Updater) { } return nil, nil } - proc := t.TempDir() - if err := os.MkdirAll(filepath.Join(proc, "3999999"), 0o755); err != nil { - t.Fatal(err) - } - exe := filepath.Join(u.config.InstallDir, "pilot-daemon") + " (deleted)" - if err := os.Symlink(exe, filepath.Join(proc, "3999999", "exe")); err != nil { - t.Fatal(err) - } - u.procRoot = proc - u.killFn = func(pid int, sig syscall.Signal) error { + sys := newFakeSystem(t, u.config.InstallDir) + sys.addReplacedDaemon(3999999, cgroupPilotService) + sys.writeFile("pilot-daemon.service", daemonUnit(sys.daemon, "always")) + sys.restarts = true + sys.onKill = func(pid int, sig syscall.Signal) error { r.mu.Lock() defer r.mu.Unlock() r.calls = append(r.calls, fmt.Sprintf("kill %d %d", pid, sig)) return r.fail } + sys.attach(u) } func (r *restartSpy) count() int { diff --git a/zz_supervisor_test.go b/zz_supervisor_test.go new file mode 100644 index 0000000..f6dddae --- /dev/null +++ b/zz_supervisor_test.go @@ -0,0 +1,817 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "syscall" + "testing" + "time" +) + +// Cgroup lines as a real /proc//cgroup shows them. +const ( + cgroupPilotService = "0::/system.slice/pilot-daemon.service\n" + cgroupContainer = "0::/\n" + cgroupSession = "0::/user.slice/user-1000.slice/session-3.scope\n" +) + +// fakeSystem is a fake /proc plus a systemd unit directory, for driving the +// Linux restart path on any OS. Its kill hook plays systemd: after a +// successful SIGTERM the old pid disappears and, when restarts is set, a new +// daemon process on the installed binary appears in the same cgroup. +type fakeSystem struct { + t *testing.T + proc string + unitDir string + daemon string // installed daemon binary path + + mu sync.Mutex + nextPid int + kills []string + restarts bool // "systemd" starts the daemon again after SIGTERM + delay time.Duration // how long it takes to do so + onKill func(pid int, sig syscall.Signal) error + restarted int // pid of the new daemon, once started +} + +func newFakeSystem(t *testing.T, installDir string) *fakeSystem { + t.Helper() + fs := &fakeSystem{ + t: t, + proc: t.TempDir(), + unitDir: t.TempDir(), + daemon: filepath.Join(installDir, "pilot-daemon"), + nextPid: 4000000, + } + // Non-pid noise that real /proc has. + if err := os.WriteFile(filepath.Join(fs.proc, "uptime"), []byte("1 1"), 0o644); err != nil { + t.Fatal(err) + } + return fs +} + +// addProc adds a process whose exe link points at exe, with the given +// /proc//cgroup content ("" = no cgroup file). +func (fs *fakeSystem) addProc(pid int, exe, cgroup string) { + fs.t.Helper() + dir := filepath.Join(fs.proc, fmt.Sprint(pid)) + if err := os.MkdirAll(dir, 0o755); err != nil { + fs.t.Fatal(err) + } + if err := os.Symlink(exe, filepath.Join(dir, "exe")); err != nil { + fs.t.Fatal(err) + } + if cgroup != "" { + if err := os.WriteFile(filepath.Join(dir, "cgroup"), []byte(cgroup), 0o644); err != nil { + fs.t.Fatal(err) + } + } +} + +// addReplacedDaemon adds the daemon as it looks right after an update: its +// binary was renamed over, so /proc//exe reads " (deleted)". +func (fs *fakeSystem) addReplacedDaemon(pid int, cgroup string) { + fs.addProc(pid, fs.daemon+" (deleted)", cgroup) +} + +func (fs *fakeSystem) writeFile(rel, content string) { + fs.t.Helper() + p := filepath.Join(fs.unitDir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + fs.t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + fs.t.Fatal(err) + } +} + +func (fs *fakeSystem) alive(pid int) bool { + _, err := os.Lstat(filepath.Join(fs.proc, fmt.Sprint(pid), "exe")) + return err == nil +} + +func (fs *fakeSystem) killCount() int { + fs.mu.Lock() + defer fs.mu.Unlock() + return len(fs.kills) +} + +// attach wires u to the fake system. It does not force the OS: callers that +// want the Linux path on any host set u.goos = "linux". +func (fs *fakeSystem) attach(u *Updater) { + u.procRoot = fs.proc + u.systemdDirs = []string{fs.unitDir} + u.restartWait = 2 * time.Second + u.restartPoll = time.Millisecond + u.killFn = func(pid int, sig syscall.Signal) error { + fs.mu.Lock() + fs.kills = append(fs.kills, fmt.Sprintf("kill %d %d", pid, sig)) + hook, restarts, delay := fs.onKill, fs.restarts, fs.delay + fs.mu.Unlock() + if hook != nil { + if err := hook(pid, sig); err != nil { + return err + } + } + dir := filepath.Join(fs.proc, fmt.Sprint(pid)) + cgroup, _ := os.ReadFile(filepath.Join(dir, "cgroup")) + if err := os.RemoveAll(dir); err != nil { // the old daemon exits + fs.t.Error(err) + } + if restarts { + start := func() { + fs.mu.Lock() + newPid := fs.nextPid + fs.nextPid++ + fs.restarted = newPid + fs.mu.Unlock() + fs.addProc(newPid, fs.daemon, string(cgroup)) + } + if delay > 0 { + time.AfterFunc(delay, start) + } else { + start() + } + } + return nil + } +} + +// daemonUnit is the pilot-daemon.service install.sh writes, with the given +// Restart= policy ("" = no Restart= line). +func daemonUnit(daemon, restart string) string { + unit := "[Unit]\nDescription=Pilot Protocol Daemon\nAfter=network-online.target\n\n" + + "[Service]\nType=simple\nUser=pilot\n" + + "ExecStart=" + daemon + " \\\n" + + " -registry registry.example:9000 \\\n" + + " -listen :4000 \\\n" + + " -encrypt -hostname=node-1\n" + if restart != "" { + unit += "Restart=" + restart + "\n" + } + return unit + "RestartSec=5\n\n[Install]\nWantedBy=multi-user.target\n" +} + +// supervisedLinuxUpdater returns an Updater on the Linux restart path with a +// fake daemon (pid 77, already replaced on disk) in pilot-daemon.service. +func supervisedLinuxUpdater(t *testing.T, restart string) (*Updater, *fakeSystem) { + t.Helper() + installDir := t.TempDir() + fs := newFakeSystem(t, installDir) + fs.addReplacedDaemon(77, cgroupPilotService) + fs.writeFile("pilot-daemon.service", daemonUnit(fs.daemon, restart)) + fs.restarts = true + u := &Updater{config: Config{InstallDir: installDir}, goos: "linux"} + fs.attach(u) + return u, fs +} + +// TestSignalDaemonRestartLinux_UnsupervisedDaemonIsLeftRunning is the core of +// UPD49-1: SIGTERM is only sent when systemd will start the daemon again. A +// daemon started by `pilotctl daemon start` in a container, WSL or CI has no +// supervisor; stopping it took the node offline while the status file said +// "updated". Now it keeps running the old binary and the error says how to +// restart it. +func TestSignalDaemonRestartLinux_UnsupervisedDaemonIsLeftRunning(t *testing.T) { + t.Parallel() + cases := []struct { + name string + cgroup string + unit string // pilot file name -> content written when non-empty + want []string + }{ + { + name: "container, cgroup v2 namespace root", + cgroup: cgroupContainer, + want: []string{"not run by a systemd service", "pilotctl daemon stop && pilotctl daemon start"}, + }, + { + name: "login session scope", + cgroup: cgroupSession, + want: []string{"not run by a systemd service", "pilotctl daemon stop && pilotctl daemon start"}, + }, + { + name: "cgroup v1 login session", + cgroup: "12:pids:/user.slice/user-1000.slice/session-2.scope\n" + + "4:cpu,cpuacct:/user.slice\n" + + "1:name=systemd:/user.slice/user-1000.slice/session-2.scope\n", + want: []string{"not run by a systemd service"}, + }, + { + name: "no cgroup file", + cgroup: "", + want: []string{"cannot tell whether a service manager would start it again", "pilotctl daemon stop"}, + }, + { + name: "child of another service (CI runner with Restart=always)", + cgroup: "0::/system.slice/actions.runner.org.host.service\n", + unit: "[Service]\nExecStart=/home/runner/actions-runner/runsvc.sh\nRestart=always\nKillMode=process\n", + want: []string{"actions.runner.org.host.service", `starts "/home/runner/actions-runner/runsvc.sh"`, "pilotctl daemon stop"}, + }, + { + name: "systemd user service", + cgroup: "0::/user.slice/user-1000.slice/user@1000.service/app.slice/pilot-daemon.service\n", + want: []string{"systemd user service pilot-daemon.service", "systemctl --user restart pilot-daemon"}, + }, + { + name: "service whose unit file cannot be found", + cgroup: "0::/system.slice/pilot-daemon.service\n", + want: []string{"could not be read", "sudo systemctl restart pilot-daemon"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + fs := newFakeSystem(t, installDir) + fs.addReplacedDaemon(77, tc.cgroup) + if tc.unit != "" { + unitName := tc.cgroup[strings.LastIndex(tc.cgroup, "/")+1 : len(tc.cgroup)-1] + fs.writeFile(unitName, tc.unit) + } + fs.restarts = true + u := &Updater{config: Config{InstallDir: installDir}, goos: "linux"} + fs.attach(u) + + err := u.signalDaemonRestart() + if err == nil { + t.Fatal("signalDaemonRestart = nil; an unsupervised daemon must be reported, not stopped") + } + if fs.killCount() != 0 { + t.Errorf("daemon was signalled (%v); nothing would start it again", fs.kills) + } + if !fs.alive(77) { + t.Error("daemon pid 77 is gone") + } + for _, want := range append(tc.want, "left running the old version") { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + }) + } +} + +// TestSignalDaemonRestartLinux_RestartPolicy: pilot-daemon exits 0 on +// SIGTERM, which systemd restarts only under Restart=always / on-success (or +// when RestartForceExitStatus lists it). Units written by install.sh up to +// v1.9.0 say Restart=on-failure and are never rewritten by updates. +func TestSignalDaemonRestartLinux_RestartPolicy(t *testing.T) { + t.Parallel() + cases := []struct { + name string + restart string // Restart= in the unit ("" = none) + files map[string]string // extra unit-dir files (drop-ins) + cgroup string // default cgroupPilotService + fragment string // unit file name, default pilot-daemon.service + signal bool + want string // expected error text when not signalled + }{ + {name: "always (install.sh since web4 1ee4db7d)", restart: "always", signal: true}, + {name: "on-success", restart: "on-success", signal: true}, + {name: "on-failure (install.sh up to v1.9.0)", restart: "on-failure", want: "has Restart=on-failure"}, + {name: "unset", restart: "", want: "has Restart=no"}, + {name: "on-abnormal", restart: "on-abnormal", want: "has Restart=on-abnormal"}, + {name: "upper-case value", restart: "Always", signal: true}, + { + name: "drop-in overrides always with on-failure", restart: "always", + files: map[string]string{"pilot-daemon.service.d/override.conf": "[Service]\nRestart=on-failure\n"}, + want: "has Restart=on-failure", + }, + { + name: "drop-in overrides on-failure with always", restart: "on-failure", + files: map[string]string{"pilot-daemon.service.d/override.conf": "[Service]\nRestart=always\n"}, + signal: true, + }, + { + name: "drop-ins apply in file-name order", restart: "on-failure", + files: map[string]string{ + "pilot-daemon.service.d/20-late.conf": "[Service]\nRestart=always\n", + "pilot-daemon.service.d/10-early.conf": "[Service]\nRestart=no\n", + }, + signal: true, + }, + { + name: "top-level service.d drop-in", restart: "always", + files: map[string]string{"service.d/10-all.conf": "[Service]\nRestart=on-failure\n"}, + want: "has Restart=on-failure", + }, + { + name: "RestartForceExitStatus=0 restarts an on-failure unit", restart: "on-failure", + files: map[string]string{"pilot-daemon.service.d/force.conf": "[Service]\nRestartForceExitStatus=0\n"}, + signal: true, + }, + { + name: "RestartPreventExitStatus=SIGTERM stops always", restart: "always", + files: map[string]string{"pilot-daemon.service.d/prevent.conf": "[Service]\nRestartPreventExitStatus=1 SIGTERM\n"}, + want: "has Restart=always", + }, + { + name: "drop-in replaces ExecStart with a wrapper", restart: "always", + files: map[string]string{"pilot-daemon.service.d/wrap.conf": "[Service]\nExecStart=\nExecStart=/opt/wrap.sh --run\n"}, + want: `starts "/opt/wrap.sh"`, + }, + { + name: "template instance", restart: "always", + cgroup: "0::/system.slice/system-pilot\\x2ddaemon.slice/pilot-daemon@main.service\n", + fragment: "pilot-daemon@.service", signal: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + fs := newFakeSystem(t, installDir) + cgroup := tc.cgroup + if cgroup == "" { + cgroup = cgroupPilotService + } + fragment := tc.fragment + if fragment == "" { + fragment = "pilot-daemon.service" + } + fs.addReplacedDaemon(77, cgroup) + fs.writeFile(fragment, daemonUnit(fs.daemon, tc.restart)) + for rel, content := range tc.files { + fs.writeFile(rel, content) + } + fs.restarts = true + u := &Updater{config: Config{InstallDir: installDir}, goos: "linux"} + fs.attach(u) + + err := u.signalDaemonRestart() + if tc.signal { + if err != nil { + t.Fatalf("signalDaemonRestart: %v", err) + } + if fs.killCount() != 1 || fs.kills[0] != fmt.Sprintf("kill 77 %d", syscall.SIGTERM) { + t.Errorf("kills = %v, want one SIGTERM to 77", fs.kills) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("signalDaemonRestart = %v, want error mentioning %q", err, tc.want) + } + if fs.killCount() != 0 || !fs.alive(77) { + t.Errorf("daemon was stopped (%v) although systemd would not start it again", fs.kills) + } + }) + } +} + +// TestSignalDaemonRestartLinux_OnFailureMessage pins the guidance given for +// the pre-v1.9.0 unit, the most common unsupervised case on systemd hosts. +func TestSignalDaemonRestartLinux_OnFailureMessage(t *testing.T) { + t.Parallel() + u, fs := supervisedLinuxUpdater(t, "on-failure") + err := u.signalDaemonRestart() + if err == nil { + t.Fatal("want an error") + } + for _, want := range []string{ + "restart daemon:", "pid 77", "pilot-daemon.service has Restart=on-failure", + "sudo systemctl restart pilot-daemon", "re-running install.sh", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + if fs.killCount() != 0 { + t.Errorf("kills = %v", fs.kills) + } +} + +// TestSignalDaemonRestartLinux_ReportsDaemonThatDoesNotComeBack: even with +// Restart=always, if no new daemon appears after SIGTERM, the restart failed +// and must be reported rather than recorded as success. +func TestSignalDaemonRestartLinux_ReportsDaemonThatDoesNotComeBack(t *testing.T) { + t.Parallel() + u, fs := supervisedLinuxUpdater(t, "always") + fs.restarts = false + u.restartWait = 50 * time.Millisecond + + err := u.signalDaemonRestart() + if err == nil { + t.Fatal("signalDaemonRestart = nil although the daemon never came back") + } + for _, want := range []string{"sent SIGTERM to the daemon (pid 77)", "did not start it again", "systemctl status pilot-daemon", "sudo systemctl start pilot-daemon"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + if fs.killCount() != 1 { + t.Errorf("kills = %v, want 1", fs.kills) + } +} + +// TestSignalDaemonRestartLinux_WaitsForNewDaemon: systemd starts the daemon +// again after RestartSec; the restart succeeds once the new process runs the +// installed binary, and the old "(deleted)" process never counts. +func TestSignalDaemonRestartLinux_WaitsForNewDaemon(t *testing.T) { + t.Parallel() + u, fs := supervisedLinuxUpdater(t, "always") + fs.delay = 30 * time.Millisecond + // Another process still on a replaced binary must not satisfy the wait. + fs.addReplacedDaemon(78, cgroupPilotService) + fs.onKill = func(pid int, _ syscall.Signal) error { + if pid != 77 { + return fmt.Errorf("unexpected pid %d", pid) + } + return nil + } + + start := time.Now() + if err := u.signalDaemonRestart(); err != nil { + t.Fatalf("signalDaemonRestart: %v", err) + } + if time.Since(start) < fs.delay { + t.Errorf("returned after %v, before the new daemon started", time.Since(start)) + } + fs.mu.Lock() + restarted := fs.restarted + fs.mu.Unlock() + if restarted == 0 || !fs.alive(restarted) { + t.Errorf("new daemon pid %d not running", restarted) + } +} + +// TestSignalDaemonRestartLinux_StopAbortsWait: Stop() must not wait out the +// restart window. +func TestSignalDaemonRestartLinux_StopAbortsWait(t *testing.T) { + t.Parallel() + u, fs := supervisedLinuxUpdater(t, "always") + fs.restarts = false + u.restartWait = time.Minute + u.stopCh = make(chan struct{}) + close(u.stopCh) + + done := make(chan error, 1) + go func() { done <- u.signalDaemonRestart() }() + select { + case err := <-done: + if err == nil || !strings.Contains(err.Error(), "updater stopped") { + t.Errorf("signalDaemonRestart = %v, want stopped error", err) + } + case <-time.After(10 * time.Second): + t.Fatal("signalDaemonRestart did not return after Stop") + } +} + +// TestRunOnce_LinuxUnsupervisedDaemon reproduces UPD49-1 end to end: a +// container node whose daemon was started by `pilotctl daemon start` runs +// `pilotctl update`. The update installs, the daemon keeps running (old +// binary) and the status file says so, instead of "updated" with the node +// silently offline. +func TestRunOnce_LinuxUnsupervisedDaemon(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{"daemon": "new-daemon"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + fs := newFakeSystem(t, u.config.InstallDir) + fs.addReplacedDaemon(3999999, cgroupContainer) + fs.restarts = true + u.goos = "linux" + fs.attach(u) + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if fs.killCount() != 0 || !fs.alive(3999999) { + t.Fatalf("unsupervised daemon was stopped: %v", fs.kills) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultUpdated || st.CurrentVersion != "v2.0.0" { + t.Errorf("status = %+v", st) + } + if !strings.Contains(st.RestartError, "pilotctl daemon stop && pilotctl daemon start") { + t.Errorf("restart_error = %q, want the manual restart command", st.RestartError) + } + if got := u.LastStatus().RestartError; got != st.RestartError { + t.Errorf("LastStatus().RestartError = %q, want %q", got, st.RestartError) + } +} + +// TestRunOnce_LinuxOnFailureUnit: the systemd half of UPD49-1. A unit from +// an old installer (Restart=on-failure) is not stopped; restart_error says +// how to restart it. +func TestRunOnce_LinuxOnFailureUnit(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{"daemon": "new-daemon"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + fs := newFakeSystem(t, u.config.InstallDir) + fs.addReplacedDaemon(3999999, cgroupPilotService) + fs.writeFile("pilot-daemon.service", daemonUnit(fs.daemon, "on-failure")) + fs.restarts = true + u.goos = "linux" + fs.attach(u) + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if fs.killCount() != 0 || !fs.alive(3999999) { + t.Fatalf("daemon under Restart=on-failure was stopped: %v", fs.kills) + } + st := mustReadStatus(t, statusPath) + if st.LastResult != ResultUpdated || !strings.Contains(st.RestartError, "Restart=on-failure") { + t.Errorf("status = %s / restart_error %q", st.LastResult, st.RestartError) + } +} + +// TestRunOnce_LinuxAlwaysUnitRestartsDaemon: the supported install. The +// daemon is stopped, comes back on the new binary, and restart_error is +// empty. +func TestRunOnce_LinuxAlwaysUnitRestartsDaemon(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v2.0.0", map[string]string{"daemon": "new-daemon"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + fs := newFakeSystem(t, u.config.InstallDir) + fs.addReplacedDaemon(3999999, cgroupPilotService) + fs.writeFile("pilot-daemon.service", daemonUnit(fs.daemon, "always")) + fs.restarts = true + u.goos = "linux" + fs.attach(u) + if err := writeStatusFile(statusPath, Status{RestartError: "an old failure"}); err != nil { + t.Fatal(err) + } + + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if fs.killCount() != 1 { + t.Errorf("kills = %v, want 1", fs.kills) + } + if st := mustReadStatus(t, statusPath); st.LastResult != ResultUpdated || st.RestartError != "" { + t.Errorf("status = %s / restart_error %q, want updated with no restart error", st.LastResult, st.RestartError) + } +} + +// TestRecoverPendingRestart_LinuxUnsupervised: the auto path (the updater +// replaced itself; the new process restarts the daemon) applies the same +// rule and records the result. +func TestRecoverPendingRestart_LinuxUnsupervised(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + statusPath := filepath.Join(t.TempDir(), StatusFileName) + if err := os.WriteFile(filepath.Join(installDir, ".pilot-version"), []byte("v2.0.0\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(installDir, "pilot-daemon"), []byte("d"), 0o755); err != nil { + t.Fatal(err) + } + fs := newFakeSystem(t, installDir) + fs.addReplacedDaemon(3999999, cgroupSession) + u := &Updater{config: Config{InstallDir: installDir, StatusPath: statusPath}, goos: "linux"} + fs.attach(u) + + u.recoverPendingRestart() + if fs.killCount() != 0 { + t.Errorf("kills = %v", fs.kills) + } + if st := mustReadStatus(t, statusPath); !strings.Contains(st.RestartError, "not run by a systemd service") { + t.Errorf("restart_error = %q", st.RestartError) + } +} + +// TestRunCheck_ClearsRestartErrorOnceDaemonIsCurrent: restart_error would +// otherwise stay until the next release, long after the operator restarted +// the daemon by hand. A check that finds the daemon on the installed binary +// clears it; one that finds it still on a replaced binary keeps it. +func TestRunCheck_ClearsRestartErrorOnceDaemonIsCurrent(t *testing.T) { + t.Parallel() + srv := newReleaseServer(t, "v1.0.0", map[string]string{"daemon": "d"}) + u, statusPath := newTestUpdater(t, srv.Server, "v1.0.0") + fs := newFakeSystem(t, u.config.InstallDir) + fs.addReplacedDaemon(500, cgroupContainer) + u.goos = "linux" + fs.attach(u) + const restartErr = "restart daemon: daemon (pid 500) left running the old version" + if err := writeStatusFile(statusPath, Status{RestartError: restartErr}); err != nil { + t.Fatal(err) + } + + // Still on the replaced binary: keep the error. + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if st := mustReadStatus(t, statusPath); st.RestartError != restartErr || st.LastResult != ResultUpToDate { + t.Fatalf("restart_error = %q (%s), want kept while the daemon is on the old binary", st.RestartError, st.LastResult) + } + + // Operator ran `pilotctl daemon stop && pilotctl daemon start`. + if err := os.RemoveAll(filepath.Join(fs.proc, "500")); err != nil { + t.Fatal(err) + } + fs.addProc(501, fs.daemon, cgroupContainer) + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if st := mustReadStatus(t, statusPath); st.RestartError != "" { + t.Errorf("restart_error = %q, want cleared once the daemon runs the installed binary", st.RestartError) + } + + // macOS has no /proc to check, so the error is kept there. + if err := writeStatusFile(statusPath, Status{RestartError: restartErr}); err != nil { + t.Fatal(err) + } + u.goos = "darwin" + if err := u.RunOnce(); err != nil { + t.Fatalf("RunOnce: %v", err) + } + if st := mustReadStatus(t, statusPath); st.RestartError != restartErr { + t.Errorf("darwin: restart_error = %q, want kept (no /proc to check)", st.RestartError) + } +} + +func TestServiceUnitOf(t *testing.T) { + t.Parallel() + cases := []struct { + name, cgroup string + unit string + user bool + }{ + {"v2 system service", "0::/system.slice/pilot-daemon.service\n", "pilot-daemon.service", false}, + {"v1 name=systemd", "11:memory:/system.slice/pilot-daemon.service\n1:name=systemd:/system.slice/pilot-daemon.service\n", "pilot-daemon.service", false}, + {"hybrid", "1:name=systemd:/system.slice/pilot-daemon.service\n0::/system.slice/pilot-daemon.service\n", "pilot-daemon.service", false}, + {"delegated sub-cgroup", "0::/system.slice/pilot-daemon.service/payload\n", "pilot-daemon.service", false}, + {"escaped name", "0::/system.slice/_cgroup.service\n", "cgroup.service", false}, + {"user service", "0::/user.slice/user-1000.slice/user@1000.service/app.slice/pilot-daemon.service\n", "pilot-daemon.service", true}, + {"user manager scope", "0::/user.slice/user-1000.slice/user@1000.service/init.scope\n", "", false}, + {"session", cgroupSession, "", false}, + {"container root", cgroupContainer, "", false}, + {"docker from host", "0::/system.slice/docker-0123abcd.scope\n", "", false}, + {"v1 without systemd", "5:cpu,cpuacct:/docker/0123\n", "", false}, + {"garbage", "not a cgroup file", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fs := newFakeSystem(t, t.TempDir()) + fs.addProc(10, "/bin/x", tc.cgroup) + unit, user, err := serviceUnitOf(fs.proc, 10) + if err != nil || unit != tc.unit || user != tc.user { + t.Errorf("serviceUnitOf = %q, %v, %v; want %q, %v", unit, user, err, tc.unit, tc.user) + } + }) + } + if _, _, err := serviceUnitOf(t.TempDir(), 10); err == nil { + t.Error("missing cgroup file must be an error") + } +} + +func TestServiceUnitParse(t *testing.T) { + t.Parallel() + var su serviceUnit + su.parse([]byte(`# comment +[Unit] +Restart=always +ExecStart=/not/the/service + +[Service] +; another comment +ExecStart=-"/opt/pilot bin/pilot-daemon" \ +# a comment inside a continued line is skipped + -listen :4000 \ + -encrypt +Restart = On-Failure +RestartForceExitStatus=1 2 +RestartForceExitStatus= +RestartPreventExitStatus=SIGKILL +ExecStart=/second/command + +[Install] +Restart=always +`)) + if su.execStart != "/opt/pilot bin/pilot-daemon" { + t.Errorf("execStart = %q", su.execStart) + } + if su.restart != "on-failure" || su.restartSetting() != "on-failure" { + t.Errorf("restart = %q", su.restart) + } + if len(su.forceExit) != 0 { + t.Errorf("forceExit = %v, want reset by the empty assignment", su.forceExit) + } + if len(su.preventExit) != 1 || su.preventExit[0] != "SIGKILL" { + t.Errorf("preventExit = %v", su.preventExit) + } + if su.restartsOnCleanExit() { + t.Error("on-failure must not restart after a clean exit") + } + + // An empty ExecStart= resets it; the next one wins. + su.parse([]byte("[Service]\nExecStart=\nExecStart=@/usr/bin/pilot-daemon pilot -x\nRestart=\n")) + if su.execStart != "/usr/bin/pilot-daemon" || su.restartSetting() != "no" { + t.Errorf("after reset: execStart %q restart %q", su.execStart, su.restartSetting()) + } +} + +func TestExecProgramAndTemplateOf(t *testing.T) { + t.Parallel() + for in, want := range map[string]string{ + "/a/pilot-daemon -x": "/a/pilot-daemon", + "-/a/pilot-daemon": "/a/pilot-daemon", + "!!/a/pilot-daemon\t-x": "/a/pilot-daemon", + "+:'/a b/pilot-daemon' x": "/a b/pilot-daemon", + `"/unterminated`: "/unterminated", + " ": "", + } { + if got := execProgram(in); got != want { + t.Errorf("execProgram(%q) = %q, want %q", in, got, want) + } + } + for in, want := range map[string]string{ + "pilot-daemon@main.service": "pilot-daemon@.service", + "pilot-daemon@.service": "", + "pilot-daemon.service": "", + "a.b@c": "", + } { + if got := templateOf(in); got != want { + t.Errorf("templateOf(%q) = %q, want %q", in, got, want) + } + } +} + +func TestSameFile(t *testing.T) { + t.Parallel() + dir := t.TempDir() + real := filepath.Join(dir, "real") + if err := os.MkdirAll(real, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(real, "pilot-daemon"), []byte("d"), 0o755); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + if !sameFile(filepath.Join(link, "pilot-daemon"), filepath.Join(real, "pilot-daemon")) { + t.Error("symlinked install dir must match") + } + if !sameFile(real+"/./pilot-daemon", filepath.Join(real, "pilot-daemon")) { + t.Error("uncleaned path must match") + } + if sameFile("", filepath.Join(real, "pilot-daemon")) || sameFile("/nope/pilot-daemon", filepath.Join(real, "pilot-daemon")) { + t.Error("different or empty program must not match") + } +} + +func TestLoadServiceUnit_Errors(t *testing.T) { + t.Parallel() + if _, err := loadServiceUnit([]string{t.TempDir()}, "pilot-daemon.service"); err == nil { + t.Error("missing unit must be an error") + } + // Higher-priority directory wins for the fragment and for a drop-in of + // the same name. + hi, lo := t.TempDir(), t.TempDir() + for dir, restart := range map[string]string{hi: "always", lo: "no"} { + if err := os.WriteFile(filepath.Join(dir, "p.service"), []byte("[Service]\nExecStart=/x\nRestart="+restart+"\n"), 0o644); err != nil { + t.Fatal(err) + } + } + su, err := loadServiceUnit([]string{hi, lo}, "p.service") + if err != nil || su.restart != "always" || su.fragment != filepath.Join(hi, "p.service") { + t.Fatalf("fragment precedence: %+v, %v", su, err) + } + for dir, restart := range map[string]string{hi: "on-success", lo: "no"} { + if err := os.MkdirAll(filepath.Join(dir, "p.service.d"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "p.service.d", "x.conf"), []byte("[Service]\nRestart="+restart+"\n"), 0o644); err != nil { + t.Fatal(err) + } + } + if su, err := loadServiceUnit([]string{hi, lo}, "p.service"); err != nil || su.restart != "on-success" { + t.Errorf("drop-in precedence: restart %q, %v", su.restart, err) + } +} + +// TestDaemonOnInstalledBinary covers the /proc reading behind clearing +// restart_error. +func TestDaemonOnInstalledBinary(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + fs := newFakeSystem(t, installDir) + u := &Updater{config: Config{InstallDir: installDir}, goos: "linux", procRoot: fs.proc} + if u.daemonOnInstalledBinary() { + t.Error("no daemon running: want false") + } + fs.addProc(20, fs.daemon, "") + if !u.daemonOnInstalledBinary() { + t.Error("daemon on the installed binary: want true") + } + fs.addReplacedDaemon(21, "") + if u.daemonOnInstalledBinary() { + t.Error("a daemon still on a replaced binary: want false") + } + u.procRoot = filepath.Join(fs.proc, "missing") + if u.daemonOnInstalledBinary() { + t.Error("unreadable /proc: want false") + } + if (&Updater{goos: "darwin", procRoot: fs.proc}).daemonOnInstalledBinary() { + t.Error("darwin: want false") + } +} diff --git a/zz_systemd_e2e_test.go b/zz_systemd_e2e_test.go new file mode 100644 index 0000000..07f6421 --- /dev/null +++ b/zz_systemd_e2e_test.go @@ -0,0 +1,230 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +//go:build linux + +package updater + +import ( + "fmt" + "io" + "os" + "os/exec" + "os/signal" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// End-to-end checks of the Linux daemon restart against a real systemd, +// the setup the UPD49-1 review reproduced the bug on. They need root and +// systemd as PID 1, so they are skipped unless PILOT_UPDATER_SYSTEMD_E2E=1. +// To run them in a throwaway container: +// +// GOOS=linux go test -c -o updater.test . +// docker run -d --name upd-e2e --privileged --cgroupns=private \ +// -v "$PWD":/e2e:ro /sbin/init +// docker exec -e PILOT_UPDATER_SYSTEMD_E2E=1 upd-e2e \ +// /e2e/updater.test -test.run 'TestSystemdE2E' -test.v +// +// The fake daemon is this test binary run as TestFakeDaemonProcess: like +// pilot-daemon it exits 0 on SIGTERM. + +const fakeDaemonEnv = "PILOT_UPDATER_FAKE_DAEMON" + +// TestFakeDaemonProcess is not a test: it is the fake daemon process. +func TestFakeDaemonProcess(t *testing.T) { + if os.Getenv(fakeDaemonEnv) == "" { + t.Skip("fake daemon process for TestSystemdE2E_*") + } + sig := make(chan os.Signal, 1) + signal.Notify(sig, syscall.SIGTERM) + <-sig + os.Exit(0) // clean exit, as pilot-daemon does on SIGTERM +} + +func requireSystemdE2E(t *testing.T) { + t.Helper() + if os.Getenv("PILOT_UPDATER_SYSTEMD_E2E") != "1" { + t.Skip("set PILOT_UPDATER_SYSTEMD_E2E=1 to run against a real systemd (root, systemd as PID 1)") + } + if os.Geteuid() != 0 { + t.Skip("needs root") + } + if comm, _ := os.ReadFile("/proc/1/comm"); strings.TrimSpace(string(comm)) != "systemd" { + t.Skip("PID 1 is not systemd") + } +} + +func copyFile(t *testing.T, src, dst string) { + t.Helper() + in, err := os.Open(src) + if err != nil { + t.Fatal(err) + } + defer in.Close() + out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) + if err != nil { + t.Fatal(err) + } + if _, err := io.Copy(out, in); err != nil { + t.Fatal(err) + } + if err := out.Close(); err != nil { + t.Fatal(err) + } +} + +func systemctl(t *testing.T, args ...string) string { + t.Helper() + out, err := exec.Command("systemctl", args...).CombinedOutput() + if err != nil { + t.Fatalf("systemctl %s: %v: %s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) +} + +func mainPID(t *testing.T, unit string) int { + t.Helper() + pid, _ := strconv.Atoi(systemctl(t, "show", "-p", "MainPID", "--value", unit)) + return pid +} + +func exeOf(pid int) string { + exe, _ := os.Readlink(fmt.Sprintf("/proc/%d/exe", pid)) + return exe +} + +// e2eInstall creates an install dir holding the fake daemon, and returns +// the updater for it and the path of a "new release" daemon binary. +func e2eInstall(t *testing.T) (*Updater, string, string) { + t.Helper() + installDir := t.TempDir() + daemon := filepath.Join(installDir, "pilot-daemon") + copyFile(t, os.Args[0], daemon) + newBinary := filepath.Join(t.TempDir(), "pilot-daemon-new") + copyFile(t, os.Args[0], newBinary) + u := &Updater{config: Config{InstallDir: installDir}, restartWait: 20 * time.Second} + return u, daemon, newBinary +} + +// startUnit installs and starts a service running the fake daemon. +func startUnit(t *testing.T, name, daemon, restart string) int { + t.Helper() + unitPath := "/etc/systemd/system/" + name + unit := fmt.Sprintf("[Unit]\nDescription=updater e2e %s\n\n[Service]\nType=simple\n"+ + "Environment=%s=1\nExecStart=%s \\\n -test.run=^TestFakeDaemonProcess$\n"+ + "Restart=%s\nRestartSec=1\n", name, fakeDaemonEnv, daemon, restart) + if err := os.WriteFile(unitPath, []byte(unit), 0o644); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = exec.Command("systemctl", "stop", name).Run() + _ = os.Remove(unitPath) + _ = exec.Command("systemctl", "daemon-reload").Run() + }) + systemctl(t, "daemon-reload") + systemctl(t, "start", name) + deadline := time.Now().Add(10 * time.Second) + for { + if pid := mainPID(t, name); pid != 0 && exeOf(pid) == daemon { + return pid + } + if time.Now().After(deadline) { + t.Fatalf("%s did not start: %s", name, systemctl(t, "show", "-p", "ActiveState,SubState,MainPID", name)) + } + time.Sleep(50 * time.Millisecond) + } +} + +// TestSystemdE2E_RestartAlways: the supported unit. The daemon is stopped +// and systemd brings it back on the new binary. +func TestSystemdE2E_RestartAlways(t *testing.T) { + requireSystemdE2E(t) + u, daemon, newBinary := e2eInstall(t) + const unit = "pilot-e2e-always.service" + oldPid := startUnit(t, unit, daemon, "always") + if err := replaceBinary(newBinary, daemon); err != nil { + t.Fatal(err) + } + if got := exeOf(oldPid); got != daemon+" (deleted)" { + t.Fatalf("after replace, exe = %q", got) + } + + if err := u.signalDaemonRestart(); err != nil { + t.Fatalf("signalDaemonRestart: %v", err) + } + newPid := mainPID(t, unit) + if newPid == 0 || newPid == oldPid || exeOf(newPid) != daemon { + t.Fatalf("after restart: MainPID %d (old %d), exe %q; want a new daemon on %s", newPid, oldPid, exeOf(newPid), daemon) + } + if state := systemctl(t, "show", "-p", "ActiveState", "--value", unit); state != "active" { + t.Errorf("ActiveState = %s", state) + } +} + +// TestSystemdE2E_RestartOnFailure: the unit install.sh wrote up to v1.9.0. +// Before the fix the daemon was stopped and systemd left it down (exit 0 is +// not a failure). Now it keeps running and the error says so. +func TestSystemdE2E_RestartOnFailure(t *testing.T) { + requireSystemdE2E(t) + u, daemon, newBinary := e2eInstall(t) + const unit = "pilot-e2e-onfailure.service" + oldPid := startUnit(t, unit, daemon, "on-failure") + if err := replaceBinary(newBinary, daemon); err != nil { + t.Fatal(err) + } + + err := u.signalDaemonRestart() + if err == nil || !strings.Contains(err.Error(), "Restart=on-failure") { + t.Fatalf("signalDaemonRestart = %v, want the Restart=on-failure error", err) + } + time.Sleep(2 * time.Second) // longer than RestartSec + if pid := mainPID(t, unit); pid != oldPid { + t.Errorf("MainPID = %d, want the untouched daemon %d", pid, oldPid) + } + if state := systemctl(t, "show", "-p", "ActiveState", "--value", unit); state != "active" { + t.Errorf("ActiveState = %s, want active (the node must stay online)", state) + } +} + +// TestSystemdE2E_Unsupervised: a daemon started like `pilotctl daemon +// start` (detached with Setsid, no service) is not stopped. +func TestSystemdE2E_Unsupervised(t *testing.T) { + requireSystemdE2E(t) + u, daemon, newBinary := e2eInstall(t) + cmd := exec.Command(daemon, "-test.run=^TestFakeDaemonProcess$") + cmd.Env = append(os.Environ(), fakeDaemonEnv+"=1") + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + exited := make(chan error, 1) + go func() { exited <- cmd.Wait() }() + t.Cleanup(func() { _ = cmd.Process.Kill() }) + deadline := time.Now().Add(5 * time.Second) + for exeOf(cmd.Process.Pid) != daemon { + if time.Now().After(deadline) { + t.Fatal("fake daemon did not start") + } + time.Sleep(20 * time.Millisecond) + } + if err := replaceBinary(newBinary, daemon); err != nil { + t.Fatal(err) + } + + err := u.signalDaemonRestart() + if err == nil || !strings.Contains(err.Error(), "pilotctl daemon stop && pilotctl daemon start") { + t.Fatalf("signalDaemonRestart = %v, want the unsupervised error", err) + } + select { + case err := <-exited: + t.Fatalf("unsupervised daemon was stopped: %v", err) + case <-time.After(time.Second): + } + if got := exeOf(cmd.Process.Pid); got != daemon+" (deleted)" { + t.Errorf("daemon exe = %q, want it still running the old binary", got) + } +}