Prevent shape removal during server restart

[robacourt]

Summary

Fixes bug 6 from the restart-aware oracle property-test triage (#4648 / bugs.md): a healthy subquery shape returning a 409 (must-refetch) after a StackSupervisor restart.

A single targeted fix, with a deterministic regression test that fails without the fix and passes with it. This is the part of the fix that was battle-tested in rob/restore-subqueries-bug-5.

Problem

Bug 6 is a shutdown race. During a restart's stack teardown, a dependency consumer's inline GenServer.call into its materializer (notify_materializer_of_new_changes/3) races the materializer's death and exits with :noproc. The consumer crashes with a non-shutdown reason, which routes through handle_writer_termination and removes the shape from disk. Because the ShapeLogCollector is already gone mid-shutdown, the removal half-completes — the handle is deleted from SQLite but cleanup fails — so after the restart the handle is missing and a fresh poll on that optimized: true shape gets a 409.

Solution

• consumer.ex — notify_materializer_of_new_changes/3 catches the :exit (:noproc, and transient :normal/:shutdown) from its inline materializer call. The pending monitored :DOWN then drives a clean stop instead of the abnormal-shutdown path that removes the shape.

Test Plan

• consumer_test.exs — "dependency consumer survives a :noproc from its materializer without removing the shape". Without the fix: the consumer crashes with {:noproc, …} and logs "Removing shape …".

---

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.05%. Comparing base (4051e0f) to head (6aef3c7).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4666   +/-   ##
=======================================
  Coverage   60.04%   60.05%           
=======================================
  Files         397      397           
  Lines       43766    43766           
  Branches    12586    12590    +4     
=======================================
+ Hits        26281    26282    +1     
  Misses      17403    17403           
+ Partials       82       81    -1     

Flag	Coverage Δ
packages/agents	72.64% <ø> (ø)
packages/agents-mcp	77.70% <ø> (ø)
packages/agents-mobile	80.67% <ø> (ø)
packages/agents-runtime	83.73% <ø> (+0.01%)	⬆️
packages/agents-server	75.65% <ø> (-0.03%)	⬇️
packages/agents-server-ui	8.32% <ø> (ø)
packages/electric-ax	51.06% <ø> (ø)
packages/experimental	87.73% <ø> (ø)
packages/react-hooks	86.48% <ø> (ø)
packages/start	82.83% <ø> (ø)
packages/typescript-client	91.89% <ø> (+0.11%)	⬆️
packages/y-electric	56.05% <ø> (ø)
typescript	60.05% <ø> (+<0.01%)	⬆️
unit-tests	60.05% <ø> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

[claude]

Claude Code Review

Summary

Since my last review the PR has been narrowed to a single, root-cause fix in consumer.ex: the earlier defensive changes in shape_status.ex and api.ex are gone. What remains is the notify_materializer_of_new_changes/3 catch that stops a dependency consumer from crashing (and thereby removing its shape from disk) when its inline materializer call races the materializer's shutdown. This is a cleaner, more focused change than the original three-part version, with a deterministic regression test and a matching changeset. Recommend merge after considering the one carried-over suggestion below.

What's Working Well

• Correct deferral to the monitored :DOWN. I re-traced the flow against the current code: the consumer monitors the materializer at consumer.ex:217 (Process.monitor(pid, tag: :materializer_down)), so swallowing the inline GenServer.call exit lets the pending :DOWN reach handle_materializer_down/2 (consumer.ex:1266). That function does the right thing per state: {:noreply, state} when already terminating?, and {:stop, reason, state} for :shutdown/{:shutdown, _} — neither removes the shape. Crashing on the inline call instead would route through the abnormal-shutdown path and delete the shape. The mechanism matches the PR description.
• The swallowed reasons don't mask real errors. :normal/:shutdown are not error conditions, and for a genuinely abnormal materializer death the :DOWN still arrives with the real reason and falls to the stop_and_clean/1 clause — so the decision to remove is still made by handle_materializer_down/2, not silently suppressed. The catch only prevents the consumer from dying on the inline call; it doesn't change removal semantics outside the shutdown race.
• Tight, deterministic test. The new test patches Materializer.new_changes to exit({:noproc, {GenServer, :call, ...}}) — the exact shape of a call to a dead process — drives a real dependency-table change through ShapeLogCollector, and asserts both that the consumer stays alive and that remove_shape is never called (it raises if it is). All helpers (patch_shape_status, complete_txn_fragment, activate_mocks_for_descendant_procs, @shape_with_subquery) exist. Codecov confirms the modified lines are covered.
• Changeset scope now matches the code. The current wording ("Stop subquery shapes from being spuriously removed during a server restart") correctly drops the "(and held requests from crashing)" phrasing from the earlier review-comment suggestion, since the api.ex request-crash fix is no longer part of this PR.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None.

Suggestions (Nice to Have)

1. (Carried over) The catch enumerates exit reasons but still omits :killed.

File: packages/sync-service/lib/electric/shapes/consumer.ex:1074-1077

The four clauses cover :noproc, {:noproc, _}, {:normal, _}, {:shutdown, _}, but not {:killed, _} (nor :timeout). If the materializer overruns its shutdown timeout and is brutally killed, the inline GenServer.call exits with {:killed, {GenServer, :call, ...}}, which is unmatched and would crash the consumer via the very path this PR closes.

Worth noting this window is genuinely narrow: a clean stack teardown exits the materializer with :shutdown (covered), and :killed only occurs on a shutdown-timeout brutal-kill. There's also a subtlety — even the :DOWN handler routes a :killed reason to stop_and_clean/1 (removal) unless terminating? is set — so this is arguably a broader shutdown-ordering question than this one catch. Given the design intent ("never crash the consumer on the inline notify call; let the monitored :DOWN decide"), a logged catch-all is arguably more robust than an enumerated list:

catch
  :exit, reason ->
    Logger.debug(fn ->
      "Materializer notify call exited (#{inspect(reason)}); " <>
        "deferring to monitored :DOWN for #{state.shape_handle}"
    end)
    :ok
end

Non-blocking — :noproc is the dominant race and is covered.

Issue Conformance

No GitHub issue is linked (only the internal #4648 / bugs.md triage reference), which is a minor convention warning as before. The rewritten PR description is accurate and thorough: it now describes exactly one fix and its deterministic test, matching the diff with no scope creep. Note that this PR now addresses only the root-cause of bug 6 (spurious shape removal); the earlier defensive guards against 409s/held-request crashes have been dropped — if those were tracking separate observed failure modes, confirm they're covered elsewhere or intentionally out of scope.

Previous Review Status

• Resolved: Previous suggestion [Merged on #3] Write inserts, updates and deletes to Vaxine #2 (broad rescue ArgumentError in shape_status.ex/api.ex) — no longer applicable; those files were removed from the PR when it was narrowed to the single consumer.ex fix.
• Still open: Previous suggestion Working with Postgres WAL format from Elixir #1 (missing :killed/timeout in the enumerated catch) — the consumer.ex code is unchanged on this point. Re-stated above; still non-blocking.
• The unrelated TypeScript CI failure noted last time is no longer relevant to this diff; codecov now reports all tests successful.

---

Review iteration: 2 | 2026-07-16

[alco]

Nice one!

[github-actions]

This PR has been released! 🚀

The following packages include changes from this PR:

• @core/sync-service@1.7.8

Thanks for contributing to Electric!