Follow-up to #65, which is already merged. The review of that PR returned BLOCK with two high-severity defects; the fix commit was pushed after the merge landed, so none of it is in main.
What happened
main @ 369d51a is the squash of the first commit only (c5a91cf).
d71c884 (review fixes) is on the old branch and was never merged — PR #65 is closed.
Verified against main directly: ADMISSION_MAX_INFLIGHT 0 hits, func (rw *responseWriter) Flush 0 hits, ModifyResponse 0 hits, Snapshot 0 hits.
The image in production (forgejo.riotpiao.com/rock/api-gateway@sha256:64f2bfc1…, 3/3 replicas) is built from that squash, so the defects below are live right now.
Live state confirmed
https://api.riotpiao.com/metrics on the running pods:
The observer itself works: it is reading the real vLLM exposition body and the startup self-check passed (observer_degraded 0). No requests have parked yet, so the two defects below have not been observable in the wild — they are proven from code, not from a live failure.
Defects fixed here
1. The parked-stream keep-alive never runs in production (high).internal/tracing/middleware.go wraps the ResponseWriter in a struct that embeds the interface and implements only WriteHeader/Write, so w.(http.Flusher) in internal/proxy/proxy.go is false for every real request (cmd/gateway/main.go → tracing.Middleware → router). prewritten therefore never becomes true, the heartbeat never starts, and a parked streaming request writes zero bytes for up to ADMISSION_MAX_PARK_SECONDS (120) — the client timeout the feature exists to prevent. Fixed by adding Flush() and Unwrap() to the wrapper.
2. Upstream status masked once the SSE status is spent (high, latent behind 1).httputil.ReverseProxy calls rw.WriteHeader(res.StatusCode) unconditionally; after the park wrote 200, net/http keeps 200 and the upstream error body is appended raw into the event stream, which a streaming client reads as an empty completion and reports as success. Fixed with a ModifyResponse that frames an upstream >= 400 as an event: error when the status has already been written.
3. Lost wakeup in the park loop.Get() and Tick() were taken under two separate lock acquisitions, so a poll landing between them pushed the waiter onto the next generation's channel — up to one extra poll interval (2s) parked. Snapshot() now returns state and channel under one lock.
4. observer_degraded pinned at 1 forever. It was raised only on the first poll's parse failure and SetObserverDegraded(false) had no call site, so one truncated response pinned the gauge for the pod's life, while genuine metric-name drift appearing later was never reported at all. markDegraded now tracks current parsability in both directions and only touches the exporter on a change.
5. ADMISSION_MAX_INFLIGHT silently defaulted. Now set explicitly to 4 in k8s/deployment.yaml with the coupling to the engine's --max-num-seqs documented, instead of quietly inheriting VLLM_MAX_NUM_SEQS.
Verification
go build ./... clean · go vet ./... clean · go test ./... 368 passed, 0 failed · structural-check.sh 0 errors (1 pre-existing :latest warning) · go.mod unchanged.
Cherry-picked onto main rather than merged from the old branch, so this diff is exactly the five fixes and nothing else: 5 files, +80/−8.
Still open
The reviewer's BLOCK also rested on nine functions over CRAP>30 purely from 0% coverage — the deliberate "no new tests" decision. Worth recording that defect 1 is a one-line type assertion that a httptest case through the real middleware chain would have caught immediately. Raising it here so the decision is visible rather than implied.
Follow-up to #65, which is already merged. The review of that PR returned BLOCK with two high-severity defects; the fix commit was pushed **after** the merge landed, so none of it is in `main`.
## What happened
- `main` @ `369d51a` is the squash of the first commit only (`c5a91cf`).
- `d71c884` (review fixes) is on the old branch and was never merged — PR #65 is closed.
- Verified against `main` directly: `ADMISSION_MAX_INFLIGHT` 0 hits, `func (rw *responseWriter) Flush` 0 hits, `ModifyResponse` 0 hits, `Snapshot` 0 hits.
- The image in production (`forgejo.riotpiao.com/rock/api-gateway@sha256:64f2bfc1…`, 3/3 replicas) is built from that squash, so the defects below are **live right now**.
## Live state confirmed
`https://api.riotpiao.com/metrics` on the running pods:
```
observer_degraded 0
vllm_requests_running 1
vllm_kv_usage_perc 0.008293092588704587
admission_parked_total 0
engine_state_stale_total 0
```
The observer itself works: it is reading the real vLLM exposition body and the startup self-check passed (`observer_degraded 0`). No requests have parked yet, so the two defects below have not been observable in the wild — they are proven from code, not from a live failure.
## Defects fixed here
**1. The parked-stream keep-alive never runs in production (high).** `internal/tracing/middleware.go` wraps the `ResponseWriter` in a struct that embeds the interface and implements only `WriteHeader`/`Write`, so `w.(http.Flusher)` in `internal/proxy/proxy.go` is false for every real request (`cmd/gateway/main.go` → `tracing.Middleware` → `router`). `prewritten` therefore never becomes true, the heartbeat never starts, and a parked streaming request writes zero bytes for up to `ADMISSION_MAX_PARK_SECONDS` (120) — the client timeout the feature exists to prevent. Fixed by adding `Flush()` and `Unwrap()` to the wrapper.
**2. Upstream status masked once the SSE status is spent (high, latent behind 1).** `httputil.ReverseProxy` calls `rw.WriteHeader(res.StatusCode)` unconditionally; after the park wrote 200, net/http keeps 200 and the upstream error body is appended raw into the event stream, which a streaming client reads as an empty completion and reports as success. Fixed with a `ModifyResponse` that frames an upstream `>= 400` as an `event: error` when the status has already been written.
**3. Lost wakeup in the park loop.** `Get()` and `Tick()` were taken under two separate lock acquisitions, so a poll landing between them pushed the waiter onto the next generation's channel — up to one extra poll interval (2s) parked. `Snapshot()` now returns state and channel under one lock.
**4. `observer_degraded` pinned at 1 forever.** It was raised only on the *first* poll's parse failure and `SetObserverDegraded(false)` had no call site, so one truncated response pinned the gauge for the pod's life, while genuine metric-name drift appearing later was never reported at all. `markDegraded` now tracks current parsability in both directions and only touches the exporter on a change.
**5. `ADMISSION_MAX_INFLIGHT` silently defaulted.** Now set explicitly to `4` in `k8s/deployment.yaml` with the coupling to the engine's `--max-num-seqs` documented, instead of quietly inheriting `VLLM_MAX_NUM_SEQS`.
## Verification
`go build ./...` clean · `go vet ./...` clean · `go test ./...` 368 passed, 0 failed · `structural-check.sh` 0 errors (1 pre-existing `:latest` warning) · `go.mod` unchanged.
Cherry-picked onto `main` rather than merged from the old branch, so this diff is exactly the five fixes and nothing else: 5 files, +80/−8.
## Still open
The reviewer's BLOCK also rested on nine functions over CRAP>30 purely from 0% coverage — the deliberate "no new tests" decision. Worth recording that defect 1 is a one-line type assertion that a `httptest` case through the real middleware chain would have caught immediately. Raising it here so the decision is visible rather than implied.
- tracing: responseWriter now forwards Flush and exposes Unwrap. Without it
w.(http.Flusher) failed for every request passing the tracing middleware, so
the parked-stream keep-alive never started: a parked streaming request wrote
nothing for up to ADMISSION_MAX_PARK_SECONDS, which is exactly the client
timeout the keep-alive was added to prevent.
- proxy: once the SSE status line is spent, ModifyResponse frames an upstream
response >= 400 as an event: error instead of letting a raw error body land
inside the stream, where a streaming client reads it as an empty completion
and reports success. net/http keeps the first status, so the upstream status
cannot be forwarded after the park has written one.
- engine_state: new Snapshot() returns the state and its tick channel under one
lock. Get and Tick taken separately can miss the poll that lands between the
two calls, parking a request for an extra interval.
- engine_state: observer_degraded now tracks current parsability rather than
being raised once on the first poll and never cleared, which pinned it at 1
after a single truncated response and never reported drift that appeared
later. markDegraded only touches the exporter when the value changes.
- deployment: ADMISSION_MAX_INFLIGHT is set explicitly to 4, documenting the
coupling to the engine's --max-num-seqs, instead of silently defaulting to
VLLM_MAX_NUM_SEQS.
rock
merged commit 727b9e1cb0 into main2026-09-30 12:19:26 +00:00
rock
deleted branch fix/cag4-admission-review-fixes-642026-09-30 12:19:28 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Follow-up to #65, which is already merged. The review of that PR returned BLOCK with two high-severity defects; the fix commit was pushed after the merge landed, so none of it is in
main.What happened
main@369d51ais the squash of the first commit only (c5a91cf).d71c884(review fixes) is on the old branch and was never merged — PR #65 is closed.maindirectly:ADMISSION_MAX_INFLIGHT0 hits,func (rw *responseWriter) Flush0 hits,ModifyResponse0 hits,Snapshot0 hits.forgejo.riotpiao.com/rock/api-gateway@sha256:64f2bfc1…, 3/3 replicas) is built from that squash, so the defects below are live right now.Live state confirmed
https://api.riotpiao.com/metricson the running pods:The observer itself works: it is reading the real vLLM exposition body and the startup self-check passed (
observer_degraded 0). No requests have parked yet, so the two defects below have not been observable in the wild — they are proven from code, not from a live failure.Defects fixed here
1. The parked-stream keep-alive never runs in production (high).
internal/tracing/middleware.gowraps theResponseWriterin a struct that embeds the interface and implements onlyWriteHeader/Write, sow.(http.Flusher)ininternal/proxy/proxy.gois false for every real request (cmd/gateway/main.go→tracing.Middleware→router).prewrittentherefore never becomes true, the heartbeat never starts, and a parked streaming request writes zero bytes for up toADMISSION_MAX_PARK_SECONDS(120) — the client timeout the feature exists to prevent. Fixed by addingFlush()andUnwrap()to the wrapper.2. Upstream status masked once the SSE status is spent (high, latent behind 1).
httputil.ReverseProxycallsrw.WriteHeader(res.StatusCode)unconditionally; after the park wrote 200, net/http keeps 200 and the upstream error body is appended raw into the event stream, which a streaming client reads as an empty completion and reports as success. Fixed with aModifyResponsethat frames an upstream>= 400as anevent: errorwhen the status has already been written.3. Lost wakeup in the park loop.
Get()andTick()were taken under two separate lock acquisitions, so a poll landing between them pushed the waiter onto the next generation's channel — up to one extra poll interval (2s) parked.Snapshot()now returns state and channel under one lock.4.
observer_degradedpinned at 1 forever. It was raised only on the first poll's parse failure andSetObserverDegraded(false)had no call site, so one truncated response pinned the gauge for the pod's life, while genuine metric-name drift appearing later was never reported at all.markDegradednow tracks current parsability in both directions and only touches the exporter on a change.5.
ADMISSION_MAX_INFLIGHTsilently defaulted. Now set explicitly to4ink8s/deployment.yamlwith the coupling to the engine's--max-num-seqsdocumented, instead of quietly inheritingVLLM_MAX_NUM_SEQS.Verification
go build ./...clean ·go vet ./...clean ·go test ./...368 passed, 0 failed ·structural-check.sh0 errors (1 pre-existing:latestwarning) ·go.modunchanged.Cherry-picked onto
mainrather than merged from the old branch, so this diff is exactly the five fixes and nothing else: 5 files, +80/−8.Still open
The reviewer's BLOCK also rested on nine functions over CRAP>30 purely from 0% coverage — the deliberate "no new tests" decision. Worth recording that defect 1 is a one-line type assertion that a
httptestcase through the real middleware chain would have caught immediately. Raising it here so the decision is visible rather than implied.