Repository: vercel/next.js
Pull request: Turbopack: handle removed routes

### Why?

Before `get_endpoints` gives you a lot of `Vc<Endpoint>`, which we keep around and subscribe to. That is a problem as endpoint can also vanish at any time. e. g. when the folder is removed.
`Vc<Endpoint>` was not able to represent this state. The subscription throw an error as the Vc is no longer existant. So the error was technically correct here (not a bug in turbo-tasks).
But in practice we don't want to have this error. We want to have a clean end of the subscription without any error. That's why I change it to `Vc<Option<Endpoint>>`, which allows to represent the non-existance of an endpoint.
This can (resp. need to be) handled in subscriptions, which can report e. g. a `NotFound` result or a change event (for server_changed).

### What?

avoid having inactive Vcs when routes are removed.

instead use an `Option<Endpoint>` to allow respresenting removed endpoints and handle these in subscriptions correctly.

Closes PACK-4291

Changed files:
filename: crates/napi/src/next_api/endpoint.rs
patch: @@ -3,10 +3,11 @@ use std::{ops::Deref, sync::Arc};
 use anyhow::Result;
 use napi::{bindgen_prelude::External, JsFunction};
 use next_api::{
+    operation::OptionEndpoint,
     paths::ServerPath,
     route::{
-        endpoint_server_changed_operation, endpoint_write_to_disk_operation, Endpoint,
-        EndpointOutputPaths,
+        endpoint_client_changed_operation, endpoint_server_changed_operation,
+        endpoint_write_to_disk_operation, EndpointOutputPaths,
     },
 };
 use tracing::Instrument;
@@ -71,7 +72,7 @@ impl From<Option<EndpointOutputPaths>> for NapiWrittenEndpoint {
                 server_paths: server_paths.into_iter().map(From::from).collect(),
                 ..Default::default()
             },
