aksd: fix session-start drop on mid-session telemetry grant

Route trackSessionStart through emitInternal so it bypasses the
consent gate the same way emitConsentEvent does; its only production
caller (initTelemetry) already runs inside a consent transition and is
gated on telemetryEnabled, so the ordinary-event gate was silently
dropping session-start on every mid-session opt-in.

Also resolve (rather than abandon) the app-version promise on
TelemetryBoot's effect cleanup, so a StrictMode double-mount cannot
leave the promise pending forever and silently disable telemetry, and
reword a consent.ts comment that overstated what revoke actually
clears.

Add an integration test composing the real consent.ts against the
real index.ts, which fails without the trackSessionStart fix.

Author: gambtho

plugins/aks-desktop/src/components/TelemetryBoot.tsx

@@ -92,11 +92,15 @@ function startAppVersionResolution(): { promise: Promise<string>; cleanup: () =>
     }, APP_CONFIG_RESPONSE_TIMEOUT_MS);
   }
 
+  // Resolves rather than abandoning the promise: under a StrictMode
+  // double-invoke, effect B (the consent effect below) can already be
+  // awaiting this promise when effect A's cleanup fires. Leaving it
+  // unsettled would hang that await forever, and because
+  // previousEnabledRef is no longer null on the re-run, the re-invoked
+  // effect A/B pair would never call initTelemetry either — silently
+  // disabling telemetry for the whole session.
   const cleanup = () => {
-    settled = true;
-    window.clearTimeout(requestTimer);
-    window.clearTimeout(fallbackTimer);
-    stopListening();
+    settle('unknown');
   };
   return { promise, cleanup };
 }
