diff --git a/TODO.md b/TODO.md index 1b4f13f..8905768 100644 --- a/TODO.md +++ b/TODO.md @@ -30,7 +30,12 @@ confirm make check still passes. `internal/resolver/livedns_test.go` adds a package-wide concurrency gate (so parallel tests stop bursting at the first root server), retry with exponential backoff on transport failures only, and - quorum instead of unanimity for multi-nameserver assertions. The + quorum instead of unanimity for multi-nameserver assertions. Quorum + tolerates silence only: every per-nameserver status must be in a + closed allowlist (`ok`/`timeout`/`error`, or + `nxdomain`/`timeout`/`error`), so a wrong answer from a minority — + `nodata` today, any status added later — fails the test instead of + sliding through under the majority. The `make test` cap moved to the new org-wide 60s hard cap / 20s target with a 90s `-timeout` backstop; `REPO_POLICIES.md` re-vendored byte-identical from `sneak/prompts`. No mocks, no `-short`, no build diff --git a/internal/resolver/livedns_harness_test.go b/internal/resolver/livedns_harness_test.go index 066337d..87ce704 100644 --- a/internal/resolver/livedns_harness_test.go +++ b/internal/resolver/livedns_harness_test.go @@ -16,6 +16,16 @@ import ( // perform no DNS resolution of any kind, so they neither mock DNS // nor depend on it. +// Names for the synthetic status maps below. Nothing is ever queried +// at them: they are map keys handed to the package's pure counting +// helpers, not a stand-in for a nameserver. +const ( + nsExample1 = "ns1.example." + nsExample2 = "ns2.example." + nsExample3 = "ns3.example." + nsExample4 = "ns4.example." +) + func TestLiveQuorumIsStrictMajority(t *testing.T) { t.Parallel() @@ -41,20 +51,20 @@ func TestStatusCountingIgnoresSilentNameservers(t *testing.T) { t.Parallel() results := map[string]*resolver.NameserverResponse{ - "ns1.example.": { - Nameserver: "ns1.example.", + nsExample1: { + Nameserver: nsExample1, Status: resolver.StatusOK, }, - "ns2.example.": { - Nameserver: "ns2.example.", + nsExample2: { + Nameserver: nsExample2, Status: resolver.StatusOK, }, - "ns3.example.": { - Nameserver: "ns3.example.", + nsExample3: { + Nameserver: nsExample3, Status: resolver.StatusTimeout, }, - "ns4.example.": { - Nameserver: "ns4.example.", + nsExample4: { + Nameserver: nsExample4, Status: resolver.StatusError, }, } @@ -106,14 +116,126 @@ func TestRetryLiveGivesEachAttemptADeadline(t *testing.T) { retryLive(t, "deadline", func(ctx context.Context) error { deadline, ok := ctx.Deadline() assert.True(t, ok, "attempt should carry a deadline") - assert.LessOrEqual( - t, time.Until(deadline), liveAttemptTimeout, - ) + + remaining := time.Until(deadline) + + assert.LessOrEqual(t, remaining, liveAttemptTimeout) + + // Lower bound too: without one this passes for a + // deadline far shorter than intended, which would + // silently turn every live attempt into an instant + // timeout. + assert.Greater(t, remaining, liveAttemptTimeout/2) return nil }) } +// TestUnsanctionedStatusesRejectsWrongAnswers is the regression test +// for the defect this allowlist exists to prevent: a minority of +// nameservers answering WRONGLY while quorum keeps the suite green. +// nodata is the case that motivated it — it is a wrong answer, not +// silence, and it was previously banned by neither test. +func TestUnsanctionedStatusesRejectsWrongAnswers(t *testing.T) { + t.Parallel() + + // Four nameservers, three OK and one answering nodata: a + // quorum of three is satisfied and no NXDOMAIN is present, so + // the old blocklist assertions both passed on this input. + results := map[string]*resolver.NameserverResponse{ + nsExample1: { + Nameserver: nsExample1, + Status: resolver.StatusOK, + }, + nsExample2: { + Nameserver: nsExample2, + Status: resolver.StatusOK, + }, + nsExample3: { + Nameserver: nsExample3, + Status: resolver.StatusOK, + }, + nsExample4: { + Nameserver: nsExample4, + Status: resolver.StatusNoData, + }, + } + + assert.GreaterOrEqual( + t, + countStatus(results, resolver.StatusOK), + liveQuorum(len(results)), + ) + assert.Zero(t, countStatus(results, resolver.StatusNXDomain)) + + // nodata is an ANSWER, so it never triggers a retry: nothing + // but the allowlist stands between it and a false green. + assert.Equal(t, len(results), answeredCount(results)) + + assert.Equal( + t, + []string{nsExample4 + "=nodata"}, + unsanctionedStatuses( + results, + resolver.StatusOK, + resolver.StatusTimeout, + resolver.StatusError, + ), + "nodata must be reported as an unsanctioned status", + ) +} + +func TestUnsanctionedStatusesToleratesSilenceOnly(t *testing.T) { + t.Parallel() + + results := map[string]*resolver.NameserverResponse{ + nsExample1: { + Nameserver: nsExample1, + Status: resolver.StatusNXDomain, + }, + nsExample2: { + Nameserver: nsExample2, + Status: resolver.StatusTimeout, + }, + nsExample3: { + Nameserver: nsExample3, + Status: resolver.StatusError, + }, + } + + allowed := []string{ + resolver.StatusNXDomain, + resolver.StatusTimeout, + resolver.StatusError, + } + + assert.Empty( + t, + unsanctionedStatuses(results, allowed...), + "timeout and error are non-answers and are tolerated", + ) + + // The same silent nameservers do not count towards a quorum. + assert.Equal(t, 1, answeredCount(results)) + + // An unknown status is treated as silence by answeredCount — + // so it retries and fails loudly — and is unsanctioned by the + // allowlist rather than quietly permitted. + const laterStatus = "some-status-added-later" + + results[nsExample4] = &resolver.NameserverResponse{ + Nameserver: nsExample4, + Status: laterStatus, + } + + assert.Equal(t, 1, answeredCount(results)) + assert.Equal( + t, + []string{nsExample4 + "=" + laterStatus}, + unsanctionedStatuses(results, allowed...), + ) +} + func TestRunLiveBoundsConcurrency(t *testing.T) { t.Parallel() diff --git a/internal/resolver/livedns_test.go b/internal/resolver/livedns_test.go index 55ea755..b864460 100644 --- a/internal/resolver/livedns_test.go +++ b/internal/resolver/livedns_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "sort" "strings" "testing" @@ -44,6 +45,15 @@ import ( // nameservers, a strict majority answering as expected is // enough; a server that fails to answer is tolerated, while a // server that answers *wrongly* still fails the test. +// +// The tolerance in (3) is expressed as an ALLOWLIST of sanctioned +// statuses, never as a blocklist of known-bad ones. A blocklist bans +// the one wrong answer its author thought of and silently admits +// every other status, including any added to the resolver later; an +// allowlist fails on anything nobody explicitly sanctioned. Silence +// (timeout, error) is the only thing quorum exists to tolerate. A +// *wrong answer* — nxdomain for a name that exists, ok for one that +// does not, nodata for either — is never tolerated at any count. const ( // liveAttempts is how many times a live DNS operation is @@ -172,14 +182,62 @@ func countStatus( return n } +// liveAnswerStatuses is the closed set of statuses that count as a +// nameserver having ANSWERED at all, whether or not the test agrees +// with the answer. It is deliberately an allowlist: a status added +// to the resolver later is treated as silence, so it can only ever +// cause a retry and then a loud failure, never a quiet pass. +func liveAnswerStatuses() []string { + return []string{ + resolver.StatusOK, + resolver.StatusNXDomain, + resolver.StatusNoData, + } +} + // answeredCount counts the nameservers that produced an answer of // any kind, as opposed to failing or timing out. func answeredCount( results map[string]*resolver.NameserverResponse, ) int { - return len(results) - - countStatus(results, resolver.StatusError) - - countStatus(results, resolver.StatusTimeout) + answers := liveAnswerStatuses() + + n := 0 + + for _, resp := range results { + if slices.Contains(answers, resp.Status) { + n++ + } + } + + return n +} + +// unsanctionedStatuses returns "nameserver=status" for every result +// whose status the caller did not explicitly sanction, sorted for a +// stable failure message. Callers pass the full closed set they will +// accept — the expected answer plus whichever non-answers (timeout, +// error) quorum is allowed to tolerate — so that any status outside +// it fails the test by name. +func unsanctionedStatuses( + results map[string]*resolver.NameserverResponse, + allowed ...string, +) []string { + offenders := make([]string, 0, len(results)) + + for ns, resp := range results { + if slices.Contains(allowed, resp.Status) { + continue + } + + offenders = append( + offenders, fmt.Sprintf("%s=%s", ns, resp.Status), + ) + } + + sort.Strings(offenders) + + return offenders } // describeStatuses renders per-nameserver statuses for use in diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 721847e..13bdcde 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -324,12 +324,21 @@ func TestQueryAllNameservers_AllReturnOK(t *testing.T) { describeStatuses(results), ) - // Any nameserver claiming google.com does not exist is a - // real failure and is never tolerated. - assert.Zero( + // Quorum tolerates SILENCE only. Every individual result must + // be either the expected answer or a non-answer: ok, timeout + // or error, and nothing else. Stated as a closed allowlist so + // that a wrong answer no one thought to ban — nxdomain and + // nodata today, any status added later — fails here rather + // than sliding through under the quorum. + assert.Empty( t, - countStatus(results, resolver.StatusNXDomain), - "no nameserver should report NXDOMAIN: %s", + unsanctionedStatuses( + results, + resolver.StatusOK, + resolver.StatusTimeout, + resolver.StatusError, + ), + "every nameserver must answer OK or not answer at all: %s", describeStatuses(results), ) } @@ -352,12 +361,20 @@ func TestQueryAllNameservers_NXDomainFromAllNS( describeStatuses(results), ) - // Silence is tolerated; a positive answer for a name that - // does not exist is not. - assert.Zero( + // Silence is tolerated; any actual answer other than NXDOMAIN + // is not. Closed allowlist for the same reason as above: a + // server answering `ok` or `nodata` for a name that must not + // exist is a wrong answer, not a slow one. + assert.Empty( t, - countStatus(results, resolver.StatusOK), - "no nameserver should answer OK: %s", + unsanctionedStatuses( + results, + resolver.StatusNXDomain, + resolver.StatusTimeout, + resolver.StatusError, + ), + "every nameserver must report NXDOMAIN or not answer "+ + "at all: %s", describeStatuses(results), ) }