Skip to content

fix: show errors when running a detached process #940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
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
6 changes: 5 additions & 1 deletion cmd/thv/app/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,11 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
if runForeground {
return workloadManager.RunWorkload(ctx, runConfig)
}
return workloadManager.RunWorkloadDetached(runConfig)
err = workloadManager.RunWorkloadDetached(runConfig)
if err != nil {
return fmt.Errorf("failed to run MCP workload detached: %v", err)
}
return nil
}

// parseCommandArguments processes command-line arguments to find everything after the -- separator
Expand Down
22 changes: 17 additions & 5 deletions pkg/transport/proxy/transparent/transparent_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,12 @@ func (p *TransparentProxy) Start(ctx context.Context) error {
}

func (p *TransparentProxy) monitorHealth(parentCtx context.Context) {
ticker := time.NewTicker(10 * time.Second)
ticker := time.NewTicker(30 * time.Second)
defer ticker.Stop()

var failCount int
const maxFails = 5

for {
select {
case <-parentCtx.Done():
Expand All @@ -162,11 +165,20 @@ func (p *TransparentProxy) monitorHealth(parentCtx context.Context) {
case <-ticker.C:
alive := p.healthChecker.CheckHealth(parentCtx)
if alive.Status != healthcheck.StatusHealthy {
logger.Infof("Health check failed for %s; initiating proxy shutdown", p.containerName)
if err := p.Stop(parentCtx); err != nil {
logger.Errorf("Failed to stop proxy for %s: %v", p.containerName, err)
failCount++
if failCount >= maxFails {
logger.Infof("Health check failed %d times; shutting down proxy for %s", maxFails, p.containerName)
if err := p.Stop(parentCtx); err != nil {
logger.Errorf("Failed to stop proxy for %s: %v", p.containerName, err)
}
return
}
} else {
// Reset counter on success
if failCount > 0 {
logger.Infof("Health check recovered for %s; resetting failure counter", p.containerName)
}
return
failCount = 0
}
}
}
Expand Down
27 changes: 22 additions & 5 deletions pkg/workloads/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,28 @@ func (*defaultManager) RunWorkloadDetached(runConfig *runner.RunConfig) error {
if err != nil {
logger.Warnf("Warning: Failed to create log file: %v", err)
} else {
defer logFile.Close()
logger.Infof("Logging to: %s", logFilePath)
defer func() {
// Read and print the log file contents when closing
if err := logFile.Close(); err != nil {
logger.Warnf("Warning: failed to close log file: %v", err)
return
}
// Print the log file contents
// This is done in a deferred function to ensure it runs after the log file is closed
Copy link
Member

Choose a reason for hiding this comment

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

Is the log file not closed in the block above?

defer func() {
// print the log file contents
data, err := os.ReadFile(logFilePath) // #nosec G304 - This is safe as logFilePath is controlled by the application
if err != nil {
logger.Warnf("Warning: failed to read log file: %v", err)
return
}
fmt.Println("\n\n=== Log file contents ===")
fmt.Print(string(data))
fmt.Println("=== End of log file ===")
}()
}()
logger.Infof("Logging to: %s - Please check log for more information about process success", logFilePath)

}

// Prepare the command arguments for the detached process
Expand Down Expand Up @@ -368,9 +388,6 @@ func (*defaultManager) RunWorkloadDetached(runConfig *runner.RunConfig) error {
logger.Warnf("Warning: Failed to write PID file: %v", err)
}

logger.Infof("MCP server is running in the background (PID: %d)", detachedCmd.Process.Pid)
logger.Infof("Use 'thv stop %s' to stop the server", runConfig.ContainerName)

return nil
}

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/proxy_oauth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ var _ = Describe("Proxy OAuth Authentication E2E", Serial, func() {

By("Reconnecting via MCP to trigger token refresh")
proxyURL := fmt.Sprintf("http://localhost:%d/sse", proxyPort)
err = e2e.WaitForMCPServerReady(config, proxyURL, "sse", 10*time.Second)
err = e2e.WaitForMCPServerReady(config, proxyURL, "sse", 30*time.Second)
Expect(err).ToNot(HaveOccurred(), "MCP server not ready after token expiry")

mcpClient, err := e2e.NewMCPClientForSSE(config, proxyURL)
Expand Down