@@ -106,7 +110,9 @@ function startAppVersionResolution(): { promise: Promise<string>; cleanup: () =>
  * for the rest of the process — toggling the setting takes effect
  * immediately via `grantConsent`/`revokeConsent`, no restart required.
  * Renders nothing. StrictMode double-mount is handled by initTelemetry's
- * internal initAttempted guard.
+ * internal initAttempted guard, plus resolving (rather than abandoning)
+ * the app-version promise on cleanup so a re-invoked mount can still
+ * proceed to call initTelemetry at all.
  */
 export default function TelemetryBoot(): null {
   const { enabled } = useTelemetryConfig();

plugins/aks-desktop/src/telemetry/consent.integration.test.ts

@@ -0,0 +1,156 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache 2.0.
+
+// Every other test mocks one side of the consent/index seam: consent.test.ts
+// mocks `./index` wholesale, and index.test.ts never composes with
+// `consent.ts`. This file composes the REAL consent.ts against the REAL
+// index.ts, mocking only the App Insights SDK transport (the same
+// vi.hoisted pattern index.test.ts uses), and asserts on the event names
+// that actually reach that transport across a full revoke -> grant cycle.
+
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+const aiMocks = vi.hoisted(() => {
+  const trackEvent = vi.fn();
+  const loadAppInsights = vi.fn();
+  const addTelemetryInitializer = vi.fn();
+  const unload = vi.fn();
+  const config: Record<string, unknown> = {};
+  const context = { location: {}, user: {} };
+  const ApplicationInsightsCtor = vi.fn().mockImplementation(() => ({
+    trackEvent,
+    loadAppInsights,
+    addTelemetryInitializer,
+    unload,
+    config,
+    context,
+  }));
+  return {
+    trackEvent,
+    loadAppInsights,
+    addTelemetryInitializer,
+    unload,
+    config,
+    context,
+    ApplicationInsightsCtor,
+  };
+});
+const {
+  trackEvent,
+  loadAppInsights,
+  addTelemetryInitializer,
+  unload,
+  config,
+  context,
+  ApplicationInsightsCtor,
+} = aiMocks;
+
+vi.mock('@microsoft/applicationinsights-web', () => ({
+  ApplicationInsights: aiMocks.ApplicationInsightsCtor,
+}));
+
+const registerMock = vi.hoisted(() => ({ registerHeadlampEventCallback: vi.fn() }));
+vi.mock('@kinvolk/headlamp-plugin/lib', async () => {
+  const actual = await vi.importActual<any>('@kinvolk/headlamp-plugin/lib');
+  return { ...actual, registerHeadlampEventCallback: registerMock.registerHeadlampEventCallback };
+});
+
+// Imported AFTER mocks are registered. Both the orchestrator (consent.ts)
+// and the producers/gate (index.ts) are real — nothing under test here is
+// mocked.
+import { grantConsent, revokeConsent } from './consent';
+import { __resetForTests, initTelemetry, setTelemetryEnabled, trackFeature } from './index';
+
+const VALID_INSTALL_ID = '11111111-1111-4111-8111-111111111111';
+const SESSION_PROPS = {
+  appVersion: '1.0.0',
+  locale: 'en-US',
+  os: 'linux' as const,
+  arch: 'x64',
+  electronVersion: '32.1.0',
+  headlampVersion: '0.30.0',
+};
+
+function realInitialize(): void {
+  initTelemetry({
+    connectionString: 'InstrumentationKey=test',
+    installId: VALID_INSTALL_ID,
+    sessionProps: SESSION_PROPS,
+  });
+}
+
+function eventNames(): string[] {
+  return trackEvent.mock.calls.map(([envelope]) => envelope.name);
+}
+
+beforeEach(() => {
+  trackEvent.mockClear();
+  loadAppInsights.mockClear();
+  addTelemetryInitializer.mockClear();
+  unload.mockClear();
+  for (const key of Object.keys(config)) delete config[key];
+  ApplicationInsightsCtor.mockReset();
+  ApplicationInsightsCtor.mockImplementation(() => ({
+    trackEvent,
+    loadAppInsights,
+    addTelemetryInitializer,
+    unload,
+    config,
+    context,
+  }));
+  context.location = {};
+  context.user = {};
+  registerMock.registerHeadlampEventCallback.mockClear();
+  __resetForTests();
+  setTelemetryEnabled(true);
+  realInitialize();
+  trackEvent.mockClear();
+});
+
+afterEach(() => {
+  // Don't call vi.restoreAllMocks(): it would wipe the ApplicationInsightsCtor
+  // mockImplementation re-applied in beforeEach.
+});
+
+describe('consent <-> index integration: revoke -> grant cycle', () => {
+  it('revoke delivers the revoked event and suppresses an ordinary event during the transition', async () => {
+    const revokePromise = revokeConsent();
+    // An ordinary event fired mid-transition (gate closed by beginConsentTransition)
+    // must not reach the transport.
+    trackFeature({ feature: 'headlamp.logs', status: 'opened' });
+    await revokePromise;
+
+    expect(eventNames()).toContain('headlamp.telemetry-consent');
+    expect(eventNames()).not.toContain('headlamp.feature');
+    const consentCall = trackEvent.mock.calls.find(
+      ([e]) => e.name === 'headlamp.telemetry-consent'
+    );
+    expect(consentCall?.[0].properties.consent).toBe('revoked');
+  });
+
+  it('grant delivers both session-start and the granted consent event', async () => {
+    // Simulate a real disable (as revokeConsent would have left things),
+    // then grant. `initialize` here is the real initTelemetry, exactly as
+    // TelemetryBoot wires it up.
+    await revokeConsent();
+    trackEvent.mockClear();
+
+    await grantConsent(() => {
+      realInitialize();
+    });
+
+    // This is the assertion that would fail before the session-start fix:
+    // grantConsent closes the gate via beginConsentTransition before calling
+    // initialize, and initTelemetry's trackSessionStart used to route
+    // through the gated `emit`, so it was dropped every time. With
+    // trackSessionStart routed through emitInternal (bypassing the gate,
+    // the same carve-out emitConsentEvent already takes), it reaches the
+    // transport even while the gate is still closed during initialize().
+    expect(eventNames()).toContain('headlamp.session-start');
+    expect(eventNames()).toContain('headlamp.telemetry-consent');
+    const consentCall = trackEvent.mock.calls.find(
+      ([e]) => e.name === 'headlamp.telemetry-consent'
+    );
+    expect(consentCall?.[0].properties.consent).toBe('granted');
+  });
+});

plugins/aks-desktop/src/telemetry/consent.ts

@@ -65,10 +65,13 @@ export async function revokeConsent(): Promise<void> {
     // would kill the freshly (re-)enabled SDK. Leave state untouched.
     return;
   }
-  // Revoke clears everything derived from an enabled session. The crash
-  // marker (workstream 2) and the feature timing map (workstream 3) must
-  // also be cleared here once they exist, or a later grant would report
-  // activity belonging to an opted-out period.
+  // setTelemetryEnabled(false) clears pendingEvents and unloads the SDK,
+  // but leaves session-derived state such as emittedShapeFor and
+  // errorCounts untouched across the opted-out period. The crash marker
+  // (workstream 2) and the feature timing map (workstream 3) will need
+  // explicit clearing calls added here once they exist, or a later grant
+  // would report activity belonging to an opted-out period — this
+  // function does not already clear session-derived state for them.
   setTelemetryEnabled(false);
   endConsentTransition(gen);
 }

plugins/aks-desktop/src/telemetry/index.test.ts

@@ -667,6 +667,18 @@ describe('consent transition primitives', () => {
     expect(trackEvent).toHaveBeenCalledWith(expect.objectContaining({ name: 'headlamp.feature' }));
   });
 
+  it('trackSessionStart still emits while the consent gate is closed', () => {
+    // trackSessionStart is initTelemetry's last step, and the grant side of
+    // a consent transition closes the gate before calling initTelemetry.
+    // If trackSessionStart routed through the gated `emit`, every
+    // mid-session grant would silently drop session-start.
+    beginConsentTransition();
+    trackSessionStart(SESSION_PROPS);
+    expect(trackEvent).toHaveBeenCalledWith(
+      expect.objectContaining({ name: 'headlamp.session-start' })
+    );
+  });
+
   it('emitConsentEvent is a no-op when telemetry is disabled', () => {
     setTelemetryEnabled(false);
     emitConsentEvent('revoked');

plugins/aks-desktop/src/telemetry/index.ts

@@ -369,8 +369,16 @@ export function emitConsentEvent(consent: TelemetryConsent): void {
   emitInternal('headlamp.telemetry-consent', { consent: safeConsent });
 }
 
+/**
+ * Bypasses the ordinary-event gate. Its one production call site is
+ * initTelemetry, which runs inside a consent transition (the grant side
+ * closes the gate before re-initializing) and is already gated on
+ * telemetryEnabled above. Routing this through `emit` instead would drop
+ * session-start on every mid-session grant, since the gate does not reopen
+ * until after initTelemetry returns. Do not "fix" this back to `emit`.
+ */
 export function trackSessionStart(p: SessionStartProps): void {
-  emit('headlamp.session-start', {
+  emitInternal('headlamp.session-start', {
     appVersion: p.appVersion,
     locale: localeLanguage(p.locale),
     os: p.os,