Skip to content

Comments

Use provided waitUntil for pending revalidates (#74164)#6

Open
MitchLewis930 wants to merge 1 commit intopr_036_beforefrom
pr_036_after
Open

Use provided waitUntil for pending revalidates (#74164)#6
MitchLewis930 wants to merge 1 commit intopr_036_beforefrom
pr_036_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

User description

PR_036


PR Type

Enhancement


Description

  • Use provided waitUntil callback for pending revalidates instead of fallback strategy

  • Add debug logging to track pending revalidates promise resolution

  • Update test to use retry for more reliable revalidation completion detection


Diagram Walkthrough

flowchart LR
  A["Pending Revalidates"] --> B["Check for waitUntil callback"]
  B --> C{waitUntil available?}
  C -->|Yes| D["Pass promise to waitUntil"]
  C -->|No| E["Fallback to options.waitUntil"]
  D --> F["Add debug logging"]
  E --> F
  F --> G["Promise resolves"]
Loading

File Walkthrough

Relevant files
Enhancement
base-server.ts
Use provided waitUntil for pending revalidates                     

packages/next/src/server/base-server.ts

  • Extract pendingWaitUntil from context.renderOpts
  • Check if context.renderOpts.waitUntil callback is available
  • Pass pending revalidates promise to waitUntil callback if provided
  • Clear pendingWaitUntil to prevent fallback handling in sendResponse
+10/-0   
module.ts
Add debug logging for pending revalidates completion         

packages/next/src/server/route-modules/app-route/module.ts

  • Add .finally() block to pending revalidates promise
  • Log debug message when pending revalidates promise completes
  • Use NEXT_PRIVATE_DEBUG_CACHE environment variable for conditional
    logging
+8/-1     
app-render.tsx
Prefer waitUntil callback over options assignment               

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

  • Store pending revalidates promise in pendingPromise variable
  • Check if renderOpts.waitUntil callback exists before assigning
  • Pass promise to waitUntil callback if available, otherwise use
    options.waitUntil
  • Add .finally() block with debug logging for promise completion
  • Apply same pattern in two locations within the file
+24/-4   
Tests
app-fetch-deduping.test.ts
Use retry for revalidation completion detection                   

test/e2e/app-dir/app-fetch-deduping/app-fetch-deduping.test.ts

  • Replace waitFor import with retry from next-test-utils
  • Replace fixed delay with retry loop for revalidation completion
  • Wrap render and assertion in retry block with 10 second timeout
  • Improve test reliability by polling instead of waiting fixed duration
+5/-6     

Currently we are using `pendingWaitUntil` on `renderOpts` to handle
pending revalidations (fetch updates and revalidate tag calls). This
uses our old strategy of `waitUntil` inside of `sendResponse` which just
keeps the stream open until the promise resolves. This isn't ideal if we
have a proper waitUntil strategy provided so this updates to ensure we
use that instead if available.

Also adds some debug logs so we can track when this pending revalidates
promise is resolved.
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Sensitive info logging

Description: Debug logging gated by process.env.NEXT_PRIVATE_DEBUG_CACHE prints request URLs (url /
requestStore.url) to stdout, which can expose sensitive query parameters or path data in
logs if enabled (similar logging also appears later in this file around lines 1474-1477
and in packages/next/src/server/route-modules/app-route/module.ts around lines 600-606).
app-render.tsx [1307-1317]

Referred Code
]).finally(() => {
  if (process.env.NEXT_PRIVATE_DEBUG_CACHE) {
    console.log('pending revalidates promise finished for:', url)
  }
})

