fix(wallet): deliver the courtesy disconnect before tearing the relay down
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 <noreply@anthropic.com>
This commit is contained in:
parent
167ec21474
commit
dcda4fce6a
6 changed files with 230 additions and 14 deletions
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
98
packages/wallet/src/integration/disconnect-delivery.test.ts
Normal file
98
packages/wallet/src/integration/disconnect-delivery.test.ts
Normal file
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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 ----
|
||||
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
const disconnectMsg: DisconnectMessage = {
|
||||
action: RelayMsgAction.Disconnect,
|
||||
reason: DisconnectReason.UserDisconnect,
|
||||
time: Math.floor(Date.now() / 1000),
|
||||
};
|
||||
conn.client.relay(disconnectMsg).catch(() => {});
|
||||
}
|
||||
|
||||
// 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();
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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: {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue