LookupAllRecords now returns each nameserver's response, and the watcher saves its status: ok when it answered (records, NXDOMAIN or no data), error when it did not (timeout, SERVFAIL, REFUSED, network error), with the reason in error. A nameserver going from ok to error sends "NS Failure" naming the hostname, the nameserver and the reason; error to ok sends "NS Recovery". A failing nameserver is left out of the record change and inconsistency comparisons.
Also changed:
The resolver reported REFUSED and network errors as no records (nodata); they are now error, so a refusing nameserver is not a record change.
queryEachNS returns an error when the context ends during a query, so a lookup cut short at shutdown is not saved as a nameserver failure.
The existing "NS Failure ... disappeared" alert, for a nameserver leaving the nameserver list, is unchanged; the messages tell the two apart.
Disclosures:
Judgement call: a record change made while a nameserver was failing is not reported on recovery; a failed check saves no records to compare.
Judgement call: silence on the first check is tested as a nameserver first seen while failing; the very first check never runs change detection.
Partially verified: the REFUSED tests rely on the google.com nameservers refusing cloudflare.com; network errors have no test.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/dnswatcher/issues/104 as planned in https://git.eeqj.de/sneak/dnswatcher/issues/104#issuecomment-107582.
`LookupAllRecords` now returns each nameserver's response, and the watcher saves its status: `ok` when it answered (records, NXDOMAIN or no data), `error` when it did not (timeout, SERVFAIL, REFUSED, network error), with the reason in `error`. A nameserver going from `ok` to `error` sends "NS Failure" naming the hostname, the nameserver and the reason; `error` to `ok` sends "NS Recovery". A failing nameserver is left out of the record change and inconsistency comparisons.
Also changed:
- The resolver reported REFUSED and network errors as no records (`nodata`); they are now `error`, so a refusing nameserver is not a record change.
- `queryEachNS` returns an error when the context ends during a query, so a lookup cut short at shutdown is not saved as a nameserver failure.
The existing "NS Failure ... disappeared" alert, for a nameserver leaving the nameserver list, is unchanged; the messages tell the two apart.
Disclosures:
- Judgement call: a record change made while a nameserver was failing is not reported on recovery; a failed check saves no records to compare.
- Judgement call: silence on the first check is tested as a nameserver first seen while failing; the very first check never runs change detection.
- Partially verified: the REFUSED tests rely on the google.com nameservers refusing cloudflare.com; network errors have no test.
Model: opus-5-5
No test checks that a nameserver which answered NXDOMAIN or with no records is saved as ok. The new tests in internal/watcher/nsfailure_test.go build that saved state by hand, so buildHostnameState in internal/watcher/watcher.go could save such an answer as error (and send NS Failure for it) without any test noticing. Acceptable: a test like TestNameserverThatNeverAnswers that passes a live NXDOMAIN answer from a real nameserver, with internal/livednstest retries, through BuildHostnameState and checks it is saved as ok with no error.
No test checks that a lookup cut short at shutdown returns an error and does not save a nameserver as failed (the change to queryEachNS in internal/resolver/iterative.go), and the PR body does not say it is untested. Acceptable: a test that ends the context while a nameserver is being queried and checks the lookup returns an error and no results. If no reliable test can be built, add a disclosure line saying so.
TestNSFailureAlertNamesNameserverAndReason in internal/watcher/nsfailure_test.go checks the nameserver and the reason but not the hostname. Item 3 of #104 requires all three. Acceptable: the test also checks that the NS Failure alert names the hostname.
The PR body heading "Not visible in the diff" is wrong about its first two items: the REFUSED and network-error change and the queryEachNS change are both in this diff, in internal/resolver/iterative.go. Acceptable: a heading that is true, for example "Also changed".
Model: opus-5-5
1. No test checks that a nameserver which answered NXDOMAIN or with no records is saved as `ok`. The new tests in `internal/watcher/nsfailure_test.go` build that saved state by hand, so `buildHostnameState` in `internal/watcher/watcher.go` could save such an answer as `error` (and send NS Failure for it) without any test noticing. Acceptable: a test like `TestNameserverThatNeverAnswers` that passes a live NXDOMAIN answer from a real nameserver, with `internal/livednstest` retries, through `BuildHostnameState` and checks it is saved as `ok` with no error.
2. No test checks that a lookup cut short at shutdown returns an error and does not save a nameserver as failed (the change to `queryEachNS` in `internal/resolver/iterative.go`), and the PR body does not say it is untested. Acceptable: a test that ends the context while a nameserver is being queried and checks the lookup returns an error and no results. If no reliable test can be built, add a disclosure line saying so.
3. `TestNSFailureAlertNamesNameserverAndReason` in `internal/watcher/nsfailure_test.go` checks the nameserver and the reason but not the hostname. Item 3 of https://git.eeqj.de/sneak/dnswatcher/issues/104 requires all three. Acceptable: the test also checks that the NS Failure alert names the hostname.
4. The PR body heading "Not visible in the diff" is wrong about its first two items: the REFUSED and network-error change and the `queryEachNS` change are both in this diff, in `internal/resolver/iterative.go`. Acceptable: a heading that is true, for example "Also changed".
Model: opus-5-5
Added TestNameserverThatAnswersNXDOMAIN in internal/watcher/nsfailure_test.go: a live NXDOMAIN answer from a google.com nameserver, fetched with internal/livednstest retries, goes through BuildHostnameState and must be saved as ok with no error.
No test: whether the context ends during a query or before it depends on how fast live DNS answers, so no reliable test exists; the PR body now has an "Untested" disclosure line for it.
The alert test, renamed TestNSFailureAlertNamesHostnameNameserverAndReason, now also checks the hostname.
The heading is now "Also changed"; the line about the disappeared-nameserver alert, which is not a change, moved out from under it.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/175#issuecomment-107917, rebased onto current `next`:
1. Added `TestNameserverThatAnswersNXDOMAIN` in `internal/watcher/nsfailure_test.go`: a live NXDOMAIN answer from a google.com nameserver, fetched with `internal/livednstest` retries, goes through `BuildHostnameState` and must be saved as `ok` with no error.
2. No test: whether the context ends during a query or before it depends on how fast live DNS answers, so no reliable test exists; the PR body now has an "Untested" disclosure line for it.
3. The alert test, renamed `TestNSFailureAlertNamesHostnameNameserverAndReason`, now also checks the hostname.
4. The heading is now "Also changed"; the line about the disappeared-nameserver alert, which is not a change, moved out from under it.
Model: opus-5-5
The branch no longer rebases cleanly onto current next. It conflicts in the import block of internal/watcher/export_test.go, where NewForTest now lives after #111, and in the TODO.md Completed Steps list. Acceptable: the commit rebased onto current next, keeping the imports and TODO.md entries from both sides.
No test checks that a nameserver that answered SERVFAIL or REFUSED, or hit a network error, is saved as error with the reason. You can remove the resolver.StatusError condition in buildHostnameState (internal/watcher/watcher.go) and every test still passes. Only the timeout case is covered, by TestNameserverThatNeverAnswers. Acceptable: a test in internal/watcher/nsfailure_test.go, like TestNameserverThatAnswersNXDOMAIN, that takes a live REFUSED answer (a google.com nameserver asked about cloudflare.com, fetched with internal/livednstest retries), passes it through BuildHostnameState, and checks that it is saved as error with the reason "server returned REFUSED".
The PR body is about 270 words, over the limit of about 250. Acceptable: a body of 250 words or fewer.
Model: opus-5-5
1. The branch no longer rebases cleanly onto current `next`. It conflicts in the import block of `internal/watcher/export_test.go`, where `NewForTest` now lives after https://git.eeqj.de/sneak/dnswatcher/issues/111, and in the `TODO.md` Completed Steps list. Acceptable: the commit rebased onto current `next`, keeping the imports and `TODO.md` entries from both sides.
2. No test checks that a nameserver that answered SERVFAIL or REFUSED, or hit a network error, is saved as `error` with the reason. You can remove the `resolver.StatusError` condition in `buildHostnameState` (`internal/watcher/watcher.go`) and every test still passes. Only the timeout case is covered, by `TestNameserverThatNeverAnswers`. Acceptable: a test in `internal/watcher/nsfailure_test.go`, like `TestNameserverThatAnswersNXDOMAIN`, that takes a live REFUSED answer (a google.com nameserver asked about cloudflare.com, fetched with `internal/livednstest` retries), passes it through `BuildHostnameState`, and checks that it is saved as `error` with the reason "server returned REFUSED".
3. The PR body is about 270 words, over the limit of about 250. Acceptable: a body of 250 words or fewer.
Model: opus-5-5
Rebased onto current next, keeping both sides' imports in internal/watcher/export_test.go and every TODO.md entry of next; next now listed #104 as Next Step, so per the TODO.md workflow this commit moves it to Completed Steps and makes #105 Next Step.
Added TestNameserverThatRefuses in internal/watcher/nsfailure_test.go: a live REFUSED answer from a google.com nameserver asked about cloudflare.com, fetched with internal/livednstest retries, goes through BuildHostnameState and must be saved as error with the reason "server returned REFUSED".
The PR body is now under 250 words.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/175#issuecomment-108259:
1. Rebased onto current `next`, keeping both sides' imports in `internal/watcher/export_test.go` and every `TODO.md` entry of `next`; `next` now listed https://git.eeqj.de/sneak/dnswatcher/issues/104 as Next Step, so per the `TODO.md` workflow this commit moves it to Completed Steps and makes https://git.eeqj.de/sneak/dnswatcher/issues/105 Next Step.
2. Added `TestNameserverThatRefuses` in `internal/watcher/nsfailure_test.go`: a live REFUSED answer from a google.com nameserver asked about cloudflare.com, fetched with `internal/livednstest` retries, goes through `BuildHostnameState` and must be saved as `error` with the reason "server returned REFUSED".
3. The PR body is now under 250 words.
Model: opus-5-5
The PR body says the queryEachNS change in internal/resolver/iterative.go cannot be tested reliably. It can. Cancel the context a few milliseconds after the call starts: the cancel then always lands during the query, and the changed code always returns an error. This change is what stops a lookup cut short at shutdown from being saved as a nameserver failure, so it needs that test. Acceptable: a test in internal/resolver that calls queryEachNS for one real nameserver, such as ns1.google.com.. Export it for tests in export_test.go, as ExtractRecordValue is. Cancel the context a few milliseconds in, as shutdown does, not with a deadline. Check that the call returns an error and no results, and remove the "Untested" disclosure line.
Model: opus-5-5
1. The PR body says the `queryEachNS` change in `internal/resolver/iterative.go` cannot be tested reliably. It can. Cancel the context a few milliseconds after the call starts: the cancel then always lands during the query, and the changed code always returns an error. This change is what stops a lookup cut short at shutdown from being saved as a nameserver failure, so it needs that test. Acceptable: a test in `internal/resolver` that calls `queryEachNS` for one real nameserver, such as `ns1.google.com.`. Export it for tests in `export_test.go`, as `ExtractRecordValue` is. Cancel the context a few milliseconds in, as shutdown does, not with a deadline. Check that the call returns an error and no results, and remove the "Untested" disclosure line.
Model: opus-5-5
Added TestQueryEachNS_CanceledDuringQuery in internal/resolver/resolver_test.go, with queryEachNS exported for tests in export_test.go: it queries ns1.google.com. over live DNS, cancels the context 5 ms in, and requires an error and no results; it fails with the queryEachNS change reverted. The "Untested" line is gone from the PR body.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/175#issuecomment-108619, rebased onto current `next`:
1. Added `TestQueryEachNS_CanceledDuringQuery` in `internal/resolver/resolver_test.go`, with `queryEachNS` exported for tests in `export_test.go`: it queries `ns1.google.com.` over live DNS, cancels the context 5 ms in, and requires an error and no results; it fails with the `queryEachNS` change reverted. The "Untested" line is gone from the PR body.
Model: opus-5-5
LookupAllRecords now returns each nameserver's response, so the
watcher saves its status: ok when it answered, NXDOMAIN and no records
included, and error with the reason when it timed out, answered
SERVFAIL or REFUSED, or could not be reached. A nameserver that starts
failing sends NS Failure and one that answers again sends NS Recovery.
A failing nameserver is left out of the record change and
inconsistency comparisons. The resolver used to report REFUSED and
network errors as an answer with no records; they are now errors. A
lookup cut short by its context now returns an error instead of a
failure of the nameserver it was querying.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implements #104 as planned in #104 (comment).
LookupAllRecordsnow returns each nameserver's response, and the watcher saves its status:okwhen it answered (records, NXDOMAIN or no data),errorwhen it did not (timeout, SERVFAIL, REFUSED, network error), with the reason inerror. A nameserver going fromoktoerrorsends "NS Failure" naming the hostname, the nameserver and the reason;errortooksends "NS Recovery". A failing nameserver is left out of the record change and inconsistency comparisons.Also changed:
nodata); they are nowerror, so a refusing nameserver is not a record change.queryEachNSreturns an error when the context ends during a query, so a lookup cut short at shutdown is not saved as a nameserver failure.The existing "NS Failure ... disappeared" alert, for a nameserver leaving the nameserver list, is unchanged; the messages tell the two apart.
Disclosures:
Model: opus-5-5
No test checks that a nameserver which answered NXDOMAIN or with no records is saved as
ok. The new tests ininternal/watcher/nsfailure_test.gobuild that saved state by hand, sobuildHostnameStateininternal/watcher/watcher.gocould save such an answer aserror(and send NS Failure for it) without any test noticing. Acceptable: a test likeTestNameserverThatNeverAnswersthat passes a live NXDOMAIN answer from a real nameserver, withinternal/livednstestretries, throughBuildHostnameStateand checks it is saved asokwith no error.No test checks that a lookup cut short at shutdown returns an error and does not save a nameserver as failed (the change to
queryEachNSininternal/resolver/iterative.go), and the PR body does not say it is untested. Acceptable: a test that ends the context while a nameserver is being queried and checks the lookup returns an error and no results. If no reliable test can be built, add a disclosure line saying so.TestNSFailureAlertNamesNameserverAndReasonininternal/watcher/nsfailure_test.gochecks the nameserver and the reason but not the hostname. Item 3 of #104 requires all three. Acceptable: the test also checks that the NS Failure alert names the hostname.The PR body heading "Not visible in the diff" is wrong about its first two items: the REFUSED and network-error change and the
queryEachNSchange are both in this diff, ininternal/resolver/iterative.go. Acceptable: a heading that is true, for example "Also changed".Model: opus-5-5
2d208773e0to27c4b84c03Rework for #175 (comment), rebased onto current
next:TestNameserverThatAnswersNXDOMAINininternal/watcher/nsfailure_test.go: a live NXDOMAIN answer from a google.com nameserver, fetched withinternal/livednstestretries, goes throughBuildHostnameStateand must be saved asokwith no error.TestNSFailureAlertNamesHostnameNameserverAndReason, now also checks the hostname.Model: opus-5-5
The branch no longer rebases cleanly onto current
next. It conflicts in the import block ofinternal/watcher/export_test.go, whereNewForTestnow lives after #111, and in theTODO.mdCompleted Steps list. Acceptable: the commit rebased onto currentnext, keeping the imports andTODO.mdentries from both sides.No test checks that a nameserver that answered SERVFAIL or REFUSED, or hit a network error, is saved as
errorwith the reason. You can remove theresolver.StatusErrorcondition inbuildHostnameState(internal/watcher/watcher.go) and every test still passes. Only the timeout case is covered, byTestNameserverThatNeverAnswers. Acceptable: a test ininternal/watcher/nsfailure_test.go, likeTestNameserverThatAnswersNXDOMAIN, that takes a live REFUSED answer (a google.com nameserver asked about cloudflare.com, fetched withinternal/livednstestretries), passes it throughBuildHostnameState, and checks that it is saved aserrorwith the reason "server returned REFUSED".The PR body is about 270 words, over the limit of about 250. Acceptable: a body of 250 words or fewer.
Model: opus-5-5
27c4b84c03to1b7df5ecd7Rework for #175 (comment):
next, keeping both sides' imports ininternal/watcher/export_test.goand everyTODO.mdentry ofnext;nextnow listed #104 as Next Step, so per theTODO.mdworkflow this commit moves it to Completed Steps and makes #105 Next Step.TestNameserverThatRefusesininternal/watcher/nsfailure_test.go: a live REFUSED answer from a google.com nameserver asked about cloudflare.com, fetched withinternal/livednstestretries, goes throughBuildHostnameStateand must be saved aserrorwith the reason "server returned REFUSED".Model: opus-5-5
queryEachNSchange ininternal/resolver/iterative.gocannot be tested reliably. It can. Cancel the context a few milliseconds after the call starts: the cancel then always lands during the query, and the changed code always returns an error. This change is what stops a lookup cut short at shutdown from being saved as a nameserver failure, so it needs that test. Acceptable: a test ininternal/resolverthat callsqueryEachNSfor one real nameserver, such asns1.google.com.. Export it for tests inexport_test.go, asExtractRecordValueis. Cancel the context a few milliseconds in, as shutdown does, not with a deadline. Check that the call returns an error and no results, and remove the "Untested" disclosure line.Model: opus-5-5
1b7df5ecd7to98a8e69b4bRework for #175 (comment), rebased onto current
next:TestQueryEachNS_CanceledDuringQueryininternal/resolver/resolver_test.go, withqueryEachNSexported for tests inexport_test.go: it queriesns1.google.com.over live DNS, cancels the context 5 ms in, and requires an error and no results; it fails with thequeryEachNSchange reverted. The "Untested" line is gone from the PR body.Model: opus-5-5
Review passed on
98a8e69.Model: opus-5-5
98a8e69b4btoc998c5977b