Lab K5: Fix It — A Real k-NN Issue

Background

The k-NN plugin's native-memory circuit breaker is one of the most-reported, least-understood subsystems in the repo. It guards memory that no JVM tool can see (faiss/nmslib graphs live off-heap — see native integration and memory), it is entangled with the NativeMemoryCacheManager Guava cache and with cluster-settings plumbing, and its "tripped / un-tripped" hysteresis has sharp edges. Two real issues frame the territory:

• k-NN #1582 — Investigate rearchitecture of the native memory circuit breaker — the breaker is too entangled with the cache and the settings layer; the eviction-vs-rejection relationship is muddy.
• k-NN #585 — circuit-breaker limit config bug — a concrete defect in how the knn.memory.circuit_breaker.limit setting was handled.

This Fix-It lab works in the flavor of #585: a settings-plumbing bug where the configured circuit-breaker limit is not honored. You will reproduce the symptom, locate the code with grep in org.opensearch.knn.index.memory.* and KNNSettings, apply a minimal diff, write a unit test that asserts the corrected behavior, build and test, and ship a PR with DCO sign-off and a CHANGELOG.md entry. The point is not just the patch — it is doing it the way k-NN maintainers expect, in a subsystem where a careless test leaks off-heap memory.

Note on terminology: the cluster manager (formerly master) publishes the cluster setting knn.memory.circuit_breaker.limit; each data node applies it locally to its own NativeMemoryCacheManager. The bug in this lab is on the apply side — per-node — which is exactly why it is invisible from the cluster-settings API alone.

Warning: This lab uses a representative reconstruction of a #585-flavored bug so it is reproducible on today's tree (the original was fixed years ago). The class names, settings keys, and APIs are real; grep your checkout to find where the equivalent logic lives now, because this file has been refactored more than once.

Why This Lab Matters for Contributors

• Settings-plumbing bugs are a classic, well-scoped contributor on-ramp: small surface, high impact, easy to test once you find them. They are also where backward-compat rules bite — see Issue Roadmap Stage 8: Plugin Compatibility.
• The native-memory breaker is the difference between "node degrades gracefully under vector load" and "the Linux OOM-killer reaps the process." A limit that is silently ignored is a production landmine.
• This subsystem punishes sloppy tests harder than any other: faiss allocations are off-heap, so a test that loads a graph and forgets to free it leaks native memory across the whole test JVM. You will learn to test it without leaking.
• "Fix a config bug, add a regression test, un-break the limit" is a concrete, mergeable PR with a clean before/after.

Prerequisites

• native integration and memory read — you understand NativeMemoryCacheManager, the native breaker, and that the limit is a percentage of node RAM (or an absolute size), not a percentage of -Xmx.
• You can build and test the plugin (lab-k1): ./gradlew test, ./gradlew run.
• You can read a cluster setting via the API and set it dynamically.

cd ~/src/oss-repos/k-NN
./gradlew test --tests "*KNNSettings*"   # sanity: the settings tests compile & run

Step 1: Reproduce the symptom

The reported behavior: an operator sets knn.memory.circuit_breaker.limit to a smaller value to bound native memory, but the cache keeps growing past it — the new limit is ignored until restart, or an absolute value like "4kb" is parsed wrong. Reproduce it against a running node.

./gradlew run    # single node with k-NN

# Read the current native breaker settings (this is the cluster-published view):
curl -s 'localhost:9200/_cluster/settings?include_defaults=true&flat_settings=true&pretty' \
  | grep -E 'knn\.memory\.circuit_breaker|knn\.circuit_breaker'

# Set a deliberately tiny absolute limit so any graph trips it:
curl -XPUT 'localhost:9200/_cluster/settings' -H 'Content-Type: application/json' -d '
{ "persistent": { "knn.memory.circuit_breaker.limit": "1kb" } }'

# Index a faiss field, refresh, warm it up so a native graph loads:
curl -XPUT 'localhost:9200/cb' -H 'Content-Type: application/json' -d '
{ "settings": { "index.knn": true },
  "mappings": { "properties": { "e": { "type": "knn_vector", "dimension": 8,
    "method": { "name": "hnsw", "engine": "faiss", "space_type": "l2" } } } } }'
