fix(payments): cancel in-flight payment verification on transport close - #96
Merged
ContextVM-org merged 1 commit intoSep 17, 2026
Conversation
NostrServerTransport.close() had no way to reach the payment middlewares, so a verifyPayment poll started before shutdown kept running for up to verifyTimeoutMs and, if it settled, still forwarded the paid tool call, published payment_accepted, or granted explicit-gating authorization into a store nobody would read. - NostrServerTransport exposes `closeSignal: AbortSignal`, aborted at the top of close() before any teardown. Transport-owned, so it cannot be disarmed by consumers reassigning `onclose`. - withServerPayments passes it to both middlewares as an optional `abortSignal` factory param (not via ctx). - verifyPaymentUnlessShutdown bridges the shared signal to each request's own per-verify controller (addEventListener on start, removeEventListener in finally) and checks `aborted` after the verify settles, so a late success is not acted on either. - Transparent path: clean return, no payment_accepted, no forward; the pending entry is left to expire on its TTL (cancel-not-drain, CEP-8 redelivery dedup untouched). Explicit path: no grant; pending cleared as for any failed verify. - LNbits processor polls with sleepWithAbort so it wakes on abort. No change in behavior when close() is never called. Closes ContextVM#81
Contributor
|
Approved, nice work |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #81
Follows the review notes on the issue point by point (thanks for the guidance, it was more specific than the original proposal and I went with it wherever the two differed).
What was wrong
NostrServerTransport.close()had no way to reach the payment middlewares. AverifyPaymentpoll that started before shutdown kept running for up toverifyTimeoutMs, and if it settled it still forwarded the paid tool call, publishedpayment_accepted, or granted explicit-gating authorization into a store nobody would read.The fix
1. Transport-owned close notification.
NostrServerTransportgets acloseSignal: AbortSignalgetter, backed by a privateAbortControllerthat is aborted as the first statement ofclose(), beforetaskQueue.shutdown(). Nothing is chained throughonclose, so a consumer reassigning it afterwithServerPayments()cannot disarm this.2. Factory-level
abortSignal. BothcreateServerPaymentsMiddlewareandcreateExplicitGatingMiddlewaretake an optionalabortSignalat construction (not viactx; both factories are still usable standalone without a transport).withServerPaymentsreadstransport.closeSignaland passes it to both.The shared signal is bridged to each request's own per-verify controller in one helper,
verifyPaymentUnlessShutdown(server-payments-utils.ts):addEventListener('abort', ..., { once: true })on the shared signal when the verify starts,removeEventListenerinfinallyonce it settles, so a long-lived transport does not accumulate a listener per payment. NoAbortSignal.any, keeping Node 18 support.3.
abortedis checked after the verify settles, not only while polling. That check gatespayment_accepted,forward(), and the explicit-gating grant, so a verify that happens to succeed during the shutdown race is not acted on either.return. No rethrow, noonerrornoise at shutdown.Cancel-not-drain, on purpose. Side effects stop; payment-dedup state is not touched. In the transparent middleware the
pendingentry is left to expire on its normal TTL, so a redelivery of the same request event still hits the CEP-8 double-charge guard (there is a test for exactly that). In the explicit middleware the pending entry is cleared, matching the "existing catch already cleans up" note. If you would rather that entry also be left in place, that is a two-line change and I am happy to flip it.No behavior change when
close()is never called: with a signal that never fires, the helper is equivalent to the previouswithTimeout(...).finally(abort). Invoice issuance, dedup, capacity, and double-charge protection are untouched.4. Optional, flagged separately: the LNbits processor now polls with
sleepWithAbortinstead ofsleep, so it wakes on abort instead of finishing its current poll interval (same helper NWC already uses). Four lines; easy to drop if you would rather keep this PR to the middlewares.Tests
All deterministic: the fake processor exposes a manually resolvable
verifyPayment, so the test decides when it settles relative toclose(). No timer-based fakes, no sockets.server-payments.test.ts(+4): late settle after abort sends nopayment_acceptedand does not forward; abort cuts the verify short and the run settles in well under the timeout with no rethrow, and the pending entry still dedups a redelivery; the listener on the shared signal is added once and removed once; a signal that never fires leaves the paid flow unchanged.server-explicit-gating.test.ts(+3): late settle after abort does not grant and a retry mints a fresh -32042; abort cuts the verify short with no grant and no leaked rejection; a signal that never fires leaves the grant flow unchanged.server-transport-payments.shutdown.test.ts(new): integration over the in-memoryMockRelayHubwith a realNostrServerTransport,withServerPayments, and an MCP client. Callstransport.close()mid-verify and assertscloseSignaland the per-request signal both abort, no tool execution, nopayment_acceptedevent on the relay,grantnever called, and that a processor honoring the signal settles within 1s rather than minutes.process.on('unhandledRejection')in all three and asserted empty.Existing tests in
server-payments.test.ts,server-explicit-gating.test.ts,server-transport-payments.test.ts, andnostr-server-transport*.test.tspass with no changes to their assertions (the two unit files only have newdescribeblocks appended).Changeset added as a patch.
Verification
Run locally on Bun 1.4.2 (Linux):
bun lint,bun typecheck: cleanbun run build,bun run verify-exports,bunx publint: cleanbun test src: 562 pass, 5 skip, 0 failNODE_ENV=test bun test --concurrent --timeout=60000(the CI invocation): my tests pass under concurrency as well