diff --git a/changelog.d/1275-forecast-issue-fix.md b/changelog.d/1275-forecast-issue-fix.md new file mode 100644 index 000000000..2013bc4f2 --- /dev/null +++ b/changelog.d/1275-forecast-issue-fix.md @@ -0,0 +1,3 @@ +- Preserve enable during first-load replacement (asturksever, #1275). +- Release disabled weather camera listeners and make teardown resource leaks fail the browser probe (asturksever, #1275). +- Keep visible-page weather teardown probes focused through the browser testing API (asturksever, #1275). diff --git a/changelog.d/1276-terrain-issue-fix.md b/changelog.d/1276-terrain-issue-fix.md new file mode 100644 index 000000000..17e8f4b50 --- /dev/null +++ b/changelog.d/1276-terrain-issue-fix.md @@ -0,0 +1 @@ +- Bound each height request to one upstream chunk (asturksever, #1276). diff --git a/docs/CURRENT-STATE.md b/docs/CURRENT-STATE.md index 814c78e01..fafcb9a54 100644 --- a/docs/CURRENT-STATE.md +++ b/docs/CURRENT-STATE.md @@ -9,10 +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. -Share links and structured views accept only own keys in their style registries. -Inherited names such as `constructor` and `__proto__` fall back to the existing -normal/null defaults without interrupting other restored fields. Programmatic -visual-style calls ignore unknown or non-string values before mutating 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 @@ -275,6 +275,7 @@ catalog construction owns each instance. Wind animates one forecast without advancing forecast time. Separate Weather observation layers provide radar and satellite history; none claims measured cloud volume. +Weather imagery releases its camera move listener on disable and registers it once on re-enable. Disabling also clears active imagery work. Weather imagery drapes onto the globe basemap. On photorealistic 3D Tiles each observed product is instead a raised, translucent shell: one rectangle over the product bounds at a fixed height (global infrared 5.5 km, regional infrared diff --git a/scripts/qa-weather-teardown.mjs b/scripts/qa-weather-teardown.mjs index b64d1d59c..bf7b00269 100644 --- a/scripts/qa-weather-teardown.mjs +++ b/scripts/qa-weather-teardown.mjs @@ -61,6 +61,10 @@ const browser = await puppeteer.launch({ const boot = async () => { const page = await browser.newPage(); + // This probe measures active rendering; an OS-minimized window must not + // silently exercise the application's intentional hidden-page pause. + const focus = await page.createCDPSession(); + await focus.send('Emulation.setFocusEmulationEnabled', { enabled: true }); await page.setViewport({ width: 1400, height: 920, deviceScaleFactor: 1 }); const errors = []; page.on('pageerror', (e) => @@ -141,7 +145,7 @@ const snapshot = (page) => ), }, weather, - windCells: wind.gpu?.pathCount ?? wind.paths ?? null, + windCells: wind.gpu?.pathCount ?? wind.paths ?? 0, enabled: [...dm.layers].filter(([, e]) => e.enabled).map(([id]) => id), }; }); @@ -220,7 +224,7 @@ try { const baseline = await snapshot(a.page); console.log('baseline', JSON.stringify(baseline)); for (const id of WEATHER) await setLayer(a.page, id, true); - await a.page + const weatherReady = await a.page .waitForFunction( () => ['weather-radar', 'weather-satellite', 'weather-lightning'].every( @@ -233,7 +237,13 @@ try { ), { timeout: 120_000 }, ) - .catch(() => console.log(' !! weather not ready')); + .then( + () => true, + () => false, + ); + results.weatherReady = weatherReady; + if (!weatherReady) + console.log(' !! weather not ready — feature coverage incomplete'); await sleep(2000); // exercise: history playback, wind reading, storm selection, product switch await a.page.evaluate(() => { @@ -331,11 +341,25 @@ try { 'errors (control run):', results.errorsB.length ? results.errorsB : 'none', ); + // Resource ownership is an acceptance check; provider availability remains + // separately reported diagnostic evidence in both runs. + if ( + !weatherReady || + Object.keys(leak).length || + [...a.errors, ...b.errors].some((e) => e.startsWith('pageerror:')) + ) { + process.exitCode = 1; + } +} catch (error) { + results.failure = { name: error.name, message: error.message }; + throw error; +} finally { + // Preserve completed resource evidence even if a later provider or browser + // phase aborts. A partial run still exits nonzero. writeFileSync( path.join(outDir, 'results.json'), JSON.stringify(results, null, 2), ); console.log(`wrote ${path.join(outDir, 'results.json')}`); -} finally { await browser.close(); } diff --git a/server/providers/terrain.js b/server/providers/terrain.js index 32083d23a..c678ab7c0 100644 --- a/server/providers/terrain.js +++ b/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, diff --git a/src/layers/weather/index.js b/src/layers/weather/index.js index db62ea98a..757764fe6 100644 --- a/src/layers/weather/index.js +++ b/src/layers/weather/index.js @@ -296,10 +296,6 @@ export function createWeatherLayer({ motion?.addEventListener?.('change', onVisibility); documentRef?.addEventListener?.('visibilitychange', onVisibility); registerClock(); - removeCamera = viewer.camera.moveEnd?.addEventListener(() => { - checkHost(); - if (enabled) notify(); - }); }, attachShellServices(services) { imageryHost = @@ -315,10 +311,18 @@ export function createWeatherLayer({ }, enable() { enabled = true; + if (!removeCamera) { + removeCamera = viewer.camera.moveEnd?.addEventListener(() => { + checkHost(); + notify(); + }); + } registerClock(); }, disable() { enabled = false; + removeCamera?.(); + removeCamera = null; unregisterClock?.(); unregisterClock = null; ++generation; @@ -357,7 +361,7 @@ export function createWeatherLayer({ signal: controller.signal, }); if (!enabled || controller.signal.aborted || request !== controller) - return false; + return enabled && !signal?.aborted && request !== controller; if (snapshot.unavailable) { error = msg( 'Weather source unavailable; previous observation retained', @@ -376,7 +380,9 @@ export function createWeatherLayer({ ) await show(snapshot.latest, controller.signal); return ( - !controller.signal.aborted && request === controller && enabled + enabled && + !signal?.aborted && + (!controller.signal.aborted || request !== controller) ); } const time = @@ -388,9 +394,14 @@ export function createWeatherLayer({ (satellite && rendering.getDiagnostics().infrared !== infrared) ) await show(time, controller.signal); - return !controller.signal.aborted && request === controller && enabled; + return ( + enabled && + !signal?.aborted && + (!controller.signal.aborted || request !== controller) + ); } catch (cause) { - if (controller.signal.aborted || request !== controller) return false; + if (controller.signal.aborted || request !== controller) + return enabled && !signal?.aborted && request !== controller; error = cause?.message || msg('Weather unavailable'); stop(); return true; diff --git a/src/layers/weather/weather.test.mjs b/src/layers/weather/weather.test.mjs index 1a4ba5177..486d42520 100644 --- a/src/layers/weather/weather.test.mjs +++ b/src/layers/weather/weather.test.mjs @@ -694,7 +694,11 @@ test('superseding a pending stage preserves the newer manifest loading state', a await flush(); const newer = h.layer.update(); await flush(); - assert.equal(await old, false); + assert.equal( + await old, + true, + 'replacement is accepted by the enable lifecycle', + ); assert.equal( h.layer.getStats().loading, true, @@ -2311,3 +2315,44 @@ for (const product of ['radar', 'clouds-regional', 'lightning']) { assert.equal(h.viewer.camera.moveEnd.size, 0); }); } + +test('a product replacement accepts supersession but never paints the obsolete first response', async () => { + const pending = []; + const h = layerHarness({ + id: 'weather-satellite', + feed: { + getSnapshot: (args) => { + const task = deferred(); + pending.push({ ...args, ...task }); + return task.promise; + }, + }, + }); + const first = h.layer.update(); + h.layer.setParams({ product: 'clouds' }); + assert.equal(pending.length, 2); + pending[0].resolve({ ...snapshot, product: 'clouds-regional' }); + assert.equal(await first, true); + assert.equal(h.stages.length, 0); + pending[1].resolve({ ...snapshot, product: 'clouds' }); + await flush(); + h.stages.at(-1).finish(); + await flush(); + assert.equal(h.layer.getParams().product, 'clouds'); + h.layer.destroy(); +}); + +test('disabled weather releases its camera listener and re-enabling binds only once', () => { + const h = layerHarness(); + h.layer.disable(); + assert.equal(h.moveEnd.size, 0, 'disabled layers own no active camera work'); + h.layer.enable(); + h.layer.enable(); + assert.equal(h.moveEnd.size, 1); + h.layer.disable(); + assert.equal(h.moveEnd.size, 0); + h.layer.enable(); + assert.equal(h.moveEnd.size, 1); + h.layer.destroy(); + assert.equal(h.moveEnd.size, 0); +}); diff --git a/src/layers/wind/index.js b/src/layers/wind/index.js index c76e2501c..aa6161f06 100644 --- a/src/layers/wind/index.js +++ b/src/layers/wind/index.js @@ -180,7 +180,7 @@ export function createWindLayer({ overlay: requestedScalar(), }); if (!enabled || controller.signal.aborted || request !== controller) - return false; + return enabled && !signal?.aborted && request !== controller; manifest = snapshot; error = null; if (sampledPosition) sample(sampledPosition); @@ -194,7 +194,8 @@ export function createWindLayer({ } return true; } catch (cause) { - if (controller.signal.aborted || request !== controller) return false; + if (controller.signal.aborted || request !== controller) + return enabled && !signal?.aborted && request !== controller; error = cause?.message || msg('Wind source unavailable'); return true; } finally { diff --git a/src/layers/wind/index.test.mjs b/src/layers/wind/index.test.mjs index ac843f62a..3b5e09697 100644 --- a/src/layers/wind/index.test.mjs +++ b/src/layers/wind/index.test.mjs @@ -412,3 +412,36 @@ test('wind unit chips appear only alongside a speed legend, including canvas tra for (const overlay of ['pressure', 'temperature', 'none']) { layer.setParams({ overlay }); assert.deepEqual(unitChips(), []); } layer.destroy(); }); + +test('a parameter replacement during manager enable does not roll the layer back', async () => { + const { LayerLifecycle } = await import('../../data/lifecycle.js'); + const pending = []; + const rendering = Object.fromEntries( + ['attach', 'start', 'stop', 'clear', 'destroy', 'setField'].map((name) => [ + name, + () => {}, + ]), + ); + const layer = createWindLayer({ + feed: { + getSnapshot: (args) => + new Promise((resolve) => pending.push({ ...args, resolve })), + }, + createRendering: () => rendering, + }); + const manager = new LayerLifecycle({ container: {} }); + manager.register(layer); + const enabled = manager.setEnabled('wind', true); + for (let i = 0; i < 20 && !pending.length; i++) await Promise.resolve(); + layer.setParams({ model: 'ifs' }); + await Promise.resolve(); + pending[0].resolve(snapshot('gfs')); + assert.equal(await enabled, true); + assert.equal(manager.isEnabled('wind'), true); + assert.equal(layer.getStats().loading, true); + pending[1].resolve(snapshot('ifs')); + for (let i = 0; i < 10; i++) await Promise.resolve(); + assert.equal(layer.getStats().model, 'IFS'); + assert.equal(layer.getStats().count, 4); + await manager.destroyAll(); +}); diff --git a/src/terrainHeightsBounds.test.mjs b/src/terrainHeightsBounds.test.mjs index cd1a9ad36..3746f4fe4 100644 --- a/src/terrainHeightsBounds.test.mjs +++ b/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); +}); diff --git a/src/tooling/environmentProviders.test.mjs b/src/tooling/environmentProviders.test.mjs index b6faeb34c..bbb0e9d6e 100644 --- a/src/tooling/environmentProviders.test.mjs +++ b/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);