Skip to content

Add panic recovery to mcp inspect background goroutines#33217

Merged
pelikhan merged 2 commits into
mainfrom
copilot/fix-goroutine-recovery-convention
May 19, 2026
Merged

Add panic recovery to mcp inspect background goroutines#33217
pelikhan merged 2 commits into
mainfrom
copilot/fix-goroutine-recovery-convention

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 19, 2026

pkg/cli/mcp_inspect_inspector.go had two production goroutines that did not follow the codebase’s panic-isolation convention. A panic in either path could bypass graceful background cleanup and make gh aw mcp inspect fail noisily instead of containing the fault.

  • What changed

    • Added the standard defer recover() guard to the MCP server monitor goroutine.
    • Added the same guard to the cleanup wait goroutine that gates shutdown completion.
  • Why these paths matter

    • The server monitor goroutine waits on background MCP server processes and reports exit errors.
    • The cleanup wait goroutine signals completion of process shutdown; if it ever panics, the parent path falls back to the timeout branch instead of completing cleanly.
  • Result

    • All six production go func() sites under pkg/ now follow the same panic-recovery pattern used elsewhere in the CLI.
go func(serverCmd *exec.Cmd, serverName string) {
	defer wg.Done()
	defer func() {
		if r := recover(); r != nil {
			mcpInspectorLog.Printf("Panic in MCP server monitor for %s (recovered): %v", serverName, r)
		}
	}()

	if err := serverCmd.Wait(); err != nil && verbose {
		fmt.Fprintln(os.Stderr, console.FormatWarningMessage(
			fmt.Sprintf("Server %s exited with error: %v", serverName, err),
		))
	}
}(cmd, config.Name)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix two goroutines to follow defer recover convention Add panic recovery to mcp inspect background goroutines May 19, 2026
Copilot AI requested a review from pelikhan May 19, 2026 05:36
@pelikhan pelikhan marked this pull request as ready for review May 19, 2026 08:23
Copilot AI review requested due to automatic review settings May 19, 2026 08:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds panic recovery to two background goroutines in the mcp inspect command path so that panics are logged rather than crashing the CLI, aligning these sites with the codebase's existing panic-isolation convention.

Changes:

  • Add defer recover() to the MCP server monitor goroutine that waits on server processes.
  • Add defer recover() to the cleanup wait goroutine that signals shutdown completion.
Show a summary per file
File Description
pkg/cli/mcp_inspect_inspector.go Adds panic recovery guards to two background goroutines, logging recovered panics via mcpInspectorLog.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@pelikhan pelikhan merged commit d05cad8 into main May 19, 2026
4 checks passed
@pelikhan pelikhan deleted the copilot/fix-goroutine-recovery-convention branch May 19, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two production goroutines in pkg/cli/mcp_inspect_inspector.go skip the codebase's defer recover() convention

3 participants