proxy/config.go (1)

141-146:Consider making OnStartup a named type for future extensibility

Inline struct is fine now, but if hooks grow, a named type (e.g., type OnStartupHooks struct { Preload []string ... }) can improve clarity and reuse across config, tests, and docs.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

proxy/discardWriter.go (1)

5-28:ResponseWriter contract fixed; Flush support added

Header is now stable, Write returns len(p), status is tracked, and Flush is provided. This resolves prior issues flagged in earlier reviews about breaking http semantics and panic risks in flusher paths.

proxy/helpers_test.go (1)

72-78:Use the already-initialized simpleResponderPath in YAML

Switching to the package-level simpleResponderPath makes the YAML generation stable across platforms. One nit: TestMain (Line 24) still recomputes the path via getSimpleResponderPath(). Consider reusing simpleResponderPath there to avoid drift between the path you validate and the one you embed in configs.

proxy/events.go (1)

53-60:Consider documenting Success semantics

The ModelPreloadedEvent type is clear. Add a brief comment clarifying what “Success” means (e.g., process swap ok vs. dummy request ok) to prevent ambiguous interpretations by subscribers.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

proxy/proxymanager.go (1)

101-127:Preload event may report success on proxy failure; add cancellation, endpoint targeting, and error handling

Current flow emits Success: true even if ProxyRequest fails, doesn’t cancel on shutdown, and always probes “/”. Tighten this:

• Emit Success: false if ProxyRequest fails.
• Use pm.shutdownCtx so preload cancels cleanly.
• Prefer each model’s CheckEndpoint (fallback “/”).
• Handle http.NewRequest error and log proxy errors.
• Copy the preload slice defensively in the goroutine.

 // run any startup hooks if len(config.Hooks.OnStartup.Preload) > 0 {-// do it in the background, don't block startup -- not sure if good idea yet+// do it in the background, don't block startup go func() {-discardWriter := &DiscardWriter{}-for _, realModelName := range config.Hooks.OnStartup.Preload {+// defensive copy in case config is ever mutated later+preload := append([]string(nil), config.Hooks.OnStartup.Preload...)+for _, realModelName := range preload {+select {+case <-pm.shutdownCtx.Done():+pm.proxyLogger.Infof("Preload cancelled due to shutdown")+return+default:+} proxyLogger.Infof("Preloading model: %s", realModelName) processGroup, _, err := pm.swapProcessGroup(realModelName)  if err != nil {-event.Emit(ModelPreloadedEvent{-ModelName: realModelName,-Success:   false,-})+event.Emit(ModelPreloadedEvent{ModelName: realModelName, Success: false}) proxyLogger.Errorf("Failed to preload model %s: %v", realModelName, err) continue-} else {-req, _ := http.NewRequest("GET", "/", nil)-processGroup.ProxyRequest(realModelName, discardWriter, req)-event.Emit(ModelPreloadedEvent{-ModelName: realModelName,-Success:   true,-}) }+// Prefer model-specific check endpoint if configured+endpoint := strings.TrimSpace(pm.config.Models[realModelName].CheckEndpoint)+if endpoint == "" {+endpoint = "/"+}+req, reqErr := http.NewRequestWithContext(pm.shutdownCtx, "GET", endpoint, nil)+if reqErr != nil {+event.Emit(ModelPreloadedEvent{ModelName: realModelName, Success: false})+proxyLogger.Errorf("Failed to build preload request for %s: %v", realModelName, reqErr)+continue+}+// Use a fresh DiscardWriter per request+if err := processGroup.ProxyRequest(realModelName, &DiscardWriter{}, req); err != nil {+event.Emit(ModelPreloadedEvent{ModelName: realModelName, Success: false})+proxyLogger.Errorf("Preload request failed for %s: %v", realModelName, err)+continue+}+event.Emit(ModelPreloadedEvent{ModelName: realModelName, Success: true}) } }() }

config.example.yaml (1)

223-231:Fix typo and clarify alias support in startup hooks

• Typo: “dictionar” → “dictionary”.
• Minor grammar: “models sections” → “models section”.
• Add a note that aliases are supported and resolved to real model IDs (this matches the config cleanup logic via RealModelName).

Apply this diff:

-  # - optional, default: empty dictionar+  # - optional, default: empty dictionary@@-        # - model names must match keys in the models sections+        # - model names must match keys in the models section+        # - aliases are supported; entries are resolved to real model IDs

proxy/proxymanager_test.go (1)

868-885:Count only successful preloads and assert both model IDs observed

The test currently waits for any two events, including failures. Tighten to count successes and track model names to avoid false positives.

-preloadChan := make(chan ModelPreloadedEvent, 2) // buffer for 2 expected events--unsub := event.On(func(e ModelPreloadedEvent) {-preloadChan <- e-})+successCh := make(chan string, 2) // buffer for 2 expected successes+unsub := event.On(func(e ModelPreloadedEvent) {+if e.Success {+successCh <- e.ModelName+}+})@@-for i := 0; i < 2; i++ {-select {-case <-preloadChan:-case <-time.After(5 * time.Second):-t.Fatal("timed out waiting for models to preload")-}-}+want := map[string]bool{"model1": false, "model2": false}+timeout := time.After(10 * time.Second)+for seen := 0; seen < 2; {+select {+case name := <-successCh:+if _, ok := want[name]; ok && !want[name] {+want[name] = true+seen+++}+case <-timeout:+t.Fatalf("timed out waiting for successful preloads; seen=%v", want)+}+}

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

Learnt from: mostlygeekPR: mostlygeek/llama-swap#117File: proxy/config.go:0-0Timestamp: 2025-05-05T23:54:14.205ZLearning: In the llama-swap project, automatic port assignment does not include port availability checks. It's the user's responsibility to provide an appropriate startPort that doesn't conflict with other services.