fix: properly account strategy retries on a per strategy basis

Signed-off-by: Jérôme Benoit <jerome.benoit@piment-noir.org>

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
index 066d9224f7a85491f55a411812571d32a20fdefc..cadec2e3322c03eea4b4fb7e2117c9383e2d4ddf 100644 (file)
--- a/pnpm-lock.yaml
+++ b/pnpm-lock.yaml
@@ -1989,8 +1989,8 @@ packages:
     engines: {node: '>=20.17'}
     hasBin: true
 
-  listr2@9.0.1:
-    resolution: {integrity: sha512-SL0JY3DaxylDuo/MecFeiC+7pedM0zia33zl0vcjgwcq1q1FWWF1To9EIauPbl8GbMCU0R2e0uJ8bZunhYKD2g==}
+  listr2@9.0.2:
+    resolution: {integrity: sha512-VVd7cS6W+vLJu2wmq4QmfVj14Iep7cz4r/OWNk36Aq5ZOY7G8/BfCrQFexcwB1OIxB3yERiePfE/REBjEFulag==}
     engines: {node: '>=20.0.0'}
 
   locate-path@6.0.0:
@@ -4848,7 +4848,7 @@ snapshots:
       commander: 14.0.0
       debug: 4.4.1(supports-color@8.1.1)
       lilconfig: 3.1.3
-      listr2: 9.0.1
+      listr2: 9.0.2
       micromatch: 4.0.8
       nano-spawn: 1.0.2
       pidtree: 0.6.0
@@ -4857,7 +4857,7 @@ snapshots:
     transitivePeerDependencies:
       - supports-color
 
-  listr2@9.0.1:
+  listr2@9.0.2:
     dependencies:
       cli-truncate: 4.0.0
       colorette: 2.0.20

diff --git a/src/pools/abstract-pool.ts b/src/pools/abstract-pool.ts
index 332656a000ba7beae5d087eee5f3763c049adc9c..d074b72edf358ee96a8b96c1e11f30f9ca3aeef4 100644 (file)
--- a/src/pools/abstract-pool.ts
+++ b/src/pools/abstract-pool.ts
@@ -99,7 +99,8 @@ export abstract class AbstractPool<
       minSize: this.minimumNumberOfWorkers,
       ready: this.ready,
       started: this.started,
-      strategyRetries: this.workerChoiceStrategiesContext?.retriesCount ?? 0,
+      strategyRetries:
+        this.workerChoiceStrategiesContext?.getStrategyRetries() ?? 0,
       type: this.type,
       version,
       worker: this.worker,

diff --git a/src/pools/selection-strategies/abstract-worker-choice-strategy.ts b/src/pools/selection-strategies/abstract-worker-choice-strategy.ts
index ed8af76072f8da01a73ae182939533462f19272a..6714934ab475b2a840b770b2a03ca11d8b962367 100644 (file)
--- a/src/pools/selection-strategies/abstract-worker-choice-strategy.ts
+++ b/src/pools/selection-strategies/abstract-worker-choice-strategy.ts
@@ -28,6 +28,9 @@ export abstract class AbstractWorkerChoiceStrategy<
   /** @inheritDoc */
   public abstract readonly name: WorkerChoiceStrategy
 
+  /** @inheritDoc */
+  public retriesCount: number
+
   /** @inheritDoc */
   public readonly strategyPolicy: StrategyPolicy = Object.freeze({
     dynamicWorkerReady: true,
@@ -45,12 +48,12 @@ export abstract class AbstractWorkerChoiceStrategy<
   /**
    * The next worker node key.
    */
-  protected nextWorkerNodeKey: number | undefined = 0
+  protected nextWorkerNodeKey: number | undefined
 
   /**
    * The previous worker node key.
    */
-  protected previousWorkerNodeKey = 0
+  protected previousWorkerNodeKey: number
 
   /**
    * Constructs a worker choice strategy bound to the pool.
@@ -61,6 +64,9 @@ export abstract class AbstractWorkerChoiceStrategy<
     protected readonly pool: IPool<Worker, Data, Response>,
     protected opts?: WorkerChoiceStrategyOptions
   ) {
+    this.retriesCount = 0
+    this.nextWorkerNodeKey = 0
+    this.previousWorkerNodeKey = 0
     this.choose = this.choose.bind(this)
     this.setOptions(this.opts)
   }

diff --git a/src/pools/selection-strategies/selection-strategies-types.ts b/src/pools/selection-strategies/selection-strategies-types.ts
index 19a0e221567d7ef485cdb43fc18e2011fc0b167a..2947c9824528eff739e4d64f8fea053bdf9af0cc 100644 (file)
--- a/src/pools/selection-strategies/selection-strategies-types.ts
+++ b/src/pools/selection-strategies/selection-strategies-types.ts
@@ -86,6 +86,10 @@ export interface IWorkerChoiceStrategy {
    * @returns `true` if the reset is successful, `false` otherwise.
    */
   readonly reset: () => boolean
+  /**
+   * The worker choice strategy execution retries count.
+   */
+  retriesCount: number
   /**
    * Sets the worker choice strategy options.
    * @param opts - The worker choice strategy options.

diff --git a/src/pools/selection-strategies/worker-choice-strategies-context.ts b/src/pools/selection-strategies/worker-choice-strategies-context.ts
index 41d2fef89a2603f8c8f5c6ea60ab7f4a581461be..5bb425509ed964efc2bc2374700c58117159920a 100644 (file)
--- a/src/pools/selection-strategies/worker-choice-strategies-context.ts
+++ b/src/pools/selection-strategies/worker-choice-strategies-context.ts
@@ -28,11 +28,6 @@ export class WorkerChoiceStrategiesContext<
   Data = unknown,
   Response = unknown
 > {
-  /**
-   * The number of worker choice strategies execution retries.
-   */
-  public retriesCount: number
-
   /**
    * The default worker choice strategy in the context.
    */
@@ -90,7 +85,6 @@ export class WorkerChoiceStrategiesContext<
       buildWorkerChoiceStrategiesTaskStatisticsRequirements(
         this.workerChoiceStrategies
       )
-    this.retriesCount = 0
     this.retries = getWorkerChoiceStrategiesRetries<Worker, Data, Response>(
       this.pool,
       opts
@@ -121,6 +115,17 @@ export class WorkerChoiceStrategiesContext<
     return this.workerChoiceStrategiesPolicy
   }
 
+  /**
+   * Gets the number of worker choice strategies execution retries.
+   * @returns The number of retries.
+   */
+  public getStrategyRetries (): number {
+    return Array.from(
+      this.workerChoiceStrategies,
+      ([_, workerChoiceStrategy]) => workerChoiceStrategy.retriesCount
+    ).reduce((accumulator, retries) => accumulator + retries, 0)
+  }
+
   /**
    * Gets the active worker choice strategies in the context task statistics requirements.
    * @returns The strategies task statistics requirements.
@@ -242,13 +247,13 @@ export class WorkerChoiceStrategiesContext<
     let workerNodeKey: number | undefined = workerChoiceStrategy.choose()
     let retriesCount = 0
     while (workerNodeKey == null && retriesCount < this.retries) {
-      workerNodeKey = workerChoiceStrategy.choose()
       retriesCount++
-      this.retriesCount++
+      workerNodeKey = workerChoiceStrategy.choose()
     }
+    workerChoiceStrategy.retriesCount = retriesCount
     if (workerNodeKey == null) {
       throw new Error(
-        `Worker node key chosen by ${workerChoiceStrategy.name} is null or undefined after ${retriesCount.toString()} retries (max: ${this.retries.toString()})`
+        `Worker node key chosen by ${workerChoiceStrategy.name} is null or undefined after ${workerChoiceStrategy.retriesCount.toString()} retries (max: ${this.retries.toString()})`
       )
     }
     return workerNodeKey

diff --git a/src/worker/abstract-worker.ts b/src/worker/abstract-worker.ts
index 0741963c51513369a3d6e7d410c9867a1f0fc41f..ba8b00aa01f3e8b1c8d50428ab5d3d2a2c02c505 100644 (file)
--- a/src/worker/abstract-worker.ts
+++ b/src/worker/abstract-worker.ts
@@ -198,14 +198,10 @@ export abstract class AbstractWorker<
         DEFAULT_TASK_NAME,
         this.taskFunctions.get(DEFAULT_TASK_NAME)
       ),
-      ...(defaultTaskFunctionName !== DEFAULT_TASK_NAME
-        ? [
-            buildTaskFunctionProperties(
-              defaultTaskFunctionName,
-              this.taskFunctions.get(defaultTaskFunctionName)
-            ),
-          ]
-        : []),
+      buildTaskFunctionProperties(
+        defaultTaskFunctionName,
+        this.taskFunctions.get(defaultTaskFunctionName)
+      ),
       ...taskFunctionsProperties,
     ]
   }

diff --git a/tests/pools/selection-strategies/selection-strategies.test.mjs b/tests/pools/selection-strategies/selection-strategies.test.mjs
index 276579280c35643cded6739b4a0fd0b5e0b4daa5..c188949d3a5f8bf25dd938253fadb576f5d892c9 100644 (file)
--- a/tests/pools/selection-strategies/selection-strategies.test.mjs
+++ b/tests/pools/selection-strategies/selection-strategies.test.mjs
@@ -100,6 +100,11 @@ describe('Selection strategies test suite', () => {
           workerChoiceStrategy
         ).name
       ).toBe(workerChoiceStrategy)
+      expect(
+        pool.workerChoiceStrategiesContext.workerChoiceStrategies.get(
+          workerChoiceStrategy
+        ).retriesCount
+      ).toBe(0)
       expect(
         pool.workerChoiceStrategiesContext.workerChoiceStrategies.get(
           workerChoiceStrategy

diff --git a/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs b/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs
index 9bb0ea7932f4cf583c994d009cd5766aa54f51a6..f3e7bf42868b470672a21a2e975b2766f5007658 100644 (file)
--- a/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs
+++ b/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs
@@ -1,5 +1,5 @@
 import { expect } from '@std/expect'
-import { createStubInstance, restore, stub } from 'sinon'
+import { restore, stub } from 'sinon'
 
 import {
   DynamicThreadPool,
@@ -84,72 +84,83 @@ describe('Worker choice strategies context test suite', () => {
     expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe(
       WorkerChoiceStrategies.ROUND_ROBIN
     )
-    const workerChoiceStrategyUndefinedStub = createStubInstance(
-      RoundRobinWorkerChoiceStrategy,
-      {
-        choose: stub().returns(undefined),
-      }
-    )
-    workerChoiceStrategiesContext.workerChoiceStrategies.set(
-      workerChoiceStrategiesContext.defaultWorkerChoiceStrategy,
-      workerChoiceStrategyUndefinedStub
-    )
-    expect(() => workerChoiceStrategiesContext.execute()).toThrow(
-      new Error(
-        `Worker node key chosen by ${workerChoiceStrategyUndefinedStub.name} is null or undefined after ${workerChoiceStrategiesContext.retries.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})`
+    const workerChoiceStrategyUndefinedStub =
+      workerChoiceStrategiesContext.workerChoiceStrategies.get(
+        workerChoiceStrategiesContext.defaultWorkerChoiceStrategy
       )
-    )
-    const workerChoiceStrategyNullStub = createStubInstance(
-      RoundRobinWorkerChoiceStrategy,
-      {
-        choose: stub().returns(null),
-      }
-    )
-    workerChoiceStrategiesContext.workerChoiceStrategies.set(
-      workerChoiceStrategiesContext.defaultWorkerChoiceStrategy,
-      workerChoiceStrategyNullStub
-    )
-    expect(() => workerChoiceStrategiesContext.execute()).toThrow(
-      new Error(
-        `Worker node key chosen by ${workerChoiceStrategyNullStub.name} is null or undefined after ${workerChoiceStrategiesContext.retries.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})`
+    const chooseUndefinedStub = stub(
+      workerChoiceStrategyUndefinedStub,
+      'choose'
+    ).returns(undefined)
+    let err
+    try {
+      workerChoiceStrategiesContext.execute()
+    } catch (e) {
+      err = e
+    }
+    expect(err).toBeInstanceOf(Error)
+    expect(err.message).toBe(
+      `Worker node key chosen by ${workerChoiceStrategyUndefinedStub.name} is null or undefined after ${workerChoiceStrategyUndefinedStub.retriesCount.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})`
+    )
+    expect(chooseUndefinedStub.callCount).toBe(
+      workerChoiceStrategyUndefinedStub.retriesCount + 1
+    )
+    expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(
+      workerChoiceStrategyUndefinedStub.retriesCount
+    )
+    chooseUndefinedStub.restore()
+    const workerChoiceStrategyNullStub =
+      workerChoiceStrategiesContext.workerChoiceStrategies.get(
+        workerChoiceStrategiesContext.defaultWorkerChoiceStrategy
       )
-    )
+    const chooseNullStub = stub(workerChoiceStrategyNullStub, 'choose').returns(
+      null
+    )
+    err = undefined
+    try {
+      workerChoiceStrategiesContext.execute()
+    } catch (e) {
+      err = e
+    }
+    expect(err).toBeInstanceOf(Error)
+    expect(err.message).toBe(
+      `Worker node key chosen by ${workerChoiceStrategyNullStub.name} is null or undefined after ${workerChoiceStrategyNullStub.retriesCount.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})`
+    )
+    expect(chooseNullStub.callCount).toBe(
+      workerChoiceStrategyNullStub.retriesCount + 1
+    )
+    expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(
+      workerChoiceStrategyNullStub.retriesCount
+    )
+    chooseNullStub.restore()
   })
 
   it('Verify that execute() retry until a worker node is chosen', () => {
     const workerChoiceStrategiesContext = new WorkerChoiceStrategiesContext(
       fixedPool
     )
-    const workerChoiceStrategyStub = createStubInstance(
-      RoundRobinWorkerChoiceStrategy,
-      {
-        choose: stub()
-          .onCall(0)
-          .returns(undefined)
-          .onCall(1)
-          .returns(undefined)
-          .onCall(2)
-          .returns(undefined)
-          .onCall(3)
-          .returns(undefined)
-          .onCall(4)
-          .returns(undefined)
-          .returns(1),
-      }
-    )
     expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe(
       WorkerChoiceStrategies.ROUND_ROBIN
     )
-    workerChoiceStrategiesContext.workerChoiceStrategies.set(
-      workerChoiceStrategiesContext.defaultWorkerChoiceStrategy,
-      workerChoiceStrategyStub
-    )
-    const chosenWorkerKey = workerChoiceStrategiesContext.execute()
-    expect(
+    const workerChoiceStrategyStub =
       workerChoiceStrategiesContext.workerChoiceStrategies.get(
         workerChoiceStrategiesContext.defaultWorkerChoiceStrategy
-      ).choose.callCount
-    ).toBe(6)
+      )
+    stub(workerChoiceStrategyStub, 'choose')
+      .onCall(0)
+      .returns(undefined)
+      .onCall(1)
+      .returns(undefined)
+      .onCall(2)
+      .returns(undefined)
+      .onCall(3)
+      .returns(undefined)
+      .onCall(4)
+      .returns(undefined)
+      .returns(1)
+    const chosenWorkerKey = workerChoiceStrategiesContext.execute()
+    expect(workerChoiceStrategyStub.choose.callCount).toBe(6)
+    expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(5)
     expect(chosenWorkerKey).toBe(1)
   })
 
@@ -157,25 +168,17 @@ describe('Worker choice strategies context test suite', () => {
     const workerChoiceStrategiesContext = new WorkerChoiceStrategiesContext(
       fixedPool
     )
-    const workerChoiceStrategyStub = createStubInstance(
-      RoundRobinWorkerChoiceStrategy,
-      {
-        choose: stub().returns(0),
-      }
-    )
     expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe(
       WorkerChoiceStrategies.ROUND_ROBIN
     )
-    workerChoiceStrategiesContext.workerChoiceStrategies.set(
-      workerChoiceStrategiesContext.defaultWorkerChoiceStrategy,
-      workerChoiceStrategyStub
-    )
-    const chosenWorkerKey = workerChoiceStrategiesContext.execute()
-    expect(
+    const workerChoiceStrategyStub =
       workerChoiceStrategiesContext.workerChoiceStrategies.get(
         workerChoiceStrategiesContext.defaultWorkerChoiceStrategy
-      ).choose.calledOnce
-    ).toBe(true)
+      )
+    stub(workerChoiceStrategyStub, 'choose').returns(0)
+    const chosenWorkerKey = workerChoiceStrategiesContext.execute()
+    expect(workerChoiceStrategyStub.choose.calledOnce).toBe(true)
+    expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(0)
     expect(chosenWorkerKey).toBe(0)
   })
 
@@ -183,25 +186,17 @@ describe('Worker choice strategies context test suite', () => {
     const workerChoiceStrategiesContext = new WorkerChoiceStrategiesContext(
       dynamicPool
     )
-    const workerChoiceStrategyStub = createStubInstance(
-      RoundRobinWorkerChoiceStrategy,
-      {
-        choose: stub().returns(0),
-      }
-    )
     expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe(
       WorkerChoiceStrategies.ROUND_ROBIN
     )
-    workerChoiceStrategiesContext.workerChoiceStrategies.set(
-      workerChoiceStrategiesContext.defaultWorkerChoiceStrategy,
-      workerChoiceStrategyStub
-    )
-    const chosenWorkerKey = workerChoiceStrategiesContext.execute()
-    expect(
+    const workerChoiceStrategyStub =
       workerChoiceStrategiesContext.workerChoiceStrategies.get(
         workerChoiceStrategiesContext.defaultWorkerChoiceStrategy
-      ).choose.calledOnce
-    ).toBe(true)
+      )
+    stub(workerChoiceStrategyStub, 'choose').returns(0)
+    const chosenWorkerKey = workerChoiceStrategiesContext.execute()
+    expect(workerChoiceStrategyStub.choose.calledOnce).toBe(true)
+    expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(0)
     expect(chosenWorkerKey).toBe(0)
   })