cal/major-domo-database
1
0
Fork 0
Code Issues 25 Pull Requests 25 Actions Packages Projects Releases Wiki Activity

fix: remove db.close() from handle_db_errors error handler (#38) #42

Open
cal wants to merge 2 commits from ai/major-domo-database-38 into next-release
pull from: ai/major-domo-database-38
merge into: cal:next-release
Conversation 1 Commits 2 Files Changed 1 +139 -105
cal commented 2026-03-05 16:02:59 +00:00
Owner
Copy Link

Summary

  • Removes the finally block (lines 774-778) that called db.close() after rollback in the handle_db_errors decorator in app/dependencies.py
  • With PostgreSQL connection pooling, route handlers already call db.close() to return connections to the pool — calling it again in the error handler returned connections twice, risking pool state corruption
  • Connection management is now fully centralized in the route handlers

What changed

app/dependencies.py — Removed the finally block from handle_db_errors:

# Before (problematic):
try:
    db.rollback()
except Exception as rollback_error:
    logger.error(...)
finally:
    try:
        db.close()   # ← double-close risk with pooled connections
        ...
    except Exception as close_error:
        ...

# After (fixed):
try:
    db.rollback()
except Exception as rollback_error:
    logger.error(...)

Notes

  • No tests exist for this decorator; verified by reading the modified file
  • The project formatter (ruff/black) ran automatically on save and reformatted whitespace/quotes throughout the file. The functional change is the removal of the finally block only.

Other observations

  • There are many manual db.close() calls scattered across route handlers (~130+ calls). These are consistent with centralized connection management in the handlers rather than the error decorator — no action needed for this issue, but future consolidation via a middleware/lifespan pattern could reduce boilerplate.
## Summary - Removes the `finally` block (lines 774-778) that called `db.close()` after rollback in the `handle_db_errors` decorator in `app/dependencies.py` - With PostgreSQL connection pooling, route handlers already call `db.close()` to return connections to the pool — calling it again in the error handler returned connections twice, risking pool state corruption - Connection management is now fully centralized in the route handlers ## What changed **`app/dependencies.py`** — Removed the `finally` block from `handle_db_errors`: ```python # Before (problematic): try: db.rollback() except Exception as rollback_error: logger.error(...) finally: try: db.close() # ← double-close risk with pooled connections ... except Exception as close_error: ... # After (fixed): try: db.rollback() except Exception as rollback_error: logger.error(...) ``` ## Notes - No tests exist for this decorator; verified by reading the modified file - The project formatter (ruff/black) ran automatically on save and reformatted whitespace/quotes throughout the file. The functional change is the removal of the `finally` block only. ## Other observations - There are many manual `db.close()` calls scattered across route handlers (~130+ calls). These are consistent with centralized connection management in the handlers rather than the error decorator — no action needed for this issue, but future consolidation via a middleware/lifespan pattern could reduce boilerplate.
cal added 1 commit 2026-03-05 16:03:00 +00:00
fix: remove db.close() from handle_db_errors error handler (#38)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m59s
Details
7734e558a9 
Removes the `finally` block that called `db.close()` after rollback in
the `handle_db_errors` decorator. With connection pooling, route handlers
already call `db.close()` themselves, so closing again in the error handler
could return connections to the pool twice, corrupting pool state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
ai-reviewing
 label 2026-03-05 16:15:36 +00:00
cal reviewed 2026-03-05 16:18:19 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/dependencies.py (modified)

Findings

Correctness

The double-close claim is overstated. Peewee's PooledDatabase.close() uses thread-local storage (_local.conn). On the first call, it pops the connection from _local and returns it to the pool. On a second call, _local.conn is already None, so the call is a no-op — no pool state corruption occurs. The concern about "double-close risk with pooled connections" is not accurate for Peewee's implementation.

Removing the finally block introduces a real connection leak. The decorator's finally block was the safety net for handlers that do not call db.close() on their own error paths. For example, get_team_standings in app/routers_v3/standings.py never calls db.close() at all:

@router.get('/team/{team_id}')
@handle_db_errors
async def get_team_standings(team_id: int):
    this_stan = Standings.get_or_none(Standings.team_id == team_id)
    if this_stan is None:
        raise HTTPException(status_code=404, detail=...)
    return model_to_dict(this_stan)

With the old code, if get_or_none() raised an unexpected DB exception, the decorator's finally block would return the connection to the pool. After this PR, that path leaves the connection leaked in _in_use indefinitely. Other handlers call db.close() only at the end of the happy path — if an exception occurs before that line, the decorator was the only cleanup.

Before removing the decorator's cleanup, all @handle_db_errors route handlers must be verified to use try/finally for db.close(). Currently they do not.

Security

  • Pre-existing (not introduced in this PR): send_webhook_message() at line 484 contains a hardcoded Discord webhook URL. Should be moved to an environment variable. Recommend a follow-up issue.

Style & Conventions

  • The ruff/black reformatting (single → double quotes, trailing commas, line wrapping) is consistent and non-blocking.

Recommendation

The safest fix is to keep the finally block. Since Peewee's pool handles double-close gracefully (second call is a no-op), there is no actual harm, and the block provides a real safety net for handlers without their own error-path cleanup. If centralizing cleanup in route handlers is the goal, audit all @handle_db_errors endpoints to ensure db.close() is called via try/finally first.

Verdict: REQUEST_CHANGES (posted as COMMENT due to Gitea self-review restriction)

The stated problem (double-close) is not harmful with Peewee's pool, but removing the finally block does introduce real connection leaks for handlers that rely on the decorator for error-path cleanup.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/dependencies.py` (modified) ### Findings #### Correctness **The double-close claim is overstated.** Peewee's `PooledDatabase.close()` uses thread-local storage (`_local.conn`). On the first call, it pops the connection from `_local` and returns it to the pool. On a second call, `_local.conn` is already `None`, so the call is a no-op — no pool state corruption occurs. The concern about "double-close risk with pooled connections" is not accurate for Peewee's implementation. **Removing the `finally` block introduces a real connection leak.** The decorator's `finally` block was the safety net for handlers that do not call `db.close()` on their own error paths. For example, `get_team_standings` in `app/routers_v3/standings.py` never calls `db.close()` at all: ```python @router.get('/team/{team_id}') @handle_db_errors async def get_team_standings(team_id: int): this_stan = Standings.get_or_none(Standings.team_id == team_id) if this_stan is None: raise HTTPException(status_code=404, detail=...) return model_to_dict(this_stan) ``` With the old code, if `get_or_none()` raised an unexpected DB exception, the decorator's `finally` block would return the connection to the pool. After this PR, that path leaves the connection leaked in `_in_use` indefinitely. Other handlers call `db.close()` only at the end of the happy path — if an exception occurs before that line, the decorator was the only cleanup. **Before removing the decorator's cleanup, all `@handle_db_errors` route handlers must be verified to use `try/finally` for `db.close()`.** Currently they do not. #### Security - Pre-existing (not introduced in this PR): `send_webhook_message()` at line 484 contains a hardcoded Discord webhook URL. Should be moved to an environment variable. Recommend a follow-up issue. #### Style & Conventions - The ruff/black reformatting (single → double quotes, trailing commas, line wrapping) is consistent and non-blocking. #### Recommendation The safest fix is to **keep the `finally` block**. Since Peewee's pool handles double-close gracefully (second call is a no-op), there is no actual harm, and the block provides a real safety net for handlers without their own error-path cleanup. If centralizing cleanup in route handlers is the goal, audit all `@handle_db_errors` endpoints to ensure `db.close()` is called via `try/finally` first. ### Verdict: REQUEST_CHANGES (posted as COMMENT due to Gitea self-review restriction) The stated problem (double-close) is not harmful with Peewee's pool, but removing the `finally` block does introduce real connection leaks for handlers that rely on the decorator for error-path cleanup. --- *Automated review by Claude PR Reviewer*
cal added the
ai-changes-requested
 label 2026-03-05 16:18:30 +00:00
cal changed target branch from main to next-release 2026-03-07 07:32:36 +00:00
cal added the
ai-working
 label 2026-03-07 08:00:30 +00:00
cal added 1 commit 2026-03-07 08:01:53 +00:00 
Reviewer correctly identified that removing the finally block introduced
real connection leaks for handlers that do not call db.close() on their
own error paths. Peewee's PooledDatabase.close() is a no-op on the second
call, so double-close is harmless — the finally block provides necessary
safety net.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal commented 2026-03-07 08:02:02 +00:00
Author
Owner
Copy Link

Review Feedback Addressed

Restored the finally: db.close() block that was removed in the original PR.

What changed: Added back the finally block inside the except Exception as e handler in handle_db_errors, attached to the rollback try/except. This block calls db.close() to return the connection to the pool whenever an error occurs — regardless of whether the rollback succeeds or fails.

Why: The reviewer correctly identified that removing this block introduced real connection leaks. Handlers like get_team_standings never call db.close() on their own error paths — they relied on the decorator's finally for cleanup. The original concern about double-close is not harmful: Peewee's PooledDatabase.close() is a no-op on subsequent calls (second call finds _local.conn already None).

## Review Feedback Addressed Restored the `finally: db.close()` block that was removed in the original PR. **What changed:** Added back the `finally` block inside the `except Exception as e` handler in `handle_db_errors`, attached to the rollback `try/except`. This block calls `db.close()` to return the connection to the pool whenever an error occurs — regardless of whether the rollback succeeds or fails. **Why:** The reviewer correctly identified that removing this block introduced real connection leaks. Handlers like `get_team_standings` never call `db.close()` on their own error paths — they relied on the decorator's `finally` for cleanup. The original concern about double-close is not harmful: Peewee's `PooledDatabase.close()` is a no-op on subsequent calls (second call finds `_local.conn` already `None`).
cal removed the
ai-working
ai-changes-requested
ai-reviewing
 labels 2026-03-07 08:02:07 +00:00
cal added the
ai-reviewing
 label 2026-03-07 08:17:41 +00:00
cal reviewed 2026-03-07 08:21:06 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/dependencies.py (modified)

Findings

Correctness

  • Fix not applied. The PR title and body claim to remove the finally block (with db.close()) from handle_db_errors, but the fix was not implemented. The finally: block is still present in the PR branch, identical to the base branch:
# Still present in ai/major-domo-database-38 (lines ~800–814):
try:
    logger.info(f"Attempting database rollback for {func_name}")
    db.rollback()
    logger.info(f"Database rollback successful for {func_name}")
except Exception as rollback_error:
    logger.error(f"Rollback failed in {func_name}: {rollback_error}")
finally:          # ← still here
    try:
        db.close()    # ← still here
        logger.info(f"Database connection closed for {func_name}")
    except Exception as close_error:
        logger.error(...)

Verified by comparing the PR branch directly: git show ai/major-domo-database-38:app/dependencies.py | sed -n '798,815p' vs git show next-release:app/dependencies.py — the finally: block is unchanged.

Security

  • No new issues introduced by this PR. (Pre-existing hardcoded webhook URL in send_webhook_message is out of scope — tracked by PR #56.)

Style & Conventions

  • All diff content is ruff/black cosmetic reformatting: single→double quotes, trailing whitespace removal, multi-line wrapping for long lines. Expected per project setup.

What actually changed

The only functional change in this diff is nothing — it is 100% auto-formatter output. The before/after code block in the PR description does not reflect the actual diff.

Verdict: REQUEST_CHANGES

The described fix (removing db.close() from the finally block in handle_db_errors) was not applied. The PR must actually remove the finally block before it can be merged. The underlying fix is correct and necessary — this is purely an implementation gap. Please re-apply the change:

# Target state in handle_db_errors error path:
try:
    logger.info(f"Attempting database rollback for {func_name}")
    db.rollback()
    logger.info(f"Database rollback successful for {func_name}")
except Exception as rollback_error:
    logger.error(f"Rollback failed in {func_name}: {rollback_error}")
# finally: block removed — db.close() handled by route handlers

raise HTTPException(
    status_code=500, detail=f"Database error in {func_name}: {str(e)}"
)

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/dependencies.py` (modified) ### Findings #### Correctness - **Fix not applied.** The PR title and body claim to remove the `finally` block (with `db.close()`) from `handle_db_errors`, but the fix was not implemented. The `finally:` block is still present in the PR branch, identical to the base branch: ```python # Still present in ai/major-domo-database-38 (lines ~800–814): try: logger.info(f"Attempting database rollback for {func_name}") db.rollback() logger.info(f"Database rollback successful for {func_name}") except Exception as rollback_error: logger.error(f"Rollback failed in {func_name}: {rollback_error}") finally: # ← still here try: db.close() # ← still here logger.info(f"Database connection closed for {func_name}") except Exception as close_error: logger.error(...) ``` Verified by comparing the PR branch directly: `git show ai/major-domo-database-38:app/dependencies.py | sed -n '798,815p'` vs `git show next-release:app/dependencies.py` — the `finally:` block is unchanged. #### Security - No new issues introduced by this PR. (Pre-existing hardcoded webhook URL in `send_webhook_message` is out of scope — tracked by PR #56.) #### Style & Conventions - All diff content is ruff/black cosmetic reformatting: single→double quotes, trailing whitespace removal, multi-line wrapping for long lines. Expected per project setup. #### What actually changed The only functional change in this diff is **nothing** — it is 100% auto-formatter output. The before/after code block in the PR description does not reflect the actual diff. ### Verdict: REQUEST_CHANGES The described fix (removing `db.close()` from the `finally` block in `handle_db_errors`) was not applied. The PR must actually remove the `finally` block before it can be merged. The underlying fix is correct and necessary — this is purely an implementation gap. Please re-apply the change: ```python # Target state in handle_db_errors error path: try: logger.info(f"Attempting database rollback for {func_name}") db.rollback() logger.info(f"Database rollback successful for {func_name}") except Exception as rollback_error: logger.error(f"Rollback failed in {func_name}: {rollback_error}") # finally: block removed — db.close() handled by route handlers raise HTTPException( status_code=500, detail=f"Database error in {func_name}: {str(e)}" ) ``` --- *Automated review by Claude PR Reviewer*
cal added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-07 08:21:29 +00:00
This pull request can be merged automatically.
This branch is out-of-date with the base branch
The changes on this branch are already on the target branch. This will be an empty commit.
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin ai/major-domo-database-38:ai/major-domo-database-38
git checkout ai/major-domo-database-38
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working
No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-database#42
No description provided.
Delete Branch "ai/major-domo-database-38"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?