diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 6b30941..e828103 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -1403,25 +1403,123 @@ func (p *prog) pfStabilizationLoopWithMaxWait(ctx context.Context, stableRequire } if time.Since(stableSince) >= stableRequired { - // The active loop retains ownership until the dedicated post-stable - // repair finishes. It never re-enters stabilization recursively. - mainLog.Load().Info().Msgf("DNS intercept: pf stable for %s — reconciling anchor rules", stableRequired) - result := p.reconcilePFAnchorAfterStabilization() - if result != pfAnchorCheckRestored && result != pfAnchorCheckIntact { - p.scheduleDelayedRechecks() - } - routes, domainlessServers, exemptions := p.refreshDNSAfterVPNSettle("pf_stabilized") - if routes == 0 && domainlessServers == 0 && exemptions == 0 { - p.scheduleDNSAfterVPNSettleRefresh("pf_stabilized_followup", pfAnchorRecheckDelayLong) - } - if p.hasPendingTunnelReconcile() { - p.scheduleDelayedRechecks() - } + p.finishPFStabilization(stableRequired) return } } } +// finishPFStabilization runs the work stabilization exists to do, once the ruleset has +// held still for the required window. The active loop retains ownership throughout: it +// never re-enters stabilization recursively. +func (p *prog) finishPFStabilization(stableRequired time.Duration) { + mainLog.Load().Info().Msgf("DNS intercept: pf stable for %s — reconciling anchor rules", stableRequired) + result := p.reconcilePFAnchorAfterStabilization() + if result != pfAnchorCheckRestored && result != pfAnchorCheckIntact { + p.scheduleDelayedRechecks() + } + routes, domainlessServers, exemptions := p.refreshDNSAfterVPNSettle("pf_stabilized") + if routes == 0 && domainlessServers == 0 && exemptions == 0 { + p.scheduleDNSAfterVPNSettleRefresh("pf_stabilized_followup", pfAnchorRecheckDelayLong) + } + if p.hasPendingTunnelReconcile() { + p.scheduleDelayedRechecks() + } + p.verifyInterceptAfterStabilization() +} + +// probePFInterceptFn and forceReloadPFInterceptFn are the functional verification seams +// shared by both probers. +var ( + probePFInterceptFn = (*prog).probePFIntercept + forceReloadPFInterceptFn = (*prog).forceReloadPFMainRuleset +) + +// pfFunctionalProbeOwnerWait bounds how long the post-stabilization verifier waits for +// another prober to release functional-probe ownership. A probe monitor that is only +// standing down releases it at once; one that is genuinely probing holds it for its whole +// window, and then the verifier steps aside. A var so tests can shorten the wait. +var pfFunctionalProbeOwnerWait = 2 * time.Second + +// pfFunctionalProbeOwnerPoll is how often that wait re-tries the claim. +const pfFunctionalProbeOwnerPoll = 25 * time.Millisecond + +// interceptProbeMonitorAllowed reports whether the probe monitor may run at all. +// +// The monitor must consult this before claiming ownership. Claiming first and checking +// second means a monitor that is about to stand down still takes the flag, and the +// post-stabilization verifier - which sees a set flag as "somebody else is probing" - +// skips. Neither probes, and the outage lasts until the next watchdog tick. +func (p *prog) interceptProbeMonitorAllowed() bool { + return p.dnsInterceptState != nil && !p.pfStabilizing.Load() +} + +// claimFunctionalProbeOwner takes ownership of functional probing, waiting up to wait for +// a current owner to release it. It reports whether ownership was acquired; the caller +// releases with pfMonitorRunning.Store(false). +func (p *prog) claimFunctionalProbeOwner(wait time.Duration) bool { + deadline := time.Now().Add(wait) + for { + if p.pfMonitorRunning.CompareAndSwap(false, true) { + return true + } + if !time.Now().Before(deadline) { + return false + } + time.Sleep(pfFunctionalProbeOwnerPoll) + } +} + +// verifyInterceptAfterStabilization proves pf is actually translating once stabilization +// has finished, and repairs it once if it is not. +// +// The reconcile above verifies rule text. That cannot distinguish a live redirect from an +// anchor pf has stopped evaluating, and after sleep/wake with a VPN reconnect those come +// apart: rules present, references present, post-load verification passed, and every query +// through the system resolver timing out. Nothing else notices until the periodic watchdog +// runs its own probe - the interception probe monitor stands down while stabilization owns +// pf and is not re-armed afterwards - so recovery waits up to a full watchdog interval on a +// host whose link and default route are already back. +// +// One probe, then at most one forced reload and one confirming probe. Deliberately not the +// probe monitor: that keeps probing for ~7.5s and can force a reload per failed probe, +// where this path needs a single bounded repair and then hands back to the watchdog. +func (p *prog) verifyInterceptAfterStabilization() { + if p.dnsInterceptState == nil || p.pfExecBackoffActive() { + return + } + // The probe monitor is the other functional prober, so only one of us may run - but + // "somebody holds the flag" is not the same as "somebody is probing". A monitor that + // started while stabilization owns pf stands down immediately, and skipping on sight + // left nobody probing at all. Wait briefly for the holder to release instead. + if !p.claimFunctionalProbeOwner(pfFunctionalProbeOwnerWait) { + mainLog.Load().Warn().Msgf("DNS intercept: post-stabilization probe skipped — another prober held ownership for %s; leaving recovery to the watchdog", pfFunctionalProbeOwnerWait) + return + } + defer p.pfMonitorRunning.Store(false) + + // Ownership can take a moment to arrive; make sure there is still an intercept to check. + if p.dnsInterceptState == nil { + return + } + + if probePFInterceptFn(p) { + mainLog.Load().Debug().Msg("DNS intercept: post-stabilization probe passed — interception is translating") + return + } + + mainLog.Load().Warn().Msg("DNS intercept: post-stabilization rules are intact but the probe FAILED — forcing one reload") + if !forceReloadPFInterceptFn(p) { + mainLog.Load().Error().Msg("DNS intercept: post-stabilization forced reload did not run — leaving recovery to the watchdog") + return + } + if probePFInterceptFn(p) { + mainLog.Load().Info().Msg("DNS intercept: interception restored by the post-stabilization reload") + return + } + mainLog.Load().Error().Msg("DNS intercept: interception still not translating after the post-stabilization reload — the watchdog will retry") +} + var runPFAnchorCheckCommand = func(args ...string) ([]byte, error) { return exec.Command("pfctl", args...).CombinedOutput() } @@ -1930,6 +2028,13 @@ func buildDNSQueryPacket(domain string) []byte { // The backoff schedule provides both fast detection (immediate + 500ms) and extended // coverage (up to ~8s) to win the race against async pf reloads by hypervisors. func (p *prog) pfInterceptMonitor() { + // Eligibility first, ownership second. A monitor that is about to stand down must not + // take the flag on its way out: the post-stabilization verifier reads that flag as + // "another prober is working" and would step aside for a prober that never probes. + if !p.interceptProbeMonitorAllowed() { + mainLog.Load().Debug().Msg("DNS intercept monitor: not starting — intercept disabled or stabilizing") + return + } if !p.pfMonitorRunning.CompareAndSwap(false, true) { mainLog.Load().Debug().Msg("DNS intercept monitor: already running, skipping") return @@ -1946,23 +2051,23 @@ func (p *prog) pfInterceptMonitor() { if delay > 0 { time.Sleep(delay) } - if p.dnsInterceptState == nil || p.pfStabilizing.Load() { + if !p.interceptProbeMonitorAllowed() { mainLog.Load().Debug().Msg("DNS intercept monitor: aborting — intercept disabled or stabilizing") return } - if p.probePFIntercept() { + if probePFInterceptFn(p) { mainLog.Load().Debug().Msgf("DNS intercept monitor: probe %d/%d passed", i+1, len(delays)) continue // working now — keep monitoring in case it breaks later in the window } // Probe failed — pf translation is broken. Force full reload. mainLog.Load().Warn().Msgf("DNS intercept monitor: probe %d/%d FAILED — pf translation broken, forcing full ruleset reload", i+1, len(delays)) - p.forceReloadPFMainRuleset() + forceReloadPFInterceptFn(p) // Verify the reload fixed it time.Sleep(200 * time.Millisecond) - if p.probePFIntercept() { + if probePFInterceptFn(p) { mainLog.Load().Info().Msg("DNS intercept monitor: probe passed after reload — interception restored") // Continue monitoring in case the hypervisor reloads pf again } else { diff --git a/cmd/cli/dns_intercept_darwin_test.go b/cmd/cli/dns_intercept_darwin_test.go index e375d08..f8515c5 100644 --- a/cmd/cli/dns_intercept_darwin_test.go +++ b/cmd/cli/dns_intercept_darwin_test.go @@ -828,3 +828,183 @@ func TestExemptVPNDNSServersDeferredWhileStabilizing(t *testing.T) { t.Error("pfEnsureRunning was left held by a deferred exemption") } } + +// stubStabilizationProbe replaces the post-stabilization verification seams and returns +// counters for probe and forced-reload calls. +func stubStabilizationProbe(t *testing.T, probeResults []bool, reloadOK bool) (probes, reloads *int) { + t.Helper() + originalProbe, originalReload := probePFInterceptFn, forceReloadPFInterceptFn + t.Cleanup(func() { + probePFInterceptFn, forceReloadPFInterceptFn = originalProbe, originalReload + }) + probeCalls, reloadCalls := 0, 0 + probePFInterceptFn = func(*prog) bool { + result := false + if probeCalls < len(probeResults) { + result = probeResults[probeCalls] + } + probeCalls++ + return result + } + forceReloadPFInterceptFn = func(*prog) bool { + reloadCalls++ + return reloadOK + } + return &probeCalls, &reloadCalls +} + +// TestPostStabilizationVerifiesInterceptionFunctionally is the post-wake continuity +// boundary: the reconcile above it only proves rule text, and QA saw rules intact, +// references intact and post-load verification passed while every query through the system +// resolver timed out. Nothing else probes until the periodic watchdog, because the probe +// monitor stands down while stabilization owns pf, so recovery waited for that tick. +func TestPostStabilizationVerifiesInterceptionFunctionally(t *testing.T) { + // Probe fails once, then passes after the reload. + probes, reloads := stubStabilizationProbe(t, []bool{false, true}, true) + + p := &prog{dnsInterceptState: &pfState{}} + p.verifyInterceptAfterStabilization() + + if *probes != 2 { + t.Errorf("probe calls = %d, want 2: one to detect and one to confirm the repair", *probes) + } + if *reloads != 1 { + t.Errorf("forced reloads = %d, want exactly 1 bounded repair", *reloads) + } +} + +// TestPostStabilizationProbePassSkipsReload keeps the healthy path free of a pf reload, +// which would flush states and kill in-flight DoH connections for nothing. +func TestPostStabilizationProbePassSkipsReload(t *testing.T) { + probes, reloads := stubStabilizationProbe(t, []bool{true}, true) + + p := &prog{dnsInterceptState: &pfState{}} + p.verifyInterceptAfterStabilization() + + if *probes != 1 || *reloads != 0 { + t.Errorf("probe calls = %d, forced reloads = %d, want 1/0", *probes, *reloads) + } +} + +// TestPostStabilizationRepairIsBounded pins the "one bounded recovery" contract: a probe +// that never passes must not turn into a reload loop here - the watchdog owns retries. +func TestPostStabilizationRepairIsBounded(t *testing.T) { + probes, reloads := stubStabilizationProbe(t, []bool{false, false, false}, true) + + p := &prog{dnsInterceptState: &pfState{}} + p.verifyInterceptAfterStabilization() + + if *reloads != 1 { + t.Errorf("forced reloads = %d, want 1: the repair must not loop", *reloads) + } + if *probes != 2 { + t.Errorf("probe calls = %d, want 2", *probes) + } +} + +// TestPostStabilizationWaitsForAProberThatStandsDown is the interleaving that made +// "skip when the flag is set" wrong. A probe monitor started by an ignored network change +// claims functional-probe ownership and then aborts, because stabilization still owns pf. +// If the verifier treats the claimed flag as "somebody is probing", neither path probes and +// the outage lasts until the next watchdog tick - the exact window this is meant to close. +func TestPostStabilizationWaitsForAProberThatStandsDown(t *testing.T) { + probes, reloads := stubStabilizationProbe(t, []bool{false, true}, true) + + p := &prog{dnsInterceptState: &pfState{}} + // Model the monitor's claim-then-abort: ownership is held, then released. + p.pfMonitorRunning.Store(true) + released := make(chan struct{}) + go func() { + time.Sleep(50 * time.Millisecond) + p.pfMonitorRunning.Store(false) + close(released) + }() + + p.verifyInterceptAfterStabilization() + <-released + + if *probes != 2 { + t.Errorf("probe calls = %d, want 2: the verifier must wait out a prober that stands down", *probes) + } + if *reloads != 1 { + t.Errorf("forced reloads = %d, want 1", *reloads) + } +} + +// TestPostStabilizationYieldsToAProberThatKeepsProbing is the other half of the handoff: +// when the holder is genuinely working through its probe sequence, the verifier must step +// aside rather than run a second prober against the same pf state. +func TestPostStabilizationYieldsToAProberThatKeepsProbing(t *testing.T) { + originalWait := pfFunctionalProbeOwnerWait + pfFunctionalProbeOwnerWait = 30 * time.Millisecond + t.Cleanup(func() { pfFunctionalProbeOwnerWait = originalWait }) + + probes, reloads := stubStabilizationProbe(t, []bool{false}, true) + + p := &prog{dnsInterceptState: &pfState{}} + p.pfMonitorRunning.Store(true) // held for the whole wait + p.verifyInterceptAfterStabilization() + + if *probes != 0 || *reloads != 0 { + t.Errorf("probe calls = %d, forced reloads = %d, want 0/0 while another prober is working", *probes, *reloads) + } +} + +// TestInterceptMonitorDoesNotClaimOwnershipWhileStabilizing pins the source of that race: +// a monitor which cannot do useful work must not take functional-probe ownership on its way +// out, or it starves the post-stabilization verifier. +func TestInterceptMonitorDoesNotClaimOwnershipWhileStabilizing(t *testing.T) { + probes, reloads := stubStabilizationProbe(t, []bool{false}, true) + + p := &prog{dnsInterceptState: &pfState{}} + p.pfStabilizing.Store(true) + if p.interceptProbeMonitorAllowed() { + t.Fatal("the probe monitor considers itself eligible while stabilization owns pf") + } + + p.pfInterceptMonitor() + + if *probes != 0 || *reloads != 0 { + t.Errorf("probe calls = %d, forced reloads = %d, want 0/0 from a monitor that cannot run", *probes, *reloads) + } + if !p.claimFunctionalProbeOwner(0) { + t.Error("the aborted monitor left functional-probe ownership taken; the verifier would skip") + } + p.pfMonitorRunning.Store(false) +} + +// TestFinishPFStabilizationRunsFunctionalVerification wires the fix to the production +// completion path: deleting the verification call, or reordering it before the reconcile, +// makes this fail. +func TestFinishPFStabilizationRunsFunctionalVerification(t *testing.T) { + stubPFAnchorCheckCommand(t, map[string]string{ + "-sn": `rdr-anchor "com.controld.ctrld"`, + "-sr": `anchor "com.controld.ctrld"`, + "-a com.controld.ctrld -sr": "pass in quick on lo0", + "-a com.controld.ctrld -sn": "rdr on lo0", + }) + originalResolver := initializeOsResolver + initializeOsResolver = func(bool) []string { return nil } + t.Cleanup(func() { initializeOsResolver = originalResolver }) + + probes, reloads := stubStabilizationProbe(t, []bool{false, true}, true) + + p := &prog{dnsInterceptState: &pfState{}} + p.pfStabilizing.Store(true) + p.finishPFStabilization(time.Millisecond) + + if *probes == 0 { + t.Fatal("stabilization completed without probing functional interception; recovery would wait for the watchdog") + } + if *reloads != 1 { + t.Errorf("forced reloads = %d, want 1", *reloads) + } + + p.pfDelayedRecheckMu.Lock() + timers := append([]*time.Timer(nil), p.pfDelayedRecheckTimers...) + p.pfDelayedRecheckTimers = nil + p.pfDelayedRecheckMu.Unlock() + for _, timer := range timers { + timer.Stop() + } +}