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

Per-nameserver query status is discarded: NS failure and NS recovery notifications never fire #104

New Issue
Open
opened 2026-08-09 03:38:35 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 03:38:35 +02:00
Collaborator
Copy Link

Two of the README's headline notification types — NS query failure and NS recovery — are advertised prominently but cannot fire. The resolver computes the information correctly and the watcher then throws it away.

This is the largest functional gap standing between this repo and a defensible 1.0: a nameserver going dark is exactly the event a DNS monitoring daemon exists to report, and today it is silently invisible.

Current state (audited against origin/main, commit 9347a28)

The data exists and is correct at the resolver layer. internal/resolver/iterative.go:596-609 classifies each per-nameserver outcome into StatusOK, StatusNXDomain, StatusNoData, StatusTimeout, or StatusError (constants at internal/resolver/resolver.go:16-20), and queryEachNS populates the error path at iterative.go:686-696.

It is then discarded in two places:

  1. LookupAllRecords (internal/resolver/iterative.go:714-729) returns only NameserverResponse.Records, dropping .Status and .Error entirely (iterative.go:723-726).
  2. watcher.buildHostnameState (internal/watcher/watcher.go:339-359) then hardcodes Status: statusOK for every nameserver key present in the result (watcher.go:353).

The consequences cascade:

  • A nameserver that timed out, or returned SERVFAIL/REFUSED, is recorded as status: "ok" with an empty record set — indistinguishable from a nameserver that legitimately holds no records for that name.
  • The recovery branch in detectNSDisappearances (internal/watcher/watcher.go:428-445) is gated on prevNS.Status != statusError. Since Status is never anything but statusOK, that branch is unreachable dead code.
  • The words "timeout", "SERVFAIL", and "REFUSED" never reach a notification. They exist only inside resolver internals that watcher never reads.
  • The persisted error field on NameserverRecordState (internal/state/state.go:44-49) is never populated for nameserver entries, so the state file cannot show why a nameserver is unhealthy either.

The README claims (README.md:47-56):

> NS query failure: A nameserver that previously responded becomes unreachable (timeout, SERVFAIL, REFUSED, network error). This is distinct from "responded with no records."
> NS recovery: A previously-unreachable nameserver starts responding again.

Neither is true today. This also makes the blanket claim at README.md:106-107 ("Every observable state change produces a notification") false.

Definition of done

  1. LookupAllRecords (or a sibling method) preserves each nameserver's Status and Error instead of flattening to records-only. Changing the existing signature is acceptable — update all call sites. Do not add a second parallel code path that can drift from the first.
  2. buildHostnameState stores the real per-nameserver status rather than hardcoding statusOK, and populates NameserverRecordState.Error with the failure detail when the query failed.
  3. A distinct "NS query failure" notification fires when a nameserver that previously had a non-failing status starts failing. It must name the hostname, the nameserver, and the failure kind (timeout / SERVFAIL / REFUSED / network error) — the README promises that specificity.
  4. A distinct "NS recovery" notification fires when a nameserver whose stored status was a failure starts responding again. The now-dead branch at watcher.go:428-445 becomes genuinely reachable — verify that with a test rather than by inspection.
  5. A failing nameserver must not be reported as a "Record Change" merely because its record set went empty. Distinguishing "unreachable" from "responded with no records" is the explicit point of this issue; a test must cover exactly that distinction.
  6. Decide and document how StatusNXDomain / StatusNoData / StatusTimeout map onto persisted state. The README's status table (README.md:368-371) currently lists only ok and error. Either map the richer resolver statuses down to that pair, or widen the table — but code and README must agree when you are done, and the README table must be updated in the same commit.
  7. First-run behaviour is unchanged: a nameserver observed as failing on the very first check establishes a baseline silently and does not notify. This mirrors the existing first-run-baseline rule and must have a test.
  8. Tests cover, at minimum: failure fires once and not repeatedly on each subsequent identical check; recovery fires; empty-but-healthy does not produce a spurious failure notification; and first-run does not notify. Use the existing mocked resolver/portcheck/tlscheck interfaces in internal/watcher/interfaces.go — the watcher tests already work this way.
  9. make check is green and total make test wall time stays well under the 20-second policy ceiling. TODO.md is updated in the same commit as the work.

The finishing commit's title must end with (closes #N) referencing this issue.

Hard constraints

  • DNS is never mocked in this repository. Resolver tests use live DNS; robustness comes from tolerant assertions and sensible timeouts. The watcher tests mock the resolver interface (a seam above DNS), which is permitted and already the established pattern — do not confuse the two, and do not introduce a mock, fake, or stub DNS client, resolver transport, or nameserver anywhere.
  • Do not modify .golangci.yml, and do not change the golangci-lint version pin.

Out of scope

Glue / NS-IP change detection is a separate gap tracked in its own issue. Do not fold it in.

Two of the README's headline notification types — **NS query failure** and **NS recovery** — are advertised prominently but cannot fire. The resolver computes the information correctly and the watcher then throws it away. This is the largest functional gap standing between this repo and a defensible 1.0: a nameserver going dark is exactly the event a DNS monitoring daemon exists to report, and today it is silently invisible. ## Current state (audited against `origin/main`, commit `9347a28`) The data exists and is correct at the resolver layer. `internal/resolver/iterative.go:596-609` classifies each per-nameserver outcome into `StatusOK`, `StatusNXDomain`, `StatusNoData`, `StatusTimeout`, or `StatusError` (constants at `internal/resolver/resolver.go:16-20`), and `queryEachNS` populates the error path at `iterative.go:686-696`. It is then discarded in two places: 1. `LookupAllRecords` (`internal/resolver/iterative.go:714-729`) returns only `NameserverResponse.Records`, dropping `.Status` and `.Error` entirely (`iterative.go:723-726`). 2. `watcher.buildHostnameState` (`internal/watcher/watcher.go:339-359`) then hardcodes `Status: statusOK` for **every** nameserver key present in the result (`watcher.go:353`). The consequences cascade: - A nameserver that timed out, or returned SERVFAIL/REFUSED, is recorded as `status: "ok"` with an empty record set — **indistinguishable from a nameserver that legitimately holds no records for that name.** - The recovery branch in `detectNSDisappearances` (`internal/watcher/watcher.go:428-445`) is gated on `prevNS.Status != statusError`. Since `Status` is never anything but `statusOK`, that branch is **unreachable dead code**. - The words "timeout", "SERVFAIL", and "REFUSED" never reach a notification. They exist only inside `resolver` internals that `watcher` never reads. - The persisted `error` field on `NameserverRecordState` (`internal/state/state.go:44-49`) is never populated for nameserver entries, so the state file cannot show why a nameserver is unhealthy either. The README claims (`README.md:47-56`): > **NS query failure**: A nameserver that previously responded becomes unreachable (timeout, SERVFAIL, REFUSED, network error). This is distinct from "responded with no records." > **NS recovery**: A previously-unreachable nameserver starts responding again. Neither is true today. This also makes the blanket claim at `README.md:106-107` ("Every observable state change produces a notification") false. ## Definition of done 1. `LookupAllRecords` (or a sibling method) preserves each nameserver's `Status` and `Error` instead of flattening to records-only. Changing the existing signature is acceptable — update all call sites. Do not add a second parallel code path that can drift from the first. 2. `buildHostnameState` stores the real per-nameserver status rather than hardcoding `statusOK`, and populates `NameserverRecordState.Error` with the failure detail when the query failed. 3. A **distinct** "NS query failure" notification fires when a nameserver that previously had a non-failing status starts failing. It must name the hostname, the nameserver, and the failure kind (timeout / SERVFAIL / REFUSED / network error) — the README promises that specificity. 4. A **distinct** "NS recovery" notification fires when a nameserver whose stored status was a failure starts responding again. The now-dead branch at `watcher.go:428-445` becomes genuinely reachable — verify that with a test rather than by inspection. 5. A failing nameserver must **not** be reported as a "Record Change" merely because its record set went empty. Distinguishing "unreachable" from "responded with no records" is the explicit point of this issue; a test must cover exactly that distinction. 6. Decide and document how `StatusNXDomain` / `StatusNoData` / `StatusTimeout` map onto persisted state. The README's status table (`README.md:368-371`) currently lists only `ok` and `error`. Either map the richer resolver statuses down to that pair, or widen the table — but code and README must agree when you are done, and the README table must be updated in the same commit. 7. First-run behaviour is unchanged: a nameserver observed as failing on the very first check establishes a baseline silently and does not notify. This mirrors the existing first-run-baseline rule and must have a test. 8. Tests cover, at minimum: failure fires once and not repeatedly on each subsequent identical check; recovery fires; empty-but-healthy does not produce a spurious failure notification; and first-run does not notify. Use the existing mocked resolver/portcheck/tlscheck interfaces in `internal/watcher/interfaces.go` — the watcher tests already work this way. 9. `make check` is green and total `make test` wall time stays well under the 20-second policy ceiling. `TODO.md` is updated in the same commit as the work. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Hard constraints - **DNS is never mocked in this repository.** Resolver tests use live DNS; robustness comes from tolerant assertions and sensible timeouts. The watcher tests mock the *resolver interface* (a seam above DNS), which is permitted and already the established pattern — do not confuse the two, and do not introduce a mock, fake, or stub DNS client, resolver transport, or nameserver anywhere. - Do not modify `.golangci.yml`, and do not change the golangci-lint version pin. ## Out of scope Glue / NS-IP change detection is a separate gap tracked in its own issue. Do not fold it in.
clawbot added this to the 1.0 milestone 2026-08-09 03:38:35 +02:00
clawbot commented 2026-08-09 06:55:32 +02:00
Author
Collaborator
Copy Link

