[hijzy]

Summary

• Move expensive embedding-maintenance reads into a dedicated worker, preventing large SQLite scans from blocking the main event loop.
• Coalesce identical concurrent maintenance queries and avoid redundant scans after an upgrade has completed.
• Add targeted repository queries and include the maintenance reader worker in runtime bundles.
• Improve dev-start.sh dependency detection using package manifest and lockfile fingerprints.
• Automatically install missing dependencies such as electron and jszip.
• Resolve hoisted Electron packages correctly and ensure the Electron runtime is available before startup.

Testing

• Added embedding-maintenance reader tests covering:
  • concurrent query coalescing
  • completed-upgrade scan avoidance
  • 60,000 synthetic memory records
  • event-loop and health-endpoint responsiveness
  • SQLite open failures, backoff, and shutdown behavior
• Updated embedding maintenance and provenance upgrade tests.
• Added development launcher regression tests covering dependency refresh and hoisted Electron resolution.
• Updated runtime bundle tests to verify the maintenance reader worker is packaged.

[Memtensor-AI]

🤖 Open Code Review

Target: PR #797
Task: 0b0fb610e8284288
Base: v1.2.6
Head: main

🔍 OpenCodeReview found 20 issue(s) in this PR.

⚠️ Review warnings (6)

• tool output exceeded the byte/line budget; use narrower file ranges or paginated searches; omitted output is not reviewed
• bounded analysis finalized with explicit task_done after reviewing all group files
• bounded analysis finalized with explicit task_done after reviewing all group files
• bounded analysis finalized with explicit task_done after reviewing all group files
• bounded analysis finalized with explicit task_done after reviewing all group files
• bounded analysis finalized with explicit task_done after reviewing all group files

---

1. Memory/src/storage/repositories.ts (L1671-L1677)

This query drops both guards the sibling embeddingMaintenanceEntriesByIds applies: user_id = ? and status = 'active'. Consequences:

1. It returns the raw content of memories owned by any user for whatever ids are passed in, so a mis-resolved/foreign id silently leaks cross-tenant content and breaks the per-user isolation this repository class enforces elsewhere.
2. Entries are filtered to status = 'active', but sources are not, so sources can be returned for archived/deleted-status rows that have no matching maintenance entry — the caller then compares a job hash against a memory that should not be part of the maintenance run.

Suggest mirroring the sibling's signature/clauses.

💡 Suggested Change

Before:

  embeddingMaintenanceSourcesByIds(ids: readonly string[]): Array<{ id: string; content: string }> {
    if (ids.length === 0) return [];
    return this.db.prepare(`
      SELECT id, content FROM user_memories
      WHERE id IN (SELECT CAST(value AS TEXT) FROM json_each(?)) AND deleted_at IS NULL
    `).all(toJson(ids)) as Array<{ id: string; content: string }>;
  }

After:

  embeddingMaintenanceSourcesByIds(userId: string, ids: readonly string[]): Array<{ id: string; content: string }> {
    if (ids.length === 0) return [];
    return this.db.prepare(`
      SELECT id, content FROM user_memories
      WHERE user_id = ? AND status = 'active' AND deleted_at IS NULL
        AND id IN (SELECT CAST(value AS TEXT) FROM json_each(?))
    `).all(userId, toJson(ids)) as Array<{ id: string; content: string }>;
  }

---

2. Memory/src/storage/repositories.ts (L729-L735)

getManyWithoutVectors is getMany minus the attachMemoryVectors step (and a different id-list encoding), and getWithoutVectors is get minus hydrate. Since memoryFromSql already returns a complete MemoryRow and vectors are attached via a symbol, the only difference is the vector hydration. Consider extracting the row-selection into one private helper (e.g. selectMemoryRows(ids) / selectMemoryRow(id)) and letting get/getMany add hydration, so the two paths cannot drift (e.g. if a new filter such as status != 'deleted' is later added to one of them).

---

3. Memory/src/storage/sqlite-vec-store.ts (L37)

provenanceUpgradeEntries moved the upgrade-candidate selection from TypeScript into SQL, and the previous implementation only treated vec_action as an upgrade candidate for memory_layer = 'L1' rows (the maintenanceEntries() branch joins vec_action with WHERE m.memory_layer = 'L1'). The new query matches v.vector_field = 'vec_action' on every layer, so any non-L1 row that happens to carry a vec_action vector becomes an upgrade candidate — a silent widening of the upgrade scope.

