refactor(tvix/castore): add try_into_anonymous_node, rename to try_* r/8836

We have two places where we parse protos and want their names to be
empty:

 - Receiving a root node in a nar-bridge NAR request
 - Processing the CalculateNAR gRPC call

We don't have any place where we want to keep a name as bytes::Bytes
around, yet we used the `into_name_bytes_and_node` method.

It was also a bit wrongly named - it wasn't very clear the name was
not validated, and that the function may fail.

This moves the "splitting off the name as bytes::Bytes" part into a
private helper, only leaving the `try_into_name_and_node` and
`try_into_anonymous_node` methods around.

Change-Id: I2c7fd9871d49ec67450d7efa6a30d96197fb319c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12664
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: Marijan Petričević <marijan.petricevic94@gmail.com>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>

10 files changed, 59 insertions, 31 deletions

diff --git a/tvix/build/src/buildservice/oci.rs b/tvix/build/src/buildservice/oci.rs
index 7b88518e924f..89efbb4285d0 100644
--- a/tvix/build/src/buildservice/oci.rs
+++ b/tvix/build/src/buildservice/oci.rs
@@ -131,7 +131,7 @@ where
         let root_nodes: BTreeMap<PathComponent, Node> =
             BTreeMap::from_iter(request.inputs.iter().map(|input| {
                 // We know from validation this is Some.
-                input.clone().into_name_and_node().unwrap()
+                input.clone().try_into_name_and_node().unwrap()
             }));
         let patterns = ReferencePattern::new(request.refscan_needles.clone());
         // NOTE: impl Drop for FuseDaemon unmounts, so if the call is cancelled, umount.
diff --git a/tvix/build/src/oci/spec.rs b/tvix/build/src/oci/spec.rs
index ce70ad91e9a9..e80e442f0350 100644
--- a/tvix/build/src/oci/spec.rs
+++ b/tvix/build/src/oci/spec.rs
@@ -266,7 +266,7 @@ fn configure_mounts<'a>(
     for input in inputs {
         let (input_name, _input) = input
             .clone()
-            .into_name_and_node()
+            .try_into_name_and_node()
             .expect("invalid input name");
 
         let input_name = std::str::from_utf8(input_name.as_ref()).expect("invalid input name");
diff --git a/tvix/build/src/proto/mod.rs b/tvix/build/src/proto/mod.rs
index e6c94f5b56c9..0e106bb4cf63 100644
--- a/tvix/build/src/proto/mod.rs
+++ b/tvix/build/src/proto/mod.rs
@@ -128,7 +128,7 @@ impl TryFrom<BuildRequest> for crate::buildservice::BuildRequest {
         for (i, node) in value.inputs.iter().enumerate() {
             let (name, node) = node
                 .clone()
-                .into_name_and_node()
+                .try_into_name_and_node()
                 .map_err(|e| ValidateBuildRequestError::InvalidInputNode(i, e))?;
 
             if name.as_ref() <= last_name.as_ref() {
diff --git a/tvix/castore/src/errors.rs b/tvix/castore/src/errors.rs
index bf8fd236dbef..1c4605200842 100644
--- a/tvix/castore/src/errors.rs
+++ b/tvix/castore/src/errors.rs
@@ -52,6 +52,10 @@ pub enum DirectoryError {
     /// Invalid name encountered
     #[error("Invalid name: {0}")]
     InvalidName(PathComponentError),
+    /// This can occur if a protobuf node with a name is passed where we expect
+    /// it to be anonymous.
+    #[error("Name is set when it shouldn't")]
+    NameInAnonymousNode,
     /// Elements are not in sorted order. Can only happen on protos
     #[error("{:?} is not sorted", .0.as_bstr())]
     WrongSorting(bytes::Bytes),
diff --git a/tvix/castore/src/proto/mod.rs b/tvix/castore/src/proto/mod.rs
index 15e55ad049cf..89c68a4ad97b 100644
--- a/tvix/castore/src/proto/mod.rs
+++ b/tvix/castore/src/proto/mod.rs
@@ -124,7 +124,7 @@ impl TryFrom<Directory> for crate::Directory {
                 Node {
                     node: Some(node::Node::Directory(e)),
                 }
-                .into_name_and_node()?,
+                .try_into_name_and_node()?,
             );
         }
 
@@ -133,7 +133,7 @@ impl TryFrom<Directory> for crate::Directory {
                 Node {
                     node: Some(node::Node::File(e)),
                 }
-                .into_name_and_node()?,
+                .try_into_name_and_node()?,
             )
         }
 
@@ -142,7 +142,7 @@ impl TryFrom<Directory> for crate::Directory {
                 Node {
                     node: Some(node::Node::Symlink(e)),
                 }
-                .into_name_and_node()?,
+                .try_into_name_and_node()?,
             )
         }
 
@@ -191,22 +191,20 @@ impl From<crate::Directory> for Directory {
 }
 
 impl Node {
-    /// Converts a proto [Node] to a [crate::Node], and splits off the name.
-    pub fn into_name_and_node(self) -> Result<(PathComponent, crate::Node), DirectoryError> {
-        let (unvalidated_name, node) = self.into_name_bytes_and_node()?;
+    /// Converts a proto [Node] to a [crate::Node], and splits off the name as a [PathComponent].
+    pub fn try_into_name_and_node(self) -> Result<(PathComponent, crate::Node), DirectoryError> {
+        let (name_bytes, node) = self.try_into_unchecked_name_and_checked_node()?;
         Ok((
-            unvalidated_name
-                .try_into()
-                .map_err(DirectoryError::InvalidName)?,
+            name_bytes.try_into().map_err(DirectoryError::InvalidName)?,
             node,
         ))
     }
 
-    /// Converts a proto [Node] to a [crate::Node], and splits off the name and returns it as a
-    /// [bytes::Bytes].
-    ///
-    /// Note: the returned name is not validated.
-    pub fn into_name_bytes_and_node(self) -> Result<(bytes::Bytes, crate::Node), DirectoryError> {
+    /// Converts a proto [Node] to a [crate::Node], and splits off the name as a
+    /// [bytes::Bytes] without doing any checking of it.
+    fn try_into_unchecked_name_and_checked_node(
+        self,
+    ) -> Result<(bytes::Bytes, crate::Node), DirectoryError> {
         match self.node.ok_or_else(|| DirectoryError::NoNodeSet)? {
             node::Node::Directory(n) => {
                 let digest = B3Digest::try_from(n.digest)
@@ -247,6 +245,20 @@ impl Node {
         }
     }
 
+    /// Converts a proto [Node] to a [crate::Node], and splits off the name and returns it as a
+    /// [bytes::Bytes].
+    ///
+    /// The name must be empty.
+    pub fn try_into_anonymous_node(self) -> Result<crate::Node, DirectoryError> {
+        let (name, node) = Self::try_into_unchecked_name_and_checked_node(self)?;
+
+        if !name.is_empty() {
+            return Err(DirectoryError::NameInAnonymousNode);
+        }
+
+        Ok(node)
+    }
+
     /// Constructs a [Node] from a name and [crate::Node].
     /// The name is a [bytes::Bytes], not a [PathComponent], as we have use an
     /// empty name in some places.
diff --git a/tvix/castore/src/proto/tests/mod.rs b/tvix/castore/src/proto/tests/mod.rs
index 8efb92870374..9f6330914bff 100644
--- a/tvix/castore/src/proto/tests/mod.rs
+++ b/tvix/castore/src/proto/tests/mod.rs
@@ -1,4 +1,5 @@
 use super::{node, Node, SymlinkNode};
+use crate::DirectoryError;
 
 mod directory;
 
@@ -11,7 +12,7 @@ fn convert_symlink_empty_target_invalid() {
             target: "".into(),
         })),
     }
-    .into_name_and_node()
+    .try_into_name_and_node()
     .expect_err("must fail validation");
 }
 
@@ -25,6 +26,22 @@ fn convert_symlink_target_null_byte_invalid() {
             target: "foo\0".into(),
         })),
     }
