From 7c43de9846bf388be4272efdbec4d2ac6a5bc2af Mon Sep 17 00:00:00 2001 From: Beinan Date: Thu, 8 Oct 2026 20:29:21 +0000 Subject: [PATCH] fix(merge): gate error-code emission and never let a code outrank 401/403 Review of #338 found two defects. Mixed-version regression: tagging the message changed the exact string an old master matches on, so a stale preparation completed by a new executor and classified by an old master went back to a one-hour cooldown - the production failure #338 set out to fix. Emission is now off by default and enabled with MAINTENANCE_ERROR_CODES=1 once every master and executor runs a code-aware reader. The code moves to a suffix, and the legacy exact-text matcher tolerates that suffix, so even a reader with only the text path is not fooled once emission is on. Classification priority: the code was checked before HTTP status, so "HTTP 403: ... [LC_STALE_PREPARATION]" classified Retryable. The PR description claimed a test for this; there was none. Order is now ownership, then explicit 401/403, then code, then text. corrected() reuses classify() instead of its own rule, and only ever softens a stored DataOrConfiguration record. Tests: 401/403 wrapping the code in either position classify DataOrConfiguration and are not softened by corrected(); emission is off by default; the exact legacy string is still matched byte-for-byte; a tagged string is recognised by the text path alone. api, merge (7 unit) and core prepared/stale suites pass; clippy -D warnings, fmt, typos clean. Refs #333. Co-Authored-By: Claude Fable 5 --- crates/lance-context-api/src/lib.rs | 42 ++++++++++- crates/lance-context-merge/src/failure.rs | 90 +++++++++++++++++++++-- docs/concurrent-indexing.md | 15 ++-- 3 files changed, 130 insertions(+), 17 deletions(-) diff --git a/crates/lance-context-api/src/lib.rs b/crates/lance-context-api/src/lib.rs index 4332084..8a7c005 100644 --- a/crates/lance-context-api/src/lib.rs +++ b/crates/lance-context-api/src/lib.rs @@ -1517,9 +1517,36 @@ impl MaintenanceErrorCode { } } - /// Render `message` with this code attached. + /// Render `message` with this code attached, if code emission is + /// enabled for this process; otherwise return `message` unchanged. + /// + /// Emission is gated because older masters classify these errors by + /// exact text and would treat a tagged message as a permanent error + /// with a one-hour cooldown. Deploy readers (every master and executor + /// understanding `parse`) first, then set `MAINTENANCE_ERROR_CODES=1`. + /// The default flips to on once no pre-code binary remains supported. pub fn tag(self, message: &str) -> String { - format!("[{}] {message}", self.as_str()) + if Self::emission_enabled() { + format!("{message} [{}]", self.as_str()) + } else { + message.to_string() + } + } + + /// Render with the code regardless of the process gate. For tests and + /// for callers that have verified every reader in the fleet. + pub fn tag_forced(self, message: &str) -> String { + format!("{message} [{}]", self.as_str()) + } + + pub fn emission_enabled() -> bool { + use std::sync::OnceLock; + static ENABLED: OnceLock = OnceLock::new(); + *ENABLED.get_or_init(|| { + std::env::var("MAINTENANCE_ERROR_CODES") + .map(|v| matches!(v.trim(), "1" | "true" | "on" | "yes")) + .unwrap_or(false) + }) } /// Find the first code tag in an error string, regardless of wrapping. @@ -1834,11 +1861,18 @@ mod tests { #[test] fn maintenance_error_code_round_trips_through_wrappers() { let code = MaintenanceErrorCode::StalePreparation; - let tagged = code.tag("index metadata changed; reprepare"); + let tagged = code.tag_forced("index metadata changed; reprepare"); assert_eq!( tagged, - "[LC_STALE_PREPARATION] index metadata changed; reprepare" + "index metadata changed; reprepare [LC_STALE_PREPARATION]" ); + if std::env::var("MAINTENANCE_ERROR_CODES").is_err() { + assert_eq!( + code.tag("index metadata changed; reprepare"), + "index metadata changed; reprepare", + "emission is off by default for mixed-version safety" + ); + } let wrapped = format!("Invalid user input: {tagged}, crates/x/src/y.rs:1:2"); assert_eq!(MaintenanceErrorCode::parse(&wrapped), Some(code)); assert_eq!( diff --git a/crates/lance-context-merge/src/failure.rs b/crates/lance-context-merge/src/failure.rs index 7306a7a..7a196c0 100644 --- a/crates/lance-context-merge/src/failure.rs +++ b/crates/lance-context-merge/src/failure.rs @@ -33,13 +33,17 @@ pub fn now_ms() -> u64 { } pub fn classify(error: &str) -> FailureClass { - // A stable code wins over any prose, path or wrapper format. Ownership - // is checked first because an unresolved owner must never be retried - // as an ordinary conflict. + // Priority: unresolved ownership, then explicit permanent signals that + // must never be laundered by a code (auth failures), then the stable + // code, then legacy text. A code beats prose, paths and wrapper formats + // but never beats a 401/403 that wraps it. let lowered = error.to_ascii_lowercase(); if lowered.contains("ownership unresolved") { return FailureClass::OwnershipUnresolved; } + if has_http_status(&lowered, &["401", "403"]) { + return FailureClass::DataOrConfiguration; + } if let Some(code) = MaintenanceErrorCode::parse(error) { return match code { MaintenanceErrorCode::StalePreparation => FailureClass::Retryable, @@ -83,6 +87,17 @@ pub fn classify(error: &str) -> FailureClass { // Do not exempt arbitrary errors merely containing "reprepare". fn is_preparation_conflict(error: &str) -> bool { let message = error.strip_prefix("invalid user input: ").unwrap_or(error); + // Tolerate a trailing lowercase code suffix on the message segment so a + // tagged string is also recognised by the text path (defence in depth). + let message = message + .split_once(" [lc_stale_preparation]") + .map_or(message, |(head, tail)| { + // Re-join head with the location tail if the code sat before it. + if tail.starts_with(", crates/") { + return head; + } + message + }); let message = match message.split_once(", crates/lance-context-core/src/store_base.rs:") { Some((message, location)) => { let Some((line, column)) = location.split_once(':') else { @@ -169,10 +184,11 @@ impl ShardFailure { // moving the failure timestamp, or mutating ownership/durable records. // The next fenced completion persists the normal retry result. fn corrected(mut self) -> Self { + // One rule set: whatever `classify` says today is the effective + // class. Only a DataOrConfiguration record can be softened; a stored + // Retryable is never hardened on read. if self.class == FailureClass::DataOrConfiguration - && (MaintenanceErrorCode::parse(&self.last_error) - == Some(MaintenanceErrorCode::StalePreparation) - || is_preparation_conflict(&self.last_error.to_ascii_lowercase())) + && classify(&self.last_error) == FailureClass::Retryable { self.class = FailureClass::Retryable; let (attention, delay) = Self::retry_policy(&self.class, self.consecutive_attempts); @@ -418,7 +434,7 @@ mod tests { #[test] fn coded_preparation_conflicts_classify_regardless_of_wrapper_or_path() { let tagged = MaintenanceErrorCode::StalePreparation - .tag("compaction source fragment changed; reprepare"); + .tag_forced("compaction source fragment changed; reprepare"); for error in [ format!( "Invalid user input: {tagged}, crates/lance-context-core/src/store_base.rs:1529:24" @@ -430,11 +446,39 @@ mod tests { ] { assert_eq!(classify(&error), FailureClass::Retryable, "{error}"); } - // Ownership still wins, and the code does not launder a 403. + // Ownership still wins, and the code does not launder a 401/403 + // regardless of where the status appears relative to the code. assert_eq!( classify(&format!("ownership unresolved: {tagged}")), FailureClass::OwnershipUnresolved ); + for error in [ + format!("HTTP 403: {tagged}"), + format!("{tagged}; status 403"), + format!("status code 401 {tagged}"), + ] { + assert_eq!( + classify(&error), + FailureClass::DataOrConfiguration, + "{error}" + ); + // And corrected() must not soften such a record either. + let old = ShardFailure { + target: "table".into(), + endpoint: "master:compact".into(), + class: FailureClass::DataOrConfiguration, + consecutive_attempts: 1, + last_failure_ms: 2, + next_retry_ms: 3_602_000, + needs_attention: true, + last_error: error.clone(), + }; + assert_eq!( + old.corrected().class, + FailureClass::DataOrConfiguration, + "{error}" + ); + } // An old DataOrConfiguration record carrying a code is corrected too. let old = ShardFailure { target: "table".into(), @@ -452,6 +496,36 @@ mod tests { assert_eq!(corrected.last_failure_ms, 2); } + /// Mixed-version rollout: a master older than the code reader classifies + /// by exact text. Emission must therefore be off by default so new + /// executors keep producing the exact legacy string, and the legacy text + /// path must still recognise a tagged string once emission is enabled. + #[test] + fn code_emission_is_off_by_default_and_text_path_tolerates_the_code() { + // Default: no code in the string (unless the test env sets the var). + if std::env::var("MAINTENANCE_ERROR_CODES").is_err() { + assert_eq!( + MaintenanceErrorCode::StalePreparation.tag("index metadata changed; reprepare"), + "index metadata changed; reprepare" + ); + } + // Legacy exact matcher, byte-for-byte, as an old master sees a + // message produced with emission off. + let plain = "Invalid user input: index metadata changed; reprepare, crates/lance-context-core/src/store_base.rs:1529:24"; + assert!(is_preparation_conflict(&plain.to_ascii_lowercase())); + // With emission on, the text path alone (no code parsing) still + // recognises it, so a reader with only the text path is not fooled. + let tagged = format!( + "Invalid user input: {}, crates/lance-context-core/src/store_base.rs:1529:24", + MaintenanceErrorCode::StalePreparation.tag_forced("index metadata changed; reprepare") + ); + assert!( + is_preparation_conflict(&tagged.to_ascii_lowercase()), + "{tagged}" + ); + assert_eq!(classify(&tagged), FailureClass::Retryable); + } + #[test] fn old_conflict_records_recover_without_resetting_attempts_or_clock() { for attempts in [1, 4, 15, u32::MAX] { diff --git a/docs/concurrent-indexing.md b/docs/concurrent-indexing.md index f095509..5b058c6 100644 --- a/docs/concurrent-indexing.md +++ b/docs/concurrent-indexing.md @@ -36,11 +36,16 @@ the original snapshot read version, allowing Lance to check intervening commits as well. Newly appended fragments remain unindexed and query planning scans them; index publication must preserve merge watermarks and newly appended rows. -Stale-preparation validation failures carry the stable code -`[LC_STALE_PREPARATION]` in their message. The failure classifier recognises the -code regardless of how Lance wraps the error or where the check lives in the -source tree; the older exact-text matcher remains only for failure records -written by binaries that predate the code. These failures use the ordinary +Stale-preparation validation failures can carry the stable code +`[LC_STALE_PREPARATION]` at the end of their message. The failure classifier +recognises the code regardless of how Lance wraps the error or where the check +lives in the source tree, but never above an explicit HTTP 401/403 or an +unresolved-ownership prefix. Emission is **off by default** +(`MAINTENANCE_ERROR_CODES=1` enables it): a master older than the code reader +classifies by exact text and would treat a tagged message as a permanent error +with a one-hour cooldown. Roll out readers to every master and executor first, +then enable emission. The exact-text matcher stays for records from older +binaries and also tolerates the code suffix. These failures use the ordinary bounded retry budget. The first two failed attempts wait two seconds; repeated failures back off and eventually require attention. Existing records incorrectly classified as data/configuration errors