mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-28 14:07:55 +00:00
Jack HamiltonandGitHub
9e7366fc05
* fix: close KnowledgeStore connections in connectors_router endpoints KnowledgeStore opens a persistent SQLite connection in __init__ but was never closed at the three instantiation sites in connectors_router.py, leaking one file descriptor (plus its WAL handle) per call. The worst offender is _connector_summary(), which is invoked per-connector on every GET /connectors poll from the frontend — after ~100 polls the backend hits the default macOS per-process FD limit (256) and asyncio's accept() starts failing with OSError: [Errno 24] Too many open files. Add __enter__/__exit__ to KnowledgeStore (close() already existed) and wrap all three instantiation sites in `with` blocks so the connection is released deterministically when the scope exits. - connectors/store.py: add context manager protocol - server/connectors_router.py: use `with KnowledgeStore() as store:` in _connector_summary, _ingest, and _run_sync * test: cover KnowledgeStore context manager + connectors_router close leak - test_store.py: add two tests for the new __enter__/__exit__ protocol verifying the connection closes on normal exit and on exceptions. - test_connectors_router.py: add a regression test that monkeypatches KnowledgeStore.close to count invocations and asserts every store opened by GET /v1/connectors is paired with a close. Also fix a pre-existing bug in the test_connectors_router fixture: the router is created with prefix="/v1/connectors" internally (line 92 of connectors_router.py), and the fixture was wrapping it again with prefix="/v1", producing "/v1/v1/connectors". All 6 existing router tests were failing with 404 before this fix. 5 of them now pass; the remaining failure (test_trigger_sync) is an unrelated KeyError on a missing response field and is out of scope for this PR. * test: drop connectors_router regression test — skipped in CI anyway The regression test added in the previous commit relies on FastAPI being importable, but CI's dev-extra install does not include fastapi (it lives in the `server` optional group). All tests in test_connectors_router.py silently skip on CI with "fastapi not installed", so the regression test would never actually execute there. Revert test_connectors_router.py to the upstream main version. The fixture bug I fixed (double prefix) and the connection-leak regression test both deserve a separate PR scoped to test infrastructure — that PR should either add fastapi to dev deps, split server tests into their own CI job, or both. The KnowledgeStore context manager tests in test_store.py are kept because they have no fastapi dependency and will run in CI.