From a9197358742c229988267cf0766011561d0b2c1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5vard=20Kittelsen?= Date: Fri, 14 Aug 2026 08:45:16 +0200 Subject: [PATCH] fix(wallet): bound the deferral, keep the registry synchronous, add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on the previous commit, which correctly identified the race and the shape of the answer. Three things needed to change before it could ship, and the reason each matters is easy to miss. DEFER THE TEARDOWN, NOT THE REGISTRY The previous commit moved everything into clean(), including this.connections.delete() and the connectionsChanged emit. That leaves the connection in the map until the publish settles, so after disconnect() returns: - getConnections() still lists a connection the user just disconnected; - connect() matches by URI at the top of the function, so reconnecting to the same URI hands back the dying connection — which clean() then deletes, leaving the caller holding an id for something already gone. Registry removal is now synchronous and only conn.cleanup() — the transport teardown, the part that was killing the in-flight publish — is deferred. BOUND THE WAIT There was no timeout: if relay() never settles, clean() never runs and the interval, the socket and the map entry leak permanently. That is not a corner case. relay() enqueues rather than publishes when the client is not ready, so disconnecting while the relay is unreachable — precisely when a user reaches for disconnect — can hang forever. DISCONNECT_PUBLISH_TIMEOUT_MS bounds it: an undelivered courtesy message is a smaller problem than a socket that never closes. RESTORE THE TYPES doDisconnect had lost its parameter annotations and its `private` modifier, and the message literal its DisconnectMessage type. Under "strict": true that is TS7006 on both parameters — the package does not compile — and dropping `private` widened the public API by accident. Formatting is back to the repo's prettier config. TESTS None of the above is visible from the wallet's side: its own state is correct either way and only the peer notices, which is how the original bug shipped. disconnect.test.ts only ever covered dapp -> wallet. disconnect-delivery.test.ts covers wallet -> dapp over a live relay. Verified by reverting the fix: the two delivery tests time out after 20s, and the two invariant tests — immediate registry removal, and reconnecting on the same URI — pass either way, which is precisely why they are there. Co-Authored-By: Claude Opus 5 --- .../integration/disconnect-delivery.test.ts | 98 +++++++++++++++++++ .../wallet/src/wallet-connection-manager.ts | 84 +++++++++++----- 2 files changed, 157 insertions(+), 25 deletions(-) create mode 100644 packages/wallet/src/integration/disconnect-delivery.test.ts 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/wallet-connection-manager.ts b/packages/wallet/src/wallet-connection-manager.ts index dd189b1..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; @@ -213,37 +222,62 @@ export class WalletConnectionManager extends EventEmitter { - for (const seq of conn.signSequences) { - this.activeSignSequences.delete(seq); - } - clearInterval(conn.notificationProcessor ?? undefined); - conn.cleanup(); - this.connections.delete(connectionId); - this.emit("connectionsChanged"); + if (!sendMessage || !conn.client) { + conn.cleanup(); + return; } - if (sendMessage && conn.client) { - const disconnectMsg = { - action: RelayMsgAction.Disconnect, - reason: DisconnectReason.UserDisconnect, - time: Math.floor(Date.now() / 1000), - }; - conn.client.relay(disconnectMsg) - .then(() => { - clean() - }) - .catch(() => clean() ); + const disconnectMsg: DisconnectMessage = { + action: RelayMsgAction.Disconnect, + reason: DisconnectReason.UserDisconnect, + time: Math.floor(Date.now() / 1000), + }; - } else { - clean() - } + // 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(); + }); } /**