From be321a38b5a69703248dec906af53c6866109ddf Mon Sep 17 00:00:00 2001 From: carlos <21148423+carlos-gn@users.noreply.github.com> Date: Tue, 3 Feb 2026 15:51:31 +0100 Subject: [PATCH 1/5] Add inspector auto-cleanup on interrupt Handle SIGINT/SIGTERM signals in the inspector command to automatically remove the inspector container when interrupted. Previously, interrupting left orphaned containers requiring manual cleanup. - Add signal handling with cleanup on Ctrl+C - Clean up container in both ready and startup states - Add E2E tests for auto-cleanup scenarios - Update existing tests to verify cleanup behavior Affected components: cmd/thv/app/inspector.go, test/e2e/ Signed-off-by: carlos <21148423+carlos-gn@users.noreply.github.com> --- cmd/thv/app/inspector.go | 51 ++- test/e2e/inspector_autocleanup_test.go | 412 +++++++++++++++++++++++++ test/e2e/inspector_test.go | 116 +------ 3 files changed, 469 insertions(+), 110 deletions(-) create mode 100644 test/e2e/inspector_autocleanup_test.go diff --git a/cmd/thv/app/inspector.go b/cmd/thv/app/inspector.go index 539406d7e9..48d2fd573a 100644 --- a/cmd/thv/app/inspector.go +++ b/cmd/thv/app/inspector.go @@ -9,7 +9,9 @@ import ( "encoding/hex" "fmt" "net/http" + "os/signal" "strconv" + "syscall" "time" "github.com/spf13/cobra" @@ -37,8 +39,7 @@ func inspectorCommand() *cobra.Command { Use: "inspector [workload-name]", Short: "Launches the MCP Inspector UI and connects it to the specified MCP server", Long: `Launches the MCP Inspector UI and connects it to the specified MCP server`, - Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { return inspectorCmdFunc(cmd, args) }, } @@ -92,7 +93,8 @@ func waitForInspectorReady(ctx context.Context, port int, statusChan chan bool) } func inspectorCmdFunc(cmd *cobra.Command, args []string) error { - ctx := cmd.Context() + ctx, stopSignal := signal.NotifyContext(cmd.Context(), syscall.SIGINT, syscall.SIGTERM) + defer stopSignal() // Get server name from args if len(args) == 0 || args[0] == "" { @@ -178,8 +180,23 @@ func inspectorCmdFunc(cmd *cobra.Command, args []string) error { "http://localhost:%d?transport=%s&serverUrl=http://host.docker.internal:%d/%s&MCP_PROXY_AUTH_TOKEN=%s", inspectorUIPort, transportTypeStr, serverPort, suffix, authToken) logger.Infof("Inspector UI is now available at %s", inspectorURL) + logger.Infof("Press Ctrl+C to stop the inspector container") + + <-ctx.Done() + logger.Info("Interrupt received, stopping inspector container...") + + cleanupErr := cleanupInspectorContainer(context.Background(), "inspector") + if cleanupErr != nil { + logger.Warnf("Failed to cleanup inspector container: %v", cleanupErr) + } else { + logger.Info("Inspector container cleaned up successfully") + } return nil case <-ctx.Done(): + logger.Info("Context cancelled during inspector startup, cleaning up...") + if cleanupErr := cleanupInspectorContainer(context.Background(), "inspector"); cleanupErr != nil { + logger.Warnf("Failed to cleanup inspector container: %v", cleanupErr) + } return fmt.Errorf("context cancelled while waiting for workload to start") } } @@ -214,3 +231,31 @@ func getServerPortAndTransport(ctx context.Context, serverName string) (int, typ return 0, types.TransportTypeSSE, fmt.Errorf("server with name %s not found", serverName) } + +func cleanupInspectorContainer(ctx context.Context, name string) error { + rt, err := container.NewFactory().Create(ctx) + if err != nil { + return fmt.Errorf("failed to create runtime for cleanup: %w", err) + } + + manager, err := workloads.NewManagerFromRuntime(rt) + if err != nil { + return fmt.Errorf("failed to create workload manager for cleanup: %w", err) + } + + cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + complete, err := manager.DeleteWorkloads(cleanupCtx, []string{name}) + if err != nil { + return fmt.Errorf("failed to cleanup inspector container: %w", err) + } + + if complete != nil { + if err := complete(); err != nil { + return fmt.Errorf("cleanup completion error: %w", err) + } + } + + return nil +} diff --git a/test/e2e/inspector_autocleanup_test.go b/test/e2e/inspector_autocleanup_test.go new file mode 100644 index 0000000000..4bce04c5a9 --- /dev/null +++ b/test/e2e/inspector_autocleanup_test.go @@ -0,0 +1,412 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e_test + +import ( + "context" + "fmt" + "net/http" + "os/exec" + "strings" + "syscall" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/stacklok/toolhive/test/e2e" +) + +// inspectorAutoCleanupTestHelper contains functionality for testing inspector auto-cleanup +type inspectorAutoCleanupTestHelper struct { + config *e2e.TestConfig + mcpServerName string + inspectorName string // Always "inspector" + inspectorCmd *exec.Cmd + httpClient *http.Client + inspectorURL string +} + +// newInspectorAutoCleanupTestHelper creates a new test helper for auto-cleanup testing +func newInspectorAutoCleanupTestHelper(config *e2e.TestConfig, mcpServerName string) *inspectorAutoCleanupTestHelper { + return &inspectorAutoCleanupTestHelper{ + config: config, + mcpServerName: mcpServerName, + inspectorName: "inspector", + httpClient: &http.Client{Timeout: 5 * time.Second}, + } +} + +// setupMCPServer starts an MCP server for the inspector to connect to +func (h *inspectorAutoCleanupTestHelper) setupMCPServer() { + By("Starting an MCP server for inspector to connect to") + e2e.NewTHVCommand(h.config, "run", "--name", h.mcpServerName, "fetch").ExpectSuccess() + err := e2e.WaitForMCPServer(h.config, h.mcpServerName, 60*time.Second) + Expect(err).ToNot(HaveOccurred(), "MCP server should be running") +} + +// startInspector starts the inspector command and returns the process +func (h *inspectorAutoCleanupTestHelper) startInspector() { + args := []string{"inspector", h.mcpServerName} + GinkgoWriter.Printf("Starting inspector with args: %v\n", args) + + cmd := e2e.StartLongRunningTHVCommand(h.config, args...) + h.inspectorCmd = cmd + + // Build the expected inspector URL + h.inspectorURL = "http://localhost:6274" +} + +// interruptInspector sends SIGINT to the inspector process +func (h *inspectorAutoCleanupTestHelper) interruptInspector() error { + if h.inspectorCmd == nil { + return fmt.Errorf("inspector command not started") + } + + GinkgoWriter.Printf("Sending SIGINT to inspector process (PID: %d)\n", h.inspectorCmd.Process.Pid) + return h.inspectorCmd.Process.Signal(syscall.SIGINT) +} + +// waitForInspectorReady waits for the inspector UI to become accessible +func (h *inspectorAutoCleanupTestHelper) waitForInspectorReady(timeout time.Duration) error { + GinkgoWriter.Printf("Waiting for inspector UI to be ready at %s\n", h.inspectorURL) + + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + ticker := time.NewTicker(2 * time.Second) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return fmt.Errorf("timeout waiting for inspector UI to be ready") + case <-ticker.C: + resp, err := h.httpClient.Get(h.inspectorURL) + if err == nil && resp.StatusCode == 200 { + _ = resp.Body.Close() + GinkgoWriter.Println("Inspector UI is ready") + return nil + } + if resp != nil { + _ = resp.Body.Close() + } + GinkgoWriter.Println("Inspector UI not ready yet, waiting...") + } + } +} + +// waitForInspectorExit waits for the inspector process to exit +func (h *inspectorAutoCleanupTestHelper) waitForInspectorExit(timeout time.Duration) error { + if h.inspectorCmd == nil { + return fmt.Errorf("inspector command not started") + } + + GinkgoWriter.Printf("Waiting for inspector process to exit (timeout: %v)\n", timeout) + + done := make(chan error, 1) + go func() { + done <- h.inspectorCmd.Wait() + }() + + select { + case err := <-done: + GinkgoWriter.Printf("Inspector process exited with error: %v\n", err) + return nil + case <-time.After(timeout): + return fmt.Errorf("timeout waiting for inspector process to exit") + } +} + +// verifyInspectorContainerExists checks if the inspector container exists +func (h *inspectorAutoCleanupTestHelper) verifyInspectorContainerExists() bool { + stdout, _ := e2e.NewTHVCommand(h.config, "list", "--all").ExpectSuccess() + return strings.Contains(stdout, h.inspectorName) +} + +// verifyInspectorContainerGone checks if the inspector container is removed +func (h *inspectorAutoCleanupTestHelper) verifyInspectorContainerGone() bool { + stdout, _ := e2e.NewTHVCommand(h.config, "list", "--all").ExpectSuccess() + return !strings.Contains(stdout, h.inspectorName) +} + +// verifyUIAccessible checks if the inspector UI is accessible +func (h *inspectorAutoCleanupTestHelper) verifyUIAccessible() bool { + if h.inspectorURL == "" { + return false + } + + resp, err := h.httpClient.Get(h.inspectorURL) + if err == nil { + _ = resp.Body.Close() + return resp.StatusCode == 200 + } + return false +} + +// verifyUIInaccessible checks if the inspector UI is not accessible +func (h *inspectorAutoCleanupTestHelper) verifyUIInaccessible() bool { + if h.inspectorURL == "" { + return true // No URL means it's inaccessible + } + + resp, err := h.httpClient.Get(h.inspectorURL) + if resp != nil { + _ = resp.Body.Close() + } + return err != nil +} + +// cleanup performs final cleanup of any remaining containers +func (h *inspectorAutoCleanupTestHelper) cleanup() { + // Clean up MCP server + err := e2e.StopAndRemoveMCPServer(h.config, h.mcpServerName) + if err != nil { + GinkgoWriter.Printf("Warning: Failed to cleanup MCP server: %v\n", err) + } + + // Clean up inspector container if it still exists + if h.verifyInspectorContainerExists() { + err = e2e.StopAndRemoveMCPServer(h.config, h.inspectorName) + if err != nil { + GinkgoWriter.Printf("Warning: Failed to cleanup inspector container: %v\n", err) + } + } +} + +var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cleanup"), func() { + var config *e2e.TestConfig + + BeforeEach(func() { + config = e2e.NewTestConfig() + + // Check if thv binary is available + err := e2e.CheckTHVBinaryAvailable(config) + Expect(err).ToNot(HaveOccurred(), "thv binary should be available") + }) + + Context("Normal operation scenarios", func() { + It("should auto-cleanup container when interrupted after ready", func() { + mcpServerName := fmt.Sprintf("mcp-autoclean-%d", GinkgoRandomSeed()) + helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) + + defer helper.cleanup() + + By("Starting an MCP server for inspector to connect to") + helper.setupMCPServer() + + By("Starting inspector command") + helper.startInspector() + + By("Waiting for inspector UI to be ready") + err := helper.waitForInspectorReady(45 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") + + By("Verifying inspector UI is accessible") + Expect(helper.verifyUIAccessible()).To(BeTrue(), "UI should be accessible") + + By("Verifying inspector container exists") + Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist") + + By("Waiting a moment before sending interrupt to ensure inspector is fully ready") + time.Sleep(5 * time.Second) + + By("Sending interrupt signal to inspector") + err = helper.interruptInspector() + Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(10 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") + + By("Verifying inspector container is cleaned up") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") + + By("Verifying UI is no longer accessible") + Expect(helper.verifyUIInaccessible()).To(BeTrue(), "UI should be inaccessible") + + By("Verifying no orphaned containers remain") + stdout, _ := e2e.NewTHVCommand(config, "list", "--all").ExpectSuccess() + Expect(stdout).ToNot(ContainSubstring("inspector"), "No inspector should remain") + }) + + It("should handle multiple start-stop cycles without conflicts", func() { + mcpServerName := fmt.Sprintf("mcp-multicycles-%d", GinkgoRandomSeed()) + helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) + + defer helper.cleanup() + + By("Starting an MCP server for inspector to connect to") + helper.setupMCPServer() + + // Test multiple cycles + for i := range 3 { + By(fmt.Sprintf("Starting inspector cycle %d", i+1)) + + By("Starting inspector command") + helper.startInspector() + + By("Waiting for inspector UI to be ready") + err := helper.waitForInspectorReady(30 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") + + By("Verifying inspector container exists") + Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist") + + By("Sending interrupt signal to inspector") + err = helper.interruptInspector() + Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(10 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") + + By("Verifying inspector container is cleaned up") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") + + By("Verifying UI is no longer accessible") + Expect(helper.verifyUIInaccessible()).To(BeTrue(), "UI should be inaccessible") + + // Small delay between cycles + time.Sleep(2 * time.Second) + } + + By("Verifying no orphaned containers remain after all cycles") + stdout, _ := e2e.NewTHVCommand(config, "list", "--all").ExpectSuccess() + Expect(stdout).ToNot(ContainSubstring("inspector"), "No inspector should remain") + }) + }) + + Context("Interruption scenarios", func() { + It("should auto-cleanup container when interrupted during startup", func() { + mcpServerName := fmt.Sprintf("mcp-earlyint-%d", GinkgoRandomSeed()) + helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) + + defer helper.cleanup() + + By("Starting an MCP server for inspector to connect to") + helper.setupMCPServer() + + By("Starting inspector command") + helper.startInspector() + + By("Immediately sending interrupt signal (before ready)") + // Give it a moment to start but interrupt before it's ready + time.Sleep(2 * time.Second) + err := helper.interruptInspector() + Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(15 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") + + By("Verifying inspector container is cleaned up") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") + + By("Verifying no orphaned containers remain") + stdout, _ := e2e.NewTHVCommand(config, "list", "--all").ExpectSuccess() + Expect(stdout).ToNot(ContainSubstring("inspector"), "No inspector should remain") + }) + + It("should auto-cleanup container when interrupted immediately after start", func() { + mcpServerName := fmt.Sprintf("mcp-immediateint-%d", GinkgoRandomSeed()) + helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) + + defer helper.cleanup() + + By("Starting an MCP server for inspector to connect to") + helper.setupMCPServer() + + By("Starting inspector command") + helper.startInspector() + + By("Immediately sending interrupt signal (minimal delay)") + // Interrupt almost immediately + time.Sleep(500 * time.Millisecond) + err := helper.interruptInspector() + Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(15 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") + + By("Verifying inspector container is cleaned up") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") + }) + }) + + Context("Container lifecycle verification", func() { + It("should properly transition container states during lifecycle", func() { + mcpServerName := fmt.Sprintf("mcp-lifecycle-%d", GinkgoRandomSeed()) + helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) + + defer helper.cleanup() + + By("Verifying no inspector container exists initially") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "No inspector container should exist initially") + + By("Starting an MCP server for inspector to connect to") + helper.setupMCPServer() + + By("Starting inspector command") + helper.startInspector() + + By("Waiting for inspector container to be created and running") + // Give container time to be created + time.Sleep(5 * time.Second) + Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist after start") + + By("Waiting for inspector UI to be ready") + err := helper.waitForInspectorReady(45 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") + + By("Verifying inspector UI is accessible") + Expect(helper.verifyUIAccessible()).To(BeTrue(), "UI should be accessible") + + By("Sending interrupt signal to inspector") + err = helper.interruptInspector() + Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(10 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") + + By("Verifying inspector container is completely removed (not just stopped)") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be completely removed") + + By("Verifying UI is no longer accessible") + Expect(helper.verifyUIInaccessible()).To(BeTrue(), "UI should be inaccessible") + }) + }) + + Context("Error handling and edge cases", func() { + It("should handle cleanup when container creation fails", func() { + // Test scenario where container creation might fail + // This is more complex to simulate, so we'll test the graceful handling + mcpServerName := fmt.Sprintf("mcp-errorcase-%d", GinkgoRandomSeed()) + helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) + + defer helper.cleanup() + + By("Starting an MCP server for inspector to connect to") + helper.setupMCPServer() + + By("Starting inspector command and immediately interrupting") + helper.startInspector() + + // Interrupt very quickly to potentially catch container during creation + time.Sleep(100 * time.Millisecond) + err := helper.interruptInspector() + Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(15 * time.Second) + // We expect this to succeed even if container creation was in progress + Expect(err).ToNot(HaveOccurred(), "Inspector should handle interrupt gracefully") + + By("Verifying no orphaned containers remain") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "No inspector containers should remain") + }) + }) +}) diff --git a/test/e2e/inspector_test.go b/test/e2e/inspector_test.go index c188537911..1b61ee50f5 100644 --- a/test/e2e/inspector_test.go +++ b/test/e2e/inspector_test.go @@ -5,7 +5,6 @@ package e2e_test import ( "fmt" - "net/http" "strings" "time" @@ -20,8 +19,6 @@ type inspectorTestHelper struct { config *e2e.TestConfig mcpServerName string inspectorName string - client *http.Client - inspectorURL string } var _ = Describe("Inspector", Label("mcp", "e2e"), func() { @@ -43,10 +40,8 @@ var _ = Describe("Inspector", Label("mcp", "e2e"), func() { AfterEach(func() { if config.CleanupAfter { - // Clean up both servers - err := e2e.StopAndRemoveMCPServer(config, inspectorName) - Expect(err).ToNot(HaveOccurred(), "Should be able to stop and remove inspector") - err = e2e.StopAndRemoveMCPServer(config, mcpServerName) + // Only clean up MCP server - inspector should auto-cleanup + err := e2e.StopAndRemoveMCPServer(config, mcpServerName) Expect(err).ToNot(HaveOccurred(), "Should be able to stop and remove MCP server") } }) @@ -106,11 +101,12 @@ var _ = Describe("Inspector", Label("mcp", "e2e"), func() { AfterEach(func() { if config.CleanupAfter { - // Clean up both servers - err := e2e.StopAndRemoveMCPServer(config, inspectorName) - Expect(err).ToNot(HaveOccurred(), "Should be able to stop and remove inspector") - err = e2e.StopAndRemoveMCPServer(config, mcpServerName) + // Clean up MCP server + err := e2e.StopAndRemoveMCPServer(config, mcpServerName) Expect(err).ToNot(HaveOccurred(), "Should be able to stop and remove MCP server") + + // Fallback cleanup for inspector in case auto-cleanup failed + helper.cleanupInspector() } }) @@ -173,34 +169,6 @@ var _ = Describe("Inspector", Label("mcp", "e2e"), func() { }) }) - Context("when testing inspector connectivity", func() { - It("should make inspector UI accessible when running", func() { - By("Starting inspector in background using goroutine") - done := helper.startInspectorInBackground(60 * time.Second) - - By("Waiting for inspector UI to be ready") - helper.waitForInspectorUI(45 * time.Second) - - By("Verifying inspector UI responds with valid content") - helper.verifyInspectorUIAccessible() - - By("Stopping inspector process and waiting for cleanup") - helper.waitForInspectorCompletion(done, 16*time.Second) - - By("Cleaning up any remaining inspector containers explicitly") - helper.cleanupInspector() - - By("Verifying inspector container is removed") - helper.waitForInspectorUIUnavailable(20 * time.Second) - - By("Verifying port bindings are cleaned up") - helper.verifyInspectorUIUnavailable() - - By("Verifying no orphaned inspector containers remain") - stdout, _ := e2e.NewTHVCommand(config, "list", "--all").ExpectSuccess() - Expect(stdout).ToNot(BeNil(), "Should get valid list output") - }) - }) }) }) @@ -210,80 +178,14 @@ func newInspectorTestHelper(config *e2e.TestConfig, mcpServerName, inspectorName config: config, mcpServerName: mcpServerName, inspectorName: inspectorName, - client: &http.Client{Timeout: 5 * time.Second}, - inspectorURL: "http://localhost:6274", - } -} - -// startInspectorInBackground starts the inspector in a goroutine with timeout -func (h *inspectorTestHelper) startInspectorInBackground(timeout time.Duration, args ...string) chan error { - done := make(chan error, 1) - go func() { - cmdArgs := append([]string{"inspector"}, args...) - cmdArgs = append(cmdArgs, h.mcpServerName) - _, _, err := e2e.NewTHVCommand(h.config, cmdArgs...).RunWithTimeout(timeout) - done <- err - }() - return done -} - -// waitForInspectorUI waits for the inspector UI to become accessible -func (h *inspectorTestHelper) waitForInspectorUI(timeout time.Duration) { - Eventually(func() bool { - response, err := h.client.Get(h.inspectorURL) - if response != nil { - response.Body.Close() - } - return err == nil && response != nil && response.StatusCode == 200 - }, timeout, 2*time.Second).Should(BeTrue(), "Inspector UI should become accessible") -} - -// waitForInspectorUIUnavailable waits for the inspector UI to become inaccessible -func (h *inspectorTestHelper) waitForInspectorUIUnavailable(timeout time.Duration) { - Eventually(func() bool { - response, err := h.client.Get(h.inspectorURL) - if response != nil { - response.Body.Close() - } - return err != nil - }, timeout, 2*time.Second).Should(BeTrue(), "Inspector UI should no longer be accessible after cleanup") -} - -// verifyInspectorUIAccessible verifies that the inspector UI is accessible -func (h *inspectorTestHelper) verifyInspectorUIAccessible() { - response, err := h.client.Get(h.inspectorURL) - Expect(err).ToNot(HaveOccurred(), "Inspector UI should be accessible") - Expect(response).ToNot(BeNil(), "Response should not be nil") - Expect(response.StatusCode).To(Equal(200), "Inspector UI should return 200 OK") - _ = response.Body.Close() -} - -// verifyInspectorUIUnavailable verifies that the inspector UI is not accessible -func (h *inspectorTestHelper) verifyInspectorUIUnavailable() { - response, err := h.client.Get(h.inspectorURL) - if response != nil { - response.Body.Close() - } - Expect(err).To(HaveOccurred(), "Port 6274 should no longer be accessible") -} - -// waitForInspectorCompletion waits for the inspector process to complete or timeout -func (*inspectorTestHelper) waitForInspectorCompletion(done chan error, timeout time.Duration) { - select { - case err := <-done: - if err != nil && !strings.Contains(err.Error(), "context deadline exceeded") { - GinkgoWriter.Printf("Inspector ended with error: %v\n", err) - } - case <-time.After(timeout): - GinkgoWriter.Println("Inspector may still be running after timeout") } } -// cleanupInspector performs cleanup of inspector containers +// cleanupInspector performs cleanup of inspector containers (fallback for test failures) func (h *inspectorTestHelper) cleanupInspector() { err := e2e.StopAndRemoveMCPServer(h.config, h.inspectorName) if err != nil { - GinkgoWriter.Printf("Note: Cleanup returned error (may be expected): %v\n", err) + GinkgoWriter.Printf("Note: Fallback cleanup returned error (may be expected): %v\n", err) } time.Sleep(3 * time.Second) // Give time for cleanup to complete } From a5232b7db8ad6a82f752dbb2be5cc83801a148eb Mon Sep 17 00:00:00 2001 From: carlos <21148423+carlos-gn@users.noreply.github.com> Date: Tue, 3 Feb 2026 16:55:22 +0100 Subject: [PATCH 2/5] Remove blocking exit, bad idea Signed-off-by: carlos <21148423+carlos-gn@users.noreply.github.com> --- cmd/thv/app/inspector.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/cmd/thv/app/inspector.go b/cmd/thv/app/inspector.go index 48d2fd573a..ec9f562b76 100644 --- a/cmd/thv/app/inspector.go +++ b/cmd/thv/app/inspector.go @@ -180,17 +180,7 @@ func inspectorCmdFunc(cmd *cobra.Command, args []string) error { "http://localhost:%d?transport=%s&serverUrl=http://host.docker.internal:%d/%s&MCP_PROXY_AUTH_TOKEN=%s", inspectorUIPort, transportTypeStr, serverPort, suffix, authToken) logger.Infof("Inspector UI is now available at %s", inspectorURL) - logger.Infof("Press Ctrl+C to stop the inspector container") - <-ctx.Done() - logger.Info("Interrupt received, stopping inspector container...") - - cleanupErr := cleanupInspectorContainer(context.Background(), "inspector") - if cleanupErr != nil { - logger.Warnf("Failed to cleanup inspector container: %v", cleanupErr) - } else { - logger.Info("Inspector container cleaned up successfully") - } return nil case <-ctx.Done(): logger.Info("Context cancelled during inspector startup, cleaning up...") From 917af40040628a3921f8e87b3ecb0d37197802ea Mon Sep 17 00:00:00 2001 From: carlos <21148423+carlos-gn@users.noreply.github.com> Date: Tue, 3 Feb 2026 19:51:03 +0100 Subject: [PATCH 3/5] Fix tests Signed-off-by: carlos <21148423+carlos-gn@users.noreply.github.com> --- test/e2e/inspector_autocleanup_test.go | 144 +++++-------------------- 1 file changed, 27 insertions(+), 117 deletions(-) diff --git a/test/e2e/inspector_autocleanup_test.go b/test/e2e/inspector_autocleanup_test.go index 4bce04c5a9..4fdfc03a84 100644 --- a/test/e2e/inspector_autocleanup_test.go +++ b/test/e2e/inspector_autocleanup_test.go @@ -35,6 +35,7 @@ func newInspectorAutoCleanupTestHelper(config *e2e.TestConfig, mcpServerName str mcpServerName: mcpServerName, inspectorName: "inspector", httpClient: &http.Client{Timeout: 5 * time.Second}, + inspectorURL: "http://localhost:6274", } } @@ -53,9 +54,6 @@ func (h *inspectorAutoCleanupTestHelper) startInspector() { cmd := e2e.StartLongRunningTHVCommand(h.config, args...) h.inspectorCmd = cmd - - // Build the expected inspector URL - h.inspectorURL = "http://localhost:6274" } // interruptInspector sends SIGINT to the inspector process @@ -133,10 +131,6 @@ func (h *inspectorAutoCleanupTestHelper) verifyInspectorContainerGone() bool { // verifyUIAccessible checks if the inspector UI is accessible func (h *inspectorAutoCleanupTestHelper) verifyUIAccessible() bool { - if h.inspectorURL == "" { - return false - } - resp, err := h.httpClient.Get(h.inspectorURL) if err == nil { _ = resp.Body.Close() @@ -145,19 +139,6 @@ func (h *inspectorAutoCleanupTestHelper) verifyUIAccessible() bool { return false } -// verifyUIInaccessible checks if the inspector UI is not accessible -func (h *inspectorAutoCleanupTestHelper) verifyUIInaccessible() bool { - if h.inspectorURL == "" { - return true // No URL means it's inaccessible - } - - resp, err := h.httpClient.Get(h.inspectorURL) - if resp != nil { - _ = resp.Body.Close() - } - return err != nil -} - // cleanup performs final cleanup of any remaining containers func (h *inspectorAutoCleanupTestHelper) cleanup() { // Clean up MCP server @@ -186,9 +167,9 @@ var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cle Expect(err).ToNot(HaveOccurred(), "thv binary should be available") }) - Context("Normal operation scenarios", func() { - It("should auto-cleanup container when interrupted after ready", func() { - mcpServerName := fmt.Sprintf("mcp-autoclean-%d", GinkgoRandomSeed()) + Context("Normal operation", func() { + It("should start inspector and make UI accessible", func() { + mcpServerName := fmt.Sprintf("mcp-happypath-%d", GinkgoRandomSeed()) helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) defer helper.cleanup() @@ -209,30 +190,16 @@ var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cle By("Verifying inspector container exists") Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist") - By("Waiting a moment before sending interrupt to ensure inspector is fully ready") - time.Sleep(5 * time.Second) - - By("Sending interrupt signal to inspector") - err = helper.interruptInspector() - Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") - - By("Waiting for inspector process to exit") + By("Waiting for inspector process to exit (returns immediately after ready)") err = helper.waitForInspectorExit(10 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") - - By("Verifying inspector container is cleaned up") - Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after becoming ready") - By("Verifying UI is no longer accessible") - Expect(helper.verifyUIInaccessible()).To(BeTrue(), "UI should be inaccessible") - - By("Verifying no orphaned containers remain") - stdout, _ := e2e.NewTHVCommand(config, "list", "--all").ExpectSuccess() - Expect(stdout).ToNot(ContainSubstring("inspector"), "No inspector should remain") + By("Verifying inspector container still exists after process exits") + Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should still exist") }) - It("should handle multiple start-stop cycles without conflicts", func() { - mcpServerName := fmt.Sprintf("mcp-multicycles-%d", GinkgoRandomSeed()) + It("should allow manual cleanup after inspector exits", func() { + mcpServerName := fmt.Sprintf("mcp-manualclean-%d", GinkgoRandomSeed()) helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) defer helper.cleanup() @@ -240,45 +207,32 @@ var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cle By("Starting an MCP server for inspector to connect to") helper.setupMCPServer() - // Test multiple cycles - for i := range 3 { - By(fmt.Sprintf("Starting inspector cycle %d", i+1)) - - By("Starting inspector command") - helper.startInspector() - - By("Waiting for inspector UI to be ready") - err := helper.waitForInspectorReady(30 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") - - By("Verifying inspector container exists") - Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist") + By("Starting inspector command") + helper.startInspector() - By("Sending interrupt signal to inspector") - err = helper.interruptInspector() - Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") + By("Waiting for inspector UI to be ready") + err := helper.waitForInspectorReady(45 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") - By("Waiting for inspector process to exit") - err = helper.waitForInspectorExit(10 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") + By("Waiting for inspector process to exit") + err = helper.waitForInspectorExit(10 * time.Second) + Expect(err).ToNot(HaveOccurred(), "Inspector should exit after becoming ready") - By("Verifying inspector container is cleaned up") - Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") + By("Verifying inspector container still exists") + Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should still exist") - By("Verifying UI is no longer accessible") - Expect(helper.verifyUIInaccessible()).To(BeTrue(), "UI should be inaccessible") + By("Manually stopping the inspector container") + e2e.NewTHVCommand(config, "stop", helper.inspectorName).ExpectSuccess() - // Small delay between cycles - time.Sleep(2 * time.Second) - } + By("Manually removing the inspector container") + e2e.NewTHVCommand(config, "rm", helper.inspectorName).ExpectSuccess() - By("Verifying no orphaned containers remain after all cycles") - stdout, _ := e2e.NewTHVCommand(config, "list", "--all").ExpectSuccess() - Expect(stdout).ToNot(ContainSubstring("inspector"), "No inspector should remain") + By("Verifying inspector container is gone after manual cleanup") + Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be gone after manual cleanup") }) }) - Context("Interruption scenarios", func() { + Context("Startup interruption scenarios", func() { It("should auto-cleanup container when interrupted during startup", func() { mcpServerName := fmt.Sprintf("mcp-earlyint-%d", GinkgoRandomSeed()) helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) @@ -336,50 +290,6 @@ var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cle }) }) - Context("Container lifecycle verification", func() { - It("should properly transition container states during lifecycle", func() { - mcpServerName := fmt.Sprintf("mcp-lifecycle-%d", GinkgoRandomSeed()) - helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) - - defer helper.cleanup() - - By("Verifying no inspector container exists initially") - Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "No inspector container should exist initially") - - By("Starting an MCP server for inspector to connect to") - helper.setupMCPServer() - - By("Starting inspector command") - helper.startInspector() - - By("Waiting for inspector container to be created and running") - // Give container time to be created - time.Sleep(5 * time.Second) - Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist after start") - - By("Waiting for inspector UI to be ready") - err := helper.waitForInspectorReady(45 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") - - By("Verifying inspector UI is accessible") - Expect(helper.verifyUIAccessible()).To(BeTrue(), "UI should be accessible") - - By("Sending interrupt signal to inspector") - err = helper.interruptInspector() - Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") - - By("Waiting for inspector process to exit") - err = helper.waitForInspectorExit(10 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should exit after interrupt") - - By("Verifying inspector container is completely removed (not just stopped)") - Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be completely removed") - - By("Verifying UI is no longer accessible") - Expect(helper.verifyUIInaccessible()).To(BeTrue(), "UI should be inaccessible") - }) - }) - Context("Error handling and edge cases", func() { It("should handle cleanup when container creation fails", func() { // Test scenario where container creation might fail From 43a744ae79b09646707ba1fed2352c179edb5656 Mon Sep 17 00:00:00 2001 From: carlos <21148423+carlos-gn@users.noreply.github.com> Date: Wed, 4 Feb 2026 16:04:58 +0100 Subject: [PATCH 4/5] Simplify tests Signed-off-by: carlos <21148423+carlos-gn@users.noreply.github.com> --- test/e2e/inspector_autocleanup_test.go | 140 ------------------------- 1 file changed, 140 deletions(-) diff --git a/test/e2e/inspector_autocleanup_test.go b/test/e2e/inspector_autocleanup_test.go index 4fdfc03a84..b005d2c06c 100644 --- a/test/e2e/inspector_autocleanup_test.go +++ b/test/e2e/inspector_autocleanup_test.go @@ -4,9 +4,7 @@ package e2e_test import ( - "context" "fmt" - "net/http" "os/exec" "strings" "syscall" @@ -24,8 +22,6 @@ type inspectorAutoCleanupTestHelper struct { mcpServerName string inspectorName string // Always "inspector" inspectorCmd *exec.Cmd - httpClient *http.Client - inspectorURL string } // newInspectorAutoCleanupTestHelper creates a new test helper for auto-cleanup testing @@ -34,8 +30,6 @@ func newInspectorAutoCleanupTestHelper(config *e2e.TestConfig, mcpServerName str config: config, mcpServerName: mcpServerName, inspectorName: "inspector", - httpClient: &http.Client{Timeout: 5 * time.Second}, - inspectorURL: "http://localhost:6274", } } @@ -66,35 +60,6 @@ func (h *inspectorAutoCleanupTestHelper) interruptInspector() error { return h.inspectorCmd.Process.Signal(syscall.SIGINT) } -// waitForInspectorReady waits for the inspector UI to become accessible -func (h *inspectorAutoCleanupTestHelper) waitForInspectorReady(timeout time.Duration) error { - GinkgoWriter.Printf("Waiting for inspector UI to be ready at %s\n", h.inspectorURL) - - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - ticker := time.NewTicker(2 * time.Second) - defer ticker.Stop() - - for { - select { - case <-ctx.Done(): - return fmt.Errorf("timeout waiting for inspector UI to be ready") - case <-ticker.C: - resp, err := h.httpClient.Get(h.inspectorURL) - if err == nil && resp.StatusCode == 200 { - _ = resp.Body.Close() - GinkgoWriter.Println("Inspector UI is ready") - return nil - } - if resp != nil { - _ = resp.Body.Close() - } - GinkgoWriter.Println("Inspector UI not ready yet, waiting...") - } - } -} - // waitForInspectorExit waits for the inspector process to exit func (h *inspectorAutoCleanupTestHelper) waitForInspectorExit(timeout time.Duration) error { if h.inspectorCmd == nil { @@ -129,16 +94,6 @@ func (h *inspectorAutoCleanupTestHelper) verifyInspectorContainerGone() bool { return !strings.Contains(stdout, h.inspectorName) } -// verifyUIAccessible checks if the inspector UI is accessible -func (h *inspectorAutoCleanupTestHelper) verifyUIAccessible() bool { - resp, err := h.httpClient.Get(h.inspectorURL) - if err == nil { - _ = resp.Body.Close() - return resp.StatusCode == 200 - } - return false -} - // cleanup performs final cleanup of any remaining containers func (h *inspectorAutoCleanupTestHelper) cleanup() { // Clean up MCP server @@ -167,71 +122,6 @@ var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cle Expect(err).ToNot(HaveOccurred(), "thv binary should be available") }) - Context("Normal operation", func() { - It("should start inspector and make UI accessible", func() { - mcpServerName := fmt.Sprintf("mcp-happypath-%d", GinkgoRandomSeed()) - helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) - - defer helper.cleanup() - - By("Starting an MCP server for inspector to connect to") - helper.setupMCPServer() - - By("Starting inspector command") - helper.startInspector() - - By("Waiting for inspector UI to be ready") - err := helper.waitForInspectorReady(45 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") - - By("Verifying inspector UI is accessible") - Expect(helper.verifyUIAccessible()).To(BeTrue(), "UI should be accessible") - - By("Verifying inspector container exists") - Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should exist") - - By("Waiting for inspector process to exit (returns immediately after ready)") - err = helper.waitForInspectorExit(10 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should exit after becoming ready") - - By("Verifying inspector container still exists after process exits") - Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should still exist") - }) - - It("should allow manual cleanup after inspector exits", func() { - mcpServerName := fmt.Sprintf("mcp-manualclean-%d", GinkgoRandomSeed()) - helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) - - defer helper.cleanup() - - By("Starting an MCP server for inspector to connect to") - helper.setupMCPServer() - - By("Starting inspector command") - helper.startInspector() - - By("Waiting for inspector UI to be ready") - err := helper.waitForInspectorReady(45 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should become ready") - - By("Waiting for inspector process to exit") - err = helper.waitForInspectorExit(10 * time.Second) - Expect(err).ToNot(HaveOccurred(), "Inspector should exit after becoming ready") - - By("Verifying inspector container still exists") - Expect(helper.verifyInspectorContainerExists()).To(BeTrue(), "Container should still exist") - - By("Manually stopping the inspector container") - e2e.NewTHVCommand(config, "stop", helper.inspectorName).ExpectSuccess() - - By("Manually removing the inspector container") - e2e.NewTHVCommand(config, "rm", helper.inspectorName).ExpectSuccess() - - By("Verifying inspector container is gone after manual cleanup") - Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be gone after manual cleanup") - }) - }) - Context("Startup interruption scenarios", func() { It("should auto-cleanup container when interrupted during startup", func() { mcpServerName := fmt.Sprintf("mcp-earlyint-%d", GinkgoRandomSeed()) @@ -289,34 +179,4 @@ var _ = Describe("Inspector Auto-Cleanup", Label("mcp", "e2e", "inspector", "cle Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "Container should be cleaned up") }) }) - - Context("Error handling and edge cases", func() { - It("should handle cleanup when container creation fails", func() { - // Test scenario where container creation might fail - // This is more complex to simulate, so we'll test the graceful handling - mcpServerName := fmt.Sprintf("mcp-errorcase-%d", GinkgoRandomSeed()) - helper := newInspectorAutoCleanupTestHelper(config, mcpServerName) - - defer helper.cleanup() - - By("Starting an MCP server for inspector to connect to") - helper.setupMCPServer() - - By("Starting inspector command and immediately interrupting") - helper.startInspector() - - // Interrupt very quickly to potentially catch container during creation - time.Sleep(100 * time.Millisecond) - err := helper.interruptInspector() - Expect(err).ToNot(HaveOccurred(), "Should be able to interrupt inspector") - - By("Waiting for inspector process to exit") - err = helper.waitForInspectorExit(15 * time.Second) - // We expect this to succeed even if container creation was in progress - Expect(err).ToNot(HaveOccurred(), "Inspector should handle interrupt gracefully") - - By("Verifying no orphaned containers remain") - Expect(helper.verifyInspectorContainerGone()).To(BeTrue(), "No inspector containers should remain") - }) - }) }) From 3431c56d56b329e2f46f84b50c6278587cae688d Mon Sep 17 00:00:00 2001 From: carlos <21148423+carlos-gn@users.noreply.github.com> Date: Wed, 4 Feb 2026 16:35:18 +0100 Subject: [PATCH 5/5] Fix flaky test Signed-off-by: carlos <21148423+carlos-gn@users.noreply.github.com> --- cmd/thv/app/inspector.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/thv/app/inspector.go b/cmd/thv/app/inspector.go index ec9f562b76..b248fd9769 100644 --- a/cmd/thv/app/inspector.go +++ b/cmd/thv/app/inspector.go @@ -155,6 +155,10 @@ func inspectorCmdFunc(cmd *cobra.Command, args []string) error { false, // Do not isolate network ) if err != nil { + // Clean up any partially created container if deployment was interrupted + if cleanupErr := cleanupInspectorContainer(context.Background(), "inspector"); cleanupErr != nil { + logger.Debugf("Failed to cleanup inspector container after deployment error: %v", cleanupErr) + } return fmt.Errorf("failed to create inspector workload: %w", err) }