Add stopCmd to model configuration #136
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigLoader
participant ModelConfig
participant Process
User->>ConfigLoader: Load configuration
ConfigLoader->>ModelConfig: Initialize ModelConfig
alt OS is Windows and CmdStop is empty
ModelConfig->>ModelConfig: Set CmdStop to default Windows stop command
end
ConfigLoader->>Process: Pass ModelConfig
User->>Process: Start process
Process->>Process: Run command
User->>Process: StopImmediately()
alt CmdStop is set
Process->>Process: Replace ${PID} in CmdStop
Process->>Process: Execute stop command
else
Process->>Process: Send SIGTERM to process
end
sequenceDiagram
participant Test
participant ConfigLoader
participant ModelConfig
participant Process
Test->>ConfigLoader: Load config with custom CmdStop
ConfigLoader->>ModelConfig: Assign CmdStop from config
ConfigLoader->>Process: Pass ModelConfig
Test->>Process: Start()
Process->>Process: Process starts
Test->>Process: StopImmediately()
Process->>Process: Use custom CmdStop for termination
Process->>Test: State transitions to Stopped
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
proxy/process.go (1)
403-405: Clean up commented-out code.Remove the commented-out code as it's no longer needed and is being replaced with the new implementation below.
- // if err := p.terminateProcess(); err != nil { - // p.proxyLogger.Debugf("<%s> Process already terminated: %v (normal during shutdown)", p.ID, err) - // }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
proxy/config.go(3 hunks)proxy/config_windows_test.go(2 hunks)proxy/process.go(1 hunks)proxy/process_stop.go(0 hunks)proxy/process_stop_windows.go(0 hunks)proxy/process_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- proxy/process_stop.go
- proxy/process_stop_windows.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
proxy/process_test.go (1)
proxy/process.go (4)
NewProcess(78-106)StateReady(23-23)StopImmediately(36-36)StateStopped(21-21)
proxy/config_windows_test.go (1)
proxy/config.go (1)
LoadConfigFromReader(104-192)
proxy/process.go (1)
proxy/config.go (1)
SanitizeCommand(239-272)
🪛 GitHub Actions: Windows CI
proxy/process_test.go
[error] 60-60: TestProcess_AutomaticallyStartsUpstream failed: Expected status code 200, got 502. Also, failed to send SIGTERM to process: not supported by windows.
[error] 437-437: TestProcess_ForceStopWithKill failed: Expected error message containing 'wsarecv: An existing connection was forcibly closed by the remote host' but got '12345'. Failed to send SIGTERM to process: not supported by windows.
[error] 1-1: TestProcess_StopCmd encountered error: 'kill: 3364: No such process' and failed to exec stop command with exit status 1.
proxy/config_windows_test.go
[error] 52-53: TestConfig_WindowsCmdStopIsSet failed due to YAML parsing error: 'yaml: line 2: found character that cannot start any token'. Also, expected command 'taskkill /f /t /pid ${PID}' was not found.
🔇 Additional comments (4)
proxy/config.go (2)
20-20: Good addition of the new CmdStop field.The new field allows configuring custom stop commands for different platforms, which aligns with the PR objective of adding a stopCmd to model configuration.
163-168: Good implementation of Windows-specific default stop command.Setting a default Windows stop command if none is provided is a great approach that ensures Windows systems have a platform-appropriate termination method. The
taskkill /f /t /pid ${PID}command is correctly used to forcibly terminate processes and their child processes on Windows.proxy/process.go (2)
407-426: Well-implemented custom stop command execution.This is a good implementation that:
- Replaces the placeholder with the actual PID
- Properly sanitizes the command
- Sets up output redirection
- Handles execution errors
This flexible approach allows for platform-specific termination commands.
427-431:❓ Verification inconclusive
Handle Windows-specific signal limitations.
The SIGTERM fallback won't work on Windows (error:
Failed to send SIGTERM to process: not supported by windows), but this code path should rarely be taken on Windows since a default stop command is set during config loading.Consider adding a runtime check to handle Windows differently:
} else { + if runtime.GOOS == "windows" { + p.proxyLogger.Errorf("<%s> No CmdStop defined and SIGTERM not supported on Windows", p.ID) + // Fall back to Kill as last resort + p.upstreamWasStoppedWithKill = true + if err := p.cmd.Process.Kill(); err != nil { + p.proxyLogger.Errorf("<%s> Failed to kill process: %v", p.ID, err) + } + return + } if err := p.cmd.Process.Signal(syscall.SIGTERM); err != nil { p.proxyLogger.Errorf("<%s> Failed to send SIGTERM to process: %v", p.ID, err) return } }Let's verify how frequently this code path might be hit on Windows:
🏁 Script executed:
#!/bin/bash # Check if there are any cases where a ModelConfig on Windows could have empty CmdStop # Look for any test code that creates ModelConfig without setting CmdStop rg "ModelConfig\s*{\s*[^}]*\s*}" --type go | grep -v "CmdStop" # Look for any code that directly modifies CmdStop to empty rg "config.*CmdStop\s*=\s*\"\"" --type goLength of output: 229
proxy/process.go (lines 427–431): Add Windows‐specific fallback for process termination
Snippet showing current state:
if err := p.cmd.Process.Signal(syscall.SIGTERM); err != nil { p.proxyLogger.Errorf("<%s> Failed to send SIGTERM to process: %v", p.ID, err) return } }Handle Windows‐specific signal limitations. On Windows
SIGTERMis unsupported, so we should detectruntime.GOOS == "windows"and fall back toKill:} else { + if runtime.GOOS == "windows" { + p.proxyLogger.Errorf("<%s> No CmdStop defined and SIGTERM not supported on Windows", p.ID) + p.upstreamWasStoppedWithKill = true + if err := p.cmd.Process.Kill(); err != nil { + p.proxyLogger.Errorf("<%s> Failed to kill process: %v", p.ID, err) + } + return + } if err := p.cmd.Process.Signal(syscall.SIGTERM); err != nil { p.proxyLogger.Errorf("<%s> Failed to send SIGTERM to process: %v", p.ID, err) return } }Verification attempt:
rg 'ModelConfig\s*{\s*[^}]*}' --type go | grep -v 'CmdStop' rg 'config.*CmdStop\s*=\s*""' --type goThe first command failed due to regex parsing issues, so we could not confirm automatically whether any
ModelConfigever omitsCmdStop. Please manually verify that every config on Windows has a non‐emptyCmdStop, or apply this fallback to guard against the rare case where it does not.
Ref: #130
cmdStopconfiguration option for models.taskkill /f /t /p ${PID}Process.terminateProcess()functionsWhere this might be useful:
cmdStopcan bepodman kill my-containerordocker stop my-containerSummary by CodeRabbit