Repository navigation
fix(memory): measure the embedding width instead of waiting for a probe - #12180
diegosouzapw merged 6 commits into
Conversation
3c289e1 to
56513ba
Compare
|
Retargeted from |
|
Heads-up @ntdatt812 — I retargeted this from Because the branch was cut from
— while the other 24 are Could you re-derive the branch from the current git remote add upstream https://github.com/diegosouzapw/OmniRoute.git
git fetch upstream
git switch -c fix/12154-embedding-lazy-probe upstream/release/v3.8.51
git checkout <your-branch> -- src/lib/memory/embedding/index.ts src/lib/memory/reindex.ts src/lib/memory/store.ts tests/unit/memory-vec-lazy-probe-12154.test.tsThe fix itself looks right and #12154 is a genuine bug (memories stored but never vectorized, while |
|
CI is red on this PR and neither failure is the diff. Evidence rather than a re-run request. Build — not a build failure at all: The same thing hit #12177 in the same window, and Vitest — one test, The mechanism looks like a fixed-sleep race rather than luck. The helper is: async function flushQueuedSync() {
await new Promise<void>((resolve) => setTimeout(resolve, 10));
await Promise.resolve();
}The hook has to complete If it is worth fixing rather than re-running, the smallest change is to wait on the condition instead of the clock: await vi.waitFor(() =>
expect(fetchMock).toHaveBeenCalledWith("/api/providers/connection-1/sync-models?mode=sync", expect.anything()),
);which keeps the negative test above it honest — that one asserts the call is never made, so it still needs a bounded wait rather than a condition. Happy to send it as its own PR if you would like; it is unrelated to this branch and should not ride on it. This branch's own suite, |
resolveEmbeddingSource() reports dimensions: null for any source the
hard-coded registry does not describe, and a self-hosted endpoint is by
definition absent from it. Both write paths then deadlocked on that null:
- scheduleVectorUpsert called ensureReady() with the null resolution, which
declines to create vec_memories, and then ignored the {ready:false} answer
and upserted anyway -- straight into the catch, so every memory was stored,
marked needs_reindex, and never vectorized;
- reindexPending refused to embed until the width was known, and the width
could only ever come from an embedding.
Nothing surfaced it: POST /api/memory returned 200 and the health check
stayed green while rowCount stayed at 0.
The comment on EmbeddingResolution.dimensions already calls this a lazy
probe; nobody performed the probe. The upsert path holds a finished vector
when it calls ensureReady, so measure it there, and let reindex spend one
embedding up front to measure -- reusing that vector rather than paying for
it twice. withMeasuredDimensions rebuilds the signature the same way the
resolution did, identity first, so two endpoints serving the same model id
still reindex independently.
scheduleVectorUpsert now also honours a {ready:false} answer instead of
upserting into a table that is not there.
Fixes diegosouzapw#12154
56513ba to
4436689
Compare
…green runReindexBatch grew past max-lines-per-function and cognitive-complexity when the lazy-probe path landed. Split measure/ready/item helpers without changing the diegosouzapw#12154 behavior. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
|
Babysit (/sweep-reds round 4) — not merging. Did not merge Reds before on Own complexity (this PR's diff): already extracted in Remaining Fast Quality Gates hit is inherited, not this diff: That file is not in this PR. The 0→1 is Units 3/4 + 4/4 + ESLint are inherited (drain in #12327 / #12331):
Local: HOLD until the base-red drain lands. HEAD remains |
9327990
into
diegosouzapw:release/v3.8.51
|
Merged into |
|
Merging with |
…be (diegosouzapw#12180) * fix(memory): measure the embedding width instead of waiting for a probe resolveEmbeddingSource() reports dimensions: null for any source the hard-coded registry does not describe, and a self-hosted endpoint is by definition absent from it. Both write paths then deadlocked on that null: - scheduleVectorUpsert called ensureReady() with the null resolution, which declines to create vec_memories, and then ignored the {ready:false} answer and upserted anyway -- straight into the catch, so every memory was stored, marked needs_reindex, and never vectorized; - reindexPending refused to embed until the width was known, and the width could only ever come from an embedding. Nothing surfaced it: POST /api/memory returned 200 and the health check stayed green while rowCount stayed at 0. The comment on EmbeddingResolution.dimensions already calls this a lazy probe; nobody performed the probe. The upsert path holds a finished vector when it calls ensureReady, so measure it there, and let reindex spend one embedding up front to measure -- reusing that vector rather than paying for it twice. withMeasuredDimensions rebuilds the signature the same way the resolution did, identity first, so two endpoints serving the same model id still reindex independently. scheduleVectorUpsert now also honours a {ready:false} answer instead of upserting into a table that is not there. Fixes diegosouzapw#12154 * chore(changelog): point the fragment at the real PR number * fix(memory): extract reindex helpers so the complexity ratchet stays green runReindexBatch grew past max-lines-per-function and cognitive-complexity when the lazy-probe path landed. Split measure/ready/item helpers without changing the diegosouzapw#12154 behavior. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
2 participants
Fixes #12154. Your diagnosis is right, and tracing it showed the two paths are deadlocked on the same null in opposite ways.
A lazy probe nobody performs
EmbeddingResolution.dimensionsis documented as "null antes da 1ª chamada (lazy probe)" — null until the first call. Nothing ever performs that call:scheduleVectorUpsert(store.ts)ensureReady()with the null resolution — which declines to createvec_memories— then ignored the{ready:false}answer and upserted anyway, straight into thecatch. Memory stored,needs_reindexset, never vectorized.runReindexBatch(reindex.ts)ensureReady()before embedding, so it returned{processed:0, errors:0}and the queue never drained.So the width could only come from an embedding, and neither path would embed until it had the width. That is why your instances A and B sat at
rowCount: 0with 24 pending, and why C only worked from a signature an older build had already written. Nothing surfaced it because the upsert is fire-and-forget:POST /api/memorystill returns 200 and health still reportsworking: true.The fix: measure it
withMeasuredDimensions(resolution, width)fills the pending null in from a vector that has actually come back, and rebuilds the signature the way the resolution built it — identity first, then model — so two endpoints serving the same model id still get separate signatures and reindex independently. A width the registry already supplied is never overwritten, and a nonsense measurement (0, negative, fractional,NaN) is ignored rather than written into a signature.store.tsalready holds a finished vector when it callsensureReady, so it just passesvector.length. It now also honours a{ready:false}answer instead of upserting into a table that is not there.reindex.tsspends one embedding up front to measure, and reuses that vector for its own item rather than paying for it twice.Registry-described models are unaffected: their
dimensionsis already non-null, so the helper returns the resolution unchanged and neither path changes behaviour.Tests
tests/unit/memory-vec-lazy-probe-12154.test.ts, 7 tests: the pending null is filled; the signature keeps the endpoint identity and gains the width; a resolution without an identity signs by model; a known width is never overwritten; four kinds of nonsense measurement are ignored; a source-less resolution stays unusable; and two endpoints serving the same model id do not collide.Commands run
Changed test files:
tests/unit/memory-vec-lazy-probe-12154.test.ts(new).The unit tests cover the resolution/signature logic, which is where the null originated. I have not run a self-hosted TEI server against this build, so the end-to-end claim — that
vec_memoriesis now created androwCountclimbs — is reasoned from the two call sites above rather than observed. If you have one of the affected instances handy, that is the check worth doing before merge.