o/snapstate: disable pending refresh notifications when marker interf…

…ace is detected

Avoid sending desktop notifications for pending refreshes if a snap has the marker
interface connected.

This allows delegating those notifications to snaps like snapd-desktop-integration
without suffering from double notifications.

* o/{ifacestate,snapstate}: extract hasActiveConnection helper (thanks @pedronis)

Signed-off-by: Zeyad Gouda <[email protected]>

overlord/ifacestate/export_test.go

@@ -59,6 +59,7 @@ var (
 	AllocHotplugSeq              = allocHotplugSeq
 	AddHotplugSeqWaitTask        = addHotplugSeqWaitTask
 	AddHotplugSlot               = addHotplugSlot
+	HasActiveConnection          = hasActiveConnection
 
 	BatchConnectTasks                = batchConnectTasks
 	FirstTaskAfterBootWhenPreseeding = firstTaskAfterBootWhenPreseeding

overlord/ifacestate/helpers.go

@@ -45,6 +45,10 @@ import (
 	"github.com/snapcore/snapd/timings"
 )
 
+func init() {
+	snapstate.HasActiveConnection = hasActiveConnection
+}
+
 var (
 	snapdAppArmorServiceIsDisabled = snapdAppArmorServiceIsDisabledImpl
 	profilesNeedRegeneration       = profilesNeedRegenerationImpl
@@ -1370,3 +1374,17 @@ func (m *InterfaceManager) discardSecurityProfilesLate(name string, rev snap.Rev
 	}
 	return nil
 }
+
+func hasActiveConnection(st *state.State, iface string) (bool, error) {
+	conns, err := getConns(st)
+	if err != nil {
+		return false, err
+	}
+	for _, cstate := range conns {
+		// look for connected interface
+		if !cstate.Undesired && !cstate.HotplugGone && cstate.Interface == iface {
+			return true, nil
+		}
+	}
+	return false, nil
+}

overlord/ifacestate/helpers_test.go

@@ -770,3 +770,21 @@ func (s *helpersSuite) TestDiscardLateBackendViaSnapstate(c *C) {
 		{"this-fails", "12", "app"},
 	})
 }
+
+func (s *helpersSuite) TestHasActiveConnection(c *C) {
+	s.st.Lock()
+	defer s.st.Unlock()
+
+	s.st.Set("conns", map[string]map[string]string{
+		"consumer-1:browser-support core:browser-support": {"interface": "browser-support"},
+		"consumer-2:home core:home":                       {"interface": "home"},
+	})
+
+	active, err := ifacestate.HasActiveConnection(s.st, "snap-refresh-observe")
+	c.Assert(err, IsNil)
+	c.Check(active, Equals, false)
+
+	active, err = ifacestate.HasActiveConnection(s.st, "browser-support")
+	c.Assert(err, IsNil)
+	c.Check(active, Equals, true)
+}

overlord/snapstate/autorefresh.go

