Recover metadata saves after a failed write instead of locking up forever

[amirrza777]

Relates to #7

Background

@nk-coding clarified on #27 that the original #7 report isn't about a reroute resetting after a specific edit, it's that in some cases something goes wrong internally and after that, no layout changes can be made any more for that session. #27 fixed a real but separate gap (the generated pipeline never got a chance at GED remapping at all), but does not explain a permanent lockup. This PR targets that.

The bug

ModelState.saveMetadata() writes metadata to disk fire-and-forget from the metadata setter, using a small state machine (SAVED / SAVING / SAVING_HAS_CHANGES) to coalesce concurrent writes. The write itself was never wrapped in error handling:

this.metadataSaveState = MetadataSaveState.SAVING;
await this.languageServices.shared.workspace.FileSystemProvider.writeMetadata(...);
if (this.metadataSaveState === MetadataSaveState.SAVING) {
    this.metadataSaveState = MetadataSaveState.SAVED;
}

If that write ever throws, for any reason (a network blip, a backend error, anything), the exception skips straight past the line that resets the state back to SAVED. Since the metadata setter only attempts a new write when the state is SAVED, every future layout edit for the rest of that session silently no-ops from then on, with nothing visible to the user (it's an unhandled rejection off a fire-and-forget call, it doesn't surface as an error anywhere).

A deeper root cause was found afterward: WebSocketApi.connectedPromise was built with only a resolve callback, never a reject, so a failed connection attempt left ensureConnected() awaiters hanging forever instead of erroring, which is how the write above could stay stuck mid-call rather than actually throwing. That's fixed too, see the PR comments below for the full trace.

The fix

Wrapped the write in try/catch. On failure, silently reset the state back to SAVED so the next edit can retry. This doesn't lose the failed write's data: _metadata in memory already holds every change cumulatively regardless of which individual disk writes succeeded, so the very next edit's write picks up everything, including whatever didn't make it to disk the first time.

The connection layer is what tells the user something went wrong: a toast notification fires the first time the connection drops unexpectedly, and another once it's restored, rather than a console log only visible in devtools.

Verification

There's no test runner set up in this repo to add a proper unit test against, so I wrote a standalone script mirroring the exact state machine, with a write that fails once then succeeds:

• Old code: after the first failed write, the second edit's write is never even attempted again. Permanently stuck.
• Fixed code: after the first failed write, the state resets, and the second edit's write succeeds normally.

Also had a live regression check done in the running app: rerouting an edge and then editing the file to shift statement IDs (twice, in two separate rounds) still correctly preserves the custom routing after this change, so the normal path is unaffected. Later verified further by actually killing the backend container mid-edit and confirming the connection-lost/restored notifications and metadata recovery all work as described, see PR comments for details.

[amirrza777]

Update: the original fix here only covered the case where the metadata write's promise actually rejects. Testing this live by killing the backend mid-edit showed that never happened: the console never printed the catch block's log line, meaning the write just hung instead of failing.

Root cause: webSocketApi.ts's connectedPromise was built with only a resolve callback captured, never a reject. When a connection attempt failed, handleClose correctly rejected every pending per-message request, but it never touched connectedPromise itself. So anything still awaiting ensureConnected() when the connection dropped (for example a write that started right as the WebSocket died) hung forever: never resolved, never rejected. Since saveMetadata() awaits that call directly, its await stayed stuck mid-function, metadataSaveState never got reset off SAVING, and every later edit silently no-opped through the SAVING_HAS_CHANGES branch. That is the actual "no layout changes can be made any more" behavior, not just an uncaught rejection.

I added a connectedReject alongside connectedResolve, and handleClose now rejects the connection promise the same way it already rejects pending requests. Also fixed handleError, which was throwing instead of doing anything useful (harmless since handleClose always follows, but still wrong).

Verified live: stopped the backend container, made an edit, confirmed the catch block's log now fires (it did not before), restarted the backend, made a new edit, reloaded, and the new edit persisted correctly.

[nk-coding]

I don't know what this should really improve
I agree that errors should not be silent
but a console error does not relly improve this (failed network requests can be seen in the network tab regardless)
what I think would really improve the current state would be a notification, like this is already done for the execution state updates
besides, the error fixed here should basically never happen, and is definitely not the primary cause of the issue, when network requests fail something worse is happening

