fix(unstable_cache): avoid undefined coercion in invocation cache key#91077
fix(unstable_cache): avoid undefined coercion in invocation cache key#91077Neiland85 wants to merge 1 commit intovercel:canaryfrom
Conversation
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
There was a problem hiding this comment.
Pull request overview
This PR updates unstable_cache invocation cache-key generation to avoid undefined being lossy when arguments are serialized, improving cache-key uniqueness and reducing accidental collisions.
Changes:
- Adjusted invocation key construction to use a
JSON.stringifyreplacer that encodesundefinedvalues as a sentinel string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const invocationKey = `${fixedKey}-${JSON.stringify(args, (_, value) => | ||
| value === undefined ? '__undefined__' : value | ||
| )}` |
There was a problem hiding this comment.
Using a literal string sentinel for undefined reintroduces a cache-key collision: an argument value that is exactly '__undefined__' will serialize identically to an actual undefined. Since this key is used to look up cached results, that can return an incorrect cached value. Consider using an unambiguous encoding (e.g., a tagged wrapper object / tuple with a reserved type marker, plus escaping for strings that might collide) instead of a plain string sentinel.
| // @TODO stringify is likely not safe here. We will coerce undefined to null which will make | ||
| // the keyspace smaller than the execution space |
There was a problem hiding this comment.
The TODO comment about JSON.stringify coercing undefined to null is now outdated with the new replacer. Please update this comment to reflect the current behavior (and/or the remaining known issues with using JSON.stringify for cache-key material).
| // @TODO stringify is likely not safe here. We will coerce undefined to null which will make | |
| // the keyspace smaller than the execution space | |
| // NOTE: JSON.stringify is still not a perfect cache-key serializer. We rely on a replacer | |
| // to encode `undefined` values explicitly, but object property order and non-JSON types | |
| // (e.g. functions, Symbols, BigInts) may still collapse distinct argument shapes. |
| const invocationKey = `${fixedKey}-${JSON.stringify(args, (_, value) => | ||
| value === undefined ? '__undefined__' : value | ||
| )}` |
There was a problem hiding this comment.
This changes cache-key generation semantics for unstable_cache invocations with undefined values in args (including nested undefined inside objects). There are existing e2e tests for unstable_cache, but none appear to cover argument-key serialization; adding a regression test that demonstrates undefined vs null (and/or omitted properties) produce distinct cache keys would help prevent future collisions/regressions.
Neiland85
force-pushed
the
neiland85/fix-unstable-cache-revalidation
branch
from
2f50c24 to
a0c0513
Compare
No description provided.