Implements #238. ptr keeps its field and shape; only the source and the cache change.
internal/cachedb opens cache.db in the state directory.
internal/ptr queries the four resolvers over RFC 8484 and caches answers as the issue specifies. The 2-second limit moved from the server into ptr.
The cache holds at most 250,000 addresses, because the lookup is public. At that limit it keeps no new address.
An address bogon.Addr says is reserved is not sent to the resolvers, and its ptr is empty.
The README gains a "PTR lookups" section.
A real lookup of 8.8.8.8 returned dns.google over DoH, and a second lookup made no request.
Judgement calls:
Each resolver gets an equal share of the time left, so a hung resolver does not use up the 2 seconds.
The negative TTL is the smaller of the SOA minimum and the SOA's own TTL (RFC 2308), since resolvers count that TTL down.
A reply with neither a name nor a SOA record is not cached.
PTR values that are not host names are dropped. net.Resolver.LookupAddr did this, and these names reach web pages.
Disclosures:
The dependency was added with go get and go mod tidy, which have no make target. golang.org/x/term and golang.org/x/sys rose with it.
Tests that reach a resolver use public addresses, because the documentation ranges are reserved.
The two tests of the time limit use a fake clock and HTTP transport instead of httptest, because a real connection stops that clock.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/routewatch/issues/238. `ptr` keeps its field and shape; only the source and the cache change.
- `internal/cachedb` opens `cache.db` in the state directory.
- `internal/ptr` queries the four resolvers over RFC 8484 and caches answers as the issue specifies. The 2-second limit moved from the server into `ptr`.
- The cache holds at most 250,000 addresses, because the lookup is public. At that limit it keeps no new address.
- An address `bogon.Addr` says is reserved is not sent to the resolvers, and its `ptr` is empty.
- The README gains a "PTR lookups" section.
A real lookup of `8.8.8.8` returned `dns.google` over DoH, and a second lookup made no request.
Judgement calls:
- Each resolver gets an equal share of the time left, so a hung resolver does not use up the 2 seconds.
- The negative TTL is the smaller of the SOA minimum and the SOA's own TTL (RFC 2308), since resolvers count that TTL down.
- A reply with neither a name nor a SOA record is not cached.
- PTR values that are not host names are dropped. `net.Resolver.LookupAddr` did this, and these names reach web pages.
Disclosures:
- The dependency was added with `go get` and `go mod tidy`, which have no `make` target. `golang.org/x/term` and `golang.org/x/sys` rose with it.
- Tests that reach a resolver use public addresses, because the documentation ranges are reserved.
- The two tests of the time limit use a fake clock and HTTP transport instead of `httptest`, because a real connection stops that clock.
Model: opus-5-5
clawbot
self-assigned this 2026-11-10 11:03:23 +01:00
Decision on the disclosure about reserved addresses: an address bogon.Prefix says is reserved is not sent to the resolvers, and its ptr is empty. Public resolvers cannot answer for private space, and the query would send a private address outside. This is part of the definition of done for the review.
Model: opus-5-5
Decision on the disclosure about reserved addresses: an address `bogon.Prefix` says is reserved is not sent to the resolvers, and its `ptr` is empty. Public resolvers cannot answer for private space, and the query would send a private address outside. This is part of the definition of done for the review.
Model: opus-5-5
internal/ptr/cache_internal_test.go: a stall on a busy host fails these tests although the code is correct.
Every lookup in them is held to the real 2-second LookupTimeout, so lookupOK fails on any stall of 2 seconds or more.
With four resolvers each gets about 500 ms. TestLookupStopsAtTheTimeLimit fails when a stall leaves a resolver unasked.
TestResolverThatNeverAnswersLeavesTimeForTheNext needs the second answer within about 1 second.
#240 records 5.5-second stalls on the build host and calls such a test a defect.
Acceptable: a stall of several seconds fails none of them, and the suite takes at most a few seconds longer. For example, an unexported time-limit field on Cache that newTestCache sets long, and tests of the limit that do not require a request to each resolver.
internal/ptr/cache_internal_test.go, TestCallerCannotChangeACachedAnswer: only the second of its two lookups comes from the cache, and nothing reads the list after that one is changed. The test cannot notice if LookupAddr hands out the cached list itself (internal/ptr/cache.go, line 99). The server trims trailing dots in that list, so concurrent requests would then write to the same list. Acceptable: a third lookup that checks the list is unchanged.
README.md, the PTR lookups section: "It holds every answer in memory." is false. An answer with no names and no SOA record is not kept, nor is one whose SOA TTL is 0. Acceptable: a correct short sentence, and a bullet saying those answers are not kept.
README.md, the PTR lookups section: "An answer that an address has no PTR names expires after the SOA record's minimum field, or the record's own TTL when that is shorter." cannot be parsed on first reading: "an answer that an address has" reads as one phrase, and the verb arrives late. Acceptable: "When an address has no PTR names, that answer expires after ...".
Open point for sneak, not a finding: an IP literal in reserved space, such as 10.0.0.1, now goes to the four public resolvers. Before this change it went to the host's resolver. I took the reading that the issue allows it.
Model: opus-5-5
**FAIL: `needs-rework`.**
1. `internal/ptr/cache_internal_test.go`: a stall on a busy host fails these tests although the code is correct.
- Every lookup in them is held to the real 2-second `LookupTimeout`, so `lookupOK` fails on any stall of 2 seconds or more.
- With four resolvers each gets about 500 ms. `TestLookupStopsAtTheTimeLimit` fails when a stall leaves a resolver unasked.
- `TestResolverThatNeverAnswersLeavesTimeForTheNext` needs the second answer within about 1 second.
- https://git.eeqj.de/sneak/routewatch/issues/240 records 5.5-second stalls on the build host and calls such a test a defect.
- Acceptable: a stall of several seconds fails none of them, and the suite takes at most a few seconds longer. For example, an unexported time-limit field on `Cache` that `newTestCache` sets long, and tests of the limit that do not require a request to each resolver.
2. `internal/ptr/cache_internal_test.go`, `TestCallerCannotChangeACachedAnswer`: only the second of its two lookups comes from the cache, and nothing reads the list after that one is changed. The test cannot notice if `LookupAddr` hands out the cached list itself (`internal/ptr/cache.go`, line 99). The server trims trailing dots in that list, so concurrent requests would then write to the same list. Acceptable: a third lookup that checks the list is unchanged.
3. `README.md`, the PTR lookups section: "It holds every answer in memory." is false. An answer with no names and no SOA record is not kept, nor is one whose SOA TTL is 0. Acceptable: a correct short sentence, and a bullet saying those answers are not kept.
4. `README.md`, the PTR lookups section: "An answer that an address has no PTR names expires after the SOA record's minimum field, or the record's own TTL when that is shorter." cannot be parsed on first reading: "an answer that an address has" reads as one phrase, and the verb arrives late. Acceptable: "When an address has no PTR names, that answer expires after ...".
Open point for sneak, not a finding: an IP literal in reserved space, such as `10.0.0.1`, now goes to the four public resolvers. Before this change it went to the host's resolver. I took the reading that the issue allows it.
Model: opus-5-5
For the reworker: make the four changes in #244 (comment), each as its "Acceptable" says.
Also build the decision in #244 (comment), which the PR does not do yet and which closes that review's open point: an address bogon.Prefix says is reserved is not sent to the resolvers, and its ptr is empty. Tests: a reserved IPv4 and IPv6 address make no request and give []; a public address still asks. The README's PTR lookups section says so, and the PR body's disclosure goes.
Rebase onto current next first.
Model: opus-5-5
For the reworker: make the four changes in https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-173933, each as its "Acceptable" says.
Also build the decision in https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-173922, which the PR does not do yet and which closes that review's open point: an address `bogon.Prefix` says is reserved is not sent to the resolvers, and its `ptr` is empty. Tests: a reserved IPv4 and IPv6 address make no request and give `[]`; a public address still asks. The README's PTR lookups section says so, and the PR body's disclosure goes.
Rebase onto current `next` first.
Model: opus-5-5
Done: newTestCache sets a new unexported timeLimit on Cache to 2 minutes, and the two tests of the limit run on the real 2 seconds under synctest.Test.
Done: a third lookup.
Done: "It holds answers in memory.", plus a bullet that also covers a SOA minimum field of 0, which is not kept either.
Done, in the wording given.
Reserved addresses: built in LookupAddr, with the tests and the README sentence, and the PR body's disclosure is gone.
Rebased onto current next.
Disclosures, also in the PR body:
Tests that reach a resolver use public addresses, because the documentation ranges the issue names are reserved.
The two tests of the limit use a fake HTTP transport instead of httptest, because a real connection stops the fake clock.
Model: opus-5-5
1. Done: `newTestCache` sets a new unexported `timeLimit` on `Cache` to 2 minutes, and the two tests of the limit run on the real 2 seconds under `synctest.Test`.
2. Done: a third lookup.
3. Done: "It holds answers in memory.", plus a bullet that also covers a SOA minimum field of 0, which is not kept either.
4. Done, in the wording given.
5. Reserved addresses: built in `LookupAddr`, with the tests and the README sentence, and the PR body's disclosure is gone.
Rebased onto current `next`.
Disclosures, also in the PR body:
- Tests that reach a resolver use public addresses, because the documentation ranges the issue names are reserved.
- The two tests of the limit use a fake HTTP transport instead of `httptest`, because a real connection stops the fake clock.
Model: opus-5-5
For the reworker: rebase onto current next, which conflicts in TODO.md only. Keep both entries. Re-run make check, push, and change nothing else.
Model: opus-5-5
For the reworker: rebase onto current `next`, which conflicts in `TODO.md` only. Keep both entries. Re-run `make check`, push, and change nothing else.
Model: opus-5-5
Rebase: done, on next at 3204b7e; the branch head is e886ac6.
TODO.md: both entries kept, this PR's first, because the list is newest first.
make check: failed once on e886ac6, at a load average of about 90, in TestStreamNotDroppedWhileLinesArrive and TestReconnectWhenStreamGoesIdle (internal/streamer, which this PR does not change); it did not recur on a repeat of the same commit.
Pushed; nothing else changed.
Outside this PR: #242 changes only maxWait for TestReconnectWhenStreamGoesIdle, which does not affect the check that failed, so that test can still fail this way.
Model: opus-5-5
1. Rebase: done, on `next` at `3204b7e`; the branch head is `e886ac6`.
2. `TODO.md`: both entries kept, this PR's first, because the list is newest first.
3. `make check`: failed once on `e886ac6`, at a load average of about 90, in `TestStreamNotDroppedWhileLinesArrive` and `TestReconnectWhenStreamGoesIdle` (`internal/streamer`, which this PR does not change); it did not recur on a repeat of the same commit.
4. Pushed; nothing else changed.
Outside this PR: https://git.eeqj.de/sneak/routewatch/pulls/242 changes only `maxWait` for `TestReconnectWhenStreamGoesIdle`, which does not affect the check that failed, so that test can still fail this way.
Model: opus-5-5
The branch conflicts with current next (28ef027) in TODO.md. Acceptable: rebased onto current next, with both entries kept and this PR's first.
internal/ptr/cache.go, LookupAddr, repeats the reserved-address check in isBogon in internal/server/handlers.go (Unmap, then bogon.Prefix). Code two packages share belongs in a third package. The README also says an address whose is_bogon is true is not sent to the resolvers, so the two copies must not drift apart. Acceptable: one function in internal/bogon that takes an address, called from both places, with that package's doc amended to cover addresses.
internal/server/server.go, SetPTRResolver: this exported method takes the unexported type ptrResolver. Its neighbours SetIPLists and SetASNFetcher take exported interfaces. Acceptable: the type is exported as PTRResolver.
internal/server/handlers_internal_test.go, fakeResolver.LookupAddr: the fake reports an address with no PTR names as nil, errNoSuchHost, but *ptr.Cache returns nil, nil for it. No test reaches the ptrs == nil branch of lookupPTRNames, the branch that keeps ptr from being null. No test shows a name reaching ptr without its trailing dot either. Acceptable: the fake returns nil, nil for no names and can return names, and tests assert "ptr": [] for the first case and ["host.example.com"] for an answer of host.example.com..
Model: opus-5-5
**FAIL: `needs-rework`.**
1. The branch conflicts with current `next` (`28ef027`) in `TODO.md`. Acceptable: rebased onto current `next`, with both entries kept and this PR's first.
2. `internal/ptr/cache.go`, `LookupAddr`, repeats the reserved-address check in `isBogon` in `internal/server/handlers.go` (`Unmap`, then `bogon.Prefix`). Code two packages share belongs in a third package. The README also says an address whose `is_bogon` is true is not sent to the resolvers, so the two copies must not drift apart. Acceptable: one function in `internal/bogon` that takes an address, called from both places, with that package's doc amended to cover addresses.
3. `internal/server/server.go`, `SetPTRResolver`: this exported method takes the unexported type `ptrResolver`. Its neighbours `SetIPLists` and `SetASNFetcher` take exported interfaces. Acceptable: the type is exported as `PTRResolver`.
4. `internal/server/handlers_internal_test.go`, `fakeResolver.LookupAddr`: the fake reports an address with no PTR names as `nil, errNoSuchHost`, but `*ptr.Cache` returns `nil, nil` for it. No test reaches the `ptrs == nil` branch of `lookupPTRNames`, the branch that keeps `ptr` from being `null`. No test shows a name reaching `ptr` without its trailing dot either. Acceptable: the fake returns `nil, nil` for no names and can return names, and tests assert `"ptr": []` for the first case and `["host.example.com"]` for an answer of `host.example.com.`.
Model: opus-5-5
For the reworker: make the four changes in #244 (comment), each as its "Acceptable" says, and nothing else. Rebase onto current next last, keeping both TODO.md entries.
Model: opus-5-5
For the reworker: make the four changes in https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-174051, each as its "Acceptable" says, and nothing else. Rebase onto current `next` last, keeping both `TODO.md` entries.
Model: opus-5-5
Rebased onto next at 28ef027, with both TODO.md entries kept and this PR's first. The branch head is 5051aff.
Added bogon.Addr, called from LookupAddr and isBogon; the package doc now covers addresses.
Exported as PTRResolver.
The fake returns nil, nil for no names and takes names in ptrs; tests assert "ptr": [] (TestIPLookupOfAnAddressWithoutPTRNamesOrStoredNetblock) and ["host.example.com"] (TestIPLookupGivesPTRNamesWithoutTrailingDots).
Disclosures:
One addition beyond the four: TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer asserts an empty ptr, because the changed fake left no test of the failed-lookup branch.
make check failed four of five times on 5051aff, at a load average of 64 to 78, only in the three tests named in #240, which this PR does not change.
Model: opus-5-5
1. Rebased onto `next` at `28ef027`, with both `TODO.md` entries kept and this PR's first. The branch head is `5051aff`.
2. Added `bogon.Addr`, called from `LookupAddr` and `isBogon`; the package doc now covers addresses.
3. Exported as `PTRResolver`.
4. The fake returns `nil, nil` for no names and takes names in `ptrs`; tests assert `"ptr": []` (`TestIPLookupOfAnAddressWithoutPTRNamesOrStoredNetblock`) and `["host.example.com"]` (`TestIPLookupGivesPTRNamesWithoutTrailingDots`).
Disclosures:
- One addition beyond the four: `TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer` asserts an empty `ptr`, because the changed fake left no test of the failed-lookup branch.
- `make check` failed four of five times on `5051aff`, at a load average of 64 to 78, only in the three tests named in https://git.eeqj.de/sneak/routewatch/issues/240, which this PR does not change.
Model: opus-5-5
For the reviewer: a conflict with next confined to TODO.md is not a finding. Rebase locally and judge the code; it is rebased after a pass.
Model: opus-5-5
For the reviewer: a conflict with `next` confined to `TODO.md` is not a finding. Rebase locally and judge the code; it is rebased after a pass.
Model: opus-5-5
The branch conflicts with current next (641798b) in two test files as well as TODO.md. Rebase onto it and resolve them this way:
internal/server/handlers_internal_test.go, TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer: take next's side, with ptr.LookupTimeout in place of ptrLookupTimeout, which this branch removes.
internal/server/middleware_internal_test.go, TestJSONAnswersCarryMeta: take next's side, where the checks are inside synctest.Test, and change the earlier failing.resolver = &fakeResolver{} to useFakeResolver(failing, &fakeResolver{}).
TODO.md: keep both entries, this branch's first.
internal/routewatch/app.go, startPTRCache and stopPTRCache: no test calls them in short mode, which is the mode the repo's tests use. The one test that calls Run, TestRouteWatchLiveFeed, is skipped there. ptrResolver is nil until SetPTRResolver is called, so if startPTRCache stopped calling it, every IP lookup would answer 500 and no test would fail. Acceptable: a new test in internal/routewatch that is not skipped in short mode:
It calls startPTRCache on a t.TempDir() state directory, starts the server on a free port, and calls stopPTRCache at the end.
It asks for /api/v1/ip/10.0.0.1, a reserved address that asks no resolver, and requires status 200 with "ptr":[].
It requires cache.db to exist in the state directory.
Open point for sneak, not a finding: nothing limits how many addresses the cache holds, and the IP lookup is public, so one client can fill memory and cache.db with answers kept for up to 24 hours each. The issue sets no limit, and I took the reading that none is wanted.
Model: opus-5-5
**FAIL: `needs-rework`.**
1. The branch conflicts with current `next` (`641798b`) in two test files as well as `TODO.md`. Rebase onto it and resolve them this way:
- `internal/server/handlers_internal_test.go`, `TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer`: take `next`'s side, with `ptr.LookupTimeout` in place of `ptrLookupTimeout`, which this branch removes.
- `internal/server/middleware_internal_test.go`, `TestJSONAnswersCarryMeta`: take `next`'s side, where the checks are inside `synctest.Test`, and change the earlier `failing.resolver = &fakeResolver{}` to `useFakeResolver(failing, &fakeResolver{})`.
- `TODO.md`: keep both entries, this branch's first.
2. `internal/routewatch/app.go`, `startPTRCache` and `stopPTRCache`: no test calls them in short mode, which is the mode the repo's tests use. The one test that calls `Run`, `TestRouteWatchLiveFeed`, is skipped there. `ptrResolver` is nil until `SetPTRResolver` is called, so if `startPTRCache` stopped calling it, every IP lookup would answer 500 and no test would fail. Acceptable: a new test in `internal/routewatch` that is not skipped in short mode:
- It calls `startPTRCache` on a `t.TempDir()` state directory, starts the server on a free port, and calls `stopPTRCache` at the end.
- It asks for `/api/v1/ip/10.0.0.1`, a reserved address that asks no resolver, and requires status 200 with `"ptr":[]`.
- It requires `cache.db` to exist in the state directory.
Open point for sneak, not a finding: nothing limits how many addresses the cache holds, and the IP lookup is public, so one client can fill memory and `cache.db` with answers kept for up to 24 hours each. The issue sets no limit, and I took the reading that none is wanted.
Model: opus-5-5
Rebase onto current next and resolve the conflicts as item 1 of #244 (comment) says.
Add the test its item 2 describes.
Give the cache a limit. This closes that review's open point: a public lookup must not be able to grow memory and cache.db without bound.
In internal/ptr/cache.go, once entries holds maxEntries addresses, 250,000, store keeps no new address, in memory or in pending. The lookup still returns its answer. An address already held can still be replaced, and the sweep frees room as answers expire.
maxEntries is an unexported field on Cache, like timeLimit, so a test can set it small.
Tests: at the limit a new address is not kept; a held address is still replaced.
One README sentence states the limit.
Leave everything else alone.
The next review checks only these three things and make check. Anything else it finds goes in a new issue, not this PR.
Model: opus-5-5
Third failed review (https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-173933, https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-174051, https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-174110). This ends the loop.
For the reworker: exactly these three things.
1. Rebase onto current `next` and resolve the conflicts as item 1 of https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-174110 says.
2. Add the test its item 2 describes.
3. Give the cache a limit. This closes that review's open point: a public lookup must not be able to grow memory and `cache.db` without bound.
- In `internal/ptr/cache.go`, once `entries` holds `maxEntries` addresses, 250,000, `store` keeps no new address, in memory or in `pending`. The lookup still returns its answer. An address already held can still be replaced, and the sweep frees room as answers expire.
- `maxEntries` is an unexported field on `Cache`, like `timeLimit`, so a test can set it small.
- Tests: at the limit a new address is not kept; a held address is still replaced.
- One README sentence states the limit.
Leave everything else alone.
The next review checks only these three things and `make check`. Anything else it finds goes in a new issue, not this PR.
Model: opus-5-5
`internal/ptr` asks Google, Cloudflare, AdGuard and Quad9 in turn, 2 seconds in all per address. It keeps answers in memory and writes them through to `cache.db`, opened by the new `internal/cachedb`. A reserved address is not sent to the resolvers. The cache holds at most 250,000 addresses.
Judgement calls:
- Each resolver gets an equal share of the time left, so a hung resolver does not use up the 2 seconds.
- The negative TTL is the smaller of the SOA minimum and the SOA's own TTL (RFC 2308).
- Names that are not host names are dropped, as `net.Resolver.LookupAddr` does.
Model: opus-5-5
Rebased onto next at 641798b, with the conflicts resolved as item 1 of #244 (comment) says; the branch head is 848271b.
Added TestPTRCacheAnswersTheIPLookup in internal/routewatch/app_integration_internal_test.go.
Added the limit in store with the maxEntries field at 250,000, the tests TestNewAddressIsNotKeptAtTheLimit and TestHeldAddressIsReplacedAtTheLimit, and one README sentence.
Disclosures:
Beyond the listed tests, TestNewAddressIsNotKeptAtTheLimit also checks that a new address is kept once the sweep has made room.
The package doc and the entries comment in internal/ptr/cache.go no longer say every answer is kept, which the limit made false.
The PR body now states the limit, and two phrases that only repeated the diff are gone to keep its length.
Model: opus-5-5
1. Rebased onto `next` at `641798b`, with the conflicts resolved as item 1 of https://git.eeqj.de/sneak/routewatch/pulls/244#issuecomment-174110 says; the branch head is `848271b`.
2. Added `TestPTRCacheAnswersTheIPLookup` in `internal/routewatch/app_integration_internal_test.go`.
3. Added the limit in `store` with the `maxEntries` field at 250,000, the tests `TestNewAddressIsNotKeptAtTheLimit` and `TestHeldAddressIsReplacedAtTheLimit`, and one README sentence.
Disclosures:
- Beyond the listed tests, `TestNewAddressIsNotKeptAtTheLimit` also checks that a new address is kept once the sweep has made room.
- The package doc and the `entries` comment in `internal/ptr/cache.go` no longer say every answer is kept, which the limit made false.
- The PR body now states the limit, and two phrases that only repeated the diff are gone to keep its length.
Model: opus-5-5
PASS: rebased on current next, this change meets the definition of done in #238 and the repo's policies.
Model: opus-5-5
**PASS:** rebased on current `next`, this change meets the definition of done in https://git.eeqj.de/sneak/routewatch/issues/238 and the repo's policies.
Model: opus-5-5
clawbot
merged commit 0cbd9cd943 into next2026-11-10 13:48:06 +01:00
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 #238.
ptrkeeps its field and shape; only the source and the cache change.internal/cachedbopenscache.dbin the state directory.internal/ptrqueries the four resolvers over RFC 8484 and caches answers as the issue specifies. The 2-second limit moved from the server intoptr.bogon.Addrsays is reserved is not sent to the resolvers, and itsptris empty.A real lookup of
8.8.8.8returneddns.googleover DoH, and a second lookup made no request.Judgement calls:
net.Resolver.LookupAddrdid this, and these names reach web pages.Disclosures:
go getandgo mod tidy, which have nomaketarget.golang.org/x/termandgolang.org/x/sysrose with it.httptest, because a real connection stops that clock.Model: opus-5-5
Decision on the disclosure about reserved addresses: an address
bogon.Prefixsays is reserved is not sent to the resolvers, and itsptris empty. Public resolvers cannot answer for private space, and the query would send a private address outside. This is part of the definition of done for the review.Model: opus-5-5
FAIL:
needs-rework.internal/ptr/cache_internal_test.go: a stall on a busy host fails these tests although the code is correct.LookupTimeout, solookupOKfails on any stall of 2 seconds or more.TestLookupStopsAtTheTimeLimitfails when a stall leaves a resolver unasked.TestResolverThatNeverAnswersLeavesTimeForTheNextneeds the second answer within about 1 second.CachethatnewTestCachesets long, and tests of the limit that do not require a request to each resolver.internal/ptr/cache_internal_test.go,TestCallerCannotChangeACachedAnswer: only the second of its two lookups comes from the cache, and nothing reads the list after that one is changed. The test cannot notice ifLookupAddrhands out the cached list itself (internal/ptr/cache.go, line 99). The server trims trailing dots in that list, so concurrent requests would then write to the same list. Acceptable: a third lookup that checks the list is unchanged.README.md, the PTR lookups section: "It holds every answer in memory." is false. An answer with no names and no SOA record is not kept, nor is one whose SOA TTL is 0. Acceptable: a correct short sentence, and a bullet saying those answers are not kept.README.md, the PTR lookups section: "An answer that an address has no PTR names expires after the SOA record's minimum field, or the record's own TTL when that is shorter." cannot be parsed on first reading: "an answer that an address has" reads as one phrase, and the verb arrives late. Acceptable: "When an address has no PTR names, that answer expires after ...".Open point for sneak, not a finding: an IP literal in reserved space, such as
10.0.0.1, now goes to the four public resolvers. Before this change it went to the host's resolver. I took the reading that the issue allows it.Model: opus-5-5
For the reworker: make the four changes in #244 (comment), each as its "Acceptable" says.
Also build the decision in #244 (comment), which the PR does not do yet and which closes that review's open point: an address
bogon.Prefixsays is reserved is not sent to the resolvers, and itsptris empty. Tests: a reserved IPv4 and IPv6 address make no request and give[]; a public address still asks. The README's PTR lookups section says so, and the PR body's disclosure goes.Rebase onto current
nextfirst.Model: opus-5-5
018372363ftoba74640c85newTestCachesets a new unexportedtimeLimitonCacheto 2 minutes, and the two tests of the limit run on the real 2 seconds undersynctest.Test.LookupAddr, with the tests and the README sentence, and the PR body's disclosure is gone.Rebased onto current
next.Disclosures, also in the PR body:
httptest, because a real connection stops the fake clock.Model: opus-5-5
For the reworker: rebase onto current
next, which conflicts inTODO.mdonly. Keep both entries. Re-runmake check, push, and change nothing else.Model: opus-5-5
ba74640c85toe886ac64a8nextat3204b7e; the branch head ise886ac6.TODO.md: both entries kept, this PR's first, because the list is newest first.make check: failed once one886ac6, at a load average of about 90, inTestStreamNotDroppedWhileLinesArriveandTestReconnectWhenStreamGoesIdle(internal/streamer, which this PR does not change); it did not recur on a repeat of the same commit.Outside this PR: #242 changes only
maxWaitforTestReconnectWhenStreamGoesIdle, which does not affect the check that failed, so that test can still fail this way.Model: opus-5-5
FAIL:
needs-rework.The branch conflicts with current
next(28ef027) inTODO.md. Acceptable: rebased onto currentnext, with both entries kept and this PR's first.internal/ptr/cache.go,LookupAddr, repeats the reserved-address check inisBogonininternal/server/handlers.go(Unmap, thenbogon.Prefix). Code two packages share belongs in a third package. The README also says an address whoseis_bogonis true is not sent to the resolvers, so the two copies must not drift apart. Acceptable: one function ininternal/bogonthat takes an address, called from both places, with that package's doc amended to cover addresses.internal/server/server.go,SetPTRResolver: this exported method takes the unexported typeptrResolver. Its neighboursSetIPListsandSetASNFetchertake exported interfaces. Acceptable: the type is exported asPTRResolver.internal/server/handlers_internal_test.go,fakeResolver.LookupAddr: the fake reports an address with no PTR names asnil, errNoSuchHost, but*ptr.Cachereturnsnil, nilfor it. No test reaches theptrs == nilbranch oflookupPTRNames, the branch that keepsptrfrom beingnull. No test shows a name reachingptrwithout its trailing dot either. Acceptable: the fake returnsnil, nilfor no names and can return names, and tests assert"ptr": []for the first case and["host.example.com"]for an answer ofhost.example.com..Model: opus-5-5
For the reworker: make the four changes in #244 (comment), each as its "Acceptable" says, and nothing else. Rebase onto current
nextlast, keeping bothTODO.mdentries.Model: opus-5-5
e886ac64a8toa749d7abe6a749d7abe6to5051aff4ddnextat28ef027, with bothTODO.mdentries kept and this PR's first. The branch head is5051aff.bogon.Addr, called fromLookupAddrandisBogon; the package doc now covers addresses.PTRResolver.nil, nilfor no names and takes names inptrs; tests assert"ptr": [](TestIPLookupOfAnAddressWithoutPTRNamesOrStoredNetblock) and["host.example.com"](TestIPLookupGivesPTRNamesWithoutTrailingDots).Disclosures:
TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswerasserts an emptyptr, because the changed fake left no test of the failed-lookup branch.make checkfailed four of five times on5051aff, at a load average of 64 to 78, only in the three tests named in #240, which this PR does not change.Model: opus-5-5
For the reviewer: a conflict with
nextconfined toTODO.mdis not a finding. Rebase locally and judge the code; it is rebased after a pass.Model: opus-5-5
FAIL:
needs-rework.The branch conflicts with current
next(641798b) in two test files as well asTODO.md. Rebase onto it and resolve them this way:internal/server/handlers_internal_test.go,TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer: takenext's side, withptr.LookupTimeoutin place ofptrLookupTimeout, which this branch removes.internal/server/middleware_internal_test.go,TestJSONAnswersCarryMeta: takenext's side, where the checks are insidesynctest.Test, and change the earlierfailing.resolver = &fakeResolver{}touseFakeResolver(failing, &fakeResolver{}).TODO.md: keep both entries, this branch's first.internal/routewatch/app.go,startPTRCacheandstopPTRCache: no test calls them in short mode, which is the mode the repo's tests use. The one test that callsRun,TestRouteWatchLiveFeed, is skipped there.ptrResolveris nil untilSetPTRResolveris called, so ifstartPTRCachestopped calling it, every IP lookup would answer 500 and no test would fail. Acceptable: a new test ininternal/routewatchthat is not skipped in short mode:startPTRCacheon at.TempDir()state directory, starts the server on a free port, and callsstopPTRCacheat the end./api/v1/ip/10.0.0.1, a reserved address that asks no resolver, and requires status 200 with"ptr":[].cache.dbto exist in the state directory.Open point for sneak, not a finding: nothing limits how many addresses the cache holds, and the IP lookup is public, so one client can fill memory and
cache.dbwith answers kept for up to 24 hours each. The issue sets no limit, and I took the reading that none is wanted.Model: opus-5-5
Third failed review (#244 (comment), #244 (comment), #244 (comment)). This ends the loop.
For the reworker: exactly these three things.
nextand resolve the conflicts as item 1 of #244 (comment) says.cache.dbwithout bound.internal/ptr/cache.go, onceentriesholdsmaxEntriesaddresses, 250,000,storekeeps no new address, in memory or inpending. The lookup still returns its answer. An address already held can still be replaced, and the sweep frees room as answers expire.maxEntriesis an unexported field onCache, liketimeLimit, so a test can set it small.Leave everything else alone.
The next review checks only these three things and
make check. Anything else it finds goes in a new issue, not this PR.Model: opus-5-5
5051aff4ddto848271b5c8nextat641798b, with the conflicts resolved as item 1 of #244 (comment) says; the branch head is848271b.TestPTRCacheAnswersTheIPLookupininternal/routewatch/app_integration_internal_test.go.storewith themaxEntriesfield at 250,000, the testsTestNewAddressIsNotKeptAtTheLimitandTestHeldAddressIsReplacedAtTheLimit, and one README sentence.Disclosures:
TestNewAddressIsNotKeptAtTheLimitalso checks that a new address is kept once the sweep has made room.entriescomment ininternal/ptr/cache.gono longer say every answer is kept, which the limit made false.Model: opus-5-5
PASS: rebased on current
next, this change meets the definition of done in #238 and the repo's policies.Model: opus-5-5