diff --git a/rust/lancedb/src/remote/table.rs b/rust/lancedb/src/remote/table.rs index 0180d37be..e22f2e368 100644 --- a/rust/lancedb/src/remote/table.rs +++ b/rust/lancedb/src/remote/table.rs @@ -27,7 +27,7 @@ use crate::table::MergeBranchResult; use crate::table::MergeResult; use crate::table::Tags; use crate::table::UpdateResult; -use crate::table::checkpoint::{CODE_INVALID_TABLE_STATE, classify_lsm_fault}; +use crate::table::checkpoint::classify_lsm_fault; use crate::table::lsm_stats::GetLsmStatsResponse; use crate::table::merge::MergeFilter; use crate::table::query::create_multi_vector_plan; @@ -910,17 +910,6 @@ impl RemoteTable { } } - /// Whether a 404 body is a registry miss (`InvalidTableState`) rather - /// than the table not existing (`TableNotFound`). An unparseable body - /// reads as "not a miss", so the ambiguous case surfaces as the plain - /// not-found error instead of driving a re-claim loop. - fn is_registry_miss(body: &str) -> bool { - serde_json::from_str::(body) - .ok() - .and_then(|v| v.get("code").and_then(|c| c.as_u64())) - == Some(CODE_INVALID_TABLE_STATE) - } - /// Send an LSM operator request and decode its JSON body, classifying /// a failure into an [`Error::LsmRoute`] the checkpoint loop can act on. /// @@ -946,11 +935,12 @@ impl RemoteTable { let status = response.status(); let body = response.text().await.err_to_http(request_id.clone())?; if !status.is_success() { - // A 404 that is not a registry miss means the table does not - // exist, and must arrive as the same error every other route - // reports. These routes bypass `check_table_response`, so - // `handle_table_not_found` never runs for them. - if status == StatusCode::NOT_FOUND && !Self::is_registry_miss(&body) { + // A 404 means the table does not exist — on every route on this + // surface, since the server assigns one meaning per status. It must + // arrive as the same error every other route reports; these routes + // bypass `check_table_response`, so `handle_table_not_found` never + // runs for them. A lost claim is 421, classified below. + if status == StatusCode::NOT_FOUND { return Err(Error::TableNotFound { name: self.name.clone(), source: Box::new(Error::Http { @@ -6189,10 +6179,11 @@ mod tests { assert_eq!(memtables[0].indexes, vec!["vec_idx".to_string()]); } - /// A 404 for a table that does not exist must arrive as - /// `TableNotFound`, like every other route — not as a lost claim the - /// loop re-issues from flush until its cap, then blames on a crash - /// loop. The two are distinguished only by the body's `code`. + /// A 404 must arrive as `TableNotFound`, like every other route — not as + /// a lost claim the loop re-issues from flush until its cap, then blames + /// on a crash loop. The two are distinguished by *status*: 404 is "no such + /// table", 421 is "this node holds no claim". They shared 404 once, and + /// the loop chased a name that never existed. #[tokio::test(start_paused = true)] async fn test_missing_table_is_not_read_as_a_lost_claim() { let calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); @@ -6217,8 +6208,9 @@ mod tests { ); } - /// The other 404: the node lost its claim mid-loop. That one *does* - /// re-issue from flush, which is the call that re-claims and replays. + /// A lost claim — 421, not 404. It *does* re-issue from flush, which is + /// the call that re-claims and replays. Sharing 404 with "no such table" + /// is what previously sent this loop after a name that never existed. #[tokio::test(start_paused = true)] async fn test_registry_miss_reissues_from_flush() { let calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); @@ -6234,8 +6226,8 @@ mod tests { if path.contains("compact_lsm") { if n < 4 { return http::Response::builder() - .status(404) - .body(r#"{"code":19,"error":"WAL table not claimed"}"#.to_string()) + .status(421) + .body(r#"{"code":19,"error":"table not claimed"}"#.to_string()) .unwrap(); } return accepted(); diff --git a/rust/lancedb/src/table/checkpoint.rs b/rust/lancedb/src/table/checkpoint.rs index 069a252e2..053a12434 100644 --- a/rust/lancedb/src/table/checkpoint.rs +++ b/rust/lancedb/src/table/checkpoint.rs @@ -38,55 +38,54 @@ use crate::{Error, Result, Table}; /// How a failed LSM request should be handled by the checkpoint loop. /// -/// Five distinct conditions used to arrive at a client as one 503. This is -/// the classification that keeps them apart, and it reads the body's `code` -/// on 503 only — every other status is already unambiguous. +/// Reads the status, which the server assigns one meaning apiece and phalanx +/// relays. The body's `code` is consulted on 503 alone, and only to tell a +/// server 503 from a proxy's. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum LsmFault { - /// 429, or 503 with `ServiceUnavailable`: contention, a fenced writer, - /// no ready slot, or transport. Retry with backoff. + /// 429 (latch held, pool saturated, or the pod replaying its WAL), or a + /// 503 that did not come from the server. Retry with backoff. /// /// Fenced and slot-unavailable are deliberately not distinguished from /// contention here: the client action is identical. Retry, - /// 404: the registry entry vanished under a node restart mid-loop. - /// `flush` is the call that re-claims and replays, so re-issue from - /// there — with a cap, or a crash-looping node turns the loop into a - /// spin. + /// 421: the owning node holds no claim on the table — its registry entry + /// vanished under a restart mid-loop. `flush` is the call that re-claims + /// and replays, so re-issue from there — with a cap, or a crash-looping + /// node turns the loop into a spin. ReissueFromFlush, /// 503 with `InvalidTableState`: the owning node is draining. Terminal. Draining, - /// 409 (dropping), 400 (not WAL-backed), or anything else. Terminal. + /// 404 (no such table), 409 (dropping), 400 (not WAL-backed), or anything + /// unrecognized. Terminal. Fatal, } -/// Lance-namespace `ErrorCode::InvalidTableState`. Nothing else in the -/// server maps to it, which is what makes it usable as the draining signal: -/// a `WalDraining` variant that mapped to the generic service-unavailable -/// code would be byte-identical to every other 503 on the wire, leaving -/// this classifier matching message strings — exactly what a string `code` -/// field was rejected to avoid. +/// Lance-namespace `ErrorCode::InvalidTableState`, which the server attaches +/// to a draining 503. Its job here is to distinguish a 503 the server sent +/// from one an ingress or proxy sent on its behalf — the latter carries no +/// code at all, and is retryable. pub(crate) const CODE_INVALID_TABLE_STATE: u64 = 19; /// Classify an LSM-route failure from its status and response body. /// -/// The body must be inspected **before** any helper that folds it into a -/// string and keeps only the status, or the discrimination designed across -/// three crates is lost at the last hop. +/// The status carries the condition: the server assigns exactly one meaning +/// per status and phalanx relays it. The body is read for one thing only — +/// see the 503 arm. pub(crate) fn classify_lsm_fault(status: u16, body: &str) -> LsmFault { match status { + // Latch held, compactor pool saturated, or the pod replaying its WAL. 429 => LsmFault::Retry, - // A 404 is a registry miss only when the body says so. Left - // unchecked, a typo'd table name — also a 404 — is read as "the - // node lost its claim", and the loop re-issues from flush until the - // cap and then blames a crash loop for a name that never existed. - // `send_lsm_route` turns the other 404 into `TableNotFound` before - // this is reached; the check stays so a direct caller cannot - // reintroduce the confusion. - 404 if body_code(body) == Some(CODE_INVALID_TABLE_STATE) => LsmFault::ReissueFromFlush, - 404 => LsmFault::Fatal, + // The owning node holds no claim; `flush` re-claims and replays. + 421 => LsmFault::ReissueFromFlush, + // The body check is *not* disambiguating phalanx's own 503s — it asks + // whether this 503 came from phalanx at all. An ingress or proxy 503 + // between here and the server carries no code, and treating that as a + // draining node would abort a checkpoint on a transient hop failure. 503 if body_code(body) == Some(CODE_INVALID_TABLE_STATE) => LsmFault::Draining, 503 => LsmFault::Retry, + // 404 (no such table), 409 (dropping), 400 (malformed). `send_lsm_route` + // turns 404 into `TableNotFound` before this is reached. _ => LsmFault::Fatal, } } @@ -112,8 +111,8 @@ const POLL_INTERVAL: Duration = Duration::from_secs(1); /// merge — fails loudly instead of spinning. const MAX_IDLE_POLLS: usize = 10; -/// Cap on re-issues from `flush` after a 404. A node in a crash loop would -/// otherwise turn flush → compact → 404 → flush into a spin. +/// Cap on re-issues from `flush` after a 421. A node in a crash loop would +/// otherwise turn flush → compact → 421 → flush into a spin. const MAX_REISSUES: usize = 3; /// Base backoff between retries, doubled per consecutive retry up to @@ -299,7 +298,11 @@ mod tests { /// that only asserts "it errored". #[test] fn taxonomy_round_trips() { - assert_eq!(classify_lsm_fault(429, r#"{"code":21}"#), LsmFault::Retry); + assert_eq!( + classify_lsm_fault(429, r#"{"code":21}"#), + LsmFault::Retry, + "contention, saturation, and a pod replaying its WAL all arrive here" + ); assert_eq!( classify_lsm_fault(503, r#"{"code":19}"#), LsmFault::Draining, @@ -311,9 +314,9 @@ mod tests { "fenced / no-slot / transport stay retryable" ); assert_eq!( - classify_lsm_fault(404, r#"{"code":19}"#), + classify_lsm_fault(421, r#"{"code":19}"#), LsmFault::ReissueFromFlush, - "a registry miss says InvalidTableState and must re-claim" + "a lost claim has its own status and must re-claim" ); assert_eq!( classify_lsm_fault(404, r#"{"code":4}"#), @@ -325,8 +328,9 @@ mod tests { } /// A 503 whose body is missing, truncated, or not the expected JSON must - /// fall back to *retryable*, never to terminal: mistaking a healthy node - /// for a draining one aborts the checkpoint on a lie. + /// fall back to *retryable*, never to terminal. This is the proxy case: + /// an ingress 503 carries no code, and mistaking it for a draining node + /// aborts the checkpoint on a lie. #[test] fn unparseable_503_body_is_retryable() { assert_eq!(classify_lsm_fault(503, ""), LsmFault::Retry);