From 9651ffdb0df4254c1c16d11948482a417f33d802 Mon Sep 17 00:00:00 2001 From: Caleb Jasik Date: Tue, 22 Sep 2026 11:52:28 -0500 Subject: [PATCH] Cancel the context when Main hands back no Control (#1889) The goroutines Main starts stop only when its context does, and only a caller holding the Control can arrange that. Cancelling solely on error left a config test, which returns no Control and no error, with the lighthouse query worker and a hostname resolver per dns named static host running for the life of the process. Every embedder that validates a config in process rather than by exec'ing pays for that on each config load, dnclient and the apple clients included. Co-authored-by: Nate Brown --- main.go | 7 +++-- main_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index ee52e7b7..52f7bea8 100644 --- a/main.go +++ b/main.go @@ -32,9 +32,12 @@ const _ = noiseutil.RejectHeadroom - 4*maxRoutines func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, deviceFactory overlay.DeviceFactory) (retcon *Control, reterr error) { ctx, cancel := context.WithCancel(context.Background()) - // Automatically cancel the context if Main returns an error, to signal all created goroutines to quit. + // The goroutines started below stop only when this context does, and only a caller holding the + // Control can arrange that. Cancel whenever we are not handing one back, which covers an error + // and a config test alike: a config test used to leave the lighthouse query worker, and a + // hostname resolver per dns named static host, running for the life of the process. defer func() { - if reterr != nil { + if retcon == nil { cancel() } }() diff --git a/main_test.go b/main_test.go index bbaef347..e217ffbf 100644 --- a/main_test.go +++ b/main_test.go @@ -1,12 +1,21 @@ package nebula import ( + "fmt" + "net/netip" + "os" + "path/filepath" "testing" + "time" + "github.com/slackhq/nebula/cert" + cert_test "github.com/slackhq/nebula/cert_test" "github.com/slackhq/nebula/config" "github.com/slackhq/nebula/test" "github.com/slackhq/nebula/util" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestParseCpuAffinity(t *testing.T) { @@ -49,3 +58,68 @@ func TestParseCpuAffinity(t *testing.T) { assert.Nil(t, parseCpuAffinity(newConfig([]any{1 << 20}), l, 1)) } } + +// TestMain_ConfigTestReleasesItsGoroutines pins the rule that Main only leaves goroutines running +// when it hands back a Control to stop them with. +// +// A config test gets no Control, so anything it started had nothing to stop it: the lighthouse +// query worker, and a hostname resolver per dns named static host, ran for the life of the +// process. That matters to every embedder that validates a config in process rather than by +// exec'ing, dnclient and the apple clients included, because they do it on each config load and +// the leak accumulates. +func TestMain_ConfigTestReleasesItsGoroutines(t *testing.T) { + defer goleak.VerifyNone(t, goleak.IgnoreCurrent()) + + l := test.NewLogger() + dir := t.TempDir() + + before := time.Now().Add(-time.Hour) + after := time.Now().Add(time.Hour) + ca, _, caKey, caPEM := cert_test.NewTestCaCert(cert.Version2, cert.Curve_CURVE25519, before, after, nil, nil, nil) + networks := []netip.Prefix{netip.MustParsePrefix("10.0.0.1/24")} + _, _, keyPEM, certPEM := cert_test.NewTestCert( + cert.Version2, cert.Curve_CURVE25519, ca, caKey, "config-test", before, after, networks, nil, nil) + + caPath := filepath.Join(dir, "ca.pem") + certPath := filepath.Join(dir, "cert.pem") + keyPath := filepath.Join(dir, "key.pem") + require.NoError(t, os.WriteFile(caPath, caPEM, 0o600)) + require.NoError(t, os.WriteFile(certPath, certPEM, 0o600)) + require.NoError(t, os.WriteFile(keyPath, keyPEM, 0o600)) + + // A static host by address, not by name: the query worker is the goroutine under test and a + // hostname would drag a real dns lookup into a unit test. + configBody := fmt.Sprintf(` +pki: + ca: %s + cert: %s + key: %s +static_host_map: + "10.0.0.2": ["192.0.2.1:4242"] +lighthouse: + hosts: + - "10.0.0.2" +listen: + host: 127.0.0.1 + port: 0 +tun: + disabled: true +firewall: + outbound: + - port: any + proto: any + host: any + inbound: + - port: any + proto: any + host: any +`, caPath, certPath, keyPath) + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte(configBody), 0o600)) + + c := config.NewC(l) + require.NoError(t, c.Load(dir)) + + ctrl, err := Main(c, true, "config-test", l, nil) + require.NoError(t, err) + require.Nil(t, ctrl, "a config test hands back nothing to stop, so it must stop itself") +}