@@ -725,17 +725,34 @@ func getTime(st *state.State, timeKey string) (time.Time, error) {
 //
 // This allows the, possibly slow, communication with each snapd session agent,
 // to be performed without holding the snap state lock.
-var asyncPendingRefreshNotification = func(context context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+var asyncPendingRefreshNotification = func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {
 	logger.Debugf("notifying agents about pending refresh for snap %q", refreshInfo.InstanceName)
-	// TODO: disable this behind refresh-app-awareness-ux experimental flag since
-	// this will be replaced (or used as fallback) when new notices flow is used.
+
 	go func() {
-		if err := client.PendingRefreshNotification(context, refreshInfo); err != nil {
+		client := userclient.New()
+		if err := client.PendingRefreshNotification(ctx, refreshInfo); err != nil {
 			logger.Noticef("Cannot send notification about pending refresh: %v", err)
 		}
 	}()
 }
 
+// maybeAsyncPendingRefreshNotification broadcasts desktop notification in a goroutine.
+//
+// The notification is sent only if no snap has the marker "snap-refresh-observe"
+// interface connected.
+func maybeAsyncPendingRefreshNotification(ctx context.Context, st *state.State, refreshInfo *userclient.PendingSnapRefreshInfo) {
+	markerExists, err := HasActiveConnection(st, "snap-refresh-observe")
+	if err != nil {
+		logger.Noticef("Cannot send notification about pending refresh: %v", err)
+		return
+	}
+	if markerExists {
+		// found snap with marker interface, skip notification
+		return
+	}
+	asyncPendingRefreshNotification(ctx, refreshInfo)
+}
+
 type timedBusySnapError struct {
 	err           *BusySnapError
 	timeRemaining time.Duration
@@ -796,10 +813,11 @@ func inhibitRefresh(st *state.State, snapst *SnapState, snapsup *SnapSetup, info
 		// increasing frequency, allowing the user to understand the urgency.
 		busyErr.timeRemaining = (maxInhibition - now.Sub(*snapst.RefreshInhibitedTime)).Truncate(time.Second)
 	default:
+		// XXX: should we drop this notification?
 		// if the refresh inhibition window has ended, notify the user that the
 		// refresh is happening now and ignore the error
 		refreshInfo := busyErr.PendingSnapRefreshInfo()
-		asyncPendingRefreshNotification(context.TODO(), userclient.New(), refreshInfo)
+		maybeAsyncPendingRefreshNotification(context.TODO(), st, refreshInfo)
 		// important to return "nil" type here instead of
 		// setting busyErr to nil as otherwise we return a nil
 		// interface which is not the nil type

overlord/snapstate/autorefresh_test.go

@@ -989,7 +989,7 @@ func (s *autoRefreshTestSuite) TestInitialInhibitRefreshWithinInhibitWindow(c *C
 	s.state.Lock()
 	defer s.state.Unlock()
 
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {
 		c.Fatal("shouldn't trigger pending refresh notification unless it was an auto-refresh and we're overdue")
 	})
 	defer restore()
@@ -1024,7 +1024,7 @@ func (s *autoRefreshTestSuite) TestSubsequentInhibitRefreshWithinInhibitWindow(c
 	s.state.Lock()
 	defer s.state.Unlock()
 
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {
 		c.Fatal("shouldn't trigger pending refresh notification unless it was an auto-refresh and we're overdue")
 	})
 	defer restore()
@@ -1066,7 +1066,7 @@ func (s *autoRefreshTestSuite) TestInhibitRefreshRefreshesWhenOverdue(c *C) {
 	defer s.state.Unlock()
 
 	notificationCount := 0
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {
 		notificationCount++
 		c.Check(refreshInfo.InstanceName, Equals, "pkg")
 		c.Check(refreshInfo.TimeRemaining, Equals, time.Duration(0))
@@ -1100,7 +1100,7 @@ func (s *autoRefreshTestSuite) TestInhibitNoNotificationOnManualRefresh(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {
 		c.Fatal("shouldn't trigger pending refresh notification if refresh was manual")
 	})
 	defer restore()
@@ -1322,3 +1322,47 @@ func (s *autoRefreshTestSuite) TestMaybeAddRefreshInhibitNotice(c *C) {
 	checkRefreshInhibitNotice(c, st, 2)
 	checkLastRecordedInhibitedSnaps(c, st, []string{"some-other-snap"})
 }
+
+func (s *autoRefreshTestSuite) TestMaybeAsyncPendingRefreshNotification(c *C) {
+	var connCheckCalled int
+	restore := snapstate.MockHasActiveConnection(func(st *state.State, iface string) (bool, error) {
+		connCheckCalled++
+		c.Check(iface, Equals, "snap-refresh-observe")
+		// no snap has the marker interface connected
+		return false, nil
+	})
+	defer restore()
+
+	expectedInfo := &userclient.PendingSnapRefreshInfo{
+		InstanceName:  "pkg",
+		TimeRemaining: 10 * time.Second,
+	}
+	var notificationCalled int
+	restore = snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, psri *userclient.PendingSnapRefreshInfo) {
+		notificationCalled++
+		c.Check(psri, Equals, expectedInfo)
+	})
+	defer restore()
+
+	snapstate.MaybeAsyncPendingRefreshNotification(context.TODO(), s.state, expectedInfo)
+	c.Check(connCheckCalled, Equals, 1)
+	c.Check(notificationCalled, Equals, 1)
+}
+
+func (s *autoRefreshTestSuite) TestMaybeAsyncPendingRefreshNotificationSkips(c *C) {
+	var connCheckCalled int
+	restore := snapstate.MockHasActiveConnection(func(st *state.State, iface string) (bool, error) {
+		connCheckCalled++
+		c.Check(iface, Equals, "snap-refresh-observe")
+		// marker interface found
+		return true, nil
+	})
+	defer restore()
+
+	restore = snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, psri *userclient.PendingSnapRefreshInfo) {
+		c.Fatal("shouldn't trigger pending refresh notification because marker interface is connected")
+	})
+	defer restore()
+
+	snapstate.MaybeAsyncPendingRefreshNotification(context.TODO(), s.state, &userclient.PendingSnapRefreshInfo{})
+}

overlord/snapstate/export_test.go

@@ -270,14 +270,22 @@ func MockLocalInstallLastCleanup(t time.Time) (restore func()) {
 	}
 }
 
-func MockAsyncPendingRefreshNotification(fn func(context.Context, *userclient.Client, *userclient.PendingSnapRefreshInfo)) (restore func()) {
+func MockAsyncPendingRefreshNotification(fn func(context.Context, *userclient.PendingSnapRefreshInfo)) (restore func()) {
 	old := asyncPendingRefreshNotification
 	asyncPendingRefreshNotification = fn
 	return func() {
 		asyncPendingRefreshNotification = old
 	}
 }
 
+func MockHasActiveConnection(fn func(st *state.State, iface string) (bool, error)) (restore func()) {
+	old := HasActiveConnection
+	HasActiveConnection = fn
+	return func() {
+		HasActiveConnection = old
+	}
+}
+
 // re-refresh related
 var (
 	RefreshedSnaps     = refreshedSnaps
@@ -382,10 +390,11 @@ var (
 
 // autorefresh
 var (
-	InhibitRefresh               = inhibitRefresh
-	MaxInhibition                = maxInhibition
-	MaxDuration                  = maxDuration
-	MaybeAddRefreshInhibitNotice = maybeAddRefreshInhibitNotice
+	InhibitRefresh                       = inhibitRefresh
+	MaxInhibition                        = maxInhibition
+	MaxDuration                          = maxDuration
+	MaybeAddRefreshInhibitNotice         = maybeAddRefreshInhibitNotice
+	MaybeAsyncPendingRefreshNotification = maybeAsyncPendingRefreshNotification
 )
 
 type RefreshCandidate = refreshCandidate

overlord/snapstate/handlers.go

@@ -899,7 +899,7 @@ func (m *SnapManager) doPreDownloadSnap(t *state.Task, tomb *tomb.Tomb) error {
 func asyncRefreshOnSnapClose(st *state.State, snapName string, refreshInfo *userclient.PendingSnapRefreshInfo) error {
 	// there's already a goroutine waiting for this snap to close so just notify
 	if IsSnapMonitored(st, snapName) {
-		asyncPendingRefreshNotification(context.TODO(), userclient.New(), refreshInfo)
+		maybeAsyncPendingRefreshNotification(context.TODO(), st, refreshInfo)
 		return nil
 	}
 
@@ -919,7 +919,7 @@ func asyncRefreshOnSnapClose(st *state.State, snapName string, refreshInfo *user
 	}
 
 	// notify the user about the blocked refresh
-	asyncPendingRefreshNotification(context.TODO(), userclient.New(), refreshInfo)
+	maybeAsyncPendingRefreshNotification(context.TODO(), st, refreshInfo)
 
 	go continueRefreshOnSnapClose(st, snapName, done, refreshCtx)
 	return nil

overlord/snapstate/refresh_test.go

@@ -277,7 +277,7 @@ func (s *refreshSuite) TestDoHardRefreshFlowRefreshInhibitionTimeout(c *C) {
 	snapst.RefreshInhibitedTime = &pastInstant
 	snapstate.Set(s.state, snapst.InstanceName(), snapst)
 
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {})
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {})
 	defer restore()
 
 	// Pretend that the snap is running.

overlord/snapstate/snapstate.go

@@ -859,6 +859,10 @@ var AddSnapToQuotaGroup = func(st *state.State, snapName string, quotaGroup stri
 	panic("internal error: snapstate.AddSnapToQuotaGroup is unset")
 }
 
+var HasActiveConnection = func(st *state.State, iface string) (bool, error) {
+	panic("internal error: snapstate.HasActiveConnection is unset")
+}
+
 var generateSnapdWrappers = backend.GenerateSnapdWrappers
 
 // FinishRestart will return a Retry error if there is a pending restart

overlord/snapstate/snapstate_test.go

@@ -337,7 +337,7 @@ SNAPD_APPARMOR_REEXEC=1
 
 	// mock so the actual notification code isn't called. It races with the SetRootDir
 	// call in the TearDown function. It's harmless but triggers go test -race
-	s.AddCleanup(snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.Client, *userclient.PendingSnapRefreshInfo) {}))
+	s.AddCleanup(snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.PendingSnapRefreshInfo) {}))
 }
 
 func (s *snapmgrBaseTest) TearDownTest(c *C) {

overlord/snapstate/snapstate_update_test.go

@@ -10696,7 +10696,7 @@ func (s *snapmgrTestSuite) TestDownloadTaskWaitsForPreDownload(c *C) {
 	defer restore()
 
 	var notified bool
-	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.Client, *userclient.PendingSnapRefreshInfo) {
+	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.PendingSnapRefreshInfo) {
 		notified = true
 	})
 	defer restore()
@@ -10790,7 +10790,7 @@ func (s *snapmgrTestSuite) TestPreDownloadTaskContinuesAutoRefreshIfSoftCheckOk(
 	defer restore()
 
 	var notified bool
-	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.Client, *userclient.PendingSnapRefreshInfo) {
+	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.PendingSnapRefreshInfo) {
 		notified = true
 	})
 	defer restore()
