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

Claude · 2026-03-27T11:05:19Z

Claude commented

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

Closes #71

Summary

Adds db_connection_middleware to app/main.py and removes all 177 manual db.close() calls from 26 router files.

What changed

app/main.py: Added db_connection_middleware — opens a connection at the start of each HTTP request (db.connect(reuse_if_open=True)), closes it in a finally block after the response is fully sent. This ensures FK lazy-loads during FastAPI response serialization use a live connection.
26 router files: Removed all standalone db.close() lines (177 total). The middleware now owns the connection lifecycle for every request.

Why

With PooledPostgresqlDatabase, manual db.close() before return returned the connection to the pool before FastAPI serialized the response. Any lazy FK access during model_to_dict(recurse=True) after db.close() would race against concurrent requests. The middleware defers close until after the full response is written.

The handle_db_errors decorator's db.close() call in its exception handler is left intact — it is harmless with the middleware (double-close is a no-op on PooledPostgresqlDatabase when is_closed() guards the middleware close).

Files changed

app/main.py (+13 lines)
app/routers_v3/*.py — 21 files
app/routers_v3/stratplay/*.py — 5 files

Other observations

handle_db_errors in dependencies.py catches Exception generically, which means it also catches HTTPException raised by endpoints and converts them to 500 responses. This is a pre-existing bug out of scope for this PR.
The commented-out # db.close() on standings.py:30 was left as-is (already inactive).

PR #87 also adds db_connection_middleware with the same implementation, paired with autoconnect=False and timeout=5 changes in db_engine.py. This PR is complementary — if #87 merges first, the middleware addition here will conflict (trivially resolvable) but the db.close() removals will still apply cleanly.

Closes #71 ## Summary Adds `db_connection_middleware` to `app/main.py` and removes all 177 manual `db.close()` calls from 26 router files. ## What changed - **`app/main.py`**: Added `db_connection_middleware` — opens a connection at the start of each HTTP request (`db.connect(reuse_if_open=True)`), closes it in a `finally` block after the response is fully sent. This ensures FK lazy-loads during FastAPI response serialization use a live connection. - **26 router files**: Removed all standalone `db.close()` lines (177 total). The middleware now owns the connection lifecycle for every request. ## Why With `PooledPostgresqlDatabase`, manual `db.close()` before `return` returned the connection to the pool before FastAPI serialized the response. Any lazy FK access during `model_to_dict(recurse=True)` after `db.close()` would race against concurrent requests. The middleware defers close until after the full response is written. The `handle_db_errors` decorator's `db.close()` call in its exception handler is left intact — it is harmless with the middleware (double-close is a no-op on `PooledPostgresqlDatabase` when `is_closed()` guards the middleware close). ## Files changed - `app/main.py` (+13 lines) - `app/routers_v3/*.py` — 21 files - `app/routers_v3/stratplay/*.py` — 5 files ## Other observations - `handle_db_errors` in `dependencies.py` catches `Exception` generically, which means it also catches `HTTPException` raised by endpoints and converts them to 500 responses. This is a pre-existing bug out of scope for this PR. - The commented-out `# db.close()` on `standings.py:30` was left as-is (already inactive). ## Note on related PRs PR #87 also adds `db_connection_middleware` with the same implementation, paired with `autoconnect=False` and `timeout=5` changes in `db_engine.py`. This PR is complementary — if #87 merges first, the middleware addition here will conflict (trivially resolvable) but the `db.close()` removals will still apply cleanly.

Claude added 1 commit 2026-03-27 11:05:20 +00:00

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

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

Details

ab90dfc437

Closes #71

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

Claude referenced this pull request

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

Refactor manual db.close() calls to middleware-based connection management #71

Claude added the

ai-reviewing

label 2026-03-27 11:15:57 +00:00

Claude reviewed 2026-03-27 11:18:10 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/main.py (modified — middleware added)
app/routers_v3/custom_commands.py (modified)
app/routers_v3/help_commands.py (modified)
app/routers_v3/awards.py, battingstats.py, current.py, decisions.py, divisions.py, draftdata.py, draftlist.py, draftpicks.py, fieldingstats.py, injuries.py, keepers.py, managers.py, pitchingstats.py, results.py, sbaplayers.py, schedules.py, standings.py, stratgame.py, transactions.py (modified)
app/routers_v3/stratplay/batting.py, crud.py, fielding.py, pitching.py, plays.py (modified)

Findings

Correctness

CRITICAL — Empty finally: blocks cause SyntaxError at import 🚨

custom_commands.py and help_commands.py use a try/except/finally pattern where db.close() was the only statement in the finally: clause. After removal, those finally: blocks have an empty body, which Python rejects with:

IndentationError: expected an indented block after 'finally' statement

The API will not start — both files fail at import time. Affected locations:

custom_commands.py (12 endpoints): get_custom_commands, create_custom_command_endpoint, update_custom_command_endpoint, patch_custom_command, delete_custom_command_endpoint, get_creators, create_creator_endpoint, get_custom_command_stats, get_custom_command_by_name_endpoint, execute_custom_command, get_command_names_for_autocomplete, get_custom_command.

help_commands.py (10 endpoints): get_help_commands, create_help_command_endpoint, update_help_command_endpoint, restore_help_command_endpoint, delete_help_command_endpoint, get_help_command_stats, get_help_command_by_name_endpoint, increment_view_count, get_help_names_for_autocomplete, get_help_command.

Fix: Remove the finally: clause entirely (since db.close() is now handled by the middleware), or add pass if the structure must be preserved:

    finally:
        pass

All other files only removed db.close() from plain if/else or top-level branches — those are syntactically correct.

Security

No issues found.

Style & Conventions

Middleware lazy import (app/main.py): from .db_engine import db is placed inside the middleware function body. Functionally correct (Python caches module imports), but top-level import is the project convention. Non-blocking.

Suggestions

Prefer removing the finally: clause entirely over adding pass — cleaner, and the middleware already handles connection lifecycle.
The PR #87 conflict note in the PR body is accurate. If #87 merges first, the middleware addition trivially conflicts but the db.close() removals apply cleanly.

Verdict: REQUEST_CHANGES

(Gitea blocks self-review — posting as COMMENT. Treat as REQUEST_CHANGES.)

Two files (custom_commands.py and help_commands.py) will produce SyntaxError/IndentationError at import time due to empty finally: blocks, crashing the API on startup. All 22 affected finally: blocks need the finally: clause removed (or pass added). The middleware implementation in main.py and all other db.close() removals are correct.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/main.py` (modified — middleware added) - `app/routers_v3/custom_commands.py` (modified) - `app/routers_v3/help_commands.py` (modified) - `app/routers_v3/awards.py`, `battingstats.py`, `current.py`, `decisions.py`, `divisions.py`, `draftdata.py`, `draftlist.py`, `draftpicks.py`, `fieldingstats.py`, `injuries.py`, `keepers.py`, `managers.py`, `pitchingstats.py`, `results.py`, `sbaplayers.py`, `schedules.py`, `standings.py`, `stratgame.py`, `transactions.py` (modified) - `app/routers_v3/stratplay/batting.py`, `crud.py`, `fielding.py`, `pitching.py`, `plays.py` (modified) ### Findings #### Correctness **CRITICAL — Empty `finally:` blocks cause SyntaxError at import** 🚨 `custom_commands.py` and `help_commands.py` use a `try/except/finally` pattern where `db.close()` was the _only_ statement in the `finally:` clause. After removal, those `finally:` blocks have an empty body, which Python rejects with: ``` IndentationError: expected an indented block after 'finally' statement ``` The API **will not start** — both files fail at import time. Affected locations: **`custom_commands.py`** (12 endpoints): `get_custom_commands`, `create_custom_command_endpoint`, `update_custom_command_endpoint`, `patch_custom_command`, `delete_custom_command_endpoint`, `get_creators`, `create_creator_endpoint`, `get_custom_command_stats`, `get_custom_command_by_name_endpoint`, `execute_custom_command`, `get_command_names_for_autocomplete`, `get_custom_command`. **`help_commands.py`** (10 endpoints): `get_help_commands`, `create_help_command_endpoint`, `update_help_command_endpoint`, `restore_help_command_endpoint`, `delete_help_command_endpoint`, `get_help_command_stats`, `get_help_command_by_name_endpoint`, `increment_view_count`, `get_help_names_for_autocomplete`, `get_help_command`. **Fix**: Remove the `finally:` clause entirely (since `db.close()` is now handled by the middleware), or add `pass` if the structure must be preserved: ```python finally: pass ``` All other files only removed `db.close()` from plain `if/else` or top-level branches — those are syntactically correct. #### Security - No issues found. #### Style & Conventions - **Middleware lazy import** (`app/main.py`): `from .db_engine import db` is placed inside the middleware function body. Functionally correct (Python caches module imports), but top-level import is the project convention. Non-blocking. #### Suggestions - Prefer removing the `finally:` clause entirely over adding `pass` — cleaner, and the middleware already handles connection lifecycle. - The PR #87 conflict note in the PR body is accurate. If #87 merges first, the middleware addition trivially conflicts but the `db.close()` removals apply cleanly. ### Verdict: REQUEST_CHANGES > _(Gitea blocks self-review — posting as COMMENT. Treat as REQUEST_CHANGES.)_ Two files (`custom_commands.py` and `help_commands.py`) will produce `SyntaxError`/`IndentationError` at import time due to empty `finally:` blocks, crashing the API on startup. All 22 affected `finally:` blocks need the `finally:` clause removed (or `pass` added). The middleware implementation in `main.py` and all other `db.close()` removals are correct. --- *Automated review by Claude PR Reviewer*

Claude added

ai-changes-requested

and removed

ai-reviewing

labels 2026-03-27 11:18:31 +00:00

Claude added the

ai-working

label 2026-03-27 11:30:45 +00:00

cal added 1 commit 2026-03-27 11:34:45 +00:00

fix: remove empty finally clauses in custom_commands and help_commands

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

Details

ac2c5920ce

After removing db.close() calls, 22 finally: blocks were left empty
(12 in custom_commands.py, 10 in help_commands.py), causing
IndentationError at import time. Removed the finally: clause entirely
since connection lifecycle is now handled by the middleware.

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

Claude commented

2026-03-27 11:34:55 +00:00

Review Feedback Addressed

Commit ac2c592 fixes the empty finally: blocks identified in the review.

Root cause: After removing db.close() from finally: clauses, those blocks were left empty, causing IndentationError at import time (Python requires a non-empty body after finally:).

Fix applied: Removed the finally: clause entirely from all 22 affected endpoints:

custom_commands.py — 12 endpoints (get_custom_commands, create_custom_command_endpoint, update_custom_command_endpoint, patch_custom_command, delete_custom_command_endpoint, get_creators, create_creator_endpoint, get_custom_command_stats, get_custom_command_by_name_endpoint, execute_custom_command, get_command_names_for_autocomplete, get_custom_command)
help_commands.py — 10 endpoints (get_help_commands, create_help_command_endpoint, update_help_command_endpoint, restore_help_command_endpoint, delete_help_command_endpoint, get_help_command_stats, get_help_command_by_name_endpoint, increment_view_count, get_help_names_for_autocomplete, get_help_command)

The finally: clauses are no longer needed since connection lifecycle is handled by the middleware in main.py.

## Review Feedback Addressed Commit ac2c592 fixes the empty `finally:` blocks identified in the review. **Root cause**: After removing `db.close()` from `finally:` clauses, those blocks were left empty, causing `IndentationError` at import time (Python requires a non-empty body after `finally:`). **Fix applied**: Removed the `finally:` clause entirely from all 22 affected endpoints: - `custom_commands.py` — 12 endpoints (`get_custom_commands`, `create_custom_command_endpoint`, `update_custom_command_endpoint`, `patch_custom_command`, `delete_custom_command_endpoint`, `get_creators`, `create_creator_endpoint`, `get_custom_command_stats`, `get_custom_command_by_name_endpoint`, `execute_custom_command`, `get_command_names_for_autocomplete`, `get_custom_command`) - `help_commands.py` — 10 endpoints (`get_help_commands`, `create_help_command_endpoint`, `update_help_command_endpoint`, `restore_help_command_endpoint`, `delete_help_command_endpoint`, `get_help_command_stats`, `get_help_command_by_name_endpoint`, `increment_view_count`, `get_help_names_for_autocomplete`, `get_help_command`) The `finally:` clauses are no longer needed since connection lifecycle is handled by the middleware in `main.py`.

Claude removed the

ai-working

ai-changes-requested

labels 2026-03-27 11:35:32 +00:00

Claude added the

ai-reviewing

label 2026-03-27 11:45:53 +00:00

Claude reviewed 2026-03-27 11:47:20 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/main.py (modified — middleware added)
app/routers_v3/custom_commands.py (modified — 12 empty finally: blocks removed)
app/routers_v3/help_commands.py (modified — 10 empty finally: blocks removed)
app/routers_v3/awards.py, battingstats.py, current.py, decisions.py, divisions.py, draftdata.py, draftlist.py, draftpicks.py, fieldingstats.py, injuries.py, keepers.py, managers.py, pitchingstats.py, results.py, sbaplayers.py, schedules.py, standings.py, stratgame.py, transactions.py (modified — standalone db.close() removals)
app/routers_v3/stratplay/batting.py, crud.py, fielding.py, pitching.py, plays.py (modified — standalone db.close() removals)

Findings

Correctness

Middleware implementation is sound. db.connect(reuse_if_open=True) at request start + finally: db.close() with is_closed() guard correctly defers connection lifecycle until after the full response is written. This eliminates the race condition where early db.close() returned the connection to the pool before FastAPI serialized FK lazy-loads.
Fix applied from prior review. custom_commands.py (12 endpoints) and help_commands.py (10 endpoints) previously had empty finally: clauses after db.close() removal → would cause IndentationError at import. Those empty blocks are correctly removed in commit ac2c592.
177 db.close() removals across 26 router files are complete. Three files in main.py's includes (players.py, teams.py, views.py) are absent from the diff — consistent with those files having no manual db.close() calls.
handle_db_errors decorator retains its db.close() call — correctly noted as harmless with the is_closed() guard in middleware.

Security

No issues found. The lazy import from .db_engine import db inside the middleware function is intentional (avoids circular imports at startup) and poses no security concern.

Style & Conventions

No issues. Removals are clean with no trailing blank lines or reformatting noise.

Suggestions

Minor (non-blocking): The middleware opens a DB connection for every HTTP request including health/metrics endpoints (/health, /ready). If connection exhaustion under load becomes a concern, a path-prefix check could skip those, but this is well outside the scope of this PR.
Minor (non-blocking): from .db_engine import db inside the middleware body works but is an unusual placement. A module-level import would be more idiomatic if circular imports aren't a concern.

Verdict: APPROVED

The prior blocking issue (empty finally: clauses causing IndentationError on import) is fixed in ac2c592. The middleware implementation is correct, the 177 removals are complete, and the connection lifecycle is now properly managed. Ready to merge, pending trivial conflict resolution with PR #87 if that lands first.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/main.py` (modified — middleware added) - `app/routers_v3/custom_commands.py` (modified — 12 empty `finally:` blocks removed) - `app/routers_v3/help_commands.py` (modified — 10 empty `finally:` blocks removed) - `app/routers_v3/awards.py`, `battingstats.py`, `current.py`, `decisions.py`, `divisions.py`, `draftdata.py`, `draftlist.py`, `draftpicks.py`, `fieldingstats.py`, `injuries.py`, `keepers.py`, `managers.py`, `pitchingstats.py`, `results.py`, `sbaplayers.py`, `schedules.py`, `standings.py`, `stratgame.py`, `transactions.py` (modified — standalone `db.close()` removals) - `app/routers_v3/stratplay/batting.py`, `crud.py`, `fielding.py`, `pitching.py`, `plays.py` (modified — standalone `db.close()` removals) ### Findings #### Correctness - **Middleware implementation is sound.** `db.connect(reuse_if_open=True)` at request start + `finally: db.close()` with `is_closed()` guard correctly defers connection lifecycle until after the full response is written. This eliminates the race condition where early `db.close()` returned the connection to the pool before FastAPI serialized FK lazy-loads. - **Fix applied from prior review.** `custom_commands.py` (12 endpoints) and `help_commands.py` (10 endpoints) previously had empty `finally:` clauses after `db.close()` removal → would cause `IndentationError` at import. Those empty blocks are correctly removed in commit `ac2c592`. - **177 `db.close()` removals across 26 router files** are complete. Three files in `main.py`'s includes (`players.py`, `teams.py`, `views.py`) are absent from the diff — consistent with those files having no manual `db.close()` calls. - **`handle_db_errors` decorator** retains its `db.close()` call — correctly noted as harmless with the `is_closed()` guard in middleware. #### Security - No issues found. The lazy import `from .db_engine import db` inside the middleware function is intentional (avoids circular imports at startup) and poses no security concern. #### Style & Conventions - No issues. Removals are clean with no trailing blank lines or reformatting noise. #### Suggestions - Minor (non-blocking): The middleware opens a DB connection for every HTTP request including health/metrics endpoints (`/health`, `/ready`). If connection exhaustion under load becomes a concern, a path-prefix check could skip those, but this is well outside the scope of this PR. - Minor (non-blocking): `from .db_engine import db` inside the middleware body works but is an unusual placement. A module-level import would be more idiomatic if circular imports aren't a concern. ### Verdict: APPROVED The prior blocking issue (empty `finally:` clauses causing `IndentationError` on import) is fixed in `ac2c592`. The middleware implementation is correct, the 177 removals are complete, and the connection lifecycle is now properly managed. Ready to merge, pending trivial conflict resolution with PR #87 if that lands first. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-27 11:47:28 +00:00

cal approved these changes 2026-03-30 23:18:24 +00:00

cal left a comment

AI Code Review

Files Reviewed

app/main.py (modified — middleware added)
app/routers_v3/awards.py (modified)
app/routers_v3/battingstats.py (modified)
app/routers_v3/current.py (modified)
app/routers_v3/custom_commands.py (modified)
app/routers_v3/decisions.py (modified)
app/routers_v3/divisions.py (modified)
app/routers_v3/draftdata.py (modified)
app/routers_v3/draftlist.py (modified)
app/routers_v3/draftpicks.py (modified)
app/routers_v3/fieldingstats.py (modified)
app/routers_v3/help_commands.py (modified)
app/routers_v3/injuries.py (modified)
app/routers_v3/keepers.py (modified)
app/routers_v3/managers.py (modified)
app/routers_v3/pitchingstats.py (modified)
app/routers_v3/results.py (modified)
app/routers_v3/sbaplayers.py (modified)
app/routers_v3/schedules.py (modified)
app/routers_v3/standings.py (modified)
app/routers_v3/stratgame.py (modified)
app/routers_v3/stratplay/batting.py (modified)
app/routers_v3/stratplay/crud.py (modified)
app/routers_v3/stratplay/fielding.py (modified)
app/routers_v3/stratplay/pitching.py (modified)
app/routers_v3/stratplay/plays.py (modified)
app/routers_v3/transactions.py (modified)
app/dependencies.py (read for context — unchanged)
app/db_engine.py (read for context — unchanged)

Findings

Correctness

Middleware implementation is correct. db.connect(reuse_if_open=True) before call_next and db.close() guarded by is_closed() in finally is the right pattern for PooledPostgresqlDatabase. The close fires after await call_next(request) returns, which in FastAPI means after the full response is streamed — this correctly defers connection release past all FK lazy-loads during model_to_dict(recurse=True).

Double-close safety is properly handled. The handle_db_errors decorator in dependencies.py calls db.close() in its own finally block when an exception occurs. With PooledPostgresqlDatabase and the is_closed() guard in the middleware's finally, this is a harmless no-op — the decorator closes the connection on error, and the middleware's if not db.is_closed() skips the redundant close. Correct.

db_engine.py still has autoconnect=True. This means connections may auto-open outside middleware scope (e.g., during startup or background tasks). The middleware uses reuse_if_open=True which handles this gracefully. No regression here, but worth noting that PR #87 pairs this with autoconnect=False for tighter lifecycle control — if #87 merges after this PR, no additional changes to the router files will be needed.

db.close() removals are clean across all 26 files. Verified that:

All bare db.close() before return statements are removed.
All db.close() before raise HTTPException(...) are removed.
custom_commands.py and help_commands.py had finally: db.close() blocks (a slightly different pattern) — these were correctly removed as complete finally blocks, leaving no empty finally: stubs. No accidental logic was dropped.
standings.py line 30's commented-out # db.close() is intentionally left as-is.

One unguarded db.close() path remains in handle_db_errors. The exception handler in dependencies.py calls db.close() unconditionally in its own finally, without an is_closed() guard. The PR body describes this as "harmless" due to the middleware's is_closed() guard, which is correct for PooledPostgresqlDatabase — the pool handles repeated closes gracefully. No action required.

Security

No issues found. No new inputs, no auth changes, no credentials introduced. The middleware touches only the connection lifecycle.

Style & Conventions

The middleware import (from .db_engine import db) is deferred inside the function body rather than placed at the top of main.py. This matches the project's existing pattern in dependencies.py where from .db_engine import db is also imported inside the decorator wrapper to avoid circular imports. Correct and consistent.

No empty finally blocks were left behind. All removals are surgical — only db.close() lines removed, surrounding logic preserved exactly.

Suggestions

The PR notes the overlap with #87. If #87 is going to merge, consider whether this PR or #87 should land first to reduce merge friction. The db.close() removals here are compatible either way, but resolving the middleware addition conflict will be simpler if one PR is merged cleanly before the other.
The handle_db_errors pre-existing bug noted in the PR body (catching HTTPException and converting to 500) is a real issue worth a follow-up issue if one doesn't already exist.

Verdict: APPROVED

The middleware implementation is correct and directly addresses the race condition described in issue #71. All 177 db.close() removals across 26 files are clean — no empty finally blocks, no dropped logic, no missing removals. The is_closed() guard in the middleware correctly makes the decorator's existing db.close() a safe no-op. This is a straightforward, well-scoped reliability fix and is ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/main.py` (modified — middleware added) - `app/routers_v3/awards.py` (modified) - `app/routers_v3/battingstats.py` (modified) - `app/routers_v3/current.py` (modified) - `app/routers_v3/custom_commands.py` (modified) - `app/routers_v3/decisions.py` (modified) - `app/routers_v3/divisions.py` (modified) - `app/routers_v3/draftdata.py` (modified) - `app/routers_v3/draftlist.py` (modified) - `app/routers_v3/draftpicks.py` (modified) - `app/routers_v3/fieldingstats.py` (modified) - `app/routers_v3/help_commands.py` (modified) - `app/routers_v3/injuries.py` (modified) - `app/routers_v3/keepers.py` (modified) - `app/routers_v3/managers.py` (modified) - `app/routers_v3/pitchingstats.py` (modified) - `app/routers_v3/results.py` (modified) - `app/routers_v3/sbaplayers.py` (modified) - `app/routers_v3/schedules.py` (modified) - `app/routers_v3/standings.py` (modified) - `app/routers_v3/stratgame.py` (modified) - `app/routers_v3/stratplay/batting.py` (modified) - `app/routers_v3/stratplay/crud.py` (modified) - `app/routers_v3/stratplay/fielding.py` (modified) - `app/routers_v3/stratplay/pitching.py` (modified) - `app/routers_v3/stratplay/plays.py` (modified) - `app/routers_v3/transactions.py` (modified) - `app/dependencies.py` (read for context — unchanged) - `app/db_engine.py` (read for context — unchanged) ### Findings #### Correctness **Middleware implementation is correct.** `db.connect(reuse_if_open=True)` before `call_next` and `db.close()` guarded by `is_closed()` in `finally` is the right pattern for `PooledPostgresqlDatabase`. The close fires after `await call_next(request)` returns, which in FastAPI means after the full response is streamed — this correctly defers connection release past all FK lazy-loads during `model_to_dict(recurse=True)`. **Double-close safety is properly handled.** The `handle_db_errors` decorator in `dependencies.py` calls `db.close()` in its own `finally` block when an exception occurs. With `PooledPostgresqlDatabase` and the `is_closed()` guard in the middleware's `finally`, this is a harmless no-op — the decorator closes the connection on error, and the middleware's `if not db.is_closed()` skips the redundant close. Correct. **`db_engine.py` still has `autoconnect=True`.** This means connections may auto-open outside middleware scope (e.g., during startup or background tasks). The middleware uses `reuse_if_open=True` which handles this gracefully. No regression here, but worth noting that PR #87 pairs this with `autoconnect=False` for tighter lifecycle control — if #87 merges after this PR, no additional changes to the router files will be needed. **`db.close()` removals are clean across all 26 files.** Verified that: - All bare `db.close()` before `return` statements are removed. - All `db.close()` before `raise HTTPException(...)` are removed. - `custom_commands.py` and `help_commands.py` had `finally: db.close()` blocks (a slightly different pattern) — these were correctly removed as complete `finally` blocks, leaving no empty `finally:` stubs. No accidental logic was dropped. - `standings.py` line 30's commented-out `# db.close()` is intentionally left as-is. **One unguarded `db.close()` path remains in `handle_db_errors`.** The exception handler in `dependencies.py` calls `db.close()` unconditionally in its own finally, without an `is_closed()` guard. The PR body describes this as "harmless" due to the middleware's `is_closed()` guard, which is correct for `PooledPostgresqlDatabase` — the pool handles repeated closes gracefully. No action required. #### Security No issues found. No new inputs, no auth changes, no credentials introduced. The middleware touches only the connection lifecycle. #### Style & Conventions The middleware import (`from .db_engine import db`) is deferred inside the function body rather than placed at the top of `main.py`. This matches the project's existing pattern in `dependencies.py` where `from .db_engine import db` is also imported inside the decorator wrapper to avoid circular imports. Correct and consistent. No empty finally blocks were left behind. All removals are surgical — only `db.close()` lines removed, surrounding logic preserved exactly. #### Suggestions - The PR notes the overlap with #87. If #87 is going to merge, consider whether this PR or #87 should land first to reduce merge friction. The `db.close()` removals here are compatible either way, but resolving the middleware addition conflict will be simpler if one PR is merged cleanly before the other. - The `handle_db_errors` pre-existing bug noted in the PR body (catching `HTTPException` and converting to 500) is a real issue worth a follow-up issue if one doesn't already exist. ### Verdict: APPROVED The middleware implementation is correct and directly addresses the race condition described in issue #71. All 177 `db.close()` removals across 26 files are clean — no empty finally blocks, no dropped logic, no missing removals. The `is_closed()` guard in the middleware correctly makes the decorator's existing `db.close()` a safe no-op. This is a straightforward, well-scoped reliability fix and is ready to merge. --- *Automated review by Claude PR Reviewer*

cal added 1 commit 2026-03-30 23:23:19 +00:00

Merge branch 'main' into issue/71-refactor-manual-db-close-calls-to-middleware-based

Build Docker Image / build (pull_request) Failing after 4m14s

Details

c415fb8be7

cal added 1 commit 2026-03-30 23:32:16 +00:00

ci: retrigger build after transient Docker Hub push failure

Build Docker Image / build (pull_request) Successful in 6m34s

Details

172a7a662e