From c5b7b15ce14367715de4dbb89604d15ab966aaf9 Mon Sep 17 00:00:00 2001
From: Oliver Calder <oliver.calder@canonical.com>
Date: Sat, 8 Feb 2025 00:36:06 -0600
Subject: [PATCH] o/snapstate: remove unnecessary check for non-empty snapID
 before installing metadata

The `keepAuxStoreInfo` helper was already a no-op if `snapID == ""`, so
remove the additional check in the calling handler. Additionally, make
the backend store metadata helpers explicitly return `nil` early if
`snapID` is `""`, rather than relying on all functions called within to
guarantee this.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
---
 overlord/snapstate/backend/metadata.go |  5 ++-
 overlord/snapstate/handlers.go         | 49 +++++++++++++++-----------
 2 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/overlord/snapstate/backend/metadata.go b/overlord/snapstate/backend/metadata.go
index ca896c56325..6f2a3145fed 100644
--- a/overlord/snapstate/backend/metadata.go
+++ b/overlord/snapstate/backend/metadata.go
@@ -23,6 +23,9 @@ package backend
 // with the given snap ID. At the moment, this metadata includes auxiliary
 // store information.
 func InstallStoreMetadata(snapID string, aux *AuxStoreInfo) error {
+	if snapID == "" {
+		return nil
+	}
 	if err := keepAuxStoreInfo(snapID, aux); err != nil {
 		return err
 	}
@@ -34,7 +37,7 @@ func InstallStoreMetadata(snapID string, aux *AuxStoreInfo) error {
 // with the given snap ID. At the moment, this metadata includes auxiliary
 // store information. If hasOtherInstances is true, does nothing.
 func DiscardStoreMetadata(snapID string, hasOtherInstances bool) error {
-	if hasOtherInstances {
+	if hasOtherInstances || snapID == "" {
 		return nil
 	}
 	if err := discardAuxStoreInfo(snapID); err != nil {
diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go
index 2aab6ef8c8b..324dbfabd6b 100644
--- a/overlord/snapstate/handlers.go
+++ b/overlord/snapstate/handlers.go
@@ -2389,27 +2389,34 @@ func (m *SnapManager) doLinkSnap(t *state.Task, _ *tomb.Tomb) (err error) {
 		snapst.LastRefreshTime = &now
 	}
 
-	if cand.Snap.SnapID != "" {
-		// write the auxiliary store info
-		aux := &backend.AuxStoreInfo{
-			Media:    snapsup.Media,
-			StoreURL: snapsup.StoreURL,
-			// XXX we store this for the benefit of old snapd
-			Website: snapsup.Website,
-		}
-		err := backend.InstallStoreMetadata(cand.Snap.SnapID, aux)
-		if err != nil {
-			return err
-		}
-		if len(snapst.Sequence.Revisions) == 1 {
-			defer func() {
-				if IsErrAndNotWait(err) {
-					// the install is getting undone, and there are no more of this snap
-					// try to remove the metadata we just installed
-					backend.DiscardStoreMetadata(cand.Snap.SnapID, otherInstances)
-				}
-			}()
-		}
+	// Assemble the auxiliary store info
+	aux := &backend.AuxStoreInfo{
+		Media:    snapsup.Media,
+		StoreURL: snapsup.StoreURL,
+		// XXX we store this for the benefit of old snapd
+		Website: snapsup.Website,
+	}
+	// Write the revision-agnostic store metadata for this snap. If snap ID is
+	// empty, InstallStoreMetadata is a no-op, so no need to check beforehand.
+	// XXX: Previously, err := ... was used and didn't error, indicating that
+	// the err checked in IsErrAndNotWait was the local err (which we know to
+	// be non-nil at time of the check), not the err returned from the
+	// function. Is this analysis correct, and was this intentional?
+	// XXX: Also, why is cand.Snap.SnapID used instead of snapsup.SnapID?
+	// Previously, all of this was inside `if cand.Snap.SnapID != "" {`, and
+	// AFAICT, snapsup.SnapID should never be "".
+	err = backend.InstallStoreMetadata(cand.Snap.SnapID, aux)
+	if err != nil {
+		return err
+	}
+	if len(snapst.Sequence.Revisions) == 1 {
+		defer func() {
+			if IsErrAndNotWait(err) {
+				// the install is getting undone, and there are no more of this snap
+				// try to remove the metadata we just installed
+				backend.DiscardStoreMetadata(cand.Snap.SnapID, otherInstances)
+			}
+		}()
 	}
 
 	// Compatibility with old snapd: check if we have auto-connect task and