[manager] CORRECTION to item 8 of the definition of done — read this before starting.

I wrote item 8 saying to "use the existing mocked resolver/portcheck/tlscheck interfaces in internal/watcher/interfaces.go — the watcher tests already work this way." That was accurate for main and is wrong for the state this issue will actually be implemented against. Following it would undo work already in the merge queue.

What changed

PR #97 is merge-ready and awaiting @sneak. It deletes mockResolver entirely. Verified directly:

  • origin/main: internal/watcher/watcher_test.go contains 6 references to mockResolver.
  • PR #97 head (a535ae8): zero references. The watcher tests now construct the real iterative resolver with resolver.NewFromLogger(log) and query live DNS.

PR #97 also removes resolver.NewFromLoggerWithClient — the constructor that existed solely to inject a fake DNS client — and deletes the TESTING.md carve-out that previously permitted DNS mocks in packages consuming the resolver. The live-DNS policy now applies to every package, with no exceptions.

So the seam I pointed at no longer exists, and re-creating it would directly reverse #97.

The pattern to follow instead

PR #97 establishes how to test change-detection without mocking DNS: seed the state store with a synthetic previous observation that live DNS cannot possibly match, then run against live DNS and assert on the resulting transition. It uses reserved .invalid nameserver names and RFC 5737 documentation addresses (192.0.2.0/24, 203.0.113.0/24) as the synthetic prior state.

That maps cleanly onto this issue:

  • NS failure: record a nameserver in prior state as healthy that live DNS will not return as healthy, then assert exactly one failure notification naming that nameserver.
  • NS recovery: record a nameserver that live DNS will return, marked in prior state as failed, then assert exactly one recovery notification.
  • Failure is not a record change: this is the assertion that matters most in this issue and it is fully expressible against live DNS — assert the notification produced is the failure type, not a "Record Change".

Note PR #97 keeps the port checker, TLS checker, and notifier as test doubles, and deliberately generalised their assertions ("all ports open", "one cert for any address") so they hold for whatever live DNS returns — including multi-IP and AAAA answers. Those are not DNS and remain legitimate test doubles. Only the DNS/resolver layer is off limits.

Also note #97 explicitly dropped exact StatusTimeout assertions, because a real unreachable address may fail fast via ICMP unreachable rather than timing out, depending on network path. Do not assert a specific failure classification where the network can legitimately produce a different one — item 3 of the DoD asks the notification to name the failure kind, which is about what the code reports, not about pinning live DNS to one outcome. Assert tolerantly.

