apache / apache/polaris

InMemoryEntityCache: byName map is unbounded and can OOM the JVM

Open
#5,386 5 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Java
Stars
2.1k
Forks
522
Avg merge
1d 22h
Merged PRs (30d)
137

Description

## Describe the bug

We're experiencing what looks to be a memory leak which leads to OOM. I had an agent assist with the following write up:

**Environment.** Polaris `1.3.0-incubating` REST catalog, one instance, 4 GiB pod,
`MaxRAMPercentage=80`, OpenJDK. Steady production traffic, roughly 1,500 table GETs
(`GET /v1/{prefix}/namespaces/{namespace}/tables/{table}`) per 5-minute window plus
normal Iceberg operations.

**Observed.** Heap used by long-lived objects (the JVM old generation) grows in a
sawtooth from ~900 MB to ~2.2 GB until `ExitOnOutOfMemoryError` kills the JVM. After
a full GC, ~57% of the heap is still in use, so the floor keeps climbing across
crash/restart cycles. Container memory peaks at 2.8–3.3 GB of 4 GiB. Metric:
`jvm_memory_used_bytes{area="heap",id="PS Old Gen"}` (the `id` value depends on the
garbage collector in use; with G1 it appears as `G1 Old Gen`).

## Suspected Root cause

`polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java`
keeps the same entities in two maps with different lifetimes:

- `byId` (L62): Caffeine cache — `maximumWeight` (L94, default 100 MiB via
`ENTITY_CACHE_WEIGHER_TARGET`, `FeatureConfiguration` L494–502), `EntityWeigher`
(L95), `expireAfterAccess(1h)` (L96), removal listener (L97), optional
`softValues` gated on `ENTITY_CACHE_SOFT_VALUES` (L99–103, default false,
`BehaviorChangeConfiguration` L64). Bounded.
- `byName` (L63): plain `ConcurrentHashMap` (constructed L77). No size, weight, or
TTL. Its only removal path is the `byId` removal listener (L80–89): when an entry
leaves `byId` it calls the two-arg, value-conditional `byName.remove(nameKey,
value)` (L87), which no-ops if `byName` currently holds a *different* object.

The two maps diverge on every concurrent double load. `cacheNewEntry` (L134, called
by all load paths) `merge`s into `byId` (L143–148), keeping the *first* object when
versions are equal (`isNewer` L174–178 uses strict `>`), then does an unconditional
`byName.put(nameKey, entry)` (L154). So when two requests race on the same name while
the cache is cold (right after an eviction, or right after a restart), each loads its
own `ResolvedPolarisEntity`: `byId` keeps the first one, `byName` ends up pointing at
the last.

When `byId` later evicts its object, the listener's value-conditional remove (L87)
matches nothing, and the name key plus its entry stay in `byName` while `byId` is
empty. That is the orphan.

`getEntityByName` is a bare `byName.get` (L240–243); there is no sweep and no TTL on
`byName`. The class javadoc (L56) still claims "a limit of 100k entities and a 1h TTL"; no
100k-entity limit is configured anywhere (the `byId` bound is a weight budget, not a count),
and the TTL does not apply to `byName`. The doc is stale in both directions.

Single-threaded access is clean: evict the object `byId` holds and the listener removes
the matching `byName` entry. The divergence requires a concurrent double load, which is
why quiet deployments probably never hit this and it surfaces under real load.

## Amplifiers

1. `EntityWeigher` weighs only the entity name and properties lengths plus a fixed
1000-byte overhead; the grant lists carried by `ResolvedPolarisEntity`
(`grantRecordsAsGrantee`, `grantRecordsAsSecurable` — two `List`)
are entirely unweighed. In a permission-heavy catalog the 100 MiB `byId` budget
badly underestimates real residency, so weight-pressure eviction fires earlier
than memory warrants — more evictions, more racing reloads, more divergences — and
each retained entry is large, since it carries both grant lists.
2. Every table GET resolves several names (each namespace level, the table, the caller
principal, each caller role), so the set of distinct `byName` keys accumulates over
time and never shrinks.

## Reproduction

Point a heavy load at a single instance with a permission-heavy catalog:
bursts of concurrent table GETs (e.g. 50–200 concurrent against the same set of
tables), repeated over an hour. Watch old-generation heap usage, or better, dump the
`InMemoryEntityCache.byName` map: entries whose value is a different object than
`byId.get(entityId)` (more than one `ResolvedPolarisEntity` per id) are the leak.

## Proposed fix

Two independent changes, in order of invasiveness, **plus** two follow-ons:

1. **Root fix.** In `cacheNewEntry`, after the `byId.asMap().merge(...)`, store the
object `byId` actually holds in `byName` (read it back via
`byId.getIfPresent(id)`) instead of the locally loaded object. This removes the
identity divergence at the source and makes the existing removal listener correct
again. Implementation gotchas:
- The name key must be computed from the *read-back* object, not the locally loaded
one — a concurrent rename can win the `merge`, and putting the renamed entity
under the old name key would be a new divergence.
- If the read-back is `null` (concurrent eviction between merge and read-back), skip
the `byName` put.
- This shrinks but does not eliminate the race: an eviction of the read-back object
between `getIfPresent` and `put` can still form an orphan. The window drops from
"any concurrent double load" to "eviction interleaving within nanoseconds" —
acceptable, and item 2 bounds the residue regardless.
2. **Bound.** Give `byName` the same guarantees as `byId`: a Caffeine cache with the
same weigher and target and a 1h `expireAfterAccess`. This bounds the map in
steady state and self-cleans stale keys without waiting for a version bump.
Correctness is already preserved by the resolver's per-request change-tracking
re-validation, so a stale `byName` entry costs a validation round, not a wrong
answer. Note: with the same knob, the combined `byId` + `byName` weight budget is
2× the target — call this out (or give `byName` a fraction).
3. **Amplifier fix.** Weigh the grant lists in `EntityWeigher` (fixes the
eviction-rate amplifier).
4. **Doc fix.** Correct the stale L56 javadoc.
5. **Regression test.** The existing `InMemoryEntityCacheTest` (covers hit/miss,
rename, weigher, batch concurrency L997–1021) has **no test** for the
double-load divergence → eviction → orphan scenario. The upstream patch must add
one (deterministic divergence seed; force eviction; assert the invariant).

## Versions

** The bug appears to span 1.1.0 → main.**

Contributor guide

Open the contributing guide

Research direction

Start with polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java, especially cacheNewEntry, the byId removal listener, getEntityByName, and EntityWeigher. Then read InMemoryEntityCacheTest, including its weigher and batch-concurrency coverage, and add a deterministic regression for double-load divergence followed by eviction. Done means byName remains bounded and does not retain an orphaned entity after eviction, with the documented behavior and relevant tests updated.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
65/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.