Skip to content

fix: drain telemetry on shutdown #573

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.

Sign up for GitHub

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

Open
Soumyajit2288 wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −225
Open

fix: drain telemetry on shutdown #573

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
254 changes: 29 additions & 225 deletions src/utils/capture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,227 +88,9 @@ export function sanitizeError(error: any): { message: string, code?: string } {
};
}

/**
* Send an event to telemetry
* @param event Event name
* @param properties Optional event properties
*/
// TODO(cleanup): captureBase is now dead code — no caller remains after the GA
// removal (only referenced in a comment). It still carries the full GA4-flavored
// send path. Remove it, or repurpose it as the shared proxy transport.
export const captureBase = async (captureURL: string, event: string, properties?: any) => {
try {
// Env kill-switch takes precedence over config (tests/CI).
if (isTelemetryDisabledByEnv()) {
return;
}

// Check if telemetry is enabled in config (defaults to true if not set)
const telemetryEnabled = await configManager.getValue('telemetryEnabled');

// If telemetry is explicitly disabled or GA credentials are missing, don't send
if (isTelemetryDisabledValue(telemetryEnabled) || !captureURL) {
return;
}

// Get or create the client ID if not already initialized
if (uniqueUserId === 'unknown') {
uniqueUserId = await configManager.getOrCreateClientId();
}

// Get current client information for all events
// For remote calls, attribute to the originating remote client (carried on
// the tool call) instead of the device's local currentClient.
const effectiveClient =
currentCallIsRemote && currentRemoteClient ? currentRemoteClient : currentClient;
let clientContext = {};
if (effectiveClient) {
clientContext = {
client_name: effectiveClient.name,
client_version: effectiveClient.version,
};
}

// Track if user saw onboarding page
const sawOnboardingPage = await configManager.getValue('sawOnboardingPage');
if (sawOnboardingPage !== undefined) {
clientContext = { ...clientContext, saw_onboarding_page: sawOnboardingPage };
}

// Create a deep copy of properties to avoid modifying the original objects
// This ensures we don't alter error objects that are also returned to the AI
let sanitizedProperties;
try {
sanitizedProperties = properties ? JSON.parse(JSON.stringify(properties)) : {};
} catch (e) {
sanitizedProperties = {}
}

// Sanitize error objects if present
if (sanitizedProperties.error) {
// Handle different types of error objects
if (typeof sanitizedProperties.error === 'object' && sanitizedProperties.error !== null) {
const sanitized = sanitizeError(sanitizedProperties.error);
sanitizedProperties.error = sanitized.message;
if (sanitized.code) {
sanitizedProperties.errorCode = sanitized.code;
}
} else if (typeof sanitizedProperties.error === 'string') {
sanitizedProperties.error = sanitizeError(sanitizedProperties.error).message;
}
}

// Remove any properties that might contain paths
const sensitiveKeys = ['path', 'filePath', 'directory', 'file_path', 'sourcePath', 'destinationPath', 'fullPath', 'rootPath'];
for (const key of Object.keys(sanitizedProperties)) {
const lowerKey = key.toLowerCase();
if (sensitiveKeys.some(sensitiveKey => lowerKey.includes(sensitiveKey)) &&
lowerKey !== 'fileextension') { // keep fileExtension as it's safe
delete sanitizedProperties[key];
}
}

// Is MCP installed with DXT
let isDXT: string = 'false';
if (process.env.MCP_DXT) {
isDXT = 'true';
}

// Is MCP running in a container - use robust detection
const { getSystemInfo } = await import('./system-info.js');
const systemInfo = getSystemInfo();
const isContainer: string = systemInfo.docker.isContainer ? 'true' : 'false';
const containerType: string = systemInfo.docker.containerType || 'none';
const orchestrator: string = systemInfo.docker.orchestrator || 'none';

// Add container metadata (with privacy considerations)
let containerName: string = 'none';
let containerImage: string = 'none';

if (systemInfo.docker.isContainer && systemInfo.docker.containerEnvironment) {
const env = systemInfo.docker.containerEnvironment;

// Container name - sanitize to remove potentially sensitive info
if (env.containerName) {
// Keep only alphanumeric chars, dashes, and underscores
// Remove random IDs and UUIDs for privacy
containerName = env.containerName
.replace(/[0-9a-f]{8,}/gi, 'ID') // Replace long hex strings with 'ID'
.replace(/[0-9]{8,}/g, 'ID') // Replace long numeric IDs with 'ID'
.substring(0, 50); // Limit length
}

// Docker image - sanitize registry info for privacy
if (env.dockerImage) {
// Remove registry URLs and keep just image:tag format
containerImage = env.dockerImage
.replace(/^[^/]+\/[^/]+\//, '') // Remove registry.com/namespace/ prefix
.replace(/^[^/]+\//, '') // Remove simple registry.com/ prefix
.replace(/@sha256:.*$/, '') // Remove digest hashes
.substring(0, 100); // Limit length
}
}

// Detect if we're running through Smithery at runtime
let runtimeSource: string = 'unknown';
const processArgs = process.argv.join(' ');
try {
if (processArgs.includes('@smithery/cli') || processArgs.includes('smithery')) {
runtimeSource = 'smithery-runtime';
} else if (processArgs.includes('npx')) {
runtimeSource = 'npx-runtime';
} else {
runtimeSource = 'direct-runtime';
}
} catch (error) {
// Ignore detection errors
}

// Prepare standard properties
const baseProperties = {
timestamp: new Date().toISOString(),
platform: platform(),
isContainer,
containerType,
orchestrator,
containerName,
containerImage,
runtimeSource,
isDXT,
app_version: VERSION,
engagement_time_msec: "100"
};

// Combine with sanitized properties and client context
const eventProperties = {
...baseProperties,
...clientContext,
// Attribute events to the remote path when the in-flight tool call
// came from a remote device. Placed before sanitizedProperties so an
// explicit `remote` passed by the caller (e.g. captureRemote) wins.
...(currentCallIsRemote ? { remote: String(true) } : {}),
...sanitizedProperties
};

// Prepare telemetry payload
const payload = {
client_id: uniqueUserId,
non_personalized_ads: false,
timestamp_micros: Date.now() * 1000,
events: [{
name: event,
params: eventProperties
}]
};

// Send data to telemetry endpoint
const postData = JSON.stringify(payload);

const options = {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'Content-Length': Buffer.byteLength(postData)
}
};

const req = https.request(captureURL, options, (res) => {
// Response handling (optional)
let data = '';
res.on('data', (chunk) => {
data += chunk;
});

res.on('end', () => {
const success = res.statusCode === 200 || res.statusCode === 204;
if (!success) {
// Optional debug logging
// console.debug(`Telemetry tracking error: ${res.statusCode} ${data}`);
}
});
});

req.on('error', (error) => {
// Silently fail - we don't want analytics issues to break functionality
});

// Set timeout to prevent blocking the app
req.setTimeout(3000, () => {
req.destroy();
});

// Send data
req.write(postData);
req.end();

} catch (error) {
// Silently fail - we don't want analytics issues to break functionality
}
};

/**
* Build the standard event properties used by the telemetry proxy.
* Extracted from captureBase so both paths get identical data.
* Shared property builder for the live telemetry proxy path.
*/
const buildEventProperties = async (properties?: any) => {
if (uniqueUserId === 'unknown') {
Expand Down Expand Up @@ -474,25 +256,47 @@ const postTelemetryPayload = async (endpoint: string, payload: string): Promise<
});
};

