From 57ec06cf98794589532f72ef6bfd89d0eac8bef6 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 22 Jun 2026 11:02:06 +0800 Subject: [PATCH 1/3] fix(keys): avoid duplicate metrics from KeyIndex desync KeyIndex could emit the same key more than once, surfacing as duplicate metrics (apache/apisix#11934). Two issues combine to cause this: 1. KeyIndex:list() iterated the stored key values (self.keys), which can transiently hold the same key value at two different slots when an expired metric is re-added at a new slot before the old slot is reclaimed. The duplicated value was then emitted twice. list() now walks the slots but only emits a key at the slot the index points at (self.index[key] == idx), so each key is listed exactly once while preserving ordering. 2. When add() re-adds a key whose shared-dict entry had already expired, it cleared the local index/keys but did not bump delete_count or clear the dict slot. Other workers therefore never did a full sync and kept the stale slot in their local self.keys, desynchronizing the index. add() now mirrors remove(): it clears the dict slot, increments the local deleted counter and bumps delete_count. Adds a regression test (TestKeyIndex:testExpiredReAddNoDuplicate) that reproduces the duplicate emission before the fix and passes after. --- prometheus_keys.lua | 22 +++++++++++++++++++--- prometheus_test.lua | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 45d8066..18493ae 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -91,9 +91,18 @@ function KeyIndex:list() self:sync() local copy = {} local i = 1 - for _, v in pairs(self.keys) do - copy[i] = v - i = i + 1 + -- Walk the key slots in order, but only emit a key when the slot is the one + -- the index currently points at (self.index[key] == idx). self.keys can + -- transiently hold the same key value in two different slots (e.g. when an + -- expired metric is re-added at a new slot before the old slot is reclaimed); + -- listing the raw self.keys values would emit duplicate metrics. Consulting + -- the index guarantees each key is listed exactly once, at its canonical slot. + for idx = 0, self.last do + local key = self.keys[idx] + if key and self.index[key] == idx then + copy[i] = key + i = i + 1 + end end return copy end @@ -125,6 +134,13 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) self.index[key] = nil self.keys[idx] = nil self.expire_keys[idx] = nil + self.dict:set(self.key_prefix .. idx, nil) + self.deleted = self.deleted + 1 + -- Bump delete_count so other workers do a full sync and reclaim the + -- stale slot. Without this the old slot lingers in their local + -- self.keys while the metric is re-added at a new slot, which + -- desynchronizes the index and causes duplicate metric emission. + self.dict:incr(self.delete_count, 1, 0) expired = true end end diff --git a/prometheus_test.lua b/prometheus_test.lua index 0fbc0b0..b3887a8 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -804,6 +804,44 @@ function TestKeyIndex:testList() luaunit.assertEquals(keys[3], "key3") end +-- Regression test for apache/apisix#11934 (duplicate metrics). +-- A key with an exptime is added and synced (self.last now tracks N). The key +-- then expires in the underlying shared dict without going through remove(), so +-- neither key_count nor delete_count changes and the next sync() is a no-op, +-- leaving self.index still pointing at the now-vanished slot. Re-adding the key +-- therefore takes the "expired" path in add() and allocates a new slot while +-- the stale slot lingers in self.keys. Before the fix, delete_count was not +-- bumped (other workers never re-synced) and list() iterated self.keys, so the +-- same key was emitted twice -> duplicate metrics. +function TestKeyIndex:testExpiredReAddNoDuplicate() + self.key_index:add("expkey", "eviction_err", 1) + self.key_index:sync() + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "expkey") + luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) + luaunit.assertEquals(self.key_index.index["expkey"], 1) + + -- Let the key expire in the underlying shared dict. key_count and + -- delete_count are untouched, so the next sync() inside add() is a no-op and + -- the local index keeps pointing at the (now gone) slot 1. + sleep(2) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), nil) + + -- Re-adding the now-expired key takes the expired branch and allocates slot 2. + self.key_index:add("expkey", "eviction_err", 1) + luaunit.assertEquals(ngx.logs, nil) + luaunit.assertEquals(self.dict:get("_prefix_key_2"), "expkey") + + -- delete_count must have been bumped on the expired re-add path so that + -- other workers do a full sync and drop the stale slot. + luaunit.assertEquals(self.dict:get("_prefix_delete_count"), 1) + + -- list() must report the key exactly once, not twice. + local keys = self.key_index:list() + luaunit.assertEquals(#keys, 1) + luaunit.assertEquals(keys[1], "expkey") +end + function TestKeyIndex:testSync() self.key_index:sync() luaunit.assertEquals(ngx.logs, nil) From 5d442404903ab8bf20d9bb04a37b7dc6296f505b Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 22 Jun 2026 13:51:38 +0800 Subject: [PATCH 2/3] test(keys): assert add() return value instead of ngx.logs Address review: KeyIndex:add() reports failures via its return value, so assert it directly instead of checking the global ngx.logs which can be stale/unrelated. --- prometheus_test.lua | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/prometheus_test.lua b/prometheus_test.lua index b3887a8..2718836 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -814,7 +814,8 @@ end -- bumped (other workers never re-synced) and list() iterated self.keys, so the -- same key was emitted twice -> duplicate metrics. function TestKeyIndex:testExpiredReAddNoDuplicate() - self.key_index:add("expkey", "eviction_err", 1) + local err = self.key_index:add("expkey", "eviction_err", 1) + luaunit.assertEquals(err, nil) self.key_index:sync() luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1) luaunit.assertEquals(self.dict:get("_prefix_key_1"), "expkey") @@ -828,8 +829,8 @@ function TestKeyIndex:testExpiredReAddNoDuplicate() luaunit.assertEquals(self.dict:get("_prefix_key_1"), nil) -- Re-adding the now-expired key takes the expired branch and allocates slot 2. - self.key_index:add("expkey", "eviction_err", 1) - luaunit.assertEquals(ngx.logs, nil) + err = self.key_index:add("expkey", "eviction_err", 1) + luaunit.assertEquals(err, nil) luaunit.assertEquals(self.dict:get("_prefix_key_2"), "expkey") -- delete_count must have been bumped on the expired re-add path so that From 1d0105784e513ecc232233509a081559471aed4b Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 22 Jun 2026 16:29:21 +0800 Subject: [PATCH 3/3] refactor(keys): list() O(live), narrow expired detection, check shdict rc Address review feedback on the duplicate-metrics fix: - list(): iterate self.keys via pairs() instead of scanning 0..self.last. self.last grows monotonically with every add and is never reclaimed, so the slot-range scan was O(total slots ever created) on long-lived, high-churn workers. pairs() keeps it O(live keys) with the same index-based de-duplication. It also avoids a latent miss: sync() can reset self.last to key_count, below the highest populated local slot. - add() expired path: only treat the key as expired when expire() returns err == "not found". A non-"not found" failure no longer deletes a slot that may still be live (which would now also drop the metric); it is logged and the existing slot is kept to avoid creating a duplicate. - add() expired path: check the delete_count incr() return value and propagate eviction/forcible like remove() does, per the shared-dict error-handling convention. Drop the redundant dict:set(slot, nil): when expire() reports "not found" the slot is already gone. - tests: fix SimpleDict:expire to return true on success (matches ngx.shared.DICT); add a second-worker convergence assertion to the regression test; add testListDeduplicatesStaleSlot to isolate the list() guard. --- prometheus_keys.lua | 59 +++++++++++++++++++++++++++------------------ prometheus_test.lua | 34 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 23 deletions(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 18493ae..fc7c053 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -91,15 +91,17 @@ function KeyIndex:list() self:sync() local copy = {} local i = 1 - -- Walk the key slots in order, but only emit a key when the slot is the one - -- the index currently points at (self.index[key] == idx). self.keys can - -- transiently hold the same key value in two different slots (e.g. when an - -- expired metric is re-added at a new slot before the old slot is reclaimed); - -- listing the raw self.keys values would emit duplicate metrics. Consulting - -- the index guarantees each key is listed exactly once, at its canonical slot. - for idx = 0, self.last do - local key = self.keys[idx] - if key and self.index[key] == idx then + -- Emit a key only from the slot the index currently points at + -- (self.index[key] == idx). self.keys can transiently hold the same key value + -- in two different slots (e.g. when an expired metric is re-added at a new + -- slot before the old slot is reclaimed); listing the raw self.keys values + -- would emit duplicate metrics. Consulting the index guarantees each key is + -- listed exactly once, at its canonical slot. Iterating self.keys (not + -- 0..self.last) keeps this O(live keys): self.last grows monotonically with + -- every add and is never reclaimed, so a slot range scan would walk every + -- dead slot ever created on long-lived, high-churn workers. + for idx, key in pairs(self.keys) do + if self.index[key] == idx then copy[i] = key i = i + 1 end @@ -127,21 +129,32 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) -- key already exists, if has exptime, set expire local expired = false if exptime then - local ok = self.dict:expire(self.key_prefix .. self.index[key], exptime) - -- if key has already expired, remove it from the index + local ok, err = self.dict:expire(self.key_prefix .. self.index[key], exptime) if not ok then - local idx = self.index[key] - self.index[key] = nil - self.keys[idx] = nil - self.expire_keys[idx] = nil - self.dict:set(self.key_prefix .. idx, nil) - self.deleted = self.deleted + 1 - -- Bump delete_count so other workers do a full sync and reclaim the - -- stale slot. Without this the old slot lingers in their local - -- self.keys while the metric is re-added at a new slot, which - -- desynchronizes the index and causes duplicate metric emission. - self.dict:incr(self.delete_count, 1, 0) - expired = true + if err == "not found" then + -- The slot already expired in the shared dict. Drop the stale + -- local state and bump delete_count so other workers do a full + -- sync and reclaim the slot; without this the old slot lingers in + -- their local self.keys while the metric is re-added at a new slot, + -- desynchronizing the index and causing duplicate metric emission. + -- The dict slot is already gone (expire returned "not found"), so + -- there is no slot to clear here. + local idx = self.index[key] + self.index[key] = nil + self.keys[idx] = nil + self.expire_keys[idx] = nil + self.deleted = self.deleted + 1 + local _, incr_err, forcible = self.dict:incr(self.delete_count, 1, 0) + if incr_err or forcible then + return incr_err or err_msg_lru_eviction + end + expired = true + else + -- Unexpected expire error: the slot may still be live, so leave it + -- as-is rather than re-adding it, which would create a duplicate. + ngx.log(ngx.ERR, "failed to renew expire for key '", key, "': ", + tostring(err)) + end end end if not expired then diff --git a/prometheus_test.lua b/prometheus_test.lua index 2718836..814b7a8 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -58,6 +58,7 @@ function SimpleDict:expire(k, exptime) return nil, "not found" end self.dict[k]["expired"] = os.time() + exptime + return true -- match ngx.shared.DICT:expire, which returns true on success end function SimpleDict:ttl(k) self:get(k) @@ -822,6 +823,14 @@ function TestKeyIndex:testExpiredReAddNoDuplicate() luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) luaunit.assertEquals(self.key_index.index["expkey"], 1) + -- A second worker sharing the same shared dict syncs the initial state, so it + -- now holds slot 1 in its local self.keys/index. This is the worker that the + -- delete_count bump must later force to re-sync and reclaim the stale slot. + local worker2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) + worker2:sync() + luaunit.assertEquals(worker2.index["expkey"], 1) + luaunit.assertEquals(#worker2:list(), 1) + -- Let the key expire in the underlying shared dict. key_count and -- delete_count are untouched, so the next sync() inside add() is a no-op and -- the local index keeps pointing at the (now gone) slot 1. @@ -841,6 +850,31 @@ function TestKeyIndex:testExpiredReAddNoDuplicate() local keys = self.key_index:list() luaunit.assertEquals(#keys, 1) luaunit.assertEquals(keys[1], "expkey") + + -- The second worker must converge: its next sync() sees the bumped + -- delete_count, does a full sync, drops the stale slot 1 and picks up slot 2. + -- Without the delete_count bump it would keep slot 1 forever and list() the + -- key twice. + worker2:sync() + luaunit.assertEquals(worker2.keys[1], nil) + luaunit.assertEquals(worker2.index["expkey"], 2) + local keys2 = worker2:list() + luaunit.assertEquals(#keys2, 1) + luaunit.assertEquals(keys2[1], "expkey") +end + +-- list() must de-duplicate when self.keys transiently holds the same key in two +-- slots (the window before a stale slot is reclaimed): only the slot the index +-- points at is canonical. This isolates the list() guard from the add() path. +function TestKeyIndex:testListDeduplicatesStaleSlot() + self.key_index.keys = {[1] = "dup", [2] = "dup"} + self.key_index.index = {dup = 2} + self.key_index.last = 2 + + local keys = self.key_index:list() + luaunit.assertEquals(ngx.logs, nil) + luaunit.assertEquals(#keys, 1) + luaunit.assertEquals(keys[1], "dup") end function TestKeyIndex:testSync()