crossbar/docs/plans/v1.1/01-review-fixes.md

v1.1 task 01: review fixes — cancelled clients are recorded; empty usage is []

Branch: v1.1 (create it from master: git switch master && git switch -c v1.1; git status --short must be empty first, otherwise stop) Commit subject: Review fixes: record cancelled requests as 499; empty usage is an array

What the reviewer observed

1. A client that disconnects mid-stream leaves no accounting row: after curl -m 0.4 -N … against a streaming completion, /_crossbar/usage stayed empty. Task 05's rule 5 said the reverse proxy's ErrorHandler does nothing on context.Canceled; rule 6 said "record what you have when ServeHTTP returns". The second rule was not applied on that path, and the same gap exists for a client that gives up while waiting in the limiter queue (rule 4 said "just return, log 499"). Cancelled requests held a slot and cost prefill; usage and error rate must see them. The task text was ambiguous (owner's fault); the fix is still needed.
2. GET /_crossbar/usage with no rows answers null. The spec said a JSON array. Clients iterate the result; null is not iterable.

Files

• Copy (never edit afterwards): internal/proxy/cancel_test.go, internal/admin/usage_empty_test.go
• Modify: files under internal/proxy/ as needed (forward.go, proxy.go), internal/admin/admin_ops.go (or wherever the usage handler lives), docs/implementer-log.md

Rules

1. Every request that reached step 3 of ServeHTTP (a lease was acquired) writes exactly one store.Request row, on every exit path: normal completion, upstream error (502), queue full (503), client cancelled while queued (499, Err: "client cancelled while queued"), client cancelled during the forward (499, Err: "client cancelled", with whatever tokens the tee had seen). Detect the forward case with r.Context().Err() != nil after rp.ServeHTTP returns, or in the ErrorHandler when errors.Is(err, context.Canceled); do not write to the client in that case, do not mark the host down, but do record. Status 499 is not an HTTP status the client sees; it is the row's status (and the log line's), as nginx does.
2. /_crossbar/usage JSON encodes an empty result as []: initialise the slice (rows := []store.UsageRow{} / make(..., 0)) before encoding, on every by value and with or without since. The text form prints its header line even with no rows.
3. Environment fact you need: when the client disconnects while httputil.ReverseProxy is copying the response and the request came through a real http.Server, ServeHTTP does not return — it panics with http.ErrAbortHandler, which the server swallows. Code after rp.ServeHTTP never runs on that path. Write the accounting row from a deferred function in forward: recover(), record (status 499 when the recovered value is http.ErrAbortHandler or the request context is done), then re-panic with the same value so the server keeps its semantics. Exactly one row per request on every path.
4. Nothing else changes. Existing tests must keep passing; the two new ones must pass.

Steps

• 1. Branch and copy.

git switch master && git switch -c v1.1
cp docs/plans/v1.1/_files/internal/proxy/cancel_test.go internal/proxy/
cp docs/plans/v1.1/_files/internal/admin/usage_empty_test.go internal/admin/

• 2. See them fail. go test -run 'Cancel|UsageEmpty' ./internal/proxy/ ./internal/admin/. Expected: all three tests fail (no 499 row, body "null"). If one passes already, stop and report.
• 3. Fix. gofmt -w internal/.
• 4. See everything pass. go test -race -count=2 ./.... The cancel tests are timing-based with generous margins.
• 5. Run the gate. make gate. Expected last line: gate: ok.
• 6. Log and commit. Row v1.1/01-review-fixes.

git add internal/proxy internal/admin docs/implementer-log.md
git commit

Done when

• Step 2 failed before the fix and go test -race -count=2 ./... passes after; make gate prints gate: ok; both copied tests byte-identical to _files/.

Stop and report if

• Step 2 passes before any change, or the cancel tests fail intermittently after the fix (report the failure text).