Skip to content

Conversation

@zdql
Copy link
Contributor

@zdql zdql commented Nov 20, 2025

Execute settlement in parallel. If it succeeds, we carry on with the finalize flow. If it fails, we don't finalize or refund.

@zdql zdql requested a review from rsproule November 20, 2025 20:49
@railway-app
Copy link

railway-app bot commented Nov 20, 2025

🚅 Deployed to the echo-pr-693 environment in echo

Service Status Web Updated (UTC)
echo ◻️ Removed (View Logs) Web Nov 20, 2025 at 10:24 pm

@vercel
Copy link
Contributor

vercel bot commented Nov 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
assistant-ui-template Ready Ready Preview Comment Nov 20, 2025 8:52pm
echo-control Ready Ready Preview Comment Nov 20, 2025 8:52pm
echo-next-boilerplate Ready Ready Preview Comment Nov 20, 2025 8:52pm
echo-next-image Ready Ready Preview Comment Nov 20, 2025 8:52pm
echo-next-sdk-example Ready Ready Preview Comment Nov 20, 2025 8:52pm
echo-video-template Ready Ready Preview Comment Nov 20, 2025 8:52pm
echo-vite-sdk-example Ready Ready Preview Comment Nov 20, 2025 8:52pm
next-chat-template Ready Ready Preview Comment Nov 20, 2025 8:52pm
react-boilerplate Ready Ready Preview Comment Nov 20, 2025 8:52pm
react-chat Ready Ready Preview Comment Nov 20, 2025 8:52pm
react-image Ready Ready Preview Comment Nov 20, 2025 8:52pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
component-registry Skipped Skipped Nov 20, 2025 8:52pm

}

// Case 2: Settle failed but model succeeded
if (!settleResult && modelResult.success) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case 2 attempts to send a response when one has already been sent by the settle function, causing an error.

View Details
📝 Patch Details
diff --git a/packages/app/server/src/handlers.ts b/packages/app/server/src/handlers.ts
index 131d35d9..d8af4084 100644
--- a/packages/app/server/src/handlers.ts
+++ b/packages/app/server/src/handlers.ts
@@ -54,11 +54,6 @@ export async function handleX402Request({
       provider: provider.getType(),
       url: req.url,
     });
-    modelRequestService.handleResolveResponse(
-      res,
-      isStream,
-      data
-    );
     return;
   }
 

Analysis

Double response sent in handleX402Request when settle fails but model succeeds

What fails: In handleX402Request(), Case 2 (lines 46-62), when the settle() function fails and returns undefined after sending a 402 response, but modelRequestService.executeModelRequest() succeeds, the code attempts to send another response via handleResolveResponse(). This causes an error: "Cannot set headers after they are sent to the client".

How to reproduce:

// In handleX402Request, Case 2 is triggered when:
// 1. settle() fails (e.g., getSmartAccount fails, validateXPaymentHeader fails, insufficient payment, etc.)
//    - settle() calls buildX402Response() which sends res.status(402).json(resBody)
//    - settle() returns undefined
// 2. AND modelRequestService.executeModelRequest() succeeds (Promise.all waits for both)
// Result: Both promises complete, Case 2 condition is true (!settleResult && modelResult.success)
// Code calls modelRequestService.handleResolveResponse(res, isStream, data)
// This attempts to call res.json() on an already-responded connection

What happens vs expected: Throws unhandled error "Cannot set headers after they are sent to the client" when attempting to send the second response.

Expected behavior: Since settle() already sent the 402 response, the function should return without attempting to send another response, consistent with Case 1 behavior when both settle and model fail.

Fix: Remove the handleResolveResponse() call in Case 2 (lines 54-57), allowing settle's 402 response to be the final response to the client. The logging of the failure condition is still preserved for debugging.


// Case 1: Settle failed and model failed
if (!settleResult && !modelResult.success) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return null here works ? upstream stuff handles this correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both fail, the model's error response should throw all the way up to the root and return with that

Comment on lines +105 to +111
await finalize(
paymentAmountDecimal,
transactionCosts.rawTransactionCost,
transactionCosts.totalAppProfit,
transactionCosts.echoProfit,
payload
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like previously we would catch and do the refund if this threw. is this intentional? aren't we missing the outer try/catch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could try/catch here, again if this fails it will throw all the way up to the root and return an error.

Do you think we need to re-throw in this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the case im thinking about is
/settle succeeds, then something else fails, so we never refund().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if settle succeeds and model request fails that would catch in a different branch. In that case we gave out tokens for free, we wouldn't refund or finalize. if settle succeds and model fails, we just refund.

If both succeed, we can just fire the finalize and forget. If finalize fails, the error will bubble up to the next() call and return with the correct error message

@zdql zdql merged commit ca11b1e into master Nov 20, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants