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

[Dokujaa]

• Apply recommended fix to get_async_db(): use explicit session management instead of async context manager
• Remove redundant session.close() in get_db_session() after async with block
• Fix synchronous commit() calls on async sessions in test files
• Ensures proper cleanup of database connections and prevents pool exhaustion

Let me know if this is sufficient. I think swapping to await db.commit() is better, but let me know.

should fix Issue (#65)

[lingtonglu]

Hey @Dokujaa , thanks for the fix PR.

I think the following fixes are very solid:

• Remove redundant session.close() in get_db_session() after async with block
• Fix synchronous commit() calls on async sessions in test files

However, I don't fully get the fix to function get_async_db and how it would address the db pool cleaning up issue. Can you walk me through this and help me better understand it?

It seems that we did have a redundant code by utilizing the context manager and manually close the session in the final block as well. The part of "use explicit session management instead of async context manager" looks good to me. But I still don't know why the previous implementation caused an error there.

[Dokujaa]

Hey @lingtonglu. Thanks for reviewing my PR. I mainly went with this approach per @wilsonccccc's earlier comment on issue #65

What was mainly causing this issue was that the original block used async with (this should auto-close) and also calls session.close() in the finally block. This was closing the session twice. Pairing this with the sync commit calls caused some connections to remain checked out until the garbage collector. Now, background tasks should create their own short-lived sessions. This should make it so that connections are returned deterministically, which fixes the pool warnings.

If I missed something, or you don't believe this implementation is proper, feel free to let me know. I'll make the changes!

[lingtonglu]

Hey @Dokujaa , the fix to not closing the session twice is a valid one. But I still don't think that's the root cause of the warning message. And that "sync commit" calls are in the test script. All the production code is using the async call.

@wilsonccccc , do you have any suggestion on this?

[wilsonccccc]

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

[wilsonccccc]

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

[wilsonccccc]

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