Merge pull request #594 from Lob26/fix/terrain-height-cache-bound

fix(terrain): bound the client terrain-height cache with LRU eviction (closes #327)

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Changelog
 
+- Bound the client terrain-height cache at 20 000 entries with least-recently-used eviction, so a long session no longer retains every coordinate it ever resolved. Consumer reads promote their entry and a batch still reports every point it resolved (Pedro Lobato, #594).
+
 - Release CCTV media streams whose upstream falls silent after answering. The
   15-second media deadline covered only the wait for response headers, so a
   camera that replied and then stopped sending held both the proxy connection

docs/CURRENT-STATE.md

@@ -879,6 +879,22 @@ under `gods-eye-view/server/providers/`. Portable terrain mechanics, traffic til
 math and GBFS source rules are available under `gods-eye-view/sources/`.
 The browser layers and their rendering remain in their existing modules.
 
+## Terrain height cache bound
+
+The client terrain-height resolver's in-memory cache is bounded at 20 000
+entries (`DEFAULT_MAX_CACHE_ENTRIES`, overridable per instance through
+`createTerrainHeights({ maxCacheEntries })`). Coordinate rounding makes repeated
+visits to one place share a key but does not bound how many distinct places a
+session visits, and the cache previously had no eviction, so a long session
+retained every coordinate it ever resolved. Eviction is least-recently-used:
+a consumer read promotes its entry, so the cell being watched is not dropped for
+having been resolved early. The bound sits far above a city-scale session, so
+normal use never evicts and issues no additional proxy requests. A batch's
+results are assembled from what that call resolved, so eviction during a large
+batch cannot report a just-resolved point as unresolved. Re:Earth and
+geoid-fallback entry semantics, the fallback cooldown and the abort-time flush
+are unchanged.
+
 ## Landmark annotation identity
 
 When a landmark geocode contains only address components, annotations retain

scripts/format-scope.json

@@ -633,6 +633,7 @@
   "src/data/surfaceServices.js",
   "src/annotations/resolver.js",
   "src/services/terrainHeights.js",
+  "src/services/terrainHeights.test.mjs",
   "src/services/groundFloor.js",
   "src/services/meshFloorSampler.js",
   "src/services/groundSnap.js",

src/services/terrainHeights.js

@@ -1,7 +1,19 @@
 import { ensureGeoidReady, geoidHeight } from '../data/geoid.js';
 
-/** Construct an instance-owned terrainHeights service with explicit dependencies. */
-export function createTerrainHeights({ source, signal }) {
+/** Default live-entry ceiling for the in-memory cache (see CACHE_MAX_ENTRIES). */
+export const DEFAULT_MAX_CACHE_ENTRIES = 20_000;
+
+/**
+ * Construct an instance-owned terrainHeights service with explicit dependencies.
+ * `maxCacheEntries` overrides the cache bound; it exists so the bound is
+ * testable without minting twenty thousand coordinates, mirroring the injectable
+ * `maxBytes` on the GBFS reader.
+ */
+export function createTerrainHeights({
+  source,
+  signal,
+  maxCacheEntries = DEFAULT_MAX_CACHE_ENTRIES,
+}) {
   if (typeof source?.getHeights !== 'function')
     throw new TypeError('Terrain heights require a source');
   signal?.throwIfAborted();
@@ -45,15 +57,71 @@ export function createTerrainHeights({ source, signal }) {
   /** Avoid repeatedly hitting a known-failing proxy from warm fallback reads. */
   const GEOID_FALLBACK_COOLDOWN_MS = 60_000;
 
+  /**
+   * Live entry ceiling for the cache below. The rounding in point 2 of the file
+   * header makes repeated visits to one place share a key; it does nothing to
+   * bound the number of DISTINCT places a session visits, and this cache had no
+   * eviction, so a long session kept every coordinate it ever resolved for the
+   * lifetime of the page.
+   *
+   * This is a bound, not a budget. At the ~111 m floor grid a city-scale
+   * session stays well under it, so eviction is the pathological case rather
+   * than the normal one and a well-behaved session sees no extra network. Sized
+   * above the sibling caches (adsb.lol points 80, track history 200, TomTom
+   * tiles 256) because a miss here costs a proxy round trip, not just RAM.
+   */
+  const CACHE_MAX_ENTRIES =
+    Number.isFinite(maxCacheEntries) && maxCacheEntries > 0
+      ? Math.floor(maxCacheEntries)
+      : DEFAULT_MAX_CACHE_ENTRIES;
+
   /**
    * In-memory cache: `"lat.toFixed(5),lon.toFixed(5)"` -> `{ellipsoid, source}`.
    * Module-scoped (not exported) — the only reads are through
    * `cachedEllipsoidalGround` and the internal lookup in
-   * `resolveEllipsoidalGround`.
+   * `resolveEllipsoidalGround`. Bounded at CACHE_MAX_ENTRIES; write through
+   * `cacheStore` and read a consumer-facing hit through `cacheTouch` so the
+   * eviction order stays least-recently-used.
    * @type {Map<string, {ellipsoid: number, source: 'reearth'|'geoid-fallback', retryAt?: number}>}
    */
   const cache = new Map();
 
+  /**
+   * Store an entry and drop the coldest ones once past the cap. `Map` iterates
+   * in insertion order, so deleting the first key evicts the least recently
+   * used provided every write and every consumer read re-inserts its key.
+   *
+   * Recency rather than plain insertion order because revisits here are
+   * spatial: a session comes back to cells near where it already was, so the
+   * cell it is parked on must not be evicted merely for having been resolved
+   * early.
+   * @param {string} key
+   * @param {{ellipsoid: number, source: 'reearth'|'geoid-fallback', retryAt?: number}} entry
+   */
+  function cacheStore(key, entry) {
+    cache.delete(key);
+    cache.set(key, entry);
+    while (cache.size > CACHE_MAX_ENTRIES) {
+      const coldest = cache.keys().next().value;
+      if (coldest === undefined) break;
+      cache.delete(coldest);
+    }
+  }
+
+  /**
+   * Read an entry and promote it to newest, so a coordinate a consumer keeps
+   * asking about survives eviction. Returns undefined on a miss.
+   * @param {string} key
+   * @returns {{ellipsoid: number, source: 'reearth'|'geoid-fallback', retryAt?: number}|undefined}
+   */
+  function cacheTouch(key) {
+    const entry = cache.get(key);
+    if (entry === undefined) return undefined;
+    cache.delete(key);
+    cache.set(key, entry);
+    return entry;
+  }
+
   /**
    * Builds the rounded cache key shared between the in-memory cache and the
    * `points` string sent to the proxy (see file header, point 2).
@@ -74,7 +142,7 @@ export function createTerrainHeights({ source, signal }) {
    * @returns {number|null}
    */
   function cachedEllipsoidalGround(lat, lon) {
-    const entry = cache.get(cacheKey(lat, lon));
+    const entry = cacheTouch(cacheKey(lat, lon));
     return entry ? entry.ellipsoid : null;
   }
 
@@ -91,7 +159,7 @@ export function createTerrainHeights({ source, signal }) {
    * @returns {number|null}
    */
   function cachedRealEllipsoidalGround(lat, lon) {
-    const entry = cache.get(cacheKey(lat, lon));
+    const entry = cacheTouch(cacheKey(lat, lon));
     return entry && entry.source === 'reearth' ? entry.ellipsoid : null;
   }
 
@@ -213,6 +281,12 @@ export function createTerrainHeights({ source, signal }) {
       uncached.push(item);
     }
 
+    // What this call resolved, so the assembly below can report a point even
+    // if a later chunk's writes pushed it past CACHE_MAX_ENTRIES. Without it a
+    // batch big enough to evict its own earlier chunks would report points as
+    // unresolved that it had in fact just resolved.
+    const resolvedThisCall = new Map();
+
     // Resolve the network path in sequential <=CHUNK_SIZE chunks. Each chunk's
     // failure is isolated to that chunk's points (geoid fallback), so a single
     // bad chunk doesn't lose results for the rest of a large batch.
@@ -230,7 +304,9 @@ export function createTerrainHeights({ source, signal }) {
           // one contact frozen at the geoid while its neighbors resolved).
           // An omitted point now caches nothing and retries on the next warm.
           if (Number.isFinite(ellipsoid)) {
-            cache.set(item.key, { ellipsoid, source: 'reearth' });
+            const entry = { ellipsoid, source: 'reearth' };
+            resolvedThisCall.set(item.key, entry);
+            cacheStore(item.key, entry);
           }
         }
       } catch {
@@ -247,11 +323,13 @@ export function createTerrainHeights({ source, signal }) {
             item.lon,
             item.sourceOrthometricM,
           );
-          cache.set(item.key, {
+          const entry = {
             ellipsoid,
             source: 'geoid-fallback',
             retryAt: Date.now() + GEOID_FALLBACK_COOLDOWN_MS,
-          });
+          };
+          resolvedThisCall.set(item.key, entry);
+          cacheStore(item.key, entry);
         }
       }
     }
@@ -260,7 +338,7 @@ export function createTerrainHeights({ source, signal }) {
     // the upstream omitted has NO entry (round 6 — deliberately uncached so it
     // retries later): report it unresolved instead of throwing.
     return work.map((item) => {
-      const entry = cache.get(item.key);
+      const entry = cache.get(item.key) || resolvedThisCall.get(item.key);
       return entry
         ? { ellipsoid: entry.ellipsoid, source: entry.source }
         : { ellipsoid: null, source: 'unresolved' };

src/services/terrainHeights.test.mjs

@@ -0,0 +1,130 @@
+import { test } from 'node:test';
+import assert from 'node:assert/strict';
+import {
+  createTerrainHeights,
+  DEFAULT_MAX_CACHE_ENTRIES,
+} from './terrainHeights.js';
+
+/**
+ * A terrain source that always resolves, so these tests exercise the cache and
+ * never the geoid-fallback path. Heights are derived from the coordinate so an
+ * assertion can name the value it expects.
+ */
+function stubSource() {
+  const batches = [];
+  return {
+    batches,
+    async getHeights(chunk) {
+      batches.push(chunk.map((item) => item.key));
+      return chunk.map((item) => ({ ellipsoid: heightAt(item.lon) }));
+    },
+  };
+}
+
+/** Distinct 5-decimal coordinates along the equator: 0.001, 0.002, … */
+const lonAt = (i) => i / 1000;
+const heightAt = (lon) => 1000 + Math.round(lon * 1000);
+const pointAt = (i) => ({ lat: 0, lon: lonAt(i) });
+
+test('the default cache bound is exported so callers can reason about it', () => {
+  assert.equal(DEFAULT_MAX_CACHE_ENTRIES, 20_000);
+});
+
+test('the in-memory cache stops growing once it reaches its entry bound', async () => {
+  const terrain = createTerrainHeights({
+    source: stubSource(),
+    maxCacheEntries: 4,
+  });
+
+  // Six distinct coordinates through a cache that holds four.
+  for (let i = 0; i < 6; i += 1) {
+    await terrain.resolveEllipsoidalGround([pointAt(i)]);
+  }
+
+  // The two coldest are gone. Before the bound existed the cache kept every
+  // coordinate a session ever resolved, so both of these still answered.
+  assert.equal(terrain.cachedEllipsoidalGround(0, lonAt(0)), null);
+  assert.equal(terrain.cachedEllipsoidalGround(0, lonAt(1)), null);
+
+  // The four most recent are untouched — a bound, not a flush.
+  for (let i = 2; i < 6; i += 1) {
+    assert.equal(
+      terrain.cachedEllipsoidalGround(0, lonAt(i)),
+      heightAt(lonAt(i)),
+      `coordinate ${i} stayed warm`,
+    );
+  }
+});
+
+test('a coordinate a consumer keeps reading outlives an equally old one it ignores', async () => {
+  const terrain = createTerrainHeights({
+    source: stubSource(),
+    maxCacheEntries: 4,
+  });
+  for (let i = 0; i < 4; i += 1) {
+    await terrain.resolveEllipsoidalGround([pointAt(i)]);
+  }
+
+  // Read coordinate 0 and ignore coordinate 1. They are otherwise the same age,
+  // so insertion order alone could not tell them apart.
+  assert.equal(
+    terrain.cachedEllipsoidalGround(0, lonAt(0)),
+    heightAt(lonAt(0)),
+  );
+
+  // Two fresh coordinates push the cache two entries past its bound.
+  for (let i = 4; i < 6; i += 1) {
+    await terrain.resolveEllipsoidalGround([pointAt(i)]);
+  }
+
+  assert.equal(
+    terrain.cachedEllipsoidalGround(0, lonAt(0)),
+    heightAt(lonAt(0)),
+    'the cell being read survived eviction',
+  );
+  assert.equal(
+    terrain.cachedEllipsoidalGround(0, lonAt(1)),
+    null,
+    'the same-age cell nobody read was evicted instead',
+  );
+});
+
+test('a batch larger than the cache bound still reports every point it resolved', async () => {
+  const terrain = createTerrainHeights({
+    source: stubSource(),
+    maxCacheEntries: 4,
+  });
+  const batch = Array.from({ length: 10 }, (_, i) => pointAt(i));
+
+  const results = await terrain.resolveEllipsoidalGround(batch);
+
+  // Eviction inside the call must not turn a point this batch resolved into an
+  // unresolved one: the results are assembled from what the call produced, not
+  // from whatever survived in the cache by the time it finished.
+  assert.equal(results.length, batch.length);
+  results.forEach((result, i) => {
+    assert.equal(result.source, 'reearth', `point ${i} reported resolved`);
+    assert.equal(result.ellipsoid, heightAt(lonAt(i)));
+  });
+
+  // The cache still honours the bound afterwards.
+  const warm = batch.filter(
+    (point) => terrain.cachedEllipsoidalGround(point.lat, point.lon) !== null,
+  );
+  assert.equal(warm.length, 4);
+});
+
+test('an out-of-range cache bound falls back to the default instead of disabling the cache', async () => {
+  for (const maxCacheEntries of [0, -1, Number.NaN, undefined]) {
+    const terrain = createTerrainHeights({
+      source: stubSource(),
+      maxCacheEntries,
+    });
+    await terrain.resolveEllipsoidalGround([pointAt(1)]);
+    assert.equal(
+      terrain.cachedEllipsoidalGround(0, lonAt(1)),
+      heightAt(lonAt(1)),
+      `maxCacheEntries=${String(maxCacheEntries)} kept a working cache`,
+    );
+  }
+});