OpenFn / OpenFn/lightning

Collaborative editor: live save/interaction paths still crash on a slow or unreachable cross-node SharedDoc (follow-up to #4817)

Open
#4,877 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Elixir
Stars
296
Forks
86
Avg merge
1d 13h
Merged PRs (30d)
50

Description

Context

#4817 / #4845 stopped the cleanup path from crashing when the cross-node SharedDoc.unobserve/1 hits a SharedDoc on a node that is slow (:timeout) or gone (:noconnection/nodedown). Both unobserve sites — terminate/2 and the parent-:DOWN handler — now route through safe_unobserve/1, which catches the exit and logs a warning.

That fix covers only the cleanup call. The live interaction paths make the same kind of unguarded cross-node GenServer.call to the SharedDoc and remain exposed to the identical failure mode. This issue tracks that remaining gap (the F1/F2 follow-ups from the original investigation).

Where it still happens

Every Session → SharedDoc interaction is a synchronous GenServer.call to a pid that, in a multi-node cluster, frequently lives on another node (the SharedDoc is a cluster-wide singleton resolved via :pg; the Session is started locally on the node handling the channel). None of these are guarded:

  • handle_call({:save_workflow, …}) (session.ex:320), which the channel invokes via Session.save_workflow/2 with a 10s call timeout (session.ex:234, workflow_channel.ex:355), touches the SharedDoc twice:
    • get_document/1SharedDoc.get_doc/1 (session.ex:468, default 5s timeout)
    • merge_saved_workflow_into_ydoc/2SharedDoc.update_doc/2 (session.ex:527, 5s timeout)
  • reset_workflow and the other entry points forward to the SharedDoc the same way: :get_doc (session.ex:275), {:update_doc, …} (session.ex:294), {:send_yjs_message, …}, {:start_sync, …}.

Mechanism

When the SharedDoc's node is slow or has left the cluster, the nested GenServer.call exits inside handle_call, which crashes the Session. A Phoenix channel blocked in Session.save_workflow/2 then inherits the Session's exit reason (via the call's monitor) and itself crashes, dropping the client's editor connection. This is the same failure family as #4817, but triggered on the live save/interaction path rather than during cleanup.

Note that #4845 does not help here: when handle_call crashes, gen_server runs terminate/2 (now tolerant), but the process still exits with the original get_doc/update_doc crash reason — gen_server re-raises it — so the channel still inherits the crash and a crash report is still emitted.

Consistency hazard worth calling out

merge_saved_workflow_into_ydoc/2 runs after the DB commit in the save_workflow pipeline. If the cross-node update_doc there times out or hits a dead node, the workflow row has already been persisted, but:

  • the Y.Doc was not updated (lock_version not bumped in the doc, server errors not cleared), and
  • the client receives a crash / dropped connection instead of {:ok, …}.

So a save can succeed in the database while the user sees it as a failure.

Broader resilience gap (the existing TODO)

Per the # TODO: we need to have a strategy for handling the shared doc process crashing at session.ex:426:

  • The Session sets up exactly one monitor — Process.monitor(parent_pid) (session.ex:100). It does not monitor the SharedDoc. SharedDoc.observe/1 makes the SharedDoc monitor the Session, not the reverse.
  • Consequently a SharedDoc crash or node loss delivers no message to the Session. The {:DOWN, …} handler only acts on ref == parent_ref; everything else falls through. The Session retains a stale shared_doc_pid and crashes on the next call to it.
  • The TODO also flags that uncoordinated recreation by many sessions at once would create a thundering herd (each start_document reloading state from the DB — same collision neighbourhood as the races under #4816).

Open questions (unanswered in the code today)

  1. Detection — should the Session monitor the SharedDoc cross-node so it learns when the remote disappears, instead of finding out by crashing on the next call?
  2. Live-path tolerance — should save_workflow / reset_workflow and the yjs message paths return a structured error (and surface a degraded/reconnecting state to the client) rather than crashing the Session and the channel?
  3. Coordinated recreation — single-owner recreation of a SharedDoc after node loss, to avoid the thundering-herd reload.
  4. Save consistency — how to handle a SharedDoc failure that occurs after the DB commit (saved-but-merge-failed), so the client isn't told a successful save failed.

References

  • #4817 / #4845 — cleanup-path unobserve crash (the part already fixed).
  • #4816 — related save-flow races (workflows_pkey / constraint class).
  • Original investigation follow-ups F1 (channel-boundary hardening) and F2 (SharedDoc-crash strategy).

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the cited Session paths in session.ex, especially the live SharedDoc calls around lines 234, 275, 294, 320, 426, 468, and 527, then trace the channel boundary in workflow_channel.ex:355. Map the failure behavior for save, reset, and Yjs interactions; done requires an agreed strategy that prevents the documented crashes and addresses the post-commit consistency case.

Written by the indexing model from the issue text.

Assessment

Tech stack
elixir
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.