From 8f70c5fda1a69ee83bbec5604f8c6a33c4e14ed5 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 22 Sep 2026 12:55:27 -0700 Subject: [PATCH] fix(tunnel): stop leaking a cloudflared process per restart on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linux binds cloudflared to ephemerd at the kernel level with Pdeathsig, which — as its own comment says — "survives SIGKILL of ephemerd, panics, or any exit path that skips deferred cleanup". Windows had nothing. cloudflared_other.go claimed "the graceful Close() path is the sole shutdown mechanism there", but that path calls cmd.Process.Signal(syscall.SIGTERM), and Go's os.Process.Signal rejects every signal except os.Kill on Windows. The error was logged at Debug and dropped. Stopping the service — which is what every `mayfly apply` and `node upgrade` does — killed ephemerd without running close(), and the tunnel client simply kept running. Measured on mfl-win-amd64-102 on 2026-09-22: live ephemerd pid : 6908 children of live : 1 orphaned : 25 orphan parents still alive: 0 orphan RAM MB : 868 oldest orphan dated 2026-09-09. All 25 share one tunnel config and held 96 established connections to Cloudflare's edge against 4 for the live daemon. Cloudflare load-balances across every connection registered for a tunnel, so the large majority of inbound webhook deliveries were being routed to clients whose ephemerd was gone. That is a strong candidate for the "webhook listener wedged for hours" behaviour seen after upgrades. The fix is the Windows counterpart to Pdeathsig: a Job Object marked KILL_ON_JOB_CLOSE. The kernel terminates the child when the last handle to the job closes, which happens when ephemerd exits by any means — service stop, crash, or kill. Closing the job is also a guaranteed kill, so close() releases it AFTER the graceful attempts rather than instead of them: a clean cloudflared shutdown closes its edge connections properly, an abrupt one leaves Cloudflare to time them out. A bind failure warns instead of failing Listen — a tunnel that works but might leak beats no tunnel — but it is a warning, not a debug line, because it is the difference between one stray process and one per restart forever. Tests run for real on Windows: the child dies when the job is released, and an unstarted process is rejected rather than panicking on a nil cmd.Process. Revert-verified — reducing bindChildLifetime to the old no-op fails with "the child is unbound". Builds and vets clean for linux, windows and darwin. --- pkg/tunnel/cloudflared.go | 29 ++++++++ pkg/tunnel/cloudflared_linux.go | 6 ++ pkg/tunnel/cloudflared_other.go | 19 +++-- pkg/tunnel/cloudflared_windows.go | 93 ++++++++++++++++++++++++ pkg/tunnel/cloudflared_windows_test.go | 98 ++++++++++++++++++++++++++ 5 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 pkg/tunnel/cloudflared_windows.go create mode 100644 pkg/tunnel/cloudflared_windows_test.go diff --git a/pkg/tunnel/cloudflared.go b/pkg/tunnel/cloudflared.go index 203fb31..4d09eba 100644 --- a/pkg/tunnel/cloudflared.go +++ b/pkg/tunnel/cloudflared.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "log/slog" "net" "net/http" @@ -49,6 +50,7 @@ type Cloudflared struct { ln net.Listener waitDone chan struct{} // closed by the single waiter goroutine after cmd.Wait returns closing bool // set by close() so the waiter can tell shutdown from crash + lifetime io.Closer // OS-level parent binding (Windows Job Object); nil elsewhere } // NewCloudflared validates options and returns a provider. It does not @@ -168,6 +170,18 @@ func (c *Cloudflared) start(ctx context.Context) (net.Listener, string, error) { _ = ln.Close() return nil, "", fmt.Errorf("cloudflared: start subprocess: %w", err) } + // Bind the child to this process at the OS level. On Windows this is the + // ONLY thing that reaps cloudflared when ephemerd is stopped as a service, + // because that path never runs close(). Failing to bind is not fatal — a + // tunnel that works but may leak beats no tunnel at all — but it is the + // difference between one stray process and one per restart forever, so it + // is a warning rather than a debug line. + lifetime, err := bindChildLifetime(cmd) + if err != nil { + slog.Warn("could not bind cloudflared's lifetime to ephemerd; it may outlive this process", + "error", err) + } + c.lifetime = lifetime c.cmd = cmd c.ln = ln c.closing = false @@ -213,11 +227,26 @@ func (c *Cloudflared) close() error { cmd := c.cmd ln := c.ln waitDone := c.waitDone + lifetime := c.lifetime c.closing = true c.cmd = nil c.ln = nil + c.lifetime = nil c.mu.Unlock() + // Releasing the OS binding is itself a guaranteed kill on Windows (the Job + // Object is KILL_ON_JOB_CLOSE), so it runs after the graceful attempts + // below rather than instead of them — a clean cloudflared shutdown closes + // its edge connections properly, an abrupt one leaves Cloudflare to time + // them out. + defer func() { + if lifetime != nil { + if err := lifetime.Close(); err != nil { + slog.Debug("releasing cloudflared lifetime binding", "error", err) + } + } + }() + if ln != nil { _ = ln.Close() } diff --git a/pkg/tunnel/cloudflared_linux.go b/pkg/tunnel/cloudflared_linux.go index 439f181..ceab527 100644 --- a/pkg/tunnel/cloudflared_linux.go +++ b/pkg/tunnel/cloudflared_linux.go @@ -3,6 +3,7 @@ package tunnel import ( + "io" "os/exec" "syscall" ) @@ -20,3 +21,8 @@ func applyPdeathsig(cmd *exec.Cmd) { // stay attached to it (and thus get reaped when it does). cmd.SysProcAttr.Setpgid = true } + +// bindChildLifetime is a no-op on Linux: Pdeathsig above already binds the +// child to this process at the kernel level, and it applies from the moment +// the child is created rather than after Start. +func bindChildLifetime(_ *exec.Cmd) (io.Closer, error) { return nil, nil } diff --git a/pkg/tunnel/cloudflared_other.go b/pkg/tunnel/cloudflared_other.go index 18bf344..11f0e4c 100644 --- a/pkg/tunnel/cloudflared_other.go +++ b/pkg/tunnel/cloudflared_other.go @@ -1,10 +1,19 @@ -//go:build !linux +//go:build !linux && !windows package tunnel -import "os/exec" +import ( + "io" + "os/exec" +) -// applyPdeathsig is a no-op off Linux — the Pdeathsig field isn't part of -// SysProcAttr on other platforms. The graceful Close() path is the sole -// shutdown mechanism there. +// applyPdeathsig is a no-op here — the Pdeathsig field isn't part of +// SysProcAttr on these platforms. The graceful Close() path is the sole +// shutdown mechanism. func applyPdeathsig(_ *exec.Cmd) {} + +// bindChildLifetime is a no-op here. Linux uses Pdeathsig and Windows uses a +// Job Object; neither has an equivalent on the remaining platforms (darwin is +// the one that matters, and it runs ephemerd under launchd rather than as a +// tunnel host), so the graceful path stands alone. +func bindChildLifetime(_ *exec.Cmd) (io.Closer, error) { return nil, nil } diff --git a/pkg/tunnel/cloudflared_windows.go b/pkg/tunnel/cloudflared_windows.go new file mode 100644 index 0000000..dd1afc7 --- /dev/null +++ b/pkg/tunnel/cloudflared_windows.go @@ -0,0 +1,93 @@ +//go:build windows + +package tunnel + +import ( + "fmt" + "io" + "os/exec" + "unsafe" + + "golang.org/x/sys/windows" +) + +// applyPdeathsig is a no-op on Windows: SysProcAttr has no Pdeathsig field. +// bindChildLifetime below provides the equivalent guarantee. +func applyPdeathsig(_ *exec.Cmd) {} + +// bindChildLifetime puts cloudflared in a Job Object marked +// KILL_ON_JOB_CLOSE, so the kernel terminates it when the last handle to the +// job goes away — which happens automatically when ephemerd's process exits, +// by any means. +// +// This is the Windows counterpart to Pdeathsig on Linux, and it exists because +// Windows had NOTHING. The old comment claimed "the graceful Close() path is +// the sole shutdown mechanism there", but that path signals SIGTERM, and Go's +// os.Process.Signal rejects every signal except os.Kill on Windows. The error +// was logged at Debug and dropped. So a service stop — every `mayfly apply`, +// every `node upgrade` — killed ephemerd without running cleanup and left the +// tunnel client running. +// +// Measured on mfl-win-amd64-102 2026-09-22: 25 orphaned cloudflared processes, +// oldest 13 days old, 868 MB resident, holding 96 established connections to +// Cloudflare's edge against 4 for the live daemon. They all share one tunnel +// config, and Cloudflare load-balances across every connection registered for +// a tunnel, so the overwhelming majority of inbound webhook deliveries were +// being handed to clients whose ephemerd was gone. +// +// The returned Closer must be held for as long as the child should live: +// closing it is itself a guaranteed kill, which is why close() uses it as the +// backstop after the graceful path. +func bindChildLifetime(cmd *exec.Cmd) (io.Closer, error) { + if cmd.Process == nil { + return nil, fmt.Errorf("cloudflared: bind lifetime: process not started") + } + + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return nil, fmt.Errorf("cloudflared: create job object: %w", err) + } + + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + _ = windows.CloseHandle(job) + return nil, fmt.Errorf("cloudflared: set job limits: %w", err) + } + + // OpenProcess rather than reusing the handle inside os.Process: that one is + // owned by the runtime and closed when the Process is released, which would + // make assignment racy against Wait. + h, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(cmd.Process.Pid)) + if err != nil { + _ = windows.CloseHandle(job) + return nil, fmt.Errorf("cloudflared: open child process: %w", err) + } + defer func() { _ = windows.CloseHandle(h) }() + + if err := windows.AssignProcessToJobObject(job, h); err != nil { + _ = windows.CloseHandle(job) + return nil, fmt.Errorf("cloudflared: assign to job object: %w", err) + } + return &jobHandle{h: job}, nil +} + +// jobHandle closes the Job Object, which kills everything still in it. +type jobHandle struct{ h windows.Handle } + +func (j *jobHandle) Close() error { + if j == nil || j.h == 0 { + return nil + } + h := j.h + j.h = 0 + return windows.CloseHandle(h) +} diff --git a/pkg/tunnel/cloudflared_windows_test.go b/pkg/tunnel/cloudflared_windows_test.go new file mode 100644 index 0000000..cf2853a --- /dev/null +++ b/pkg/tunnel/cloudflared_windows_test.go @@ -0,0 +1,98 @@ +//go:build windows + +package tunnel + +import ( + "errors" + "os" + "os/exec" + "testing" + "time" +) + +// alive reports whether pid still exists. OpenProcess-based checks are the +// reliable way on Windows; os.FindProcess always succeeds there. +func alive(t *testing.T, pid int) bool { + t.Helper() + out, err := exec.Command("powershell", "-NoProfile", "-Command", + "if (Get-Process -Id "+itoa(pid)+" -ErrorAction SilentlyContinue) { 'yes' } else { 'no' }").Output() + if err != nil { + t.Fatalf("probing pid %d: %v", pid, err) + } + return string(out[:3]) == "yes" +} + +func itoa(i int) string { + if i == 0 { + return "0" + } + var b []byte + for i > 0 { + b = append([]byte{byte('0' + i%10)}, b...) + i /= 10 + } + return string(b) +} + +// The whole point of the Job Object: releasing it kills the child, with no +// cooperation from the child and no signal — because Windows has no SIGTERM +// for os.Process.Signal, which is exactly why cloudflared leaked one process +// per ephemerd restart until 2026-09-22 (25 orphans, oldest 13 days). +func TestBindChildLifetimeKillsChildOnRelease(t *testing.T) { + cmd := exec.Command("powershell", "-NoProfile", "-Command", "Start-Sleep -Seconds 120") + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + pid := cmd.Process.Pid + t.Cleanup(func() { _ = cmd.Process.Kill() }) + + closer, err := bindChildLifetime(cmd) + if err != nil { + t.Fatalf("bindChildLifetime: %v", err) + } + if closer == nil { + t.Fatal("bindChildLifetime returned a nil closer on windows; the child is unbound") + } + if !alive(t, pid) { + t.Fatal("child died before the job was released; test proves nothing") + } + + if err := closer.Close(); err != nil { + t.Fatalf("closing job: %v", err) + } + + // KILL_ON_JOB_CLOSE is synchronous-ish but not instantaneous. + deadline := time.Now().Add(10 * time.Second) + for alive(t, pid) { + if time.Now().After(deadline) { + t.Fatal("child survived the job handle closing: the lifetime binding does not bind") + } + time.Sleep(200 * time.Millisecond) + } + + // Double close must not blow up — close() releases it and the deferred + // path can run again on a reused Cloudflared. + if err := closer.Close(); err != nil { + t.Errorf("second Close() = %v, want nil", err) + } +} + +// A process that has not started cannot be assigned to a job. Returning an +// error (rather than panicking on a nil cmd.Process) keeps Listen's failure +// path a warning instead of a crash. +func TestBindChildLifetimeRejectsUnstartedProcess(t *testing.T) { + cmd := exec.Command("powershell", "-NoProfile", "-Command", "exit") + closer, err := bindChildLifetime(cmd) + if err == nil { + if closer != nil { + _ = closer.Close() + } + t.Fatal("bindChildLifetime(unstarted) = nil error, want an error") + } + if closer != nil { + t.Error("bindChildLifetime returned both an error and a closer") + } + if errors.Is(err, os.ErrClosed) { + t.Errorf("unexpected error kind: %v", err) + } +}