Skip to content

feat(model-vector): dedupe reject-phrase embedding across workers via a required store - #2514

Merged
jfallows merged 3 commits into
developfrom
feature/model-vector-store-backed-reject-embedding
Sep 3, 2026
Merged

feat(model-vector): dedupe reject-phrase embedding across workers via a required store#2514
jfallows merged 3 commits into
developfrom
feature/model-vector-store-backed-reject-embedding

Conversation

@jfallows

@jfallows jfallows commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • VectorModelHandlerImpl is a per-worker factory that eagerly embeds every configured reject phrase in its constructor, once per attach. Since every namespace binding is replicated to and attached independently on every EngineWorker, an N-worker engine fires N identical, redundant embed() calls against the configured embedding provider for the exact same input, all at once, at attach time.
  • Add a required store reference on the vector model config (a new StoredConfig, alongside VaultedConfig/GuardedConfig/CatalogedConfig/EmbeddedConfig) so a worker checks a shared cache first, takes a short-lived lock to embed for real only on a cache miss, and every other worker (or, with a distributed store, replica) polls the cache with capped backoff until the winner's result lands. If the lock owner fails without writing a result, the lock simply expires and the next poll wins it instead. A failed embed is never cached, so a transient error doesn't permanently poison the result.
  • store is required, not optional: model-vector is incubator-only and has never shipped in a release, so there's no backward-compatibility reason to make the dedup opt-in.

Test plan

  • config/engine.conf, incubator/model-vector.conf, incubator/model-vector.spec, incubator/model-vector all green (./mvnw clean verify), plus a full ./mvnw clean install -DskipTests reactor build to confirm nothing else regressed from the new shared StoredConfig class or VectorModelConfig's changed constructor.
  • Unit tests in VectorModelHandlerImplTest simulate two workers sharing one store (as store-memory shares its backing map across every EngineWorker in one process): only one of them ever calls EmbeddingHandler.embed(), the loser picks up the winner's cached result via its own retry/backoff, and a failed embed releases the lock for the next attempt without caching the failure.
  • VectorModelIT scenarios load a real zilla.yaml referencing a type: test store end-to-end through the engine's own config-loading/name-resolution path, not just direct unit construction.
  • SchemaTest confirms the fixture config validates now that store is a required property.

jfallows and others added 2 commits September 2, 2026 23:15
… an optional store

VectorModelHandlerImpl is a per-worker factory that eagerly embeds every
configured reject phrase in its constructor, once per attach. Since every
namespace binding is replicated to and attached independently on every
EngineWorker, an N-worker engine fires N identical, redundant embed() calls
against the configured embedding provider for the exact same input, all at
once, at attach time -- unnecessary load against whatever service backs the
embedding, worst case a synchronized burst against a provider with limited
concurrency.

Add an optional `store` reference (a new StoredConfig, alongside VaultedConfig/
GuardedConfig/CatalogedConfig/EmbeddedConfig) so a worker can check a shared
cache before embedding, take a short-lived lock to embed for real only when
the cache is empty, and have every other worker (or, with a distributed store,
replica) poll the cache with capped backoff until the winner's result lands --
including recovering if the lock owner fails without ever writing a result,
since the lock simply expires and the next poll wins it instead. Configuring
no store preserves the current per-worker-independent behavior exactly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
model-vector is incubator-only and has never shipped in a release, so there
is no backward-compatibility reason to keep the store reference optional.
Require it in the schema, drop the now-unreachable no-store code path from
VectorModelHandlerImpl, and update every fixture/test that predates the
property to configure one -- consolidating the config/spec IT fixtures back
down to a single reject.yaml now that every scenario needs a store anyway.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jfallows jfallows changed the title feat(model-vector): dedupe reject-phrase embedding across workers via an optional store feat(model-vector): dedupe reject-phrase embedding across workers via a required store Sep 3, 2026
…property

store became a required vector model property in this same PR; this example
predates it and never got one, so its zilla.yaml no longer validated --
confirmed as the tcp.echo.embedding CI failure. Add a memory store (in-process
only, matching this example's single-instance, no-external-dependency scope)
and reference it from the model.

Verified locally: the stack becomes healthy and all three test.sh scenarios
(accepted message, two differently-worded rejections) pass against a
freshly built zilla:develop-SNAPSHOT image.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jfallows
jfallows merged commit 96cfd71 into develop Sep 3, 2026
84 checks passed
@jfallows
jfallows deleted the feature/model-vector-store-backed-reject-embedding branch September 3, 2026 23:54
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