milestone 19: hygiene sweep - dead ecs surface, single-source constants, tls classification, frontend state hazards, docker smoke network fix
This commit is contained in:
+20
-20
@@ -119,19 +119,19 @@ The repo's history proves this theme's stakes: commit 35f2324 fixed a process-ki
|
||||
- **[low — CLOSED m16] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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)
|
||||
@@ -177,10 +177,10 @@ The repo's history proves this theme's stakes: commit 35f2324 fixed a process-ki
|
||||
- **[medium — CLOSED m16] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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)
|
||||
@@ -194,7 +194,7 @@ The repo's history proves this theme's stakes: commit 35f2324 fixed a process-ki
|
||||
- **[medium — CLOSED m16] 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.**
|
||||
- **[low — CLOSED m19] 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 — CLOSED m16] src/web/handlers/settings.zig:312 — config_lock held across argon2id hashing (19 MiB, t=2) in the settings PUT.**
|
||||
@@ -220,42 +220,42 @@ The repo's history proves this theme's stakes: commit 35f2324 fixed a process-ki
|
||||
- **[medium — CLOSED m16] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[medium — CLOSED m19] 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.**
|
||||
- **[medium — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.**
|
||||
- **[low — CLOSED m19] 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.
|
||||
|
||||
---
|
||||
|
||||
Reference in New Issue
Block a user