// TODO(behavior): capture() is now fire-and-forget — every `await capture(...)`
// call site resolves before the network send completes. Fine for the long-running
// MCP server, but events fired right before process exit (e.g. opt-out, feedback)
// can be silently dropped. If we need delivery guarantees on short-lived paths,
// expose an awaitable variant or flush-before-exit hook.
// capture() stays fire-and-forget during normal operation; pending sends are
// tracked so shutdown paths can drain them without blocking indefinitely.
const pendingCaptures = new Set<Promise<void>>();

export async function flushTelemetry(timeoutMs = 2000): Promise<void> {
if (pendingCaptures.size === 0) return;

let timeoutHandle: NodeJS.Timeout | undefined;
try {
await Promise.race([
Promise.allSettled([...pendingCaptures]),
new Promise<void>(resolve => {
timeoutHandle = setTimeout(resolve, timeoutMs);
timeoutHandle.unref?.();
})
]);
} finally {
if (timeoutHandle) clearTimeout(timeoutHandle);
}
}

process.once('beforeExit', () => {
void flushTelemetry();
});
Comment on lines +280 to +282

@coderabbitai coderabbitai Bot Jul 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 . || true

Repository: 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.ts

Repository: 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.


export const capture = async (event: string, properties?: any) => {
// Tool calls fired programmatically by the widget UIs must produce zero
// telemetry — drop every event raised while serving one.
if (isInsideUiOriginCall()) {
return;
}
void (async () => {
const pending = (async () => {
try {
const eventProperties = await buildEventProperties(properties);
await sendToTelemetryProxy(event, eventProperties);
} catch {
// Silent fail — telemetry should never break functionality
}
})();
pendingCaptures.add(pending);
void pending.finally(() => pendingCaptures.delete(pending));
}

export const capture_call_tool = capture;
Expand Down