Skip to content

fix(milvus): make get_milvus_client directory-create loop-safe for milvus-lite - #146

Open
sahilkanger wants to merge 2 commits into
topoteretes:mainfrom
sahilkanger:fix/113-milvus-get-milvus-client-async-loop
Open

fix(milvus): make get_milvus_client directory-create loop-safe for milvus-lite#146
sahilkanger wants to merge 2 commits into
topoteretes:mainfrom
sahilkanger:fix/113-milvus-get-milvus-client-async-loop

Conversation

@sahilkanger

Copy link
Copy Markdown
Contributor

fix(milvus): make get_milvus_client directory-create loop-safe for milvus-lite

Refs #113

Summary

MilvusAdapter.get_milvus_client() crashes when used with a local (milvus-lite) file URI
from inside cognee's running event loop. It tried to ensure the parent directory exists by
driving cognee's async file-storage helper from a sync method: loop.run_until_complete(...)
raises RuntimeError on the already-running loop, and the except RuntimeError fallback calls
asyncio.run(...), which also raises inside a running loop. This PR replaces that block with a
plain, loop-safe os.makedirs(db_dir, exist_ok=True).

Root cause

get_milvus_client() is synchronous but is called from async adapter methods
(create_collection, etc.) that run inside cognee's event loop. Both branches of the old
directory-ensure logic touch the event loop:

loop = asyncio.get_event_loop()
loop.run_until_complete(file_storage.ensure_directory_exists())   # RuntimeError: loop already running
except RuntimeError:
    asyncio.run(file_storage.ensure_directory_exists())           # RuntimeError: asyncio.run() from running loop

So any local milvus-lite run fails before a collection can be created. Server URIs (http...)
skip the block and were unaffected.

Solution

A local milvus-lite URI is always a real filesystem path, so ensuring its parent directory needs
no async storage abstraction:

if not self.url.startswith("http"):
    db_dir = os.path.dirname(self.url)
    if db_dir:
        os.makedirs(db_dir, exist_ok=True)

os was already imported; the now-unused get_file_storage import is removed. Version bumped
0.1.2 → 0.1.3 per the repo's fix-bump convention.

Scope note (the other half of #113)

The issue also surfaces a KeyError: 'milvus' under ENABLE_BACKEND_ACCESS_CONTROL=True, because
Milvus ships no DatasetDatabaseHandler. I deliberately left that out of this PR: while
investigating I found the adapter doesn't currently scope its connection to database_name (the
MilvusClient is created without db_name) and prune() drops every collection on the
connection — so simply mirroring the Qdrant handler (dataset → vector_database_name, delete →
prune()) would make deleting one dataset wipe all of them. Doing multi-tenant Milvus correctly
needs a real per-dataset isolation model (server db_name create/drop vs. per-file milvus-lite)
that should be validated against a live instance. I've left a separate comment on #113 to align
with maintainers on the intended isolation model before building it, rather than ship a data-loss
risk here. Happy to follow up with that PR once the design is agreed.

Changes

0cd6d0a fix(milvus): make get_milvus_client directory-create loop-safe for milvus-lite

 .../milvus_adapter.py                 | 22 +++++++---------------
 packages/vector/milvus/pyproject.toml |  2 +-
 2 files changed, 8 insertions(+), 16 deletions(-)

Testing performed

  • python -m py_compile on the changed module — OK.
  • ruff check / ruff format --check on the package — clean.
  • Not run: the package's CI scripts (examples/example.py, tests/test_milvus.py) exercise this
    path with a local URI, but need LLM/embedding keys and (for the example) a reachable Milvus;
    those secrets aren't available to a fork PR locally. The change is a self-contained,
    loop-safe directory create.

Checklist

  • Change is focused and minimal
  • Follows the project's code style / conventions
  • Tests added or updated where appropriate (CI validates via example/test scripts; see Testing)
  • Documentation updated where appropriate (n/a — no public API change)
  • lint passes locally (ruff check on the package)
  • format passes locally (ruff format --check on the package)

The repo-wide `ruff check` / `ruff format --check` gates were red on every
PR because scrapegraph_task.py had un-sorted imports (I001) and trailing
whitespace (W291). Auto-fixed with ruff (no behavior change) so this PR's
CI can go green. Unrelated to the pinecone change in this PR.
@sahilkanger
sahilkanger force-pushed the fix/113-milvus-get-milvus-client-async-loop branch from 6e81d26 to 0c41ea7 Compare July 23, 2026 20:35
@sahilkanger

Copy link
Copy Markdown
Contributor Author

Two notes on CI: (1) Milvus Adapter Test fails with LLMAPIKeyNotSetError — it runs the example/test scripts which need the repo's LLM secret, unavailable to fork PRs, so it fails before reaching get_milvus_client. (2) the ruff Run Formatting Check red is the repo-wide ruff-latest drift (0.16.0 reformats ~25 pre-existing files) — I opened #147 to pin ruff and fix that for all PRs. This change itself is lint/format-clean. Glad to approve-and-run if that helps validate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant