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) + } +}