74 lines
4.2 KiB
Markdown
74 lines
4.2 KiB
Markdown
# 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.**
|
|
|
|
```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).
|