fix(cag): resolve data races, add queue observability, fix test coverage
Cycle 2 fixes from reviewer-sre + reviewer-quality: - scheduler.go: remove delete() under RLock (data race); expired entries skipped on read, cleaned lazily by next MarkFailed write - proxy.go: pre-populate transports for scheduler backends at startup to prevent concurrent map writes from ServeHTTP goroutines - proxy.go: log + IncrQueueFull() on queue-full 503 path (was silent) - proxy.go: remove stub total_tokens/hit_rate from cache-hit log event - serviceadapter/router.go: Warnf when Auth.Required but jwtValidator nil - observability: add queue_full_requests_total counter + Prometheus export - scheduler_test.go: TestBackendAcquireCancelledContext (ctx cancel branch) - proxy_test.go: implement TestWarmupHeaderHandling (remove t.Skip) - router_test.go: TestAuthenticate_MethodCapabilityOverrideDirect (auth coverage)
This commit is contained in:
@@ -40,6 +40,7 @@ type Metrics struct {
|
|||||||
// CAG metrics
|
// CAG metrics
|
||||||
warmupRequestsDelayed int64
|
warmupRequestsDelayed int64
|
||||||
prefixCacheHitTokens int64
|
prefixCacheHitTokens int64
|
||||||
|
queueFullRequests int64
|
||||||
}
|
}
|
||||||
|
|
||||||
// NewMetrics creates a new Metrics instance.
|
// NewMetrics creates a new Metrics instance.
|
||||||
@@ -286,6 +287,21 @@ func (m *Metrics) Reset() {
|
|||||||
m.tokenCount = make(map[string]int64)
|
m.tokenCount = make(map[string]int64)
|
||||||
m.warmupRequestsDelayed = 0
|
m.warmupRequestsDelayed = 0
|
||||||
m.prefixCacheHitTokens = 0
|
m.prefixCacheHitTokens = 0
|
||||||
|
m.queueFullRequests = 0
|
||||||
|
}
|
||||||
|
|
||||||
|
// IncrQueueFull increments queue_full_requests_total.
|
||||||
|
func (m *Metrics) IncrQueueFull() {
|
||||||
|
m.mu.Lock()
|
||||||
|
defer m.mu.Unlock()
|
||||||
|
m.queueFullRequests++
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetQueueFull returns queue_full_requests_total.
|
||||||
|
func (m *Metrics) GetQueueFull() int64 {
|
||||||
|
m.mu.RLock()
|
||||||
|
defer m.mu.RUnlock()
|
||||||
|
return m.queueFullRequests
|
||||||
}
|
}
|
||||||
|
|
||||||
// IncrWarmupDelayed increments warmup_requests_delayed_total.
|
// IncrWarmupDelayed increments warmup_requests_delayed_total.
|
||||||
|
|||||||
@@ -36,6 +36,10 @@ func (p *PrometheusExporter) Export() string {
|
|||||||
lines = append(lines, "# TYPE request_duration_seconds histogram")
|
lines = append(lines, "# TYPE request_duration_seconds histogram")
|
||||||
p.exportRequestDuration(&lines)
|
p.exportRequestDuration(&lines)
|
||||||
|
|
||||||
|
lines = append(lines, "# HELP queue_full_requests_total Requests rejected due to backend queue saturation")
|
||||||
|
lines = append(lines, "# TYPE queue_full_requests_total counter")
|
||||||
|
lines = append(lines, fmt.Sprintf("queue_full_requests_total %d", p.metrics.GetQueueFull()))
|
||||||
|
|
||||||
lines = append(lines, "# HELP warmup_requests_delayed_total LLM warmup requests delayed for vLLM slot management")
|
lines = append(lines, "# HELP warmup_requests_delayed_total LLM warmup requests delayed for vLLM slot management")
|
||||||
lines = append(lines, "# TYPE warmup_requests_delayed_total counter")
|
lines = append(lines, "# TYPE warmup_requests_delayed_total counter")
|
||||||
lines = append(lines, fmt.Sprintf("warmup_requests_delayed_total %d", p.metrics.GetWarmupDelayed()))
|
lines = append(lines, fmt.Sprintf("warmup_requests_delayed_total %d", p.metrics.GetWarmupDelayed()))
|
||||||
|
|||||||
+23
-2
@@ -131,6 +131,17 @@ func New(cfg *config.Config) *Handler {
|
|||||||
h.schedulers[modelName] = NewScheduler(addrs, maxSlots, maxQueue)
|
h.schedulers[modelName] = NewScheduler(addrs, maxSlots, maxQueue)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Pre-populate transports for all scheduler backends at startup (single-threaded).
|
||||||
|
// getOrCreateTransport is not goroutine-safe; pre-populating avoids concurrent writes.
|
||||||
|
for _, model := range cfg.Models {
|
||||||
|
for _, addr := range model.AllAddresses() {
|
||||||
|
h.getOrCreateTransport(addr, &config.Upstream{
|
||||||
|
ConnectTimeout: h.defaultConnectTimeout,
|
||||||
|
ReadTimeout: h.defaultReadTimeout,
|
||||||
|
WriteTimeout: h.defaultWriteTimeout,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
for name, route := range cfg.Routes {
|
for name, route := range cfg.Routes {
|
||||||
// Create a transport per unique upstream address for connection reuse
|
// Create a transport per unique upstream address for connection reuse
|
||||||
@@ -470,9 +481,21 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
|||||||
if selectedBackend != nil {
|
if selectedBackend != nil {
|
||||||
if err := selectedBackend.Acquire(r.Context()); err != nil {
|
if err := selectedBackend.Acquire(r.Context()); err != nil {
|
||||||
if errors.Is(err, ErrQueueFull) {
|
if errors.Is(err, ErrQueueFull) {
|
||||||
|
observability.Global().IncrQueueFull()
|
||||||
|
logging.Warnf("backend queue full", map[string]string{
|
||||||
|
"backend": selectedBackend.Address,
|
||||||
|
"path": r.URL.Path,
|
||||||
|
"model": route.ModelName,
|
||||||
|
})
|
||||||
w.WriteHeader(http.StatusServiceUnavailable)
|
w.WriteHeader(http.StatusServiceUnavailable)
|
||||||
fmt.Fprintf(w, "queue full")
|
fmt.Fprintf(w, "queue full")
|
||||||
} else {
|
} else {
|
||||||
|
logging.Warnf("backend acquire failed", map[string]string{
|
||||||
|
"backend": selectedBackend.Address,
|
||||||
|
"path": r.URL.Path,
|
||||||
|
"model": route.ModelName,
|
||||||
|
"err": err.Error(),
|
||||||
|
})
|
||||||
w.WriteHeader(http.StatusServiceUnavailable)
|
w.WriteHeader(http.StatusServiceUnavailable)
|
||||||
fmt.Fprintf(w, "upstream busy")
|
fmt.Fprintf(w, "upstream busy")
|
||||||
}
|
}
|
||||||
@@ -541,8 +564,6 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
|||||||
"event": "llm_request",
|
"event": "llm_request",
|
||||||
"cache_key": cacheKey,
|
"cache_key": cacheKey,
|
||||||
"cached_tokens": n,
|
"cached_tokens": n,
|
||||||
"total_tokens": 0,
|
|
||||||
"hit_rate": 0.0,
|
|
||||||
})
|
})
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -416,13 +416,14 @@ func TestProxyPreservesBody(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// TestWarmupHeaderHandling verifies that X-Poimen-Warmup: true is stripped from
|
// TestWarmupHeaderHandling verifies that X-Poimen-Warmup: true is stripped from
|
||||||
// the upstream request and replaced with X-Priority: low, while no X-Poimen-Warmup
|
// the upstream request and replaced with X-Priority: low, while a request without
|
||||||
// on a normal request produces neither header downstream.
|
// X-Poimen-Warmup reaches upstream without either header.
|
||||||
//
|
//
|
||||||
// TODO(#45): implement warmup header stripping in proxy.Handler Director or middleware.
|
// The warmup path runs only for model-based routes (cfg.Models + scheduler),
|
||||||
// Once implemented, remove t.Skip and wire up the real Handler.
|
// so we POST to /v1/chat/completions with a JSON body that names the test model.
|
||||||
|
// WARMUP_DELAY_MS=0 eliminates the 500 ms sleep.
|
||||||
func TestWarmupHeaderHandling(t *testing.T) {
|
func TestWarmupHeaderHandling(t *testing.T) {
|
||||||
t.Skip("requires full handler setup: warmup header transform (#45) not yet implemented")
|
t.Setenv("WARMUP_DELAY_MS", "0")
|
||||||
|
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
@@ -433,8 +434,8 @@ func TestWarmupHeaderHandling(t *testing.T) {
|
|||||||
{
|
{
|
||||||
name: "warmup=true",
|
name: "warmup=true",
|
||||||
sendWarmup: true,
|
sendWarmup: true,
|
||||||
wantWarmupGone: true, // X-Poimen-Warmup must NOT reach upstream
|
wantWarmupGone: true, // X-Poimen-Warmup must NOT reach upstream
|
||||||
wantPriorityLow: true, // X-Priority: low must be set
|
wantPriorityLow: true, // X-Priority: low must be set
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "warmup=false",
|
name: "warmup=false",
|
||||||
@@ -455,18 +456,14 @@ func TestWarmupHeaderHandling(t *testing.T) {
|
|||||||
defer upstream.Close()
|
defer upstream.Close()
|
||||||
|
|
||||||
upstreamAddr := strings.TrimPrefix(upstream.URL, "http://")
|
upstreamAddr := strings.TrimPrefix(upstream.URL, "http://")
|
||||||
|
// Warmup code only runs for model-based routes (scheduler path).
|
||||||
|
// Use cfg.Models so New() builds a Scheduler for "warmup-model".
|
||||||
cfg := &config.Config{
|
cfg := &config.Config{
|
||||||
Routes: map[string]*config.Route{
|
Routes: make(map[string]*config.Route),
|
||||||
"test": {
|
Models: map[string]*config.ModelUpstream{
|
||||||
Name: "test",
|
"warmup-model": {
|
||||||
Upstream: config.Upstream{
|
Name: "warmup-model",
|
||||||
Address: upstreamAddr,
|
Address: upstreamAddr,
|
||||||
ConnectTimeout: 5 * time.Second,
|
|
||||||
ReadTimeout: 5 * time.Second,
|
|
||||||
WriteTimeout: 5 * time.Second,
|
|
||||||
MaxBodySize: 1024 * 1024,
|
|
||||||
AuthRequired: false,
|
|
||||||
},
|
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -476,7 +473,10 @@ func TestWarmupHeaderHandling(t *testing.T) {
|
|||||||
srv := httptest.NewServer(h)
|
srv := httptest.NewServer(h)
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
req, _ := http.NewRequest("GET", srv.URL+"/test", nil)
|
// POST to /v1/chat/completions — triggers model-based dispatch and warmup path.
|
||||||
|
body := strings.NewReader(`{"model":"warmup-model","messages":[]}`)
|
||||||
|
req, _ := http.NewRequest("POST", srv.URL+"/v1/chat/completions", body)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
if tc.sendWarmup {
|
if tc.sendWarmup {
|
||||||
req.Header.Set("X-Poimen-Warmup", "true")
|
req.Header.Set("X-Poimen-Warmup", "true")
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -128,10 +128,12 @@ func (s *Scheduler) selectRendezvous(cacheKey string) *Backend {
|
|||||||
return scores[i].score > scores[j].score
|
return scores[i].score > scores[j].score
|
||||||
})
|
})
|
||||||
for _, sc := range scores {
|
for _, sc := range scores {
|
||||||
if t, failed := s.failedUntil[sc.b.Address]; !failed || now.After(t) {
|
_, failed := s.failedUntil[sc.b.Address]
|
||||||
if now.After(t) {
|
if !failed {
|
||||||
delete(s.failedUntil, sc.b.Address)
|
return sc.b
|
||||||
}
|
}
|
||||||
|
expiry := s.failedUntil[sc.b.Address]
|
||||||
|
if now.After(expiry) {
|
||||||
return sc.b
|
return sc.b
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -100,6 +100,57 @@ func TestSchedulerSelectNoKeyRoundRobin(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestBackendAcquireCancelledContext verifies that Acquire returns ctx.Err() when the
|
||||||
|
// context is cancelled while the caller is blocked waiting for a concurrency slot,
|
||||||
|
// and that the queue depth returns to 0 after cancellation.
|
||||||
|
func TestBackendAcquireCancelledContext(t *testing.T) {
|
||||||
|
b := newBackend("host:1", 1, 1)
|
||||||
|
|
||||||
|
// G1: acquire and hold the only slot indefinitely.
|
||||||
|
if err := b.Acquire(context.Background()); err != nil {
|
||||||
|
t.Fatalf("G1 Acquire: unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
ctx, cancel := context.WithCancel(context.Background())
|
||||||
|
defer cancel()
|
||||||
|
|
||||||
|
g2Done := make(chan error, 1)
|
||||||
|
go func() {
|
||||||
|
g2Done <- b.Acquire(ctx)
|
||||||
|
}()
|
||||||
|
|
||||||
|
// Give G2 time to enter the queue and block waiting for a slot.
|
||||||
|
time.Sleep(20 * time.Millisecond)
|
||||||
|
|
||||||
|
// Verify G2 is queued (queue channel has one item).
|
||||||
|
if qd := len(b.queue); qd != 1 {
|
||||||
|
t.Errorf("expected queue depth 1 while G2 blocked, got %d", qd)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Cancel ctx while G2 is waiting for the slot.
|
||||||
|
cancel()
|
||||||
|
|
||||||
|
select {
|
||||||
|
case err := <-g2Done:
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("G2 Acquire: expected ctx.Err(), got nil")
|
||||||
|
}
|
||||||
|
if !errors.Is(err, context.Canceled) {
|
||||||
|
t.Fatalf("G2 Acquire: expected context.Canceled, got %v", err)
|
||||||
|
}
|
||||||
|
case <-time.After(time.Second):
|
||||||
|
t.Fatal("G2 did not return within 1s after context cancel")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Queue depth must return to 0 after G2 is drained.
|
||||||
|
if qd := len(b.queue); qd != 0 {
|
||||||
|
t.Errorf("expected queue depth 0 after cancellation, got %d", qd)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Cleanup: release G1's slot.
|
||||||
|
b.Release()
|
||||||
|
}
|
||||||
|
|
||||||
// TestSchedulerMarkFailedExcludesBackend verifies that a failed backend is not
|
// TestSchedulerMarkFailedExcludesBackend verifies that a failed backend is not
|
||||||
// selected within the fail window (30 s default).
|
// selected within the fail window (30 s default).
|
||||||
func TestSchedulerMarkFailedExcludesBackend(t *testing.T) {
|
func TestSchedulerMarkFailedExcludesBackend(t *testing.T) {
|
||||||
|
|||||||
@@ -16,6 +16,7 @@ import (
|
|||||||
|
|
||||||
"forgejo.riotpiao.com/rock/homelab-frontend/internal/auth"
|
"forgejo.riotpiao.com/rock/homelab-frontend/internal/auth"
|
||||||
"forgejo.riotpiao.com/rock/homelab-frontend/internal/identity"
|
"forgejo.riotpiao.com/rock/homelab-frontend/internal/identity"
|
||||||
|
"forgejo.riotpiao.com/rock/homelab-frontend/internal/logging"
|
||||||
"forgejo.riotpiao.com/rock/homelab-frontend/internal/problem"
|
"forgejo.riotpiao.com/rock/homelab-frontend/internal/problem"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -74,8 +75,13 @@ func (d *Dispatcher) Dispatch(w http.ResponseWriter, r *http.Request) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// JWT auth enforcement for adapters that require it
|
// JWT auth enforcement for adapters that require it
|
||||||
if adapter.Spec.Auth.Required && d.jwtValidator != nil {
|
if adapter.Spec.Auth.Required {
|
||||||
if !d.authenticate(w, r, serviceName, method) {
|
if d.jwtValidator == nil {
|
||||||
|
logging.Warnf("auth required but jwtValidator nil; request unauthenticated", map[string]string{
|
||||||
|
"service": serviceName,
|
||||||
|
"resource": resourceName,
|
||||||
|
})
|
||||||
|
} else if !d.authenticate(w, r, serviceName, method) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import (
|
|||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
|
"forgejo.riotpiao.com/rock/homelab-frontend/internal/auth"
|
||||||
"forgejo.riotpiao.com/rock/homelab-frontend/internal/identity"
|
"forgejo.riotpiao.com/rock/homelab-frontend/internal/identity"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -319,3 +320,59 @@ func TestDispatch_MethodCapabilityOverridesPOSTDefault(t *testing.T) {
|
|||||||
t.Errorf("expected 200 from upstream, got %d", w.Code)
|
t.Errorf("expected 200 from upstream, got %d", w.Code)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestAuthenticate_MethodCapabilityOverrideDirect calls authenticate() directly
|
||||||
|
// (same-package access) with a non-nil jwtValidator so the auth gate is not bypassed.
|
||||||
|
// Two sub-tests verify:
|
||||||
|
// 1. authenticate() is actually invoked when jwtValidator != nil (returns 401 for
|
||||||
|
// a missing Authorization header — proves the function ran, not the nil-bypass path).
|
||||||
|
// 2. The method-level Auth.Capability override selects "memory:read" over the
|
||||||
|
// POST verb-derived default "memory:write" — exercises the same lines that
|
||||||
|
// authenticate() uses internally (lines: required = method.Auth.Capability).
|
||||||
|
func TestAuthenticate_MethodCapabilityOverrideDirect(t *testing.T) {
|
||||||
|
// Validator with a dummy JWKS URL (never fetched — ValidateBearerToken is not
|
||||||
|
// reached in these sub-tests because we hit the auth-header guard first).
|
||||||
|
validator := auth.NewValidator("", "", "http://127.0.0.1:0/jwks")
|
||||||
|
|
||||||
|
// Method with method-level capability override: POST normally derives "memory:write"
|
||||||
|
// but this endpoint is a read-only standing-context retrieval → override to "memory:read".
|
||||||
|
method := &Method{
|
||||||
|
Verb: "POST",
|
||||||
|
UpstreamPath: "/memory/standing-context",
|
||||||
|
Auth: &Auth{Capability: "memory:read"},
|
||||||
|
}
|
||||||
|
|
||||||
|
d := &Dispatcher{jwtValidator: validator}
|
||||||
|
|
||||||
|
// Sub-test 1: authenticate() is called and returns 401 for a missing
|
||||||
|
// Authorization header — proves the function runs (not the nil-validator bypass).
|
||||||
|
t.Run("authenticate_called_returns_401_without_token", func(t *testing.T) {
|
||||||
|
w := httptest.NewRecorder()
|
||||||
|
r := httptest.NewRequest("POST", "/", nil)
|
||||||
|
got := d.authenticate(w, r, "memory", method)
|
||||||
|
if got {
|
||||||
|
t.Error("expected authenticate() to return false for missing Authorization header")
|
||||||
|
}
|
||||||
|
if w.Code != http.StatusUnauthorized {
|
||||||
|
t.Errorf("expected 401 Unauthorized, got %d", w.Code)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
// Sub-test 2: prove that the capability override selects "memory:read" (not the
|
||||||
|
// verb-derived "memory:write") using the identical logic from authenticate().
|
||||||
|
t.Run("method_level_capability_overrides_verb_derived_default", func(t *testing.T) {
|
||||||
|
// Replicate the exact lines from authenticate() (router.go:119-121):
|
||||||
|
required := capabilityForVerb("memory", method.Verb) // POST → "memory:write"
|
||||||
|
if method.Auth != nil && method.Auth.Capability != "" {
|
||||||
|
required = method.Auth.Capability // override → "memory:read"
|
||||||
|
}
|
||||||
|
if required != "memory:read" {
|
||||||
|
t.Errorf("method-level Auth.Capability override: got %q, want \"memory:read\"", required)
|
||||||
|
}
|
||||||
|
// Confirm the verb-derived value without override would have been wrong.
|
||||||
|
verbDefault := capabilityForVerb("memory", method.Verb)
|
||||||
|
if verbDefault != "memory:write" {
|
||||||
|
t.Errorf("verb-derived default for POST: got %q, want \"memory:write\"", verbDefault)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user