for i in $(seq 1 200); do
  curl -s -XPOST 'localhost:9200/cb/_doc' -H 'Content-Type: application/json' \
    -d "{\"e\":[$i,1,2,3,4,5,6,7]}" >/dev/null
done
curl -s -XPOST 'localhost:9200/cb/_refresh' >/dev/null
curl -s -XPOST 'localhost:9200/_plugins/_knn/warmup/cb?pretty' >/dev/null

# Now check whether the breaker tripped. With a 1kb limit and a real graph, it MUST.
curl -s 'localhost:9200/_plugins/_knn/stats?pretty' \
  | grep -E 'graph_memory|circuit_breaker|cache_capacity|eviction'

The bug's signature: graph_memory_usage exceeds the configured limit but knn.circuit_breaker.triggered reads false, or the dynamically-updated limit is not reflected in the cache's maximumWeight. The configured limit was parsed and stored but never propagated to the live cache. That disconnect is the bug class #585 is about.

Warning: Do not conclude "the breaker is broken" from RSS alone. glibc's allocator keeps freed arenas, so RSS lags eviction (see native integration § eviction). Judge by the cache accounting in _plugins/_knn/stats, not by ps.

Step 2: Locate the code

Two suspects: where the setting is defined and parsed (KNNSettings), and where the limit is applied to the cache (org.opensearch.knn.index.memory.*). Grep both.

# The setting definition, parsing, and the byte/percentage resolution:
grep -rn "circuit_breaker.limit\|CIRCUIT_BREAKER_LIMIT\|getCircuitBreakerLimit\|ByteSizeValue\|parseknnMemoryCircuitBreakerValue\|KNN_MEMORY_CIRCUIT_BREAKER_LIMIT" \
  src/main/java/org/opensearch/knn/index/KNNSettings.java

# Where the cache reads the limit to size its maximumWeight, and the settings-update hook:
grep -rn "maximumWeight\|setMaximumWeight\|circuitBreakerLimit\|rebuildCache\|addSettingsUpdateConsumer\|onCacheConfigChanged" \
  src/main/java/org/opensearch/knn/index/memory/NativeMemoryCacheManager.java \
  src/main/java/org/opensearch/knn/index/KNNSettings.java

You are looking for the seam where a dynamic settings update should call back into the cache and rebuild it with the new weight bound — and isn't, or where the parse of an absolute size ("4kb", "60%") returns the wrong number of bytes.

# Trace the update consumer registration — the dynamic-settings callback:
grep -rn "clusterSettings.addSettingsUpdateConsumer\|settingsUpdateConsumer\|consumer" \
  src/main/java/org/opensearch/knn/index/KNNSettings.java

The two #585-flavored failure modes you might find:

This lab fixes the stale-limit mode: the update consumer doesn't propagate to the cache.

Step 3: The diff

The fix wires the dynamic settings update into a cache rebuild, so changing knn.memory.circuit_breaker.limit at runtime actually re-bounds the live cache. The exact file/lines drift — this is the shape; adapt the surrounding lines to your tree.

--- a/src/main/java/org/opensearch/knn/index/KNNSettings.java
+++ b/src/main/java/org/opensearch/knn/index/KNNSettings.java
@@ -1,5 +1,7 @@
 package org.opensearch.knn.index;

