rust/lancedb
1
0
Fork 0
mirror of https://github.com/lancedb/lancedb.git synced 2026-11-14 14:46:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(table): classify LSM faults by status, not by a body code

Browse Source 
A lost claim and a missing table both arrived as 404, told apart only by
the body's `code`. `is_registry_miss` existed for exactly that, and the
server defeated it by coding every 404 the same — so a checkpoint against
a table that did not exist re-issued from flush three times and then
reported that the owning node kept losing its claim.

The server now assigns one meaning per status, so the status is the
classification: 421 is a lost claim, 404 is no such table. `send_lsm_route`
turns every 404 into `TableNotFound` and `is_registry_miss` is gone.

The body is still read on 503, but for a different question than before:
not which of phalanx's 503s this is, but whether it came from phalanx at
all. An ingress 503 carries no code, and reading it as a draining node
would abort a checkpoint on a transient hop failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Daniel Rammer
2026-08-03 15:26:27 -05:00
co-authored by Claude Opus 5
parent feeedc9d25
commit e97f2a8298
2 changed files with 56 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
rust/lancedb/src/remote/table.rs
+17 -25
View File
@@ -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<S: HttpSend> RemoteTable<S> {
}
}
/// 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::<serde_json::Value>(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<S: HttpSend> RemoteTable<S> {
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();
rust/lancedb/src/table/checkpoint.rs
+39 -35
View File
@@ -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);