Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
79 changes: 79 additions & 0 deletions misc/process-cmd-test/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package main

import (
"context"
"fmt"
"os"
"os/exec"
"os/signal"
"syscall"
"time"
)

/*
**
Test how exec.Cmd.CommandContext behaves under certain conditions:*
- process is killed externally, what happens with cmd.Wait() *
✔︎ it returns. catches crashes.*
- process ignores SIGTERM*
✔︎ `kill()` is called after cmd.WaitDelay*
- this process exits, what happens with children (kill -9 <this process' pid>)*
x they stick around. have to be manually killed.*
- .WithTimeout()'s cancel is called *
✔︎ process is killed after it ignores sigterm, cmd.Wait() catches it.*
- parent receives SIGINT/SIGTERM, what happens
✔︎ waits for child process to exit, then exits gracefully.
*/
func main() {

// swap between these to use kill -9 <pid> on the cli to sim external crash
ctx, cancel := context.WithCancel(context.Background())
//ctx, cancel := context.WithTimeout(context.Background(), 1000*time.Millisecond)
defer cancel()

//cmd := exec.CommandContext(ctx, "sleep", "1")
cmd := exec.CommandContext(ctx,
"../../build/simple-responder_darwin_arm64",
"-ignore-sig-term", /* so it doesn't exit on receiving SIGTERM, test cmd.WaitTimeout */
)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr

// set a wait delay before signing sig kill
cmd.WaitDelay = 500 * time.Millisecond
cmd.Cancel = func() error {
fmt.Println("✔︎ Cancel() called, sending SIGTERM")
cmd.Process.Signal(syscall.SIGTERM)
return nil
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check error return value from Signal()

The error return value from cmd.Process.Signal should be checked, even in test code, to help debug signal delivery issues.

 	cmd.Cancel = func() error {
 		fmt.Println("✔︎ Cancel() called, sending SIGTERM")
-		cmd.Process.Signal(syscall.SIGTERM)
-		return nil
+		if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
+			fmt.Printf("Failed to send SIGTERM: %v\n", err)
+			return err
+		}
+		return nil
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cmd.Cancel = func() error {
fmt.Println("✔︎ Cancel() called, sending SIGTERM")
cmd.Process.Signal(syscall.SIGTERM)
return nil
}
cmd.Cancel = func() error {
fmt.Println("✔︎ Cancel() called, sending SIGTERM")
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
fmt.Printf("Failed to send SIGTERM: %v\n", err)
return err
}
return nil
}
🧰 Tools
🪛 golangci-lint (1.64.8)

52-52: Error return value of cmd.Process.Signal is not checked

(errcheck)

🤖 Prompt for AI Agents
In misc/process-cmd-test/main.go around lines 50 to 54, the error returned by
cmd.Process.Signal(syscall.SIGTERM) is not checked. Modify the Cancel function
to capture the error returned by Signal, check if it is non-nil, and handle it
appropriately, such as logging the error or returning it, to ensure any issues
with signal delivery are detected and can be debugged.


if err := cmd.Start(); err != nil {
fmt.Println("Error starting process:", err)
return
}

// catch signals. Calls cancel() which will cause cmd.Wait() to return and
// this program to eventually exit gracefully.
sigChan := make(chan os.Signal, 1)
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)
go func() {
signal := <-sigChan
fmt.Printf("✔︎ Received signal: %d, Killing process... with cancel before exiting\n", signal)
cancel()
}()

fmt.Printf("✔︎ Parent Pid: %d, Process Pid: %d\n", os.Getpid(), cmd.Process.Pid)
fmt.Println("✔︎ Process started, cmd.Wait() ... ")
if err := cmd.Wait(); err != nil {
fmt.Println("✔︎ cmd.Wait returned, Error:", err)
} else {
fmt.Println("✔︎ cmd.Wait returned, Process exited on its own")
}
fmt.Println("✔︎ Child process exited, Done.")
}
15 changes: 5 additions & 10 deletions proxy/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,6 @@ type Process struct {
// used to block on multiple start() calls
waitStarting sync.WaitGroup

// for managing shutdown state
shutdownCtx context.Context
shutdownCancel context.CancelFunc

// for managing concurrency limits
concurrencyLimitSemaphore chan struct{}

Expand All @@ -77,7 +73,6 @@ type Process struct {
}

func NewProcess(ID string, healthCheckTimeout int, config ModelConfig, processLogger *LogMonitor, proxyLogger *LogMonitor) *Process {
ctx, cancel := context.WithCancel(context.Background())
concurrentLimit := 10
if config.ConcurrencyLimit > 0 {
concurrentLimit = config.ConcurrencyLimit
Expand All @@ -93,8 +88,6 @@ func NewProcess(ID string, healthCheckTimeout int, config ModelConfig, processLo
healthCheckTimeout: healthCheckTimeout,
healthCheckLoopInterval: 5 * time.Second, /* default, can not be set by user - used for testing */
state: StateStopped,
shutdownCtx: ctx,
shutdownCancel: cancel,

// concurrency limit
concurrencyLimitSemaphore: make(chan struct{}, concurrentLimit),
Expand Down Expand Up @@ -266,15 +259,18 @@ func (p *Process) start() error {
loop:
// Ready Check loop
for {
currentState := p.CurrentState()
if currentState != StateStarting {
return errors.New("health check interrupted due to shutdown")
}

select {
case <-checkDeadline.Done():
if curState, err := p.swapState(StateStarting, StateFailed); err != nil {
return fmt.Errorf("health check timed out after %vs AND state swap failed: %v, current state: %v", maxDuration.Seconds(), err, curState)
} else {
return fmt.Errorf("health check timed out after %vs", maxDuration.Seconds())
}
case <-p.shutdownCtx.Done():
return errors.New("health check interrupted due to shutdown")
case exitErr := <-p.cmdWaitChan:
if exitErr != nil {
p.proxyLogger.Warnf("<%s> upstream command exited prematurely with error: %v", p.ID, exitErr)
Expand Down Expand Up @@ -392,7 +388,6 @@ func (p *Process) Shutdown() {
return
}

p.shutdownCancel()
p.stopCommand(p.gracefulStopTimeout)

// just force it to this state since there is no recovery from shutdown
Expand Down
Loading