chore: pr comments

Author: joker23

packages/sdk/node-client/__tests__/NodeClient.bootstrap.test.ts

@@ -109,6 +109,25 @@ it('re-identifying with new bootstrap data replaces previous flags', async () =>
   expect(client.boolVariation('my-boolean-flag', false)).toBe(true);
 });
 
+it('warns that waitForNetworkResults is ignored when combined with bootstrap', async () => {
+  const client = createClient('client-side-id', { kind: 'user', key: 'bob' }, {
+    initialConnectionMode: 'offline',
+    sendEvents: false,
+    diagnosticOptOut: true,
+    localStoragePath: tmpRoot,
+    logger,
+  });
+
+  await client.start({ bootstrap: goodBootstrapData });
+  const result = await client.identify(
+    { kind: 'user', key: 'alice' },
+    { bootstrap: goodBootstrapData, waitForNetworkResults: true },
+  );
+
+  expect(result.status).toBe('completed');
+  expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('waitForNetworkResults'));
+});
+
 it('returns defaults when no bootstrap data is provided', async () => {
   const client = createClient('client-side-id', { kind: 'user', key: 'bob' }, {
     initialConnectionMode: 'offline',

packages/sdk/node-client/__tests__/NodeClient.dataSource.test.ts

@@ -2,6 +2,7 @@ import type { LDLogger } from '@launchdarkly/js-client-sdk-common';
 
 import { createClient } from '../src';
 import NodeDataManager from '../src/NodeDataManager';
+import { NodeClient } from '../src/NodeClient';
 import { makeMockPlatform, mockFetch } from './NodeClient.mocks';
 
 // Replace NodePlatform's constructor with one that returns the mock platform. Lets us
@@ -307,3 +308,101 @@ it('setConnectionMode streaming -> offline tears down the EventSource', async ()
 
   await client.close();
 });
+
+it('keeps event-sending state consistent with the mode under concurrent setConnectionMode', async () => {
+  const fakePlatform = makeMockPlatform({
+    requests: {
+      fetch: mockFetch('', 202),
+      createEventSource: jest.fn(() => ({
+        addEventListener: jest.fn(),
+        close: jest.fn(),
+        onclose: jest.fn(),
+        onerror: jest.fn(),
+        onopen: jest.fn(),
+        onretrying: jest.fn(),
+      })),
+      getEventSourceCapabilities: () => ({ readTimeout: true, headers: true, customMethod: true }),
+    },
+  });
+  NodePlatformMock.mockImplementationOnce(() => fakePlatform);
+
+  // Use the implementation directly so we can assert on the internal event-sending flag,
+  // which governs background (timer-driven) analytics delivery.
+  const client = new NodeClient(
+    'client-side-id',
+    { kind: 'user', key: 'bob' },
+    {
+      initialConnectionMode: 'streaming',
+      sendEvents: true,
+      diagnosticOptOut: true,
+      logger,
+    },
+  );
+
+  await client.start({ bootstrap: bootstrapData });
+
+  // Fire two transitions without awaiting between them. Without serialization the offline
+  // transition could settle while event-sending is left enabled.
+  const p1 = client.setConnectionMode('streaming');
+  const p2 = client.setConnectionMode('offline');
+  await Promise.all([p1, p2]);
+
+  expect(client.getConnectionMode()).toBe('offline');
+  expect(client.isOffline()).toBe(true);
+  // When offline, background analytics delivery must be disabled.
+  // eslint-disable-next-line no-underscore-dangle
+  expect((client as any)._eventSendingEnabled).toBe(false);
+
+  await client.close();
+});
+
+it('rejects identify called after close without waiting for the identify timeout', async () => {
+  const fakePlatform = makeMockPlatform();
+  NodePlatformMock.mockImplementationOnce(() => fakePlatform);
+
+  const client = createClient(
+    'client-side-id',
+    { kind: 'user', key: 'bob' },
+    {
+      initialConnectionMode: 'streaming',
+      sendEvents: false,
+      diagnosticOptOut: true,
+      logger,
+    },
+  );
+
+  await client.start({ bootstrap: bootstrapData });
+  await client.close();
+
+  const start = Date.now();
+  const result = await client.identify({ kind: 'user', key: 'alice' });
+  const elapsed = Date.now() - start;
+
+  expect(result.status).toBe('error');
+  // Should fail fast, not sit until the 5s identify timeout.
+  expect(elapsed).toBeLessThan(1000);
+});
+
+it('does not read cached flags when bootstrap is provided', async () => {
+  const fakePlatform = makeMockPlatform();
+  NodePlatformMock.mockImplementationOnce(() => fakePlatform);
+
+  const client = createClient(
+    'client-side-id',
+    { kind: 'user', key: 'bob' },
+    {
+      initialConnectionMode: 'streaming',
+      sendEvents: false,
+      diagnosticOptOut: true,
+      logger,
+    },
+  );
+
+  await client.start({ bootstrap: bootstrapData });
+
+  // Bootstrap and cache are mutually exclusive: cached flags must not be consulted (and so
+  // cannot overwrite) the freshly applied bootstrap data.
+  expect(fakePlatform.storage!.get).not.toHaveBeenCalled();
+
+  await client.close();
+});

packages/sdk/node-client/__tests__/NodeClient.events.test.ts

@@ -166,3 +166,63 @@ it('includes authorization and user-agent headers on the events request', async
 
   await client.close();
 });
+
+it('delivers events tracked across an offline transition once back online', async () => {
+  const fetchMock = mockFetch('', 202);
+  const fakePlatform = makeMockPlatform({
+    requests: {
+      fetch: fetchMock,
+      createEventSource: jest.fn(() => ({
+        addEventListener: jest.fn(),
+        close: jest.fn(),
+        onclose: jest.fn(),
+        onerror: jest.fn(),
+        onopen: jest.fn(),
+        onretrying: jest.fn(),
+      })),
+      getEventSourceCapabilities: () => ({ readTimeout: true, headers: true, customMethod: true }),
+    },
+  });
+  NodePlatformMock.mockImplementationOnce(() => fakePlatform);
+
+  const client = createClient(
+    'client-side-id',
+    { kind: 'user', key: 'bob' },
+    {
+      initialConnectionMode: 'streaming',
+      sendEvents: true,
+      diagnosticOptOut: true,
+      logger,
+    },
+  );
+
+  await client.start({ bootstrap: bootstrapData });
+
+  client.track('eventA');
+  await client.setConnectionMode('offline');
+  client.track('eventB');
+  await client.setConnectionMode('streaming');
+  client.track('eventC');
+  await client.flush();
+
+  const customKeys = new Set<string>();
+  fetchMock.mock.calls
+    .filter(([url]: [string]) => url.includes('/events/bulk/'))
+    .forEach((call: any) => {
+      try {
+        JSON.parse(call[1].body).forEach((e: any) => {
+          if (e.kind === 'custom') {
+            customKeys.add(e.key);
+          }
+        });
+      } catch {
+        // not JSON, skip
+      }
+    });
+
+  expect(customKeys.has('eventA')).toBe(true);
+  expect(customKeys.has('eventB')).toBe(true);
+  expect(customKeys.has('eventC')).toBe(true);
+
+  await client.close();
+});

packages/sdk/node-client/src/NodeClient.ts

@@ -28,6 +28,10 @@ import NodePlatform from './platform/NodePlatform';
 export class NodeClient extends LDClientImpl {
   private readonly _plugins: LDPlugin[];
 
+  // Serializes connection-mode transitions so concurrent calls cannot leave event-sending
+  // state out of sync with the active connection mode.
+  private _connectionModeQueue: Promise<void> = Promise.resolve();
+
   constructor(envKey: string, initialContext: LDContext, options: NodeOptions = {}) {
     const { logger: customLogger, debug } = options;
     const logger = customLogger ?? basicLogger({ level: debug ? 'debug' : 'info' });
@@ -45,7 +49,7 @@ export class NodeClient extends LDClientImpl {
       initialContext,
     };
 
-    const platform = new NodePlatform(logger, options);
+    const platform = new NodePlatform(logger, validatedNodeOptions);
     const endpoints = browserFdv1Endpoints(envKey);
 
     super(
@@ -99,14 +103,24 @@ export class NodeClient extends LDClientImpl {
   }
 
   async setConnectionMode(mode: ConnectionMode): Promise<void> {
-    if (mode === 'offline') {
-      this.setEventSendingEnabled(false, true);
-    }
-    const dataManager = this.dataManager as NodeDataManager;
-    await dataManager.setConnectionMode(mode);
-    if (mode !== 'offline') {
-      this.setEventSendingEnabled(true, false);
-    }
+    const task = this._connectionModeQueue.then(async () => {
+      const dataManager = this.dataManager as NodeDataManager;
+      if (mode === 'offline') {
+        // Disable analytics, then drain any queued events before tearing down the data source.
+        this.setEventSendingEnabled(false, false);
+        await this.flush();
+      }
+      try {
+        await dataManager.setConnectionMode(mode);
+      } finally {
+        // Read the mode back so event-sending always matches the mode that actually took
+        // effect, even if the transition failed partway.
+        this.setEventSendingEnabled(dataManager.getConnectionMode() !== 'offline', false);
+      }
+    });
+    // Keep the queue alive even if a transition fails; the failure still propagates to this caller.
+    this._connectionModeQueue = task.catch(() => {});
+    return task;
   }
 
   getConnectionMode(): ConnectionMode {

packages/sdk/node-client/src/NodeDataManager.ts

@@ -4,6 +4,7 @@ import {
   ConnectionMode,
   Context,
   DataSourcePaths,
+  DataSourceState,
   FlagManager,
   internal,
   LDEmitter,
@@ -19,7 +20,6 @@ import type { ValidatedOptions } from './options';
 const logTag = '[NodeDataManager]';
 
 export default class NodeDataManager extends BaseDataManager {
-  protected networkAvailable: boolean = true;
   protected connectionMode: ConnectionMode = 'streaming';
 
   constructor(
@@ -60,21 +60,36 @@ export default class NodeDataManager extends BaseDataManager {
   ): Promise<void> {
     if (this.closed) {
       this._debugLog('Identify called after data manager was closed.');
+      identifyReject(new Error('Client has been closed.'));
       return;
     }
     this.context = context;
 
-    let identifyResolved = false;
+    const offline = this.connectionMode === 'offline';
+
+    // Bootstrap and cache are mutually exclusive: when bootstrap data is provided it
+    // resolves identify immediately, so we must not also load (and potentially overwrite
+    // with) stale cached flags.
     if (identifyOptions?.bootstrap) {
+      if (identifyOptions.waitForNetworkResults) {
+        this.logger.warn(
+          `${logTag} 'waitForNetworkResults' is ignored when 'bootstrap' is provided.`,
+        );
+      }
       this._finishIdentifyFromBootstrap(context, identifyOptions, identifyResolve);
-      identifyResolved = true;
+      if (!offline) {
+        // Open a connection for ongoing updates, but identify is already resolved so no
+        // callbacks are forwarded.
+        this._setupConnection(context);
+      }
+      return;
     }
 
-    const offline = this.connectionMode === 'offline';
     const waitForNetworkResults = !!identifyOptions?.waitForNetworkResults && !offline;
 
     const loadedFromCache = await this.flagManager.loadCached(context);
-    if (loadedFromCache && !waitForNetworkResults && !identifyResolved) {
+    let identifyResolved = false;
+    if (loadedFromCache && !waitForNetworkResults) {
       this._debugLog('Identify completing with cached flags');
       identifyResolve();
       identifyResolved = true;
@@ -87,9 +102,7 @@ export default class NodeDataManager extends BaseDataManager {
         this._debugLog(
           'Offline identify - no cached flags, using defaults or already loaded flags.',
         );
-        if (!identifyResolved) {
-          identifyResolve();
-        }
+        identifyResolve();
       }
       return;
     }
@@ -111,6 +124,7 @@ export default class NodeDataManager extends BaseDataManager {
       bootstrapParsed = readFlagsFromBootstrap(this.logger, identifyOpts.bootstrap);
     }
     this.flagManager.setBootstrap(context, bootstrapParsed);
+    this.dataSourceStatusManager.requestStateUpdate(DataSourceState.Valid);
     this._debugLog('Identify - Initialization completed from bootstrap');
 
     identifyResolve();
@@ -121,7 +135,12 @@ export default class NodeDataManager extends BaseDataManager {
     identifyResolve?: () => void,
     identifyReject?: (err: Error) => void,
   ) {
-    const rawContext = Context.toLDContext(context)!;
+    const rawContext = Context.toLDContext(context);
+    if (!rawContext) {
+      this.logger.error(`${logTag} Unable to convert context; cannot establish connection.`);
+      identifyReject?.(new Error('Invalid context.'));
+      return;
+    }
 
     const plainContextString = JSON.stringify(rawContext);
     const requestor = makeRequestor(
@@ -161,15 +180,12 @@ export default class NodeDataManager extends BaseDataManager {
         this.logger.warn(
           `${logTag} _setupConnection called with unsupported connectionMode '${this.connectionMode}'.`,
         );
+        this.updateProcessor = undefined;
         return;
     }
     this.updateProcessor!.start();
   }
 
-  setNetworkAvailability(available: boolean): void {
-    this.networkAvailable = available;
-  }
-
   async setConnectionMode(mode: ConnectionMode): Promise<void> {
     if (this.closed) {
       this._debugLog('setting connection mode after data manager was closed');
@@ -187,6 +203,7 @@ export default class NodeDataManager extends BaseDataManager {
     switch (mode) {
       case 'offline':
         this.updateProcessor?.close();
+        this.updateProcessor = undefined;
         break;
       case 'polling':
       case 'streaming':

packages/sdk/node-client/src/options.ts

@@ -72,5 +72,12 @@ export default function validateOptions(opts: NodeOptions, logger: LDLogger): Va
     }
   });
 
+  if (output.tlsParams?.rejectUnauthorized === false) {
+    logger.warn(
+      'TLS certificate verification is disabled via tlsParams.rejectUnauthorized=false. ' +
+        'This is insecure and should not be used in production.',
+    );
+  }
+
   return output;
 }