Part of duplicate code analysis: #1810

Summary

GetOrLaunch and GetOrLaunchForSession in internal/launcher/launcher.go each implement the same double-check locking idiom: read-lock → check cache → unlock → write-lock → double-check cache → create. The two implementations are structurally identical, differing only in the cache they target (l.connections vs l.sessionPool).

Duplication Details

Pattern: Double-check locking for cached connections

• Severity: Medium

• Occurrences: 2 (GetOrLaunch lines 64–132, GetOrLaunchForSession lines 134–188)

• Locations:

  • internal/launcher/launcher.go lines 70–90 (read-lock check + write-lock double-check, l.connections)
  • internal/launcher/launcher.go lines 141–176 (same structure but via l.sessionPool.Get())

• Code Sample:

// GetOrLaunch (lines 70–90)
l.mu.RLock()
if conn, ok := l.connections[serverID]; ok {
    l.mu.RUnlock()
    logger.LogDebugWithServer(serverID, "backend", "Reusing existing backend connection: %s", serverID)
    logLauncher.Printf("Reusing existing connection: serverID=%s", serverID)
    return conn, nil
}
l.mu.RUnlock()
// ...
l.mu.Lock()
defer l.mu.Unlock()
if conn, ok := l.connections[serverID]; ok {
    logger.LogDebugWithServer(serverID, "backend", "Backend connection created by another goroutine: %s", serverID)
    logLauncher.Printf("Connection created by another goroutine: serverID=%s", serverID)
    return conn, nil
}

// GetOrLaunchForSession (lines 156–176) — same structure, different cache
if conn, exists := l.sessionPool.Get(serverID, sessionID); exists {
    logger.LogDebugWithServer(serverID, "backend", "Reusing session connection: server=%s, session=%s", serverID, sessionID)
    logLauncher.Printf("Reusing session connection: serverID=%s, sessionID=%s", serverID, sessionID)
    return conn, nil
}
l.mu.Lock()
defer l.mu.Unlock()
if conn, exists := l.sessionPool.Get(serverID, sessionID); exists {
    logger.LogDebugWithServer(serverID, "backend", "Session connection created by another goroutine: ...")
    logLauncher.Printf("Session connection created by another goroutine: serverID=%s, sessionID=%s", serverID, sessionID)
    return conn, nil
}

Impact Analysis

• Maintainability: Any changes to the locking strategy (e.g., adding timeouts, switching to sync.Map, adding metrics) must be applied to both functions independently.
• Bug Risk: Medium — a subtle bug introduced in one (e.g., forgetting the double-check) would be easy to miss because the two functions look similar but are read separately.
• Code Bloat: ~25 lines of structurally identical locking ceremony duplicated.

Refactoring Recommendations

1. Extract a getOrCreate generic helper that encapsulates the read→write→double-check cycle:

   // getOrCreate attempts to get a cached item with double-check locking.
   // getter is called under RLock; creator is called under write-lock after a second getter check.
   func getOrCreate[V any](
       mu *sync.RWMutex,
       getter func() (V, bool),
       creator func() (V, error),
   ) (V, error) {
       mu.RLock()
       if v, ok := getter(); ok {
           mu.RUnlock()
           return v, nil
       }
       mu.RUnlock()
       mu.Lock()
       defer mu.Unlock()
       if v, ok := getter(); ok {
           return v, nil
       }
       return creator()
   }

   Then GetOrLaunch and GetOrLaunchForSession each become a single getOrCreate call.

2. Alternatively, delegate to SessionConnectionPool — since SessionConnectionPool.Get/Set already provides a cleaner abstraction, consider whether l.connections could be replaced with a single-session pool variant, unifying the two paths.

Implementation Checklist

• Evaluate generic helper vs SessionConnectionPool consolidation approach
• Implement chosen approach in internal/launcher/launcher.go
• Run make test-unit and make test-integration to confirm concurrent correctness
• Verify race detector (go test -race) passes on launcher tests

Parent Issue

See parent analysis report: #1810
Related to #1810

Generated by Duplicate Code Detector · ◷

• expires on Mar 19, 2026, 3:00 AM UTC