Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 1, 2025

Addresses resource leak from disposable vector stores (InMemoryVectorStore, QdrantVectorStore) being created but never disposed in GetVectorCollection.

Changes

  • Retrieved vector stores from DI container instead of creating new instances

    • VectorDBType.Memory: kernel.Services.GetRequiredService<VectorStore>()
    • VectorDBType.Qdrant: kernel.Services.GetRequiredService<VectorStore>()
    • VectorDBType.Redis: kernel.Services.GetRequiredService<VectorStore>()
    • Default case: kernel.Services.GetRequiredService<VectorStore>()
  • Removed Redis using block that was incorrectly disposing the store before the collection could be used

  • Removed unused IDatabase database variable

Rationale

Vector stores are already registered in DI via ConfigVectorStore (e.g., AddInMemoryVectorStore(), AddQdrantVectorStore()). Creating new instances bypassed DI lifecycle management, causing leaks. The DI container now handles disposal.

// Before: Creates undisposed instance
vectorStore = new InMemoryVectorStore();

// After: Retrieves DI-managed instance
vectorStore = kernel.Services.GetRequiredService<VectorStore>();

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI self-assigned this Nov 1, 2025
Copilot AI changed the title [WIP] Update Microsoft Memory Kernel PR based on feedback Fix vector store disposal by retrieving from DI container Nov 1, 2025
Copilot AI requested a review from JeffreySu November 1, 2025 05:40
@JeffreySu JeffreySu marked this pull request as ready for review November 1, 2025 07:51
@JeffreySu JeffreySu closed this Nov 1, 2025
@JeffreySu
Copy link
Contributor

@copilot retry this task again

@JeffreySu
Copy link
Contributor

@copilot retry this task again, because it was conflicted. base on the latest code from #160

1 similar comment
@JeffreySu
Copy link
Contributor

@copilot retry this task again, because it was conflicted. base on the latest code from #160

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.

2 participants