Add proper health reporting to cron

cron/trigger/trigger.go

@@ -51,6 +51,7 @@
 }
 
 type Service struct {
+	services.StateMachine
 	capabilities.CapabilityInfo
 	limitsFactory           limits.Factory
 	fastestScheduleInterval limits.TimeLimiter
@@ -178,7 +179,7 @@
 	return nil
 }
 
 func (s *Service) RegisterTrigger(ctx context.Context, triggerID string, metadata capabilities.RequestMetadata, input *crontypedapi.Config) (<-chan capabilities.TriggerAndId[*crontypedapi.Payload], caperrors.Error) {
 	ctx = metadata.ContextWithCRE(ctx)
 	var muCh sync.RWMutex // extra synchronization to prevent the cron task from racing to send on the closed chan and re-register itself
 	// hold the lock until we call triggers.Write
@@ -336,12 +337,10 @@
 			}
 		})
 
-	if s.scheduler == nil {
+	// If service has already started, job will be scheduled immediately
+	if !s.IfNotStopped(func() { job, err = s.scheduler.NewJob(jobDef, task, gocron.WithName(triggerID)) }) {
 		return nil, caperrors.NewPublicSystemError(errors.New("cannot register a new trigger, service has been closed"), caperrors.Internal)
 	}
-
-	// If service has already started, job will be scheduled immediately
-	job, err = s.scheduler.NewJob(jobDef, task, gocron.WithName(triggerID))
 	if err != nil {
 		s.lggr.Errorw("failed to create new job", "err", err)
 		return nil, caperrors.NewPublicSystemError(fmt.Errorf("RegisterTrigger failed to create new job: %s", err), caperrors.Internal)
@@ -352,7 +351,7 @@
 		// errors if job no longer exists on scheduler
 		s.lggr.Errorw("failed to get next run time", "err", err)
 		// ensure that it is out of scheduler
-		err := s.scheduler.RemoveJob(job.ID())
+		s.IfNotStopped(func() { err = s.scheduler.RemoveJob(job.ID()) })
 		return nil, caperrors.NewPublicSystemError(fmt.Errorf("RegisterTrigger failed to remove job: %s", err), caperrors.Internal)
 	}
 
@@ -400,10 +399,10 @@
 	jobID := trigger.job.ID()
 
 	// Remove job from scheduler
-	if s.scheduler == nil {
+	var err error
+	if !s.IfNotStopped(func() { err = s.scheduler.RemoveJob(jobID) }) {
 		return caperrors.NewPublicSystemError(errors.New("cannot unregister a new trigger, service has been closed"), caperrors.Internal)
 	}
-	err := s.scheduler.RemoveJob(jobID)
 	if err != nil {
 		return caperrors.NewPublicSystemError(fmt.Errorf("UnregisterTrigger failed to remove job from scheduler: %s", err), caperrors.Internal)
 	}
@@ -421,10 +420,10 @@
 
 // Start the service.
 func (s *Service) Start(ctx context.Context) error {
-	if s.scheduler == nil {
-		return errors.New("service has shutdown, it must be built again to restart")
-	}
+	return s.StartOnce(s.Name(), s.start)
+}
 
+func (s *Service) start() error {
 	s.scheduler.Start()
 
 	for triggerID, trigger := range s.triggers.ReadAll() {
@@ -449,31 +448,29 @@
 // After this call the Service cannot be started again,
 // The service will need to be re-built to start scheduling again.
 func (s *Service) Close() error {
-	if s.scheduler == nil {
-		return errors.New("service has shutdown, it must be built again to restart")
+	err := s.StopOnce(s.Name(), s.close)
+	if errors.Is(err, services.ErrCannotStopUnstarted) {
+		// gocron runs a goroutine from construction rather than from Start, so a service closed
+		// without having started, e.g. when Initialise fails, still has to shut the scheduler down.
+		return s.close()
 	}
+	return err
+}
 
+func (s *Service) close() error {
 	err := s.scheduler.Shutdown()
 	s.cronLogger.stop()
 	if err != nil {
 		return fmt.Errorf("scheduler shutdown encountered a problem: %s", err)
 	}
 
-	// After .Shutdown() the scheduler cannot be started again,
-	// but calling .Start() on it will not error. Set to nil to mark closed.
-	s.scheduler = nil
-
 	s.lggr.Info(s.Name() + " closed")
 
 	return nil
 }
 
-func (s *Service) Ready() error {
-	return nil
-}
-
 func (s *Service) HealthReport() map[string]error {
-	return map[string]error{s.Name(): nil}
+	return map[string]error{s.Name(): s.Healthy()}
 }
 
 func (s *Service) Name() string {

cron/trigger/trigger_test.go

@@ -1067,6 +1067,29 @@ func TestCronTrigger_UnregisterTriggerError(t *testing.T) {
 	})
 }
 
+func TestCronTrigger_ReadyAndHealthFollowLifecycle(t *testing.T) {
+	ts, err := NewTriggerService(logger.Nop(), clockwork.NewRealClock(), limits.Factory{})
+	require.NoError(t, err)
+
+	require.Error(t, ts.Ready())
+	require.Error(t, ts.HealthReport()[ts.Name()])
+
+	require.NoError(t, ts.Start(t.Context()))
+	require.NoError(t, ts.Ready())
+	require.NoError(t, ts.HealthReport()[ts.Name()])
+
+	require.NoError(t, ts.Close())
+	require.Error(t, ts.Ready())
+	require.Error(t, ts.HealthReport()[ts.Name()])
+}
+
+func TestCronTrigger_CloseBeforeStart(t *testing.T) {
+	ts, err := NewTriggerService(logger.Nop(), clockwork.NewRealClock(), limits.Factory{})
+	require.NoError(t, err)
+
+	require.NoError(t, ts.Close())
+}
+
 func TestCronTrigger_CloseStartErrors(t *testing.T) {
 	fakeClock := clockwork.NewRealClock()
 	ts, err := NewTriggerService(logger.Nop(), fakeClock, limits.Factory{})