fix(memory): harden kernel reliability#1891
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR updates agent memory contracts, storage, write coordination, embedding recovery, maintenance flows, reindex routing, renderer controls, documentation, and tests. ChangesAgent Memory Reindex & Write-Path Overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts (1)
39-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTrack orphan reconciliation in in-flight cleanup.
reconcileOrphanVectorsOnce()is fire-and-forget, socleanupAgent()can clearorphanVectorReconciled/orphanVectorReconcileRetryAtbefore that task settles.withAgentLock()still serializes it throughvectorStore.settleAgent(), but a late completion can repopulate stale per-agent state and suppress the next reconcile for the same agent/key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts` around lines 39 - 49, Track orphan reconciliation as an in-flight task in EmbeddingPipeline so cleanupAgent() does not clear orphanVectorReconciled/orphanVectorReconcileRetryAt before reconcileOrphanVectorsOnce() settles. Add a per-agent promise/settlement entry alongside the existing warmup/drain maps, register it when reconcileOrphanVectorsOnce() starts, and remove or ignore stale completion updates after cleanupAgent() has run. Make sure withAgentLock() and vectorStore.settleAgent() still serialize the work, but late completions do not repopulate stale agent state.
🧹 Nitpick comments (5)
test/main/presenter/memoryPresenter.test.ts (1)
2932-3013: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated inline
IMemoryVectorStorestubs — consider a shared factory.Several tests hand-roll the full
IMemoryVectorStoreshape inline instead of using the existingFakeVectorStore(already imported/used elsewhere in this file, e.g. around line 3195) with targeted overrides. This diff had to addlistMemoryIdsto 5+ separate inline stubs; a sharedmakeVectorStoreStub(overrides)helper (or reusingFakeVectorStorewith method overrides) would prevent this churn on the next interface change.Also applies to: 4118-4118, 6517-6517, 6655-6665
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/memoryPresenter.test.ts` around lines 2932 - 3013, The tests are duplicating full IMemoryVectorStore stubs inline, which makes every interface change require edits in many places. Refactor the affected cases in MemoryPresenter tests to use the existing FakeVectorStore or a shared makeVectorStoreStub(overrides) helper, and keep only the per-test behavior overrides such as upsert, query, deleteByMemoryIds, or isUsable. This will centralize the IMemoryVectorStore shape and avoid repeated additions like listMemoryIds across the file.test/main/presenter/memoryExtraction.test.ts (1)
537-801: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsolidate duplicate
FakeRepositoryimplementation.
makeFakeRepo()duplicates most ofFakeRepository(memoryFakes.ts) — insert, requeueForEmbedding, listEmbeddingStatusIds, runInTransaction, listWorkingCandidates, etc. — and is passed viarepository: repo as anyat call sites, so TypeScript never verifies it actually satisfiesMemoryRepositoryPort. This PR had to add ~7 new methods to both copies in lockstep; future port changes will require the same duplicate maintenance, and a missed method here would only fail at runtime instead of compile time.Consider replacing
makeFakeRepo()withnew FakeRepository()frommemoryFakes.tsand dropping theas anycast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/memoryExtraction.test.ts` around lines 537 - 801, `makeFakeRepo()` is a duplicated fake repository that mirrors `FakeRepository` and is being passed as `repository: repo as any`, which bypasses compile-time checking. Replace the local fake in memoryExtraction.test.ts with the shared `FakeRepository` from memoryFakes.ts so methods like `insert`, `requeueForEmbedding`, `listEmbeddingStatusIds`, `runInTransaction`, and `listWorkingCandidates` stay in sync. Remove the `as any` cast at the call sites so TypeScript verifies the object still satisfies `MemoryRepositoryPort`.src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts (1)
87-122: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winError-retry cooldown isn't engaged when
requeuedcomes back 0 despite foundretryIds.If
listEmbeddingStatusIdsreturns ids butrequeueForEmbeddingrequeues 0 rows (e.g., a race where the rows' status changed between the two calls),errorRetryAt/errorRetryAfterIdare never updated (Lines 116-120 only runif (requeued > 0)). This means the nextdrainPendingEmbeddingscall with no pending rows will immediately re-run the samelistEmbeddingStatusIds+requeueForEmbeddingpair without the intendedERROR_RETRY_COOLDOWN_MScooldown, effectively bypassing the cooldown on repeated calls until a requeue eventually succeeds.🔧 Proposed fix: always record retry attempt timestamp
- if (requeued > 0) { - this.errorRetryAt.set(agentId, now) + if (retryIds.length) { + this.errorRetryAt.set(agentId, now) + } + if (requeued > 0) { this.errorRetryAfterId.set(agentId, retryIds[retryIds.length - 1]) pending = this.ctx.deps.repository.listPendingEmbedding(limit, agentId) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts` around lines 87 - 122, The retry cooldown in the embedding pipeline is only recorded when `requeueForEmbedding` returns a positive count, so `drainPendingEmbeddings` can immediately retry the same `listEmbeddingStatusIds` path after a 0-row requeue. Update the `EmbeddingPipeline` logic to always set `errorRetryAt` for the agent whenever retry IDs are found and an attempt is made, and only set `errorRetryAfterId` from the latest attempted ID when appropriate, so the cooldown in `ERROR_RETRY_COOLDOWN_MS` is enforced even if `requeueForEmbedding` returns 0.src/renderer/settings/components/MemoryHealthSection.vue (1)
153-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: announce reindex state change to assistive tech.
The label/icon swap on click is visual-only; screen-reader users focused elsewhere won't be notified that reindexing started unless the button itself is re-announced. Consider
aria-live="polite"on a wrapping element oraria-busy="reindexing"on the button for better feedback.♿ Optional accessibility hint
<Button type="button" variant="outline" size="sm" class="h-7 px-2 text-xs" :disabled="reindexing" + :aria-busy="reindexing" `@click`="emit('reindex')" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/settings/components/MemoryHealthSection.vue` around lines 153 - 157, The reindexing status change is only reflected visually through the Icon swap in MemoryHealthSection.vue, so assistive tech may not announce it. Update the reindex trigger UI in MemoryHealthSection to expose the state change via an accessibility attribute such as aria-busy on the button or aria-live on a nearby wrapper, and ensure it toggles based on the reindexing state so screen readers are notified when reindexing starts and ends.test/renderer/components/MemoryHealthSection.test.ts (1)
226-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a disabled-state assertion to lock in the guard.
The tests cover the emit and the label swap but don't assert the button is actually
disabledwhilereindexingistrue. Since theButtonmock's<button>inherits thedisabledfallthrough attribute automatically (no declared props on the mock), this is a cheap addition that protects against a future regression re-enabling double-submits.✅ Suggested addition
const running = mountSection({ health: loadedHealth, reindexing: true }) expect(running.find('button').text()).toContain( 'settings.deepchatAgents.memoryManager.health.reindexing' ) + expect(running.find('button').attributes('disabled')).not.toBeUndefined()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/MemoryHealthSection.test.ts` around lines 226 - 239, The MemoryHealthSection test covers the reindex emit and label change but does not assert the button is disabled during reindexing. Update the test around the mountSection/loadedHealth/reindexing flow to also verify the rendered button has the disabled state when reindexing is true, using the existing button lookup in MemoryHealthSection.test.ts so the guard against double-submits stays covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/presenter/memoryPresenter/infra/vectorStoreManager.ts`:
- Around line 178-238: The fallback logic in deleteVectorsForMemoryIdsOpening
incorrectly couples embedding and dimension resolution, so a valid
options.embeddingModel can be overwritten when only embeddingDim is missing.
Update the resolution flow in
VectorStoreManager.deleteVectorsForMemoryIdsOpening to handle targetEmbedding
and targetDimensions independently: keep the parsed embeddingFingerprint result
when present, only fall back to resolveAgentConfig/getCurrentEmbeddingDimension
for the missing dimension, and preserve the originally requested store identity
when opening/deleting vectors.
In `@src/main/presenter/memoryPresenter/services/rowMutations.ts`:
- Around line 126-150: The provenance absorption flow in rowMutations should be
atomic with the fold: handleProvenanceHit() currently revives archived owners
before applyContentUpdate() enters the transaction, so a failure later in the
same flow can leave the owner revived without the fold. Refactor
handleProvenanceHit() to return only a decision (or defer the revive mutation),
and move the absorbed-status update into the existing
repository.runInTransaction block in applyContentUpdate() so the revive,
category update, and markSuperseded all commit or roll back together; apply the
same fix in the other affected call site as well.
In `@src/main/presenter/memoryPresenter/services/writeCoordinator.ts`:
- Around line 410-417: The fallback in writeCoordinator’s challenge flow can act
on a stale target because it passes target.id directly into
reviveProvenanceOwner without re-validating liveness first. Add the same
archived/superseded check used in the successful challenge path before calling
reviveProvenanceOwner, and only proceed when the current target is still live.
Use the existing currentTarget handling in writeCoordinator as the reference
point so the duplicate-folding path mirrors the guarded mutation path.
In `@src/main/presenter/sqlitePresenter/tables/agentMemoryAudit.ts`:
- Around line 236-271: `hasForgetEvent` is using a fixed `LIMIT 200` before
checking whether a row references the target `memoryId`, so older forget/archive
events can be skipped. Update the query in `agentMemoryAudit.ts` so the
`memoryId` filter is applied in SQL (or remove the limit/window entirely) and
keep the existing `memory/restore` override behavior in `hasForgetEvent`. Ensure
the lookup still uses `metadataReferencesMemoryId`-equivalent matching semantics
for `input_refs_json` and `output_refs_json`.
---
Outside diff comments:
In `@src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts`:
- Around line 39-49: Track orphan reconciliation as an in-flight task in
EmbeddingPipeline so cleanupAgent() does not clear
orphanVectorReconciled/orphanVectorReconcileRetryAt before
reconcileOrphanVectorsOnce() settles. Add a per-agent promise/settlement entry
alongside the existing warmup/drain maps, register it when
reconcileOrphanVectorsOnce() starts, and remove or ignore stale completion
updates after cleanupAgent() has run. Make sure withAgentLock() and
vectorStore.settleAgent() still serialize the work, but late completions do not
repopulate stale agent state.
---
Nitpick comments:
In `@src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts`:
- Around line 87-122: The retry cooldown in the embedding pipeline is only
recorded when `requeueForEmbedding` returns a positive count, so
`drainPendingEmbeddings` can immediately retry the same `listEmbeddingStatusIds`
path after a 0-row requeue. Update the `EmbeddingPipeline` logic to always set
`errorRetryAt` for the agent whenever retry IDs are found and an attempt is
made, and only set `errorRetryAfterId` from the latest attempted ID when
appropriate, so the cooldown in `ERROR_RETRY_COOLDOWN_MS` is enforced even if
`requeueForEmbedding` returns 0.
In `@src/renderer/settings/components/MemoryHealthSection.vue`:
- Around line 153-157: The reindexing status change is only reflected visually
through the Icon swap in MemoryHealthSection.vue, so assistive tech may not
announce it. Update the reindex trigger UI in MemoryHealthSection to expose the
state change via an accessibility attribute such as aria-busy on the button or
aria-live on a nearby wrapper, and ensure it toggles based on the reindexing
state so screen readers are notified when reindexing starts and ends.
In `@test/main/presenter/memoryExtraction.test.ts`:
- Around line 537-801: `makeFakeRepo()` is a duplicated fake repository that
mirrors `FakeRepository` and is being passed as `repository: repo as any`, which
bypasses compile-time checking. Replace the local fake in
memoryExtraction.test.ts with the shared `FakeRepository` from memoryFakes.ts so
methods like `insert`, `requeueForEmbedding`, `listEmbeddingStatusIds`,
`runInTransaction`, and `listWorkingCandidates` stay in sync. Remove the `as
any` cast at the call sites so TypeScript verifies the object still satisfies
`MemoryRepositoryPort`.
In `@test/main/presenter/memoryPresenter.test.ts`:
- Around line 2932-3013: The tests are duplicating full IMemoryVectorStore stubs
inline, which makes every interface change require edits in many places.
Refactor the affected cases in MemoryPresenter tests to use the existing
FakeVectorStore or a shared makeVectorStoreStub(overrides) helper, and keep only
the per-test behavior overrides such as upsert, query, deleteByMemoryIds, or
isUsable. This will centralize the IMemoryVectorStore shape and avoid repeated
additions like listMemoryIds across the file.
In `@test/renderer/components/MemoryHealthSection.test.ts`:
- Around line 226-239: The MemoryHealthSection test covers the reindex emit and
label change but does not assert the button is disabled during reindexing.
Update the test around the mountSection/loadedHealth/reindexing flow to also
verify the rendered button has the disabled state when reindexing is true, using
the existing button lookup in MemoryHealthSection.test.ts so the guard against
double-submits stays covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7edef38-e20e-447b-9c0b-cd30c255a4c9
📒 Files selected for processing (52)
docs/architecture/agent-memory-system/spec.mdsrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/infra/embeddingPipeline.tssrc/main/presenter/memoryPresenter/infra/memoryVectorStore.tssrc/main/presenter/memoryPresenter/infra/vectorStoreManager.tssrc/main/presenter/memoryPresenter/runtimeConstants.tssrc/main/presenter/memoryPresenter/services/conflictService.tssrc/main/presenter/memoryPresenter/services/maintenanceService.tssrc/main/presenter/memoryPresenter/services/managementService.tssrc/main/presenter/memoryPresenter/services/retrievalService.tssrc/main/presenter/memoryPresenter/services/rowMutations.tssrc/main/presenter/memoryPresenter/services/workingMemoryService.tssrc/main/presenter/memoryPresenter/services/writeCoordinator.tssrc/main/presenter/memoryPresenter/types.tssrc/main/presenter/sqlitePresenter/tables/agentMemory.tssrc/main/presenter/sqlitePresenter/tables/agentMemoryAudit.tssrc/main/routes/index.tssrc/renderer/api/MemoryClient.tssrc/renderer/settings/components/MemoryHealthSection.vuesrc/renderer/settings/components/MemoryManagerPanel.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/routes.tssrc/shared/contracts/routes/memory.routes.tstest/main/presenter/agentMemoryTable.test.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memoryExtraction.test.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memoryVectorStore.test.tstest/main/routes/dispatcher.test.tstest/main/routes/memoryDto.test.tstest/renderer/api/clients.test.tstest/renderer/components/MemoryHealthSection.test.tstest/renderer/components/MemoryManagerDialog.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/presenter/sqlitePresenter/tables/agentMemoryAudit.ts (1)
236-270: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded scan on every
hasForgetEventcall.Removing
LIMIT 200correctly fixes the previously-flagged missed-forget-event bug, but the query now has no bound at all — it fetches every completedmemory/forget/memory/archive/memory/restorerow for the agent on each call, then filters in JS bymemoryId. For agents with long audit histories, and given this is likely invoked per-memory during vector recall/reconciliation, this can become a real hot-path cost that grows unbounded with the agent's history.Consider adding a cheap SQL-level pre-filter on
input_refs_json/output_refs_json(e.g.LIKE '%"memoryId":"' || ? || '"%') to narrow the row set before the exactmetadataReferencesMemoryIdcheck, keeping correctness while avoiding pulling the entire history into memory each time.⚡ Proposed SQL pre-filter
hasForgetEvent(agentId: string, memoryId: string): boolean { const rows = this.db .prepare( `SELECT event_type, actor_type, input_refs_json, output_refs_json FROM agent_memory_audit WHERE agent_id = ? AND status = 'completed' AND ( (event_type = 'memory/forget' AND actor_type = 'runtime') OR (event_type = 'memory/archive' AND actor_type = 'user') OR event_type = 'memory/restore' ) + AND (input_refs_json LIKE ? OR output_refs_json LIKE ?) ORDER BY created_at DESC, id DESC` ) - .all(agentId) as Array<{ + .all(agentId, `%"memoryId":"${memoryId}"%`, `%"memoryId":"${memoryId}"%`) as Array<{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/sqlitePresenter/tables/agentMemoryAudit.ts` around lines 236 - 270, `hasForgetEvent` now scans the full `agent_memory_audit` history for every call, which can become expensive on long-lived agents. Update the query in `agentMemoryAudit.ts` to add a cheap SQL-side pre-filter on `input_refs_json` and `output_refs_json` before the existing `metadataReferencesMemoryId` check, so only likely-matching rows are loaded while preserving the current restore/forget/archive precedence logic in `hasForgetEvent`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/presenter/sqlitePresenter/tables/agentMemoryAudit.ts`:
- Around line 236-270: `hasForgetEvent` now scans the full `agent_memory_audit`
history for every call, which can become expensive on long-lived agents. Update
the query in `agentMemoryAudit.ts` to add a cheap SQL-side pre-filter on
`input_refs_json` and `output_refs_json` before the existing
`metadataReferencesMemoryId` check, so only likely-matching rows are loaded
while preserving the current restore/forget/archive precedence logic in
`hasForgetEvent`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1679d5bc-2bda-421e-bc20-7e339f516d3c
📒 Files selected for processing (9)
src/main/presenter/memoryPresenter/infra/embeddingPipeline.tssrc/main/presenter/memoryPresenter/infra/vectorStoreManager.tssrc/main/presenter/memoryPresenter/services/rowMutations.tssrc/main/presenter/memoryPresenter/services/writeCoordinator.tssrc/main/presenter/sqlitePresenter/tables/agentMemoryAudit.tssrc/renderer/settings/components/MemoryHealthSection.vuetest/main/presenter/agentMemoryTable.test.tstest/main/presenter/memoryPresenter.test.tstest/renderer/components/MemoryHealthSection.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- test/renderer/components/MemoryHealthSection.test.ts
- src/renderer/settings/components/MemoryHealthSection.vue
- test/main/presenter/agentMemoryTable.test.ts
- src/main/presenter/memoryPresenter/infra/vectorStoreManager.ts
- src/main/presenter/memoryPresenter/services/writeCoordinator.ts
- src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts
- src/main/presenter/memoryPresenter/services/rowMutations.ts
- test/main/presenter/memoryPresenter.test.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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation / i18n