Bound Reservoir NFT reads with a deadline covering headers and body - #97
Conversation
portdeveloper
left a comment
portdeveloper
left a comment
There was a problem hiding this comment.
The production deadline covers headers and the body correctly. The cancellation observer in test/nft-deadline.test.mjs:86-91 repeats the race tracked in #95: it only subscribes to close, so a socket that closed before the check is reported as uncancelled. I ran this exact helper against a real socket after its close event and got false.
Please handle the already-closed state and add a regression for it, as #96 does for history. Also assert the stalled request reached the fixture before indexing stalledSockets[0], and give connection setup a reasonable test budget; the current 120 ms can expire before a loaded runner opens the socket. Keep the independent outer timeout and real cancellation assertions, and rerun both OS jobs.
Closes portdeveloper#94. fetchReservoir called fetch and res.json() with no deadline, so a stalled indexer kept get_nfts waiting indefinitely. The deadline portdeveloper#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 portdeveloper#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.
Review on portdeveloper#97: the cancellation observer repeated the race portdeveloper#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 portdeveloper#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.
3c0e052 to
6e7e869
Compare
|
Fixed, and thank you — the observer did repeat the #95 race. I reproduced it here before changing anything: destroying a socket, waiting for its close, then calling the old helper returned Three changes, mirroring #96 rather than inventing a second shape:
The independent outer timeout and the real cancellation assertions are unchanged. Full suite: 350 pass, 0 fail; both OS jobs are rerunning on this push. |
2 participants
Closes #94.
fetchReservoircalledfetchandres.json()with no deadline, so a stalled indexer keptget_nftswaiting indefinitely. The deadline #93 added for the explorer reads covers/history, not this path.What changed
One
AbortControllernow 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 infinally, so a normal answer leaves nothing pending on the event loop. An expiry is reported as an NFT-read error naming the path and the deadline, rather than as a bareAbortErrorabout a signal. A non-finite deadline falls back to the default, sincesetTimeouttreatsNaNas fire-now and that would fail every read rather than none.The no-indexer and no-key refusals, the malformed-row handling and the truncation notice from #68 are untouched. The change stays inside the NFT indexer read path;
tools.mjscallsgetNfts(owner)exactly as before.Tests
Four cases in
test/nft-deadline.test.mjs, against a loopback server rather than a fetch double, mirroring how the history deadline is tested: a double that ignoressignallooks exactly like one that honours it, so only a real connection can show the request was cancelled rather than merely abandoned.NaN): falls back to the default instead of failing instantlyVerified all four fail on
mainbefore the fix and pass after.What I ran
npm test— 348 pass, 0 fail.npm run build— clean.npm run smokestops earlier than this path, at the ERC-20 balance step, on a public RPC returningrequests limited to 15/sec; that call is untouched by this diff, so I could not get aSMOKE_OKfrom a shared endpoint today. No indexer credentials and no writes are needed for any of the new tests.