feat: improvements to config reloader, 100% coverage (#1933)

Increased test coverage of reloader to 100%.

---------

Co-authored-by: Chris Stockton <[email protected]>

Author: cstockton

hack/coverage.sh

@@ -1,6 +1,6 @@
 FAIL=false
 
-for PKG in "crypto"
+for PKG in "crypto" "reloader"
 do
     UNCOVERED_FUNCS=$(go tool cover -func=coverage.out | grep "^github.com/supabase/auth/internal/$PKG/" | grep -v '100.0%$')
     UNCOVERED_FUNCS_COUNT=$(echo "$UNCOVERED_FUNCS" | wc -l)

internal/reloader/handler_race_test.go

@@ -50,6 +50,9 @@ func TestAtomicHandlerRaces(t *testing.T) {
 
 				hr.Store(hrFunc)
 
+				// Calling string should be safe
+				hr.String()
+
 				got := hr.load()
 				_, ok := hrFuncMap[got]
 				if !ok {

internal/reloader/handler_test.go

@@ -2,6 +2,7 @@ package reloader
 
 import (
 	"net/http"
+	"sync/atomic"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -11,23 +12,31 @@ func TestAtomicHandler(t *testing.T) {
 	// for ptr identity
 	type testHandler struct{ http.Handler }
 
+	var calls atomic.Int64
 	hrFn := func() http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			calls.Add(1)
+		})
 	}
 
 	hrFunc1 := &testHandler{hrFn()}
 	hrFunc2 := &testHandler{hrFn()}
 	assert.NotEqual(t, hrFunc1, hrFunc2)
 
 	// a new AtomicHandler should be non-nil
-	hr := NewAtomicHandler(nil)
+	hr := NewAtomicHandler(hrFunc1)
 	assert.NotNil(t, hr)
+	assert.Equal(t, "reloader.AtomicHandler", hr.String())
 
-	// should have no stored handler
+	// should implement http.Handler
 	{
-		hrCur := hr.load()
-		assert.Nil(t, hrCur)
-		assert.Equal(t, true, hrCur == nil)
+		v := (http.Handler)(hr)
+		before := calls.Load()
+		v.ServeHTTP(nil, nil)
+		after := calls.Load()
+		if exp, got := before+1, after; exp != got {
+			t.Fatalf("exp %v to be %v after handler was called", got, exp)
+		}
 	}
 
 	// should be non-nil after store

internal/reloader/reloader.go

@@ -3,8 +3,9 @@ package reloader
 
 import (
 	"context"
-	"log"
+	"errors"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/fsnotify/fsnotify"
@@ -27,28 +28,24 @@ type Reloader struct {
 	watchDir   string
 	reloadIval time.Duration
 	tickerIval time.Duration
+	watchFn    func() (watcher, error)
+	reloadFn   func(dir string) (*conf.GlobalConfiguration, error)
 }
 
 func NewReloader(watchDir string) *Reloader {
 	return &Reloader{
 		watchDir:   watchDir,
 		reloadIval: reloadInterval,
 		tickerIval: tickerInterval,
+		watchFn:    newFSWatcher,
+		reloadFn:   defaultReloadFn,
 	}
 }
 
 // reload attempts to create a new *conf.GlobalConfiguration after loading the
 // currently configured watchDir.
 func (rl *Reloader) reload() (*conf.GlobalConfiguration, error) {
-	if err := conf.LoadDirectory(rl.watchDir); err != nil {
-		return nil, err
-	}
-
-	cfg, err := conf.LoadGlobalFromEnv()
-	if err != nil {
-		return nil, err
-	}
-	return cfg, nil
+	return rl.reloadFn(rl.watchDir)
 }
 
 // reloadCheckAt checks if reloadConfig should be called, returns true if config
@@ -66,9 +63,10 @@ func (rl *Reloader) reloadCheckAt(at, lastUpdate time.Time) bool {
 }
 
 func (rl *Reloader) Watch(ctx context.Context, fn ConfigFunc) error {
-	wr, err := fsnotify.NewWatcher()
+	wr, err := rl.watchFn()
 	if err != nil {
-		log.Fatal(err)
+		logrus.WithError(err).Error("reloader: error creating fsnotify Watcher")
+		return err
 	}
 	defer wr.Close()
 
@@ -77,7 +75,7 @@ func (rl *Reloader) Watch(ctx context.Context, fn ConfigFunc) error {
 
 	// Ignore errors, if watch dir doesn't exist we can add it later.
 	if err := wr.Add(rl.watchDir); err != nil {
-		logrus.WithError(err).Error("watch dir failed")
+		logrus.WithError(err).Error("reloader: error watching config directory")
 	}
 
 	var lastUpdate time.Time
@@ -92,7 +90,7 @@ func (rl *Reloader) Watch(ctx context.Context, fn ConfigFunc) error {
 			// scenarios and wr.WatchList() does not grow which aligns with
 			// the documented behavior.
 			if err := wr.Add(rl.watchDir); err != nil {
-				logrus.WithError(err).Error("watch dir failed")
+				logrus.WithError(err).Error(err)
 			}
 
 			// Check to see if the config is ready to be relaoded.
@@ -105,17 +103,18 @@ func (rl *Reloader) Watch(ctx context.Context, fn ConfigFunc) error {
 
 			cfg, err := rl.reload()
 			if err != nil {
-				logrus.WithError(err).Error("config reload failed")
+				logrus.WithError(err).Error("reloader: error loading config")
 				continue
 			}
 
 			// Call the callback function with the latest cfg.
 			fn(cfg)
 
-		case evt, ok := <-wr.Events:
+		case evt, ok := <-wr.Events():
 			if !ok {
-				logrus.WithError(err).Error("fsnotify has exited")
-				return nil
+				err := errors.New("reloader: fsnotify event channel was closed")
+				logrus.WithError(err).Error(err)
+				return err
 			}
 
 			// We only read files ending in .env
@@ -130,12 +129,95 @@ func (rl *Reloader) Watch(ctx context.Context, fn ConfigFunc) error {
 				evt.Op.Has(fsnotify.Write):
 				lastUpdate = time.Now()
 			}
-		case err, ok := <-wr.Errors:
+		case err, ok := <-wr.Errors():
 			if !ok {
-				logrus.Error("fsnotify has exited")
-				return nil
+				err := errors.New("reloader: fsnotify error channel was closed")
+				logrus.WithError(err).Error(err)
+				return err
 			}
-			logrus.WithError(err).Error("fsnotify has reported an error")
+			logrus.WithError(err).Error(
+				"reloader: fsnotify has reported an error")
 		}
 	}
 }
+
+func defaultReloadFn(dir string) (*conf.GlobalConfiguration, error) {
+	if err := conf.LoadDirectory(dir); err != nil {
+		return nil, err
+	}
+
+	cfg, err := conf.LoadGlobalFromEnv()
+	if err != nil {
+		return nil, err
+	}
+	return cfg, nil
+}
+
+type watcher interface {
+	Add(path string) error
+	Close() error
+	Events() chan fsnotify.Event
+	Errors() chan error
+}
+
+type fsNotifyWatcher struct {
+	wr *fsnotify.Watcher
+}
+
+func newFSWatcher() (watcher, error) {
+	wr, err := fsnotify.NewWatcher()
+	return &fsNotifyWatcher{wr}, err
+}
+
+func (o *fsNotifyWatcher) Add(path string) error       { return o.wr.Add(path) }
+func (o *fsNotifyWatcher) Close() error                { return o.wr.Close() }
+func (o *fsNotifyWatcher) Errors() chan error          { return o.wr.Errors }
+func (o *fsNotifyWatcher) Events() chan fsnotify.Event { return o.wr.Events }
+
+type mockWatcher struct {
+	mu      sync.Mutex
+	err     error
+	eventCh chan fsnotify.Event
+	errorCh chan error
+	addCh   chan string
+}
+
+func newMockWatcher(err error) *mockWatcher {
+	wr := &mockWatcher{
+		err:     err,
+		eventCh: make(chan fsnotify.Event, 1024),
+		errorCh: make(chan error, 1024),
+		addCh:   make(chan string, 1024),
+	}
+	return wr
+}
+
+func (o *mockWatcher) getErr() error {
+	o.mu.Lock()
+	defer o.mu.Unlock()
+	err := o.err
+	return err
+}
+
+func (o *mockWatcher) setErr(err error) {
+	o.mu.Lock()
+	defer o.mu.Unlock()
+	o.err = err
+}
+
+func (o *mockWatcher) Add(path string) error {
+	o.mu.Lock()
+	defer o.mu.Unlock()
+	if err := o.err; err != nil {
+		return err
+	}
+
+	select {
+	case o.addCh <- path:
+	default:
+	}
+	return nil
+}
+func (o *mockWatcher) Close() error                { return o.getErr() }
+func (o *mockWatcher) Events() chan fsnotify.Event { return o.eventCh }
+func (o *mockWatcher) Errors() chan error          { return o.errorCh }

internal/reloader/reloader_test.go

@@ -2,15 +2,279 @@ package reloader
 
 import (
 	"bytes"
-	"log"
+	"context"
+	"errors"
 	"os"
+	"path"
 	"path/filepath"
 	"testing"
 	"time"
 
+	"github.com/fsnotify/fsnotify"
 	"github.com/stretchr/testify/assert"
+	"github.com/supabase/auth/internal/conf"
+	"golang.org/x/sync/errgroup"
 )
 
+func TestWatch(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*10)
+	defer cancel()
+
+	dir, cleanup := helpTestDir(t)
+	defer cleanup()
+
+	// test broken watcher
+	{
+		sentinelErr := errors.New("sentinel")
+		rr := mockReloadRecorder()
+		rl := NewReloader(dir)
+		rl.watchFn = func() (watcher, error) { return nil, sentinelErr }
+
+		err := rl.Watch(ctx, rr.configFn)
+		if exp, got := sentinelErr, err; exp != got {
+			assert.Equal(t, exp, got)
+		}
+	}
+
+	// test watch invalid dir
+	{
+		doneCtx, doneCancel := context.WithCancel(ctx)
+		doneCancel()
+
+		rr := mockReloadRecorder()
+		rl := NewReloader(path.Join(dir, "__not_found__"))
+		err := rl.Watch(doneCtx, rr.configFn)
+		if exp, got := context.Canceled, err; exp != got {
+			assert.Equal(t, exp, got)
+		}
+	}
+
+	// test watch error chan closed
+	{
+		rr := mockReloadRecorder()
+		wr := newMockWatcher(nil)
+		wr.errorCh <- errors.New("sentinel")
+		close(wr.errorCh)
+
+		rl := NewReloader(dir)
+		rl.watchFn = func() (watcher, error) { return wr, nil }
+
+		err := rl.Watch(ctx, rr.configFn)
+		assert.NotNil(t, err)
+
+		msg := "reloader: fsnotify error channel was closed"
+		if exp, got := msg, err.Error(); exp != got {
+			assert.Equal(t, exp, got)
+		}
+	}
+
+	// test watch event chan closed
+	{
+		rr := mockReloadRecorder()
+		wr := newMockWatcher(nil)
+		close(wr.eventCh)
+
+		rl := NewReloader(dir)
+		rl.reloadIval = time.Second / 100
+		rl.watchFn = func() (watcher, error) { return wr, nil }
+
+		err := rl.Watch(ctx, rr.configFn)
+		if err == nil {
+			assert.NotNil(t, err)
+		}
+
+		msg := "reloader: fsnotify event channel was closed"
+		if exp, got := msg, err.Error(); exp != got {
+			assert.Equal(t, exp, got)
+		}
+	}
+
+	// test watch error chan
+	{
+		rr := mockReloadRecorder()
+		wr := newMockWatcher(nil)
+		wr.errorCh <- errors.New("sentinel")
+
+		rl := NewReloader(dir)
+		rl.watchFn = func() (watcher, error) { return wr, nil }
+
+		egCtx, egCancel := context.WithCancel(ctx)
+		defer egCancel()
+
+		var eg errgroup.Group
+		eg.Go(func() error {
+			return rl.Watch(egCtx, rr.configFn)
+		})
+
+		// need to ensure errorCh drains so test isn't racey
+		eg.Go(func() error {
+			defer egCancel()
+
+			tr := time.NewTicker(time.Second / 100)
+			defer tr.Stop()
+
+			for {
+				select {
+				case <-egCtx.Done():
+					return egCtx.Err()
+				case <-tr.C:
+					if len(wr.errorCh) == 0 {
+						return nil
+					}
+				}
+			}
+		})
+
+		err := eg.Wait()
+		if exp, got := context.Canceled, err; exp != got {
+			assert.Equal(t, exp, got)
+		}
+	}
+
+	// test an end to end config reload
+	{
+		rr := mockReloadRecorder()
+		wr := newMockWatcher(nil)
+		rl := NewReloader(dir)
+		rl.watchFn = func() (watcher, error) { return wr, wr.getErr() }
+		rl.reloadFn = rr.reloadFn
+
+		// Need to lower reload ival to pickup config write quicker.
+		rl.reloadIval = time.Second / 10
+		rl.tickerIval = rl.reloadIval / 10
+
+		egCtx, egCancel := context.WithCancel(ctx)
+		defer egCancel()
+
+		var eg errgroup.Group
+		eg.Go(func() error {
+			return rl.Watch(egCtx, rr.configFn)
+		})
+
+		// Copy a full and valid example configuration to trigger Watch
+		{
+			select {
+			case <-egCtx.Done():
+				assert.Nil(t, egCtx.Err())
+			case v := <-wr.addCh:
+				assert.Equal(t, v, dir)
+			}
+
+			name := helpCopyEnvFile(t, dir, "01_example.env", "testdata/50_example.env")
+			wr.eventCh <- fsnotify.Event{
+				Name: name,
+				Op:   fsnotify.Create,
+			}
+			select {
+			case <-egCtx.Done():
+				assert.Nil(t, egCtx.Err())
+			case cfg := <-rr.configCh:
+				assert.NotNil(t, cfg)
+				assert.Equal(t, cfg.External.Apple.Enabled, false)
+			}
+		}
+
+		{
+			drain(rr.configCh)
+			drain(rr.reloadCh)
+
+			name := helpWriteEnvFile(t, dir, "02_example.env", map[string]string{
+				"GOTRUE_EXTERNAL_APPLE_ENABLED": "true",
+			})
+			wr.eventCh <- fsnotify.Event{
+				Name: name,
+				Op:   fsnotify.Create,
+			}
+			select {
+			case <-egCtx.Done():
+				assert.Nil(t, egCtx.Err())
+			case cfg := <-rr.configCh:
+				assert.NotNil(t, cfg)
+				assert.Equal(t, cfg.External.Apple.Enabled, true)
+			}
+		}
+
+		{
+			name := helpWriteEnvFile(t, dir, "03_example.env.bak", map[string]string{
+				"GOTRUE_EXTERNAL_APPLE_ENABLED": "false",
+			})
+			wr.eventCh <- fsnotify.Event{
+				Name: name,
+				Op:   fsnotify.Create,
+			}
+		}
+
+		{
+			// empty the reload ch
+			drain(rr.reloadCh)
+
+			name := helpWriteEnvFile(t, dir, "04_example.env", map[string]string{
+				"GOTRUE_SMTP_PORT": "ABC",
+			})
+			wr.eventCh <- fsnotify.Event{
+				Name: name,
+				Op:   fsnotify.Create,
+			}
+
+			select {
+			case <-egCtx.Done():
+				assert.Nil(t, egCtx.Err())
+			case p := <-rr.reloadCh:
+				if exp, got := dir, p; exp != got {
+					assert.Equal(t, exp, got)
+				}
+			}
+		}
+
+		{
+			name := helpWriteEnvFile(t, dir, "05_example.env", map[string]string{
+				"GOTRUE_SMTP_PORT": "2222",
+			})
+			wr.eventCh <- fsnotify.Event{
+				Name: name,
+				Op:   fsnotify.Create,
+			}
+			select {
+			case <-egCtx.Done():
+				assert.Nil(t, egCtx.Err())
+			case cfg := <-rr.configCh:
+				assert.NotNil(t, cfg)
+				assert.Equal(t, cfg.SMTP.Port, 2222)
+			}
+		}
+
+		// test the wr.Add doesn't exit if bad watch dir is given during tick
+		{
+			// set the error on watcher
+			sentinelErr := errors.New("sentinel")
+			wr.setErr(sentinelErr)
+
+			name := helpWriteEnvFile(t, dir, "05_example.env", map[string]string{
+				"GOTRUE_SMTP_PORT": "2222",
+			})
+			wr.eventCh <- fsnotify.Event{
+				Name: name,
+				Op:   fsnotify.Create,
+			}
+			select {
+			case <-egCtx.Done():
+				assert.Nil(t, egCtx.Err())
+			case cfg := <-rr.configCh:
+				assert.NotNil(t, cfg)
+				assert.Equal(t, cfg.SMTP.Port, 2222)
+			}
+		}
+
+		// test cases ran, end context to unblock Wait()
+		egCancel()
+
+		err := eg.Wait()
+		if exp, got := context.Canceled, err; exp != got {
+			assert.Equal(t, exp, got)
+		}
+	}
+}
+
 func TestReloadConfig(t *testing.T) {
 	dir, cleanup := helpTestDir(t)
 	defer cleanup()
@@ -21,9 +285,7 @@ func TestReloadConfig(t *testing.T) {
 	helpCopyEnvFile(t, dir, "01_example.env", "testdata/50_example.env")
 	{
 		cfg, err := rl.reload()
-		if err != nil {
-			t.Fatal(err)
-		}
+		assert.Nil(t, err)
 		assert.NotNil(t, cfg)
 		assert.Equal(t, cfg.External.Apple.Enabled, false)
 	}
@@ -33,9 +295,7 @@ func TestReloadConfig(t *testing.T) {
 	})
 	{
 		cfg, err := rl.reload()
-		if err != nil {
-			t.Fatal(err)
-		}
+		assert.Nil(t, err)
 		assert.NotNil(t, cfg)
 		assert.Equal(t, cfg.External.Apple.Enabled, true)
 	}
@@ -45,12 +305,30 @@ func TestReloadConfig(t *testing.T) {
 	})
 	{
 		cfg, err := rl.reload()
-		if err != nil {
-			t.Fatal(err)
-		}
+		assert.Nil(t, err)
 		assert.NotNil(t, cfg)
 		assert.Equal(t, cfg.External.Apple.Enabled, true)
 	}
+
+	// test cfg reload failure
+	helpWriteEnvFile(t, dir, "04_example.env", map[string]string{
+		"PORT":             "INVALIDPORT",
+		"GOTRUE_SMTP_PORT": "ABC",
+	})
+	{
+		cfg, err := rl.reload()
+		assert.NotNil(t, err)
+		assert.Nil(t, cfg)
+	}
+
+	// test directory loading failure
+	{
+		cleanup()
+
+		cfg, err := rl.reload()
+		assert.NotNil(t, err)
+		assert.Nil(t, cfg)
+	}
 }
 
 func TestReloadCheckAt(t *testing.T) {
@@ -136,21 +414,21 @@ func helpTestDir(t testing.TB) (dir string, cleanup func()) {
 	dir = filepath.Join("testdata", t.Name())
 	err := os.MkdirAll(dir, 0750)
 	if err != nil && !os.IsExist(err) {
-		t.Fatal(err)
+		assert.Nil(t, err)
 	}
 	return dir, func() { os.RemoveAll(dir) }
 }
 
 func helpCopyEnvFile(t testing.TB, dir, name, src string) string {
 	data, err := os.ReadFile(src) // #nosec G304
 	if err != nil {
-		log.Fatal(err)
+		assert.Nil(t, err)
 	}
 
 	dst := filepath.Join(dir, name)
 	err = os.WriteFile(dst, data, 0600)
 	if err != nil {
-		t.Fatal(err)
+		assert.Nil(t, err)
 	}
 	return dst
 }
@@ -166,8 +444,47 @@ func helpWriteEnvFile(t testing.TB, dir, name string, values map[string]string)
 
 	dst := filepath.Join(dir, name)
 	err := os.WriteFile(dst, buf.Bytes(), 0600)
-	if err != nil {
-		t.Fatal(err)
-	}
+	assert.Nil(t, err)
 	return dst
 }
+
+func mockReloadRecorder() *reloadRecorder {
+	rr := &reloadRecorder{
+		configCh: make(chan *conf.GlobalConfiguration, 1024),
+		reloadCh: make(chan string, 1024),
+	}
+	return rr
+}
+
+func drain[C ~chan T, T any](ch C) (out []T) {
+	for {
+		select {
+		case v := <-ch:
+			out = append(out, v)
+		default:
+			return out
+		}
+	}
+}
+
+type reloadRecorder struct {
+	configCh chan *conf.GlobalConfiguration
+	reloadCh chan string
+}
+
+func (o *reloadRecorder) reloadFn(dir string) (*conf.GlobalConfiguration, error) {
+	defer func() {
+		select {
+		case o.reloadCh <- dir:
+		default:
+		}
+	}()
+	return defaultReloadFn(dir)
+}
+
+func (o *reloadRecorder) configFn(gc *conf.GlobalConfiguration) {
+	select {
+	case o.configCh <- gc:
+	default:
+	}
+}