fix(memory): fix atomicity and recovery - #2327
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe PR updates vector coverage verification, synchronous memory-write bookkeeping, and persona state transitions. It adds specifications and regression tests for lease expiry, cancellation, partial writes, and transactional persona activation. ChangesVector coverage verification
Memory write cancellation
Persona state transactions
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Warmup
participant CoverageVerifier
participant SQLite
participant VectorStore
Warmup->>CoverageVerifier: start verification with operation fence
CoverageVerifier->>SQLite: enumerate memory IDs by page
CoverageVerifier->>VectorStore: enumerate embedded IDs by page
CoverageVerifier->>VectorStore: requeue missing vectors and delete orphans
CoverageVerifier-->>Warmup: readiness result
sequenceDiagram
participant WriteCoordinator
participant DecisionProvider
participant MemoryRepository
participant CancellationState
WriteCoordinator->>DecisionProvider: process extraction decision
DecisionProvider-->>WriteCoordinator: batch outcome
WriteCoordinator->>MemoryRepository: commit touched outcome
CancellationState->>WriteCoordinator: disable, clear, or dispose
WriteCoordinator-->>MemoryRepository: stop later writes
Merge Risk: ⚪ Minimal · up to The updated coverage, cancellation, and persona-transition paths have targeted regression coverage, with no concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 6 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
Review: fix(memory): fix atomicity and recovery
Verdict: Approve. Three real bugs, three small targeted fixes, each backed by a regression test. No over-engineering, no breaking changes, and the code follows patterns already used elsewhere in src/main/memory.
What this PR fixes
1. Persona approve/rollback could leave an agent with no active persona (personaService.ts)
Approve and rollback each write two rows: mark the current persona superseded, mark the target active. These ran as two separate writes. If the second one failed (or the process died between them), the old persona was already superseded and nothing was active — a state with no obvious recovery path for the user.
Fix: wrap both writes in the existing runInTransaction port. This is the same pattern mergeService, conflictService, and managementService already use, so nothing new is invented here. The SQLite integration test proves a mid-transaction failure rolls both writes back and the active persona survives.
2. Partially-committed extraction batches lost their bookkeeping on cancellation (writeCoordinator.ts)
Before: a candidate could restore an archived claim and commit it to SQLite, then the batch awaited the decision provider. Mutation epoch / working-projection-dirty flags were only recorded after the whole batch returned. If memory was disabled while the provider was pending, that bookkeeping was skipped entirely — and after re-enabling, injection kept using the stale working projection, silently missing the row that was actually committed.
Fix: a new recordBatchOutcome helper records markDomainMutationCommitted + markWorkingMemoryDirty at the moment each candidate commits, before any further provider await. Events, embedding triggers and consolidation still respect the operation fence, so a cancelled batch cannot fire late work. The !resolvedModel guard at the single-remember path avoids double bookkeeping. Direct remembers (no model) keep their old synchronous path.
3. Coverage verification was bounded by the wrong limit, causing a permanent rebuild loop (embeddingPipeline.ts)
Before: store warm-up verified vector coverage using the embedding drain's 200-batch guard (~102,400 IDs at 512 per page). A healthy store above that size could never pass verification: warm-up reported failure and triggered a full vector reindex — but rebuilding doesn't reduce the ID count, so every subsequent warm repeated the same "recovery". Net effect: large healthy stores were stuck in a destructive loop.
Fix: keep the same 512-ID keyset pages, but bound total work by the existing 30s vector store lease deadline instead of the batch count, checking cancellation/epoch/generation between pages and yielding so those checks can actually run. A listing that doesn't finish now just defers the warm — it neither certifies readiness nor triggers a reset. Repair work (requeue missing rows, delete orphan vectors) is batched at 512 IDs with yields. Also drops the duplicate ID arrays/sets the old code built (one shared Set now), which is a straight memory win at scale.
Verification
pnpm vitest run test/main/memory/writeCoordinator.test.ts test/main/memory/embeddingPipeline.test.ts test/main/memory/memoryUpdateNative.test.ts— 100/100 pass locally, native SQLite included.pnpm typecheck— clean.- Boundary tests at 102,399 / 102,400 / 102,401 IDs demonstrate the old cap is gone, and the 102,401 case (equal counts on both sides but one missing + one orphan) guards against count-only certification.
- The disable/clear/dispose matrix proves: committed rows survive cancellation, no late events / embedding / provider work after cancel, and the disable→re-enable regression fails on the base implementation per the spec.
Minor notes (non-blocking)
- When verification doesn't finish, the deferred error reuses the
vector-store-unavailablereason. Slightly misleading in diagnostics — "verification unfinished" would read better. Cosmetic only. - If verification ever exceeds the 30s lease deadline on a real 100k-row store, the lease marks the store suspect and it drains and reopens. The spec openly notes the tests use port fixtures, not a real-database latency benchmark; keyset listing of 100k IDs should be far under 30s in practice, but this is the thing to watch if slow warm loops at scale ever get reported.
- Test additions are proportionate: every new case maps to an acceptance criterion in the two specs, no coverage padding detected.
Detailed analysis (per file)
personaService.ts— approve (L205–L214) and rollback (L293–L302) now wrap their twosetPersonaStatecalls inrunInTransaction. There is no await between thegetActivePersonaread and the transaction, and the main process is single-threaded, so no TOCTOU window is introduced.runInTransactionis better-sqlite3's native transaction (handles nesting via savepoints).writeCoordinator.ts— bookkeeping moved from two completion sites (batch loop L420, single-remember L1386) intorecordBatchOutcome(L803), called at all three commit points: immediate settlement, first apply, retry apply. Candidates that settle asnoop(forgotten / concurrent-update) correctly skip bookkeeping. Outcomes recorded via rawoutcomesByIndex.setfor retry-cap overflow are all noops, so no path loses bookkeeping. On cancellation the extractionfinallyblock skipsfinalizeCommittedExtraction(fence-gated) while the ingestion cursor stays put — committed rows are picked up by the next drain, matching the stated contract.embeddingPipeline.ts—verifyVectorCoveragenow enumerates SQLite rows inside the store lease withisCurrent()(fence + read epoch + embedding identity + lease generation) checked between every page; the repository query isORDER BY id ASC+id > ?, so keyset pagination is sound. Sidecar enumeration subtracts from the sharedmissingIdsset; anything not subtracted is an orphan and gets deleted. Duplicate vectors in the store naturally land inextras(second occurrence fails thedelete) and are removed — a nice implicit dedupe. Partial requeue followed by anisCurrent()failure is safe: requeued rows leave the ready set, so re-running verification is idempotent. The removedauthoritativeListingTruncatedauto-reindex has no remaining references.docs/issues/*/spec.md— follows the existingdocs/issues/<slug>/spec.mdconvention; the two specs accurately describe the problems, designs and validation.
All three commits build on each other cleanly and the branch carries no unrelated changes.
2 participants
Changes
No schema or public API changes. Splitting
agentMemory.tsremains out of scope.Validation
Summary by CodeRabbit