fix: replace bare except in db connection_pool with Exception#264
fix: replace bare except in db connection_pool with Exception#264harshadkhetpal wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR adds PostgreSQL SSL configuration support to the connection pool by reading environment variables for SSL mode and root certificate, and enhances connection cleanup by executing additional reset commands for application context variables before returning connections to the pool. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/utils/db/connection_pool.py`:
- Around line 94-95: The except Exception: pass in connection_pool.py silently
swallows rollback/cleanup errors; replace it with logging of the exception
(e.g., use the module logger or logger.exception) including context (which
connection/transaction and that the rollback failed), so update the except block
that wraps the rollback/cleanup call to call logger.exception("Failed to
rollback connection %s", conn_id) or similar and then continue (do not swallow
without logging); reference the rollback/cleanup call and the surrounding
function (the except after the rollback() call in the connection pool
release/cleanup routine) when making the change.
- Around line 100-113: The cleanup block currently rolls back and attempts to
RESET session vars using connection.rollback(), connection.cursor() and
cur.execute(...), but if that reset fails the code still calls
pool.putconn(connection), which returns a tainted connection to the pool; change
the control flow so that when the RESET/cleanup (the try around
connection.rollback()/cur.execute()) raises an exception you do NOT return the
connection to the pool but instead discard it (use pool.putconn(connection,
close=True) or connection.close()) and log the error via logger.warning; retain
the existing normal-path behavior of calling pool.putconn(connection) only when
cleanup succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 93b13558-6444-4e1e-ac6c-5a4c05f41c77
📒 Files selected for processing (1)
server/utils/db/connection_pool.py
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Don’t silently swallow rollback failures.
At Line 94, except Exception: pass hides cleanup failures and makes incident debugging harder.
Proposed fix
- except Exception:
- pass
+ except Exception as rollback_error:
+ logger.warning("Rollback failed after connection error: %s", rollback_error)As per coding guidelines, "Flask error handlers and try/except blocks should include proper logging."
🧰 Tools
🪛 Ruff (0.15.10)
[error] 94-95: try-except-pass detected, consider logging the exception
(S110)
[warning] 94-94: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/db/connection_pool.py` around lines 94 - 95, The except
Exception: pass in connection_pool.py silently swallows rollback/cleanup errors;
replace it with logging of the exception (e.g., use the module logger or
logger.exception) including context (which connection/transaction and that the
rollback failed), so update the except block that wraps the rollback/cleanup
call to call logger.exception("Failed to rollback connection %s", conn_id) or
similar and then continue (do not swallow without logging); reference the
rollback/cleanup call and the surrounding function (the except after the
rollback() call in the connection pool release/cleanup routine) when making the
change.
| try: | ||
| connection.rollback() | ||
| with connection.cursor() as cur: | ||
| cur.execute( | ||
| "RESET myapp.current_user_id; RESET myapp.current_org_id;" | ||
| ) | ||
| connection.commit() | ||
| except Exception as e: | ||
| logger.warning("Failed to reset session vars on pool return: %s", e) | ||
| try: | ||
| pool.putconn(connection) | ||
| logger.debug("Returned connection to pool") | ||
| except Exception as e: | ||
| logger.error(f"Error returning connection to pool: {e}") | ||
| logger.error("Error returning connection to pool: %s", e) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify session-context usage and pool return patterns tied to this risk.
rg -nP 'SET\s+myapp\.current_(user_id|org_id)|RESET\s+myapp\.current_(user_id|org_id)' server -C2
rg -nP '\bputconn\s*\(' server/utils/db/connection_pool.py -C2Repository: Arvo-AI/aurora
Length of output: 46336
🏁 Script executed:
#!/bin/bash
# Read the full connection_pool.py to understand pool initialization and context
head -150 server/utils/db/connection_pool.py | cat -n
# Check the actual pool class used
rg -n 'class.*Pool|SimpleConnectionPool|ThreadedConnectionPool|import.*pool' server/utils/db/connection_pool.py -A2Repository: Arvo-AI/aurora
Length of output: 6846
🏁 Script executed:
#!/bin/bash
# Verify psycopg2 ThreadedConnectionPool.putconn() API signature
# Check if psycopg2 is available and what version
python3 <<'EOF'
try:
import psycopg2.pool
import inspect
# Get the signature of putconn method
pool_class = psycopg2.pool.ThreadedConnectionPool
putconn_method = getattr(pool_class, 'putconn', None)
if putconn_method:
sig = inspect.signature(putconn_method)
print(f"putconn signature: {sig}")
print(f"\nputconn full docstring:\n{putconn_method.__doc__}")
else:
print("putconn method not found")
# Also check SimpleConnectionPool
simple_pool = psycopg2.pool.SimpleConnectionPool
putconn_simple = getattr(simple_pool, 'putconn', None)
if putconn_simple:
sig_simple = inspect.signature(putconn_simple)
print(f"\n\nSimpleConnectionPool.putconn signature: {sig_simple}")
print(f"Docstring: {putconn_simple.__doc__}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: Arvo-AI/aurora
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check psycopg2 documentation via web or source references
# Search for any tests, comments, or error handling around putconn in the codebase
rg -n 'putconn' server -B5 -A5
# Also check if there are any issues or TODOs about connection cleanup
rg -n 'TODO|FIXME|XXX.*pool|XXX.*reset|XXX.*session' server/utils/db -iRepository: Arvo-AI/aurora
Length of output: 2630
🏁 Script executed:
#!/bin/bash
# Search for psycopg2 pool documentation or examples in requirements/setup files
find . -name 'requirements*.txt' -o -name 'setup.py' -o -name 'pyproject.toml' | head -10
xargs grep -i psycopg2 2>/dev/null || echo "No version constraint found"
# Check psycopg2 source or documentation online - let's look for any usage hints
# or check if there's a way to close/discard connections
rg -n 'close.*connection|connection.*close' server/utils/db -A2 -B2 | head -40Repository: Arvo-AI/aurora
Length of output: 3060
🌐 Web query:
psycopg2 ThreadedConnectionPool putconn close parameter API
💡 Result:
The putconn(conn, key=None, close=False) method in psycopg2.pool.ThreadedConnectionPool (and its parent AbstractConnectionPool) returns a connection to the pool. The close parameter is a boolean (default False). If close=True, the connection is discarded (closed) instead of being returned for reuse. The key parameter (optional, default None) associates the connection with a key for consistent retrieval via getconn(key). Usage example from official docs: pool = psycopg2.pool.ThreadedConnectionPool(1, 20, host='localhost', dbname='test') conn = pool.getconn # use connection... pool.putconn(conn) # returns to pool (close=False by default) # or pool.putconn(conn, close=True) # discards/closes it This API is unchanged in the latest psycopg2 2.9.11 docs (as of 2026). Note: psycopg2 pools are basic; for advanced features consider psycopg3 pools or third-party like psycopg2-pool.
Citations:
- 1: https://www.psycopg.org/docs/pool.html
- 2: https://dba.stackexchange.com/questions/196656/python-postgres-psycopg2-threadedconnectionpool-exhausted
- 3: https://stackoverflow.com/questions/48532301/python-postgres-psycopg2-threadedconnectionpool-exhausted
Discard connections when reset/cleanup fails before returning to pool.
Between lines 100 and 113, if the RESET of myapp.current_user_id / myapp.current_org_id fails, the connection is still returned to the pool via pool.putconn(connection). A subsequent request will reuse the connection with stale session variables, bypassing RLS enforcement for that connection.
The proper fix uses the close parameter supported by psycopg2's putconn() API:
Proposed fix
if connection:
+ should_discard = False
try:
connection.rollback()
with connection.cursor() as cur:
cur.execute(
"RESET myapp.current_user_id; RESET myapp.current_org_id;"
)
connection.commit()
except Exception as e:
+ should_discard = True
logger.warning("Failed to reset session vars on pool return: %s", e)
try:
- pool.putconn(connection)
+ pool.putconn(connection, close=should_discard)
except Exception as e:
logger.error("Error returning connection to pool: %s", e)🧰 Tools
🪛 Ruff (0.15.10)
[warning] 107-107: Do not catch blind exception: Exception
(BLE001)
[warning] 111-111: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/db/connection_pool.py` around lines 100 - 113, The cleanup block
currently rolls back and attempts to RESET session vars using
connection.rollback(), connection.cursor() and cur.execute(...), but if that
reset fails the code still calls pool.putconn(connection), which returns a
tainted connection to the pool; change the control flow so that when the
RESET/cleanup (the try around connection.rollback()/cur.execute()) raises an
exception you do NOT return the connection to the pool but instead discard it
(use pool.putconn(connection, close=True) or connection.close()) and log the
error via logger.warning; retain the existing normal-path behavior of calling
pool.putconn(connection) only when cleanup succeeds.
|
Please address the coderabbit comments so we can approve this PR |
3 participants
Summary
Replaces bare
except:clauses with explicitexcept Exception:(orexcept BaseException:where the block re-raises) in the database connection pool module.Change:
Why
except:catchesSystemExitandKeyboardInterrupt, which can prevent graceful shutdown and mask critical signals.except BaseException:is used so signals still propagate correctly.Testing
No behavior change for normal exception paths — only interrupt/signal propagation is corrected.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes