Skip to content

fix: prevent startup panic when database pool creation fails#3708

Open
tusharmath wants to merge 3 commits into
mainfrom
fix/db-pool-unwrap-panic
Open

fix: prevent startup panic when database pool creation fails#3708
tusharmath wants to merge 3 commits into
mainfrom
fix/db-pool-unwrap-panic

Conversation

@tusharmath

Copy link
Copy Markdown
Collaborator

Summary

Fix an app crash at startup caused by DatabasePool::try_from(...).unwrap() when the SQLite database is unavailable (e.g. the file is locked by another process), by deferring pool creation to the first connection request so failures surface as recoverable errors.

Changes

  • Replace the fallible TryFrom<PoolConfig> constructor with an infallible DatabasePool::new that stores the config and lazily builds the pool
  • Initialize the pool on the first get_connection call, retrying with exponential backoff and caching the pool on success
  • Update ForgeRepo::new to drop the unwrap() on pool construction
  • Add regression tests covering non-panicking construction with a broken path, error propagation from get_connection, and lazy initialization/caching

Key Implementation Details

The pool is now held as Mutex<Option<DbPool>>: construction never touches the database, and the first get_connection builds the pool under the lock, reusing the existing retry_with_backoff logic that previously ran eagerly in TryFrom. A poisoned mutex is mapped to an error rather than a panic. The in-memory test constructor pre-populates the pool so its behaviour is unchanged.

Testing

Run the new unit tests in crates/forge_repo/src/database/pool.rs: test_new_with_broken_path_does_not_panic_or_connect, test_get_connection_with_broken_path_returns_error, and test_get_connection_initializes_pool_lazily. To verify end-to-end, start the app while another process holds a lock on the SQLite file and confirm it no longer panics at startup.

@github-actions github-actions Bot added the type: fix Iterations on existing features or infrastructure. label Jul 17, 2026
@tusharmath
tusharmath enabled auto-merge (squash) July 17, 2026 05:43

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the fix! This is a solid improvement to the database initialization flow.

Replacing the eager pool creation and unwrap() with lazy initialization makes startup much more robust, especially when the database is temporarily unavailable or locked. The new implementation surfaces initialization failures as recoverable errors instead of crashing the application, which is a much better user experience.

I also appreciate the added regression tests covering:

  • construction with an invalid database path,
  • recoverable connection failures,
  • lazy initialization behavior, and
  • successful pool caching after first use.

The refactoring keeps the API straightforward while improving reliability. Looks good to me—nice work!

@github-actions

Copy link
Copy Markdown

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

@github-actions github-actions Bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. type: fix Iterations on existing features or infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants