feat(index): scope base-model evictions via a CacheEvent adapter_scoped flag#164
Open
EdHasNoLife wants to merge 4 commits into
Open
feat(index): scope base-model evictions via a CacheEvent adapter_scoped flag#164EdHasNoLife wants to merge 4 commits into
EdHasNoLife wants to merge 4 commits into
Conversation
CacheEvent.adapter_id was a plain proto3 string, so an empty value meant both a legacy producer (sweep every partition) and could not express an adapter-aware producer evicting the base partition. A base-model eviction was thus treated as the cross-partition wildcard and dropped live LoRA hints for the same token hash. Make adapter_id an optional string (proto3 presence) and branch on presence, not emptiness: present (even empty) narrows the removal to that one partition; absent keeps the conservative legacy sweep. The subscriber always sets it; the server carries presence into the index via ev.AdapterId != nil. Wire-compatible (old producers read as absent). New tests at the index and the full PublishEvent wire-to-index seam; grpc-contract.md updated to match.
Codex reviewFiles revieweddocs/design/
pkg/adapters/engine/
pkg/index/
pkg/server/
pkg/server/proto/inferencecache/v1alpha1/
proto/inferencecache/v1alpha1/
FindingsBlockingNone. Should-fix
Nit
Per-category coverage
Verdictchanges-requested |
buf breaking rejects changing CacheEvent.adapter_id from proto3 implicit to explicit presence (a cardinality change), and the repo gates on it. Carry the presence signal in a new, purely additive bool adapter_scoped = 8 instead: adapter_scoped=true means adapter_id is authoritative — the removal targets exactly that partition, including "" for a base-model eviction — while false (legacy producers) keeps the conservative cross-partition sweep. Index-side semantics (Event.AdapterSet) and all tests are unchanged; only the wire signal moves from a pointer to the bool. Also fixes the SpareLoRA->SparesLoRA test name and the grpc-contract PREFIX_EVICTED prose. buf breaking/lint, go test ./... pass.
Codex reviewFiles reviewed
FindingsBlockingNone. Should-fixNone. Nit
Per-category coverage
Verdictapprove-with-nits |
Codex nits: the index-side field and comments called the eviction-scope signal 'proto3 presence', but the landed wire contract is the independent adapter_scoped bool. Rename index.Event.AdapterSet -> AdapterScoped and reword the comments + the test name so the two mechanisms aren't conflated. No behavior change; go test ./... , buf lint/breaking pass.
Codex reviewFiles reviewed
FindingsBlocking
Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
Fix a mixed-version regression: a pre-adapter_scoped producer (new server, not- yet-upgraded subscriber) sends adapter_id="sql-lora" without adapter_scoped, and the new server would wildcard-sweep it — wiping another adapter's live hint. Scope the removal when adapter_scoped is set OR adapter_id is non-empty; only an empty adapter_id with adapter_scoped unset takes the legacy cross-partition sweep. New TestAdapterPartitionEvictionNonEmptyAdapterScopesWithoutFlag; proto/contract comments updated. go test ./..., buf lint/breaking pass.
EdHasNoLife
changed the title
Codex reviewFiles reviewed
FindingsBlockingNone. Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linear: CAC-243 (part 1 of the epic — the eviction over-sweep)
Problem
The index partitions cache entries by adapter, but
PREFIX_EVICTEDcould not tell a base-model eviction apart from a legacy sweep.CacheEvent.adapter_id=""was ambiguous — it meant both "no adapter field → sweep every partition" and could not express "adapter-aware producer evicting the base ("") partition." So an adapter-aware subscriber's base-model eviction was treated as the cross-partition wildcard, dropping live LoRA-partition hints for the same token-derived hash. Strict non-regression and self-healing viaReportCacheState, but it undercut adapter-precise eviction.Fix — an additive
adapter_scopedflagAdd a new
bool adapter_scoped = 8toCacheEvent(adapter_idstays a plainstring = 7). The server decides the removal scope as:adapter_scopedadapter_id"""sql-lora""sql-lora"adapter_scopedproducer's LoRA eviction keeps narrowing on a mixed-version fleet""i.e. scope when
adapter_scoped || adapter_id != ""; sweep only when both are empty/false.kvevent-subscriber) always setsadapter_scopedon the evictions it emits (post fail-closed it always knows the adapter).inferencecache_service) carries the flag intoindex.Event.AdapterScoped; the index'sApplyEventapplies the table above.optional adapter_id: makingadapter_ida proto3optionalchanges its cardinality (implicit→explicit presence), whichbuf breakingrejects and this repo gates on. A new bool is purely additive.Tests
pkg/index— base eviction withadapter_scopedspares a co-resident LoRA hint (the over-sweep this fixes); a non-emptyadapter_idwithout the flag still narrows (upgrade compat); a fully empty/unset eviction still sweeps (legacy); scoped-to-its-adapter; ALL_CLEARED stays adapter-agnostic.pkg/adapters/engine—EvictedEventsetsadapter_scopedfor both base and a named adapter.pkg/server— full wire→index seam throughPublishEvent: a base-scoped eviction removes only the base partition, LoRA untouched.Scope
First of the CAC-243 sub-parts (the self-contained eviction correctness fix). Runtime adapter-name resolution and the managed-sidecar config surface remain separate follow-ups.
docs/design/grpc-contract.mdupdated to match the proto;make proto-genidempotent,proto-lint+buf breakingclean, fullgo test ./...green.