Fix #133 SanitizeCommand removes comments (#134)

* Add windows specific test changes
* Change the command line parsing library - Possible breaking changes for windows users!

.github/ISSUE_TEMPLATE/bug-report.md

@@ -0,0 +1,37 @@
+---
+name: Bug Report
+about: Something is not working as expected...
+title: ''
+labels: bug
+assignees: ''
+
+---
+
+**Describe the bug**
+A clear and concise description of what the bug is.
+
+**Expected behaviour**
+A clear and concise description of what you expected to happen.
+
+**Operating system and version**
+
+- OS: (linux, osx, windows, freebsd, etc)
+- GPUs: (list architecture)
+
+**My Configuration**
+
+```yaml
+# copy / paste your configuration here
+```
+
+**Proxy Logs**
+
+```
+# copy / paste from /logs
+```
+
+**Upstream Logs**
+
+```
+# copy/paste from /logs
+```

.github/workflows/go-ci-windows.yml

@@ -0,0 +1,50 @@
+name: Windows CI
+
+on:
+  push:
+    branches: [ "main" ]
+
+  pull_request:
+    branches: [ "main" ]
+
+  # Allows manual triggering of the workflow
+  workflow_dispatch:
+
+jobs:
+
+  run-tests:
+    runs-on: windows-latest
+    steps:
+    - uses: actions/checkout@v4
+
+    - name: Set up Go
+      uses: actions/setup-go@v4
+      with:
+        go-version: '1.23'
+
+    # cache simple-responder to save the build time
+    - name: Restore Simple Responder
+      id: restore-simple-responder
+      uses: actions/cache/restore@v4
+      with:
+        path: ./build
+        key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}
+
+    # necessary for testing proxy/Process swapping
+    - name: Create simple-responder
+      if: steps.restore-simple-responder.outputs.cache-hit != 'true'
+      shell: bash
+      run: make simple-responder-windows
+
+    - name: Save Simple Responder
+      # nothing new to save ... skip this step
+      if: steps.restore-simple-responder.outputs.cache-hit != 'true'
+      id: save-simple-responder
+      uses: actions/cache/save@v4
+      with:
+        path: ./build
+        key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}
+
+    - name: Test all
+      shell: bash
+      run: make test-all

.github/workflows/go-ci.yml

@@ -1,6 +1,4 @@
-# This workflow will build a golang project
-
-name: CI
+name: Linux CI
 
 on:
   push:
@@ -24,9 +22,26 @@ jobs:
       with:
         go-version: '1.23'
 
+    # cache simple-responder to save the build time
+    - name: Restore Simple Responder
+      id: restore-simple-responder
+      uses: actions/cache/restore@v4
+      with:
+        path: ./build
+        key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}
+
     # necessary for testing proxy/Process swapping
     - name: Create simple-responder
       run: make simple-responder
 
+    - name: Save Simple Responder
+      # nothing new to save ... skip this step
+      if: steps.restore-simple-responder.outputs.cache-hit != 'true'
+      id: save-simple-responder
+      uses: actions/cache/save@v4
+      with:
+        path: ./build
+        key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}
+
     - name: Test all
       run: make test-all

Makefile

@@ -46,6 +46,10 @@ simple-responder:
 	GOOS=darwin GOARCH=arm64 go build -o $(BUILD_DIR)/simple-responder_darwin_arm64 misc/simple-responder/simple-responder.go
 	GOOS=linux GOARCH=amd64 go build -o $(BUILD_DIR)/simple-responder_linux_amd64 misc/simple-responder/simple-responder.go
 
+simple-responder-windows:
+	@echo "Building simple responder for windows"
+	GOOS=windows GOARCH=amd64 go build -o $(BUILD_DIR)/simple-responder.exe misc/simple-responder/simple-responder.go
+
 # Ensure build directory exists
 $(BUILD_DIR):
 	mkdir -p $(BUILD_DIR)

go.mod

@@ -5,7 +5,6 @@ go 1.23.0
 require (
 	github.com/fsnotify/fsnotify v1.9.0
 	github.com/gin-gonic/gin v1.10.0
-	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
 	github.com/stretchr/testify v1.9.0
 	github.com/tidwall/gjson v1.18.0
 	github.com/tidwall/sjson v1.2.5
@@ -13,6 +12,7 @@ require (
 )
 
 require (
+	github.com/billziss-gh/golib v0.2.0 // indirect
 	github.com/bytedance/sonic v1.11.6 // indirect
 	github.com/bytedance/sonic/loader v0.1.1 // indirect
 	github.com/cloudwego/base64x v0.1.4 // indirect

go.sum

@@ -1,3 +1,5 @@
+github.com/billziss-gh/golib v0.2.0 h1:NyvcAQdfvM8xokKkKotiligKjKXzuQD4PPykg1nKc/8=
+github.com/billziss-gh/golib v0.2.0/go.mod h1:mZpUYANXZkDKSnyYbX9gfnyxwe0ddRhUtfXcsD5r8dw=
 github.com/bytedance/sonic v1.11.6 h1:oUp34TzMlL+OY1OUWxHqsdkgC/Zfc85zGqw9siXjrc0=
 github.com/bytedance/sonic v1.11.6/go.mod h1:LysEHSvpvDySVdC2f87zGWf6CIKJcAvqab1ZaiQtds4=
 github.com/bytedance/sonic/loader v0.1.1 h1:c+e5Pt1k/cy5wMveRDyk2X4B9hF4g7an8N3zCYjJFNM=

proxy/config.go

@@ -4,11 +4,12 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"runtime"
 	"sort"
 	"strconv"
 	"strings"
 
-	"github.com/google/shlex"
+	"github.com/billziss-gh/golib/shlex"
 	"gopkg.in/yaml.v3"
 )
 
@@ -228,14 +229,30 @@ func AddDefaultGroupToConfig(config Config) Config {
 }
 
 func SanitizeCommand(cmdStr string) ([]string, error) {
-	// Remove trailing backslashes
-	cmdStr = strings.ReplaceAll(cmdStr, "\\ \n", " ")
-	cmdStr = strings.ReplaceAll(cmdStr, "\\\n", " ")
+	var cleanedLines []string
+	for _, line := range strings.Split(cmdStr, "\n") {
+		trimmed := strings.TrimSpace(line)
+		// Skip comment lines
+		if strings.HasPrefix(trimmed, "#") {
+			continue
+		}
+		// Handle trailing backslashes by replacing with space
+		if strings.HasSuffix(trimmed, "\\") {
+			cleanedLines = append(cleanedLines, strings.TrimSuffix(trimmed, "\\")+" ")
+		} else {
+			cleanedLines = append(cleanedLines, line)
+		}
+	}
+
+	// put it back together
+	cmdStr = strings.Join(cleanedLines, "\n")
 
 	// Split the command into arguments
-	args, err := shlex.Split(cmdStr)
-	if err != nil {
-		return nil, err
+	var args []string
+	if runtime.GOOS == "windows" {
+		args = shlex.Windows.Split(cmdStr)
+	} else {
+		args = shlex.Posix.Split(cmdStr)
 	}
 
 	// Ensure the command is not empty

proxy/config_posix_test.go

@@ -0,0 +1,42 @@
+//go:build !windows
+
+package proxy
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestConfig_SanitizeCommand(t *testing.T) {
+	// Test a command with spaces and newlines
+	args, err := SanitizeCommand(`python model1.py \
+		-a "double quotes" \
+		--arg2 'single quotes'
+		-s
+		# comment 1
+		--arg3 123 \
+
+		  # comment 2
+		--arg4 '"string in string"'
+
+
+		# this will get stripped out as well as the white space above
+		-c "'single quoted'"
+		`)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"python", "model1.py",
+		"-a", "double quotes",
+		"--arg2", "single quotes",
+		"-s",
+		"--arg3", "123",
+		"--arg4", `"string in string"`,
+		"-c", `'single quoted'`,
+	}, args)
+
+	// Test an empty command
+	args, err = SanitizeCommand("")
+	assert.Error(t, err)
+	assert.Nil(t, args)
+}

proxy/config_test.go

@@ -258,34 +258,6 @@ func TestConfig_FindConfig(t *testing.T) {
 	assert.Equal(t, ModelConfig{}, modelConfig)
 }
 
-func TestConfig_SanitizeCommand(t *testing.T) {
-
-	// Test a command with spaces and newlines
-	args, err := SanitizeCommand(`python model1.py \
-    -a "double quotes" \
-    --arg2 'single quotes'
-	-s
-	--arg3 123 \
-	--arg4 '"string in string"'
-	-c "'single quoted'"
-	`)
-	assert.NoError(t, err)
-	assert.Equal(t, []string{
-		"python", "model1.py",
-		"-a", "double quotes",
-		"--arg2", "single quotes",
-		"-s",
-		"--arg3", "123",
-		"--arg4", `"string in string"`,
-		"-c", `'single quoted'`,
-	}, args)
-
-	// Test an empty command
-	args, err = SanitizeCommand("")
-	assert.Error(t, err)
-	assert.Nil(t, args)
-}
-
 func TestConfig_AutomaticPortAssignments(t *testing.T) {
 
 	t.Run("Default Port Ranges", func(t *testing.T) {

proxy/config_windows_test.go

@@ -0,0 +1,42 @@
+//go:build windows
+
+package proxy
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestConfig_SanitizeCommand(t *testing.T) {
+	// does not support single quoted strings like in config_posix_test.go
+	args, err := SanitizeCommand(`python model1.py \
+
+	-a "double quotes" \
+	-s
+	--arg3 123 \
+
+	   # comment 2
+	--arg4 '"string in string"'
+
+
+
+	# this will get stripped out as well as the white space above
+	-c "'single quoted'"
+	`)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"python", "model1.py",
+		"-a", "double quotes",
+		"-s",
+		"--arg3", "123",
+		"--arg4", "'string in string'", // this is a little weird but the lexer says so...?
+		"-c", `'single quoted'`,
+	}, args)
+
+	// Test an empty command
+	args, err = SanitizeCommand("")
+	assert.Error(t, err)
+	assert.Nil(t, args)
+
+}

proxy/helpers_test.go

@@ -45,7 +45,12 @@ func TestMain(m *testing.M) {
 func getSimpleResponderPath() string {
 	goos := runtime.GOOS
 	goarch := runtime.GOARCH
-	return filepath.Join("..", "build", fmt.Sprintf("simple-responder_%s_%s", goos, goarch))
+
+	if goos == "windows" {
+		return filepath.Join("..", "build", "simple-responder.exe")
+	} else {
+		return filepath.Join("..", "build", fmt.Sprintf("simple-responder_%s_%s", goos, goarch))
+	}
 }
 
 func getTestPort() int {

proxy/process.go

@@ -70,6 +70,9 @@ type Process struct {
 
 	// stop timeout waiting for graceful shutdown
 	gracefulStopTimeout time.Duration
+
+	// track that this happened
+	upstreamWasStoppedWithKill bool
 }
 
 func NewProcess(ID string, healthCheckTimeout int, config ModelConfig, processLogger *LogMonitor, proxyLogger *LogMonitor) *Process {
@@ -97,7 +100,8 @@ func NewProcess(ID string, healthCheckTimeout int, config ModelConfig, processLo
 		concurrencyLimitSemaphore: make(chan struct{}, concurrentLimit),
 
 		// stop timeout
-		gracefulStopTimeout: 5 * time.Second,
+		gracefulStopTimeout:        5 * time.Second,
+		upstreamWasStoppedWithKill: false,
 	}
 }
 
@@ -217,9 +221,12 @@ func (p *Process) start() error {
 
 		// there is a race condition when SIGKILL is used, p.cmd.Wait() returns, and then
 		// the code below fires, putting an error into cmdWaitChan. This code is to prevent this
-		if exitErr != nil && exitErr.Error() == "signal: killed" {
+		if p.upstreamWasStoppedWithKill {
+			p.proxyLogger.Debugf("<%s> process was killed, NOT sending exitErr: %v", p.ID, exitErr)
+			p.upstreamWasStoppedWithKill = false
 			return
 		}
+
 		p.cmdWaitChan <- exitErr
 	}()
 
@@ -400,6 +407,7 @@ func (p *Process) stopCommand(sigtermTTL time.Duration) {
 	select {
 	case <-sigtermTimeout.Done():
 		p.proxyLogger.Debugf("<%s> Process timed out waiting to stop, sending KILL signal (normal during shutdown)", 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)
 		}

proxy/process_test.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"runtime"
 	"sync"
 	"testing"
 	"time"
@@ -432,7 +433,12 @@ func TestProcess_ForceStopWithKill(t *testing.T) {
 
 		// unexpected EOF because the kill happened, the "1" is sent before the kill
 		// then the unexpected EOF is sent after the kill
-		assert.Equal(t, "1unexpected EOF\n", w.Body.String())
+		if runtime.GOOS == "windows" {
+			assert.Contains(t, w.Body.String(), "wsarecv: An existing connection was forcibly closed by the remote host")
+		} else {
+			assert.Contains(t, w.Body.String(), "unexpected EOF")
+		}
+
 		close(waitChan)
 	}()
 

proxy/proxymanager.go

@@ -374,7 +374,8 @@ func (pm *ProxyManager) proxyOAIHandler(c *gin.Context) {
 
 	// dechunk it as we already have all the body bytes see issue #11
 	c.Request.Header.Del("transfer-encoding")
-	c.Request.Header.Add("content-length", strconv.Itoa(len(bodyBytes)))
+	c.Request.Header.Set("content-length", strconv.Itoa(len(bodyBytes)))
+	c.Request.ContentLength = int64(len(bodyBytes))
 
 	if err := processGroup.ProxyRequest(realModelName, c.Writer, c.Request); err != nil {
 		pm.sendErrorResponse(c, http.StatusInternalServerError, fmt.Sprintf("error proxying request: %s", err.Error()))

proxy/proxymanager_test.go

@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/tidwall/gjson"
 )
 
 func TestProxyManager_SwapProcessCorrectly(t *testing.T) {
@@ -448,7 +449,6 @@ func TestProxyManager_AudioTranscriptionHandler(t *testing.T) {
 // Test useModelName in configuration sends overrides what is sent to upstream
 func TestProxyManager_UseModelName(t *testing.T) {
 	upstreamModelName := "upstreamModel"
-
 	modelConfig := getTestSimpleResponderConfig(upstreamModelName)
 	modelConfig.UseModelName = upstreamModelName
 
@@ -473,6 +473,12 @@ func TestProxyManager_UseModelName(t *testing.T) {
 		proxy.ServeHTTP(w, req)
 		assert.Equal(t, http.StatusOK, w.Code)
 		assert.Contains(t, w.Body.String(), upstreamModelName)
+
+		// make sure the content length was set correctly
+		// simple-responder will return the content length it got in the response
+		body := w.Body.Bytes()
+		contentLength := int(gjson.GetBytes(body, "h_content_length").Int())
+		assert.Equal(t, len(fmt.Sprintf(`{"model":"%s"}`, upstreamModelName)), contentLength)
 	})
 
 	t.Run("useModelName over rides requested model: /v1/audio/transcriptions", func(t *testing.T) {