-            None => Self {
+            Some(EndpointOutputPaths::NotFound) | None => Self {
                 r#type: "none".to_string(),
                 ..Default::default()
             },
@@ -85,10 +86,10 @@ impl From<Option<EndpointOutputPaths>> for NapiWrittenEndpoint {
 //    some async functions (in this case `endpoint_write_to_disk`) can cause
 //    higher-ranked lifetime errors. See https://github.com/rust-lang/rust/issues/102211
 // 2. the type_complexity clippy lint.
-pub struct ExternalEndpoint(pub VcArc<Box<dyn Endpoint>>);
+pub struct ExternalEndpoint(pub VcArc<OptionEndpoint>);
 
 impl Deref for ExternalEndpoint {
-    type Target = VcArc<Box<dyn Endpoint>>;
+    type Target = VcArc<OptionEndpoint>;
 
     fn deref(&self) -> &Self::Target {
         &self.0
@@ -105,7 +106,7 @@ struct WrittenEndpointWithIssues {
 
 #[turbo_tasks::function(operation)]
 async fn get_written_endpoint_with_issues_operation(
-    endpoint_op: OperationVc<Box<dyn Endpoint>>,
+    endpoint_op: OperationVc<OptionEndpoint>,
 ) -> Result<Vc<WrittenEndpointWithIssues>> {
     let write_to_disk_op = endpoint_write_to_disk_operation(endpoint_op);
     let (written, issues, diagnostics, effects) =
@@ -214,7 +215,7 @@ impl Eq for EndpointIssuesAndDiags {}
 
 #[turbo_tasks::function(operation)]
 async fn subscribe_issues_and_diags_operation(
-    endpoint_op: OperationVc<Box<dyn Endpoint>>,
+    endpoint_op: OperationVc<OptionEndpoint>,
     should_include_issues: bool,
 ) -> Result<Vc<EndpointIssuesAndDiags>> {
     let changed_op = endpoint_server_changed_operation(endpoint_op);
@@ -241,13 +242,6 @@ async fn subscribe_issues_and_diags_operation(
     }
 }
 
-#[turbo_tasks::function(operation)]
-fn endpoint_client_changed_operation(
-    endpoint_op: OperationVc<Box<dyn Endpoint>>,
-) -> Vc<Completion> {
-    endpoint_op.connect().client_changed()
-}
-
 #[napi(ts_return_type = "{ __napiType: \"RootTask\" }")]
 pub fn endpoint_client_changed_subscribe(
     #[napi(ts_arg_type = "{ __napiType: \"Endpoint\" }")] endpoint: External<ExternalEndpoint>,

filename: crates/napi/src/next_api/project.rs
patch: @@ -9,7 +9,8 @@ use napi::{
 use next_api::{
     entrypoints::Entrypoints,
     operation::{
-        EntrypointsOperation, InstrumentationOperation, MiddlewareOperation, RouteOperation,
+        EntrypointsOperation, InstrumentationOperation, MiddlewareOperation, OptionEndpoint,
+        RouteOperation,
     },
     project::{
         DefineEnv, DraftModeOptions, PartialProjectOptions, Project, ProjectContainer,
@@ -580,7 +581,7 @@ pub struct NapiRoute {
 
 impl NapiRoute {
     fn from_route(pathname: String, value: RouteOperation, turbo_tasks: &NextTurboTasks) -> Self {
-        let convert_endpoint = |endpoint: OperationVc<Box<dyn Endpoint>>| {
+        let convert_endpoint = |endpoint: OperationVc<OptionEndpoint>| {
             Some(External::new(ExternalEndpoint(VcArc::new(
                 turbo_tasks.clone(),
                 endpoint,

filename: crates/next-api/src/operation.rs
patch: @@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
 use turbo_rcstr::RcStr;
 use turbo_tasks::{
     debug::ValueDebugFormat, get_effects, trace::TraceRawVcs, CollectiblesSource, FxIndexMap,
-    NonLocalValue, OperationVc, ResolvedVc, Vc,
+    NonLocalValue, OperationValue, OperationVc, ResolvedVc, TaskInput, Vc,
 };
 use turbopack_core::{diagnostics::Diagnostic, issue::IssueDescriptionExt};
 
@@ -25,9 +25,9 @@ pub struct EntrypointsOperation {
     pub routes: FxIndexMap<RcStr, RouteOperation>,
     pub middleware: Option<MiddlewareOperation>,
     pub instrumentation: Option<InstrumentationOperation>,
-    pub pages_document_endpoint: OperationVc<Box<dyn Endpoint>>,
-    pub pages_app_endpoint: OperationVc<Box<dyn Endpoint>>,
-    pub pages_error_endpoint: OperationVc<Box<dyn Endpoint>>,
+    pub pages_document_endpoint: OperationVc<OptionEndpoint>,
+    pub pages_app_endpoint: OperationVc<OptionEndpoint>,
+    pub pages_error_endpoint: OperationVc<OptionEndpoint>,
 }
 
 /// Removes diagnostics, issues, and effects from the top-level `entrypoints` operation so that
@@ -53,96 +53,181 @@ impl EntrypointsOperation {
             routes: e
                 .routes
                 .iter()
-                .map(|(k, v)| (k.clone(), wrap_route(v, entrypoints)))
+                .map(|(k, v)| (k.clone(), pick_route(entrypoints, k.clone(), v)))
                 .collect(),
-            middleware: e.middleware.as_ref().map(|m| MiddlewareOperation {
-                endpoint: wrap(m.endpoint, entrypoints),
+            middleware: e.middleware.as_ref().map(|_| MiddlewareOperation {
+                endpoint: pick_endpoint(entrypoints, EndpointSelector::Middleware),
             }),
             instrumentation: e
                 .instrumentation
                 .as_ref()
-                .map(|i| InstrumentationOperation {
-                    node_js: wrap(i.node_js, entrypoints),
-                    edge: wrap(i.edge, entrypoints),
+                .map(|_| InstrumentationOperation {
+                    node_js: pick_endpoint(entrypoints, EndpointSelector::InstrumentationNodeJs),
+                    edge: pick_endpoint(entrypoints, EndpointSelector::InstrumentationEdge),
                 }),
-            pages_document_endpoint: wrap(e.pages_document_endpoint, entrypoints),
-            pages_app_endpoint: wrap(e.pages_app_endpoint, entrypoints),
-            pages_error_endpoint: wrap(e.pages_error_endpoint, entrypoints),
+            pages_document_endpoint: pick_endpoint(entrypoints, EndpointSelector::PagesDocument),
+            pages_app_endpoint: pick_endpoint(entrypoints, EndpointSelector::PagesApp),
+            pages_error_endpoint: pick_endpoint(entrypoints, EndpointSelector::PagesError),
         }
         .cell())
     }
 }
 
-fn wrap_route(route: &Route, entrypoints: OperationVc<Entrypoints>) -> RouteOperation {
+fn pick_route(entrypoints: OperationVc<Entrypoints>, key: RcStr, route: &Route) -> RouteOperation {
     match route {
-        Route::Page {
-            html_endpoint,
-            data_endpoint,
-        } => RouteOperation::Page {
-            html_endpoint: wrap(*html_endpoint, entrypoints),
-            data_endpoint: wrap(*data_endpoint, entrypoints),
+        Route::Page { .. } => RouteOperation::Page {
+            html_endpoint: pick_endpoint(entrypoints, EndpointSelector::RoutePageHtml(key.clone())),
+            data_endpoint: pick_endpoint(entrypoints, EndpointSelector::RoutePageData(key)),
         },
-        Route::PageApi { endpoint } => RouteOperation::PageApi {
-            endpoint: wrap(*endpoint, entrypoints),
+        Route::PageApi { .. } => RouteOperation::PageApi {
+            endpoint: pick_endpoint(entrypoints, EndpointSelector::RoutePageApi(key)),
         },
         Route::AppPage(pages) => RouteOperation::AppPage(
             pages
                 .iter()
-                .map(|p| AppPageRouteOperation {
+                .enumerate()
+                .map(|(i, p)| AppPageRouteOperation {
                     original_name: p.original_name.clone(),
-                    html_endpoint: wrap(p.html_endpoint, entrypoints),
-                    rsc_endpoint: wrap(p.rsc_endpoint, entrypoints),
+                    html_endpoint: pick_endpoint(
+                        entrypoints,
+                        EndpointSelector::RouteAppPageHtml(key.clone(), i),
+                    ),
+                    rsc_endpoint: pick_endpoint(
+                        entrypoints,
+                        EndpointSelector::RouteAppPageRsc(key.clone(), i),
+                    ),
                 })
                 .collect(),
         ),
-        Route::AppRoute {
-            original_name,
-            endpoint,
-        } => RouteOperation::AppRoute {
+        Route::AppRoute { original_name, .. } => RouteOperation::AppRoute {
             original_name: original_name.clone(),
-            endpoint: wrap(*endpoint, entrypoints),
+            endpoint: pick_endpoint(entrypoints, EndpointSelector::RouteAppRoute(key)),
         },
         Route::Conflict => RouteOperation::Conflict,
     }
 }
 
-/// Given a resolved `Endpoint` and the `Entrypoints` operation that it comes from, connect the
-/// operation and return a `OperationVc` of the `Entrypoint`. This `Endpoint` operation will keep
-/// the entire `Entrypoints` operation alive.
+#[derive(
+    Debug,
+    Clone,
+    TaskInput,
+    Serialize,
+    Deserialize,
+    TraceRawVcs,
+    PartialEq,
+    Eq,
+    Hash,
+    ValueDebugFormat,
+    NonLocalValue,
+    OperationValue,
+)]
+enum EndpointSelector {
+    RoutePageHtml(RcStr),
+    RoutePageData(RcStr),
+    RoutePageApi(RcStr),
+    RouteAppPageHtml(RcStr, usize),
+    RouteAppPageRsc(RcStr, usize),
+    RouteAppRoute(RcStr),
+    InstrumentationNodeJs,
+    InstrumentationEdge,
+    Middleware,
+    PagesDocument,
+    PagesApp,
+    PagesError,
+}
+
+#[turbo_tasks::value(transparent)]
+pub struct OptionEndpoint(Option<ResolvedVc<Box<dyn Endpoint>>>);
+
+/// Given a selector and the `Entrypoints` operation that it comes from, connect the operation and
+/// return an `OperationVc` containing the selected value. The returned operation will keep the
+/// entire `Entrypoints` operation alive.
 #[turbo_tasks::function(operation)]
-fn wrap(
-    endpoint: ResolvedVc<Box<dyn Endpoint>>,
+async fn pick_endpoint(
     op: OperationVc<Entrypoints>,
-) -> Vc<Box<dyn Endpoint>> {
-    let _ = op.connect();
-    *endpoint
+    selector: EndpointSelector,
+) -> Result<Vc<OptionEndpoint>> {
+    let endpoints = op.connect().strongly_consistent().await?;
+    let endpoint = match selector {
+        EndpointSelector::InstrumentationNodeJs => {
+            endpoints.instrumentation.as_ref().map(|i| i.node_js)
+        }
+        EndpointSelector::InstrumentationEdge => endpoints.instrumentation.as_ref().map(|i| i.edge),
+        EndpointSelector::Middleware => endpoints.middleware.as_ref().map(|m| m.endpoint),
+        EndpointSelector::PagesDocument => Some(endpoints.pages_document_endpoint),
+        EndpointSelector::PagesApp => Some(endpoints.pages_app_endpoint),
+        EndpointSelector::PagesError => Some(endpoints.pages_error_endpoint),
+        EndpointSelector::RoutePageHtml(name) => {
+            if let Some(Route::Page { html_endpoint, .. }) = endpoints.routes.get(&name) {
+                Some(*html_endpoint)
+            } else {
+                None
+            }
+        }
+        EndpointSelector::RoutePageData(name) => {
+            if let Some(Route::Page { data_endpoint, .. }) = endpoints.routes.get(&name) {
+                Some(*data_endpoint)
+            } else {
+                None
+            }
+        }
+        EndpointSelector::RoutePageApi(name) => {
+            if let Some(Route::PageApi { endpoint }) = endpoints.routes.get(&name) {
+                Some(*endpoint)
+            } else {
+                None
+            }
+        }
+        EndpointSelector::RouteAppPageHtml(name, i) => {
+            if let Some(Route::AppPage(pages)) = endpoints.routes.get(&name) {
+                pages.get(i).as_ref().map(|p| p.html_endpoint)
+            } else {
+                None
+            }
+        }
+        EndpointSelector::RouteAppPageRsc(name, i) => {
+            if let Some(Route::AppPage(pages)) = endpoints.routes.get(&name) {
+                pages.get(i).as_ref().map(|p| p.rsc_endpoint)
+            } else {
+                None
+            }
+        }
+        EndpointSelector::RouteAppRoute(name) => {
+            if let Some(Route::AppRoute { endpoint, .. }) = endpoints.routes.get(&name) {
+                Some(*endpoint)
+            } else {
+                None
+            }
+        }
+    };
+    Ok(Vc::cell(endpoint))
 }
 
 #[derive(Serialize, Deserialize, TraceRawVcs, PartialEq, Eq, ValueDebugFormat, NonLocalValue)]
 pub struct InstrumentationOperation {
-    pub node_js: OperationVc<Box<dyn Endpoint>>,
-    pub edge: OperationVc<Box<dyn Endpoint>>,
+    pub node_js: OperationVc<OptionEndpoint>,
+    pub edge: OperationVc<OptionEndpoint>,
 }
 
 #[derive(Serialize, Deserialize, TraceRawVcs, PartialEq, Eq, ValueDebugFormat, NonLocalValue)]
 pub struct MiddlewareOperation {
-    pub endpoint: OperationVc<Box<dyn Endpoint>>,
+    pub endpoint: OperationVc<OptionEndpoint>,
 }
 
 #[turbo_tasks::value(shared)]
 #[derive(Clone, Debug)]
 pub enum RouteOperation {
     Page {
-        html_endpoint: OperationVc<Box<dyn Endpoint>>,
-        data_endpoint: OperationVc<Box<dyn Endpoint>>,
+        html_endpoint: OperationVc<OptionEndpoint>,
+        data_endpoint: OperationVc<OptionEndpoint>,
     },
     PageApi {
-        endpoint: OperationVc<Box<dyn Endpoint>>,
+        endpoint: OperationVc<OptionEndpoint>,
     },
     AppPage(Vec<AppPageRouteOperation>),
     AppRoute {
         original_name: RcStr,
-        endpoint: OperationVc<Box<dyn Endpoint>>,
+        endpoint: OperationVc<OptionEndpoint>,
     },
     Conflict,
 }
@@ -160,6 +245,6 @@ pub enum RouteOperation {
 )]
 pub struct AppPageRouteOperation {
     pub original_name: RcStr,
-    pub html_endpoint: OperationVc<Box<dyn Endpoint>>,
-    pub rsc_endpoint: OperationVc<Box<dyn Endpoint>>,
+    pub html_endpoint: OperationVc<OptionEndpoint>,
+    pub rsc_endpoint: OperationVc<OptionEndpoint>,
 }

filename: crates/next-api/src/route.rs
patch: @@ -10,7 +10,7 @@ use turbopack_core::{
     output::OutputAssets,
 };
 
-use crate::{paths::ServerPath, project::Project};
+use crate::{operation::OptionEndpoint, paths::ServerPath, project::Project};
 
 #[derive(
     TraceRawVcs,
@@ -97,17 +97,36 @@ async fn endpoint_output_assets_operation(
 }
 
 #[turbo_tasks::function(operation)]
-pub fn endpoint_write_to_disk_operation(
-    endpoint: OperationVc<Box<dyn Endpoint>>,
-) -> Vc<EndpointOutputPaths> {
-    endpoint_write_to_disk(endpoint.connect())
+pub async fn endpoint_write_to_disk_operation(
+    endpoint: OperationVc<OptionEndpoint>,
+) -> Result<Vc<EndpointOutputPaths>> {
+    Ok(if let Some(endpoint) = *endpoint.connect().await? {
+        endpoint_write_to_disk(*endpoint)
+    } else {
+        EndpointOutputPaths::NotFound.cell()
+    })
+}
+
+#[turbo_tasks::function(operation)]
+pub async fn endpoint_server_changed_operation(
+    endpoint: OperationVc<OptionEndpoint>,
+) -> Result<Vc<Completion>> {
+    Ok(if let Some(endpoint) = *endpoint.connect().await? {
+        endpoint.server_changed()
+    } else {
+        Completion::new()
+    })
 }
 
 #[turbo_tasks::function(operation)]
-pub fn endpoint_server_changed_operation(
-    endpoint: OperationVc<Box<dyn Endpoint>>,
-) -> Vc<Completion> {
-    endpoint.connect().server_changed()
+pub async fn endpoint_client_changed_operation(
+    endpoint: OperationVc<OptionEndpoint>,
+) -> Result<Vc<Completion>> {
+    Ok(if let Some(endpoint) = *endpoint.connect().await? {
+        endpoint.client_changed()
+    } else {
+        Completion::new()
+    })
 }
 
 #[turbo_tasks::value(shared)]
@@ -131,6 +150,7 @@ pub enum EndpointOutputPaths {
         server_paths: Vec<ServerPath>,
         client_paths: Vec<RcStr>,
     },
+    NotFound,
 }
 
 /// The routes as map from pathname to route. (pathname includes the leading

filename: test/development/app-hmr/hmr.test.ts
patch: @@ -50,6 +50,8 @@ describe(`app-dir-hmr`, () => {
         // The new page should be rendered
         const newHTML = await next.render('/folder-renamed')
         expect(newHTML).toContain('Hello')
+
+        expect(next.cliOutput).not.toContain('FATAL')
       } finally {
         // Rename it back
         await next.renameFolder('app/folder-renamed', 'app/folder')
@@ -89,7 +91,6 @@ describe(`app-dir-hmr`, () => {
         // details are unimportant.
         expect(fastRefreshLogs).toEqual(
           expect.arrayContaining([
-            { source: 'log', message: '[Fast Refresh] rebuilding' },
             {
               source: 'log',
               message: expect.stringContaining('[Fast Refresh] done in '),
@@ -102,6 +103,8 @@ describe(`app-dir-hmr`, () => {
       await retry(async () => {
         expect(await browser.elementByCss('p').text()).toBe('mac')
       })
+
+      expect(next.cliOutput).not.toContain('FATAL')
     })
 
     it.each(['node', 'node-module-var', 'edge', 'edge-module-var'])(
@@ -118,7 +121,7 @@ describe(`app-dir-hmr`, () => {
             expect(logs).toEqual(
               expect.arrayContaining([
                 expect.objectContaining({
-                  message: '[Fast Refresh] rebuilding',
+                  message: expect.stringContaining('[Fast Refresh] done'),
                   source: 'log',
                 }),
               ])
@@ -143,6 +146,8 @@ describe(`app-dir-hmr`, () => {
         await retry(async () => {
           expect(await browser.elementByCss('p').text()).toBe('mac')
         })
+
+        expect(next.cliOutput).not.toContain('FATAL')
       }
     )
 
@@ -168,10 +173,6 @@ describe(`app-dir-hmr`, () => {
       // TODO: Should assert on all logs but these are cluttered with logs from our test utils (e.g. playwright tracing or webdriver)
       expect(logs).toEqual(
         expect.arrayContaining([
-          {
-            message: '[Fast Refresh] rebuilding',
-            source: 'log',
-          },
           {
             message: expect.stringContaining('[Fast Refresh] done in'),
             source: 'log',
@@ -187,6 +188,8 @@ describe(`app-dir-hmr`, () => {
       )
       // No MPA navigation triggered
       expect(await browser.eval('window.__TEST_NO_RELOAD')).toEqual(true)
+
+      expect(next.cliOutput).not.toContain('FATAL')
     })
   })
 })