fix(ci): eliminate chronic E2E timeout — Runtime.Close Docker-verify waste (#478)

* fix(ci): skip Docker-cleanup verification when isolation unused

Root cause of the chronic E2E red (failing on every main commit for
weeks; v0.31.0/.1 shipped through it): Runtime.Close() and
Server.Stop() always ran a Docker container-cleanup verification —
a docker ps probe + a 15x1s poll loop + 2s sleep (~17s) — even when
no Docker isolation was ever used. Every runtime-constructing unit
test paid ~17s in t.Cleanup; across ~dozens of tests under -race the
internal/runtime package hung past 12 min, blowing CI's
-race -timeout 2m step.

Fix: add Manager.UsesDockerIsolation() (same predicate as the Docker
recovery monitor) and gate both cleanup-verification blocks on it.
No isolation => no managed containers => nothing to verify. Production
behavior with Docker isolation is unchanged.

Also add testing.Short() guards to the heaviest property/timing tests
(TestRapidQuarantineStateMachine ~270s, TestRapidInvariant ~70s, and
five ~18s timing tests) so the fast unit step stays lean; they still
run in the main-only stress-tests job.

Result (local, exact CI condition -short -race -timeout 2m):
- internal/runtime: hang >12m  -> ok 20s
- internal/server:  slow       -> ok 18s
- internal/upstream/contracts: ok

Related #468

* ci(e2e): -short the fast unit step; heavy tests move to stress job

Pairs with the Runtime.Close Docker-verify fix. The fast unit step
now passes -short (activating the testing.Short guards) with a 4m
race timeout for headroom; the heavy property/timing tests run
unguarded in the main-only stress-tests job so coverage is preserved.

Related #468

Author: Dumbris

.github/workflows/e2e-tests.yml

@@ -91,7 +91,10 @@ jobs:
         run: go build -tags nogui -o mcpproxy ./cmd/mcpproxy
 
       - name: Run unit tests
-        run: go test -v -race -timeout 2m -skip "Binary|MCP|E2E|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/...
+        # -short skips the heavy property/timing tests (TestRapidQuarantine* etc.);
+        # they run unguarded in the main-only stress-tests job below so coverage
+        # is preserved. Bumped to 4m for headroom under -race on slow runners.
+        run: go test -v -short -race -timeout 4m -skip "Binary|MCP|E2E|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/...
 
       # Binary tests are too flaky in CI due to server startup timing issues
       # All Binary tests consistently timeout waiting for server to be ready
@@ -325,21 +328,21 @@ jobs:
           if [ -d "${{ matrix.log_path_check }}" ]; then
             echo "✓ Log directory created successfully"
             ls -la "${{ matrix.log_path_check }}"
-            
+
             # Check if log file exists and has content
             if [ -f "${{ matrix.log_path_check }}/main.log" ]; then
               echo "✓ Log file created successfully"
               echo "Log file size: $(wc -c < "${{ matrix.log_path_check }}/main.log") bytes"
               echo "First few lines:"
               head -3 "${{ matrix.log_path_check }}/main.log"
-              
+
               # Verify log contains expected content
               if grep -q "Log directory configured" "${{ matrix.log_path_check }}/main.log"; then
                 echo "✓ Log contains expected startup messages"
               else
                 echo "⚠ Log missing expected startup messages"
               fi
-              
+
               if grep -q "${{ matrix.log_standard }}" "${{ matrix.log_path_check }}/main.log"; then
                 echo "✓ Log contains OS standard compliance information"
               else
@@ -476,6 +479,13 @@ jobs:
           mkdir -p web/frontend
           cp -r frontend/dist web/frontend/
 
+      - name: Run heavy runtime property/timing tests
+        run: |
+          # These are -short-skipped in the fast unit step; run them here
+          # (unguarded, generous timeout) so invariant coverage is preserved.
+          go test -v -race -timeout 20m ./internal/runtime \
+            -run 'TestRapidQuarantineStateMachine|TestRapidInvariant_ChangedNeverAutoApproved|TestApplyConfig_ListenAddressChange|TestIsDockerAvailable_NegativeTTLShorter|TestIsDockerAvailable_PositiveTTLHonored|TestToolCacheInvalidation_DisableServerRemovesTools|TestRuntimeStatusSnapshotReflectsRunningAndListen'
+
       - name: Run concurrent stress tests
         run: |
           # Run the concurrent test multiple times to catch race conditions

internal/runtime/apply_config_restart_test.go

@@ -17,6 +17,9 @@ import (
 // TestApplyConfig_ListenAddressChange tests that listen address changes are saved to disk
 // even though they require a restart
 func TestApplyConfig_ListenAddressChange(t *testing.T) {
+	if testing.Short() {
+		t.Skip("server restart timing test (~18s under -race); runs in the stress-tests CI job")
+	}
 	// Create temp directory and config file
 	tmpDir := t.TempDir()
 	cfgPath := filepath.Join(tmpDir, "config.json")

internal/runtime/docker_probe_test.go

@@ -39,6 +39,9 @@ func TestIsDockerAvailable_CachesResult(t *testing.T) {
 // result expires after 5m (negative TTL) — so users who launch Docker
 // after mcpproxy starts see the flip within minutes, not only after restart.
 func TestIsDockerAvailable_NegativeTTLShorter(t *testing.T) {
+	if testing.Short() {
+		t.Skip("real-time TTL test (~18s under -race); runs in the stress-tests CI job")
+	}
 	rt := newTestRuntime(t)
 
 	// Prime with a negative result 6 minutes old — past negative TTL (5m)
@@ -65,6 +68,9 @@ func TestIsDockerAvailable_NegativeTTLShorter(t *testing.T) {
 // result within 15m is reused (no re-probe) to avoid spending 2s on
 // `docker info` on every telemetry heartbeat.
 func TestIsDockerAvailable_PositiveTTLHonored(t *testing.T) {
+	if testing.Short() {
+		t.Skip("real-time TTL test (~18s under -race); runs in the stress-tests CI job")
+	}
 	rt := newTestRuntime(t)
 
 	// 10 minutes old — well within positive TTL (15m).

internal/runtime/runtime.go

@@ -580,8 +580,12 @@ func (r *Runtime) Close() error {
 			}
 		}
 
-		// Verify all containers stopped with retry loop (15 attempts = 15 seconds)
-		if r.upstreamManager.HasDockerContainers() {
+		// Verify all containers stopped with retry loop (15 attempts = 15 seconds).
+		// Only when Docker isolation could have launched containers — otherwise
+		// the `docker ps` probe + this loop are pure waste (and add ~17s per
+		// Close in test processes, which made internal/runtime exceed CI's
+		// -race timeout). No isolation ⇒ no managed containers ⇒ nothing to verify.
+		if r.upstreamManager.UsesDockerIsolation() && r.upstreamManager.HasDockerContainers() {
 			if r.logger != nil {
 				r.logger.Warn("Docker containers still running after shutdown, verifying cleanup...")
 			}

internal/runtime/runtime_test.go

@@ -79,6 +79,9 @@ func TestRuntimeUpdateStatusBroadcasts(t *testing.T) {
 }
 
 func TestRuntimeStatusSnapshotReflectsRunningAndListen(t *testing.T) {
+	if testing.Short() {
+		t.Skip("server startup/listen timing test (~18s under -race); runs in the stress-tests CI job")
+	}
 	rt := newTestRuntime(t)
 
 	rt.SetRunning(true)

internal/runtime/tool_invalidation_test.go

@@ -398,6 +398,9 @@ func TestToolCacheInvalidation_MultipleServers(t *testing.T) {
 // removes its tools from the search index (Issue #285 fix).
 // This ensures disabled servers' tools don't appear in search results.
 func TestToolCacheInvalidation_DisableServerRemovesTools(t *testing.T) {
+	if testing.Short() {
+		t.Skip("heavy server-lifecycle test (~18s under -race); runs in the stress-tests CI job")
+	}
 	tempDir := t.TempDir()
 	cfg := &config.Config{
 		DataDir:           tempDir,

internal/runtime/tool_quarantine_invariant_test.go

@@ -353,6 +353,9 @@ func (m *quarantineModel) checkInvariant(t *rapid.T) {
 // TestRapidQuarantineStateMachine runs a property-based state machine test
 // using rapid to verify quarantine invariants hold across random action sequences.
 func TestRapidQuarantineStateMachine(t *testing.T) {
+	if testing.Short() {
+		t.Skip("heavy property test (~270s under -race); runs in the stress-tests CI job")
+	}
 	rapid.Check(t, func(t *rapid.T) {
 		// Set up runtime with quarantine enabled
 		tempDir, err := os.MkdirTemp("", "quarantine-rapid-*")
@@ -498,6 +501,9 @@ func TestRapidQuarantineStateMachine(t *testing.T) {
 // TestRapidInvariant_ChangedNeverAutoApproved is a focused property test that
 // specifically targets the "changed→approved without user action" invariant.
 func TestRapidInvariant_ChangedNeverAutoApproved(t *testing.T) {
+	if testing.Short() {
+		t.Skip("heavy property test (~70s under -race); runs in the stress-tests CI job")
+	}
 	rapid.Check(t, func(t *rapid.T) {
 		tempDir, err := os.MkdirTemp("", "quarantine-changed-*")
 		if err != nil {

internal/server/server.go

@@ -1447,8 +1447,11 @@ func (s *Server) StopServer() error {
 		_ = s.logger.Sync()
 	}
 
-	// NEW: Verify all containers stopped with retry loop (instead of arbitrary 3s sleep)
-	if s.runtime.UpstreamManager().HasDockerContainers() {
+	// NEW: Verify all containers stopped with retry loop (instead of arbitrary 3s sleep).
+	// Gated on Docker isolation actually being in use — otherwise the `docker ps`
+	// probe + verifyContainersCleanedUp loop are pure waste (and add ~17s per
+	// Stop in test processes). No isolation ⇒ no managed containers ⇒ nothing to verify.
+	if s.runtime.UpstreamManager().UsesDockerIsolation() && s.runtime.UpstreamManager().HasDockerContainers() {
 		s.logger.Warn("STOPSERVER - Docker containers still running, verifying cleanup...")
 		_ = s.logger.Sync()
 		s.verifyContainersCleanedUp(cleanupCtx)

internal/upstream/manager.go

@@ -234,6 +234,16 @@ func (m *Manager) shouldEnableDockerRecovery() bool {
 	return false
 }
 
+// UsesDockerIsolation reports whether this manager could have launched
+// Docker-isolated containers (global isolation on, a per-server isolation, or
+// a docker command). When false, no container cleanup verification is needed
+// on shutdown — shelling out to `docker ps` would be pure waste (and, in test
+// processes, adds ~17s/Close via the verification loop). Same predicate the
+// Docker recovery monitor uses, so behavior stays consistent.
+func (m *Manager) UsesDockerIsolation() bool {
+	return m.shouldEnableDockerRecovery()
+}
+
 // SetLogConfig sets the logging configuration for upstream server loggers
 func (m *Manager) SetLogConfig(logConfig *config.LogConfig) {
 	m.mu.Lock()