fix(proxy): harden MITM proxy reliability and scale for bulk installs (#314)

* fix(proxy): harden MITM proxy reliability and scale for bulk installs

Deep-dive analysis of dropped connections during large installs (5000+
packages with concurrent downloads) surfaced three issues, each verified
with a reproduction test before fixing.

1. Transient upstream errors tore down whole keep-alive tunnels.
   goproxy returns false (closing the entire MITM client tunnel) when a
   single upstream round-trip errors. Under load, CDN-fronted registries
   (e.g. Cloudflare for registry.npmjs.org) intermittently reset
   connections, so one transient reset dropped a pooled keep-alive socket
   and surfaced to the package manager as ECONNRESET / "socket hang up".
   Fix: route upstream round-trips through a resilient round tripper that
   retries idempotent, body-less requests with bounded linear backoff,
   absorbing transient resets and keeping the tunnel alive. A reproduction
   test shows the tunnel count drop from 3 to 1 across a transient failure.

2. Head-of-line amplification against the external analysis service.
   Concurrent requests for the same package version each issued their own
   gRPC call. Fix: de-duplicate in-flight analyses with singleflight so a
   burst of identical requests collapses into one upstream call.

3. Per-request goproxy verbose logging on the hot path.
   proxy.Verbose was always on, formatting several log lines per request
   even when discarded below debug level. Fix: enable goproxy verbose
   logging only when PMG runs at debug level.

Note: the hypothesis that the http.Server Read/WriteTimeout leaks onto
hijacked CONNECT tunnels was investigated and disproven (Go clears the
deadlines on Hijack); the behavioral guard tests for long-lived
connections and slow transfers are retained.

* fix: Type assertion error handling

---------

Co-authored-by: Claude <noreply@anthropic.com>

proxy/interceptors/base_registry.go

@@ -13,6 +13,7 @@ import (
 	"github.com/safedep/pmg/internal/audit"
 	"github.com/safedep/pmg/proxy"
 	gobreaker "github.com/sony/gobreaker/v2"
+	"golang.org/x/sync/singleflight"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -26,6 +27,13 @@ type baseRegistryInterceptor struct {
 	confirmationChan chan *ConfirmationRequest
 	circuitBreaker   *gobreaker.CircuitBreaker[*analyzer.PackageVersionAnalysisResult]
 	execContext      InterceptorContext
+
+	// inflight collapses concurrent analyses of the same package version into a
+	// single upstream call. During a large install the same transitive
+	// dependency is frequently requested across several connections at once.
+	// Without de-duplication each would issue its own gRPC call to the external
+	// analysis service, amplifying load and latency (head-of-line blocking).
+	inflight singleflight.Group
 }
 
 func newAnalyzerCircuitBreaker(name string) *gobreaker.CircuitBreaker[*analyzer.PackageVersionAnalysisResult] {
@@ -110,31 +118,51 @@ func (b *baseRegistryInterceptor) analyzePackage(
 
 	log.Debugf("[%s] Analyzing package %s@%s", ctx.RequestID, packageName, packageVersion)
 
-	result, err := b.circuitBreaker.Execute(func() (*analyzer.PackageVersionAnalysisResult, error) {
-		analysisCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
-		defer cancel()
-
-		res, err := b.analyzer.Analyze(analysisCtx, pkgVersion)
-		if err != nil {
-			// NotFound means the package is not in the analysis DB — this is expected
-			// and should not count as a circuit breaker failure.
-			// Since gRPC v1.75.0, status.FromError unwraps error chains via errors.As.
-			if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
-				log.Debugf("[%s] Package %s@%s not found in analysis DB, allowing", ctx.RequestID, packageName, packageVersion)
-				return &analyzer.PackageVersionAnalysisResult{
-					PackageVersion: pkgVersion,
-					Action:         analyzer.ActionAllow,
-				}, nil
-			}
+	key := ecosystem.String() + ":" + packageName + ":" + packageVersion
+	resultAny, err, _ := b.inflight.Do(key, func() (interface{}, error) {
+		// Re-check the cache: a previous in-flight analysis for this key may
+		// have populated it after our own cache miss above.
+		if cached, ok := b.cache.Get(ecosystem.String(), packageName, packageVersion); ok {
+			return cached, nil
 		}
 
-		return res, err
+		result, err := b.circuitBreaker.Execute(func() (*analyzer.PackageVersionAnalysisResult, error) {
+			analysisCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+			defer cancel()
+
+			res, err := b.analyzer.Analyze(analysisCtx, pkgVersion)
+			if err != nil {
+				// NotFound means the package is not in the analysis DB — this is expected
+				// and should not count as a circuit breaker failure.
+				// Since gRPC v1.75.0, status.FromError unwraps error chains via errors.As.
+				if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
+					log.Debugf("[%s] Package %s@%s not found in analysis DB, allowing", ctx.RequestID, packageName, packageVersion)
+					return &analyzer.PackageVersionAnalysisResult{
+						PackageVersion: pkgVersion,
+						Action:         analyzer.ActionAllow,
+					}, nil
+				}
+			}
+
+			return res, err
+		})
+		if err != nil {
+			return nil, err
+		}
+
+		b.cache.Set(ecosystem.String(), packageName, packageVersion, result)
+		return result, nil
 	})
 	if err != nil {
 		return nil, fmt.Errorf("analyzer failed: %w", err)
 	}
 
-	b.cache.Set(ecosystem.String(), packageName, packageVersion, result)
+	// Fail loudly rather than panic in the proxy hot path if a future change to
+	// the singleflight closure ever returns a different type or a nil result.
+	result, ok := resultAny.(*analyzer.PackageVersionAnalysisResult)
+	if !ok || result == nil {
+		return nil, fmt.Errorf("analyzer returned unexpected result type %T for %s@%s", resultAny, packageName, packageVersion)
+	}
 
 	log.Debugf("[%s] Analysis complete for %s@%s: action=%d", ctx.RequestID, packageName, packageVersion, result.Action)
 

proxy/interceptors/inflight_test.go

@@ -0,0 +1,67 @@
+package interceptors
+
+import (
+	"context"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	packagev1 "buf.build/gen/go/safedep/api/protocolbuffers/go/safedep/messages/package/v1"
+	"github.com/safedep/pmg/analyzer"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// slowAtomicAnalyzer counts calls atomically and blocks briefly so concurrent
+// callers overlap inside analyzePackage.
+type slowAtomicAnalyzer struct {
+	calls atomic.Int64
+	delay time.Duration
+}
+
+func (s *slowAtomicAnalyzer) Name() string { return "slow-atomic" }
+
+func (s *slowAtomicAnalyzer) Analyze(_ context.Context, pv *packagev1.PackageVersion) (*analyzer.PackageVersionAnalysisResult, error) {
+	s.calls.Add(1)
+	time.Sleep(s.delay)
+	return &analyzer.PackageVersionAnalysisResult{PackageVersion: pv, Action: analyzer.ActionAllow}, nil
+}
+
+// TestAnalyzePackageDeduplicatesConcurrentCalls verifies that a burst of
+// concurrent analyses for the same package version collapses into a single
+// upstream analyzer call (head-of-line mitigation), while distinct packages
+// are analyzed independently.
+func TestAnalyzePackageDeduplicatesConcurrentCalls(t *testing.T) {
+	mock := &slowAtomicAnalyzer{delay: 50 * time.Millisecond}
+	base := newTestBaseInterceptor(mock)
+	ctx := newTestRequestContext()
+
+	const concurrency = 50
+
+	var wg sync.WaitGroup
+	for i := 0; i < concurrency; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			result, err := base.analyzePackage(ctx, packagev1.Ecosystem_ECOSYSTEM_NPM, "same-pkg", "1.0.0")
+			assert.NoError(t, err)
+			require.NotNil(t, result)
+			assert.Equal(t, analyzer.ActionAllow, result.Action)
+		}()
+	}
+	wg.Wait()
+
+	assert.EqualValues(t, 1, mock.calls.Load(),
+		"concurrent analyses of the same package version should collapse into one upstream call")
+
+	// A subsequent call for the same package is served from cache (no new call).
+	_, err := base.analyzePackage(ctx, packagev1.Ecosystem_ECOSYSTEM_NPM, "same-pkg", "1.0.0")
+	require.NoError(t, err)
+	assert.EqualValues(t, 1, mock.calls.Load(), "result should be cached after the first analysis")
+
+	// A different package version triggers its own analysis.
+	_, err = base.analyzePackage(ctx, packagev1.Ecosystem_ECOSYSTEM_NPM, "other-pkg", "2.0.0")
+	require.NoError(t, err)
+	assert.EqualValues(t, 2, mock.calls.Load(), "distinct package versions are analyzed independently")
+}