Merge pull request #50 from endorama/fix-global-failed

Fix global failed status when multiple fatal checks have mixed results

health.go

@@ -97,6 +97,9 @@ type State struct {
 	// Err is the error returned from a failed health check
 	Err string `json:"error,omitempty"`
 
+	// Fatal shows if the check will affect global result
+	Fatal bool `json:"fatal,omitempty"`
+
 	// Details contains more contextual detail about a
 	// failing health check.
 	Details interface{} `json:"details,omitempty"` // contains JSON message (that can be marshaled)
@@ -121,7 +124,6 @@ type Health struct {
 	StatusListener IStatusListener
 
 	active     *sBool // indicates whether the healthcheck is actively running
-	failed     *sBool // indicates whether the healthcheck has encountered a fatal error in one of its deps
 	configs    []*Config
 	states     map[string]State
 	statesLock sync.Mutex
@@ -136,7 +138,6 @@ func New() *Health {
 		states:     make(map[string]State, 0),
 		runners:    make(map[string]chan struct{}, 0),
 		active:     newBool(),
-		failed:     newBool(), // init as false
 		statesLock: sync.Mutex{},
 	}
 }
@@ -227,13 +228,18 @@ func (h *Health) Stop() error {
 //
 // The map key is the name of the check.
 func (h *Health) State() (map[string]State, bool, error) {
-	return h.safeGetStates(), h.failed.val(), nil
+	return h.safeGetStates(), h.Failed(), nil
 }
 
 // Failed will return the basic state of overall health. This should be used when
 // details about the failure are not needed
 func (h *Health) Failed() bool {
-	return h.failed.val()
+	for _, val := range h.safeGetStates() {
+		if val.Fatal && val.isFailure() {
+			return true
+		}
+	}
+	return false
 }
 
 func (h *Health) startRunner(cfg *Config, ticker *time.Ticker, stop <-chan struct{}) {
@@ -247,6 +253,7 @@ func (h *Health) startRunner(cfg *Config, ticker *time.Ticker, stop <-chan struc
 			Status:    "ok",
 			Details:   data,
 			CheckTime: time.Now(),
+			Fatal:     cfg.Fatal,
 		}
 
 		if err != nil {
@@ -260,11 +267,6 @@ func (h *Health) startRunner(cfg *Config, ticker *time.Ticker, stop <-chan struc
 			stateEntry.Status = "failed"
 		}
 
-		// Toggle the global failed state if check is configured as fatal
-		if cfg.Fatal {
-			h.failed.set(err != nil)
-		}
-
 		h.safeUpdateState(stateEntry)
 	}
 

health_test.go

@@ -169,8 +169,17 @@ func TestFailed(t *testing.T) {
 		h := setupNewTestHealth()
 		checker1 := &fakes.FakeICheckable{}
 		checker1.StatusReturns(nil, fmt.Errorf("things broke"))
+		checker2 := &fakes.FakeICheckable{}
+		checker2.StatusReturns(nil, nil)
 
 		cfgs := []*Config{
+			// order *is* relevant, failing check should be first
+			{
+				Name:     "bar",
+				Checker:  checker2,
+				Interval: testCheckInterval,
+				Fatal:    true,
+			},
 			{
 				Name:     "foo",
 				Checker:  checker1,
@@ -235,8 +244,17 @@ func TestState(t *testing.T) {
 		h := setupNewTestHealth()
 		checker1 := &fakes.FakeICheckable{}
 		checker1.StatusReturns(nil, fmt.Errorf("things broke"))
+		checker2 := &fakes.FakeICheckable{}
+		checker2.StatusReturns(nil, nil)
 
 		cfgs := []*Config{
+			// order *is* relevant, failing check should be first
+			{
+				Name:     "bar",
+				Checker:  checker2,
+				Interval: testCheckInterval,
+				Fatal:    true,
+			},
 			{
 				Name:     "foo",
 				Checker:  checker1,
@@ -415,7 +433,7 @@ func TestStartRunner(t *testing.T) {
 		}
 
 		// Since nothing has failed, healthcheck should _not_ be in failed state
-		Expect(h.failed.val()).To(BeFalse())
+		Expect(h.Failed()).To(BeFalse())
 	})
 
 	t.Run("Happy path - no checkers is noop", func(t *testing.T) {
@@ -470,7 +488,7 @@ func TestStartRunner(t *testing.T) {
 		Expect(h.states[cfgs[1].Name].Err).To(Equal(checker2Error.Error()))
 
 		// Since nothing has failed, healthcheck should _not_ be in failed state
-		Expect(h.failed.val()).To(BeFalse())
+		Expect(h.Failed()).To(BeFalse())
 	})
 
 	t.Run("Happy path - 1 checker fails (fatal)", func(t *testing.T) {
@@ -514,7 +532,7 @@ func TestStartRunner(t *testing.T) {
 		Expect(h.states[cfgs[1].Name].Err).To(Equal(checker2Err.Error()))
 
 		// Since second checker has failed fatally, global healthcheck state should be failed as well
-		Expect(h.failed.val()).To(BeTrue())
+		Expect(h.Failed()).To(BeTrue())
 	})
 }