feat: partition the routing index by LoRA adapter so cache hints can't alias#159
Merged
Merged
Conversation
The content fingerprint is derived from token IDs alone, so two requests with identical tokens under different LoRA adapters produce the same prefix_hash and collided on one index entry — a LookupRoute for adapter A could be handed a replica holding only adapter B's KV. Runtime LoRA load/unload makes that ordinary rather than theoretical. Partition the index, not the hash. The key becomes (tenant, model, hash_scheme, adapter, prefix_hash); pkg/fingerprint and its golden vectors are untouched. - proto: adapter_id on LookupRouteRequest (11), LookupRouteResponse (5), PrefixEntry (5), CacheStateUpdate (8), CacheEvent (7). Additive only. - subscriber: decode BlockStored.lora_id (it was already on the wire and ignored), resolve it to a stable identity via --lora-adapter-names, and stamp it per entry; PREFIX_EVICTED names its partition so one adapter's eviction no longer drops another's identical hash. - server: thread adapter through ingest and lookup; echo the partition on hint-bearing responses. Empty/unset adapter is the default partition and is exactly the prior behavior, so non-LoRA deployments are unaffected. The (tenant, model, hash_scheme) scope deliberately stays adapter-free: it answers a replica membership question, so UNKNOWN_HASH_SCHEME keeps its meaning.
Codex reviewFiles reviewed
FindingsBlockingNone. Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
Review follow-ups on the LoRA index-partition change. Only a NIL lora_id (base model) uses the default "" partition; ANY non-nil lora_id — including a single-adapter deployment — resolves to a configured name or the lora:<id> fallback, never "". So a single-LoRA deployment does not "just work": the subscriber's ingest identifier and the gateway's LookupRoute query identifier must agree end-to-end or every lookup silently misses. Corrected the wording in three places that conflated "single-adapter LoRA" with "LoRA-free": - --lora-adapter-names flag help (cmd/kvevent-subscriber/main.go) - Config.AdapterNames doc (pkg/adapters/engine/config.go) - the compat claim in docs/design/grpc-contract.md Also hardened the two negative adapter probes in the install-smoke script: they only rejected PREFIX_MATCH, so a grpcurl failure or empty response falsely passed. They now require the RPC to succeed with a non-empty body AND assert the expected fail-open reason_code (AFFINITY_HINT) — an RPC error is now a red, not a green. Dropped the stats from the seed ingest so the fail-open code is deterministically AFFINITY_HINT rather than possibly TENANT_HOT.
Codex reviewFiles reviewed
FindingsBlockingNone. Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
Two more review follow-ups on the LoRA index-partition change. 1. proto/inferencecache/v1alpha1/inferencecache.proto — the adapter_id (11) comment still carried the "single-adapter deployment uses the empty partition and is unaffected" claim that was corrected everywhere else, contradicting grpc-contract.md/config.go/main.go. Applied the same correction: only a nil lora_id uses the "" partition; any LoRA adapter, even one, resolves to a name or lora:<id> and needs end-to-end identifier agreement. Regenerated the .pb.go (the comment flows into generated code; proto-gen stays idempotent) and gave the request/response adapter_id fields an inline description in grpc-contract.md to match. 2. pkg/index/index.go — the tenant-quota eviction tie-break added adapter but still tied on keys differing by model or hashScheme, leaving the victim dependent on map iteration order (contradicting its own comment). It now breaks age ties on the full remaining key (model, hashScheme, adapter, prefixHash), mirroring the cap-sweep comparator. Added a test that pins deterministic victim selection across each of those fields (64 fresh index instances per field so an incomplete comparator flakes here).
Codex reviewFiles reviewed
FindingsBlocking
Should-fixNone. Nit
Per-category coverage
Verdictchanges-requested |
Name the two gaps the startup flag can't close — runtime-loaded adapters, and the managed (webhook-injected) sidecar — as tracked follow-ups, so the docs stop implying they're solved by "supply the map" / "set it explicitly". Correct the index.go partition comment: the default partition is the non-LoRA case, not "single-adapter".
Codex reviewFiles reviewed
FindingsBlocking
Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
An adapter-aware producer emits adapter="" for a genuine base-model eviction, so the "" cross-partition sweep can also drop live LoRA-partition hints for the same prefix hash. Conservative soft state (re-added by ReportCacheState) and a strict non-regression, but the comment shouldn't frame "" as only the pre-adapter case. Precise fix (adapter_id presence on the event) is a tracked follow-up.
Codex reviewFiles reviewed
FindingsBlocking
Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
Reconcile the adapter index partition (this branch) with #154 (T1/T2 tier tagging from the block lifecycle) and #156 (subscriber stats). Key resolutions: - proto PrefixEntry: keep both new fields — main's CacheTier tier = 5, and move this branch's adapter_id from 5 to the next free number, 6. All other adapter_id sites (11/5/8/7) were conflict-free. pb.go regenerated. - eviction path (positional.go/forwarder.go): each evicted prefix now carries BOTH its adapter partition (scope the drop) AND its token count (re-report at T2 when a paired L2 tier retains it). The T2 downgrade re-report is stamped with the entry's adapter so it lands in the right partition. - BlockStored entries stamp adapter_id AND Tier T1; subscriber keeps both the --ignore-block-removed (now T2-aware) and --lora-adapter-names flags. - docs (grpc-contract.md): PrefixEntry documents tier=5 + adapter_id=6. Full build, go test ./..., vet, gofmt, and buf lint all pass.
Codex reviewFiles reviewed
FindingsBlockingNone. Should-fix
Nit
Per-category coverage
Verdictchanges-requested |
…files) Post-merge consistency, all in files this PR already changes: - grpc-contract.md: the adapter-partition section said PrefixEntry.adapter_id is field 5; the merge moved it to 6 (5 is now tier). Correct the number. - grpc-contract.md + proto PrefixEntry.tier comment: the tier last-write-wins identity now includes adapter_id (partitioned key), not (…, prefix_hash) alone. - index.go: prefixesByTenant + Aggregate comments count a distinct key that now includes adapter. Cross-doc key drift in files this PR does not touch stays a tracked follow-up. pb.go regenerated (comment mirror only, no wire change); go test ./..., vet, gofmt, buf lint pass.
Codex reviewFiles reviewed
FindingsBlocking
Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
An unmapped non-nil lora_id previously fell back to a replica-local "lora:<id>" partition, which could place different adapters in one partition across replicas whose --lora-modules order differs — the cross-replica alias the partition exists to prevent. Now AdapterID reports ok=false for an unmapped id and the Reporter drops the BlockStored (warned once per id) rather than indexing it. The adapter gets no routing hint — a cache miss, never a wrong replica — until --lora-adapter-names maps it, so LoRA caching now REQUIRES the map. Base-model (nil lora_id) traffic is unchanged. Docs (config/flag/proto/grpc-contract/wiring) and tests updated; new TestReporterFailsClosedOnUnmappedAdapter. go test ./..., vet, gofmt, buf lint pass.
Codex reviewFiles reviewed
FindingsBlockingNone. Should-fix
NitNone. Per-category coverage
Verdictchanges-requested |
heymrbox
approved these changes
Jul 23, 2026
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
2 participants
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-181
Problem
The content fingerprint (
pkg/fingerprint, XXH3-64 / seed 1337) is derived from token IDs alone. Two requests with identical tokens under different LoRA adapters therefore produce the sameprefix_hash/block_hashesand collided on a single index entry — aLookupRoutefor adapter A could be handed a replica that only holds adapter B's KV for those tokens.SMG v1.8.0's runtime LoRA load/unload/list (adapters swap under a live engine, no restart) makes that ordinary rather than theoretical. vLLM's
BlockStoredevent has always carriedlora_idas its trailing field; the subscriber decoded throughblock_sizeand dropped it.Approach
Partition the index, not the hash. The index key becomes
(tenant_id, model_id, hash_scheme, adapter_id, prefix_hash)—adapter_idjoins the existing partition ahead of the content hash.pkg/fingerprintand its golden vectors are untouched (make verify-golden-vectorsis part of the gate that passed). The hash still answers "what content is this?"; the partition answers "whose KV is it?". Matching inside a partition — exact-match and the block-chain longest-leading-run rule — is byte-for-byte unchanged.Wire changes (all additive vs
main, v1alpha1-compatible)LookupRouteRequeststring adapter_id = 11— partition to look up inLookupRouteResponsestring adapter_id = 5— echo of the partition consultedPrefixEntrystring adapter_id = 6— per-entry, because one replica holds KV for several adapters at once (field 5 isCacheTier tier, from the merged #154)CacheStateUpdatestring adapter_id = 8— per-update default for entries that set noneCacheEventstring adapter_id = 7— narrows aPREFIX_EVICTEDto one partitionThis branch merged
main(which addedCacheTier tier = 5toPrefixEntryin #154 and the T1/T2 tier-tagging path), soPrefixEntry.adapter_idlands at 6. All fields are additive vsmain— no removals. Protos regenerated withmake proto-gen(verified idempotent, no drift);make proto-lintclean.Adapter identity is a stable string, not the engine's integer
vLLM's
lora_idis an internal load-order integer, so the same integer can mean different adapters on two replicas whose--lora-modulesorder differs — and the index is shared across replicas. Partitioning on the raw int would re-create the aliasing across replicas instead of within one.The subscriber maps it via
--lora-adapter-names "1=sql-lora,2=chat-lora"to the same string the gateway sends asadapter_id. An unmapped non-nillora_idis fail-closed: its blocks are dropped (not cached) rather than indexed under a replica-locallora:<id>that could alias different adapters across replicas — so LoRA caching requires the map. An unmapped adapter gets no routing hint (a cache miss, never a wrong replica) until it is named; the drop is logged once per id. An undecodablelora_idfails the batch rather than decoding to "no adapter": mislabelling an adapter's blocks as base-model would put them in the default partition, which is the exact bug being fixed.Scope & follow-ups
The
--lora-adapter-namesmap is resolved once at startup, so this PR covers adapters known at startup. Two cases are deliberately out of scope, tracked in CAC-243:--lora-adapter-nameson a container it doesn't define, so the flag is self-managed-subscriber-scoped until a managed config surface (aCacheBackendfield the webhook forwards) lands.Neither is a correctness regression — a dropped/unmapped adapter gets no hint (a miss, never a wrong replica). Cross-doc key-sync in docs this PR doesn't touch is tracked in CAC-245.
Backwards compatibility
Empty/unset adapter is the default partition and is exactly the prior behavior — a LoRA-free deployment ingests and looks up under
""and is completely unaffected, with nothing to configure. Older producers and older gateways both land there. (This is not a "single LoRA adapter just works" claim: only a nillora_iduses""; any non-nil id must be mapped, or it is fail-closed.)Producer and consumer must agree on the identifier (a lookup with an adapter will not match entries ingested without one, and vice versa) — the same rule
hash_schemealready imposes, and documented as such.PREFIX_EVICTEDwithout an adapter keeps the original conservative cross-partition sweep. With one it drops only that partition, so a single adapter's GPU eviction no longer discards another adapter's still-valid hint for the identical (token-derived) hash.Deliberately NOT partitioned: the
(tenant, model, hash_scheme)scopescopeKeydoes not gainadapter_id. It answers a replica-membership question, and the surfaces reading it — the distinguishing-power denominator, theTENANT_HOTserving check, theAFFINITY_HINTcandidate set, and theUNKNOWN_HASH_SCHEMEclassifier — carry no per-adapter cache-content claim (TENANT_HOT/AFFINITY_HINTshipmatched_tokens=0by contract). Adapters also load and unload under a live engine, so adapter residency is a property of individual KV entries, not of a replica's membership in an engine domain.Consequence, pinned by a test: a lookup for an unseen adapter in a known engine domain is a novel-prefix miss (
NO_HINT/AFFINITY_HINT), notUNKNOWN_HASH_SCHEME— that code keeps meaning "wronghash_scheme".Tests
pkg/index— different adapters + same tokens do not alias; same adapter still hits (with unchangedmatched_tokens); absent adapter is legacy behavior in both directions; per-entry adapter overrides the update-level default; the chain path partitions identically; scoped vs. unscoped eviction;ALL_CLEAREDstays adapter-agnostic; the miss classifier is unaffected.pkg/adapters/engine—lora_iddecodes (present / explicit nil / absent 5-field tuple / undecodable → error);AdapterIDreturnsok=falsefor an unmapped id andParseAdapterNames;Storedstamps the adapter while leaving the fingerprint identical; identical tokens under two adapters share a hash but not a partition; Reporter end-to-end across a mixed-adapter batch (mapped + base forwarded, unmapped fail-closed / dropped), incl. the partition-scoped eviction; a non-LoRA run stays entirely in the default partition.pkg/server— full round trip (engine event carryinglora_id→ subscriber →ReportCacheState→ index partition →LookupRoute): same adapterPREFIX_MATCHwith the response echoingadapter_id, a different adapter and an absent adapter both off the prefix-match path on the identical hash; plus a no-adapter regression test.adapter_id→PREFIX_MATCHwhile a different and an absentadapter_idstay off the prefix-match path. No engine traffic needed.Validation
make cigreen (naming, internal-refs, fmt, vet, golangci-lint, Prometheus rules, golden vectors, race tests, build) — also via the pre-push hook.make proto-genidempotent;make proto-lintclean. Fullgo test ./...passes after themainmerge.Docs
docs/design/grpc-contract.mdgets the new fields plus an "Update — adapter (LoRA) index partition" section, and its standing caveat ("prefixes that differ only by LoRA adapter share a key … tracked as a follow-up") is now resolved.docs/design/kvevent-subscriber-wiring.mddocuments--lora-adapter-names, the fail-closed behavior, and a "Limitations of the static flag" section (runtime loads + managed injection → CAC-243).