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

fix data races in cache, resolver, qlog; add cache/zonefile/blocklist tests; update STATUS

Browse Source 
- cache.Entry.Hits and LRU.perShard become atomics; SetCapacity now evicts
  down to the new budget; packKey fully qualifies names before packing
- resolver.Pool rewritten copy-on-write (atomic.Pointer to an immutable
  state); Upstream is immutable with shared *upstreamStats, so health
  survives a Rebind and the query path stops racing the config hook
- Pool.Exchange no longer marks a dead upstream healthy: Success was
  called before the error check, so the breaker never tripped
- qlog live ring publishes through atomic pointers
- blocklist.Holder.Get nil-safe; Builder lists each source once
- web: fix toast(string) misuse that rendered empty mutation toasts
- add unit tests for cache (with benchmarks) and zonefile
- STATUS.md: green -race integration suite, remaining failures and the
  broken bbolt snapshot round trip documented
This commit is contained in:
developer committed 2026-09-27 12:59:02 +00:00
1 parent 10e8a9b91a
commit aa12b03eb9
7 files changed
+228 -144
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

STATUS.md
+147 -115
View File
@@ -1,8 +1,8 @@
# fancy-dns — development status and continuation plan
Last updated: session checkpoint. Everything below reflects the state of the
working tree at the time of writing, including known-failing tests and the
exact next command to run.
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
@@ -47,8 +47,8 @@ internal/
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 (subagent-built)
deploy/ example config, example zones
web/ Vite + React + TS + Tailwind console
deploy/ example config, example zones, k8s manifests
```
### Query pipeline order (engine.Handle)
@@ -65,133 +65,155 @@ Cache is a single shard-local lookup; only a miss touches the network.
### Done and verified
- All packages compile; `go vet ./...` clean.
- `go test ./internal/app/` **passes** (integration suite, ~20 tests) with a fake
upstream on loopback: resolve/caching, all blocklist shapes, allow-over-block,
local zones (exact/wildcard/NODATA/NXDOMAIN), ACL deny + allow_domains +
skip_blocklist, all four block actions, malformed query FORMERR, TCP parity,
API auth (401/403/200), domain add→blocked→delete round trip, ACL update,
upstream update, settings validate/save + rejected-write-leaves-file-alone,
blocklist toggle both directions, dashboard/status shape, logs, cache
endpoints, reload failure keeps serving + success clears error, SSE stream
emits `ready` and `entry`, metrics protected/unprotected, concurrency smoke.
- Verified live end-to-end against the real binary on :15353/:18080 with dig.
- Subagent-verified working examples of `/api/domains/check` for hosts-file,
RPZ-wildcard and inline rules.
- `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.
### Known failures (fix these first)
### Fixed this session (races and their root causes)
1. **`go test -race ./internal/app/` still reports DATA RACE and two failures.**
- `TestDomainAddDeleteRoundTrip` and `TestUpstreamUpdate` fail **only under
`-race`** (they pass without it), which points at timing, not logic.
- I had just started fixing races by moving `engine.sem`, `engine.semSize`
and `engine.upstreamTimeout` behind atomics. The `applyConfig` hook now
writes `e.sem.Store(&sem)`; `Handle` does `sem := e.sem.Load()`. **This
refactor is mid-flight** — re-read `internal/engine/engine.go` before
touching it. Remaining races are almost certainly in
`config.Config` field access from `applyConfig` (e.g. `a.pool.Rebind`
called from the swap hook while queries read `cfg`), and in
`harness_test.go` reading the config file the server rewrites (partially
fixed by snapshotting `doc` before `Start` — verify).
- `internal/app/dbg_test.go` was deleted; do not reintroduce it.
2. **Frontend `web/` is incomplete.** The foundation/pages subagents each ran
out of budget. `src/lib/types.ts`, `format.ts`, `cn.ts`, `vite.config.ts`
exist; `src/index.css` is still the purple Vite template; `src/lib/api.ts`,
`src/hooks/*`, `src/components/ui/*`, `src/components/layout/*` and all of
`src/pages/*` are missing. `npm run build` and `npx tsc -b --noEmit` have
never passed. `internal/web/dist/index.html` is a placeholder so the binary
compiles.
3. **No unit tests outside `internal/app/`.** `blocklist`, `cache`, `zonefile`,
`acl`, `resolver`, `localzone` have no direct tests.
4. **Missing deliverables:** `Makefile`, `.gitignore`, `Dockerfile`, k8s
manifests, CI workflow, `README.md`.
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
- `Makefile`, `.gitignore`, `Dockerfile`, `deploy/k8s/`,
`.github/workflows/ci.yml` and a real `README.md` (it is still a 0-byte
file) were never written.
- End-to-end verification of the freshly built console against the running
binary: the bundle compiles and embeds, but nobody has clicked through it.
- `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 — green tests (highest priority)
### Step 1 — the bbolt round trip (highest priority)
```bash
go build ./... && go vet ./...
go test -race ./internal/app/ -count=1 2>&1 | grep -A20 "DATA RACE" | head -60
go test -race ./internal/cache/ -run 'TestStore' -count=1 -v
```
Expected races to fix, in likelihood order:
- `config.Store` swap hook running `app.onConfigSwap` → `pool.Rebind` and
`reloadLocalZones` while the query path reads the old `*config.Config`.
Fix direction: make `Pool.Rebind` build a new slice and swap an
`atomic.Pointer` rather than mutating in place under `sync.RWMutex` while
readers hold `RLock` on a *different* field.
- `blocklist.Manager` `m.running` map vs `RefreshAll` goroutine + API handler.
Already mutex-guarded; confirm it is the *Stats* pointer that races.
- `qlog.Logger.live` ring: `Log` writes `l.live[pos%len]` from many goroutines
while `Recent` reads. Needs a seqlock or a per-slot atomic pointer.
- Harness reading the config file path from a test goroutine.
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.
For each race, prefer removing shared mutable state over adding locks: the
design already uses `atomic.Pointer` holders for ACL, blocklist, localzone and
pool; extend that pattern.
### Step 2 — the five blocklist gaps
### Step 2 — unit tests for the hot packages
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.
Priority order (highest bug-density first):
- `blocklist`: `Set.Match` for `*.x` (subdomains only) vs `**.x` (apex too)
vs exact; allow-over-block priority; hosts/adblock/plain/rpz parser cases;
`DetectFormat` sniffing; duplicate handling.
- `cache`: LRU eviction, TTL clamping, `TTLFor` on NXDOMAIN/NODATA/SERVFAIL,
serve-stale window, key pack/unpack round-trip, bbolt snapshot→load.
- `zonefile`: `$TTL` inheritance, omitted owner/TTL/class, parens, `@`,
`$ORIGIN` relative vs absolute names, `ParseTTL`/`SerialNext`.
- `acl`: first-match-wins, most-specific CIDR, PTR suffix, `DomainSet.Match`.
- `localzone`: delegation cut, closest-wildcard (RFC 1034 §4.3.3), NODATA.
- `resolver`: breaker open/close, parallel race, TCP fallback on truncation.
- Add `BenchmarkLookup` for `Set.Match` and `cache.LRU.Get` to substantiate
the "great performance" claim.
### Step 3 — the remaining unit tests
### Step 3 — finish the frontend
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).
Respawn two subagents with disjoint file ownership (this worked structurally;
they just need more budget):
### Step 4 — the frontend and deploy deliverables
- **Foundation agent** owns `src/index.css` (replace the purple template with
dark-first CSS vars + one emerald accent), `src/lib/api.ts`, `src/hooks/*`,
`src/components/ui/*`, `src/components/layout/*`. The exact prop contract I
gave them is in the transcript and must be kept.
- **Pages agent** owns `src/pages/*` only, coding against the same contract.
Then wire `src/App.tsx` + `src/main.tsx` yourself (react-router, `AppShell`,
`ToastProvider`, theme init) and run `npm run build`. Once `web/dist` is real,
the `//go:embed all:dist` in `internal/web/web.go` picks it up with no Go
change; delete the placeholder `internal/web/dist/index.html`.
API contract for the frontend is fixed and implemented — see
`internal/api/handlers.go` route table and `internal/app/hooks.go` payload
shapes. `/api/logs/stream` is SSE with events `ready`, `entry`, `stats`, and
`X-Accel-Buffering: no`.
### Step 4 — deploy + docs
- `Makefile`: `build`, `test`, `race`, `vet`, `web`, `lint`, `run`, `image`.
- `Dockerfile`: multi-stage — node build for `web/`, then Go build with
`-ldflags "-X ...app.Version=..."`; distroless/scratch; `EXPOSE 53/udp 53/tcp
853/tcp 443/tcp 8080/tcp`; non-root; `VOLUME /var/lib/fancy-dns`.
- k8s: Deployment (hostNetwork or a NodePort Service for :53), Service,
ConfigMap for the YAML, PVC for the bbolt files, readiness probe on
`/api/healthz`, optional Ingress for the console.
- CI: `go vet`, `go test -race`, `gofmt -l`, `npm ci && tsc -b && npm run build`.
- `README.md`: quickstart, config reference, zone-file semantics, API
reference, performance notes, the security caveats that actually matter
(`trusted_proxies` and `X-Forwarded-For`; `metrics.protect`; the
`trust_loopback` + `auth.mode` matrix).
Finish the console (pages exist; verify them in a browser against a running
binary), then write `Makefile`, `Dockerfile`, `deploy/k8s/`, CI and the
`README.md`. 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.
- **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, and a new allow rule can rescue a name many
lists blocked. `invalidateBlocklistCache()` in `internal/app/hooks.go`.
- **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
@@ -199,9 +221,19 @@ shapes. `/api/logs/stream` is SSE with events `ready`, `entry`, `stats`, and
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 (this was a real bug caught by `TestBlocklistToggle`).
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.