@@ -10905,7 +10905,7 @@ func (s *snapmgrTestSuite) TestDownloadTaskMonitorsSnapStoppedAndNotifiesOnSoftC
 	defer restore()
 
 	var notified bool
-	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.Client, *userclient.PendingSnapRefreshInfo) {
+	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.PendingSnapRefreshInfo) {
 		notified = true
 	})
 	defer restore()
@@ -11009,7 +11009,7 @@ func (s *snapmgrTestSuite) TestDownloadTaskMonitorsRepeated(c *C) {
 	defer restore()
 
 	var notified bool
-	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.Client, *userclient.PendingSnapRefreshInfo) {
+	restore = snapstate.MockAsyncPendingRefreshNotification(func(context.Context, *userclient.PendingSnapRefreshInfo) {
 		notified = true
 	})
 	defer restore()
@@ -11094,7 +11094,7 @@ func (s *snapmgrTestSuite) TestUnlinkMonitorSnapOnHardCheckFailure(c *C) {
 	}}
 
 	var notified bool
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(_ context.Context, _ *userclient.Client, pendingInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(_ context.Context, pendingInfo *userclient.PendingSnapRefreshInfo) {
 		c.Check(pendingInfo.InstanceName, Equals, "some-snap")
 		c.Check(pendingInfo.TimeRemaining, Equals, snapstate.MaxInhibition)
 		notified = true
@@ -11206,7 +11206,7 @@ func (s *snapmgrTestSuite) TestRefreshForcedOnRefreshInhibitionTimeout(c *C) {
 	}
 
 	var notified int
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(_ context.Context, _ *userclient.Client, pendingInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(_ context.Context, pendingInfo *userclient.PendingSnapRefreshInfo) {
 		c.Check(pendingInfo.TimeRemaining, Equals, time.Duration(0))
 		notified++
 	})
@@ -11400,7 +11400,7 @@ func (s *snapmgrTestSuite) TestPreDownloadWithIgnoreRunningRefresh(c *C) {
 		"some-snap": {SnapSetup: *snapsup},
 	})
 
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {})
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {})
 	defer restore()
 
 	restore = snapstate.MockRefreshAppsCheck(func(info *snap.Info) error {
@@ -11496,7 +11496,7 @@ func (s *snapmgrTestSuite) TestPreDownloadCleansSnapDownloads(c *C) {
 		"some-snap": {SnapSetup: *snapsup},
 	})
 
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {})
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {})
 	defer restore()
 
 	restore = snapstate.MockRefreshAppsCheck(func(info *snap.Info) error {
@@ -11584,7 +11584,7 @@ func (s *snapmgrTestSuite) TestMonitoringIsPersistedAndRestored(c *C) {
 	})
 
 	var notified bool
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {})
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {})
 	defer restore()
 
 	restore = snapstate.MockRefreshAppsCheck(func(info *snap.Info) error {
@@ -11672,7 +11672,7 @@ func (s *snapmgrTestSuite) testNoMonitoringWithCands(c *C, cands map[string]*sna
 	s.state.Set("refresh-candidates", cands)
 
 	var notified bool
-	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, refreshInfo *userclient.PendingSnapRefreshInfo) {
 		notified = true
 	})
 	defer restore()