Conversation
… new one
When a key's index slot is reclaimed from the shared dict, add() allocated a
new slot number and bumped delete_count so that every other worker would drop
the stale slot on its next sync(). Both are avoidable: the slot number is free,
so the key can simply take it back.
Re-claiming in place keeps the key's identity, which removes the condition the
duplicate-metric fix was written for -- a key can no longer occupy two slots --
and has three further effects:
* key_count stops growing on expiry churn. It only ever grows, and
sync_range(0, key_count) walks every slot it ever handed out, so each
re-allocation makes every later full sync more expensive, permanently.
* delete_count stops moving, so no worker is pushed into a full
sync_range(0, key_count) from its request path. With several workers on one
dict that scan is O(key_count) shared-dict reads each, all contending for
the same shm mutex.
* that full sync is itself what wiped index[key] for every slot whose node had
ttl-expired but not yet been reclaimed, so those keys were re-allocated too.
One bump therefore cost far more than one slot.
expire() only reports "not found" once the node is physically gone. A node that
has merely ttl-expired is resurrected by expire() and keeps its slot, so this
branch is reached only after flush_expired() (or an LRU eviction) has removed
it, and the slot is genuinely free.
Races are handled by relying on dict:add() being atomic. A peer that re-claims
the slot first makes add() return "exists"; the value is then compared against
the key before the slot is accepted. If it cannot be re-claimed at all -- out of
memory, or the slot holds a different key because key_count was evicted and
restarted -- the old behaviour of allocating a new slot still applies, and
list() keeps reporting the key once from the slot index points at.
Measured on OpenResty with 10 workers sharing a 512m dict, 144k slots, a 4000
combination rotation expiring and returning, and flush_expired() once a second:
total CPU key_count delete_count duplicates
before 421% (peak 970%) +1368 268 0
after 13% (peak 40%) +0 0 0
testExpiredReAddNoDuplicate now asserts the slot is re-claimed, key_count does
not grow and delete_count stays unset, and that the second worker stays correct
without being told anything. testExpiredReAddFallbackNoDuplicate covers the
fallback path, where the slot is stolen between expire() and add().
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe change updates expired-key re-add handling. ChangesExpired key re-add
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The fallback path keeps worker listings synchronized without requiring a delete-count update, so no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: E2e Test Quality ReviewExplanation Blocking issue: the changed regression tests are component-level tests only. They call Resolution Add an end-to-end test that runs Nginx/OpenResty with a real shared dictionary. Register an expiring metric through the public API, record it, let the slot expire, record it again, and collect metrics from multiple workers. Assert that the metric is present exactly once after reclamation. Keep the existing unit tests for slot identity and fallback behavior, and add a real concurrent-worker case for the reclamation race if possible.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Comment |
2 participants
Problem
KeyIndex:add()handles a key whose index slot was reclaimed from the shared dict by allocating a new slot number and bumpingdelete_count, so that every other worker drops the stale slot on its nextsync(). Both are avoidable — the old slot number is free, so the key can simply take it back.Keeping the slot number removes the condition the duplicate-metric fix in #14 was written for (a key can no longer occupy two slots) and avoids three costs that turn out to matter a great deal in production:
1.
key_countgrows on expiry churn, permanently. It only ever increases, andsync_range(0, key_count)walks every slot it ever handed out — dead ones included. Every re-allocation makes every later full sync more expensive, for the lifetime of the dict.2.
delete_countpushes every worker into a full sync from its request path.sync()is called fromadd(), so a bump means each worker performssync_range(0, key_count)— that many shared-dict reads, all contending for the same shm mutex. With several workers on one dict this is a spin-lock convoy:ngx_shmtx_lockdominates CPU while almost no useful work happens.3. That full sync is itself what makes
key_countexplode.sync_rangeclearsindex[key]for every slot whose node has ttl-expired but not yet been reclaimed —dict:getreturns nil for those. Those keys then have no slot on their nextadd()and are re-allocated too. One bump costs far more than one slot:key_countgrowthdelete_countgrowthControl (no bump, everything else identical):
key_count+2.Because the growth feeds the cost of the next sync, this is self-reinforcing: the more a dict is bumped the more expensive each bump becomes, so a process can diverge from an identically configured peer with no difference in load.
Fix
Re-claim the same slot number:
expire()only reports"not found"once the node is physically gone. A node that has merely ttl-expired is resurrected byexpire()and keeps its slot — so this branch is reached only afterflush_expired()(#18) or an LRU eviction has removed it, and the slot is genuinely free.Put differently: for a live slot and for a ttl-expired-but-present slot the library already reuses the slot in place. This change extends that to the third state rather than introducing a new strategy.
dict:getdict:expiretrueniltrue(resurrects)nil"not found"Races
dict:add()is atomic. A peer that re-claims the slot first makes it return"exists"; the value is compared against the key before the slot is accepted. If the slot cannot be re-claimed at all — out of memory, or it holds a different key becausekey_countwas evicted and restarted — the previous behaviour of allocating a new slot still applies, andlist()keeps reporting the key once from the slotindexpoints at.The
forcibleflag from the re-claimingadd()is surfaced througherr_msg_lru_eviction, as the new-slot path already does.The canonical-slot check in
list()from #14 is kept: it costs nothing and still covers the fallback path and mixed-version rolling upgrades.Measurements
OpenResty 1.29.2.4, 10 workers on one 512m dict, 144k slots pre-loaded, a 4000-combination rotation expiring and returning,
flush_expired()once a second, 20s:key_countgrowthdelete_countConcurrency and correctness, same setup at 1500 rps over a 3000-combination rotation, 25s, repeated with the dict sized 512m / 4m / 1m (the last one runs with 0 bytes free, so the LRU is evicting throughout):
key_countdelete_countself.keys5,001 is the rotation size plus the error metric, i.e. no growth at all. The peer-won branch (
add()returning"exists"with a matching value) was exercised 1–5 times per run, and the fallback branch never was, including with the dict full.For comparison, v1.0.0 under the 512m run reaches
key_count12,936 withdelete_count795.Removing only the bump and keeping the new-slot allocation was also measured: it does stop the convoy, but
key_countstill grows (13,553 vs 14,221) and the stale entries in each worker'sself.keysare never collected — 9,712 of 12,713 entries were dead after 25s, whichlist()walks on every scrape. Re-claiming the slot avoids both.Tests
testExpiredReAddNoDuplicatenow asserts the slot is re-claimed,key_countdoes not grow,delete_countstays unset, and the second worker remains correct without being told anything —list()still reports the key exactly once, which is what the test is for.testExpiredReAddFallbackNoDuplicateis added for the fallback path: the slot is stolen betweenexpire()reporting it gone andadd()trying to re-claim it, which is the only way another key can end up on that slot number. Both workers must still list the key once.lua prometheus_test.lua: 46 tests, 45 pass.TestPrometheus.testPrintfTablefails identically on unmodifiedmainunder LuaJIT (trailing-nil vararg handling differs from the Lua 5.2 CI runs).luacheckis clean on all four files.Relationship to #20
#20 compacts the index so dead slots do not accumulate. It stays useful — slots freed by an explicit
remove(), and any index that has already grown, still need reclaiming. This change removes the largest source of dead slots rather than collecting them afterwards, so the two are complementary.Summary by CodeRabbit
Bug Fixes
Tests