test: concurrency tests in configFile

src/config/configFile.ts

@@ -8,7 +8,7 @@
 import * as fs from 'fs';
 import { constants as fsConstants, Stats as fsStats } from 'fs';
 import { homedir as osHomedir } from 'os';
-import { dirname as pathDirname, join as pathJoin } from 'path';
+import { basename, dirname as pathDirname, join as pathJoin } from 'path';
 import { lock, lockSync } from 'proper-lockfile';
 import { parseJsonMap } from '@salesforce/kit';
 import { Global } from '../global';
@@ -179,7 +179,7 @@ export class ConfigFile<
       this.hasRead = true;
       if ((err as SfError).code === 'ENOENT') {
         if (!throwOnNotFound) {
-          this.setContentsFromFileContents({} as P, process.hrtime.bigint());
+          this.setContentsFromFileContents({} as P);
           return this.getContents();
         }
       }
@@ -215,7 +215,7 @@ export class ConfigFile<
       this.hasRead = true;
       if ((err as SfError).code === 'ENOENT') {
         if (!throwOnNotFound) {
-          this.setContentsFromFileContents({} as P, process.hrtime.bigint());
+          this.setContentsFromFileContents({} as P);
           return this.getContents();
         }
       }
@@ -244,7 +244,16 @@ export class ConfigFile<
     let unlockFn: (() => Promise<void>) | undefined;
     // lock the file.  Returns an unlock function to call when done.
     try {
-      unlockFn = await lock(this.getPath(), lockRetryOptions);
+      if (await this.exists()) {
+        unlockFn = await lock(this.getPath(), lockRetryOptions);
+      } else {
+        // lock the entire directory to keep others from trying to create the file while we are
+        unlockFn = await lock(basename(this.getPath()), lockRetryOptions);
+        this.logger.debug(
+          `No file found at ${this.getPath()}.  Write will create it.  Locking the entire directory until file is written.`
+        );
+      }
+
       // get the file modstamp.  Do this after the lock acquisition in case the file is being written to.
       const fileTimestamp = (await fs.promises.stat(this.getPath(), { bigint: true })).mtimeNs;
 
@@ -253,6 +262,7 @@ export class ConfigFile<
         parseJsonMap<P>(await fs.promises.readFile(this.getPath(), 'utf8'), this.getPath()),
         fileTimestamp
       );
+
       // merge the new contents into the in-memory LWWMap
       this.contents.merge(stateFromFile);
     } catch (err) {

src/config/lwwMap.ts

@@ -6,6 +6,7 @@
  */
 
 import { entriesOf } from '@salesforce/ts-types';
+import { nowBigInt } from '../util/time';
 import { LWWRegister } from './lwwRegister';
 import { ConfigContents, Key } from './configStackTypes';
 
@@ -19,15 +20,18 @@ export type LWWState<P> = {
  * @param contents object aligning with ConfigContents
  * @param timestamp a bigInt that sets the timestamp.  Defaults to the current time
  * construct a LWWState from an object
- *
+ * @param keysToCheckForDeletion if a key is in this array, AND not in the contents, it will be marked as deleted
  * */
-export const stateFromContents = <P extends ConfigContents>(
-  contents: P,
-  timestamp = process.hrtime.bigint()
-): LWWState<P> =>
+export const stateFromContents = <P extends ConfigContents>(contents: P, timestamp?: bigint): LWWState<P> =>
   Object.fromEntries(
-    entriesOf(contents).map(([key, value]) => [key, new LWWRegister({ timestamp, value })])
-  ) as unknown as LWWState<P>;
+    entriesOf(contents).map(
+      ([key, value]): [keyof P, LWWRegister<P[typeof key] | typeof SYMBOL_FOR_DELETED>['state']] => [
+        key,
+        new LWWRegister<typeof value>({ timestamp: timestamp ?? nowBigInt(), value }),
+      ]
+    )
+    // I'd love to get rid of this ASsertion but don't know how.
+  ) as LWWState<P>;
 
 export class LWWMap<P extends ConfigContents> {
   /** map of key to LWWRegister.  Used for managing conflicts */
@@ -56,7 +60,10 @@ export class LWWMap<P extends ConfigContents> {
     ) as LWWState<P>;
   }
 
+  // Merge top-level properties of the incoming state with the current state.
+  // The value with the latest timestamp wins.
   public merge(state: LWWState<P>): LWWState<P> {
+    // properties that are in the incoming state but not the current state might have been deleted.
     // recursively merge each key's register with the incoming state for that key
     for (const [key, remote] of entriesOf(state)) {
       const local = this.#data.get(key);
@@ -75,7 +82,7 @@ export class LWWMap<P extends ConfigContents> {
     // if the register already exists, set the value
     if (register) register.set(value);
     // otherwise, instantiate a new `LWWRegister` with the value
-    else this.#data.set(key, new LWWRegister({ timestamp: process.hrtime.bigint(), value }));
+    else this.#data.set(key, new LWWRegister({ timestamp: nowBigInt(), value }));
   }
 
   public get<K extends Key<P>>(key: K): P[K] | undefined {
@@ -88,7 +95,10 @@ export class LWWMap<P extends ConfigContents> {
   public delete<K extends Key<P>>(key: K): void {
     this.#data.set(
       key,
-      new LWWRegister<typeof SYMBOL_FOR_DELETED>({ timestamp: process.hrtime.bigint(), value: SYMBOL_FOR_DELETED })
+      new LWWRegister<typeof SYMBOL_FOR_DELETED>({
+        timestamp: nowBigInt(),
+        value: SYMBOL_FOR_DELETED,
+      })
     );
   }
 

test/unit/config/configFileConcurrency.test.ts

@@ -0,0 +1,193 @@
+/*
+ * Copyright (c) 2023, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import { join } from 'node:path';
+import { tmpdir } from 'node:os';
+import { rm } from 'node:fs/promises';
+import { expect } from 'chai';
+import { sleep } from '@salesforce/kit';
+import { ConfigFile } from '../../../src/exported';
+
+const FILENAME = 'concurrency.json';
+const sharedLocation = join('sfdx-core-ut', 'test', 'configFile');
+
+class TestConfig extends ConfigFile<ConfigFile.Options> {
+  public static getOptions(
+    filename: string,
+    isGlobal: boolean,
+    isState?: boolean,
+    filePath?: string
+  ): ConfigFile.Options {
+    return {
+      rootFolder: tmpdir(),
+      filename,
+      isGlobal,
+      isState,
+      filePath,
+    };
+  }
+
+  public static getFileName() {
+    return FILENAME;
+  }
+}
+
+/* file and node - clock timestamps aren't precise enough to run in a UT.
+ *  the goal of this and the `sleep` is to put a bit of space between operations
+ *  to simulate real-world concurrency where it's unlikely to hit the same ms
+ */
+const SLEEP_FUDGE_MS = 5;
+
+describe('concurrency', () => {
+  beforeEach(async () => {
+    await rm(join(tmpdir(), '.sfdx', 'sfdx-core-ut'), { recursive: true, force: true });
+  });
+  afterEach(async () => {
+    await rm(join(tmpdir(), '.sfdx', 'sfdx-core-ut'), { recursive: true, force: true });
+  });
+
+  it('merges in new props from file saved since a prop was set in memory', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config1.set('x', 0);
+    await sleep(SLEEP_FUDGE_MS);
+    const config2 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config2.set('x', 1);
+    await config2.write();
+
+    const c1output = await config1.write();
+    expect(c1output.x).to.equal(1);
+  });
+
+  it('newer props beat older files', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+
+    const config2 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config2.set('x', 1);
+    await config2.write();
+    await sleep(SLEEP_FUDGE_MS);
+
+    config1.set('x', 0);
+    await sleep(SLEEP_FUDGE_MS);
+
+    const c1output = await config1.write();
+    expect(c1output.x).to.equal(0);
+  });
+
+  it('"deleted" (missing) props in a file do not delete from contents"', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config1.set('x', 0);
+    await sleep(SLEEP_FUDGE_MS);
+
+    const config2 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config2.set('x', 1);
+    config2.unset('x');
+    await sleep(SLEEP_FUDGE_MS);
+
+    await config2.write();
+    await sleep(SLEEP_FUDGE_MS);
+
+    const c1output = await config1.write();
+    expect(c1output.x).to.equal(0);
+  });
+
+  it('newer deleted props beat older files', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config1.set('x', 0);
+    await sleep(SLEEP_FUDGE_MS);
+
+    const config2 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config2.set('x', 1);
+
+    await config2.write();
+    await sleep(SLEEP_FUDGE_MS);
+
+    config1.unset('x');
+
+    const c1output = await config1.write();
+    expect(c1output.x).to.equal(undefined);
+  });
+
+  it('deleted in both memory and file', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config1.set('x', 0);
+    await sleep(SLEEP_FUDGE_MS);
+
+    const config2 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    config2.set('x', 1);
+
+    await config2.write();
+    await sleep(SLEEP_FUDGE_MS);
+
+    config2.unset('x');
+    await config2.write();
+
+    await sleep(SLEEP_FUDGE_MS);
+    config1.unset('x');
+
+    const c1output = await config1.write();
+    expect(c1output.x).to.equal(undefined);
+  });
+
+  it('parallel writes from different processes produce valid json when file exists', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    await config1.write();
+    const config2 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    const config3 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+    const config4 = new TestConfig(TestConfig.getOptions(FILENAME, true, true, sharedLocation));
+
+    config1.set('x', 0);
+    await sleep(SLEEP_FUDGE_MS);
+    config2.set('x', 1);
+    config2.set('b', 2);
+    await sleep(SLEEP_FUDGE_MS);
+    config3.set('x', 2);
+    config3.set('c', 3);
+    await sleep(SLEEP_FUDGE_MS);
+    config4.set('x', 3);
+    await sleep(SLEEP_FUDGE_MS);
+
+    // simulate non-deterministic parallel operations from different processes.  We can't guarantee the order of files,
+    // so this might result in a set prop being deleted.
+    await Promise.all([config1.write(), config2.write(), config3.write(), config4.write()]);
+    await Promise.all([config1.read(), config2.read(), config3.read(), config4.read()]);
+    // timestamps on the files are treated as the stamp for all props,
+    // since we lose the CRDT prop - level timestamps when writing to json
+    expect(config1.get('x')).to.be.greaterThanOrEqual(0).and.lessThanOrEqual(3);
+    if (config1.has('b')) {
+      expect(config1.get('b')).to.equal(2);
+    }
+    if (config1.has('c')) {
+      expect(config1.get('c')).to.equal(3);
+    }
+    if (config2.has('b')) {
+      expect(config2.get('b')).to.equal(2);
+    }
+    if (config2.has('c')) {
+      expect(config2.get('c')).to.equal(3);
+    }
+  });
+
+  it('parallel writes, reverse order, with deletes', async () => {
+    const config1 = new TestConfig(TestConfig.getOptions('test', true, true, sharedLocation));
+    const config2 = new TestConfig(TestConfig.getOptions('test', true, true, sharedLocation));
+    const config3 = new TestConfig(TestConfig.getOptions('test', true, true, sharedLocation));
+    const config4 = new TestConfig(TestConfig.getOptions('test', true, true, sharedLocation));
+
+    config1.set('x', 7);
+    await sleep(SLEEP_FUDGE_MS);
+    config2.set('x', 8);
+    await sleep(SLEEP_FUDGE_MS);
+    config3.set('x', 9);
+    await sleep(SLEEP_FUDGE_MS);
+    config4.unset('x');
+
+    await Promise.all([config4.write(), config3.write(), config2.write(), config1.write()]);
+    await Promise.all([config1.read(), config2.read(), config3.read(), config4.read()]);
+    if (config4.has('x')) {
+      expect(config4.get('x')).to.be.greaterThanOrEqual(7).and.lessThanOrEqual(9);
+    }
+  });
+});

