crypto: remove Argon2 KDF derivation from its job setup

[panva]

Fixes: #62861

cc @jasnell @ranisalt

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1832/

• Sync ~2x throughput. Perfectly matches the fix. Near-uniform ~100% improvement across configurations.
• Async: ~2-3x throughput. Benchmarks can now overlap work across threads instead of serializing on the Argon2Job constructor.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.62%. Comparing base (53b8d3c) to head (c1c57ec).
⚠️ Report is 84 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #62863   +/-   ##
=======================================
  Coverage   89.62%   89.62%           
=======================================
  Files         706      706           
  Lines      219137   219125   -12     
  Branches    41981    41977    -4     
=======================================
- Hits       196404   196397    -7     
+ Misses      14614    14606    -8     
- Partials     8119     8122    +3     

Files with missing lines	Coverage Δ
src/crypto/crypto_argon2.cc	64.13% <ø> (+1.63%)	⬆️
src/node_errors.h	86.48% <ø> (ø)

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tniessen]

This sort of test is bound to become flaky at some point since it relies on timing of CPU-bound operations, which is inherently unpredictable if the system is under load.

Would it be possible to instead configure the job with unreasonably large parameters such that, if the job were to be run, the test would crash or timeout? We can still measure how long the constructor call takes and fail the test if it takes more than, say, five seconds.

[panva]

• that's why it's in pummel, not parallel.
• load would affect both equally

We can still measure how long the constructor call takes

That's bound to be even more flaky "under load" than a comparison that checks for "orders of magnitude" of difference (* 10n)

Would it be possible to instead configure the job with unreasonably large parameters such that, if the job were to be run, the test would crash or timeout?

I feel uneasy about intentionally introducing tests that time out instead of fail.

I'm ready to forego the test entirely if it indeed turns out to be flaky.

[tniessen]

Just in case it was not clear, my suggestion would be to skip running the job altogether and instead to only call the constructor. In the absence of a bug, that should terminate almost instantaneously. If there is a bug that causes the Argon2 computation to be performed during job setup, only then would the test take a considerable amount of time, or perhaps even time out. Catching that unlikely case by allowing a very long duration for the constructor call (five seconds or perhaps even longer) sounds less likely to end up being flaky to me than a relative comparison of the time of the constructor call versus the job execution.

In any case, this is not a blocking suggestion, just my two cents :-)

[panva]

I understood, but

Catching that unlikely case by allowing a very long duration for the constructor call (five seconds or perhaps even longer) sounds less likely to end up being flaky to me than a relative comparison of the time of the constructor call versus the job execution

Can we set a test file specific timeout?

In any case, this is not a blocking suggestion, just my two cents :-)

I very much appreciate your suggestions, i'm just trying to figure out how to apply them in practice.

[tniessen]

My understanding is that without this patch, the constructor of Argon2Job will already execute the Argon2 hash function, and this test is supposed to ensure that it does not.

const ctorStart = process.hrtime.bigint();
const job = new Argon2Job(kCryptoJobSync, ...hugeKdfArgsThatWouldTakeForever);
const ctorNs = process.hrtime.bigint() - ctorStart;
assert.ok(ctorNs < 60_000_000_000n);

If hugeKdfArgsThatWouldTakeForever.passes is sufficiently large (e.g., the maximum permitted value), the test should fail (without your patch) on any realistic hardware, either because the constructor does terminate but takes longer than 60 seconds or, and this is more likely, the test is executed using the standard Node.js test framework and will be killed due to a timeout. With the patch in place, it should terminate immediately.

[panva]

The config would have to actually be one "that takes forever", in which case only the nodejs test framework can kill it because these two will never run

const ctorNs = process.hrtime.bigint() - ctorStart;
assert.ok(ctorNs < 60_000_000_000n);

And that's the part i'm uneasy about, relying on the test runner to timeout the test/kill the process rather than the test being self contained.

If the config was one that doesn't take forever and we tried to find a hard value that it musn't take longer then than we're looking at flakyness again.

[tniessen]

One could add a worker thread that kills the process after a timeout, but personally, I think it is fair to rely on the test runner to kill the process in the unlikely case that this precise bug will ever resurface. If you feel uneasy about that, it is perfectly fine to leave the test as is.

[ranisalt]

I particularly don't mind, but the other KDF functions are checking the params in AdditionalConfig and Argon2 now does not. Probably not relevant to the end user. Catching an execution error later on is not an issue as your new tests cover this. Would ✔️

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72920/

[nodejs-github-bot]

Landed in ce21c87

[panva]

Adding more dont-land labels as I investigate some more potential bugs.

[panva]

Adding more dont-land labels as I investigate some more potential bugs.

unconfirmed