fix(sync-service): handle PK updates without Materializer key lookup crash

[alco]

Summary

Fixes a Materializer crash that occurred when applying an update which changes a row's primary key value.

Fixes #3948

Problem

When a row's primary key column was updated in Postgres (for example task_id in task_user_acl), Materializer could crash with a key lookup error:

** (KeyError) key "...new_key..." not found in: %{ "...old_key..." => ... }

Root Cause

apply_changes/2 in materializer.ex looked up the previous row by key (the new key after update), while the index still stored the previous row under old_key (the key before update).

Fix

This PR makes three changes in materializer.ex:

1. Extract old_key from UpdatedRecord in the pattern match.
2. Add old_key = old_key || key to handle updates where PK does not actually change.
3. Use old_key for index removal (Map.pop!(index, old_key)) and related tag-index updates.

Tests

Added coverage for PK-update behavior:

• update that changes the primary key is handled correctly
• update that changes the primary key but keeps the same value

These verify both correct handling of PK-change updates and no spurious events when only the key identity changes while tracked values remain equal.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.77%. Comparing base (9ea6a31) to head (349f4ac).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3949       +/-   ##
===========================================
+ Coverage   75.75%   88.77%   +13.02%     
===========================================
  Files          11       25       +14     
  Lines         693     2415     +1722     
  Branches      172      604      +432     
===========================================
+ Hits          525     2144     +1619     
- Misses        167      269      +102     
- Partials        1        2        +1     

Flag	Coverage Δ
packages/experimental	87.73% <ø> (ø)
packages/react-hooks	86.48% <ø> (ø)
packages/start	82.83% <ø> (ø)
packages/typescript-client	94.01% <ø> (?)
packages/y-electric	56.05% <ø> (ø)
typescript	88.77% <ø> (+13.02%)	⬆️
unit-tests	88.77% <ø> (+13.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6762daaba8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Drop old key from unchanged tags on PK updates

When an UpdatedRecord changes the primary key but keeps the same move tag(s), removed_move_tags is empty, so this branch never removes old_key from tag_indices and only adds key. That leaves stale keys in the tag index while index has already been rewritten to the new key (Map.pop!/Map.put below), so a later move-out event can attempt Map.pop!(index, old_key) and crash the materializer. This affects shapes with dependency tags where PK updates occur without tag changes.

Useful? React with 👍 / 👎.

[claude]

Claude Code Review

Summary

This PR fixes a crash in the Materializer when processing UpdatedRecord changes where the primary key column is updated: the index lookup was using the new key (key) but the index was still indexed by the old key (old_key). The fix is minimal, correct, and well-tested.

What's Working Well

• Surgical fix: Only 4 lines changed in the implementation. The old_key = old_key || key idiom cleanly handles the case where the PK didn't change and old_key is nil.
• Map.pop!/2 over Map.fetch!/2 + Map.delete/2: The change to {old_value, index} = Map.pop!(index, old_key) atomically removes and retrieves in one step — cleaner than the previous two-step approach.
• Test coverage: Both scenarios (PK change with value change, PK change with same value) are explicitly tested with clear assertions on both state and emitted events.
• Changeset included with an accurate patch-level description.
• PR description closely mirrors the issue, making the intent easy to follow.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None.

Suggestions (Nice to Have)

Potential index inconsistency for tag-only PK updates

File: packages/sync-service/lib/electric/shapes/consumer/materializer.ex:393-419

When old_key != key (PK changed) but columns_present = false and has_tag_updates = true (tag change only, no tracked-column change), the code correctly updates tag_indices using old_key/key, but the main index is left with the old key:

if columns_present or has_tag_updates do
  tag_indices =
    tag_indices
    |> remove_row_from_tag_indices(old_key, removed_move_tags)
    |> add_row_to_tag_indices(key, move_tags)   # ← tag_indices updated with new key

  if columns_present do
    {old_value, index} = Map.pop!(index, old_key)
    index = Map.put(index, key, value)           # ← index updated with new key
    ...
  else
    {{index, tag_indices}, counts_and_events}    # ← index still has old_key!
  end

If this path is taken and a subsequent move-out event matches the new tag (now pointing to key in tag_indices), the Map.pop!(index, key) in the move-out handler would crash with a KeyError, since the index still maps old_key → value.

In practice this is unlikely to be reachable — Postgres WAL UPDATE events always include the full new row, so the tracked column will be present in record whenever the PK changes, keeping columns_present = true. But it may be worth adding a guard or a test for completeness if this code path can be exercised.

Missing test for tag-change + PK-change combination

The two new tests cover columns_present = true (tracked value in record). A third test exercising has_tag_updates = true + PK change would close the remaining coverage gap and document the behavior for the else branch above.

Issue Conformance

The implementation precisely matches what issue #3948 requested. The issue even included the exact diff as a suggested fix, and the PR faithfully implements it — including the old_key = old_key || key nil-guard and the Map.pop! switch. No scope creep, no missing pieces.

CI Note

The one failing CI test (Electric.ShapeCacheTest::test get_or_create_shape_handle/2 against real db crashes when initial snapshot query fails to return data quickly enough) is in an unrelated module (ShapeCache) and shows a killed exit, consistent with a pre-existing flaky/timeout test. It does not appear to be caused by this PR.

---

Review iteration: 1 | 2026-03-03