mats/fancy-dns
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Rewrite STATUS.md to match the tree after the bbolt/blocklist/acl/resolver fixes
ci / Console typecheck and build (push) Successful in 14s
Details
ci / Go build, vet and test (push) Failing after 15s
Details
ci / Binary with the console embedded (push) Skipped
Details
ci / Container image (push) Skipped
Details

Browse Source 
Records the four bugs the first end-to-end run surfaced, what is green, what
is still without a suite, and what is in flight, so the next session does not
start from the previous plan.
This commit is contained in:
developer committed 2026-09-27 13:32:17 +00:00
1 parent 26134ba437
commit 2b84474228
1 file changed
+128 -156
Download Patch File Download Diff File Expand all files Collapse all files
STATUS.md
+128 -156
View File
@@ -1,8 +1,7 @@
# fancy-dns — development status and continuation plan
Last updated: end of the race-fix / test-coverage session. Everything below
reflects the working tree, including the known-failing tests and the exact
commands to reproduce them.
Last updated after the bbolt/blocklist/acl/resolver fix session and the first
end-to-end run of the console. Everything below reflects the working tree.
## Goal
@@ -26,13 +25,15 @@ A lightweight, high-performance recursive DNS server with:
| Cluster sync | Valkey (Redis protocol) | simple, mature Go client, easy local dev |
| Secure DNS | DoT + DoH, no DNSSEC validation | avoids signer/anchor machinery; pass-through only |
| Hot cache | Sharded LRU in memory, bbolt for snapshots | no I/O on the hot path; warm restart |
| k8s networking | No hostNetwork, LoadBalancer Service | the server never sets SO_REUSEPORT, so two host-network pods cannot share :53 |
## Package layout
```
cmd/fancy-dns/main.go entrypoint: flags, signals, SIGHUP reload
internal/
config/ Config struct, Duration type, Validate, atomic Store + hooks
config/ Config struct, Duration type, Validate, atomic Store + hooks,
${VAR} environment expansion in Load
zonefile/ RFC 1035 master-file parser ($TTL/$ORIGIN, parens, serial math)
blocklist/ Set (exact/sub/root maps), 4 parsers, Manager (fetch+compile+swap)
cache/ Key, Entry, sharded LRU, TTL policy, serve-stale, bbolt Store
@@ -65,156 +66,125 @@ Cache is a single shard-local lookup; only a miss touches the network.
### Done and verified
- `go build ./...`, `go vet ./...` and `gofmt -l` are all clean.
- **`go test -race ./internal/app/` is green — zero data races.** The two
`-race`-only failures and all ten reported races are fixed (see below).
- `go test ./internal/zonefile/` is green (the full parser suite).
- Frontend **builds and type-checks**: `npx tsc -b --noEmit` and `npm run build`
both exit 0, `npx oxlint` reports 0 errors / 10 warnings, and the real bundle
lands in `internal/web/dist/`. The `//go:embed all:dist` picks it up with no
Go change.
- One real frontend bug fixed along the way: four pages used a `toast(string)`
duck-typed shim while `useToast()` takes `ToastOptions`, so every mutation
toast rendered an empty title.
- `gofmt -l ./cmd ./internal` empty, `go build ./...` clean, `go vet ./...` clean.
- **`go test -race ./internal/... -count=1` is green** for `acl`, `app`,
`blocklist`, `cache`, `config`, `resolver`, `zonefile`.
- Test suites exist and are green for: `acl` (26 tests + 4 benchmarks),
`blocklist`, `cache` (incl. benchmarks), `config`, `resolver`, `zonefile`.
- `internal/localzone`, `internal/engine` and `internal/qlog` are being written
right now — see "in flight" below.
- `README.md` is being written from the source right now.
### Fixed this session (races and their root causes)
### The four bugs from the first end-to-end run
All of these were found by `-race` and fixed at the source, not with extra
locking:
Found by running the binary and querying it, not by a test. All four are being
fixed in `internal/zonefile` + `internal/localzone`:
1. **`cache.Entry.Hits` → `atomic.Uint32`.** A counter on an entry shared by
every reader raced the LRU walk used by the API. The struct is now never
copied by value.
2. **`LRU.perShard` → `atomic.Int64`.** `SetCapacity` (a config reload, from the
API goroutine) rewrote the per-shard budget while every query shard was
inserting.
3. **`LRU.SetCapacity` now evicts.** Shrinking the budget changed the number but
left the old entries resident until they happened to be overwritten; a
reload that lowers `cache.size` could not actually reclaim memory. It now
trims each shard to the new budget and counts the evictions.
4. **`resolver.Pool` rewritten as copy-on-write.** This was the bulk of the
races: `Rebind` mutated `p.ups` / `p.cfg` under a mutex that the query path
never took, so `Exchange`, `Healthy`, `All` and the `dns.Client` memo map
were all racing. The pool now holds an immutable `poolState` behind
`atomic.Pointer`; `Rebind` builds the next state and swaps it with one
store. `Upstream` is immutable and holds a shared `*upstreamStats`, so the
live health, latency and breaker state of an unchanged server survives a
reload while a query in flight keeps a consistent view.
5. **`resolver.Pool.Exchange` no longer marks dead upstreams healthy.** A
pre-existing bug: `exchangeOne` called `up.Success(rtt)` before checking the
error, so a dead upstream was marked up, had its consecutive-failure count
reset and had `LastError` cleared on every failed exchange — **the circuit
breaker could never trip through the query path.**
6. **`qlog` live ring → `[]atomic.Pointer[liveEntry]`.** `Log` wrote
`l.live[pos%len]` from every query goroutine while `Recent` walked the ring
under a read lock the writers never took. Publishing is now one atomic word.
7. **`blocklist.Holder.Get` is nil-safe**, and `Builder` keeps an `ordered` set
so a source appears once in `Sources()` whether it was added by a rule or by
`NoteSource` (it used to be appended twice).
8. **`cache.packKey` fully qualifies the name before packing.** Keys are stored
canonically (lowercase, no trailing dot) but the wire format needs a
trailing dot, so `dns.PackDomainName` rejected every key.
1. **`$TTL 60` is ignored.** Every record came back with the SOA minimum
(3600). `Zone.DefaultTTL` is documented as the `$TTL` value; RFC 1035 §5.2
says an explicit `$TTL` wins.
2. **Records are swallowed when the previous record has an odd token count.**
`ns1 A 192.0.2.53` followed by `txt TXT "hello world"` parses as one record
`ns1 TXT "TXT" "hello world"` — the TXT record is lost entirely. Data loss
in `assemble`/`Parse`.
3. **A delegated name answers NXDOMAIN instead of a referral.** With
`sub NS ns1.sub.example.test.`, `host.sub.example.test. A` returned
`NXDOMAIN aa=true`; it must be NOERROR, AA false, NS in Authority.
4. **A wildcard answer keeps the literal wildcard owner.** A query for
`x.wild.example.test. A` returned an answer owned by
`*.wild.example.test.`. RFC 4592 §3.3.1 requires the owner name to be
replaced with the query name.
### Known failures (fix these next)
### Fixed earlier and worth not re-breaking
Run: `go test -race -timeout 120s ./internal/... -count=1`
- **`cache.Snapshot`/`Load` never round-tripped.** `Snapshot` packed the whole
key while `Load` decoded the record header out of the *key's* remainder
instead of the value, so all 25 entries were skipped — every restart was a
cold cache. There is now an explicit `encodeRecord`/`decodeRecord` pair with
a version byte and a CRC-32C trailer, so a future format change is detected
rather than silently misparsed. `Load` also drops entries already past
`ExpiresAt` and rewrites the TTL to the time actually left, so a served TTL
never jumps back up across a restart.
- **ACL PTR suffix matched across label boundaries.** A group scoped to
`example.com.` also swallowed `ample.com.` and `notexample.com.` — a client
could land in a group it does not belong to. `matchPTR` now requires the
suffix to start a label, and `ptrSuffixes` normalises the config value to FQDN
form so the check works even for a config that skipped `Validate`.
- **The ACL `*` catch-all was dropped** from the rendered domain patterns, so
the engine's per-query `NewDomainSet` recompile lost the "everything" case.
- **`Upstream.Success` never closed an open breaker.** A recovered upstream
stayed excluded for the whole cooldown — the mirror image of the older bug
where a failed exchange marked a dead upstream healthy.
- **`Exchange` fanned out in parallel even under `policy: failover`.** It only
took the parallel path when exactly one upstream was chosen, so a failover
list of two raced both.
- **Blocklist:** a leading UTF-8 BOM is stripped, `!allow example.com` works,
`SourceMeta` exists for a source that only contributed a rule, `Pattern()`
reports the bare name again (`WireName()` produces the wildcard form for the
API), an RPZ record with no explicit TTL inherits the zone default, a hosts
line with no address in the first field says so, the adblock parser handles
CRLF and adblock lines pasted into a plain list, and over-long lines are
reported rather than accepted.
- **`TestHolderSwapIsAtomic` had the wrong invariant**, not the Holder: after a
swap to a set that does not block `a.example.com`, that name is legitimately
unblocked. The swap is a single `atomic.Pointer` store.
- **CI could not have passed.** `go.mod` requires Go 1.27.1; CI pinned 1.23 and
the Dockerfile pinned `golang:1.23-alpine`. Both now track the toolchain.
- **The Dockerfile clobbered the console.** It copied the source tree *after*
the console bundle, and a fresh clone's `internal/web/dist` holds only
`.gitkeep`.
- **The k8s set could not have worked.** `hostNetwork: true` with
`replicas: 2`, and the server never sets SO_REUSEPORT, so the second pod
could not bind :53. It now runs without hostNetwork behind a LoadBalancer
Service, with a separate ClusterIP Service for the Ingress (a headless
Service cannot back one), a ServiceAccount, an API-key Secret, per-pod PVCs,
and a kustomization. The HPA is gone — it cannot drive a StatefulSet.
- **The API key could not come from a Secret**, because the config had no
variable expansion. `config.Load` now expands `${VAR}`, `${VAR:-default}` and
`${NAME-default}` and fails to start on an unset variable with no default, so
a missing secret is a crash loop rather than an unauthenticated console.
Expansion skips YAML comments, so documenting `${VAR}` in the ConfigMap does
not break the file.
1. **bbolt snapshot round trip is broken — the warm-restart path does not work.**
`internal/cache/store.go` `Snapshot` writes 25 entries and `Load` skips all
25 (`TestStoreSnapshotAndLoad`, `TestStoreLoadSkipsExpiredEntries`,
`TestStoreTruncateAndStats` show `loaded = 0, skipped = N`). The write side
and the read side disagree about the record layout — most likely the
`binary.Uvarint` header that `Load` decodes (`expUnix`, `staleUnix`) is not
what `Snapshot` emits. **This is the highest-value remaining bug**: without
it every restart is a cold cache. Start at `Snapshot` (around the `put`
call) and `Load` (`internal/cache/store.go:78`).
2. **`internal/blocklist` — 5 failing tests, all real implementation gaps**, not
test bugs. Do not "fix" them by relaxing the assertions; they are the spec:
- `TestSetIntrospection`: `SourceMeta("b")` is missing for a source that was
only ever mentioned by a rule, and `Pattern()` reports the internal `*.` /
`**.` form where the caller wrote a bare domain.
- `TestHolderSwapIsAtomic`: reports a half-applied set. The swap is a single
`atomic.Pointer` store, so either the test's invariant is wrong or
`Holder` publishes before the set is fully built — check which before
changing anything.
- `TestParseHosts`: a hosts line whose first field is not an address should
emit the notice "no address in first field, falling back to a plain
domain list"; it silently produces nothing.
- `TestParseRPZ`: bare RPZ owner names are sniffed as `FormatPlain` (the
format sniffer only knows the classes `IN`/`CH`/`HS`), and the
no-subdomain form is compiled as a wildcard.
- `TestParseRobustness`: CRLF input to the adblock parser produces no rules
at all, and an over-long line is accepted instead of rejected.
- Also unreported by a test but found by inspection: no parser strips a
leading UTF-8 BOM, and the documented inline allow marker
`!allow example.com` does not work (only `!example.com` does).
3. **No tests yet for `acl`, `localzone`, `resolver`, `config`, `qlog`,
`engine`, `server`, `api`, `metrics`, `cluster`.** `internal/acl` and
`internal/localzone` were started but the agent ran out of budget;
`internal/resolver` was not started at all.
### Open decision
### Not started / incomplete deliverables
- **Unknown config keys are silently ignored.** Writing `block:` where the
struct wants `blocklist:` produced a plausible-looking server instead of an
error. `yaml.Decoder.KnownFields(true)` would reject it, but that is a
behaviour change for anyone carrying extra keys in their config. Not done
unilaterally.
- **`README.md` is still a 0-byte file.** Everything else in the deploy set
exists (written in a parallel pass) and is tracked: `Makefile` (4 KB),
`Dockerfile`, `.gitignore`, `.github/workflows/ci.yml` (6 KB),
`deploy/config.example.yaml`, `deploy/local.example.zone`, `deploy/zones/`.
None of it has been *reviewed against the code* — the CI workflow and the
Makefile targets (`make web`, `make race`, `make image`) need a
`make -n`/dry-run pass, and every config key, port and CLI flag the docs
mention has to be checked against `internal/config` and `cmd/fancy-dns`.
- **`deploy/k8s/` has only two of the seven expected manifests**
(`00-config.yaml`, `10-deployment.yaml`). Still missing: Service, PVC,
ServiceAccount + Secret (the API key), Ingress (needs
`proxy_buffering off` for the SSE log stream), namespace and kustomization.
- End-to-end verification of the console against a running binary: the bundle
compiles and embeds, but nobody has clicked through the pages.
- `internal/web/dist` is gitignored except `.gitkeep`, so a fresh clone cannot
build the binary without running `npm ci && npm run build` first. Either CI
builds the bundle before `go build`, or the ignore rule comes off.
### No tests yet for
`server`, `api`, `metrics`, `cluster`. `app` has integration-level tests that
pass; the rest have no suite at all.
### In flight (subagents — do not edit these files)
- `internal/zonefile` + `internal/localzone` — repair the truncated
`localzone_test.go` and fix the four bugs above.
- `internal/engine` + `internal/qlog` — first suites for the query pipeline and
the query log.
- `README.md` — written from the source, with a "known inconsistencies" list.
Still to delegate after those: `server` + `api`, then `metrics` + `cluster`.
## Continuation plan, in order
### Step 1 — the bbolt round trip (highest priority)
```bash
go test -race ./internal/cache/ -run 'TestStore' -count=1 -v
```
Read `Snapshot` and `Load` in `internal/cache/store.go` as a pair and make the
record format explicit — a small `encodeRecord`/`decodeRecord` pair with one
version byte, so a future change cannot silently desync them the way this one
did. Then add a `t.TempDir()` round-trip test that survives a process restart
and a partially written file.
### Step 2 — the five blocklist gaps
Fix the implementation against the existing assertions (see item 2 above), then
`go test -race ./internal/blocklist/ -count=1` to green. The BOM strip and the
`!allow` marker are the two an operator is most likely to hit.
### Step 3 — the remaining unit tests
Same checklist as before, minus what is now covered (`blocklist`, `cache`,
`zonefile` have suites; `cache` and `blocklist` have benchmarks). Priority:
`resolver` (breaker open/close, parallel race, TCP fallback on truncation,
failover, `Rebind` under `-race` — with a `dns.Server` fake on `127.0.0.1:0`),
`acl` (first-match-wins, most-specific CIDR, PTR suffix, `DomainSet` wildcard
forms), `localzone` (delegation cut, closest-wildcard per RFC 1034 §4.3.3,
NODATA vs NXDOMAIN), `config` (`Duration` parsing, `Validate`, the store hooks).
### Step 4 — review the deploy set and write the README
The console pages exist and build; verify them in a browser against a running
binary. Then read `Makefile`, `Dockerfile` and `.github/workflows/ci.yml`
against the code and fix what does not match, add the missing k8s manifests
(Service, PVC, ServiceAccount + Secret, Ingress with `proxy_buffering off` for
`/api/logs/stream`, namespace, kustomization), and write `README.md` from the
source: config reference, zone-file semantics, blocklist formats, API routes,
the `trusted_proxies` / `metrics.protect` / `auth.mode` security caveats, and
the limitations (no DNSSEC validation, no zone transfer, no dynamic update).
CI must run `npm ci && npm run build` before `go build`, because the Go build
embeds `internal/web/dist`.
1. Land the three in-flight pieces, then re-run
`go test -race ./internal/... -count=1 -timeout 300s`.
2. `internal/server` suite: all four listeners behind one handler, per-listener
config, the UDP read-deadline reset (see the design notes), shutdown.
3. `internal/api` suite: every route, the three auth modes, the constant-time
API-key comparison, `X-Forwarded-For` honoured *only* from `trusted_proxies`,
CORS, rate limiting, and the SSE stream (open a subscriber, assert
delivery, unsubscribe, assert no goroutine leak).
4. `internal/metrics` + `internal/cluster`: the collectors, the rolling rings,
the Valkey lease, pub/sub sync, and divergence detection (a `miniredis`
instance is the obvious test server).
5. Decide on `KnownFields(true)`.
6. Re-run the console end to end (`/api/healthz`, `/api/whoami`, a page from
each route) — it was verified once and the API surface has changed since.
## Design notes worth preserving
@@ -222,33 +192,35 @@ embeds `internal/web/dist`.
and yields a private copy, so serving never mutates shared state and TTL
ageing is free. Same trade dnsmasq makes.
- **Everything the query path reads is behind an `atomic.Pointer` holder** —
ACL, blocklist, localzone, and now the upstream pool. Readers load the
pointer once and work against a value that cannot change under them. This is
the pattern the whole race cleanup settled on: prefer a new immutable
snapshot over a lock.
ACL, blocklist, localzone, and now the upstream pool. Readers load the pointer
once and work against a value that cannot change under them. Prefer a new
immutable snapshot over a lock.
- **Cache invalidation on rule changes flushes the cache** rather than guessing
which subtrees are affected. A newly blocked name must not keep serving its
cached good answer. `invalidateBlocklistCache()` in `internal/app/hooks.go`.
which subtrees are affected. `invalidateBlocklistCache()` in
`internal/app/hooks.go`.
- **API responses share the field names in `internal/app/hooks.go`.** If you
rename one, grep the frontend too.
- **API keys are compared by SHA-256 hash, constant-time**, and
`X-Forwarded-For` is honoured *only* from `trusted_proxies` — otherwise a
client could spoof its identity past the ACL and the rate limiter.
- **UDP listener resets its read deadline every iteration.** A deadline set
once expires permanently and turns the loop into a spin that never reads
another packet (a real bug caught by `TestBlocklistToggle`).
`X-Forwarded-For` is honoured *only* from `trusted_proxies`.
- **UDP listener resets its read deadline every iteration.** A deadline set once
expires permanently and turns the loop into a spin that never reads another
packet (caught by `TestBlocklistToggle`).
- **Only the Valkey lease holder fetches lists**, so N instances do not hit a
mirror N times. Followers serve their compiled copy.
- **Blocked/serve-stale behaviour:** a served-stale answer gets TTL 1, an
expired upstream answer is never cached, and a truncated answer is never
cached (it would be served truncated forever).
- **`PatternKind.Pattern` is the display form; `WireName` is the wildcard
form.** The internal `*.` / `**.` prefixes must not leak into the UI, but the
API needs them to re-add a rule unchanged.
## Test-hygiene traps
- `tsc -b` replays **cached diagnostics** from `node_modules/.tmp/*.tsbuildinfo`.
After a large edit, `rm node_modules/.tmp/*.tsbuildinfo` or you will debug
errors that no longer exist.
After a large edit, `rm node_modules/.tmp/*.tsbuildinfo`.
- `vite build` empties `outDir` and **deletes `internal/web/dist/.gitkeep`**.
Restore it or the directory disappears from git.
- A test that closes a stop channel *after* `wg.Wait()` deadlocks the whole
package and looks like "the suite hangs". Close first, then wait.
- An agent that runs out of budget mid-append leaves a file that does not
parse. `gofmt -l` on the package is the fastest way to see it.