Merge pull request #1276 from bilawalsidhu/codex/fix-terrain-request-bound

fix(terrain): bound each height request to one upstream chunk

changelog.d/1276-terrain-issue-fix.md

@@ -0,0 +1 @@
+- Bound each height request to one upstream chunk (asturksever, #1276).

docs/CURRENT-STATE.md

@@ -9,7 +9,10 @@ broader suffix variants. Follow-ups retain boundary provenance, and cancelled
 lookups preserve the previous analyst cohort and boundary source. Navigation
 search keeps a successful geocode, with no search or view-based region recovery.
 
-Wind and weather parameter changes may supersede a first-load request without rejecting the layer’s enable transaction. Obsolete responses cannot publish data; a replacement request remains visible through the loading state. Explicit caller cancellation, disable, and destroy still reject the obsolete update. Source failures continue to report their existing unavailable state.
+Terrain height requests accept at most 64 points, matching the browser service's
+existing batch size and one bounded upstream chunk. Larger requests return HTTP
+413 before disk-cache loading or upstream work; malformed coordinates remain 400.
+The browser continues to split larger workloads into sequential 64-point requests.
 
 Shared distance checks use `src/geoDistance.js` with `{ lat, lon }` points and
 the IUGG mean spherical Earth radius, 6371.0088 km. `greatCircleKm` is

server/providers/terrain.js

@@ -20,8 +20,8 @@ import { admitSameSiteKeyless } from './common/same-site.js';
  * cache and serve-stale shape. Cache entries and stale fallback are keyed per
  * 5dp point, so reordered and partially overlapping batches reuse prior work.
  * Only missing/stale points go upstream; the response is rebuilt in exact
- * request order. Larger requests are chunked sequentially, and one failing
- * chunk does not discard the chunks that resolved.
+ * request order. Each request is bounded to one 64-point upstream chunk;
+ * larger client workloads are split by the browser terrain service.
  *
  * Bounds, because every distinct 5dp point is its own key: coordinates must be
  * in WGS-84 range (rejected at parse), the point cache has an entry ceiling
@@ -55,7 +55,7 @@ export function terrainHeightsProxy(options = {}) {
   // 2026-08-21). 64 points costs 5.6-12 s, inside the timeout at both ends of
   // that range.
   const UPSTREAM_CHUNK = 64;
-  const MAX_POINTS = 2000;
+  const MAX_POINTS = UPSTREAM_CHUNK;
   /**
    * Entry ceiling for the point cache, and with it the file written from it.
    *
@@ -255,7 +255,6 @@ export function terrainHeightsProxy(options = {}) {
           res.end(JSON.stringify({ error: 'Rate limit exceeded' }));
           return;
         }
-        await loadDiskOnce();
         const parsedUrl = new URL(req.url || '', 'http://internal');
         const rawPoints = parsedUrl.searchParams.get('points');
         const points = parseTerrainPoints(rawPoints);
@@ -267,12 +266,13 @@ export function terrainHeightsProxy(options = {}) {
           return;
         }
         if (points.length > MAX_POINTS) {
-          send(500, {
+          send(413, {
             error: `too many points (${points.length}); max ${MAX_POINTS} per request`,
           });
           return;
         }
 
+        await loadDiskOnce();
         const outcome = await resolveTerrainHeightRequest({
           points,
           cache: mem,

src/terrainHeightsBounds.test.mjs

@@ -366,3 +366,24 @@ test('a second client keeps its own quota', async (t) => {
   });
   assert.notEqual(neighbour.statusCode, 429);
 });
+
+test('one 64-point chunk is accepted; larger batches fail before cache or upstream work', async (t) => {
+  const root = await mkdtemp(path.join(tmpdir(), 'terrain-batch-limit-'));
+  t.after(() => rm(root, { recursive: true, force: true }));
+  const asked = stubTerrainUpstream(t);
+  const route = install(
+    terrainHeightsProxy({ cachePath: path.join(root, 'cache.json') }),
+  ).get('/api/terrain/heights');
+  const points = (count) =>
+    Array.from({ length: count }, (_, i) => `${i / 1000},30`).join(';');
+  for (const count of [65, 2001]) {
+    const response = await request(route, { url: `/?points=${points(count)}` });
+    assert.equal(response.statusCode, 413);
+    assert.match(response.body.error, /max 64/);
+    assert.equal(asked.length, 0);
+  }
+  const response = await request(route, { url: `/?points=${points(64)}` });
+  assert.equal(response.statusCode, 200);
+  assert.equal(response.body.results.length, 64);
+  assert.equal(asked.length, 64);
+});

src/tooling/environmentProviders.test.mjs

@@ -98,7 +98,7 @@ test('standalone composition mounts every extracted provider exactly once withou
     assert.equal(plugins.filter((p) => p.name === factory().name).length, 1);
 });
 
-test('terrain middleware chunks missing points and reconstructs repeated/reordered requests from cache', async (t) => {
+test('terrain middleware bounds missing points and reconstructs repeated/reordered requests from cache', async (t) => {
   isolate(t);
   let calls = 0;
   const sizes = [];
@@ -116,25 +116,23 @@ test('terrain middleware chunks missing points and reconstructs repeated/reorder
     });
   });
   const request = install(terrainHeightsProxy());
-  const points = Array.from({ length: 257 }, (_, i) => `${i / 100},1`);
+  const points = Array.from({ length: 64 }, (_, i) => `${i / 100},1`);
   const res = await request('/?points=' + points.join(';'));
   assert.equal(res.status, 200);
-  // 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(sizes, [64]);
+  const reordered = await request('/?points=0.63,1;0,1;0.63,1');
   assert.deepEqual(json(reordered), {
-    results: [{ ellipsoid: 102.56 }, { ellipsoid: 100 }, { ellipsoid: 102.56 }],
+    results: [{ ellipsoid: 100.63 }, { ellipsoid: 100 }, { ellipsoid: 100.63 }],
   });
-  // Five chunks for the first batch; the reordered request is fully cached.
-  assert.equal(calls, 5);
+  // One bounded batch; the reordered request is fully cached.
+  assert.equal(calls, 1);
   assert.equal((await request('/?points=invalid')).status, 400);
   assert.equal(
     (await request('/?points=' + Array(2001).fill('0,1').join(';'))).status,
-    500,
+    413,
   );
-  // Rejected requests add no upstream calls: still the five from the first batch.
-  assert.equal(calls, 5);
+  // Rejected requests add no upstream calls.
+  assert.equal(calls, 1);
 });
 
 test('terrain middleware migrates valid legacy disk points without fabricating omitted heights', async (t) => {
@@ -388,21 +386,26 @@ test('terrain middleware retains successful chunks around a failure and retries
   });
   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);
+  const urls = [0, 64, 128].map(
+    (start) => '/?points=' + points.slice(start, start + 64).join(';'),
+  );
   assert.deepEqual(
-    requests.map((p) => p.length),
-    [64, 64, 2],
+    await Promise.all(urls.map(async (url) => (await request(url)).status)),
+    [200, 502, 200],
+  );
+  assert.deepEqual(
+    requests.map((p) => p.length).sort((a, b) => a - b),
+    [2, 64, 64],
   );
   fail = false;
-  const recovered = await request(url);
-  assert.equal(recovered.status, 200);
+  const recovered = await Promise.all(urls.map((url) => request(url)));
+  assert.ok(recovered.every((response) => response.status === 200));
   assert.deepEqual(
     requests[3],
     Array.from({ length: 64 }, (_, i) => [(i + 64) / 100, 1]),
   );
   assert.deepEqual(
-    json(recovered).results,
+    recovered.flatMap((response) => json(response).results),
     Array.from({ length: 130 }, (_, i) => ({ ellipsoid: i / 100 + 100 })),
   );
   assert.equal(requests.length, 4);