Bug_042_MUST_FIX : Test Case VND-GET-004 — get_vendor_details leaks database session when vendor is not found

[steadhac]

Component: finbot/tools/data/vendor.py · get_vendor_details

Root cause:

db = next(get_db())
vendor_repo = VendorRepository(db, session_context)
vendor = vendor_repo.get_vendor(vendor_id)
if not vendor:
    raise ValueError("Vendor not found")
return vendor.to_dict()
The db session is never closed on the error path. No try/finally or context manager wraps the database access.

Steps to reproduce:

1. Call get_vendor_details with a non-existent vendor_id
2. ValueError is raised
3. db.close() is never called

Expected behavior: db.close() is called even when an exception is raised

Actual behavior: Database session is leaked on every failed vendor lookup

How to execute:

pytest tests/unit/tools/test_vendor.py::TestGetVendorDetailsDefects::test_vnd_get_004_db_session_not_closed_on_exception -v

Proposed fix:

db = next(get_db())
try:
    vendor_repo = VendorRepository(db, session_context)
    vendor = vendor_repo.get_vendor(vendor_id)
    if not vendor:
        raise ValueError("Vendor not found")
    return vendor.to_dict()
finally:
    db.close()

Impact:
Same class of issue as INV-GET-004. Under load, repeated lookups for invalid vendor IDs exhaust the connection pool, causing a denial-of-service across all database-backed operations.

Acceptance criteria:

• test_vnd_get_004_db_session_not_closed_on_exception passes — db.close() called after ValueError
• test_vnd_get_001 through test_vnd_get_003 continue to pass

[Jean-Regis-M]

Hi @steadhac , I’ve looked into issue #144 . The function get_vendor_details currently does not close the database session when the vendor is not found, leading to a session leak. I plan to add a try/finally block (or use a context manager) to ensure the session is always closed, even in the error path. I’ll also add a test that verifies the session is properly released. I’d like to work on this.

[deveshreddyp]

Hey @steadhac, @saikishu, and @Jean-Regis-M! I was reviewing this issue and PR #258 to see how the session leaks were being handled, and I noticed something interesting about the current main branch implementation.

The issue description mentions db = next(get_db()), but the get_vendor_details function actually wraps the database calls using the with db_session() as db: context manager.

Looking at finbot/core/data/database.py, db_session is decorated with @contextmanager and already natively utilizes a finally: db.close() block. Because of how Python handles context managers, that finally block is guaranteed to execute and clean up the session even when the raise ValueError("Vendor not found") early exit is triggered inside the with block.

Because the context manager guarantees the teardown, there is actually no database leak occurring here! Adding an explicit try/finally block inside the with block would be redundant.

Let me know if I'm missing some older context here, but I believe the architecture is already perfectly safe and we can likely close this out as a false positive/works-as-intended to keep the codebase clean! 🛡️