fix(payments): transparent lifecycle durability — no double charges, no lost paid results - #80
Open
ContextVM-org wants to merge 9 commits into
Open
fix(payments): transparent lifecycle durability — no double charges, no lost paid results#80ContextVM-org wants to merge 9 commits into
ContextVM-org wants to merge 9 commits into
Conversation
…no lost paid results Bundles the three CEP-8 transparent-payment fixes validated against master (see rs-sdk #111 for the reference implementation of the same divergences): - Retention split on invoice issuance: post-invoice failures keep the pending entry until TTL, so a spec-blessed redelivery of the same request event cannot mint (and charge) a second invoice. Pre-invoice failures still delete the entry so retries are free. - A failed payment_accepted publish no longer aborts the forward; the notification is a SHOULD, the capability result is the point. - Pending capacity fails closed: purge expired, then refuse new priced requests (best-effort payment_rejected, no invoice minted) instead of silently evicting a live payment's dedup entry. 0 refuses all. - Route survival: the correlation route (+ session) is snapshotted when an invoice is issued; the response router falls back to it on route miss, so paid results survive duplicate-delivery cleanup popping the route and LRU eviction of the idle paying client's session. The dead "recreate session" eviction guard is removed (a synchronous veto would corrupt the session LRU's capacity accounting; snapshots make it unnecessary).
bun 1.4.0 (released Aug 20) regressed WebSocket upgrades made after a macrotask (oven-sh/bun#39766): server.upgrade() fails on the reconnect path, so the four relay-reconnection tests time out. Verified on the PR's base commit 476237b — same code, bun 1.3.14: 20/20 pass; bun 1.4.0: the exact 4 CI failures. The last green master run (Aug 19) predates the release. Revert to `latest` once a bun release carries oven-sh/bun#39642.
This reverts commit 1fbba47.
…Event semantics) bun 1.4.0 corrected CloseEvent reporting (oven-sh/bun#31518): a frame-based server close now reads as wasClean=true, and a received 1001 is no longer remapped. The mock relay's pause()/stop() relied on the old behavior — close(1011)/close(1001) were observed as unclean, which is what applesauce-relay keys reconnects on. On 1.4.0 those closes read as clean, so pools never reconnected after a simulated outage and the four relay reconnection tests timed out. Use ServerWebSocket.terminate() instead: a hard reset with no close frame (1006, wasClean=false) on every bun version — and the faithful simulation of a dead relay anyway. Verified on both runtimes: the four affected tests and the full relay/transport/payments suites (483 tests) pass on bun 1.3.14 and 1.4.0.
…on-positive TTLs Bundles the two remaining verified findings from the rs-sdk #111/#112 review round: Writer leak on the gating drop path. Every inbound tools/call with a progressToken reserves an OpenStreamWriter before middleware runs. A request dropped by middleware (explicit gating -32042/-32043/-32000, transparent payment_rejected, capacity refusal, or any custom gating) never produces the normal-path response that is the only reaper for never-started writers, so the reservation (writer + closures + abort controller + metadata) leaked until transport teardown — one per gated streaming invocation on exactly the path a busy paid API hammers. A throwing middleware chain leaked the correlation route too. Fix: cleanupDroppedRequest is now the single owner of per-request state and releases the writer alongside the route; the chain-error path runs the same cleanup; the release logic is extracted as ServerOpenStreamFactory.releaseUnusedWriter and reused by deferIfStreamActive's never-started branch. Non-positive TTLs. paymentTtlMs: 0 (both lifecycles) and invoice ttl: 0 (explicit gating grant) birth-expired the pending/grant entry while the invoice stayed payable, disarming the redelivery dedup (CEP-8). Both now fall back to the default window, mirroring the guard getVerificationTimeoutMs already applied. Also documents the run-to-completion atomicity assumption of claim()+trySetPending() for porters (insight from the rs port).
…ures, harden verify cleanup Three verified items from the consolidated rs-sdk CEP-8 analysis: Double registration double-charges. withServerPayments never failed and addInboundMiddleware pushes, so a second call on the same transport registered a second middleware pair with its own dedup closures — every priced request minted two invoices and double-charged. The registration entry point now refuses with an error (WeakSet keyed by transport). Client handler-failure hangs the request. In the transparent client lifecycle, paymentPolicy and canHandle declines synthesize a -32000 decline that resolves the pending MCP request, but a handler.handle() throw only rethrew to onerror and synthesized nothing — the request hung until the server TTL. Handler crashes now synthesize the same decline error (still surfacing on onerror for observability). Crashed verify leaves pending until TTL. The detached explicit-gating verification task's outer catch only logged; a store failure inside the inner catch could pin the identity as pending for the whole TTL, answering every retry with -32043. The outer catch now clears the pending identity (idempotent on the normal path).
…ent id The inbound coordinator rewrites request ids to the Nostr event id so responses from the single shared McpServer are routable when clients' JSON-RPC id spaces collide. The normal response exit path restores the client's original id from the correlation route before publishing; the targeted-response exit path used by -32042/-32043 gating errors skipped that restore, leaking the internal routing key into the JSON-RPC id on the wire. routeTargeted now applies the same restore. Non-breaking for SDK users (the client transport restores ids independently and ignores the wire value); aligns the wire with JSON-RPC 2.0 and CEP-8's examples, and with rs-sdk servers. Falls back to today's behavior when no route is recorded (redelivery edge).
…and disambiguate retry state Four client-side defects reported by the rs-sdk maintainers, verified against this branch (double-wrap and retry_after:0 event-id collision confirmed with runtime repros): 1. withClientPayments had no re-wrap guard. A chained double wrap ran two payment pipelines with separate dedup sets, so one payment_required paid twice (the outer wrap also lost the event-id context: requestEventId 'unknown', no pending lookup, no decline synthesis). A sibling wrap silently replaced the first wrapper's trampolines. Both configurations now fail fast via a module-level WeakSet, mirroring the server-side registration guard. 2. A -32043 with retry_after: 0 re-sent the cached request on the next tick. When the request was not the session's first send (tags already latched), the retry was byte-identical to the original event - same content, tags, and second-resolution created_at - so relays and servers swallowed it as a duplicate and the pending cycle stalled until TTL. Retries are now floored at minRetryDelayMs (default 1s). 3. retryCounts was an unbounded closure Map; abandoned requests leaked their counters forever. Now an LruCache(1000), bounded together with the raw-request cache. 4. rawRequestCache keyed by String(id), so numeric 5 and string "5" shared one entry and a -32043 answered to one retried the other's payload. Both maps now key by JSON.stringify(id).
The onmessageWithContext context shape was declared three times (NostrClientTransport inline, client-payments.ts, its test), and the duplicates already drifted once — the double-wrap defect hinged on exactly this property. Export TransportWithContext and MessageContext from nostr-client-transport.ts and consume them everywhere. Type-only change; no runtime diff.
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
1 participant
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.
Bundles the three CEP-8 transparent-payment fixes validated against master (see rs-sdk #111 for the reference implementation of the same divergences):