From 91eebf934028b81c159c8c9b7888a5062d869f78 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 16 Jul 2026 14:20:32 +0800 Subject: [PATCH 1/3] fix(keys): prevent infinite loop in KeyIndex:add() when key_count falls behind occupied slots key_count is an ordinary shared-dict node: it is only refreshed when a new key is registered, so under steady traffic it goes cold and, once the dict is full, gets LRU-evicted while higher-numbered slots survive. sync() then reads it as 0 (and incr() re-creates it at 1), so add() keeps calling dict:add() on the same occupied slot: "exists" -> retry -> "exists", forever, without yielding. The worker spins at 100% CPU with the shared-dict lock hot, independent of traffic, until restart. Advance key_count past the occupied slot on a repeated collision so the next sync() adopts the slot's occupant and progress resumes. The first collision keeps the old plain retry, preserving the benign concurrent add race behavior. Repair work per call is bounded and degrades to the existing dict-full error path; counter progress is shared, so later calls converge. Ref: apache/apisix#12275 --- prometheus_keys.lua | 32 +++++++++++++++++++++ prometheus_test.lua | 69 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index fc7c053..7f7a18d 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -7,6 +7,13 @@ local KeyIndex = {} KeyIndex.__index = KeyIndex +-- Upper bound on how far a single add() call may advance key_count while it +-- repairs a counter that has fallen behind occupied slots (see the comment in +-- add()). It bounds the worst-case latency of one call; the advanced counter +-- is shared through the dict, so later calls resume where this one stopped +-- and the index converges even when far more slots need repairing. +local MAX_KEY_COUNT_REPAIRS = 1000 + -- check and remove expired keys local function remove_expired_keys(_, self) @@ -123,6 +130,8 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) end for _, key in pairs(keys) do + local retried = false + local repairs = 0 while true do local N = self:sync() if self.index[key] ~= nil then @@ -178,6 +187,29 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) elseif err ~= "exists" then return "Unexpected error adding a key: " .. err end + + -- "exists": slot N is already occupied although key_count reported N-1. + -- Once per key this can be a benign race with another worker that has + -- created slot N but not incremented key_count yet, so retry and let + -- sync() pick the new slot up. If it repeats, key_count has fallen + -- behind the occupied slots: it is an ordinary shared-dict node, so on + -- a full dict it can be LRU-evicted (it is only refreshed when new keys + -- are registered, so it goes cold under steady traffic) and incr() then + -- re-creates it at 1, far below the surviving slots. Retrying the same + -- slot forever would spin the worker at 100% CPU with the shared-dict + -- lock held hot (apache/apisix#12275). Advance the counter past the + -- occupied slot instead: the next sync() adopts that slot's occupant + -- and progress resumes. + if retried then + self.dict:incr(self.key_count, 1, 0) + repairs = repairs + 1 + if repairs >= MAX_KEY_COUNT_REPAIRS then + return (err_msg_lru_eviction .. "; key index: key_count fell " .. + "behind occupied slots; advanced it by " .. repairs .. + " without finding a free slot, dropping key: " .. key) + end + end + retried = true end end end diff --git a/prometheus_test.lua b/prometheus_test.lua index 814b7a8..1f0c13a 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -24,6 +24,10 @@ function SimpleDict:add(k, v, exptime) if k == "willnotfitk" or v == "willnotfitv" then forcible = true end + self:get(k) -- prunes the key if it has expired, like ngx.shared.DICT does + if self.dict and self.dict[k] then + return false, "exists", false -- match ngx.shared.DICT:add on present keys + end self:set(k, v, exptime) return true, nil, forcible -- ok, err, forcible end @@ -877,6 +881,71 @@ function TestKeyIndex:testListDeduplicatesStaleSlot() luaunit.assertEquals(keys[1], "dup") end +-- Regression test for apache/apisix#12275 (workers pinned at 100% CPU). +-- key_count is an ordinary shared-dict node: it is only refreshed when a new +-- key is registered, so under steady traffic it goes cold and, once the dict +-- is full, gets LRU-evicted while higher-numbered slots survive. sync() then +-- reads it as 0 and add() keeps calling dict:add() on the same occupied slot: +-- "exists" -> retry -> "exists", forever, without yielding. Before the fix +-- this call never returned and the worker spun at 100% CPU; the fix advances +-- key_count past occupied slots so add() terminates and repairs the counter. +function TestKeyIndex:testAddAfterKeyCountEvicted() + for i = 1, 50 do + self.key_index:add("key" .. i, "eviction_err") + end + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 50) + + -- the shared dict force-evicts its LRU-tail node, which is key_count + self.dict:delete("_prefix_key_count") + + local err = self.key_index:add("newkey", "eviction_err") + luaunit.assertEquals(err, nil) + luaunit.assertEquals(self.dict:get("_prefix_key_51"), "newkey") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 51) + luaunit.assertEquals(self.key_index.index["newkey"], 51) +end + +-- Same eviction, seen from a freshly started worker (e.g. respawned after a +-- crash, or after nginx reload) whose local index is empty: it must adopt the +-- surviving slots while repairing the counter instead of spinning. +function TestKeyIndex:testAddAfterKeyCountEvictedFreshWorker() + for i = 1, 50 do + self.key_index:add("key" .. i, "eviction_err") + end + self.dict:delete("_prefix_key_count") + + local worker2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) + local err = worker2:add("newkey", "eviction_err") + luaunit.assertEquals(err, nil) + luaunit.assertEquals(self.dict:get("_prefix_key_51"), "newkey") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 51) + -- surviving slots were adopted into the local index along the way + luaunit.assertEquals(worker2.index["key7"], 7) + luaunit.assertEquals(worker2.index["newkey"], 51) +end + +-- One add() call bounds its repair work (MAX_KEY_COUNT_REPAIRS = 1000) and +-- degrades by dropping the sample, like the existing dict-full path. The +-- advanced counter is shared, so a later call resumes and converges. +function TestKeyIndex:testKeyCountRepairIsBounded() + local keys = {} + for i = 1, 1500 do + keys[i] = "key" .. i + end + self.key_index:add(keys, "eviction_err") + self.dict:delete("_prefix_key_count") + + local err = self.key_index:add("newkey", "eviction_err") + luaunit.assertStrContains(err, "key_count fell behind occupied slots") + -- bounded progress was made and persisted for the next call + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1000) + + err = self.key_index:add("newkey", "eviction_err") + luaunit.assertEquals(err, nil) + luaunit.assertEquals(self.dict:get("_prefix_key_1501"), "newkey") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1501) +end + function TestKeyIndex:testSync() self.key_index:sync() luaunit.assertEquals(ngx.logs, nil) From 6ab2eb6a8fefc29f5556c7b0e41b90168e566635 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 16 Jul 2026 14:41:49 +0800 Subject: [PATCH 2/3] fix(keys): fail fast on repair incr errors and surface forcible evictions Address review feedback: the repair increment's error/forcible results were ignored. A hard incr failure now returns immediately (mirroring the add() error path) instead of consuming the repair budget, and a forcible re-creation of key_count is surfaced through the existing LRU-eviction warning. The repair cap still counts attempts, not successes: it exists to guarantee the loop terminates. --- prometheus_keys.lua | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 7f7a18d..cf2561d 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -132,6 +132,7 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) for _, key in pairs(keys) do local retried = false local repairs = 0 + local repair_forcible = false while true do local N = self:sync() if self.index[key] ~= nil then @@ -179,7 +180,7 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) if exptime and exptime > 0 then self.expire_keys[N] = true end - if forcible or forcible2 then + if forcible or forcible2 or repair_forcible then return (err_msg_lru_eviction .. "; key index: add key: idx=" .. self.key_prefix .. N .. ", key=" .. key) end @@ -201,7 +202,21 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) -- occupied slot instead: the next sync() adopts that slot's occupant -- and progress resumes. if retried then - self.dict:incr(self.key_count, 1, 0) + local _, incr_err, forcible3 = self.dict:incr(self.key_count, 1, 0) + if incr_err then + -- hard failure (e.g. "no memory"): give up immediately, mirroring + -- the add() error path above, instead of burning the repair budget + -- on retries that cannot succeed. + return "Unexpected error advancing key_count: " .. incr_err + end + if forcible3 then + -- re-creating an evicted key_count displaced another entry; surface + -- it through the LRU-eviction warning on the success path, like + -- forcible/forcible2. + repair_forcible = true + end + -- The cap counts attempts, not successes: it exists to guarantee the + -- loop terminates. repairs = repairs + 1 if repairs >= MAX_KEY_COUNT_REPAIRS then return (err_msg_lru_eviction .. "; key index: key_count fell " .. From 71cd6bc1f688df4e2bc01e1ad423de3b92c8895f Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 16 Jul 2026 16:55:32 +0800 Subject: [PATCH 3/3] fix(keys): report forcible repair evictions on the existing-key exit path repair_forcible was only checked on the new-slot success path: when the repair crawl adopts the requested key from an occupied slot, add() exited through the existing-key break and silently swallowed the fact that the repair increments had forcibly displaced other entries. Return the LRU-eviction error there too, mirroring forcible/forcible2, with a regression test covering the adopted-key path. --- prometheus_keys.lua | 8 ++++++++ prometheus_test.lua | 28 +++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index cf2561d..0c5f102 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -168,6 +168,14 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) end end if not expired then + if repair_forcible then + -- the key was adopted from an occupied slot after repair + -- increments that forcibly displaced other entries; report the + -- eviction just like the new-slot success path does. + return (err_msg_lru_eviction .. "; key index: adopted key after " .. + "key_count repair: idx=" .. self.key_prefix .. self.index[key] .. + ", key=" .. key) + end break end end diff --git a/prometheus_test.lua b/prometheus_test.lua index 1f0c13a..edb2017 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -33,7 +33,8 @@ function SimpleDict:add(k, v, exptime) end function SimpleDict:incr(k, v, init) local forcible = false - if k == "willnotfitk" or v == "willnotfitv" then + if k == "willnotfitk" or v == "willnotfitv" or + (self.forcible_keys and self.forcible_keys[k]) then forcible = true end if not self.dict[k] then self.dict[k] = {value = init} end @@ -946,6 +947,31 @@ function TestKeyIndex:testKeyCountRepairIsBounded() luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1501) end +-- If the repair increments forcibly displaced other entries and the requested +-- key then turns out to already exist (it gets adopted from an occupied slot +-- during the repair crawl), the eviction must still be reported through the +-- LRU-eviction error, exactly like the new-slot success path reports +-- forcible/forcible2. +function TestKeyIndex:testKeyCountRepairReportsForcibleEviction() + for i = 1, 50 do + self.key_index:add("key" .. i, "eviction_err") + end + self.dict:delete("_prefix_key_count") + -- re-creating/incrementing key_count now displaces other entries + self.dict.forcible_keys = {["_prefix_key_count"] = true} + + -- a worker with an empty local index registers a key that already exists in + -- the dict at a slot ahead of the (rewound) counter: the repair crawl adopts + -- it and exits through the existing-key break. + local worker2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) + local err = worker2:add("key30", "eviction_err") + luaunit.assertStrContains(err, + "eviction_err; key index: adopted key after key_count repair: " .. + "idx=_prefix_key_30, key=key30") + luaunit.assertEquals(worker2.index["key30"], 30) + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 30) +end + function TestKeyIndex:testSync() self.key_index:sync() luaunit.assertEquals(ngx.logs, nil)