From 1248a0bd1c14d8359d80aaad8a2daf2045468533 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 9 Aug 2026 00:39:00 +0200 Subject: [PATCH] feat(update-check): wire the Checker into the service, opt-in via env flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third piece of #591. --update-check-enabled/--update-check-interval (UPDATE_CHECK_ENABLED/UPDATE_CHECK_INTERVAL), default off/24h, following the same local main.go flag pattern as discovery-enabled — not pkg/config, which soundtouch-service doesn't import at all (correction to the issue's proposed location, see the design doc). Background goroutine modeled on startDeviceDiscovery: startup jitter (0-5min), skips the immediate check if the persisted last-check is still fresh, backs off retries to no sooner than 1h after a failure, logs once per newly-detected version. The decision logic (shouldCheckImmediately, shouldSkipDueToBackoff, logUpdateIfNewlyAvailable) is split into pure, directly-testable functions rather than living inline in the goroutine. Server gets a SetUpdateChecker/UpdateCheckResult pair (nil-safe) so the next two pieces (announcement, /api/setup/version) can read the current state without importing updatecheck's construction details. Manually verified against a running instance: enabled via flags, no panic, service stays responsive (jitter means the actual first check can take up to 5 minutes to fire, so this only confirms the wiring, not a live GitHub response — that's covered by the previous commit's httptest-backed unit tests). Refs #591 --- cmd/soundtouch-service/main.go | 122 ++++++++++++++++++ cmd/soundtouch-service/update_check_test.go | 112 ++++++++++++++++ .../handlers/handlers_updatecheck_test.go | 34 +++++ pkg/service/handlers/server.go | 26 ++++ 4 files changed, 294 insertions(+) create mode 100644 cmd/soundtouch-service/update_check_test.go create mode 100644 pkg/service/handlers/handlers_updatecheck_test.go diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index f9506d0..9b2e9d6 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "log" + "math/rand" "net" "net/http" "net/url" @@ -32,6 +33,7 @@ import ( "github.com/gesellix/bose-soundtouch/pkg/service/soundtouchweb" "github.com/gesellix/bose-soundtouch/pkg/service/spotify" "github.com/gesellix/bose-soundtouch/pkg/service/stockholm" + "github.com/gesellix/bose-soundtouch/pkg/service/updatecheck" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" "github.com/urfave/cli/v2" @@ -314,6 +316,18 @@ func main() { Value: "5m", EnvVars: []string{"DISCOVERY_INTERVAL"}, }, + &cli.BoolFlag{ + Name: "update-check-enabled", + Usage: "Periodically check GitHub for a newer release (opt-in; the only network call this makes beyond speaker/provider traffic)", + Value: false, + EnvVars: []string{"UPDATE_CHECK_ENABLED"}, + }, + &cli.StringFlag{ + Name: "update-check-interval", + Usage: "Update check interval", + Value: "24h", + EnvVars: []string{"UPDATE_CHECK_INTERVAL"}, + }, &cli.BoolFlag{ Name: "dns-discovery", Usage: "Enable DNS discovery server", @@ -600,6 +614,10 @@ func main() { startDeviceDiscovery(server) + updateChecker := updatecheck.NewChecker(ds, "gesellix/Bose-SoundTouch", version) + server.SetUpdateChecker(updateChecker) + startUpdateCheck(updateChecker, config.updateCheckEnabled, config.updateCheckInterval) + var stockholmHandler *stockholm.Handler if config.stockholmDir != "" { @@ -703,6 +721,8 @@ type serviceConfig struct { tlsExtraHosts []string discoveryEnabled bool discoveryInterval time.Duration + updateCheckEnabled bool + updateCheckInterval time.Duration domains []string spotifyClientID string spotifyClientSecret string @@ -793,6 +813,16 @@ func loadConfig(c *cli.Context) serviceConfig { discoveryInterval = 5 * time.Minute } + updateCheckEnabled := c.Bool("update-check-enabled") + updateCheckIntervalStr := c.String("update-check-interval") + + updateCheckInterval, err := time.ParseDuration(updateCheckIntervalStr) + if err != nil { + log.Printf("Warning: Failed to parse update check interval %s, using default 24h: %v", sanitizeLog(updateCheckIntervalStr), err) + + updateCheckInterval = 24 * time.Hour + } + spotifyClientID := c.String("spotify-client-id") spotifyClientSecret := c.String("spotify-client-secret") spotifyRedirectURI := c.String("spotify-redirect-uri") @@ -842,6 +872,8 @@ func loadConfig(c *cli.Context) serviceConfig { tlsExtraHosts: tlsExtraHosts, discoveryEnabled: discoveryEnabled, discoveryInterval: discoveryInterval, + updateCheckEnabled: updateCheckEnabled, + updateCheckInterval: updateCheckInterval, domains: domains, spotifyClientID: spotifyClientID, spotifyClientSecret: spotifyClientSecret, @@ -1198,6 +1230,96 @@ func startDeviceDiscovery(server *handlers.Server) { }() } +// startUpdateCheck runs the opt-in periodic check against GitHub Releases +// in the background (#591, _/i591/design-update-check.md). Unlike +// discovery, enabled/interval are fixed at startup (env-only for v1, no +// Settings-tab control — see the design doc's answer on that trade-off), +// so they're plain arguments, not read live from Settings each tick. +func startUpdateCheck(checker *updatecheck.Checker, enabled bool, interval time.Duration) { + if !enabled { + return + } + + go func() { + time.Sleep(randomJitter(5 * time.Minute)) + + lastResult := checker.LastResult() + lastLoggedVersion := lastResult.LatestVersion + + var lastErrorAt time.Time + + if shouldCheckImmediately(lastResult.CheckedAt, interval, time.Now()) { + lastErrorAt, lastLoggedVersion = runUpdateCheckTick(checker, lastLoggedVersion) + } + + for { + time.Sleep(interval) + + if shouldSkipDueToBackoff(lastErrorAt, time.Now()) { + continue + } + + lastErrorAt, lastLoggedVersion = runUpdateCheckTick(checker, lastLoggedVersion) + } + }() +} + +// randomJitter returns a random duration in [0, upperBound) — the startup +// delay so many installs restarting together (e.g. after a Docker image +// bump) don't all hit GitHub at once. Not a security-sensitive use of +// randomness. +func randomJitter(upperBound time.Duration) time.Duration { + if upperBound <= 0 { + return 0 + } + + return time.Duration(rand.Int63n(int64(upperBound))) //nolint:gosec +} + +// shouldCheckImmediately reports whether a check should run right at +// startup (after jitter) rather than waiting a full interval: true when +// there's no persisted last-check time, or it's stale (older than one +// interval). Pure/testable — no sleeping. +func shouldCheckImmediately(lastCheckedAt time.Time, interval time.Duration, now time.Time) bool { + return lastCheckedAt.IsZero() || now.Sub(lastCheckedAt) >= interval +} + +// shouldSkipDueToBackoff reports whether a tick should be skipped because +// the last attempt failed less than an hour ago — so a short +// UPDATE_CHECK_INTERVAL doesn't hammer GitHub while it's erroring. A zero +// lastErrorAt means "no recent failure", never skip. Pure/testable. +func shouldSkipDueToBackoff(lastErrorAt, now time.Time) bool { + return !lastErrorAt.IsZero() && now.Sub(lastErrorAt) < time.Hour +} + +// logUpdateIfNewlyAvailable logs once when result reports a version newer +// than lastLoggedVersion, and returns the version to remember as "already +// logged" — unchanged when there's nothing new, so a persistently-available +// update doesn't spam the log every tick. Pure/testable. +func logUpdateIfNewlyAvailable(result updatecheck.Result, lastLoggedVersion string) string { + if result.Available && result.LatestVersion != "" && result.LatestVersion != lastLoggedVersion { + log.Printf("[UpdateCheck] Update available: %s (current %s) — %s", + result.LatestVersion, result.CurrentVersion, result.ReleaseURL) + + return result.LatestVersion + } + + return lastLoggedVersion +} + +// runUpdateCheckTick performs one check, logs on failure, and returns the +// updated (lastErrorAt, lastLoggedVersion) pair for the caller to carry +// into the next iteration. +func runUpdateCheckTick(checker *updatecheck.Checker, lastLoggedVersion string) (time.Time, string) { + result, err := checker.CheckNow(context.Background()) + if err != nil { + log.Printf("[UpdateCheck] check failed: %v", err) + return time.Now(), lastLoggedVersion + } + + return time.Time{}, logUpdateIfNewlyAvailable(result, lastLoggedVersion) +} + // newEmbeddedWebApp builds the soundtouch-player application for embedding in the // service router: release metadata from the build vars, the service's public // ServiceURL (used by Play URL for speaker-fetched stream URLs and shown in the diff --git a/cmd/soundtouch-service/update_check_test.go b/cmd/soundtouch-service/update_check_test.go new file mode 100644 index 0000000..04253a4 --- /dev/null +++ b/cmd/soundtouch-service/update_check_test.go @@ -0,0 +1,112 @@ +package main + +import ( + "testing" + "time" + + "github.com/gesellix/bose-soundtouch/pkg/service/updatecheck" +) + +func TestShouldCheckImmediately(t *testing.T) { + now := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + interval := 24 * time.Hour + + cases := []struct { + name string + lastCheckedAt time.Time + want bool + }{ + {"never checked", time.Time{}, true}, + {"stale (older than interval)", now.Add(-25 * time.Hour), true}, + {"exactly one interval ago", now.Add(-interval), true}, + {"recent (within interval)", now.Add(-1 * time.Hour), false}, + } + + for _, tc := range cases { + if got := shouldCheckImmediately(tc.lastCheckedAt, interval, now); got != tc.want { + t.Errorf("%s: shouldCheckImmediately() = %v, want %v", tc.name, got, tc.want) + } + } +} + +func TestShouldSkipDueToBackoff(t *testing.T) { + now := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + + cases := []struct { + name string + lastErrorAt time.Time + want bool + }{ + {"no recent failure", time.Time{}, false}, + {"failed 30 minutes ago", now.Add(-30 * time.Minute), true}, + {"failed exactly 1 hour ago", now.Add(-time.Hour), false}, + {"failed 2 hours ago", now.Add(-2 * time.Hour), false}, + } + + for _, tc := range cases { + if got := shouldSkipDueToBackoff(tc.lastErrorAt, now); got != tc.want { + t.Errorf("%s: shouldSkipDueToBackoff() = %v, want %v", tc.name, got, tc.want) + } + } +} + +func TestLogUpdateIfNewlyAvailable(t *testing.T) { + cases := []struct { + name string + result updatecheck.Result + lastLoggedVersion string + want string + }{ + { + name: "nothing available", + result: updatecheck.Result{Available: false}, + lastLoggedVersion: "", + want: "", + }, + { + name: "newly available", + result: updatecheck.Result{Available: true, LatestVersion: "v1.1.0"}, + lastLoggedVersion: "", + want: "v1.1.0", + }, + { + name: "already logged this version", + result: updatecheck.Result{Available: true, LatestVersion: "v1.1.0"}, + lastLoggedVersion: "v1.1.0", + want: "v1.1.0", + }, + { + name: "a newer version than what was logged", + result: updatecheck.Result{Available: true, LatestVersion: "v1.2.0"}, + lastLoggedVersion: "v1.1.0", + want: "v1.2.0", + }, + } + + for _, tc := range cases { + if got := logUpdateIfNewlyAvailable(tc.result, tc.lastLoggedVersion); got != tc.want { + t.Errorf("%s: logUpdateIfNewlyAvailable() = %q, want %q", tc.name, got, tc.want) + } + } +} + +func TestRandomJitter(t *testing.T) { + if got := randomJitter(0); got != 0 { + t.Errorf("randomJitter(0) = %v, want 0", got) + } + + upperBound := 5 * time.Minute + for i := 0; i < 20; i++ { + got := randomJitter(upperBound) + if got < 0 || got >= upperBound { + t.Fatalf("randomJitter(%v) = %v, want in [0, %v)", upperBound, got, upperBound) + } + } +} + +func TestStartUpdateCheck_DisabledIsANoOp(t *testing.T) { + // Must return immediately without spawning anything that could touch a + // nil-repo Checker or block — disabled is the default, so this path + // runs on every install that hasn't opted in. + startUpdateCheck(nil, false, time.Hour) +} diff --git a/pkg/service/handlers/handlers_updatecheck_test.go b/pkg/service/handlers/handlers_updatecheck_test.go new file mode 100644 index 0000000..199b515 --- /dev/null +++ b/pkg/service/handlers/handlers_updatecheck_test.go @@ -0,0 +1,34 @@ +package handlers + +import ( + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/service/updatecheck" +) + +// TestUpdateCheckResult_NilCheckerIsSafe verifies the default (opt-in +// checker never registered) returns a safe zero value rather than +// panicking — the common case, since UPDATE_CHECK_ENABLED defaults to +// false. +func TestUpdateCheckResult_NilCheckerIsSafe(t *testing.T) { + s := NewServer(nil, nil, "http://localhost", false, false, false) + + result := s.UpdateCheckResult() + if result.Available { + t.Error("Expected a nil checker to report Available=false") + } +} + +// TestUpdateCheckResult_ReflectsRegisteredChecker verifies SetUpdateChecker +// wires the checker in and UpdateCheckResult reads through to it. +func TestUpdateCheckResult_ReflectsRegisteredChecker(t *testing.T) { + s := NewServer(nil, nil, "http://localhost", false, false, false) + + checker := updatecheck.NewChecker(nil, "owner/repo", "v1.0.0") + s.SetUpdateChecker(checker) + + result := s.UpdateCheckResult() + if result.CurrentVersion != "v1.0.0" { + t.Errorf("Expected UpdateCheckResult to read through to the registered checker, got %+v", result) + } +} diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 9010a49..94380ba 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -30,6 +30,7 @@ import ( "github.com/gesellix/bose-soundtouch/pkg/service/setup" "github.com/gesellix/bose-soundtouch/pkg/service/spotify" "github.com/gesellix/bose-soundtouch/pkg/service/tts" + "github.com/gesellix/bose-soundtouch/pkg/service/updatecheck" "github.com/gesellix/bose-soundtouch/pkg/ssh" "github.com/miekg/dns" ) @@ -70,6 +71,7 @@ type Server struct { mgmtPassword string adminAreaAuth string // "" (unset) / "enabled" / "disabled" — see datastore.Settings.AdminAreaAuth dismissedAnnouncements map[string]time.Time // announcement id -> most recent dismissal; see RecordDismissal + updateChecker *updatecheck.Checker // nil unless SetUpdateChecker was called (opt-in, see #591) spotifyClientID string spotifyClientSecret string spotifyRedirectURI string @@ -1126,6 +1128,30 @@ func (s *Server) IsAnnouncementDismissed(id string) bool { return ok } +// SetUpdateChecker registers the opt-in periodic update checker (#591). Not +// called at all unless UPDATE_CHECK_ENABLED — leave nil otherwise; +// UpdateCheckResult handles that safely. +func (s *Server) SetUpdateChecker(c *updatecheck.Checker) { + s.mu.Lock() + defer s.mu.Unlock() + + s.updateChecker = c +} + +// UpdateCheckResult returns the last known update-check result, or the +// zero value (Available: false) if the check was never enabled. +func (s *Server) UpdateCheckResult() updatecheck.Result { + s.mu.RLock() + checker := s.updateChecker + s.mu.RUnlock() + + if checker == nil { + return updatecheck.Result{} + } + + return checker.LastResult() +} + // SetInternalPaths sets the internal paths for the server. func (s *Server) SetInternalPaths(paths []string) { s.mu.Lock()