[amirrza777]

Fair, a console.error alone doesn't help anyone who isn't already in devtools. Replaced it with actual toast notifications using the same pattern as the execution-state notifications in this file: a warning toast the first time the connection drops unexpectedly ("Connection lost: changes won't be saved until the connection is restored"), and a success toast once it reconnects ("Connection restored: your changes are saving again"). Deduped with a flag so it fires once per outage instead of once per retry attempt in the reconnect loop. Verified both toasts render correctly live, by stopping and restarting the backend container while editing.

On whether this is the primary cause of #7: probably not the only one, agreed, but it is a real one. I reproduced the exact reported symptom (no layout changes save for the rest of the session, no visible error) by killing the backend mid-edit, and traced it to connectedPromise never having a reject path, so ensureConnected() just hangs forever on a failed attempt instead of erroring. That part of the fix stays regardless of how often a real connection drop happens in practice, since once it does happen the session is permanently broken with zero recovery, silently, which seems worth closing off even if it is rare. The notification is a separate, additive improvement on top so that when it does happen, the user actually finds out instead of just losing work.

[nk-coding]

there are still several console.error, pls try to avoid these

[amirrza777]

Follow-up: also removed the other console.error, in modelState.ts's catch block. The WebSocketApi notification is what tells the user now, so that catch just recovers the save state machine silently. handleError is a true no-op too, and I suppressed a harmless "unhandled rejection" console warning a scheduled reconnect attempt could otherwise produce. Console is clean end to end during a real outage now, verified live.

[nk-coding]

lgtm

[szschaler]

Again, I agree that this fixes a bug, but it's not clear to me that it fixes the bug. It would be good to do a thorough analysis of what goes wrong in the management of edge waypoints in particular.

[Copilot]

Pull request overview

This PR addresses a “permanent lockup” failure mode where metadata persistence can get stuck after a failed write/connection attempt, preventing further layout changes from being saved during the session.

Changes:

• Make WebSocketApi.ensureConnected() fail fast by rejecting the connection promise on connection-close, and add one-per-outage user notifications for disconnect/restoration.
• Harden ModelState.saveMetadata() by catching failed writeMetadata() calls and resetting the save state so future edits can retry.
• Increase the workbench Docker build heap limit via NODE_OPTIONS to avoid OOM during npm run build.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
infra/docker/workbench/Dockerfile	Raises Node heap limit for the workbench build to reduce build-time OOM failures.
app/packages/workbench/src/data/api/areas/webSocketApi.ts	Ensures failed connection attempts reject (avoiding hung awaiters) and adds disconnect/reconnect user notifications.
app/packages/language-shared/src/diagram-server/modelState.ts	Prevents the metadata save state machine from getting stuck after a failed metadata write.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[amirrza777]

@szschaler On the analysis: this PR's fix and #27's fix address two different mechanisms, worth being precise about which is which.

#28 (this PR): the connection/save lockup. This one is deterministically reproducible, not the rare kind of bug: kill the backend mid-edit, and before this fix, saveMetadata()'s await hangs forever because connectedPromise had no reject path, so metadataSaveState gets stuck at SAVING permanently and every future edit silently no-ops for the rest of the session. After the fix, the same steps every time: the write properly rejects, the state recovers, and the next edit saves normally. I re-verified this again just now (stop backend, edit, confirm recovery) and it's consistent every time I've tried it, not something that only worked once or twice.

#27: the GED/ID-matching gap for the generated pipeline. Niklas already pointed out on that PR that my live test there doesn't prove anything, since the actual #7 bug is rare and history-dependent, and I'd overclaimed by treating "didn't reproduce it" as "fixed." I'm not repeating that claim here.

Where that leaves things: this PR closes off one concrete, reliably-reproducible way metadata persistence could get permanently stuck (connection drop with no reject path). I can't say it's the only way "waypoints go wrong" given #7 itself is described as rare and dependent on prior actions, there may be another mechanism I haven't found. If it recurs for anyone, the same two things I suggested on #27 would help narrow it down: what was done right before it happened, or I can add diagnostic logging to the metadata validation path so a future occurrence leaves something concrete to look at.