perf(daemon): fire-and-forget sendRealtimeTx#402
perf(daemon): fire-and-forget sendRealtimeTx#402andreabadesso wants to merge 3 commits intomasterfrom
Conversation
Tier 2 fix #8 from #395. The SQS publish for real-time wallet tx notifications was being awaited in the handler, blocking event processing on network I/O for a best-effort notification. Make it fire-and-forget with a .catch() for logging, matching the existing pattern used for invokeOnTxPushNotificationRequestedLambda right below it. The outer try/catch is no longer needed — errors from the async publish now flow through the .catch() handler, and the block has no other code that can throw synchronously. Savings only applies when the handler actually has wallets to notify (estimated 5-30 ms when it fires, per the issue). Also adds sendRealtimeTx to the utils mock in services.test.ts — it was previously undefined there and worked only because the prior try/catch swallowed the TypeError; the fire-and-forget pattern surfaces it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes make Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
The integration tests mocked sendRealtimeTx with plain jest.fn(), which returns undefined. The old production code awaited the call inside a try/catch, so the undefined return was harmless (await undefined ≡ await Promise.resolve(undefined)). After the fire-and-forget refactor, the code calls `.catch()` on the return value — which throws TypeError: Cannot read properties of undefined (reading 'catch') when the mock returns undefined instead of a Promise. Without the old try/catch to swallow the error, this crashed handleVertexAccepted, the sync hung, and integration tests timed out after 30s. The production sendRealtimeTx is `async` so it always returns a Promise; the fix is just to make the mocks match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes an await on sendRealtimeTx within handleVertexAccepted so the daemon doesn’t block event processing on best-effort SQS publishing, while still logging async failures via .catch().
Changes:
- Convert
sendRealtimeTxto fire-and-forget inhandleVertexAccepted, preserving error logging via.catch() - Update unit/integration test mocks so
sendRealtimeTxis a resolving async mock (avoids runtimeTypeErrorand matches new async usage)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/daemon/src/services/index.ts | Makes realtime SQS publish non-blocking and logs async failures without stalling the handler |
| packages/daemon/tests/services/services.test.ts | Adds sendRealtimeTx to the ../../src/utils mock as an async-resolving function |
| packages/daemon/tests/integration/token_creation.test.ts | Updates ../../src/utils/aws mock so sendRealtimeTx resolves (matches fire-and-forget async usage) |
| packages/daemon/tests/integration/token_created_hybrid_with_reorg.test.ts | Same: ensure sendRealtimeTx mock resolves as a promise |
| packages/daemon/tests/integration/balances.test.ts | Same: ensure sendRealtimeTx mock resolves as a promise |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/daemon/src/services/index.ts (1)
531-540: Consider surfacing SQS publish failures beyond logs.With the await removed, failures in
sendRealtimeTxno longer affect the handler or state machine — they're only visible as log lines. If real-time tx delivery is user-impacting, consider emitting a metric/counter on.catch(or a span event on the active tracer) so that a sustained SQS outage is observable beyond grepping logs. Also, on shutdown, any in-flight publish is dropped silently; that's the accepted trade-off per the PR description, but worth keeping in mind for graceful shutdown draining if this becomes a hot path.No behavioral issue with the change itself — the
asyncdeclaration ofsendRealtimeTxensures any synchronous throw (e.g., theNEW_TX_SQSguard) becomes a rejection and is caught by.catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/daemon/src/services/index.ts` around lines 531 - 540, The fire-and-forget call to sendRealtimeTx(Array.from(seenWallets), txData) only logs errors on rejection; update the .catch handler to also emit an observability signal (e.g., increment a failure metric/counter or add a span/event to the active tracer) so sustained SQS publish failures are visible beyond logs; locate the sendRealtimeTx invocation and its .catch block and call your metrics client or tracer API there (and include contextual tags like seenWallets length and txData identifiers) while preserving the non-blocking behavior and existing logger.error calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/daemon/src/services/index.ts`:
- Around line 531-540: The fire-and-forget call to
sendRealtimeTx(Array.from(seenWallets), txData) only logs errors on rejection;
update the .catch handler to also emit an observability signal (e.g., increment
a failure metric/counter or add a span/event to the active tracer) so sustained
SQS publish failures are visible beyond logs; locate the sendRealtimeTx
invocation and its .catch block and call your metrics client or tracer API there
(and include contextual tags like seenWallets length and txData identifiers)
while preserving the non-blocking behavior and existing logger.error calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e6aba5b-129b-4d9d-886f-c2bfffda20d3
📒 Files selected for processing (5)
packages/daemon/__tests__/integration/balances.test.tspackages/daemon/__tests__/integration/token_created_hybrid_with_reorg.test.tspackages/daemon/__tests__/integration/token_creation.test.tspackages/daemon/__tests__/services/services.test.tspackages/daemon/src/services/index.ts
Motivation
Tier 2 fix #8 from #395. The SQS publish for real-time wallet tx notifications was being awaited inside the event handler, blocking sync progress on network I/O for a best-effort notification. This PR makes it fire-and-forget, matching the pattern already used for
invokeOnTxPushNotificationRequestedLambdaright below it.Branches directly off
master— independent of the benchmarking stack (#396–#400).Acceptance Criteria
sendRealtimeTxcall no longer awaited in thehandleVertexAcceptedpath.catch()(same behavior as before, just doesn't block the handler)Change
Test mock note
Also adds
sendRealtimeTx: jest.fn().mockResolvedValue(undefined)to the'../../src/utils'mock inservices.test.ts. It was missing from that mock before — the oldawait sendRealtimeTx(...)inside a try/catch was silently swallowing aTypeError: sendRealtimeTx is not a functionin the affected test paths. Fire-and-forget surfaces the crash, so the mock needs the function. No production code is affected by the mock change.Savings
Per #395: ~5–30 ms when the notification actually fires (only for txs touching wallets). Doesn't apply to the common sync-from-scratch case.
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor