fix(keys): avoid duplicate metrics from KeyIndex desync

[AlinsRan]

Problem

KeyIndex could emit the same key more than once, which surfaces downstream as duplicate Prometheus metrics. This fixes the upstream report apache/apisix#11934 ("apisix reports duplicate metrics"), whose root cause lives in this fork rather than in apisix itself.

Root cause

Two issues combine:

1. KeyIndex:list() iterated the stored key values rather than the index. It looped over self.keys (keyed by slot number) and copied every value. self.keys can transiently hold the same key value at two different slots — e.g. when an expired metric is re-added at a new slot before the old slot is reclaimed — so the duplicated value was emitted twice.

   for _, v in pairs(self.keys) do
     copy[i] = v
     i = i + 1
   end

2. delete_count was not bumped when an expired metric was re-added. In add(), when a key whose shared-dict entry had already expired is re-added, the code cleared the local index/keys/expire_keys but did not clear the dict slot or bump delete_count:

   if not ok then
     local idx = self.index[key]
     self.index[key] = nil
     self.keys[idx] = nil
     self.expire_keys[idx] = nil
     expired = true
   end

   Because delete_count was unchanged, other workers never triggered a full sync (KeyIndex:sync only does a full re-sync when self.deleted ~= delete_count). They kept the stale slot in their local self.keys while the metric was re-added at a new slot — desynchronizing the index and causing the duplicate emission described above.

Fix

1. list() now walks the slots but only emits a key at the slot the index currently points at (self.index[key] == idx), so each key is listed exactly once while preserving ordering.
2. add() now mirrors remove() on the expired-re-add path: it clears the dict slot, increments the local deleted counter and bumps delete_count, so other workers do a full sync and reclaim the stale slot.

Public API and observable behavior are unchanged.

Test

Adds TestKeyIndex:testExpiredReAddNoDuplicate, which:

• adds a key with an exptime and syncs,
• lets it expire in the shared dict without going through remove() (so key_count/delete_count are untouched and the next sync() is a no-op, leaving the index pointing at the vanished slot),
• re-adds the key (hitting the expired branch),
• asserts delete_count was bumped and that list() returns the key exactly once.

Verified fail-before / pass-after:

• Unpatched: delete_count == nil and list() returns ["expkey", "expkey"] (test fails).
• Patched: delete_count == 1 and list() returns ["expkey"] (test passes).

Full unit suite and luacheck (per CI config) pass; the only remaining failure is the pre-existing, unrelated TestPrometheus.testPrintfTable (a luaunit table-formatting difference present on main before this change).

Follow-up

Once a release is cut, an apisix-side rockspec bump will follow to pull in the fixed version.

Summary by CodeRabbit

• Bug Fixes
  • Fixed a case where re-adding an already-expired key could lead to duplicate metric/list emissions.
  • Improved key enumeration to avoid duplicates when stale slot data is present, ensuring each active key is returned exactly once per listing and stays consistent across workers.
• Tests
  • Added regression coverage for re-adding keys after expiration (without an explicit remove), including multi-worker deduplication verification.
  • Added a direct test to confirm listings de-duplicate when the same key temporarily appears in multiple slots.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 241feed3-8d5f-48bd-8f1f-5e12eab2be86

📥 Commits

Reviewing files that changed from the base of the PR and between 5d44240 and 1d01057.

📒 Files selected for processing (2)

• prometheus_keys.lua
• prometheus_test.lua

---

📝 Walkthrough

Walkthrough

KeyIndex:list() is changed from iterating self.keys via pairs() to walking numeric slots 0..self.last and emitting only on canonical index matches. KeyIndex:add() now clears the stale shared-dict slot and increments delete_count when expire() fails on an expired key. Two regression tests cover both the full expiry-and-re-add scenario and direct list deduplication.

Changes

Duplicate Key Emission Fix

Layer / File(s)	Summary
KeyIndex deduplication logic prometheus_keys.lua	list() traverses slots 0..self.last and checks self.index[key] == idx to emit each key exactly once. add() on the expired-key path clears the stale slot, increments delete_count, and allocates a new slot to force cross-worker resync.
Test helper and regression tests prometheus_test.lua	SimpleDict:expire returns true. testExpiredReAddNoDuplicate validates the full expired re-add scenario with new slot, incremented delete_count, and de-duplicated output. testListDeduplicatesStaleSlot directly asserts list deduplication when slots contain duplicates.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
E2e Test Quality Review	⚠️ Warning	Missing E2E test (Blocking). Tests verify KeyIndex.list() deduplication at unit level but not the actual duplicate metrics symptom in Prometheus.collect() output. Other criteria (readability, error...	Add E2E test: create/increment Prometheus metric with exptime, let it expire, re-add it, call collect(), verify metric appears exactly once in output (not duplicated).

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(keys): avoid duplicate metrics from KeyIndex desync' directly and clearly summarizes the main change: fixing a bug that caused duplicate Prometheus metrics due to KeyIndex desynchronization.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Security Check	✅ Passed	No security vulnerabilities detected. PR fixes a shared resource synchronization bug; contains no credential/secret leakage, DB operations, auth bypasses, TLS misconfigurations, or secret reference...

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@prometheus_test.lua`:
- Line 817: The KeyIndex:add() method call on self.key_index is not validating
the return value directly. Instead of relying on checking ngx.logs which is
global state and can be stale, capture the return value from
self.key_index:add("expkey", "eviction_err", 1) and assert it directly to verify
the operation succeeded or failed as expected. Apply the same fix to the similar
calls around lines 831-832.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c3b895e6-734f-4c98-bdd8-150cbbf6348d

📥 Commits

Reviewing files that changed from the base of the PR and between f86aa3e and 57ec06c.

📒 Files selected for processing (2)

• prometheus_keys.lua
• prometheus_test.lua

[membphis]

Approved. No blocking issues found.