fix(nwc): bound every CLI daemon request with a two-tier deadline

Graft the cleaner daemon-client structure onto the review follow-up:

- Replace the manual AbortController/setTimeout with AbortSignal.timeout;
  the signal stays armed while the body is read, so stalled bodies are
  bounded on all routes, not just /nwc/*.
- 120s default deadline for every route; 600s for value-moving wallet
  routes (/wallet/send/*, /wallet/receive/*) whose mint operations can
  legitimately run longer. A wedged daemon can no longer hang the CLI
  forever on any route. The 'payment outcome is unknown' timeout warning
  now applies on both tiers.
- Rename rebuild log reasons to 'stall': wallet-error replies no longer
  rebuild the connection, so 'timeout' was inaccurate.
- Derive the NWC test client type from WalletAdapterOptions so the tests
  keep compiling when the legacy CocodClient is replaced (#118).
- Rewrite the daemon-client deadline tests around AbortSignal.timeout,
  asserting the requested tier per route and covering stalled bodies on
  long-running routes. Update docs/nwc-timeouts.md to match.
This commit is contained in:
redshift
2026-10-02 16:47:09 +08:00
parent 01fe864257
commit ad49fbe64c
5 changed files with 100 additions and 63 deletions
+1 -1
View File
@@ -6,7 +6,7 @@ The wallet adapter adds an overall deadline: 15 seconds per read attempt and 45
Normal NIP-47 wallet errors do not rebuild the shared relay connection: a wallet error proves a response arrived, and rebuilding could interrupt unrelated payments. Transport failures and timeouts, including the library's own timeout, still trigger recovery. Normal NIP-47 wallet errors do not rebuild the shared relay connection: a wallet error proves a response arrived, and rebuilding could interrupt unrelated payments. Transport failures and timeouts, including the library's own timeout, still trigger recovery.
CLI `/nwc/*` requests have a 120-second deadline covering headers and response-body consumption. Other daemon routes are not subject to this cap because mint payment operations may run longer and cannot be cancelled by aborting the CLI request. Every CLI daemon request has a deadline covering headers and response-body consumption: 120 seconds by default, and 600 seconds for value-moving wallet routes (`/wallet/send/*`, `/wallet/receive/*`), whose mint operations can legitimately run longer. Aborting the CLI request never cancels the daemon-side operation; the longer bound only delays how soon the CLI reports the stall.
A payment timeout is an **unknown outcome**, not proof that no payment occurred. Promise deadlines do not cancel the underlying operation. Check the mint quote, wallet transactions, and Cashu balance before creating and paying another invoice. A payment timeout is an **unknown outcome**, not proof that no payment occurred. Promise deadlines do not cancel the underlying operation. Check the mint quote, wallet transactions, and Cashu balance before creating and paying another invoice.
+9 -3
View File
@@ -1,6 +1,6 @@
import { afterAll, beforeEach, describe, expect, it, mock } from "bun:test"; import { afterAll, beforeEach, describe, expect, it, mock } from "bun:test";
import { RestrictedError } from "applesauce-wallet-connect/helpers/error"; import { RestrictedError } from "applesauce-wallet-connect/helpers/error";
import type { CocodClient } from "./cocod-client"; import type { WalletAdapterOptions } from "./index";
/** /**
* Regression tests for the NWC hang fixed in this change. * Regression tests for the NWC hang fixed in this change.
@@ -107,13 +107,19 @@ mock.module("applesauce-relay", () => ({ RelayPool: MockRelayPool }));
const { createWalletAdapter } = await import("./index"); const { createWalletAdapter } = await import("./index");
function makeClient(): CocodClient { /**
* Type of the injected wallet client, derived from the adapter options so this
* test keeps compiling when the legacy `CocodClient` is replaced (see #118).
*/
type WalletClientOption = NonNullable<WalletAdapterOptions["walletClient"]>;
function makeClient(): WalletClientOption {
return { return {
getBalances: async () => ({ "https://mint.example": 0 }), getBalances: async () => ({ "https://mint.example": 0 }),
getDefaultMint: async () => "https://mint.example", getDefaultMint: async () => "https://mint.example",
receiveBolt11: async () => ({ invoice: "lnbc-test-invoice" }), receiveBolt11: async () => ({ invoice: "lnbc-test-invoice" }),
receiveCashu: async () => "ok", receiveCashu: async () => "ok",
} as unknown as CocodClient; } as unknown as WalletClientOption;
} }
function makeAdapter(timeoutMs = 25) { function makeAdapter(timeoutMs = 25) {
+2 -2
View File
@@ -182,7 +182,7 @@ export async function createWalletAdapter(
logger.warn( logger.warn(
`[nwc] ${label} failed (${(error as Error).message}); rebuilding NWC connection and retrying`, `[nwc] ${label} failed (${(error as Error).message}); rebuilding NWC connection and retrying`,
); );
rebuildNwcConnection("reconnected after timeout"); rebuildNwcConnection("reconnected after stall");
const retry = wallet; const retry = wallet;
if (!retry?.service) throw error; if (!retry?.service) throw error;
return await withTimeout( return await withTimeout(
@@ -211,7 +211,7 @@ export async function createWalletAdapter(
} catch (error) { } catch (error) {
// Include the library's own timeout, but not normal wallet error replies. // Include the library's own timeout, but not normal wallet error replies.
if (!(error instanceof WalletBaseError)) { if (!(error instanceof WalletBaseError)) {
rebuildNwcConnection("reconnected after payment timeout"); rebuildNwcConnection("reconnected after payment stall");
} }
throw error; throw error;
} }
+39 -27
View File
@@ -1,21 +1,32 @@
import { afterEach, describe, expect, test } from "bun:test"; import { afterEach, describe, expect, test } from "bun:test";
import { callDaemonUrl, DAEMON_REQUEST_TIMEOUT_MS } from "./daemon-client"; import {
callDaemonUrl,
DAEMON_LONG_REQUEST_TIMEOUT_MS,
DAEMON_REQUEST_TIMEOUT_MS,
} from "./daemon-client";
import { DEFAULT_CONFIG } from "./config"; import { DEFAULT_CONFIG } from "./config";
const originalFetch = globalThis.fetch; const originalFetch = globalThis.fetch;
const originalSetTimeout = globalThis.setTimeout; const originalAbortTimeout = AbortSignal.timeout;
const originalClearTimeout = globalThis.clearTimeout;
/** Deadline (ms) passed to AbortSignal.timeout by the last request. */
let requestedDeadlines: number[] = [];
afterEach(() => { afterEach(() => {
globalThis.fetch = originalFetch; globalThis.fetch = originalFetch;
globalThis.setTimeout = originalSetTimeout; AbortSignal.timeout = originalAbortTimeout;
globalThis.clearTimeout = originalClearTimeout; requestedDeadlines = [];
}); });
/**
* Record the requested deadline and arm a fast one instead, so tests do not
* wait out the real 120s/600s bounds.
*/
function shortenDeadline(): void { function shortenDeadline(): void {
globalThis.setTimeout = ((callback: (...args: unknown[]) => void, delay?: number, ...args: unknown[]) => AbortSignal.timeout = ((ms: number) => {
originalSetTimeout(callback, delay === DAEMON_REQUEST_TIMEOUT_MS ? 20 : delay, ...args) requestedDeadlines.push(ms);
) as typeof setTimeout; return originalAbortTimeout(20);
}) as typeof AbortSignal.timeout;
} }
function stalledResponse(status = 200): void { function stalledResponse(status = 200): void {
@@ -33,7 +44,7 @@ function stalledResponse(status = 200): void {
const request = (path = "/nwc/status") => const request = (path = "/nwc/status") =>
callDaemonUrl("http://daemon.example", path, {}, DEFAULT_CONFIG); callDaemonUrl("http://daemon.example", path, {}, DEFAULT_CONFIG);
describe("NWC daemon request deadline", () => { describe("daemon request deadline", () => {
test("bounds the wait for response headers", async () => { test("bounds the wait for response headers", async () => {
shortenDeadline(); shortenDeadline();
globalThis.fetch = ((_input: string | URL | Request, init?: RequestInit) => new Promise((_resolve, reject) => { globalThis.fetch = ((_input: string | URL | Request, init?: RequestInit) => new Promise((_resolve, reject) => {
@@ -50,26 +61,27 @@ describe("NWC daemon request deadline", () => {
}); });
} }
test("does not impose the NWC deadline on mint payment routes", async () => { test("applies the default deadline to ordinary routes", async () => {
let deadlines = 0; shortenDeadline();
globalThis.setTimeout = ((callback: (...args: unknown[]) => void, delay?: number, ...args: unknown[]) => { globalThis.fetch = (async () => Response.json({ output: "ok" })) as unknown as typeof fetch;
if (delay === DAEMON_REQUEST_TIMEOUT_MS) deadlines++; expect(await request("/nwc/status")).toEqual({ output: "ok" });
return originalSetTimeout(callback, delay, ...args); expect(requestedDeadlines).toEqual([DAEMON_REQUEST_TIMEOUT_MS]);
}) as typeof setTimeout;
globalThis.fetch = (async () => Response.json({ output: "paid" })) as unknown as typeof fetch;
expect(await request("/wallet/send/bolt11")).toEqual({ output: "paid" });
expect(deadlines).toBe(0);
}); });
test("clears the deadline after consuming a successful response", async () => { for (const path of ["/wallet/send/bolt11", "/wallet/receive/cashu"]) {
let cleared = 0; test(`uses the long deadline for value-moving route ${path}`, async () => {
globalThis.clearTimeout = ((timer) => { shortenDeadline();
cleared++; globalThis.fetch = (async () => Response.json({ output: "paid" })) as unknown as typeof fetch;
originalClearTimeout(timer as ReturnType<typeof setTimeout>); expect(await request(path)).toEqual({ output: "paid" });
}) as typeof clearTimeout; expect(requestedDeadlines).toEqual([DAEMON_LONG_REQUEST_TIMEOUT_MS]);
globalThis.fetch = (async () => Response.json({ output: "ok" })) as unknown as typeof fetch; });
expect(await request()).toEqual({ output: "ok" }); }
expect(cleared).toBe(1);
test("bounds a stalled body on a long-running route too", async () => {
shortenDeadline();
stalledResponse();
await expect(request("/wallet/send/bolt11")).rejects.toThrow("payment outcome is unknown");
expect(requestedDeadlines).toEqual([DAEMON_LONG_REQUEST_TIMEOUT_MS]);
}); });
test("preserves HTTP errors rather than labeling them connection failures", async () => { test("preserves HTTP errors rather than labeling them connection failures", async () => {
+38 -19
View File
@@ -57,12 +57,31 @@ class DaemonConnectionError extends Error {
} }
/** /**
* Upper bound for NWC requests, including response-body consumption. * Upper bound for a single daemon request, including response-body
* Other routes can perform long-running mint payments; aborting the client * consumption. The daemon bounds its own NWC operations, so this only guards
* does not cancel those operations, so do not impose this cap on them. * against a wedged server; without it a hung request would block the CLI
* forever.
*/ */
export const DAEMON_REQUEST_TIMEOUT_MS = 120_000; export const DAEMON_REQUEST_TIMEOUT_MS = 120_000;
/**
* Upper bound for value-moving wallet routes. A cashu melt/swap can
* legitimately run longer than {@link DAEMON_REQUEST_TIMEOUT_MS} (the mint has
* no request timeout in routstrd), so these get a more generous bound that
* still prevents an indefinite CLI hang.
*/
export const DAEMON_LONG_REQUEST_TIMEOUT_MS = 600_000;
/** Routes that may legitimately outlive the default request timeout. */
const LONG_RUNNING_ROUTES = ["/wallet/send/", "/wallet/receive/"];
function requestTimeoutMs(path: string): number {
const pathname = path.split("?")[0] ?? path;
return LONG_RUNNING_ROUTES.some((route) => pathname.startsWith(route))
? DAEMON_LONG_REQUEST_TIMEOUT_MS
: DAEMON_REQUEST_TIMEOUT_MS;
}
export function getDaemonBaseUrl(config: RoutstrdConfig): string { export function getDaemonBaseUrl(config: RoutstrdConfig): string {
if (config.daemonUrl) { if (config.daemonUrl) {
return config.daemonUrl.replace(/\/$/, ""); return config.daemonUrl.replace(/\/$/, "");
@@ -106,40 +125,40 @@ export async function callDaemonUrl(
if (authorization) headers.set("Authorization", authorization); if (authorization) headers.set("Authorization", authorization);
if (bodyString) headers.set("Content-Type", "application/json"); if (bodyString) headers.set("Content-Type", "application/json");
const controller = new AbortController(); const timeoutMs = requestTimeoutMs(path);
const timeoutId = path.startsWith("/nwc/") const timeoutError = () =>
? setTimeout(() => controller.abort(), DAEMON_REQUEST_TIMEOUT_MS) new Error(
: undefined; `Daemon request timed out after ${timeoutMs / 1000}s; ` +
try { "any payment outcome is unknown — check before retrying",
);
// The signal stays armed while the body is read, so a daemon that sends
// headers and then stalls the body cannot hang the CLI either. Aborting
// here never cancels the daemon's operation — see timeoutError's warning.
const signal = AbortSignal.timeout(timeoutMs);
let response: Response; let response: Response;
try { try {
response = await fetch(url, { response = await fetch(url, {
method, method,
headers, headers,
body: bodyString, body: bodyString,
signal: controller.signal, signal,
}); });
} catch (error) { } catch (error) {
if (controller.signal.aborted) throw error; if (signal.aborted) throw timeoutError();
// Only connection failures qualify for alternate-host retries. // Only connection failures qualify for alternate-host retries.
throw new DaemonConnectionError(error); throw new DaemonConnectionError(error);
} }
try {
if (!response.ok) { if (!response.ok) {
const errorData = (await response.json()) as { error?: string }; const errorData = (await response.json()) as { error?: string };
throw new Error(errorData.error || `HTTP ${response.status}`); throw new Error(errorData.error || `HTTP ${response.status}`);
} }
return await response.json() as CommandResponse; return (await response.json()) as CommandResponse;
} catch (error) { } catch (error) {
if (controller.signal.aborted) { if (signal.aborted) throw timeoutError();
throw new Error(
`Daemon request timed out after ${DAEMON_REQUEST_TIMEOUT_MS / 1000}s; ` +
"any payment outcome is unknown — check before retrying",
);
}
throw error; throw error;
} finally {
if (timeoutId !== undefined) clearTimeout(timeoutId);
} }
} }