+import org.opensearch.knn.index.memory.NativeMemoryCacheManager;
+
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.unit.ByteSizeValue;
@@ -120,9 +122,18 @@ public class KNNSettings {
     private void setClusterSettings(final ClusterSettings clusterSettings) {
         this.clusterSettings = clusterSettings;
-        // BUG: the limit setting is registered as dynamic, but nothing propagates a
-        //      changed value into the live NativeMemoryCacheManager weight bound, so a
-        //      runtime update is silently ignored until the next node restart.
-        clusterSettings.addSettingsUpdateConsumer(KNN_MEMORY_CIRCUIT_BREAKER_LIMIT, newLimit -> {
-            // value stored in the settings map, but the cache is never rebuilt
-        });
+        // FIX: on a dynamic update of the circuit-breaker limit, rebuild the cache so its
+        //      maximumWeight reflects the new bound immediately (no restart required).
+        clusterSettings.addSettingsUpdateConsumer(KNN_MEMORY_CIRCUIT_BREAKER_LIMIT, newLimit -> {
+            NativeMemoryCacheManager.getInstance().rebuildCache();
+        });
     }

--- a/src/main/java/org/opensearch/knn/index/memory/NativeMemoryCacheManager.java
+++ b/src/main/java/org/opensearch/knn/index/memory/NativeMemoryCacheManager.java
@@ -60,12 +60,27 @@ public class NativeMemoryCacheManager implements Closeable {

-    // BUG: maximumWeight is read once at construction from the limit; a later settings
-    //      update has no path to change the live cache's bound.
-    private void initCache() {
-        long maxWeightKb = KNNSettings.getCircuitBreakerLimit().getKb();
-        this.cache = CacheBuilder.newBuilder()
-            .maximumWeight(maxWeightKb)
-            .weigher((k, v) -> v.getSizeInKb())
-            .removalListener(this::onRemoval)
-            .build();
-    }
+    private void initCache() {
+        this.cache = buildCache(KNNSettings.getCircuitBreakerLimit().getKb());
+    }
+
+    private Cache<String, NativeMemoryAllocation> buildCache(long maxWeightKb) {
+        return CacheBuilder.newBuilder()
+            .maximumWeight(maxWeightKb)
+            .weigher((String k, NativeMemoryAllocation v) -> v.getSizeInKb())
+            .removalListener(this::onRemoval)
+            .build();
+    }
+
+    /**
+     * Rebuild the cache against the CURRENT circuit-breaker limit. Invalidating the old
+     * cache fires the RemovalListener for every entry, which frees the native memory via
+     * JNI — so a shrunk limit immediately evicts down to the new bound, and nothing leaks.
+     */
+    public synchronized void rebuildCache() {
+        long newMaxWeightKb = KNNSettings.getCircuitBreakerLimit().getKb();
+        Cache<String, NativeMemoryAllocation> old = this.cache;
+        this.cache = buildCache(newMaxWeightKb);
+        if (old != null) {
+            old.invalidateAll();   // RemovalListener -> JNI free() for every entry
+        }
+    }

The crucial detail is the last comment: invalidateAll() fires the RemovalListener, which calls the engine's free across JNI. Rebuilding the cache without invalidating the old one would leak every loaded faiss graph — a non-obvious off-heap leak. The fix re-bounds the cache and releases native memory in one atomic, synchronized step.

Note: A real PR would likely also debounce or guard the rebuild (it briefly drops all cached graphs, so the next queries pay reload cost), and reconcile with the rearchitecture direction in k-NN #1582. Note that in your PR description — maintainers will ask how your fix relates to the planned rework.

Step 4: A unit test that asserts the corrected behavior — without leaking

The test must prove the limit propagates, without loading a real faiss graph (that allocates off-heap; a unit test that loads and forgets to free leaks native memory for the rest of the JVM run). Assert on the cache's configured bound, not on a live graph.

/*
 * SPDX-License-Identifier: Apache-2.0
 *
 * The OpenSearch Contributors require contributions made to
 * this file be licensed under the Apache-2.0 license or a
 * compatible open source license.
 */
package org.opensearch.knn.index.memory;

import org.opensearch.common.settings.ClusterSettings;
import org.opensearch.common.settings.Settings;
import org.opensearch.knn.KNNTestCase;
import org.opensearch.knn.index.KNNSettings;

import static org.opensearch.knn.index.KNNSettings.KNN_MEMORY_CIRCUIT_BREAKER_LIMIT;

public class NativeMemoryCircuitBreakerLimitTests extends KNNTestCase {

    /**
     * Updating knn.memory.circuit_breaker.limit at runtime must re-bound the live cache.
     * Before the fix, the cache kept its construction-time maximumWeight and the update
     * was silently ignored. We assert on the cache's CONFIGURED weight, never load a
     * native graph, and free nothing — so this test allocates no off-heap memory.
     */
    public void testDynamicLimitUpdateRebuildsCacheBound() {
        // Register settings with a starting limit.
        Settings initial = Settings.builder()
            .put(KNN_MEMORY_CIRCUIT_BREAKER_LIMIT.getKey(), "10kb")
            .build();
        ClusterSettings clusterSettings = new ClusterSettings(
            initial, KNNSettings.state().getSettings());     // grep for the real registration helper
        KNNSettings.state().setClusterSettings(clusterSettings);

        NativeMemoryCacheManager cm = NativeMemoryCacheManager.getInstance();
        cm.rebuildCache();
        long before = cm.getMaxCacheSizeInKilobytes();
        assertEquals("initial limit should be honored", 10L, before);

        // Apply a dynamic update to a smaller limit.
        clusterSettings.applySettings(Settings.builder()
            .put(KNN_MEMORY_CIRCUIT_BREAKER_LIMIT.getKey(), "4kb")
            .build());

        long after = cm.getMaxCacheSizeInKilobytes();
        assertEquals("dynamic update must re-bound the live cache", 4L, after);
        assertTrue("limit must actually shrink, not stay stale", after < before);
    }

    @Override
    public void tearDown() throws Exception {
        // Hygiene: drop any cache state so this test never leaks native allocations into
        // the shared test JVM. invalidateAll() fires the RemovalListener (JNI free()).
        NativeMemoryCacheManager.getInstance().invalidateAll();
        super.tearDown();
    }
}

Warning — native memory hygiene in tests: Every k-NN test that touches the cache must leave it empty. Off-heap allocations are not GC'd; a leaked faiss pointer survives the test method and inflates the test JVM's RSS, which can OOM-kill the entire test run on CI. Always invalidateAll() (or the equivalent) in tearDown, and prefer asserting on cache configuration over loading real graphs. This is the single rule that separates a green k-NN test PR from a flaky, memory-leaking one.

./gradlew test --tests "org.opensearch.knn.index.memory.NativeMemoryCircuitBreakerLimitTests"

Step 5: Build, test, and verify end to end

# 1) Unit test (fast inner loop).
./gradlew test --tests "*NativeMemoryCircuitBreakerLimit*"

# 2) Full module test + precommit (formatting, license headers, forbidden APIs).
./gradlew spotlessApply
./gradlew precommit

# 3) Manual proof on a running node: the Step-1 reproduction now behaves.
./gradlew run
# ...repeat the Step 1 curls: set limit to "1kb", warmup the faiss field...
# Now the breaker trips / the cache evicts down to the new bound, visible in stats:
curl -s 'localhost:9200/_plugins/_knn/stats?pretty' \
  | grep -E 'graph_memory|circuit_breaker|eviction'
# Lower the limit at runtime and confirm cache_capacity reflects it WITHOUT a restart:
curl -XPUT 'localhost:9200/_cluster/settings' -H 'Content-Type: application/json' \
  -d '{ "persistent": { "knn.memory.circuit_breaker.limit": "2kb" } }'
curl -s 'localhost:9200/_plugins/_knn/stats?pretty' | grep -E 'cache_capacity|eviction'

A real fix changes the manual reproduction from "limit ignored until restart" to "limit honored immediately," with the eviction count climbing as the shrunk cache sheds entries.

Step 6: Ship the PR (DCO, CHANGELOG, compat)

git checkout -b fix/native-cb-limit-dynamic-update

# CHANGELOG entry — k-NN keeps a Keep-a-Changelog style CHANGELOG.md:
#   ### Fixed
#   - Honor dynamic updates to knn.memory.circuit_breaker.limit by rebuilding the
#     native-memory cache bound at runtime ([#NNNN](https://github.com/opensearch-project/k-NN/pull/NNNN))

git add src/main/java/org/opensearch/knn/index/KNNSettings.java \
        src/main/java/org/opensearch/knn/index/memory/NativeMemoryCacheManager.java \
        src/test/java/org/opensearch/knn/index/memory/NativeMemoryCircuitBreakerLimitTests.java \
        CHANGELOG.md

# DCO sign-off is mandatory in opensearch-project repos (-s adds Signed-off-by):
git commit -s -m "Honor dynamic native-memory circuit-breaker limit updates

The dynamic settings update for knn.memory.circuit_breaker.limit was
registered but never propagated to the live NativeMemoryCacheManager, so
a runtime limit change was ignored until restart. Rebuild the cache on
update (invalidateAll -> JNI free, no native leak) so the new bound takes
effect immediately. Adds a regression test asserting the cache re-bounds
without loading a native graph.

Relates to #585 and the rearchitecture discussion in #1582."

Note on compatibility (Stage 8): changing settings behavior is a compatibility surface. The fix honors the documented contract (the limit is dynamic), so it is a bug fix, not a breaking change — but in the PR, state explicitly that the setting key, type, and default are unchanged. See Issue Roadmap Stage 8: Plugin Compatibility for what reviewers check: setting renames, default changes, and BWC of serialized state are the things that get a PR blocked.

Pitfalls

Expected Output

Before the fix, after setting the limit to "1kb" and warming a graph:

"graph_memory_usage" : 37,            # KB, well over the 1kb limit
"circuit_breaker_triggered" : false,  # WRONG — limit silently ignored
"cache_capacity_reached" : false

After the fix:

"graph_memory_usage" : 0,             # cache evicted down to the new bound
"circuit_breaker_triggered" : true,
"cache_capacity_reached" : true,
"eviction_count" : 1                  # the shrink fired the RemovalListener (JNI free)

And the regression test:

> Task :test
org.opensearch.knn.index.memory.NativeMemoryCircuitBreakerLimitTests >
  testDynamicLimitUpdateRebuildsCacheBound PASSED

BUILD SUCCESSFUL

Stretch Goals

1. Fix the other failure mode. Reconstruct the mis-parse variant: make parseknnMemoryCircuitBreakerValue resolve a percentage against the wrong base, write a test that pins the byte count for both "60%" and "4gb", and fix it. Compare to the stale-limit fix — which is easier to catch in review, and why?
2. Read the real #585. Open k-NN #585, find the actual merged fix PR, and diff your approach against the maintainers'. What did they do that you didn't (debounce? a dedicated config object? a test you missed)?
3. Engage #1582. Read k-NN #1582 and write two paragraphs on how a clean rearchitecture would make your fix unnecessary — separating the breaker policy from the cache mechanism so a limit change does not require a cache rebuild at all.
4. Find a live one. Search https://github.com/opensearch-project/k-NN/issues?q=is%3Aissue+is%3Aopen+label%3Abug+circuit+breaker and ... memory cache eviction for a current, real defect in this subsystem and take it.

Validation / Self-check

1. What is the difference between the setting being stored and the setting being applied? Which side was broken in this lab, and why is that invisible from GET /_cluster/settings?
2. Why must rebuildCache() call invalidateAll() on the old cache? What leaks if it doesn't, and why won't the GC catch that leak?
3. Why does the unit test assert on the cache's configured bound instead of loading a real faiss graph and watching it get evicted?
4. Why is RSS a misleading signal for whether the breaker worked? What is the reliable signal instead?
5. Is the native-memory circuit-breaker limit a percentage of -Xmx or of node RAM? How does that change the failure analysis for an OOM-killed node?
6. What two CI gates will reject your PR if you forget them, and what does each enforce?
7. How does your point fix relate to the rearchitecture in #1582? When you propose a point fix in a subsystem with an open redesign, what should the PR description say?

When you can reproduce the symptom, locate the seam in KNNSettings/NativeMemoryCacheManager, fix it without leaking native memory, prove it with a non-leaking regression test, and ship it with DCO + CHANGELOG, you have done real k-NN maintenance work. Revisit native integration and memory for the full breaker model, and Stage 8: Plugin Compatibility for the compat lens every settings change is read through. Then prove your fix doesn't regress performance in lab-k6: Benchmark Recall and Latency.