Repository navigation
fix: one DB engine and RAG service per event loop, closed by the loop that opened them - #1267
Merged
Merged
Conversation
The app's DB engines lived in one slot per pool, shared by every event loop. Production runs one loop, but a test process runs several at once (the e2e uvicorn threads, each test's loop, each asyncio.run), and each cross-loop access took the slot over: it disposed the other loop's engine while that loop's fire-and-forget hooks still held connections, which then leaked. Engines bound to loops that ended (run_db's asyncio.run, the eval bench's servers) were never disposed at all. That left 31 unclosed asyncpg connections in the e2e smoke and kept ResourceWarning exempt. roboco/db/base.py now keeps a pool's engines per event loop (_PoolEngines, _PRIMARY_ENGINES / _BACKGROUND_ENGINES): get_engine / get_session_factory return the running loop's, an engine built with no loop running is claimed by the first loop to use it, and close_db disposes only the running loop's. Every loop's owner closes its engines before the loop ends: production's lifespan (unchanged), a new lifespan on the e2e app, E2EStack.run_db for its asyncio.run, and the root conftest for each test's loop (no longer suppressing close_db's errors). The cross-loop rebind and disposal, the e2e conftest's per-test reset and the tests' _DbHolder resets are gone; the DB tests cover the per-loop behavior instead. pytest now fails on ResourceWarning too. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…touched The conventions check flags 12 comments trailing code in test_background_engines.py, test_data_integrity.py, test_vault_v2.py and test_db_background_pool.py; each now sits on its own line above the code. Every file's AST is identical. The map's line citations follow. Co-Authored-By: Claude Opus 5.5 <[email protected]>
`python -m roboco.cli` (with or without `--db-only`), `python -m roboco.vault rebuild` and the decisions export each run their work in their own `asyncio.run`. The engine that work opens belongs to that loop, and once the loop ends nothing can close it: its connections died with the process, without a Terminate to Postgres. `run_then_close_db` awaits the work, then `close_db`, before the loop ends. Each entry point wraps its `asyncio.run` work in it. The full orchestrator run still closes through the API lifespan first; the wrapper's `close_db` then finds nothing left, or closes an engine something opened after the lifespan ended. New tests: the helper closes after the work and when it raises, and each entry point closes after its work. The entry-point tests fail on the old code. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tests The conventions check flags three trailing comments in the two test files the previous commit touched. Each moves onto its own line above the assertion it explains. Both files' ASTs are identical. Co-Authored-By: Claude Opus 5.5 <[email protected]>
`tests/unit/test_cli.py` shares its basename with `tests/unit/conventions/test_cli.py`. Neither directory is a package, so pytest imports both as `test_cli` and stops collection with an import file mismatch. Renamed to `test_cli_entrypoint.py`. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ned it The full suite's last leak: 9 sockets and 6 transports to Postgres, found by garbage collection in whichever later test it ran in. A journal route test built the real OptimalService on its own loop and nothing closed it; about 900 files later, a test's `close_optimal_service` closed it from another loop. Its vector-store pools couldn't close their sockets there, and `VectorStore.close` swallowed the `Event loop is closed` error on the grounds that "the connections died with the loop". They hadn't. - `roboco/services/optimal.py`: `_OptimalServiceHolder` keeps one service, and one init lock, per event loop, as `roboco/db/base.py` keeps engines. `close_optimal_service` closes only the running loop's. Production runs one loop, so it still has one service. - `VectorStore.close` lets that error propagate: a close can only reach another loop's pool through a missing close on the right one. - `close_shared_embedder` closes the embedder's sync HTTP client; it only dropped the reference. - Each loop's owner closes its RAG service with its engines: the root conftest for each test's loop, a lifespan for the e2e app. The e2e `run_db` closes through `run_then_close_db`. - A guard in the root conftest fails a test that leaves an engine or a RAG service on a loop that has ended, and names it. The per-loop registries keep such objects alive, so without it the leak is silent. - Its first full runs found three tests that opened the global engine on a loop nobody closed: `test_middleware.py`'s flow-verb requests wrote their latency sample through it (now skipped there; the telemetry tests cover that write), `test_i_will_plan_rich_required.py` used the real choreographer dependency (now always mocked), and the e2e `test_video_pipeline.py` ran the render step under a bare `asyncio.run`, leaving a live background-pool connection (now through `run_then_close_db`). New tests fail on the old code: each loop gets its own service, closing one loop's leaves another's alone, `VectorStore.close` raises on an ended loop, and the embedder's client is closed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… matches `JournalService.search_entries` kept a RAG match only if it sat in the agent's journal, which it looked up with `get_journal(agent_id)`. That looks a journal up by its own id, so it never found one, and journal search always returned an empty list. It now uses `get_journal_by_agent`. Nothing caught it: - the route test accepted a 500 the route can't return, and built the real RAG service to get its empty list (the leak the previous commit closes); - one service test patched `get_journal` to return the journal for any id; - the other documented the "near-miss in production" and asserted only that a list came back. The route test now stubs RAG, inserts its entry directly (the create route would start indexing it into the real RAG), and asserts the route returns that entry and searched this agent's journal index. The service tests assert the agent's own match is returned without patching, and that another agent's match is skipped. The route test and the own-match service test fail on the old code. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…xes touched The conventions check flags eleven trailing comments in `journal.py`, `test_video_pipeline.py`, `test_journal_service.py`, `test_i_will_plan_rich_required.py` and `test_middleware.py`. Each moves onto its own line above the code it explains. Every file's AST is identical; the map's line citations follow. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks for opening your first pull request on RoboCo! Quick checklist before review (most of these are enforced by CI, but worth a glance):
See CONTRIBUTING.md for the full workflow and the Code of Conduct for the community standards we follow. Welcome aboard — a maintainer will review shortly. |
The note said 9 sockets and 6 transports. The failure held 18 unraisable `ResourceWarning`s, and pytest printed 15 of them: those 9 sockets and 6 transports. The note now says so. Co-Authored-By: Claude Opus 5.5 <[email protected]>
rennf93
marked this pull request as ready for review
October 31, 2026 19:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to trail #32 (change 118): give each event loop its own database engines and RAG service, closed by the loop that opened them, and make pytest fail on
ResourceWarningtoo. This closes the leaks #1264 left open (it landed through train 4, #1265), and a journal search bug that one of them was hiding.Why
#1264 left
ResourceWarningexempt fromfilterwarnings = ["error"]. The e2e smoke still had 31 unclosed connections. The full suite had another leak: 18 unraisableResourceWarnings, which failed whichever later test the garbage collector happened to run in.Both came from process-wide async resources in a process that runs several event loops: each test's loop, each
asyncio.run, the e2e harness's uvicorn threads, the eval bench's servers. asyncpg connections belong to the loop that opened them.run_db'sasyncio.run, the eval bench's servers.OptimalServicewas one per process.tests/integration/test_journal_routes.py::test_search_my_entries_returns_listbuilt the real one on its own loop, and nothing closed it. About 900 files later,test_optimal_grounding.pyclosed it from another loop. Its vector-store pools couldn't close their sockets there, andVectorStore.closeswallowed theEvent loop is closederror on the grounds that "the connections died with the loop". They hadn't: they stayed open until garbage collection. These are the 18 warnings; the 15 that pytest printed were 9 sockets and 6 transports to Postgres.python -m roboco.cli(with or without--db-only),python -m roboco.vault rebuildand the decisions export run their work under their ownasyncio.run, and never closed the engine it opened. The connections died with the process, without a Terminate to Postgres.Rewriting the journal route test exposed a production bug.
JournalService.search_entriesfiltered RAG matches throughget_journal(agent_id), which looks a journal up by its own id, not its agent's. It never found one, so journal search always returned an empty list. The route test accepted a 500 the route can't return. One service test patchedget_journal, and the other documented the "near-miss in production" and asserted only that a list came back.What changed
roboco/db/base.py(commit 1):_PoolEngineskeeps one engine per event loop, for each of the primary and background pools. A loop never takes over another's engine.close_dbcloses only the running loop's engines. An engine built with no loop running is claimed by the first loop to use it._DbHolder,_BackgroundDbHolder,_rebind_holder_to_current_loopand_dispose_on_its_loopare gone.run_then_close_dbawaits the work, thenclose_db, before the loop ends.roboco/cli.py,roboco/vault.pyandroboco/services/decisions/export.pywrap theirasyncio.runwork in it.roboco/services/optimal.py(commit 6):OptimalServiceis one per event loop (_OptimalServiceHolder), and so is its init lock.close_optimal_servicecloses only the running loop's. Production runs one loop, so it still has one service.VectorStore.closeno longer swallowsEvent loop is closed. A close can only reach another loop's pool through a missing close on the right one, and that should fail loudly.close_shared_embeddercloses the embedder's sync HTTP client; before, it only dropped the reference.JournalService.search_entries(commit 7) usesget_journal_by_agent.run_then_close_db;_close_this_loops_db_and_rag;_close_db_and_rag_on_shutdown;run_db:run_then_close_db._nothing_left_open_on_an_ended_loop, fails any test that leaves an engine or RAG service on a loop that has ended, and names it. Without it, such a leak would be silent: the per-loop registries keep the objects alive, so their connections never reach the garbage collector and never warn.test_middleware.py: its flow-verb requests wrote their latency sample through the real engine. A module fixture skips that write;test_verb_latency_telemetry.pycovers the write itself.test_i_will_plan_rich_required.py: its first test used the real choreographer dependency, which opens a database session even for a request its validation rejects. Its app now always gets a mocked one.test_video_pipeline.pyruns the real render step under its ownasyncio.run, which opened the background pool and left a live connection on it. It now runs throughrun_then_close_db.pyproject.toml:filterwarnings = ["error"].ResourceWarningis no longer exempt.test_cli_entrypoint.py. Astest_cli.pyit shared a basename withtests/unit/conventions/test_cli.py, which stops collection, since neither directory is a package. My targeted runs never collected both; the full suite did._PoolEngines,get_engine,close_db,run_then_close_db,get_optimal_service,close_optimal_service,search_entriesand the two conftest fixtures, with dated notes. The stale "event-loop-bound singleton" note is rewritten.Verification
PYTHONASYNCIODEBUG=1makes each unclosed asyncpg connection report where it was opened.test_optimal_grounding.py, and a detector watching the RAG holder named the test that built the service.filterwarnings = ["error"]and no exemption:ResourceWarnings;VectorStore.closeraising on an ended loop;mypy roboco/ tests/,mypy roboco/runtime/orchestrator.py, vulture, xenon, bandit at CI's threshold, import-linter. The codebase map is 0 errors and 0 warnings, with the concat in sync and reflow clean. Conventions has no findings in the touched files except three god classes,JournalService,NotificationServiceandOptimalService. They predate this branch and belong to later waves of the god-class plan.🤖 Generated with Claude Code