Resolve request ID confusion (#85809)

Setting the cache status for a page in dev mode is a sequential
operation that only requires the HTML request ID to identify WebSocket
clients across different browser tabs/windows. This is different from
connecting the React debug channel (after which the cache status
handling was modeled), which requires both the HTML request ID as well
as the request ID to correctly associate debug chunks for a given client
with the matching request, which might be for the HTML document, a
client-side navigation, or a server function call.

With this PR, we're removing the unused `requestId` parameter from the
`setCacheStatus` function. We're also renaming the WebSocket client's
request ID to `htmlRequestId`, as well as renaming the associated maps
and variables accordingly.

This avoids confusion and better reflects the purpose of these IDs. One
of those confusions was apparent in the `setReactDebugChannel` methods,
which is now cleared up as well.

Author: unstubbable

packages/next/src/server/app-render/app-render.tsx

@@ -792,7 +792,7 @@ async function generateDynamicFlightRenderResultWithStagesInDev(
     // Before we kick off the render, we set the cache status back to it's initial state
     // in case a previous render bypassed the cache.
     if (setCacheStatus) {
-      setCacheStatus('ready', htmlRequestId, requestId)
+      setCacheStatus('ready', htmlRequestId)
     }
 
     const result = await renderWithRestartOnCacheMissInDev(
@@ -812,7 +812,7 @@ async function generateDynamicFlightRenderResultWithStagesInDev(
 
     // Set cache status to bypass when specifically bypassing caches in dev
     if (setCacheStatus) {
-      setCacheStatus('bypass', htmlRequestId, requestId)
+      setCacheStatus('bypass', htmlRequestId)
     }
 
     debugChannel = setReactDebugChannel && createDebugChannel()
@@ -2613,7 +2613,7 @@ async function renderToStream(
           // This lets the client know not to cache anything based on this render.
           if (renderOpts.setCacheStatus) {
             // we know this is available  when cacheComponents is enabled, but typeguard to be safe
-            renderOpts.setCacheStatus('bypass', htmlRequestId, requestId)
+            renderOpts.setCacheStatus('bypass', htmlRequestId)
           }
           payload._bypassCachesInDev = createElement(WarnForBypassCachesInDev, {
             route: workStore.route,
@@ -3060,7 +3060,6 @@ async function renderWithRestartOnCacheMissInDev(
   const {
     htmlRequestId,
     renderOpts,
-    requestId,
     componentMod: {
       routeModule: {
         userland: { loaderTree },
@@ -3203,7 +3202,7 @@ async function renderWithRestartOnCacheMissInDev(
   }
 
   if (process.env.NODE_ENV === 'development' && setCacheStatus) {
-    setCacheStatus('filling', htmlRequestId, requestId)
+    setCacheStatus('filling', htmlRequestId)
   }
 
   // Cache miss. We will use the initial render to fill caches, and discard its result.
@@ -3276,7 +3275,7 @@ async function renderWithRestartOnCacheMissInDev(
   )
 
   if (process.env.NODE_ENV === 'development' && setCacheStatus) {
-    setCacheStatus('filled', htmlRequestId, requestId)
+    setCacheStatus('filled', htmlRequestId)
   }
 
   return {

packages/next/src/server/app-render/types.ts

@@ -109,11 +109,7 @@ export interface RenderOptsPartial {
   }
   isOnDemandRevalidate?: boolean
   isPossibleServerAction?: boolean
-  setCacheStatus?: (
-    status: ServerCacheStatus,
-    htmlRequestId: string,
-    requestId: string
-  ) => void
+  setCacheStatus?: (status: ServerCacheStatus, htmlRequestId: string) => void
   setIsrStatus?: (key: string, value: boolean | undefined) => void
   setReactDebugChannel?: (
     debugChannel: { readable: ReadableStream<Uint8Array> },

packages/next/src/server/dev/debug-channel.ts

@@ -9,21 +9,16 @@ export interface ReactDebugChannelForBrowser {
   // Might also get a writable stream as return channel in the future.
 }
 
-const reactDebugChannelsByRequestId = new Map<
+const reactDebugChannelsByHtmlRequestId = new Map<
   string,
   ReactDebugChannelForBrowser
 >()
 
 export function connectReactDebugChannel(
   requestId: string,
+  debugChannel: ReactDebugChannelForBrowser,
   sendToClient: (message: HmrMessageSentToBrowser) => void
 ) {
-  const debugChannel = reactDebugChannelsByRequestId.get(requestId)
-
-  if (!debugChannel) {
-    return
-  }
-
   const reader = debugChannel.readable
     .pipeThrough(
       // We're sending the chunks in batches to reduce overhead in the browser.
@@ -37,8 +32,6 @@ export function connectReactDebugChannel(
       requestId,
       chunk: null,
     })
-
-    reactDebugChannelsByRequestId.delete(requestId)
   }
 
   const onError = (err: unknown) => {
@@ -63,13 +56,30 @@ export function connectReactDebugChannel(
   reader.read().then(progress, onError)
 }
 
-export function setReactDebugChannel(
-  requestId: string,
+export function connectReactDebugChannelForHtmlRequest(
+  htmlRequestId: string,
+  sendToClient: (message: HmrMessageSentToBrowser) => void
+) {
+  const debugChannel = reactDebugChannelsByHtmlRequestId.get(htmlRequestId)
+
+  if (!debugChannel) {
+    return
+  }
+
+  reactDebugChannelsByHtmlRequestId.delete(htmlRequestId)
+
+  connectReactDebugChannel(htmlRequestId, debugChannel, sendToClient)
+}
+
+export function setReactDebugChannelForHtmlRequest(
+  htmlRequestId: string,
   debugChannel: ReactDebugChannelForBrowser
 ) {
-  reactDebugChannelsByRequestId.set(requestId, debugChannel)
+  // TODO: Clean up after a timeout, in case the client never connects, e.g.
+  // when CURL'ing the page, or loading the page with JavaScript disabled etc.
+  reactDebugChannelsByHtmlRequestId.set(htmlRequestId, debugChannel)
 }
 
-export function deleteReactDebugChannel(requestId: string) {
-  reactDebugChannelsByRequestId.delete(requestId)
+export function deleteReactDebugChannelForHtmlRequest(htmlRequestId: string) {
+  reactDebugChannelsByHtmlRequestId.delete(htmlRequestId)
 }

packages/next/src/server/dev/hot-middleware.ts

@@ -71,8 +71,8 @@ function getStatsForSyncEvent(
 }
 
 export class WebpackHotMiddleware {
-  private clientsWithoutRequestId = new Set<ws>()
-  private clientsByRequestId: Map<string, ws> = new Map()
+  private clientsWithoutHtmlRequestId = new Set<ws>()
+  private clientsByHtmlRequestId: Map<string, ws> = new Map()
   private closed = false
   private clientLatestStats: { ts: number; stats: webpack.Stats } | null = null
   private middlewareLatestStats: { ts: number; stats: webpack.Stats } | null =
@@ -163,20 +163,20 @@ export class WebpackHotMiddleware {
    * and we still want to show the client overlay with the error while
    * the error page should be rendered just fine.
    */
-  onHMR = (client: ws, requestId: string | null) => {
+  onHMR = (client: ws, htmlRequestId: string | null) => {
     if (this.closed) return
 
-    if (requestId) {
-      this.clientsByRequestId.set(requestId, client)
+    if (htmlRequestId) {
+      this.clientsByHtmlRequestId.set(htmlRequestId, client)
     } else {
-      this.clientsWithoutRequestId.add(client)
+      this.clientsWithoutHtmlRequestId.add(client)
     }
 
     client.addEventListener('close', () => {
-      if (requestId) {
-        this.clientsByRequestId.delete(requestId)
+      if (htmlRequestId) {
+        this.clientsByHtmlRequestId.delete(htmlRequestId)
       } else {
-        this.clientsWithoutRequestId.delete(client)
+        this.clientsWithoutHtmlRequestId.delete(client)
       }
     })
 
@@ -228,8 +228,8 @@ export class WebpackHotMiddleware {
     })
   }
 
-  getClient = (requestId: string): ws | undefined => {
-    return this.clientsByRequestId.get(requestId)
+  getClient = (htmlRequestId: string): ws | undefined => {
+    return this.clientsByHtmlRequestId.get(htmlRequestId)
   }
 
   publishToClient = (client: ws, message: HmrMessageSentToBrowser) => {
@@ -251,8 +251,8 @@ export class WebpackHotMiddleware {
     }
 
     for (const wsClient of [
-      ...this.clientsWithoutRequestId,
-      ...this.clientsByRequestId.values(),
+      ...this.clientsWithoutHtmlRequestId,
+      ...this.clientsByHtmlRequestId.values(),
     ]) {
       this.publishToClient(wsClient, message)
     }
@@ -270,12 +270,12 @@ export class WebpackHotMiddleware {
     // inferring it from the presence of a request ID.
 
     if (!this.config.cacheComponents) {
-      for (const wsClient of this.clientsByRequestId.values()) {
+      for (const wsClient of this.clientsByHtmlRequestId.values()) {
         this.publishToClient(wsClient, message)
       }
     }
 
-    for (const wsClient of this.clientsWithoutRequestId) {
+    for (const wsClient of this.clientsWithoutHtmlRequestId) {
       this.publishToClient(wsClient, message)
     }
   }
@@ -290,30 +290,35 @@ export class WebpackHotMiddleware {
     this.closed = true
 
     for (const wsClient of [
-      ...this.clientsWithoutRequestId,
-      ...this.clientsByRequestId.values(),
+      ...this.clientsWithoutHtmlRequestId,
+      ...this.clientsByHtmlRequestId.values(),
     ]) {
       // it's okay to not cleanly close these websocket connections, this is dev
       wsClient.terminate()
     }
 
-    this.clientsWithoutRequestId.clear()
-    this.clientsByRequestId.clear()
+    this.clientsWithoutHtmlRequestId.clear()
+    this.clientsByHtmlRequestId.clear()
   }
 
-  deleteClient = (client: ws, requestId: string | null) => {
-    if (requestId) {
-      this.clientsByRequestId.delete(requestId)
+  deleteClient = (client: ws, htmlRequestId: string | null) => {
+    if (htmlRequestId) {
+      this.clientsByHtmlRequestId.delete(htmlRequestId)
     } else {
-      this.clientsWithoutRequestId.delete(client)
+      this.clientsWithoutHtmlRequestId.delete(client)
     }
   }
 
   hasClients = () => {
-    return this.clientsWithoutRequestId.size + this.clientsByRequestId.size > 0
+    return (
+      this.clientsWithoutHtmlRequestId.size + this.clientsByHtmlRequestId.size >
+      0
+    )
   }
 
   getClientCount = () => {
-    return this.clientsWithoutRequestId.size + this.clientsByRequestId.size
+    return (
+      this.clientsWithoutHtmlRequestId.size + this.clientsByHtmlRequestId.size
+    )
   }
 }