fix: avoid retrying remote branch deletions (#4854)

docs/src/python/python.md

@@ -813,8 +813,8 @@ so the fetch APIs cannot read their bytes yet.
 ## Exceptions
 
 Remote HTTP responses with structured resource details raise a subclass of
-`HttpError`. Older servers that omit those details raise plain `HttpError`, so
-`except HttpError` remains a compatible catch-all:
+`HttpError`. Older servers that omit those details generally raise plain
+`HttpError`, so `except HttpError` remains a compatible catch-all:
 
 ```python
 from lancedb.remote.errors import (
@@ -835,6 +835,12 @@ except HttpError as error:
     print("request failed:", error.request_id, error.status_code)
 ```
 
+Remote branch deletion sends a single request. If the server rejects it with
+HTTP 409, it raises `ConflictError` with the server's reason, including when an
+older server omits structured resource details. A structured `already_exists`
+response keeps its more specific `AlreadyExistsError` type. Both are subclasses
+of `HttpError`.
+
 SQL response failures raise `SqlError`, retaining the numeric `grpc_code` and
 `request_id`. Servers that report a recognized `x-lancedb-error-code` select a
 specific input-error subclass. HTTP failures use the corresponding `HttpError`
@@ -900,6 +906,8 @@ generic HTTP/SQL exceptions with their transport status and current request ID.
 
 ::: lancedb.remote.errors.AlreadyExistsError
 
+::: lancedb.remote.errors.ConflictError
+
 ::: lancedb.exceptions.MissingValueError
 
 ::: lancedb.exceptions.MissingColumnError

python/python/lancedb/remote/errors.py

@@ -149,6 +149,8 @@ def _http_error_class(
             return input_class
     if error_code == "already_exists":
         return AlreadyExistsError
+    if status_code == 409:
+        return ConflictError
     if error_code != "not_found" or resource_kind is None:
         return None
     return {
@@ -268,6 +270,22 @@ class AlreadyExistsError(HttpError):
     """A remote resource already exists."""
 
 
+class ConflictError(HttpError):
+    """A remote operation conflicts with the current state of a resource.
+
+    For example, deleting a branch whose files are still needed by another
+    branch raises this error. The server's reason is included in the message.
+
+    Examples
+    --------
+    >>> error = HttpError("main depends on the branch's files", "req-1", 409)
+    >>> isinstance(error, ConflictError)
+    True
+    >>> error.status_code
+    409
+    """
+
+
 class TableNotFoundError(HttpError, ValueError, _NamespaceTableNotFoundError):
     """A remote table was not found.
 

python/python/tests/test_errors.py

@@ -10,6 +10,7 @@ from lance_namespace.errors import TableNotFoundError as NamespaceTableNotFoundE
 from lancedb.db import DBConnection
 from lancedb.remote.errors import (
     AlreadyExistsError,
+    ConflictError,
     FunctionNotFoundError,
     HttpError,
     LanceDBClientError,
@@ -72,6 +73,20 @@ def test_pickle_http_transport_error():
     assert restored.request_id == "req-789"
 
 
+@pytest.mark.parametrize("error_code", [None, "conflict", "future_conflict_code"])
+def test_http_conflict_error_preserves_reason_and_metadata(error_code):
+    reason = "Conflict: branch 'exp' cannot be deleted, main depends on its files"
+    original = HttpError(reason, "req-conflict", 409, error_code)
+    for error in [original, pickle.loads(pickle.dumps(original))]:
+        assert type(error) is ConflictError
+        assert isinstance(error, HttpError)
+        assert str(error) == reason
+        assert error.request_id == "req-conflict"
+        assert error.status_code == 409
+        assert error.error_code == error_code
+        assert error.is_transport_error is False
+
+
 @pytest.mark.parametrize(
     "error_code,resource_kind,status_code,expected_type",
     [
@@ -251,6 +266,9 @@ def test_sql_errors_use_only_server_codes(code, category, type_name):
         (400, "schema_mismatch", "SchemaMismatchError"),
         (404, "table_not_found", "TableNotFoundError"),
         (404, "reference_not_found", "ReferenceNotFoundError"),
+        (409, None, "ConflictError"),
+        (409, "conflict", "ConflictError"),
+        (500, "conflict", "HttpError"),
         (500, "column_not_found", "HttpError"),
         (503, "table_not_found", "HttpError"),
     ],

python/python/tests/test_remote_db.py

@@ -45,6 +45,7 @@ from lancedb.query import (
 from lancedb.remote import ClientConfig
 from lancedb.remote.errors import (
     AlreadyExistsError,
+    ConflictError,
     HttpError,
     NamespaceNotFoundError,
     RetryError,
@@ -733,6 +734,43 @@ def test_remote_table_branches_sync():
         table.branches.delete("exp")
 
 
+@pytest.mark.asyncio
+@pytest.mark.parametrize("use_async", [False, True])
+@pytest.mark.parametrize("json_response", [False, True])
+async def test_remote_branch_delete_conflict_is_not_retried(use_async, json_response):
+    request_ids = []
+    reason = "Conflict: branch 'exp' cannot be deleted, main depends on its files"
+
+    def handler(request):
+        if request.path == "/v1/table/test/describe/":
+            _branch_open_handler(request)
+            return
+
+        assert request.path == "/v1/table/test/branches/delete/"
+        content_len = int(request.headers["Content-Length"])
+        assert json.loads(request.rfile.read(content_len)) == {"name": "exp"}
+        request_ids.append(request.headers["x-request-id"])
+        request.send_response(409)
+        request.end_headers()
+        body = json.dumps({"code": 14, "error": reason}) if json_response else reason
+        request.wfile.write(body.encode())
+
+    if use_async:
+        async with mock_lancedb_connection_async(handler) as db:
+            table = await db.open_table("test")
+            with pytest.raises(ConflictError, match=re.escape(reason)) as exc_info:
+                await table.branches.delete("exp")
+    else:
+        with mock_lancedb_connection(handler) as db:
+            table = db.open_table("test")
+            with pytest.raises(ConflictError, match=re.escape(reason)) as exc_info:
+                table.branches.delete("exp")
+
+    assert request_ids == [exc_info.value.request_id]
+    assert exc_info.value.status_code == 409
+    assert isinstance(exc_info.value, HttpError)
+
+
 def test_remote_table_cherry_pick_defaults_to_execute():
     cherry_pick_bodies = []
     diff = {

rust/lancedb/src/remote/table.rs

@@ -2796,7 +2796,9 @@ impl<S: HttpSend> BaseTable for RemoteTable<S> {
             .client
             .post(&format!("/v1/table/{}/branches/delete/", self.identifier))
             .json(&serde_json::json!({ "name": name }));
-        let (request_id, response) = self.send(request, true).await?;
+        // Deletion is not idempotent: preserve conflicts and partial failures
+        // instead of replaying a request that may already have removed the ref.
+        let (request_id, response) = self.send(request, false).await?;
         if response.status() == StatusCode::NOT_FOUND {
             return Err(Error::TableNotFound {
                 name: format!("{} (branch: {})", self.name, name),
@@ -16007,6 +16009,73 @@ mod tests {
         assert!(matches!(err, Error::TableNotFound { .. }), "got {err:?}");
     }
 
+    #[tokio::test]
+    async fn test_delete_branch_conflict_is_not_retried() {
+        let requests = Arc::new(AtomicUsize::new(0));
+        let captured = requests.clone();
+        let reason = "Conflict: branch 'exp' cannot be deleted, main depends on its files";
+        let table = Table::new_with_handler("my_table", move |request| {
+            assert_eq!(request.url().path(), "/v1/table/my_table/branches/delete/");
+            captured.fetch_add(1, Ordering::SeqCst);
+            http::Response::builder()
+                .status(409)
+                .body(json!({ "code": 14, "error": reason }).to_string())
+                .unwrap()
+        });
+
+        let err = table.delete_branch("exp").await.unwrap_err();
+        assert_eq!(requests.load(Ordering::SeqCst), 1);
+        assert!(err.to_string().contains(reason), "got {err:?}");
+        assert!(
+            matches!(
+                err,
+                Error::Http {
+                    status_code: Some(StatusCode::CONFLICT),
+                    ..
+                }
+            ),
+            "expected the original conflict, got {err:?}"
+        );
+    }
+
+    #[tokio::test]
+    async fn test_delete_branch_partial_failure_is_not_retried() {
+        let requests = Arc::new(AtomicUsize::new(0));
+        let captured = requests.clone();
+        let table = Table::new_with_handler("my_table", move |_| {
+            // The ref was removed before the first response failed. Replaying
+            // the deletion would hide that failure behind a missing-branch error.
+            if captured.fetch_add(1, Ordering::SeqCst) == 0 {
+                http::Response::builder()
+                    .status(500)
+                    .body("failed after removing branch ref")
+                    .unwrap()
+            } else {
+                http::Response::builder()
+                    .status(404)
+                    .body("no such branch")
+                    .unwrap()
+            }
+        });
+
+        let err = table.delete_branch("exp").await.unwrap_err();
+        assert_eq!(requests.load(Ordering::SeqCst), 1);
+        assert!(
+            err.to_string().contains("failed after removing branch ref"),
+            "got {err:?}"
+        );
+        assert!(
+            matches!(
+                err,
+                Error::Http {
+                    status_code: Some(StatusCode::INTERNAL_SERVER_ERROR),
+                    ..
+                }
+            ),
+            "expected the original server error, got {err:?}"
+        );
+    }
+
     fn sample_branch_diff_json() -> &'static str {
         r#"{
             "fromBranch":"exp",