fix: abandon the parked connect await when an unestablished close aborts

[alejandro-jimenez-dcl]

Thread-affinity correction (post-consolidation): the original fix forced
useCurrentSynchronizationContext: false on every ConnectAsync, which dropped the
connecting thread's SynchronizationContext. The social-service RPC transport starts its
receive loop in the connect continuation, so a main-thread connect that lost its context
moved every RPC response - and the passport friendship-status continuation that flips UI
buttons active - onto a background thread, faulting Unity graphics ("Graphics device is
null") and crashing natively. Now marshals back only when a SynchronizationContext exists
(marshalBackToIssuingContext = SynchronizationContext.Current != null), restoring the
main-thread receive loop for social/comms transports while preserving the no-throw behaviour
on the context-less V8 script-invoke thread. Regression test
ResumeOnTheCallerSynchronizationContextAfterConnecting; v16 EditMode RED 6/1-fail → GREEN 6/6.

Problem

Scene WebSocket closes race the connect handshake: mono's ClientWebSocket.CloseAsync NREs
mid-handshake (Sentry UNITY-EXPLORER-PNT, 624 events) and throws
InvalidOperationException: The WebSocket is not connected on a never-connected socket.
Both surface out of DCLWebSocket.CloseAsync/Dispose on the desktop path.

Root cause

Two layers. (1) CloseAsync called the inner close handshake regardless of connection
state. (2) Deeper - verified against the corefx source Unity's mono ships: a ConnectAsync
parked on an unanswered HTTP upgrade read is structurally unreachable from outside - the
socket-dispose cancellation registrations are scoped to ConnectSocketAsync only (gone once
TCP connects), the upgrade-read loop's NetworkStream.ReadAsync ignores its token once the
read is pending, and handle-level Abort() mid-handshake only cancels a source nothing
listens to at that stage. No fix shape that keeps awaiting the BCL task can work.

Fix

DCLWebSocket (desktop branch): CloseAsync gates on the public state machine - when
State is not (Open or CloseReceived or CloseSent) it takes an abort path instead of the
inner close: it calls Abort(), which owns the abort pair (cancel the per-socket
connectAbort CTS, then ws.Abort()) so aborting mid-handshake through either public API
unparks the connect. Dispose() tears the CTS down via the existing
SafeCancelAndDispose() and stays idempotent. ConnectAsync
awaits the BCL connect via .AsUniTask().AttachExternalCancellation(linked.Token) (caller
token + connectAbort.Token), abandoning the parked BCL task; AttachExternalCancellation
observes and discards the abandoned task's eventual outcome (double-completion of the
vendored UniTaskCompletionSourceCore is a verified silent no-op). The caller-visible
contract matches WHATWG "fail the connection": connect completes canceled, onerror/onclose
fire. Residual: the abandoned BCL task parks until the TCP peer dies - same as pin behavior
for an unanswered server.

Behavior notes

• WebSocketArchipelagoLiveConnection.DisconnectAsync on a never-connected socket now
  returns SuccessResult where mono previously threw InvalidOperationException →
  ErrorResult. Callers treat disconnect failure as log-only today; disconnecting an
  unconnected connection reporting success is the more correct contract, but it is a flip.
• DCLWebSocket.Dispose() remains idempotent: the added CTS teardown goes through the
  existing SafeCancelAndDispose(), so a second Dispose() (scene-teardown rental loop)
  stays a no-op instead of throwing ObjectDisposedException.

Test

New EditMode Utility.Tests.DCLWebSocketCloseAsyncShould (5 tests):
AbortInsteadOfThrowingWhenClosedDuringHandshake (pin: NRE),
CompleteCleanlyOnANeverConnectedSocket (pin: InvalidOperationException),
UnparkAPendingConnectWhenAborted (public Abort() mid-handshake must complete the parked
connect), ConnectFromAThreadWithoutSynchronizationContext (the V8-thread constraint), and
an open-socket guard proving the real close handshake still runs.

Validation

Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 2/3 as intended (the exact NRE and
InvalidOperationException signatures; guard passed) / GREEN PASS 3/3. Two earlier fix
shapes (state guard alone; linked-CTS into ws.ConnectAsync) deterministically failed the
hang-guard on the same lane - the parked-read analysis above is validation-driven, not
theoretical.

Note: the initial adversarial review covered the state-guard shape; the connect-abandon
mechanism was added in the validation repair round and needs a re-review before merge.

Related: Sentry UNITY-EXPLORER-PNT (close-during-handshake NRE), UNITY-EXPLORER-PK9
(CloseAsync WebSocketException on dropped sockets, expected to collapse with this)

Includes inspection-warning cleanup in all touched files.

[github-actions]

🚦 CI Status

Build skipped — no changes detected under Explorer/.

No C# files changed — lint ratchet skipped.

Some Unity tests failed ❌

TESTS SUITE	Result	Passed	Failed	Skipped
EditMode	❌ 2 failed	25058	2	13
PlayMode	✅ Passed	236	0	37

Failed tests (2)

• [editmode] DCL.Tests.CodeConventionsTests.VerifyShouldNotUseConcurrentCollection
• [editmode] DCL.Tests.CodeConventionsTests.VerifyShouldNotUseThreadingApiDirectly("Assets/DCL/Infrastructure/Utility/Networking/DCLWebSocket.cs")

[decentraland-bot]

STEP 1 — Context & Scope

Files changed: 3 (1 production, 1 new test, 1 meta).

• DCLWebSocket.cs — desktop WebSocket wrapper; adds connectAbort CTS, state gate in CloseAsync, external-cancellation abandon in ConnectAsync, CTS cancel in Abort().
• DCLWebSocketCloseAsyncShould.cs — new, 6 EditMode tests covering close-during-handshake, abort-during-handshake, never-connected close, open-socket handshake guard, no-SynchronizationContext thread, and SynchronizationContext resume.

Surrounding context examined: WebSocketArchipelagoLiveConnection, ClientWebSocketApiImplementation, WebSocketRpcTransport, DappWeb3EthereumApi, LocalSceneDevelopmentController, DClWebSocketDefinitions.cs (WebSocketState enum), CodeConventionTests.cs (threading and concurrent-collection convention enforcers), excludes_threading.txt.

STEP 2 — Root-cause check

PASS. The PR correctly identifies two layers of root cause:

1. CloseAsync entered the inner close handshake regardless of connection state, causing NRE when mono's inner socket was null mid-upgrade.
2. A ConnectAsync parked on an unanswered HTTP upgrade read is structurally unreachable from outside via the BCL's cancellation/abort surface.

The fix addresses both: a state gate prevents the close handshake on non-established connections, and AttachExternalCancellation with a dedicated connectAbort CTS abandons the unreachable BCL task. This is not a symptom-level null-check — it restructures the close/abort contract to match WHATWG semantics.

STEP 3 — Design & integration

PASS. The fix is correctly placed in DCLWebSocket itself — it is the abstraction layer over ClientWebSocket that all callers use. The mono quirk (inner socket null during upgrade) belongs here, not in callers.

Owner search: DCLWebSocket is created by:

• WebSocketArchipelagoLiveConnection.Current.New() — creates new instance on reconnect via TryUpdateWebSocket(). Old instance disposed.
• ClientWebSocketApiImplementation.WebSocketRental — creates new instance per CreateWebSocket() call. Disposed via Dispose() on the rental.
• WebSocketRpcTransport constructor — single-use. Disposed via Dispose().
• DappWeb3EthereumApi — field set in ConnectToRpcAsync. Disposed in DisconnectFromRpcAsync.
• LocalSceneDevelopmentController — field set in ConnectToServerAsync.

No new lifecycle entity is introduced. The connectAbort CTS matches the single-use lifecycle of ClientWebSocket (which cannot be reconnected). Every caller that reconnects creates a new DCLWebSocket instance, getting a fresh CTS.

Teardown trace:

• connectAbort → created in field initializer, canceled in Abort() (with ODE catch for racing Dispose()), disposed via SafeCancelAndDispose() in Dispose(). ✅
• linked CTS in ConnectAsync → using scoped, disposed on method exit. ✅

Caller impact of OperationCanceledException from aborted connect:

• WebSocketArchipelagoLiveConnection.ConnectAsync — catches Exception → ErrorResult. ✅
• ClientWebSocketApiImplementation.ConnectAsync — no catch; propagates. The scene runtime bridge handles cancellation from its CTS teardown path. Acceptable.
• WebSocketRpcTransport.ConnectAsync — no catch; propagates. Caller-level handling expected. Acceptable.
• DappWeb3EthereumApi — propagates to SendWithoutConfirmationAsync which has catch (Exception) + finally. ✅

STEP 4 — Member audit

• connectAbort (new private field): used by ConnectAsync (creates linked CTS from it), Abort() (cancels it), Dispose() (safe-cancel-and-dispose). 3 consumers. Correct single-responsibility: owns the signal that abandons a parked connect. ✅
• marshalBackToIssuingContext (local in ConnectAsync): single-use, captures SynchronizationContext.Current != null to pass to AsUniTask. Correctly scoped as a local. ✅
• CloseAsync signature change (String? → string?): correct C# keyword alias convention per CLAUDE.md. ✅

STEP 5 — Line-level review

P1 — CI-breaking: SynchronizationContext in forbidden threading API list

DCLWebSocket.cs line 110 introduces SynchronizationContext.Current, which is in excludes_threading.txt. The file is NOT in WEBGL_THREAD_SAFETY_EXCLUDED_PATHS (it is in WEB_SOCKETS_EXCLUDED_PATHS, which is used by a different test). CI fails with:

VerifyShouldNotUseThreadingApiDirectly("Assets/DCL/Infrastructure/Utility/Networking/DCLWebSocket.cs")

Fix: Add DCLWebSocket.cs to WEBGL_THREAD_SAFETY_EXCLUDED_PATHS in CodeConventionTests.cs:

private static readonly string[] WEBGL_THREAD_SAFETY_EXCLUDED_PATHS = {
    "Assets/DCL/Input/UnityInputSystem/DCLInput.cs",
    "Assets/Plugins/UUAV/Packages/UUAV/Runtime/UUAVPlayer.cs",
    "Assets/DCL/Infrastructure/Utility/Networking/DCLWebSocket.cs", // desktop-only; SynchronizationContext.Current gates the connect continuation's thread affinity
};

P1 — CI-breaking: BlockingCollection in concurrent collection scan

DCLWebSocketCloseAsyncShould.cs uses System.Collections.Concurrent.BlockingCollection (for the SingleThreadSynchronizationContext test helper). CI fails with:

VerifyShouldNotUseConcurrentCollection

Fix: Add the test file to the ignorePaths in VerifyShouldNotUseConcurrentCollection:

string[] ignorePaths = new []
{
    "Assets/DCL/Infrastructure/Utility/Multithreading/DCLConcurrentDictionary.cs",
    "Assets/DCL/Infrastructure/Utility/Multithreading/DCLConcurrentBag.cs",
    "Assets/DCL/Infrastructure/Utility/Multithreading/DCLConcurrentQueue.cs",
    "Assets/DCL/Infrastructure/Utility/Tests/DCLWebSocketCloseAsyncShould.cs", // test-only SynchronizationContext pump uses BlockingCollection
};

Both fixes are in Explorer/Assets/DCL/Tests/Editor/CodeConventionTests.cs (not in the PR's diff).

No other blocking issues

The production and test code is well-structured. Sub-agent reviews (security, architecture, code quality) confirmed no additional P0/P1 findings.

STEP 6 — Complexity

COMPLEX. Modifies async/UniTask cancellation flows, introduces AttachExternalCancellation with a linked CTS, adds SynchronizationContext-conditional thread-affinity handling, and affects the WebSocket connect/close/abort lifecycle used by networking, social, and scene subsystems.

STEP 7 — QA assessment

QA_REQUIRED: YES. Changes affect runtime networking code (WebSocket connect/close lifecycle) used by scene communication, archipelago transport, and social-service RPC. While the fix prevents crashes, the behavioral change (close on non-established socket returning success instead of error) and the thread-affinity correction need validation in the live client.

STEP 8 — Non-blocking warnings

No Main scene modification. No warnings.

STEP 9 — Verdict

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async/CancellationToken patterns, SynchronizationContext thread-affinity, and WebSocket lifecycle used by networking/social/scene subsystems
QA_REQUIRED: YES

Summary: The fix logic is architecturally sound and the test coverage is thorough (6 tests covering every identified failure mode including a SynchronizationContext resume regression test). The FAIL is solely due to two code-convention test failures in CI — DCLWebSocket.cs needs to be added to WEBGL_THREAD_SAFETY_EXCLUDED_PATHS (for its new SynchronizationContext usage) and the test file needs to be added to the concurrent-collection ignore list (for its BlockingCollection usage). Once those two exclusions are added in CodeConventionTests.cs, the review verdict should flip to PASS.

---

Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

[decentraland-bot]

PR #9811, run #32264708003

Builds: Windows change, Windows baseline, macOS change, macOS baseline

How to read this table

• Each build is measured 3 times. The values are the median, and (min–max) is the lowest and highest of those runs — a wide range means the metric is noisy and small differences are not trustworthy.
• Δ is Change minus Baseline (a negative Δ means Change is faster).
• 🟢 faster / 🔴 slower — a real difference: larger than both 3% and the run-to-run range.
• ⚪ within noise — the difference is smaller than how much the build varies between its own runs, so it cannot be told apart from random variation. Treat it as no change.
• Exceptions per run — the average number of exceptions in a run's log; more than the baseline is flagged 🔴 even when frame times look fine. The Exception breakdown under each table groups them by the explorer's report category and exception type (as totals across the runs).
• A run that logged unusually many exceptions (at least 10 and 5× the median of its build's runs — e.g. a service was down during it) is excluded from all numbers and called out under the table.

Intel Core i5

Metric	Baseline	Change	Δ	Result
Samples	2313 (×3)	1766 (×3)
CPU average	38.6 ms (33.4–38.8)	50.3 ms (42.1–54.0)	11.7 ms	⚪ within noise
CPU 1% worst	322.6 ms (57.0–343.5)	834.6 ms (739.0–887.4)	512.0 ms	🔴 159% slower
CPU 0.1% worst	344.1 ms (341.1–360.3)	862.0 ms (783.4–911.1)	517.8 ms	🔴 150% slower
GPU average	9.5 ms (9.2–9.6)	10.5 ms (9.6–10.5)	0.9 ms	🔴 10% slower
GPU 1% worst	35.6 ms (23.5–37.7)	87.7 ms (55.7–92.2)	52.1 ms	🔴 146% slower
GPU 0.1% worst	44.4 ms (39.8–45.0)	95.8 ms (85.0–98.2)	51.4 ms	🔴 116% slower
Exceptions per run	66	66	0	⚪ none new

Exception breakdown

Exception	Baseline (3 runs)	Change (3 runs)
[UI] DllNotFoundException	192	192
[ENGINE] NullReferenceException	3	3
[ENGINE] ObjectDisposedException	3	3

Apple M1

Metric	Baseline	Change	Δ	Result
Samples	4105 (×3)	4169 (×3)
CPU average	21.8 ms (21.8–22.9)	21.5 ms (21.4–22.4)	-0.3 ms	⚪ within noise
CPU 1% worst	215.9 ms (215.7–217.7)	185.7 ms (169.6–227.9)	-30.2 ms	⚪ within noise
CPU 0.1% worst	226.3 ms (222.9–228.8)	239.9 ms (233.5–240.9)	13.6 ms	🔴 6% slower
GPU average	2.5 ms (2.0–3.2)	2.9 ms (1.6–5.2)	0.4 ms	⚪ within noise
GPU 1% worst	34.3 ms (34.2–36.2)	34.7 ms (34.4–35.5)	0.4 ms	⚪ within noise
GPU 0.1% worst	36.3 ms (34.8–37.5)	37.0 ms (36.7–37.6)	0.7 ms	⚪ within noise
Exceptions per run	0	0	0	⚪ none new

[alejandro-jimenez-dcl]

Superseded by #9828, which compounds this fix together with the rest of the bugsweep batch. Closing in favor of that combined PR.