Middleware: per-Write logging goroutine (race), bodiless requests unlogged, shared next #8

New Issue

2026-09-07T13:23:38Z

claude commented

2026-09-07 13:23:38 +00:00

Found while working on the robots.txt change. internal/server/middlewares has several issues; both files carry a // TODO rewrite this already.

`Log` (`log.go`)

go w.logRequest(bytes) on every Write (line 64). A handler that writes the body in chunks spawns one goroutine per chunk, all reading the same *LogWriter fields (statusCode, start, req) with no synchronisation — a data race, and the request emits multiple log lines. logRequest should run once, after the handler returns (call it from Log.ServeHTTP after l.next.ServeHTTP, not from Write).
Bodiless responses are never logged. logRequest is only reached from Write. A handler that calls WriteHeader and returns (204, 304, a redirect, an error with an empty body) produces no log line at all.
Nil *zerolog.Event deref (line 79–97). The if/else if chain has no final else. A statusCode below 100 (e.g. a 0 that slipped through) leaves e == nil, then e.Str(...) panics. Give it a default branch.
LogWriter drops optional ResponseWriter interfaces. It wraps http.ResponseWriter but doesn't forward http.Flusher, http.Hijacker or io.ReaderFrom. The static file server and the ReadFile audio handler lose ReadFrom (slower copies), and anything streaming loses Flush.

`CorsOrigin` (`cors.go`)

c.next is stored on a single shared instance (line 19). router.Use calls Middleware once at setup so this happens to work, but the instance is not safe to register on more than one router / chain, and it's the reason for the // TODO. The mux idiom is return http.HandlerFunc(func(w, r){ ...; next.ServeHTTP(w, r) }).
origin field / NewCorsOrigin(origin string) argument are dead. ServeHTTP hardcodes Access-Control-Allow-Origin: * (line 26) and never reads c.origin. Either honour the argument or drop it.
The comment on line 24–25 flags that * is a dev-only setting that should be tightened before deployment — zampler.aneur.in is deployed, so this is worth closing out.

Same l.next = next shared-instance pattern in Log.Middleware.

Found while working on the robots.txt change. `internal/server/middlewares` has several issues; both files carry a `// TODO rewrite this` already. ## `Log` (`log.go`) - **`go w.logRequest(bytes)` on every `Write` (line 64).** A handler that writes the body in chunks spawns one goroutine per chunk, all reading the same `*LogWriter` fields (`statusCode`, `start`, `req`) with no synchronisation — a data race, and the request emits multiple log lines. `logRequest` should run once, after the handler returns (call it from `Log.ServeHTTP` after `l.next.ServeHTTP`, not from `Write`). - **Bodiless responses are never logged.** `logRequest` is only reached from `Write`. A handler that calls `WriteHeader` and returns (204, 304, a redirect, an error with an empty body) produces no log line at all. - **Nil `*zerolog.Event` deref (line 79–97).** The `if/else if` chain has no final `else`. A `statusCode` below 100 (e.g. a `0` that slipped through) leaves `e == nil`, then `e.Str(...)` panics. Give it a default branch. - **`LogWriter` drops optional `ResponseWriter` interfaces.** It wraps `http.ResponseWriter` but doesn't forward `http.Flusher`, `http.Hijacker` or `io.ReaderFrom`. The static file server and the `ReadFile` audio handler lose `ReadFrom` (slower copies), and anything streaming loses `Flush`. ## `CorsOrigin` (`cors.go`) - **`c.next` is stored on a single shared instance (line 19).** `router.Use` calls `Middleware` once at setup so this happens to work, but the instance is not safe to register on more than one router / chain, and it's the reason for the `// TODO`. The mux idiom is `return http.HandlerFunc(func(w, r){ ...; next.ServeHTTP(w, r) })`. - **`origin` field / `NewCorsOrigin(origin string)` argument are dead.** `ServeHTTP` hardcodes `Access-Control-Allow-Origin: *` (line 26) and never reads `c.origin`. Either honour the argument or drop it. - The comment on line 24–25 flags that `*` is a dev-only setting that should be tightened before deployment — `zampler.aneur.in` is deployed, so this is worth closing out. Same `l.next = next` shared-instance pattern in `Log.Middleware`.

claude referenced this issue from a commit

2026-09-07 16:22:21 +00:00

fix middleware races, ReadFile error paths, and lib.Coalesce

claude closed this issue

2026-09-07 16:28:24 +00:00

claude commented

2026-09-07 16:28:35 +00:00

Fixed in 7a6e1aa (PR #15).

Fixed in 7a6e1aa (PR #15).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: zampler/zampler#8

Middleware: per-Write logging goroutine (race), bodiless requests unlogged, shared next #8

Log (log.go)

CorsOrigin (cors.go)

`Log` (`log.go`)

`CorsOrigin` (`cors.go`)