payment in services.js calls the Stellar facilitator synchronously in the HTTP request path — a slow or unresponsive facilitator stalls every service request and blocks Express worker threads indefinitely#256
Conversation
Updated File
📝 WalkthroughWalkthroughAdds a Facilitator Resilience
Sequence Diagram(s)sequenceDiagram
participant Client
participant WrappedFacilitatorClient
participant CircuitBreaker
participant HTTPFacilitatorClient
participant FacilitatorService
Client->>WrappedFacilitatorClient: verifyTransaction(payload)
WrappedFacilitatorClient->>CircuitBreaker: isCircuitOpen()?
alt circuit OPEN
CircuitBreaker-->>WrappedFacilitatorClient: true
WrappedFacilitatorClient-->>Client: throw 503 FACILITATOR_UNAVAILABLE
else circuit CLOSED or HALF_OPEN
CircuitBreaker-->>WrappedFacilitatorClient: false
WrappedFacilitatorClient->>HTTPFacilitatorClient: Promise.race([verifyTransaction, rejectAfter(timeoutMs)])
HTTPFacilitatorClient->>FacilitatorService: HTTP verify request
FacilitatorService-->>HTTPFacilitatorClient: response / timeout
HTTPFacilitatorClient-->>WrappedFacilitatorClient: result or error
WrappedFacilitatorClient->>CircuitBreaker: recordFacilitatorResult(success|failure)
WrappedFacilitatorClient-->>Client: result or rethrown error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)backend/src/index.jsFile contains syntax errors that prevent linting: Line 89: Expected a statement but instead found '})'. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/.env.example (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
.env.examplekeys in dotenv-linter order.
dotenv-linterreportsFACILITATOR_TIMEOUT_MSshould appear beforeFACILITATOR_URL.Proposed reorder
-FACILITATOR_URL=http://localhost FACILITATOR_TIMEOUT_MS=10000 +FACILITATOR_URL=http://localhost🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/.env.example` around lines 6 - 7, The .env.example entries are out of dotenv-linter order, so reorder the FACILITATOR settings so FACILITATOR_TIMEOUT_MS appears before FACILITATOR_URL. Keep the existing values unchanged and adjust only the key order in the .env.example file.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/routes/services.js`:
- Around line 93-105: The half-open path in isCircuitOpen currently allows every
request through, so add a single in-flight probe guard for the circuit breaker
state in backend/src/routes/services.js. Update the logic around isCircuitOpen
and the request handling at the facilitator route so only the first half-open
request is allowed to probe while all concurrent non-probe requests immediately
return FACILITATOR_UNAVAILABLE with 503. Use the existing
circuitState/CIRCUIT_STATE.HALF_OPEN flow and related route handler logic to
track and release the probe.
- Around line 178-181: The facilitator failure catch in the `verifyTransaction`
flow is treating all rejections as availability misses, which can trip the
breaker on expected payment/auth errors. Update the catch in `services.js` to
distinguish invalid payment/auth responses from real transport or facilitator
outages, and only call `recordFacilitatorResult(false)` for the latter; keep
logging the failure with `logger.warn` and use the existing
`verifyTransaction`/facilitator response handling to decide when to skip breaker
accounting.
---
Nitpick comments:
In `@backend/.env.example`:
- Around line 6-7: The .env.example entries are out of dotenv-linter order, so
reorder the FACILITATOR settings so FACILITATOR_TIMEOUT_MS appears before
FACILITATOR_URL. Keep the existing values unchanged and adjust only the key
order in the .env.example file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fc5ced5-e263-493c-b759-24e765b2cd19
📒 Files selected for processing (4)
backend/.env.examplebackend/src/config.jsbackend/src/index.jsbackend/src/routes/services.js
| // Check circuit breaker state | ||
| function isCircuitOpen() { | ||
| if (circuitState === CIRCUIT_STATE.CLOSED) return false; | ||
| if (circuitState === CIRCUIT_STATE.OPEN) { | ||
| if (Date.now() >= circuitOpenUntil) { | ||
| // Transition to half-open | ||
| circuitState = CIRCUIT_STATE.HALF_OPEN; | ||
| logger.info('Circuit breaker transitioning to half-open state'); | ||
| } | ||
| return circuitState === CIRCUIT_STATE.OPEN; | ||
| } | ||
| // Half-open: let one request through | ||
| return false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Gate half-open to a single in-flight probe.
Line 104 currently lets every concurrent request through while the breaker is half-open. If the facilitator is still slow/down, the request path can stall again until one probe completes, instead of immediately returning 503 FACILITATOR_UNAVAILABLE for non-probe requests.
Proposed half-open probe guard
let circuitState = CIRCUIT_STATE.CLOSED;
let consecutiveFailures = 0;
let circuitOpenUntil = 0;
+let halfOpenProbeInFlight = false;
const MAX_CONSECUTIVE_FAILURES = 5;
const CIRCUIT_OPEN_DURATION_MS = 30000; // 30 seconds
const HALF_OPEN_PROBE_INTERVAL_MS = 10000; // 10 seconds
@@
if (Date.now() >= circuitOpenUntil) {
// Transition to half-open
circuitState = CIRCUIT_STATE.HALF_OPEN;
+ halfOpenProbeInFlight = false;
logger.info('Circuit breaker transitioning to half-open state');
}
return circuitState === CIRCUIT_STATE.OPEN;
}
- // Half-open: let one request through
+ // Half-open: let exactly one probe through.
+ if (halfOpenProbeInFlight) return true;
+ halfOpenProbeInFlight = true;
return false;
}
@@
function recordFacilitatorResult(success) {
+ const wasHalfOpen = circuitState === CIRCUIT_STATE.HALF_OPEN;
+ if (wasHalfOpen) halfOpenProbeInFlight = false;
+
if (success) {
consecutiveFailures = 0;
- if (circuitState === CIRCUIT_STATE.HALF_OPEN) {
+ if (wasHalfOpen) {
circuitState = CIRCUIT_STATE.CLOSED;
logger.info('Circuit breaker closed after successful probe');
}
@@
circuitState = CIRCUIT_STATE.OPEN;
circuitOpenUntil = Date.now() + CIRCUIT_OPEN_DURATION_MS;
+ halfOpenProbeInFlight = false;
logger.warn({ consecutiveFailures }, 'Circuit breaker opened due to consecutive failures');
}
}Also applies to: 109-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/services.js` around lines 93 - 105, The half-open path in
isCircuitOpen currently allows every request through, so add a single in-flight
probe guard for the circuit breaker state in backend/src/routes/services.js.
Update the logic around isCircuitOpen and the request handling at the
facilitator route so only the first half-open request is allowed to probe while
all concurrent non-probe requests immediately return FACILITATOR_UNAVAILABLE
with 503. Use the existing circuitState/CIRCUIT_STATE.HALF_OPEN flow and related
route handler logic to track and release the probe.
| } catch (err) { | ||
| const latency = Date.now() - startTime; | ||
| logger.warn({ err, latency_ms: latency }, 'Facilitator call failed'); | ||
| recordFacilitatorResult(false); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@x402/core 2.9.0 HTTPFacilitatorClient verifyTransaction thrown errors invalid payment transport timeout
💡 Result:
The errors encountered when using HTTPFacilitatorClient in @x402/core (and its corresponding Python implementation) regarding invalid payment, transport issues, and timeouts are largely attributed to the architectural limitations of early x402 facilitator client versions and underlying network conditions [1][2]. Key causes and context: 1. Hardcoded Timeouts and Client Lifecycle: In versions around v2.0.1 - 2.9.0, the FacilitatorClient often instantiated a fresh HTTP client (e.g., httpx.AsyncClient) for every request with default timeout settings [1]. This lack of persistent connection management prevented custom configuration of timeouts, retries, or connection pooling, causing requests to fail prematurely on slower or high-latency networks [1]. 2. Impedance Mismatch with L2 Finality: Developers have reported "transport timeout" errors (often manifesting as ConnectionTimeout) when interacting with L2 chains like Base [2]. The facilitator's synchronous-style request context often assumes faster block confirmation times than are realistic under network congestion, leading to timeouts even if the transaction eventually succeeds on-chain [2]. 3. Invalid Payment / Payload Errors: Errors such as invalid_payment or invalid_payload often occur when the facilitator rejects a request before full validation [3][4][5]. While sometimes due to malformed payloads or incorrect headers (like missing signatures) [3][4], reports indicate that these errors can also be triggered by environment-specific or project-level authentication issues with the facilitator service (e.g., CDP account restrictions), which return generic invalid_payload responses even when the actual transaction data is correct [5]. Recommended Mitigation Strategies: - Dependency Injection: If using a version that supports it (introduced to address these issues), inject a pre-configured HTTP client (e.g., with a custom Timeout(60.0)) into the FacilitatorClient constructor to manage the lifecycle and increase timeout thresholds [1]. - Reconciliation Logic: Since a transport timeout does not necessarily mean a transaction failure, implement a reconciliation loop that checks the status of the submitted transaction hash on-chain (e.g., via eth_getTransactionReceipt) before assuming the payment failed [2]. - Verify Environment/Authentication: If receiving persistent invalid_payload errors, double-check that your API credentials have sufficient permissions and that the environment (Mainnet vs. Testnet) matches your facilitator configuration [5]. For up-to-date implementation, consult the latest documentation as newer versions of the x402 ecosystem are shifting toward more robust, network-aware timeout profiles and non-blocking settlement state machines [2].
Citations:
- 1: https://github.com/coinbase/x402/issues/651
- 2: https://github.com/coinbase/x402/issues/1062
- 3: https://github.com/x402-foundation/x402/blob/main/specs/x402-specification-v2.md
- 4: https://github.com/x402-foundation/x402/blob/main/specs/transports-v2/http.md
- 5: https://github.com/coinbase/x402/issues/1171
Exclude expected payment failures from the breaker. If verifyTransaction rejects for invalid payment/auth responses, this catch path counts them as availability misses and a few bad requests can trip the circuit for valid users. Only transport or facilitator outages should call recordFacilitatorResult(false).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/services.js` around lines 178 - 181, The facilitator
failure catch in the `verifyTransaction` flow is treating all rejections as
availability misses, which can trip the breaker on expected payment/auth errors.
Update the catch in `services.js` to distinguish invalid payment/auth responses
from real transport or facilitator outages, and only call
`recordFacilitatorResult(false)` for the latter; keep logging the failure with
`logger.warn` and use the existing `verifyTransaction`/facilitator response
handling to decide when to skip breaker accounting.
|
@veemakama please fix ci |
2 participants
Requirements Check List
✅ 1. **Configurable FACILITATOR_TIMEOUT_MS env var (default: 10,000 ms) — Added to backend/src/config.js and backend/.env.example .
✅ 2. Timeout for facilitator calls — Wrapped HTTPFacilitatorClient with a WrappedFacilitatorClient that uses Promise.race with a timeout promise!
✅ 3. Circuit breaker — Implemented a state machine (closed/open/half-open):
✅ 4. Facilitator status in GET /healthz — Added checkFacilitatorHealth function in services.js , imported in index.js and updated /healthz endpoint now returns facilitator status with status , latency_ms , and optional error .
✅ 5. Logging — Added:
✅ 6. Tests — Dependencies installed, tests ran and passed!
Closes #246
Summary by CodeRabbit
New Features
/healthzresponse, including status and response time.Bug Fixes