if (renderOpts.waitUntil) {
  renderOpts.waitUntil(pendingPromise)
} else {
  options.waitUntil = pendingPromise
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured URL logging: New console.log debug statements output unstructured strings and include url, which may
contain sensitive query parameters and violates structured/no-sensitive-data logging
requirements.

Referred Code
]).finally(() => {
  if (process.env.NEXT_PRIVATE_DEBUG_CACHE) {
    console.log('pending revalidates promise finished for:', url)
  }

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid double-handling promise by clearing property

To prevent a promise from being handled twice, clear
context.renderOpts.pendingWaitUntil after it has been processed, instead of only
clearing the local variable.

packages/next/src/server/base-server.ts [2544-2553]

-let pendingWaitUntil = context.renderOpts.pendingWaitUntil
+const { pendingWaitUntil, waitUntil } = context.renderOpts
 
-// Attempt using provided waitUntil if available
-// if it's not we fallback to sendResponse's handling
-if (pendingWaitUntil) {
-  if (context.renderOpts.waitUntil) {
-    context.renderOpts.waitUntil(pendingWaitUntil)
-    pendingWaitUntil = undefined
-  }
+// Attempt using provided waitUntil if available. If it's not, we fallback
+// to sendResponse's handling.
+if (pendingWaitUntil && waitUntil) {
+  waitUntil(pendingWaitUntil)
+  // Clear it from the renderOpts so it's not handled again by `sendResponse`.
+  context.renderOpts.pendingWaitUntil = undefined
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential bug where a promise might be handled twice because the code only nullifies a local variable instead of the property on the context object.

Medium
Filter undefined promises before aggregation

Filter out potentially undefined values from the array passed to Promise.all to
make the promise handling more robust and explicit.

packages/next/src/server/app-render/app-render.tsx [1301-1311]

-const pendingPromise = Promise.all([
-  workStore.incrementalCache?.revalidateTag(
-    workStore.revalidatedTags || []
-  ),
-  ...Object.values(workStore.pendingRevalidates || {}),
-  ...(workStore.pendingRevalidateWrites || []),
-]).finally(() => {
+const revalidatePromises = [
+  ...(workStore.revalidatedTags
+    ? [workStore.incrementalCache!.revalidateTag(workStore.revalidatedTags)]
+    : []),
+  ...Object.values(workStore.pendingRevalidates ?? []),
+  ...(workStore.pendingRevalidateWrites ?? []),
+]
+const pendingPromise = Promise.all(revalidatePromises).finally(() => {
   if (process.env.NEXT_PRIVATE_DEBUG_CACHE) {
     console.log('pending revalidates promise finished for:', url)
   }
 })
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that undefined values can be passed to Promise.all, and proposes a more robust way to construct the promises array, improving code clarity and safety.

Low
High-level
Centralize waitUntil logic for consistency

The logic for handling the waitUntil callback is duplicated in app-render.tsx
and base-server.ts. This should be centralized into a shared utility function to
improve maintainability.

Examples:

packages/next/src/server/app-render/app-render.tsx [1313-1317]
      if (renderOpts.waitUntil) {
        renderOpts.waitUntil(pendingPromise)
      } else {
        options.waitUntil = pendingPromise
      }
packages/next/src/server/base-server.ts [2546-2553]
            // Attempt using provided waitUntil if available
            // if it's not we fallback to sendResponse's handling
            if (pendingWaitUntil) {
              if (context.renderOpts.waitUntil) {
                context.renderOpts.waitUntil(pendingWaitUntil)
                pendingWaitUntil = undefined
              }
            }

Solution Walkthrough:

Before:

// In app-render.tsx
const pendingPromise = Promise.all(...)
if (renderOpts.waitUntil) {
  renderOpts.waitUntil(pendingPromise)
} else {
  options.waitUntil = pendingPromise
}

// In base-server.ts
let pendingWaitUntil = context.renderOpts.pendingWaitUntil
if (pendingWaitUntil) {
  if (context.renderOpts.waitUntil) {
    context.renderOpts.waitUntil(pendingWaitUntil)
    pendingWaitUntil = undefined
  }
}

After:

// In a new utility file
function handlePendingRevalidates(promise, renderOpts, options?) {
  if (renderOpts.waitUntil) {
    renderOpts.waitUntil(promise)
    // Potentially clear a property on renderOpts if needed
  } else if (options) {
    options.waitUntil = promise
  }
}

// In app-render.tsx
const pendingPromise = Promise.all(...)
handlePendingRevalidates(pendingPromise, renderOpts, options)

// In base-server.ts
if (context.renderOpts.pendingWaitUntil) {
  handlePendingRevalidates(context.renderOpts.pendingWaitUntil, context.renderOpts)
  context.renderOpts.pendingWaitUntil = undefined
}
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a repeated logic pattern for handling waitUntil, which is a valid maintainability concern, but the contexts in app-render.tsx and base-server.ts have subtle differences, making centralization less straightforward than implied.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants