fix(keys): prevent infinite loop in KeyIndex:add() when key_count falls behind occupied slots#16
Conversation
…ls 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesKey count repair
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant KeyIndex
participant SharedDict
Worker->>KeyIndex: add(key)
KeyIndex->>SharedDict: add(slot, key)
SharedDict-->>KeyIndex: exists
KeyIndex->>SharedDict: advance key_count
KeyIndex->>SharedDict: retry allocation
KeyIndex-->>Worker: allocated slot or repair error
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_keys.lua`:
- Around line 203-209: Update the repair branch in the key-management flow
around self.dict:incr and the repairs counter to capture and validate incr’s
error and forcible result before incrementing repairs. Only count a successful,
non-forcible repair; propagate or handle failures consistently with the existing
LRU-eviction path so a failed or forcible recreation cannot be treated as a
completed repair.
🪄 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: 4829e5c8-229b-4082-b0cf-dcaeee64ed59
📒 Files selected for processing (2)
prometheus_keys.luaprometheus_test.lua
…ions 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.
|
P2 (non-blocking): repair_forcible is checked only on the new-slot success path. If repairing key_count causes incr(..., 0) to return forcible = true and the next sync() discovers that the requested key already exists, KeyIndex:add() breaks and returns success, so the eviction is not reported. Please consider checking repair_forcible before the existing-key break and adding a regression test for this path. |
…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.
|
Good catch, fixed in 71cd6bc: the existing-key exit now returns the LRU-eviction error when |
3 participants
Problem
KeyIndex:add()retriesdict:add()on the same slot forever when the slot is occupied but__ngx_prom__key_countreports a smaller value:key_countis 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 it gets LRU-evicted while higher-numbered slots survive (forcible eviction always removes the LRU-tail node).sync()then reads it asnil → 0,incr()re-creates it at 1, and every subsequent new-series registration collides with a surviving slot:"exists"→ retry →"exists", forever, without yielding. The worker spins at 100% CPU with the shared-dict lock hot, independent of traffic, until the process is restarted — each failedadd()also LRU-refreshes the blocking slot, so the state never self-heals.This is the root cause of apache/apisix#12275 (workers pegged at 100% CPU even with traffic removed;
ngx_shmtx_lockdominating perf, shdict get:store ≈ 2:1 — exactly this loop body). Full analysis and profiles: apache/apisix#12275 (comment)Minimal reproduction against the real shared dict:
git clone https://github.com/api7/nginx-lua-prometheus resty -I nginx-lua-prometheus --shdict 'prometheus_metrics 10m' repro.luarepro.lua (hangs before this fix, returns after)
Fix
On a repeated
"exists"collision, advancekey_countpast the occupied slot instead of retrying it forever; the nextsync()adopts the slot's occupant and progress resumes. The first collision keeps the old plain retry, so the benign race (another worker created the slot but has not incrementedkey_countyet) behaves exactly as before.Repair work per call is bounded (
MAX_KEY_COUNT_REPAIRS = 1000); beyond that the call degrades like the existing dict-full path (returns the LRU-eviction error, sample dropped). The advanced counter is shared through the dict, so later calls resume where the previous one stopped and the index converges even when far more slots need repairing.Tests
SimpleDict:addin the test mock now returns"exists"for present keys, matchingngx.shared.DICT:add(it previously always succeeded, so this code path was untestable).testAddAfterKeyCountEvicted— the regression test: hangs forever on the unfixed code (killed only by timeout), passes in milliseconds with the fix.testAddAfterKeyCountEvictedFreshWorker— same eviction seen from a worker with an empty local index (respawn/reload); it must adopt surviving slots while repairing.testKeyCountRepairIsBounded— one call performs at most 1000 repairs and returns the degradation error; the next call resumes and converges.Full unit suite (41 tests) and the docker-based integration suite pass.
Ref: apache/apisix#12275
Summary by CodeRabbit