fix: delete stale vectors when clearing messages#1152
Merged
Conversation
🦋 Changeset detectedLatest commit: 2acd40a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMemory.clearMessages now deletes associated vector embeddings (by collecting message vector IDs per conversation or per user with pagination) before clearing message storage when a vector adapter is configured; deletion errors are logged and do not block message clearing. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Memory as Memory
participant Storage as MessageStorageAdapter
participant Vector as VectorAdapter
participant Logger as Logger
Caller->>Memory: clearMessages(userId[, conversationId])
alt conversationId provided
Memory->>Storage: getMessages(conversationId)
else no conversationId
Memory->>Storage: getConversationsByUserId(page...)
Storage-->>Memory: conversation pages
Memory->>Storage: getMessages(conversation) (per page)
end
Storage-->>Memory: messages
Memory->>Vector: deleteBatch([msg_<conv>_<msgId>...]) (if vector configured)
Vector-->>Memory: result / error
Memory->>Logger: warn on vector deletion errors (non-blocking)
Memory->>Storage: clearMessages(userId[, conversationId])
Storage-->>Memory: ack
Memory-->>Caller: resolved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/memory/index.ts">
<violation number="1" location="packages/core/src/memory/index.ts:407">
P1: User-wide clears only collect vectors from the first page of conversations, so stale vectors remain once a user has more than the adapter's default page size.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/memory/index.ts (1)
179-184: Consider usingthis.logger?.warnfor consistent logging.The class has a
loggerproperty and usesconsole.warndirectly here, while other similar warning patterns in the codebase (e.g., line 304 indeleteConversation, line 371 ingetMessagesWithSemanticSearch) also useconsole.warn. However, for observability and configurability, consider usingthis.logger?.warnwith a fallback toconsole.warnif needed, similar to howloggeris used insaveMessageWithContext(lines 1056-1058).♻️ Optional: Use logger with console fallback
} catch (error) { - console.warn( + (this.logger?.warn ?? console.warn)( `Failed to delete vectors while clearing messages for user ${userId}${conversationId ? ` conversation ${conversationId}` : ""}:`, error, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/memory/index.ts` around lines 179 - 184, Replace the direct console.warn call in the catch block that logs deletion failures with the class logger: use this.logger?.warn(...) and provide a console fallback (e.g., console.warn(...)) so logging is consistent with other methods (see saveMessageWithContext, deleteConversation, getMessagesWithSemanticSearch); keep the same message format and include the error object when calling this.logger?.warn to preserve details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/memory/index.ts`:
- Around line 179-184: Replace the direct console.warn call in the catch block
that logs deletion failures with the class logger: use this.logger?.warn(...)
and provide a console fallback (e.g., console.warn(...)) so logging is
consistent with other methods (see saveMessageWithContext, deleteConversation,
getMessagesWithSemanticSearch); keep the same message format and include the
error object when calling this.logger?.warn to preserve details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5991c27a-1e5c-49cd-879e-3104e2d65146
📒 Files selected for processing (3)
.changeset/mean-pets-perform.mdpackages/core/src/memory/index.tspackages/core/src/memory/semantic-search.spec.ts
Deploying voltagent with
|
| Latest commit: |
2acd40a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1c59f707.voltagent.pages.dev |
| Branch Preview URL: | https://fix-memory-clear-vectors-114.voltagent.pages.dev |
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/memory/semantic-search.spec.ts">
<violation number="1" location="packages/core/src/memory/semantic-search.spec.ts:41">
P2: This helper overrides the wrong method, so the new test never exercises paginated user-wide cleanup.</violation>
</file>
<file name="packages/core/src/memory/index.ts">
<violation number="1" location="packages/core/src/memory/index.ts:412">
P2: This paginated query needs a deterministic unique sort. As written, offset paging over `queryConversations()` can skip conversations with identical timestamps, leaving some vectors undeleted during user-wide `clearMessages()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let offset = 0; | ||
|
|
||
| while (true) { | ||
| const conversations = await this.storage.queryConversations({ |
Contributor
There was a problem hiding this comment.
P2: This paginated query needs a deterministic unique sort. As written, offset paging over queryConversations() can skip conversations with identical timestamps, leaving some vectors undeleted during user-wide clearMessages().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/memory/index.ts, line 412:
<comment>This paginated query needs a deterministic unique sort. As written, offset paging over `queryConversations()` can skip conversations with identical timestamps, leaving some vectors undeleted during user-wide `clearMessages()`.</comment>
<file context>
@@ -404,12 +406,27 @@ export class Memory {
+ let offset = 0;
+
+ while (true) {
+ const conversations = await this.storage.queryConversations({
+ userId,
+ limit: VECTOR_CLEAR_CONVERSATION_PAGE_SIZE,
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/memory/semantic-search.spec.ts (1)
526-572: Make pagination assertion explicit to avoid false positives.After forcing pagination in the adapter, add assertions on
queryConversationscall sequence (e.g., offsets0,2) so this test proves multi-page traversal instead of only final state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/memory/semantic-search.spec.ts` around lines 526 - 572, Add explicit assertions that pagination occurred by spying/mocking the PagedConversationStorageAdapter.queryConversations calls and asserting it was invoked for multiple offsets (e.g., 0 then 2) when clearMessages runs; locate the test that creates the PagedConversationStorageAdapter instance (symbol PagedConversationStorageAdapter) and, before calling pagedMemory.clearMessages(userId), attach a spy to pagedStorage.queryConversations and after clearMessages assert the call sequence includes queries with offset 0 and offset 2 (or expected page sizes) to prove multi-page traversal rather than only checking final cleared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/memory/semantic-search.spec.ts`:
- Around line 36-52: The test helper currently overrides
PagedConversationStorageAdapter.getConversationsByUserId but
Memory.clearMessages uses storage.queryConversations, so the pagination path
isn't exercised; update the helper to override queryConversations (not
getConversationsByUserId) in class PagedConversationStorageAdapter and implement
it to always page results in small chunks (e.g., respect offset but enforce a
fixed page size by using limit = Math.min(options?.limit ?? Infinity,
this.defaultPageSize) or ignore incoming limit in favor of this.defaultPageSize)
so multi-page traversal is triggered; reference the class
PagedConversationStorageAdapter, method queryConversations, and ensure
compatibility with Memory.clearMessages which calls storage.queryConversations.
---
Nitpick comments:
In `@packages/core/src/memory/semantic-search.spec.ts`:
- Around line 526-572: Add explicit assertions that pagination occurred by
spying/mocking the PagedConversationStorageAdapter.queryConversations calls and
asserting it was invoked for multiple offsets (e.g., 0 then 2) when
clearMessages runs; locate the test that creates the
PagedConversationStorageAdapter instance (symbol
PagedConversationStorageAdapter) and, before calling
pagedMemory.clearMessages(userId), attach a spy to
pagedStorage.queryConversations and after clearMessages assert the call sequence
includes queries with offset 0 and offset 2 (or expected page sizes) to prove
multi-page traversal rather than only checking final cleared state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc70dc2e-28ab-4f33-8dc5-74a2e64f486b
📒 Files selected for processing (2)
packages/core/src/memory/index.tspackages/core/src/memory/semantic-search.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/memory/index.ts
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
When
Memory.clearMessages()is used with semantic memory enabled, the conversation messages are removed from storage but the vector entries remain in the vector store. That allows semantic search to keep returning cleared conversation content.What is the new behavior?
Memory.clearMessages()now deletes the related vector entries before clearing message storage. This keeps semantic search results in sync with message deletion for both conversation-scoped clears and user-scoped clears.fixes #1148
Notes for reviewers
packages/core/src/memory/semantic-search.spec.tspnpm --dir packages/core exec vitest run src/memory/semantic-search.spec.tsexamples/basebefore the fix and confirmed semantic search results are removed after the fixSummary by cubic
Memory.clearMessages()now deletes related vector entries before removing messages and handles storage pagination so user-wide cleanup covers all conversations. Fixes #1148.deleteBatchfor conversation clears and user-wide clears across storage pages in@voltagent/core.Written for commit 2acd40a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores