Skip to content

[P1] 2.0.0-rc.6: Moving an array row and editing it in one setter overwrites another row #3282

Description

@brenelz

Describe the bug

A plain array store can lose a row when an array operation moves its elements and a subsequent statement edits a moved row in the same setter. For example, draft.reverse(); draft[1].count++ changes IDs [1, 2] into [1, 1] instead of [2, 1].

Priority assessment: P1. This is silent data corruption from ordinary store updates. Attaching a deep observer changes the result: the same operations pass when observed and fail when unobserved.

Platform and version

  • Source checkout: next at 472232bf; package version 2.0.0-rc.6.
  • macOS, Node.js v25.1.0, Vitest 4.1.6. No browser or DOM required.
  • Verified against the tracked @solidjs/signals source, rather than a downloaded npm release.

Reproduction

In a checkout of the commit above with dependencies installed, save the following as packages/signals/tests/store/array-move-probes.test.ts and run from the repository root:

pnpm --dir packages/signals exec vitest run tests/store/array-move-probes.test.ts
import {
  createRenderEffect,
  createRoot,
  createStore,
  deep,
  flush,
  snapshot,
  untrack,
} from "../../src/index.ts";

for (const operation of ["unshift", "reverse", "splice"] as const) {
  for (const observed of [false, true]) {
    it(`${operation} then a row edit preserves other rows (${observed ? "deep observed" : "unobserved"})`, () => {
      let dispose!: () => void;
      const initial = [
        { id: 1, count: 0 },
        { id: 2, count: 0 },
      ];
      const expected = structuredClone(initial);
      const [store, setStore] = createRoot((d) => {
        dispose = d;
        const pair = createStore(initial);
        if (observed)
          createRenderEffect(
            () => deep(pair[0]),
            () => {},
          );
        return pair;
      });
      const edit = (rows: typeof initial) => {
        if (operation === "unshift") rows.unshift({ id: 3, count: 0 });
        else if (operation === "reverse") rows.reverse();
        else rows.splice(0, 0, { id: 3, count: 0 });
        rows[1].count++;
      };
      try {
        flush();
        edit(expected);
        setStore(edit);
        flush();
        expect(untrack(() => snapshot(store))).toEqual(expected);
        expect(initial).toEqual([
          { id: 1, count: 0 },
          { id: 2, count: 0 },
        ]);
      } finally {
        dispose();
      }
    });
  }
}

Expected and actual behavior

For the reverse case:

Expected: [{ id: 2, count: 0 }, { id: 1, count: 1 }]
Actual:   [{ id: 1, count: 1 }, { id: 1, count: 1 }]

The unshift() and splice() insertion cases similarly overwrite the newly inserted row. The test matrix produces 3 failures and 3 passing deep-observed controls. The same results occur with __DEV__ and __TEST__ both enabled and both disabled when compiling local source.

Source analysis

The relevant path is privatizeCommitted(). A moved row target retains its original parent index. Committing its pending object overlay path-copies the row and unconditionally assigns the clone through that old index, overwriting whichever row now occupies the slot. The later compare-and-swap in drainFolds() does not prevent this earlier write.

Related context: #3271 addressed a derived-store ancestor write being discarded. This reproduction uses a plain store and corrupts a different array row; it still fails on the commit above, which includes that fix.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions