From bd1def3ec4dce82fb069815ab5de29f359bae78d Mon Sep 17 00:00:00 2001
From: Florian Klink <flokli@flokli.de>
Date: Fri, 8 Mar 2024 23:04:35 +0200
Subject: [PATCH] fix(tvix/store/grpc/pathinfo): skip_all fields, handle errors

request only contains the outer metadata wrapping, and that's not too
interesting:

> Request { metadata: MetadataMap { headers: {"content-type":
> "application/grpc", "user-agent": "grpc-go/1.60.1", "te": "trailers",
> "grpc-accept-encoding": "gzip"} }, message: Streaming, extensions:
> Extensions }

Drop these fields for now, and rely on the underlying implementations to
add instrumentation for the application-specific fields.

Also, ensure we handle all error cases properly, and log them. We
don't use `err` from instrument, as that'd also log an error on
`Status::not_found`.

Change-Id: Id1b983cb8b059c148c8a376f8802a1d28c59ba97
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11103
Reviewed-by: Connor Brewster <cbrewster@hey.com>
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
---
 .../src/proto/grpc_pathinfoservice_wrapper.rs | 35 +++++++++++--------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
index a5c5776cd..9f4581822 100644
--- a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
+++ b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
@@ -27,7 +27,7 @@ where
 {
     type ListStream = BoxStream<'static, tonic::Result<proto::PathInfo, Status>>;
 
-    #[instrument(skip(self))]
+    #[instrument(skip_all)]
     async fn get(
         &self,
         request: Request<proto::GetPathInfoRequest>,
@@ -43,7 +43,7 @@ where
                     Ok(None) => Err(Status::not_found("PathInfo not found")),
                     Ok(Some(path_info)) => Ok(Response::new(path_info)),
                     Err(e) => {
-                        warn!("failed to retrieve PathInfo: {}", e);
+                        warn!(err = %e, "failed to get PathInfo");
                         Err(e.into())
                     }
                 }
@@ -51,7 +51,7 @@ where
         }
     }
 
-    #[instrument(skip(self))]
+    #[instrument(skip_all)]
     async fn put(&self, request: Request<proto::PathInfo>) -> Result<Response<proto::PathInfo>> {
         let path_info = request.into_inner();
 
@@ -60,13 +60,13 @@ where
         match self.inner.put(path_info).await {
             Ok(path_info_new) => Ok(Response::new(path_info_new)),
             Err(e) => {
-                warn!("failed to insert PathInfo: {}", e);
+                warn!(err = %e, "failed to put PathInfo");
                 Err(e.into())
             }
         }
     }
 
-    #[instrument(skip(self))]
+    #[instrument(skip_all)]
     async fn calculate_nar(
         &self,
         request: Request<castorepb::Node>,
@@ -74,21 +74,26 @@ where
         match request.into_inner().node {
             None => Err(Status::invalid_argument("no root node sent")),
             Some(root_node) => {
-                let (nar_size, nar_sha256) = self
-                    .inner
-                    .calculate_nar(&root_node)
-                    .await
-                    .expect("error during nar calculation"); // TODO: handle error
+                if let Err(e) = root_node.validate() {
+                    warn!(err = %e, "invalid root node");
+                    Err(Status::invalid_argument("invalid root node"))?
+                }
 
-                Ok(Response::new(proto::CalculateNarResponse {
-                    nar_size,
-                    nar_sha256: nar_sha256.to_vec().into(),
-                }))
+                match self.inner.calculate_nar(&root_node).await {
+                    Ok((nar_size, nar_sha256)) => Ok(Response::new(proto::CalculateNarResponse {
+                        nar_size,
+                        nar_sha256: nar_sha256.to_vec().into(),
+                    })),
+                    Err(e) => {
+                        warn!(err = %e, "error during NAR calculation");
+                        Err(e.into())
+                    }
+                }
             }
         }
     }
 
-    #[instrument(skip(self))]
+    #[instrument(skip_all, err)]
     async fn list(
         &self,
         _request: Request<proto::ListPathInfoRequest>,