milestone 18: collapse duplicated infrastructure into shared listener core, crud list helper, resource shells, transport race, name and line helpers, ui modules
This commit is contained in:
@@ -286,7 +286,10 @@ store-deinit and the refs==0 assert holds. `dot_alpn` (["dot"]) lives in app.zig
|
||||
Deviation: `doh_server.serve` module entry is NOT used — it constructs the DohServer in
|
||||
its own task frame, so WebState could never get the pointer the `nxdns_doh_server_*`
|
||||
family needs; both listeners bind in app.zig's frame instead (tcp_server style).
|
||||
`doh_server.serve` remains as unused pub API. metrics.zig: `DohListenerSample`
|
||||
`doh_server.serve` remains as unused pub API. **Superseded by milestone-18 ruling 1: the
|
||||
unused `doh_server.serve` is deleted. Nothing ever called it, and a dead second
|
||||
composition path over the listener core is exactly the divergence that milestone
|
||||
collapsed. app.zig's in-frame bind is now the only way DoH comes up.** metrics.zig: `DohListenerSample`
|
||||
(ruling-10 four + `bad_requests`); DoT renders `dot_server.StatsSnapshot` directly;
|
||||
accept-side counters stay off the exposition; unwired listeners omit the families.
|
||||
WebState gains `doh_listener`/`dot_listener` optional pointers. build.zig (out of
|
||||
|
||||
@@ -406,6 +406,136 @@ dead `doh_server.serve`, the two `normalizeName` copies and their
|
||||
shared InlineError. `npm run test`, `typecheck`, `lint` green.
|
||||
- [ ] Full suite green: `zig build test -Dintegration`.
|
||||
|
||||
## Recorded (implementation)
|
||||
|
||||
Accepted deviations and findings from the built milestone. Each was
|
||||
reviewed and accepted at integration; the rulings above stand except as
|
||||
recorded here.
|
||||
|
||||
### Ruling 1 (S1)
|
||||
|
||||
- `Cfg` supplies more than the four listed members: `Owner`,
|
||||
`ConnPayload`, `serveConn`, `read_buffer_len`, `write_buffer_len`,
|
||||
plus `log` (the owner's `std.log` scope) and `name`, because the
|
||||
accept loop and shutdown log and the text had to stay byte-identical.
|
||||
Two optional decls: `refuse` (absent means close the stream; the web
|
||||
listener supplies its 503) and `initPayload`/`deinitPayload`, which
|
||||
exist only for the web arena's create-in-listen / destroy-in-deinit
|
||||
lifecycle.
|
||||
- The read/write staging buffers live in the core's `Conn`, not in
|
||||
`ConnPayload` — all four listeners have exactly one of each and
|
||||
differ only in size. The web listener's `recv_buf`/`send_buf` are now
|
||||
`read_buf`/`write_buf`.
|
||||
- Stats layout: core counters live at `server.core.stats.*`
|
||||
(`listener.CoreStats`); listener-specific counters stay on the owner
|
||||
at `server.stats.*`. `tcp_server.Stats` is an alias of `CoreStats`.
|
||||
Exported snapshots stay flat, so /metrics output is unchanged except
|
||||
the tcp rename. `idle_timeouts` sits in `CoreStats` per the ruling,
|
||||
which gives the web listener a counter it never bumps; it exports no
|
||||
family, so nothing is visible.
|
||||
- The docs-reference update for the rename has no target:
|
||||
`nxdns_tcp_server_accepted_total` appears in no file under docs/ or
|
||||
web/. Only the metrics name test changed.
|
||||
- 18 duplicated unit tests were deleted (6 tcp, 6 dot, 3 doh, 3 web)
|
||||
and replaced by 6 shared claim-rule tests in `listener.zig`.
|
||||
`web/server_integration_test.zig`'s `withServer` now returns a local
|
||||
plain-`u64` `Counters` struct instead of `server.Stats`.
|
||||
- File-ownership breach: S1 edited two files owned by other sessions,
|
||||
both mechanical fallout of the sanctioned `deinit` shape change —
|
||||
three call sites in `src/app.zig` (S5) and one line in
|
||||
`src/web/web_integration_test.zig` (S3). Both owners reviewed and
|
||||
kept the edits. S1 also added the required `src/tests.zig` import
|
||||
line for the new file.
|
||||
|
||||
### Ruling 2 (S2)
|
||||
|
||||
- `listRowsBound(comptime Row, database, gpa, comptime sql, args:
|
||||
anytype, comptime readRow)` exists beside `listRows` for the bound
|
||||
queries; `freeRow` exists beside `freeRows`. Unknown owning field
|
||||
shapes are a `@compileError` as ruled. The milestone-4 sample
|
||||
correction was made by S2, not the orchestrator.
|
||||
|
||||
### Ruling 3 (S3)
|
||||
|
||||
- `plural` is a separate descriptor member: the "listing X" log context
|
||||
differs from the JSON envelope key for three resources
|
||||
(`local_records`, `client_prefixes`, `forward_zones`), so deriving one
|
||||
from the other would change three log strings.
|
||||
- `view` is an optional descriptor member: rules and local records map
|
||||
rows through `RuleView`/`RecordView`; without it neither could adopt
|
||||
without changing its response body. When absent, the row serializes
|
||||
as-is with no copy.
|
||||
- `remove` has two comptime-detected arities: three handlers' decision
|
||||
functions read rows and take an `Allocator` before the id, three do
|
||||
not. The generator validates the full signature of whichever shape it
|
||||
finds.
|
||||
|
||||
### Ruling 4 (S4)
|
||||
|
||||
- The ruling's line numbers were stale after m17 (pool.zig's race sites
|
||||
were at 184-206 and 306-335).
|
||||
- `closeBlocked` branches at comptime on the close method's parameter
|
||||
count: `tls_client.TlsStream.close()` takes no `Io` (it owns the one
|
||||
it was built with), while the other three closes take `io`.
|
||||
- "The stashed-cause accessor" is three accessors, one per collapse
|
||||
point: the send phase reads `req.connection.?.stream_writer.err`,
|
||||
`receiveHead` reads `Connection.getReadError()`, and the body read
|
||||
consults `Response.bodyErr()` first (HTTP framing faults) then the
|
||||
connection.
|
||||
- `Connection.getReadError` can panic (it reads `stream_reader.err.?`).
|
||||
The unwrap guards the plain-connection no-cause case; the TLS case
|
||||
relies on std's documented contract that a cause exists after
|
||||
`error.ReadFailed`.
|
||||
- `raceWithin` requires the raced function's return type to be exactly
|
||||
`ExchangeError!T` at comptime. Stricter than the ruling asked; it is
|
||||
what keeps every failure path inside the race group. Consequence:
|
||||
`manager.zig`'s `fetchWithin` (optional per the ruling) cannot adopt
|
||||
it as written — its raced function has a different error set.
|
||||
- The acceptance line "`fn expire(` production copies are gone" is
|
||||
scoped to ruling 4's transport files. `filter/manager.zig`,
|
||||
`storage/logger.zig` and `server/listener.zig` keep their own — none
|
||||
is transport code.
|
||||
|
||||
### Rulings 5, 6, 7 (S5, S4)
|
||||
|
||||
- `nextBoundedLine` returns `.long_line` from the
|
||||
EndOfStream-during-discard arm, and the next call returns `null`. The
|
||||
two originals disagreed there (`compile` counted then broke;
|
||||
`collectSample` returned without counting); this shape preserves both
|
||||
behaviors — `compile` counts exactly as before, `collectSample`
|
||||
ignores the event and sees `null`. Safe because
|
||||
`discardDelimiterInclusive` drains the stream before reporting
|
||||
`EndOfStream`.
|
||||
- The length check runs on the raw line for both callers, as the ruling
|
||||
placed it. One input changes classification: a line of exactly
|
||||
`max_line_len + 1` bytes ending in `\r` was compiled before and now
|
||||
counts as `long_lines`. Accepted as the intended reading.
|
||||
- `check`'s per-message `FAIL` lines render import's path constant
|
||||
(`FAIL config: <line:col: message>`), not the file name. `checkImpl`
|
||||
prints the file path immediately above and `check` examines one
|
||||
source per run, and this keeps `check` and `import` rendering the
|
||||
same failure identically — the acceptance criterion.
|
||||
|
||||
### Rulings 8, 9 (S6)
|
||||
|
||||
- The fuzz-suite wiring did not fold into `addTestSuite` (the ruling's
|
||||
own fallback): it lives in a separate `addFuzzSuite` helper, with a
|
||||
shared `sourceModule` helper. The aarch64 target resolves from
|
||||
`cross_targets[1]` behind a comptime prefix guard.
|
||||
- `pairInput` moved into `smith_encode.zig` rather than staying in
|
||||
blocklist_fuzz: m15's fuzz files had made it a two-copy duplicate,
|
||||
which is the condition ruling 9 exists to remove.
|
||||
|
||||
### Ruling 10 (S7)
|
||||
|
||||
- `ui/classes.ts` exports 17 constants, not the 8 listed — the extra
|
||||
ones are the focus-ring fragment and compositions the 15 adopting
|
||||
files needed to drop their literals without re-spelling anything.
|
||||
- Seven ring-less focusable controls were fixed by adoption, not the
|
||||
two the ruling named. The acceptance grep ("every input/button/select
|
||||
carries the focus-visible fragment") was taken as the invariant over
|
||||
the anti-requirement's count of two, which undercounted.
|
||||
|
||||
## Anti-requirements
|
||||
|
||||
- No behavioral changes: this milestone moves code. The only sanctioned
|
||||
|
||||
+11
-2
@@ -1258,18 +1258,27 @@ predicate in S5 needs the true count.
|
||||
SQLite and is invalidated by the next `step`; a repository that returns a borrowed slice is a
|
||||
use-after-free waiting for the second row.
|
||||
- Every `list` builds into a `std.ArrayList(T)` with an `errdefer` that frees **both** every element
|
||||
already appended and every string of the partially-built element:
|
||||
already appended and every string of the partially-built element. The two list-level `errdefer`s
|
||||
run in reverse declaration order, so the free pass is declared **after** `out.deinit` to run
|
||||
**before** it — the other order reads `out.items` once the backing array is already released:
|
||||
|
||||
```zig
|
||||
var out: std.ArrayList(model.Group) = .empty;
|
||||
errdefer freeGroups(gpa, out.items);
|
||||
errdefer out.deinit(gpa);
|
||||
errdefer freeGroups(gpa, out.items);
|
||||
while (try stmt.step()) {
|
||||
const name = try stmt.columnTextAlloc(gpa, 0);
|
||||
errdefer gpa.free(name);
|
||||
try out.append(gpa, .{ .name = name, .safe_search = stmt.columnBool(1) });
|
||||
}
|
||||
```
|
||||
|
||||
> **Correction (milestone 18, ruling 2).** This sample originally declared the two `errdefer`s in
|
||||
> the opposite order, which frees the backing array before the pass that walks it — a
|
||||
> use-after-free. Every repository written from it silently corrected the order; the sample above
|
||||
> is the corrected one. Milestone 18 moved the whole choreography into
|
||||
> `src/storage/repositories/crud.zig` as `listRows`/`freeRows`, so a new repository delegates to
|
||||
> that helper instead of copying this shape.
|
||||
- `freeX` frees every heap string in every element and is idempotent against an empty slice.
|
||||
- The callers of `list` may pass an arena; `freeX` must still be correct against a general-purpose
|
||||
allocator, because the tests use `std.testing.allocator`.
|
||||
|
||||
Reference in New Issue
Block a user