Consequences for whoever picks this up

  1. Do not add, restore, or recreate mockResolver, NewFromLoggerWithClient, or any fake/stub DNS client, resolver, transport, or nameserver. DNS is never mocked in this repository. This is the single issue in the 1.0 backlog where that temptation is strongest, because the thing being tested is nameserver failure.
  2. This issue must be implemented on top of #97, not on top of current main. Branching off main today means writing tests against a mockResolver that is about to be deleted, then having them destroyed in the rebase.
  3. Watch total make test wall time against the 20-second policy ceiling. This issue adds failure-path coverage, and #97 already moved the watcher suite onto live DNS — the two compound. Measure, do not assume.

This issue stays blocked until #97 merges. I am not dispatching an implementer for it before then. Same correction applies to #105.

**[manager] CORRECTION to item 8 of the definition of done — read this before starting.** I wrote item 8 saying to "use the existing mocked resolver/portcheck/tlscheck interfaces in `internal/watcher/interfaces.go` — the watcher tests already work this way." **That was accurate for `main` and is wrong for the state this issue will actually be implemented against.** Following it would undo work already in the merge queue. ## What changed [PR #97](https://git.eeqj.de/sneak/dnswatcher/pulls/97) is `merge-ready` and awaiting @sneak. It **deletes `mockResolver` entirely.** Verified directly: - `origin/main`: `internal/watcher/watcher_test.go` contains 6 references to `mockResolver`. - PR #97 head (`a535ae8`): **zero** references. The watcher tests now construct the real iterative resolver with `resolver.NewFromLogger(log)` and query live DNS. PR #97 also removes `resolver.NewFromLoggerWithClient` — the constructor that existed solely to inject a fake DNS client — and deletes the `TESTING.md` carve-out that previously permitted DNS mocks in packages consuming the resolver. **The live-DNS policy now applies to every package, with no exceptions.** So the seam I pointed at no longer exists, and re-creating it would directly reverse #97. ## The pattern to follow instead PR #97 establishes how to test change-detection without mocking DNS: **seed the state store with a synthetic previous observation that live DNS cannot possibly match, then run against live DNS and assert on the resulting transition.** It uses reserved `.invalid` nameserver names and RFC 5737 documentation addresses (`192.0.2.0/24`, `203.0.113.0/24`) as the synthetic prior state. That maps cleanly onto this issue: - **NS failure**: record a nameserver in prior state as healthy that live DNS will not return as healthy, then assert exactly one failure notification naming that nameserver. - **NS recovery**: record a nameserver that live DNS *will* return, marked in prior state as failed, then assert exactly one recovery notification. - **Failure is not a record change**: this is the assertion that matters most in this issue and it is fully expressible against live DNS — assert the notification produced is the failure type, not a "Record Change". Note PR #97 keeps the port checker, TLS checker, and notifier as test doubles, and deliberately generalised their assertions ("all ports open", "one cert for any address") so they hold for whatever live DNS returns — including multi-IP and AAAA answers. **Those are not DNS and remain legitimate test doubles.** Only the DNS/resolver layer is off limits. Also note #97 explicitly dropped exact `StatusTimeout` assertions, because a real unreachable address may fail fast via ICMP unreachable rather than timing out, depending on network path. **Do not assert a specific failure classification where the network can legitimately produce a different one** — item 3 of the DoD asks the notification to name the failure kind, which is about what the code reports, not about pinning live DNS to one outcome. Assert tolerantly. ## Consequences for whoever picks this up 1. **Do not add, restore, or recreate `mockResolver`, `NewFromLoggerWithClient`, or any fake/stub DNS client, resolver, transport, or nameserver.** DNS is never mocked in this repository. This is the single issue in the 1.0 backlog where that temptation is strongest, because the thing being tested *is* nameserver failure. 2. **This issue must be implemented on top of #97, not on top of current `main`.** Branching off `main` today means writing tests against a `mockResolver` that is about to be deleted, then having them destroyed in the rebase. 3. Watch total `make test` wall time against the 20-second policy ceiling. This issue adds failure-path coverage, and #97 already moved the watcher suite onto live DNS — the two compound. Measure, do not assume. **This issue stays blocked until #97 merges.** I am not dispatching an implementer for it before then. Same correction applies to #105.
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 1.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#104
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?