fix(memory): prevent config ABA fence bypass - #1972
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a unified per-agent execution token and configuration fingerprint model for memory operations. Runtime, maintenance, extraction, retrieval, injection, provider cancellation, agent configuration resolution, and regression tests now reject stale work across destructive and enabled/embedding ABA transitions. ChangesMemory execution fence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRuntime
participant MemoryRuntimeCoordinator
participant MemoryPresenter
participant RetrievalService
participant MemoryProviderGateway
participant VectorStoreManager
AgentRuntime->>MemoryRuntimeCoordinator: contribute session memory
MemoryRuntimeCoordinator->>MemoryPresenter: capture execution token
MemoryPresenter->>RetrievalService: build injection or retrieve
RetrievalService->>MemoryProviderGateway: run embedding request
RetrievalService->>VectorStoreManager: query memory vectors
VectorStoreManager-->>RetrievalService: return retrieval data
RetrievalService-->>MemoryPresenter: return fenced result
MemoryPresenter-->>MemoryRuntimeCoordinator: return injection or null
MemoryRuntimeCoordinator->>MemoryRuntimeCoordinator: validate token before accounting and anchors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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/agent/deepchat/memory/memoryRuntimeCoordinator.ts (1)
370-377: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFence the extraction write against session-agent reassignment.
extractAndStorecan persist underexecutionToken.agentIdwhile awaited. If the session switches Agents during that call, Line 377 only suppresses the cursor and anchor afterward—the stale memory write has already occurred.Pass a session-assignment fence into the persistence boundary, or invalidate/abort the write before its commit when the session Agent changes.
🤖 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/agent/deepchat/memory/memoryRuntimeCoordinator.ts` around lines 370 - 377, The extraction persistence path in memoryRuntimeCoordinator’s extractAndStore call must be fenced against session-agent reassignment during the await. Pass the current session assignment or executionToken fence into extractAndStore, and ensure the persistence boundary validates it immediately before committing so reassigned sessions cannot store stale memory; retain the existing post-call canContinueExecution check for cursor and anchor updates.
🤖 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/agent/deepchat/memory/memoryRuntimeCoordinator.ts`:
- Around line 316-318: Update the queue-admission flow around ensureSessionEpoch
and task so the session epoch is captured when the task is enqueued, before it
can wait behind earlier tasks. Pass that captured epoch to the later validation
and task invocation, and do not call ensureSessionEpoch at task execution time;
preserve expectedEpoch validation and discard tasks whose admission epoch is no
longer current.
In `@src/main/presenter/memoryPresenter/core/executionIdentity.ts`:
- Around line 21-25: Update memoryEmbeddingFingerprint to use an unambiguous
encoding of providerId and modelId, such as JSON.stringify on the ordered pair,
while preserving the existing null behavior and overload return types.
In `@src/main/presenter/memoryPresenter/core/providerCancellation.ts`:
- Around line 1-12: Separate capacity-exhaustion errors from stale-cancellation
errors in createMemoryProviderAbortError and isMemoryProviderAbortError.
Introduce a distinct code or reason for provider request capacity exhaustion,
update providerGateway.ts to use it, and ensure retrievalService.ts only
suppresses execution-invalidation cancellations rather than all matching errors.
In `@test/main/presenter/memory/maintenanceService.test.ts`:
- Around line 1233-1264: Add teardown to the test using the MemoryPresenter
instance created in “runs builtin maintenance even when managed config
enumeration fails”: call await presenter.dispose() in the finally block before
restoring fake timers, ensuring cleanup runs even when assertions or timer
advancement fail.
---
Outside diff comments:
In `@src/main/agent/deepchat/memory/memoryRuntimeCoordinator.ts`:
- Around line 370-377: The extraction persistence path in
memoryRuntimeCoordinator’s extractAndStore call must be fenced against
session-agent reassignment during the await. Pass the current session assignment
or executionToken fence into extractAndStore, and ensure the persistence
boundary validates it immediately before committing so reassigned sessions
cannot store stale memory; retain the existing post-call canContinueExecution
check for cursor and anchor updates.
🪄 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: 1922836d-b4b3-440c-ad37-8ace3e0621e6
📒 Files selected for processing (28)
docs/architecture/agent-memory-system/spec.mddocs/issues/memory-config-aba-fence/plan.mddocs/issues/memory-config-aba-fence/spec.mddocs/issues/memory-config-aba-fence/tasks.mdscripts/lib/memory-architecture-guard.mjssrc/main/agent/deepchat/deepChatAgentRepository.tssrc/main/agent/deepchat/memory/memoryRuntimeCoordinator.tssrc/main/presenter/agentRepository/index.tssrc/main/presenter/index.tssrc/main/presenter/memoryPresenter/context.tssrc/main/presenter/memoryPresenter/core/executionIdentity.tssrc/main/presenter/memoryPresenter/core/injectionPort.tssrc/main/presenter/memoryPresenter/core/providerCancellation.tssrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/infra/providerGateway.tssrc/main/presenter/memoryPresenter/infra/vectorStoreManager.tssrc/main/presenter/memoryPresenter/injection.tssrc/main/presenter/memoryPresenter/ports.tssrc/main/presenter/memoryPresenter/services/retrievalService.tssrc/main/presenter/memoryPresenter/types.tstest/main/agent/deepchat/memory/memoryRuntimeCoordinator.test.tstest/main/presenter/agentRepository.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/memory/maintenanceService.test.tstest/main/presenter/memory/retrievalService.test.tstest/main/presenter/memory/serviceTestSupport.tstest/main/presenter/memoryExtraction.test.tstest/main/presenter/memoryRuntimeContext.test.ts
…fence # Conflicts: # src/main/presenter/agentRuntimePresenter/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/agentRuntimePresenter/sessionSettingsCoordinator.ts`:
- Around line 130-145: Persist the reassigned agent ID alongside the other
session settings in the reassignment flow before or when calling
instance.setAgentId(nextAgentId). Update the relevant sessionStore persistence
method so newSessionsTable.agent_id is written with nextAgentId, ensuring
getSessionAgentId() restores the reassigned value after eviction or restart.
🪄 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: 47104c88-8f51-4c31-81ca-1b870c9d349c
📒 Files selected for processing (3)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/sessionSettingsCoordinator.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts
Comment on lines
+130
to
+145
| this.deps.sessionStore.updateSessionConfiguration( | ||
| sessionId, | ||
| nextProviderId, | ||
| nextModelId, | ||
| buildPersistedGenerationSettingsReplacement(generationSettings), | ||
| permissionMode | ||
| ) | ||
|
|
||
| const instance = this.deps.getInstance(sessionId) | ||
| instance.setRuntimeState({ | ||
| status: state?.status ?? 'idle', | ||
| providerId: nextProviderId, | ||
| modelId: nextModelId, | ||
| permissionMode | ||
| }) | ||
| instance.setAgentId(nextAgentId) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether agent_id persistence happens inside updateSessionConfiguration or elsewhere.
rg -nP -C10 'updateSessionConfiguration\s*\(' --type ts src/main/presenter/agentRuntimePresenter/sessionStore.ts
rg -nP '\bagent_id\b' --type ts -C3 src/main
rg -nP '\.setSessionAgentContext\s*\(' --type ts -C5 src/mainRepository: ThinkInAIXYZ/deepchat
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- sessionStore updateSessionConfiguration ---'
sed -n '182,240p' src/main/presenter/agentRuntimePresenter/sessionStore.ts
echo
echo '--- sessionSettingsCoordinator relevant region ---'
sed -n '110,170p' src/main/presenter/agentRuntimePresenter/sessionSettingsCoordinator.ts
echo
echo '--- index getSessionAgentId references ---'
rg -n -C4 'getSessionAgentId|agent_id|session agent id|setSessionAgentId|setSessionAgentContext' src/main/presenter/agentRuntimePresenter/index.ts src/main/presenter/agentRuntimePresenter -g '*.ts'Repository: ThinkInAIXYZ/deepchat
Length of output: 42962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for agent id persistence/update helpers ---'
rg -n -C3 'update.*agent|agent_id.*update|set.*agent.*context|newSessionsTable.*agent_id|updateSessionAgent|setSessionAgentId|agentId.*persist' src/main/presenter/agentRuntimePresenter src/main -g '*.ts'
echo
echo '--- newSessionsTable methods ---'
rg -n -C4 'class .*NewSessionsTable|interface .*NewSessionsTable|update.*agent_id|get.*agent_id|insert.*agent_id|agent_id' src/main -g '*newSessions*' -g '*.ts'
echo
echo '--- SessionAgentContextUpdate type ---'
rg -n -C4 'type SessionAgentContextUpdate|interface SessionAgentContextUpdate' src/main -g '*.ts'Repository: ThinkInAIXYZ/deepchat
Length of output: 50378
Persist the reassigned agent id
In src/main/presenter/agentRuntimePresenter/sessionSettingsCoordinator.ts:130-145, instance.setAgentId(nextAgentId) only updates memory. getSessionAgentId() rehydrates from newSessionsTable.agent_id, so this reassignment will revert after eviction/restart unless nextAgentId is written to persistence too.
🤖 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/agentRuntimePresenter/sessionSettingsCoordinator.ts`
around lines 130 - 145, Persist the reassigned agent ID alongside the other
session settings in the reassignment flow before or when calling
instance.setAgentId(nextAgentId). Update the relevant sessionStore persistence
method so newSessionsTable.agent_id is written with nextAgentId, ensuring
getSessionAgentId() restores the reassigned value after eviction or restart.
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
2 participants
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
Bug Fixes
Enhancements
Documentation
Tests