-    .into_name_and_node()
+    .try_into_name_and_node()
     .expect_err("must fail validation");
 }
+
+/// Create a node with a name, and ensure our ano
+#[test]
+fn convert_anonymous_with_name_fail() {
+    assert_eq!(
+        DirectoryError::NameInAnonymousNode,
+        Node {
+            node: Some(node::Node::Symlink(SymlinkNode {
+                name: "foo".into(),
+                target: "somewhereelse".into(),
+            })),
+        }
+        .try_into_anonymous_node()
+        .expect_err("must fail")
+    )
+}
diff --git a/tvix/glue/src/tvix_store_io.rs b/tvix/glue/src/tvix_store_io.rs
index 839d2ae85845..fa3326ff937c 100644
--- a/tvix/glue/src/tvix_store_io.rs
+++ b/tvix/glue/src/tvix_store_io.rs
@@ -292,9 +292,9 @@ impl TvixStoreIO {
                             .zip(build_result.outputs_needles.iter())
                             .zip(drv.outputs.iter())
                         {
-                            let (_, output_node) = output
+                            let output_node = output
                                 .clone()
-                                .into_name_bytes_and_node()
+                                .try_into_anonymous_node()
                                 .expect("invalid node");
 
                             let output_needles: Vec<_> = output_needles
@@ -365,7 +365,7 @@ impl TvixStoreIO {
                         build_result
                             .outputs
                             .into_iter()
-                            .map(|e| e.into_name_and_node().expect("invalid node"))
+                            .map(|e| e.try_into_name_and_node().expect("invalid node"))
                             .find(|(output_name, _output_node)| {
                                 output_name.as_ref() == s.as_bytes()
                             })
diff --git a/tvix/nar-bridge/src/nar.rs b/tvix/nar-bridge/src/nar.rs
index d88719b02be9..abc0d854d7c7 100644
--- a/tvix/nar-bridge/src/nar.rs
+++ b/tvix/nar-bridge/src/nar.rs
@@ -56,16 +56,11 @@ pub async fn get_head(
             StatusCode::NOT_FOUND
         })?;
 
-    let (root_name, root_node) = root_node.into_name_bytes_and_node().map_err(|e| {
+    let root_node = root_node.try_into_anonymous_node().map_err(|e| {
         warn!(err=%e, "root node validation failed");
         StatusCode::BAD_REQUEST
     })?;
 
-    if !root_name.as_ref().is_empty() {
-        warn!("root node has name, which it shouldn't");
-        return Err(StatusCode::BAD_REQUEST);
-    }
-
     Ok((
         // headers
         [
diff --git a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
index d003d4bdb728..4325e79d5077 100644
--- a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
+++ b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
@@ -77,9 +77,9 @@ where
         &self,
         request: Request<castorepb::Node>,
     ) -> Result<Response<proto::CalculateNarResponse>> {
-        let (_, root_node) = request
+        let root_node = request
             .into_inner()
-            .into_name_bytes_and_node()
+            .try_into_anonymous_node()
             .map_err(|e| {
                 warn!(err = %e, "invalid root node");
                 Status::invalid_argument("invalid root node")
diff --git a/tvix/store/src/proto/mod.rs b/tvix/store/src/proto/mod.rs
index 807f03854ddc..d9edb81f655d 100644
--- a/tvix/store/src/proto/mod.rs
+++ b/tvix/store/src/proto/mod.rs
@@ -274,7 +274,7 @@ impl TryFrom<PathInfo> for crate::pathinfoservice::PathInfo {
         let (name, node) = value
             .node
             .ok_or_else(|| ValidatePathInfoError::NoNodePresent)?
-            .into_name_and_node()
+            .try_into_name_and_node()
             .map_err(ValidatePathInfoError::InvalidRootNode)?;
 
         Ok(Self {