Swap refreshed Data Library features in place, on every tick

The live refresh had three faults. Its data age counted from when a
fetch finished, so with the manager ticking every cacheSeconds the next
tick found the data just short of due and data refreshed every other
tick. It switched the layer off and on to redraw, which removed the old
features before the new ones were built, closed an open feature card and,
if the rebuild failed, left the layer on but empty. And the tick the
manager runs right after enable refetched after a failed first load, so
a second failure switched the layer off instead of showing UNAVAILABLE.

The local GeoJSON layer gains replaceFeatures(): it builds the new set
while the old one stays drawn, removes the old source only once the new
one is in the scene, keeps a selected feature that is still in the new
set (same id, or same place and label), and on a failed rebuild puts the
old features, records and selection back and rejects. The library's
refresh uses it, counts age from the request, allows a little slack on
the tick, and does nothing until a first load has succeeded.

src/data/localGeojsonCore.js

@@ -365,6 +365,12 @@ export function createLocalGeoJsonLayer(
    * @type {Array<object>|null}
    */
   let _cachedFeatures = null;
+  /**
+   * The source a `replaceFeatures()` rebuild is replacing: it stays drawn
+   * until the new one is in the scene, and comes back if the rebuild fails.
+   * @type {{source: object, stemRecords: object[], count: number, lastUpdate: number|null, error: string|null}|null}
+   */
+  let _outgoing = null;
   /**
    * Globe-LOD active set: the record ids allowed to carry a live stem right
    * now. This bounds geometry refreshes and ground-sample work to the
@@ -449,7 +455,19 @@ export function createLocalGeoJsonLayer(
    * survive a disable is the per-frame visualizer walk and the entity memory.
    * @param {object} viewer
    */
+  const releaseOutgoing = (viewer) => {
+    if (!_outgoing) return;
+    const { source } = _outgoing;
+    _outgoing = null;
+    try {
+      viewer?.dataSources?.remove(source, true);
+    } catch {
+      /* already gone */
+    }
+  };
+
   const releaseDataSource = (viewer) => {
+    releaseOutgoing(viewer);
     if (!_dataSource) return;
     const source = _dataSource;
     _dataSource = null;
@@ -489,7 +507,7 @@ export function createLocalGeoJsonLayer(
     }
   };
 
-  return {
+  const layer = {
     id,
     name,
     icon,
@@ -726,7 +744,7 @@ export function createLocalGeoJsonLayer(
                 // Store references for bounded stem scaling and native picking.
                 feature.__localBaseCarto = carto;
                 feature.__localBaseCartesian = base;
-                registerEntityContext(feature, {
+                const context = registerEntityContext(feature, {
                   id: `${id}:${recordId}`,
                   layerId: id,
                   layerName: name,
@@ -769,6 +787,7 @@ export function createLocalGeoJsonLayer(
                 _stemRecords.push({
                   id: recordId,
                   entity: feature,
+                  context,
                   carto,
                   base,
                   tip,
@@ -1111,6 +1130,99 @@ export function createLocalGeoJsonLayer(
       _cachedFeatures = null;
     },
 
+    /**
+     * Draw a new feature set in place of the current one while the layer is
+     * on (a live loader's refresh). The current features stay drawn until
+     * the new ones are in the scene; a selected feature stays selected when
+     * the new set still has it (same id, or same place and label); a failed
+     * rebuild keeps the current features and rejects. Off, the set is only
+     * kept for the next enable().
+     * @param {object} viewer
+     * @param {object[]} features GeoJSON features.
+     */
+    replaceFeatures: async (viewer, features) => {
+      if (_destroyed) return;
+      const previousFeatures = _cachedFeatures;
+      _cachedFeatures = features;
+      if (!_enabled || !_dataSource || _loadPromise) return;
+      const selected =
+        viewer?.selectedEntity?.__localLayerId === id
+          ? viewer.selectedEntity
+          : null;
+      const selectedLabel = selected
+        ? _stemRecords.find((record) => record.entity === selected)?.context
+            ?.label
+        : undefined;
+      _outgoing = {
+        source: _dataSource,
+        stemRecords: _stemRecords,
+        count: _count,
+        lastUpdate: _lastUpdate,
+        error: _error,
+      };
+      const outgoing = _outgoing;
+      _dataSource = null;
+      await coreEnable(viewer);
+      // Disabled or destroyed meanwhile: release took both sources.
+      if (_destroyed || !_enabled || _outgoing !== outgoing) return;
+      _outgoing = null;
+      if (!_dataSource) {
+        // The rebuild failed: the current features come back as they were.
+        const failure = _error;
+        _cachedFeatures = previousFeatures;
+        _dataSource = outgoing.source;
+        _stemRecords = outgoing.stemRecords;
+        _count = outgoing.count;
+        _lastUpdate = outgoing.lastUpdate;
+        _error = outgoing.error;
+        _stemGeometryDirty = true;
+        _lastVisibilityUpdate = Number.NEGATIVE_INFINITY;
+        for (const record of _stemRecords)
+          if (record.context)
+            registerEntityContext(record.entity, record.context);
+        if (selected && viewer.selectedEntity === selected)
+          selectEntityContext(selected);
+        throw new Error(failure || 'dataset unavailable');
+      }
+      try {
+        viewer?.dataSources?.remove(outgoing.source, true);
+      } catch {
+        /* already gone */
+      }
+      // Records the new set no longer has go with the old source.
+      removeEntityContextsForLayer(id, {
+        retainIds: new Set(
+          _stemRecords.map((record) => record.context?.id).filter(Boolean),
+        ),
+      });
+      if (selected) {
+        const same = (record) => {
+          const old = selected.__localBaseCarto;
+          const next = record.entity.__localBaseCarto;
+          return (
+            old &&
+            next &&
+            Math.abs(old.longitude - next.longitude) < 1e-9 &&
+            Math.abs(old.latitude - next.latitude) < 1e-9 &&
+            record.context?.label === selectedLabel
+          );
+        };
+        const match = _stemRecords.find((record) =>
+          selected.id !== undefined && record.entity.id === selected.id
+            ? true
+            : same(record),
+        );
+        if (match) {
+          viewer.selectedEntity = match.entity;
+          selectEntityContext(match.entity);
+        } else if (viewer.selectedEntity === selected) {
+          viewer.selectedEntity = undefined;
+          clearSelectedEntityContextForLayer(id);
+        }
+      }
+      viewer?.scene?.requestRender?.();
+    },
+
     destroy: (viewer) => {
       if (_destroyed) return;
       _destroyed = true;
@@ -1135,6 +1247,8 @@ export function createLocalGeoJsonLayer(
       _error = null;
     },
   };
+  const coreEnable = layer.enable;
+  return layer;
 }
 
 function compareLocalOverlayRecords(a, b) {

src/library/datasetLayer.test.mjs

@@ -15,21 +15,27 @@ const entry = {
   licence: { name: 'CC0', url: 'https://example.org/licence' },
 };
 
-const point = (name) => ({
-  type: 'Feature',
-  properties: { name },
-  geometry: {
-    type: 'Polygon',
-    coordinates: [
-      [
-        [0, 0],
-        [0.01, 0],
-        [0, 0.01],
-        [0, 0],
+// One small triangle per name, each in its own place.
+const point = (name) => {
+  const x = name.charCodeAt(0) / 100;
+  return {
+    type: 'Feature',
+    properties: { name },
+    geometry: {
+      type: 'Polygon',
+      coordinates: [
+        [
+          [x, 0],
+          [x + 0.01, 0],
+          [x, 0.01],
+          [x, 0],
+        ],
       ],
-    ],
-  },
-});
+    },
+  };
+};
+// A clock that is never 0, which reads as "never loaded".
+const START = 1_000_000;
 
 const deferred = () => {
   let resolve;
@@ -91,7 +97,7 @@ const result = (...names) => ({
 });
 
 test('a library dataset refreshes on the manager tick once older than cacheSeconds', async (t) => {
-  t.mock.timers.enable({ apis: ['Date'], now: 0 });
+  t.mock.timers.enable({ apis: ['Date'], now: START });
   const env = harness(t, [result('a'), result('b', 'c')]);
   assert.equal(env.layer.refreshInterval, 60_000);
   await env.layer.enable(env.viewer);
@@ -105,8 +111,84 @@ test('a library dataset refreshes on the manager tick once older than cacheSecon
   assert.equal(env.sources.size, 1, 'the old source is replaced, not kept');
 });
 
+test('a slow upstream still refreshes on every tick, not every other one', async (t) => {
+  t.mock.timers.enable({ apis: ['Date'], now: START });
+  // Each request takes two seconds to answer.
+  const slow = (value) => async () => {
+    t.mock.timers.tick(2000);
+    return value;
+  };
+  const env = harness(t, [
+    slow(result('a')),
+    slow(result('b')),
+    slow(result('c')),
+  ]);
+  await env.layer.enable(env.viewer);
+  // The manager ticks every refreshInterval from when the layer came on.
+  t.mock.timers.setTime(START + 60_000);
+  await env.layer.update(env.viewer);
+  t.mock.timers.setTime(START + 120_000);
+  await env.layer.update(env.viewer);
+  assert.equal(env.calls(), 3);
+});
+
+test('a failed first load is not retried by the tick that follows enable', async (t) => {
+  t.mock.timers.enable({ apis: ['Date'], now: START });
+  t.mock.method(console, 'error', () => {});
+  const env = harness(t, [
+    () => Promise.reject(new Error('HTTP 503')),
+    result('a'),
+  ]);
+  await env.layer.enable(env.viewer);
+  assert.ok(env.layer.getStats().error);
+  // Resolving (not rejecting) keeps the manager from failing the switch-on.
+  await env.layer.update(env.viewer);
+  assert.equal(env.calls(), 1);
+});
+
+test('a refresh keeps the old features drawn until the new ones are in, and the selection', async (t) => {
+  t.mock.timers.enable({ apis: ['Date'], now: START });
+  const env = harness(t, [result('a', 'b'), result('a', 'c')]);
+  await env.layer.enable(env.viewer);
+  const [first] = env.sources;
+  const selected = first.entities.values.find(
+    (entity) => entity.properties.name.getValue() === 'a',
+  );
+  env.viewer.selectedEntity = selected;
+  const sizes = [];
+  const add = env.viewer.dataSources.add;
+  env.viewer.dataSources.add = async (source) => {
+    sizes.push(env.sources.size);
+    return add(source);
+  };
+  t.mock.timers.tick(60_000);
+  await env.layer.update(env.viewer);
+  assert.deepEqual(sizes, [1], 'the old source was still drawn');
+  assert.equal(env.sources.size, 1);
+  const [next] = env.sources;
+  assert.notEqual(next, first);
+  assert.ok(next.entities.values.includes(env.viewer.selectedEntity));
+  assert.equal(env.viewer.selectedEntity.properties.name.getValue(), 'a');
+});
+
+test('a refresh whose rebuild fails keeps the drawn features', async (t) => {
+  t.mock.timers.enable({ apis: ['Date'], now: START });
+  t.mock.method(console, 'error', () => {});
+  const env = harness(t, [result('a'), result('b', 'c')]);
+  await env.layer.enable(env.viewer);
+  const [first] = env.sources;
+  env.viewer.dataSources.add = async () => {
+    throw new Error('scene full');
+  };
+  t.mock.timers.tick(60_000);
+  await assert.rejects(env.layer.update(env.viewer));
+  assert.deepEqual([...env.sources], [first]);
+  assert.equal(env.layer.getStats().count, 1);
+  assert.equal(env.layer.getStats().error, null);
+});
+
 test('a stale library dataset reloads on the next enable', async (t) => {
-  t.mock.timers.enable({ apis: ['Date'], now: 0 });
+  t.mock.timers.enable({ apis: ['Date'], now: START });
   const env = harness(t, [result('a'), result('b', 'c')]);
   await env.layer.enable(env.viewer);
   env.layer.disable(env.viewer);
@@ -120,7 +202,7 @@ test('a stale library dataset reloads on the next enable', async (t) => {
 });
 
 test('a failed refresh keeps the drawn features', async (t) => {
-  t.mock.timers.enable({ apis: ['Date'], now: 0 });
+  t.mock.timers.enable({ apis: ['Date'], now: START });
   const env = harness(t, [
     result('a'),
     () => Promise.reject(new Error('HTTP 503')),
@@ -133,7 +215,7 @@ test('a failed refresh keeps the drawn features', async (t) => {
 });
 
 test('disable during a refresh cancels it and draws nothing late', async (t) => {
-  t.mock.timers.enable({ apis: ['Date'], now: 0 });
+  t.mock.timers.enable({ apis: ['Date'], now: START });
   const pending = deferred();
   let refreshSignal = null;
   const env = harness(t, [

src/library/layers.js

@@ -161,13 +161,25 @@ export function createLibraryDatasetLayer(
   let rowListener = null;
   let legendCounts = null;
   // Live data: features older than the source's cacheSeconds are loaded
-  // again, on the next enable and on a timer while the layer is on.
+  // again, on the next enable and on the manager's refresh tick while the
+  // layer is on. Their age counts from when they were requested, and a tick
+  // allows a little slack, so a tick every cacheSeconds refreshes on every
+  // tick rather than every other one.
   const maxAgeMs = entry.source.cacheSeconds * 1000;
+  const tickSlackMs = Math.min(5000, maxAgeMs / 10);
+  /** When the drawn data was requested; 0 until a load succeeds. */
   let loadedAt = 0;
-  let prefetched = null;
   let refreshController = null;
   let on = false;
-  const stale = () => Date.now() - loadedAt >= maxAgeMs;
+  const age = () => Date.now() - loadedAt;
+  function take(result, requestedAt) {
+    loadedAt = requestedAt;
+    truncated = result.truncated;
+    loaded = result.features.length;
+    extent = featureExtent(result.features);
+    if (colorBy) legendCounts = countColorKeys(result.features);
+    rowListener?.();
+  }
   const colorBy = entry.display.colorBy || null;
   const layer = createLocalGeoJsonLayer(
     {
@@ -186,15 +198,9 @@ export function createLibraryDatasetLayer(
             colorBy.colors[String(properties?.__gevColorKey ?? '')] || null
         : null,
       loadFeatures: async ({ signal }) => {
-        const result =
-          prefetched ?? (await source.loadDataset(entry, { signal }));
-        prefetched = null;
-        loadedAt = Date.now();
-        truncated = result.truncated;
-        loaded = result.features.length;
-        extent = featureExtent(result.features);
-        if (colorBy) legendCounts = countColorKeys(result.features);
-        rowListener?.();
+        const requestedAt = Date.now();
+        const result = await source.loadDataset(entry, { signal });
+        take(result, requestedAt);
         return result.features;
       },
     },
@@ -207,7 +213,6 @@ export function createLibraryDatasetLayer(
   const cancelRefresh = () => {
     refreshController?.abort();
     refreshController = null;
-    prefetched = null;
   };
   return Object.assign(layer, {
     libraryEntryId: entry.id,
@@ -215,27 +220,30 @@ export function createLibraryDatasetLayer(
     async enable(viewer, ...rest) {
       on = true;
       registerDynamicCredit(viewer, libraryCredit(entry));
-      if (stale()) layer.dropCachedFeatures();
+      if (loadedAt && age() >= maxAgeMs) layer.dropCachedFeatures();
       return enable(viewer, ...rest);
     },
     /**
-     * The manager's refresh tick: once the data is stale, load it again and
-     * swap it in only when the new set is ready, so the old features stay
-     * drawn while the request runs and stay if it fails.
+     * The manager's refresh tick: once the data is due, load it again and
+     * swap it in where it is drawn, so the old features (and a selected
+     * one) stay while the request runs and stay if it fails. Nothing to
+     * refresh before a first load has succeeded: the tick that follows
+     * enable() must not retry a failed load and fail the switch-on.
      */
     async update(viewer) {
-      if (!on || !stale() || refreshController) return;
+      if (!on || !loadedAt || refreshController) return;
+      if (age() < maxAgeMs - tickSlackMs) return;
       const current = new AbortController();
       refreshController = current;
+      const requestedAt = Date.now();
       try {
         const result = await source.loadDataset(entry, {
           signal: current.signal,
         });
         if (!on || current.signal.aborted) return;
-        prefetched = result;
-        layer.dropCachedFeatures();
-        disable(viewer);
-        await enable(viewer);
+        await layer.replaceFeatures(viewer, result.features);
+        if (!on || current.signal.aborted) return;
+        take(result, requestedAt);
       } finally {
         if (refreshController === current) refreshController = null;
       }