feat: improve config hot-reload with SIGHUP support #430
Conversation
WalkthroughReplaces previous fsnotify-driven reload plumbing with a ProxyManager-based reload model: ProxyManager created once, supports SetVersion, StartConfigWatcher and ReloadConfig; adds SIGHUP-triggered reloads; introduces debouncer, config watcher, per-model restart semantics, new config fields, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)proxy/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-10-07T05:41:52.728ZApplied to files:
📚 Learning: 2025-11-25T05:40:06.444ZApplied to files:
📚 Learning: 2025-10-29T05:26:34.964ZApplied to files:
🧬 Code graph analysis (1)llama-swap.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
proxy/config_reload_test.go (1)
214-228: Test name doesn't match behavior.The test is named "reload with invalid config keeps old config", but it doesn't actually test loading an invalid config. It just verifies that model1 exists after creating the ProxyManager.
Consider enhancing this test or renaming it
To test the actual error-handling behavior, you could attempt to reload a syntactically invalid YAML config and verify the old config is preserved:
t.Run("reload with invalid config keeps old config", func(t *testing.T) { initialCfg := config.Config{ Models: map[string]config.ModelConfig{ "model1": {Cmd: "echo hello", Proxy: "http://localhost:8080", CheckEndpoint: "none"}, }, } initialCfg = config.AddDefaultGroupToConfig(initialCfg) pm := New(initialCfg) defer pm.Shutdown() - // This should work - we test file-based reload separately + // Verify initial state _, found := pm.config.RealModelName("model1") assert.True(t, found) + + // Attempt to reload with invalid config (empty models) + invalidCfg := config.Config{ + Models: map[string]config.ModelConfig{}, + } + // Note: AddDefaultGroupToConfig would make this valid, so skip it + // This simulates a config that passes YAML parsing but fails validation + + err := pm.ReloadConfig(invalidCfg) + // Depending on validation, this might succeed or fail + // For now, just verify model1 still exists + _, found = pm.config.RealModelName("model1") + assert.True(t, found, "model1 should still exist after reload attempt") })Alternatively, rename the test to reflect what it actually tests (e.g., "creates with initial config").
proxy/config_reload.go (1)
74-81: Consider adding the watch path before starting the goroutine.Starting
watchLoop(line 74) before successfully adding the path (line 76) creates a complex cleanup scenario ifwatcher.Addfails. While the current cleanup logic should work correctly, moving thewatcher.Addcall beforego cw.watchLoop()would simplify the initialization sequence and make the code easier to reason about.🔎 Proposed refactor
- // Start watching - go cw.watchLoop() - if err := watcher.Add(path); err != nil { - close(cw.stopChan) // Signal watchLoop to stop cw.debouncer.stop() watcher.Close() return nil, err } + // Start watching + go cw.watchLoop() + return cw, nil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
llama-swap.go(1 hunks)proxy/config/config.go(1 hunks)proxy/config/config_test.go(1 hunks)proxy/config/model_config.go(1 hunks)proxy/config_reload.go(1 hunks)proxy/config_reload_test.go(1 hunks)proxy/debounce.go(1 hunks)proxy/debounce_test.go(1 hunks)proxy/proxymanager.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
proxy/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Run
make test-devwhen making iterative changes to code under theproxy/directory - this runsgo testandstaticcheck, and all static checking errors must be fixed
Files:
proxy/config/config.goproxy/config/model_config.goproxy/config/config_test.goproxy/config_reload_test.goproxy/debounce_test.goproxy/proxymanager.goproxy/debounce.goproxy/config_reload.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:40:06.444Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - this runs `go test` and `staticcheck`, and all static checking errors must be fixed
📚 Learning: 2025-11-25T05:40:06.444Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:40:06.444Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - this runs `go test` and `staticcheck`, and all static checking errors must be fixed
Applied to files:
proxy/config/config_test.goproxy/config_reload_test.goproxy/debounce_test.goproxy/config_reload.go
📚 Learning: 2025-10-29T05:26:34.964Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.
Applied to files:
proxy/config_reload_test.go
📚 Learning: 2025-10-07T05:41:52.728Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 337
File: proxy/config/config.go:268-291
Timestamp: 2025-10-07T05:41:52.728Z
Learning: In the mostlygeek/llama-swap repository's proxy/config/config.go, macro-in-macro substitution requires that referenced macros be declared earlier in the YAML file (declaration order matters). A macro can only reference macros that appear before it in the configuration. The YAML order is preserved by the UnmarshalYAML implementation for MacroList, and the LIFO substitution approach is intentional.
Applied to files:
llama-swap.go
🧬 Code graph analysis (4)
proxy/config_reload_test.go (2)
proxy/config/model_config.go (1)
ModelConfig(10-44)proxy/config/config.go (3)
Config(112-142)AddDefaultGroupToConfig(414-456)LoadConfig(162-169)
proxy/proxymanager.go (2)
proxy/config/config.go (2)
Config(112-142)LoadConfig(162-169)proxy/processgroup.go (1)
NewProcessGroup(29-55)
proxy/config_reload.go (1)
proxy/config/model_config.go (1)
ModelConfig(10-44)
llama-swap.go (2)
proxy/proxymanager.go (1)
New(59-177)proxy/config/config.go (1)
LoadConfig(162-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (18)
proxy/debounce_test.go (1)
11-55: LGTM! Comprehensive test coverage for debouncer.The test suite thoroughly covers the debouncer's key behaviors: single execution after delay, coalescing of rapid triggers, and stop prevention. The use of
atomic.Int32ensures thread-safe counter access across goroutines.proxy/config/config.go (1)
140-141: LGTM! Clear field addition for global restart control.The
ReloadRestartModelsfield with defaultfalseprovides opt-in restart behavior during config reload, which is the safe default for hot-reload scenarios.proxy/config/model_config.go (1)
42-43: LGTM! Pointer type enables tri-state per-model override.Using
*boolcorrectly allowsForceRestartto have three states: nil (use global setting), false (never restart), and true (always restart). This provides fine-grained control over model restart behavior.proxy/config/config_test.go (1)
765-814: LGTM! Tests verify correct parsing and defaults for reload fields.The tests appropriately verify:
ReloadRestartModelsis parsed when set and defaults tofalseForceRestartis parsed when set and defaults tonil(allowing global setting to apply)This ensures the config fields behave as documented.
Based on learnings, run
make test-devto validate these changes along with the rest of the proxy/ directory.proxy/debounce.go (1)
8-55: LGTM! Thread-safe debouncer with correct lock handling.The implementation correctly:
- Uses
sync.Mutexto protect shared state (timer,stopped)- Cancels any pending timer before starting a new one in
trigger()- Releases the lock before executing
fn()in the timer callback (line 42), preventing deadlock iffn()needs to acquire other locks- Sets the
stoppedflag atomically instop()to prevent race conditionsThe design is sound and thread-safe.
Based on learnings, run
make test-devto validate these changes.proxy/proxymanager.go (3)
179-225: LGTM! ReloadConfig provides atomic config updates with selective restarts.The implementation correctly:
- Acquires the ProxyManager lock for atomic config replacement
- Identifies models requiring restart using
shouldRestartModelbefore modifying state- Updates existing process groups' config references in-place
- Gracefully stops only affected models via
process.Stop()- Creates new process groups for new group IDs
The approach minimizes disruption by restarting only models with material config changes.
Based on learnings, run
make test-devto validate these changes.
227-251: LGTM! Config watcher with error recovery.The watcher correctly:
- Uses a 2-second debounce to coalesce rapid file system events during saves
- Preserves the old config on load or apply errors (fault-tolerant)
- Logs errors clearly to aid debugging
The error handling ensures the system remains operational even if a config update fails.
441-446: Excellent deadlock prevention fix!Stopping the config watcher before acquiring the ProxyManager lock is critical because:
- The watcher callback invokes
ReloadConfig(), which acquires the lock- If the watcher were stopped inside the lock, a concurrent reload could deadlock
The comment clearly explains the reasoning. This demonstrates careful consideration of shutdown sequencing.
proxy/config_reload_test.go (3)
14-138: LGTM! Comprehensive test coverage for restart logic.The tests thoroughly verify:
TestModelNeedsRestart: Change detection for fields that affect model runtime (cmd, proxy, env, etc.) vs. metadata-only changes (ttl, aliases)TestShouldRestartModel: Integration of globalReloadRestartModelsflag with per-modelForceRestartoverride, including both true/false override scenariosThe test cases are well-structured and cover edge cases.
Based on learnings, run
make test-devto validate these changes.
140-181: LGTM! Watcher test validates debouncing behavior.The test correctly:
- Creates a temporary config file
- Starts a watcher with a 50ms debounce
- Modifies the file after allowing the watcher to initialize
- Waits 200ms (4× the debounce delay) to ensure the reload completes
- Uses
atomic.Int32for thread-safe counting across goroutines- Verifies exactly one reload occurs (confirming debounce coalescing)
231-287: LGTM! End-to-end integration test validates watcher and reload.The test correctly:
- Creates a temporary config file and initializes ProxyManager
- Starts the config watcher
- Verifies initial state (model1 present, model2 absent)
- Modifies the file to add model2
- Waits 2500ms for the 2-second debounce plus processing time
- Acquires the ProxyManager lock before reading
pm.config(thread-safe)- Verifies model2 is now available
This provides confidence that the hot-reload mechanism works end-to-end.
Based on learnings, run
make test-devto validate these changes.llama-swap.go (3)
77-77: LGTM! SIGHUP signal handling enabled.Adding
syscall.SIGHUPto the signal notification enables graceful config reload viakill -HUP <pid>, complementing the file-based auto-reload.
84-94: LGTM! Cleaner ProxyManager initialization and watcher setup.The refactored initialization:
- Creates the ProxyManager once and sets its version metadata
- Directly assigns it as the HTTP handler (simpler lifecycle)
- Conditionally starts the file watcher with non-fatal error handling
This is more straightforward than the prior approach described in the PR objectives.
96-125: LGTM! Unified signal handling with graceful reload and shutdown.The consolidated signal handler correctly:
- SIGHUP (lines 99-109): Reloads config immediately (no debounce), logs errors but continues serving with the current config (resilient)
- SIGINT/SIGTERM (lines 112-124): Shuts down ProxyManager first (stops models/watchers), then gracefully shuts down the HTTP server with a 5-second timeout
The shutdown sequence is correct: stopping the ProxyManager before the HTTP server ensures requests in-flight can complete against running models.
proxy/config_reload.go (4)
12-34: LGTM! Restart detection logic is well-scoped.The function correctly identifies fields that require a model process restart (Cmd, CmdStop, Proxy, CheckEndpoint, Env, ConcurrencyLimit) while allowing runtime-configurable fields (Aliases, UnloadAfter, Metadata, etc.) to be updated without restart.
36-48: LGTM! Override precedence is correctly implemented.The function properly implements the precedence hierarchy: short-circuits if no restart needed, honors per-model ForceRestart override when set, and falls back to the global setting.
108-119: LGTM! Thread-safe and idempotent cleanup.The stop method correctly implements thread-safe, idempotent cleanup with proper resource shutdown ordering.
1-119: Run tests after changes.As per coding guidelines for
proxy/**/*.gofiles, runmake test-devto executego testandstaticcheckto ensure all changes pass tests and static analysis.Based on coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/config_reload.go (1)
106-111: Error logging added as requested.The watcher error case now logs errors via
log.Printf, addressing the past review comment. While this uses the standard library logger rather than the structured logger used elsewhere, this is a reasonable compromise sinceconfigWatcheris a standalone component without access to ProxyManager's logger context.Optional: Consider accepting a logger parameter
For consistency with the rest of the codebase's structured logging, you could optionally pass a logger to
newConfigWatcher:func newConfigWatcher(path string, debounceDelay time.Duration, logger Logger, onReload func(path string)) (*configWatcher, error)However, this adds complexity and the current approach is acceptable for error reporting.
Based on learnings, run
make test-devto validate changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
proxy/config/config.go(1 hunks)proxy/config/model_config.go(1 hunks)proxy/config_reload.go(1 hunks)proxy/config_reload_test.go(1 hunks)proxy/debounce.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/config/config.go
🧰 Additional context used
📓 Path-based instructions (1)
proxy/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Run
make test-devwhen making iterative changes to code under theproxy/directory - this runsgo testandstaticcheck, and all static checking errors must be fixed
Files:
proxy/config/model_config.goproxy/config_reload.goproxy/config_reload_test.goproxy/debounce.go
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:40:06.444Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:40:06.444Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - this runs `go test` and `staticcheck`, and all static checking errors must be fixed
Applied to files:
proxy/config_reload.goproxy/config_reload_test.go
📚 Learning: 2025-10-29T05:26:34.964Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.
Applied to files:
proxy/config_reload_test.go
🧬 Code graph analysis (2)
proxy/config_reload.go (1)
proxy/config/model_config.go (1)
ModelConfig(10-46)
proxy/config_reload_test.go (3)
proxy/config/model_config.go (1)
ModelConfig(10-46)proxy/config/config.go (4)
Config(112-143)AddDefaultGroupToConfig(415-457)GroupConfig(74-79)LoadConfig(163-170)proxy/proxymanager.go (1)
New(59-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (12)
proxy/debounce.go (3)
11-26: LGTM! Clean debouncer implementation.The struct design and constructor are straightforward and appropriate for the use case.
30-51: LGTM! Thread-safe trigger implementation.The double-lock pattern is correct: the outer lock protects timer state transitions, while the callback's lock check prevents execution after stop(). Releasing the mutex before calling fn() avoids blocking the debouncer if fn() is slow.
54-63: LGTM! Proper cleanup with idempotency.The stop() method correctly prevents multiple stops and ensures all resources are cleaned up.
proxy/config/model_config.go (1)
42-45: LGTM! Well-designed per-model restart override.The
ForceRestart *boolfield correctly supports three states (true/false/nil) for flexible restart control. The nil default (via UnmarshalYAML not initializing it) properly enables inheritance of the globalReloadRestartModelssetting.proxy/config_reload_test.go (5)
14-83: LGTM! Comprehensive restart detection tests.The test cases thoroughly cover all restart-triggering field changes and correctly distinguish between fields that require restart versus those that don't (ttl, aliases).
85-138: LGTM! Thorough restart policy tests.The test cases validate the correct precedence of per-model
ForceRestartover the global restart setting, with good coverage of edge cases.
140-181: LGTM! Proper watcher behavior test.The test correctly uses atomic operations for thread-safe counting and validates that debouncing results in a single reload after file modification.
183-243: LGTM! Solid config reload tests.The tests validate that
ReloadConfigcorrectly updates model availability and handles empty configurations appropriately (treating them as valid but resulting in no available models).
245-301: LGTM! Comprehensive watcher integration test.The test validates end-to-end config watching behavior with appropriate debounce wait time (2500ms for 2s debounce) and proper locking around config access.
proxy/config_reload.go (3)
14-49: LGTM! Correct restart decision logic.The restart detection correctly identifies fields requiring process restart (Cmd, CmdStop, Proxy, CheckEndpoint, Env, ConcurrencyLimit) and properly implements the precedence: per-model
ForceRestartoverrides the globalreloadRestartModelssetting.
53-90: LGTM! Well-structured watcher initialization.The constructor properly handles errors by adding the path before starting the goroutine and cleaning up the watcher on failure. The initialization order ensures resources are set up correctly before the watch loop starts.
116-127: LGTM! Proper cleanup with correct ordering.The stop method safely shuts down all resources with proper synchronization and idempotency.
Implements Task 2 from the config hot-reload plan. Adds intelligent model diff detection to determine when running models need to be restarted during configuration reloads. Changes: - proxy/config_reload.go: New file with modelNeedsRestart and shouldRestartModel functions - proxy/config_reload_test.go: Comprehensive table-driven tests for both functions The modelNeedsRestart function detects when critical fields change (Cmd, CmdStop, Proxy, CheckEndpoint, Env) that require a process restart, while ignoring non-critical changes (TTL, Aliases). The shouldRestartModel function combines config diff detection with the global reloadRestartModels setting and per-model forceRestart override to determine final restart behavior. All tests passing. Follows TDD Red-Green-Refactor cycle. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add configWatcher and configPath fields to ProxyManager struct - Implement StartConfigWatcher method with 2-second debounce - On config file change: - Log "Config file changed, reloading..." - Load new config from file - If load fails: log error and keep old config - If load succeeds: call ReloadConfig to apply changes - Update Shutdown to stop watcher if running - Add TestProxyManagerConfigWatcher test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace the old approach that fully restarted ProxyManager on config change with the new graceful hot-reload that: - Keeps running models alive (unless forceRestart is set) - Atomically updates config without shutdown - Uses built-in 2-second debounce The --watch-config flag now uses ProxyManager's integrated config watcher for a much cleaner implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Upstream ProxyManager uses sync.Mutex, not sync.RWMutex.
SIGHUP triggers immediate config reload without debounce. SIGINT/SIGTERM continue to trigger graceful shutdown. Usage: kill -HUP <pid> or docker kill -s SIGHUP <container>
- Add docstrings to debouncer, configWatcher, and config fields - Fix initialization sequence: watcher.Add before starting goroutine - Add error logging in watchLoop - Fix test to actually test config reload behavior
Mootikins
force-pushed
the
feature/config-hot-reload-upstream
branch
from
07ab4de to
acc676d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
llama-swap.go (1)
112-124:defer cancel()inside loop won't execute until goroutine exits.The
defer cancel()on line 115 is placed inside theforloop. While this works because the function returns immediately after, it's unconventional and could cause issues if the code is modified later. Consider callingcancel()explicitly before or aftersrv.Shutdown().🔎 Suggested fix
// SIGINT or SIGTERM - shutdown fmt.Printf("Received signal %v, shutting down...\n", sig) ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) - defer cancel() pm.Shutdown() if err := srv.Shutdown(ctx); err != nil { fmt.Printf("Server shutdown error: %v\n", err) } + cancel() close(exitChan) returnproxy/config_reload_test.go (2)
85-138: LGTM! Good coverage of ForceRestart interactions.The tests properly verify the priority: per-model
ForceRestartoverrides globalReloadRestartModels. TheboolPtrhelper is a clean pattern.Consider adding a test case for
ForceRestart=truewith no config changes to verify it doesn't cause unnecessary restarts (assuming that's the intended behavior based onshouldRestartModelrequiringmodelNeedsRestartto return true first).
214-242: Consider renaming test for clarity.The test is named "reload with invalid config keeps old config" but actually tests that an empty config (which is valid) correctly removes the previous models. The assertion on line 241 confirms model1 is removed, not kept. Consider renaming to better reflect the actual behavior being tested:
- t.Run("reload with invalid config keeps old config", func(t *testing.T) { + t.Run("reload with empty config removes all models", func(t *testing.T) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
llama-swap.goproxy/config/config.goproxy/config/config_test.goproxy/config/model_config.goproxy/config_reload.goproxy/config_reload_test.goproxy/debounce.goproxy/debounce_test.goproxy/proxymanager.go
✅ Files skipped from review due to trivial changes (1)
- proxy/config/config.go
🚧 Files skipped from review as they are similar to previous changes (4)
- proxy/debounce.go
- proxy/config_reload.go
- proxy/config/config_test.go
- proxy/proxymanager.go
🧰 Additional context used
📓 Path-based instructions (1)
proxy/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Run
make test-devwhen making iterative changes to code under theproxy/directory - this runsgo testandstaticcheck, and all static checking errors must be fixed
Files:
proxy/debounce_test.goproxy/config/model_config.goproxy/config_reload_test.go
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:40:06.444Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:40:06.444Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - this runs `go test` and `staticcheck`, and all static checking errors must be fixed
Applied to files:
proxy/debounce_test.goproxy/config_reload_test.go
📚 Learning: 2025-10-29T05:26:34.964Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.
Applied to files:
proxy/config_reload_test.go
🧬 Code graph analysis (2)
llama-swap.go (4)
proxy/proxymanager.go (1)
New(59-197)proxy/config/config.go (1)
LoadConfig(173-180)proxy/proxymanager_api.go (1)
pm(103-200)proxy/events.go (1)
ReloadingState(37-39)
proxy/config_reload_test.go (2)
proxy/config/model_config.go (1)
ModelConfig(10-46)proxy/config/config.go (4)
Config(118-153)AddDefaultGroupToConfig(443-485)GroupConfig(80-85)LoadConfig(173-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (7)
proxy/debounce_test.go (1)
11-55: LGTM! Clean and comprehensive debouncer tests.The test coverage is solid, covering the three key behaviors: delayed execution, coalescing rapid triggers, and stop cancellation. Good use of
atomic.Int32for thread-safe counting.One minor consideration: timing-based tests can occasionally be flaky under CI load. The current margins (100ms sleep for 50ms debounce) should be sufficient, but if flakiness occurs, consider increasing the margin or using a polling approach with
assert.Eventually.llama-swap.go (2)
84-94: LGTM! Clean initialization flow.The separation of concerns is good: ProxyManager creation, version setting, and optional config watcher are clearly delineated. Non-fatal warning for watcher failure is appropriate since
--watch-configis an optional convenience feature.
96-110: LGTM! SIGHUP handling with appropriate error recovery.The immediate reload (no debounce) for SIGHUP aligns with the PR objectives for production use. Error handling correctly preserves the current config on failure, and the loop continues to handle subsequent signals.
proxy/config/model_config.go (1)
41-45: LGTM! Well-designed optional override field.Using
*boolcorrectly enables three-state semantics:true(force restart),false(prevent restart), andnil(inherit globalReloadRestartModelssetting). The inline documentation clearly explains the behavior.proxy/config_reload_test.go (3)
14-83: LGTM! Comprehensive table-driven tests for restart detection.Good coverage of fields that should trigger restart (Cmd, Proxy, CmdStop, Env, CheckEndpoint, ConcurrencyLimit) vs. those that shouldn't (TTL, Aliases). The table-driven approach makes it easy to add more cases.
140-181: LGTM! Integration test for file watcher.Good use of
t.TempDir()for automatic cleanup andatomic.Int32for thread-safe reload counting. The timing margins (200ms wait for 50ms debounce) should be sufficient.
245-301: LGTM! End-to-end watcher integration test.Good coverage of the full workflow: start watcher, modify file, verify reload. The 2.5s sleep appropriately accounts for the 2s production debounce delay.
Note: The explicit
pm.Lock()/pm.Unlock()on lines 296-298 for accessingpm.configsuggests this is thread-safe access pattern. Verify this is consistent with other concurrent access patterns in the codebase. The earlier test at line 210-211 accessespm.config.RealModelName()without locking—ensure the difference is intentional (e.g., no concurrent modification in that test vs. active watcher goroutine here).
- Call cancel() explicitly after srv.Shutdown() instead of using defer inside the signal handling loop - Rename test "reload with invalid config keeps old config" to "reload with empty config removes all models" to better describe behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 participant
Summary
This PR refactors the existing
--watch-configfunctionality and adds SIGHUP signal support for config reloading.Key improvements:
ReloadConfig()to atomically update config without recreating ProxyManagerkill -HUP <pid>triggers immediate config reload (no debounce)New config options:
reloadRestartModels(global): Whether to restart models on config change (default: true)forceRestart(per-model): Override global setting for specific modelsBehavior
--watch-configkill -HUP <pid>docker kill -s SIGHUP)Both can be used together.
Changes
llama-swap.go: Refactored signal handling, SIGHUP supportproxy/proxymanager.go: AddedReloadConfig(),StartConfigWatcher()proxy/config_reload.go: Model diff detection logicproxy/debounce.go: Reusable debouncer utilityproxy/config/: New config fieldsRelated
This supersedes the approach in #420 by providing a cleaner implementation that avoids the race condition on
srv.Handler(identified by CodeRabbit in that PR).🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.