Merge branch 'fix/disconnect-race' into 'master'

Deliver the courtesy disconnect before tearing the relay down

See merge request riftenlabs/lib/wizardconnect!32

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.
 

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:

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);
+  });
+});

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 ----
 

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<WalletConnectionManage
     const conn = this.connections.get(connectionId);
     if (!conn) return;
 
-    if (sendMessage && conn.client) {
+    // 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.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),
     };
-      conn.client.relay(disconnectMsg).catch(() => {});
-    }
 
-    for (const seq of conn.signSequences) {
-      this.activeSignSequences.delete(seq);
-    }
-    clearInterval(conn.notificationProcessor ?? undefined);
+    // 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();
-    this.connections.delete(connectionId);
-    this.emit("connectionsChanged");
+    };
+
+    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();
+      });
   }
 
   /**

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: {