fancy-dns/STATUS.md

# 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.

## Goal

A lightweight, high-performance recursive DNS server with:

- **Go backend** — sharded LRU cache backed by bbolt, blocklist replication from
  multiple formats, per-client ACLs, query logging, Prometheus metrics.
- **Secure DNS** — DNS-over-TLS (RFC 7858) and DNS-over-HTTPS (RFC 8484).
  DNSSEC validation was explicitly scoped **out**; AD/CD/extended-RCODE are
  passed through only.
- **React + Tailwind admin console** embedded in the binary.
- **Config as zone files + YAML**, hot-reloadable, with **Valkey/Redis** for
  cross-instance sync (chosen over AMQP).

## Architecture decisions (settled with the user)

| Decision | Choice | Why |
|---|---|---|
| Scope | Full production | clustering, metrics, ACLs, query logs, hot reload, Docker, k8s, CI |
| Config store | YAML + DNS master files | reviewable/diffable in git |
| 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 |

## Package layout

```
cmd/fancy-dns/main.go          entrypoint: flags, signals, SIGHUP reload
internal/
  config/       Config struct, Duration type, Validate, atomic Store + hooks
  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
  resolver/     Upstream pool: health, circuit breakers, parallel/failover/random
  acl/          compiled groups, CIDR/PTR/tag matching, per-group domain policy
  localzone/    authoritative data from master files, wildcards, delegations
  engine/       THE QUERY PIPELINE (see below), reverse-PTR cache
  server/       UDP, TCP, DoT, DoH listeners; all funnel into one handler
  qlog/         bbolt query-log ring + live in-memory ring + SSE fanout
  metrics/      Prometheus collectors + rolling time-series rings
  cluster/      Valkey pub/sub, leader lease, divergence detection
  api/          admin REST + SSE, auth, CORS, rate limiting
  app/          wiring, lifecycle, API hook implementations
  web/          go:embed of the built console
web/            Vite + React + TS + Tailwind console
deploy/         example config, example zones, k8s manifests
```

### Query pipeline order (engine.Handle)

```
validate → ACL deny → per-client concurrency → global concurrency → rate limit
  → group allow/deny domains → local zone → blocklist → cache → upstream
  → cache put → finalize (EDNS0, UDP truncation) → metrics + qlog
```

Cache is a single shard-local lookup; only a miss touches the network.

## Status

### 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.

### Fixed this session (races and their root causes)

All of these were found by `-race` and fixed at the source, not with extra
locking:

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.

### Known failures (fix these next)

Run: `go test -race -timeout 120s ./internal/... -count=1`

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.

### Not started / incomplete deliverables

- **`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.

## 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`.

## Design notes worth preserving

- **Cache stores packed wire bytes, not `*dns.Msg`.** A hit costs one `Unpack`
  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.
- **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`.
- **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`).
- **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).

## 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.
- `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.