From 02ed5cf12983c6df6124174a534ab514acfee361 Mon Sep 17 00:00:00 2001 From: "lancedb-gatefixer[bot]" <313497061+lancedb-gatefixer[bot]@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:16:13 -0700 Subject: [PATCH] fix: require checkout before restoring remote tables (#4406) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remote handles at the latest version have no checkout version, so `restore()` sent `{"version":null}` and surfaced a server deserialization error. Validate the checkout version before constructing the request and return `Error::InvalidInput` with the same message as local tables. This shared Rust fix covers Python sync/async and TypeScript callers. Add a regression test proving that restore sends no request on a fresh handle, after `checkout_latest()`, or after a successful restore clears checkout state. It also verifies that a valid restore sends the checked-out version. Update the schema-cache and branch restore tests to check out a version before restoring and require success. Validation: - The new regression failed against the original implementation when the mock received a null version. - `cargo test --locked --profile ci --features remote -p lancedb --lib restore` — 6 tests passed. - `cargo fmt --all` and `git diff --check` passed. - `cargo clippy --quiet --locked --profile ci --features remote -p lancedb --tests --examples` passed. Fixes #4381 Co-authored-by: Gatefixer <313497061+lancedb-gatefixer[bot]@users.noreply.github.com> --- rust/lancedb/src/remote/table.rs | 64 +++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/rust/lancedb/src/remote/table.rs b/rust/lancedb/src/remote/table.rs index 1ea72bf14..2fc58e1e5 100644 --- a/rust/lancedb/src/remote/table.rs +++ b/rust/lancedb/src/remote/table.rs @@ -2272,11 +2272,14 @@ impl BaseTable for RemoteTable { Ok(Some(Arc::new(snapshot))) } async fn restore(&self) -> Result<()> { + let read_snapshot = self.snapshot_read_state().await; + let version = read_snapshot.version.ok_or_else(|| Error::InvalidInput { + message: "you must run checkout before running restore".to_string(), + })?; let mut request = self .client .post(&format!("/v1/table/{}/restore/", self.identifier)); - let read_snapshot = self.snapshot_read_state().await; - let mut body = serde_json::json!({ "version": read_snapshot.version }); + let mut body = serde_json::json!({ "version": version }); self.apply_branch_body(&mut body); request = request.json(&body); @@ -8163,6 +8166,43 @@ mod tests { .unwrap(); } + #[tokio::test] + async fn test_restore_requires_checkout() { + let request_count = Arc::new(AtomicUsize::new(0)); + let request_count_clone = request_count.clone(); + let table = Table::new_with_handler("my_table", move |request| { + request_count_clone.fetch_add(1, Ordering::SeqCst); + assert_eq!(request_body_json(&request)["version"], 42); + let body = match request.url().path() { + "/v1/table/my_table/describe/" => r#"{"version":42,"schema":{"fields":[]}}"#, + "/v1/table/my_table/restore/" => r#"{"version":43}"#, + path => panic!("unexpected request path: {path}"), + }; + http::Response::builder().status(200).body(body).unwrap() + }); + + let err = table.restore().await.unwrap_err(); + assert!(matches!(err, Error::InvalidInput { message } + if message == "you must run checkout before running restore")); + assert_eq!(request_count.load(Ordering::SeqCst), 0); + + table.checkout(42).await.unwrap(); + table.checkout_latest().await.unwrap(); + let err = table.restore().await.unwrap_err(); + assert!(matches!(err, Error::InvalidInput { message } + if message == "you must run checkout before running restore")); + assert_eq!(request_count.load(Ordering::SeqCst), 1); + + table.checkout(42).await.unwrap(); + table.restore().await.unwrap(); + assert_eq!(request_count.load(Ordering::SeqCst), 3); + + let err = table.restore().await.unwrap_err(); + assert!(matches!(err, Error::InvalidInput { message } + if message == "you must run checkout before running restore")); + assert_eq!(request_count.load(Ordering::SeqCst), 3); + } + #[tokio::test] async fn test_fails_if_checkout_version_doesnt_exist() { let table = Table::new_with_handler("my_table", |request| { @@ -10992,22 +11032,24 @@ mod tests { } }); + table.checkout(1).await.unwrap(); + // First schema call let schema1 = table.schema().await.unwrap(); - assert_eq!(call_count.load(Ordering::SeqCst), 1); + assert_eq!(call_count.load(Ordering::SeqCst), 2); // Second schema call uses cache let schema2 = table.schema().await.unwrap(); assert_eq!(Arc::as_ptr(&schema2), Arc::as_ptr(&schema1)); - assert_eq!(call_count.load(Ordering::SeqCst), 1); + assert_eq!(call_count.load(Ordering::SeqCst), 2); // Restore operation - let _ = table.restore().await; + table.restore().await.unwrap(); // Schema call after restore should re-fetch (cache invalidated) let schema3 = table.schema().await.unwrap(); assert_ne!(Arc::as_ptr(&schema3), Arc::as_ptr(&schema1)); - assert_eq!(call_count.load(Ordering::SeqCst), 2); + assert_eq!(call_count.load(Ordering::SeqCst), 3); } /// Test that centralized error handling invalidates cache on query errors @@ -13828,8 +13870,17 @@ mod tests { .status(200) .body("{}".to_string()) .unwrap(), + "/v1/table/my_table/describe/" => { + assert_eq!(request_body_json(&request)["branch"], "exp"); + assert_eq!(request_body_json(&request)["version"], 1); + http::Response::builder() + .status(200) + .body(r#"{"version":1,"schema":{"fields":[]}}"#.to_string()) + .unwrap() + } "/v1/table/my_table/restore/" => { assert_eq!(request_body_json(&request)["branch"], "exp"); + assert_eq!(request_body_json(&request)["version"], 1); http::Response::builder() .status(200) .body(r#"{"version":1}"#.to_string()) @@ -13841,6 +13892,7 @@ mod tests { .create_branch("exp", Ref::Version(None, None)) .await .unwrap(); + branch.checkout(1).await.unwrap(); branch.restore().await.unwrap(); }