Fix shape refresh

Author: robacourt

packages/sync-service/lib/electric/shape_cache/shape_status.ex

@@ -108,6 +108,17 @@ defmodule Electric.ShapeCache.ShapeStatus do
       generation = System.unique_integer([:positive, :monotonic])
       populate_shape_meta_table(stack_id, generation)
 
+      # Rows still on an old generation belong to handles that no longer exist
+      # (removed by another instance). Prune their reverse {id, handle} mappings
+      # before deleting the meta rows, otherwise the stale ids keep resolving via
+      # handle_for_id/2 and leak across refreshes.
+      stale_ids =
+        :ets.select(shape_meta_table(stack_id), [
+          {{:_, :_, :_, :_, :"$1", :"$2"}, [{:"/=", :"$1", generation}], [:"$2"]}
+        ])
+
+      Enum.each(stale_ids, &:ets.delete(shape_id_table(stack_id), &1))
+
       :ets.select_delete(shape_meta_table(stack_id), [
         {{:_, :_, :_, :_, :"$1", :_}, [{:"/=", :"$1", generation}], [true]}
       ])
@@ -549,8 +560,22 @@ defmodule Electric.ShapeCache.ShapeStatus do
         # any shapes where the snapshot didn't complete have been deleted
         # so there is no intermediate started-but-not-complete state
         # and completed implies started
-        id = mint_id(stack_id)
-        :ets.insert(shape_id_table(stack_id), {id, handle})
+
+        # Reuse the existing id for a handle already known to us (the refresh/1
+        # case) so ids stay stable across a refresh; only mint for handles we've
+        # not seen (first boot via initialize/1, or shapes a previous instance
+        # added while we were read-only). Re-minting here would orphan the old
+        # {id, handle} reverse mapping (see the sweep in refresh/1).
+        id =
+          case id_for_handle(stack_id, handle) do
+            {:ok, existing_id} ->
+              existing_id
+
+            :error ->
+              new_id = mint_id(stack_id)
+              :ets.insert(shape_id_table(stack_id), {new_id, handle})
+              new_id
+          end
 
         true =
           :ets.insert(table, {handle, hash, snapshot_complete?, start_time, generation, id})

packages/sync-service/test/electric/shape_cache/shape_status_test.exs

@@ -437,17 +437,40 @@ defmodule Electric.ShapeCache.ShapeStatusTest do
       assert ShapeStatus.shape_handle_for_log(stack_id, id) == "unknown, id: #{id}"
     end
 
-    test "ids minted on the restore path are resolvable both ways", %{stack_id: stack_id} do
-      {:ok, {handle, _id}} = ShapeStatus.add_shape(stack_id, shape!())
+    test "refresh preserves the id of an existing shape", %{stack_id: stack_id} do
+      {:ok, {handle, id}} = ShapeStatus.add_shape(stack_id, shape!())
       ShapeStatus.ShapeDb.mark_snapshot_complete(stack_id, handle)
 
       # refresh/1 repopulates the meta table from SQLite via
-      # populate_shape_meta_table/2, which mints a fresh id per restored handle.
+      # populate_shape_meta_table/2, reusing (not re-minting) the id for a handle
+      # it already knows, so the id stays stable across a refresh.
+      :ok = ShapeStatus.refresh(stack_id)
+
+      assert {:ok, ^id} = ShapeStatus.id_for_handle(stack_id, handle)
+      assert {:ok, ^handle} = ShapeStatus.handle_for_id(stack_id, id)
+    end
+
+    test "refresh prunes the reverse id mapping for handles removed elsewhere", %{
+      stack_id: stack_id
+    } do
+      {:ok, {removed_handle, removed_id}} = ShapeStatus.add_shape(stack_id, shape!())
+      {:ok, {kept_handle, kept_id}} = ShapeStatus.add_shape(stack_id, shape2!())
+      ShapeStatus.ShapeDb.mark_snapshot_complete(stack_id, removed_handle)
+      ShapeStatus.ShapeDb.mark_snapshot_complete(stack_id, kept_handle)
+
+      # Simulate another instance deleting the shape from the shared SQLite while
+      # our in-memory meta/reverse tables still reference it.
+      :ok = ShapeStatus.ShapeDb.remove_shape(stack_id, removed_handle)
+
       :ok = ShapeStatus.refresh(stack_id)
 
-      assert {:ok, restored_id} = ShapeStatus.id_for_handle(stack_id, handle)
-      assert is_integer(restored_id)
-      assert {:ok, ^handle} = ShapeStatus.handle_for_id(stack_id, restored_id)
+      # the removed handle's pre-refresh id no longer resolves in either direction
+      assert :error = ShapeStatus.id_for_handle(stack_id, removed_handle)
+      assert :error = ShapeStatus.handle_for_id(stack_id, removed_id)
+
+      # the surviving shape keeps its id
+      assert {:ok, ^kept_id} = ShapeStatus.id_for_handle(stack_id, kept_handle)
+      assert {:ok, ^kept_handle} = ShapeStatus.handle_for_id(stack_id, kept_id)
     end
 
     test "list_shapes_with_ids returns each shape's handle, id and shape", %{stack_id: stack_id} do