nic-6443 commented on code in PR #13632:
URL: https://github.com/apache/apisix/pull/13632#discussion_r3503009133
##########
apisix/plugins/ai-cache.lua:
##########
@@ -126,16 +159,35 @@ function _M.access(conf, ctx)
ctx.ai_cache_status = "MISS"
return
end
- release(conf, red)
-
if res ~= nil and res ~= ngx_null then
local cached = core.json.decode(res)
if cached and cached.body then
+ release(conf, red)
return serve_hit(conf, ctx, cached)
end
core.log.warn("ai-cache: discarding malformed cache entry for ",
ctx.ai_cache_key)
end
+ -- L1 miss: attempt an L2 semantic lookup when the layer is configured.
+ -- `red` is still open and reused inside lookup() for the L1 backfill SET.
+ -- pcall ensures ANY throw (e.g. FFI pack failure on malformed embedding)
stays
+ -- fail-open and never leaks the connection or 500s the request.
+ if has_layer(conf, "semantic") and conf.semantic then
+ local ok, hit = pcall(semantic.lookup, red, conf, ctx, body)
Review Comment:
On an L1 miss `red` is still checked out from the keepalive pool here, but
`semantic.lookup` calls `embed()` first — an outbound HTTP request to the
embedding provider, bounded by `timeout` (default 5s) — and only touches `red`
afterwards for `ensure_index`/`knn_search`. So a pooled Redis connection stays
pinned for the whole embedding round-trip; under load that drains
`redis_keepalive_pool` and forces a fresh TCP connection to Redis on every miss.
`embed()` doesn't need `red`, so computing the embedding before reusing the
connection (or releasing/re-acquiring around it) would avoid holding the pool
slot across the network call. What do you think?
##########
apisix/plugins/ai-cache/vector-search/redis.lua:
##########
@@ -0,0 +1,123 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local ffi = require("ffi")
+local ffi_new = ffi.new
+local ffi_str = ffi.string
+local tonumber = tonumber
+local ipairs = ipairs
+local type = type
+
+local _M = {}
+
+local ensured = {}
+
+-- little-endian FLOAT32 blob (RediSearch VECTOR PARAMS / HSET value)
+function _M.pack_float32(vec)
+ local n = #vec
+ local buf = ffi_new("float[?]", n)
+ for i = 1, n do
+ buf[i - 1] = vec[i]
+ end
+ return ffi_str(buf, n * 4)
+end
+
+
+function _M.ensure_index(red, index, prefix, dim)
+ if ensured[index] then
Review Comment:
`ensured` is keyed only by the index name, not by the Redis target. If two
ai-cache instances both keep `index` at the default `ai-cache` with the same
embedding model/dim but different `redis_host`, they collapse to the same key
here — so once `FT.CREATE` has run against one host, a request routed to the
other skips creation. The read path self-heals (an `FT.SEARCH` error resets
`ensured[index]`), but a write landing in that window persists an unindexed
HASH until the reset happens.
It does converge, so this is minor, but keying `ensured` by
host+port+db+index would make it correct regardless of shared index names. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
