fix(proxy): admission-control review fixes missing from the #65 merge (#66)
CI / CI (push) Successful in 43m8s
CI / CI (push) Successful in 43m8s
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. --------- Co-authored-by: Poimen <[email protected]> Reviewed-on: #66 Co-authored-by: poimen <[email protected]>
This commit was merged in pull request #66.
This commit is contained in:
@@ -127,6 +127,30 @@ func (o *EngineObserver) Degraded() bool {
|
||||
return o.degraded
|
||||
}
|
||||
|
||||
// Snapshot returns the latest state together with the tick channel that the
|
||||
// next successful poll will close. Both are read under one lock on purpose:
|
||||
// calling Get and Tick separately can miss the wakeup that lands between the
|
||||
// two calls, parking a request for an extra poll interval.
|
||||
func (o *EngineObserver) Snapshot() (EngineState, <-chan struct{}) {
|
||||
o.mu.Lock()
|
||||
defer o.mu.Unlock()
|
||||
return o.state, o.tick
|
||||
}
|
||||
|
||||
// markDegraded records whether the metrics feed is currently unusable and
|
||||
// mirrors that into observer_degraded. It is cleared on the next parseable
|
||||
// scrape, so one truncated response cannot pin the gauge for the process
|
||||
// lifetime while metric-name drift after the first poll is still reported.
|
||||
func (o *EngineObserver) markDegraded(v bool) {
|
||||
o.mu.Lock()
|
||||
changed := o.degraded != v
|
||||
o.degraded = v
|
||||
o.mu.Unlock()
|
||||
if changed {
|
||||
observability.Global().SetObserverDegraded(v)
|
||||
}
|
||||
}
|
||||
|
||||
// poll scrapes /metrics once. startup marks the first poll, whose failure is a
|
||||
// self-diagnosis signal rather than a transient blip.
|
||||
func (o *EngineObserver) poll(ctx context.Context, startup bool) {
|
||||
@@ -161,13 +185,11 @@ func (o *EngineObserver) poll(ctx context.Context, startup bool) {
|
||||
|
||||
running, waiting, kvUsage, err := ParseEngineMetrics(resp.Body)
|
||||
if err != nil {
|
||||
// Metric-name drift must be loud, never a silent fall-back to fail-open
|
||||
// admission (decision Q21). The gauge tracks current state, so it clears
|
||||
// on the next parseable scrape instead of pinning for the pod's life.
|
||||
o.markDegraded(true)
|
||||
if startup {
|
||||
// Startup self-check (decision Q21): metric-name drift must be
|
||||
// loud, never a silent fall-back to fail-open admission.
|
||||
o.mu.Lock()
|
||||
o.degraded = true
|
||||
o.mu.Unlock()
|
||||
observability.Global().SetObserverDegraded(true)
|
||||
logging.Errorf("engine metrics unusable at startup", err, map[string]string{
|
||||
"address": o.addr,
|
||||
})
|
||||
@@ -181,6 +203,7 @@ func (o *EngineObserver) poll(ctx context.Context, startup bool) {
|
||||
return
|
||||
}
|
||||
|
||||
o.markDegraded(false)
|
||||
o.recordSuccess(running, waiting, kvUsage)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user