fix(terrain): keep cold height requests within the browser deadline (#348)

Use 64-point browser and upstream terrain batches to reduce cold-cache timeouts. Preserve successful chunk results, distinguish absent readings from malformed responses, and reject fabricated zero heights.

Original terrain resilience contribution by ItzDESTAQ, with browser batching, validation and regression follow-up by Sameh Khamis.

Validated with unit/build/format/boundary checks, 108 tracking checks, 5 terrain-outage checks, controlled slow-upstream comparison and live browser checks.

Co-authored-by: ItzDESTAQ <[email protected]>
Co-authored-by: Sameh Khamis <[email protected]>

src/data/terrainHeights.js

@@ -17,7 +17,7 @@
 //      single request much past ~700-1500 points (depending on coordinate
 //      precision) risks a raw socket-level 431 *before* the proxy's own
 //      request handler runs. So every network request this module issues is
-//      chunked at CHUNK_SIZE (200 points), well under that ceiling, and
+//      chunked at CHUNK_SIZE (64 points), well under that ceiling, and
 //      chunks are sent SEQUENTIALLY (not in parallel) to keep this client
 //      well-behaved against a single dev-server proxy.
 //   2. The proxy's disk cache is keyed by the raw `points` query string, with
@@ -32,7 +32,9 @@
 import { ensureGeoidReady, geoidHeight } from './geoid.js';
 
 /** Max points per outgoing request to `/api/terrain/heights` (see file header, point 1). */
-const CHUNK_SIZE = 200;
+// Match the server's upstream batch so sequential upstream work also fits
+// within this client's 30-second request deadline when Re:Earth slows.
+const CHUNK_SIZE = 64;
 
 /** Avoid repeatedly hitting a known-failing proxy from warm fallback reads. */
 const GEOID_FALLBACK_COOLDOWN_MS = 60_000;
@@ -116,7 +118,7 @@ async function fetchChunk(chunk) {
   // the response's own lon/lat fields to re-match — those are also present
   // for callers who want them, but this module doesn't need them.
   for (let i = 0; i < chunk.length; i += 1) {
-    const ellipsoid = Number(body.results[i]?.ellipsoid);
+    const ellipsoid = body.results[i]?.ellipsoid;
     if (!Number.isFinite(ellipsoid)) throw new Error(`non-finite ellipsoid height at index ${i}`);
     out.set(chunk[i].key, ellipsoid);
   }
@@ -145,7 +147,7 @@ function geoidFallback(lat, lon, sourceOrthometricM) {
  * - Results already warm in the in-memory cache are returned without any
  *   network call.
  * - Remaining (deduplicated) coordinates are sent to `/api/terrain/heights`
- *   in sequential chunks of <=200 points.
+ *   in sequential chunks of <=64 points.
  * - If a chunk request fails (network error, non-ok HTTP, malformed body),
  *   every point in THAT chunk falls back to geoid math
  *   (`sourceOrthometricM + geoidHeight` or `geoidHeight` alone) rather than

src/data/terrainHeights.test.mjs

@@ -79,7 +79,7 @@ test('resolveEllipsoidalGround: happy path maps results IN ORDER', async () => {
   assert.ok(capturedUrl.includes('points='));
 });
 
-test('resolveEllipsoidalGround: a 300-point input issues exactly 2 fetches (chunked at <=200/request) AND preserves order across the chunk boundary', async () => {
+test('resolveEllipsoidalGround: a 300-point input issues exactly 5 fetches (chunked at <=64/request) AND preserves order across the chunk boundary', async () => {
   // Each coord carries a DISTINGUISHABLE lat/lon derived from its index so
   // the oracle below can be an independent function of the point's INPUT
   // identity — not an echo of whatever the request happened to send. This is
@@ -119,11 +119,10 @@ test('resolveEllipsoidalGround: a 300-point input issues exactly 2 fetches (chun
     async () => {
       const out = await resolveEllipsoidalGround(coords);
       assert.equal(out.length, 300);
-      // Positional correctness at and across the chunk boundary (chunk 1 =
-      // [0,199], chunk 2 = [200,299]): each output must equal the oracle of
+      // Positional correctness across every chunk boundary: outputs equal the oracle of
       // its OWN input coord. The module rounds to 5dp before sending, so
       // compare against the oracle of the rounded coord to stay exact.
-      for (const k of [0, 1, 199, 200, 299]) {
+      for (let k = 0; k < coords.length; k += 1) {
         const lat = Number(coords[k].lat.toFixed(5));
         const lon = Number(coords[k].lon.toFixed(5));
         assert.equal(
@@ -135,9 +134,10 @@ test('resolveEllipsoidalGround: a 300-point input issues exactly 2 fetches (chun
       }
     }
   );
-  assert.equal(fetchCount, 2, `expected 2 fetches for 300 points at <=200/chunk, got ${fetchCount}`);
+  assert.equal(fetchCount, 5);
+  assert.deepEqual(chunkSizes, [64, 64, 64, 64, 44]);
   for (const size of chunkSizes) {
-    assert.ok(size <= 200, `chunk size ${size} exceeds the 200-point ceiling`);
+    assert.ok(size <= 64, `chunk size ${size} exceeds the 64-point ceiling`);
   }
   assert.equal(
     chunkSizes.reduce((a, b) => a + b, 0),
@@ -350,3 +350,15 @@ test('resolveEllipsoidalGround: an empty input array resolves to an empty array
   );
   assert.equal(fetchCount, 0);
 });
+
+test('resolveEllipsoidalGround: null and string heights cannot become real zeroes', async () => {
+  for (const [index, value] of [null, '', '0'].entries()) {
+    await withFakeFetch(
+      async () => ({ ok: true, json: async () => ({ results: [{ ellipsoid: value }] }) }),
+      async () => {
+        const out = await resolveEllipsoidalGround([{ lat: 11 + index, lon: 41 }]);
+        assert.equal(out[0].source, 'geoid-fallback');
+      }
+    );
+  }
+});

src/data/terrainHeightsProxy.js

@@ -60,6 +60,28 @@ export function validTerrainResult(result) {
   return Boolean(result) && Number.isFinite(result.ellipsoid);
 }
 
+/**
+ * A row the upstream DID return for its position, but with no usable height:
+ * Re:Earth sporadically answers with a real `elevation` and a null
+ * `geoid`/`ellipsoid`. Measured at ~0.16% of points, and transient — the same
+ * coordinate resolves on a later poll. That is an absent datum, not a failed
+ * refresh, so the point is left uncached to be re-asked while the client
+ * covers it through its bundled geoid. A null/undefined entry is NOT this
+ * case: it means the position was dropped from the response, which is a
+ * genuine upstream fault.
+ * @param {unknown} result
+ */
+export function absentTerrainDatum(result) {
+  return (
+    typeof result === 'object' &&
+    result !== null &&
+    !Array.isArray(result) &&
+    result.ellipsoid === null &&
+    result.geoid === null &&
+    Number.isFinite(result.elevation)
+  );
+}
+
 /**
  * Convert Retry-After (delta-seconds or HTTP-date) to milliseconds.
  * @param {string|null|undefined} value
@@ -194,24 +216,33 @@ export async function resolveTerrainHeightRequest({
 
   let cacheChanged = false;
   let upstreamError = null;
+  let absentPoints = 0;
   if (missing.length > 0) {
     try {
       const fetched = await fetchMissing(missing.map((item) => item.point));
       if (!Array.isArray(fetched))
         throw new Error('malformed upstream response (no results array)');
       const fetchedAt = now();
+      let omittedPositions = 0;
       for (let i = 0; i < missing.length; i += 1) {
         const result = fetched[i];
-        if (!validTerrainResult(result)) continue;
-        cache.set(missing[i].key, { at: fetchedAt, result });
-        cacheChanged = true;
+        if (validTerrainResult(result)) {
+          cache.set(missing[i].key, { at: fetchedAt, result });
+          cacheChanged = true;
+        } else if (absentTerrainDatum(result)) {
+          // Upstream answered but had no height. Transient, so deliberately
+          // left uncached: the next poll re-asks and usually succeeds.
+          absentPoints += 1;
+        } else {
+          // Missing or malformed positions remain upstream faults.
+          omittedPositions += 1;
+        }
       }
-      if (
-        fetched.length !== missing.length ||
-        missing.some((_, i) => !validTerrainResult(fetched[i]))
-      ) {
+      if (omittedPositions > 0 || fetched.length !== missing.length) {
+        const dropped =
+          omittedPositions || Math.abs(fetched.length - missing.length);
         upstreamError = new Error(
-          'upstream omitted one or more terrain heights',
+          `upstream omitted ${dropped} terrain position(s)`,
         );
       }
     } catch (error) {
@@ -232,6 +263,8 @@ export async function resolveTerrainHeightRequest({
       },
       cacheChanged,
       upstreamError,
+      absentPoints,
+      requestedPoints: unique.size,
     };
   }
   return {
@@ -239,5 +272,7 @@ export async function resolveTerrainHeightRequest({
     body: { results },
     cacheChanged,
     upstreamError,
+    absentPoints,
+    requestedPoints: unique.size,
   };
 }

src/data/terrainHeightsProxy.test.mjs

@@ -145,3 +145,85 @@ test('stale per-point entries serve through a failed refresh only when every poi
     ['stale-b', 'stale-a'],
   );
 });
+
+test('a null-height row counts as absent, not a refresh failure, and is re-asked next poll', async () => {
+  const cache = new Map();
+  let calls = 0;
+  const options = {
+    points: [
+      [-97.428, 40.194],
+      [139.729, 35.693],
+    ],
+    cache,
+    ttlMs: 30 * 24 * 3600_000,
+    now: () => 1_000_000,
+    // Re:Earth's observed transient shape: real elevation, null geoid/ellipsoid.
+    fetchMissing: async (missing) => {
+      calls += 1;
+      return missing.map(([lon, lat]) =>
+        lon === -97.428
+          ? { lon, lat, elevation: 443.855, geoid: null, ellipsoid: null }
+          : result('real', 69.93),
+      );
+    },
+  };
+
+  const first = await resolveTerrainHeightRequest(options);
+  assert.equal(
+    first.upstreamError,
+    null,
+    'upstream answered every position; absence is not its fault',
+  );
+  assert.equal(first.absentPoints, 1);
+  assert.equal(first.requestedPoints, 2);
+  assert.equal(
+    first.status,
+    502,
+    'still no usable height, and none may be fabricated',
+  );
+  assert.equal(
+    cache.has(terrainPointKey([-97.428, 40.194])),
+    false,
+    'absent points stay uncached',
+  );
+  assert.equal(
+    cache.get(terrainPointKey([139.729, 35.693])).result.id,
+    'real',
+    'a batch-mate with a real height still caches',
+  );
+
+  await resolveTerrainHeightRequest(options);
+  assert.equal(calls, 2, 'a transient null must be re-asked, never suppressed');
+});
+
+test('a dropped position remains a genuine upstream fault', async () => {
+  const response = await resolveTerrainHeightRequest({
+    points: [
+      [1, 1],
+      [2, 2],
+    ],
+    cache: new Map(),
+    ttlMs: 10_000,
+    now: () => 5_000,
+    fetchMissing: async () => [result('ok-a', 11)],
+  });
+
+  assert.notEqual(response.upstreamError, null);
+  assert.match(response.upstreamError.message, /omitted/);
+  assert.equal(response.absentPoints, 0, 'an omission is not an absence');
+});
+
+test('malformed height objects remain refresh failures, not absent readings', async () => {
+  for (const row of [{}, [], { ellipsoid: '0' }, { ellipsoid: null }]) {
+    const response = await resolveTerrainHeightRequest({
+      points: [[1, 2]],
+      cache: new Map(),
+      ttlMs: 10_000,
+      fetchMissing: async () => [row],
+    });
+    assert.equal(response.status, 502);
+    assert.ok(response.upstreamError);
+    assert.equal(response.absentPoints, 0);
+    assert.equal(response.cacheChanged, false);
+  }
+});

src/tooling/environmentProviders.test.mjs

@@ -88,18 +88,22 @@ test('terrain middleware chunks missing points and reconstructs repeated/reorder
   const points = Array.from({ length: 257 }, (_, i) => `${i / 100},1`);
   const res = await request('/?points=' + points.join(';'));
   assert.equal(res.status, 200);
-  assert.deepEqual(sizes, [256, 1]);
+  // Tracks UPSTREAM_CHUNK in server/providers/terrain.js (64): 257 points
+  // split into four full chunks and a remainder.
+  assert.deepEqual(sizes, [64, 64, 64, 64, 1]);
   const reordered = await request('/?points=2.56,1;0,1;2.56,1');
   assert.deepEqual(json(reordered), {
     results: [{ ellipsoid: 102.56 }, { ellipsoid: 100 }, { ellipsoid: 102.56 }],
   });
-  assert.equal(calls, 2);
+  // Five chunks for the first batch; the reordered request is fully cached.
+  assert.equal(calls, 5);
   assert.equal((await request('/?points=invalid')).status, 400);
   assert.equal(
     (await request('/?points=' + Array(2001).fill('0,1').join(';'))).status,
     500,
   );
-  assert.equal(calls, 2);
+  // Rejected requests add no upstream calls: still the five from the first batch.
+  assert.equal(calls, 5);
 });
 
 test('terrain middleware migrates valid legacy disk points without fabricating omitted heights', async (t) => {
@@ -254,3 +258,40 @@ test('GBFS keeps host/path/method guards, response caps and distinct information
   );
   assert.equal((await request(target('station_status.json'))).status, 502);
 });
+
+test('terrain middleware retains successful chunks around a failure and retries only missing points', async (t) => {
+  isolate(t);
+  const requests = [];
+  let fail = true;
+  t.mock.method(globalThis, 'fetch', async (raw) => {
+    const points = new URL(raw).searchParams
+      .get('points')
+      .split(';')
+      .map((p) => p.split(',').map(Number));
+    requests.push(points);
+    if (fail && points[0][0] === 0.64) return new Response('', { status: 400 });
+    return Response.json({
+      results: points.map(([lon]) => ({ ellipsoid: lon + 100 })),
+    });
+  });
+  const request = install(terrainHeightsProxy());
+  const points = Array.from({ length: 130 }, (_, i) => `${i / 100},1`);
+  const url = '/?points=' + points.join(';');
+  assert.equal((await request(url)).status, 502);
+  assert.deepEqual(
+    requests.map((p) => p.length),
+    [64, 64, 2],
+  );
+  fail = false;
+  const recovered = await request(url);
+  assert.equal(recovered.status, 200);
+  assert.deepEqual(
+    requests[3],
+    Array.from({ length: 64 }, (_, i) => [(i + 64) / 100, 1]),
+  );
+  assert.deepEqual(
+    json(recovered).results,
+    Array.from({ length: 130 }, (_, i) => ({ ellipsoid: i / 100 + 100 })),
+  );
+  assert.equal(requests.length, 4);
+});