fix(backend, tests): too many db connections in integration tests #7983
Conversation
WalkthroughThis change introduces a new class-scoped fixture Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
github-actions
bot
added
the
group/backend
fatih-acar
force-pushed
the
fac-fix-db-fixture-duplicate
branch
from
0206d1d to
2c002de
Compare
Since the neo4j driver upgrade, we must ensure the neo4j driver is properly closed and that a closed driver is not re-used. We disabled singletons during DI to get that behavior. A side effect is that during some integration tests that leverage local workers, some tasks/flows depend on the injected database instance, which would trigger new driver instances with new db connections. Those instances would not get cleaned since it's not using a singleton nor used within a context manager that would close the underlying db driver. Fix this by introducing a new class level fixture for the db instance and use it within the injected dependency. Signed-off-by: Fatih Acar <fatih@opsmill.com>
fatih-acar
force-pushed
the
fac-fix-db-fixture-duplicate
branch
from
2c002de to
15f4890
Compare
CodSpeed Performance ReportMerging #7983 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/tests/helpers/test_app.py (1)
142-143: Consider adding a clarifying comment.The
_dbfunction accepts asingletonparameter to match thebuild_databasesignature but intentionally ignores it to always return the sharedclass_dbinstance. A brief comment would help future maintainers understand this design choice.🔎 Suggested comment
async def _db(singleton: bool = True) -> InfrahubDatabase: + # Always return class_db to prevent duplicate connections during tests return class_dbbackend/tests/integration/git/test_git_repository.py (1)
77-78: Consider adding a clarifying comment.Same as in
backend/tests/helpers/test_app.py, the_dbfunction ignores thesingletonparameter. A comment would clarify this intentional design choice.🔎 Suggested comment
async def _db(singleton: bool = True) -> InfrahubDatabase: + # Always return class_db to prevent duplicate connections during tests return class_db
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/helpers/test_app.pybackend/tests/integration/git/test_git_repository.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Use Async whenever possible in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and code validation in PythonUse ruff and mypy to validate and lint Python files
Files:
backend/tests/integration/git/test_git_repository.pybackend/tests/helpers/test_app.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasksTest files should mirror source structure:
infrahub/core/node.pyshould have corresponding test attests/unit/core/test_node.py
Files:
backend/tests/integration/git/test_git_repository.pybackend/tests/helpers/test_app.py
backend/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.py: Follow Python coding standards defined indev/guidelines/backend/python.md
Use type hints for Python in backend code
backend/**/*.py: Use async/await for all I/O operations in Python code
Type hint all function parameters and return values in Python code
Use Pydantic models for data structures in Python code
Files:
backend/tests/integration/git/test_git_repository.pybackend/tests/helpers/test_app.py
🧬 Code graph analysis (1)
backend/tests/integration/git/test_git_repository.py (3)
backend/tests/conftest.py (2)
dependency_provider(121-122)_db(137-138)backend/infrahub/server.py (1)
lifespan(108-111)backend/infrahub/workers/dependencies.py (1)
build_database(63-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/tests/integration/git/test_git_repository.py (1)
70-82: Good use of AsyncGenerator type annotation.The return type
AsyncGenerator[InfrahubTestClient, None]correctly reflects the fixture's behavior with theyieldstatement. The pattern of usingdependency_provider.scopeandlifespancontext manager is consistent with the changes inbackend/tests/helpers/test_app.py.
| @pytest.fixture(scope="class") | ||
| async def class_db(self) -> InfrahubDatabase: | ||
| return await build_database(singleton=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual file and see the current state
head -n 130 backend/tests/helpers/test_app.py | tail -n 20Repository: opsmill/infrahub
Length of output: 793
🏁 Script executed:
# Find the InfrahubDatabase class definition
find backend -name "*.py" -type f | xargs grep -l "class InfrahubDatabase" | head -5Repository: opsmill/infrahub
Length of output: 196
🏁 Script executed:
# Search for InfrahubDatabase class and its methods
rg -nP --type=py "class InfrahubDatabase" -A 10 backend/Repository: opsmill/infrahub
Length of output: 4003
🏁 Script executed:
# Let's look at the full InfrahubDatabase class definition to find cleanup methods
rg -nP --type=py "class InfrahubDatabase" -A 150 backend/infrahub/database/__init__.py | head -200Repository: opsmill/infrahub
Length of output: 8777
🏁 Script executed:
# Also search for close, cleanup, or async context manager methods
rg -nP --type=py "(def close|def cleanup|async def __aenter__|async def __aexit__|def __del__)" backend/infrahub/database/__init__.pyRepository: opsmill/infrahub
Length of output: 171
🏁 Script executed:
# Get the details of the async context manager and close method
sed -n '269,330p' backend/infrahub/database/__init__.pyRepository: opsmill/infrahub
Length of output: 2204
🏁 Script executed:
# Also check how build_database is implemented
rg -nP --type=py "def build_database" -A 20 backend/Repository: opsmill/infrahub
Length of output: 1649
🏁 Script executed:
# Check how other fixtures handle database cleanup in test files
rg -nP --type=py "async def.*db.*\)" backend/tests/ -A 3 | head -50Repository: opsmill/infrahub
Length of output: 3165
🏁 Script executed:
# Look for fixture patterns with yield in the test files
rg -nP --type=py "@pytest.fixture.*\nyield" backend/tests/ -A 2 | head -40Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Look for fixtures with yield patterns more broadly
rg -nP --type=py "yield" backend/tests/helpers/test_app.py -B 5 -A 2Repository: opsmill/infrahub
Length of output: 2158
🏁 Script executed:
# Check the conftest and other test fixtures to see if they use yield for cleanup
rg -nP --type=py "@pytest.fixture" backend/tests/conftest.py -A 5 | head -80Repository: opsmill/infrahub
Length of output: 2901
🏁 Script executed:
# Look specifically at other class-scoped fixtures to see cleanup patterns
rg -nP --type=py '@pytest.fixture\(scope="class"\)' backend/tests/ -A 5 | head -100Repository: opsmill/infrahub
Length of output: 8272
🏁 Script executed:
# Look at the full module-level db fixture in conftest.py to see cleanup pattern
sed -n '125,148p' backend/tests/conftest.pyRepository: opsmill/infrahub
Length of output: 856
🏁 Script executed:
# Check if class_db fixture is used elsewhere and how
rg -nP --type=py "class_db" backend/tests/helpers/test_app.py -B 2 -A 5Repository: opsmill/infrahub
Length of output: 1133
🏁 Script executed:
# Check if pytest fixtures use AsyncGenerator type annotation in the codebase
rg -nP --type=py "AsyncGenerator\[" backend/tests/ | head -20Repository: opsmill/infrahub
Length of output: 1529
🏁 Script executed:
# Let's look at more examples of async fixtures with yield to confirm the pattern
rg -nP --type=py "@pytest.fixture.*\nasync def.*yield" backend/tests/helpers/test_app.py -B 1 -A 10 | head -60Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Verify that the return type annotation is indeed present in the actual code
sed -n '123,125p' backend/tests/helpers/test_app.pyRepository: opsmill/infrahub
Length of output: 197
Implement yield-based cleanup for the database fixture.
The class_db fixture should use yield with cleanup logic to properly close the database connection when the test class completes, matching the pattern in the module-level db fixture. Update the return type to AsyncGenerator[InfrahubDatabase, None] and add cleanup:
@pytest.fixture(scope="class")
-async def class_db(self) -> InfrahubDatabase:
- return await build_database(singleton=False)
+async def class_db(self) -> AsyncGenerator[InfrahubDatabase, None]:
+ db = await build_database(singleton=False)
+ yield db
+ await db.close()🤖 Prompt for AI Agents
In backend/tests/helpers/test_app.py around lines 123 to 125, the class_db
fixture currently returns the database directly and must be converted to a
yield-based async fixture that yields an InfrahubDatabase and then performs
cleanup; change the type hint to AsyncGenerator[InfrahubDatabase, None], create
the db with await build_database(singleton=False), yield it to tests, and after
the yield call the appropriate close/teardown method (await db.close() or
equivalent) to ensure the connection is closed when the test class completes.
| async def test_client( | ||
| self, | ||
| dependency_provider: Provider, | ||
| db: InfrahubDatabase, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test_app.py file around line 131
head -n 150 backend/tests/helpers/test_app.py | tail -n 30Repository: opsmill/infrahub
Length of output: 1398
🏁 Script executed:
# Find the test_client fixture definition
rg -nP --type=py -B5 -A20 'def test_client\(' backend/tests/helpers/test_app.pyRepository: opsmill/infrahub
Length of output: 1415
🏁 Script executed:
# Let's also check the overall structure of the fixture
cat -n backend/tests/helpers/test_app.pyRepository: opsmill/infrahub
Length of output: 14353
🏁 Script executed:
# Check if db parameter is used anywhere in test_client fixture body
sed -n '128,147p' backend/tests/helpers/test_app.py | grep -c '\bdb\b' | head -1Repository: opsmill/infrahub
Length of output: 61
🏁 Script executed:
# Count occurrences of 'db' in the fixture - should be only in parameter list
sed -n '128,147p' backend/tests/helpers/test_app.py | grep 'db'Repository: opsmill/infrahub
Length of output: 281
🏁 Script executed:
# Verify what initialize_registry depends on to understand dependency chain
rg -nP --type=py -A15 'async def initialize_registry' backend/tests/helpers/test_app.pyRepository: opsmill/infrahub
Length of output: 737
Remove the unused db parameter from the test_client fixture.
The db parameter is never referenced in the fixture implementation—it only uses class_db. Since initialize_registry (a dependency of test_client) already ensures db is initialized, the explicit db parameter is redundant and should be removed.
🤖 Prompt for AI Agents
In backend/tests/helpers/test_app.py around line 131, the test_client fixture
declares an unused parameter `db` (it only uses `class_db` and
initialize_registry already ensures the DB is initialized); remove `db` from the
fixture signature (and from any local parameter list) so the fixture only takes
the dependencies it actually uses, and run tests to confirm no other references
to `db` remain in that fixture.
| @pytest.fixture(scope="class") | ||
| async def class_db(self) -> InfrahubDatabase: | ||
| return await build_database(singleton=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the fixture in question
head -n 80 backend/tests/integration/git/test_git_repository.py | tail -n 20Repository: opsmill/infrahub
Length of output: 176
🏁 Script executed:
# Check the referenced fixture in test_app.py at lines 123-125
sed -n '120,130p' backend/tests/helpers/test_app.pyRepository: opsmill/infrahub
Length of output: 384
🏁 Script executed:
# Search for InfrahubDatabase class and check for close() method
rg "class InfrahubDatabase" -A 30Repository: opsmill/infrahub
Length of output: 10998
🏁 Script executed:
# Search for how db.close() is used elsewhere
rg "await db\.close\(\)" --type pyRepository: opsmill/infrahub
Length of output: 110
🏁 Script executed:
# Get the referenced fixture from test_app.py
sed -n '123,130p' backend/tests/helpers/test_app.pyRepository: opsmill/infrahub
Length of output: 313
🏁 Script executed:
# Search for AsyncGenerator usage in test fixtures
rg "AsyncGenerator" backend/tests --type py -B 2 -A 4Repository: opsmill/infrahub
Length of output: 12219
🏁 Script executed:
# Search for pytest fixtures that use yield for cleanup
rg "@pytest.fixture" backend/tests --type py -A 5 | grep -A 5 "yield"Repository: opsmill/infrahub
Length of output: 9310
Add proper cleanup to the database fixture with correct return type.
This fixture creates an InfrahubDatabase instance without closing it, causing connection leaks. The PR aims to fix these leaks after the neo4j driver upgrade. Change the return type to AsyncGenerator[InfrahubDatabase, None] and implement cleanup using yield and await db.close():
@pytest.fixture(scope="class")
-async def class_db(self) -> InfrahubDatabase:
- return await build_database(singleton=False)
+async def class_db(self) -> AsyncGenerator[InfrahubDatabase, None]:
+ db = await build_database(singleton=False)
+ yield db
+ await db.close()Note: The same issue exists in backend/tests/helpers/test_app.py lines 123-125.
🤖 Prompt for AI Agents
In backend/tests/integration/git/test_git_repository.py around lines 66 to 68
(and similarly in backend/tests/helpers/test_app.py around lines 123 to 125),
the pytest fixture returns an InfrahubDatabase without closing it, leaking
connections; change the fixture signature to return an
AsyncGenerator[InfrahubDatabase, None], allocate the database into a local
variable, use yield to return it, and after the yield call await db.close() to
ensure proper async cleanup (also update imports to include
typing.AsyncGenerator if not present).
| await default_branch.save(db=db) | ||
| return schema_branch | ||
|
|
||
| @pytest.fixture(scope="class") |
There was a problem hiding this comment.
could this fixture be defined next to the db fixture in tests/conftest.py
and maybe it should be called db_class or db_class_scope
I think we might want to have scope="class" versions of some other fixtures and they would be easier to read if they don't all start with class_
Since the neo4j driver upgrade, we must ensure the neo4j driver is properly closed and that a closed driver is not re-used. We disabled singletons during DI to get that behavior.
A side effect is that during some integration tests that leverage local workers, some tasks/flows depend on the injected database instance, which would trigger new driver instances with new db connections. Those instances would not get cleaned since it's not using a singleton nor used within a context manager that would close the underlying db driver.
Fix this by introducing a new class level fixture for the db instance and use it within the injected dependency.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.