[codex] add stable multi-resolver support#1
Merged
Conversation
A read or write error on any one resolver entry previously propagated to DNSPacketConn via recvChan, tearing down the entire tunnel session — the opposite of what multi-resolver is meant to deliver. In practice only DoT triggered the cascade (UDP/DoH workers retry silently), but a mixed setup with one flaky DoT would reconnect in a loop. - Reader goroutines now mark their entry dead on transport error and exit, instead of pushing error-bearing results onto recvChan. - resolverEntry gains a sticky atomic dead flag; selection helpers and probe targeting skip dead entries; recomputeState short-circuits on them so the decay logic cannot flip Down back to Unknown. - MultiResolver gains an aliveCount/allDead pair; the last dying reader closes allDead, causing ReadFrom/WriteTo to return a terminal "all resolvers are down" error. This is the only condition that should cascade to tunnel reconnect. - WriteTo retries with the next alive entry on a writePacket failure, marking the failed entry dead. Error wrapping handles the empty case without a %w nil. - ReadFrom drains any buffered packet from a deceased reader before surfacing the terminal error, avoiding packet loss at teardown. - markDead holds mu during the transition and returns true only on the alive-to-dead edge, so concurrent observers (reader + writer seeing the same Close) cannot double-decrement aliveCount. Unit tests in client/multi_resolver_test.go exercise both symptoms with a fake net.PacketConn: - TestMultiResolver_DeadEntryDoesNotBreakReadFrom — ReadFrom must return bytes from the working entry after a peer errors. - TestMultiResolver_FailedEntryExcludedFromSelection — selectPrimary must stop returning the failed entry after its reader exits.
Exercise the multi-resolver client path end-to-end with three Docker-based scenarios covering the happy path, a mid-flight resolver kill, and a forging resolver: - multi-resolver: two healthy CoreDNS forwarders, both serving the tunnel domain; verifies the client works with -udp flag repeated. - multi-resolver-runtime-failure: two healthy resolvers, then docker kill one mid-flight; asserts HTTP traffic keeps flowing through the survivor AND that no new tunnel sessions are created — proving the single-entry failure is isolated, not escalated to a full session reconnect. - multi-resolver-forge: one healthy resolver plus one CoreDNS instance using the template plugin to inject NXDOMAIN for every query; asserts the tunnel still delivers while the UDP worker's forged-response filter absorbs the injections, and sanity-checks the forging path by grepping dns-forge's own logs for NXDOMAIN to rule out a round-robin false pass. All three tests use explicit -idle-timeout 10s -keepalive 2s on both client and server (matching recovery/transport-recovery) so they stay anchored to a production-like timing profile and won't drift when the branch's defaults are later reconciled with main. The full e2e suite runner (run-test.sh) is updated to include the three new tests after transport-recovery.
Brings in all commits from net2share/vaydns main through v0.2.8: tuned session defaults (10s/2s/500ms), NULL/CAA record type support, server EDNS MTU advertisement fix, -v version flag, CGO_ENABLED=0 in CI, and documentation updates. Conflict in vaydns-client/main.go resolved by keeping both branches' additions: StringSliceFlag for repeatable -udp/-doh/-dot, plus the -v showVersion flag. dns/dns.go: accepted main's alignment and dropped the PR's gratuitous whitespace churn; added RcodeRefused = 5 to the merged Rcode block. Default timeouts follow main (10s idle, 2s keepalive, 500ms session check, 15s handshake).
The multi-resolver refactor split the single "resolver" variable into three per-type loops but only re-added the UTLSClientHelloID assignment to the DoH loop, silently dropping fingerprint camouflage for every DoT resolver. Pre-refactor main set this field unconditionally on the single configured resolver, so single-resolver DoT users on main got their fingerprint; multi-resolver DoT users on this branch do not. Restore the assignment in the DoT loop, mirroring the DoH loop's pattern.
The decay logic in recomputeState read each counter with atomic Load,
then wrote back Load-1 with atomic Store. Concurrent Adds from
writePacket, evaluateIncoming, and expirePending (all running in
different goroutines) could land between the Load and Store and be
silently overwritten — classic lost-update on the invalid/timeout
counters. timeoutCount.Store(0) in the valid-response path had the
same shape.
The Go race detector does not catch this: mixed atomic operations on
the same field are considered synchronized, and the bug is a logic
lost-update rather than an unsynchronized memory access.
Move validCount, invalidCount, and timeoutCount from atomic.Int64 to
plain int64 fields protected by the entry's existing mu, and
consolidate the several small mu acquisitions in evaluateIncoming,
expirePending, and writePacket into one per branch. recomputeState's
decay becomes a simple "if count > 0 { count-- }" under the lock.
dead and MultiResolver.aliveCount remain atomic: they are read from
selection hot paths that should not acquire e.mu on every iteration.
resolverScore now acquires e.mu instead of calling stateSnapshot()
and reading counters separately — side benefit: eliminates a minor
pre-existing inconsistency where the state and the counter values
could come from different snapshots.
…ed logs Each resolver entry now owns a labeled ForgedStats instance so operators can see exactly which resolver is targeted by DNS injection. Milestone logs include the resolver address: forged DNS responses from 8.8.8.8:53: total=10 SERVFAIL=0 NXDOMAIN=10 other=0 In multi-resolver mode, the reader goroutine filters forged responses (QR=1, RCODE != NoError) at the MultiResolver layer and records them per-entry, preventing double-counting at the DNSPacketConn safety net. In single-resolver mode, ForgedStats is created by the Tunnel and shared between UDPPacketConn and DNSPacketConn, matching pre-PR behavior with the new labeled format. - ForgedStats gains a Label field; Record() includes "from <label>" in milestone log lines (falls back to unlabeled for empty label). - NewUDPPacketConn and GetResolverConnection accept an external ForgedStats pointer so the caller controls sharing and labeling. - evaluateIncoming returns a forged bool; readPacket and startReaders use it to skip forwarding forged responses to recvChan. - NewMultiResolver creates per-entry labeled ForgedStats at construction. - e2e/multi-resolver-forge extended to assert the labeled milestone log fires on the client side with the forging resolver's address. - Unit tests: ForgedStats labeled/unlabeled milestone, per-entry recording, forged-not-forwarded, per-entry label distinctness.
The multi-resolver PR changed NewTunnel from accepting a single Resolver to accepting []Resolver, breaking every existing library consumer. The package doc example and docs/client-library.md still showed the old single-resolver call pattern. Restore NewTunnel(Resolver, TunnelServer) as a thin wrapper that delegates to the new NewTunnelMulti([]Resolver, TunnelServer). Existing code compiles unchanged; multi-resolver users call NewTunnelMulti explicitly. Update the package doc example and docs/client-library.md to show both the single-resolver and multi-resolver patterns. Fix stale default values in the doc's session-options comment block to match main's current defaults (10s idle, 2s keepalive).
…checks The health state machine decayed invalidCount and timeoutCount by 1 on every recomputeState call. Since evaluateIncoming calls recomputeState after every response, each invalid response added 1 and immediately decayed 1 — net zero. A resolver actively returning error RCODEs (REFUSED, NXDOMAIN, SERVFAIL) was never detected as problematic; its state stayed Unknown and round-robin kept sending queries to it. Move the decay into expirePending (the once-per-second health tick), and only apply it when no pending IDs expired that tick. This ensures: - Response-driven invalidCount grows at the response rate between ticks and can cross the rateLimitThreshold within seconds. - Timeout-driven accumulation from bulk pending-ID expirations is not immediately cancelled by decay in the same tick. - Once the resolver is excluded and stops receiving traffic, counters decay at 1/sec back to zero, allowing re-probing. Add e2e/multi-resolver-health-detection: uses -udp-shared-socket so forged NXDOMAINs reach evaluateIncoming directly (the affected path). After a 20s detection window and a 10-fetch traffic burst, asserts that dns-forge received ≤15 queries during the burst (probe-only), not the ~50% share it would get without health-based exclusion.
…gging Delete SelectionBest, SelectionSmart, selectBestScore, resolverScore, the SelectionMode type, and the mode field/parameter — all unreachable from the CLI (hardcoded to round-robin) and conceptually questionable for a DNS tunnel where resolver latency is dwarfed by the tunnel path. selectPrimary now delegates directly to selectRoundRobinHealthy. NewMultiResolver logs each resolver's type and address at startup, plus the total UDP worker goroutine count when per-query UDP entries are present, so operators can see the resource cost of their configuration. Remove stray blank line in Outbound.Start.
Verify that -dnstt-compat (8-byte ClientID, padding prefixes, forced TXT, 2m idle / 10s keepalive) works correctly when combined with multiple UDP resolvers. The wire format operates above the MultiResolver transport layer with no shared state, but this test guards against regressions in the interaction.
…tests - Validate empty resolver slice in NewTunnelMulti to prevent panic on Resolvers[0] access in InitiateResolverConnection. - Remove the direct isRateLimitedResponse state assignment in evaluateIncoming — recomputeState unconditionally overwrites it, so the assignment had no observable effect. Remove the now-callerless isRateLimitedResponse function. Rate-limit classification happens via the accumulation threshold in recomputeState. - Remove unused Tunnel.MultiResolverStats and Tunnel.MultiResolverValidInvalidCounts methods and their underlying MultiResolver.ResolverStats and MultiResolver.ValidInvalidCounts (no caller in the codebase). - Fix QF1012 lint: WriteString(fmt.Sprintf(...)) → fmt.Fprintf in renderResolverStatsTable. - Add defensive comment on the forgedStats nil guard in evaluateIncoming explaining it is always non-nil in multi-resolver mode. - Change health stats table log level from Trace to Debug for easier operator access. - Add TestMultiResolver_WriteToFailsOverToNextEntry: closes one entry's transport, verifies WriteTo marks it dead and succeeds via the next alive entry.
xyecoc-moJlokococ
marked this pull request as ready for review
June 10, 2026 10:08
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds multi-resolver support to vaydns-client and its Go client library so DNS queries can be spread across multiple DoH/DoT/UDP resolvers with per-resolver health tracking and failover.
Changes:
- Extend CLI flags (
-doh/-dot/-udp) to be repeatable and wire them into a new multi-resolver tunnel path. - Introduce
client.MultiResolverwith health tracking, probing, and per-resolver forged-response attribution. - Add docs + manpage updates and new E2E scenarios covering multi-resolver behavior and failure modes.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vaydns-client/main.go | Makes resolver flags repeatable and builds tunnels via NewTunnelMulti. |
| man/vaydns-client.1 | Documents repeatable transport flags and multi-resolver behavior. |
| e2e/run-test.sh | Adds new E2E directories to the test runner. |
| e2e/multi-resolver/run.sh | Smoke test for multi-resolver mode. |
| e2e/multi-resolver/docker-compose.yml | Compose stack for the multi-resolver smoke test. |
| e2e/multi-resolver-runtime-failure/run.sh | Verifies a resolver dying mid-flight is isolated without session rebuild. |
| e2e/multi-resolver-runtime-failure/docker-compose.yml | Compose stack for runtime failure test. |
| e2e/multi-resolver-health-detection/run.sh | Verifies health detection routes around a forging resolver. |
| e2e/multi-resolver-health-detection/docker-compose.yml | Compose stack for health detection test. |
| e2e/multi-resolver-forge/run.sh | Verifies tunnel works when one resolver injects forged NXDOMAINs. |
| e2e/multi-resolver-forge/docker-compose.yml | Compose stack for forging test. |
| e2e/multi-resolver-forge/Corefile.forge | CoreDNS config that always returns NXDOMAIN. |
| e2e/multi-resolver-dnstt-compat/run.sh | Verifies multi-resolver works with -dnstt-compat. |
| e2e/multi-resolver-dnstt-compat/docker-compose.yml | Compose stack for dnstt-compat multi-resolver test. |
| docs/client-library.md | Documents NewTunnelMulti and updates example session settings. |
| dns/dns.go | Adds missing DNS REFUSED RCODE constant. |
| client/udp.go | Changes UDPPacketConn construction to accept optional shared ForgedStats. |
| client/multi_resolver_test.go | Adds unit tests for multi-resolver read/write behavior and forged stats. |
| client/multi_resolver.go | Implements MultiResolver (health, probing, per-entry forged stats). |
| client/dns.go | Adds labeled forged-response milestone logs. |
| client/client.go | Introduces NewTunnelMulti and updates tunnel resolver initialization path. |
| README.md | Documents multi-resolver CLI usage and behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+425
to
+432
| case <-ticker.C: | ||
| now := time.Now() | ||
| stats := make([]ResolverStat, 0, len(mr.entries)) | ||
| for _, e := range mr.entries { | ||
| e.expirePending(now) | ||
| stats = append(stats, e.snapshot()) | ||
| } | ||
| log.Debug("\n" + renderResolverStatsTable(stats, now)) |
Comment on lines
+626
to
+628
| func (mr *MultiResolver) LocalAddr() net.Addr { | ||
| return mr.entries[0].conn.LocalAddr() | ||
| } |
| // like xray-core) or call ListenAndServe for a fully managed session. | ||
| type Tunnel struct { | ||
| Resolver Resolver | ||
| Resolvers []Resolver |
Comment on lines
848
to
+853
| // Start begins accepting connections on bind and forwarding them through the | ||
| // first resolver/server pair. | ||
| func (o *Outbound) Start(bind string) error { | ||
| resolver := o.Resolvers[0] | ||
| tunnelServer := o.TunnelServers[0] | ||
|
|
||
| tunnel, err := NewTunnel(resolver, tunnelServer) | ||
| tunnel, err := NewTunnelMulti(o.Resolvers, tunnelServer) |
Comment on lines
+29
to
+32
| func (s *StringSliceFlag) Set(value string) error { | ||
| *s = append(*s, value) | ||
| return nil | ||
| } |
Comment on lines
+131
to
+135
| // Give the reader goroutine time to handle the error. Under the buggy | ||
| // implementation it pushes an error-bearing multiReadResult onto | ||
| // recvChan and exits; under a correct implementation it would quietly | ||
| // mark the entry down and exit without poisoning recvChan. | ||
| time.Sleep(100 * time.Millisecond) |
Comment on lines
+234
to
+238
| // TestForgedStats_LabeledMilestoneLog verifies that ForgedStats.Record() | ||
| // fires a milestone log at the expected thresholds and that the log line | ||
| // includes the resolver label in the "from <addr>" format. | ||
| func TestForgedStats_LabeledMilestoneLog(t *testing.T) { | ||
| stats := &ForgedStats{Label: "1.2.3.4:53"} |
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
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
This branch ports the hiddify multi-resolver work onto VayDNS v0.2.8 and keeps the bug-fix follow-up from hiddify#1 rather than only the original net2share#64 patch.
What changed
-doh,-dot, and-udpflags.NewTunnel, addingNewTunnelMultifor multi-resolver callers.Notes
The original PR net2share#64 was useful but had edge-case risks around failed resolver entries, API compatibility, and health counter races. This branch is based on the fixed hiddify follow-up that addresses those issues.
Validation
go test ./...go build -buildvcs=false ./...