Skip to content

fix dashboard revenue isolation and precision - #212

Open
ChrisEarlAmar wants to merge 1 commit into
Base360-AI:mainfrom
ChrisEarlAmar:main
Open

ChrisEarlAmar wants to merge 1 commit into
Base360-AI:mainfrom
ChrisEarlAmar:main

Conversation

@ChrisEarlAmar

Copy link
Copy Markdown

No description provided.

Comment on lines +57 to +60
engine = self.engine
self.engine = None
self.session_factory = None
if engine is not None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The close() method in DatabasePool nullifies self.session_factory without a lock, creating a race condition where in-flight requests can crash during application shutdown.
Severity: HIGH

Suggested Fix

The close() method should acquire the _initialization_lock before nullifying self.engine and self.session_factory. This will ensure that no new sessions can be requested while the pool is being torn down, preventing the race condition.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: backend/app/core/database_pool.py#L57-L60

Potential issue: A race condition exists in the `DatabasePool.close()` method. During
application shutdown, `self.session_factory` is set to `None` without acquiring the
`_initialization_lock`. If an in-flight request simultaneously calls
`db_pool.get_session()`, it will find `self.session_factory` is `None` and raise a
`RuntimeError: "Database pool has not been initialized"`. This occurs in deployments
using default uvicorn settings without an explicit `timeout_graceful_shutdown`, causing
requests in progress during a restart to fail instead of completing gracefully.

Did we get this right? 👍 / 👎 to inform future reviews.

if database_url.startswith("postgresql://"):
return database_url.replace("postgresql://", "postgresql+asyncpg://", 1)
if database_url.startswith("postgres://"):
return database_url.replace("postgres://", "postgresql+asyncpg://", 1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The _async_database_url function silently passes through unrecognized URL schemes, causing create_async_engine to fail if a synchronous driver is configured via DATABASE_URL.
Severity: MEDIUM

Suggested Fix

In _async_database_url, validate the URL scheme. If the scheme is not one of the recognized async-compatible schemes, raise a ValueError with an informative message to prevent the application from starting with an invalid configuration.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: backend/app/core/database_pool.py#L17

Potential issue: The `_async_database_url` helper function does not validate the
database URL scheme. If an operator misconfigures the `DATABASE_URL` environment
variable with a synchronous driver (e.g., `postgresql+psycopg2://`), the function passes
it through silently. This causes `create_async_engine()` to raise an
`InvalidRequestError`. While this error is caught during application startup, it
prevents the database pool from initializing. Subsequent API requests that require a
database connection will then fail with an unhandled exception when they try to
initialize the pool again with the invalid URL.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant