Fix stale index copies on scoped put after get

Jeremie flagged that putInIndexAfterGet_DoNotUse could leave a stale
copy of an item in any index that wasn't part of the caller's scoped
indexNames list, since each InMemoryIndex holds its own independent
copy of an item's data.

When an existing item is being overwritten, always remove the stale
copy from every index it's actually cached by (not just the requested
ones), then repopulate the union of "indexes that had it" and the
caller's requested indexes. New items (never cached before) are
unaffected and still only populate the requested/scoped index(es),
preserving the original anti-"memory island" guarantee.

- InMemoryIndex.remove() now returns whether it actually removed an
  entry, so callers can tell which indexes held the item.
- _removeFromIndices() returns the list of indexes an item was
  actually removed from.
- _putInternal (backing both put() and putInIndexAfterGet_DoNotUse())
  uses that list to compute which indexes to repopulate.

Added a regression test exercising Jeremie's exact scenario: an item
cached across 3 indexes via a normal put(), then a scoped put via 2 of
them with changed data -- asserts the 3rd (untouched by indexNames)
index reflects the new data instead of the old, stale copy.

Author: Eliran Eretz-Kedosha

src/InMemoryProvider.ts

@@ -21,6 +21,7 @@ import {
   takeRight,
   drop,
   take,
+  unionBy,
 } from "lodash";
 import {
   DbIndexFTSFromRangeQueries,
@@ -403,14 +404,17 @@ class InMemoryStore implements DbStore {
    * an optional 3rd parameter on it) so that the normal, always-safe `put()` required by the shared
    * `DbStore` interface can never accidentally be called with scoping semantics.
    *
-   * @param indexNames Scoping hint: items are only removed from / re-written into the primary key
-   * plus the listed index(es) -- every other index on the store is left completely untouched. This
-   * lets callers who fetched data through a single index (e.g. a ranged read served from that
-   * index) cache the results without seeding "islands" of items into unrelated indexes that were
-   * never actually queried/loaded for those items, and without evicting an already-cached item from
-   * indexes this call didn't touch. Note that an index left untouched here can only go stale in
-   * content if the item's data changes without ever going through the real (unscoped) `put()` path --
-   * real writes always resync every index.
+   * @param indexNames Scoping hint for *newly seen* items only: an item this store has never
+   * cached before is only written into the primary key plus the listed index(es) -- every other
+   * index on the store is left untouched, so callers who fetched data through a single index (e.g.
+   * a ranged read served from that index) can cache the results without seeding "islands" of items
+   * into unrelated indexes that were never actually queried/loaded for those items.
+   *
+   * If the item was already cached, the stale copy is removed from -- and the fresh copy is
+   * re-written into -- every index it was actually present in (not just the listed one(s)), so an
+   * index this call didn't ask for can never end up holding stale content. The listed index(es) are
+   * additionally guaranteed to receive the fresh copy even if the item wasn't previously tracked by
+   * them, so the ranged read that triggered this call still gets a correctly populated cache there.
    */
   putInIndexAfterGet_DoNotUse(
     itemOrItems: ItemType | ItemType[],
@@ -434,28 +438,43 @@ class InMemoryStore implements DbStore {
         )!!!;
         const existingItem = this._mergedData.get(pk);
 
-        // Scope both the removal (of the stale copy) and the re-add (of the new copy) to the
-        // same index list, so an index this call didn't ask for is never touched either way.
-        const indexesToTouch = indexNames
+        // Scoping hint from the caller: for a brand-new item (never cached before) this is where
+        // the fresh copy is written. Left undefined (i.e. every index) for the normal, unscoped
+        // put() path.
+        const requestedIndexes = indexNames
           ? filter(this._storeSchema.indexes, (index) =>
               includes(indexNames, index.name)
             )
           : this._storeSchema.indexes;
 
+        let indexesToPopulate = requestedIndexes;
+
         if (existingItem) {
-          // We're going to overwrite the PK anyways - don't remove PK
-          this._removeFromIndices(
+          // Always remove the stale copy from every index it's actually present in -- each index
+          // holds its own copy of the item, so scoping the removal to just the requested index(es)
+          // would leave an un-refreshed, stale copy behind in any other index the item was already
+          // tracked by (see PR #87 discussion).
+          const indexesThatHadItem = this._removeFromIndices(
             pk,
             existingItem,
             /** RemovePrimaryKey */ false,
-            indexesToTouch
+            this._storeSchema.indexes ?? []
           );
+
+          // Re-populate every index the item was actually already cached by (so it's refreshed,
+          // never left stale) plus whichever index(es) this call is scoped to (so a ranged read
+          // still gets a correctly populated cache there, even for an index the item wasn't
+          // previously tracked by).
+          indexesToPopulate = indexNames
+            ? unionBy(indexesThatHadItem, requestedIndexes ?? [], "name")
+            : this._storeSchema.indexes;
         }
+
         this._mergedData.set(pk, item);
         (this.openPrimaryKey() as InMemoryIndex).put(item);
 
-        if (indexesToTouch) {
-          for (const index of indexesToTouch) {
+        if (indexesToPopulate) {
+          for (const index of indexesToPopulate) {
             (this.openIndex(index.name) as InMemoryIndex).put(item);
           }
         }
@@ -605,29 +624,44 @@ class InMemoryStore implements DbStore {
     item: ItemType,
     removePrimaryKey: boolean,
     indexesToRemoveFrom: IndexSchema[] = this._storeSchema.indexes ?? []
-  ) {
+  ): IndexSchema[] {
     // Don't need to remove from primary key on Puts because set is enough
     // 1. If it's an existing key then it will get overwritten
     // 2. If it's a new key then we need to add it
     if (removePrimaryKey) {
       (this.openPrimaryKey() as InMemoryIndex).remove(key);
     }
 
+    const indexesThatHadItem: IndexSchema[] = [];
+
     each(indexesToRemoveFrom, (index: IndexSchema) => {
       const ind = this.openIndex(index.name) as InMemoryIndex;
       const indexKeys = ind.internal_getKeysFromItem(item);
 
       // when it's a unique index, value is the item.
       // in case of a non-unique index, value is an array of items,
       // and we want to only remove items that have the same primary key
+      let removedFromThisIndex = false;
       if (ind.isUniqueIndex()) {
-        each(indexKeys, (indexKey: string) => ind.remove(indexKey));
+        each(indexKeys, (indexKey: string) => {
+          if (ind.remove(indexKey)) {
+            removedFromThisIndex = true;
+          }
+        });
       } else {
-        each(indexKeys, (idxKey: string) =>
-          ind.remove({ idxKey, primaryKey: key })
-        );
+        each(indexKeys, (idxKey: string) => {
+          if (ind.remove({ idxKey, primaryKey: key })) {
+            removedFromThisIndex = true;
+          }
+        });
+      }
+
+      if (removedFromThisIndex) {
+        indexesThatHadItem.push(index);
       }
     });
+
+    return indexesThatHadItem;
   }
 }
 
@@ -739,22 +773,24 @@ class InMemoryIndex extends DbIndexFTSFromRangeQueries {
    * Removes item from index. For non-unique indices, a pair of index value and a primary key is required.
    * @param key a string, if it's a unique index, a pair of key value and a primary key, if it's a non-unique index
    * @param skipTransactionOnCreation
-   * @returns
+   * @returns Whether an entry was actually found and removed.
    */
   public remove(
     key: string | { primaryKey: string; idxKey: string },
     skipTransactionOnCreation?: boolean
-  ) {
+  ): boolean {
     if (!skipTransactionOnCreation && !this._trans!.internal_isOpen()) {
       throw new Error("InMemoryTransaction already closed");
     }
 
     if (typeof key === "string") {
+      const hadKey = this._indexTree.has(key);
       this._indexTree.delete(key);
+      return hadKey;
     } else {
       const idxItems = this._indexTree.get(key.idxKey);
       if (!idxItems) {
-        return;
+        return false;
       }
 
       const idxItemsWithoutItem = idxItems.filter((idxItem) => {
@@ -765,6 +801,11 @@ class InMemoryIndex extends DbIndexFTSFromRangeQueries {
         return idxItemPrimaryKeyVal !== key.primaryKey;
       });
 
+      if (idxItemsWithoutItem.length === idxItems.length) {
+        // Nothing matched key.primaryKey -- no-op.
+        return false;
+      }
+
       // if we removed all items, remove the index tree node.
       // otherwise, update the index value with the new array
       // sans the primary key item
@@ -773,6 +814,7 @@ class InMemoryIndex extends DbIndexFTSFromRangeQueries {
       } else {
         this._indexTree.set(key.idxKey, idxItemsWithoutItem);
       }
+      return true;
     }
   }
 

src/tests/ObjectStoreProvider.spec.ts

@@ -2279,6 +2279,120 @@ describe("ObjectStoreProvider", function () {
             );
         });
 
+        it("put with indexNames refreshes -- rather than skips -- any OTHER index the item was already tracked by, so it never goes stale", (done) => {
+          // indexNames scoping is an InMemoryProvider-only optimization -- other providers either maintain
+          // indexes natively (indexeddb) or ignore the extra parameter entirely.
+          if (provName.indexOf("memory") === -1) {
+            done();
+            return;
+          }
+
+          openProvider(
+            provName,
+            {
+              version: 1,
+              stores: [
+                {
+                  name: "test",
+                  primaryKeyPath: "id",
+                  indexes: [
+                    { name: "indexA", keyPath: "a" },
+                    { name: "indexB", keyPath: "b" },
+                    { name: "indexC", keyPath: "c" },
+                  ],
+                },
+              ],
+            },
+            true
+          )
+            .then((prov) => {
+              const maybeScopedPutProv = asScopedIndexPutProvider(prov);
+              assert(!!maybeScopedPutProv);
+              const scopedPutProv = maybeScopedPutProv!!!;
+
+              // A real (unscoped) write populates the item into all three indexes.
+              return prov
+                .put("test", { id: "item1", a: "a-1", b: "b-1", c: "c-1" })
+                .then(() =>
+                  Promise.all([
+                    prov.getAll("test", "indexA"),
+                    prov.getAll("test", "indexB"),
+                    prov.getAll("test", "indexC"),
+                  ])
+                )
+                .then(([byIndexA, byIndexB, byIndexC]) => {
+                  assert.equal(byIndexA.length, 1);
+                  assert.equal(byIndexB.length, 1);
+                  assert.equal(byIndexC.length, 1);
+                })
+                .then(() => {
+                  // A later scoped "write after get" only lists indexA and indexB -- indexC is not
+                  // in the scoped list, but the item's data has changed. indexC must still be
+                  // refreshed with the new data (not left holding its own stale, un-refreshed copy)
+                  // since it already had this item tracked before this call.
+                  return scopedPutProv.putInIndexAfterGet_DoNotUse(
+                    "test",
+                    {
+                      id: "item1",
+                      a: "a-1-updated",
+                      b: "b-1-updated",
+                      c: "c-1-updated",
+                    },
+                    ["indexA", "indexB"]
+                  );
+                })
+                .then(() =>
+                  Promise.all([
+                    prov.getAll("test", "indexA"),
+                    prov.getAll("test", "indexB"),
+                    prov.getAll("test", "indexC"),
+                  ])
+                )
+                .then(([byIndexA, byIndexB, byIndexC]) => {
+                  assert.equal(byIndexA.length, 1);
+                  assert.equal((byIndexA[0] as any).a, "a-1-updated");
+
+                  assert.equal(byIndexB.length, 1);
+                  assert.equal((byIndexB[0] as any).b, "b-1-updated");
+
+                  // The key regression check: indexC was not in the scoped indexNames list for this
+                  // call, but it already tracked item1, so it must be refreshed with the new data
+                  // rather than left holding the stale "c-1" copy.
+                  assert.equal(byIndexC.length, 1);
+                  assert.equal((byIndexC[0] as any).c, "c-1-updated");
+                })
+                .then(() => {
+                  // A brand-new item (never cached before) scoped to a subset of indexes must still
+                  // only populate the requested index(es) -- the anti-"memory island" guarantee this
+                  // scoping exists for is unaffected by the staleness fix above.
+                  return scopedPutProv
+                    .putInIndexAfterGet_DoNotUse(
+                      "test",
+                      { id: "item2", a: "a-2", b: "b-2", c: "c-2" },
+                      ["indexA"]
+                    )
+                    .then(() =>
+                      Promise.all([
+                        prov.getAll("test", "indexA"),
+                        prov.getAll("test", "indexB"),
+                        prov.getAll("test", "indexC"),
+                      ])
+                    )
+                    .then(([byIndexA, byIndexB, byIndexC]) => {
+                      assert.equal(byIndexA.length, 2);
+                      assert.equal(byIndexB.length, 1); // item2 not seeded into indexB
+                      assert.equal(byIndexC.length, 1); // item2 not seeded into indexC
+                    });
+                })
+                .then(() => prov.close())
+                .catch((e) => prov.close().then(() => Promise.reject(e)));
+            })
+            .then(
+              () => done(),
+              (err) => done(err)
+            );
+        });
+
         it("Invalid Key Type", (done) => {
           openProvider(
             provName,