Skip to content

fix: bound embeddings probe by a configurable timeout - #38

Open
jinserk wants to merge 1 commit into
jayzeng:mainfrom
jinserk:fix/embed-probe-timeout
Open

jinserk wants to merge 1 commit into
jayzeng:mainfrom
jinserk:fix/embed-probe-timeout

Conversation

@jinserk

@jinserk jinserk commented Sep 5, 2026

Copy link
Copy Markdown

Summary

memory_status reports Embeddings: ? unknown on setups where qmd, the models, and semantic search are all working fine. The embeddings are not the problem — the readiness probe is.

probeEmbeddings() races qmd vsearch against a hardcoded 4_000ms timer. That probe is not cheap: it runs LLM query expansion plus an embed and rerank pass. Measured on a working install (n=12, warm, idle):

min 2.35s   median 2.91s   max 3.55s

Only ~0.45s of headroom. Writes schedule a background qmd update/qmd embed, which competes for CPU and the embedding model, so the probe crosses 4s exactly when a write just happened — which is when users are most likely to run memory_status.

Reproduced against real qmd with a concurrent re-index:

old hardcoded 4s   -> unknown  4005ms
new default 15s    -> ready    6048ms

Two things compound it:

  1. The catch maps any failure to "unknown", so a slow probe is indistinguishable from a broken one. The status output implies embeddings are at fault when they are healthy.
  2. PI_MEMORY_QMD_SEARCH_TIMEOUT_MS does not apply. Real searches honor it (runQmdSearch), but the probe's literal 4_000 overrides it — so the knob that looks like it should fix this has no effect.

Changes

  • add DEFAULT_EMBED_PROBE_TIMEOUT_MS (15s) and getEmbedProbeTimeoutMs(), overridable via PI_MEMORY_EMBED_PROBE_TIMEOUT_MS
  • pass the probe budget into runQmdSearch via an optional timeout override, so an abandoned probe cannot leave a 60s LLM query running
  • surface the effective timeout in the unknown hint and in the memory_status configuration block
  • document the variable in README.md

Alternative considered

Parsing qmd status instead — no LLM, ~14x faster (0.21s vs 2.91s), and it reports vector counts directly. Rejected: its Vectors: N embedded count is index-global, not per-collection, so it would report ready from another collection's embeddings while pi-memory had none. qmd status also has no --json mode, making the parse fragile across versions. Correctness over speed.

Happy to switch if you would prefer the faster path and consider that tradeoff acceptable.

Tests

8 new cases in test/unit.test.ts: default floor, env override, invalid values, ready/missing detection, child-process bounding, and a genuine timeout still yielding unknown.

Verified red/green per AGENTS.md — reverting only the probe change fails 3 of the new tests:

(fail) survives a slow probe that would trip the old hardcoded 4s race
(fail) bounds the qmd child process by the probe timeout, not the search timeout
(fail) still reports unknown when the probe genuinely times out

Commands run:

bun test test/unit.test.ts   190 pass, 0 fail   (baseline before changes: 182 pass, 0 fail)
npm run build                clean
npm run lint                 clean

Compatibility

No change to on-disk memory formats. qmd invocation is unchanged apart from the child timeout value. Default behavior only widens the probe budget; "missing" detection is untouched, so the existing ensureQmdEmbed() self-heal path still fires for genuinely missing embeddings.

`memory_status` reported `Embeddings: ? unknown` on healthy setups where
qmd, the models, and semantic search all worked correctly.

`probeEmbeddings()` raced `qmd vsearch` against a hardcoded 4000ms timer.
That probe is not cheap: it runs LLM query expansion plus an embed and
rerank pass. Measured latency on a working install (n=12, warm, idle):

    min 2.35s   median 2.91s   max 3.55s

That leaves only ~0.45s of headroom. Writes schedule a background
`qmd update`/`qmd embed`, which competes for CPU and the embedding model,
so the probe crosses 4s exactly when a write just happened - which is
when users are most likely to run `memory_status`. Reproduced against
real qmd with a concurrent re-index:

    old hardcoded 4s   -> unknown  4005ms
    new default 15s    -> ready    6048ms

The `catch` maps any failure to "unknown", so a slow probe was
indistinguishable from a broken one and the status output implied the
embeddings were at fault when they were fine.

Also note `PI_MEMORY_QMD_SEARCH_TIMEOUT_MS` did not apply here: real
searches honor it, but the probe's literal `4_000` overrode it, so the
knob that looked like it should fix this had no effect.

Changes:
- add DEFAULT_EMBED_PROBE_TIMEOUT_MS (15s) and getEmbedProbeTimeoutMs(),
  overridable via PI_MEMORY_EMBED_PROBE_TIMEOUT_MS
- pass the probe budget to runQmdSearch via an optional timeout override
  so an abandoned probe cannot leave a 60s LLM query running
- surface the effective timeout in the "unknown" hint and in the
  memory_status configuration block
- document the variable in README.md

Considered parsing `qmd status` instead, since it needs no LLM and is
~14x faster (0.21s vs 2.91s). Rejected: its `Vectors: N embedded` count
is index-global, not per-collection, so it would report "ready" from
another collection's embeddings while pi-memory had none. `qmd status`
also has no --json mode, making the parse fragile. Correctness over speed.

Tests: 8 new cases covering the default floor, env override, invalid
values, ready/missing detection, child-process bounding, and a genuine
timeout still yielding "unknown". Verified red/green: reverting only the
probe change fails 3 of them.

  bun test test/unit.test.ts   190 pass, 0 fail (was 182 pass)
  npm run build                clean
  npm run lint                 clean

No change to on-disk memory formats. qmd invocation is unchanged apart
from the child timeout value.

Signed-off-by: Jinserk Baik <jvby@novonordisk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant