sneak/dnswatcher
1
0
Fork 0
Code Issues 37 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

internal/resolver tests query live nameservers and fail nondeterministically #93

New Issue
Open
opened 2026-08-04 02:28:02 +02:00 by clawbot · 7 comments
clawbot commented 2026-08-04 02:28:02 +02:00
Collaborator
Copy Link

The internal/resolver tests on main query live nameservers (ns1.google.com etc.) and fail nondeterministically — a different subset each run — even in environments where direct dig @ns1.google.com works. This makes make check / CI flaky.

Definition of done rewritten 2026-08-10 to match sneak's ruling (#93 (comment)). The original DoD proposed a network build tag or a -short skip; both are now explicitly off the table. His words:

> make the cap 60s in both and never use mocking, always use live resolvers and assume the build and run environments have full unmodified unrestricted internet access. it is ok if they fail due to a bad build environment that alters dns packets.

Definition of done

  1. TESTING.md: the "within the 30-second target" wording becomes 60 seconds.
  2. REPO_POLICIES.md: the make test ceiling of 20 seconds becomes 60 seconds, and the -timeout backstop is raised consistently with it. See the caveat below before editing this file.
  3. script/test: the go test -timeout value must not be lower than the new cap, or the cap is unreachable in practice.
  4. Robustness, not gating, is how the flakiness is addressed: retries with backoff, querying several independent nameservers and accepting a quorum rather than requiring every server to answer, and sensible per-query timeouts — now with a 60s budget to work inside.
  5. No mocking, no fakes, no stubs, no -short, no network build tag, no build-tag-gated tests. Live resolvers always, in every package.
  6. No tolerance engineering for restricted egress. Assume full unmodified unrestricted internet access. A failure caused by an environment that mangles DNS packets is an acceptable outcome and must not be worked around.
  7. No change to production (non-test) resolver behaviour.
  8. make check green, and green across repeated consecutive runs.

Scoped to the above only — it does NOT resolve the feature/resolver reconciliation or the DNSSEC work in #59.

Caveat on REPO_POLICIES.md

That file is org-canonical and vendored from sneak/prompts. Changing this repo's copy in isolation creates drift. Whether 60s is the new org-wide cap or an approved dnswatcher divergence is raised at sneak/prompts#41. That question must not block this work; land the edit here in a way that matches whichever answer comes back.

The finishing commit's title must end with (closes #93).

The `internal/resolver` tests on `main` query live nameservers (`ns1.google.com` etc.) and fail nondeterministically — a different subset each run — even in environments where direct `dig @ns1.google.com` works. This makes `make check` / CI flaky. **Definition of done rewritten 2026-08-10 to match sneak's ruling** (https://git.eeqj.de/sneak/dnswatcher/issues/93#issuecomment-50438). The original DoD proposed a `network` build tag or a `-short` skip; both are now explicitly off the table. His words: > make the cap 60s in both and never use mocking, always use live resolvers and assume the build and run environments have full unmodified unrestricted internet access. it is ok if they fail due to a bad build environment that alters dns packets. ## Definition of done 1. **`TESTING.md`**: the "within the 30-second target" wording becomes 60 seconds. 2. **`REPO_POLICIES.md`**: the `make test` ceiling of 20 seconds becomes 60 seconds, and the `-timeout` backstop is raised consistently with it. See the caveat below before editing this file. 3. **`script/test`**: the `go test -timeout` value must not be lower than the new cap, or the cap is unreachable in practice. 4. **Robustness, not gating**, is how the flakiness is addressed: retries with backoff, querying several independent nameservers and accepting a quorum rather than requiring every server to answer, and sensible per-query timeouts — now with a 60s budget to work inside. 5. **No mocking, no fakes, no stubs, no `-short`, no `network` build tag, no build-tag-gated tests.** Live resolvers always, in every package. 6. **No tolerance engineering for restricted egress.** Assume full unmodified unrestricted internet access. A failure caused by an environment that mangles DNS packets is an acceptable outcome and must not be worked around. 7. No change to production (non-test) resolver behaviour. 8. `make check` green, and green across repeated consecutive runs. Scoped to the above only — it does NOT resolve the `feature/resolver` reconciliation or the DNSSEC work in https://git.eeqj.de/sneak/dnswatcher/issues/59. ## Caveat on `REPO_POLICIES.md` That file is org-canonical and vendored from `sneak/prompts`. Changing this repo's copy in isolation creates drift. Whether 60s is the new org-wide cap or an approved dnswatcher divergence is raised at https://git.eeqj.de/sneak/prompts/issues/41. That question must not block this work; land the edit here in a way that matches whichever answer comes back. The finishing commit's title must end with ` (closes #93)`.
clawbot commented 2026-08-07 18:27:10 +02:00
Author
Collaborator
Copy Link

