Fix lease manager next check scheduling

worker/lease/manager.go

@@ -124,9 +124,6 @@ type Manager struct {
 	// timer tracks when nextTimeout would expire and triggers when it does
 	timer clock.Timer
 
-	// muNextTimeout protects accesses to nextTimeout
-	muNextTimeout sync.Mutex
-
 	// claims is used to deliver lease claim requests to the loop.
 	claims chan claim
 
@@ -579,25 +576,24 @@ func (manager *Manager) computeNextTimeout(leases map[lease.Key]lease.Info) {
 	// occur when the global clock updater ticks the clock, so this avoids
 	// too frequently checking with the potential of having no work to do.
 	// The blanket addition of a second is no big deal.
-	nextTick.Add(time.Second)
+	nextTick = nextTick.Add(time.Second)
 
 	nextDuration := nextTick.Sub(now).Round(time.Millisecond)
 	manager.config.Logger.Tracef("[%s] next expire in %v %v", manager.logContext, nextDuration, nextTick)
 	manager.setNextTimeout(nextTick)
 }
 
 func (manager *Manager) setNextTimeout(t time.Time) {
-	manager.muNextTimeout.Lock()
-	defer manager.muNextTimeout.Unlock()
+	now := manager.config.Clock.Now()
 
 	// Ensure we never walk the next check back without have performed a
-	// scheduled check *unless* we're just starting up.
-	if !manager.nextTimeout.IsZero() && !t.Before(manager.nextTimeout) {
+	// scheduled check *unless* we think our last check was in the past.
+	if !manager.nextTimeout.Before(now) && !t.Before(manager.nextTimeout) {
 		return
 	}
 	manager.nextTimeout = t
 
-	d := t.Sub(manager.config.Clock.Now())
+	d := t.Sub(now)
 	if manager.timer == nil {
 		manager.timer = manager.config.Clock.NewTimer(d)
 	} else {

worker/lease/manager_block_test.go

@@ -72,6 +72,43 @@ func (s *WaitUntilExpiredSuite) TestLeadershipExpires(c *gc.C) {
 	})
 }
 
+func (s *WaitUntilExpiredSuite) TestBlockChecksRescheduled(c *gc.C) {
+	fix := &Fixture{
+		leases: map[corelease.Key]corelease.Info{
+			key("postgresql"): {
+				Holder: "postgresql/0",
+				Expiry: offset(time.Second),
+			},
+			key("mysql"): {
+				Holder: "mysql/0",
+				Expiry: offset(4 * time.Second),
+			},
+			key("redis"): {
+				Holder: "redis/0",
+				Expiry: offset(7 * time.Second),
+			},
+		},
+	}
+	fix.RunTest(c, func(manager *lease.Manager, clock *testclock.Clock) {
+		blockTest := newBlockTest(c, manager, key("redis"))
+		blockTest.assertBlocked(c)
+
+		// Advance past the first expiry.
+		c.Assert(clock.WaitAdvance(3*time.Second, testing.ShortWait, 1), jc.ErrorIsNil)
+		blockTest.assertBlocked(c)
+
+		// Advance past the second expiry. We should have had a check scheduled.
+		c.Assert(clock.WaitAdvance(3*time.Second, testing.ShortWait, 1), jc.ErrorIsNil)
+		blockTest.assertBlocked(c)
+
+		// Advance past the last expiry. We should have had a check scheduled
+		// that causes the redis lease to be unblocked.
+		c.Assert(clock.WaitAdvance(3*time.Second, testing.ShortWait, 1), jc.ErrorIsNil)
+		err := blockTest.assertUnblocked(c)
+		c.Check(err, jc.ErrorIsNil)
+	})
+}
+
 func (s *WaitUntilExpiredSuite) TestLeadershipChanged(c *gc.C) {
 	fix := &Fixture{
 		leases: map[corelease.Key]corelease.Info{