Skip to content

Conversation

@itzlambda
Copy link
Collaborator

@itzlambda itzlambda commented Dec 13, 2025

Summary

Unifies SSH retry timeout handling and improves error messages for better UX.

Changes

  • Extract RENTAL_READY_TIMEOUT constant (5 minutes) to replace duplicate definitions
  • Fix timeout inconsistency: handlers were using 120s while poll functions used 300s
  • Improve spinner message from "SSH not ready yet" to "Waiting for SSH..."
  • Move raw SSH error details to debug logs (reduces noise for users)
  • Provide actionable suggestion with explicit basilica ssh <rental_id> command

Summary by CodeRabbit

  • Improvements
    • Clarified SSH readiness messaging from "SSH not ready yet, retrying..." to "Waiting for SSH..." for better user guidance
    • Standardized GPU rental activation and SSH readiness timeout to 300 seconds across the system
    • Enhanced error logging when SSH connection attempts fail, providing better diagnostic information

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

- Extract RENTAL_READY_TIMEOUT constant (5 min) to replace duplicates
- Fix timeout inconsistency: handlers used 120s, poll functions 300s
- Improve spinner message to "Waiting for SSH..."
- Move raw SSH error to debug logs, cleaner user-facing message
- Actionable suggestion: "basilica ssh <rental_id>"
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 13, 2025

Walkthrough

This PR standardizes timeout handling in GPU rental SSH and polling operations by introducing a reusable RENTAL_READY_TIMEOUT constant (300 seconds), replacing hard-coded duration values across multiple functions, updating user-facing messaging, and adding debug logging for failed SSH retry attempts.

Changes

Cohort / File(s) Summary
Timeout Standardization & SSH Readiness
crates/basilica-cli/src/cli/handlers/gpu_rental.rs
Adds new constant RENTAL_READY_TIMEOUT (300s); replaces hard-coded Duration::from_secs(120) and Duration::from_secs(300) values with the constant across multiple functions (handle_secure_cloud_rental_with_offering, handle_community_cloud_rental_with_selection, poll_rental_status, poll_secure_cloud_rental_status, retry_ssh_connection); updates SSH waiting message from "SSH not ready yet, retrying..." to "Waiting for SSH..."; adds debug logging when final SSH retry fails

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verification points: Ensure all hard-coded duration values have been consistently replaced with the new constant across all five affected functions; confirm messaging changes match intended UX updates

Possibly related PRs

Poem

🐰 Hop, hop! The timeouts now unite,
One constant rules them all so tight!
SSH waits with patient cheer,
Three hundred seconds, crystal clear!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main changes: unifying SSH retry timeouts and improving error messages through better logging.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 ssh-retry-basilica

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4d5bae1 and 7d6a6f3.

📒 Files selected for processing (1)
  • crates/basilica-cli/src/cli/handlers/gpu_rental.rs (7 hunks)
🔇 Additional comments (6)
crates/basilica-cli/src/cli/handlers/gpu_rental.rs (6)

36-38: LGTM! Good centralization of timeout configuration.

Introducing a shared constant eliminates the previous inconsistency between handler timeouts (120s) and poll function timeouts (300s). The 5-minute window is reasonable for GPU rental provisioning.


524-530: LGTM! Timeout now consistent with poll functions.

This increases the SSH retry timeout from 120s to 300s, which aligns with the rental polling timeout. Users will wait longer before seeing SSH failures, but this is appropriate since SSH services may take time to become available after provisioning.


684-690: Consistent with secure cloud handler.


1902-1920: LGTM! Replaced local constant with shared timeout.

The poll_rental_status function now uses the centralized RENTAL_READY_TIMEOUT constant. The exponential backoff logic (2s initial, 10s max interval) remains unchanged.


1989-2008: LGTM! Consistent use of shared timeout.

The secure cloud polling uses longer intervals (5s initial, 15s max) compared to community cloud (2s initial, 10s max), which is appropriate for typically longer secure cloud provisioning times.


2111-2143: LGTM! Improved user experience and debuggability.

Good improvements:

  • Cleaner spinner message reduces terminal noise
  • Raw SSH errors moved to debug logs helps troubleshooting without cluttering user output
  • Actionable suggestion with explicit basilica ssh <rental_id> command guides users on next steps

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@itzlambda itzlambda merged commit e40c075 into main Dec 13, 2025
14 checks passed
@itzlambda itzlambda deleted the ssh-retry-basilica branch December 13, 2025 09:44
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