Diagnose facilitator failures instead of quoting them, and give redis real CI coverage - #714
Merged
Merged
Conversation
Settlement failed 15 times in eleven minutes today across Base, Solana, Polygon and Arbitrum, and every log line said the same thing: Facilitator settle failed (502): <html> <head> <title>Coinbase</title> <meta name="robots" content="noindex"> <meta property="viewport" ... That is 200 characters of boilerplate and no diagnosis. @x402/core truncates an error body at 200 chars, and on an HTML page the budget is gone before the first word of the message. So "their origin fell over" and "an edge is refusing our egress IP" were indistinguishable - and they demand opposite responses: wait, or build a relay. That distinction is not hypothetical here. Four relays already exist (Yahoo, Nasdaq, Sei, Nodely) because third parties block Railway's egress IPs; Nodely 403s it outright. And api.cdp.coinbase.com sits behind Cloudflare, which is what serves an HTML page where an API would return JSON. I guessed upstream outage from those 200 characters and had no business doing so. Same defect payments.js already documents for network-level failures, one layer up: a cause we discard is not a cause we do not have. That fix read err.cause; this one reads the response before the vendor truncates it. The wrapper only touches non-2xx, non-JSON responses from registered facilitator hosts. It CLONES before reading - consuming the caller's body would break settlement outright - and swallows every internal failure, because a diagnostic that can break the thing it describes is worse than the blindness it cures. It logs once at boot so a silent failure to install is visible now rather than during the next incident. Two of my own bugs, both caught by the tests rather than by review: - The classifier folded header NAMES into the text it searched, so the literal string "cf-mitigated:" appeared on every response and a plain origin 502 was reported as Cloudflare blocking us - precisely the confusion this exists to end. Values only now. - Hosts lived inside the wrapper, so with facilitators registering one at a time only the first was ever watched: the wrapper would exist and cover nothing. They live outside it, and an assertion proves a host registered after installation is still watched. Verified live rather than assumed: booted the paid path and confirmed the install line appears with a real facilitator registered, so client.url extraction works and this is not dead code. 24 offline assertions in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The failure hooks log the CHAIN and never the client, and that is why today's diagnosis went wrong twice. Settle failures on Solana, Polygon and Arbitrum all came back in Coinbase's words - but the boot log routes those three to PayAI and only Base to CDP: Multi-chain facilitator routing: CDP (Base + Bazaar) → PayAI (remaining chains) Clients are tried in order, so the error that surfaces is the FIRST one tried, not the one that owns the chain. "PayAI rejected this" and "CDP was tried first and never got past the edge" are different failures with different fixes, and the log could not tell them apart. I read those lines and concluded a CDP outage, which the evidence never supported. The label is PREFIXED onto the message and attached as a property, never substituted. isPreBroadcastSettleRejection matches `settle failed (402)` as a substring and the summarizer scans the message for the facilitator's JSON body, so replacing it would have silently broken the fallback's safety classification - a logging change quietly costing revenue. There is an assertion for exactly that: a prefixed 402 must still classify as a safe-to-retry pre-broadcast rejection. First label wins, so an inner client that already named itself is not overwritten by an outer one, and double-wrapping cannot double-prefix. An unwritable error is rethrown unchanged - a label is a nicety and must never turn a facilitator error into a different failure. Verified by boot on the paid path, and the seven payment suites (supported-guard, stellar-confirm, svm-clarify, price-premium, mpp-shim, payer-header-order, payment-verify) all pass unchanged. 30 offline assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cted to one Dependabot's redis 4 -> 6 (#708) is two majors on the library behind src/cache.js and src/shared-limit.js, and its CI was green. That green was worth nothing: no workflow ran a redis, and test-shared-limit.js injects a fake store on purpose - it proves "two callers share one counter", and a fake proves that exactly - so the CLIENT path was never exercised. createClient options, connect, command shapes, quit: all untested. Same worthless green as the tesseract 5 -> 7 trap, which passed 210 assertions while silently emptying a paid tool's output. It matters because prod is NOT in-memory. Verified against Railway rather than against notes: REDIS_URL and RATE_LIMIT_REPLICAS are both set on the production service and the project runs a dedicated Redis. The shared limiter FAILS CLOSED, so a client regression refuses trials rather than degrading quietly. The memory note claiming "prod is in-memory, low risk, take anytime" was wrong in the reassuring direction and would have merged the bump. So: a redis service container on the test job, and an integration test that drives the real client - cap-of-1 across callers, the over-limit decrement, refund flooring, and a cache set/get round trip. It asserts degraded === false, because the fail-closed path would otherwise let every assertion pass for the wrong reason with no server at all. It REQUIRES a server and exits 1 without one. It does not skip: a skipped integration test is precisely why this went untested, and "no redis in CI" means the service container is gone, which is the regression this guards. Caught while writing it: peek() returns { limited, count, degraded }, not `.used` as I had assumed - read the source instead of guessing, since a wrong assertion here would have looked like a redis failure. NOT verified locally (no redis or docker on this machine), so CI is the first real run. Flagged rather than glossed. This makes #708 decidable instead of a coin flip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Their deploy was CANCELLED by the next push - deploy.yml uses cancel-in-progress, so any push kills the run in flight, and cancelled is not green. Production is on c0fbae2 (the diagnostics) without the labels. This ships them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Today's settle failures logged 200 characters of
<html><head><title>Coinbase</title>and nothing else, and I twice drew the wrong conclusion from them.Diagnostics.
@x402/coretruncates an error body at 200 chars; on an HTML page that budget is gone before the first word. The wrapper reads the response before the vendor truncates, strips markup, and classifies: Cloudflare challenge/block, access denied, rate limited, origin failure behind the edge, or gateway timeout — keepingcf-ray,serverandretry-after. It only touches non-2xx non-JSON responses from registered facilitator hosts, clones before reading (consuming the body would break settlement), swallows every internal error, and logs once at boot so a silent failure to install is visible immediately.Labels. The failure hooks logged the chain and never the client, so Solana/Polygon/Arbitrum failures read as Coinbase's words though the boot log routes those to PayAI and only Base to CDP — clients are tried in order, so the surfacing error is the first tried, not the chain's owner. The label is prefixed, never substituted:
isPreBroadcastSettleRejectionmatchessettle failed (402)as a substring, so replacing the message would silently break the fallback's safety classification. Pinned by an assertion.Redis CI. Nothing had ever connected to a redis:
test-shared-limit.jsinjects a fake store by design, so the client path was untested and #708 (redis 4→6, two majors) had a meaningless green. Prod is not in-memory —REDIS_URLandRATE_LIMIT_REPLICASare set on the production service. Now there's aredis:7-alpineservice container and an integration test driving the real client (cap-of-1, over-limit decrement, refund flooring, cache round trip), assertingdegraded === falseso it can't pass via the fail-closed path. It exits 1 rather than skipping without a server.Three of my own bugs were caught by these tests rather than by review: the classifier searched header names so every page read as a Cloudflare block; the watched-host set lived inside the wrapper so only the first facilitator was ever covered; and I asserted on a
peek().usedfield that does not exist.