dns: cancel extra-records retry on shutdown and close watcher on setup error

After Remove/Rename, the extra-records filewatcher retried with
context.Background and the default 15-minute backoff budget, so Close
could not stop Run. Cancel that retry when closeCh closes. If the file
is still missing after the budget, watch the parent directory so a later
recreate is seen.

Close the fsnotify watcher on NewExtraRecordsManager error paths after
NewWatcher succeeds.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>

(cherry picked from commit a48a42baf4)

hscontrol/dns/extrarecords.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"sync"
 
 	"github.com/cenkalti/backoff/v5"
@@ -37,17 +38,24 @@ func NewExtraRecordsManager(path string) (*ExtraRecordsMan, error) {
 		return nil, fmt.Errorf("creating watcher: %w", err)
 	}
 
+	closeWatcher := func() {
+		_ = watcher.Close()
+	}
+
 	fi, err := os.Stat(path)
 	if err != nil {
+		closeWatcher()
 		return nil, fmt.Errorf("getting file info: %w", err)
 	}
 
 	if fi.IsDir() {
+		closeWatcher()
 		return nil, fmt.Errorf("%w: %s", ErrPathIsDirectory, path)
 	}
 
 	records, hash, err := readExtraRecordsFromPath(path)
 	if err != nil {
+		closeWatcher()
 		return nil, fmt.Errorf("reading extra records from path: %w", err)
 	}
 
@@ -64,6 +72,8 @@ func NewExtraRecordsManager(path string) (*ExtraRecordsMan, error) {
 
 	err = watcher.Add(path)
 	if err != nil {
+		closeWatcher()
+
 		return nil, fmt.Errorf("adding path to watcher: %w", err)
 	}
 
@@ -103,15 +113,21 @@ func (e *ExtraRecordsMan) Run() {
 				// If a file is removed or renamed, fsnotify will lose track of it
 				// and not watch it. We will therefore attempt to re-add it with a backoff.
 			case fsnotify.Remove, fsnotify.Rename:
-				_, err := backoff.Retry(context.Background(), func() (struct{}, error) {
-					if _, err := os.Stat(e.path); err != nil { //nolint:noinlineerr
-						return struct{}{}, err
+				err := e.waitUntilPathExists()
+				if err != nil {
+					select {
+					case <-e.closeCh:
+						return
+					default:
 					}
 
-					return struct{}{}, nil
-				}, backoff.WithBackOff(backoff.NewExponentialBackOff()))
-				if err != nil {
 					log.Error().Caller().Err(err).Msgf("extra records filewatcher retrying to find file after delete")
+
+					addErr := e.watcher.Add(filepath.Dir(e.path))
+					if addErr != nil {
+						log.Error().Caller().Err(addErr).Msgf("extra records filewatcher watching parent after delete failed")
+					}
+
 					continue
 				}
 
@@ -141,6 +157,29 @@ func (e *ExtraRecordsMan) Close() {
 	close(e.closeCh)
 }
 
+func (e *ExtraRecordsMan) waitUntilPathExists() error {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	go func() {
+		select {
+		case <-e.closeCh:
+			cancel()
+		case <-ctx.Done():
+		}
+	}()
+
+	_, err := backoff.Retry(ctx, func() (struct{}, error) {
+		if _, err := os.Stat(e.path); err != nil { //nolint:noinlineerr
+			return struct{}{}, err
+		}
+
+		return struct{}{}, nil
+	}, backoff.WithBackOff(backoff.NewExponentialBackOff()))
+
+	return err
+}
+
 func (e *ExtraRecordsMan) UpdateCh() <-chan []tailcfg.DNSRecord {
 	return e.updateCh
 }

hscontrol/dns/extrarecords_shutdown_test.go

@@ -43,3 +43,32 @@ func TestUpdateRecordsDoesNotBlockShutdown(t *testing.T) {
 		t.Fatal("updateRecords parked on a blocking send and did not return after Close")
 	}
 }
+
+// TestWaitUntilPathExistsReturnsOnClose ensures Close unblocks the
+// Remove/Rename retry. A missing extra-records file must not pin Run
+// for the 15-minute backoff budget after shutdown.
+func TestWaitUntilPathExistsReturnsOnClose(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "extra.json")
+	require.NoError(t, os.WriteFile(path,
+		[]byte(`[{"name":"a.example.com","type":"A","value":"100.64.0.1"}]`), 0o600))
+
+	er, err := NewExtraRecordsManager(path)
+	require.NoError(t, err)
+
+	require.NoError(t, os.Remove(path))
+
+	done := make(chan error, 1)
+
+	go func() {
+		done <- er.waitUntilPathExists()
+	}()
+
+	er.Close()
+
+	select {
+	case err := <-done:
+		require.Error(t, err)
+	case <-time.After(2 * time.Second):
+		t.Fatal("waitUntilPathExists did not return after Close while the extra-records file was missing")
+	}
+}