fix activity webhook returning 200 before chain work finishes#990
fix activity webhook returning 200 before chain work finishes#990mooncitydev wants to merge 1 commit intoexactly:mainfrom
Conversation
|
WalkthroughRefactors the activity hook's error handling and control flow in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| }), | ||
| }), | ||
| validatorHook({ code: "bad alchemy", status: 200, debug }), | ||
| validatorHook({ code: "bad alchemy", debug }), |
There was a problem hiding this comment.
🔴 Removed status: 200 from webhook validatorHook causes Alchemy to retry on validation failures
The status: 200 parameter was removed from the validatorHook call, so it now defaults to status: 400 (server/utils/validatorHook.ts:17). When Alchemy sends a webhook payload that fails validation, the endpoint now returns 400 instead of 200. Alchemy retries webhooks on non-200 responses, so malformed payloads will be retried repeatedly until the retry limit is exhausted — wasting resources and generating duplicate Sentry errors.
Every other webhook hook in the codebase explicitly passes status: 200 to acknowledge the webhook even when validation fails:
server/hooks/bridge.ts:92—status: 200server/hooks/block.ts:108—status: 200server/hooks/manteca.ts:147—status: 200server/hooks/persona.ts:207—status: 200
| validatorHook({ code: "bad alchemy", debug }), | |
| validatorHook({ code: "bad alchemy", status: 200, debug }), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| try { | ||
| const activityResults = await Promise.allSettled( | ||
| [...pokes].map(([account, { publicKey, factory, source, assets }]) => | ||
| continueTrace({ sentryTrace, baggage }, () => | ||
| withScope((scope) => | ||
| startSpan( | ||
| { name: "account activity", op: "exa.activity", attributes: { account }, forceTransaction: true }, | ||
| async (span) => { | ||
| scope.setUser({ id: account }); | ||
| const isDeployed = !!(await client.getCode({ address: account })); | ||
| scope.setTag("exa.new", !isDeployed); | ||
| if (!isDeployed) { | ||
| try { | ||
| await createWalletClient({ chain, transport, account: keeper.account }) | ||
| .extend((wallet) => extender(wallet, { publicClient: client, traceClient: client })) | ||
| .exaSend( | ||
| { name: "create account", op: "exa.account", attributes: { account } }, | ||
| { | ||
| address: factory, | ||
| functionName: "createAccount", | ||
| args: [0n, [decodePublicKey(publicKey, bytesToBigInt)]], | ||
| abi: exaAccountFactoryAbi, | ||
| }, | ||
| chain.id === exaChain.id ? undefined : { fees: "auto" }, | ||
| ); | ||
| track({ event: "AccountFunded", userId: account, properties: { source } }); | ||
| } catch (error: unknown) { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: "account_failed" }); | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| if (chain.id !== exaChain.id) { | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| return; | ||
| } | ||
| if (assets.has(ETH)) assets.delete(WETH); | ||
| const results = await Promise.allSettled( | ||
| [...assets] | ||
| .filter((asset) => marketsByAsset.has(asset) || asset === ETH) | ||
| .map(async (asset) => | ||
| withRetry( | ||
| () => | ||
| keeper | ||
| .exaSend( | ||
| { name: "poke account", op: "exa.poke", attributes: { account, asset } }, | ||
| { | ||
| address: account, | ||
| abi: [...exaPluginAbi, ...upgradeableModularAccountAbi, ...auditorAbi, ...marketAbi], | ||
| ...(asset === ETH | ||
| ? { functionName: "pokeETH" } | ||
| : { | ||
| functionName: "poke", | ||
| args: [marketsByAsset.get(asset)!], // eslint-disable-line @typescript-eslint/no-non-null-assertion | ||
| }), | ||
| }, | ||
| { ignore: ["NoBalance()"] }, | ||
| ) | ||
| .then((receipt) => { | ||
| if (receipt) return receipt; | ||
| throw new Error("NoBalance()"); | ||
| }), | ||
| { | ||
| delay: 2000, | ||
| retryCount: 5, | ||
| shouldRetry: ({ error }) => { | ||
| if (error instanceof Error && error.message === "NoBalance()") return true; | ||
| withScope((captureScope) => { | ||
| captureScope.setUser({ id: account }); | ||
| captureException(error, { level: "error", fingerprint: revertFingerprint(error) }); | ||
| }); | ||
| return true; | ||
| if (chain.id !== exaChain.id) { | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| return; | ||
| } | ||
| if (assets.has(ETH)) assets.delete(WETH); | ||
| const pokeResults = await Promise.allSettled( | ||
| [...assets] | ||
| .filter((asset) => marketsByAsset.has(asset) || asset === ETH) | ||
| .map(async (asset) => | ||
| withRetry( | ||
| () => | ||
| keeper | ||
| .exaSend( | ||
| { name: "poke account", op: "exa.poke", attributes: { account, asset } }, | ||
| { | ||
| address: account, | ||
| abi: [...exaPluginAbi, ...upgradeableModularAccountAbi, ...auditorAbi, ...marketAbi], | ||
| ...(asset === ETH | ||
| ? { functionName: "pokeETH" } | ||
| : { | ||
| functionName: "poke", | ||
| args: [marketsByAsset.get(asset)!], // eslint-disable-line @typescript-eslint/no-non-null-assertion | ||
| }), | ||
| }, | ||
| { ignore: ["NoBalance()"] }, | ||
| ) | ||
| .then((receipt) => { | ||
| if (receipt) return receipt; | ||
| throw new Error("NoBalance()"); | ||
| }), | ||
| { | ||
| delay: 2000, | ||
| retryCount: 5, | ||
| shouldRetry: ({ error }) => { | ||
| if (error instanceof Error && error.message === "NoBalance()") return true; | ||
| withScope((captureScope) => { | ||
| captureScope.setUser({ id: account }); | ||
| captureException(error, { level: "error", fingerprint: revertFingerprint(error) }); | ||
| }); | ||
| return true; | ||
| }, | ||
| }, | ||
| }, | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| for (const result of results) { | ||
| if (result.status === "fulfilled") continue; | ||
| if (result.reason instanceof Error && result.reason.message === "NoBalance()") { | ||
| withScope((captureScope) => { | ||
| captureScope.setUser({ id: account }); | ||
| captureScope.addEventProcessor((event) => { | ||
| if (event.exception?.values?.[0]) event.exception.values[0].type = "NoBalance"; | ||
| return event; | ||
| ); | ||
| for (const result of pokeResults) { | ||
| if (result.status === "fulfilled") continue; | ||
| if (result.reason instanceof Error && result.reason.message === "NoBalance()") { | ||
| withScope((captureScope) => { | ||
| captureScope.setUser({ id: account }); | ||
| captureScope.addEventProcessor((event) => { | ||
| if (event.exception?.values?.[0]) event.exception.values[0].type = "NoBalance"; | ||
| return event; | ||
| }); | ||
| captureException(result.reason, { | ||
| level: "warning", | ||
| fingerprint: ["{{ default }}", "NoBalance"], | ||
| }); | ||
| }); | ||
| captureException(result.reason, { | ||
| level: "warning", | ||
| fingerprint: ["{{ default }}", "NoBalance"], | ||
| }); | ||
| }); | ||
| continue; | ||
| continue; | ||
| } | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: "poke_failed" }); | ||
| throw result.reason; | ||
| } | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: "poke_failed" }); | ||
| throw result.reason; | ||
| } | ||
| autoCredit(account) | ||
| .then(async (auto) => { | ||
| span.setAttribute("exa.autoCredit", auto); | ||
| if (!auto) return; | ||
| const credential = await database.query.credentials.findFirst({ | ||
| where: eq(credentials.account, account), | ||
| columns: {}, | ||
| with: { | ||
| cards: { | ||
| columns: { id: true, mode: true }, | ||
| where: inArray(cards.status, ["ACTIVE", "FROZEN"]), | ||
| autoCredit(account) | ||
| .then(async (auto) => { | ||
| span.setAttribute("exa.autoCredit", auto); | ||
| if (!auto) return; | ||
| const credential = await database.query.credentials.findFirst({ | ||
| where: eq(credentials.account, account), | ||
| columns: {}, | ||
| with: { | ||
| cards: { | ||
| columns: { id: true, mode: true }, | ||
| where: inArray(cards.status, ["ACTIVE", "FROZEN"]), | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| if (!credential || credential.cards.length === 0) return; | ||
| const card = credential.cards[0]; | ||
| span.setAttribute("exa.card", card?.id); | ||
| if (card?.mode !== 0) return; | ||
| await database.update(cards).set({ mode: 1 }).where(eq(cards.id, card.id)); | ||
| span.setAttribute("exa.mode", 1); | ||
| sendPushNotification({ | ||
| userId: account, | ||
| headings: t("Card mode changed"), | ||
| contents: t("Credit mode activated"), | ||
| }).catch((error: unknown) => captureException(error)); | ||
| }) | ||
| .catch((error: unknown) => captureException(error)); | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| }, | ||
| }); | ||
| if (!credential || credential.cards.length === 0) return; | ||
| const card = credential.cards[0]; | ||
| span.setAttribute("exa.card", card?.id); | ||
| if (card?.mode !== 0) return; | ||
| await database.update(cards).set({ mode: 1 }).where(eq(cards.id, card.id)); | ||
| span.setAttribute("exa.mode", 1); | ||
| sendPushNotification({ | ||
| userId: account, | ||
| headings: t("Card mode changed"), | ||
| contents: t("Credit mode activated"), | ||
| }).catch((error: unknown) => captureException(error)); | ||
| }) | ||
| .catch((error: unknown) => captureException(error)); | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| }, | ||
| ), | ||
| ), | ||
| ), | ||
| ).catch((error: unknown) => { | ||
| withScope((scope) => { | ||
| scope.setUser({ id: account }); | ||
| captureException(error, { level: "error", fingerprint: revertFingerprint(error) }); | ||
| }); | ||
| throw error; | ||
| }), | ||
| ), | ||
| ) | ||
| .then((results) => { | ||
| getActiveSpan()?.setStatus( | ||
| results.every((result) => result.status === "fulfilled") | ||
| ? { code: SPAN_STATUS_OK } | ||
| : { code: SPAN_STATUS_ERROR, message: "activity_failed" }, | ||
| ); | ||
| }) | ||
| .catch((error: unknown) => captureException(error)); | ||
| return c.json({}); | ||
| ).catch((error: unknown) => { | ||
| withScope((scope) => { | ||
| scope.setUser({ id: account }); | ||
| captureException(error, { level: "error", fingerprint: revertFingerprint(error) }); | ||
| }); | ||
| throw error; | ||
| }), | ||
| ), | ||
| ); | ||
| getActiveSpan()?.setStatus( | ||
| activityResults.every((result) => result.status === "fulfilled") | ||
| ? { code: SPAN_STATUS_OK } | ||
| : { code: SPAN_STATUS_ERROR, message: "activity_failed" }, | ||
| ); | ||
| const rejected = activityResults.filter( | ||
| (result): result is PromiseRejectedResult => result.status === "rejected", | ||
| ); | ||
| if (rejected.length > 0) { | ||
| return c.json({ code: "activityFailed", failed: rejected.length }, 500); | ||
| } | ||
| return c.json({}); | ||
| } catch (error: unknown) { | ||
| captureException(error); | ||
| return c.json({ code: "activityError" }, 500); | ||
| } |
There was a problem hiding this comment.
🚩 Behavioral shift: webhook now blocks on processing and returns 500 on failure
The old code was fire-and-forget: Promise.allSettled(...) was not awaited, the handler immediately returned c.json({}) with a 200 status, and processing happened in the background. The new code awaits Promise.allSettled and returns 500 if any account processing fails (server/hooks/activity.ts:318-319). This means:
- The HTTP response is delayed until all account creation, poke retries (up to 5 retries × 2s delay each), and error handling complete.
- Alchemy receives 500 on processing failures and will retry the entire webhook, potentially re-triggering account creation and poke operations for accounts that already succeeded.
This may be intentional (the commit message says "await alchemy activity hook work before returning 200"), but the retry implications are worth considering — especially since Promise.allSettled collects both fulfilled and rejected results, so partial failures (e.g., 3/5 accounts succeed, 2 fail) will return 500 and Alchemy will retry all 5.
Was this helpful? React with 👍 or 👎 to provide feedback.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the activity hook to use async/await for improved error handling and response management. The logic now explicitly awaits the results of account activities and returns a 500 status code if any operations fail. Feedback was provided regarding an unawaited promise chain in the autoCredit logic, which could lead to race conditions, and a suggestion to throw errors when expected database records are missing to better reflect data integrity issues.
| autoCredit(account) | ||
| .then(async (auto) => { | ||
| span.setAttribute("exa.autoCredit", auto); | ||
| if (!auto) return; | ||
| const credential = await database.query.credentials.findFirst({ | ||
| where: eq(credentials.account, account), | ||
| columns: {}, | ||
| with: { | ||
| cards: { | ||
| columns: { id: true, mode: true }, | ||
| where: inArray(cards.status, ["ACTIVE", "FROZEN"]), | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| if (!credential || credential.cards.length === 0) return; | ||
| const card = credential.cards[0]; | ||
| span.setAttribute("exa.card", card?.id); | ||
| if (card?.mode !== 0) return; | ||
| await database.update(cards).set({ mode: 1 }).where(eq(cards.id, card.id)); | ||
| span.setAttribute("exa.mode", 1); | ||
| sendPushNotification({ | ||
| userId: account, | ||
| headings: t("Card mode changed"), | ||
| contents: t("Credit mode activated"), | ||
| }).catch((error: unknown) => captureException(error)); | ||
| }) | ||
| .catch((error: unknown) => captureException(error)); | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| }, | ||
| }); | ||
| if (!credential || credential.cards.length === 0) return; | ||
| const card = credential.cards[0]; | ||
| span.setAttribute("exa.card", card?.id); | ||
| if (card?.mode !== 0) return; | ||
| await database.update(cards).set({ mode: 1 }).where(eq(cards.id, card.id)); | ||
| span.setAttribute("exa.mode", 1); | ||
| sendPushNotification({ | ||
| userId: account, | ||
| headings: t("Card mode changed"), | ||
| contents: t("Credit mode activated"), | ||
| }).catch((error: unknown) => captureException(error)); | ||
| }) | ||
| .catch((error: unknown) => captureException(error)); |
There was a problem hiding this comment.
The autoCredit promise chain is not awaited, which means the parent function can resolve before the logic finishes. Additionally, when an expected database record (like a card) is not found, the code should throw an error rather than returning silently. This indicates a system or data integrity issue, making a 5xx-level error more appropriate.
await autoCredit(account)
.then(async (auto) => {
span.setAttribute("exa.autoCredit", auto);
if (!auto) return;
const credential = await database.query.credentials.findFirst({
where: eq(credentials.account, account),
columns: {},
with: {
cards: {
columns: { id: true, mode: true },
where: inArray(cards.status, ["ACTIVE", "FROZEN"]),
},
},
});
if (!credential || credential.cards.length === 0) {
throw new Error("Expected card record not found for account");
}
const card = credential.cards[0];
span.setAttribute("exa.card", card.id);
if (card.mode !== 0) return;
await database.update(cards).set({ mode: 1 }).where(eq(cards.id, card.id));
span.setAttribute("exa.mode", 1);
sendPushNotification({
userId: account,
headings: t("Card mode changed"),
contents: t("Credit mode activated"),
}).catch((error: unknown) => captureException(error));
})
.catch((error: unknown) => captureException(error));References
- Throw an error when an expected database record (like a card) is not found. This indicates a system or data integrity issue, not a client-side error that can be fixed by the user, so a 5xx-level error is more appropriate than a 4xx.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/hooks/activity.ts (1)
141-159:⚠️ Potential issue | 🔴 CriticalAdd idempotency guards before enabling full-payload retries
Line [318] now returns 500 for any rejected account, which will make upstream retry the entire webhook payload. Because side effects happen earlier (Line [141] push notification, Line [200] analytics tracking), successful accounts can receive duplicate side effects on retry. This is a non-idempotent retry hazard and should be guarded (e.g., durable dedupe key per activity/account before side effects).
Also applies to: 200-200, 318-320
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee86c277-4d49-45a9-9d65-aa8ccff47679
📒 Files selected for processing (1)
server/hooks/activity.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 113b92f208
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (rejected.length > 0) { | ||
| return c.json({ code: "activityFailed", failed: rejected.length }, 500); | ||
| } |
There was a problem hiding this comment.
Returning 500 when any account pipeline rejects causes Alchemy to retry the entire webhook payload, not just the failed account. In this handler, side effects (e.g., sendPushNotification in the transfer loop and successful exaSend calls for other accounts) can already be committed before this response is decided, and there is no webhook-event idempotency check, so a single failing account can repeatedly duplicate notifications/transactions for accounts that already succeeded.
Useful? React with 👍 / 👎.
1 participant
hey, noticed the alchemy address activity hook was firing Promise.allSettled for deploy/poke work but never awaited it, then returned 200 right away. so the caller thought everything succeeded while txs could still fail in the background and only hit sentry.
now we await that pipeline and return 500 when any account pipeline rejects so alchemy (or whatever sits in front) can retry instead of silently losing the run.
also dropped status 200 on the valibot validator hook for bad payloads so invalid json/signature cases go through the default 400 from validatorHook instead of pretending success.
cheers
Summary by CodeRabbit