From d2eeb0a1f4feb03c53c7b8535f9429715a75b59b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Thu, 10 Sep 2026 03:31:54 +0200 Subject: [PATCH] [client] Address review findings on the updater mode split Route every mode transition through one locked helper so the staged managed version cannot outlive the decision that created it. Both download-only transitions kept pendingVersion, letting Install put on the old enforced release after management disabled updates or the platform fell back to download-only. The helper also bumps a generation counter. handleUpdate and NotifyUI copy the mode under the mutex but publish and install after releasing it, so a reset landing in that window could still emit an enforced notification. Both now recheck the generation right before the side effect and drop the stale work. Reset on every connection attempt instead of once per run: the backoff loop reconnects without re-entering run, and since disconnects no longer force download-only, a fetch in the disconnected gap republished the enforced notification before the new network map arrived. A missing AutoUpdateSettings now logs that it defaults to download-only rather than claiming auto-update is disabled, which contradicted the notifications that mode still emits. Co-Authored-By: Claude Opus 5 (1M context) --- client/internal/connect.go | 5 +- client/internal/engine.go | 10 ++- client/internal/updater/manager.go | 67 ++++++++++++------- client/internal/updater/manager_linux_test.go | 6 +- client/internal/updater/manager_mode_test.go | 28 ++++++++ 5 files changed, 84 insertions(+), 32 deletions(-) diff --git a/client/internal/connect.go b/client/internal/connect.go index e826335bf..b42ef9818 100644 --- a/client/internal/connect.go +++ b/client/internal/connect.go @@ -274,7 +274,6 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan stateManager.RegisterState(&sshconfig.ShutdownState{}) if c.updateManager != nil { - c.updateManager.ResetMode() c.updateManager.CheckUpdateSuccess(c.ctx) } @@ -293,6 +292,10 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan return nil } + if c.updateManager != nil { + c.updateManager.ResetMode() + } + // suspend connection attempts while the OS reports no usable network if waited, err := c.netMgr.Wait(c.ctx); err != nil { return nil diff --git a/client/internal/engine.go b/client/internal/engine.go index 7374c817a..44bed1e65 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -967,8 +967,14 @@ func (e *Engine) handleAutoUpdateVersion(autoUpdateSettings *mgmProto.AutoUpdate return } - if autoUpdateSettings == nil || autoUpdateSettings.Version == disableAutoUpdate { - log.Infof("auto-update is disabled") + if autoUpdateSettings == nil { + log.Infof("no auto-update settings received, defaulting to download-only") + e.updateManager.SetDownloadOnly() + return + } + + if autoUpdateSettings.Version == disableAutoUpdate { + log.Infof("auto-update is disabled, switching to download-only") e.updateManager.SetDownloadOnly() return } diff --git a/client/internal/updater/manager.go b/client/internal/updater/manager.go index 69cce7a71..d8332b398 100644 --- a/client/internal/updater/manager.go +++ b/client/internal/updater/manager.go @@ -45,6 +45,7 @@ type Manager struct { stateManager *statemanager.Manager mode updateMode + modeGen uint64 forceUpdate bool // true when management sets AlwaysUpdate; skips UI interaction and installs directly lastTrigger time.Time @@ -62,7 +63,7 @@ type Manager struct { pendingVersion *v.Version // updateMutex protects update, expectedVersion, updateToLatestVersion, - // mode, forceUpdate, pendingVersion, and lastTrigger fields + // mode, modeGen, forceUpdate, pendingVersion, and lastTrigger fields updateMutex sync.Mutex // installMutex and installing guard against concurrent installation attempts @@ -158,11 +159,7 @@ func (m *Manager) Start(ctx context.Context) { func (m *Manager) SetDownloadOnly() { m.updateMutex.Lock() - m.mode = modeDownloadOnly - m.forceUpdate = false - m.expectedVersion = nil - m.updateToLatestVersion = false - m.lastTrigger = time.Time{} + m.setModeLocked(modeDownloadOnly) m.updateMutex.Unlock() select { @@ -185,30 +182,26 @@ func (m *Manager) SetVersion(expectedVersion string, forceUpdate bool) { if expectedVersion == "" { log.Errorf("empty expected version provided") - m.expectedVersion = nil - m.updateToLatestVersion = false - m.mode = modeDownloadOnly + m.setModeLocked(modeDownloadOnly) return } - if expectedVersion == latestVersion { - m.updateToLatestVersion = true - m.expectedVersion = nil - } else { - expectedSemVer, err := v.NewVersion(expectedVersion) + var expectedSemVer *v.Version + if expectedVersion != latestVersion { + parsed, err := v.NewVersion(expectedVersion) if err != nil { log.Errorf("error parsing version: %v", err) return } - if m.expectedVersion != nil && m.expectedVersion.Equal(expectedSemVer) { + if m.mode == modeManaged && m.expectedVersion != nil && m.expectedVersion.Equal(parsed) { return } - m.expectedVersion = expectedSemVer - m.updateToLatestVersion = false + expectedSemVer = parsed } - m.lastTrigger = time.Time{} - m.mode = modeManaged + m.setModeLocked(modeManaged) + m.expectedVersion = expectedSemVer + m.updateToLatestVersion = expectedSemVer == nil m.forceUpdate = forceUpdate select { @@ -221,12 +214,7 @@ func (m *Manager) ResetMode() { m.updateMutex.Lock() defer m.updateMutex.Unlock() - m.mode = modeUndecided - m.forceUpdate = false - m.expectedVersion = nil - m.updateToLatestVersion = false - m.pendingVersion = nil - m.lastTrigger = time.Time{} + m.setModeLocked(modeUndecided) } // Install triggers the installation of the pending version. It is called when the user clicks the install button in the UI. @@ -276,6 +264,7 @@ func (m *Manager) NotifyUI() { return } mode := m.mode + gen := m.modeGen pendingVersion := m.pendingVersion latestVersion := m.update.LatestVersion() m.updateMutex.Unlock() @@ -292,6 +281,9 @@ func (m *Manager) NotifyUI() { if err != nil || currentVersion.GreaterThanOrEqual(latestVersion) { return } + if m.modeChanged(gen) { + return + } m.statusRecorder.PublishEvent( cProto.SystemEvent_INFO, cProto.SystemEvent_SYSTEM, @@ -302,7 +294,7 @@ func (m *Manager) NotifyUI() { return } - if pendingVersion != nil { + if pendingVersion != nil && !m.modeChanged(gen) { m.statusRecorder.PublishEvent( cProto.SystemEvent_INFO, cProto.SystemEvent_SYSTEM, @@ -368,6 +360,7 @@ func (m *Manager) handleUpdate(ctx context.Context) { } mode := m.mode + gen := m.modeGen forceUpdate := m.forceUpdate curLatestVersion := m.update.LatestVersion() @@ -407,6 +400,11 @@ func (m *Manager) handleUpdate(ctx context.Context) { } m.updateMutex.Unlock() + if m.modeChanged(gen) { + log.Debugf("auto-update mode changed while checking %s, discarding", updateVersion) + return + } + if mode == modeDownloadOnly { m.statusRecorder.PublishEvent( cProto.SystemEvent_INFO, @@ -434,6 +432,23 @@ func (m *Manager) handleUpdate(ctx context.Context) { ) } +func (m *Manager) modeChanged(gen uint64) bool { + m.updateMutex.Lock() + defer m.updateMutex.Unlock() + + return m.modeGen != gen +} + +func (m *Manager) setModeLocked(mode updateMode) { + m.mode = mode + m.modeGen++ + m.forceUpdate = false + m.expectedVersion = nil + m.updateToLatestVersion = false + m.pendingVersion = nil + m.lastTrigger = time.Time{} +} + func (m *Manager) install(ctx context.Context, pendingVersion *v.Version) error { m.statusRecorder.PublishEvent( cProto.SystemEvent_CRITICAL, diff --git a/client/internal/updater/manager_linux_test.go b/client/internal/updater/manager_linux_test.go index c2df0d39a..a6117818f 100644 --- a/client/internal/updater/manager_linux_test.go +++ b/client/internal/updater/manager_linux_test.go @@ -96,14 +96,14 @@ func Test_SetVersion_FallsBackToDownloadOnly_Linux(t *testing.T) { defer recorder.UnsubscribeFromEvents(sub) m := NewManager(recorder, statemanager.New(tmpFile)) - m.update = &versionUpdateMock{latestVersion: v.Must(v.NewSemver("1.0.1"))} + m.update = &versionUpdateMock{latestVersion: v.Must(v.NewSemver("1.0.5"))} m.currentVersion = "1.0.0" m.Start(context.Background()) m.SetVersion("1.0.1", false) ver, enforced := waitForUpdateEvent(sub, 500*time.Millisecond) - if ver != "1.0.1" { - t.Fatalf("expected download-only event for 1.0.1, got %q", ver) + if ver != "1.0.5" { + t.Fatalf("expected download-only event for fetched 1.0.5, got %q", ver) } if enforced { t.Error("Linux fallback must never have enforced metadata") diff --git a/client/internal/updater/manager_mode_test.go b/client/internal/updater/manager_mode_test.go index 472888268..b7955becc 100644 --- a/client/internal/updater/manager_mode_test.go +++ b/client/internal/updater/manager_mode_test.go @@ -87,3 +87,31 @@ func Test_ResetMode_ReturnsToUndecided(t *testing.T) { t.Fatalf("expected enforced event again after reset, got %q enforced=%v", ver, enforced) } } + +func Test_SetDownloadOnly_ClearsPendingVersion(t *testing.T) { + tmpFile := path.Join(t.TempDir(), "update-test-pending.json") + recorder := peer.NewRecorder("") + sub := recorder.SubscribeToEvents() + defer recorder.UnsubscribeFromEvents(sub) + + m := NewManager(recorder, statemanager.New(tmpFile)) + m.update = &versionUpdateMock{latestVersion: v.Must(v.NewSemver("1.0.1"))} + m.currentVersion = "1.0.0" + m.autoUpdateSupported = func() bool { return true } + m.Start(context.Background()) + defer m.Stop() + + m.SetVersion("1.0.1", false) + if ver, enforced := waitForUpdateEvent(sub, 500*time.Millisecond); ver != "1.0.1" || !enforced { + t.Fatalf("expected enforced event for 1.0.1, got %q enforced=%v", ver, enforced) + } + + m.SetDownloadOnly() + if ver, enforced := waitForUpdateEvent(sub, 500*time.Millisecond); ver != "1.0.1" || enforced { + t.Fatalf("expected download-only event for 1.0.1, got %q enforced=%v", ver, enforced) + } + + if err := m.Install(context.Background()); err == nil { + t.Fatal("Install in download-only mode must not install the staged managed version") + } +}