Skip to content

Fix unclosed asyncpg connections causing SQLAlchemy pool warnings (#65) #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Dokujaa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unclosed asyncpg connections causing SQLAlchemy pool warnings (#65) #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions app/core/database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,11 @@ def get_db():

# Async dependency
async def get_async_db():
async with AsyncSessionLocal() as session:
Copy link
Contributor

@wilsonccccc wilsonccccc Sep 13, 2025

Choose a reason for hiding this comment

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

I personally prefer "async with" is because:

  1. The session is automatically closed when exiting the context (whether normally or due to an exception)
  2. If an exception occurs, the session is still properly closed
  3. Guarantees that database connections are returned to the pool
  4. Prevents connection leaks even if code fails

compare with "session = " statement, it requires:

  1. Manual cleanup required to call await session.close()
  2. If an exception occurs before close(), the session might leak
  3. Requires try/finally blocks everywhere

try:
yield session
finally:
Copy link
Contributor

@wilsonccccc wilsonccccc Sep 13, 2025

Choose a reason for hiding this comment

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

The async with AsyncSessionLocal() as session: context manager already handles the session cleanup automatically. The finally block is redundant and could potentially cause issues. I think the proper fix should be just simply remove the finally block, and add a catch block to catch the try: yield session, which need to await session.rollback().

try:
    yield session
except Exception:
    try:
        await session.rollback()
    except Exception:
        pass
    raise

await session.close()
Copy link
Contributor

@wilsonccccc wilsonccccc Sep 13, 2025

Choose a reason for hiding this comment

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

I think most likely the issue is a race Condition: The get_async_db() dependency was trying to close a session while a commit operation was still in progress.

  1. A request used get_user_by_api_key() dependency
  2. The dependency called await db.commit() to update the API key timestamp
  3. The session was in a "committing" state
  4. The finally block in get_async_db() tried to call await session.close()
  5. SQLAlchemy threw IllegalStateChangeError because you can't close a session while it's committing

session = AsyncSessionLocal()
try:
yield session
finally:
await session.close()


@asynccontextmanager
Expand All @@ -89,8 +89,6 @@ async def get_db_session():
except Exception:
await session.rollback()
raise
finally:
await session.close()


def get_connection_info():
Expand Down
4 changes: 2 additions & 2 deletions tests/cache/test_async_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ async def test_async_cache_warming():
hashed_password="dummy_hash",
)
db.add(user)
db.commit()
await db.commit()

# Create a test API key
test_api_key = "test_key_123"
Expand All @@ -546,7 +546,7 @@ async def test_async_cache_warming():
encrypted_api_key=encrypted_key,
)
db.add(provider_key)
db.commit()
await db.commit()

# Warm the cache
await warm_cache_async(db)
Expand Down
4 changes: 2 additions & 2 deletions tests/mock_testing/add_mock_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ async def setup_mock_provider(username: str, force: bool = False):
# If force is set and provider exists, delete the existing one
if existing_provider and force:
db.delete(existing_provider)
db.commit()
await db.commit()
print(f"🗑️ Deleted existing mock provider for user '{username}'.")

# Create a mock API key - it doesn't need to be secure as it's not used
Expand Down Expand Up @@ -83,7 +83,7 @@ async def setup_mock_provider(username: str, force: bool = False):
)

db.add(provider_key)
db.commit()
await db.commit()

# Invalidate provider key cache for this user to force refresh
provider_service_cache.delete(f"provider_keys:{user.id}")
Expand Down
Loading