Reframing this issue under project policy before any implementation happens.

Policy: DNS is never mocked in dnswatcher

Per sneak (2026-08-07): DNS is never mocked in this project. Ever. No mock resolvers, no fake DNS servers, no stubbed lookups — not in tests, not anywhere. dnswatcher's entire purpose is correct behavior against the real DNS, and tests exercise real resolution against live nameservers by design. Flaky live tests are a robustness problem, never a justification for mocks. Any future worker picking this up: do not propose or implement mocked/hermetic DNS tests here — the change will be rejected. (This also means the "hermetic mocked tests" on origin/feature/resolver mentioned in TODO.md cannot land as-is.) This policy is being documented in the README via #94.

Where that leaves this issue

The current definition of done proposes guarding the live tests behind an opt-in (network build tag or -short skip). That is not mocking — the tests still hit real DNS when they run — but it changes what runs by default: make check would no longer exercise resolution at all unless opted in. That trade-off is an owner decision, so laying out the options:

  1. Make the live tests robust and keep them running by default. Retries with exponential backoff; query several independent nameservers and accept a quorum rather than requiring every server to answer; longer per-query timeouts. Default make check keeps exercising real DNS; flakiness is engineered out rather than routed around.
  2. Option 1 plus an opt-in strict/live-heavy tier. The default suite stays live but lean and robust (bounded query counts so it fits the 20-second make test budget in REPO_POLICIES.md); an opt-in tier (build tag) runs the exhaustive/strict variants that are too slow or too environment-sensitive for every run.
  3. Gate all live tests behind an opt-in (the current DoD): default make check becomes offline-deterministic; live tests run only on demand. Fastest and most deterministic default, but the default check then proves nothing about the one thing dnswatcher exists to do.

Recommendation

Option 1, falling back to option 2 only if robustness work can't keep the default suite reliably inside the 20-second make test budget. The repo's ethos is that testing against the real DNS is the test suite's value — a green main should mean the resolver actually resolves. The observed nondeterminism (different subset failing each run against ns1.google.com etc.) is exactly the failure mode that retry-with-backoff plus multi-nameserver quorum is designed to absorb. Notably, today's full make check run in a clean environment passed all 39 resolver tests live, so the baseline is closer to "occasionally flaky" than "unusably broken" — robustness looks achievable.

@sneak: please pick an option (1/2/3). Once decided, the DoD here should be rewritten to match, and implementation can proceed. Assigning to you for the decision.

Reframing this issue under project policy before any implementation happens. ## Policy: DNS is never mocked in dnswatcher Per sneak (2026-08-07): **DNS is never mocked in this project. Ever.** No mock resolvers, no fake DNS servers, no stubbed lookups — not in tests, not anywhere. dnswatcher's entire purpose is correct behavior against the real DNS, and tests exercise real resolution against live nameservers by design. Flaky live tests are a robustness problem, never a justification for mocks. Any future worker picking this up: do not propose or implement mocked/hermetic DNS tests here — the change will be rejected. (This also means the "hermetic mocked tests" on `origin/feature/resolver` mentioned in `TODO.md` cannot land as-is.) This policy is being documented in the README via #94. ## Where that leaves this issue The current definition of done proposes guarding the live tests behind an opt-in (`network` build tag or `-short` skip). That is **not** mocking — the tests still hit real DNS when they run — but it changes what runs by default: `make check` would no longer exercise resolution at all unless opted in. That trade-off is an owner decision, so laying out the options: 1. **Make the live tests robust and keep them running by default.** Retries with exponential backoff; query several independent nameservers and accept a quorum rather than requiring every server to answer; longer per-query timeouts. Default `make check` keeps exercising real DNS; flakiness is engineered out rather than routed around. 2. **Option 1 plus an opt-in strict/live-heavy tier.** The default suite stays live but lean and robust (bounded query counts so it fits the 20-second `make test` budget in `REPO_POLICIES.md`); an opt-in tier (build tag) runs the exhaustive/strict variants that are too slow or too environment-sensitive for every run. 3. **Gate all live tests behind an opt-in** (the current DoD): default `make check` becomes offline-deterministic; live tests run only on demand. Fastest and most deterministic default, but the default check then proves nothing about the one thing dnswatcher exists to do. ## Recommendation **Option 1**, falling back to option 2 only if robustness work can't keep the default suite reliably inside the 20-second `make test` budget. The repo's ethos is that testing against the real DNS *is* the test suite's value — a green `main` should mean the resolver actually resolves. The observed nondeterminism (different subset failing each run against `ns1.google.com` etc.) is exactly the failure mode that retry-with-backoff plus multi-nameserver quorum is designed to absorb. Notably, today's full `make check` run in a clean environment passed all 39 resolver tests live, so the baseline is closer to "occasionally flaky" than "unusably broken" — robustness looks achievable. @sneak: please pick an option (1/2/3). Once decided, the DoD here should be rewritten to match, and implementation can proceed. Assigning to you for the decision.
sneak was assigned by clawbot 2026-08-07 18:27:16 +02:00
clawbot commented 2026-08-09 03:42:53 +02:00
Author
Collaborator
Copy Link

[manager] Not working this issue — the approach decision is parked with @sneak. Adding one finding from the 1.0 backlog audit that bears directly on that decision, because it was not visible when this issue was written.

This issue's definition of done is now in direct conflict with TESTING.md on main.

Item 1 of the DoD here proposes guarding the live tests behind "a network build tag, or a testing.Short() skip with -short wired into script/test".

TESTING.md (added since, and reinforced by PR #97) says the opposite in as many words:

> - Do not add -short flags to skip slow tests
> - Do not increase -timeout to hide hanging queries
>
> Flaky failures from transient network issues are acceptable and should be investigated as potential resolver bugs, not papered over with mocks or skip flags.

So both of this issue's suggested mechanisms are explicitly forbidden by the repo's own testing policy. Implementing this as written would require either amending TESTING.md or overriding it — which is exactly why it is your call, not mine.

Two further data points from the audit:

  1. The suite is currently green and fast. make check on main (9347a28) passed in 7.8 seconds total, with lint clean. The resolver's live-DNS tests were not the bottleneck in this environment. Whatever the flakiness is, it is not reproducing here right now — which argues for the "investigate as a resolver bug" path over the gating path.

  2. There is a second, quieter cost. docker build runs make check, so the live-DNS coupling means the Docker build requires outbound UDP:53 from the build environment. In a network-restricted CI runner, essentially every resolver test would have to exhaust its retries against unreachable root servers before failing. Worst case that is bounded by a per-test 60s context (resolver_test.go:35-44) but plausibly exceeds the 30s go test -timeout. If the flakiness you saw was in CI rather than locally, egress restrictions are a likelier root cause than resolver bugs — and that would point at a third option neither this issue nor TESTING.md currently considers: fix the build environment's DNS egress, and change no test code at all.

Also worth noting: TESTING.md line 21 refers to keeping the suite "within the 30-second target", while REPO_POLICIES.md sets the make test ceiling at 20 seconds (with a 30s timeout as the backstop). Those two numbers should be reconciled whenever this issue is resolved.

No action taken. Flagging only so the decision is made against current facts.

**[manager]** Not working this issue — the approach decision is parked with @sneak. Adding one finding from the 1.0 backlog audit that bears directly on that decision, because it was not visible when this issue was written. **This issue's definition of done is now in direct conflict with `TESTING.md` on `main`.** Item 1 of the DoD here proposes guarding the live tests behind "a `network` build tag, or a `testing.Short()` skip with `-short` wired into `script/test`". `TESTING.md` (added since, and reinforced by [PR #97](https://git.eeqj.de/sneak/dnswatcher/pulls/97)) says the opposite in as many words: > - **Do not add `-short` flags** to skip slow tests > - **Do not increase `-timeout`** to hide hanging queries > > Flaky failures from transient network issues are acceptable and should be investigated as potential resolver bugs, not papered over with mocks or skip flags. So both of this issue's suggested mechanisms are explicitly forbidden by the repo's own testing policy. Implementing this as written would require either amending `TESTING.md` or overriding it — which is exactly why it is your call, not mine. **Two further data points from the audit:** 1. **The suite is currently green and fast.** `make check` on `main` (`9347a28`) passed in **7.8 seconds** total, with lint clean. The resolver's live-DNS tests were not the bottleneck in this environment. Whatever the flakiness is, it is not reproducing here right now — which argues for the "investigate as a resolver bug" path over the gating path. 2. **There is a second, quieter cost.** `docker build` runs `make check`, so the live-DNS coupling means the Docker build requires outbound UDP:53 from the build environment. In a network-restricted CI runner, essentially every resolver test would have to exhaust its retries against unreachable root servers before failing. Worst case that is bounded by a per-test 60s context (`resolver_test.go:35-44`) but plausibly exceeds the 30s `go test -timeout`. If the flakiness you saw was in CI rather than locally, egress restrictions are a likelier root cause than resolver bugs — and that would point at a third option neither this issue nor `TESTING.md` currently considers: fix the build environment's DNS egress, and change no test code at all. **Also worth noting:** `TESTING.md` line 21 refers to keeping the suite "within the 30-second target", while `REPO_POLICIES.md` sets the `make test` ceiling at **20 seconds** (with a 30s timeout as the backstop). Those two numbers should be reconciled whenever this issue is resolved. No action taken. Flagging only so the decision is made against current facts.
sneak commented 2026-08-09 17:08:31 +02:00
Owner
Copy Link

make the cap 60s in both and never use mocking, always use live resolvers and assume
the build and run environments have full unmodified unrestricted internet access. it is ok if they fail due to a bad build environment that alters dns packets.

make the cap 60s in both and never use mocking, always use live resolvers and assume the build and run environments have full unmodified unrestricted internet access. it is ok if they fail due to a bad build environment that alters dns packets.
clawbot commented 2026-08-10 13:17:58 +02:00
Author
Collaborator
Copy Link

[dispatcher] DECIDED by sneak 2026-08-09. His words verbatim:

make the cap 60s in both and never use mocking, always use live resolvers and assume the build and run environments have full unmodified unrestricted internet access. it is ok if they fail due to a bad build environment that alters dns packets.

So, unambiguously:

  • Option 1. Live resolvers always. No mocking, no -short, no network build tag. The two mechanisms this issue's original DoD proposed are both off the table, which resolves the conflict with TESTING.md in TESTING.md's favour.
  • The cap is 60 seconds in both documents — TESTING.md's target and REPO_POLICIES.md's ceiling, which currently disagree at 30s and 20s. Reconcile both to 60s.
  • Assume full, unmodified, unrestricted internet access in build and run environments. A failure caused by an environment that mangles DNS packets is acceptable and is not something to engineer around. That also disposes of the egress-restriction theory raised in the audit comment above: do not add environment-tolerance for it.

Rewrite the DoD to match and implement. The robustness work (retries, multi-nameserver quorum, sensible timeouts) is the sanctioned way to address flakiness, now with a 60s budget to work inside rather than 20s.

One thing needing care, flagged rather than assumed: REPO_POLICIES.md is the org-canonical file vendored from sneak/prompts. Changing this repo's copy to 60s in isolation creates drift from canonical. I have raised that upstream at sneak/prompts#41 to establish whether 60s is the new org-wide cap or an approved dnswatcher divergence — do not let that block the test work here, but land the REPO_POLICIES.md edit in a way that matches whichever answer comes back.

**[dispatcher] DECIDED by sneak 2026-08-09.** His words verbatim: > make the cap 60s in both and never use mocking, always use live resolvers and assume the build and run environments have full unmodified unrestricted internet access. it is ok if they fail due to a bad build environment that alters dns packets. So, unambiguously: - **Option 1.** Live resolvers always. No mocking, no `-short`, no `network` build tag. The two mechanisms this issue's original DoD proposed are both off the table, which resolves the conflict with `TESTING.md` in `TESTING.md`'s favour. - **The cap is 60 seconds in both documents** — `TESTING.md`'s target and `REPO_POLICIES.md`'s ceiling, which currently disagree at 30s and 20s. Reconcile both to 60s. - **Assume full, unmodified, unrestricted internet access** in build and run environments. A failure caused by an environment that mangles DNS packets is acceptable and is not something to engineer around. That also disposes of the egress-restriction theory raised in the audit comment above: do not add environment-tolerance for it. Rewrite the DoD to match and implement. The robustness work (retries, multi-nameserver quorum, sensible timeouts) is the sanctioned way to address flakiness, now with a 60s budget to work inside rather than 20s. One thing needing care, flagged rather than assumed: `REPO_POLICIES.md` is the org-canonical file vendored from `sneak/prompts`. Changing this repo's copy to 60s in isolation creates drift from canonical. I have raised that upstream at https://git.eeqj.de/sneak/prompts/issues/41 to establish whether 60s is the new org-wide cap or an approved dnswatcher divergence — do not let that block the test work here, but land the `REPO_POLICIES.md` edit in a way that matches whichever answer comes back.
sneak was unassigned by clawbot 2026-08-10 14:30:03 +02:00
clawbot self-assigned this 2026-08-10 14:30:03 +02:00
clawbot commented 2026-08-10 14:54:39 +02:00
Author
Collaborator
Copy Link

[worker] Implementation plan for #93, per the rewritten DoD. Working on next, single commit ending (closes #93).

Baseline measured first

On next (cc86473), make test runs the whole suite in 4.2 s, internal/resolver in 2.0 s, and I got 6 consecutive clean runs with zero failures. So the nondeterminism is not reproducing in this environment right now; the robustness work below targets the mechanisms that produce it rather than a failure I can currently trigger.

Where the nondeterminism actually comes from

Three structural properties of the current live tests, all test-side:

  1. Burst fan-out at one root server. ~35 resolver tests all call t.Parallel(), this host has 48 CPUs, so go test will run essentially all of them at once. Every one of them starts an independent iterative resolution, and queryServers walks rootServerList() in fixed order — so all ~35 resolutions fire their first query at 198.41.0.4 within milliseconds of each other. Root servers rate-limit. That is a very good fit for the reported symptom of a different subset failing each run.
  2. Every live call is single-shot. There is no retry anywhere in the test file; one dropped UDP packet anywhere in a delegation chain fails a test outright.
  3. Unanimity assertions. TestQueryAllNameservers_AllReturnOK and TestQueryAllNameservers_NXDomainFromAllNS require every google.com nameserver to answer. With 4 nameservers that is 4 independent chances to fail per run, and no tolerance for one being slow.

What I will do

Docs / caps (DoD 1-3):

  • TESTING.md: "within the 30-second target" becomes 60. One-line surgical edit only, so it merges cleanly with #97.
  • REPO_POLICIES.md: make test ceiling 20 s becomes 60 s, and the documented -timeout backstop is raised with it.
  • script/test: -timeout 30s raised to the same new backstop value.
  • Backstop value: -timeout 120s. It must strictly exceed the 60 s cap or the cap is unreachable; it must also exceed the per-test live context (60 s) so a hung test is reported by its own context rather than by a whole-suite panic. 2x the cap gives room for -race overhead. Stated in the PR body.
  • I will note plainly in the PR body that this repo's REPO_POLICIES.md now diverges from canonical pending sneak/prompts#41.

Robustness (DoD 4), all in internal/resolver test code:

  • New internal/resolver/livedns_test.go holding the live-DNS helpers, so resolver_test.go itself takes only small call-site edits and stays mergeable with #97.
  • Retry with exponential backoff around every live operation: 3 attempts, 10 s per-attempt context, 500 ms base backoff. Worst case ~31 s, inside the 60 s per-test ceiling. The retry predicate is deliberately transport-level — "did we get an answer at all" — never the semantic assertion, so a genuine resolver bug still fails on the first attempt's assertion instead of being retried away.
  • Bounded live concurrency: a package-level semaphore limits simultaneous live resolutions to a handful. Tests keep t.Parallel(); only their network work is throttled. This is the direct fix for the burst-at-one-root problem, and the 60 s budget is what makes it affordable.
  • Quorum instead of unanimity: a strict majority of the discovered nameservers must answer as expected; nameservers that time out are tolerated, but any nameserver returning a wrong answer (NXDOMAIN for google.com, or records for a name that must not exist) still fails the test. That keeps the assertions meaningful while absorbing one slow server.
  • One live NS lookup per domain per process run instead of one per test, feeding the nine tests whose subject is QueryNameserver rather than NS discovery. This is a live query executed once and shared, not a recorded fixture; NS discovery itself remains directly exercised by its own live tests.

Explicitly not doing: no mocks, fakes or stubs; no -short; no build tags; no skips; no environment-tolerance for restricted egress; no padding of timeouts in place of real work.

Production code (DoD 7): no changes. Note that the single most effective lever against root-server rate limiting would be randomising the order in which queryServers tries rootServerList(), which lives in production code — out of scope here, and I will flag it in the PR body as an observation rather than acting on it.

Verification (DoD 8): make check (Docker lint included) run repeatedly, not once; I will report the exact number of consecutive runs and any flake I see, including flakes I cannot reproduce.

**[worker]** Implementation plan for https://git.eeqj.de/sneak/dnswatcher/issues/93, per the rewritten DoD. Working on `next`, single commit ending ` (closes #93)`. ## Baseline measured first On `next` (`cc86473`), `make test` runs the whole suite in **4.2 s**, `internal/resolver` in **2.0 s**, and I got **6 consecutive clean runs** with zero failures. So the nondeterminism is not reproducing in this environment right now; the robustness work below targets the mechanisms that produce it rather than a failure I can currently trigger. ## Where the nondeterminism actually comes from Three structural properties of the current live tests, all test-side: 1. **Burst fan-out at one root server.** ~35 resolver tests all call `t.Parallel()`, this host has 48 CPUs, so `go test` will run essentially all of them at once. Every one of them starts an independent iterative resolution, and `queryServers` walks `rootServerList()` in fixed order — so all ~35 resolutions fire their first query at `198.41.0.4` within milliseconds of each other. Root servers rate-limit. That is a very good fit for the reported symptom of *a different subset failing each run*. 2. **Every live call is single-shot.** There is no retry anywhere in the test file; one dropped UDP packet anywhere in a delegation chain fails a test outright. 3. **Unanimity assertions.** `TestQueryAllNameservers_AllReturnOK` and `TestQueryAllNameservers_NXDomainFromAllNS` require *every* `google.com` nameserver to answer. With 4 nameservers that is 4 independent chances to fail per run, and no tolerance for one being slow. ## What I will do **Docs / caps (DoD 1-3):** - `TESTING.md`: "within the 30-second target" becomes 60. One-line surgical edit only, so it merges cleanly with https://git.eeqj.de/sneak/dnswatcher/pulls/97. - `REPO_POLICIES.md`: `make test` ceiling 20 s becomes 60 s, and the documented `-timeout` backstop is raised with it. - `script/test`: `-timeout 30s` raised to the same new backstop value. - Backstop value: **`-timeout 120s`**. It must strictly exceed the 60 s cap or the cap is unreachable; it must also exceed the per-test live context (60 s) so a hung test is reported by its own context rather than by a whole-suite panic. 2x the cap gives room for `-race` overhead. Stated in the PR body. - I will note plainly in the PR body that this repo's `REPO_POLICIES.md` now diverges from canonical pending https://git.eeqj.de/sneak/prompts/issues/41. **Robustness (DoD 4), all in `internal/resolver` test code:** - New `internal/resolver/livedns_test.go` holding the live-DNS helpers, so `resolver_test.go` itself takes only small call-site edits and stays mergeable with https://git.eeqj.de/sneak/dnswatcher/pulls/97. - **Retry with exponential backoff** around every live operation: 3 attempts, 10 s per-attempt context, 500 ms base backoff. Worst case ~31 s, inside the 60 s per-test ceiling. The retry predicate is deliberately *transport-level* — "did we get an answer at all" — never the semantic assertion, so a genuine resolver bug still fails on the first attempt's assertion instead of being retried away. - **Bounded live concurrency**: a package-level semaphore limits simultaneous live resolutions to a handful. Tests keep `t.Parallel()`; only their network work is throttled. This is the direct fix for the burst-at-one-root problem, and the 60 s budget is what makes it affordable. - **Quorum instead of unanimity**: a strict majority of the discovered nameservers must answer as expected; nameservers that time out are tolerated, but any nameserver returning a *wrong* answer (NXDOMAIN for `google.com`, or records for a name that must not exist) still fails the test. That keeps the assertions meaningful while absorbing one slow server. - **One live NS lookup per domain per process run** instead of one per test, feeding the nine tests whose subject is `QueryNameserver` rather than NS discovery. This is a live query executed once and shared, not a recorded fixture; NS discovery itself remains directly exercised by its own live tests. **Explicitly not doing:** no mocks, fakes or stubs; no `-short`; no build tags; no skips; no environment-tolerance for restricted egress; no padding of timeouts in place of real work. **Production code (DoD 7):** no changes. Note that the single most effective lever against root-server rate limiting would be randomising the order in which `queryServers` tries `rootServerList()`, which lives in production code — out of scope here, and I will flag it in the PR body as an observation rather than acting on it. **Verification (DoD 8):** `make check` (Docker lint included) run repeatedly, not once; I will report the exact number of consecutive runs and any flake I see, including flakes I cannot reproduce.
clawbot commented 2026-08-10 15:14:57 +02:00
Author
Collaborator
Copy Link

[worker] Implemented and pushed as 9cb2c2b on next, accumulated into #136. Full write-up is in that PR body; summary and verification here.

What was built

No mocks, fakes, stubs, recorded fixtures, -short, build tags or skips, and no environment-tolerance for restricted egress. Production resolver behaviour is unchanged.

New internal/resolver/livedns_test.go carries all the live-DNS machinery, so resolver_test.go itself only changes at call sites:

  • Bounded live concurrency. A package-wide semaphore caps live resolutions in flight at 6. This is the fix for what I believe was the actual root cause: every test in the package is parallel and this host has 48 cores, so ~35 iterative resolutions all started at once and, because queryServers walks rootServerList() in fixed order, all aimed their first query at 198.41.0.4. Root servers rate-limit that, which matches the reported symptom of a different arbitrary subset failing each run. Tests keep t.Parallel(); only their network work is throttled.
  • Retry with exponential backoff. Three attempts per live operation, 8s deadline each, 500ms base backoff doubling. The retry predicate is strictly transport-level — "did a nameserver answer at all" — never the assertion under test, so a resolver that answers incorrectly still fails on the first attempt instead of being retried into a false green.
  • Quorum instead of unanimity. TestQueryAllNameservers_AllReturnOK and _NXDomainFromAllNS no longer require every nameserver to answer; a strict majority suffices. A silent nameserver is tolerated, a wrong one is not: no server may report NXDOMAIN for google.com, and none may answer OK for a name that must not exist. The retry predicate also fires when fewer than a quorum answered at all.

New internal/resolver/livedns_harness_test.go tests that machinery directly — quorum arithmetic, status counting, the concurrency bound, per-attempt deadlines, and recovery from a transient failure. It performs no DNS resolution of any kind.

Caps and backstop

Landed against the org-wide ruling at sneak/prompts#41 (comment), which arrived while this was in flight and supersedes the caveat in the DoD above: 60s hard cap, 20s target, overage between the two filed as an improvement bug, 90s -timeout backstop.

  • script/test: -timeout 30s becomes -timeout 90s. The old value would have killed a 60s-capped suite at half its allowance, making the cap unreachable. Nothing else in the repo carried the old 30s.
  • REPO_POLICIES.md: not hand-edited. Copied byte-for-byte from prompts/REPO_POLICIES.md on sneak/prompts branch org-wide-60s-test-cap (52b5192), verified with cmp and sha256sum (bcf11c31...). This is the new canonical text, not a divergence. Re-vendoring the whole file also picks up drift this copy had fallen behind on (the last_modified date and the canonical golangci-lint v2.12.2 pin paragraph).
  • TESTING.md: "within the 30-second target" becomes 60, as a single-line edit so it merges cleanly when #97 lands.

Verification

Everything through make / script/ entrypoints; lint in Docker.

Ten consecutive make check runs, all exit 0, zero failures, and zero (cached) lines — Go's test cache reports ok pkg (cached) without executing anything, which would prove nothing about nondeterminism, so every run was forced to actually execute and each log checked. Wall times 26-47s (dominated by the Docker lint stage, which was confirmed executing rather than cached at 39.7s).

make test alone: 3.6-4.1s over three timed uncached runs. internal/resolver moved from 2.0s to ~2.9s, which is the concurrency gate's cost. Inside the 20s target, so no improvement bug is owed under the new two-tier rule.

What those runs do not prove, stated plainly. Live DNS was healthy the entire time: no live-DNS retry fired even once across the ten runs, and no flake was observed before or after the change either — six pre-change baseline runs on cc86473 were also clean. So the runs show the change is not itself flaky and does not slow the suite; they do not demonstrate recovery from a real DNS failure, because none occurred. The retry path is instead proven by TestRetryLiveRecoversFromTransientFailure, which is the sole source of the single retrying in 500ms line in each log.

One thing left alone, deliberately

The largest remaining lever against root-server rate limiting would be to stop queryServers trying rootServerList() in the same fixed order, spreading load across all thirteen roots instead of concentrating on a.root-servers.net. That is production code and item 7 of the DoD scopes this work as test-side, so I did not touch it. Flagging it as a question rather than filing it as a defect, since fixed-order root selection may well be a deliberate simplicity choice. @sneak, say the word if you want it as a separate issue.

**[worker]** Implemented and pushed as `9cb2c2b` on `next`, accumulated into https://git.eeqj.de/sneak/dnswatcher/pulls/136. Full write-up is in that PR body; summary and verification here. ## What was built **No mocks, fakes, stubs, recorded fixtures, `-short`, build tags or skips, and no environment-tolerance for restricted egress.** Production resolver behaviour is unchanged. New `internal/resolver/livedns_test.go` carries all the live-DNS machinery, so `resolver_test.go` itself only changes at call sites: - **Bounded live concurrency.** A package-wide semaphore caps live resolutions in flight at 6. This is the fix for what I believe was the actual root cause: every test in the package is parallel and this host has 48 cores, so ~35 iterative resolutions all started at once and, because `queryServers` walks `rootServerList()` in fixed order, all aimed their first query at `198.41.0.4`. Root servers rate-limit that, which matches the reported symptom of a different arbitrary subset failing each run. Tests keep `t.Parallel()`; only their network work is throttled. - **Retry with exponential backoff.** Three attempts per live operation, 8s deadline each, 500ms base backoff doubling. The retry predicate is strictly *transport-level* — "did a nameserver answer at all" — never the assertion under test, so a resolver that answers **incorrectly** still fails on the first attempt instead of being retried into a false green. - **Quorum instead of unanimity.** `TestQueryAllNameservers_AllReturnOK` and `_NXDomainFromAllNS` no longer require every nameserver to answer; a strict majority suffices. A silent nameserver is tolerated, a **wrong** one is not: no server may report NXDOMAIN for `google.com`, and none may answer OK for a name that must not exist. The retry predicate also fires when fewer than a quorum answered at all. New `internal/resolver/livedns_harness_test.go` tests that machinery directly — quorum arithmetic, status counting, the concurrency bound, per-attempt deadlines, and recovery from a transient failure. It performs no DNS resolution of any kind. ## Caps and backstop Landed against the org-wide ruling at https://git.eeqj.de/sneak/prompts/issues/41#issuecomment-53166, which arrived while this was in flight and supersedes the caveat in the DoD above: **60s hard cap, 20s target, overage between the two filed as an improvement bug, 90s `-timeout` backstop.** - `script/test`: `-timeout 30s` becomes `-timeout 90s`. The old value would have killed a 60s-capped suite at half its allowance, making the cap unreachable. Nothing else in the repo carried the old `30s`. - `REPO_POLICIES.md`: **not hand-edited.** Copied byte-for-byte from `prompts/REPO_POLICIES.md` on `sneak/prompts` branch `org-wide-60s-test-cap` (`52b5192`), verified with `cmp` and `sha256sum` (`bcf11c31...`). This is the new canonical text, not a divergence. Re-vendoring the whole file also picks up drift this copy had fallen behind on (the `last_modified` date and the canonical golangci-lint v2.12.2 pin paragraph). - `TESTING.md`: "within the 30-second target" becomes 60, as a single-line edit so it merges cleanly when https://git.eeqj.de/sneak/dnswatcher/pulls/97 lands. ## Verification Everything through `make` / `script/` entrypoints; lint in Docker. **Ten consecutive `make check` runs, all exit 0, zero failures, and zero `(cached)` lines** — Go's test cache reports `ok pkg (cached)` without executing anything, which would prove nothing about nondeterminism, so every run was forced to actually execute and each log checked. Wall times 26-47s (dominated by the Docker lint stage, which was confirmed executing rather than cached at 39.7s). **`make test` alone: 3.6-4.1s** over three timed uncached runs. `internal/resolver` moved from 2.0s to ~2.9s, which is the concurrency gate's cost. Inside the 20s target, so no improvement bug is owed under the new two-tier rule. **What those runs do not prove, stated plainly.** Live DNS was healthy the entire time: **no live-DNS retry fired even once** across the ten runs, and no flake was observed before or after the change either — six pre-change baseline runs on `cc86473` were also clean. So the runs show the change is not itself flaky and does not slow the suite; they do **not** demonstrate recovery from a real DNS failure, because none occurred. The retry path is instead proven by `TestRetryLiveRecoversFromTransientFailure`, which is the sole source of the single `retrying in 500ms` line in each log. ## One thing left alone, deliberately The largest remaining lever against root-server rate limiting would be to stop `queryServers` trying `rootServerList()` in the same fixed order, spreading load across all thirteen roots instead of concentrating on `a.root-servers.net`. That is production code and item 7 of the DoD scopes this work as test-side, so I did not touch it. Flagging it as a question rather than filing it as a defect, since fixed-order root selection may well be a deliberate simplicity choice. @sneak, say the word if you want it as a separate issue.
clawbot commented 2026-08-10 15:44:10 +02:00
Author
Collaborator
Copy Link

[manager] Landed on next (9cb2c2b + rework 87bce43), carried by #136. First review failed, rework passed a fresh review.

Worth recording, since it nearly shipped: the quorum change tolerated nodata as a wrong answer, so one bad nameserver in four passed. Fixed as a closed allowlist, verified by injection.

Two boundaries this unit deliberately does not cover, both tracked: test caching means a repeat green proves nothing (#139), and no live-DNS retry ever fired in ~20 runs, so real-failure recovery is evidenced only synthetically. make test uncached is 5.96s, inside the 20s target, so no improvement issue is owed.

error (and therefore SERVFAIL) is sanctioned in the quorum tests by design — a minority of actively SERVFAILing nameservers is tolerated.

**[manager]** Landed on `next` (`9cb2c2b` + rework `87bce43`), carried by https://git.eeqj.de/sneak/dnswatcher/pulls/136. First review failed, rework passed a fresh review. Worth recording, since it nearly shipped: the quorum change tolerated `nodata` as a wrong answer, so one bad nameserver in four passed. Fixed as a closed allowlist, verified by injection. Two boundaries this unit deliberately does not cover, both tracked: test caching means a repeat green proves nothing (https://git.eeqj.de/sneak/dnswatcher/issues/139), and no live-DNS retry ever fired in ~20 runs, so real-failure recovery is evidenced only synthetically. `make test` uncached is 5.96s, inside the 20s target, so no improvement issue is owed. `error` (and therefore SERVFAIL) is sanctioned in the quorum tests by design — a minority of actively SERVFAILing nameservers is tolerated.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#93
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?