refactor(tvix/castore): remove `name` from Nodes r/8504

Nodes only have names if they're contained inside a Directory, or if
they're a root node and have something else possibly giving them a name
externally.

This removes all `name` fields in the three different Nodes, and instead
maintains it inside a BTreeMap inside the Directory.

It also removes the NamedNode trait (they don't have a get_name()), as
well as Node::rename(self, name), and all [Partial]Ord implementations
for Node (as they don't have names to use for sorting).

The `nodes()`, `directories()`, `files()` iterators inside a `Directory`
now return a tuple of Name and Node, as does the RootNodesProvider.

The different {Directory,File,Symlink}Node struct constructors got
simpler, and the {Directory,File}Node ones became infallible - as
there's no more possibility to represent invalid state.

The proto structs stayed the same - there's now from_name_and_node and
into_name_and_node to convert back and forth between the two `Node`
structs.

Some further cleanups:

The error types for Node validation were renamed. Everything related to
names is now in the DirectoryError (not yet happy about the naming)

There's some leftover cleanups to do:
 - There should be a from_(sorted_)iter and into_iter in Directory, so
   we can construct and deconstruct in one go.
   That should also enable us to implement conversions from and to the
   proto representation that moves, rather than clones.

 - The BuildRequest and PathInfo structs are still proto-based, so we
   still do a bunch of conversions back and forth there (and have some
   ugly expect there). There's not much point for error handling here,
   this will be moved to stricter types in a followup CL.

Change-Id: I7369a8e3a426f44419c349077cb4fcab2044ebb6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12205
Tested-by: BuildkiteCI
Reviewed-by: yuka <yuka@yuka.dev>
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: benjaminedwardwebb <benjaminedwardwebb@gmail.com>
Reviewed-by: Connor Brewster <cbrewster@hey.com>

5 files changed, 99 insertions, 102 deletions

diff --git a/tvix/glue/src/builtins/derivation.rs b/tvix/glue/src/builtins/derivation.rs
index 27ae2e1d6925..a79545437a67 100644
--- a/tvix/glue/src/builtins/derivation.rs
+++ b/tvix/glue/src/builtins/derivation.rs
@@ -577,10 +577,7 @@ pub(crate) mod derivation_builtins {
                     })
                     .map_err(DerivationError::InvalidDerivation)?;
 
-            let root_node = Node::File(
-                FileNode::new(store_path.to_string().into(), blob_digest, blob_size, false)
-                    .map_err(|e| ErrorKind::TvixError(Rc::new(e)))?,
-            );
+            let root_node = Node::File(FileNode::new(blob_digest, blob_size, false));
 
             // calculate the nar hash
             let (nar_size, nar_sha256) = state
@@ -600,7 +597,10 @@ pub(crate) mod derivation_builtins {
             state
                 .path_info_service
                 .put(PathInfo {
-                    node: Some((&root_node).into()),
+                    node: Some(tvix_castore::proto::Node::from_name_and_node(
+                        store_path.to_string().into(),
+                        root_node,
+                    )),
                     references: reference_paths
                         .iter()
                         .map(|x| bytes::Bytes::copy_from_slice(x.digest()))
diff --git a/tvix/glue/src/builtins/import.rs b/tvix/glue/src/builtins/import.rs
index 1cf4145427b8..a09dc5a06b35 100644
--- a/tvix/glue/src/builtins/import.rs
+++ b/tvix/glue/src/builtins/import.rs
@@ -213,16 +213,7 @@ mod import_builtins {
                     .tokio_handle
                     .block_on(async { blob_writer.close().await })?;
 
-                let root_node = Node::File(
-                    FileNode::new(
-                        // The name gets set further down, while constructing the PathInfo.
-                        "".into(),
-                        blob_digest,
-                        blob_size,
-                        false,
-                    )
-                    .map_err(|e| tvix_eval::ErrorKind::TvixError(Rc::new(e)))?,
-                );
+                let root_node = Node::File(FileNode::new(blob_digest, blob_size, false));
 
                 let ca_hash = if recursive_ingestion {
                     let (_nar_size, nar_sha256) = state
diff --git a/tvix/glue/src/fetchers/mod.rs b/tvix/glue/src/fetchers/mod.rs
index 88d6da9f1e3c..46db5d093009 100644
--- a/tvix/glue/src/fetchers/mod.rs
+++ b/tvix/glue/src/fetchers/mod.rs
@@ -10,9 +10,7 @@ use tokio::io::{AsyncBufRead, AsyncRead, AsyncWrite, AsyncWriteExt, BufReader};
 use tokio_util::io::{InspectReader, InspectWriter};
 use tracing::{instrument, warn, Span};
 use tracing_indicatif::span_ext::IndicatifSpanExt;
-use tvix_castore::{
-    blobservice::BlobService, directoryservice::DirectoryService, FileNode, Node, ValidateNodeError,
-};
+use tvix_castore::{blobservice::BlobService, directoryservice::DirectoryService, FileNode, Node};
 use tvix_store::{nar::NarCalculationService, pathinfoservice::PathInfoService, proto::PathInfo};
 use url::Url;
 
@@ -329,10 +327,7 @@ where
 
                 // Construct and return the FileNode describing the downloaded contents.
                 Ok((
-                    Node::File(
-                        FileNode::new(vec![].into(), blob_writer.close().await?, blob_size, false)
-                            .map_err(|e| FetcherError::Io(std::io::Error::other(e.to_string())))?,
-                    ),
+                    Node::File(FileNode::new(blob_writer.close().await?, blob_size, false)),
                     CAHash::Flat(actual_hash),
                     blob_size,
                 ))
@@ -527,13 +522,7 @@ where
 
                 // Construct and return the FileNode describing the downloaded contents,
                 // make it executable.
-                let root_node = Node::File(
-                    FileNode::new(vec![].into(), blob_digest, file_size, true).map_err(
-                        |e: ValidateNodeError| {
-                            FetcherError::Io(std::io::Error::other(e.to_string()))
-                        },
-                    )?,
-                );
+                let root_node = Node::File(FileNode::new(blob_digest, file_size, true));
 
                 Ok((root_node, CAHash::Nar(actual_hash), file_size))
             }
@@ -557,9 +546,6 @@ where
         // Calculate the store path to return, by calculating from ca_hash.
         let store_path = build_ca_path(name, &ca_hash, Vec::<String>::new(), false)?;
 
-        // Rename the node name to match the Store Path.
-        let node = node.rename(store_path.to_string().into());
-
         // If the resulting hash is not a CAHash::Nar, we also need to invoke
         // `calculate_nar` to calculate this representation, as it's required in
         // the [PathInfo].
@@ -577,7 +563,10 @@ where
 
         // Construct the PathInfo and persist it.
         let path_info = PathInfo {
-            node: Some((&node).into()),
+            node: Some(tvix_castore::proto::Node::from_name_and_node(
+                store_path.to_string().into(),
+                node.clone(),
+            )),
             references: vec![],
             narinfo: Some(tvix_store::proto::NarInfo {
                 nar_size,
@@ -589,20 +578,12 @@ where
             }),
         };
 
-        let path_info = self
-            .path_info_service
+        self.path_info_service
             .put(path_info)
             .await
             .map_err(|e| FetcherError::Io(e.into()))?;
 
-        Ok((
-            store_path,
-            (&path_info.node.unwrap().node.unwrap())
-                .try_into()
-                .map_err(|e: ValidateNodeError| {
-                    FetcherError::Io(std::io::Error::other(e.to_string()))
-                })?,
-        ))
+        Ok((store_path, node))
     }
 }
 
diff --git a/tvix/glue/src/tvix_build.rs b/tvix/glue/src/tvix_build.rs
index 7fff0b9cf337..77fa5d92a197 100644
--- a/tvix/glue/src/tvix_build.rs
+++ b/tvix/glue/src/tvix_build.rs
@@ -1,7 +1,7 @@
 //! This module contains glue code translating from
 //! [nix_compat::derivation::Derivation] to [tvix_build::proto::BuildRequest].
 
-use std::collections::{BTreeMap, BTreeSet};
+use std::collections::BTreeMap;
 
 use bytes::Bytes;
 use nix_compat::{derivation::Derivation, nixbase32};
@@ -39,7 +39,7 @@ const NIX_ENVIRONMENT_VARS: [(&str, &str); 12] = [
 #[allow(clippy::mutable_key_type)]
 pub(crate) fn derivation_to_build_request(
     derivation: &Derivation,
-    inputs: BTreeSet<Node>,
+    inputs: BTreeMap<bytes::Bytes, Node>,
 ) -> std::io::Result<BuildRequest> {
     debug_assert!(derivation.validate(true).is_ok(), "drv must validate");
 
@@ -109,7 +109,10 @@ pub(crate) fn derivation_to_build_request(
             .into_iter()
             .map(|(key, value)| EnvVar { key, value })
             .collect(),
-        inputs: inputs.iter().map(Into::into).collect(),
+        inputs: inputs
+            .into_iter()
+            .map(|(name, node)| tvix_castore::proto::Node::from_name_and_node(name, node))
+            .collect(),
         inputs_dir: nix_compat::store_path::STORE_DIR[1..].into(),
         constraints,
         working_dir: "build".into(),
@@ -189,10 +192,9 @@ fn calculate_pass_as_file_env(k: &str) -> (String, String) {
 
 #[cfg(test)]
 mod test {
-    use std::collections::BTreeSet;
-
     use bytes::Bytes;
     use nix_compat::derivation::Derivation;
+    use std::collections::BTreeMap;
     use tvix_build::proto::{
         build_request::{AdditionalFile, BuildConstraints, EnvVar},
         BuildRequest,
@@ -206,14 +208,9 @@ mod test {
     use lazy_static::lazy_static;
 
     lazy_static! {
-        static ref INPUT_NODE_FOO: Node = Node::Directory(
-            DirectoryNode::new(
-                Bytes::from("mp57d33657rf34lzvlbpfa1gjfv5gmpg-bar"),
-                DUMMY_DIGEST.clone(),
-                42,
-            )
-            .unwrap()
-        );
+        static ref INPUT_NODE_FOO_NAME: Bytes = "mp57d33657rf34lzvlbpfa1gjfv5gmpg-bar".into();
+        static ref INPUT_NODE_FOO: Node =
+            Node::Directory(DirectoryNode::new(DUMMY_DIGEST.clone(), 42,));
     }
 
     #[test]
@@ -222,9 +219,11 @@ mod test {
 
         let derivation = Derivation::from_aterm_bytes(aterm_bytes).expect("must parse");
 
-        let build_request =
-            derivation_to_build_request(&derivation, BTreeSet::from([INPUT_NODE_FOO.clone()]))
-                .expect("must succeed");
+        let build_request = derivation_to_build_request(
+            &derivation,
+            BTreeMap::from([(INPUT_NODE_FOO_NAME.clone(), INPUT_NODE_FOO.clone())]),
+        )
+        .expect("must succeed");
 
         let mut expected_environment_vars = vec![
             EnvVar {
@@ -261,7 +260,10 @@ mod test {
                 command_args: vec![":".into()],
                 outputs: vec!["nix/store/fhaj6gmwns62s6ypkcldbaj2ybvkhx3p-foo".into()],
                 environment_vars: expected_environment_vars,
-                inputs: vec![(&*INPUT_NODE_FOO).into()],
+                inputs: vec![tvix_castore::proto::Node::from_name_and_node(
+                    INPUT_NODE_FOO_NAME.clone(),
+                    INPUT_NODE_FOO.clone()
+                )],
                 inputs_dir: "nix/store".into(),
                 constraints: Some(BuildConstraints {
                     system: derivation.system.clone(),
@@ -285,7 +287,7 @@ mod test {
         let derivation = Derivation::from_aterm_bytes(aterm_bytes).expect("must parse");
 
         let build_request =
-            derivation_to_build_request(&derivation, BTreeSet::from([])).expect("must succeed");
+            derivation_to_build_request(&derivation, BTreeMap::from([])).expect("must succeed");
 
         let mut expected_environment_vars = vec![
             EnvVar {
@@ -355,7 +357,7 @@ mod test {
         let derivation = Derivation::from_aterm_bytes(aterm_bytes).expect("must parse");
 
         let build_request =
-            derivation_to_build_request(&derivation, BTreeSet::from([])).expect("must succeed");
+            derivation_to_build_request(&derivation, BTreeMap::from([])).expect("must succeed");
 
         let mut expected_environment_vars = vec![
             // Note how bar and baz are not present in the env anymore,
diff --git a/tvix/glue/src/tvix_store_io.rs b/tvix/glue/src/tvix_store_io.rs
index 8c5631382844..4185c9554810 100644
--- a/tvix/glue/src/tvix_store_io.rs
+++ b/tvix/glue/src/tvix_store_io.rs
@@ -4,9 +4,9 @@ use futures::{StreamExt, TryStreamExt};
 use nix_compat::nixhash::NixHash;
 use nix_compat::store_path::StorePathRef;
 use nix_compat::{nixhash::CAHash, store_path::StorePath};
+use std::collections::BTreeMap;
 use std::{
     cell::RefCell,
-    collections::BTreeSet,
     io,
     path::{Path, PathBuf},
     sync::Arc,
@@ -21,7 +21,7 @@ use tvix_store::nar::NarCalculationService;
 use tvix_castore::{
     blobservice::BlobService,
     directoryservice::{self, DirectoryService},
-    {NamedNode, Node},
+    Node,
 };
 use tvix_store::{pathinfoservice::PathInfoService, proto::PathInfo};
 
@@ -120,12 +120,22 @@ impl TvixStoreIO {
             .await?
         {
             // if we have a PathInfo, we know there will be a root_node (due to validation)
-            Some(path_info) => path_info
-                .node
-                .as_ref()
-                .expect("no node")
-                .try_into()
-                .expect("invalid node"),
+            // TODO: use stricter typed BuildRequest here.
+            Some(path_info) => {
+                let (name, node) = path_info
+                    .node
+                    .expect("no node")
+                    .into_name_and_node()
+                    .expect("invalid node");
+
+                assert_eq!(
+                    store_path.to_string().as_bytes(),
+                    name.as_ref(),
+                    "returned node basename must match requested store path"
+                );
+
+                node
+            }
             // If there's no PathInfo found, this normally means we have to
             // trigger the build (and insert into PathInfoService, after
             // reference scanning).
@@ -189,7 +199,7 @@ impl TvixStoreIO {
                         // Provide them, which means, here is where we recursively build
                         // all dependencies.
                         #[allow(clippy::mutable_key_type)]
-                        let mut input_nodes: BTreeSet<Node> =
+                        let mut inputs: BTreeMap<Bytes, Node> =
                             futures::stream::iter(drv.input_derivations.iter())
                                 .map(|(input_drv_path, output_names)| {
                                     // look up the derivation object
@@ -217,6 +227,7 @@ impl TvixStoreIO {
                                                 .clone()
                                         })
                                         .collect();
+
                                     // For each output, ask for the castore node.
                                     // We're in a per-derivation context, so if they're
                                     // not built yet they'll all get built together.
@@ -231,7 +242,7 @@ impl TvixStoreIO {
                                                 .await?;
 
                                             if let Some(node) = node {
-                                                Ok(node)
+                                                Ok((output_path.to_string().into(), node))
                                             } else {
                                                 Err(io::Error::other("no node produced"))
                                             }
@@ -245,26 +256,30 @@ impl TvixStoreIO {
                                 .try_collect()
                                 .await?;
 
-                        // add input sources
                         // FUTUREWORK: merge these who things together
                         #[allow(clippy::mutable_key_type)]
-                        let input_nodes_input_sources: BTreeSet<Node> =
+                        // add input sources
+                        let input_sources: BTreeMap<_, _> =
                             futures::stream::iter(drv.input_sources.iter())
                                 .then(|input_source| {
-                                    Box::pin(async {
-                                        let node = self
-                                            .store_path_to_node(input_source, Path::new(""))
-                                            .await?;
-                                        if let Some(node) = node {
-                                            Ok(node)
-                                        } else {
-                                            Err(io::Error::other("no node produced"))
+                                    Box::pin({
+                                        let input_source = input_source.clone();
+                                        async move {
+                                            let node = self
+                                                .store_path_to_node(&input_source, Path::new(""))
+                                                .await?;
+                                            if let Some(node) = node {
+                                                Ok((input_source.to_string().into(), node))
+                                            } else {
+                                                Err(io::Error::other("no node produced"))
+                                            }
                                         }
                                     })
                                 })
                                 .try_collect()
                                 .await?;
-                        input_nodes.extend(input_nodes_input_sources);
+
+                        inputs.extend(input_sources);
 
                         span.pb_set_message(&format!("🔨Building {}", &store_path));
 
@@ -273,7 +288,7 @@ impl TvixStoreIO {
                         // operations, so dealt with in the Some(…) match arm
 
                         // synthesize the build request.
-                        let build_request = derivation_to_build_request(&drv, input_nodes)?;
+                        let build_request = derivation_to_build_request(&drv, inputs)?;
 
                         // create a build
                         let build_result = self
@@ -287,17 +302,21 @@ impl TvixStoreIO {
 
                         // For each output, insert a PathInfo.
                         for output in &build_result.outputs {
-                            let root_node = output.try_into().expect("invalid root node");
+                            let (output_name, output_node) =
+                                output.clone().into_name_and_node().expect("invalid node");
 
                             // calculate the nar representation
                             let (nar_size, nar_sha256) = self
                                 .nar_calculation_service
-                                .calculate_nar(&root_node)
+                                .calculate_nar(&output_node)
                                 .await?;
 
                             // assemble the PathInfo to persist
                             let path_info = PathInfo {
-                                node: Some((&root_node).into()),
+                                node: Some(tvix_castore::proto::Node::from_name_and_node(
+                                    output_name,
+                                    output_node,
+                                )),
                                 references: vec![], // TODO: refscan
                                 narinfo: Some(tvix_store::proto::NarInfo {
                                     nar_size,
@@ -330,14 +349,15 @@ impl TvixStoreIO {
                         }
 
                         // find the output for the store path requested
+                        let s = store_path.to_string();
+
                         build_result
                             .outputs
                             .into_iter()
-                            .map(|output_node| Node::try_from(&output_node).expect("invalid node"))
-                            .find(|output_node| {
-                                output_node.get_name() == store_path.to_string().as_bytes()
-                            })
+                            .map(|e| e.into_name_and_node().expect("invalid node"))
+                            .find(|(output_name, _output_node)| output_name == s.as_bytes())
                             .expect("build didn't produce the store path")
+                            .1
                     }
                 }
             }
@@ -380,12 +400,16 @@ impl TvixStoreIO {
                 },
             )?;
 
-        // assemble a new root_node with a name that is derived from the nar hash.
-        let root_node = root_node.rename(output_path.to_string().into_bytes().into());
-        tvix_store::import::log_node(&root_node, path);
+        tvix_store::import::log_node(name.as_bytes(), &root_node, path);
 
-        let path_info =
-            tvix_store::import::derive_nar_ca_path_info(nar_size, nar_sha256, Some(ca), root_node);
+        // construct a PathInfo
+        let path_info = tvix_store::import::derive_nar_ca_path_info(
+            nar_size,
+            nar_sha256,
+            Some(ca),
+            output_path.to_string().into(),
+            root_node,
+        );
 
         Ok((
             path_info,
@@ -540,13 +564,12 @@ impl EvalIO for TvixStoreIO {
                             self.directory_service.as_ref().get(&digest).await
                         })? {
                             let mut children: Vec<(bytes::Bytes, FileType)> = Vec::new();
-                            for node in directory.nodes() {
+                            // TODO: into_nodes() to avoid cloning
+                            for (name, node) in directory.nodes() {
                                 children.push(match node {
-                                    Node::Directory(e) => {
-                                        (e.get_name().clone(), FileType::Directory)
-                                    }
-                                    Node::File(e) => (e.get_name().clone(), FileType::Regular),
-                                    Node::Symlink(e) => (e.get_name().clone(), FileType::Symlink),
+                                    Node::Directory(_) => (name.clone(), FileType::Directory),
+                                    Node::File(_) => (name.clone(), FileType::Regular),
+                                    Node::Symlink(_) => (name.clone(), FileType::Symlink),
                                 })
                             }
                             Ok(children)