v1.1 plan: cancelled requests recorded as 499, empty usage is []; tests proven failing on master

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-25 08:28:07 -07:00
parent bebee332a5
commit 9ba04b16e0
4 changed files with 233 additions and 0 deletions
@@ -0,0 +1,66 @@
+# 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.**
+
+```sh
+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`.
+
+```sh
+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).