Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions proxy/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,18 @@ func (p *Process) ProxyRequest(w http.ResponseWriter, r *http.Request) {
startDuration = time.Since(beginStartTime)
}

// recover from http.ErrAbortHandler panics that can occur when the client
// disconnects before the response is sent
defer func() {
if r := recover(); r != nil {
if r == http.ErrAbortHandler {
p.proxyLogger.Infof("<%s> recovered from client disconnection during streaming", p.ID)
} else {
p.proxyLogger.Infof("<%s> recovered from panic: %v", p.ID, r)
}
}
}()

if p.reverseProxy != nil {
p.reverseProxy.ServeHTTP(w, r)
} else {
Expand Down
71 changes: 71 additions & 0 deletions proxy/process_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,3 +494,74 @@ func TestProcess_EnvironmentSetCorrectly(t *testing.T) {
assert.Equal(t, len(process1.cmd.Environ())+2, len(process2.cmd.Environ()), "process2 should have 2 more environment variables than process1")

}

// TestProcess_ReverseProxyPanicIsHandled tests that panics from
// httputil.ReverseProxy in Process.ProxyRequest(w, r) do not bubble up and are
// handled appropriately.
//
// httputil.ReverseProxy will panic with http.ErrAbortHandler when it has sent headers
// can't copy the body. This can be caused by a client disconnecting before the full
// response is sent from some reason.
//
// bug: https://github.com/mostlygeek/llama-swap/issues/362
// see: https://github.com/golang/go/issues/23643 (where panic was added to httputil.ReverseProxy)
func TestProcess_ReverseProxyPanicIsHandled(t *testing.T) {
Comment on lines +498 to +508
Copy link

@coderabbitai coderabbitai bot Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Confirm linked deadlock fix (ProcessGroup defer Unlock) is present

PR objectives mention deferring pg.Unlock() in ProcessGroup.ProxyRequest to avoid deadlocks on panic. This file doesn’t show that change. Please verify it exists.

Run:


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "Locate ProcessGroup.ProxyRequest and lock/unlock usage..."
# Show candidate files
fd -t f -e go | xargs -I{} rg -nH -C3 'type\s+ProcessGroup\b|ProxyRequest\s*\(' {} || true
echo "Check for defer Unlock in ProcessGroup.ProxyRequest:"
rg -nP --type go '(?s)func\s*\(\s*\*?ProcessGroup\s*\)\s*ProxyRequest\s*\(.*\)\s*\{.*' -n | cut -d: -f1 | sort -u | while read -r f; do
  echo "== $f ==";
  rg -n -C3 'func\s*\(\s*\*?ProcessGroup\s*\)\s*ProxyRequest\s*\(' "$f";
  rg -n -C2 'Lock\(|Unlock\(|defer\s+.*Unlock\(' "$f";
done

Length of output: 11505


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== Full ProcessGroup.ProxyRequest method ==="
sed -n '57,85p' proxy/processgroup.go

Length of output: 796


Add defer pg.Unlock() immediately after pg.Lock() in ProcessGroup.ProxyRequest to prevent deadlock on panic

The ProxyRequest call at line 73 in proxy/processgroup.go can panic (as mentioned in the test comment). Currently, the unlock is deferred only via direct calls (lines 80, 82), which will not execute if a panic occurs during the ProxyRequest call. This leaves the mutex locked indefinitely, causing a deadlock. Add defer pg.Unlock() right after the pg.Lock() call on line 63.

🤖 Prompt for AI Agents
In proxy/processgroup.go around lines 63 to 82, the function locks the
ProcessGroup with pg.Lock() but only calls pg.Unlock() in code paths that won't
run if ProxyRequest panics, causing a deadlock; add defer pg.Unlock()
immediately after pg.Lock() (line 63) so the mutex is always released on panic,
and remove or omit the later explicit pg.Unlock() calls in that function to
avoid double-unlocking.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK because Process.ProxyRequest(w,r) will recover() from any panics. The condition that created the original deadlock should not be able to happen anymore.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mostlygeek, understood! You're absolutely right—with the recover() now in place within Process.ProxyRequest, any panic (including http.ErrAbortHandler from httputil.ReverseProxy) will be caught there and won't propagate up to ProcessGroup.ProxyRequest. This means the mutex in ProcessGroup.ProxyRequest will be released normally via the existing pg.Unlock() calls, preventing the deadlock scenario described in issue #362.

Thank you for the clarification!

// Add defer/recover to catch any panics that aren't handled by ProxyRequest
// If this recover() is hit, it means ProxyRequest didn't handle the panic properly
defer func() {
if r := recover(); r != nil {
t.Fatalf("ProxyRequest should handle panics from reverseProxy.ServeHTTP, but panic was not caught: %v", r)
}
}()

expectedMessage := "panic_test"
config := getTestSimpleResponderConfig(expectedMessage)

process := NewProcess("panic-test", 5, config, debugLogger, debugLogger)
defer process.Stop()

// Start the process
err := process.start()
assert.Nil(t, err)
assert.Equal(t, StateReady, process.CurrentState())

// Create a custom ResponseWriter that simulates a client disconnect
// by panicking when Write is called after headers are sent
panicWriter := &panicOnWriteResponseWriter{
ResponseRecorder: httptest.NewRecorder(),
shouldPanic: true,
}

// Make a request that will trigger the panic
req := httptest.NewRequest("GET", "/slow-respond?echo=test&delay=100ms", nil)

// This should panic inside reverseProxy.ServeHTTP when the panicWriter.Write() is called.
// ProxyRequest should catch and handle this panic gracefully.
process.ProxyRequest(panicWriter, req)

// If we get here, the panic was properly recovered in ProxyRequest
// The process should still be in a ready state
assert.Equal(t, StateReady, process.CurrentState())
}

// panicOnWriteResponseWriter is a ResponseWriter that panics on Write
// to simulate a client disconnect after headers are sent
// used by: TestProcess_ReverseProxyPanicIsHandled
type panicOnWriteResponseWriter struct {
*httptest.ResponseRecorder
shouldPanic bool
headerWritten bool
}

func (w *panicOnWriteResponseWriter) WriteHeader(statusCode int) {
w.headerWritten = true
w.ResponseRecorder.WriteHeader(statusCode)
}

func (w *panicOnWriteResponseWriter) Write(b []byte) (int, error) {
if w.shouldPanic && w.headerWritten {
// Simulate the panic that httputil.ReverseProxy throws
panic(http.ErrAbortHandler)
}
return w.ResponseRecorder.Write(b)
}