fix(telemetry): use SDK session integration instead of manual management (#232)

## Summary

- Move session tracking from manual `startSession`/`endSession`/`flush`
in `withTelemetry()` to the SDK's built-in `processSessionIntegration` +
a `beforeExit` handler
- The manual approach missed unhandled rejections and other paths that
bypass `withTelemetry`'s try/catch
- The SDK already starts a session during `Sentry.init()` and auto-marks
crashes for unhandled exceptions via `mechanism.handled: false`

## Changes

**`src/lib/telemetry.ts`**
- Remove `Sentry.startSession()`, `captureSession()`, `endSession()`,
and `client.flush()` from `withTelemetry()`
- Add `createBeforeExitHandler(client)` — ends OK sessions the SDK skips
(clean exit → `'exited'`), flushes pending events, includes re-entry
guard
- Extract `markSessionCrashed()` — checks both current scope and
isolation scope since `processSessionIntegration` stores the session on
the isolation scope
- Register the `beforeExit` handler in `initSentry()` when telemetry is
enabled

**`test/lib/telemetry-session.test.ts`** (new)
- 5 tests for `createBeforeExitHandler` (OK session, non-OK session, no
session, re-entry guard, flush)
- 4 tests for `markSessionCrashed` (current scope, isolation scope, no
session, scope priority)
- Separate file because Sentry scope mocking poisons the SDK's global
state and breaks other tests in the same worker

Author: BYK

src/lib/telemetry.ts

@@ -49,11 +49,36 @@ async function initTelemetryContext(): Promise<void> {
   }
 }
 
+/**
+ * Mark the active session as crashed.
+ *
+ * Checks both current scope and isolation scope since processSessionIntegration
+ * stores the session on the isolation scope. Called when a command error
+ * propagates through withTelemetry — the SDK auto-marks crashes for truly
+ * uncaught exceptions (mechanism.handled === false), but command errors need
+ * explicit marking.
+ *
+ * @internal Exported for testing
+ */
+export function markSessionCrashed(): void {
+  const session =
+    Sentry.getCurrentScope().getSession() ??
+    Sentry.getIsolationScope().getSession();
+  if (session) {
+    session.status = "crashed";
+  }
+}
+
 /**
  * Wrap CLI execution with telemetry tracking.
  *
- * Creates a Sentry session and span for the command execution.
- * Captures any unhandled exceptions and reports them.
+ * Creates a Sentry span for the command execution and captures exceptions.
+ * Session lifecycle is managed by the SDK's processSessionIntegration
+ * (started during Sentry.init) and a beforeExit handler (registered in
+ * initSentry) that ends healthy sessions and flushes events. This ensures
+ * sessions are reliably tracked even for unhandled rejections and other
+ * paths that bypass this function's try/catch.
+ *
  * Telemetry can be disabled via SENTRY_CLI_NO_TELEMETRY=1 env var.
  *
  * @param callback - The CLI execution function to wrap, receives the span for naming
@@ -71,9 +96,6 @@ export async function withTelemetry<T>(
   // Initialize user and instance context
   await initTelemetryContext();
 
-  Sentry.startSession();
-  Sentry.captureSession();
-
   try {
     return await Sentry.startSpanManual(
       { name: "cli.command", op: "cli.command", forceTransaction: true },
@@ -92,31 +114,47 @@ export async function withTelemetry<T>(
       e instanceof AuthError && e.reason === "not_authenticated";
     if (!isExpectedAuthState) {
       Sentry.captureException(e);
-      const session = Sentry.getCurrentScope().getSession();
-      if (session) {
-        session.status = "crashed";
-      }
+      markSessionCrashed();
     }
     throw e;
-  } finally {
-    Sentry.endSession();
-    // Flush with a timeout to ensure events are sent before process exits
-    try {
-      await client.flush(3000);
-    } catch {
-      // Ignore flush errors - telemetry should never block CLI
-    }
   }
 }
 
 /**
- * Initialize Sentry for telemetry.
+ * Create a beforeExit handler that ends healthy sessions and flushes events.
  *
- * @param enabled - Whether telemetry is enabled
- * @returns The Sentry client, or undefined if initialization failed
+ * The SDK's processSessionIntegration only ends non-OK sessions (crashed/errored).
+ * This handler complements it by ending OK sessions (clean exit → 'exited')
+ * and flushing pending events. Includes a re-entry guard since flush is async
+ * and causes beforeExit to re-fire when complete.
+ *
+ * @param client - The Sentry client to flush
+ * @returns A handler function for process.on("beforeExit")
  *
  * @internal Exported for testing
  */
+export function createBeforeExitHandler(client: Sentry.BunClient): () => void {
+  let isFlushing = false;
+  return () => {
+    if (isFlushing) {
+      return;
+    }
+    isFlushing = true;
+
+    const session = Sentry.getIsolationScope().getSession();
+    if (session?.status === "ok") {
+      Sentry.endSession();
+    }
+
+    // Flush pending events before exit. Convert PromiseLike to Promise
+    // for proper error handling. The async work causes beforeExit to
+    // re-fire when complete, which the isFlushing guard handles.
+    Promise.resolve(client.flush(3000)).catch(() => {
+      // Ignore flush errors — telemetry should never block CLI exit
+    });
+  };
+}
+
 /**
  * Integrations to exclude for CLI.
  * These add overhead without benefit for short-lived CLI processes.
@@ -128,6 +166,17 @@ const EXCLUDED_INTEGRATIONS = new Set([
   "Modules", // Lists all loaded modules - unnecessary for CLI telemetry
 ]);
 
+/** Current beforeExit handler, tracked so it can be replaced on re-init */
+let currentBeforeExitHandler: (() => void) | null = null;
+
+/**
+ * Initialize Sentry for telemetry.
+ *
+ * @param enabled - Whether telemetry is enabled
+ * @returns The Sentry client, or undefined if initialization failed
+ *
+ * @internal Exported for testing
+ */
 export function initSentry(enabled: boolean): Sentry.BunClient | undefined {
   const environment = process.env.NODE_ENV ?? "development";
 
@@ -187,6 +236,23 @@ export function initSentry(enabled: boolean): Sentry.BunClient | undefined {
 
     // Tag whether targeting self-hosted Sentry (not SaaS)
     Sentry.setTag("is_self_hosted", !isSentrySaasUrl(getSentryBaseUrl()));
+
+    // End healthy sessions and flush events when the event loop drains.
+    // The SDK's processSessionIntegration starts a session during init and
+    // registers its own beforeExit handler that ends non-OK (crashed/errored)
+    // sessions. We complement it by ending OK sessions (clean exit → 'exited')
+    // and flushing pending events. This covers unhandled rejections and other
+    // paths that bypass withTelemetry's try/catch.
+    // Ref: https://nodejs.org/api/process.html#event-beforeexit
+    //
+    // Replace previous handler on re-init (e.g., auto-login retry calls
+    // withTelemetry → initSentry twice) to avoid duplicate handlers with
+    // independent re-entry guards and stale client references.
+    if (currentBeforeExitHandler) {
+      process.removeListener("beforeExit", currentBeforeExitHandler);
+    }
+    currentBeforeExitHandler = createBeforeExitHandler(client);
+    process.on("beforeExit", currentBeforeExitHandler);
   }
 
   return client;

test/lib/telemetry-session.test.ts

@@ -0,0 +1,213 @@
+/**
+ * Telemetry Session Tests
+ *
+ * Tests for session lifecycle management:
+ * - beforeExit handler (createBeforeExitHandler)
+ * - Session crash marking (markSessionCrashed)
+ *
+ * These tests are in a separate file because they mock Sentry's scope methods
+ * (getCurrentScope, getIsolationScope) which conflicts with the SDK's internal
+ * scope machinery. Running in a separate Bun test worker prevents interference
+ * with other telemetry tests.
+ */
+
+import { afterEach, describe, expect, spyOn, test } from "bun:test";
+// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
+import * as Sentry from "@sentry/bun";
+import {
+  createBeforeExitHandler,
+  markSessionCrashed,
+} from "../../src/lib/telemetry.js";
+
+describe("createBeforeExitHandler", () => {
+  /**
+   * Create a minimal mock BunClient for testing the beforeExit handler.
+   * Only needs flush() since that's all the handler uses from the client.
+   */
+  function createMockClient(): Sentry.BunClient {
+    return {
+      flush: () => Promise.resolve(true),
+    } as unknown as Sentry.BunClient;
+  }
+
+  test("ends OK session on beforeExit", () => {
+    const handler = createBeforeExitHandler(createMockClient());
+
+    const mockSession = { status: "ok", errors: 0 };
+    const getIsolationScopeSpy = spyOn(
+      Sentry,
+      "getIsolationScope"
+    ).mockReturnValue({
+      getSession: () => mockSession,
+    } as unknown as Sentry.Scope);
+    // Mock endSession to prevent it from calling through to real SDK internals
+    const endSessionSpy = spyOn(Sentry, "endSession").mockImplementation(
+      () => null
+    );
+
+    handler();
+
+    expect(endSessionSpy).toHaveBeenCalled();
+
+    getIsolationScopeSpy.mockRestore();
+    endSessionSpy.mockRestore();
+  });
+
+  test("does not end non-OK session on beforeExit (SDK handles it)", () => {
+    const handler = createBeforeExitHandler(createMockClient());
+
+    const mockSession = { status: "crashed", errors: 1 };
+    const getIsolationScopeSpy = spyOn(
+      Sentry,
+      "getIsolationScope"
+    ).mockReturnValue({
+      getSession: () => mockSession,
+    } as unknown as Sentry.Scope);
+    const endSessionSpy = spyOn(Sentry, "endSession").mockImplementation(
+      () => null
+    );
+
+    handler();
+
+    expect(endSessionSpy).not.toHaveBeenCalled();
+
+    getIsolationScopeSpy.mockRestore();
+    endSessionSpy.mockRestore();
+  });
+
+  test("does not end session when no session exists", () => {
+    const handler = createBeforeExitHandler(createMockClient());
+
+    const getIsolationScopeSpy = spyOn(
+      Sentry,
+      "getIsolationScope"
+    ).mockReturnValue({
+      getSession: () => null,
+    } as unknown as Sentry.Scope);
+    const endSessionSpy = spyOn(Sentry, "endSession").mockImplementation(
+      () => null
+    );
+
+    handler();
+
+    expect(endSessionSpy).not.toHaveBeenCalled();
+
+    getIsolationScopeSpy.mockRestore();
+    endSessionSpy.mockRestore();
+  });
+
+  test("re-entry guard prevents double flush", () => {
+    const handler = createBeforeExitHandler(createMockClient());
+
+    const mockSession = { status: "ok", errors: 0 };
+    const getIsolationScopeSpy = spyOn(
+      Sentry,
+      "getIsolationScope"
+    ).mockReturnValue({
+      getSession: () => mockSession,
+    } as unknown as Sentry.Scope);
+    const endSessionSpy = spyOn(Sentry, "endSession").mockImplementation(
+      () => null
+    );
+
+    // Call twice
+    handler();
+    handler();
+
+    // endSession should only be called once due to re-entry guard
+    expect(endSessionSpy).toHaveBeenCalledTimes(1);
+
+    getIsolationScopeSpy.mockRestore();
+    endSessionSpy.mockRestore();
+  });
+
+  test("flushes client on beforeExit", () => {
+    const mockClient = createMockClient();
+    const flushSpy = spyOn(mockClient, "flush");
+    const handler = createBeforeExitHandler(mockClient);
+
+    const getIsolationScopeSpy = spyOn(
+      Sentry,
+      "getIsolationScope"
+    ).mockReturnValue({
+      getSession: () => null,
+    } as unknown as Sentry.Scope);
+
+    handler();
+
+    expect(flushSpy).toHaveBeenCalledWith(3000);
+
+    getIsolationScopeSpy.mockRestore();
+    flushSpy.mockRestore();
+  });
+});
+
+describe("markSessionCrashed", () => {
+  let getCurrentScopeSpy: ReturnType<typeof spyOn>;
+  let getIsolationScopeSpy: ReturnType<typeof spyOn>;
+
+  afterEach(() => {
+    getCurrentScopeSpy?.mockRestore();
+    getIsolationScopeSpy?.mockRestore();
+  });
+
+  test("marks session as crashed from current scope", () => {
+    const mockSession = { status: "ok", errors: 0 };
+
+    getCurrentScopeSpy = spyOn(Sentry, "getCurrentScope").mockReturnValue({
+      getSession: () => mockSession,
+    } as unknown as Sentry.Scope);
+    getIsolationScopeSpy = spyOn(Sentry, "getIsolationScope").mockReturnValue({
+      getSession: () => null,
+    } as unknown as Sentry.Scope);
+
+    markSessionCrashed();
+
+    expect(mockSession.status).toBe("crashed");
+  });
+
+  test("marks session as crashed from isolation scope when current scope has none", () => {
+    const mockSession = { status: "ok", errors: 0 };
+
+    getCurrentScopeSpy = spyOn(Sentry, "getCurrentScope").mockReturnValue({
+      getSession: () => null,
+    } as unknown as Sentry.Scope);
+    getIsolationScopeSpy = spyOn(Sentry, "getIsolationScope").mockReturnValue({
+      getSession: () => mockSession,
+    } as unknown as Sentry.Scope);
+
+    markSessionCrashed();
+
+    expect(mockSession.status).toBe("crashed");
+  });
+
+  test("does nothing when no session exists on either scope", () => {
+    getCurrentScopeSpy = spyOn(Sentry, "getCurrentScope").mockReturnValue({
+      getSession: () => null,
+    } as unknown as Sentry.Scope);
+    getIsolationScopeSpy = spyOn(Sentry, "getIsolationScope").mockReturnValue({
+      getSession: () => null,
+    } as unknown as Sentry.Scope);
+
+    // Should not throw
+    expect(() => markSessionCrashed()).not.toThrow();
+  });
+
+  test("prefers current scope session over isolation scope", () => {
+    const currentSession = { status: "ok", errors: 0 };
+    const isolationSession = { status: "ok", errors: 0 };
+
+    getCurrentScopeSpy = spyOn(Sentry, "getCurrentScope").mockReturnValue({
+      getSession: () => currentSession,
+    } as unknown as Sentry.Scope);
+    getIsolationScopeSpy = spyOn(Sentry, "getIsolationScope").mockReturnValue({
+      getSession: () => isolationSession,
+    } as unknown as Sentry.Scope);
+
+    markSessionCrashed();
+
+    // Current scope session should be marked, isolation scope left unchanged
+    expect(currentSession.status).toBe("crashed");
+    expect(isolationSession.status).toBe("ok");
+  });
+});