Don't crash ShapeLogCollector on introspection failures in the replication path

[erik-the-implementer]

Fixes the crash family from #4563: the replication hot path (Partitions, pk-column lookup, shape restore) only handled {:error, :connection_not_available} from the inspector, so every other legal inspector return crashed the ShapeLogCollector, took down the :one_for_all shapes supervision tree, and put the replication client into its 10-minute noproc retry loop.

Changes

• Partitions.handle_relation/2: handles :table_not_found (table dropped/renamed between the WAL record and introspection — nothing left to track, mirroring the existing add_shape behavior) and propagates other errors instead of crashing with a CaseClauseError.
• Partitions.add_shape/3: returns {:error, reason} instead of raising a RuntimeError on unexpected introspection errors; the shape-registration path propagates the error to the caller.
• pk_cols_of_relation: when the table was dropped before introspection, changes are keyed on the full record (the same fallback used for tables without a primary key) instead of crashing in Utils.map_while_ok; handling of the dropped table is left to the affected shapes further down the stack.
• ShapeLogCollector: unexpected introspection failures in the relation and transaction paths log a warning with the real reason and reply with {:error, :connection_not_available} — the one error the replication client knows how to recover from — pausing replication until introspection succeeds instead of crashing the collector.
• Shape restore: retries introspection in place (100ms × 100) when the connection pool isn't available instead of crashing with a MatchError in a restart loop. Skipping the shape was not an option: nothing re-registers restored shapes later, so a skipped shape would silently stop receiving updates. If the error persists past ~10s the restore still fails, but with a descriptive error.
• DirectInspector: connection-class errors returned in-band by Postgrex (e.g. "ssl recv: closed") are classified as :connection_not_available via the existing Electric.DbConnectionError classifier, so they take the established recovery path instead of leaking through as strings.

Production context

In Electric Cloud these crashes account for most of the "retry budget" Sentry noise: {:case_clause, :table_not_found}, {:case_clause, {:error, "ERROR 53200 (out_of_memory) ..."}}, query_canceled, "ssl recv: closed", the restore MatchError, and — downstream of all of them — thousands of (EXIT) no process dispatch errors per incident while the supervision tree restarts. See #4563 for the breakdown.

Notes for review

• All new behavior is covered by tests written before the fixes (9 new tests across partitions_test, shape_log_collector_test and a new direct_inspector_test).
• One known rough edge: a persistently failing introspection now produces one ShapeLogCollector warning per replication-client retry cycle (throttled only by the introspection query duration). Rate-limiting that warning could be a follow-up; the companion PR throttles the replication client's own retry logging.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.78%. Comparing base (992e2f3) to head (a4dcc7d).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4565      +/-   ##
==========================================
+ Coverage   57.16%   58.78%   +1.61%     
==========================================
  Files         340      383      +43     
  Lines       39535    42260    +2725     
  Branches    11491    12104     +613     
==========================================
+ Hits        22599    24841    +2242     
- Misses      16898    17343     +445     
- Partials       38       76      +38     

Flag	Coverage Δ
packages/agents	72.43% <ø> (ø)
packages/agents-mcp	77.70% <ø> (?)
packages/agents-mobile	78.81% <ø> (ø)
packages/agents-runtime	82.57% <ø> (ø)
packages/agents-server	75.15% <ø> (+0.23%)	⬆️
packages/agents-server-ui	7.36% <ø> (ø)
packages/electric-ax	46.42% <ø> (ø)
packages/experimental	87.73% <ø> (?)
packages/react-hooks	86.48% <ø> (?)
packages/start	82.83% <ø> (?)
packages/typescript-client	91.83% <ø> (?)
packages/y-electric	56.05% <ø> (?)
typescript	58.78% <ø> (+1.61%)	⬆️
unit-tests	58.78% <ø> (+1.61%)	⬆️

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

Incremental review (iteration 2). Since iteration 1 the branch was rebased onto current main and four follow-up commits landed, all addressing the polish items from the previous review: the in-place restore retry is now documented and bounded, the flaky-restore test now proves recovery rather than mere survival, the two hot-path error handlers were factored into a shared helper, and normalize_query_error/1 was simplified to return the bare value with callers wrapping. The core hardening is unchanged and remains solid. Ready to merge.

