From 0f3ec83172b38684c8ec74a33f8db4fb9a79df2f Mon Sep 17 00:00:00 2001
From: Dmitry Rodionov <dmitry@neon.tech>
Date: Tue, 3 May 2022 17:16:46 +0300
Subject: [PATCH] avoid detach with alive branches

---
 pageserver/src/layered_repository.rs           | 15 ++++++++++++++-
 .../batch_others/test_ancestor_branch.py       | 18 +++++++++++++++---
 .../batch_others/test_tenant_relocation.py     |  7 ++++---
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/pageserver/src/layered_repository.rs b/pageserver/src/layered_repository.rs
index e678c8f4cb..69271467a6 100644
--- a/pageserver/src/layered_repository.rs
+++ b/pageserver/src/layered_repository.rs
@@ -393,9 +393,22 @@ impl Repository for LayeredRepository {
 
     fn detach_timeline(&self, timeline_id: ZTimelineId) -> anyhow::Result<()> {
         let mut timelines = self.timelines.lock().unwrap();
+        // check no child timelines, because detach will remove files, which will brake child branches
+        // FIXME this can still be violated because we do not guarantee
+        //   that all ancestors are downloaded/attached to the same pageserver
+        let num_children = timelines
+            .iter()
+            .filter(|(_, entry)| entry.ancestor_timeline_id() == Some(timeline_id))
+            .count();
+
+        ensure!(
+            num_children == 0,
+            "Cannot detach timeline which has child timelines"
+        );
+
         ensure!(
             timelines.remove(&timeline_id).is_some(),
-            "cannot detach timeline {timeline_id} that is not available locally"
+            "Cannot detach timeline {timeline_id} that is not available locally"
         );
         Ok(())
     }
diff --git a/test_runner/batch_others/test_ancestor_branch.py b/test_runner/batch_others/test_ancestor_branch.py
index 75fe3cde0f..d6b073492d 100644
--- a/test_runner/batch_others/test_ancestor_branch.py
+++ b/test_runner/batch_others/test_ancestor_branch.py
@@ -1,11 +1,9 @@
-import subprocess
-import asyncio
 from contextlib import closing
 
 import psycopg2.extras
 import pytest
 from fixtures.log_helper import log
-from fixtures.zenith_fixtures import ZenithEnvBuilder
+from fixtures.zenith_fixtures import ZenithEnv, ZenithEnvBuilder, ZenithPageserverApiException
 
 
 #
@@ -120,3 +118,17 @@ def test_ancestor_branch(zenith_env_builder: ZenithEnvBuilder):
 
     branch2_cur.execute('SELECT count(*) FROM foo')
     assert branch2_cur.fetchone() == (300000, )
+
+
+def test_ancestor_branch_detach(zenith_simple_env: ZenithEnv):
+    env = zenith_simple_env
+
+    parent_timeline_id = env.zenith_cli.create_branch("test_ancestor_branch_detach_parent", "empty")
+
+    env.zenith_cli.create_branch("test_ancestor_branch_detach_branch1",
+                                 "test_ancestor_branch_detach_parent")
+
+    ps_http = env.pageserver.http_client()
+    with pytest.raises(ZenithPageserverApiException,
+                       match="Failed to detach inmem tenant timeline"):
+        ps_http.timeline_detach(env.initial_tenant, parent_timeline_id)
diff --git a/test_runner/batch_others/test_tenant_relocation.py b/test_runner/batch_others/test_tenant_relocation.py
index 41907adf1a..7e71c0a157 100644
--- a/test_runner/batch_others/test_tenant_relocation.py
+++ b/test_runner/batch_others/test_tenant_relocation.py
@@ -109,10 +109,11 @@ def test_tenant_relocation(zenith_env_builder: ZenithEnvBuilder,
 
     tenant = env.zenith_cli.create_tenant(UUID("74ee8b079a0e437eb0afea7d26a07209"))
     log.info("tenant to relocate %s", tenant)
-    env.zenith_cli.create_root_branch('main', tenant_id=tenant)
-    env.zenith_cli.create_branch('test_tenant_relocation', tenant_id=tenant)
 
-    tenant_pg = env.postgres.create_start(branch_name='main',
+    # attach does not download ancestor branches (should it?), just use root branch for now
+    env.zenith_cli.create_root_branch('test_tenant_relocation', tenant_id=tenant)
+
+    tenant_pg = env.postgres.create_start(branch_name='test_tenant_relocation',
                                           node_name='test_tenant_relocation',
                                           tenant_id=tenant)