diff --git a/packages/memory-router/CHANGELOG.md b/packages/memory-router/CHANGELOG.md index 8319bb8..475edbe 100644 --- a/packages/memory-router/CHANGELOG.md +++ b/packages/memory-router/CHANGELOG.md @@ -6,6 +6,10 @@ based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- New `MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS` env var (b1bbbf68), a hook-only embed-timeout override that takes precedence over the shared `MEMORY_ROUTER_EMBED_TIMEOUT_MS` added in 0.6.0 (#96). Reviewer follow-up from that PR: `MEMORY_ROUTER_EMBED_TIMEOUT_MS` overrides both the hook's confidence-gate path (5 s default) and `memory-router index`'s rebuild path (60 s default) at once, so a persistent shell-profile export meant to give `index` more headroom also raises the hook's per-prompt budget, which must never block a prompt for long. Precedence for the hook's embed call (`semanticSearch` in `src/embed/indexer.ts`) is now `MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS` > `MEMORY_ROUTER_EMBED_TIMEOUT_MS` > the 5 s default; the index-rebuild path (`rebuildIndex`) is unchanged and never reads the new var. Same guard semantics as the existing var (integer, `> 0`, `<= 2147483647`; an unset, empty, non-numeric, zero, negative, fractional, or out-of-range value falls through to the next tier). `src/embed/provider.ts` gained `resolveHookEmbedTimeoutMs()` and a shared `parseTimeoutOverride()` helper factored out of the existing `resolveEmbedTimeoutMs()` (same external behavior, no test changes needed for the pre-existing function). New tests in `tests/unit/embed-provider.test.ts` (unit-level precedence and the same 11-value invalid table as PR #96, doubled: once falling back to the shared knob, once falling back to the 5 s default) and `tests/embed-timeout-budget.test.ts` (end-to-end through `indexer.ts`: hook override affects only `semanticSearch`, precedence over the shared knob, and both invalid-fallback tiers). README "Timeout budgets" section and the `MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS` override entry updated with the new precedence order and a shell-profile-safe recipe for widening `index`'s budget without also widening the hook's. + ### Changed - The un-overridden `minSemanticScore` default is now model/provider-conditional instead of a flat 0.5: Ollama with bge-m3 resolves to the calibrated 0.78, any other Ollama model falls back to 0.78 at the provider level, and OpenAI keeps 0.5. An explicit `MEMORY_ROUTER_BLEND_MIN_SEMANTIC` still wins on every path; setting it to `0.5` reproduces the pre-upgrade flat-default behavior exactly, on every provider, for an operator who needs to roll back this change without reverting the release. Model names are normalized for the lookup (trim, lowercase, tag stripped), so `bge-m3:latest` matches the `bge-m3` entry. Upgrade cost, measured on the bge-m3 reference corpus (295 memories, no env overrides, flat-0.5 baseline vs. the new conditional default): precision 0.300 -> 0.250 and recall 0.578 -> 0.484, and `semanticContributedCount` (how many of the golden set's prompts had the semantic signal actually win a slot) 20/20 -> 14/20 — the floor now also screens out some genuine borderline matches along with the noise it was added for, not a free win. In exchange, negative controls go from 0/4 to 4/4 blocked and MRR barely moves, 0.7313 -> 0.7104. This is an independent measurement from the mm-v1-T008 calibration table under "Calibration" below (295 memories and a differently-sized golden set here vs. 289 memories/16 positive prompts there); its post-upgrade P/R/MRR land close to that table's floor-0.78 row by coincidence of the underlying cosine distribution, not because it is the same run, so do not read the two as corroborating each other. diff --git a/packages/memory-router/README.md b/packages/memory-router/README.md index 57f6120..e25262f 100644 --- a/packages/memory-router/README.md +++ b/packages/memory-router/README.md @@ -483,6 +483,7 @@ Overrides: - `OPENAI_BASE_URL`: OpenAI-compatible proxy base URL (OpenAI path only). - `MEMORY_ROUTER_OLLAMA_BASE_URL`: Ollama base URL, default `http://localhost:11434`. Ollama is queried through its OpenAI-compatible `/v1/embeddings` endpoint, unauthenticated. - `MEMORY_ROUTER_EMBED_TIMEOUT_MS`: per-request timeout override, applies to both the hook's confidence-gate path (default `5000`) and `memory-router index`'s rebuild path (default `60000`); an unset, empty, non-numeric, zero, negative, fractional, or larger-than-`2147483647` value falls back to that path's own default rather than erroring (the upper bound is Node's 32-bit timer limit; beyond it `AbortSignal.timeout` either throws or silently degrades to a 1 ms timer). +- `MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS`: timeout override for every `semanticSearch` query-embedding call, precedence over `MEMORY_ROUTER_EMBED_TIMEOUT_MS` above. That is every caller of `semanticSearch`: the UserPromptSubmit hook, the MCP `memory-search` tool, the eval runner, and the public `semanticSearch` export from `src/index.ts`. Only `memory-router index`'s rebuild path (`rebuildIndex`) never reads it. Same validation as `MEMORY_ROUTER_EMBED_TIMEOUT_MS` (an invalid value falls back to the next tier, not to an error); see "Timeout budgets" below for why this exists and the full precedence order. Model-variable precedence: @@ -496,7 +497,9 @@ Embedding dimensionality is never hardcoded: it's read off the first real embed Local Ollama setup: `ollama pull nomic-embed-text`, then run `ollama serve` (or use the app) before `memory-router index`/normal hook usage. -Timeout budgets: the hook's confidence-gate embed call defaults to a tight 5 s (it must never block a prompt for long) while `memory-router index`'s rebuild defaults to a much more generous 60 s per batch, because a real 64-input Ollama batch on the mm-v1-T008 reference corpus measured roughly 3.5-10 s warm and 11-17 s for the first batch after a cold model load (reliably the slowest), which used to blow past the old shared 5 s budget and make `index` unusable. `MEMORY_ROUTER_EMBED_TIMEOUT_MS` overrides both defaults at once. Beware what that coupling means: a value exported persistently (shell profile) to give `index` more headroom also raises the hook's per-prompt budget to the same value, and the hook must never block a prompt for long. Prefer the per-invocation form, `MEMORY_ROUTER_EMBED_TIMEOUT_MS=120000 memory-router index `, which widens the index budget without touching the hook. +Timeout budgets: every `semanticSearch` query-embedding call (the hook's confidence-gate path, the MCP `memory-search` tool, the eval runner) defaults to a tight 5 s (it must never block a prompt for long) while `memory-router index`'s rebuild defaults to a much more generous 60 s per batch, because a real 64-input Ollama batch on the mm-v1-T008 reference corpus measured roughly 3.5-10 s warm and 11-17 s for the first batch after a cold model load (reliably the slowest), which used to blow past the old shared 5 s budget and make `index` unusable. `MEMORY_ROUTER_EMBED_TIMEOUT_MS` overrides both defaults at once, and a value exported persistently (shell profile) to give `index` more headroom also raises the hook's per-prompt budget to the same value, as long as the hook knob below is unset. + +`MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS` decouples that coupling for every `semanticSearch` caller (the hook, the MCP `memory-search` tool, the eval runner, and the public `semanticSearch` export): each such embed call resolves its timeout with precedence `MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS` (hook-specific, wins if set and valid) > `MEMORY_ROUTER_EMBED_TIMEOUT_MS` (shared, still applies when the hook knob is unset or invalid) > `5000` (the hook default). `memory-router index`'s rebuild path (`rebuildIndex`) never reads the hook knob at all, so it stays governed by `MEMORY_ROUTER_EMBED_TIMEOUT_MS` (then its own `60000` default) exactly as before. A persistent `MEMORY_ROUTER_EMBED_TIMEOUT_MS=120000` export for `index` headroom, combined with `MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS=5000` to pin every `semanticSearch` caller back to its tight default, is now the safe way to widen `index`'s budget in a shell profile without also widening theirs. The per-invocation form, `MEMORY_ROUTER_EMBED_TIMEOUT_MS=120000 memory-router index `, remains an option too, since a per-invocation env var never reaches the hook process at all. #### Query-embedding cache diff --git a/packages/memory-router/src/embed/indexer.ts b/packages/memory-router/src/embed/indexer.ts index 5175f19..1d2c0b7 100644 --- a/packages/memory-router/src/embed/indexer.ts +++ b/packages/memory-router/src/embed/indexer.ts @@ -5,6 +5,7 @@ const { embedBatch, resolveProviderConfig, resolveEmbedTimeoutMs, + resolveHookEmbedTimeoutMs, INDEX_DEFAULT_TIMEOUT_MS, } = require('./provider'); const { openIndex } = require('./index-store'); @@ -248,6 +249,13 @@ async function semanticSearch( model: cfg.model, baseUrl: cfg.baseUrl, inputs: [prompt], + // Hook-specific budget: MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS, then + // the shared MEMORY_ROUTER_EMBED_TIMEOUT_MS, then the tight 5s + // hook default. Decoupled from the index-rebuild call above so a + // persistent shared-var export for `index` headroom can't also + // widen this call's prompt-blocking budget — see + // resolveHookEmbedTimeoutMs in provider.ts. + timeoutMs: resolveHookEmbedTimeoutMs(), }); } catch (err) { throw describeEmbedError(err, cfg); diff --git a/packages/memory-router/src/embed/provider.ts b/packages/memory-router/src/embed/provider.ts index acecc4c..cc7bb8d 100644 --- a/packages/memory-router/src/embed/provider.ts +++ b/packages/memory-router/src/embed/provider.ts @@ -34,23 +34,44 @@ const DEFAULT_TIMEOUT_MS = 5000; // case with margin, not just the typical case. const INDEX_DEFAULT_TIMEOUT_MS = 60_000; -// Env override for both DEFAULT_TIMEOUT_MS and INDEX_DEFAULT_TIMEOUT_MS. -// Mirrors src/gates/confidence.ts's recencyHalfLifeDays guard (a -// duration-shaped value must be strictly positive to mean anything) rather -// than that file's envFloat (which allows 0 for a weight/boost, a shape -// where 0 is a meaningful "off"). Unset, empty, non-numeric, zero, and -// negative all fall back to `fallback` unchanged. The value must also be an -// integer no larger than 2147483647: AbortSignal.timeout throws RangeError -// on fractional or > uint32 delays, and Node's 32-bit timer silently -// overflows anything above 2^31-1 to an effective 1 ms budget, so those -// values would defeat the guard's whole purpose on the hook path. -function resolveEmbedTimeoutMs(fallback: number): number { - const raw = process.env.MEMORY_ROUTER_EMBED_TIMEOUT_MS; - if (raw === undefined || raw.trim() === '') return fallback; +// Shared guard for every timeout env var below. Mirrors +// src/gates/confidence.ts's recencyHalfLifeDays guard (a duration-shaped +// value must be strictly positive to mean anything) rather than that +// file's envFloat (which allows 0 for a weight/boost, a shape where 0 is a +// meaningful "off"). Unset, empty, non-numeric, zero, and negative all +// resolve to `undefined` (caller decides the fallback). The value must +// also be an integer no larger than 2147483647: AbortSignal.timeout throws +// RangeError on fractional or > uint32 delays, and Node's 32-bit timer +// silently overflows anything above 2^31-1 to an effective 1 ms budget, so +// those values would defeat the guard's whole purpose on the hook path. +function parseTimeoutOverride(raw: string | undefined): number | undefined { + if (raw === undefined || raw.trim() === '') return undefined; const parsed = Number(raw); return Number.isInteger(parsed) && parsed > 0 && parsed <= 2_147_483_647 ? parsed - : fallback; + : undefined; +} + +// Env override for both DEFAULT_TIMEOUT_MS and INDEX_DEFAULT_TIMEOUT_MS. +function resolveEmbedTimeoutMs(fallback: number): number { + return parseTimeoutOverride(process.env.MEMORY_ROUTER_EMBED_TIMEOUT_MS) ?? fallback; +} + +// Hook-only env override, precedence over the shared +// MEMORY_ROUTER_EMBED_TIMEOUT_MS knob above. b1bbbf68: a persistent +// MEMORY_ROUTER_EMBED_TIMEOUT_MS export (shell profile) meant to give +// `memory-router index` more headroom otherwise also raised the hook's +// per-prompt budget by the same amount, and the hook (UserPromptSubmit) +// must never block a prompt for long — see README "Timeout budgets" for +// the coupling this decouples. Only src/embed/indexer.ts's semanticSearch +// consults this; rebuildIndex (the index-rebuild path) never reads it. +function resolveHookEmbedTimeoutMs(): number { + // Precedence: hook-specific override, then the shared override, then the + // 5s hook default. + return ( + parseTimeoutOverride(process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS) ?? + resolveEmbedTimeoutMs(DEFAULT_TIMEOUT_MS) + ); } async function embedBatch(opts: EmbedOptions): Promise { @@ -204,6 +225,7 @@ module.exports = { embedBatch, resolveProviderConfig, resolveEmbedTimeoutMs, + resolveHookEmbedTimeoutMs, DEFAULT_TIMEOUT_MS, INDEX_DEFAULT_TIMEOUT_MS, }; diff --git a/packages/memory-router/tests/embed-timeout-budget.test.ts b/packages/memory-router/tests/embed-timeout-budget.test.ts index 69f852f..97a476a 100644 --- a/packages/memory-router/tests/embed-timeout-budget.test.ts +++ b/packages/memory-router/tests/embed-timeout-budget.test.ts @@ -90,6 +90,22 @@ async function withEmbedTimeoutEnv( } } +// Same shape as withEmbedTimeoutEnv above, for the hook-specific knob. +async function withHookEmbedTimeoutEnv( + value: string | undefined, + fn: () => Promise, +): Promise { + const orig = process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + if (value === undefined) delete process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + else process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS = value; + try { + return await fn(); + } finally { + if (orig === undefined) delete process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + else process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS = orig; + } +} + // Same "must actually await" requirement as withEmbedTimeoutEnv above. async function withOpenAiKey(fn: () => Promise): Promise { const orig = process.env.OPENAI_API_KEY; @@ -127,27 +143,35 @@ test('semanticSearch: no env override → embedBatch call uses DEFAULT_TIMEOUT_M const dir = tmpMemoryDir(); try { await withOpenAiKey(async () => { - // Build the index first (its own capture window, discarded) so - // semanticSearch finds an index file and actually reaches the - // query-embedding call. Still needs the fetch stub - no live network - // calls anywhere in this file. - await withEmbedTimeoutEnv(undefined, async () => { - await withCapturedTimeouts(async () => { - const first = await rebuildIndex(dir); - assert.ok(first.embedded > 0); + // b1bbbf68 fix-round: also neutralize the hook-only knob, not just + // the shared one. An ambient MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS + // (e.g. exported in a shell profile, exactly as this package's own + // README recipe recommends) would otherwise win the precedence + // check below and make this "no override" test pass or fail + // depending on the operator's shell, not the code. + await withHookEmbedTimeoutEnv(undefined, async () => { + // Build the index first (its own capture window, discarded) so + // semanticSearch finds an index file and actually reaches the + // query-embedding call. Still needs the fetch stub - no live network + // calls anywhere in this file. + await withEmbedTimeoutEnv(undefined, async () => { + await withCapturedTimeouts(async () => { + const first = await rebuildIndex(dir); + assert.ok(first.embedded > 0); + }); }); - }); - await withEmbedTimeoutEnv(undefined, async () => { - const captured = await withCapturedTimeouts(async () => { - const hits = await semanticSearch('a prompt not seen before', [], dir, 5); - assert.deepEqual(hits, []); // empty `memories` arg, see embed-multi-provider.test.ts + await withEmbedTimeoutEnv(undefined, async () => { + const captured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('a prompt not seen before', [], dir, 5); + assert.deepEqual(hits, []); // empty `memories` arg, see embed-multi-provider.test.ts + }); + assert.deepEqual( + captured, + [DEFAULT_TIMEOUT_MS], + 'semanticSearch must keep the tight hook default, not the larger index default', + ); }); - assert.deepEqual( - captured, - [DEFAULT_TIMEOUT_MS], - 'semanticSearch must keep the tight hook default, not the larger index default', - ); }); }); } finally { @@ -159,18 +183,25 @@ test('MEMORY_ROUTER_EMBED_TIMEOUT_MS overrides both rebuildIndex and semanticSea const dir = tmpMemoryDir(); try { await withOpenAiKey(async () => { - await withEmbedTimeoutEnv('7777', async () => { - const rebuildCaptured = await withCapturedTimeouts(async () => { - const result = await rebuildIndex(dir); - assert.ok(result.embedded > 0); - }); - assert.deepEqual(rebuildCaptured, [7777]); + // b1bbbf68 fix-round: same ambient-hook-knob hazard as the test + // above. Without this, an ambient MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS + // would win over the shared knob this test is exercising and the + // semanticSearch assertion below would observe the ambient value + // instead of 7777. + await withHookEmbedTimeoutEnv(undefined, async () => { + await withEmbedTimeoutEnv('7777', async () => { + const rebuildCaptured = await withCapturedTimeouts(async () => { + const result = await rebuildIndex(dir); + assert.ok(result.embedded > 0); + }); + assert.deepEqual(rebuildCaptured, [7777]); - const searchCaptured = await withCapturedTimeouts(async () => { - const hits = await semanticSearch('another new prompt', [], dir, 5); - assert.deepEqual(hits, []); + const searchCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('another new prompt', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual(searchCaptured, [7777]); }); - assert.deepEqual(searchCaptured, [7777]); }); }); } finally { @@ -198,3 +229,180 @@ test('rebuildIndex: an invalid MEMORY_ROUTER_EMBED_TIMEOUT_MS (negative) falls b fs.rmSync(dir, { recursive: true, force: true }); } }); + +// b1bbbf68: MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS decouples the hook +// (semanticSearch) path's embed-timeout budget from the shared +// MEMORY_ROUTER_EMBED_TIMEOUT_MS knob above, which otherwise also raises +// the index-rebuild path's budget any time a caller sets it. Precedence: +// hook knob, then the shared knob, then DEFAULT_TIMEOUT_MS. The +// index-rebuild path must never read the hook knob. + +test('MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS overrides only semanticSearch; rebuildIndex keeps INDEX_DEFAULT_TIMEOUT_MS', async () => { + const dir = tmpMemoryDir(); + try { + await withOpenAiKey(async () => { + await withHookEmbedTimeoutEnv('4242', async () => { + await withEmbedTimeoutEnv(undefined, async () => { + const rebuildCaptured = await withCapturedTimeouts(async () => { + const result = await rebuildIndex(dir); + assert.ok(result.embedded > 0); + }); + assert.deepEqual( + rebuildCaptured, + [INDEX_DEFAULT_TIMEOUT_MS], + 'rebuildIndex must ignore the hook-only knob and keep its own default', + ); + + const searchCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('a prompt not seen before', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual(searchCaptured, [4242]); + }); + }); + }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('precedence: hook knob wins over the shared knob for semanticSearch, but the shared knob still governs rebuildIndex', async () => { + const dir = tmpMemoryDir(); + try { + await withOpenAiKey(async () => { + await withHookEmbedTimeoutEnv('111', async () => { + await withEmbedTimeoutEnv('7777', async () => { + const rebuildCaptured = await withCapturedTimeouts(async () => { + const result = await rebuildIndex(dir); + assert.ok(result.embedded > 0); + }); + assert.deepEqual( + rebuildCaptured, + [7777], + 'rebuildIndex must keep responding to the shared knob, unaffected by the hook knob', + ); + + const searchCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('another new prompt', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual( + searchCaptured, + [111], + 'semanticSearch must prefer the hook knob over the shared knob', + ); + }); + }); + }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('semanticSearch: an invalid MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS (negative) falls back to the shared knob end-to-end, not straight to DEFAULT_TIMEOUT_MS', async () => { + const dir = tmpMemoryDir(); + try { + await withOpenAiKey(async () => { + await withEmbedTimeoutEnv(undefined, async () => { + await withCapturedTimeouts(async () => { + const first = await rebuildIndex(dir); + assert.ok(first.embedded > 0); + }); + }); + + await withHookEmbedTimeoutEnv('-100', async () => { + await withEmbedTimeoutEnv('7777', async () => { + const searchCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('a third new prompt', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual( + searchCaptured, + [7777], + 'an invalid hook knob must fall back to the shared knob, not jump straight to DEFAULT_TIMEOUT_MS', + ); + }); + }); + }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('semanticSearch: an invalid MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS (negative) with no shared knob set falls back to DEFAULT_TIMEOUT_MS end-to-end', async () => { + const dir = tmpMemoryDir(); + try { + await withOpenAiKey(async () => { + await withEmbedTimeoutEnv(undefined, async () => { + await withCapturedTimeouts(async () => { + const first = await rebuildIndex(dir); + assert.ok(first.embedded > 0); + }); + + await withHookEmbedTimeoutEnv('-100', async () => { + const searchCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('a fourth new prompt', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual(searchCaptured, [DEFAULT_TIMEOUT_MS]); + }); + }); + }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +// b1bbbf68 fix-round: an ambient MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS +// (e.g. exported in a shell profile, exactly as the README's own recipe +// recommends) must never leak into a test that means to observe the +// "no hook override" or "shared knob governs" behavior. The two tests +// above learned this the hard way and now neutralize the hook knob +// themselves; this test pins the guard by simulating the ambient value +// directly (a raw env save/restore, not the withHookEmbedTimeoutEnv +// helper, since using the helper here would just prove the helper works, +// not that the neutralization is actually wired into the tests that need +// it) and re-checking both behaviors still hold once neutralized. +test('hermeticity: an ambient MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS does not leak into the no-override or shared-knob semanticSearch behavior', async () => { + const origHook = process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS = '9999'; + const dir = tmpMemoryDir(); + try { + await withOpenAiKey(async () => { + await withHookEmbedTimeoutEnv(undefined, async () => { + await withEmbedTimeoutEnv(undefined, async () => { + await withCapturedTimeouts(async () => { + const first = await rebuildIndex(dir); + assert.ok(first.embedded > 0); + }); + + const noOverrideCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('a fifth new prompt, no override', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual( + noOverrideCaptured, + [DEFAULT_TIMEOUT_MS], + 'an ambient hook knob must not leak into the no-override case once neutralized', + ); + }); + + await withEmbedTimeoutEnv('7777', async () => { + const sharedKnobCaptured = await withCapturedTimeouts(async () => { + const hits = await semanticSearch('a sixth new prompt, shared knob', [], dir, 5); + assert.deepEqual(hits, []); + }); + assert.deepEqual( + sharedKnobCaptured, + [7777], + 'an ambient hook knob must not leak into the shared-knob case once neutralized', + ); + }); + }); + }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + if (origHook === undefined) delete process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + else process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS = origHook; + } +}); diff --git a/packages/memory-router/tests/unit/embed-provider.test.ts b/packages/memory-router/tests/unit/embed-provider.test.ts index a948624..1e220d1 100644 --- a/packages/memory-router/tests/unit/embed-provider.test.ts +++ b/packages/memory-router/tests/unit/embed-provider.test.ts @@ -13,6 +13,7 @@ const { embedBatch, resolveProviderConfig, resolveEmbedTimeoutMs, + resolveHookEmbedTimeoutMs, DEFAULT_TIMEOUT_MS, INDEX_DEFAULT_TIMEOUT_MS, } = require('../../src/embed/provider'); @@ -32,6 +33,19 @@ function withEmbedTimeoutEnv(value: string | undefined, fn: () => T): T { } } +// Same shape as withEmbedTimeoutEnv above, for the hook-specific knob. +function withHookEmbedTimeoutEnv(value: string | undefined, fn: () => T): T { + const orig = process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + if (value === undefined) delete process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + else process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS = value; + try { + return fn(); + } finally { + if (orig === undefined) delete process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS; + else process.env.MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS = orig; + } +} + // ─── embedBatch ────────────────────────────────────────────────────────────── test('embedBatch: ok response — returns sorted embeddings and validates request', async () => { @@ -390,6 +404,81 @@ for (const bad of [ }); } +// ─── resolveHookEmbedTimeoutMs / MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS ──────── +// b1bbbf68: MEMORY_ROUTER_EMBED_TIMEOUT_MS overrides both the hook +// (semanticSearch) and index-rebuild paths at once, so a persistent shell +// export meant to give `index` more headroom also raises the hook's +// per-prompt budget. MEMORY_ROUTER_HOOK_EMBED_TIMEOUT_MS decouples the hook +// path: hook knob, then the shared knob, then the 5s hook default. + +test('resolveHookEmbedTimeoutMs: no env vars set → returns DEFAULT_TIMEOUT_MS', () => { + withHookEmbedTimeoutEnv(undefined, () => { + withEmbedTimeoutEnv(undefined, () => { + assert.equal(resolveHookEmbedTimeoutMs(), DEFAULT_TIMEOUT_MS); + }); + }); +}); + +test('resolveHookEmbedTimeoutMs: only the shared knob set → falls back to it (unchanged pre-existing behavior)', () => { + withHookEmbedTimeoutEnv(undefined, () => { + withEmbedTimeoutEnv('7777', () => { + assert.equal(resolveHookEmbedTimeoutMs(), 7777); + }); + }); +}); + +test('resolveHookEmbedTimeoutMs: hook knob set → wins over both the shared knob and the default (precedence)', () => { + withHookEmbedTimeoutEnv('42', () => { + withEmbedTimeoutEnv('7777', () => { + assert.equal(resolveHookEmbedTimeoutMs(), 42, 'hook knob must win over the shared knob'); + }); + withEmbedTimeoutEnv(undefined, () => { + assert.equal(resolveHookEmbedTimeoutMs(), 42, 'hook knob must win over the default'); + }); + }); +}); + +// b1bbbf68 fix-round: pins the accepted upper boundary. Its rejected +// neighbor '3000000000' is already covered by the invalid table below; +// this confirms parseTimeoutOverride's `<= 2147483647` check is inclusive, +// not off-by-one. +test('resolveHookEmbedTimeoutMs: the upper boundary value 2147483647 is accepted', () => { + withHookEmbedTimeoutEnv('2147483647', () => { + assert.equal(resolveHookEmbedTimeoutMs(), 2147483647); + }); +}); + +// Same invalid-value table as resolveEmbedTimeoutMs above (PR #96). +for (const bad of [ + '', + ' ', + 'not-a-number', + 'NaN', + '-1', + '-500', + '0', + '1500.7', + '5e9', + '3000000000', + 'Infinity', +]) { + test(`resolveHookEmbedTimeoutMs: invalid hook override ${JSON.stringify(bad)}, no shared knob → falls back to DEFAULT_TIMEOUT_MS`, () => { + withHookEmbedTimeoutEnv(bad, () => { + withEmbedTimeoutEnv(undefined, () => { + assert.equal(resolveHookEmbedTimeoutMs(), DEFAULT_TIMEOUT_MS); + }); + }); + }); + + test(`resolveHookEmbedTimeoutMs: invalid hook override ${JSON.stringify(bad)}, shared knob set → falls back to the shared knob, not straight to the default`, () => { + withHookEmbedTimeoutEnv(bad, () => { + withEmbedTimeoutEnv('7777', () => { + assert.equal(resolveHookEmbedTimeoutMs(), 7777); + }); + }); + }); +} + test('embedBatch: MEMORY_ROUTER_EMBED_TIMEOUT_MS overrides the default when timeoutMs is omitted', async () => { const origFetch = (globalThis as { fetch?: typeof fetch }).fetch; const origTimeout = AbortSignal.timeout;