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>

6 files changed, 50 insertions, 47 deletions

diff --git a/tvix/castore/src/directoryservice/directory_graph.rs b/tvix/castore/src/directoryservice/directory_graph.rs
index 0cf2f71adb06..ffb25de9a854 100644
--- a/tvix/castore/src/directoryservice/directory_graph.rs
+++ b/tvix/castore/src/directoryservice/directory_graph.rs
@@ -10,7 +10,7 @@ use petgraph::{
 use tracing::instrument;
 
 use super::order_validator::{LeavesToRootValidator, OrderValidator, RootToLeavesValidator};
-use crate::{B3Digest, Directory, DirectoryNode, NamedNode};
+use crate::{B3Digest, Directory, DirectoryNode};
 
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
@@ -69,12 +69,12 @@ pub struct ValidatedDirectoryGraph {
     root: Option<NodeIndex>,
 }
 
-fn check_edge(dir: &DirectoryNode, child: &Directory) -> Result<(), Error> {
+fn check_edge(dir: &DirectoryNode, dir_name: &[u8], child: &Directory) -> Result<(), Error> {
     // Ensure the size specified in the child node matches our records.
     if dir.size() != child.size() {
         return Err(Error::ValidationError(format!(
             "'{}' has wrong size, specified {}, recorded {}",
-            dir.get_name().as_bstr(),
+            dir_name.as_bstr(),
             dir.size(),
             child.size(),
         )));
@@ -141,19 +141,19 @@ impl<O: OrderValidator> DirectoryGraph<O> {
         }
 
         // set up edges to all child directories
-        for subdir in directory.directories() {
+        for (subdir_name, subdir_node) in directory.directories() {
             let child_ix = *self
                 .digest_to_node_ix
-                .entry(subdir.digest().clone())
+                .entry(subdir_node.digest().clone())
                 .or_insert_with(|| self.graph.add_node(None));
 
             let pending_edge_check = match &self.graph[child_ix] {
                 Some(child) => {
                     // child is already available, validate the edge now
-                    check_edge(subdir, child)?;
+                    check_edge(subdir_node, subdir_name, child)?;
                     None
                 }
-                None => Some(subdir.clone()), // pending validation
+                None => Some(subdir_node.clone()), // pending validation
             };
             self.graph.add_edge(ix, child_ix, pending_edge_check);
         }
@@ -173,7 +173,9 @@ impl<O: OrderValidator> DirectoryGraph<O> {
                 .expect("edge not found")
                 .take()
                 .expect("edge is already validated");
-            check_edge(&edge_weight, &directory)?;
+
+            // TODO: where's the name here?
+            check_edge(&edge_weight, b"??", &directory)?;
         }
 
         // finally, store the directory information in the node weight
@@ -277,11 +279,12 @@ mod tests {
     lazy_static! {
         pub static ref BROKEN_PARENT_DIRECTORY: Directory = {
             let mut dir = Directory::new();
-            dir.add(Node::Directory(DirectoryNode::new(
+            dir.add(
                 "foo".into(),
-                DIRECTORY_A.digest(),
-                DIRECTORY_A.size() + 42, // wrong!
-            ).unwrap())).unwrap();
+                Node::Directory(DirectoryNode::new(
+                    DIRECTORY_A.digest(),
+                    DIRECTORY_A.size() + 42, // wrong!
+                ))).unwrap();
             dir
         };
     }
diff --git a/tvix/castore/src/directoryservice/grpc.rs b/tvix/castore/src/directoryservice/grpc.rs
index 2079514d2b79..9696c5631949 100644
--- a/tvix/castore/src/directoryservice/grpc.rs
+++ b/tvix/castore/src/directoryservice/grpc.rs
@@ -3,8 +3,7 @@ use std::collections::HashSet;
 use super::{Directory, DirectoryPutter, DirectoryService};
 use crate::composition::{CompositionContext, ServiceBuilder};
 use crate::proto::{self, get_directory_request::ByWhat};
-use crate::ValidateDirectoryError;
-use crate::{B3Digest, Error};
+use crate::{B3Digest, DirectoryError, Error};
 use async_stream::try_stream;
 use futures::stream::BoxStream;
 use std::sync::Arc;
@@ -154,7 +153,7 @@ where
                         }
 
                         let directory = directory.try_into()
-                            .map_err(|e: ValidateDirectoryError| Error::StorageError(e.to_string()))?;
+                            .map_err(|e: DirectoryError| Error::StorageError(e.to_string()))?;
 
                         yield directory;
                     },
diff --git a/tvix/castore/src/directoryservice/order_validator.rs b/tvix/castore/src/directoryservice/order_validator.rs
index f03bb7ff9a86..6daf4a624d29 100644
--- a/tvix/castore/src/directoryservice/order_validator.rs
+++ b/tvix/castore/src/directoryservice/order_validator.rs
@@ -48,9 +48,9 @@ impl RootToLeavesValidator {
             self.expected_digests.insert(directory.digest());
         }
 
-        for subdir in directory.directories() {
+        for (_, subdir_node) in directory.directories() {
             // Allow the children to appear next
-            self.expected_digests.insert(subdir.digest().clone());
+            self.expected_digests.insert(subdir_node.digest().clone());
         }
     }
 }
@@ -79,11 +79,11 @@ impl OrderValidator for LeavesToRootValidator {
     fn add_directory(&mut self, directory: &Directory) -> bool {
         let digest = directory.digest();
 
-        for subdir in directory.directories() {
-            if !self.allowed_references.contains(subdir.digest()) {
+        for (_, subdir_node) in directory.directories() {
+            if !self.allowed_references.contains(subdir_node.digest()) {
                 warn!(
                     directory.digest = %digest,
-                    subdirectory.digest = %subdir.digest(),
+                    subdirectory.digest = %subdir_node.digest(),
                     "unexpected directory reference"
                 );
                 return false;
diff --git a/tvix/castore/src/directoryservice/tests/mod.rs b/tvix/castore/src/directoryservice/tests/mod.rs
index 7a15f44a686e..c0a0f22c989f 100644
--- a/tvix/castore/src/directoryservice/tests/mod.rs
+++ b/tvix/castore/src/directoryservice/tests/mod.rs
@@ -218,14 +218,13 @@ async fn upload_reject_dangling_pointer(directory_service: impl DirectoryService
 async fn upload_reject_wrong_size(directory_service: impl DirectoryService) {
     let wrong_parent_directory = {
         let mut dir = Directory::new();
-        dir.add(Node::Directory(
-            DirectoryNode::new(
-                "foo".into(),
+        dir.add(
+            "foo".into(),
+            Node::Directory(DirectoryNode::new(
                 DIRECTORY_A.digest(),
                 DIRECTORY_A.size() + 42, // wrong!
-            )
-            .unwrap(),
-        ))
+            )),
+        )
         .unwrap();
         dir
     };
diff --git a/tvix/castore/src/directoryservice/traverse.rs b/tvix/castore/src/directoryservice/traverse.rs
index ff667e0f65c0..de2ea33c7f9d 100644
--- a/tvix/castore/src/directoryservice/traverse.rs
+++ b/tvix/castore/src/directoryservice/traverse.rs
@@ -1,4 +1,4 @@
-use crate::{directoryservice::DirectoryService, Error, NamedNode, Node, Path};
+use crate::{directoryservice::DirectoryService, Error, Node, Path};
 use tracing::{instrument, warn};
 
 /// This descends from a (root) node to the given (sub)path, returning the Node
@@ -37,9 +37,10 @@ where
                     })?;
 
                 // look for the component in the [Directory].
-                // FUTUREWORK: as the nodes() iterator returns in a sorted fashion, we
-                // could stop as soon as e.name is larger than the search string.
-                if let Some(child_node) = directory.nodes().find(|n| n.get_name() == component) {
+                if let Some((_child_name, child_node)) = directory
+                    .nodes()
+                    .find(|(name, _node)| name.as_ref() == component)
+                {
                     // child node found, update prev_node to that and continue.
                     parent_node = child_node.clone();
                 } else {
@@ -81,21 +82,23 @@ mod tests {
         handle.close().await.expect("must upload");
 
         // construct the node for DIRECTORY_COMPLICATED
-        let node_directory_complicated = Node::Directory(
-            DirectoryNode::new(
-                "doesntmatter".into(),
-                DIRECTORY_COMPLICATED.digest(),
-                DIRECTORY_COMPLICATED.size(),
-            )
-            .unwrap(),
-        );
+        let node_directory_complicated = Node::Directory(DirectoryNode::new(
+            DIRECTORY_COMPLICATED.digest(),
+            DIRECTORY_COMPLICATED.size(),
+        ));
 
         // construct the node for DIRECTORY_COMPLICATED
-        let node_directory_with_keep =
-            Node::Directory(DIRECTORY_COMPLICATED.directories().next().unwrap().clone());
+        let node_directory_with_keep = Node::Directory(
+            DIRECTORY_COMPLICATED
+                .directories()
+                .next()
+                .unwrap()
+                .1
+                .clone(),
+        );
 
         // construct the node for the .keep file
-        let node_file_keep = Node::File(DIRECTORY_WITH_KEEP.files().next().unwrap().clone());
+        let node_file_keep = Node::File(DIRECTORY_WITH_KEEP.files().next().unwrap().1.clone());
 
         // traversal to an empty subpath should return the root node.
         {
diff --git a/tvix/castore/src/directoryservice/utils.rs b/tvix/castore/src/directoryservice/utils.rs
index f1133663ede7..4b060707d4cc 100644
--- a/tvix/castore/src/directoryservice/utils.rs
+++ b/tvix/castore/src/directoryservice/utils.rs
@@ -57,16 +57,15 @@ pub fn traverse_directory<'a, DS: DirectoryService + 'static>(
             // enqueue all child directory digests to the work queue, as
             // long as they're not part of the worklist or already sent.
             // This panics if the digest looks invalid, it's supposed to be checked first.
-            for child_directory_node in current_directory.directories() {
-                // TODO: propagate error
-                let child_digest: B3Digest = child_directory_node.digest().clone();
+            for (_, child_directory_node) in current_directory.directories() {
+                let child_digest = child_directory_node.digest();
 
-                if worklist_directory_digests.contains(&child_digest)
-                    || sent_directory_digests.contains(&child_digest)
+                if worklist_directory_digests.contains(child_digest)
+                    || sent_directory_digests.contains(child_digest)
                 {
                     continue;
                 }
-                worklist_directory_digests.push_back(child_digest);
+                worklist_directory_digests.push_back(child_digest.clone());
             }
 
             yield current_directory;