-
Notifications
You must be signed in to change notification settings - Fork 43
[Feature] settle in parallel #693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🚅 Deployed to the echo-pr-693 environment in echo
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| } | ||
|
|
||
| // Case 2: Settle failed but model succeeded | ||
| if (!settleResult && modelResult.success) { |
There was a problem hiding this comment.
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 connectionWhat 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| await finalize( | ||
| paymentAmountDecimal, | ||
| transactionCosts.rawTransactionCost, | ||
| transactionCosts.totalAppProfit, | ||
| transactionCosts.echoProfit, | ||
| payload | ||
| ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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
Execute settlement in parallel. If it succeeds, we carry on with the finalize flow. If it fails, we don't finalize or refund.