Also: the criteria ('feedback.experience.v1' duplicated in two branches, 'L2', 'vec', the internal_info.source / plugin_algorithm JSON paths) are hardcoded inside the SQL string with no explanatory comment, and the literal is also spelled out in feedback-experience.ts. Please keep the L1 guard and at least document the criteria (ideally referencing a shared constant) so the query does not silently drift from the writer.

---

4. Memory/src/service/memory-service.ts (L355-L356)

The reader is told the SQLite path only through options.db, but the repositories actually used above come from options.backend?.repositories(). The production wiring (Memory/src/server/index.ts:183) constructs MemoryService with backend and no db, so sqlitePath is undefined, EmbeddingMaintenanceReader.workerBacked stays false, and every stats/progress/upgrade-candidate read still runs synchronously on the main thread — the blocking this reader was introduced to remove. If a caller ever supplies both db and backend, the path may also not match the backend's database, making the worker read a different/empty file. Derive the path from the backend that produced this.repos (e.g. SqliteStorageBackend) with options.db?.path as fallback.

💡 Suggested Change

Before:

      sqlitePath: options.db?.path
    });

After:

      sqlitePath: options.backend instanceof SqliteStorageBackend ? options.backend.db.path : options.db?.path
    });

---

5. Memory/src/service/memory-service.ts (L3310)

stats() is now asynchronous and can reject (reader worker fatal error, SQLite failure in the read lane), but it is called outside the try. Previously the whole stats computation was guarded, so a failure degraded to dimensionError; now it propagates to /api/v1/embeddings/maintenance (Memory/src/server/viewer-api.ts:236) and turns a panel refresh into a 500. Keep the stats call inside the guarded block so the graceful degradation is preserved.

💡 Suggested Change

Before:

      return this.embeddingMaintenance().stats(dimension);

After:

      try {
        return await this.embeddingMaintenance().stats(dimension);
      } catch (error) {
        return { ...await this.embeddingMaintenance().stats(0), dimensionError: error instanceof Error ? error.message : String(error) };
      }

---

6. Memory/src/service/memory-service.ts (L629-L632)

upgradeProgress() is now awaited and can reject (DB/KV read or reader-lane failure), which aborts afterRun before reconcileAutomaticHistorySkills() runs and, with the worker change, rejects an otherwise successful drain. Wrap the maintenance reporting so the history-skill reconciliation always executes.

💡 Suggested Change

Before:

      afterRun: async () => {
        if (this.config.algorithm.enableMemoryAdd) await this.embeddingMaintenance().upgradeProgress();
        this.reconcileAutomaticHistorySkills(nowIso());
      },

After:

      afterRun: async () => {
        try {
          if (this.config.algorithm.enableMemoryAdd) await this.embeddingMaintenance().upgradeProgress();
        } finally {
          this.reconcileAutomaticHistorySkills(nowIso());
        }
      },

---

7. Memory/src/service/worker/worker-runner.ts (L492)

Awaiting afterRun makes post-drain bookkeeping part of the drain's success/failure contract: if the callback rejects, runWorkerOnce rejects even though all jobs of that drain completed, and callers lose the WorkerRunSummary. It also keeps the single scheduler execution boundary active (WorkerScheduler.execution) for the whole maintenance read, delaying subsequent drains. Isolate the hook's errors and log them rather than failing the drain.

💡 Suggested Change

Before:

      await this.deps.afterRun?.();

After:

      try {
        await this.deps.afterRun?.();
      } catch (error) {
        // Post-run bookkeeping must not fail an otherwise completed drain.
        workerLogger.warn("after_run.failed", memoryErrorFields(error));
      }

---

8. Memory/src/service/embedding/embedding-maintenance-reader.ts (L89-L93)

stop() resolves when the worker exits but never settles the entries in this.pending. The exit handler deliberately skips fail() when closing is true, so any readStats/readTargets promise that was still awaiting a worker response at shutdown (e.g. an unexpected worker exit racing with stop(), or the threadId === -1 early-return path) is left pending forever. Callers such as EmbeddingMaintenance.progress() would then hang, blocking graceful shutdown and test teardown. Reject all pending requests when stopping.

💡 Suggested Change

