Skip to content

Conversation

@7ttp
Copy link

@7ttp 7ttp commented Nov 22, 2025

Replaced panic with proper error handling when creating exec commands in containers that are still starting up. The previous implementation would panic with a 409 error when the container was not yet running, causing shuttle run to crash.

This change adds retry logic with container state inspection to gracefully wait for containers to become ready, distinguishing between transient startup delays and actual failures.

Changes made

  • Added retry mechanism with 60 attempts over 30 seconds for container readiness checks
  • Replaced .expect() calls with proper Result error handling using bail!() and .context()
  • Added container state inspection to differentiate between containers that are starting up versus containers that have failed
  • Added detailed error messages with container status and exit codes for debugging

Closes #2133

How has this been tested?

Tested locally with shuttle run using a project with DatabaseSharedPostgres resource. The container now starts successfully without panicking, properly waiting for the container to be ready before attempting to execute commands.

7ttp added 2 commits November 22, 2025 23:38
Replace panic with proper error handling when creating exec commands in containers that are still starting up. The previous implementation would panic with a 409 error when the container was not yet running, causing shuttle run to crash.

This change adds retry logic with container state inspection to gracefully wait for containers to become ready, distinguishing between transient startup delays and actual failures.

fixes shuttle-hq#2133
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 22, 2025

Greptile Overview

Greptile Summary

This PR fixes a race condition where shuttle run would panic when trying to execute commands in Docker containers that were still starting up. The fix replaces .expect() panics with proper error handling and adds retry logic with container state inspection.

Key changes:

  • Added retry mechanism (60 attempts over 30 seconds) for container readiness checks
  • Replaced panic-inducing .expect() calls with Result-based error handling using bail!() and .context()
  • Added container state inspection to differentiate between starting containers and failed containers
  • Added detailed error messages with container status and exit codes for debugging

Suggestions:

  • Consider adding function-level documentation to explain the complex retry and race condition handling logic
  • The conditional logic at lines 271-281 could be simplified as both the "running" and "else" branches perform identical operations

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - it properly handles a critical race condition that was causing panics
  • Score reflects that the core fix is solid and addresses the reported issue effectively. The retry logic with state inspection is a correct approach to handling Docker container startup timing. Minor deductions for: (1) lack of function documentation for complex logic as per project guidelines, and (2) some redundant conditional logic that could be simplified for better readability. These are style improvements rather than functional issues.
  • No files require special attention - the changes are well-contained and the logic is sound

Important Files Changed

File Analysis

Filename Score Overview
cargo-shuttle/src/provisioner_server.rs 4/5 Added retry logic with proper error handling to fix container startup race condition. Implementation is mostly solid but has minor redundant logic in state checking.

Copy link
Contributor

@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.

Additional Comments (1)

  1. cargo-shuttle/src/provisioner_server.rs, line 246-318 (link)

    style: Add function-level documentation explaining the retry logic and race condition handling.

    Context Used: Context from dashboard - Include comments explaining the purpose of functions, especially for new or complex functionality. (source)

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@jymchng
Copy link

jymchng commented Nov 28, 2025

Hi, can anyone look at this PR so that the DB startup issue can be fixed?

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants