-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(ts): telemetry batch upload #2322
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
base: next
Are you sure you want to change the base?
Conversation
|
The Please review and fix the vulnerabilities. You can try running: pnpm audit --fix --prodAudit output |
…ent promises, add Node.js-specific auto-flush, with Cloudflare Workers escape hatch
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| .finally(() => { | ||
| // Re-emit the signal to allow normal process termination | ||
| process.removeListener(signal as NodeJS.Signals, () => signalHandler(signal)); | ||
| process.kill(process.pid, signal); |
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.
Signal handler causes infinite loop on process termination
High Severity
The process.removeListener call on line 315 creates a new arrow function () => signalHandler(signal) which is a different reference than the one registered on lines 320-321 (() => signalHandler('SIGINT')). Since function references don't match, the listener isn't removed. When process.kill(process.pid, signal) re-emits the signal, the original listener catches it again, causing an infinite loop that prevents the process from ever terminating on SIGINT or SIGTERM.
This PR:
awaitable.BatchProcessor’sprocessBatchCallbackalwaysasyncand track in-flight sends.BatchProcessor.flush()and exposesTelemetryTransport.flush()to drain pending telemetry on Node.js-incompatible runtimes (e.g., Cloudflare Workers). We might revise this after Cloudflare Workers support is improved (PLEN-1039)I've also tested the changes manually with a custom Bun server, replacing
const TELEMETRY_URL = 'https://telemetry.composio.dev/v1'with a local one. I didn't add this part to the e2e test suite in@composio/corefor simplicity, and to not violate the single responsibility principle.I'll manually test this implementation on Datadog in PLEN-1057.