Skip to content

fix: NWCClient.close() hanging Jest tests #530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AkarshSahlot wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: NWCClient.close() hanging Jest tests #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions jest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
/** @type {import('ts-jest').JestConfigWithTsJest} */
module.exports = {
preset: 'ts-jest',
testEnvironment: 'node',
export default {
preset: "ts-jest",
testEnvironment: "node",
transform: {
"^.+\\.tsx?$": ["ts-jest", { useESM: true }],
},
Comment on lines +2 to +7
Copy link

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check package.json test scripts
echo "=== TEST SCRIPTS ==="
cat package.json | jq '.scripts.test, .scripts["test:*"]' 2>/dev/null || echo "Could not parse scripts"

echo -e "\n=== JEST CONFIG FILES ==="
# Check for jest config files
fd -t f -e js -e ts -e json 'jest' --max-depth 2 2>/dev/null || echo "No jest config files found"

echo -e "\n=== LOOKING FOR extensionsToTreatAsEsm ==="
# Search for extensionsToTreatAsEsm in config files
rg 'extensionsToTreatAsEsm' -t ts -t json --no-heading 2>/dev/null || echo "Not found"

echo -e "\n=== FULL jest.config.ts CONTENT ==="
cat jest.config.ts 2>/dev/null || echo "File not found"

Repository: getAlby/js-sdk

Length of output: 415

🏁 Script executed:

# Double-check if there's a setup file or if jest config has other sections
cat -n jest.config.ts

Repository: getAlby/js-sdk

Length of output: 305

Add required ESM configuration for Jest.

The useESM: true option requires two additional configurations to work correctly:

  1. Add extensionsToTreatAsEsm: ['.ts'] to the jest config to tell Jest to treat .ts files as ESM.
  2. Update the test script in package.json to include NODE_OPTIONS=--experimental-vm-modules jest or use --experimental-vm-modules flag. Currently, the test script is just "jest" with no flag, which will cause ESM imports to fail at runtime.

Without these additions, tests will fail when encountering ESM imports.

🤖 Prompt for AI Agents 
In `@jest.config.ts` around lines 2 - 7, The Jest ESM setup is incomplete: when
using transform "^.+\\.tsx?$" with ts-jest option useESM: true you must also add
extensionsToTreatAsEsm: ['.ts'] to the exported Jest config object and update
the project's test script so Jest runs with Node's experimental VM modules
(e.g., set NODE_OPTIONS=--experimental-vm-modules before running jest or add the
--experimental-vm-modules flag) to avoid runtime ESM import failures; modify the
jest config object (where transform and useESM are defined) to include
extensionsToTreatAsEsm and update package.json's "test" script accordingly.

};
2 changes: 1 addition & 1 deletion src/nwc/NWAClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
Nip47NotificationType,
} from "./types";
import { NWCClient } from "./NWCClient";
import { SubCloser } from "nostr-tools/lib/types/abstract-pool";
import { SubCloser } from "nostr-tools/abstract-pool";

