Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/services/health/health.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
dsReady, err := hm.dsc.ReadyState(ctx)
if err != nil {
log.Ctx(ctx).Warn().Err(err).Msg("could not check if the datastore was ready")
return false

Check warning on line 111 in internal/services/health/health.go

View check run for this annotation

Codecov / codecov/patch

internal/services/health/health.go#L111

Added line #L111 was not covered by tests
Copy link
Contributor

@miparnisari miparnisari Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me, but why aren't we doing both checks in parallel? Say the datastore takes forever to respond, but the dispatcher already errored out, we will be waiting datastoreReadyTimeout when we could have returned "false" very quickly.

It also seems to me that (hm *healthManager) Checker can be simplified by using backoff.Timer.

}

if !dsReady.IsReady {
Expand Down
164 changes: 164 additions & 0 deletions internal/services/health/health_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
package health

import (
"context"
"errors"
"testing"

"github.com/stretchr/testify/require"

"github.com/authzed/spicedb/internal/dispatch"
"github.com/authzed/spicedb/pkg/datastore"
v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
)

// fakeDatastoreChecker implements DatastoreChecker for testing
type fakeDatastoreChecker struct {
readyState datastore.ReadyState
err error
}

func (f *fakeDatastoreChecker) ReadyState(ctx context.Context) (datastore.ReadyState, error) {
return f.readyState, f.err
}

// fakeDispatcher implements dispatch.Dispatcher for testing
type fakeDispatcher struct {
readyState dispatch.ReadyState
}

func (f *fakeDispatcher) ReadyState() dispatch.ReadyState {
return f.readyState
}

func (f *fakeDispatcher) Close() error { return nil }

// The following methods are required by dispatch.Dispatcher interface but not used in health tests
func (f *fakeDispatcher) DispatchCheck(ctx context.Context, req *v1.DispatchCheckRequest) (*v1.DispatchCheckResponse, error) {
return nil, errors.New("not implemented")

Check failure on line 38 in internal/services/health/health_test.go

View workflow job for this annotation

GitHub Actions / Lint Go

In package github.com/authzed/spicedb/internal/services/health: found `nil` returned for value of nil-disallowed type *github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchCheckResponse
}

func (f *fakeDispatcher) DispatchExpand(ctx context.Context, req *v1.DispatchExpandRequest) (*v1.DispatchExpandResponse, error) {
return nil, errors.New("not implemented")

Check failure on line 42 in internal/services/health/health_test.go

View workflow job for this annotation

GitHub Actions / Lint Go

In package github.com/authzed/spicedb/internal/services/health: found `nil` returned for value of nil-disallowed type *github.com/authzed/spicedb/pkg/proto/dispatch/v1.DispatchExpandResponse
}

func (f *fakeDispatcher) DispatchLookupSubjects(req *v1.DispatchLookupSubjectsRequest, stream dispatch.LookupSubjectsStream) error {
return errors.New("not implemented")
}

func (f *fakeDispatcher) DispatchLookupResources2(req *v1.DispatchLookupResources2Request, stream dispatch.LookupResources2Stream) error {
return errors.New("not implemented")
}

func TestNewHealthManager(t *testing.T) {
dispatcher := &fakeDispatcher{}
dsc := &fakeDatastoreChecker{}

manager := NewHealthManager(dispatcher, dsc)

require.NotNil(t, manager)
require.NotNil(t, manager.HealthSvc())
}

func TestHealthManagerRegisterReportedService(t *testing.T) {
dispatcher := &fakeDispatcher{}
dsc := &fakeDatastoreChecker{}
manager := NewHealthManager(dispatcher, dsc)

serviceName := "test-service"
manager.RegisterReportedService(serviceName)

// Verify service was registered by checking the internal map
hm := manager.(*healthManager)
_, exists := hm.serviceNames[serviceName]
require.True(t, exists)
}

func TestHealthManagerCheckIsReady(t *testing.T) {
testCases := []struct {
name string
datastoreReady bool
datastoreError error
dispatcherReady bool
expectedResult bool
}{
{
name: "both ready",
datastoreReady: true,
datastoreError: nil,
dispatcherReady: true,
expectedResult: true,
},
{
name: "datastore not ready",
datastoreReady: false,
datastoreError: nil,
dispatcherReady: true,
expectedResult: false,
},
{
name: "dispatcher not ready",
datastoreReady: true,
datastoreError: nil,
dispatcherReady: false,
expectedResult: false,
},
{
name: "both not ready",
datastoreReady: false,
datastoreError: nil,
dispatcherReady: false,
expectedResult: false,
},
{
name: "datastore error dispatcher ready",
datastoreReady: false, // doesn't matter when there's an error
datastoreError: errors.New("datastore error"),
dispatcherReady: true,
expectedResult: false,
},
{
name: "datastore error dispatcher not ready",
datastoreReady: false, // doesn't matter when there's an error
datastoreError: errors.New("datastore error"),
dispatcherReady: false,
expectedResult: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
dispatcher := &fakeDispatcher{
readyState: dispatch.ReadyState{
IsReady: tc.dispatcherReady,
Message: func() string {
if tc.dispatcherReady {
return "dispatcher ready"
}
return "dispatcher not ready"
}(),
},
}

dsc := &fakeDatastoreChecker{
readyState: datastore.ReadyState{
IsReady: tc.datastoreReady,
Message: func() string {
if tc.datastoreReady {
return "datastore ready"
}
return "datastore not ready"
}(),
},
err: tc.datastoreError,
}

hm := &healthManager{
dispatcher: dispatcher,
dsc: dsc,
}

require.Equal(t, tc.expectedResult, hm.checkIsReady(t.Context()))
})
}
}
Loading