What's Working Well (this iteration)

• Restore retry is now bounded and thoroughly justified (shape_log_collector.ex:318-373). @restore_max_retries dropped 100 → 40, so the worst-case in-place sleep is ~4s, kept deliberately under the supervisor's 5s shutdown timeout. The new comment block is excellent: it explains why retry-in-place beats propagation (restore runs in handle_continue, not the event path, so the replication client's pause/redeliver recovery doesn't apply; ShapeLogCollector.Supervisor is max_restarts: 0 + :one_for_all, so a crash escalates to restarting the whole shape subsystem). I verified supervisor.ex:35 (strategy: :one_for_all, max_restarts: 0) — the comment's premises are accurate.
• Flaky-restore test now proves recovery, not just liveness (shape_log_collector_test.exs:120-192). It asserts the retry counter actually hit the connection error twice and routes a transaction through the restored shape, confirming the consumer registered and consumed. This is exactly the gap flagged last time.
• Error-handling dedup (shape_log_collector.ex:727-735). The duplicated map-everything-to-:connection_not_available logic in the relation and transaction handlers is now a single normalize_inspector_error/1 helper. Behavior is unchanged; both call sites are cleaner.
• normalize_query_error/1 hoist (direct_inspector.ex:108-117). The function now returns the bare String.t() | :connection_not_available and both call sites wrap it as {:error, ...}. I confirmed via grep that those are the only two callers, both wrap consistently, the @spec was updated to match, and the new direct_inspector_test.exs asserts against the bare-return contract. Clean refactor with no loose ends.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None. The iteration-1 "Important" item (permanent introspection errors → unbounded paused-replication loop on the hot path) is unchanged and remains a deliberate, documented trade-off — alive-and-paused beats crash-looping the :one_for_all tree. The PR notes already flag warning-throttling/alerting as follow-up. Not re-raising as a blocker.

Suggestions (Nice to Have)

• Redundant clause in normalize_inspector_error/1 (shape_log_collector.ex:727-735). Both clauses return the identical {:error, :connection_not_available}:

  defp normalize_inspector_error(:connection_not_available),
    do: {:error, :connection_not_available}
  
  defp normalize_inspector_error(_reason), do: {:error, :connection_not_available}

  The first clause is a no-op — the catch-all already covers :connection_not_available with the same result. It reads as documentation ("connection errors pass through unchanged"), but as written it's dead. If the intent is to keep the two cases visually distinct for a future where they diverge, fine; otherwise collapsing to the single catch-all (with the comment retained) would be marginally tidier. Purely cosmetic.
• Bound is "below 5s" only counting sleeps (shape_log_collector.ex retry doc). The ~4s figure is 40 × 100ms of Process.sleep; it excludes the 40 introspection-call durations themselves. During a pool-warmup race those checkouts fail fast, so the approximation holds — but a slow-failing introspection (e.g. a query that hangs before erroring) could push total blocking past 5s and into the shutdown timeout. Acceptable given the expected failure mode; just noting the comment's bound is sleep-only.

Issue Conformance

Unchanged from iteration 1: no formal GitHub issue is linked, but the PR body and changeset reference #4563 with a per-signature breakdown that the implementation maps onto cleanly. Adding a Fixes #4563 line would auto-close the issue.

Previous Review Status

All three iteration-1 suggestions addressed:

• ✅ Restore retry documented + bounded ("Document and bound the in-place restore retry").
• ✅ Restore test now proves partition recovery, not just survival ("Prove shape recovery in flaky-restore test").
• ✅ (Bonus cleanups) error-handling dedup + normalize_query_error hoist.

The iteration-1 "Important" finding was acknowledged as an intentional trade-off and is unchanged; no regressions observed.

---

Review iteration: 2 | 2026-06-15

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4dcc7d617

ℹ️ 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".

[robacourt]

Nice!

[github-actions]

This PR has been released! 🚀

The following packages include changes from this PR:

• @core/sync-service@1.7.3

Thanks for contributing to Electric!