export type NWAOptions = {
relayUrls: string[];
Expand Down
22 changes: 19 additions & 3 deletions src/nwc/NWCClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
EventTemplate,
SimplePool,
} from "nostr-tools";
import { normalizeURL } from "nostr-tools/utils";
import { hexToBytes, bytesToHex } from "@noble/hashes/utils";
import {
Nip47EncryptionType,
Expand Down Expand Up @@ -55,7 +56,7 @@ import {
Nip47CancelHoldInvoiceResponse,
Nip47NetworkError,
} from "./types";
import { SubCloser } from "nostr-tools/lib/types/abstract-pool";
import { SubCloser } from "nostr-tools/abstract-pool";

export interface NWCOptions {
relayUrls: string[];
Expand Down Expand Up @@ -198,8 +199,23 @@ export class NWCClient {
return getEventHash(event);
}

close() {
return this.pool.close(this.relayUrls);

async close(): Promise<void> {
const closePromises: Promise<void>[] = [];
for (const url of this.relayUrls) {
const relayUrl = normalizeURL(url);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const relay = (this.pool as any).relays.get(relayUrl);
Comment on lines +207 to +208
Copy link

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/nwc/NWCClient.ts | sed -n '200,220p'

Repository: getAlby/js-sdk

Length of output: 841

🏁 Script executed:

find . -name "package.json" -type f | head -5 | xargs grep -l "nostr-tools"

Repository: getAlby/js-sdk

Length of output: 72

🏁 Script executed:

cat -n package.json | grep -A 5 -B 5 "nostr-tools"

Repository: getAlby/js-sdk

Length of output: 439

🌐 Web query:

nostr-tools SimplePool v2.17.0 API relay connections public methods

💡 Result:

In nostr-tools / @nostr/tools “v2.x” SimplePool (including v2.17.0), relay connections are managed via the SimplePool class from @nostr/tools/pool, which (in v2.x) is essentially a thin wrapper over the pool’s base class and exposes the pool’s relay-connection and subscription/publication surface. (jsr.io)

Relay-connection / pool-management API (public surface)

Construction / environment

  • new SimplePool({ enablePing?, enableReconnect? }) (jsr.io)
  • useWebSocketImplementation(wsCtor) (needed when your runtime doesn’t provide WebSocket, e.g. Node with ws) (jsr.io)
  • Options:
    • enablePing (heartbeat/ping reliability) (jsr.io)
    • enableReconnect (auto-reconnect; can be a callback to adjust filters on reconnect) (github.com)

Establishing/keeping relay connections

  • ensureRelay(url) (get/create and connect a relay handle for a URL) (jsr.io)
  • relays (current relay set managed by the pool) (jsr.io)
  • trustedRelayURLs (track/manage “trusted” relay URLs) (jsr.io)
  • trackRelays(...) (enable relay tracking behavior) (jsr.io)
  • listConnectionStatus(...) (inspect connection state across relays) (jsr.io)
  • pruneIdleRelays(...) (drop idle relay connections) (jsr.io)
  • Hooks/controls commonly used around connection attempts:
    • allowConnectingToRelay(...) (jsr.io)
    • maxWaitForConnection(...) (jsr.io)
    • onRelayConnectionSuccess(...), onRelayConnectionFailure(...) (jsr.io)
    • automaticallyAuth(...) (auto-auth behavior, e.g., NIP-42 flows) (jsr.io)

Closing / cleanup

  • close(relayUrls) (close specific relays) (jsr.io)
  • destroy() (tear down the pool) (jsr.io)

Core “talk to relays” methods you’ll use once connected

  • publish(relays, event) (jsr.io)
  • get(relays, filter, opts?) (jsr.io)
  • querySync(relays, filter, opts?) (jsr.io)
  • Subscriptions (streaming):
    • subscribe(...), subscribeEose(...)
    • subscribeMany(...), subscribeManyEose(...)
    • subscribeMap(...) (jsr.io)
  • seenOn(eventId) (relay URLs where an event was observed) (jsr.io)

If you want, paste your current TypeScript type signature (or your import path: nostr-tools vs @nostr/tools) and I’ll map these to the exact v2.17.0 typings you have in your build.

Citations:

🌐 Web query:

nostr-tools SimplePool relays getter API documentation

💡 Result:

nostr-tools / @nostr/tools doesn’t expose a public “relays getter” on SimplePool (i.e., there’s no supported API like pool.relays() / pool.getRelays() to list currently-known/connected relays). The public surface is basically: you pass relays: string[] into each call, and SimplePool manages connections internally. [1][2]

Where it’s documented

  • The canonical docs/examples live on JSR (@nostr/tools), including SimplePool usage (get, list, publish, etc.). [1]
  • The class shape people often reference (showing internal private _conn) appears in discussions like GitHub issue #316—those internals are not part of the public API. [2]

What you can do instead (supported)

  • Track your relay URLs yourself (keep your own const relays = [...] and reuse it when calling pool.get(...), pool.list(...), pool.sub(...), etc.). [1]
  • If what you really want is “which relays did an event come from?”, use pool.seenOn(eventId) (this is a public method, but it’s per event, not a “list all relays in the pool” getter). [1]

Docs to start from

  • JSR package docs (includes SimplePool examples and options like ping/reconnect): [1]

Sources: [1] [2]

🏁 Script executed:

cat -n src/nwc/NWCClient.ts | sed -n '195,225p'

Repository: getAlby/js-sdk

Length of output: 1195

🏁 Script executed:

grep -n "relayUrls" src/nwc/NWCClient.ts | head -20

Repository: getAlby/js-sdk

Length of output: 1001

🏁 Script executed:

grep -n "this.relayUrls\|this.pool" src/nwc/NWCClient.ts | head -30

Repository: getAlby/js-sdk

Length of output: 1097

🏁 Script executed:

grep -A 5 "listConnectionStatus" src/nwc/NWCClient.ts

Repository: getAlby/js-sdk

Length of output: 211

🌐 Web query:

nostr-tools SimplePool listConnectionStatus v2.17.0 return type API

💡 Result:

In nostr-tools / @nostr/tools v2.17.0, SimplePool.listConnectionStatus() (inherited from AbstractSimplePool) has the return type:

  • Map<string, boolean> — a map of relay URL → connection status. [1][2]

Accessing SimplePool's private .relays field via as any is fragile and unsupported.

(this.pool as any).relays.get(relayUrl) reaches into an undocumented internal implementation detail of nostr-tools v2.17.0. nostr-tools does not expose a public API to enumerate relay connections or access WebSocket handles. This will silently break if the internal field is renamed, restructured, or removed in a future release, causing close() to skip awaiting WebSocket closures without any error.

While nostr-tools does provide a public listConnectionStatus() method, it only returns a Map<string, boolean> of relay URL to connection state—not the relay object itself needed to access the WebSocket.

Apply the defensive optional-chaining guard at minimum to prevent runtime errors:

Defensive guard 
       // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
-      const relay = (this.pool as any).relays.get(relayUrl);
+      const relay = (this.pool as any).relays?.get(relayUrl);
+      // NOTE: Accessing internal SimplePool.relays map — will break on nostr-tools upgrades.
       if (relay?.ws && relay.ws.readyState !== WebSocket.CLOSED) {

Consider opening a feature request upstream with nostr-tools to expose a public API for accessing relay WebSocket handles or relay close events.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const relay = (this.pool as any).relays.get(relayUrl);
// eslint-disable-next-line `@typescript-eslint/no-explicit-any`
const relay = (this.pool as any).relays?.get(relayUrl);
// NOTE: Accessing internal SimplePool.relays map — will break on nostr-tools upgrades.
🤖 Prompt for AI Agents 
In `@src/nwc/NWCClient.ts` around lines 207 - 208, The code currently accesses the
private SimplePool internals via (this.pool as any).relays.get(relayUrl), which
is fragile; update the close() logic (the method that uses relayUrl and
this.pool) to avoid brittle casting and at minimum add defensive optional
chaining and null checks before using the relay object so it won't throw if
.relays is missing or renamed; specifically, replace the direct access with a
guarded lookup like checking (this.pool as any)?.relays?.get(relayUrl) and skip
awaiting closure if the relay handle is undefined, and log a warning;
additionally consider using listConnectionStatus() or filing an upstream feature
request for a public API to obtain WebSocket handles instead of reaching into
.relays.

if (relay?.ws && relay.ws.readyState !== WebSocket.CLOSED) {
closePromises.push(
new Promise<void>((resolve) => {
relay.ws?.addEventListener("close", () => resolve(), { once: true });
}),
);
}
}
this.pool.close(this.relayUrls);
await Promise.all(closePromises);
Comment on lines +202 to +218
Copy link

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing timeout on close promises — could hang indefinitely, reproducing the original bug in a different way.

If a remote peer never sends a WebSocket close frame (network issue, unresponsive server), the promises added at Line 211-213 will never resolve, and await Promise.all(closePromises) at Line 218 will hang forever. This ironically recreates the "Jest hangs" scenario this PR aims to fix, just under different failure conditions.

Add a per-socket timeout that resolves even if the close event never fires:

Proposed fix with timeout 
       if (relay?.ws && relay.ws.readyState !== WebSocket.CLOSED) {
         closePromises.push(
           new Promise<void>((resolve) => {
-            relay.ws?.addEventListener("close", () => resolve(), { once: true });
+            const onClose = () => resolve();
+            relay.ws?.addEventListener("close", onClose, { once: true });
+            setTimeout(() => {
+              relay.ws?.removeEventListener("close", onClose);
+              resolve();
+            }, 5000);
           }),
         );
       }
🤖 Prompt for AI Agents 
In `@src/nwc/NWCClient.ts` around lines 202 - 218, The close() method can hang
because the promises waiting for relay.ws "close" events never resolve if a peer
never sends a close; modify NWCClient.close to wrap each relay close wait in a
race between the close event and a per-socket timeout (e.g., 3–10s) so the
promise always resolves; implement this for the promises pushed into
closePromises (refer to relayUrls, relay, relay.ws, closePromises, and
pool.close) and ensure proper cleanup by clearing the timeout and
removing/ignoring the event listener when the other winner resolves to avoid
leaks.

}

async encrypt(pubkey: string, content: string) {
Expand Down
2 changes: 1 addition & 1 deletion src/nwc/NWCWalletService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
Relay,
} from "nostr-tools";
import { hexToBytes } from "@noble/hashes/utils";
import { Subscription } from "nostr-tools/lib/types/abstract-relay";
import { Subscription } from "nostr-tools/abstract-relay";

import {
Nip47MakeInvoiceRequest,
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"compilerOptions": {
"module": "ESNext",
"moduleResolution": "node",
"moduleResolution": "bundler",
"lib": ["ES2021", "DOM"],
"target": "es2020",
"strict": true,
Expand Down