fix(memory): avoid first-turn cold stalls - #1828
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates DuckDB VSS packaging, workflow verification, and MemoryPresenter warm/readiness logic to use embedding-dimension metadata and bundled extension loading. ChangesMemory cold-start and bundled VSS
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
test/main/presenter/memoryPresenter.test.ts (1)
78-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the “current” row with explicit timestamps.
Both embedded fixtures inherit
createdAt = 1000fromFakeRepository, sogetCurrentEmbeddingDimension('a', 'p:m')returns4only because insertion order happens to win. Givecurrentandwrong-dimdistinct timestamps so this test keeps proving the intended winner once the SQLite lookup is ordered.Suggested fix
repo.insert({ id: 'current', agentId: 'a', kind: 'semantic', - content: 'current vector' + content: 'current vector', + createdAt: 2_000 }) @@ repo.insert({ id: 'wrong-dim', agentId: 'a', kind: 'semantic', - content: 'wrong dimension' + content: 'wrong dimension', + createdAt: 1_000 })🤖 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 78 - 99, The test setup for getCurrentEmbeddingDimension in memoryPresenter.test.ts relies on insertion order because both embedded rows share the same createdAt from FakeRepository. Update the fixtures around repo.insert and repo.updateStatus for the current and wrong-dim records so they have explicit, distinct timestamps, with current pinned as the intended latest winner, and keep using the existing repository methods and getCurrentEmbeddingDimension expectation to verify the SQLite ordering behavior.
🤖 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 @.github/workflows/build.yml:
- Around line 79-83: The DuckDB VSS smoke checks in the workflow only validate
the workspace copy before packaging, so they miss regressions in the packaged
app. Move or add a post-build verification step that runs against the unpacked
Electron artifacts/resources for each OS and arch, using the existing
install/smoke commands or their equivalent in the build job after packaging, so
the check confirms the bundled VSS asset is actually present.
In @.github/workflows/release.yml:
- Around line 173-177: The current DuckDB VSS Windows checks only validate the
pre-package runtime, so add a post-package verification step in the release
workflow that runs against the built app artifact and confirms VSS is loaded
from the bundled resources path. Update the existing Windows VSS verification
sequence around the Install and verify DuckDB VSS for Windows step (and the
matching release-job blocks it applies to) so it first packages the app, then
smoke-tests the packaged output rather than the runtime install path. Ensure the
new verification explicitly targets the shipped app’s bundled resources to catch
packaging regressions where the extension is missing.
In `@scripts/installVss.js`:
- Around line 128-166: Add a per-attempt timeout inside downloadExtension so a
stalled fetchImpl cannot block the retry loop indefinitely. Update the fetch
call in downloadExtension to use an attempt-scoped timeout signal or wrapper,
and make sure timeout failures are converted into retryable VssDownloadError
instances with the existing retry/backoff flow and onRetry callback. Keep the
change localized to downloadExtension and related timeout helpers/options in
scripts/installVss.js.
In `@scripts/smoke-duckdb-vss.js`:
- Around line 13-25: The parseArgs helper currently accepts flags like
--platform and --arch without an explicit value by falling back to the next argv
entry or host defaults, which can hide miswired smoke commands. Update parseArgs
in the smoke-duckdb-vss script to detect when a long flag is provided without an
inline “=” value and the next argv entry is missing or is another flag, then
fail fast by throwing an error instead of populating options. Keep the behavior
for valid key/value forms unchanged so the CLI only proceeds when required
values are explicitly supplied.
In `@src/main/presenter/memoryPresenter/index.ts`:
- Around line 253-254: Track and cancel pending prewarm timers for a deleted
agent in memoryPresenter’s timer management. The issue is that
cleanupDeletedAgentResources() only drains active warmups and leaves scheduled
entries in prewarmTimers, which can still fire after deletion. Update the
prewarm scheduling logic in memoryPresenter (including the
prewarmStartTimer/prewarmTimers state and the cleanupDeletedAgentResources() /
settleDeletedAgentInFlight() flow) to associate timers with agent IDs, clear any
matching timers before settleDeletedAgentInFlight(), and remove them from the
set so deleted agents cannot be reopened by late prewarm callbacks.
In `@src/main/presenter/memoryPresenter/memoryVectorStore.ts`:
- Around line 71-86: The fallback in MemoryVectorStore’s extension loading path
is still performing a network INSTALL vss from packaged builds when the bundled
extension is missing or fails to load. Update the load logic around the
try/catch in MemoryVectorStore to fail closed in packaged/runtime builds (or
disable vector recall) instead of calling INSTALL/LOAD vss, and keep the network
fallback only for dev/test paths so open does not block on a first-turn
download.
In `@src/main/presenter/sqlitePresenter/tables/agentMemory.ts`:
- Around line 808-820: The current-dimension lookup in
getCurrentEmbeddingDimension is nondeterministic because it uses LIMIT 1 without
an ORDER BY, so SQLite may return an older embedded row. Update this query in
agentMemory to deterministically select the most recent/current matching row,
using a stable sort on a suitable timestamp or primary key field that identifies
the latest embedding record, so the warmup path and fake repository always see
the same embedding_dim.
In `@test/main/presenter/agentMemoryTable.test.ts`:
- Around line 276-280: The current dimension check for
getCurrentEmbeddingDimension is nondeterministic because the underlying
agentMemory.ts query uses LIMIT 1 without an ORDER BY, so the test can alternate
between matching dimensions. Fix this by making the SQLite query in
agentMemory.ts deterministic using a stable ordering, or update the test to
avoid asserting the exact returned dimension and instead only verify the
non-null/stale embedding behavior around getCurrentEmbeddingDimension and
hasStaleEmbeddings.
In `@test/main/presenter/pluginPresenter.test.ts`:
- Around line 1360-1362: The macOS workflow test assertions in
pluginPresenter.test are too broad because the ARM runner check is currently
satisfied by the Intel runner line. Tighten the expectations around the
buildWorkflow string in the affected test cases by asserting the exact macOS
runner entries for both architectures separately, using the existing
buildWorkflow, macos-15-intel, and macos-15 references so the test proves the
ARM runner is actually present.
---
Nitpick comments:
In `@test/main/presenter/memoryPresenter.test.ts`:
- Around line 78-99: The test setup for getCurrentEmbeddingDimension in
memoryPresenter.test.ts relies on insertion order because both embedded rows
share the same createdAt from FakeRepository. Update the fixtures around
repo.insert and repo.updateStatus for the current and wrong-dim records so they
have explicit, distinct timestamps, with current pinned as the intended latest
winner, and keep using the existing repository methods and
getCurrentEmbeddingDimension expectation to verify the SQLite ordering behavior.
🪄 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: 3259c2d1-48d2-4667-b56a-b35c6b5362b5
📒 Files selected for processing (23)
.github/workflows/build.yml.github/workflows/prcheck.yml.github/workflows/release.yml.github/workflows/windows-arm64-e2e.ymldocs/issues/memory-first-turn-cold-start/plan.mddocs/issues/memory-first-turn-cold-start/spec.mddocs/issues/memory-first-turn-cold-start/tasks.mdpackage.jsonscripts/installVss.jsscripts/smoke-duckdb-vss.jssrc/main/presenter/index.tssrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/memoryVectorStore.tssrc/main/presenter/memoryPresenter/types.tssrc/main/presenter/sqlitePresenter/tables/agentMemory.tstest/main/presenter/agentMemoryTable.test.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memory-persona-eval.test.tstest/main/presenter/memoryAdd.test.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memoryRetrieval.eval.test.tstest/main/presenter/pluginPresenter.test.tstest/main/scripts/installVss.test.ts
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 `@test/main/presenter/memoryVectorStore.test.ts`:
- Around line 192-195: Avoid hard-coding the Linux-only extension name in the
fs.existsSync mock used by memoryVectorStore.test.ts. Update the mock around the
MemoryVectorStore.loadVss path so it detects the bundled extension file via the
same suffix logic the implementation uses (vss${extensionSuffix}) or otherwise
matches the extension filename dynamically, ensuring the “missing extension”
branch is exercised on macOS, Windows, and Linux. Keep the existing spyOn(fs,
'existsSync') setup, but make the false case derive from the actual extension
suffix rather than a fixed .duckdb_extension string.
🪄 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: 5de1cfc2-a167-4aea-8650-df7a3eecea6d
📒 Files selected for processing (17)
.github/workflows/build.yml.github/workflows/release.ymldocs/issues/memory-first-turn-cold-start/plan.mddocs/issues/memory-first-turn-cold-start/spec.mddocs/issues/memory-first-turn-cold-start/tasks.mdscripts/installVss.jsscripts/smoke-duckdb-vss.jssrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/memoryVectorStore.tssrc/main/presenter/sqlitePresenter/tables/agentMemory.tstest/main/presenter/agentMemoryTable.test.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memoryVectorStore.test.tstest/main/presenter/pluginPresenter.test.tstest/main/scripts/installVss.test.tstsconfig.node.tsbuildinfo
✅ Files skipped from review due to trivial changes (2)
- docs/issues/memory-first-turn-cold-start/spec.md
- docs/issues/memory-first-turn-cold-start/tasks.md
🚧 Files skipped from review as they are similar to previous changes (6)
- test/main/presenter/fakes/memoryFakes.ts
- .github/workflows/release.yml
- src/main/presenter/sqlitePresenter/tables/agentMemory.ts
- test/main/presenter/pluginPresenter.test.ts
- src/main/presenter/memoryPresenter/index.ts
- test/main/presenter/memoryPresenter.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/main/presenter/memoryVectorStore.test.ts (1)
156-160: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake these packaged-VSS path mocks separator-agnostic.
Both
endsWith('/runtime/...')checks only match POSIX paths. On Windows,path.join()produces\, so these tests stop forcing the.gzbranch and can fail for the wrong reason. This reintroduces the same cross-platform mock fragility that was fixed earlier in this file.Suggested fix
- if (filePath.endsWith('/runtime/duckdb/extensions/vss.duckdb_extension')) return false - if (filePath.endsWith('/runtime/duckdb/extensions/vss.duckdb_extension.gz')) return true + if (/[/\\]runtime[/\\]duckdb[/\\]extensions[/\\]vss\.duckdb_extension$/.test(filePath)) { + return false + } + if (/[/\\]runtime[/\\]duckdb[/\\]extensions[/\\]vss\.duckdb_extension\.gz$/.test(filePath)) { + return true + }Also applies to: 385-389
🤖 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/memoryVectorStore.test.ts` around lines 156 - 160, The `memoryVectorStore.test.ts` mocks for packaged VSS paths are hardcoded with POSIX separators, so they can miss on Windows and stop exercising the intended `.gz` branch. Update the `vi.spyOn(fs, 'existsSync')` mocks to match path separators in a separator-agnostic way, using the same approach consistently in both affected blocks so the checks work regardless of how `path.join()` formats the extension paths.
🧹 Nitpick comments (1)
src/main/presenter/memoryPresenter/memoryVectorStore.ts (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the new module constants to
SCREAMING_SNAKE_CASE.
extensionName,gunzipAsync, andpackagedVssMaterializationPromisesare new top-level constants in asrc/**/*.tsfile, so they should follow the repo’s constant naming rule. As per coding guidelines,src/**/*.{ts,tsx}: Constants should use SCREAMING_SNAKE_CASE naming.🤖 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/memoryVectorStore.ts` around lines 23 - 25, The new top-level constants in memoryVectorStore.ts do not follow the repo’s constant naming rule. Rename extensionName, gunzipAsync, and packagedVssMaterializationPromises to SCREAMING_SNAKE_CASE, and update any references within memoryVectorStore or related helpers so the module still works with the new names.Source: Coding guidelines
🤖 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 `@scripts/smoke-duckdb-vss.js`:
- Around line 111-127: The DuckDB smoke test cleanup is scoped too narrowly, so
a failure in `DuckDBInstance.create`/`instance.connect` can leave the native
instance open. Refactor `scripts/smoke-duckdb-vss.js` so the `duckdb`,
`instance`, and `connection` handles are declared outside the inner block and
all cleanup happens in one shared `finally`, closing each handle conditionally
only if it was created successfully.
---
Duplicate comments:
In `@test/main/presenter/memoryVectorStore.test.ts`:
- Around line 156-160: The `memoryVectorStore.test.ts` mocks for packaged VSS
paths are hardcoded with POSIX separators, so they can miss on Windows and stop
exercising the intended `.gz` branch. Update the `vi.spyOn(fs, 'existsSync')`
mocks to match path separators in a separator-agnostic way, using the same
approach consistently in both affected blocks so the checks work regardless of
how `path.join()` formats the extension paths.
---
Nitpick comments:
In `@src/main/presenter/memoryPresenter/memoryVectorStore.ts`:
- Around line 23-25: The new top-level constants in memoryVectorStore.ts do not
follow the repo’s constant naming rule. Rename extensionName, gunzipAsync, and
packagedVssMaterializationPromises to SCREAMING_SNAKE_CASE, and update any
references within memoryVectorStore or related helpers so the module still works
with the new names.
🪄 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: a8b4ff47-1942-409f-a994-b7833fd98041
📒 Files selected for processing (12)
.github/workflows/build.yml.github/workflows/release.ymldocs/issues/memory-first-turn-cold-start/plan.mddocs/issues/memory-first-turn-cold-start/spec.mddocs/issues/memory-first-turn-cold-start/tasks.mdscripts/afterPack.jsscripts/smoke-duckdb-vss.jssrc/main/presenter/memoryPresenter/memoryVectorStore.tstest/main/presenter/memoryVectorStore.test.tstest/main/presenter/pluginPresenter.test.tstest/main/scripts/afterPack.test.tstest/main/scripts/installVss.test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/issues/memory-first-turn-cold-start/tasks.md
- docs/issues/memory-first-turn-cold-start/spec.md
- docs/issues/memory-first-turn-cold-start/plan.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/build.yml
- .github/workflows/release.yml
- test/main/scripts/installVss.test.ts
- test/main/presenter/pluginPresenter.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/afterPack.js (1)
206-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail the macOS pack step when the VSS source asset is missing.
Line 206 silently returns if
app.asar.unpacked/.../vss.duckdb_extensionis absent. That lets a macOS package complete without generating the.b64payload, which defeats the bundled-VSS fail-closed behavior this PR is adding.Suggested fix
if (!(await pathExists(extensionPath))) { - return + throw new Error(`[afterPack] missing macOS DuckDB VSS extension: ${extensionPath}`) }🤖 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 `@scripts/afterPack.js` around lines 206 - 208, The macOS pack flow in afterPack.js currently exits early when extensionPath is missing, which lets the package succeed without the VSS asset. Update the afterPack logic around the pathExists check to fail the pack step instead of returning silently when the vss.duckdb_extension source file is absent, so the bundled-VSS path cannot complete without generating the .b64 payload.src/main/presenter/memoryPresenter/memoryVectorStore.ts (1)
71-72: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake failure cleanup close handles independently.
Line 72 relies on
close(), butclose()wraps connection and instance cleanup in onetry; ifconnection.closeSync()throws,dbInstance.closeSync()is skipped and the native instance can leak.Suggested fix
async close(): Promise<void> { - try { - if (this.connection) this.connection.closeSync() - if (this.dbInstance) this.dbInstance.closeSync() - } catch (error) { - console.error('[MemoryVectorStore] close error', error) - } + for (const closeHandle of [ + () => this.connection?.closeSync(), + () => this.dbInstance?.closeSync() + ]) { + try { + closeHandle() + } catch (error) { + console.error('[MemoryVectorStore] close error', error) + } + } }🤖 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/memoryVectorStore.ts` around lines 71 - 72, The failure cleanup in memoryVectorStore’s catch block only calls store.close(), but the current close() path can skip native instance cleanup if connection.closeSync() throws. Update the cleanup flow so connection and db instance teardown are handled independently, ensuring dbInstance.closeSync() still runs even when connection.closeSync() fails, and keep the catch-side cleanup in memoryVectorStore resilient to either failure.
🤖 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 `@scripts/smoke-duckdb-vss.js`:
- Around line 76-80: The temp directory created in the materialization helper is
leaked if any step fails before returning, so make the cleanup happen even when
base64 decode, gunzip, or write throws. Update the materialization flow around
the helper that creates materializedDir and the caller in main so the temp dir
is tracked as soon as mkdtempSync succeeds and is removed in a finally/cleanup
path regardless of whether extensionPath is returned.
In `@src/main/presenter/memoryPresenter/memoryVectorStore.ts`:
- Around line 97-104: The cache fast path in memoryVectorStore.ts returns
targetPath from getCachedExtension without verifying the file contents, so a
tampered DuckDB extension could be loaded. Update getCachedExtension to compare
the on-disk bytes at targetPath against the packaged asset bytes (or their
digest) before returning, and apply the same validation in the race-win branch
after ensureDir/mkdir so only an exact match is reused; use the existing
createHash, VSS_EXTENSION_NAME, and getCachedExtension flow to locate the fix.
---
Outside diff comments:
In `@scripts/afterPack.js`:
- Around line 206-208: The macOS pack flow in afterPack.js currently exits early
when extensionPath is missing, which lets the package succeed without the VSS
asset. Update the afterPack logic around the pathExists check to fail the pack
step instead of returning silently when the vss.duckdb_extension source file is
absent, so the bundled-VSS path cannot complete without generating the .b64
payload.
In `@src/main/presenter/memoryPresenter/memoryVectorStore.ts`:
- Around line 71-72: The failure cleanup in memoryVectorStore’s catch block only
calls store.close(), but the current close() path can skip native instance
cleanup if connection.closeSync() throws. Update the cleanup flow so connection
and db instance teardown are handled independently, ensuring
dbInstance.closeSync() still runs even when connection.closeSync() fails, and
keep the catch-side cleanup in memoryVectorStore resilient to either failure.
🪄 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: 487e39cc-ac14-4caf-a689-e703e397057e
📒 Files selected for processing (12)
.github/workflows/build.yml.github/workflows/release.ymldocs/issues/memory-first-turn-cold-start/plan.mddocs/issues/memory-first-turn-cold-start/spec.mddocs/issues/memory-first-turn-cold-start/tasks.mdscripts/afterPack.jsscripts/smoke-duckdb-vss.jssrc/main/presenter/memoryPresenter/memoryVectorStore.tstest/main/presenter/memoryVectorStore.test.tstest/main/presenter/pluginPresenter.test.tstest/main/scripts/afterPack.test.tstest/main/scripts/installVss.test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/issues/memory-first-turn-cold-start/plan.md
- docs/issues/memory-first-turn-cold-start/spec.md
- docs/issues/memory-first-turn-cold-start/tasks.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/main/scripts/afterPack.test.ts
- test/main/presenter/pluginPresenter.test.ts
- .github/workflows/release.yml
- test/main/presenter/memoryVectorStore.test.ts
Move cold vector-store opening off the recall hot path,bundle and smoke DuckDB VSS across build/release flows, and add targeted warm metadata checks plus cooldown coverage.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation / Tests