Skip to content

fix(embedding): add foreground-priority gate to prevent turn.start starvation - #2187

Open
pittosporum-seu wants to merge 1 commit into
MemTensor:mainfrom
pittosporum-seu:fix/embedding-priority-gate
Open

fix(embedding): add foreground-priority gate to prevent turn.start starvation#2187
pittosporum-seu wants to merge 1 commit into
MemTensor:mainfrom
pittosporum-seu:fix/embedding-priority-gate

Conversation

@pittosporum-seu

Copy link
Copy Markdown

Description

Add a lightweight foreground-priority gate to the embedding layer to prevent turn.start RPC starvation when the background capture pipeline (reflection, reward, skill crystallization) is running CPU-bound ONNX inference.

Problem: The local ONNX embedding provider (Xenova/all-MiniLM-L6-v2) runs inference sequentially on the main thread. When background tasks embed trace rows in a batch loop, foreground retrieval requests must wait for the entire batch to finish — often exceeding the host's 8-second prefetch timeout. This causes memory injection to be silently skipped on every affected turn.

Solution: A cooperative yield mechanism (priority-gate.ts) that:

  1. Foreground callers (retrieval) signal via enterForeground() before embedding
  2. The local provider checks isForegroundPending() between individual inference calls and yields the event loop via setImmediate
  3. This gives foreground requests a scheduling opportunity without changing the Embedder interface or adding worker threads

Design rationale: Minimal, non-breaking change. No interface changes to Embedder, no new dependencies, no worker threads. The yield is a single setImmediate between ONNX calls — negligible overhead when no foreground is pending (just a boolean check).

Related Issue (Required): Fixes #2186

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (does not change functionality, e.g. code style improvements, linting)
  • Documentation update

How Has This Been Tested?

  • Unit Test
  • Test Script Or Test Steps (please provide)
  • Pipeline Automated API Test (please provide)

Unit test: tests/unit/embedding/priority-gate.test.ts — 6 tests covering:

  • Initial state (no foreground pending)
  • Enter/release lifecycle
  • Stacking multiple foreground calls
  • Idempotent release
  • Yield behavior (immediate when no foreground, setImmediate when foreground pending)

Additionally verified:

  • npx tsc --noEmit passes with zero errors
  • Full test suite: 147/153 test files pass (6 pre-existing failures unrelated to this change — locale/path issues on Windows)

Checklist

  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have created related documentation issue/PR in MemOS-Docs (if applicable)
  • I have linked the issue to this PR (if applicable)
  • I have mentioned the person who will review this PR

Changes Made

File Change
core/embedding/priority-gate.ts New — foreground-priority gate (enterForeground, isForegroundPending, yieldIfForegroundPending)
core/embedding/providers/local.ts Yield between individual ONNX inference calls when foreground is pending
core/embedding/index.ts Export priority-gate public API
core/retrieval/retrieve.ts Wrap foreground query embed with enterForeground()/release
tests/unit/embedding/priority-gate.test.ts New — 6 unit tests for the priority gate

…arvation (MemTensor#2186)

The local ONNX embedding provider runs CPU-bound inference sequentially on
the main thread. When the background capture pipeline (reflection,
reward, skill crystallization) is embedding trace rows, a foreground
retrieval request (turn.start) must wait for the entire batch to finish,
often exceeding the host's 8s prefetch timeout.

Add a lightweight cooperative yield mechanism:
- priority-gate.ts: enterForeground()/yieldIfForegroundPending() signals
- local.ts: yield between individual ONNX inference calls when foreground pending
- retrieve.ts: mark foreground retrieval embed calls via enterForeground()

This gives the single-threaded ONNX inference a cooperative scheduling
point without changing the Embedder interface or adding worker threads.
@Memtensor-AI Memtensor-AI added area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Jul 29, 2026
@Memtensor-AI
Memtensor-AI requested review from hijzy and whipser030 July 29, 2026 17:51
@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2187
Task: 572d56844bb32b14
Base: main
Head: fix/embedding-priority-gate

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

⚠️ 1 warning(s) occurred during review.


1. apps/memos-local-plugin/core/embedding/priority-gate.ts (L54)

setImmediate is a Node.js-only API and does not exist in browser, Electron renderer, or edge/worker runtimes. If this module is ever bundled for a non-Node.js target it will throw ReferenceError: setImmediate is not defined exactly when a foreground request is blocked — the worst possible moment.

Add a cross-environment fallback so the module degrades gracefully:


2. apps/memos-local-plugin/core/embedding/providers/local.ts (L80)

The abort check is performed before the yield but not after it. After yieldIfForegroundPending() resolves, the signal may already be aborted (the foreground request that was awaited could have been aborted or a new abort could have arrived while the event loop was yielded). The next iteration will catch it on the next loop head, but that only happens after an already-queued inference call (ext(...)) completes — which is the expensive operation you are trying to interrupt.

Move the abort check to after the yield so that a freshly-aborted background task never enters ext() unnecessarily:

if (i > 0) await yieldIfForegroundPending();
if (ctx.signal?.aborted) throw new DOMException("Aborted", "AbortError");
💡 Suggested Change

Before:

      if (i > 0) await yieldIfForegroundPending();

After:

      if (i > 0) await yieldIfForegroundPending();
      if (ctx.signal?.aborted) throw new DOMException("Aborted", "AbortError");

3. apps/memos-local-plugin/core/embedding/providers/local.ts (L19)

setImmediate is a Node.js-specific API not available in browser or non-Node runtimes (Deno, Bun, Workers). Since this is an embedding provider that might be shipped in environments beyond Node.js, consider using Promise.resolve() (a microtask yield) or setTimeout(resolve, 0) (a macrotask yield) for broader portability. If Node.js is the only supported runtime this is acceptable, but it's worth documenting the assumption or adding a guard:

await new Promise<void>((resolve) =>
  typeof setImmediate === "function"
    ? setImmediate(resolve)
    : setTimeout(resolve, 0)
);
💡 Suggested Change

Before:

import { yieldIfForegroundPending } from "../priority-gate.js";

After:

  if (foregroundCount > 0) {
    await new Promise<void>((resolve) =>
      typeof setImmediate === "function"
        ? setImmediate(resolve)
        : setTimeout(resolve, 0)
    );
  }

4. apps/memos-local-plugin/core/embedding/priority-gate.ts (L28-L37)

The JSDoc says release "MUST be called", but doesn't tell callers how to guarantee that on error paths. If a foreground caller throws before invoking the release function, foregroundCount is permanently incremented and all background batches stall forever with no timeout or auto-recovery.

The retrieve.ts caller correctly uses .finally(release), but this contract should be spelled out explicitly in the JSDoc so future callers get it right from the start. Example addition to the JSDoc:

 * Always call the returned release inside a `try/finally` block (or
 * `.finally()` on a Promise chain) to guarantee cleanup on error:
 *
 *   const release = enterForeground();
 *   try { await embed(text); } finally { release(); }

5. apps/memos-local-plugin/core/embedding/priority-gate.ts (L52-L56)

yieldIfForegroundPending inlines foregroundCount > 0 instead of delegating to the already-exported isForegroundPending(). While the duplication is trivial today, if the gate condition ever changes (e.g. a threshold, a flag, a priority level), this copy would silently diverge.

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (6/6 executed). memos_local_plugin/unit: 6/6. Duration: 2s

Branch: fix/embedding-priority-gate

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin OpenClaw & Hermes status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: foreground turn.start RPC starved by background pipeline (embedding + LLM resource contention)

3 participants