You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(cachekit): an encrypted set() on Node.js awaits no promise around the synchronous encrypt (LAB-12935) - #319
On Node.js an encrypted set() no longer awaits the encryption manager around the synchronous NAPI encrypt. In bench:ir, the new secure-set-100b drops from 71,719 to 70,007 instructions per op (−2.39%) and secure-set-get from 490,893 to 488,909 (−0.40%). napi-encrypted reads +0.05%, from a comment-only change. Every A/A spread is 0.00%.
Closes LAB-12935
Change
EncryptionManagerCore.encryptNow() (package-internal). This mirrors decryptNow() from #318. Once the keys are derived, a binding that returns a plain value encrypts in the call: it returns the ciphertext, or throws the same NonceExhaustedError or EncryptionError that encrypt() rejects with. Before the keys are derived, after dispose(), and on Workers (WebCrypto encrypts asynchronously) it returns a Promise, so those paths behave as before. The public encrypt() is byte-identical.
setEntry awaits the encrypt only when it is a thenable. This is the one call site; it uses the shared isThenable, not instanceof Promise, for the reason #318 gives.
Nonce reservation is unchanged. Both bindings reserve the nonce inside their own encrypt call, before their first await, and encryptNow() adds no await before that call.
encrypt() keeps its own copy of the error classifier instead of sharing one with encryptNow(), as decrypt() does: in #318, routing decrypt() through a shared helper changed how V8 inlined napi-encrypted and cost it 5.6% more instructions. A parity test holds the copies equal.
Unchanged: the AAD, the keys, every stored byte, the nonce-exhaustion classification, and the set's failure handling (counted, then thrown with degradation off and absorbed with one rate-limited warning with it on).
Sibling paths
path
this PR
set() encrypt, Node.js
changed
wrap() miss and SWR refresh write
changed: they write through the same setEntry
set() encrypt, Workers
not applicable: WebCrypto encrypts asynchronously, so the set still awaits it
unchanged: no encrypt; plaintext-set-64b reads −0.02% and plaintext-set-1kb +0.00%
cachekit-py, cachekit-rs
no equivalent cost: a coroutine or future that returns without suspending does not yield
Tests
cache.encryption-l1.test.ts: on Node.js, once the keys are derived, an encrypted set() settles in as many microtask turns as a plaintext set(), and stores ciphertext in L1 and L2. With bindings that return Promises, as Workers' do, the set takes more turns and stores the same. With the global Promise replaced by a subclass, a fresh cache's first set(), which encrypts through the async encrypt(), stores ciphertext that a second cache decodes.
manager-core.test.ts: encryptNow() returns a Promise before key derivation, a plain value after it, throws NonceExhaustedError in the call on nonce exhaustion, rejects after dispose(), and returns a binding's Promise with its rejection mapped as encrypt() maps it. A parity test runs one failing binding, throwing and rejecting, through decrypt() and decryptNow() and through encrypt() and encryptNow(), and asserts the same error class, message and cause: an Error, nonce exhaustion under both spellings, and a thrown value that is not an Error.
utils/thenable.test.ts tests isThenable over a table: a native Promise, a Promise subclass, a plain thenable, an object whose then is not a function, a Uint8Array, a { value } wrapper, null and undefined.
encryption.protocol.test.ts: the aad_hex vectors and the v0x03 layout rows now also check the AAD encryptNow() hands the binding.
The tests that failed or spied on the cache's encrypt now intercept encryptNow, which the cache calls. The write-failure suite's encrypt step now throws in the call, as the NAPI encrypt does, and one test rejects a Promise, as the Workers bindings do.
Mutants: putting the await back unconditionally in setEntry fails the settle test; checking instanceof Promise instead of a thenable fails the replaced-Promise test; making encryptNow() always go through encrypt() fails eleven tests; dropping the mapping on its Promise branch fails five; dropping one spelling from encryptionFailed()'s nonce check, or changing decryptionFailed()'s fallback message, fails the parity test.
Local runs: on Node.js 24.22.1, vitest run passed 2,885 tests (1 skipped), test:protocol 1,004 and test:workers 307. The full suite also passed on Node.js 22.23.4 and 26.12.1. eslint, type-check, type-check:tests, prettier and check:workers-bundle are clean.
Benchmarks
bench:ir gains secure-set-100b: an encrypted cache.set() with L1 on, 100 B of msgpack, keys derived in setup. It takes the default 2,000 warm-up ops: under --trace-opt, no compile lands in its window after 2,000 (two did after 1,000). Both builds ran 8 workloads with 3 repeats, from trees built as the CI gate builds them, with the same workload file, on the same machine (Node.js 24.22.1, valgrind 3.22.0). The branch side is this PR's head.
workload
main
this branch
delta
A/A (branch)
secure-set-100b (new)
71,719
70,007
−2.39%
0.00%
secure-set-get (L1 off)
490,893
488,909
−0.40%
0.00%
napi-encrypted
41,508
41,527
+0.05%
0.00%
secure-get-l1-hit
48,874
48,869
−0.01%
0.00%
secure-wrap-l1-hit-100b
56,929
56,927
−0.00%
0.00%
plaintext-set-64b
42,365
42,357
−0.02%
0.00%
plaintext-set-1kb
220,736
220,746
+0.00%
0.00%
get-l1-hit
2,114
2,113
−0.05%
0.00%
The A/A spread on main is 0.00% for all eight. An encrypted set saves about 1,700 instructions (1,712); secure-set-get saves about 2,000, which includes the set.
napi-encrypted awaits encrypt() and decrypt(), whose code this PR does not change. It read 41,507 (−0.00%) at an earlier commit of this branch; the +19 instructions came with comment-only edits to manager-core.ts, and swapping that commit's manager-core.ts back into the head build restores 41,507. That is code layout, the effect CONTRIBUTING's "Measure it" practice describes, and well inside the workload's 4% gate.
secure-set-100b has no ir-seconds.json row yet, so the shard split counts it as the median row.
Kody Code Review — 1 suggested fix.
Paste the prompt below to your agent and all review fixed at once!
🛠️ Open Agent Prompt
A code review identified the following issues in this pull request.
Each section describes what was found and includes a reference implementation where available.
Files involved:
- packages/cachekit/src/encryption/manager-core.test.ts:469
---
### [1/1] packages/cachekit/src/encryption/manager-core.test.ts:469
Issue identified during code review:
Unsafe type assertion in packages/cachekit/src/encryption/manager-core.test.ts: `return error as Error;` casts the caught value to `Error` without any runtime check, which violates the team rule 'Avoid unsafe type assertions'. When the code under test throws a non-Error value such as a string or plain object, the cast lets it through, and test assertions on `.message` or `instanceof Error` receive `undefined` or fail for the wrong reason. Fix: guard with `if (error instanceof Error) return error;` and otherwise wrap it, for example `return new Error(String(error));`.
---
Review each issue in context, use the reference implementations as guidance, and apply fixes that are consistent with the surrounding codebase.
No actionable comments were generated in the recent review.
Summary by CodeRabbit
Performance
Encrypted cache writes on Node.js can now complete synchronously once keys are ready, while asynchronous encryption remains supported and awaited.
Added a benchmark covering encrypted 100 B cache writes on Node.js with L1 caching enabled.
Documentation
Clarified that encrypted reads and writes on Node.js use synchronous native operations after key derivation, while Cloudflare Workers use asynchronous WebCrypto operations that reads and writes await.
Walkthrough
The encryption manager and cache write path now support synchronous or asynchronous encryption. Tests cover results, error handling and ciphertext storage. The instruction-count benchmark and documentation include encrypted set() behaviour.
Changes
Encrypted cache writes
Layer / File(s)
Summary
Encryption manager contract and implementation packages/cachekit/src/encryption/manager-core.ts, packages/cachekit/src/encryption/manager-core.test.ts, packages/cachekit/test/protocol/encryption.protocol.test.ts
EncryptionManagerCore.encryptNow returns a direct result when possible and a promise otherwise. It maps nonce exhaustion and other failures. Tests cover return types, error mapping and protocol behaviour.
CacheImpl.setEntry calls encryptNow and awaits thenable results. Tests cover encrypted writes, ciphertext storage and encryption failures.
Encrypted set benchmark and documentation packages/cachekit/bench/ir-workload.mjs, CONTRIBUTING.md, packages/cachekit/README.md
The instruction-count workload list and benchmark include secure-set-100b. The performance note describes synchronous Node.js encryption and asynchronous Cloudflare Workers encryption.
No issue requiring a fix before merge was established. Encrypted writes retain their failure handling while avoiding the encryption await on the synchronous Node.js path.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title identifies the main change: avoiding an await around synchronous encryption for encrypted Node.js set() calls.
Description check
The description explains the encryption change, its tests, and the benchmark results.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 9 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Head a0b2a09 against base e28cd6e4081929f0c1429716fc7caefcf43e3e92, Node v24.22.2, valgrind-3.26.0. Instructions per op on the main thread (bench:ir --compare); median of the repeats, A/A = (max − min) / median. A workload fails above +2.00% and warns above +1.00% (its own threshold: plaintext-set-1mb at +10.00%, napi-encrypted at +4.00%, warning at half); an A/A over 0.40% reads INCONCLUSIVE.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
On Node.js an encrypted
set()no longer awaits the encryption manager around the synchronous NAPI encrypt. Inbench:ir, the newsecure-set-100bdrops from 71,719 to 70,007 instructions per op (−2.39%) andsecure-set-getfrom 490,893 to 488,909 (−0.40%).napi-encryptedreads +0.05%, from a comment-only change. Every A/A spread is 0.00%.Closes LAB-12935
Change
EncryptionManagerCore.encryptNow()(package-internal). This mirrorsdecryptNow()from #318. Once the keys are derived, a binding that returns a plain value encrypts in the call: it returns the ciphertext, or throws the sameNonceExhaustedErrororEncryptionErrorthatencrypt()rejects with. Before the keys are derived, afterdispose(), and on Workers (WebCrypto encrypts asynchronously) it returns a Promise, so those paths behave as before. The publicencrypt()is byte-identical.setEntryawaits the encrypt only when it is a thenable. This is the one call site; it uses the sharedisThenable, notinstanceof Promise, for the reason #318 gives.encryptNow()adds no await before that call.encrypt()keeps its own copy of the error classifier instead of sharing one withencryptNow(), asdecrypt()does: in #318, routingdecrypt()through a shared helper changed how V8 inlinednapi-encryptedand cost it 5.6% more instructions. A parity test holds the copies equal.Unchanged: the AAD, the keys, every stored byte, the nonce-exhaustion classification, and the set's failure handling (counted, then thrown with degradation off and absorbed with one rate-limited warning with it on).
Sibling paths
set()encrypt, Node.jswrap()miss and SWR refresh writesetEntryset()encrypt, Workersset()plaintext-set-64breads −0.02% andplaintext-set-1kb+0.00%Tests
cache.encryption-l1.test.ts: on Node.js, once the keys are derived, an encryptedset()settles in as many microtask turns as a plaintextset(), and stores ciphertext in L1 and L2. With bindings that return Promises, as Workers' do, the set takes more turns and stores the same. With the globalPromisereplaced by a subclass, a fresh cache's firstset(), which encrypts through the asyncencrypt(), stores ciphertext that a second cache decodes.manager-core.test.ts:encryptNow()returns a Promise before key derivation, a plain value after it, throwsNonceExhaustedErrorin the call on nonce exhaustion, rejects afterdispose(), and returns a binding's Promise with its rejection mapped asencrypt()maps it. A parity test runs one failing binding, throwing and rejecting, throughdecrypt()anddecryptNow()and throughencrypt()andencryptNow(), and asserts the same error class, message andcause: an Error, nonce exhaustion under both spellings, and a thrown value that is not an Error.utils/thenable.test.tstestsisThenableover a table: a native Promise, a Promise subclass, a plain thenable, an object whosethenis not a function, aUint8Array, a{ value }wrapper,nullandundefined.encryption.protocol.test.ts: theaad_hexvectors and the v0x03 layout rows now also check the AADencryptNow()hands the binding.encryptNow, which the cache calls. The write-failure suite's encrypt step now throws in the call, as the NAPI encrypt does, and one test rejects a Promise, as the Workers bindings do.Mutants: putting the await back unconditionally in
setEntryfails the settle test; checkinginstanceof Promiseinstead of a thenable fails the replaced-Promisetest; makingencryptNow()always go throughencrypt()fails eleven tests; dropping the mapping on its Promise branch fails five; dropping one spelling fromencryptionFailed()'s nonce check, or changingdecryptionFailed()'s fallback message, fails the parity test.Local runs: on Node.js 24.22.1,
vitest runpassed 2,885 tests (1 skipped),test:protocol1,004 andtest:workers307. The full suite also passed on Node.js 22.23.4 and 26.12.1. eslint,type-check,type-check:tests, prettier andcheck:workers-bundleare clean.Benchmarks
bench:irgainssecure-set-100b: an encryptedcache.set()with L1 on, 100 B of msgpack, keys derived in setup. It takes the default 2,000 warm-up ops: under--trace-opt, no compile lands in its window after 2,000 (two did after 1,000). Both builds ran 8 workloads with 3 repeats, from trees built as the CI gate builds them, with the same workload file, on the same machine (Node.js 24.22.1, valgrind 3.22.0). The branch side is this PR's head.mainsecure-set-100b(new)secure-set-get(L1 off)napi-encryptedsecure-get-l1-hitsecure-wrap-l1-hit-100bplaintext-set-64bplaintext-set-1kbget-l1-hitThe A/A spread on
mainis 0.00% for all eight. An encrypted set saves about 1,700 instructions (1,712);secure-set-getsaves about 2,000, which includes the set.napi-encryptedawaitsencrypt()anddecrypt(), whose code this PR does not change. It read 41,507 (−0.00%) at an earlier commit of this branch; the +19 instructions came with comment-only edits tomanager-core.ts, and swapping that commit'smanager-core.tsback into the head build restores 41,507. That is code layout, the effect CONTRIBUTING's "Measure it" practice describes, and well inside the workload's 4% gate.secure-set-100bhas noir-seconds.jsonrow yet, so the shard split counts it as the median row.