Skip to content

fix(ssh): prevent duplicate connections and race conditions#856

Merged
arnestrickmann merged 1 commit intomainfrom
emdash/ssh-bug-fix-6uw
Feb 14, 2026
Merged

fix(ssh): prevent duplicate connections and race conditions#856
arnestrickmann merged 1 commit intomainfrom
emdash/ssh-bug-fix-6uw

Conversation

@arnestrickmann
Copy link
Contributor

Summary

  • Add connection deduplication in SshService.connect() — coalesces in-flight connection attempts for the same ID and reuses existing connections instead of opening duplicate TCP sockets
  • Enforce a connection pool limit (max 10) with a warning threshold at 80% to prevent resource exhaustion
  • Fix stale close event handler that could remove a newer connection from the pool when an old client disconnects
  • Clean up stale connections in the reconnect handler before opening new ones
  • Add renderer-side deduplication via connectingIds set to prevent multiple React re-renders from firing parallel connects
  • Use reconnectRef to stabilize the health-check interval and avoid unnecessary teardown/recreation loops

Changes

  • src/main/services/ssh/SshService.ts — Connection deduplication, pool limits, guarded close handler
  • src/main/ipc/sshIpc.ts — Disconnect stale connection before reconnect attempt
  • src/renderer/hooks/useRemoteProject.ts — Client-side connect deduplication, stable reconnect ref, guard against reconnecting while already connecting

@vercel
Copy link

vercel bot commented Feb 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Feb 14, 2026 3:21am

Request Review

@greptile-apps
Copy link

greptile-apps bot commented Feb 14, 2026

Greptile Overview

Greptile Summary

This PR adds comprehensive connection deduplication and resource management to prevent duplicate SSH connections and race conditions across the application stack.

Key improvements:

  • Service-level deduplication in SshService.connect() coalesces parallel connection attempts for the same ID, preventing multiple TCP sockets
  • Connection pool enforcement (max 10 connections) with warning threshold at 80% prevents resource exhaustion
  • Stale close handler fix uses reference equality check to ensure only the current client's close event removes the connection from the pool
  • Reconnect safety disconnects stale connections before opening new ones in the monitor's reconnect handler
  • Client-side deduplication via module-level connectingIds set prevents React re-renders from firing parallel connections
  • Stable reconnect reference eliminates health-check interval teardown/recreation loops

The changes are defensive and well-structured, with clear guard clauses at each layer of the stack (renderer → IPC → service).

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk
  • The changes are well-designed defensive programming that adds multiple layers of deduplication without modifying core connection logic. The stale close handler fix addresses a real race condition. One minor concern is the error handler in createConnection() doesn't clean up on post-connection errors, but the close handler will handle cleanup, so this isn't critical
  • No files require special attention

Important Files Changed

Filename Overview
src/main/services/ssh/SshService.ts Added connection deduplication, pool limits, and fixed stale close handler race condition
src/main/ipc/sshIpc.ts Added cleanup of stale connections before reconnect attempts
src/renderer/hooks/useRemoteProject.ts Added client-side connect deduplication and stabilized reconnect reference to prevent interval loops

Flowchart

flowchart TD
    A[Renderer: useRemoteProject.connect] --> B{connectingIds has ID?}
    B -->|Yes| C[Return early - deduplicate]
    B -->|No| D[Add to connectingIds set]
    D --> E[IPC: sshConnect]
    E --> F[IPC Handler: sshIpc]
    F --> G[SshService.connect]
    G --> H{Connection exists?}
    H -->|Yes| I[Return existing ID]
    H -->|No| J{Pending connection?}
    J -->|Yes| K[Coalesce onto promise]
    J -->|No| L{Pool at limit?}
    L -->|Yes| M[Throw error]
    L -->|No| N[createConnection]
    N --> O[Create SSH2 Client]
    O --> P[Set up event handlers]
    P --> Q[Connect]
    Q --> R{Success?}
    R -->|Ready| S[Add to pool]
    R -->|Error| T[Reject promise]
    S --> U[Delete from pending]
    T --> V[Delete from pending]
    U --> W[Return to renderer]
    V --> X[Error to renderer]
    W --> Y[Remove from connectingIds]
    X --> Y
    
    Z[SSH2 Client: close event] --> AA{Is this client in pool?}
    AA -->|Yes| AB[Remove from pool]
    AA -->|No| AC[Ignore stale event]
    
    AD[Monitor: reconnect event] --> AE{isConnected?}
    AE -->|Yes| AF[Disconnect stale]
    AE -->|No| AG[Skip disconnect]
    AF --> AH[Call connect]
    AG --> AH
Loading

Last reviewed commit: d1bfadf

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link

greptile-apps bot commented Feb 14, 2026

Additional Comments (1)

src/main/services/ssh/SshService.ts
The error handler rejects immediately when client.on('error') fires, but doesn't clean up the client or emit the 'disconnected' event. If the error occurs after resolve() was called (e.g., during an active connection), the client may remain in the pool but be in a broken state.

Consider wrapping the error handler to clean up properly:

      client.on('error', (err: Error) => {
        if (this.connections[connectionId]?.client === client) {
          delete this.connections[connectionId];
          this.emit('disconnected', connectionId);
        }
        reject(err);
      });

@arnestrickmann arnestrickmann merged commit 2023b18 into main Feb 14, 2026
4 checks passed
@arnestrickmann arnestrickmann deleted the emdash/ssh-bug-fix-6uw branch February 15, 2026 01:35
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.

1 participant