Skip to content

fix(plugin): report filter budget timeouts as llm_timeout and make the budget configurable - #2594

Open
syzsunshine219 wants to merge 2 commits into
MemTensor:mainfrom
akushonkamen:fix/plugin-llmfilter-timeout-budget
Open

syzsunshine219 wants to merge 2 commits into
MemTensor:mainfrom
akushonkamen:fix/plugin-llmfilter-timeout-budget

Conversation

@syzsunshine219

@syzsunshine219 syzsunshine219 commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Restores the change proposed in #2506 for review against main after its former base branch, dev-v2.0.36, was deleted. The source commits and authors are preserved.

Source head: 9e0b217f79c26ae1384b34d2bc70e8b6be8f67bd

This PR is open for review. Review and CI against the current base are pending; validation claims in the original description below refer to the earlier snapshot. The original discussion and evidence remain in #2506.


Original change description

Description

Two stacked defects made local OpenAI-compatible endpoints unusable for the retrieval LLM filter (issue #2499): on every turn_start the filter's local call was doomed to abort at its 2-second cap, the abort was misreported as llm_unavailable, and the plugin silently replayed the filtering on the fallback (host/paid) provider.

Root cause:

  1. Budget timeout misclassified as provider-unavailable. mergeSignals (apps/memos-local-plugin/core/llm/fetcher.ts:382) called ctrl.abort() with no reason, so whenever the caller passed its own signal — the retrieval filter always does — AbortSignal.timeout()'s TimeoutError reason was discarded and fetch rejected with a bare AbortError. isTimeout (apps/memos-local-plugin/core/llm/fetcher.ts:338) could not recognize it either: the legacy check compares code === "ABORT_ERR", but DOMException.code is the numeric 20, so the comparison never matched. A budget timeout was therefore classified llm_unavailable and replayed on the host provider.
  2. Hard-coded filter budget. filterTimeoutMs (apps/memos-local-plugin/core/retrieval/retrieve.ts:691) clamped the filter call to a hard-coded 2_000 ms — too short for self-hosted models on consumer hardware.

Fix:

  • apps/memos-local-plugin/core/llm/fetcher.ts — mergeSignals now forwards the abort reason, so AbortSignal.timeout()'s TimeoutError survives when the caller passes its own signal; isTimeout additionally classifies AbortError / DOMException.code === 20 as a timeout. Caller cancellation keeps its distinct path (it is detected in httpPostJson via opts.signal?.aborted before this classification runs), so user-interrupted requests behave exactly as before.
  • apps/memos-local-plugin/core/retrieval/retrieve.ts (call sites at lines 432 and 493, filterTimeoutMs at line 691) plus the config plumbing (core/config/schema.ts:599, core/config/defaults.ts:322, core/retrieval/types.ts:331, core/pipeline/deps.ts:170) — new algorithm.retrieval.llmFilterTimeoutMs option (range 100–30 000 ms, default 2_000 preserved, documented in the schema) caps a single filter call. It is still clamped by the remaining retrieval deadline, so a larger value cannot extend the overall retrieval window.

Reporter expectation ① (force-disabling the filter via algorithm.lightweightMemory) is intentionally left untouched — the lightweight-mode override of llmFilterEnabled is the documented lightweight-mode contract (issue #2063).

Regression tests cover: budget-timeout classification (red: llm_unavailable, green: llm_timeout), configurable budget, legacy default, deadline clamping, and the caller-cancellation distinction.

Related Issue (Required): Fixes #2499

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Unit Test
  • Test Script Or Test Steps (please provide)

Red first, on unpatched main @ a7367d0 with only the new tests applied: the timeout-classification test fails with exactly the reported symptom (llm_unavailable, "This operation was aborted"); the budget-config test fails with 2000 ≠ 4321. All green after the fix — run from apps/memos-local-plugin/ on macOS arm64, Node 24.21.0:

  • npx vitest run tests/unit/llm/fetcher.test.ts tests/unit/retrieval/integration.test.ts → 41 passed
  • npm run test:unit → 1594 passed / 2 skipped / 0 failed
  • npm test (release gate: unit + integration + e2e) → 186 files, 1602 passed / 3 skipped / 0 failed
  • npm run lint (tsc --noEmit) → clean, exit 0

Reproduce from apps/memos-local-plugin/:

npx vitest run tests/unit/llm/fetcher.test.ts tests/unit/retrieval/integration.test.ts   # includes the #2499 regression tests
npm run test:unit
npm test
npm run lint

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 linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR(如果适用)

(The MemOS-Docs checkbox is not applicable: the new option is documented in-repo via the algorithm.retrieval schema description in core/config/schema.ts. The "mentioned the person who will review" checkbox is left unchecked as no reviewer has been assigned yet.)

AI disclosure

This fix was developed with AI assistance (an AI coding agent in an automated contributor pipeline): code change, regression tests, and this PR description. All checks listed above were executed locally; the result was reviewed before submission.

…e budget configurable

Two stacked defects made local OpenAI-compatible endpoints unusable for
the retrieval LLM filter (issue MemTensor#2499): every turn_start fired a doomed
local call that was misreported as llm_unavailable and replayed on the
fallback (host) provider.

- core/llm/fetcher.ts: mergeSignals forwarded aborts without a reason,
  flattening AbortSignal.timeout()'s TimeoutError into a bare AbortError
  whenever the caller passed its own signal (the retrieval filter always
  does); isTimeout could not recognize it either — DOMException.code is
  the numeric 20, so the legacy "ABORT_ERR" string comparison never
  matched. Forward the abort reason and classify AbortError / code 20 as
  a timeout (caller cancellation is handled before this classification).
- core/retrieval/retrieve.ts: the filter wall-clock budget was hard-coded
  at 2000 ms. Add algorithm.retrieval.llmFilterTimeoutMs (schema +
  defaults, default 2000 preserves current behavior) and clamp the filter
  budget with it, so self-hosted endpoints can opt into a larger window.

Regression tests: budget-timeout classification (red: llm_unavailable,
green: llm_timeout), configurable budget, legacy default, deadline
clamping, and caller-cancellation distinction.

Fixes MemTensor#2499
…n fetcher

Tighten isTimeout so a bare AbortError is only treated as a timeout when
its reason is a TimeoutError (i.e. from AbortSignal.timeout()); a caller
ctrl.abort() racing past the opts.signal?.aborted guard is no longer
misclassified. In mergeSignals, prefer the TimeoutError reason when both
signals are already aborted so downstream classification stays accurate.

Addresses review feedback on PR MemTensor#2506.
@Memtensor-AI Memtensor-AI added area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Nov 9, 2026
@syzsunshine219
syzsunshine219 marked this pull request as ready for review November 9, 2026 15:48
@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2594
Task: 1dd2ddd855c75bd2
Base: main
Head: fix/plugin-llmfilter-timeout-budget
Head SHA: 9e0b217f79c26ae1384b34d2bc70e8b6be8f67bd

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

⚠️ Review warnings (2)

  • 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. apps/memos-local-plugin/core/retrieval/retrieve.ts (L695-L696)

When deadlineAt is undefined the function returns undefined and the newly added llmFilterTimeoutMs is ignored entirely, so the LLM call falls back to the provider's default request timeout (client.ts: opts?.timeoutMs ?? config.timeoutMs, e.g. 45 s). The doc in types.ts states "Default 2000 (2 s)" without any caveat, and an operator lowering/raising llmFilterTimeoutMs will see no effect on no-deadline retrievals. Either apply the cap when there is no deadline (return Math.max(1, capMs ?? DEFAULT_FILTER_TIMEOUT_MS)) or document in types.ts that the value only takes effect when a retrieval deadline is set.


2. apps/memos-local-plugin/core/retrieval/retrieve.ts (L697)

This budget computation now lives in two places: here and finalFilterMergedHits in core/pipeline/memory-core.ts (~line 923), which still clamps with the hard-coded FINAL_HUB_LLM_FILTER_TIMEOUT_MS / DEADLINE_FILTER_SAFE_CUTOFF_MS and never reads config.llmFilterTimeoutMs. As a result, the new option is silently ignored for the merged local+hub filter path, so a slow self-hosted model still gets aborted there. Consider exporting this helper (or a shared timeout resolver) and reusing it in memory-core.ts so all LLM-filter call sites honour the same config.


3. apps/memos-local-plugin/core/llm/fetcher.ts (L344-L349)

Two problems with this block, both of which undercut the classification fix:

  1. The reason lookup is unreachable: fetch (undici) rejects with signal.reason itself, so a timeout surfaces as the TimeoutError DOMException (handled two lines above) and a caller abort surfaces as a bare AbortError. A DOMException has no reason property, so err.reason is always undefined here. The test helper at tests/unit/llm/fetcher.test.ts:92 documents the same semantic.
  2. code === 20 contradicts the comment right above: 20 is the legacy numeric ABORT_ERR code (TIMEOUT_ERR is 23), and Node's DOMException for a reasonless ctrl.abort() has exactly name === "AbortError", code === 20. So a plain caller cancellation still returns true from isTimeout(). Today httpPostJson short-circuits caller aborts earlier (if (opts.signal?.aborted)), but this helper's contract is now self-contradictory and will misclassify as soon as it is reused.

Suggest dropping the dead reason probe and the numeric 20 (or using 23 if a numeric timeout code is really needed).

💡 Suggested Change

Before:

    if (err.name === "AbortError") {
      const reason: unknown = (err as { reason?: unknown }).reason;
      if (reason instanceof Error && reason.name === "TimeoutError") return true;
    }
    const code: unknown = (err as { code?: unknown }).code;
    if (code === "ABORT_ERR" || code === 20) return true;

After:

    // `fetch` rejects with the signal's *reason*, so a timeout arrives as a
    // `TimeoutError` DOMException (handled above) and a caller abort as a bare
    // `AbortError` with no `reason`; the latter must not count as a timeout.
    const code: unknown = (err as { code?: unknown }).code;
    if (code === "ABORT_ERR") return true;

4. apps/memos-local-plugin/core/llm/fetcher.ts (L392-L396)

The nested ternary violates the style guide, and the reason-selection logic does not do what the comment promises: only a is probed for TimeoutError. When both signals are already aborted and a carries a non-timeout reason while b (the AbortSignal.timeout() signal) carries TimeoutError, this forwards a.reason and the budget timeout is again flattened — the exact misclassification this block is meant to prevent. Extract a small helper and check both signals (also note core/embedding/fetcher.ts:mergeSignals still has the original flattening bug and could share the fix).

💡 Suggested Change

Before:

  const immediateReason =
    (a.aborted && a.reason instanceof Error && a.reason.name === "TimeoutError")
      ? a.reason
      : a.aborted ? a.reason : b.reason;
  if (a.aborted || b.aborted) ctrl.abort(immediateReason);

After:

  const timeoutReason = (signal: AbortSignal): unknown =>
    signal.aborted && (signal.reason as { name?: string } | undefined)?.name === "TimeoutError"
      ? signal.reason
      : undefined;
  if (a.aborted || b.aborted) {
    ctrl.abort(timeoutReason(a) ?? timeoutReason(b) ?? (a.aborted ? a.reason : b.reason));
  }

5. apps/memos-local-plugin/core/llm/fetcher.ts (L397-L398)

This hand-rolled signal merging is duplicated in core/embedding/fetcher.ts (same helper, still flattening the reason and still treating ABORT_ERR as transient), so the two fetchers now diverge on identical abort semantics. Consider extracting a shared helper (e.g. core/util/abort.ts) or replacing it with AbortSignal.any([b, a]), which the repo already relies on (core/hub/client.ts, core/pipeline/orchestrator.ts) and which forwards the source reason while handling listener bookkeeping itself. The current wiring also leaks: the listener on a is only removed when a itself aborts, so a long-lived caller signal accumulates one listener per retry attempt for every completed request, and is additionally registered pointlessly when a.aborted is already true.

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Executor error: Node dependency preparation failed for memos_local_plugin: Command failed with exit code 1: npm ci --include=dev --no-audit --no-fund
stdout:
stderr: npm warn deprecated boolean@3.2.0: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
npm error code 1
npm error path /data/test-workspaces/1dd2ddd855c75bd2/repo/apps/memos-local-plugin/node_modules/onnxruntime-node
npm error command failed
npm error command sh -c node ./script/install
npm error /data/test-workspaces/1dd2ddd855c75bd2/repo/apps/memos-local-plugin/node_modules/onnxruntime-node/script/install-utils.js:57
npm error reject(new Error(Failed to download build list. HTTP status code = ${statusCode}));
npm error ^
npm error
npm error Error: Failed to download build list. HTTP status code = 302
npm error at ClientRequest. (/data/test-workspaces/1dd2ddd855c75bd2/repo/apps/memos-local-plugin/node_modules/onnxruntime-node/script/install-utils.js:57:18)
npm error at Object.onceWrapper (node:events:634:26)
npm error at ClientRequest.emit (node:events:519:28)
npm error at ClientRequest.emit (node:domain:489:12)
npm error at HTTPParser.parserOnIncomingClient (node:_http_client:780:27)
npm error at HTTPParser.parserOnHeadersComplete (node:_http_common:125:17)
npm error at TLSSocket.socketOnData (node:_http_client:615:22)
npm error at TLSSocket.emit (node:events:519:28)
npm error at TLSSocket.emit (node:domain:489:12)
npm error at addChunk (node:internal/streams/readable:561:12)
npm error
npm error Node.js v22.22.2
npm error A complete log of this run can be found in: /data/test-workspaces/_shared/npm-downloads/_logs/2026-11-09T16_34_27_558Z-debug-0.log
Branch: fix/plugin-llmfilter-timeout-budget

Tested source: 9e0b217f79c26ae1384b34d2bc70e8b6be8f67bd
Tested base: 3a16e5fe3048a209f722806f4123fa2e533588fd
Tested commit: 7bcc8d144274bd3fa6f5e9f4786f099b965585e5
These results apply only to this immutable source/base snapshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

轻量模式强制打开检索用的 LLM 过滤,它自己的 2 秒上限让本地模型每轮都被中止,然后静默改用宿主模型

4 participants