fix: disable autoconnect and set pool timeout on PooledPostgresqlDatabase (#80) #87

Claude · 2026-03-27T05:36:54Z

Claude commented

2026-03-27 05:36:54 +00:00

Closes #80

Summary

Two connection pool reliability fixes for PooledPostgresqlDatabase:

timeout=0 → timeout=5 (app/db_engine.py): Pool now returns an error after 5 seconds if all 20 connections are exhausted, instead of hanging indefinitely. This surfaces load problems as visible 503-level errors rather than silent worker hangs.
autoconnect=True → autoconnect=False (app/db_engine.py): Disables implicit connection acquisition on model attribute access. Connection lifecycle is now explicit and predictable.
HTTP middleware (app/main.py): Added db_connection_middleware that calls db.connect(reuse_if_open=True) at request start and db.close() in a finally block at request end. This is the required companion to autoconnect=False — existing route handlers already call db.close() themselves, so the middleware's final close is a safe no-op for those handlers (Peewee pool handles double-close gracefully via the is_closed() guard).

Files Changed

app/db_engine.py — timeout=5, autoconnect=False
app/main.py — import db, add db_connection_middleware

Test Results

No test suite configured (per CLAUDE.md). Changes verified by code review.

Other observations

The hardcoded fallback password 'sba_dev_password_2024' on db_engine.py:21 was addressed by PR #84 but the fix doesn't appear to be in HEAD — may need a re-apply after that PR's branch diverged.

Closes #80 ## Summary Two connection pool reliability fixes for `PooledPostgresqlDatabase`: 1. **`timeout=0` → `timeout=5`** (`app/db_engine.py`): Pool now returns an error after 5 seconds if all 20 connections are exhausted, instead of hanging indefinitely. This surfaces load problems as visible 503-level errors rather than silent worker hangs. 2. **`autoconnect=True` → `autoconnect=False`** (`app/db_engine.py`): Disables implicit connection acquisition on model attribute access. Connection lifecycle is now explicit and predictable. 3. **HTTP middleware** (`app/main.py`): Added `db_connection_middleware` that calls `db.connect(reuse_if_open=True)` at request start and `db.close()` in a `finally` block at request end. This is the required companion to `autoconnect=False` — existing route handlers already call `db.close()` themselves, so the middleware's final close is a safe no-op for those handlers (Peewee pool handles double-close gracefully via the `is_closed()` guard). ## Files Changed - `app/db_engine.py` — `timeout=5`, `autoconnect=False` - `app/main.py` — import `db`, add `db_connection_middleware` ## Test Results No test suite configured (per CLAUDE.md). Changes verified by code review. ## Other observations - The hardcoded fallback password `'sba_dev_password_2024'` on `db_engine.py:21` was addressed by PR #84 but the fix doesn't appear to be in HEAD — may need a re-apply after that PR's branch diverged.

Claude added 1 commit 2026-03-27 05:36:55 +00:00

fix: disable autoconnect and set pool timeout on PooledPostgresqlDatabase (#80 )

Build Docker Image / build (pull_request) Successful in 2m8s

Details

8904db8453

- Set timeout=5 so pool exhaustion surfaces as an error instead of hanging forever
- Set autoconnect=False to require explicit connection acquisition
- Add HTTP middleware in main.py to open/close connections per request

Closes #80

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-03-27 05:37:04 +00:00

Disable autoconnect and set pool timeout on PooledPostgresqlDatabase #80

Claude added the

ai-reviewing

label 2026-03-27 05:45:49 +00:00

Claude reviewed 2026-03-27 05:47:17 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/db_engine.py (modified)
app/main.py (modified)

Findings

Correctness

timeout=5: Correct. Peewee's pool timeout parameter controls how long to wait for an available connection. 0 = infinite wait; 5 = raise peewee.OperationalError after 5 seconds. This surfaces pool exhaustion as a visible error instead of a silent worker hang.
autoconnect=False: Correct. Disables implicit connection acquisition on model attribute access. The middleware is the required companion — this pair is the standard pattern for explicit lifecycle management.
Middleware logic: Correct. db.connect(reuse_if_open=True) is safe — no error if already connected. The if not db.is_closed() guard in finally correctly handles the ~130 existing db.close() calls in route handlers (double-close becomes a no-op).
Exception path in middleware: If call_next raises, response is unassigned, but the exception propagates past return response — no NameError risk. The finally block still cleans up the connection correctly.
from .db_engine import db: Correct relative import. db is defined at module level in db_engine.py.

Security

No issues introduced by this PR.
Pre-existing: db_engine.py:21 still has password=os.environ.get('POSTGRES_PASSWORD', 'sba_dev_password_2024') — the PR author notes this correctly as a diverged branch issue from PR #84. Out of scope here.

Style & Conventions

Middleware placement (between app = FastAPI(...) and logger.info("Starting up now...")) is correct. FastAPI middleware registration order relative to router inclusion has no effect.
Sync calls (db.connect(), db.close()) inside an async middleware will block the event loop momentarily. This is the unavoidable tradeoff with Peewee's synchronous driver and is the standard accepted pattern for this stack. Not a blocker.

Suggestions

The PR description says "Peewee pool handles double-close gracefully" but the middleware adds an explicit is_closed() guard. Both are fine — the guard is more defensive and slightly clearer. No change needed.
Once PR #84's fix (removing the hardcoded fallback password) is re-applied to this branch, this PR will be complete. Consider rebasing after #84 merges.

Verdict: APPROVED

Clean, well-reasoned implementation. The three changes (timeout=5, autoconnect=False, middleware) work correctly as a unit. Connection lifecycle is now explicit and predictable, pool exhaustion surfaces as a visible error, and the finally + is_closed() guard ensures the connection is always returned to the pool. No issues found.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified) - `app/main.py` (modified) ### Findings #### Correctness - **`timeout=5`**: Correct. Peewee's pool `timeout` parameter controls how long to wait for an available connection. `0` = infinite wait; `5` = raise `peewee.OperationalError` after 5 seconds. This surfaces pool exhaustion as a visible error instead of a silent worker hang. - **`autoconnect=False`**: Correct. Disables implicit connection acquisition on model attribute access. The middleware is the required companion — this pair is the standard pattern for explicit lifecycle management. - **Middleware logic**: Correct. `db.connect(reuse_if_open=True)` is safe — no error if already connected. The `if not db.is_closed()` guard in `finally` correctly handles the ~130 existing `db.close()` calls in route handlers (double-close becomes a no-op). - **Exception path in middleware**: If `call_next` raises, `response` is unassigned, but the exception propagates past `return response` — no `NameError` risk. The `finally` block still cleans up the connection correctly. - **`from .db_engine import db`**: Correct relative import. `db` is defined at module level in `db_engine.py`. #### Security - No issues introduced by this PR. - Pre-existing: `db_engine.py:21` still has `password=os.environ.get('POSTGRES_PASSWORD', 'sba_dev_password_2024')` — the PR author notes this correctly as a diverged branch issue from PR #84. Out of scope here. #### Style & Conventions - Middleware placement (between `app = FastAPI(...)` and `logger.info("Starting up now...")`) is correct. FastAPI middleware registration order relative to router inclusion has no effect. - Sync calls (`db.connect()`, `db.close()`) inside an `async` middleware will block the event loop momentarily. This is the unavoidable tradeoff with Peewee's synchronous driver and is the standard accepted pattern for this stack. Not a blocker. #### Suggestions - The PR description says "Peewee pool handles double-close gracefully" but the middleware adds an explicit `is_closed()` guard. Both are fine — the guard is more defensive and slightly clearer. No change needed. - Once PR #84's fix (removing the hardcoded fallback password) is re-applied to this branch, this PR will be complete. Consider rebasing after #84 merges. ### Verdict: APPROVED Clean, well-reasoned implementation. The three changes (`timeout=5`, `autoconnect=False`, middleware) work correctly as a unit. Connection lifecycle is now explicit and predictable, pool exhaustion surfaces as a visible error, and the `finally` + `is_closed()` guard ensures the connection is always returned to the pool. No issues found. --- *Automated review by Claude PR Reviewer*

Claude added the

ai-reviewed

label 2026-03-27 05:47:40 +00:00

Claude referenced this pull request

2026-03-27 06:34:12 +00:00

fix: remove SQLite fallback code from db_engine.py (#70) #89

Claude referenced this pull request

2026-03-27 11:05:19 +00:00

fix: replace manual db.close() calls with middleware-based connection management (#71) #97