[manager] implement fair eviction across instances

[shaohuaxi]

Summary

• distribute each group reclaim round sampling and eviction budgets across instances according to the active water-level dimension
• use deterministic integer allocation, bounded sampling waves, and current async credit checks to avoid group over-eviction
• preserve legacy behavior behind enable_instance_fairness and keep multi-tier migration admission ordered after reclaim admission
• add metrics, configuration/proto plumbing, design documentation, and regression coverage for allocation, water-level priority, credit, sampling, and migration interactions

Testing

• //kv_cache_manager/manager/test:CacheReclaimerTest
• //kv_cache_manager/config/test:instance_group_test
• //kv_cache_manager/manager/test:StartupConfigLoaderTest
• //kv_cache_manager:kv_cache_manager_bin
• tiered-storage reclaimer smoke tests: 5 passed

[qoderai]

Review Summary — PR #247 [manager] implement fair eviction across instances

The implementation is well-structured and the design doc is unusually thorough for a scheduler change. I read the full diff, the design doc, and the added tests end-to-end, then walked through the runtime paths (credit-aware truncation, bounded-wave sampling, largest-remainder allocation with 128-bit numerators, migration ordering). Overall this looks ready to land modulo a few small follow-ups.

What looks good

• Water-level signal is now two-dimensional and credit-aware. Splitting general_water_level_exceed_ into independent group_bytes_ / group_keys_ flags and deriving the legacy getter as their || is a clean compatibility shim. SetGeneralWaterLevelExceed(value) sets bytes=value/keys=false, which matches the pre-refactor semantics for the existing tests.
• Largest-remainder budget allocation is deterministic and overflow-safe. AllocateFairBudget uses 128-bit numerators, detects quotient > size_t::max, detects allocation > remaining, and breaks remainder ties by instance_id then insertion index — exactly the properties a fair scheduler needs. Zero-weight instances are excluded before the group budget is multiplied, so they don't amplify B_group = B·N.
• Credit-aware truncation is bounded and testable. The plan_scope_still_active lambda gives a crisp rule for when to abandon a pre-built plan, and the mid-round SetSamplingSize / SetBatchingSize test (TestFairReclaimSnapshotsRuntimeBudgetForCurrentPlan) correctly proves that runtime changes only take effect in the next round.
• Bounded-wave sampling is defensive in the right places. The wave loop checks in_flight >= workers_.size() before each wave, uses a shared deadline, and discards partial results on any failure — matching the contract in design §8.2.
• Migration ordering is preserved at the common exit. Moving TryMigrateOnGroup out of the legacy branch into TryReclaimOnGroup keeps the "reclaim admission precedes migration" invariant for both paths and is covered by TestFairReclaimStillRunsMigrationBelowReclaimThreshold.
• Test coverage is broad. 25+ new cases cover allocation determinism, dimension priority, per-instance cap, credit stop after accepted, credit stop after rejected, trigger-scope change, mid-round toggle, legacy equivalence, bounded-wave failure/timeout, and migration ordering.

Observations / small follow-ups

Three inline comments flag the only things I'd suggest addressing before merge (or explicitly deferring to a follow-up):

1. LFU / TTL silently falls back to LRU in the fair path. The legacy path has the same fallback, but in fairness mode this reinterprets the policy across an entire instance group. Suggest a dedicated metric (not just a throttled LOG_WITH_TRACE(WARN)) plus a test that configures LFU/TTL and asserts the fallback — see inline comment on cache_reclaimer.cc:2762.
2. plan_scope_still_active is stricter than the design doc. The GROUP_BYTES arm aborts if any storage type newly exceeds, and GROUP_KEYS aborts if bytes or any type newly exceeds. Defensible, but only one transition (STORAGE_TYPE_BYTES → GROUP_BYTES) is tested. Worth locking in each arm with one additional test — see inline comment on cache_reclaimer.cc:2708.
3. STORAGE_TYPE_BYTES weight sum has no multi-type test. TestFairReclaimPlanStorageTypeBytesWeight only triggers a single storage type, so the SaturatingAdd(weight, ...) accumulation and the VCNS_HF3FS skip in the weight loop are uncovered — see inline comment on cache_reclaimer.cc:2394.

Other minor notes (no action required)

• SetGeneralWaterLevelExceed as a bytes-only shim is semantically lossy but correct for the current callers (they all set it in the group-bytes context).
• The fair path re-reads water level via GetWaterLevelExceed before every instance item when credit restores — more expensive than the legacy per-instance read, but intentional for credit-aware truncation and covered by TestFairReclaimUsesWeightedBudgetAndStopsAfterAcceptedCredit.
• The water_level_before_next_item optimization path is only reachable when credit restores mid-round; current tests exercise the "stop on restore" side but not the "continue with cached read" side. Low risk.
• fair_effective_instance_count is registered as Gauge (matches the design).
• DoKeySamplingWithSize with bounded_waves && worker_sz == 1 takes the bounded loop instead of the legacy single-shot short-circuit; correct (it respects IsPaused() mid-wave), just marginally less efficient.

Thanks for the thorough design doc — it made this review significantly easier.

---

🤖 Generated by Qoder

[qoderai]

The LFU / TTL fallback here silently degrades to fair LRU and only emits a LOG_WITH_TRACE(WARN). Two small follow-ups to consider in this PR or a tracking issue:

1. Add a test that configures POLICY_LFU (or POLICY_TTL) with fairness enabled and asserts (a) the reclaim still makes progress via LRU, and (b) the fallback is observable — e.g. by bumping a dedicated metric (fair_lfu_fallback_count / fair_ttl_fallback_count) rather than only a log line. LOG_WITH_TRACE is interval-throttled, so in production it may not be discoverable.
2. Decide whether the fallback should be a hard error for fairness. The legacy path emits the same warning and falls back too (lines 1045 / 1059), so parity is preserved, but silently re-interpreting a configured policy as LRU across an entire instance group is the kind of behavior operators tend to want surfaced via metrics / admin validation.

---

🤖 Generated by Qoder

[qoderai]

Nit on plan_scope_still_active: the GROUP_BYTES arm returns false if any storage type becomes exceeded mid-round, and GROUP_KEYS returns false if either bytes or any storage type becomes exceeded. That's stricter than the design doc's phrasing "超水位维度或 Storage Type 集合发生变化" — it also aborts when an orthogonal dimension newly exceeds.

The behavior is defensible (a stricter re-plan is usually correct), but worth calling out explicitly:

• The current test (TestFairReclaimStopsWhenTriggerScopeChanges) only covers STORAGE_TYPE_BYTES → GROUP_BYTES.
• Missing coverage: GROUP_BYTES still exceeding while a storage type newly exceeds (should abort, currently untested), and GROUP_KEYS still exceeding while GROUP_BYTES newly exceeds (should abort, currently untested).

Consider adding one test per arm of the switch to lock in the chosen contract — otherwise a future refactor could silently relax it.

---

🤖 Generated by Qoder

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 01fa1559d4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".