From 0d7297718623f371226dd478109ccbcb6cfda53a Mon Sep 17 00:00:00 2001 From: "lancedb-gatefixer[bot]" <313497061+lancedb-gatefixer[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 13:20:20 -0800 Subject: [PATCH] fix: avoid retrying remote branch deletions (#4854) --- docs/src/python/python.md | 12 ++++- python/python/lancedb/remote/errors.py | 18 +++++++ python/python/tests/test_errors.py | 18 +++++++ python/python/tests/test_remote_db.py | 38 ++++++++++++++ rust/lancedb/src/remote/table.rs | 71 +++++++++++++++++++++++++- 5 files changed, 154 insertions(+), 3 deletions(-) diff --git a/docs/src/python/python.md b/docs/src/python/python.md index eb2c8d1db..3658f2ad3 100644 --- a/docs/src/python/python.md +++ b/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 diff --git a/python/python/lancedb/remote/errors.py b/python/python/lancedb/remote/errors.py index 56b5cc34d..4dc835e53 100644 --- a/python/python/lancedb/remote/errors.py +++ b/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. diff --git a/python/python/tests/test_errors.py b/python/python/tests/test_errors.py index 8721a2bb7..d5e92731f 100644 --- a/python/python/tests/test_errors.py +++ b/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"), ], diff --git a/python/python/tests/test_remote_db.py b/python/python/tests/test_remote_db.py index 994655ef9..8e6d74e86 100644 --- a/python/python/tests/test_remote_db.py +++ b/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 = { diff --git a/rust/lancedb/src/remote/table.rs b/rust/lancedb/src/remote/table.rs index 5dee8eb1d..895cc9f12 100644 --- a/rust/lancedb/src/remote/table.rs +++ b/rust/lancedb/src/remote/table.rs @@ -2796,7 +2796,9 @@ impl BaseTable for RemoteTable { .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",