Repository navigation
fix(cache): replay "use cache" params under the original cache key - #3430
Conversation
|
/bigbonk review for issues Please review exact head Focus areas:
|
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| async function roundTrip(value: unknown): Promise<unknown> { | ||
| if (typeof value !== "object" || value === null) return value; | ||
| if ("then" in value && typeof value.then === "function") { | ||
| return new FlightChunk(await roundTrip(await value)); |
There was a problem hiding this comment.
[P1] Preserve the FlightChunk instead of assimilating it in the mock
Because roundTrip is async, returning new FlightChunk(...) causes Promise resolution to assimilate its then method. The mock therefore stores the chunk resolved value rather than a React-style chunk, so adoptFlightThenables never creates the native promise that isEncodedThenableObject requires. Both positive tests fail with Invalid cache function arguments (vp test run tests/cache-callable-runtime.test.ts), and this helper also produces the two warnings that make vp run check fail. Materialize chunks without returning a thenable directly from an async function (and handle the intentional-thenable lint rule) so these tests exercise the production shape and pass.
There was a problem hiding this comment.
Fixed in 1d5ddaa. The mock now decodes into boxed results, so an async function never returns a chunk directly, and it builds chunks as native promises with own status/value/reason fields, so it no longer defines then (no unicorn/no-thenable). The thenable check is a type guard, which fixes the await-thenable warning. vp test run tests/cache-callable-runtime.test.ts passes (3/3) and vp check is clean on the changed files.
|
Posted 1 actionable inline finding. The new unit tests and |
|
/bigbonk review for issues Please review exact head The previous finding (the Flight mock assimilating chunks, which failed the unit tests and
|
| export function encodeCacheArguments(args: unknown[]): EncodedCacheArguments { | ||
| const thenableObjectPaths: ValuePath[] = []; | ||
| const path: ValuePath = []; | ||
| const active = new Set<object>(); |
There was a problem hiding this comment.
[P2] Preserve object-graph identity while encoding
active only protects the current recursion stack. If the same plain object or array containing params appears twice, each occurrence is cloned independently, so replay changes a === b from true to false; a cyclic back-reference similarly points at the unencoded source instead of the encoded placeholder. Flight normally preserves aliases and cycles. Use a source-to-placeholder WeakMap rather than only an active set.
There was a problem hiding this comment.
Fixed in de0fe69. encodeCacheArguments now maps each source object to one encoded value, created before its children are encoded, so shared references and cycles point at the same placeholder. It only copies arrays and plain records that lead to a promise-augmented object (a reverse-reference pass, findValuesToEncode), and passes everything else to Flight as-is, so Flight also keeps references shared between a params object's fields and its resolved value (for example catch-all slug arrays). Each location of a shared promise-augmented object is recorded and restores the same promise. New tests: "keeps shared references…" and "encodes cycles once…" both fail on the previous commit.
| function adoptFlightThenables(value: unknown, active = new Set<object>()): unknown { | ||
| if (typeof value !== "object" || value === null || active.has(value)) return value; | ||
| if (isThenable(value)) { | ||
| const adopted = Promise.resolve(value).then((resolved) => adoptFlightThenables(resolved)); |
There was a problem hiding this comment.
[P2] Reuse one adopted promise per Flight chunk
Real Flight caches chunks by row id, so repeated references to one promise decode to the same ReactPromise. This creates a fresh native promise on every encounter. For cached(p, p), replay can therefore serialize two promises instead of the original repeated reference and compute a different key. It also breaks nested thenable params: the chunk in outer.fields.nested and (await outer).nested is shared by Flight, but becomes two promises here, leaving the awaited one without the restored own fields. Carry a WeakMap through both this traversal and the async resolved-value traversal. The mock currently creates a new chunk for each occurrence, so it masks this case.
There was a problem hiding this comment.
Fixed in de0fe69. adoptFlightThenables now carries one WeakMap through the synchronous traversal and the resolved-value traversal, so each chunk is adopted once. For cached(p, p) both arguments decode to the same native promise, and (await outer).nested is the same promise as outer.nested, with the restored own fields. The Flight mock now shares one decoded value between repeated references, as Flight does, and the new "keeps shared references…" test covers both cases.
|
Posted 2 actionable inline findings. Confirmed the previous Flight mock issue is fixed: 3/3 targeted tests pass and |
|
/bigbonk review for issues Please review exact head Both previous findings (object-graph identity while encoding, and one adopted promise per Flight chunk) are addressed in this commit. Please confirm them and review again:
|
| const visit = (value: unknown, referrer: object | undefined): void => { | ||
| if (typeof value !== "object" || value === null) return; | ||
| const isThenableValue = isThenableObject(value); | ||
| if (!isThenableValue && !Array.isArray(value) && !isPlainRecord(value)) return; |
There was a problem hiding this comment.
[P2] Traverse Flight-supported containers when finding params
This stops at every non-plain container, but Flight serializes containers such as Map and Set recursively. A cache argument or captured closure like new Map([["params", params]]) therefore reaches Flight unchanged: after decryption map.get("params") is a React chunk with only internal fields, so synchronous slug access is lost on replay (and aliases/cycles through that container can also point back to the unencoded graph). I confirmed this shape against the bundled real Flight runtime. Please include supported containers in the graph/path codec, or otherwise prevent recording a replay invocation that cannot preserve them.
There was a problem hiding this comment.
Fixed in 3c58677. findValuesToEncode now traverses Map keys/values and Set members, and the codec records Maps and Sets that lead to params as { kind: "map", entries } / { kind: "set", values } alongside { kind: "thenable", fields, promise }. Decoding locates every recorded path first, creates one replacement per encoded value, assigns every location, then fills members — so aliases and cycles through Maps/Sets resolve to the restored graph. Flight-decoded Maps/Sets that don't lead to params also have their chunks adopted in place. New tests cover a params-keyed and params-valued Map, a Set, and cycles through both.
|
Posted 1 actionable inline finding. Confirmed both previous identity/adopted-promise findings are fixed. All 5 targeted tests pass, and focused |
|
/bigbonk review for issues Please review exact head The previous finding (params inside Flight-supported containers such as
|
| // The promise was adopted by `adoptFlightThenables`, so it is owned here. | ||
| return encoded.promise; | ||
| case "map": | ||
| return new Map(); |
There was a problem hiding this comment.
[P2] Preserve collection aliases across a thenable boundary
Creating a fresh Map/Set here splits a collection that is both an own field of a params-like thenable and part of its resolved value. For example, with outer = Object.assign(Promise.resolve({ children }), { children }) where children is a Map containing nested params, replay makes outer.children !== (await outer).children; cycles through that collection are split too. The current tests only put collections directly in the argument graph, so they do not cover this promise boundary. Carry/reuse the Flight-decoded source collection as the restoration target (for both Map and Set) and add a nested alias test.
There was a problem hiding this comment.
Fixed in 37bd92c by removing the copy entirely rather than special-casing collections. The arguments now go to Flight unchanged. The payload is { args, thenableObjects }, where each promise-augmented object found while walking arrays, plain objects, Maps, Sets and promise fields is recorded once as { fields, promise }. Flight's writtenObjects dedupes thenables too, so each recorded promise decodes to the same chunk as every other reference to it, including inside resolved values. After adoption, decoding just assigns the fields back onto that promise. Aliases and cycles through any Flight-supported container, across the promise boundary, are preserved by Flight itself. Added the nested alias test (a Map with a cycle and a Set that are both own fields of a thenable and part of its resolved value). The Flight mock now writes each object once, so it models cycles too.
|
Posted 1 actionable inline finding. Confirmed prior Map/Set traversal issue is fixed for direct aliases/cycles. All 7 targeted tests pass and focused |
|
/bigbonk review for issues Please review exact head The previous finding (collection aliases split across a thenable boundary) is addressed by a redesign in this commit. Please confirm it and review again:
|
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head The previous finding (collection aliases split across a thenable boundary) is addressed by a redesign in this commit. Focus on the diff since
|
| ); | ||
| thenableObjects.push({ fields, promise: value }); | ||
| } | ||
| pending.push(...flightMembers(value)); |
There was a problem hiding this comment.
[P2] Collect augmented promises inside resolved promise values
This traversal only follows a thenable’s enumerable own fields; it never follows its resolved value. For example, if an admitted argument is Promise.resolve({ nested: makeThenableParams({ slug: "a" }) }), the outer promise has no own keys, so nested is absent from thenableObjects. Flight still serializes it while resolving the outer promise, but on replay const { nested } = await arg yields an adopted promise with no restored fields and nested.slug changes from "a" to undefined (while await nested still works). This is a Flight-supported argument shape and can also occur in captured values. Please collect promise-augmented objects discovered in resolved values as well, and cover this wrapper-promise case in the round-trip test.
There was a problem hiding this comment.
Fixed in 828a8ed. encodeCacheArguments is now async. It walks the arguments in waves: after each synchronous pass, it awaits every thenable it found (plain promises and promise-augmented objects, with Promise.allSettled) and walks the fulfilled values in the next pass, sharing the same visited set. Flight awaits the same promises while serializing, so this adds no new wait, and rejected promises are skipped because Flight emits them as error chunks. Flight dedupes the recorded promise with the reference inside the resolved value, and the adopt memo maps both to one native promise, so decode is unchanged. Added the wrapper-promise round-trip test: Promise.resolve({ nested: params, deeper: Promise.resolve([params]) }) plus a rejected promise. It checks that nested.slug is restored, await nested works, the doubly nested reference is the same promise, and the rejection survives.
|
Posted 1 actionable inline finding. Targeted 8 tests and focused checks pass. |
|
/bigbonk review for issues Please review exact head The previous finding (params inside resolved promise values) is addressed. Focus on the diff since
|
| // Flight serializes a promise as its resolved value, which can hold more | ||
| // params. Flight awaits these promises too, and emits rejections as errors. | ||
| const settled = await Promise.allSettled(thenables); | ||
| pending = settled.flatMap((result) => (result.status === "fulfilled" ? [result.value] : [])); |
There was a problem hiding this comment.
[P2] Revisit fulfilled containers that were seen before awaiting
visited spans every wave, so a fulfilled value that aliases an object already reachable from args is skipped here. If that object gains a params thenable while the promise is pending (for example, args contains both shared and a promise whose resolver assigns shared.params = makeThenableParams(...) and then resolves to shared), the initial pass sees the empty object and the fulfilled-value pass refuses to inspect it. Because encryptActionBoundArgs now starts only after this pre-await completes, Flight does serialize the newly added params, but thenableObjects does not record it; replay therefore loses synchronous fields such as params.slug. Please allow fulfilled containers to be traversed again after an async boundary while deduplicating captured/awaited thenables separately, and cover this aliased-mutation case.
There was a problem hiding this comment.
Fixed in ad48dcf. The visited set is now per walk. Each promise is awaited once, with its settled result kept for the duration of the encode call, and then the whole graph is walked again from args, following fulfilled values. This repeats until a walk finds no new promises. Only that last walk's thenableObjects are returned, and it runs right before Flight encoding, so containers that changed while a promise was pending (including aliases of objects seen earlier) are re-read. Added the aliased-mutation test: args = [shared, later], where later assigns shared.params = makeThenableParams({ slug: "a" }) and then resolves to shared. It fails on 828a8ed and passes now.
|
Posted 1 actionable inline finding. Confirmed the previous wrapper-promise issue is fixed; all 9 targeted tests and focused checks pass. |
|
/bigbonk review for issues Please review exact head The previous finding (containers mutated while a promise is pending) is addressed. Focus on the diff since
|
| // new promises runs right before Flight encoding, so it records the fields | ||
| // Flight sees. | ||
| const settled = new Map<PromiseLike<unknown>, PromiseSettledResult<unknown>>(); | ||
| for (;;) { |
There was a problem hiding this comment.
[P2] Bound rescans of promise-valued accessors
Each fresh walk re-evaluates enumerable properties, and this loop only terminates when every promise identity it reads is already in settled. A Flight-supported plain record such as { get value() { return Promise.resolve("x") } } returns a new promise on every pass: each one settles, the next walk reads another one, and encodeCacheArguments loops forever, so the cache call/capture never completes. React Flight accepts enumerable accessors on plain objects and would serialize the single value it reads. Please avoid using unseen promise identity as the unbounded termination condition (for example, snapshot member reads or explicitly bound/reject an unstable graph) while retaining the rescan needed for the alias-mutation case.
There was a problem hiding this comment.
Fixed in 46ce00f. Own entries are now read through a helper that reads each enumerable accessor once per encodeCacheArguments call and reuses that value in later walks and for the recorded fields. Data properties are still re-read on every walk, so the alias-mutation rescan is kept. After this, a walk can only find a new promise if program code assigned one into the arguments while an earlier promise was pending, or if promises are nested more deeply. Both stop once the arguments settle. Added a test with { get wrapped() { return Promise.resolve({ params }) } }. It checks that the getter is read once, the params are recorded, and they are restored on replay. On ad48dcf the test times out.
|
Posted 1 actionable inline finding. Confirmed the prior alias-mutation issue is fixed; all 10 targeted tests and focused checks pass. |
|
/bigbonk review for issues Please review exact head The previous finding (promise-returning accessors rescanned forever) is addressed. Focus on the diff since
|
| reads = new Map(); | ||
| accessorReads.set(value, reads); | ||
| } | ||
| if (!reads.has(key)) reads.set(key, Reflect.get(value, key)); |
There was a problem hiding this comment.
[P2] Serialize the accessor snapshot that was scanned
Caching this read terminates the pre-scan, but encodeCacheArguments still returns the original args, so encryptActionBoundArgs/Flight invokes this getter again while serializing. If successive reads return distinct graphs, e.g. get wrapped() { return Promise.resolve({ params: makeThenableParams({ slug: String(++reads) }) }) }, thenableObjects records the params from the first read while args.wrapped contains the params from Flight's second read. Decode restores fields only on the first promise, leaving (await restored.wrapped).params.slug undefined and changing replay behavior/keying. The new test masks this because every getter result closes over the same params, and it checks reads before flight.roundTrip reads the getter again. The values passed to Flight need to reuse the accessor snapshot (while preserving graph identity), and the test should use per-read params and assert the final read count/restored synchronous field after serialization.
There was a problem hiding this comment.
Fixed in 261269f. The final walk now also records each object's parents. If any array or plain record Flight will serialize had a getter read, snapshotAccessors copies that object and every value that leads to it (records, arrays, Maps, Sets), filling the getters from the recorded reads, with a memo so shared references and cycles are kept. A promise whose resolved value leads to a getter is replaced by a native promise of the copied value. A promise that reaches one only through its own fields keeps its identity, because Flight doesn't serialize those fields. The recorded fields and promises are mapped through the same copies, so Flight serializes exactly the graph the fields were recorded from. Nothing is copied when there are no getters. The caller's objects are left untouched.
The test now uses per-read params (slug: String(++reads)). It asserts after flight.roundTrip that the getter was read once and (await restored.wrapped).params.slug === "1". It also checks that the self-cycle, the aliases through an array, a promise and a Map, and a params field pointing at the record all stay shared, and that the caller's getter is still a getter. On 46ce00f it fails with reads === 2.
|
Posted 1 actionable inline finding. Targeted 11 tests and focused checks pass. |
|
/bigbonk review for issues Please review exact head The previous finding (Flight re-reading getters) is addressed. Focus on the diff since
|
The params proxy hides params named like promise or React fields (`value`, `status`) from its own keys but still resolves to them. Carry the promise through Flight alongside its own fields so a replay awaits the original resolved value, and cover the real params proxy in the unit tests.
…se fields beside them
Flight serializes arrays through JSON.stringify, which reads every index below length, including holes and non-enumerable indices. Walk and copy arrays the same way, so getters at those indices are recorded and read once, and copies keep the array's length.
…e" args An array hole reads through the prototype chain, so an indexed accessor there runs on every read. Look up the chain before treating a read as a data read, so such getters are read once per encode instead of adding a new promise on every walk.
…encoding unwrapThenableObjects and the "use cache" argument encoder must agree on which promises are keyed by their fields. Export one helper from cache-runtime.ts instead of keeping a copy in cache-callable-runtime.ts.
6c6a89d to
0e65c18
Compare
A getter in cache function arguments can return a different value on every read, so the recorded promise fields may not match what Flight serializes. Instead of reading each getter once and copying every value that leads to it, treat arguments with getters (own, or inherited through an array hole) as unreplayable: the invocation is not recorded, as for other values that cannot be replayed. Captures must always be encoded, so they fall back to plain Flight encoding without recorded promise fields; they are decoded on every call, so they still key the same way on a replay. The Flight mock now decodes promises like React's ReactPromise: not a native promise, with a `then` that returns nothing.
|
/bigbonk review for issues Please review exact head This head replaces the getter snapshotting in |
| : Object.keys(value); | ||
| return keys.map((key) => { | ||
| const descriptor = Reflect.getOwnPropertyDescriptor(value, key); | ||
| if (descriptor ? !("value" in descriptor) : key in value) { |
There was a problem hiding this comment.
[P2] Distinguish inherited data properties from accessors
For an array hole, key in value is true for any inherited property, not only an accessor. A sparse array whose custom prototype provides index 0 as a data property is serialized by Flight/JSON.stringify as that inherited value and is safe to replay, but this branch throws and cache-runtime silently omits cacheFunctionInvocation; stale entries using this argument can therefore never regenerate. Walk the prototype descriptors (as the removed readsAccessor did) and bail only when the first matching descriptor is an accessor; add an inherited-data counterpart to the accessor test.
There was a problem hiding this comment.
Fixed in 645f643. ownEntries now walks the prototype chain (readsAccessor) and bails only when the first matching descriptor is an accessor, so an inherited data property at an array hole is scanned and replayed like any other value. Added scans an inherited data property at an array hole next to the accessor test.
|
Posted 1 actionable inline finding. Targeted 14 tests and focused checks pass; worktree unchanged. |
…holes An array hole reads through the prototype chain, so treat it as a getter only when the first matching descriptor is an accessor. An inherited data property is serialized by Flight like any other value and stays replayable.
|
/bigbonk review for issues Please review exact head The previous finding (inherited data properties at array holes treated as accessors) is addressed: |
| // cannot run without them. They are decoded on every call, the original | ||
| // one included, so they key the same way on a replay even without | ||
| // recorded promise fields; only synchronous reads of those fields are lost. | ||
| encoded = { args: captures, thenableObjects: [] }; |
There was a problem hiding this comment.
[P2] Preserve params fields when another capture has a getter
This fallback drops thenableObjects for the entire capture array. If a cached function captures both an unrelated getter-bearing object and params, Flight decodes params as a chunk and adoptFlightThenables turns it into a native promise without restoring fields, so even the original invocation observes params.slug as undefined. Keep the safely discovered augmented promises when bailing out of the getter branch (or otherwise preserve their fields) rather than degrading every capture in the envelope.
There was a problem hiding this comment.
Fixed in 12297ac. Captures no longer fall back wholesale: encryptCaptures calls encodeCacheArguments(captures, "skip"), which leaves getters unread and keeps scanning, so params elsewhere in the captures keep their recorded fields. Only promise fields behind a getter are lost. Invocation args still reject getters. Test: keeps params fields in captures next to a getter (fails on 645f643).
| ? Array.from({ length: value.length }, (_, index) => String(index)) | ||
| : Object.keys(value); | ||
| return keys.map((key) => { | ||
| if (readsAccessor(value, key)) throw new UnreplayableCacheArgumentsError(); |
There was a problem hiding this comment.
[P2] Allow setter-only properties to remain replayable
readsAccessor also returns true for an accessor descriptor with no getter. Reading such a property cannot execute user code and always yields undefined, so this now unnecessarily omits cacheFunctionInvocation; stale entries whose arguments contain an enumerable setter-only property can never regenerate. Reject only descriptors with a callable get (including inherited array indices).
There was a problem hiding this comment.
Fixed in 12297ac. readsAccessor is now readsGetter and returns typeof descriptor.get === "function" for the first matching descriptor on the chain, so setter-only accessors (own or inherited at an array hole) are read as undefined and stay replayable. Test: replays arguments with setter-only properties (fails on 645f643).
|
Posted 2 actionable inline findings. Confirmed the inherited-data array-hole issue is resolved. All 15 targeted tests pass, focused checks are clean, and the worktree remains unchanged. |
…only accessors Captures with a getter no longer drop every recorded promise field: the capture encoder leaves getters unread and keeps scanning, so params elsewhere in the captures keep their fields. Only a callable getter blocks a replay; a setter-only accessor reads as undefined without running code.
|
/bigbonk review for issues Please review exact head The previous two findings are addressed in |
|
LGTM! |
Problem
Response Store regeneration of a
"use cache"function that takes Next.js params writes under a different cache key than the original render, so the stale entry is never replaced.On a cache miss, the call's arguments are encrypted through Flight (
encryptActionBoundArgs) so the entry can be regenerated later. Next.js params are a promise that also exposes the param values as own fields. Flight serializes a promise as its resolved value and drops those fields, and on decode it returns a React chunk whose own fields arestatus,valueandreason. The replay's cache key is built from those internal fields instead of{ slug }, so:captureResponseStoreDataRegenerationnever sees a write to the target key, so regeneration fails;Reading
params.slugsynchronously on replay also returnedundefined.Fix
In
cache-callable-runtime.ts, the encrypted payload is now{ args, thenableObjects }:{ fields, promise }. Promises are awaited for this, as Flight awaits them anyway; rejected ones are skipped. The arguments themselves go to Flight unchanged. The walk visits exactly what Flight serializes, including every array index belowlength(holes and non-enumerable indices too);promisedecodes to the same promise as every other reference to it, including ones inside resolved values. The fields also keep their shared references with the arguments;Because the argument graph is never copied, aliases, cycles, Maps and Sets are preserved by Flight itself. That includes a collection that is both an own field of params and part of its resolved value.
Carrying the promise, instead of rebuilding it from the fields, matters because the params proxy hides params named like promise or React fields (
value,status,error) from its own keys but still resolves to them.await paramson replay therefore sees every param.Captured closure values for inline
"use cache"functions use the same encoding, which removes React internals from their keys. Response Store metadata is partitioned per worker version, so no compatibility path for the old payload shape is needed.Tests
tests/cache-callable-runtime.test.tsuses the realmakeThenableParamsproxy and a Flight mock that writes each object once (shared references and cycles) and decodes promises like React'sReactPromise(not a native promise,thenreturns nothing). It checks that:params.slug, and awaits the full params, including a param namedvalue;use-cache-params/[slug]fixture (revalidate: 1). The test asserts the stale read returns the first value and a fresh value replaces it after background regeneration.Related
#3421 (now merged, and this branch is rebased on it) changes which page-cache arguments are encoded (drops
searchParams, adds a$$isPagemarker) but does not change how they are encrypted or decoded, so it does not fix this. The two changes compose: the marker passes through the encoding unchanged.isThenableObjectis now exported fromcache-runtime.tsand used by bothunwrapThenableObjectsand the argument encoder, so they agree on which promises are keyed by their fields.