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. 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).