fix: stop y-websocket connection leaks in shared editing and reap idle /rtc connections at the gateway#6240
fix: stop y-websocket connection leaks in shared editing and reap idle /rtc connections at the gateway#6240aicam wants to merge 5 commits into
Conversation
Two distinct leaks on the SharedModel -> WebsocketProvider path exhausted the shared-editing (/rtc) connection cap and broke collaborative cursors: - Per-edit (dominant): the throwaway WorkflowGraph built for validation on every edit unconditionally created a SharedModel, opening a new /rtc socket that was never destroyed and reconnected forever. Thread an enableSharedEditing flag through WorkflowGraph -> SharedModel and pass false from ValidationWorkflowService.getValidTexeraGraph() so validation/compilation graphs run local-only (standalone Awareness, no WebsocketProvider). - Per-navigation: SharedModel.destroy() called wsProvider.disconnect() (only when connected), which leaves y-websocket's reconnect timer running so the provider is never GC'd. Call wsProvider.destroy() unconditionally instead. wsProvider is now optional; add ?. at its remaining access sites and pass enableSharedEditing=false in mock fixtures so unit tests open no sockets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
Give the shared-editing (y-websocket) /rtc path its own HTTPRoute and attach a BackendTrafficPolicy that drops idle upstream connections and raises the per-cluster max_connections circuit breaker above Envoy's 1024 default. The y-websocket reference server is single-process and every browser tab holds a long-lived WebSocket, so abandoned/leaked realtime sockets can pile up against the gateway's per-cluster connection cap and starve legitimate co-editing traffic. This is defense-in-depth for the client-side connection leak fixed in the frontend. Both the idle timeout (default 5m) and the connection ceiling (default 8192) are configurable via yWebsocketServer.trafficPolicy; set it to null to omit the policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Pull request overview
This PR addresses runaway /rtc (y-websocket) WebSocket growth caused by frontend shared-editing objects being created without proper teardown (and, in one path, created unintentionally for throwaway validation graphs). It also adds an Envoy Gateway policy as defense-in-depth to reap idle /rtc upstream connections and raise the connection ceiling for that cluster.
Changes:
- Added an
enableSharedEditingswitch so throwaway/validationWorkflowGraphinstances do not create aWebsocketProvider(avoids per-edit leaked/rtcsockets). - Fixed shared-editing teardown by destroying the provider (
wsProvider.destroy()) rather than only disconnecting, eliminating reconnect-timer “zombie” providers. - Split
/rtcinto its own GatewayHTTPRouteand added an optionalBackendTrafficPolicy(idle timeout + increasedmaxConnections) configurable via Helm values.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts | Makes wsProvider optional, supports local-only mode, and ensures provider is fully destroyed on teardown. |
| frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.ts | Threads enableSharedEditing into SharedModel construction to avoid unintended network connections. |
| frontend/src/app/workspace/service/validation/validation-workflow.service.ts | Builds validation graphs with enableSharedEditing=false to prevent per-edit WebSocket leaks. |
| frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts | Updates provider access for optionality (but one call site still needs full null-safe access). |
| frontend/src/app/workspace/service/execute-workflow/mock-workflow-plan.ts | Updates fixtures to create local-only graphs (no sockets during tests/fixtures). |
| bin/k8s/templates/base/gateway/gateway-routes.yaml | Moves /rtc to its own HTTPRoute so policies can target it without affecting other routes. |
| bin/k8s/templates/base/shared-editing-server/shared-editing-server-backend-traffic-policy.yaml | Adds optional Envoy Gateway BackendTrafficPolicy for /rtc (idle reaping + circuit breaker headroom). |
| bin/k8s/values.yaml | Adds yWebsocketServer.trafficPolicy knobs (connectionIdleTimeout, maxConnections). |
| bin/k8s/values-development.yaml | Mirrors the new yWebsocketServer.trafficPolicy knobs for dev values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aglinxinyuan
left a comment
There was a problem hiding this comment.
You manually verified the behavior after the fix. Could you also verify it before the fix? Specifically, we'd like to confirm that there were consistently multiple WebSocket connections before this change.
Also, there's no need to list the files changed in the PR description.
Envoy Gateway's BackendTrafficPolicy exposes the connection idle timeout under
spec.timeout.http.connectionIdleTimeout; spec.timeout.tcp only has connectTimeout.
The previous timeout.tcp.connectionIdleTimeout was rejected by the CRD schema
("field not declared in schema"), which blocked the whole release from applying.
Verified against a live Envoy Gateway install on minikube.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
do we need to backport the fix to v1.2? |
Yicong-Huang
left a comment
There was a problem hiding this comment.
why do we need a enableSharedEditing config? Please find a better way to pass the config. Bundling it with WorkflowGraph is not ideal.
|
Could you open a separate PR targeting the release/v1.2 branch for this fix? Thanks. |
…onstructor flag Address review feedback on apache#6240: remove the enableSharedEditing flag from WorkflowGraph/SharedModel. A /rtc WebsocketProvider is now created only when a workflow ID is present (the existing loadNewYModel contract); graphs without a wid — throwaway validation/compilation graphs, new unsaved canvases — stay local-only with a standalone Awareness and never open a socket. Also use optional chaining for the wsProvider.disconnect() call in setTempWorkflow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QfJJ5EnsWiJnAKDNJyQYDv
…older BackendTrafficPolicy is an Envoy Gateway resource, so keep all gateway-layer templates together under templates/base/gateway/. Pure file moves plus a README update; Helm renders templates recursively, so no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QfJJ5EnsWiJnAKDNJyQYDv
|
@aglinxinyuan Good point — I verified the before-fix behavior as well and added it to the description ("How was this PR tested?"). Summary of the before-fix evidence: Client-side (DevTools → Network → WS, pre-fix build): opening a workflow starts one Server-side (production y-websocket server): counting ESTABLISHED connections on the shared-editing server showed the same signature — hundreds of connections from only a handful of active users, steadily climbing while they edited, with the room names dominated by random UUIDs (the throwaway validation graphs) rather than numeric workflow IDs. That growth is what exhausted Envoy's default per-cluster limit of 1024 connections and broke shared editing in production (#6239). Tabs still running the pre-fix bundle keep leaking in this pattern today, while tabs on the fixed bundle hold exactly one connection per open workflow. Also trimmed the changed-files list from the PR description as suggested. |
|
@Yicong-Huang Both points addressed:
Backport to v1.2: I'd say yes for the frontend commit — the per-edit leak exists in v1.2 as well, and any deployment fronted by a connection-capped proxy will eventually hit it. The frontend change is small and self-contained, so it should cherry-pick cleanly. The k8s/gateway part is optional for the backport (it only applies to Envoy Gateway deployments); happy to open the backport PR once this merges if you'd like. |
I don't have screenshot but we experienced disconnected users when they used shared editing and after checking the logs, we realized that we had over 800 websocket connections for 6 shared editing sessions |
aglinxinyuan
left a comment
There was a problem hiding this comment.
The logic looks correct to me. I don't have a way to verify that this actually fixes the issue since I can't reproduce it. However, as long as this change doesn't break anything, I'm okay with it.
|
During final testing of this PR, I found another issue that is not relevant to this one but still happen in shared editing feature. To make sure fixes are done properly, I will ask @Xiao-zhen-Liu. I close this PR for now, we do not consider it for RC v1.2. |
What changes were proposed in this PR?
Fixes two distinct
y-websocket(/rtc) connection leaks in the shared-editing code path, plus a gateway-side idle-connection reaper as defense-in-depth. Both leaks live onSharedModel→WebsocketProvider, and together they let the number of open realtime connections grow far out of proportion to the number of users until the shared-editing connection cap is exhausted and collaborative cursors/awareness stop syncing.Leak B — throwaway validation graph opens a socket per edit (dominant).
On every operator/link/property edit,
WorkflowCompilingServicebuilds a throwawayWorkflowGraphviaValidationWorkflowService.getValidTexeraGraph()just to produce a logical plan for compilation.WorkflowGraph's constructor unconditionally created aSharedModel, which unconditionally opened a new auto-connecting/rtcWebSocket in a random room. That graph is read once and never destroyed, so each edit leaked a socket that reconnects forever. A single user editing a workflow accumulates dozens of permanent sockets.Fix:
SharedModelnow opens a/rtcconnection only when a workflow ID (wid) is provided — the presence of awidis already the contract for joining shared editing (loadNewYModel(workflowId, ...)). With nowid, the model stays purely local: noWebsocketProvideris created and a standaloneAwarenesskeeps downstream reads ofawareness/clientIdworking. Throwaway validation/compilation graphs, unit-test fixtures, and new unsaved canvases have nowid, so they never touch the network — by construction, with no extra configuration flag. (The previous random-room fallback provided no real collaboration anyway — each client got its own random room — so removing it changes no user-visible behavior.)Leak A — incomplete provider teardown (per-navigation).
SharedModel.destroy()calledwsProvider.disconnect()(and only when the socket happened to be OPEN). Iny-websocket@1.4.0,disconnect()does not clear the provider's_checkInterval/_resyncIntervalreconnect timers, so the provider is never garbage-collected and, if it was mid-reconnect at destroy time (common during workflow switches), keeps re-opening a zombie socket for the life of the tab.Fix: call
wsProvider.destroy()unconditionally, which clears those timers, removes thebeforeunloadhandler, broadcasts awareness removal (peers drop our cursor), and closes the socket.Behavior for the real collaborative graph is unchanged: once a workflow is loaded with its
wid, it constructs aWebsocketProviderexactly as before.wsProvideris now optional (WebsocketProvider | undefined), so its remaining access sites use?..Gateway change — reap idle
/rtcconnections (defense-in-depth).The y-websocket reference server is single-process and every browser tab holds a long-lived WebSocket, so abandoned/leaked realtime sockets can pile up against the gateway's per-cluster
max_connectionscircuit breaker (Envoy's default is 1024) and starve legitimate co-editing traffic. This PR gives the/rtcpath its ownHTTPRoute(so a policy can target just that cluster without affecting the other static routes) and attaches an Envoy GatewayBackendTrafficPolicythat:connectionIdleTimeout(default5m), andcircuitBreaker.maxConnections(default8192) for headroom.Both knobs are configurable under
yWebsocketServer.trafficPolicyinvalues.yaml; settingtrafficPolicytonullomits the policy. The policy targets only the shared-editingHTTPRouteviatargetRefs, so no other backend's limits change. This is complementary to the frontend fix — the frontend fix stops creating the leaked sockets; the gateway policy caps the blast radius of any that still linger.Note — one extra housekeeping edit: since
BackendTrafficPolicyis an Envoy Gateway resource, both traffic-policy templates now live undertemplates/base/gateway/alongside the other gateway resources. This includes moving the pre-existingagent-service-backend-traffic-policy.yamlout oftemplates/base/agent-service/— a pure file move (Helm renders templates recursively), included here so all gateway-layer templates sit in one folder.Any related issues, documentation, discussions?
Fixes #6239.
How was this PR tested?
Existing frontend unit tests continue to pass; the
mock-workflow-plan.tsfixtures construct their graphs without awid, so tests open no real/rtcsockets.Manual verification, client-side (browser DevTools → Network → WS filter):
Before the fix (reproducing the leak):
/rtcWebSocket./rtcsocket to a random room that never closed and auto-reconnected forever — the WS count climbed monotonically with every edit.After the fix:
/rtcWebSocket (unchanged).Gateway change:
helm templatewas run to confirm the chart renders valid manifests — the/rtcHTTPRouteand theBackendTrafficPolicy(withconnectionIdleTimeoutandcircuitBreaker.maxConnections) are emitted with the policy correctly targeting only the shared-editing route, and the policy is omitted whenyWebsocketServer.trafficPolicyisnull.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)