Before:

    this.stopped = new Promise<void>((resolve) => {
      if (worker.threadId === -1) {
        resolve();
        return;
      }

After:

    const stopping = new Error("Embedding maintenance reader is stopping");
    for (const pending of this.pending.values()) pending.reject(stopping);
    this.pending.clear();
    this.stopped = new Promise<void>((resolve) => {
      if (worker.threadId === -1) {
        resolve();
        return;
      }

---

9. Memory/src/service/embedding/embedding-maintenance.ts (L234)

cacheStats() is only called when refresh() receives a stats argument, but every upgrade path calls refresh(run, ...) without stats (resumeUpgrade lines this.refresh(run, targetSnapshot), this.refresh(run, []), and upgradeProgress's this.refresh(current, targets)). As a result no embedding-maintenance-stats:<runId> entry is ever written for upgrade runs, and this completed-run branch falls back to emptyStats(), so upgradeProgress() reports statsAfter: { totalSlots: 0, ready: 0, retryQueue: {…0} }. Before this change a live snapshot was returned here. Either persist stats during resumeUpgrade/upgradeProgress (read the target stats) or compute the stats in this branch instead of returning zeroed counters.

---

10. Memory/src/service/embedding/embedding-maintenance.ts (L207)

A transient reader failure (worker spawn failure, 30s worker backoff, SQLite busy) is persisted onto the durable upgrade run as run.error plus a 5-minute nextAttemptAt. Because refresh() short-circuits target evaluation whenever run.error is set, and nextUpgradeAttemptAt() now defers repair for 5 minutes, a single failed status read reports the upgrade as failed to the panel and suppresses all target progress until the next successful resumeUpgrade. Prefer surfacing the read failure only in the returned result (or store it separately) rather than poisoning the durable run state.

---

11. Memory/src/service/embedding/embedding-maintenance-reader-worker.ts (L38-L40)

Nested ternary in the message handler violates the project style rule and is hard to extend with additional operations. Use an early-return dispatch.

💡 Suggested Change

Before:

    const result = message.operation.type === "stats"
      ? readEmbeddingMaintenanceStats(repos, message.operation.request)
      : message.operation.type === "targets"

After:

    const operation = message.operation;
    const result = operation.type === "stats"
      ? readEmbeddingMaintenanceStats(repos, operation.request)
      : operation.type === "targets"

---

12. Memory/src/storage/sqlite-vec-store.ts (L85-L86)

The resolved default field here does not match how the caller keys the result. readTargetProgress builds a map with targetKey(entry) and looks it up with targetKey(target), where targetKey falls back to 'vec' when vectorField is undefined (${kind}:${id}:${vectorField ?? "vec"}). So for a memory target that carries no explicit vectorField (the old refresh() explicitly supported this shape for persisted runs via its entries.find(... vectorField !== 'vec_action') fallback, which has now been removed), the row returned here is keyed ...:vec_summary for L1 rows while the lookup key is ...:vec → the entry is invisible → health becomes "missing" and a completed target gets reported as failed (state.health === "ready" is false). Either return the resolved field to the caller so it can key by entry.vectorField, or drop the COALESCE so the SQL never reports a field the consumer cannot look up.

---

13. scripts/dev-start.sh (L151-L152)

The required-package probe resolves each package from the caller's cwd (node_can_resolve does not cd), while npm install runs at $ROOT_DIR and ensure_electron_runtime now resolves electron from $DESKTOP_DIR. If electron ends up nested in App/shell/desktop/node_modules (not hoisted, e.g. a dev install made inside the workspace) or when the script is invoked from outside the repo, the probe reports electron missing forever, the post-install re-check at line 186 still fails and the script exit 1s, permanently blocking dev startup. Probe from a fixed directory instead (e.g. node_can_resolve_from_dir "$DESKTOP_DIR" for electron, $ROOT_DIR for the root-level packages).

---

14. scripts/dev-start.sh (L120)

The fingerprint only covers the root package.json and package-lock.json. This repo is an npm-workspaces monorepo (App/shell/*, App/frontend/*, Memory, App/backend, ...), and several probed/needed packages live only in workspace manifests (electron is a devDependency of App/shell/desktop/package.json). Bumping e.g. the Electron version there leaves the fingerprint unchanged and the missing_packages probe still passes, so npm install is silently skipped and the desktop app keeps running the stale Electron/hosted dependency set. Consider folding the workspace manifests (at least $DESKTOP_DIR/package.json, ideally every path listed under workspaces) into the hashed file list.

---

15. Memory/src/service/embedding/embedding-maintenance-reader-query.ts (L172-L174)

readTargetProgress never passes outdatedVectors to healthForEntry, so per-target health can only ever be ready/missing/dimMismatch/modelMismatch — contentMismatch is unreachable here (unlike the includePlan path which computes it from currentEmbeddingMaintenanceState). Consequences: EmbeddingMaintenance.resumeUpgrade() filters its upgrade scope with byKey.get(...)?.health !== "ready" (line 172), so legacy action/feedback vectors whose content changed but whose dimension/identity still match are skipped as already up to date. Either compute the outdated set for the requested targets or document that content staleness is only covered by sourceCurrent.

---

16. Memory/src/service/embedding/embedding-maintenance-reader-query.ts (L182)

sourceCurrent uses strict equality against a single canonical text, while currentEmbeddingMaintenanceState (via maintenanceSourceTexts) accepts several equivalent historical variants (memoryValue, embeddingTextForMemory, traceSummaryEmbeddingText, canonicalWorkMemoryText). A queue row enqueued by another code path with a legacy variant therefore reports sourceCurrent: false even though the vector matches the current content, and EmbeddingMaintenance.refresh() then flips a genuinely successful target to result: "failed" (state.succeeded && state.sourceCurrent && state.health === "ready"). Use the same accepted-text-set check here for consistency.

---

17. Memory/src/service/embedding/embedding-maintenance-reader-query.ts (L167-L169)

The companion vector lookup is user-scoped (embeddingMaintenanceEntriesByIds(request.userId, ...)), but this source lookup reads any user_memories row by id without a user_id filter (and without requiring status = 'active', which embeddingMaintenanceEntriesByIds does). Targets come from persisted run state, so a stale/foreign id can pull another user's content into this user's maintenance read. Pass request.userId and reuse the same scoping/status predicates.

---

18. Memory/src/service/embedding/embedding-maintenance.ts (L117)

progress() returns null when the run can no longer be selected (e.g. another start() replaced the manual run key while the read was in flight, or runId no longer resolves). The non-null assertion hides that: start() is declared to return EmbeddingMaintenanceRunResult, so callers in the viewer API can end up with null and crash on property access. Handle the null case explicitly (re-read or throw a MemoryServiceError).

---

19. scripts/dev-start.sh (L165)

root_dependency_fingerprint does an unconditional readFileSync("$ROOT_DIR/package-lock.json"). Under set -euo pipefail a failing command substitution in an assignment aborts the whole script, so if the lockfile is missing (git-archive export, partial checkout, or a user who deleted it so npm can regenerate it) dev-start now dies with a raw Node ENOENT stack trace before it ever reaches npm install — the very command that would create the lockfile. Previously a missing lockfile was harmless. Consider degrading gracefully so an unreadable lockfile simply never matches the stamp and an install still runs (write_root_dependency_stamp needs the same treatment).

💡 Suggested Change

Before:

  expected_fingerprint="$(root_dependency_fingerprint)"

After:

  expected_fingerprint="$(root_dependency_fingerprint 2>/dev/null)" || expected_fingerprint="lockfile-unavailable"

---

20. scripts/internal/shared/bundle-runtime.mjs (L70)

This list is now an implicit contract with every new Worker(new URL("./x.js", import.meta.url), ...) spawn site in Memory (storage-startup-worker, embedding-maintenance-reader-worker, ...). A worker added without registering it here is silently degraded: it falls through to the assets branch and is copied verbatim, so its relative/bare imports are left un-bundled and it can only work by accident. Worth a short comment next to the list (like the one above dynamicPackages) stating that worker scripts spawned by URL must be registered here, so the next author knows to update it.

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

⚠️ Automated Test Results: NO TEST SCOPE

Automated tests were not run because the changed files do not map to an executable test scope.

Details: No executable test scope maps to the changed files. Automated tests were not run; add env.yaml source_mapping + execution for this path family, then rerun. Changed files: App/shell/desktop/tests/dev-cli-launcher.test.ts, Memory/src/service/embedding/embedding-maintenance-reader-query.ts, Memory/src/service/embedding/embedding-maintenance-reader-worker.ts, Memory/src/service/embedding/embedding-maintenance-reader.ts, Memory/src/service/embedding/embedding-maintenance.ts, Memory/src/service/memory-service.ts, Memory/src/service/worker/worker-runner.ts, Memory/src/storage/repositories.ts, Memory/src/storage/sqlite-vec-store.ts, Memory/tests/embedding-maintenance.test.ts
Manual review or env.yaml source_mapping/execution coverage is required before merge.

Branch: main