`workerInfo.idleTimeout` not cleared in time, leading to `AssertionError: 1 !== 0 ` since version 4.9.0

[alan-agius4]

When a worker thread is used after workerInfo.idleTimeout has been set, the idleTimeout is not cleared before currentUsage is increased. This results in an AssertionError, as the system expects the worker's currentUsage() to be 0 when the idleTimeout fires, but it is actually 1 (or more) because the worker is active.

Stack Trace:

/home/runner/_work/clickup_frontend/clickup_frontend/node_modules/.pnpm/piscina@5.0.0/node_modules/piscina/src/index.ts:447
        assert.strictEqual(workerInfo.currentUsage(), 0);
               ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
1 !== 0
    at Timeout._onTimeout (/home/runner/_work/clickup_frontend/clickup_frontend/node_modules/.pnpm/piscina@5.0.0/node_modules/piscina/src/index.ts:447:16)
    at listOnTimeout (node:internal/timers:588:17)
    at processTimers (node:internal/timers:523:7) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}

See: angular/angular-cli#30549

[bryanrideshark]

We also have this problem. I'm at the point of needing to fork this tool, can anyone explain why there's a need to have that assert in there?

Throwing and breaking the entire process in this setTimeout is the cause of this problem, but I can't see why it would be included logically. Can we just delete assert.strictEqual(workerInfo.currentUsage(), 0);? What would be a problem if workerInfo.currentUsage() was 0?

[alan-agius4]

The assertion makes sense, unless the check is included in the local statement as the worker should only be terminated when the current usage is zero. Otherwise, an active worker will be removed.

[bryanrideshark]

If it's acceptable, I have a fix which would stop the application from coming to a halt in cases where the idleTimeout is not properly removed:

#817

The logic does not need to have an application breaking assert - an if condition with an early exit from the callback would be sufficient to maintain logical consistency without making piscina unreliable.

[metcoder95]

I'd prefer if we focus on the actual fix; the reason why the assertion is there is because this cause an inconsistency in the Piscina's scheduling system which can lead to an undefined state of the pool (that's the reason of a throw assertion).

The condition should never be met, tho is a pretty precise problem; I'm trying to find a reproduction to add a regression test but it requires a pretty specific state to trigger.

[alan-agius4]

It seems that I was able to replicate this problem again, but this time far less frequently.

    error properties: Object({ generatedMessage: true, code: 'ERR_ASSERTION', actual: 1, expected: 0, operator: 'strictEqual' })
        at Timeout._onTimeout (/home/runner/.cache/bazel/_bazel_runner/f47b8283cc0f5922f9455b06771398a1/sandbox/processwrapper-sandbox/3878/execroot/_main/bazel-out/k8-fastbuild/bin/packages/angular/build/application_integration_tests_/application_integration_tests.runfiles/_main/node_modules/.aspect_rules_js/piscina@5.1.1/node_modules/piscina/src/index.ts:447:16)
        at listOnTimeout (node:internal/timers:608:17)
        at processTimers (node:internal/timers:543:7)