From dcda4fce6abe53e331643ebaeef1aad664f41f2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5vard=20Kittelsen?= Date: Tue, 18 Aug 2026 16:29:34 +0200 Subject: [PATCH] fix(wallet): deliver the courtesy disconnect before tearing the relay down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit doDisconnect fired the courtesy `disconnect` message without awaiting it, then tore the relay connection down on the next line: conn.client.relay(disconnectMsg).catch(() => {}); // fire and forget ... conn.cleanup(); // closes the pool underneath it relay() resolves only after `Promise.allSettled(pool.publish(...))` — a real round trip to every configured relay. cleanup() closed the pool while that publish was still in flight, so the message usually never left and the dapp went on believing the wallet was connected until its own liveness timeout fired. Downstream wallets were patching this out of the published package. Teardown now splits into two halves with opposite timing requirements. Registry removal stays synchronous. getConnections() is what a UI renders, and connect() returns an existing connection for a URI, so leaving this one in the map while its teardown is pending would hand a caller a dying connection. This is the one place this differs from !30 and from the downstream patches, which defer the registry removal along with the teardown. Relay teardown is deferred until the publish settles, bounded by DISCONNECT_PUBLISH_TIMEOUT_MS (5s). The bound matters: "the publish never settles" is exactly the case where a relay is unreachable, and a socket that is never closed is worse than a courtesy message that is never delivered. disconnect() keeps its synchronous void signature — not a breaking change. Why it shipped broken: disconnect.test.ts only covered dapp → wallet. Nothing exercised wallet → dapp, and the failure is invisible from the wallet's side — its own state is correct either way, and only the peer notices. disconnect-delivery.test.ts covers that direction over a live relay, including the two invariants the deferral must not break (registry cleared immediately, URI reusable afterwards). Also fixes a latent hang in the integration harness that the new file exposed. setupConnection gated both the dapp_ready send and the message handler inside the keyexchangecomplete callback, so the handshake hung on receiving the wallet's single wallet_ready for the cycle. Miss it — the dapp's subscription can come up after the wallet has already published — and key exchange never resolves, the handler never registers, no dapp_ready is ever sent, and the wallet, guarded by walletReadySentThisCycle, has nothing prompting it to retry. It now re-announces dapp_ready(wallet_discovered=false) every 2s until key exchange completes, which resets that guard and earns another wallet_ready: the recovery path mutual discovery already specifies, which the harness was not using. Plus retry: 2 on the integration config, since these tests talk to live relays and a dropped connection is an environment failure rather than a regression. docs/wallet.md gains a "Sending disconnect" section for the synchronous/deferred split and what a caller may rely on. docs/protocol.md gains the sender-side half of the courtesy-disconnect semantics, which previously read as though "no acknowledgement" licensed fire-and-forget. That reading is what produced the bug. The race was diagnosed and first fixed by hantyrram (Ronaldo Ramano) in !30, which this supersedes — the deferral is their fix; this changes only how it is scoped. Co-Authored-By: Claude Opus 5 --- docs/protocol.md | 10 +- docs/wallet.md | 26 ++++- .../integration/disconnect-delivery.test.ts | 98 +++++++++++++++++++ packages/wallet/src/integration/helpers.ts | 41 +++++++- .../wallet/src/wallet-connection-manager.ts | 65 ++++++++++-- packages/wallet/vitest.integration.config.ts | 4 + 6 files changed, 230 insertions(+), 14 deletions(-) create mode 100644 packages/wallet/src/integration/disconnect-delivery.test.ts diff --git a/docs/protocol.md b/docs/protocol.md index 492c609..4ea5c80 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -358,6 +358,12 @@ Either side may send a `disconnect` message before tearing down the relay connec courtesy notification — the remote side treats the connection as closed immediately upon receipt (no acknowledgement). +No acknowledgement does not mean fire-and-forget on the sender's side. `relay()` resolves only +after the publish has been settled against every configured relay, so the sender must keep the +relay connection open until then — closing it first kills the publish in flight and the peer +never learns of the disconnect. Sending a courtesy `disconnect` and immediately tearing the +transport down is the same as not sending one. + ```typescript enum DisconnectReason { ProtocolMismatch = "protocol_mismatch", // no common protocol found during handshake @@ -373,7 +379,9 @@ interface DisconnectMessage { ``` **Wallet side** (`WalletConnectionManager`): -- `disconnect(id)` sends `UserDisconnect` before cleaning up. +- `disconnect(id)` sends `UserDisconnect`, then tears the connection down once the publish + settles — bounded, so an unreachable relay cannot hold the socket open. The connection leaves + the registry synchronously. See [wallet.md § Sending disconnect](wallet.md#sending-disconnect). - Incoming `disconnect` emits a `remoteDisconnect` event (`connectionId`, `reason`, `message`) and removes the connection. diff --git a/docs/wallet.md b/docs/wallet.md index e4f9e3e..338bd5a 100644 --- a/docs/wallet.md +++ b/docs/wallet.md @@ -69,9 +69,11 @@ class WalletConnectionManager extends EventEmitter { connect(uri: string): string // Tear down a specific connection, sending a UserDisconnect courtesy message. + // Returns as soon as the connection has left the registry; the relay socket + // closes once the courtesy message is published. See § Sending disconnect. disconnect(connectionId: string): void - // Tear down all connections. + // Tear down all connections. Each is disconnected independently. disconnectAll(): void // Snapshot of all connections for UI rendering. @@ -154,6 +156,28 @@ wallet_discovered=true → set dappDiscovered=true, no further action `dapp_name` and `dapp_icon` are captured from the first `dapp_ready` that includes them. +### Sending disconnect + +`disconnect(id)` and `disconnectAll()` split teardown into two phases, because the two halves +have opposite timing requirements. + +**Synchronous** — the connection is removed from the registry, its pending sign sequences are +released, and `connectionsChanged` is emitted. This cannot wait: `getConnections()` is what the +UI renders, and `connect()` returns the existing connection for a URI that already has one, so a +connection left in the map during teardown would be handed back to a caller as if it were live. + +**Deferred** — the relay socket closes only after the courtesy `disconnect` message has been +published. `RelayClient.relay()` resolves after the publish settles against every configured +relay, which is a real round trip; closing the socket before that kills the publish in flight and +the dapp keeps believing the wallet is connected until its own liveness timeout fires. + +The deferral is bounded by `DISCONNECT_PUBLISH_TIMEOUT_MS` (5 s). A publish that never settles is +precisely the unreachable-relay case, and a socket that is never closed is a worse failure than a +courtesy message that is never delivered. + +Callers do not need to await anything. The observable contract is that state is correct +immediately and delivery is best-effort within the timeout. + ### Receiving disconnect When a `disconnect` message arrives from the dapp: diff --git a/packages/wallet/src/integration/disconnect-delivery.test.ts b/packages/wallet/src/integration/disconnect-delivery.test.ts new file mode 100644 index 0000000..bd67336 --- /dev/null +++ b/packages/wallet/src/integration/disconnect-delivery.test.ts @@ -0,0 +1,98 @@ +// Copyright (C) 2026 Whiterun LLC, +// This software is licensed under the GNU Lesser General Public License (LGPL), version 3.0 or later. +// A copy of the license can be found in the LICENSE file or at https://www.gnu.org/licenses/lgpl-3.0.html + +/** + * Wallet-initiated disconnect must actually reach the dapp. + * + * disconnect.test.ts covers the other direction — the dapp sends `disconnect` + * and the wallet reacts. Nothing covered wallet → dapp, which is how this got + * shipped broken: doDisconnect fired the courtesy message without awaiting it and + * then tore the relay connection down, so the publish died mid-flight and the + * dapp went on believing the wallet was connected until its own liveness + * timeout. Downstream wallets were carrying a patch for it. + * + * These tests are worth their runtime because the failure is invisible locally — + * the wallet's own state is correct either way, and only the peer notices. + */ + +import { describe, it, expect, afterEach } from "vitest"; +import { RelayMsgAction, DisconnectReason } from "@wizardconnect/core"; +import type { DisconnectMessage, ProtocolMessage } from "@wizardconnect/core"; + +import { setupConnection, waitFor, type ConnectionHandles } from "./helpers.js"; + +let handles: ConnectionHandles | null = null; + +afterEach(() => { + handles?.cleanup(); + handles = null; +}); + +/** Disconnect messages the dapp actually received off the relay. */ +function disconnectsSeenByDapp(h: ConnectionHandles): DisconnectMessage[] { + return h.dapp.messages.filter( + (msg: ProtocolMessage) => msg.action === RelayMsgAction.Disconnect, + ) as DisconnectMessage[]; +} + +describe("wallet-initiated disconnect", () => { + it("delivers the courtesy disconnect to the dapp", async () => { + handles = await setupConnection(); + expect(disconnectsSeenByDapp(handles)).toHaveLength(0); + + handles.wallet.manager.disconnect(handles.wallet.connectionId); + + // The whole point: this arrives over a real relay, which it cannot do if the + // connection is torn down while the publish is still in flight. + await waitFor(() => disconnectsSeenByDapp(handles!).length > 0, { + timeoutMs: 20000, + what: "disconnect message received by the dapp", + }); + + expect(disconnectsSeenByDapp(handles)[0].reason).toBe( + DisconnectReason.UserDisconnect, + ); + }); + + it("removes the connection immediately, without waiting for delivery", async () => { + // Teardown is deferred, but the registry must not be: a caller that + // disconnects and then inspects state should never see the dying connection. + handles = await setupConnection(); + const { manager, connectionId } = handles.wallet; + expect(Object.keys(manager.getConnections())).toContain(connectionId); + + manager.disconnect(connectionId); + + expect(Object.keys(manager.getConnections())).not.toContain(connectionId); + }); + + it("delivers a disconnect for every connection in disconnectAll", async () => { + handles = await setupConnection(); + + handles.wallet.manager.disconnectAll(); + + await waitFor(() => disconnectsSeenByDapp(handles!).length > 0, { + timeoutMs: 20000, + what: "disconnect message from disconnectAll", + }); + expect(Object.keys(handles.wallet.manager.getConnections())).toHaveLength( + 0, + ); + }); + + it("lets the dapp reconnect on the same URI afterwards", async () => { + // Deferring teardown must not leave the URI unusable — connect() returns an + // existing connection for a URI, so a stale entry would be handed back. + handles = await setupConnection(); + const { manager, connectionId } = handles.wallet; + + manager.disconnect(connectionId); + const reconnectedId = manager.connect(handles.dapp.uri); + + expect(reconnectedId).not.toBe(connectionId); + expect(Object.keys(manager.getConnections())).toContain(reconnectedId); + + manager.disconnect(reconnectedId); + }); +}); diff --git a/packages/wallet/src/integration/helpers.ts b/packages/wallet/src/integration/helpers.ts index d3b0607..71a9f4f 100644 --- a/packages/wallet/src/integration/helpers.ts +++ b/packages/wallet/src/integration/helpers.ts @@ -205,10 +205,38 @@ export async function setupConnection( } }); - // Initial dapp_ready — tells wallet we're here (wallet not yet discovered) + // Prompt one more wallet_ready, now that the handler above is registered. + // The wallet_ready that completed key exchange was consumed by + // initiateDappRelay before this handler existed, so it is not in + // walletReadyMessages and the caller's "wallet_ready with paths" wait + // needs a fresh one. dapp_discovered=false is what makes the wallet + // re-send. await sendDappReady(false); }); + // Re-announce until key exchange completes. + // + // The wallet sends exactly one wallet_ready per connection cycle, and it + // fires as soon as manager.connect() resolves — which can be before this + // dapp's relay subscription is live. If that single message is missed there + // is nothing to retry against: keyexchangecomplete never fires, so the + // handler above never registers and no dapp_ready is ever sent. The suite + // then sat until the 15s "key exchange" timeout. Under singleFork the + // previous file's teardown is still closing sockets while this runs, which + // is exactly when the race is won by the wrong side. + // + // dapp_ready(wallet_discovered=false) resets walletReadySentThisCycle on the + // wallet, so each retry earns another wallet_ready. This is the recovery path + // the protocol's mutual-discovery design already specifies — the harness + // simply was not using it. + const reannounce = setInterval(() => { + if (keyExchanged) { + clearInterval(reannounce); + return; + } + if (dappClient) sendDappReady(false).catch(() => {}); + }, 2000); + // ---- Wallet side ---- const manager = new WalletConnectionManager(adapter); @@ -216,7 +244,16 @@ export async function setupConnection( // ---- Wait for key exchange ---- - await waitFor(() => keyExchanged, { timeoutMs: 15000, what: "key exchange" }); + try { + await waitFor(() => keyExchanged, { + timeoutMs: 15000, + what: "key exchange", + }); + } finally { + // Must not outlive the wait: on timeout a leaked interval keeps publishing + // dapp_ready into later tests and holds the fork open. + clearInterval(reannounce); + } // ---- Wait for wallet_ready with paths ---- diff --git a/packages/wallet/src/wallet-connection-manager.ts b/packages/wallet/src/wallet-connection-manager.ts index 108c19d..d95f09b 100644 --- a/packages/wallet/src/wallet-connection-manager.ts +++ b/packages/wallet/src/wallet-connection-manager.ts @@ -45,6 +45,15 @@ export interface PendingSignRequest { request: SignTransactionRequest; } +/** + * How long doDisconnect waits for the courtesy `disconnect` message to be + * published before tearing the relay connection down anyway. + * + * Generous enough for a slow relay, short enough that an unreachable one cannot + * hold the socket open indefinitely. + */ +const DISCONNECT_PUBLISH_TIMEOUT_MS = 5000; + interface ActiveConnection { id: string; uri: string; @@ -217,22 +226,58 @@ export class WalletConnectionManager extends EventEmitter {}); - } - + // Drop the connection from the registry synchronously, before any awaiting. + // getConnections() must reflect the disconnect immediately, and connect() + // returns an existing connection for a URI — so leaving this one in the map + // while its teardown is pending would hand a caller a dying connection. for (const seq of conn.signSequences) { this.activeSignSequences.delete(seq); } clearInterval(conn.notificationProcessor ?? undefined); - conn.cleanup(); + conn.notificationProcessor = null; this.connections.delete(connectionId); this.emit("connectionsChanged"); + + if (!sendMessage || !conn.client) { + conn.cleanup(); + return; + } + + const disconnectMsg: DisconnectMessage = { + action: RelayMsgAction.Disconnect, + reason: DisconnectReason.UserDisconnect, + time: Math.floor(Date.now() / 1000), + }; + + // Tear down only once the courtesy message has actually gone out. + // + // relay() resolves after `Promise.allSettled(pool.publish(...))` — a real + // round trip to every configured relay. Calling conn.cleanup() straight + // after firing it closed the pool underneath the in-flight publish, so the + // disconnect usually never reached the relay and the dapp went on believing + // the wallet was connected until its own liveness timeout fired. Downstream + // wallets were patching this out of the published package. + // + // Bounded, because "the publish never settles" is exactly the case where a + // relay is unreachable, and a socket that is never closed is worse than a + // courtesy message that is never delivered. + let torndown = false; + const teardown = () => { + if (torndown) return; + torndown = true; + conn.cleanup(); + }; + + const timer = setTimeout(teardown, DISCONNECT_PUBLISH_TIMEOUT_MS); + conn.client + .relay(disconnectMsg) + .catch(() => { + // Nothing to do: we are disconnecting either way. + }) + .finally(() => { + clearTimeout(timer); + teardown(); + }); } /** diff --git a/packages/wallet/vitest.integration.config.ts b/packages/wallet/vitest.integration.config.ts index cbf19f8..c794b44 100644 --- a/packages/wallet/vitest.integration.config.ts +++ b/packages/wallet/vitest.integration.config.ts @@ -9,6 +9,10 @@ export default defineConfig({ include: ["src/integration/**/*.test.ts"], testTimeout: 60000, hookTimeout: 60000, + // These tests talk to live relays. A dropped connection or a slow publish + // is an environment failure, not a regression, and without a retry a single + // one reds the whole pipeline. + retry: 2, // Run integration tests serially to avoid relay contention pool: "forks", poolOptions: {