Skip to content

Increase ssh detection timeout#512

Merged
heisbrot merged 1 commit intomainfrom
fix/ssh-timeout
Nov 21, 2025
Merged

Increase ssh detection timeout#512
heisbrot merged 1 commit intomainfrom
fix/ssh-timeout

Conversation

@heisbrot
Copy link
Copy Markdown
Contributor

@heisbrot heisbrot commented Nov 20, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved SSH connection error handling with automatic timeout and fallback authentication method.
    • Enhanced SSH authentication to properly validate and pass JWT tokens when required.
    • Clear error state upon initiating new connection attempts for more reliable reconnection.
  • Chores

    • Updated WASM client version to v0.60.2.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 20, 2025

Walkthrough

Enhanced SSH connection handling with timeout-based server type detection, JWT token support for authentication, and improved error state management. Updated default WASM client version and refined error logging messages across the remote access module.

Changes

Cohort / File(s) Summary
SSH Detection & Token Handling
src/modules/remote-access/ssh/useSSH.ts, src/modules/remote-access/useNetBirdClient.ts
Added 20-second timeout for SSH server type detection with try/catch fallback to pubkey method. Implemented JWT token validation—if detection requires JWT but no token is present, error state is set and connection reset. Token is now conditionally passed to SSH connection based on JWT requirement. Error state is cleared on new connection attempts.
Error Logging
src/app/(remote-access)/peer/ssh/page.tsx
Updated SSH connection error log message from "Connection failed:" to "Connection error:".
Configuration
src/utils/config.ts
Bumped default WASM client version from v0.60.0 to v0.60.2 in loadConfig fallback.

Sequence Diagram

sequenceDiagram
    participant User
    participant SSH Handler
    participant Detection Service
    participant SSH Connection
    
    User->>SSH Handler: Initiate SSH connection
    activate SSH Handler
    
    SSH Handler->>Detection Service: detectSSHServerType(timeout: 20s)
    activate Detection Service
    
    alt Detection succeeds
        Detection Service-->>SSH Handler: Server type + JWT requirement
    else Detection fails/times out
        Detection Service-->>SSH Handler: Error
        SSH Handler->>SSH Handler: Fall back to pubkey
    end
    deactivate Detection Service
    
    alt JWT required
        SSH Handler->>SSH Handler: Check access token
        alt Token available
            SSH Handler->>SSH Connection: createSSHConnection(token)
        else Token missing
            SSH Handler->>SSH Handler: Log error, set error state
            SSH Handler->>SSH Handler: Disconnect & reset
            Note over SSH Handler: Operation aborted
        end
    else No JWT required
        SSH Handler->>SSH Connection: createSSHConnection(no token)
    end
    
    SSH Connection-->>SSH Handler: Connection result
    deactivate SSH Handler
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • src/modules/remote-access/ssh/useSSH.ts — Contains multiple interrelated logic changes: timeout handling, try/catch flow, token validation with conditional logic, error state management, and fallback mechanisms. Review should verify detection timeout is appropriate, token validation logic is sound, and error handling doesn't obscure underlying failures.
  • src/modules/remote-access/useNetBirdClient.ts — Signature changes to public API methods; verify that threading of new parameters (timeoutMs, jwtToken) is consistent with underlying netBirdClient implementation and that optional parameter handling is correct.

Possibly related PRs

Suggested reviewers

  • lixmal

Poem

🐰 A timeout so swift, twenty seconds to spare,
With tokens in hand and detection with care,
Server types flow with a try-catch embrace,
While WASM upgrades keep pace! 🚀✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title focuses on increasing SSH detection timeout, but the changeset includes additional modifications: error logging message changes, JWT token handling, error state management, and a wasm path version bump unrelated to the timeout feature. Consider whether the title should be more comprehensive (e.g., 'SSH timeout and connection improvements') or scope the PR to only timeout-related changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/ssh-timeout

Comment @coderabbitai help to get the list of available commands and usage tips.

@heisbrot heisbrot requested a review from lixmal November 20, 2025 17:44
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/app/(remote-access)/peer/ssh/page.tsx (1)

124-126: Align connection log wording for easier debugging

This path now logs "Connection error:" while other SSH paths use "Reconnection failed:" (here) and "Connection failed:" (in useSSH). Not a blocker, but consider standardizing the prefix to make log filtering/alerting simpler.

src/modules/remote-access/ssh/useSSH.ts (1)

27-27: SSH detection timeout and JWT guard look solid; consider minor polish

The new flow looks correct:

  • 20s SSH_DETECTION_TIMEOUT_MS is consistently used when detecting the SSH server type.
  • Detection failures fall back to pubkey without aborting the attempt.
  • When JWT is required but accessToken is missing, you short‑circuit, reset state, and surface a clear error, and setError("") prevents stale messages on subsequent attempts.

Two small optional tweaks:

  • The "No access token available" message is technically accurate but a bit low‑level; you might want a more user‑actionable string (e.g., hinting at re‑auth/logging in).
  • This path logs "Connection failed:" whereas the page logs "Connection error:"; aligning wording across paths would simplify log analysis.

Also applies to: 43-67, 86-91

src/modules/remote-access/useNetBirdClient.ts (1)

211-218: SSH wrapper updates are correct; consider adding TypeScript interface for improved type safety

The call sites in src/modules/remote-access/ssh/useSSH.ts confirm both methods use the updated signatures correctly:

  • detectSSHServerType(host, port, timeoutMs) at line 48 ✓
  • createSSHConnection(host, port, username, jwtToken?) at line 69 ✓

The wrapper implementations forward all parameters correctly. To prevent future signature drift, consider introducing a TypeScript interface for the NetBird client instead of using any, so signature changes are caught at compile time.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 936de0f and fa33ed4.

📒 Files selected for processing (4)
  • src/app/(remote-access)/peer/ssh/page.tsx (2 hunks)
  • src/modules/remote-access/ssh/useSSH.ts (3 hunks)
  • src/modules/remote-access/useNetBirdClient.ts (2 hunks)
  • src/utils/config.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build_n_push

Comment thread src/utils/config.ts
@heisbrot heisbrot merged commit 43bc069 into main Nov 21, 2025
4 checks passed
@heisbrot heisbrot deleted the fix/ssh-timeout branch November 21, 2025 09:32
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.

2 participants