fix: drain telemetry on shutdown - #573
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesTelemetry lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Process
participant Capture
participant TelemetryProxy
Process->>Capture: capture(event, properties)
Capture->>TelemetryProxy: send tracked telemetry
Process->>Capture: beforeExit
Capture->>Capture: flushTelemetry(timeout)
Capture-->>Process: settle pending sends or timeout
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/capture.ts`:
- Around line 280-282: Replace the beforeExit-only telemetry flush in the
shutdown handling with awaited flushTelemetry() calls in the SIGINT, SIGTERM,
uncaughtException, and unhandledRejection handlers, ensuring each handler
flushes before completing its exit or error path. Remove the beforeExit
registration if it is no longer needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| process.once('beforeExit', () => { | ||
| void flushTelemetry(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find explicit exits, signal handlers, and any flushTelemetry usage.
rg -nP "process\.(exit|on(ce)?\s*\(\s*['\"](SIGINT|SIGTERM|exit|beforeExit))" -g '!**/node_modules/**'
echo '--- flushTelemetry usages ---'
rg -nP '\bflushTelemetry\b' -g '!**/node_modules/**'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for shutdown/telemetry wiring.
ast-grep outline src/utils/capture.ts --view expanded || true
echo '--- capture.ts excerpt ---'
sed -n '240,340p' src/utils/capture.ts
echo '--- shutdown/telemetry references ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
"beforeExit|SIGINT|SIGTERM|uncaughtException|process\.exit|flushTelemetry|sendToTelemetryProxy|pendingCaptures" \
src .Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 22674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the top-level flow around flushTelemetry to see whether it is awaited elsewhere.
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
"\bflushTelemetry\b|\bprocess\.once\b|\bprocess\.on\b|\bprocess\.exit\b" src test . || trueRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 23827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List candidate files that may wire shutdown handlers.
git ls-files | rg '(^|/)(capture|telemetry|signal|shutdown|process).*\.(ts|js|mjs|cjs)$|package\.json$|README\.md$'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section of capture.ts and locate the definitions it depends on.
wc -l src/utils/capture.ts
echo '--- capture.ts (260-330) ---'
sed -n '260,330p' src/utils/capture.ts
echo '--- nearby symbol definitions ---'
rg -n "function flushTelemetry|const flushTelemetry|let pendingCaptures|sendToTelemetryProxy" src/utils/capture.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2812
Flush telemetry on the real shutdown paths
beforeExit won’t run for the direct process.exit() and signal-driven exits in this codebase, so shutdown telemetry can still be dropped. Call and await flushTelemetry() from the SIGINT/SIGTERM/uncaughtException/unhandledRejection handlers instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/capture.ts` around lines 280 - 282, Replace the beforeExit-only
telemetry flush in the shutdown handling with awaited flushTelemetry() calls in
the SIGINT, SIGTERM, uncaughtException, and unhandledRejection handlers,
ensuring each handler flushes before completing its exit or error path. Remove
the beforeExit registration if it is no longer needed.
1 participant
Summary
flushTelemetry()with a bounded two-second default timeoutbeforeExitcaptureBaseGA4 transport and its duplicated property-building pathRoot cause
The live
capture()path detached its async send without retaining the promise, so events emitted immediately before process exit could be dropped. The same module also retained a separate 200-line telemetry implementation with no callers.Impact
Short-lived shutdown paths now get a bounded opportunity to finish telemetry without delaying normal tool calls or allowing telemetry failures to affect functionality. Removing the dead transport leaves the existing proxy path as the single implementation.
Fixes #504.
Fixes #505.
Validation
npm run buildnode test/test-telemetry-handling.jsgit diff --checkSummary by CodeRabbit