From 9f0c74e73a50d0bc55c6ee72c7b98dd445f01a48 Mon Sep 17 00:00:00 2001
From: Kristoffer Dalby <kristoffer@tailscale.com>
Date: Fri, 5 Jun 2026 15:19:27 +0000
Subject: [PATCH] dns: release lock before extra-records channel send

A blocked send held the write lock and leaked the watcher at shutdown.
---
 hscontrol/dns/extrarecords.go               | 14 ++++++-
 hscontrol/dns/extrarecords_shutdown_test.go | 45 +++++++++++++++++++++
 2 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 hscontrol/dns/extrarecords_shutdown_test.go

diff --git a/hscontrol/dns/extrarecords.go b/hscontrol/dns/extrarecords.go
index 7096fafd..5dfc223d 100644
--- a/hscontrol/dns/extrarecords.go
+++ b/hscontrol/dns/extrarecords.go
@@ -158,11 +158,12 @@ func (e *ExtraRecordsMan) updateRecords() {
 	}
 
 	e.mu.Lock()
-	defer e.mu.Unlock()
 
 	// If there has not been any change, ignore the update.
 	if oldHash, ok := e.hashes[e.path]; ok {
 		if newHash == oldHash {
+			e.mu.Unlock()
+
 			return
 		}
 	}
@@ -171,10 +172,19 @@ func (e *ExtraRecordsMan) updateRecords() {
 
 	e.records = set.SetOf(records)
 	e.hashes[e.path] = newHash
+	toSend := e.records.Slice()
 
 	log.Trace().Caller().Interface("records", e.records).Msgf("extra records updated from path, count old: %d, new: %d", oldCount, e.records.Len())
 
-	e.updateCh <- e.records.Slice()
+	// Release the lock before the (potentially blocking) send so a slow or
+	// absent consumer cannot stall Records() readers, and abort the send on
+	// shutdown instead of leaking this goroutine on the closed-down channel.
+	e.mu.Unlock()
+
+	select {
+	case e.updateCh <- toSend:
+	case <-e.closeCh:
+	}
 }
 
 // readExtraRecordsFromPath reads a JSON file of [tailcfg.DNSRecord]
diff --git a/hscontrol/dns/extrarecords_shutdown_test.go b/hscontrol/dns/extrarecords_shutdown_test.go
new file mode 100644
index 00000000..457d58c5
--- /dev/null
+++ b/hscontrol/dns/extrarecords_shutdown_test.go
@@ -0,0 +1,45 @@
+package dns
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestUpdateRecordsDoesNotBlockShutdown ensures updateRecords does not park
+// forever on the update channel during shutdown. The send must release the
+// write lock first and abort on closeCh, otherwise the Run goroutine leaks and
+// holds the lock indefinitely when no consumer is draining the channel.
+func TestUpdateRecordsDoesNotBlockShutdown(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)
+
+	defer er.watcher.Close()
+
+	// Change the file so updateRecords passes the unchanged-hash guard and
+	// reaches the send with no consumer draining UpdateCh.
+	require.NoError(t, os.WriteFile(path,
+		[]byte(`[{"name":"b.example.com","type":"A","value":"100.64.0.2"}]`), 0o600))
+
+	done := make(chan struct{})
+
+	go func() {
+		er.updateRecords()
+		close(done)
+	}()
+
+	er.Close()
+
+	select {
+	case <-done:
+	case <-time.After(2 * time.Second):
+		t.Fatal("updateRecords parked on a blocking send and did not return after Close")
+	}
+}