fix: NWCClient.close() hanging Jest tests#530
fix: NWCClient.close() hanging Jest tests#530AkarshSahlot wants to merge 1 commit intogetAlby:masterfrom
Conversation
The close() method in NWCClient was synchronous, which meant it didn't wait for WebSocket connections to actually close before returning. This caused Jest tests to hang because of open handles. I made close() async so it waits for the underlying WebSocket capabilities to finish closing. I also updated the dependencies and config to support the latest nostr-tools exports. Changes: - Made NWCClient.close() async - Updated imports to use specific paths from nostr-tools - Set moduleResolution to bundler in tsconfig - Updated jest.config.ts to export default
📝 WalkthroughWalkthroughThis PR addresses a socket closure bug by migrating the project to ESM and refactoring NWCClient.close() to be async. The async method now captures WebSocket references before clearing the relay pool, then awaits their close events to ensure connections fully terminate before returning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@jest.config.ts`:
- Around line 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.
In `@src/nwc/NWCClient.ts`:
- Around line 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.
- Around line 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.
| export default { | ||
| preset: "ts-jest", | ||
| testEnvironment: "node", | ||
| transform: { | ||
| "^.+\\.tsx?$": ["ts-jest", { useESM: true }], | ||
| }, |
There was a problem hiding this comment.
🧩 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.tsRepository: getAlby/js-sdk
Length of output: 305
Add required ESM configuration for Jest.
The useESM: true option requires two additional configurations to work correctly:
- Add
extensionsToTreatAsEsm: ['.ts']to the jest config to tell Jest to treat.tsfiles as ESM. - Update the test script in
package.jsonto includeNODE_OPTIONS=--experimental-vm-modules jestor use--experimental-vm-modulesflag. 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.
|
|
||
| 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); | ||
| 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); |
There was a problem hiding this comment.
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.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const relay = (this.pool as any).relays.get(relayUrl); |
There was a problem hiding this comment.
🧩 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 provideWebSocket, e.g. Node withws) (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:
Closing / cleanup
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:
- 1: https://jsr.io/%40nostr/tools/doc/pool
- 2: https://jsr.io/%40nostr/tools
- 3: https://jsr.io/%40nostr/tools
- 4: https://jsr.io/%40nostr/tools
- 5: https://github.com/nbd-wtf/nostr-tools?utm_source=openai
- 6: https://jsr.io/%40nostr/tools/doc/pool
- 7: https://jsr.io/%40nostr/tools/doc/pool
- 8: https://jsr.io/%40nostr/tools/doc/pool
- 9: https://jsr.io/%40nostr/tools/doc/pool
- 10: https://jsr.io/%40nostr/tools/doc/pool
- 11: https://jsr.io/%40nostr/tools/doc/pool
- 12: https://jsr.io/%40nostr/tools/doc/pool
- 13: https://jsr.io/%40nostr/tools/doc/pool
- 14: https://jsr.io/%40nostr/tools/doc/pool
- 15: https://jsr.io/%40nostr/tools/doc/pool
- 16: https://jsr.io/%40nostr/tools/doc/pool
- 17: https://jsr.io/%40nostr/tools/doc/pool
- 18: https://jsr.io/%40nostr/tools/doc/pool
- 19: https://jsr.io/%40nostr/tools/doc/pool
- 20: https://jsr.io/%40nostr/tools/doc/pool
- 21: https://jsr.io/%40nostr/tools/doc/pool
- 22: https://jsr.io/%40nostr/tools/doc/pool
🌐 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), includingSimplePoolusage (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 callingpool.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
SimplePoolexamples 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 -20Repository: getAlby/js-sdk
Length of output: 1001
🏁 Script executed:
grep -n "this.relayUrls\|this.pool" src/nwc/NWCClient.ts | head -30Repository: getAlby/js-sdk
Length of output: 1097
🏁 Script executed:
grep -A 5 "listConnectionStatus" src/nwc/NWCClient.tsRepository: 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.
| // 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.
1 participant
Closes #525
The close() method in NWCClient was synchronous, which meant it didn't wait for WebSocket connections to actually close before returning. This caused Jest tests to hang because of open handles.
I made close() async so it waits for the underlying WebSocket capabilities to finish closing. I also updated the dependencies and config to support the latest nostr-tools exports.
Changes:
Summary by CodeRabbit
Refactor
Chores