diff --git a/TECH_DEBT.md b/TECH_DEBT.md new file mode 100644 index 0000000..549a9b4 --- /dev/null +++ b/TECH_DEBT.md @@ -0,0 +1,265 @@ +# nxdns Tech-Debt Audit — Final Report + +All findings below were adversarially verified against the code at HEAD (1ff727f). Raw finding count was 77; 6 were merged into other findings that describe the same underlying debt, leaving **71 distinct findings: 1 high, 34 medium, 36 low**. + +## Overview + +Dominant themes, in order of threat: + +1. **The verification pipeline itself is compromised.** CI watches a branch nobody pushes to, the main test binary aborts on every green run while printing `failed command`, a stale frontend bundle has already shipped a broken page once with no guard added since, the live-network suite never runs automatically, and the test aggregator is a hand-maintained list. This is the same silent-green failure class that shipped the 35f2324 fetcher bug. +2. **Copy-paste infrastructure with drift already underway.** The connection-server lifecycle (the trickiest concurrency code in the repo) exists in four hand-synced copies; the repo memory-safety choreography in ~18; the web CRUD shell in 7 handler files; Tailwind class constants in 12+ frontend files. Several copies have already diverged. +3. **Silent failures and swallowed errors.** Cancellation converted to bogus statuses, transport causes discarded before classification, an oversized cookie header producing an undiagnosable login loop, a paginated log view that silently develops row gaps. +4. **Operator-facing contract drift.** Config keys, docs, and the validator promise behavior the runtime does not deliver (per-query timeout that is per-attempt, hostname `tls://` upstreams that validate clean but can never connect, an upstream-management UI that was specced and never built). +5. **Observability debt.** Counters written and never exported, sibling listeners exporting the same metric name with different semantics, warn-level logging for routine peer behavior in violation of the module's own ruling. + +--- + +## Theme 1: The verification pipeline is compromised (11 findings) + +The repo's history proves this theme's stakes: commit 35f2324 fixed a process-killing bug that 1284 green hermetic tests hid. Several of the guards that should prevent a recurrence are absent or inert. + +- **[medium] .gitea/workflows/ci.yml:5 — CI triggers only on `main` while work happens on `master`.** + `branch.master.merge` confirms day-to-day pushes go to `origin/master`, which the workflow does not watch; local master is 3 commits ahead of both remote heads, so milestone 13+ has never run through CI. Every gate (tests, aarch64/qemu, frontend, size budgets, docker smoke) is currently inert for real work. The fix is already specified in specs/milestone-14.md §7 (make master canonical, switch ci.yml, delete origin/main); implement that spec rather than re-deciding. + +- **[medium] build.zig:48 — main test binary SIGABRTs at teardown; every green run prints `failed command`.** *(merged: build.zig:68, same defect from the output-signal angle)* + Reproduced at HEAD: `zig build test` exits 0 but the mbedTLS-linked binary aborts under `--listen=-` after all 1242 tests pass (exit 134 when run directly with `--listen`; clean in stdio mode, pointing at an interaction with zig's IPC runner rather than a production shutdown defect). Cost: the one string that should mean failure appears on every success, training humans, CI log scans, and agents to ignore failure text. Fix: root-cause the abort (debugger under `--listen`, bisect the mbedTLS threading macros); failing that, document the artifact in AGENTS.md/build.zig so nobody "fixes" it by loosening a check, and make any future non-zero test-binary exit fail the build. + +- **[medium] build.zig:24 — no freshness guard on `-Dweb-dist` despite a stale bundle having already shipped a crashing settings page.** + docs/explanation/performance-and-testing.md:159 admits the hole is still open; CI is safe only by incidental job ordering. Milestone-14 specs a hard failure for the adjacent placeholder-embed case but not this one. Fix: a build-time staleness check (hash of web/src + lockfile recorded at frontend build time, or refuse `web/dist` older than the newest file under web/src). + +- **[medium] src/tests.zig:3 — hand-maintained 115-entry test import list with no completeness guard.** + Zig collects tests only from the root module; a forgotten import means a file's tests silently never run (verified empirically: an unreferenced import drops its tests while the run stays green). The ~14 standalone integration/live/drift suites are reachable only through this list. The list is complete today. Fix: walk src/ in build.zig (or generate the aggregator via WriteFiles, as bench_stage already does) and fail when a file is unreachable from tests.zig. + +- **[medium] .gitea/workflows/live-tls.yml:7 — the only real-network tests run on manual dispatch only.** + The `-Dlive` suite (real public DoT/DoH resolvers) has no schedule; both failure modes it exists to catch have already happened once here (35f2324; a live DoT test caught real upstream cert drift in milestone 4). Non-blocking on push is correct; never running automatically is the gap. Fix: add a weekly `schedule:` cron — equally non-blocking, so it does not conflict with the milestone-1 ruling. + +- **[medium] src/filter/filter_integration_test.zig:414 — the multi-read HTTP body path through the fetcher is still never driven end to end.** + This is the exact seam where 35f2324 lived; the fix added unit tests that pre-buffer readers into the fatal state, but the loopback fixture still answers everything in one `request.respond` of a ~150-byte body. Fix: a fixture route that writes the body in several flushed chunks, or a body larger than the fixture's 8192-byte write buffer. + +- **[medium] src/dns/edns.zig:190 — stripEcs is the only attacker-facing packet-rewriting entry point absent from the fuzz targets.** + It runs on the raw client query on the default `ecs_mode = .strip` path and does the module's most delicate work (rebuild around OPT, patch RDLENGTH in place). The fuzz file's stated exclusion covers only ResponseBuilder. Verification found no latent bug in the patch arithmetic, so this stays a coverage gap. Fix: a fuzz target asserting output re-parses, contains no ECS option, and preserves everything else — the input derivation already exists in `parseTarget`. + +- **[medium] src/docs_drift_test.zig:51 — the CLI drift guard hardcodes the subcommand list instead of deriving it.** + The sibling guards derive their needles from the source of truth (routes.table, model.toSettings); this one keeps a parallel copy, so adding `nxdns stats` leaves the test green with the new command missing from docs — the exact drift it exists to catch. Fix: a public command table in cli.zig (or derive from `@typeInfo(cli.Command)`) that dispatcher, usage text, and the guard all iterate. + +- **[low] src/platform/logging.zig:610 — rotation failure paths are declared untested and cover the module's trickiest state machine.** + The exactly-one-sink-error contract spans four functions and is codified in the spec but trusted, not asserted; the only failure path with a test is a failed open. Fix: an injection seam on the deleteLocked/renameLocked wrappers so the pending-rotation recovery can be asserted. + +- **[low] tests/fuzz/blocklist_fuzz.zig:122 — fuzzing stops at the line parsers; the compiler's streaming takeDelimiter/StreamTooLong/discard loop is never fuzzed.** + The loop's own comment notes that omitting the discard loops forever, and its EndOfStream-during-discard branch has no test at all. Note: the finding's fix sketch is wrong — compiler.zig imports ../dns/, so a src/filter-rooted fuzz module fails with ImportOutsideModulePath; use a build.zig aggregator module (the bench_core.zig pattern). + +- **[low] src/web/http_util.zig:104 — the hand-rolled HTTP parsers (percent-decoder, path/cookie/query parsing, DoH base64url) are the third untrusted-byte family and the only one unfuzzed.** + The CI half of the original finding was refuted: corpus-replay-in-`zig build test` is the documented, sanctioned CI fuzz smoke, and `--fuzz=` is blocked by upstream Zig 0.16 defects on stock installs. What remains: add decodeInPlace/parsePath (and optionally dnsParam) targets in the existing harness pattern, running as corpus replay. + +## Theme 2: Duplicated infrastructure, drift already underway (11 findings) + +- **[high] src/server/dot_server.zig:3 — connection-server lifecycle machinery duplicated across four listeners.** *(merged: tcp_server.zig:218, dot_server.zig:180, dot_server.zig:452 — four findings describing the same debt)* + tcp_server, dot_server, doh_server, and web/server each carry a private copy of the same slot pool, claim/finish/beginShutdown protocol, accept loop with error mapping, cancel-protection dance, and select-based idle race. `race`/`expire`/`readPrefix`/`readBody`/`writeReply` are byte-identical between tcp and dot; dot_server's header says outright "This file mirrors tcp_server.zig". Drift is live: the counter is `accepted` on TCP vs `connections` on DoT/DoH; `decideClaim` is written two ways for identical semantics; doh_server ships a module-level `serve()` that nothing calls (dead divergent glue); the milestone-10 review log records a TLS-context leak fixed by hand-porting a pattern between copies — the predicted failure mode has already fired once. This is the most invariant-heavy concurrency code in the repo (mutex-ordered shutdown, cancel-protection windows), copied four times with no compiler help. The milestone-10 "mirror tcp_server's shape" ruling was a parallel-session build instruction, not an architectural decision against extraction. Fix: one shared listener core (comptime-parameterized slot pool + claim/shutdown + accept loop + race) with the per-connection serve function and TLS handshake stage as variation points; at minimum, extract race/expire/readPrefix/bump/Stop into src/server/ helpers. + +- **[medium] src/storage/repositories/groups_repo.zig:29 — list/free/errdefer scaffolding hand-rolled 18 times across seven repos.** + Every repo re-implements prepare → ArrayList → the load-bearing errdefer ordering → per-column columnTextAlloc → append, plus a matching freeX and allocation-failure test. Decisive detail from verification: the milestone-4 spec's own reference sample declares the two errdefers in the reverse (fatal, use-after-free) order — every implementation silently corrected it, and a future repo copied from the spec reproduces the UAF unless its author also copies the test. Fix: a shared comptime helper in crud.zig (`list(RowType, sql, readRow)` plus paired free) so the memory-safety choreography exists once. + +- **[medium] src/web/handlers/groups.zig:168 — CRUD shell duplicated across seven handler files; the 4-line configDb switch appears 40 times.** + list/get/remove handlers and the applyCreate/Update/Delete shells are identical modulo repo function and message string, and the copies have already diverged in lock scope and reload flavor (local.zig holds the lock through publish; blocklists adds pruneFiles; groups alone reads back under the lock). Fix: a comptime resource descriptor (repo fns + conflict message + reload flavor) generating the shells, keeping the genuinely different per-resource decision functions hand-written. Note the four distinct reload flavors make this less mechanical than the finding implies. + +- **[medium] web/src/features/local/RecordsTab.tsx:25 — RecordsTab/ZonesTab are structural copy-paste, and Tailwind class constants are re-declared across 12 files.** *(merged: RecordsTab.tsx:14, the class-constant half of the same debt)* + The two tabs share identical FormState/openForm/onSubmit/onDelete plumbing, mutation trio, and byte-identical class constants; the focus-visible literal appears in 21 files, and drift has already shipped: PrefixesEditor and GroupsPage inputs omit the focus-visible outline entirely, silently violating the milestone-9 accessibility floor. The project's own precedent (InlineError was hoisted and deduplicated during milestone 9) says this is house style left unapplied. Fix: shared form-shell/table-shell components and a ui/ directory (or one classes.ts module). + +- **[low] src/local/forward_client.zig:260 — race-against-deadline and stream-error-unwrap scaffolding copied across three transports.** + The Outcome union + expire() select race is byte-identical between pool.zig and forward_client.zig (`fn expire(` appears 14 times tree-wide); the cancel-protected close helpers and mapPhase/sendFailure/receiveFailure repeat between dot_client and forward_client. The predicted drift is already real: doh_client has none of the unwrap (see Theme 3). Fix: shared helpers in transport.zig, which already owns the framing helpers and error taxonomy. + +- **[low] src/local/records.zig:197 — normalizeName duplicated verbatim in forward_zones.zig, with a third partial copy in dns_cache.buildKey.** + Divergence risk is not hypothetical: filter/rules.zig and filter/compiler.zig already carry near-variants with differing byte-rejection policies. Fix: move it to dns/name.zig next to fromText (both callers already import it). + +- **[low] src/filter/compiler.zig:64 — the subtle takeDelimiter/StreamTooLong/discard streaming loop exists twice (compile and manager.collectSample).** + Both copies are correct and tested, and the cited behavioral differences are intentional per call site; the risk is a std.Io.Reader semantics change or a third copy-paste. Fix: a shared bounded-line-iterator helper. (The 35f2324 linkage in the original finding was inaccurate — that bug was reader-buffer aliasing, a different class.) + +- **[low] src/cli.zig:906 — upstream client construction duplicated between probeUpstreams and app.Upstreams.build.** + Concrete drift hazard: cli.zig hardcodes the DoH buffer sizes as literals that app.zig defines as named constants — changing the constants leaves `check` probing different buffers than `run` uses, undermining the probe's stated purpose. Fix: a shared build-one-client-per-entry helper, or at minimum shared size constants. + +- **[low] src/cli.zig:722 — ZON parse failure rendered through two channels in `check` vs `import`.** + check prints the multi-line zon_diag inline (embedding newlines mid-FAIL-line, contradicting the one-line-per-problem promise in docs/reference/configuration.md:332), import routes through reportParseFailure/Diagnostics. Fix: expose reportParseFailure (it is currently private) and route check through it. + +- **[low] build.zig:155 — test-suite module wiring duplicated block-for-block for host and aarch64, plus a re-spelled target triple.** + Divergence is caught loudly (compile/link errors, blocking CI qemu job), just late. Fix: an addTestSuite helper mirroring addExecutable, triple from cross_targets. + +- **[low] tests/fuzz/blocklist_fuzz.zig:160 — Smith corpus encoders (u32-LE length-prefix convention) copy-pasted between the two fuzz files.** + A Smith stream-format change caught in one file and missed in the other leaves that corpus silently decoding to garbage while targets stay green. Fix: a shared dependency-free tests/fuzz/smith_encode.zig — no build.zig change needed since dns_fuzz.zig already imports corpus.zig by relative path. + +## Theme 3: Silent failures and swallowed errors (11 findings) + +- **[medium] src/filter/manager.zig:541 — loadSource swallows error.Canceled into a per-source `.load_failed` status.** + Every other catch site in the file (roughly ten) propagates Canceled per the milestone-3 invariant "error.Canceled must never be recorded at all"; here a cancellation during a reload's file reads records a bogus "Canceled" load failure, consumes the one-shot cancellation signal, and publishes a snapshot with that source excluded. Fix: one more catch arm per readFileAlloc call (`error.Canceled => return error.Canceled`; Manager.Error already has the member). + +- **[medium] src/platform/tls_server.zig:430 — BIO callbacks discard the concrete transport error, including cancellation.** + net_reader.err/net_writer.err (which hold Reset/Timeout/Canceled) are never read; everything collapses to MBEDTLS_ERR_NET_RECV/SEND_FAILED, so a routine idle-budget cancel surfaces as a warn-level "mbedtls_ssl_read failed" and peer resets are indistinguishable from timeouts. The client side (dot_client's concreteRead/concreteWrite) solved exactly this. Fix: capture the stashed causes in the BIO callbacks and fold them into the error surface — note the ReadError set must be widened, not just the field captured. + +- **[medium] src/web/server.zig:616 — a Cookie header over 1024 bytes silently degrades to unauthenticated.** + copyHeader returns "" with no log; under the documented reverse-proxy-on-shared-domain deployment (foreign cookies riding along), every request 401s and the operator sees an unexplained login loop with zero diagnostics anywhere on the request path. Fix: extract only the nxdns_session pair from an oversized header (cookieValue already parses), or at minimum log the size-drop (no secret-logging conflict). + +- **[medium] web/src/features/queries/QueryLogPage.tsx:88 — load-more accumulation silently develops a mid-table row gap when the base page refetches.** + The refocus-after-30s refetch shifts the newest-100 boundary up while `extra` starts strictly below the old cursor; the missing rows are in neither, and the stale cursorOverride means load-more never heals it. On a live DNS server the trigger is routine. Fix: useInfiniteQuery (or disable background refetch while extra is non-empty), or detect the discontinuity and reset the accumulation. + +- **[low] src/upstream/doh_client.zig:157 — DoH mapError never unwraps the stashed cause behind ReadFailed/WriteFailed.** + Verification narrowed the blast radius: the pool's select harness means shutdown cancellation is handled correctly despite this, so the reachable impact is rare mid-exchange local-resource errors (e.g. ENOBUFS) recorded as peer faults against a healthy upstream — still a stated spec-invariant violation, fixed once for DoT and left in DoH. Fix: the same unwrap the other two transports carry. + +- **[low] src/filter/manager.zig:1237 — commitStatus silently drops the outcome of a source with no status entry.** + Reachable today via a race between startupPass and a web-inserted source; the milestone-5 policy says every non-ok state "is recorded and surfaced, not swallowed". Fix: warn on the fall-through or have refreshSourceLocked insert the missing entry. + +- **[low] src/web/http_util.zig:335 — static header-array overflow reported as OutOfMemory.** + A future eighth header would manifest as mysterious OOM-labelled connection drops. All call sites are static (max 3 today). Fix: std.debug.assert so the mistake fails loudly in tests. + +- **[low] web/src/features/blocklists/BlocklistForm.tsx:30 — bare `catch {}` on submit swallows everything, not just the mutation error it assumes.** + An exception outside mutateAsync never reaches mutation state, so the form silently does nothing with no console trace. The codebase's own convention (milestone-9 review: logout swallows only 401) is swallow-only-the-expected-class. Fix: rethrow or console.error anything the inline-error prop will not display. + +- **[low] tools/gen_web_assets.zig:71 — dist-shipped .gz siblings trusted unverified; orphan .gz files embed as unreachable bytes.** + The embedded-dist test partially covers this but never gunzips-and-compares, and CI's test run never sees the dist that ships (`cross -Dweb-dist` vs `test -Dintegration`). Exposure is latent (no compression plugin today). Fix: decompress and byte-compare shipped siblings; reject orphans. + +- **[low] deploy/docker/Dockerfile:20 — `${TARGETARCH:-amd64}` bypasses the fail-fast arm under the legacy builder.** + A plain docker build on an arm64 host (the Pi 5 target) packages the x86_64 binary and dies at `docker run` with exec-format, far from the mistake. Fix: drop the default and error naming buildx, or default from `uname -m`. + +- **[low] src/platform/logging.zig:287 — oversized log messages truncate mid-format with no marker.** + `mw.print(...) catch {}` then uses the partial buffer, contradicting the sink's own "never both discarded and silent" invariant and the '...'-marker pattern safe_url.zig uses. Fix: catch the fixed-writer overflow and append a marker or bump a lines_truncated stat. + +## Theme 4: Operator-facing contract drift (11 findings) + +- **[medium] src/upstream/pool.zig:110 — no overall exchange deadline; the key named `total_timeout_ms` bounds one attempt.** + With N upstreams all timing out, a query takes N × 5000ms; docs/reference/configuration.md calls the key a "per-query budget" while cli.md correctly says "per-attempt" — the two reference docs contradict each other, PLAN still promises a total budget, and validate.zig's total>=read cross-check relates knobs of different subsystems. The per-attempt code is intentional per milestone-3; the name, docs, and PLAN are the drift. Fix: either one race around the failover loop, or rename to attempt_timeout_ms and correct docs/PLAN/validator. + +- **[medium] src/config/validate.zig:609 — the validator accepts hostname `tls://` upstreams that the DoT client refuses by design on every dial.** + Literal documented spec drift: milestone-3 explicitly told the orchestrator to carry the IP-literal requirement into the phase-4 validator, and it was dropped. Worse, docs/reference/configuration.md:348 presents a hostname NextDNS `tls://` config that can never complete an exchange. Fix: apply the same IP-literal check parseResolver already enforces, at the spot where checkCollections already holds the parsed Endpoint. + +- **[medium] src/config/validate.zig:391 — no upper bound on the two fields that literally size boot allocations.** + `query_log_buffer_max = 4000000000` validates clean, `nxdns check` prints OK, `nxdns run` OOMs classified as a runtime failure. Timeouts and TTLs have maxima in the same file. Fix: sanity caps (or a byte-budget check) matching the file's established idiom; a full memory-fit guarantee is out of reach and not needed. + +- **[medium] web/src/lib/queries.ts:251 — the upstream management UI was specced in milestone 8 ("the Settings page must edit them") and never built.** + The full client layer (API wrappers, query, three mutation factories) exists with zero consumers; no route or page edits the resolver pool, so URLs/priority/enabled/tls_name are curl-only. Same pattern in miniature: ruleUpdateMutation and five get* wrappers are unused. Fix: build the editor the m8 spec called for, or delete the vestigial factories and record the scope decision. + +- **[medium] src/web/api_limiter.zig:137 — behind the documented reverse proxy, the API limiter is bypassed and the SSE cap collapses.** + Proxy-on-same-box makes every request loopback; default `localhost_exempt = true` then disables the only brake on repeated argon2 verifications (the spec-mandated brute-force defense), while remote users share one address's 3-stream SSE cap. Documented nowhere. Fix: an opt-in trusted-proxy setting (parse X-Forwarded-For only from configured proxies) or at minimum a documented warning to set `api_localhost_exempt=false` when proxying. + +- **[medium] web/src/lib/types.ts:1 — the REST contract lives in three hand-synced copies with a route-name-only drift guard.** + Zig handlers, openapi.yaml, and types.ts are each maintained by hand; the only automated guard checks route paths textually. The Zig side is genuinely well contract-tested against a real server, but the server↔yaml and server↔types.ts field-level seams are unguarded, and every frontend test stubs fetch. The milestone-9 "no codegen" ruling excludes generation, not guarding. Fix: one integration layer asserting the frontend's consumed shapes against real responses per endpoint. + +- **[low] src/dns/packet.zig:354 — BADVERS is unexpressible: addOptEcho hardcodes extended_rcode = 0 and no caller checks the EDNS version.** + A version-1 query answered locally gets NOERROR with a version-0 OPT instead of RCODE=16 (RFC 6891 MUST). Negligible operational impact (real clients send version 0; forwarded replies pass through), but the write-direction mechanism is missing entirely. Fix: an extended_rcode parameter on addOptEcho plus a version check in the handler. + +- **[low] src/server/rate_limiter.zig:5 — module docs still defer their own concurrency contract to "Phase 7", which shipped.** + The authoritative thread-safety statement points at a future that resolved (handler.limiter_mutex, app.runMaintenance); shutdown.zig references a Phase-8 restart endpoint that never shipped and now cannot. Fix: rewrite the headers to state the as-built contract. + +- **[low] docs/how-to/install-with-systemd.md:206 — `nxdns 0.1.0-dev` hardcoded in four doc transcripts with no drift guard.** + Correct today; goes silently wrong at the first tag. The milestone-14 spec already mandates the placeholder + guard. Caveat for implementation: milestone-13 requires every doc command block to be executed verbatim, so decide how placeholders coexist with executed transcripts. + +- **[low] src/platform/logging.zig:497 — manual file_pos tracking assumes exclusive ownership of the log path, undocumented for operators.** + logrotate copytruncate produces a sparse NUL-prefixed file; the docs describe built-in rotation, even claiming the file is "opened for append" (it is not — positional writes), without warning off external rotation. Fix: a warning in files-and-directories.md at minimum; a stat/length re-check before write for robustness. + +## Theme 5: Protocol robustness (5 findings) + +- **[medium] src/cache/dns_cache.zig:110 — truncated (TC=1) responses are cacheable and cached for full TTL.** + classify checks rcode and ancount but never `flags.tc`, and validateResponse doesn't either; a misbehaving upstream returning TC=1 over DoH/DoT/forward-TCP gets its partial answer cached and served (TC bit intact, even over TCP — a protocol violation that can loop retrying clients) for up to 86400s. RFC 2181 §9 forbids this; no spec declares it out of scope. Fix: reject tc in classify, arguably also in validateResponse. + +- **[medium] src/filter/parsers.zig:66 — the format sniffer misclassifies hosts files with `##` banner comments as ABP.** + hasAbpMarker runs before isComment and matches `##` unanchored with no comment guard (unlike the deliberately guarded `$` branch two lines later). Reproduced with the literal URLhaus banner style: the whole file parses as ABP, `0.0.0.0` enters the compiled domain set, and — worse than the original finding — hosts lines with inline reference comments containing URLs are silently dropped (real blocking loss). Fix caveat from verification: guarding the element-hiding branch with !isComment is a circular no-op; instead anchor the `##` match per the As-built note, or treat `##` at line start on a `#`-initial line as a banner. + +- **[medium] web/src/features/live/useLiveQueries.ts:29 — SSE cap/session-expiry detection relies on EventSource retry behavior that does not exist for HTTP rejections.** + Per the WHATWG spec a non-200 response fails the connection permanently after one error event, so the 3-consecutive-errors threshold is unreachable on exactly the 429/401 paths it was built for: the UI shows "Reconnecting…" forever, the capped state and session probe are dead, and the spec's required "too many live viewers + retry" state is violated. FakeEventSource has no readyState/reconnect semantics, so tests pass — the mocked-network class again. Fix: expose readyState on EventSourceLike, treat CLOSED in the error handler as permanent failure, and run the session probe there. + +- **[low] src/filter/fetcher.zig:169 — TLS error classification by @errorName prefix string-matching.** + Duplicated in doh_client.zig; a std rename silently downgrades TLS failures into generic buckets, and the test exercises inputs std can no longer produce (verification shows only TlsInitializationFailed and CertificateBundleLoadFailure are reachable). Contradicts transport.zig's own exhaustive-switch philosophy. Fix: name the reachable errors in the exact-match switch in both files. + +- **[low] src/platform/tls_server.zig:506 — hand-transcribed mbedtls error codes/config enums have no drift guard, unlike sizes/alignment.** + All ten values currently match the pinned 3.6.7 headers; the exposure is the eventual version bump, where a renumbered close_notify code would silently invert truncation-detection semantics. The sizes-vs-constants asymmetry is principled (hash-pinned version), so this is cheap insurance, not an oversight to be alarmed about. Fix: export the constants from mbedtls_shim.c and verify once at init. + +## Theme 6: Concurrency, locking and lifecycle coupling (6 findings) + +- **[medium] src/filter/manager.zig:609 — writer_lock held across every download of a refresh pass, stalling all web mutations' reloads.** *(merged: web/handlers/blocklists.zig:171, the same debt from the manual-refresh-endpoint angle)* + refreshAll holds writer_lock uncancelably across N × 300s download budgets plus compiles; every mutation handler ends in Manager.reload on the same lock, so an unrelated rule save parks uncancelably (on a request path with deliberately no timeout, occupying one of 64 slots) until the pass ends. The spec's serialization rationale covers only readers-never-wait; writer-vs-writer latency is unaddressed emergent behavior. DNS serving is unaffected and the stalled edit is not lost. Fix: download+compile to tmp files outside the lock, take it only for publish+DB write+reload — or a 202-with-poll job model for the manual endpoint. + +- **[medium] src/storage/retention.zig:95 — VACUUM runs even when the disk monitor says critical.** + The logger and blocklist refresher gate on `writesAllowed()`; the seventh-pass VACUUM — the most expensive write the program makes, needing roughly DB-size free space — fires unconditionally, transiently draining the exact filesystem the monitor watches, then failing SQLITE_FULL with no retry for seven daily passes. Prune and checkpoint should keep running (they free space). Fix: pass the optional monitor into runOnce the way the two sibling tasks already do, gating only the vacuum step. + +- **[medium] src/web/sse.zig:153 — Hub has no shutdown broadcast; graceful drain stalls up to 15s per idle SSE subscriber.** + Socket shutdown cannot wake a task parked on the per-slot Event; only the heartbeat write fails, up to 15s later — measured: the SSE integration teardown sits exactly 15s in group.await. Production is masked by the cancel path, but the documented "shutdown unblocks every connection" contract is violated. Fix: Hub.close(io) setting a shutdown flag and signaling every active slot's event, called from beginShutdown. + +- **[low] src/web/handlers/groups.zig:70 — reload runs outside config_lock in five handlers but inside it in local.zig; the unlocked variant is safe only via an undocumented Manager invariant.** + Safety depends on Manager.reload re-reading the database under its own writer_lock — a cross-module fact stated nowhere at the call sites; changing reload to accept pre-read rows (the shape swapLocalTables already has) would silently regress five handlers with the exact ordering bug the milestone-8 review already fixed once in local.zig. Fix: document the re-read requirement at mutations.reload, or hold config_lock uniformly and document the cost. + +- **[low] src/web/handlers/settings.zig:312 — config_lock held across argon2id hashing (19 MiB, t=2) in the settings PUT.** + Stalls every mutation handler and the settings GET for the hash duration on the Pi 5, contradicting the lock discipline auth.zig documents and the login path follows. Hashing depends only on the parsed patch; the LiveHash generation check already handles the concurrent-install race. Fix: hash before acquiring the lock. + +- **[low] src/server/doh_server.zig:66 — idle_timeout only bounds the handshake; idle keep-alive clients pin DoH slots until restart.** + The web-listener precedent the spec cites assumes clients eventually close; DoH stubs hold keep-alives by design, and with no TCP keepalive a vanished peer pins a slot permanently — while DoT on the same LAN reclaims after 10s. Fix: extend the race to receiveHead the way DoT races readPrefix, or at least rename the misleading option. + +## Theme 7: Observability debt (6 findings) + +- **[medium] src/server/udp_server.zig:38 — UDP/53 and TCP/53 listener stats are written and never read anywhere.** + dropped_no_slot/dropped_oversize increment with no metric, API field, or log line (errors log only at debug below the default level), while the DoT/DoH siblings export equivalent counters to /metrics. The module doc sells "dropped and counted" as the failure surface; the surface is unobservable. Fix: give both the same snapshotStats() + metric families the TLS listeners have. + +- **[medium] src/local/forward_client.zig:45 — ForwardClient.Stats are instrumented but discarded in production.** + The doc claims the foreign-datagram counter prevents "an unrecorded failure mode"; the only production caller builds a stack-local client per query and drops it, so the spoofing signal does not exist and truncation/failure counters never reach /metrics. Fix: aggregate into handler-owned atomics like the rest of the pipeline, or delete the Stats struct. + +- **[medium] src/server/dot_server.zig:313 — a timed-out handshake counts as tls_handshake_failures on DoT but idle_timeouts on DoH.** + The DoT classification is the recorded spec ruling; DoH drifted in the copy, and since DoH requests have no timeout, its idle_timeouts metric can only ever mean handshake stalls — the same exported name carries disjoint semantics per listener, corrupting cross-listener dashboards. Fix: align DoH's .timed_out to tls_handshake_failures per the ruling. + +- **[medium] src/platform/tls_server.zig:311 — warn-level logging for routine peer misbehavior contradicts the module's own debug ruling.** + A peer TCP drop is deliberately debug on the read path, then the close_notify against the dead socket warns; probes that send bytes or reject the cert warn per event, and .tls_server is not in the dedup scope set, so peer-driven warns can evict genuine warnings from the rotating log. Fix: apply the read path's peer-vs-local triage to close/handshake reporting, or add the scope to the dedup set. + +- **[medium] src/web/api_limiter.zig:210 — ApiLimiter.sweep is never called in production.** + Designed for background reclamation (preallocated stale_keys so sweep never allocates), but only the DNS-side limiter got scheduled in runMaintenance. Once 4096 distinct addresses are seen, the table stays full forever and every unknown-address request pays an O(4096) scan under the limiter mutex. Fix: add it to the maintenance loop, or delete sweep and own inline-eviction-only. + +- **[low] src/server/handler.zig:255 — two tracker-mutex acquisitions per query (scan + full stats copy) purely to mirror one counter.** + Fix: track() returns fullness, or dropped_full becomes an atomic readable without the mutex. + +## Theme 8: Dead code, hardcoded values, frontend state hygiene (11 findings) + +- **[medium] web/src/features/settings/SettingsPage.tsx:16 — settings field registry is stringly typed and cast-driven.** + A typo'd key compiles, renders undefined, and breaks the field (verification: an edited typo'd key is actually sent and rejected at save — still broken, tsc still green). Fix: `key: keyof Settings[S]` via a generic SectionDef, which removes every cast and makes drift a compile error. + +- **[medium] web/src/features/blocklists/BlocklistsPage.tsx:33 — blocklist refresh status lives in an observer-less query-cache entry.** + Read with non-subscribing getQueryData (renders only via the mutation's coincidental re-render) and garbage-collected after 5 minutes, after which the page claims no snapshot ever existed. This is client UI state, not server-cache state. Fix: a small store (the restartBanner useSyncExternalStore pattern already in-repo) or a real subscriber with gcTime: Infinity. + +- **[low] src/dns/edns.zig:111 — parseEcs, Ecs, ecs_family_*, extendedRcode have zero production callers.** + stripEcs filters by option code without decoding; forward mode keys on raw bytes; no path composes an extended RCODE (see BADVERS). The project's own precedent deleted ecsPayload when it lost its caller. Fix: wire extendedRcode into a BADVERS path (giving it a caller) or delete both and let git history resurrect them. + +- **[low] src/web/handlers/settings.zig:354 — dead errdefer in writeSettings implies protection it cannot provide.** + The function returns `?db.Error`, so every exit is a value return and the errdefer never fires; the real safety is three explicit rollbacks below it. Fix: return `db.Error!void` so the spec's canonical errdefer pattern becomes live and the manual rollbacks collapse. + +- **[low] web/src/features/settings/SettingsPage.tsx:220 — the settings diff compares live query data against a mount-time clone.** + A background refetch makes out-of-band changes appear as user edits; Save silently reverts them. Fix: track dirty keys and rebase untouched fields, or freeze the baseline. + +- **[low] web/src/features/rules/RulesPage.tsx:24 — default group id 1 encoded four ways with two fallback semantics.** + RulesPage preselects the alphabetically first group (the API orders by name), not the default group. Fix: one exported DEFAULT_GROUP_ID and a defaultGroupId(groups) helper. + +- **[low] web/src/routes.tsx:97 — the dashboard loader's Promise.all makes the page's per-widget error handling unreachable on first load.** + One failing endpoint blanks the whole dashboard on cold navigation; the graceful per-section degrade only runs for background refetches. Fix: prime without throwing (allSettled / fire-and-forget) so the granular UI owns failures. + +- **[low] web/src/lib/queries.ts:99 — raw `["lookup"]` / `["groups"]` literals bypass the queryKeys table at eight sites.** + A key restructure updates typed usages and leaves the literals silently invalidating nothing. Fix: a `lookupAll` entry and using queryKeys.groups. + +- **[low] src/storage/logger.zig:44 — Entry buffer sizes are file-private and re-hardcoded by callers whose comptime guards check their own copies.** + handler.zig's reason-length guard proves fit against its local 32, not logger's constant; shrinking logger's buffer passes the guard and silently truncates. Fix: make the constants pub and point the guards at them. (The two max_ip_text=45 copies derive from RFC 5952 independently; that portion is marginal.) + +- **[low] src/app.zig:609 — dev-mode asset directory in a module-level mutable global.** + Verification corrected the root cause: the fallback pointer already receives *WebState and discards it; the fix is a `dev_dir` field on WebState set at the composition root — not the signature-widening the finding proposed. + +- **[low] src/server/cert_store.zig:326 — private key PEM freed without zeroization.** + Occurs at boot and on actual cert renewals (not every poll); the parsed key stays resident in mbedTLS regardless, so this closes only the freed-page-reuse exposure. Fix must wipe inside readPem (readFileAllocOptions may reallocate, leaving intermediate copies), not just a defer at the call site. + +--- + +## Suggested cleanup order + +**Phase 1 — make green mean green (Theme 1, first).** +Nothing else can be validated until the pipeline is trustworthy. Start with ci.yml → master (already specced in milestone-14 §7), then the test-binary abort (root-cause or document), the web/dist freshness guard, the tests.zig completeness guard, and a cron on live-tls.yml. These are independent of each other and of everything below, and every later phase relies on them to verify itself. The remaining Theme-1 items (fuzz targets, multi-read fixture, docs_drift derivation) can trail in the same phase. + +**Phase 2 — contained behavioral fixes with real failure modes (Themes 3, 5, 6, 7 mediums).** +Do these before any large refactor so the refactors don't carry known bugs forward: the loadSource Canceled arm, the TC=1 check in classify, the sniffer banner rule (use the anchoring fix, not the circular guard), the VACUUM disk gate, the cookie-header extraction/log, the EventSource readyState handling, the DoH/DoT metric alignment, Hub.close, the ApiLimiter sweep wiring, and exporting the UDP/TCP listener stats. Each is localized, each has a verifier-checked fix shape, and several (sniffer, TC=1, EventSource) fix behavior users can hit today. + +**Phase 3 — contract decisions (Theme 4).** +These need a decision more than code: total_timeout rename vs. real deadline; upstream UI build vs. delete; the trusted-proxy setting; validator IP-literal and allocation bounds; the types.ts guard strategy. Decide once, then the edits are mechanical. Doing this after Phase 1 means the doc-drift guards you add here actually run. + +**Phase 4 — the duplication refactors (Theme 2).** +Largest payoff, largest risk, so they go after the pipeline is trustworthy and the known bugs are out (a refactor that must simultaneously fix behavior is how mirrored copies diverge further). Order within the theme: the listener core first (the only high, four copies, drift underway, and it subsumes several per-listener findings); then the repo list helper (the spec itself contains the fatal ordering, so future repos are landmines until this exists); then the web CRUD descriptor, transport race/close helpers, and the frontend ui/ extraction. Each collapse means every Phase-2-style fix in that family lands once from then on. + +**Phase 5 — hygiene sweep (Theme 8 and remaining lows).** +Dead code deletions (parseEcs, dead errdefer, unused mutation factories per the Phase-3 decision), pub constants, the query-key table, DEFAULT_GROUP_ID, the WebState dev_dir field, key zeroization. Low risk, no dependencies; several ride along naturally with Phase 4 files they touch. diff --git a/specs/milestone-15.md b/specs/milestone-15.md new file mode 100644 index 0000000..dbafda3 --- /dev/null +++ b/specs/milestone-15.md @@ -0,0 +1,361 @@ +# Milestone 15: make a green run mean a real pass + +Goal: repair the verification pipeline — CI that watches the branch we push to, +a test run whose output contains no false failure text, guards that make the +test list and the embedded frontend complete by construction, and coverage for +the seams the audit found unguarded. This is phase 1 of `TECH_DEBT.md`; nothing +in phases 2-5 can be validated until this lands. + +Origin: `TECH_DEBT.md` Theme 1 (lines 17-52), audited at `1ff727f`, every +finding adversarially verified. + +## Sequencing (binding) + +This milestone lands **before** milestone 14 is implemented. Ruling 1 pulls the +`ci.yml` branch switch forward from milestone-14 §7; when milestone 14's S4 +later restructures the workflows into `gates.yml`/`release.yml`, it rebases on +what this milestone leaves behind. One more overlap is binding: milestone 14's +S1 rewrites `build.zig` (deletes `cross`, adds `dist`/`verify-dist`) — it must +rebase on and **preserve** the configure-time machinery this milestone adds +there (ruling 4's import guard, ruling 5's stamp check, ruling 6b's staged +`core` module). Milestone 14's spec stands as written otherwise. + +## Rulings (binding) + +### 1. CI watches `master` + +`.gitea/workflows/ci.yml:1-11` triggers on `push`/`pull_request` for `main`. +Day-to-day pushes go to `origin/master` (`branch.master.merge` confirms), so +milestone 13+ never ran through any gate. Change both `branches:` lists to +`[master]`. Nothing else in the file changes — the `gates.yml` restructure is +milestone-14 work. + +Orchestrator, after merge: delete `origin/main`, set the Gitea default branch +to `master`, push, and confirm a run starts. (Milestone-14 §7 already rules +`master` canonical; this executes the branch part now.) + +### 2. The live-network suite runs on a schedule + +`.gitea/workflows/live-tls.yml` runs on `workflow_dispatch` only. Both failure +modes it exists to catch have each happened once (35f2324; the milestone-4 cert +drift). Add: + +```yaml +on: + workflow_dispatch: + schedule: + - cron: "0 5 * * 1" +``` + +Weekly, Monday 05:00 UTC. It stays non-blocking — this does not conflict with +the milestone-1 ruling that live tests never gate a push. Everything else in +the file is untouched. + +### 3. The teardown abort is root-caused, or documented where it lives + +Reproduction, verified at HEAD: `zig build test` exits 0 but prints as its +last line `failed command: .../test ... --listen=-`. The same binary exits 0 +in stdio mode (`1242 passed; 115 skipped; 0 failed`) and exits 134 (SIGABRT) +under `--listen=-`. The abort happens after all tests pass, in teardown, only +under zig's IPC runner. No comment anywhere in the repository records this; +the knowledge lives in one contributor's memory and `TECH_DEBT.md:24-25`. + +The cost is not the abort — it is that the one string that should mean failure +prints on every success, and everyone learns to ignore failure text. + +The session must attempt a root cause, in this order: + +1. Reproduce: run the cached test binary directly with and without + `--listen=-`. Capture the abort backtrace (`ulimit -c` / gdb / the zig + `--debug-...` runner flags — whatever this host offers). +2. Bisect the mbedTLS linkage: `addMbedtlsThreadingMacros` (build.zig), the + shim (`src/platform/mbedtls_shim.c`), and the two `extern fn` link checks in + `src/tests.zig:121-123` are the suspects. A minimal reproducer (empty test + file + mbedTLS link + `--listen`) decides whether the abort is ours or an + upstream zig 0.16 runner defect. +3. Outcome A — the cause is ours: fix it. Acceptance is a `zig build test` run + whose output contains no `failed command` line. +4. Outcome B — the cause is upstream: do not patch around it. Document it in + two places: a comment on the test wiring in `build.zig` (at the + `b.addTest` block, lines 48-69) stating the exact mechanism and the upstream + reference, and a paragraph in `AGENTS.md` stating that `failed command` + + exit 0 after a full pass is this known artifact and that any *other* + failure text is real. + +Either way: no check is loosened, no output is filtered, and the spec is +updated afterward to record which outcome held (per the spec-update rule). + +### 4. The test import list gets a completeness guard + +`src/tests.zig:3-119` is a hand-maintained `comptime` block of 115 +`_ = @import("...");` lines. Zig collects tests only from the root module; a +forgotten import silently drops a file's tests (verified empirically). The +list stays hand-written — generation via a staged copy of `src/` would point +diagnostics at cache paths — but it becomes complete by construction: + +In `build.zig`, at configure time, walk `src/` recursively. For every `*.zig` +file except `src/tests.zig` itself, require that `src/tests.zig` contains a +**line** which, after whitespace trim, is exactly `_ = @import("");` +where `` is the file's path relative to `src/`. A line match, not a +substring search: a commented-out import (`// _ = @import(...)`) trims to a +line that starts with `//` and does not match, and a path that is a prefix of +another (`db.zig` vs `db2.zig`) cannot false-match because the full line is +compared. Duplicate matching lines are an error too (they hide a botched +merge). On the first missing file, fail the configure with an error that +names it: + +``` +src/tests.zig is missing `_ = @import("filter/new_module.zig");` +``` + +No allowlist. A file with no tests still gets imported — the import is free, +and the rule stays exceptionless. The walk runs on the host in every +invocation, including the aarch64 configure. + +### 5. `web/dist` gets a freshness stamp + +A stale `web/dist` has already shipped a crashing settings page once +(`docs/explanation/performance-and-testing.md:154-163` admits the hole). +Mechanism — one implementation, in JavaScript, because the frontend CI job has +Node and no Zig: + +- New file `web/scripts/stamp-dist.mjs`. Two modes: + - default (write): hash the input set, write the hex digest to + `web/dist/.src-hash`. + - `--check`: recompute, compare with `web/dist/.src-hash`, exit 1 with + `web/dist is stale: rebuild the frontend (npm run build)` on mismatch or + missing stamp. +- The script resolves every path (input files and `web/dist/.src-hash`) from + its own location via `import.meta.url`, never from `process.cwd()`: write + mode runs from `web/` (npm script) and check mode runs from the repo root + (build.zig system command), and both must hash the same set. The + acceptance run exercises both working directories. +- Input set, sorted by path, SHA-256 over `path ++ "\x00" ++ contents ++ + "\x00"` per file: every file under `web/src/` and `web/public/`, plus + `web/index.html`, `web/package.json`, `web/package-lock.json`, + `web/vite.config.ts`, `web/tsconfig.json`, `web/tsconfig.app.json`, + `web/tsconfig.node.json`. Node's `crypto` module; no new dependency. +- `web/package.json`: `"build"` becomes `"vite build && node + scripts/stamp-dist.mjs"`. +- `build.zig`: when the resolved `-Dweb-dist` value is exactly `web/dist`, the + asset pipeline (`webAssetsIndex`) gains a dependency on + `b.addSystemCommand(&.{ "node", "web/scripts/stamp-dist.mjs", "--check" })`. + The default `web/dist-placeholder` path and any other explicit path skip the + check; the `web-dist` option description says so. + +### 6. Three new fuzz surfaces, corpus replay only + +All targets follow the existing registration shape (one `test "fuzz ..."` + +one `fn target(ctx, smith: *Smith)`), attach to `test_step`, and run as corpus +replay under plain `zig build test` — once per corpus entry plus once on empty +input. CI never passes `--fuzz` (blocked by upstream zig 0.16 defects; the +audit confirmed corpus replay is the sanctioned CI smoke). + +**6a. `stripEcs`** — the only attacker-facing packet-rewriting entry point, +absent from the fuzz targets. Add a target to `tests/fuzz/dns_fuzz.zig`. +Derive input exactly as `parseTarget` (lines 57-89) already does: it ends with +a validated `Packet` and its `OptRecord`, which are `stripEcs`'s second and +third parameters. Respect the three entry assertions +(`src/dns/edns.zig:196-204`): pass the same `bytes` the packet was parsed +from, and a separate stack `out` buffer — an assertion trip from a violated +precondition is not a finding. Assert on `.rewritten` output: it re-parses, +`findOptRecord` + `parseOpt` succeed, no ECS option (code 8) remains, and +header counts survive. No build.zig change — the module exists. + +**6b. `compiler.compile`** — the streaming +`takeDelimiter`/`StreamTooLong`/discard loop (`src/filter/compiler.zig:64-78`) +is never fuzzed, and its `error.EndOfStream`-during-discard arm (line 73) has +no test at all. `compiler.zig` imports `../dns/`, so a module rooted under +`src/filter/` fails with `ImportOutsideModulePath`. Use the staged-copy +aggregator pattern from `bench_core` (`build.zig:112-135`) — `bench_core` +already exposes `compiler` at line 124; reusing the same staged tree is +allowed. New file `tests/fuzz/compiler_fuzz.zig`, module import name `core`. +The target feeds `compile()` via `std.Io.Reader.fixed` over smith bytes with a +deliberately small reader buffer, into discarding writers, and asserts it +returns without panic and that `counts` are internally consistent. Corpus +(inline, `sliceInput` idiom): a line longer than `max_line_len` (4096) with +and without a trailing `\n`, so both the discard path and the +EndOfStream-during-discard arm replay. Add a plain unit test for the line-73 +arm alongside the existing compiler tests. + +**6c. `http_util`** — the third untrusted-byte family, unfuzzed. +`src/web/http_util.zig` imports only `std`, so the fuzz module roots at a new +file `tests/fuzz/http_util_fuzz.zig` with `addImport("http_util", )` — no aggregator. Targets: `parsePath`, +`decodeInPlace` (both `PlusRule` values), `queryValue`. Invariants to assert, +both already stated in the file: split-before-decode (a decoded segment never +gains a `/`), and decode-only-shrinks (result length ≤ input length; result is +a prefix-aliased slice of the buffer). `dnsParam`/`decodeDnsValue` stay +private in `doh_server.zig` and stay unfuzzed — recorded, out of scope. + +### 7. The multi-read fetch path gets a real fixture route + +The exact seam of 35k-killer 35f2324 is still never driven end to end: every +fixture reply is one `request.respond` of a ~130-byte body +(`src/filter/filter_integration_test.zig:414`), and the post-fix unit tests +pre-buffer readers. Two thresholds matter and they are different: a body over +16 KiB (`fetcher.min_transfer_buf`, fetcher.zig:24) forces the fetcher's +`pumpBody` loop to iterate; a body over the fixture's 8192-byte write buffer +forces the fixture to flush in parts. + +- Add a route `chunked` to the `Route` enum (line 354). Its `respond` arm uses + `respondStreaming`, writes a well-formed blocklist body of at least 24 KiB + in at least three writes with an explicit `flush()` between each, then ends + the stream. `keep_alive = false`, like every other arm (the comment at lines + 409-411 is load-bearing). +- Add one `-Dintegration` test that drives the full `refreshOnce` path through + this route and asserts the parsed domain counts. +- Prove it can fail: during development, reintroduce the + `readSliceShort(self.transfer_buf)` aliasing pattern locally and confirm the + new test dies where the old suite stayed green. Record the proof in the + session notes; do not commit the revert. + +### 8. The rotation failure paths get tests + +`src/platform/logging.zig` codifies an exactly-one-sink-error contract across +four functions (stated at lines 455-460) and its own test section admits the +rotation failure paths are uncovered (lines 604-612). Injection seam: + +```zig +var rotate_fault: enum { none, fail_delete, fail_rename } = .none; +``` + +file-private, read by `deleteLocked` (line 590) and `renameLocked` (line 597) +as their first statement (`if (rotate_fault == .fail_delete) return +error.RotateFailed;`), compiled only under `@import("builtin").is_test`. Two +new tests, each following the established shape of the failed-open test at +lines 883-909 (save and restore `state.*`, hold `std.debug.lockStderr`): + +- `fail_delete`: `prepareFileLocked` returns false, `sink_errors` rose by + exactly one, `rotate_pending` stays set, the file stays closed. +- `fail_rename`: same assertions through the rename step. + +### 9. The CLI drift guard derives its needles + +`src/docs_drift_test.zig:51` hardcodes the subcommand list; its two sibling +guards derive theirs (`routes.table`, `model.toSettings`). The source of truth +today is three parallel copies: the `if (eql(...))` parse chain +(`cli.zig:91-102`), the `main.zig:69-78` dispatch switch, and `usage_text` +(cli.zig:334). Note the spelling trap: tags are `export_`/`import_`, argv +strings are `export`/`import`. + +- Add to `cli.zig`: + + ```zig + pub const CommandName = struct { name: []const u8, tag: std.meta.Tag(Command) }; + pub const command_names = [_]CommandName{ + .{ .name = "run", .tag = .run }, + .{ .name = "check", .tag = .check }, + .{ .name = "export", .tag = .export_ }, + .{ .name = "import", .tag = .import_ }, + .{ .name = "version", .tag = .version }, + .{ .name = "help", .tag = .help }, + }; + ``` + + plus a comptime assert that `command_names.len == + @typeInfo(Command).@"union".fields.len`, so a new tag without a table entry + fails the compile. +- `parseArgs` matches the command word by iterating `command_names` (the + per-command argument parsing that follows stays as-is). +- `docs_drift_test.zig` iterates `cli.command_names` for its `## `{s}`` + needles; the hardcoded list is deleted. +- New test in `cli.zig`: every `command_names[i].name` appears in + `usage_text`. +- `main.zig`'s dispatch switch stays — it is exhaustive over the union and the + compiler already guards it. + +## Sessions + +S1-S4 run in parallel; no two sessions write the same file. The S1/S3 +interface is fixed here so neither blocks: S3 writes the fuzz source files +named in ruling 6; S1 wires them in `build.zig` with the module shapes named +there (6b gets `core` via the staged-copy pattern; 6c gets `http_util` rooted +at `src/web/http_util.zig`); both artifacts attach to `test_step` exactly like +`fuzz_tests` at build.zig:92. + +### Session S1: build system and stamp + +Owns `build.zig`, `AGENTS.md`, `web/scripts/stamp-dist.mjs`, +`web/package.json`. Rulings 3, 4, 5, and the wiring half of 6. + +### Session S2: workflows + +Owns `.gitea/workflows/ci.yml`, `.gitea/workflows/live-tls.yml`. Rulings 1 +(file edit only), 2. + +### Session S3: fuzz targets and fixture + +Owns `tests/fuzz/dns_fuzz.zig`, `tests/fuzz/compiler_fuzz.zig` (new), +`tests/fuzz/http_util_fuzz.zig` (new), +`src/filter/filter_integration_test.zig`, and the compiler unit-test addition +in `src/filter/compiler.zig`. Rulings 6 (target half), 7. + +### Session S4: guards and seams + +Owns `src/cli.zig`, `src/docs_drift_test.zig`, `src/platform/logging.zig`. +Rulings 8, 9. + +### Orchestrator + +Ruling 1's remote operations (delete `origin/main`, flip the Gitea default, +confirm a run starts), the ruling-3 outcome recorded back into this spec, and +striking the closed Theme-1 findings in `TECH_DEBT.md`. + +## Module layout + +New files: + +- `web/scripts/stamp-dist.mjs` — dist freshness stamp, write and `--check` + modes. +- `tests/fuzz/compiler_fuzz.zig` — compiler streaming-loop target. +- `tests/fuzz/http_util_fuzz.zig` — HTTP parser targets. + +## Acceptance (milestone complete) + +- [ ] `ci.yml` triggers on `master` for push and pull_request; a push to + `origin/master` starts a CI run; `origin/main` is deleted; the Gitea + default branch is `master`. +- [ ] `live-tls.yml` keeps `workflow_dispatch` and gains the weekly cron. +- [ ] Ruling 3 resolved: either `zig build test` prints no `failed command` + line, or the build.zig comment and the AGENTS.md paragraph exist and + name the upstream cause. The outcome is recorded in this spec. +- [ ] The import guard was proven able to fail: a temporary + `src/guard_probe.zig` containing one test, not imported, makes + `zig build test` fail at configure naming the file; after adding the + import it runs; the probe is then deleted. +- [ ] `npm run build` writes `web/dist/.src-hash`. Appending a byte to a + file under `web/src/` (restored after the probe — the digest hashes + paths and contents, so a bare `touch` cannot make it stale) then + running `zig build -Dweb-dist=web/dist` from the repo + root fails with the stale message; rebuilding the frontend clears it. + (The probe is the default install step, not `cross` — milestone 14 + deletes `cross`, and this check must stay valid across that rebase.) + Plain `zig build test` (placeholder path) is unaffected. +- [ ] The three fuzz surfaces run under `zig build test` as corpus replay: + the stripEcs target with its re-parse/no-ECS assertions, the compiler + target with the long-line corpus entries, the http_util targets with + both invariants. The line-73 EndOfStream arm has a unit test. +- [ ] The `chunked` fixture route writes ≥ 24 KiB in ≥ 3 flushed parts; the + new integration test passes; the session notes record the + proven-able-to-fail run against the 35f2324 aliasing pattern. +- [ ] The two rotation-failure tests pass, each asserting exactly one + `sink_error` per the lines-455-460 contract. +- [ ] `docs_drift_test.zig` contains no hardcoded subcommand list; + `cli.command_names` exists with the comptime length assert; the + usage-text test passes. +- [ ] Full suite green (`-Dintegration`), test count strictly above 1242, and + the aarch64 suite still builds and runs under qemu. + +## Anti-requirements + +- No `gates.yml`, no `release.yml`, no `dist`/`verify-dist` steps — that is + milestone 14, which rebases on this milestone. +- No `--fuzz` anywhere in CI. +- No fuzzing or relocation of `dnsParam`/`decodeDnsValue`. +- No generated `tests.zig`; the hand list stays, the guard makes it complete. +- No frontend changes beyond `web/scripts/stamp-dist.mjs` and the one-line + `build` script edit. +- No filtering, wrapping, or suppression of test-runner output to hide the + `failed command` line. +- No new dependencies, Zig or npm. diff --git a/specs/milestone-16.md b/specs/milestone-16.md new file mode 100644 index 0000000..8def161 --- /dev/null +++ b/specs/milestone-16.md @@ -0,0 +1,448 @@ +# Milestone 16: contained behavioral fixes + +Goal: fix the verified bugs and silent failures from `TECH_DEBT.md` themes 3, +5, 6 and 7 — each one localized, each with a test that fails before and +passes after. No refactors: the duplication work is milestone 18, and a +refactor that must simultaneously fix behavior is how mirrored copies diverge +further. + +**PROVISIONAL.** Written before milestone 15 was built. m15 touches +`build.zig`, workflows, fuzz files, `filter_integration_test.zig`, `cli.zig`, +`docs_drift_test.zig` and `logging.zig` — re-verify line references in those +files before starting. + +## Rulings (binding) + +### 1. `loadSource` propagates cancellation + +`src/filter/manager.zig:541-544` and `:547-550`: two identical catch sites +fold `error.Canceled` into `loadFailure(...)`, recording a bogus "Canceled" +load-failed status, consuming the one-shot cancellation, and publishing a +snapshot with the source excluded. Ten other catch sites in the file +propagate it correctly. Add `if (err == error.Canceled) return +error.Canceled;` beside the existing OutOfMemory arm at both sites — +`Manager.Error` already has the member. Unit test: a reader that fails with +Canceled during `loadSource` makes the whole call return Canceled and writes +no status. + +### 2. `commitStatus` never drops an outcome silently + +`src/filter/manager.zig:1237-1250`: the id-match loop falls through with no +log when a source has no status entry (reachable via a startupPass / +web-insert race; milestone-5 policy says every non-ok state is recorded and +surfaced). Add a `log.warn` after the loop naming the source id. Unit test: +committing a status for an unknown id emits the warning and does not crash. + +### 3. Downloads and compiles leave the writer lock + +`src/filter/manager.zig:609-624`: `refreshAll` holds `writer_lock` across +every download (300 s budget each, `app.zig:85`) and every compile; every +web mutation ends in `Manager.reload` on the same lock, so an unrelated rule +save parks uncancelably — on a request path with no timeout, occupying one +of 64 web slots — until the pass ends. The download-to-tmp flow already +exists (`download` writes `.raw.tmp` via `compiledName`, with +`deleteQuietly` defers at :646-648); the lock is just taken too early. + +Restructure: + +- New `refresh_lock: std.Io.Mutex` on Manager, serializing refresh passes + against each other only. +- Split `refreshOne` (:626-729): the fetch/detect/compile stages (:650, + :659, :668) run under `refresh_lock` alone; the publish stage (:711), the + status commit and the final `reloadLocked` run under `writer_lock`. +- `refreshAll` takes `refresh_lock` for the pass, and `writer_lock` only + per-source around publish plus once for the final reload. `refreshSource` + (:582-586) follows the same split. +- The lock-ordering rule, documented on both mutex fields: `refresh_lock` is + never acquired while holding `writer_lock`. +- `refresh_lock` serializes blocklist-**directory maintenance** too, not + just refresh passes: `pruneOrphans` (:1040, invariant comment at + :1130-1136) today assumes every temp-file writer holds `writer_lock`; + once downloads write `.raw.tmp`/`.list.tmp`/`.wild.tmp` under + `refresh_lock` alone, a concurrent source deletion (blocklists.zig + delete → reload → pruneFiles) could sweep an in-flight refresh's temp + files. `pruneOrphans` therefore takes `refresh_lock` first (then + `writer_lock` if it needs it — same order as everywhere), and its + invariant comment is rewritten. Regression test: a source deletion + during a stalled refresh must not delete the stalled refresh's temp + files. + +Integration test (`filter_integration_test.zig`): with a refresh pass parked +on a deliberately stalled fixture route, a concurrent rule mutation +completes without waiting for the pass. DNS serving was never affected and +stays covered by the existing tests. + +### 4. Truncated responses are never cached + +`src/cache/dns_cache.zig:106-131`: `classify` reads `rcode` and `ancount` +and never `flags.tc`, so a TC=1 answer from a misbehaving upstream is cached +for up to `max_ttl_seconds` (86 400 s) and re-served — TC bit intact, even +over TCP, which can loop retrying clients (RFC 2181 §9). Add `if +(p.header.flags.tc) return null;` (`flags.tc` is `src/dns/header.zig:20`). + +`transport.validateResponse` is deliberately untouched: rejecting TC there +would change failover semantics for every exchange, and the forward client +legitimately reads TC for its UDP-to-TCP retry (`forward_client.zig:178`). +Record: considered, rejected. Unit test beside the existing classify tests: +a TC=1 NOERROR response classifies as null. + +### 5. The format sniffer stops eating hosts files with `##` banners + +`src/filter/parsers.zig:54-73`: `hasAbpMarker` runs before `isComment`, and +`isElementHiding` (:86-93) matches `##` (and `#@#`, `#?#`, `#$#`, `#%#`) +unanchored with no comment guard — unlike the `$` branch two lines later, +which is guarded. A hosts file with a `##` banner parses whole as ABP: +`0.0.0.0` enters the domain set and hosts lines with inline URL comments are +silently dropped (real blocking loss, reproduced with the URLhaus banner +style). The milestone-5 as-built note (specs/milestone-5.md:1677-1678) +already *claims* the separators are matched anchored — the code never was. +Guarding the branch with `!isComment` is a circular no-op (`isComment` +consults `isElementHiding`); do not attempt it. + +Fix — a positional predicate in `isElementHiding`: a separator at offset `p` +counts as element hiding only when + +- `p == 0` and the character after the separator is not whitespace, `#`, or + end of line (a generic rule `##.ad` counts; a banner `## Title`, `####`, + or bare `##` does not), or +- `p > 0` and `line[p - 1]` is not whitespace and not `#` (a rule + `example.com##.ad` counts; prose `see ## below` does not). + +Required test cases, in `parsers.zig`: the URLhaus banner style sniffs as +hosts; `##.ad-banner` sniffs as ABP; `example.com##.ad` sniffs as ABP; +`#@#exception` after a domain sniffs as ABP; a `#`-initial line containing +`##` sniffs as a comment. The fuzz target (`blocklist_fuzz.zig`) already +covers `detectFormat` for crashes. + +### 6. VACUUM respects the disk monitor + +`src/storage/retention.zig`: `runOnce` (:78) does prune → checkpoint → +(every 7th pass, :94-95) VACUUM, and takes no monitor — the word does not +appear in the file — while its two sibling tasks gate on +`disk_monitor.writesAllowed()` (logger.zig:386 as a parameter, +manager.zig:1058 as a field). VACUUM is the most expensive write the +program makes and fires unconditionally on the exact filesystem the monitor +watches, then fails SQLITE_FULL with no retry for seven daily passes. + +`runOnce` and `run` gain `monitor: ?*disk_monitor.Monitor` (the logger's +parameter shape). Only the VACUUM step gates: prune and checkpoint keep +running — they free space. A skipped vacuum increments a new +`Stats.vacuums_gated` and retries on the *next* pass (drop the modulo-only +trigger for a `passes_since_vacuum >= vacuum_every_passes` counter that +resets on success). `app.zig:561` passes the monitor like :560 does for the +logger. Tests: extend the cadence tests (:223) with a gated pass — the +vacuum is skipped, counted, and runs on the next allowed pass. + +### 7. An oversized Cookie header degrades loudly, and the session survives + +`src/web/server.zig:612-620` (`copyHeader`): a header value over the buffer +(cookie buffer = `http_util.max_cookie_len` = 1024, http_util.zig:35) +returns `""` with no log — under the documented reverse-proxy-on-shared- +domain deployment (foreign cookies riding along), every request silently +401s and the operator sees an unexplained login loop. + +`copyHeader` stays generic. The cookie call site (:510) changes: when the +raw header value exceeds the buffer, extract only the session pair by +running `http_util.cookieValue` (:200 — it already parses pairs and returns +a slice of the original header) with the existing session-cookie name +constant against the full value, copy that pair into the buffer as +`=`, and emit one `log.debug` with the dropped size (the +`.web_server` scope at :55; no secret is logged). If even the pair does not +fit, keep the empty result but still log. Integration test +(`web_integration_test.zig`): a request with a 2 KiB cookie header whose +session pair is valid is authenticated; the same header without the pair +401s. + +### 8. Live view detects fatal SSE rejections + +`web/src/features/live/useLiveQueries.ts`: per the WHATWG spec a non-200 +response fails an EventSource permanently after one error event, so the +3-consecutive-errors threshold (:29, :125-133) is unreachable on exactly the +429/401 paths it was built for — the UI shows "Reconnecting…" forever and +the capped state and session probe are dead. `FakeEventSource` has no +readyState, so tests pass — the mocked-network class again. + +- `EventSourceLike` (:9-13) gains `readyState: number`; export `const + EVENT_SOURCE_CLOSED = 2`. The browser `EventSource` satisfies it + structurally. +- In the error handler: `readyState === EVENT_SOURCE_CLOSED` is a permanent + failure — close, set `capped`, run the session probe immediately, + bypassing the counter. Transient errors (browser auto-retry pending) keep + the existing counter path. +- `fakeEventSource.ts` gains `readyState` with the real lifecycle + (CONNECTING → OPEN on `emit("open")`, CLOSED on `close()` and on + `failFatal()`, a new test helper). +- New tests: a fatal rejection (single error event, readyState CLOSED) + reaches `capped` and calls `probeSession`; a transient error still takes + three to trip. + +### 9. A timed-out DoH handshake is a handshake failure + +`src/server/doh_server.zig:311-315` counts a handshake-race timeout as +`idle_timeouts`; DoT (:313-317) counts it as `tls_handshake_failures`, +which is the recorded spec ruling. Since DoH requests have no other timer, +its `idle_timeouts` metric can only ever mean handshake stalls — the same +exported name with disjoint semantics per listener. Align DoH's +`.timed_out` arm to `tls_handshake_failures`. + +### 10. Idle DoH keep-alive connections are reclaimed + +`src/server/doh_server.zig:70`: `idle_timeout` bounds only the handshake; +its own doc comment admits requests have none. DoH stubs hold keep-alives by +design, and with no TCP keepalive a vanished peer pins one of 64 slots until +restart — while DoT on the same LAN reclaims after 10 s. + +Extend the existing race to `receiveHead` (:333), the wait for the next +request on a keep-alive connection, using the DoT out-param precedent +(readPrefix's `out_len`): a small wrapper writes the received `Request` (or +its error) through a pointer so the raced function stays `anyerror!void`. +On `.timed_out`: bump `idle_timeouts` (now truthfully named again after +ruling 9) and close the connection. Preserve the existing error triage at +:334-346 (`HttpConnectionClosing` and `ReadFailed` return uncounted; the +three structural errors count `bad_requests`/`connection_errors` as today). +The body read and `handleRequest` stay untimed, like the web listener. + +New integration-gated test mirroring the DoT idle test +(dot_server.zig:1098): a client that completes the handshake and one request +then goes quiet is closed after a short idle budget; `idle_timeouts == 1`, +`tls_handshake_failures == 0`. DoH currently has no idle test at all. + +### 11. SSE shutdown does not wait for a heartbeat + +`src/web/sse.zig`: the Hub has no shutdown signal, so graceful drain parks +up to 15 s (`heartbeat_interval`, web/handlers/live.zig:32) per idle +subscriber — the web `beginShutdown` (web/server.zig:593-605) shuts down +sockets but nothing wakes a task inside `Hub.wait`. Measured: the SSE +integration test budgets 40 s for exactly this +(web_integration_test.zig:74-75). + +- `Wake` (:33) gains `.closed`. Hub gains `closing: bool` and `pub fn + close(self: *Hub, io: std.Io) void`: under the mutex, set the flag and + `event.set` every active slot. `wait` returns `.closed` immediately when + the flag is set. +- `live.zig:114`: `.closed => return`. +- `web/server.zig` `deinit` (:349): call the hub's close (via the state's + hub pointer) immediately before `beginShutdown` (:360). +- Test: a subscriber parked in `wait` returns `.closed` promptly after + `close`; the W10 integration teardown no longer stalls (tighten + `sse_budget` only if the heartbeat assertion itself allows it). + +### 12. The API limiter's sweep runs + +`src/web/api_limiter.zig:210`: `sweep` exists, preallocates +`stale_keys` so it never allocates, is tested — and has no production +caller; only the DNS limiter got scheduled. Once 4096 distinct addresses +have been seen, the table stays full forever and every unknown-address +request pays an O(4096) eviction scan under the limiter mutex. + +`runMaintenance` (app.zig:709) gains an `api_limiter: ?*ApiLimiter` +parameter, wired at :565 from the instance built at :380-388. In the loop, +beside the two existing tasks: `_ = api.sweep(io, Clock.awake.now(io));` — +note it locks itself, unlike the DNS limiter. Test: the maintenance-loop +integration coverage asserts a stale bucket disappears. + +### 13. UDP/53 and TCP/53 stats reach /metrics + +`src/server/udp_server.zig:38-49` and `tcp_server.zig:54-60`: both stats +structs are written and read by nothing outside their own integration +tests — `dropped_no_slot`/`dropped_oversize` have no metric, no API field, +and errors log below the default level, while the DoT/DoH siblings export +equivalent counters. The module doc sells "dropped and counted"; the count +is unobservable. + +- Both gain `pub const Snapshot` + `pub fn snapshotStats` in the exact DoT + shape (dot_server.zig:61, :232). Field names stay as-is (the + `accepted`/`connections` unification is milestone-18 work). +- `WebState` (src/web/server.zig) gains `udp_listeners: []const + *udp_server.UdpServer = &.{}` and `tcp_listeners: []const + *tcp_server.TcpServer = &.{}` — S4 adds the fields with exactly these + names and types; S2 consumes them (the app builds four listeners: + udp6/udp4/tcp6/tcp4, app.zig:506-528). +- `metrics.zig`: sum each family across its listeners into one + `nxdns_udp_server_*_total` / `nxdns_tcp_server_*_total` group via the + existing `counterGroup` derivation; extend the name-assertion test + (:722-743). +- `app.zig` wires the slices. + +### 14. Forward-client counters survive the query + +`src/local/forward_client.zig:44-57`: `Stats` (queries, udp_truncated, +foreign_datagrams, failures) is instrumented, documented as preventing "an +unrecorded failure mode" — and the only production caller builds a +stack-local client per query (handler.zig:394) and drops it, so the spoofing +signal does not exist. + +`Handler.Stats` (handler.zig:130-154) gains `forward_udp_truncated`, +`forward_foreign_datagrams`, `forward_failures` (atomic, like the other 17; +queries are already counted by `forward_zone_answers`). After the exchange +in `viaForwardZone`, fetchAdd the client's counters into them. The metrics +reflection (`dns_stat_fields`, metrics.zig:48) picks the new fields up +without an edit — assert the three new `nxdns_dns_*_total` names in the +metrics test. `ForwardClient.Stats` itself stays plain and per-instance. + +### 15. The TLS server keeps the concrete transport cause + +`src/platform/tls_server.zig:418-438`: both BIO callbacks discard the +stashed cause — `net_reader.err` / `net_writer.err` (which hold +Reset/Timeout/**Canceled**, std Io/net.zig:1260/:1324) are never read +anywhere in the file — so a routine idle-budget cancel surfaces as a warn +"mbedtls_ssl_read failed" and peer resets are indistinguishable from +timeouts. The client side solved exactly this (dot_client.zig +`concreteRead`/`concreteWrite`, :303-318). + +- `ServerStream` gains `recv_cause: ?anyerror = null`, `send_cause: + ?anyerror = null`; `bioRecv`/`bioSend` stash + `self.net_reader.err`/`self.net_writer.err` on failure before returning + the mbedtls code. +- `ReadError` (:216-223) widens with `Canceled`; the read path + (`readIntoBuffer`, :334) maps a stashed `error.Canceled` to it instead of + `TlsFailed`. Callers' race harnesses already treat cancellation + separately. +- Unit tests in the file's existing stub style: a reader whose `err` is + Canceled yields `error.Canceled`; a Reset yields `TlsFailed` with the + cause stashed for logging (ruling 16). + +### 16. Peer misbehavior logs at debug, like the read path already rules + +`src/platform/tls_server.zig`: the read path deliberately logs a peer TCP +drop at debug (:362-364, "a louder level would be a log-spam vector"), then +`close` warns on close_notify against the same dead socket (:311) and +`handshake` warns per probe (:287). `.tls_server` is not in the dedup scope +set (logging.zig:103-108), so peer-driven warns can evict genuine warnings +from the rotating log. + +- `report` (:474-479) gains a level parameter. The close_notify site passes + debug when `peer_closed` is set or the stashed cause (ruling 15) is a + peer-class error; the handshake site likewise. Local/config failures keep + warn. +- Add `.tls_server` to `isDedupScope` (logging.zig:103-108) and to its + membership test (:637-644). + +### 17. The query log heals its own gaps + +`web/src/features/queries/QueryLogPage.tsx`: the hand-rolled accumulation +(`extra`, `cursorOverride`, `generation`, :79-98) develops a silent +mid-table row gap when the base page refetches after 30 s staleness — the +newest-100 boundary moves up while `extra` starts below the old cursor, and +the stale override means load-more never heals it. On a live DNS server the +trigger is routine. + +Migrate to `useInfiniteQuery`: the endpoint already maps onto it +(`getNextPageParam: (last) => last.next_before ?? undefined`; keyset +pagination confirmed server-side, handlers/queries.zig:103-114). A +background refetch then refetches all pages in order — consistent, no gap. +The three behaviors the old code carried by hand: staleness discard is +handled by the query itself; keep the `isPlaceholderData` disable on the +button; the 401 branch is already covered by the global cache-level +`handleUnauthorized` (queryClient.ts:29-30). Delete `extra`, +`cursorOverride`, `generation`, `loadingMore`, `moreError`. Rewrite the five +load-more tests; add one for the gap scenario: base refetch with new rows +between page renders leaves no discontinuity. + +### 18. Password hashing leaves the config lock + +`src/web/handlers/settings.zig`: `applyPut` holds `config_lock` from :286 +to :341, and the argon2id call (t=2, m=19 MiB, :373-391) sits inside it at +:312 — every settings GET (:407-409 takes the same lock) and every mutation +stalls for the hash duration on the Pi 5. The login path already does it +right: copy under lock, hash unlocked, re-check generation under lock +(auth.zig:47-71), and the LiveHash generation check (auth.zig:73-79) +already closes the concurrent-install race. + +Move the password-length check and the `hashPassword` call before the lock +acquisition; everything from `loadConfig` on stays inside. The hash input +is the parsed patch only, so nothing under the lock is needed. Tests: the +existing applyPut suite passes unchanged; add one that actually +distinguishes the new behavior — "both complete" already held before the +fix, the GET merely waited out the hash. A stall seam under +`builtin.is_test` (the milestone-15 rotation-seam shape) parks +`hashPassword` on an event; the test starts a password PUT, waits until +the hash is parked, completes a settings GET **while the hash is still +parked**, then releases the seam and joins the PUT. + +## Sessions + +S1-S5 run in parallel. One cross-session interface, fixed here: S4 adds the +`WebState.udp_listeners`/`tcp_listeners` fields exactly as ruling 13 names +them; S2 wires and consumes them without touching `web/server.zig`. + +### Session S1: filter + +Owns `src/filter/manager.zig`, `src/filter/parsers.zig`, +`src/filter/filter_integration_test.zig`. Rulings 1, 2, 3, 5. + +### Session S2: cache, retention, counters, wiring + +Owns `src/cache/dns_cache.zig`, `src/storage/retention.zig`, `src/app.zig`, +`src/server/udp_server.zig`, `src/server/tcp_server.zig`, +`src/local/forward_client.zig`, `src/server/handler.zig`, +`src/web/metrics.zig`, and the storage/server integration tests those +touch. Rulings 4, 6, 12, 13 (except the WebState fields), 14. + +### Session S3: TLS listeners + +Owns `src/platform/tls_server.zig`, `src/platform/logging.zig`, +`src/server/doh_server.zig`. Rulings 9, 10, 15, 16. + +### Session S4: web server + +Owns `src/web/server.zig`, `src/web/sse.zig`, `src/web/handlers/live.zig`, +`src/web/handlers/settings.zig`, `src/web/web_integration_test.zig`, plus +the ruling-13 field additions. Rulings 7, 11, 18. + +### Session S5: frontend + +Owns `web/src/features/live/*`, `web/src/features/queries/*`. Rulings 8, +17. + +### Orchestrator + +Strikes the closed findings in `TECH_DEBT.md`; updates the stale +milestone-5 as-built line if ruling 5 changed its truth value. + +## Acceptance (milestone complete) + +- [ ] Both `loadSource` catch sites propagate Canceled; the new unit test + passes; no status row ever reads "Canceled". +- [ ] `commitStatus` warns on an unknown id (test). +- [ ] With a stalled download in flight, a concurrent rule mutation + completes (integration test); the lock-ordering comment exists on + both mutexes; a source deletion during a stalled refresh leaves the + refresh's temp files alone (regression test). +- [ ] A TC=1 response classifies as null (test); `validateResponse` is + untouched. +- [ ] The five sniffer cases pass; the URLhaus banner file parses as hosts. +- [ ] A gated pass skips only the vacuum, counts `vacuums_gated`, and + vacuums on the next allowed pass (test). +- [ ] The 2 KiB-cookie tests pass: session extracted, non-session 401s, + debug line emitted. +- [ ] A fatal SSE rejection reaches `capped` and probes the session + (frontend test); a transient error still takes three. +- [ ] DoH `.timed_out` handshake counts `tls_handshake_failures`; the + metrics name test still passes. +- [ ] The new DoH idle test passes: quiet keep-alive closed, + `idle_timeouts == 1`. +- [ ] `Hub.close` wakes a parked subscriber promptly (test); web `deinit` + calls it before `beginShutdown`. +- [ ] The API limiter sweep runs in maintenance (test). +- [ ] `/metrics` carries `nxdns_udp_server_*` and `nxdns_tcp_server_*` + families summed over four listeners (name test extended). +- [ ] `/metrics` carries the three new `nxdns_dns_forward_*` counters. +- [ ] The TLS-server cause tests pass: Canceled surfaces as Canceled, never + as a warn line; close_notify after a peer drop logs debug; + `.tls_server` is in the dedup set. +- [ ] The query log uses `useInfiniteQuery`; the gap-scenario test passes; + the accumulation state is gone. +- [ ] Hashing runs before `config_lock`; the settings suite passes. +- [ ] Full suite green: `zig build test -Dintegration`, `npm run test`, + `npm run typecheck`. + +## Anti-requirements + +- No listener-core extraction, no shared repo helpers, no shared transport + helpers — milestone 18. +- No stats field renames (`accepted` vs `connections`) — milestone 18. +- No config key renames and no trusted-proxy setting — milestone 17. +- No `useInfiniteQuery` migration anywhere but the query log. +- No new metrics beyond the families this spec names. +- No timeout on DoH request bodies or handlers — only `receiveHead` races. diff --git a/specs/milestone-17.md b/specs/milestone-17.md new file mode 100644 index 0000000..3b1b5f9 --- /dev/null +++ b/specs/milestone-17.md @@ -0,0 +1,427 @@ +# Milestone 17: contract repairs + +Goal: make the operator-facing contract true. Four decided items — a real +per-query deadline, the upstream editor milestone 8 promised, an opt-in +trusted-proxy setting, and a field-level contract guard between the server +and the frontend — plus BADVERS, the validator holes, and the doc claims +the code contradicts. `TECH_DEBT.md` Theme 4. All four decisions are made; +do not re-litigate them. + +**PROVISIONAL.** Written before milestones 15-16 were built. m15 touches +`docs_drift_test.zig` and `cli.zig`; m16 touches `pool.zig`'s neighbors, +the listeners and the settings handler. Re-verify line references before +each session starts. + +## Rulings (binding) + +### 1. `total_timeout_ms` becomes a real per-query deadline + +Today the key feeds `Pool.attempt_timeout` (app.zig:283-288 → +pool.zig:110) and bounds one attempt; with N upstreams down a query takes +N × 5000 ms. The reference docs contradict each other (configuration.md +says "per-query budget", cli.md says "per-attempt") and PLAN promises a +total budget. Decision: the total budget becomes real. + +- `model.Upstream` gains `attempt_timeout_ms: u32 = 2500`; + `total_timeout_ms` keeps its name and default (5000) and becomes the + whole-exchange deadline. `read_timeout_ms` is untouched (it feeds the + forward-zone client only, app.zig:412). +- `pool.zig`: `attempt` keeps its race, now budgeted by the new key. The + two-pass failover loop is extracted into a private + `exchangeLoopLen(self, io, query, response_buf) transport.ExchangeError!usize` + (the `exchangeLen` precedent, pool.zig:690-693: `Io.concurrent` stores + the future's return value, so the raced loop returns a length and + `exchange` rebuilds the slice). A new outer race wraps that helper with + the total budget, using the select idiom already in the file. The outer + cancel unwinds an in-flight attempt — the `entry.busy` lock is taken + cancelably on purpose (:169-174) and released by defer. Outer expiry + returns `error.Timeout`, **unconditionally**: the fast-failure paths + (all upstreams disabled → `ConnectFailed`) finish long before the + budget, so no attempted-marker is needed, and a test pins that an + all-disabled pool still fails immediately rather than waiting out the + deadline. +- `validate.zig`: `checkTimeout` both keys; the cross-check becomes + `attempt_timeout_ms <= total_timeout_ms` (the old `total >= read` check, + which related knobs of different subsystems, is deleted). +- Settings plumbing for the new key (the reflective walk carries it; the + hand-written mirrors do not): `expected_keys` (model.zig:449, sorted), + the round-trip test with a non-default value, the `SettingsView` + upstream struct in web_integration_test.zig:528, `types.ts` + `Settings.upstream`, `openapi.yaml` Settings + SettingsPatch, + SettingsPage `SECTIONS[0]`, the configuration.md row (drift-guarded), + and the cli.md/configuration.md prose — both now say: total is the + per-query budget, attempt bounds one upstream try. +- Tests: a pool test with two stalling upstreams proves the exchange + returns within the total budget, not 2 × attempt; the existing + per-attempt tests keep passing against the new key. + +### 2. The validator closes its two verified holes + +- **Hostname `tls://` upstreams.** `Endpoint.parse` accepts any host + text; the DoT client then refuses non-literals on every dial + (`resolveAddress`, dot_client.zig:74-80), so a hostname `tls://` config + validates clean and fails at query time as a peer fault. Milestone 3 + explicitly ordered this check carried into the validator and it was + dropped. Correction to the audit: the function to mirror is **not** + `parseResolver` itself (validate.zig:257 — that is the forward-zone + resolver check); the upstream block in `checkCollections` + (validate.zig:603-639) gains, for `.dot` endpoints, + `net.IpAddress.parse(endpoint.host, endpoint.port) catch` → a new + `error.UpstreamHostNotIpLiteral` diagnostic ("tls:// upstreams take an + IP literal host"). `faults.zig` picks the variant up automatically. + Fix the broken example at configuration.md:348 — the hostname NextDNS + `tls://` config can never complete an exchange; show an IP-literal + `tls://` example and move the hostname form to `https://`. +- **Boot-allocation bounds.** `query_log_buffer_max` is floor-checked + only (an Entry is ~420 bytes; `maxInt(u32)` asks for ~1.8 TB at boot), + and `cache.size` is not validated at all (grep confirms). Both get the + file's ceiling-only idiom: `1..1_000_000` with the "must be at most" + message shape. No full memory-fit guarantee — out of reach, not needed. + +### 3. The upstream editor is built + +Milestone 8 ruling 9 requires upstream CRUD in the UI ("the Settings page +must edit them"); milestone 9 dropped the obligation when the SPA was +built. The entire data layer exists with zero importers +(`upstreamsQuery`, three mutation factories at queries.ts:251-264, four +api.ts wrappers). Decision: build it. Placement deviates from m8's +literal text: a dedicated **Upstreams page** (nav entry), matching the +house resource-page pattern — record the deviation beside m8 ruling 9. + +- New `web/src/features/upstreams/UpstreamsPage.tsx` + + `UpstreamForm.tsx`, modeled on the blocklists pair (the closest + analogue; copy its named idioms: editing-state-as-row, two mutation + instances of one factory so row-toggle state never bleeds into the + form, `mutateAsync` submit, full-row resend on toggle because **PUT is + a replace**, `key={editing?.id ?? "add"}` remount, `window.confirm` + delete, split error routing). +- Fields: url, priority (number), enabled (toggle), tls_name. 400s carry + the real validator's text (the handler validates via + `mutations.checkUpstream`); surface the three 409 conflicts verbatim + ("an upstream with that url already exists", "the last enabled + upstream cannot be disabled/removed"). +- Every mutation response carries `restart_required: true` and today + nothing raises the banner outside SettingsPage. Generalize the restart + banner copy to "Changes saved. Restart nxdns to apply." and raise it on + upstream create/update/delete success. +- Wiring is three edits: `createRoute` in routes.tsx, the `routeTree` + entry, the AppShell nav item. The health table stays on the Dashboard + (health rows have no `id` — url is the only join key — and reflect the + running pool, so a new upstream appears there only after restart; the + page states this beside the restart banner). +- Dead-layer cleanup in the same stroke (the same finding): delete the + seven unused single-row `get*` wrappers (correction: seven, not five — + `getGroup` and `getUpstream` are shadowed by longer names in grep), + plus `getMetrics`, `getOpenapiYaml`, and `ruleUpdateMutation`. The + editor reads rows from the list query per house idiom, so none gains a + caller. Git history keeps them. +- Page tests in the stubbed-fetch house style: list render, add, toggle + resends the full row, delete confirms, a 409 renders inline, the + banner raises. + +### 4. Trusted proxies restore real client identity + +Behind the documented same-box reverse proxy every request is loopback: +`localhost_exempt = true` (default) then disables the API limiter — the +only brake on argon2 brute force — and all remote users share one +address's 3-stream SSE cap. No X-Forwarded-For handling exists anywhere +(verified repo-wide). Decision: opt-in trusted-proxy setting. + +- New setting `web.trusted_proxies: []const u8 = ""` — a comma-separated + list of IP literals in one string (the settings codec supports scalars + only; a list type is a compile error at model.zig:358). Empty means + off: behavior today. +- Validation: each comma-separated element must parse as an IP literal → + new `error.BadTrustedProxy` diagnostic in the web block of + `checkScalars`. +- Mechanics: when the socket peer is in the trusted set, the effective + client address is the **last** valid IP literal in the request's + `X-Forwarded-For` (the entry our proxy appended); a **missing** header + falls back to the socket peer. Implementation follows the + copy-before-dispatch constraint (http_util.zig:10-14): a new + `max_xff_len = 256` budget, a new Conn buffer, and a new `Request` + field `client_addr: address.NetAddress` computed once in + `handleRequest`. The copy keeps the **tail**, not the head: a new + bounded suffix-copy helper beside `copyHeader`, because `copyHeader` + returns empty on overflow (server.zig:608) and an empty result would + fall back to the socket peer — loopback behind the intended same-box + proxy, which `api_localhost_exempt = true` (model.zig:89, the default) + then exempts. That is the exact bypass this ruling closes: a client + ships an oversized XFF through the proxy and regains the exemption. + The proxy appends `, ` last, so the real entry always fits + in the 256-byte tail. Fail closed on the residue: header present but + no valid last IP literal in the tail means the trusted proxy violated + its contract — respond 400, never fall back to the exemptible peer. + Only a misconfigured proxy can trigger that arm; a spoofed prefix + cannot, because the proxy's appended entry always terminates the + chain. `bucketLimit` (server.zig:201-207) and the + SSE acquire/release in `live.zig:82-87` both read `client_addr` — the + SSE pair must use the same key or releases leak. `api_limiter.Config` + is untouched: `WebState.web` already carries every web field + (app.zig:472), so the trust check reads `state.web`. +- With trust configured, a proxied remote address is not loopback, so + the exemption no longer swallows it — the limiter and the per-address + SSE cap bind per real client. Requests arriving at the socket from + non-trusted, non-loopback peers are unaffected. +- Settings plumbing checklist (same list as ruling 1, plus the + hand-written web mirrors the fact-finding flagged): `expected_keys`, + round-trip test, `WebView` + `view()` in handlers/settings.zig:207-248, + `restart_required_keys` balance test, `SettingsView` web struct in the + integration test, `types.ts` `Settings.web`, openapi web schemas + (including the `required` array), SettingsPage web section, the + configuration.md row, the api.md rate-limiting section, and a warning + in the proxy/authentication how-to: when proxying without + `trusted_proxies`, set `api_localhost_exempt = false`. +- Tests: unit tests on the effective-address derivation (trusted peer + + XFF chain, untrusted peer + spoofed XFF ignored, trusted peer + no + XFF, trusted peer + an XFF over 256 bytes whose valid last entry is + still honored from the tail, trusted peer + a present-but-invalid + chain → 400); an integration test proving a proxied remote address is + rate-limited while the proxy itself stays exempt. + +### 5. A field-level contract guard between server and frontend + +The REST contract lives in three hand-synced copies. The Zig side is +genuinely well guarded (56-entry contract table parsing live responses +with `.ignore_unknown_fields = false`; route-level openapi guards — which +live in `web_integration_test.zig:1494-1590` and `openapi.zig`, not in +`docs_drift_test.zig` as the audit said). **TypeScript is guarded by +nothing**: every frontend test stubs fetch, and types.ts is strictly +narrower than the wire in places (literal unions like +`Health.status: "ok" | "degraded"`), so re-parsing into Zig structs can +never catch an out-of-union string. Decision: one integration layer +asserting the frontend's consumed shapes against real responses. + +Mechanism — captured samples validated by `tsc`, no new dependencies: + +- A new `-Dintegration` test beside the contract walk drives the real + `Env` server through every route whose contract-table kind is + `.json` **and** which the frontend consumes through an `api.ts` wrapper + — the GETs **and** the JSON-returning POST/PUT wrappers (login, the + group/client/rule/upstream/blocklist writes, refresh, pause, the + settings PUT), driven with deterministic seed payloads so the mutating + responses are stable too (plus one sample per shared error response + class). GET-only would leave every write-path response contract + unguarded. Explicitly excluded: + the `.raw` routes (`/metrics`, `/api/openapi.yaml` — not JSON, and + ruling 3 deletes their unused wrappers), the `.sse` route, and the + `.none` deletes. The endpoint-to-type mapping is derived from `api.ts`, + not invented. Captured bodies are **canonicalized**: keys sorted, every + number replaced with 0 (type-preserving; strings and booleans kept — + they come from the deterministic seed). Canonicalization is what makes + the output byte-stable across runs. +- The canonical samples render into a committed file + `web/src/lib/contractSamples.gen.ts`: for each endpoint, + `export const sample_: = ;` with the matching + import from `types.ts`. TypeScript object literals get excess-property + checking, so a server field missing from types.ts, a types.ts field + missing from the wire, and an out-of-union literal all fail + `npm run typecheck` — which already runs in CI's frontend job. +- Embedding: an anonymous-import root must be Zig, not TypeScript, so a + wrapper `web/src/lib/contract_samples.zig` exports + `pub const bytes = @embedFile("contractSamples.gen.ts");` and build.zig + registers it — the exact `docs/docs.zig` pattern. The Zig test + byte-compares the embedded bytes against the freshly rendered content + and fails on mismatch. +- Regeneration is explicit, not write-beside-the-embed (embedded bytes + carry no source-tree path): a build option `-Dcontract-samples-out` + (absolute path, wired through `build_options`) makes the test write the + fresh content there instead of comparing. The pinned command, stated in + the failure message and the docs: + `zig build test -Dintegration -Dcontract-samples-out="$PWD/web/src/lib/contractSamples.gen.ts"`. + Golden-file discipline: the server side keeps the samples current; + `tsc` keeps types.ts honest against them. +- `types.ts` gains `ErrorEnvelope` matching the shared error responses + (verify the exact envelope shape against the handlers before writing + it) so the error samples are typed too. +- Recorded residual risk: the server↔openapi seam stays route-level + only. The yaml is served verbatim and never parsed; field-level yaml + drift remains possible and is accepted — revisit only if it bites. + +### 6. BADVERS becomes expressible + +RFC 6891 §6.1.3: a version-1 EDNS query must get RCODE 16 (BADVERS) with +version 0 in the reply OPT. Nothing checks `opt.version` anywhere +(verified), and the write path cannot express it: `addOptEcho` hardcodes +`extended_rcode = 0` (packet.zig:354) and `synthesize` takes a 4-bit +`types.Rcode`. Negligible operational impact; the mechanism is the +finding. + +- `edns.zig` gains the write-side splitter beside the read-side + `extendedRcode` (:272): a 12-bit rcode splits into the header's four + bits and the OPT's upper eight. `extendedRcode` stays — it is the + read-side pair (milestone 19 assumes it survives). +- `packet.zig` gains `addOptWithRcode(request_opt, do_bit, + extended_rcode: u8)`; `addOptEcho` becomes a call with 0. +- `handler.zig`: the version check sits directly after the successful + `parseOpt` (:227-233), before the opcode check — `if (o.version != 0)` + → a BADVERS synthesis (header `.no_error` + OPT `extended_rcode = 1, + version = 0`), counted in a new `Handler.Stats.badvers` (the metrics + reflection exports it; extend the name test). +- Tests: a version-1 query yields composite rcode 16 (assert via + `edns.extendedRcode`) and version 0 in the reply OPT; a version-0 + query is unaffected. + +### 7. The stale phase comments state the as-built contract + +- `rate_limiter.zig:5-6` ("Phase 7 decides the locking"): rewrite to the + as-built truth — the handler serializes with `Handler.limiter_mutex` + (handler.zig:124), uncancelable on the query path, cancelable in the + `runMaintenance` sweep (app.zig:702-731 already states the rationale + to adopt). +- `shutdown.zig` `trigger` doc ("what a Phase 8 restart endpoint would + call"): no restart endpoint exists or is planned — m8 shipped + `restart_required` echoes instead. Rewrite to "what a test uses". +- `pool.zig:83-84`: tense only — the health endpoint exists; "Feeds + `GET /api/upstream/health`." + +### 8. Doc transcripts stop hardcoding the version + +Four sites print `0.1.0-dev` (tutorial/first-run.md:108, +install-with-docker.md:126, install-with-systemd.md:206, +upgrade.md:142 — three how-to plus one tutorial, two different +transcript shapes). Correct today; silently wrong at the first tag. +Milestone 14 mandates a placeholder and a guard but names neither; the +milestone-13 executed-transcript rule is in tension. + +- Ruling on the tension, explicit: milestone-13 ruling 3 constrains + **command lines**; pasted **output lines** may carry placeholders. + The orchestrator adds one clarifying sentence to milestone-13.md + beside ruling 3. +- The four output lines replace the version with `` (e.g. + `nxdns serving on ...`). +- The guard: `docs/docs.zig` additionally embeds the tutorial and + how-to pages; a new test in `docs_drift_test.zig` asserts no embedded + doc page contains the string `0.1.0-dev` and that the four transcript + pages contain `nxdns `. (Note m15 rewrote this file's CLI + test — rebase, don't collide.) + +### 9. The log docs stop claiming append mode + +`files-and-directories.md:143` claims the log file is "opened for +append". It is not: `.mode = .write_only` plus `seekTo(state.file_pos)` +per line, offset seeded once at open — the source comment at +logging.zig:502 says so outright ("0.16.0 has no append mode"). Under +external logrotate the divergence is destructive (copytruncate → sparse +NUL-prefixed file; rename+create → unbounded writes to the renamed +inode, `max_size_mb` unenforceable). + +Docs only: correct the row (positional writes, offset tracked +in-process), and add an operator warning — nxdns owns rotation for this +file; do not point external logrotate at it. A per-line stat re-check +was considered and rejected: a syscall per log line on the hot path to +defend against a misconfiguration the docs now forbid. Record that +decision here. + +## Sessions + +Dependency graph: S1 → S2 → S5 (they share model.zig, validate.zig, +types.ts, openapi.yaml, SettingsPage.tsx, web_integration_test.zig — the +later session rebases on the earlier). S3, S4, S6 run in parallel with +the chain and each other. + +### Session S1: deadline and validator + +Owns `src/upstream/pool.zig`, `src/config/model.zig`, +`src/config/validate.zig`, `src/app.zig`, plus the ruling-1 plumbing +trail (types.ts, openapi.yaml, SettingsPage SECTIONS[0], the +integration-test SettingsView, configuration.md, cli.md). Rulings 1, 2, +and the pool comment from ruling 7. + +### Session S2: trusted proxy (after S1) + +Owns `src/web/server.zig`, `src/web/http_util.zig`, +`src/web/handlers/live.zig`, `src/web/handlers/settings.zig`, and its +plumbing trail through the same shared files S1 touched. Ruling 4. + +### Session S3: upstream editor + +Owns `web/src/features/upstreams/*` (new), `web/src/routes.tsx`, +`web/src/shell/AppShell.tsx`, `web/src/lib/api.ts`, +`web/src/lib/queries.ts`, `web/src/features/settings/RestartBanner.tsx` +and `restartBanner.ts`, `specs/milestone-8.md` (the deviation note). +Ruling 3. + +### Session S4: BADVERS + +Owns `src/dns/packet.zig`, `src/dns/edns.zig`, `src/server/handler.zig`, +the metrics name test in `src/web/metrics.zig`. Ruling 6. + +### Session S5: contract samples (after S1 and S2) + +Owns the new generator test in `src/web/web_integration_test.zig`, +`build.zig` (the anonymous import), `src/tests.zig` (if a new file needs +an entry), `web/src/lib/contractSamples.gen.ts` (committed golden), +`web/src/lib/types.ts` (`ErrorEnvelope` only). Ruling 5. + +### Session S6: docs and comments + +Owns `src/server/rate_limiter.zig` and `src/server/shutdown.zig` +(headers only), `docs/docs.zig`, `src/docs_drift_test.zig`, the four +transcript pages, `docs/reference/files-and-directories.md`, +`specs/milestone-13.md` (the clarifying sentence). Rulings 7 (except the +pool comment), 8, 9. + +### Orchestrator + +Strikes the closed findings in `TECH_DEBT.md`; confirms PLAN's +total-budget promise now matches the code; records the m8 placement +deviation. + +## Module layout + +New files: `web/src/features/upstreams/UpstreamsPage.tsx`, +`web/src/features/upstreams/UpstreamForm.tsx`, +`web/src/lib/contractSamples.gen.ts` (generated, committed). + +Deleted surface: the seven unused `get*` wrappers, `getMetrics`, +`getOpenapiYaml`, `ruleUpdateMutation`. + +## Acceptance (milestone complete) + +- [ ] Two stalled upstreams: the exchange returns within + `total_timeout_ms` (test). `attempt_timeout_ms` exists end to end: + settings key, validator range + cross-check, docs row, SettingsPage + field, SettingsView, types.ts, openapi. +- [ ] A hostname `tls://` upstream fails `nxdns check` with the new + diagnostic; the configuration.md example connects. +- [ ] `query_log_buffer_max = 4000000000` and `cache.size = 0` both fail + validation with ceiling/floor messages. +- [ ] The Upstreams page lists, adds, edits, toggles (full-row resend), + deletes; the three 409 texts render; the restart banner raises with + the generalized copy; nav and route exist; page tests pass. +- [ ] The ten dead wrappers/factories are gone; `npm run typecheck` and + `lint` pass. +- [ ] With `trusted_proxies` set, a proxied remote address is + rate-limited and holds its own SSE budget while the proxy stays + exempt; a spoofed XFF from an untrusted peer is ignored; an + oversized XFF through the trusted proxy does not regain the + loopback exemption (tests). The how-to carries the no-trust + warning. +- [ ] `contractSamples.gen.ts` is committed and contains samples for the + JSON-returning POST/PUT wrappers, not GETs only; the Zig generator + test passes against it; deliberately removing a field from a + types.ts type fails `npm run typecheck` (proven, then reverted). +- [ ] A version-1 EDNS query answers composite RCODE 16 with version 0; + `nxdns_dns_badvers_total` appears in /metrics. +- [ ] Grep: "Phase 7" and "Phase 8" appear in no source doc comment; + the rewritten headers state the mutex and the maintenance task. +- [ ] Grep: `0.1.0-dev` appears in no docs/ page; the new drift test + passes and was proven able to fail (reinsert the literal, watch it + fail, revert). +- [ ] files-and-directories.md describes positional writes and warns off + external logrotate. +- [ ] Full suite green: `zig build test -Dintegration`, `npm run test`, + `npm run typecheck`. + +## Anti-requirements + +- No restart endpoint — the `restart_required` echo model stands. +- No hostname resolution for `tls://` upstreams — the validator rejects; + the client's own check stays as defense in depth. +- No openapi codegen and no runtime schema validation library — the + contract guard is captured samples + tsc, nothing more. +- No proxy protocol (PROXY/haproxy) support; X-Forwarded-For only. +- No per-line stat re-check in the logger (ruling 9 records why). +- No upstream health join in the editor beyond the stated restart note. +- No `Forwarded` (RFC 7239) header parsing — XFF only, recorded. diff --git a/specs/milestone-18.md b/specs/milestone-18.md new file mode 100644 index 0000000..f889631 --- /dev/null +++ b/specs/milestone-18.md @@ -0,0 +1,420 @@ +# Milestone 18: collapse the duplicated infrastructure + +Goal: the copy-paste infrastructure from `TECH_DEBT.md` Theme 2 becomes +shared code — the listener core first (the audit's only high finding, four +hand-synced copies of the most invariant-heavy concurrency code in the +repository, drift already live), then the repository memory-safety +choreography, the web CRUD shells, the transport scaffolding, the name +normalization, the line iterator, and the frontend class constants. After +this milestone, a fix in any of these families lands once. + +**PROVISIONAL.** Written before milestones 15-17 were built. m16 touches the +listeners (DoH receiveHead race, stats export), the manager (refresh_lock) +and the metrics tests; m17 touches pool.zig and the handlers. Re-verify +every line reference and re-diff the copies before each session starts. + +## Rulings (binding) + +### 1. One listener core + +`tcp_server.zig`, `dot_server.zig`, `doh_server.zig` and `web/server.zig` +each carry private copies of: the `State`/`ConnState`/`Stop`/`Claim` enums, +`retry_delay`, `bump`, the slot pool with `claim`/`finish` (the same +three-step close dance), `beginShutdown` (identical body, different log +text), `serve` with the cancel-protection dance, `acceptLoop` with the same +error mapping, `decideClaim` in two shapes plus `firstFree`, and — in the +three DNS listeners — the byte-identical `Outcome`/`Result`/`race`/`expire` +select harness. `readPrefix`/`readBody`/`writeReply` are byte-identical +between tcp and dot. The predicted failure mode already fired once: the +milestone-10 review hand-ported the `handshook` TLS-context-leak fix from +dot to doh. Six unit tests are duplicated between tcp and dot, three more +between doh and web. + +New file `src/server/listener.zig`: + +- The four enums, `retry_delay`, `bump` — plain shared declarations. +- The race harness: `Outcome`, `Result`, `race`, `expire` — one copy, + generic over the raced function (the existing `comptime f: anytype` shape + is already generic; it only needs to move). +- The framing helpers `readPrefix`, `readBody`, `writeReply` (tcp/dot + consumers; doh speaks HTTP). +- `pub fn Core(comptime Cfg: type) type` generating: the conns array with + `Cfg.ConnPayload` per slot, mutex, `run_state`, `shutdown_begun`, + `claim`/`finish`/`beginShutdown`/`decideClaim`/`firstFree`, the accept + loop, and the `serve` skeleton with the cancel-protection dance. `Cfg` + supplies exactly four things: the per-slot payload type (`ConnPayload` — + the payloads genuinely differ per listener, TLS context and buffers), + the per-connection serve function (`serveConn`), the read/write buffer + sizes, and the at-capacity behavior (close for the DNS listeners; the + web listener's `refuse` 503 for web). +- The Core does **not** own the TLS lifecycle. Certificate pin and + release, TLS-context ownership, the handshake, close_notify, and + cleanup after a late race completion form one ordered sequence + (doh_server.zig:294-330, dot_server.zig:288-323) and stay inside each + listener's `serveConn`. What is shared is a helper, + `listener.handshakeStage`, that owns the `handshook` exactly-once flag + and its late-success cleanup contract — the exact defect the + milestone-10 review hand-ported — with a doc comment stating the + caller's required ordering (pin → handshake via the helper → serve → + TLS close → release, cleanup on every early exit). Both TLS listeners + adopt it; the leak class closes there, not in the Core. +- The shared unit tests move here; the per-file duplicates are deleted. + +Unifications the extraction forces, all sanctioned: + +- Counter name: `connections` everywhere. tcp and web rename `accepted`. + This renames the m16-added metric family + `nxdns_tcp_server_accepted_total` → `nxdns_tcp_server_connections_total` + — greenfield, allowed; update the metrics name test and the docs + reference. Core stats are one shared struct; `tls_handshake_failures`, + `bad_requests`, `requests` stay listener-specific beside it, and the + exported snapshots stay flat so /metrics output is unchanged except the + tcp rename. +- `deinit` shape: all four store the allocator at `listen` (dot's + milestone-10 deviation becomes the rule) and take `(self, io)`. +- The dead `pub fn serve` at doh_server.zig:687-707 (documented as + intentional in specs/milestone-10.md:286-289) is deleted; note the + update beside that spec line. + +The per-listener files keep what genuinely differs: the connection payload +(TLS context and buffers), the serve-one-connection logic, DoH's HTTP +handling, and the web listener's request router. Every existing listener +integration test must pass unchanged apart from renamed counters. + +### 2. The repository list choreography exists once + +The `prepare → ArrayList → errdefer out.deinit → errdefer freeX → +columnTextAlloc → append` shape is hand-rolled 19 times across the seven +repository files, with the load-bearing errdefer ordering repeated 18 times +and 26 hand-written `freeX` loops. The milestone-4 spec's own reference +sample (specs/milestone-4.md:1263-1272) declares the two errdefers in the +**reverse, use-after-free order** — every implementation silently corrected +it; the next repo copied from the spec is a landmine. + +`src/storage/repositories/crud.zig` (today: `execStrict` only) gains: + +```zig +pub fn listRows( + comptime Row: type, + database: *db.Db, + gpa: Allocator, + comptime sql: []const u8, + comptime readRow: fn (*db.Stmt, Allocator) db.Error!Row, +) db.Error!std.ArrayList(Row) + +pub fn freeRows(comptime Row: type, gpa: Allocator, items: []const Row) void +``` + +`listRows` owns the errdefer choreography — written once, with the ordering +comment. `freeRows` frees every `[]const u8` **and every `?[]const u8`** +field by comptime reflection — `SourceRow.checksum` is an allocated +optional slice (sources_repo.zig:91-104) and its current destructor frees +the non-null payload explicitly; a shallow slice-only reflection leaks it. +Any other owning field shape is a `@compileError`, so a future row cannot +silently leak. The allocation-failure tests must keep a case with a +non-null checksum. +The per-row `readRow` functions stay in the repos and keep their per-column +errdefers. All 19 list functions become one-line delegations; the `freeX` +wrappers stay as thin `pub` shims where callers use them, or are deleted +where `freeRows` is called directly. Every `checkAllAllocationFailures` +test stays and must pass — they now exercise the shared helper from 20 +angles. + +Correct the milestone-4 sample in place and note the correction in that +spec. + +### 3. The web CRUD shells are generated; the decisions stay hand-written + +Seven handler files repeat the 4-line `configDb` switch 40 times in three +forms, and five of them repeat identical `list`/`get`/`remove` shells. The +four reload flavors (plain; local.zig's publish-under-lock; blocklists' +pruneFiles; groups' read-back-under-lock) are genuinely different and are +**not** unified. + +- `mutations.zig` gains `pub fn requireConfigDb(state: *server.WebState) + error{NoConfigDb}!*db.Db`. The 40 switch sites become one-line `catch` + arms producing the same three response forms (the Failure payload is the + same constant text everywhere). +- `mutations.zig` gains a comptime resource descriptor generating only the + identical trio: + + ```zig + // desc is an anonymous struct literal; `anytype` is not legal as a + // struct *field* type, so the descriptor arrives as `comptime desc: + // anytype` and is validated field-by-field at comptime (@hasField + + // @compileError on a missing or mistyped member). + pub fn Resource(comptime desc: anytype) type + // desc members: Row: type; list / get / remove: the repo fns (get and + // remove may be `null` for list-only resources); label: []const u8 + // ("an upstream", used in respondFailure contexts); envelope: + // []const u8 ("upstreams", the JSON list key). + ``` + + yielding `list`/`get`/`remove` handlers in the exact current shape. + Adopted by upstreams, groups, blocklists, clients, rules and the two + local.zig sets where the shell matches; `settings.zig` (get + applyPut + only) does not adopt. All `applyCreate`/`applyUpdate`/`applyDelete` + bodies and the per-resource decision functions (countEnabledExcept, + updateLocked/deleteLocked, pruneFiles, applyReplacePrefixes, publish, + RuleView, applyPut) stay hand-written. + +Acceptance is a grep: the 4-line switch appears zero times outside +`mutations.zig`. + +### 4. The transport scaffolding is shared, and DoH gets its missing unwrap + +- The byte-identical `Outcome` + `expire` + select-race body in + `pool.zig:253-274/310-317` and `forward_client.zig:195-215/260-267` + moves to `transport.zig` as one generic exchange-race helper (the raced + function, the budget and one comment word are the only differences + today). `manager.zig`'s `fetchWithin` may adopt it if it generalizes + without contortion; not required. +- `mapPhase` (duplicated byte-for-byte, dot_client.zig:283 / + forward_client.zig:293) moves to `transport.zig` as `pub`. +- The cancel-protected close helpers (four near-copies: + forward_client.zig:281/287, dot_client.zig:271/277) become one + `pub fn closeBlocked(io: std.Io, target: anytype) void` in transport.zig + (`target.close(io)` under swapped protection). The inline copies in + logging.zig and metrics.zig are out of scope — they are not transport + code. +- `doh_client.zig` gains the stashed-cause unwrap the other two transports + carry (`concreteRead`/`concreteWrite` precedent, dot_client.zig:303-318): + today its `mapError` never reads the http client's stashed cause behind + `ReadFailed`/`WriteFailed`, so rare mid-exchange local errors (e.g. + ENOBUFS) are recorded as peer faults against a healthy upstream — a + stated spec-invariant violation, fixed once for DoT and left in DoH. + Port the unwrap and the corresponding stub-based tests. +- `sendFailure`/`receiveFailure` stay per-file: the unwrapping genuinely + differs by stream type. + +### 5. `normalizeName` lives beside `fromText` + +The copies in `records.zig:197` and `forward_zones.zig:129` are +byte-identical (only doc comments differ). Move the function to +`src/dns/name.zig` as `pub fn normalizeText(text: []const u8, buf: +*[types.max_name_len]u8) error{BadName}![]const u8`, next to `fromText` +(both callers already import the module; it stays pure — no allocation, no +Io). Both files delete their copies and their private `NameError`. + +The other three variants are **not** unified — their policies differ on +purpose (rules.zig:195 rejects controls and space but skips `fromText` +because patterns hold `*`; compiler.zig:155 adds a two-label minimum and +counter-based reporting; dns_cache.zig:59 validates nothing because its +input is asserted). Each of the three gains a one-line comment pointing at +`name.normalizeText` and naming its own policy difference, so the next +reader knows the divergence is intentional. + +### 6. One bounded-line iterator + +The subtle `takeDelimiter`/`StreamTooLong`/`discardDelimiterInclusive` loop +exists twice (`compiler.zig:64-87`, `manager.zig collectSample:1423-1441`), +both correct, both carrying the infinite-loop-hazard comment and a +regression test. Their seven behavioral differences (termination, counting, +trimming, filtering, exit style, error set, arm order) all live *outside* +the hazardous core. New shared iterator in `src/filter/parsers.zig`: + +```zig +pub const LineEvent = union(enum) { line: []const u8, long_line }; +pub fn nextBoundedLine(r: *std.Io.Reader, max_len: usize) error{ReadFailed}!?LineEvent +``` + +encapsulating the take/discard dance (including the +EndOfStream-during-discard arm and the large-reader-buffer length check). +The limit is a **parameter** — `parsers.zig` must not import `compiler.zig` +(compiler imports parsers, so that is a cycle, and parsers.zig is a +standalone std-only fuzz-module root); both callers pass +`compiler.max_line_len` from their side. `compile` keeps its `long_lines` counting +and `\r` handling on top; `collectSample` keeps its trim/filter/count-limit +on top. Both regression tests must still pass; the m15 compiler fuzz target +now exercises the shared core. + +### 7. `cli.zig` stops re-spelling app policy + +- The DoH buffer sizes: `app.zig:89-90` names + `doh_request_buf_len = 1024` / `doh_transfer_buf_len = 4096` (private); + `cli.zig:885-886` re-spells them as bare literals, so changing the + constants would leave `nxdns check` probing different buffers than + `nxdns run` uses — undermining the probe's stated purpose. The constants + move to `doh_client.zig` as `pub const default_request_buf_len` / + `default_transfer_buf_len`; both `app.zig` and `cli.zig` use them. No + deeper unification of `probeUpstreams` vs `Upstreams.build` — the + one-at-a-time probe and the slab build are intentionally different + shapes. +- The ZON failure channels: `check` prints the multi-line `zon_diag` + rendering inline in a single `FAIL` line (cli.zig:721-729), embedding + newlines mid-record and contradicting the one-line-per-problem promise + (docs/reference/configuration.md:332); `import` routes through + `reportParseFailure` (config/import.zig:167 — currently **private**). + Make `reportParseFailure` `pub`; `check` builds a local + `validate.Diagnostics`, feeds the parse failure through it, and renders + each problem as its own `FAIL` line like its other diagnostics. + Acceptance: a config with a multi-line ZON error yields one `FAIL` line + per parser message from both `check` and `import`. + +### 8. `build.zig` wires a test suite once + +The host (lines 47-69) and aarch64 (150-179) test suites repeat twelve +wiring lines; the aarch64 triple is re-spelled at :152 instead of read from +`cross_targets` (:10-13). Extract `fn addTestSuite(b, target, optimize, +options, web_assets) *Step.Compile` mirroring the existing `addExecutable` +helper; the aarch64 call resolves its target from `cross_targets[1]` (or a +named constant both use). The two blocks' three real differences +(`linkage = .static`, `skip_foreign_checks`) stay at the call sites. The +m15 fuzz-suite wiring repeats the same shape three times — fold those calls +into the helper too if the anonymous-import needs line up; otherwise leave +them and say so. + +### 9. One Smith encoder + +`sliceInput` is byte-identical in `blocklist_fuzz.zig:160` and +`corpus.zig:123` (blocklist_fuzz cannot import corpus.zig — corpus imports +the `dns` module its build target lacks; that is why it was copied). New +dependency-free `tests/fuzz/smith_encode.zig` holding `sliceInput` and its +length self-test; `corpus.zig` and `blocklist_fuzz.zig` import it by +relative path (each fuzz module compiles its own copy — source-level dedup +is the goal). `pairInput` stays in blocklist_fuzz, `sliceIntInput` stays in +corpus — both build on the shared one. The m15 fuzz files +(`compiler_fuzz.zig`, `http_util_fuzz.zig`) adopt it where they inlined +the idiom. + +### 10. Frontend: one class vocabulary, shared form plumbing + +15 of 30 non-test `.tsx` files re-declare Tailwind class constants in two +naming conventions; the primary-button literal appears at 7 sites, the +input literal at 6, the focus-visible fragment 39 times across 17 files — +and the two inputs that *dropped* the focus ring (PrefixesEditor.tsx:13, +GroupsPage.tsx:17, the only unfocusable inputs in the app) silently violate +the milestone-9 accessibility floor. The project's own precedent +(InlineError was hoisted during milestone 9) is house style left +unapplied. + +- New `web/src/ui/classes.ts` exporting the named constants (camelCase, one + convention): `inputClass`, `buttonClass`, `primaryButtonClass`, + `thClass`, `tdClass`, `formCardClass`, `tableWrapClass`, + `retryButtonClass` — taken verbatim from the current majority literals + (all carrying the focus ring). All 15 declaring files adopt; the two + ring-less inputs are fixed by adoption. Acceptance is a grep: the three + top literals appear only in `ui/classes.ts`, and every + input/button/select in `web/src` carries the focus-visible fragment (the + one sanctioned variant is PauseWidget's negative offset). +- New `web/src/ui/useCrudForm.ts`: the mutation trio + `FormState` + + `openForm`/`onSubmit`/`onDelete` plumbing that RecordsTab and ZonesTab + share byte-for-byte, parameterized over the entity and its three + mutation factories. Both tabs adopt it; their field JSX and tables stay + local. Other pages may adopt where the shape fits; none is forced. +- The shadowing `InlineError` in DashboardPage.tsx:36: extend + `lib/InlineError.tsx` with an optional `onRetry` prop rendering the + retry button; DashboardPage imports it and the local copy is deleted. + +## Sessions + +S1-S7 run in parallel; no two sessions write the same file. Interfaces +fixed here: S4 makes the two DoH buffer constants `pub` in +`doh_client.zig`; S5 consumes them from `cli.zig`/`app.zig` — wait for S4's +commit of that one declaration or agree the exact names above and build +against them. + +### Session S1: listener core + +Owns `src/server/listener.zig` (new), `src/server/tcp_server.zig`, +`src/server/dot_server.zig`, `src/server/doh_server.zig`, +`src/web/server.zig`, `src/web/metrics.zig`, and the listener integration +tests (`tcp_server_integration_test.zig`, +`udp_server_integration_test.zig`, `src/web/server_integration_test.zig`, +`resolver_integration_test.zig`, `phase7_integration_test.zig` as needed). +Ruling 1. + +### Session S2: storage + +Owns `src/storage/repositories/*` and the milestone-4 spec correction. +Ruling 2. + +### Session S3: web handlers + +Owns `src/web/handlers/*`, `src/web/web_integration_test.zig`. Ruling 3. + +### Session S4: transport + +Owns `src/upstream/transport.zig`, `src/upstream/pool.zig`, +`src/upstream/dot_client.zig`, `src/upstream/doh_client.zig`, +`src/local/forward_client.zig`. Ruling 4, and the `pub` constants half of +ruling 7. + +### Session S5: names, lines, cli + +Owns `src/dns/name.zig`, `src/local/records.zig`, +`src/local/forward_zones.zig`, `src/filter/rules.zig`, +`src/filter/compiler.zig`, `src/filter/manager.zig`, +`src/filter/parsers.zig`, `src/cache/dns_cache.zig` (comment only), +`src/cli.zig`, `src/config/import.zig`, `src/app.zig`. Rulings 5, 6, and +the consumer half of 7. + +### Session S6: build and fuzz + +Owns `build.zig`, `tests/fuzz/*`. Rulings 8, 9. + +### Session S7: frontend + +Owns `web/src/**`. Ruling 10. + +### Orchestrator + +Strikes the closed findings in `TECH_DEBT.md`; records the tcp metric +rename in the docs reference; updates the milestone-10 deviation note +(ruling 1) and the milestone-4 sample (ruling 2). + +## Module layout + +New files: `src/server/listener.zig`, `tests/fuzz/smith_encode.zig`, +`web/src/ui/classes.ts`, `web/src/ui/useCrudForm.ts`. + +Deleted surface: the four per-listener copies of the shared machinery, the +dead `doh_server.serve`, the two `normalizeName` copies and their +`NameError`s, the duplicated listener unit tests, the DashboardPage +`InlineError`. + +## Acceptance (milestone complete) + +- [ ] All four listeners build on `listener.Core`; every listener + integration test passes; /metrics output is byte-identical to before + except `nxdns_tcp_server_connections_total` (name test updated). +- [ ] The `handshook` invariant exists once, in `listener.handshakeStage`; + grep finds no per-listener copy; both TLS listeners call it. +- [ ] Grep: `errdefer out.deinit` appears only in `crud.zig`; all 20+ + `checkAllAllocationFailures` tests pass; the milestone-4 sample shows + the safe order. +- [ ] Grep: the `configDb` switch appears zero times outside + `mutations.zig`; all web integration tests pass; the four reload + flavors are byte-unchanged. +- [ ] `pool.zig` and `forward_client.zig` share the race helper; `fn + expire(` production copies are gone (test-local copies may stay). +- [ ] DoH's mapError unwraps stashed causes; the ported stub tests pass. +- [ ] `records.zig` and `forward_zones.zig` call `name.normalizeText`; the + three intentional variants carry their pointer comments. +- [ ] Both line-iterator call sites use `nextBoundedLine`; both regression + tests pass. +- [ ] `nxdns check` renders a multi-line ZON failure as one `FAIL` line per + message (test); the buffer constants exist once, `pub`, in + `doh_client.zig`. +- [ ] `build.zig` has one `addTestSuite`; the aarch64 triple is read from + `cross_targets`; the qemu CI job passes. +- [ ] `sliceInput` exists once; both fuzz corpora still decode (the length + self-test runs from `smith_encode.zig`). +- [ ] Grep: the three top class literals live only in `ui/classes.ts`; + every input in `web/src` carries the focus-visible fragment; + RecordsTab and ZonesTab use `useCrudForm`; DashboardPage imports the + shared InlineError. `npm run test`, `typecheck`, `lint` green. +- [ ] Full suite green: `zig build test -Dintegration`. + +## Anti-requirements + +- No behavioral changes: this milestone moves code. The only sanctioned + behavior deltas are the tcp counter rename, DoH's cause unwrap, the two + restored focus rings, and the `check` ZON rendering — each named above. +- No unification of the three intentionally-divergent normalize policies. +- No unification of the four reload flavors. +- No `web/src/ui/` component library beyond `classes.ts` and + `useCrudForm.ts` — no button/table/dialog components in this milestone. +- No listener behavior additions (timeouts, keepalive) — m16 finished + those. +- No renaming of exported metric families beyond the one tcp rename. diff --git a/specs/milestone-19.md b/specs/milestone-19.md new file mode 100644 index 0000000..9d1904f --- /dev/null +++ b/specs/milestone-19.md @@ -0,0 +1,422 @@ +# Milestone 19: hygiene sweep + +Goal: remove the dead surface, unify the re-hardcoded constants, close the +small silent failures, and fix the frontend state hazards — the residue of +`TECH_DEBT.md` Theme 8 plus the remaining lows, after the behavioral fixes +(m16), the contract work (m17) and the duplication refactors (m18) have +landed. + +**PROVISIONAL.** This spec was written before milestones 16-18 were built. It +assumes: m17 shipped BADVERS (so `edns.extendedRcode` has a production +caller), m17 built the upstream editor (so the queries.ts upstream factories +have consumers and are not deleted here), and m18 shipped the frontend `ui/` +extraction and deleted doh_server's dead `serve()`. Before starting this +milestone, re-verify every line reference and update this spec where the +earlier milestones moved things. + +## Rulings (binding) + +### 1. The dead ECS parse surface is deleted + +`src/dns/edns.zig`: delete `parseEcs` (:111), `Ecs` (:98-105), `EcsError` +(:107), `ecs_family_ipv4`/`ecs_family_ipv6` (:95-96), and their in-file tests +(:371-:469 block, nine tests). `stripEcs` filters by option code without +decoding, forward mode keys on raw bytes — no production path decodes ECS. + +Trap the audit missed: `tests/fuzz/dns_fuzz.zig:86` calls `edns.parseEcs` +inside a fuzz target that the default `test` step builds. Delete that line in +the same change or the build breaks. + +`extendedRcode` (:272) is **kept**: milestone 17 gave it the BADVERS caller. +If m17 diverged and BADVERS was not built, delete it too and note the +divergence here. + +Precedent: `ecsPayload` was built and removed within milestone 7 when it lost +its only caller (specs/milestone-7.md:361). Git history keeps the code. + +### 2. `writeSettings` returns an error union + +`src/web/handlers/settings.zig:349` returns `?db.Error`, so every exit is a +value return and the `errdefer` at :354 never fires — it implies protection +it cannot provide. The real safety is two explicit `tx.rollback()` calls +(:358, :363 — the audit said three; it is two). + +Change the return type to `db.Error!void`. The errdefer becomes live +(`db.Tx.rollback` at storage/db.zig:814 is idempotent and documented safe in +an errdefer), and the two manual rollback-then-return arms collapse into +plain `try`/error returns. The one caller (`applyPut`, :327) switches from +`if (writeSettings(...)) |err|` to `catch`; its hash-free-on-failure behavior +is unchanged. + +### 3. The logger's field widths become the single source + +`src/storage/logger.zig:44-48` holds four file-private constants +(`max_domain_len = 253`, `max_client_len = 45`, `max_reason_len = 32`, +`max_upstream_len = 64`). Make all four `pub`. Then: + +- `src/server/handler.zig:67` deletes its local `max_reason_len = 32`; the + comptime guard at :69-75 checks `logger.max_reason_len`, so the guard + finally proves what its comment claims. +- `src/server/handler.zig:79` deletes its local `max_ip_text = 45` and uses + `logger.max_client_len` (the :82 `max_resolver_text` derivation follows). +- `src/server/clients.zig:32` likewise. +- `src/web/handlers/queries.zig:27-30` — the fifth copy the audit missed — + adopts `logger.max_domain_len` for the domain width. Its `max_client_len` + is **64**, not 45; the query log's client column is written by the logger + and can never exceed 45 bytes, so it adopts `logger.max_client_len` too. + If a test proves a wider value ever reaches that field, stop and record + the divergence instead of forcing it. + +Acceptance is a grep: the literals 253, 45 and 32 appear in these roles only +in logger.zig. + +### 4. `track()` reports fullness; one mutex acquisition per query + +`src/server/handler.zig:255-258` takes the tracker mutex twice per query: +`tracker.track(io, from)` and then `tracker.snapshotStats(io)` — a full +struct copy — solely to mirror `dropped_full` into the handler's atomic +`tracker_full` (:150-153). + +Change `Tracker.track`/`trackAt` (`src/server/clients.zig:89-97`) to return +`u64`: the current `dropped_full`, read under the mutex they already hold. +The handler stores that return value. `snapshotStats` stays for /metrics. + +### 5. Log truncation gets a marker and a counter + +`src/platform/logging.zig:285-288`: `mw.print(format, args) catch {}` on a +fixed 2048-byte writer, then the partial buffer is used with no marker — the +module's own never-both-discarded-and-silent invariant, violated in its own +kitchen. On `error.WriteFailed`: overwrite the final three bytes of the +buffered message with `...` (the safe_url.zig marker, src/safe_url.zig:11-14) +and increment a new `state.stats.lines_truncated`, sibling to +`lines_deduped` (:295). One test: a message over 2048 bytes ends in `...` +and bumps the counter by one. + +### 6. TLS errors are classified by name, not by name prefix + +`src/filter/fetcher.zig:169-170` and `src/upstream/doh_client.zig:160-161` +classify TLS failures with `startsWith(@errorName(err), "Tls")` / +`"Certificate"` — a std rename silently downgrades TLS failures into generic +buckets. Verified reachable set from `std.http.Client` (0.16.0) **today**: +exactly `error.TlsInitializationFailed` and +`error.CertificateBundleLoadFailure`. But this milestone lands after +milestone 18, whose ruling 4 makes the DoH path unwrap the client's stashed +read cause — and that cause set includes the record-layer members +(`TlsAlert`, `TlsBadRecordMac`, `TlsDecodeError`, ...) from +`std.crypto.tls.Client`. A two-member switch written against today's +surface would downgrade those to generic receive failures. + +In both files: replace the prefix match with an exact switch → `error.TlsFailed` +naming the two top-level members **plus**, on whichever paths m18's cause +unwrap surfaces, the `Tls*` members of the unwrapped read-cause set — +enumerated at implementation time from the pinned std sources against the +m18 code as landed, not from this spec's list. The `else` falls through to +the existing per-phase mapping. Rewrite both tests (fetcher.zig:311-313, +doh_client.zig:261-263): they currently assert on `CertificateExpired`, +which nothing can produce, and omit `CertificateBundleLoadFailure`; the new +tests use only names from the verified reachable set, including a stashed +record-layer cause on the unwrapped path. + +If m18 already extracted a shared `mapError` into transport.zig, apply this +ruling to the shared copy instead and note it here. + +### 7. The transcribed mbedTLS values get an init-time check + +`src/platform/tls_server.zig:501-511` hand-transcribes four config enums and +six error codes from the pinned Mbed TLS 3.6.7 headers with no drift guard — +unlike the sizes and alignment, which the shim reports. A renumbered +`close_notify` after a version bump would silently invert truncation +detection (:286, :351). + +`src/platform/mbedtls_shim.c` gains ten getters in the existing shape +(`int nx_const_ssl_transport_stream(void) { return MBEDTLS_SSL_TRANSPORT_STREAM; }` +etc. — the needed headers are already included). `tls_server.zig` verifies +all ten against the transcribed constants next to the existing alignment +assert at :78, and the :657 test block asserts them too. Cheap insurance, +one-time cost. + +### 8. The reload lock invariant is written down + +Fourteen call sites in four handler files (groups, rules, clients, +blocklists — the audit said five handlers; it is four files, fourteen sites) +call `mutations.reload` *after* releasing `config_lock`. That is safe only +because the production `reload_fn` re-reads the database under the manager's +own writer lock — a cross-module fact stated nowhere. `local.zig` is the +deliberate opposite (publish under the lock; the ordering rationale lives at +local.zig:80-85). + +Extend the doc comment on `mutations.reload` +(src/web/handlers/mutations.zig:122-125): the `reload_fn` contract is that it +re-reads all state from the database itself; it must never accept pre-read +rows (contrast `swapLocalTables`, which is the pre-read shape and is why +local.zig holds the lock). A future signature change to `reload` that adds +row-passing breaks fourteen call sites' correctness — the comment must say +exactly that. Documentation only; no locking change. + +### 9. `web_dev_dir` moves into WebState + +`src/app.zig:605-609` holds the `--web-dev` directory in a module-level +mutable global because the fallback handler "has no closure" — but +`serveWebDev` (:613-620) receives `*WebState` and discards it. Add +`dev_dir: []const u8 = ""` to `WebState` (src/web/server.zig, beside +`version`), set it at the composition root, read it in `serveWebDev`, delete +the global and its apologia comment. + +### 10. The private key PEM is wiped + +`src/server/cert_store.zig:326` frees the key PEM without zeroization, and +`readPem` (:345-362) uses `readFileAllocOptions`, whose grow-as-you-read can +leave intermediate copies in freed pages — a wipe at the call site cannot +reach them. Split the key path: a `readKeyPem` that reads through a single +fixed `max_pem_bytes + 1` buffer (no reallocation), copies to an exact-size +allocation, and `std.crypto.secureZero`s the big buffer before returning; the +caller wipes the returned slice before `gpa.free`. The idiom precedent is +auth.zig:228. The certificate PEM path stays as-is — it is public material. +Runs at boot and on real renewals only (:311-312 stats gate the poll). + +### 11. Header-array overflow asserts + +`src/web/http_util.zig:335` maps a too-long `extra_headers` onto +`error.OutOfMemory` — a future eighth header would surface as mysterious +OOM-labeled drops. Every one of the 62 call sites passes a compile-time +count, maximum 3 (static.zig:174, +1 for content-type = 4 of 8). Replace the +check with `std.debug.assert(extra_headers.len + 1 <= headers.len);` so the +mistake fails loudly in tests. + +### 12. Shipped `.gz` siblings are verified; orphans are rejected + +`tools/gen_web_assets.zig:71-77` embeds a dist-shipped `.gz` sibling with +zero content checks, and an orphan `.gz` (no base file) is silently embedded +as unreachable `application/octet-stream` bytes. Exposure is latent (no +frontend compression plugin today) — close it while it is cheap: + +- Sibling: decompress with `std.compress.flate.Decompress` (container + `.gzip`; the tool already uses the Compress side at :133) and byte-compare + against the base file; `std.process.fatal` on mismatch, matching the + tool's existing error style (:73-75). +- Orphan: `std.process.fatal` naming the path, instead of indexing it. + +Extend the embedded-dist test (`src/web/static.zig:413-439`) to decompress +each `.gz` entry and compare with its base — it currently checks magic bytes +and size only. + +### 13. The Dockerfile arch default comes from the host + +`deploy/docker/Dockerfile:21`: `${TARGETARCH:-amd64}` — under the legacy +builder TARGETARCH is empty, so a plain `docker build` on the Pi 5 packages +the x86_64 binary and dies at `docker run` with exec-format, far from the +mistake. Replace the default with a `uname -m` mapping resolved before the +case: + +```dockerfile +RUN arch="${TARGETARCH:-}"; \ + if [ -z "$arch" ]; then case "$(uname -m)" in \ + x86_64) arch=amd64 ;; aarch64) arch=arm64 ;; \ + *) echo "unsupported build host $(uname -m); use buildx" >&2; exit 1 ;; \ + esac; fi; \ + case "$arch" in ...existing arms... esac +``` + +The existing unsupported-arch arm stays fail-fast. + +### 14. Frontend: the settings registry is typed + +`web/src/features/settings/SettingsPage.tsx` — `SectionDef` exists (:21) but +`FieldDef.key` is `string` (:16), driving `as Record` casts +at :217, :226, :261. A typo'd key compiles and breaks the field. Make the +pair generic: + +```ts +interface FieldDef { + key: keyof Settings[S] & string; + kind: "number" | "text" | "boolean" | readonly string[]; +} +interface SectionDef { section: S; title: string; fields: readonly FieldDef[]; } +function defineSection(def: SectionDef) { return def; } +``` + +`SECTIONS` (:35-118, 12 sections) becomes a tuple of `defineSection(...)` +calls, `TLS_FIELDS` (:27) becomes `FieldDef<"doh_server" | "dot_server">[]` +compatible, and the three `Record` casts are deleted. The +per-branch value casts inside `FieldRow` (:141, :160, :174, :198) may stay — +they are narrowing on `kind`, not drift holes. Acceptance: renaming a +Settings field makes `tsc` fail on the registry. + +### 15. Frontend: the settings baseline is frozen + +Same file, :220 — `buildSettingsPatch(data.settings, edited, ...)` diffs the +mount-time clone against **live** query data, so a background refetch makes +out-of-band changes appear as user edits and Save silently reverts them. Add +a `baseline` state initialized with the same `structuredClone` and reset in +the same `onSuccess` (:234-241) where `edited` resets; the patch becomes +`buildSettingsPatch(baseline, edited, ...)`. `settingsDiff.ts` is unchanged. + +### 16. Frontend: refresh status leaves the query cache + +`web/src/features/blocklists/BlocklistsPage.tsx:33` reads the refresh +snapshot with non-subscribing `getQueryData` from a cache entry that has no +queryFn, no subscriber, and the default 5-minute gcTime — after which the +page claims no refresh ever ran. This is client UI state. New module +`web/src/features/blocklists/refreshStore.ts` in the exact shape of +`settings/restartBanner.ts` (module-level value + listener Set + +`useSyncExternalStore` hook), holding `SourceStatus[] | null`. The mutation +(`blocklistsUpdateNowMutation`, lib/queries.ts:156) writes the store instead +of `setQueryData`; the page subscribes via the hook; the +`queryKeys.blocklistSources` entry and its doc comment are deleted. Note the +prefix-invalidation side effect disappears with it (invalidateBlocklistWorld +invalidating `["blocklists"]` used to mark the entry stale — harmless, since +the entry could never refetch). + +### 17. Frontend: one default-group helper + +Four encodings of "group 1", two semantics (GroupsPage.tsx:14 named +constant; PrefixesEditor.tsx:21 and LookupPage.tsx:133 byte-identical +prefer-id-1 expressions; RulesPage.tsx:24 first-of-list — which, because the +API orders by name (groups_repo.zig:148), preselects the alphabetically +first group, not the default). New module `web/src/lib/defaultGroup.ts`: + +```ts +export const DEFAULT_GROUP_ID = 1; +export function defaultGroupId(groups: readonly Group[]): number { + return groups.find((g) => g.id === DEFAULT_GROUP_ID)?.id ?? groups[0]?.id ?? DEFAULT_GROUP_ID; +} +``` + +All four sites adopt it; RulesPage's preselect bug goes away with the +adoption. + +### 18. Frontend: the dashboard primes without throwing + +`web/src/routes.tsx:97-102` — the dashboard loader's `Promise.all` over four +`ensureQueryData` calls means one failing endpoint blanks the whole page with +`RouteError` on cold navigation, defeating the page's own per-widget +degrade (DashboardPage.tsx:68-96, which uses `useQuery` precisely to allow +it). Change **only the dashboard loader** to `Promise.allSettled`. The five +other `Promise.all` loaders stay: their pages use `useSuspenseQuery` and have +no granular fallback to preserve. + +### 19. Frontend: the query-key table is complete + +`web/src/lib/queries.ts` — eight raw literals, all in this file: seven +`["lookup"]` prefix-invalidations (:99, :125, :132, :159, :167, :189, :211) +and one `["groups"]` (:133). Add `lookupAll: ["lookup"] as const` to +`queryKeys`, use it at the seven sites, and use `queryKeys.groups` at :133. +Acceptance: no `["lookup"]`/`["groups"]` literal outside the table. + +### 20. Frontend: the form catch swallows only the expected class + +`web/src/features/blocklists/BlocklistForm.tsx:21-33` — the bare `catch {}` +wraps both the `await onSubmit` and the reset `setState` calls; anything but +the mutation rejection dies silently with no console trace. House precedent +is auth/store.tsx:85-89 (swallow only the 401 it expects, rethrow the rest), +pinned by two tests. Restructure: `try { await onSubmit(...) } catch (error) +{ if (error instanceof ApiError) return; throw error; }`, resets after the +try. The page's `` keeps rendering the mutation error as today. + +## Sessions + +S1-S4 run in parallel; no two sessions write the same file. + +### Session S1: backend surface + +Owns `src/dns/edns.zig`, `tests/fuzz/dns_fuzz.zig`, +`src/web/handlers/settings.zig`, `src/web/handlers/mutations.zig`, +`src/web/http_util.zig`, `src/server/cert_store.zig`, `src/app.zig`, +`src/web/server.zig`. Rulings 1, 2, 8, 9, 10, 11. + +### Session S2: constants, counters, classification + +Owns `src/storage/logger.zig`, `src/server/handler.zig`, +`src/server/clients.zig`, `src/web/handlers/queries.zig`, +`src/platform/logging.zig`, `src/filter/fetcher.zig`, +`src/upstream/doh_client.zig`, `src/platform/tls_server.zig`, +`src/platform/mbedtls_shim.c`. Rulings 3, 4, 5, 6, 7. + +### Session S3: frontend + +Owns everything under `web/src/`. Rulings 14-20. + +### Session S4: tools and deploy + +Owns `tools/gen_web_assets.zig`, `src/web/static.zig`, +`deploy/docker/Dockerfile`. Rulings 12, 13. + +### Orchestrator + +Re-verifies this spec's line references against post-m18 reality before S1-S4 +start; strikes the closed findings in `TECH_DEBT.md` after. + +## Module layout + +New files: + +- `web/src/features/blocklists/refreshStore.ts` — refresh snapshot store. +- `web/src/lib/defaultGroup.ts` — default-group constant and helper. + +Deleted surface: `parseEcs`/`Ecs`/`EcsError`/`ecs_family_*` (edns.zig), the +`web_dev_dir` global (app.zig), the `queryKeys.blocklistSources` entry +(queries.ts). + +## Acceptance (milestone complete) + +- [ ] `parseEcs` and its family are gone; `zig build test` compiles and + passes (the dns_fuzz caller was removed in the same change). +- [ ] `writeSettings` returns `db.Error!void`; the errdefer is the only + rollback path; the settings PUT integration tests still pass. A fault + test forces a failure after `Tx.begin` (a fault seam under + `builtin.is_test`, the milestone-15 rotation-seam shape) and proves + the errdefer rolled the transaction back: a subsequent `Tx.begin` on + the same connection succeeds. +- [ ] Grep proves the field widths exist once: 253/45/32 in their logger + roles appear only in logger.zig. +- [ ] One tracker-mutex acquisition per query: `track` returns + `dropped_full`; `/metrics` output for the tracker family is unchanged. +- [ ] An oversized log message ends in `...` and increments + `lines_truncated`; the new test pins both. +- [ ] Both TLS mapError switches name exactly the two reachable errors; the + rewritten tests use reachable names only. +- [ ] The ten mbedTLS constants are verified at init against shim getters; + a deliberately wrong transcription fails the :657 test block. +- [ ] `mutations.reload`'s doc comment states the re-read contract and the + fourteen-call-site consequence. +- [ ] `WebState.dev_dir` replaces the global; `--web-dev` serving still + works (manual check with a dev dir). +- [ ] The key PEM path reads through a fixed buffer and is wiped before + free; no `readFileAllocOptions` remains on the key path. +- [ ] `respondBytes` asserts instead of returning OutOfMemory. +- [ ] gen_web_assets rejects a corrupted shipped sibling and an orphan + `.gz` (both proven with a doctored dist in a temp dir); the embedded + test decompresses and compares. +- [ ] `docker build` (legacy builder, no buildx) on this host produces a + runnable image; the Dockerfile has no bare `:-amd64` default. +- [ ] `tsc` fails when a Settings field named in SECTIONS is renamed + (proven, then reverted). +- [ ] The settings baseline freezes at mount: a simulated background refetch + with out-of-band changes produces no phantom patch entries (frontend + test). +- [ ] The refresh snapshot survives past gcTime: a frontend test renders the + status 6+ minutes of fake time after the mutation. +- [ ] All four group-id sites import `defaultGroup.ts`; RulesPage preselects + the id-1 group when present (frontend test with a group sorting before + "default"). +- [ ] The dashboard renders per-widget errors on cold navigation with one + endpoint failing (frontend test); the other five loaders are + unchanged. +- [ ] No `["lookup"]` or `["groups"]` literal outside `queryKeys`. +- [ ] BlocklistForm rethrows a non-ApiError (frontend test, mirroring the + logout pair). +- [ ] Full suite green: `zig build test -Dintegration`, `npm run test`, + `npm run typecheck`, `npm run lint`. + +## Anti-requirements + +- No deletion of `extendedRcode` (m17 gave it a caller) and no deletion of + the upstream mutation factories (m17 built their consumer). +- No locking change in the reload handlers — ruling 8 is documentation. +- No new frontend dependencies; the store is hand-rolled like restartBanner. +- No compression plugin for the frontend build; ruling 12 guards the seam, + it does not start using it. +- No `useInfiniteQuery` migration and no QueryLogPage work — that was + milestone 16. +- No renaming of settings keys or API fields — typing the registry must not + change the wire format.