fix(webhook): cleanup stale connectors + kill cloudflared process group#173
Merged
Merged
Conversation
Two related webhook-tunnel lifecycle bugs in `buttons webhook listen`: 1. Stale connectors at the Cloudflare edge. If a prior `listen` exited without cloudflared tearing down cleanly (crash, SIGKILL, Stop() returning before the child actually died), the connector stays registered at the CF edge forwarding to a now-deallocated local port. A subsequent `listen` spins up a fresh connector but CF load- balances across both; half of /healthz probes hit the dead one and the readiness check fails intermittently. Fix: before `tunnel run` in named mode, run `cloudflared tunnel cleanup <NAME>` to drop all stale connectors at the edge. Best-effort, 10s cap, no-op when nothing stale exists. Only runs for ModeNamed (cert.pem identity); ModeQuick's ephemeral trycloudflare.com tunnels have no persistent identity to clean up. Docs: https://developers.cloudflare.com/cloudflare-one/networks/connectors/cloudflare-tunnel/do-more-with-tunnels/local-management/tunnel-commands/#cleanup 2. Tunnel.Stop() didn't reliably kill the cloudflared child on macOS. It signalled the parent PID directly; if cloudflared forked any helper or briefly ignored SIGTERM, Stop() returned while the child (and its registered edge connector) was still alive — re-creating issue 1 on the next listen. Fix: set SysProcAttr.Setpgid=true on the child so we own a process group, SIGTERM -pgid in Stop(), wait 3s, SIGKILL -pgid. Mirrors internal/engine/execute.go:killProcessGroup. Windows keeps its os.Interrupt + Kill() fallback (no Setpgid equivalent) via the existing signal_windows.go split. Test: tunnel_cleanup_test.go swaps the cleanup runner with a counter and verifies named-mode invocation, empty-name skip, and quick-mode skip — no subprocess, no cloudflared binary needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bobakemamian
force-pushed
the
fix/webhook-cleanup-stale-connectors
branch
from
c527f82 to
c4c8fcc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related lifecycle bugs in
buttons webhook listenwithcloudflared:Stale connectors stay registered at the Cloudflare edge after a prior
listenexits without a clean shutdown. CF load-balances the tunnel hostname across the dead connector (forwarding to a deallocated local port) and the fresh one, which makes/healthzreadiness probes flaky and causes real webhook traffic to miss the new process. Fix: beforecloudflared tunnel runin named mode, callcloudflared tunnel cleanup <NAME>to drop stale connectors at the edge. Best-effort with a 10s cap; no-op when nothing is stale. Only forModeNamed(cert.pem identity) — quick tunnels have no persistent identity to clean up. Docs.Tunnel.Stop()didn't reliably kill the cloudflared child on macOS — it signalled the parent PID directly, so any forked helper (or a briefly-ignored SIGTERM) could outlive us and re-create bug chore(deps): Bump docker/setup-buildx-action from 3 to 4 #1 on the next listen. Fix: setSysProcAttr.Setpgid=trueon the child, thenSIGTERM -pgid→ 3s grace →SIGKILL -pgidinStop(). Mirrorsinternal/engine/execute.go:killProcessGroup. Windows keeps its existingos.Interrupt+Kill()fallback via thesignal_windows.gosplit.Test plan
go build ./...cleango vet ./...cleango test ./internal/webhook/...green — newtunnel_cleanup_test.goverifies:StartTunnelpath never invokes cleanupbuttons webhook listen, kill -9 the parent, restart — readiness should pass first try (previously requiredcloudflared tunnel cleanupby hand)🤖 Generated with Claude Code