Sm/review qa read unlock

.github/workflows/test.yml

@@ -14,6 +14,10 @@ jobs:
   windows-unit-tests:
     needs: linux-unit-tests
     uses: salesforcecli/github-workflows/.github/workflows/unitTestsWindows.yml@main
+  nuts:
+    needs: linux-unit-tests
+    uses: salesforcecli/github-workflows/.github/workflows/nut.yml@main
+
   xNuts:
     needs: linux-unit-tests
     uses: salesforcecli/github-workflows/.github/workflows/externalNut.yml@main

package.json

@@ -35,6 +35,7 @@
     "prepack": "sf-prepack",
     "prepare": "sf-install",
     "test": "wireit",
+    "test:nuts": "mocha \"test/**/*.nut.ts\" --timeout 500000",
     "test:only": "wireit",
     "test:perf": "ts-node test/perf/logger/main.test.ts"
   },
@@ -181,4 +182,4 @@
       "command": "tsc -p typedocExamples"
     }
   }
-}
+}

src/util/fileLocking.ts

@@ -8,10 +8,9 @@ import * as fs from 'node:fs';
 import { dirname } from 'node:path';
 import { lock, lockSync, check, checkSync } from 'proper-lockfile';
 import { Duration } from '@salesforce/kit';
+import { retryDecorator } from 'ts-retry-promise';
 import { SfError } from '../sfError';
 import { Logger } from '../logger/logger';
-import { PollingClient } from '../status/pollingClient';
-import { StatusResult } from '../status/types';
 import { lockOptions, lockRetryOptions } from './lockRetryOptions';
 
 type LockInitResponse = { writeAndUnlock: (data: string) => Promise<void>; unlock: () => Promise<void> };
@@ -105,35 +104,17 @@ export const lockInitSync = (filePath: string): LockInitSyncResponse => {
  * @param filePath file path to check
  */
 export const pollUntilUnlock = async (filePath: string): Promise<void> => {
-  const options: PollingClient.Options = {
-    async poll(): Promise<StatusResult> {
-      try {
-        const locked = await check(filePath, lockRetryOptions);
-        return { completed: !locked, payload: 'File unlocked' };
-      } catch (e) {
-        if (e instanceof SfError) {
-          return { completed: true, payload: e.toObject() };
-        }
-        if (e instanceof Error) {
-          return {
-            completed: true,
-            payload: {
-              name: e.name,
-              message: e.message,
-              stack: e.stack,
-            },
-          };
-        }
-
-        return { completed: true, payload: 'Error occurred' };
-      }
-    },
-    frequency: Duration.milliseconds(10),
-    timeout: Duration.minutes(1),
-  };
-
-  const client = await PollingClient.create(options);
-  await client.subscribe();
+  try {
+    await retryDecorator(check, {
+      timeout: Duration.minutes(1).milliseconds,
+      delay: 10,
+      until: (locked) => locked === false,
+      // don't retry errors (typically enoent or access on the lockfile, therefore not locked)
+      retryIf: () => false,
+    })(filePath, lockRetryOptions);
+  } catch (e) {
+    // intentionally swallow the error, same reason as above
+  }
 };
 
 export const pollUntilUnlockSync = (filePath: string): void => {

test/nut/concurrencyConfig.ts

@@ -0,0 +1,31 @@
+/*
+ * 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 { tmpdir } from 'node:os';
+import { ConfigFile } from '../../src';
+
+export const FILENAME = 'concurrency.json';
+
+export 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;
+  }
+}

test/nut/concurrencyReadWrite.ts

@@ -0,0 +1,22 @@
+/*
+ * 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 { expect } from 'chai'; // Add this line to import the expect function
+import { TestConfig } from './concurrencyConfig';
+
+const sharedLocation = join('sfdx-core-ut', 'test', 'configFile');
+
+/** ex: `yarn ts-node test/nut/concurrencyReadWrite.ts 1` */
+(async function (i: number = parseInt(process.argv[2], 10)) {
+  const config = new TestConfig(TestConfig.getOptions('test', true, true, sharedLocation));
+  config.set('x', i);
+  await config.write();
+  const readConfig = await config.read(true, true);
+  expect(readConfig.x).to.be.a('number');
+})().catch((err) => {
+  throw err;
+});

test/unit/config/configFileConcurrency.test.ts → test/nut/configFileConcurrency.nut.ts

@@ -7,33 +7,15 @@
 import { join } from 'node:path';
 import { tmpdir } from 'node:os';
 import { rm } from 'node:fs/promises';
+import { exec } from 'node:child_process';
+import { promisify } from 'node:util';
 import { expect } from 'chai';
 import { sleep } from '@salesforce/kit';
-import { ConfigFile } from '../../../src';
+import { TestConfig, FILENAME } from './concurrencyConfig';
 
-const FILENAME = 'concurrency.json';
-const sharedLocation = join('sfdx-core-ut', 'test', 'configFile');
+const execProm = promisify(exec);
 
-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;
-  }
-}
+const sharedLocation = join('sfdx-core-ut', 'test', 'configFile');
 
 /* 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
@@ -190,4 +172,15 @@ describe('concurrency', () => {
       expect(config4.get('x')).to.be.greaterThanOrEqual(7).and.lessThanOrEqual(9);
     }
   });
+
+  it('safe reads on parallel writes', async () => {
+    const configOriginal = new TestConfig(TestConfig.getOptions('test', true, true, sharedLocation));
+    configOriginal.set('x', 0);
+    await configOriginal.write();
+    await sleep(SLEEP_FUDGE_MS);
+
+    await Promise.all(
+      Array.from({ length: 50 }).map((_, i) => execProm(`yarn ts-node test/nut/concurrencyReadWrite.ts ${i}`))
+    );
+  });
 });

test/tsconfig.json

@@ -1,6 +1,6 @@
 {
   "extends": "@salesforce/dev-config/tsconfig-test-strict",
-  "include": ["unit/**/*.ts", "perf/**/*.ts"],
+  "include": ["nut/**/*.ts", "unit/**/*.ts", "perf/**/*.ts", "nut/configFileConcurrency.nut.ts"],
   "compilerOptions": {
     "noEmit": true,
     "skipLibCheck": true,