o/snapstate/autorefresh: do not expect a valid time string in refresh.hold

overlord/snapstate/autorefresh.go

@@ -225,16 +225,18 @@ func (m *autoRefresh) clearRefreshHold() {
 func (m *autoRefresh) AtSeed() error {
 	// on classic hold refreshes for 2h after seeding
 	if release.OnClassic {
-		var t1 time.Time
-		tr := config.NewTransaction(m.state)
-		err := tr.Get("core", "refresh.hold", &t1)
-		if !config.IsNoOption(err) {
-			// already set or error
+		holdTime, err := effectiveRefreshHold(m.state)
+		if err != nil {
 			return err
 		}
+		if !holdTime.IsZero() {
+			// already set
+			return nil
+		}
 		// TODO: have a policy that if the snapd exe itself
 		// is older than X weeks/months we skip the holding?
 		now := time.Now().UTC()
+		tr := config.NewTransaction(m.state)
 		tr.Set("core", "refresh.hold", now.Add(2*time.Hour))
 		tr.Commit()
 		m.nextRefresh = now

overlord/snapstate/autorefresh_test.go

@@ -848,6 +848,54 @@ func (s *autoRefreshTestSuite) TestAtSeedPolicy(c *C) {
 	c.Check(t1.Equal(t2), Equals, true)
 }
 
+func (s *autoRefreshTestSuite) TestAtSeedRefreshHeld(c *C) {
+	// it is possible that refresh.hold has already been set to a valid time
+	// or "forever" even before snapd was started for the first time
+	r := release.MockOnClassic(true)
+	defer r()
+
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	tr := config.NewTransaction(s.state)
+	c.Assert(tr.Set("core", "refresh.hold", "forever"), IsNil)
+	tr.Commit()
+
+	af := snapstate.NewAutoRefresh(s.state)
+	err := af.AtSeed()
+	c.Assert(err, IsNil)
+	c.Check(af.NextRefresh().IsZero(), Equals, true)
+
+	// now use a valid timestamp
+	tr = config.NewTransaction(s.state)
+	c.Assert(tr.Set("core", "refresh.hold", time.Now().UTC()), IsNil)
+	tr.Commit()
+
+	af = snapstate.NewAutoRefresh(s.state)
+	err = af.AtSeed()
+	c.Assert(err, IsNil)
+	c.Check(af.NextRefresh().IsZero(), Equals, true)
+}
+
+func (s *autoRefreshTestSuite) TestAtSeedInvalidHold(c *C) {
+	// it is possible that refresh.hold has already been set to forever even
+	// before snapd was started for the first time
+	r := release.MockOnClassic(true)
+	defer r()
+
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	tr := config.NewTransaction(s.state)
+	c.Assert(tr.Set("core", "refresh.hold", "this-is-invalid-time"), IsNil)
+	tr.Commit()
+
+	af := snapstate.NewAutoRefresh(s.state)
+
+	err := af.AtSeed()
+	c.Assert(err, ErrorMatches, `parsing time "this-is-invalid-time" .*cannot parse.*`)
+}
+
 func (s *autoRefreshTestSuite) TestCanRefreshOnMetered(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()