test/unit/config/lwwMapTest.ts

@@ -9,18 +9,21 @@ import { LWWMap, LWWState, SYMBOL_FOR_DELETED } from '../../../src/config/lwwMap
 import { nowBigInt } from '../../../src/util/time';
 
 config.truncateThreshold = 0;
-const TIMESTAMP_OFFSET = BigInt(1_000_000);
+// unit is ns. 1_000_000_000 ns = 1s
+const TIMESTAMP_OFFSET = BigInt(1_000_000_000);
 
 describe('LWWMap', () => {
   type TestType = { foo: string; baz: string; opt?: number; optNull?: null };
+  let state: LWWState<TestType>;
+
   describe('all properties are known', () => {
-    const state = {
-      foo: { value: 'bar', timestamp: nowBigInt() },
-      baz: { value: 'qux', timestamp: nowBigInt() },
-    };
     let lwwMap: LWWMap<TestType>;
 
     beforeEach(() => {
+      state = {
+        foo: { value: 'bar', timestamp: nowBigInt() },
+        baz: { value: 'qux', timestamp: nowBigInt() },
+      };
       lwwMap = new LWWMap(state);
     });
 
@@ -154,24 +157,28 @@ describe('LWWMap', () => {
       expect(lwwMap.state.gone.value).to.equal(SYMBOL_FOR_DELETED);
     });
   });
-  describe('nested objects', () => {
-    const state = {
+});
+
+describe('nested objects', () => {
+  type NestedOpenEndedObject = { foo: string; baz: string; opt?: number; optNull?: null; obj: Record<string, number> };
+
+  let state: LWWState<NestedOpenEndedObject>;
+  let lwwMap: LWWMap<NestedOpenEndedObject>;
+
+  beforeEach(() => {
+    state = {
       foo: { value: 'bar', timestamp: nowBigInt() },
       baz: { value: 'qux', timestamp: nowBigInt() },
       obj: { value: { a: 1, b: 2, c: 3 }, timestamp: nowBigInt() },
     };
-    let lwwMap: LWWMap<{ foo: string; baz: string; opt?: number; optNull?: null }>;
-
-    beforeEach(() => {
-      lwwMap = new LWWMap(state);
-    });
+    lwwMap = new LWWMap(state);
+  });
 
-    it('should initialize with the correct state', () => {
-      expect(lwwMap.state).to.deep.equal(state);
-    });
+  it('should initialize with the correct state', () => {
+    expect(lwwMap.state).to.deep.equal(state);
+  });
 
-    it('should get the correct value for the entire object', () => {
-      expect(lwwMap.value).to.deep.equal({ foo: 'bar', baz: 'qux', obj: { a: 1, b: 2, c: 3 } });
-    });
+  it('should get the correct value for the entire object', () => {
+    expect(lwwMap.value).to.deep.equal({ foo: 'bar', baz: 'qux', obj: { a: 1, b: 2, c: 3 } });
   });
 });

test/unit/org/orgTest.ts

@@ -640,9 +640,8 @@ describe('Org Tests', () => {
       expect(info).has.property('value', org0Username);
 
       const org1Username = orgs[1].getUsername();
-
+      assert(org1Username);
       const stateAggregator = await StateAggregator.getInstance();
-      // @ts-expect-error: user is nullable
       stateAggregator.aliases.set('foo', org1Username);
       const user = stateAggregator.aliases.getUsername('foo');
       expect(user).eq(org1Username);