Skip to content

fix: emit telemetry eagerly in dev --logs to avoid SIGINT race #1372

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

Closed
Hweinstock wants to merge 1 commit into from
+49 −43
Closed

fix: emit telemetry eagerly in dev --logs to avoid SIGINT race #1372

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
92 changes: 49 additions & 43 deletions src/cli/commands/dev/command.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,51 +381,57 @@ export const registerDev = (program: Command) => {
console.log(`Log: ${logger.getRelativeLogPath()}`);
console.log(`Press Ctrl+C to stop\n`);

const devResult = await withCommandRunTelemetry(
'dev',
{
dev_action: 'server' as const,
ui_mode: 'terminal' as const,
has_stream: false,
agent_protocol: standardize(AgentProtocol, (config.protocol ?? 'http').toLowerCase()),
invoke_count: 0,
},
async (): Promise<Result> => {
await new Promise<void>((resolve, reject) => {
const devCallbacks = {
onLog: (level: string, msg: string) => {
const prefix = level === 'error' ? '❌' : level === 'warn' ? '⚠️' : '→';
console.log(`${prefix} ${msg}`);
logger.log(msg, level === 'error' ? 'error' : 'info');
},
onExit: (code: number | null) => {
console.log(`\nServer exited with code ${code ?? 0}`);
logger.finalize(code === 0);
if (code !== 0 && code !== null) {
reject(new Error(`Server exited with code ${code}`));
} else {
resolve();
}
},
};

const server = createDevServer(config, {
port: actualPort,
envVars: mergedEnvVars,
callbacks: devCallbacks,
});
server.start().catch(reject);

process.once('SIGINT', () => {
console.log('\nStopping server...');
collector?.stop();
server.kill();
});
// Emit telemetry eagerly before the blocking server loop.
// The global SIGINT handler calls process.exit(0) synchronously,
// which would prevent withCommandRunTelemetry from ever flushing.
{
const client = await TelemetryClientAccessor.get().catch(() => undefined);
if (client) {
client.emit('cli.command_run', 0, {
command_group: 'dev',
command: 'dev',
exit_reason: 'success',
Copy link
Copy Markdown

@agentcore-cli-automation agentcore-cli-automation May 22, 2026

Choose a reason for hiding this comment

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

Hardcoding exit_reason: 'success' here loses the failure-telemetry behavior the previous withCommandRunTelemetry wrapper provided. If server.start() rejects (e.g. binary not found, port grabbed between findAvailablePort and server.start), or onExit fires with a non-zero code, the await new Promise(...) below throws, the outer catch (error) at line 513 prints the error and exits 1 — but telemetry has already recorded this as a successful run. Previously trackCommandRun/classifyError would have emitted exit_reason: 'failure' with error_name and error_source.

A few ways to fix:

  • (a) Move the eager success emit to just before the await new Promise and wrap server.start() / startup in a try-block such that failures emit a separate failure event (and skip the success emit).
  • (b) Don't emit eagerly — instead, refactor so the dev command's SIGINT handler can perform async cleanup (e.g. teach setupGlobalCleanup to await registered async cleanups before process.exit), then keep withCommandRunTelemetry and get correct success/failure telemetry for free. Bigger change but fixes the root cause and would also help browser-mode.
  • (c) Accept the regression and document it in a comment, mirroring the note above runBrowserMode. Least work but means we lose error classification on dev-server startup failures.

My preference would be (a) since it's localized and preserves the existing failure-telemetry contract.

dev_action: 'server',
ui_mode: 'terminal',
has_stream: false,
agent_protocol: standardize(AgentProtocol, (config.protocol ?? 'http').toLowerCase()),
invoke_count: 0,
});
return { success: true as const };
await client.flush();
}
);
if (!devResult.success) throw devResult.error;
}
Copy link
Copy Markdown

@agentcore-cli-automation agentcore-cli-automation May 22, 2026

Choose a reason for hiding this comment

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

No test coverage for this fix. integ-tests/dev-server.test.ts already spawns agentcore dev --logs but doesn't pass telemetry.env and only asserts telemetry from the separate dev <prompt> invoke calls, so nothing currently verifies that the SIGINT fix works or guards against it regressing. Could you extend that test to spawn with telemetry.env, send SIGINT, wait for exit, and telemetry.assertMetricEmitted({ command: 'dev', dev_action: 'server', ui_mode: 'terminal', exit_reason: 'success' })?


await new Promise<void>((resolve, reject) => {
const devCallbacks = {
onLog: (level: string, msg: string) => {
const prefix = level === 'error' ? '❌' : level === 'warn' ? '⚠️' : '→';
console.log(`${prefix} ${msg}`);
logger.log(msg, level === 'error' ? 'error' : 'info');
},
onExit: (code: number | null) => {
console.log(`\nServer exited with code ${code ?? 0}`);
logger.finalize(code === 0);
if (code !== 0 && code !== null) {
reject(new Error(`Server exited with code ${code}`));
} else {
resolve();
}
},
};

const server = createDevServer(config, {
port: actualPort,
envVars: mergedEnvVars,
callbacks: devCallbacks,
});
server.start().catch(reject);

process.once('SIGINT', () => {
console.log('\nStopping server...');
collector?.stop();
server.kill();
});
});
process.exit(0);
}

Expand Down
Loading