return proper http codes in timeline delete endpoint (#3876)

return proper http codes in timeline delete endpoint
+ fix openapi spec for detach to include 404 responses

pageserver/src/http/openapi_spec.yml

@@ -183,6 +183,12 @@ paths:
             application/json:
               schema:
                 $ref: "#/components/schemas/ForbiddenError"
+        "404":
+          description: Timeline not found
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/NotFoundError"
         "500":
           description: Generic operation error
           content:
@@ -383,6 +389,12 @@ paths:
             application/json:
               schema:
                 $ref: "#/components/schemas/ForbiddenError"
+        "404":
+          description: Tenant not found
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/NotFoundError"
         "500":
           description: Generic operation error
           content:

pageserver/src/http/routes.rs

@@ -131,6 +131,29 @@ impl From<TenantStateError> for ApiError {
     }
 }
 
+impl From<crate::tenant::DeleteTimelineError> for ApiError {
+    fn from(value: crate::tenant::DeleteTimelineError) -> Self {
+        use crate::tenant::DeleteTimelineError::*;
+        match value {
+            NotFound => ApiError::NotFound(anyhow::anyhow!("timeline not found")),
+            HasChildren => ApiError::BadRequest(anyhow::anyhow!(
+                "Cannot delete timeline which has child timelines"
+            )),
+            Other(e) => ApiError::InternalServerError(e),
+        }
+    }
+}
+
+impl From<crate::tenant::mgr::DeleteTimelineError> for ApiError {
+    fn from(value: crate::tenant::mgr::DeleteTimelineError) -> Self {
+        use crate::tenant::mgr::DeleteTimelineError::*;
+        match value {
+            Tenant(t) => ApiError::from(t),
+            Timeline(t) => ApiError::from(t),
+        }
+    }
+}
+
 // Helper function to construct a TimelineInfo struct for a timeline
 async fn build_timeline_info(
     timeline: &Arc<Timeline>,

pageserver/src/tenant.rs

@@ -431,6 +431,16 @@ remote:
     }
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum DeleteTimelineError {
+    #[error("NotFound")]
+    NotFound,
+    #[error("HasChildren")]
+    HasChildren,
+    #[error(transparent)]
+    Other(#[from] anyhow::Error),
+}
+
 struct RemoteStartupData {
     index_part: IndexPart,
     remote_metadata: TimelineMetadata,
@@ -1307,7 +1317,7 @@ impl Tenant {
         &self,
         timeline_id: TimelineId,
         _ctx: &RequestContext,
-    ) -> anyhow::Result<()> {
+    ) -> Result<(), DeleteTimelineError> {
         // Transition the timeline into TimelineState::Stopping.
         // This should prevent new operations from starting.
         let timeline = {
@@ -1319,13 +1329,13 @@ impl Tenant {
                 .iter()
                 .any(|(_, entry)| entry.get_ancestor_timeline_id() == Some(timeline_id));
 
-            anyhow::ensure!(
-                !children_exist,
-                "Cannot delete timeline which has child timelines"
-            );
+            if children_exist {
+                return Err(DeleteTimelineError::HasChildren);
+            }
+
             let timeline_entry = match timelines.entry(timeline_id) {
                 Entry::Occupied(e) => e,
-                Entry::Vacant(_) => bail!("timeline not found"),
+                Entry::Vacant(_) => return Err(DeleteTimelineError::NotFound),
             };
 
             let timeline = Arc::clone(timeline_entry.get());

pageserver/src/tenant/mgr.rs

@@ -321,11 +321,20 @@ pub async fn get_tenant(
     }
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum DeleteTimelineError {
+    #[error("Tenant {0}")]
+    Tenant(#[from] TenantStateError),
+
+    #[error("Timeline {0}")]
+    Timeline(#[from] crate::tenant::DeleteTimelineError),
+}
+
 pub async fn delete_timeline(
     tenant_id: TenantId,
     timeline_id: TimelineId,
     ctx: &RequestContext,
-) -> Result<(), TenantStateError> {
+) -> Result<(), DeleteTimelineError> {
     let tenant = get_tenant(tenant_id, true).await?;
     tenant.delete_timeline(timeline_id, ctx).await?;
     Ok(())

test_runner/regress/test_timeline_delete.py

@@ -25,9 +25,11 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
     with pytest.raises(
         PageserverApiException,
         match=f"NotFound: tenant {invalid_tenant_id}",
-    ):
+    ) as exc:
         ps_http.timeline_delete(tenant_id=invalid_tenant_id, timeline_id=invalid_timeline_id)
 
+    assert exc.value.status_code == 404
+
     # construct pair of branches to validate that pageserver prohibits
     # deletion of ancestor timelines when they have child branches
     parent_timeline_id = env.neon_cli.create_branch("test_ancestor_branch_delete_parent", "empty")
@@ -39,7 +41,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
     ps_http = env.pageserver.http_client()
     with pytest.raises(
         PageserverApiException, match="Cannot delete timeline which has child timelines"
-    ):
+    ) as exc:
         timeline_path = (
             env.repo_dir
             / "tenants"
@@ -53,6 +55,8 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
 
         assert not timeline_path.exists()
 
+    assert exc.value.status_code == 400
+
     timeline_path = (
         env.repo_dir / "tenants" / str(env.initial_tenant) / "timelines" / str(leaf_timeline_id)
     )
@@ -71,7 +75,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
     with pytest.raises(
         PageserverApiException,
         match=f"Timeline {env.initial_tenant}/{leaf_timeline_id} was not found",
-    ):
+    ) as exc:
         ps_http.timeline_detail(env.initial_tenant, leaf_timeline_id)
 
         # FIXME leaves tenant without timelines, should we prevent deletion of root timeline?
@@ -80,3 +84,5 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
             interval=0.2,
             func=lambda: ps_http.timeline_delete(env.initial_tenant, parent_timeline_id),
         )
+
+    assert exc.value.status_code == 404