Skip to content

Fix setup SQL connect panic and add startup retries#3827

Open
ljluestc wants to merge 1 commit intosemaphoreui:developfrom
ljluestc:private/issue-1653-mysql-docker-compose
Open

Fix setup SQL connect panic and add startup retries#3827
ljluestc wants to merge 1 commit intosemaphoreui:developfrom
ljluestc:private/issue-1653-mysql-docker-compose

Conversation

@ljluestc
Copy link
Copy Markdown

@ljluestc ljluestc commented May 4, 2026

What problem does this PR solve?

Issue Number: close #1653

When running Semaphore with MySQL in Docker Compose, startup can fail during early database initialization windows.

Two concrete failures were reported:

  1. Setup panic after DB auth/connect failure due to deferred close on an uninitialized SQL connection.
  2. Setup failing too early while MySQL/user/database readiness is still converging.

What changed and how does it work?

This PR hardens SQL setup/startup behavior:

  1. Safe setup close ordering (cli/cmd/setup.go)

    • In setup flow, connect first, then defer close.
    • Prevents deferred close from running against a not-yet-initialized SQL store after connect failure.
  2. Nil-safe SQL close (db/sql/SqlDb.go)

    • SqlDbConnection.Close() now safely no-ops when SQL handles are not initialized.
    • Clears internal SQL handle map after close (d.sql = nil).
  3. SQL connect retry/backoff for container startup race (db/sql/SqlDb.go)

    • Connect() now retries ping/connect with bounded attempts and backoff.
    • On failed attempts, transient handles are closed and DB creation/readiness path is retried.
    • Final failure includes wrapped context about attempts/errors.
  4. Regression test (db/sql/SqlDb_test.go)

    • Adds test coverage to validate close-on-uninitialized-connection does not panic.

Why this approach?

  • Fixes the reported panic directly with minimal behavioral risk.
  • Makes startup robust to common containerized DB readiness timing.
  • Keeps logic centralized in SQL connection handling so both setup and runtime paths benefit.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test
    • go -C /home/calelin/dev/semaphore test ./db/sql
    • go -C /home/calelin/dev/semaphore test ./cli/setup
  • No need to test

Documentation

  • Affects user behaviors
  • Additional docs required

Release note

Improve MySQL startup reliability in Docker Compose by adding retry/backoff during SQL connect and fixing a setup panic caused by closing uninitialized SQL connections.

@fiftin fiftin marked this pull request as ready for review May 4, 2026 12:56
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf9be5d5ab

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread db/sql/SqlDb.go
Comment on lines +84 to +85
if lastErr != nil {
panic(fmt.Errorf("cannot connect to database after %d attempts: %w", connectMaxAttempts, lastErr))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Check current error state before panicking after retries

The retry loop breaks on a successful connect()+Ping, but lastErr is never cleared, so any earlier transient failure causes this panic path to execute even after a later successful attempt. In a normal startup race (e.g., DB ready on attempt 2+), setup/runtime will still fail after the loop despite having a valid connection. This regresses the intended retry behavior; the post-loop failure check should be based on the final err (or reset lastErr on success).

Useful? React with 👍 / 👎.

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.

Can't connect to MySQL, docker compose

1 participant