From 31ba173cba0f7a9111010bd78478eb69219d264e Mon Sep 17 00:00:00 2001 From: ColinkaMir <72103708+ColinkaMir@users.noreply.github.com> Date: Tue, 15 Sep 2026 05:51:17 +0000 Subject: [PATCH 1/2] Bound Reservoir NFT reads with a deadline covering headers and body Closes #94. fetchReservoir called fetch and res.json() with no deadline, so a stalled indexer kept get_nfts waiting indefinitely. The deadline #93 added for the explorer reads does not cover this path. One AbortController now covers the request and the body read, because a response whose headers arrive and whose JSON then stalls hangs just as completely as one that never answers. The timer is cleared in finally so a normal answer leaves nothing pending, and an expiry is reported as an NFT-read error naming the path and the deadline rather than as a bare AbortError. A non-finite deadline falls back to the default: setTimeout treats NaN as fire-now, which would fail every read rather than none. The no-indexer and no-key refusals, the malformed-row handling and the #68 truncation notice are untouched. Tests run against a loopback server rather than a fetch double, the same way the history deadline is tested: a double that ignores signal looks exactly like one that honours it, so only a real connection shows the request was cancelled. Stalled headers, stalled body, an ordinary answer with no timer left behind, and a nonsense deadline. --- src/wallet.mjs | 43 +++++++++-- test/nft-deadline.test.mjs | 149 +++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 7 deletions(-) create mode 100644 test/nft-deadline.test.mjs diff --git a/src/wallet.mjs b/src/wallet.mjs index 35431fa..5837c5e 100644 --- a/src/wallet.mjs +++ b/src/wallet.mjs @@ -44,6 +44,11 @@ function getReadProvider() { return readProvider; } +// Same shape of deadline as the explorer reads in #93, and for the same reason: a stalled +// indexer otherwise keeps get_nfts waiting forever. Kept separate from EXPLORER_TIMEOUT_MS +// because these are different services, and a slow indexer should not shorten history reads. +const NFT_TIMEOUT_MS = 10_000; + /** * GET a path from the Reservoir indexer (see config.reservoirUrl). * @@ -52,7 +57,7 @@ function getReadProvider() { * (this is why the explorer is the source of truth for holdings). Reservoir is the only * new network dependency; nothing else about the wallet changes. */ -async function fetchReservoir(path) { +async function fetchReservoir(path, { fetchImpl = fetch, timeoutMs = NFT_TIMEOUT_MS } = {}) { // No indexer for this network → refuse. Checked before the key so a mainnet operator // isn't sent to fetch a key for a host that doesn't exist. Reservoir has no Monad // mainnet endpoint, and answering from the testnet one would report another chain's @@ -68,11 +73,35 @@ async function fetchReservoir(path) { if (!config.reservoirApiKey) { throw new Error("RESERVOIR_API_KEY is not set. Get a free key at https://reservoir.tools, then put it in .env"); } - const res = await fetch(`${config.reservoirUrl}${path}`, { headers: { "x-api-key": config.reservoirApiKey } }); - if (!res.ok) { - throw new Error(`Reservoir API error ${res.status}${res.statusText ? ` ${res.statusText}` : ""} for ${path}`); + // A non-finite or non-positive deadline is not a laxer deadline: setTimeout treats NaN as + // "fire now", which would fail every NFT read rather than none of them. + const requested = Number(timeoutMs); + const deadline = Number.isFinite(requested) && requested > 0 ? requested : NFT_TIMEOUT_MS; + // One controller covers the request AND the body read. A response whose headers arrive and + // whose JSON then stalls hangs just as completely as one that never answers, and aborting + // after the headers still tears the body stream down. Cleared in `finally` so a normal answer + // leaves no pending timer holding the event loop open. + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), deadline); + try { + const res = await fetchImpl(`${config.reservoirUrl}${path}`, { + headers: { "x-api-key": config.reservoirApiKey }, + signal: controller.signal, + }); + if (!res.ok) { + throw new Error(`Reservoir API error ${res.status}${res.statusText ? ` ${res.statusText}` : ""} for ${path}`); + } + return await res.json(); + } catch (err) { + // The abort surfaces as an AbortError whose message is about a signal, which tells a wallet + // user nothing. Say which read timed out and after how long; everything else passes through. + if (controller.signal.aborted) { + throw new Error(`NFT read timed out after ${deadline}ms: the indexer did not answer for ${path}`); + } + throw err; + } finally { + clearTimeout(timer); } - return res.json(); } function buildWalletConfig() { @@ -384,10 +413,10 @@ export function normalizeNftPage(data) { * Returns { tokens, skipped, truncated }; see normalizeNftPage for the shape and why the two * counters are part of it. */ -export async function getNfts(ownerAddress = address) { +export async function getNfts(ownerAddress = address, { fetchImpl = fetch, timeoutMs = NFT_TIMEOUT_MS } = {}) { if (!ownerAddress) throw new Error("Wallet not initialized"); const owner = checksumAddress(ownerAddress); - const data = await fetchReservoir(`/users/${owner}/tokens/v7?limit=${NFT_PAGE_LIMIT}`); + const data = await fetchReservoir(`/users/${owner}/tokens/v7?limit=${NFT_PAGE_LIMIT}`, { fetchImpl, timeoutMs }); // Known follow-up: page past the limit via `continuation` for wallets with more. Until then // the caller is at least told the list is partial. return normalizeNftPage(data); diff --git a/test/nft-deadline.test.mjs b/test/nft-deadline.test.mjs new file mode 100644 index 0000000..a253370 --- /dev/null +++ b/test/nft-deadline.test.mjs @@ -0,0 +1,149 @@ +/** + * get_nfts must return when the indexer stalls (Issue #94). + * + * The deadline added in #93 covers the explorer reads behind `/history`; the Reservoir path + * behind `get_nfts` was a separate call with no deadline at all, so a stalled indexer kept the + * agent waiting with nothing to show and no way to give up. + * + * These run against a real loopback server rather than a fetch double on purpose, the same way + * the history deadline is tested: a double that ignores `signal` looks exactly like one that + * honours it, so only a real connection can show that the request was actually cancelled and + * not merely abandoned by a resolved promise. + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import http from "node:http"; +import { once } from "node:events"; + +// config.mjs reads the environment once at import time, so the indexer host and key have to be +// in place BEFORE wallet.mjs loads — otherwise every case here fails on the missing-key refusal +// instead of exercising the deadline. The runner gives each test file its own process, so this +// affects nothing else. Values are placeholders: the fixture below re-points the host anyway. +process.env.RESERVOIR_API_URL ||= "https://indexer.invalid"; +process.env.RESERVOIR_API_KEY ||= "test-key"; +const { getNfts } = await import("../src/wallet.mjs"); + +const OWNER = "0x8ba1f109551bD432803012645Ac136ddd64DBA72"; + +const TOKEN_PAGE = { + tokens: [ + { + token: { + contract: "0x3333333333333333333333333333333333333333", + tokenId: "7", + name: "Test token", + collection: { name: "Test collection" }, + }, + }, + ], +}; + +/** + * `stalledSockets` holds the socket each stalled request arrived on; aborting the request + * destroys it, which is how the test tells cancellation apart from "we stopped waiting". + * `fetchImpl` only re-points the host, so production code still builds its own URL and the + * real fetch and abort paths run. + */ +async function indexerFixture({ mode = "ok" } = {}) { + const stalledSockets = []; + const server = http.createServer((req, res) => { + if (mode === "headers") { + stalledSockets.push(req.socket); // never answers at all + return; + } + if (mode === "body") { + stalledSockets.push(req.socket); + res.writeHead(200, { "content-type": "application/json", "transfer-encoding": "chunked" }); + res.write('{"tokens":'); // headers fine, body never finishes + return; + } + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify(TOKEN_PAGE)); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const base = `http://127.0.0.1:${server.address().port}`; + return { + stalledSockets, + fetchImpl: (url, opts) => fetch(base + new URL(url).pathname + new URL(url).search, opts), + close() { + server.closeAllConnections(); + server.close(); + }, + }; +} + +/** + * A cancelled request's socket closes a tick after the call rejects, so this waits for the + * close rather than sampling `destroyed` and racing it. An uncancelled request never closes, + * which is the failure this has to report rather than hang on. + */ +async function wasCancelled(socket) { + return Promise.race([ + once(socket, "close").then(() => true), + new Promise((resolve) => setTimeout(() => resolve(false), 1000).unref()), + ]); +} + +/** A deadline that never fires would hang the suite: `npm test` sets no per-test timeout. */ +async function expectTimeout(promise) { + const outcome = await Promise.race([ + promise.then(() => ({ resolved: true }), (error) => ({ error })), + new Promise((resolve) => setTimeout(() => resolve({ hung: true }), 3000).unref()), + ]); + assert.equal(outcome.hung, undefined, "get_nfts hung: the deadline never fired"); + assert.equal(outcome.resolved, undefined, "get_nfts resolved although the indexer never answered"); + return outcome.error; +} + +describe("get_nfts deadline", () => { + it("gives up when the indexer never sends headers, and cancels the request", async () => { + const fx = await indexerFixture({ mode: "headers" }); + try { + const error = await expectTimeout(getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: 120 })); + assert.match(error.message, /NFT read timed out after 120ms/); + assert.equal(await wasCancelled(fx.stalledSockets[0]), true, "the stalled request was not cancelled"); + } finally { + fx.close(); + } + }); + + it("gives up when the body stalls after the headers arrive", async () => { + // The case a header-only timeout would miss: the response starts, then never ends. + const fx = await indexerFixture({ mode: "body" }); + try { + const error = await expectTimeout(getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: 120 })); + assert.match(error.message, /NFT read timed out after 120ms/); + assert.equal(await wasCancelled(fx.stalledSockets[0]), true, "the stalled body was not cancelled"); + } finally { + fx.close(); + } + }); + + it("reads an ordinary answer and leaves no timer behind", async () => { + const fx = await indexerFixture({ mode: "ok" }); + try { + const page = await getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: 5000 }); + assert.equal(page.tokens.length, 1); + assert.equal(page.tokens[0].tokenId, "7"); + // A 5s timer left pending would hold the event loop open well past this test; the suite + // finishing at all is the assertion, and this makes the intent explicit. + const pending = process.getActiveResourcesInfo().filter((r) => r === "Timeout"); + assert.equal(pending.length, 0, "the deadline timer outlived a successful read"); + } finally { + fx.close(); + } + }); + + it("treats a nonsense deadline as the default rather than as no deadline", async () => { + // Number("") is 0 and setTimeout(NaN) fires immediately: either would fail every read. + const fx = await indexerFixture({ mode: "ok" }); + try { + const page = await getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: Number.NaN }); + assert.equal(page.tokens.length, 1); + } finally { + fx.close(); + } + }); +}); From 6e7e8695c4261fb050b6d85a9de53c52560eca50 Mon Sep 17 00:00:00 2001 From: ColinkaMir <72103708+ColinkaMir@users.noreply.github.com> Date: Thu, 17 Sep 2026 06:20:55 +0000 Subject: [PATCH 2/2] Take the NFT deadline tests off the clock too Review on #97: the cancellation observer repeated the race #95 tracks. It only subscribed to close, so a socket already destroyed when the check ran was reported as uncancelled, and only after the full wait. Same three fixes #96 made for history, applied here: - wasCancelled reads the settled state before it waits, so an already-closed socket answers true on the microtask queue. - A regression covers exactly that: destroy a socket, wait for its close, then assert the observer answers true and answers without waiting. Removing the settled-state check turns this test red. - The stall deadline moves from 120ms to 500ms and the outer bound to an absolute 4000ms. 120ms could expire while a loaded runner was still opening the connection, which leaves no socket to assert on; the independent outer timeout still turns a missing production deadline into a prompt failure. - Both stall cases assert the request reached the fixture before indexing the socket, so an unreached fixture reads as itself instead of as a TypeError. --- test/nft-deadline.test.mjs | 61 +++++++++++++++++++++++++++++++++----- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/test/nft-deadline.test.mjs b/test/nft-deadline.test.mjs index a253370..37047e0 100644 --- a/test/nft-deadline.test.mjs +++ b/test/nft-deadline.test.mjs @@ -76,21 +76,35 @@ async function indexerFixture({ mode = "ok" } = {}) { /** * A cancelled request's socket closes a tick after the call rejects, so this waits for the - * close rather than sampling `destroyed` and racing it. An uncancelled request never closes, - * which is the failure this has to report rather than hang on. + * close rather than sampling and racing it. The close may equally have happened already, and + * subscribing to an event that is past reports a cancelled request as uncancelled after burning + * the whole timeout — so the settled state is checked first. An uncancelled request never + * closes, which is the failure this has to report rather than hang on. */ async function wasCancelled(socket) { + if (socket.destroyed) return true; return Promise.race([ once(socket, "close").then(() => true), new Promise((resolve) => setTimeout(() => resolve(false), 1000).unref()), ]); } +/** + * The deadline handed to `getNfts` in the stall cases. A stalled request still has to reach the + * fixture before it expires, or there is no socket left to assert on, and 120 ms could expire + * while a loaded runner was still opening the connection. 500 ms matches the budget #96 settled + * on for the history stalls and stays well under the absolute bound below, which is what turns a + * missing production deadline into a prompt failure rather than a hang. + */ +const STALL_DEADLINE_MS = 500; +/** Absolute, so it does not move with the deadline it is meant to catch the absence of. */ +const SETTLE_BOUND_MS = 4000; + /** A deadline that never fires would hang the suite: `npm test` sets no per-test timeout. */ async function expectTimeout(promise) { const outcome = await Promise.race([ promise.then(() => ({ resolved: true }), (error) => ({ error })), - new Promise((resolve) => setTimeout(() => resolve({ hung: true }), 3000).unref()), + new Promise((resolve) => setTimeout(() => resolve({ hung: true }), SETTLE_BOUND_MS).unref()), ]); assert.equal(outcome.hung, undefined, "get_nfts hung: the deadline never fired"); assert.equal(outcome.resolved, undefined, "get_nfts resolved although the indexer never answered"); @@ -101,8 +115,11 @@ describe("get_nfts deadline", () => { it("gives up when the indexer never sends headers, and cancels the request", async () => { const fx = await indexerFixture({ mode: "headers" }); try { - const error = await expectTimeout(getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: 120 })); - assert.match(error.message, /NFT read timed out after 120ms/); + const error = await expectTimeout(getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: STALL_DEADLINE_MS })); + assert.match(error.message, /NFT read timed out after 500ms/); + // Checked before indexing: without it an unreached fixture fails as "Cannot read + // properties of undefined", which points away from the cause. + assert.equal(fx.stalledSockets.length, 1, "the stalled request must have reached the fixture"); assert.equal(await wasCancelled(fx.stalledSockets[0]), true, "the stalled request was not cancelled"); } finally { fx.close(); @@ -113,8 +130,9 @@ describe("get_nfts deadline", () => { // The case a header-only timeout would miss: the response starts, then never ends. const fx = await indexerFixture({ mode: "body" }); try { - const error = await expectTimeout(getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: 120 })); - assert.match(error.message, /NFT read timed out after 120ms/); + const error = await expectTimeout(getNfts(OWNER, { fetchImpl: fx.fetchImpl, timeoutMs: STALL_DEADLINE_MS })); + assert.match(error.message, /NFT read timed out after 500ms/); + assert.equal(fx.stalledSockets.length, 1, "the stalled request must have reached the fixture"); assert.equal(await wasCancelled(fx.stalledSockets[0]), true, "the stalled body was not cancelled"); } finally { fx.close(); @@ -136,6 +154,35 @@ describe("get_nfts deadline", () => { } }); + it("reports a request cancelled before the check as cancelled, without waiting", async () => { + // The race #95 tracks and #96 fixed for history, in this file: on a fast machine the socket + // is already gone when the assertion runs, and subscribing to a close that has passed + // reported a cancelled request as uncancelled — after burning the full wait. + const server = http.createServer((req, res) => res.end("{}")); + let captured = null; + server.on("connection", (socket) => { captured ??= socket; }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + try { + await fetch(`http://127.0.0.1:${server.address().port}/`).then((res) => res.json()); + captured.destroy(); + await once(captured, "close"); + + // "Without waiting" as the event loop defines it rather than as a stopwatch does. An + // answer that is already known settles on the microtask queue and beats a macrotask every + // time, while one that subscribes to a close already past loses to it every time. No wall + // clock here to be flaky about on a loaded runner. + const answered = await Promise.race([ + wasCancelled(captured).then((verdict) => `answered:${verdict}`), + new Promise((resolve) => setImmediate(() => resolve("waited"))), + ]); + assert.equal(answered, "answered:true", "a closed socket is a cancelled request, answered without waiting"); + } finally { + server.closeAllConnections(); + server.close(); + } + }); + it("treats a nonsense deadline as the default rather than as no deadline", async () => { // Number("") is 0 and setTimeout(NaN) fires immediately: either would fail every read. const fx = await indexerFixture({ mode: "ok" });