From 2c305ca41de66be39be4d3a2e648f74c109b2ee6 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Thu, 8 Oct 2026 15:47:45 -0700 Subject: [PATCH 1/6] fix(writes): charge Blob payload apart from the row around it Every keyed write checked one 32 MiB ceiling on a batch's whole Arrow memory, which includes a managed Blob value's bytes, so a value of exactly 32 MiB never fit beside its row. KeyedBytes splits that count into payload (each managed value's logical length) and framing (the rest), each under its own 32 MiB ceiling, and every keyed check uses it: staging, keyed-write preparation, the update carry scan, branch merge buffering and proven-insert chunking, and the compatibility loader's pre-decode forecast. The halves sum to the old count. A payload refusal names its resource with 'Blob payload bytes' in place of 'bytes'. Two Lance guards pin what the Blob write plan relies on: a whole-row merge-update keeps the row's stable row id, and merge-insert refuses an external reference outside the dataset's bases. --- changelog.d/blob-payload-budget.changed.md | 13 + crates/omnigraph/src/exec/merge.rs | 68 ++--- crates/omnigraph/src/exec/staging.rs | 113 +++---- crates/omnigraph/src/loader/mod.rs | 183 +++++++---- crates/omnigraph/src/storage_layer.rs | 131 ++++++-- crates/omnigraph/src/table_store.rs | 285 +++++++++--------- .../omnigraph/src/table_store/staged_tests.rs | 106 ++++++- .../omnigraph/tests/lance_surface_guards.rs | 87 ++++++ crates/omnigraph/tests/writes.rs | 203 +++++++++++-- docs/dev/execution.md | 5 +- docs/dev/writes.md | 22 +- docs/user/blobs.md | 8 +- docs/user/cli/managed-data.md | 3 +- docs/user/mutations/index.md | 9 +- 14 files changed, 844 insertions(+), 392 deletions(-) create mode 100644 changelog.d/blob-payload-budget.changed.md diff --git a/changelog.d/blob-payload-budget.changed.md b/changelog.d/blob-payload-budget.changed.md new file mode 100644 index 000000000..47daaff0f --- /dev/null +++ b/changelog.d/blob-payload-budget.changed.md @@ -0,0 +1,13 @@ +- A managed Blob value of exactly 32 MiB now fits in a write. Incremental + writes (inserts, updates, `append` and `merge` loads and branch merges) + check managed Blob payloads and the rest of the row against separate + 32 MiB ceilings. The old single ceiling counted both, so a 32 MiB value + was always refused for the row around it. A payload refusal names its + resource with `Blob payload bytes` in place of `bytes`, for example + `keyed write Blob payload bytes for node:Document`. An update that carries + an oversized Blob cell now reports `retained keyed batch Blob payload bytes + per operation`. One operation may now hold up to 32 MiB of Blob payload + plus 32 MiB of other row data, where it held 32 MiB in all. Encoded limits + stay: `omnigraph load` and `/load/ndjson` cap each line at 32 MiB, and + HTTP request bodies keep their caps, so those paths carry about 24 MiB of + `base64:` data per value. diff --git a/crates/omnigraph/src/exec/merge.rs b/crates/omnigraph/src/exec/merge.rs index d5a158291..f0634287a 100644 --- a/crates/omnigraph/src/exec/merge.rs +++ b/crates/omnigraph/src/exec/merge.rs @@ -7,7 +7,8 @@ use crate::ordered_cursor::{ use crate::seams::{decide_seam, fail}; use crate::session::Session; use crate::storage_layer::{ - KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, KeyedWriteSemantics, ProvenInsertChunk, + KEYED_BLOB_PAYLOAD_MAX_BYTES, KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, KeyedBytes, + KeyedWriteSemantics, ProvenInsertChunk, }; use crate::table_store::certified_insert_absence_rows; use futures::StreamExt; @@ -608,7 +609,7 @@ struct StagedTableWriter { dir: TempDir, dataset: Option, buffered_rows: usize, - buffered_bytes: u64, + buffered_bytes: KeyedBytes, row_count: u64, chunk_rows: Vec, batches: Vec, @@ -627,7 +628,7 @@ impl StagedTableWriter { dir, dataset: None, buffered_rows: 0, - buffered_bytes: 0, + buffered_bytes: KeyedBytes::default(), row_count: 0, chunk_rows: Vec::new(), batches: Vec::new(), @@ -651,24 +652,16 @@ impl StagedTableWriter { &row.dataset, &input, external_preflight, - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, )? } else { - u64::try_from(input.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("branch merge row memory size exceeds u64") - })? - }; - if predicted_row_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - "branch-merge fenced entity bytes", - KEYED_WRITE_MAX_BYTES, - predicted_row_bytes, - )); + KeyedBytes::of(&input)? } - let predicted_overflow = self + .ensure_fits("branch-merge fenced entity bytes")?; + let predicted_overflow = !self .buffered_bytes .checked_add(predicted_row_bytes) - .is_none_or(|bytes| bytes > KEYED_WRITE_MAX_BYTES); + .is_ok_and(KeyedBytes::fits); if self.buffered_rows > 0 && (self.buffered_rows >= KEYED_WRITE_MAX_ROWS || predicted_overflow) { @@ -679,20 +672,11 @@ impl StagedTableWriter { let batch = self .row_batch(row, input, materializer, external_preflight) .await?; - let row_bytes = u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("branch merge row memory size exceeds u64") - })?; - if row_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - "branch-merge fenced entity bytes", - KEYED_WRITE_MAX_BYTES, - row_bytes, - )); - } - let would_exceed_bytes = self + let row_bytes = KeyedBytes::of(&batch)?.ensure_fits("branch-merge fenced entity bytes")?; + let would_exceed_bytes = !self .buffered_bytes .checked_add(row_bytes) - .is_none_or(|bytes| bytes > KEYED_WRITE_MAX_BYTES); + .is_ok_and(KeyedBytes::fits); if self.buffered_rows > 0 && (self.buffered_rows >= KEYED_WRITE_MAX_ROWS || would_exceed_bytes) { @@ -703,14 +687,9 @@ impl StagedTableWriter { .checked_add(1) .ok_or_else(|| OmniError::manifest_internal("branch merge row count overflow"))?; self.buffered_rows += 1; - self.buffered_bytes = self - .buffered_bytes - .checked_add(row_bytes) - .ok_or_else(|| OmniError::manifest_internal("branch merge byte count overflow"))?; + self.buffered_bytes = self.buffered_bytes.checked_add(row_bytes)?; self.batches.push(batch); - if self.buffered_rows >= KEYED_WRITE_MAX_ROWS - || self.buffered_bytes >= KEYED_WRITE_MAX_BYTES - { + if self.buffered_rows >= KEYED_WRITE_MAX_ROWS || self.buffered_bytes.is_full() { self.flush().await?; } Ok(()) @@ -728,7 +707,7 @@ impl StagedTableWriter { .materialize_blob_batch_bounded_with_preflight_cache( &row.dataset, batch, - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, external_preflight, &mut self.external_payloads, ) @@ -779,7 +758,7 @@ impl StagedTableWriter { .map_err(OmniError::arrow_internal)? }; self.buffered_rows = 0; - self.buffered_bytes = 0; + self.buffered_bytes = KeyedBytes::default(); self.chunk_rows.push(batch.num_rows()); let chunk_count = u64::try_from(self.chunk_rows.len()) .map_err(|_| OmniError::manifest_internal("branch merge chunk count exceeds u64"))?; @@ -4319,12 +4298,13 @@ async fn next_exact_staged_chunk( } else { arrow_select::concat::concat_batches(schema, &slices).map_err(OmniError::arrow_internal)? }; - let chunk_bytes = u64::try_from(chunk.get_array_memory_size()) - .map_err(|_| OmniError::manifest_internal("branch merge chunk bytes exceed u64"))?; - if chunk.num_rows() > KEYED_WRITE_MAX_ROWS || chunk_bytes > KEYED_WRITE_MAX_BYTES { + let chunk_bytes = KeyedBytes::of(&chunk)?; + if chunk.num_rows() > KEYED_WRITE_MAX_ROWS || !chunk_bytes.fits() { return Err(OmniError::manifest_internal(format!( - "branch merge reconstructed a keyed chunk outside its planned bound: {} rows / {chunk_bytes} bytes", - chunk.num_rows() + "branch merge reconstructed a keyed chunk outside its planned bound: {} rows / {} framing and {} payload bytes", + chunk.num_rows(), + chunk_bytes.framing, + chunk_bytes.payload ))); } Ok(chunk) @@ -5169,10 +5149,10 @@ impl Omnigraph { .preflight_persisted_blob_selection(&blob_selection) .await?; let carried_blob_bytes = blob_selection.materialized_payload_bytes(&external_preflight)?; - if carried_blob_bytes > KEYED_WRITE_MAX_BYTES { + if carried_blob_bytes > KEYED_BLOB_PAYLOAD_MAX_BYTES { return Err(OmniError::resource_limit( "materialized blob payload bytes", - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, carried_blob_bytes, )); } diff --git a/crates/omnigraph/src/exec/staging.rs b/crates/omnigraph/src/exec/staging.rs index f6ea5d1d0..cd1d960e8 100644 --- a/crates/omnigraph/src/exec/staging.rs +++ b/crates/omnigraph/src/exec/staging.rs @@ -27,8 +27,8 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; use crate::storage_layer::{ - DeletedIdBudget, KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, KeyedWriteSemantics, - SnapshotHandle, StagedHandle, retain_keyed_batch, retained_keyed_bytes, + DeletedIdBudget, KEYED_BLOB_PAYLOAD_MAX_BYTES, KEYED_WRITE_MAX_ROWS, KeyedBytes, + KeyedWriteSemantics, SnapshotHandle, StagedHandle, retain_keyed_batch, retained_keyed_bytes, }; use arrow_array::{Array, RecordBatch, StringArray, UInt32Array}; use arrow_schema::SchemaRef; @@ -76,15 +76,12 @@ impl PendingTable { self.batches.iter().map(|b| b.num_rows()).sum() } - fn total_bytes(&self) -> Result { - self.batches.iter().try_fold(0_u64, |bytes, batch| { - let batch_bytes = u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("pending keyed batch bytes exceed u64") - })?; - bytes - .checked_add(batch_bytes) - .ok_or_else(|| OmniError::manifest_internal("pending keyed byte count overflow")) - }) + fn total_bytes(&self) -> Result { + self.batches + .iter() + .try_fold(KeyedBytes::default(), |bytes, batch| { + bytes.checked_add(KeyedBytes::of(batch)?) + }) } } @@ -120,7 +117,7 @@ pub(crate) struct MutationStaging { /// In-memory accumulated batches per table (insert/update path). pub(crate) pending: HashMap, /// Monotonic retained-batch accounting, updated only by `append_batch`. - pending_bytes: u64, + pending_bytes: KeyedBytes, /// Per-table delete predicates from delete-touching ops. D₂ guarantees a /// table is write-XOR-delete within one query, so this never overlaps /// `pending`. Staged as one combined `stage_delete` per table at @@ -259,8 +256,7 @@ impl MutationStaging { ))); } } - let batch_bytes = u64::try_from(batch.get_array_memory_size()) - .map_err(|_| OmniError::manifest_internal("pending keyed batch bytes exceed u64"))?; + let batch_bytes = KeyedBytes::of(&batch)?; let pending_bytes = if matches!(mode, PendingMode::StrictInsert | PendingMode::Upsert) { let existing_rows = self .pending @@ -279,23 +275,14 @@ impl MutationStaging { } let existing_bytes = match self.pending.get(table_key) { Some(existing) => existing.total_bytes()?, - None => 0, + None => KeyedBytes::default(), }; - let bytes = existing_bytes - .checked_add(batch_bytes) - .ok_or_else(|| OmniError::manifest_internal("pending keyed byte count overflow"))?; - if bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed entity bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - bytes, - )); - } + existing_bytes + .checked_add(batch_bytes)? + .ensure_fits(&format!("keyed entity bytes for {table_key}"))?; retained_keyed_bytes(self.pending_bytes, batch_bytes)? } else { - self.pending_bytes - .checked_add(batch_bytes) - .ok_or_else(|| OmniError::manifest_internal("pending keyed byte count overflow"))? + self.pending_bytes.checked_add(batch_bytes)? }; let entry = self .pending @@ -402,7 +389,7 @@ impl MutationStaging { /// keyed-write fence, while bytes include every pending table so one graph /// mutation cannot multiply the 32 MiB retained-memory budget by touching /// several tables. - pub(crate) fn pending_resource_usage(&self, table_key: &str) -> Result<(u64, u64)> { + pub(crate) fn pending_resource_usage(&self, table_key: &str) -> Result<(u64, KeyedBytes)> { let rows = u64::try_from( self.pending .get(table_key) @@ -512,26 +499,32 @@ impl MutationStaging { .collect::>(); let copied_external_blob_bytes = external_blob_preflight.materialized_payload_bytes(&copied_external_blob_uris)?; - if copied_external_blob_bytes > KEYED_WRITE_MAX_BYTES { + if copied_external_blob_bytes > KEYED_BLOB_PAYLOAD_MAX_BYTES { return Err(OmniError::resource_limit( "materialized external blob payload bytes", - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, copied_external_blob_bytes, )); } - let retained_batch_bytes = stage_inputs.iter().try_fold(0, |bytes, (_, table, _, _)| { - if table.mode == PendingMode::Overwrite { - Ok(bytes) - } else { - retain_keyed_batch(bytes, &table.batch) - } - })?; - retained_keyed_bytes(retained_batch_bytes, copied_external_blob_bytes)?; + let retained_batch_bytes = + stage_inputs + .iter() + .try_fold(KeyedBytes::default(), |bytes, (_, table, _, _)| { + if table.mode == PendingMode::Overwrite { + Ok(bytes) + } else { + retain_keyed_batch(bytes, &table.batch) + } + })?; + retained_keyed_bytes( + retained_batch_bytes, + KeyedBytes::payload(copied_external_blob_bytes), + )?; // Only after the complete operation has passed policy, source, and // aggregate-copy admission do we read payload bytes. Reuse remains // batch-bounded so this vector cannot retain an operation-sized cache. - let mut materialized_keyed_bytes = 0; + let mut materialized_keyed_bytes = KeyedBytes::default(); for (table_key, table, _, _) in &mut stage_inputs { table.batch = match table.mode { PendingMode::StrictInsert | PendingMode::Upsert => { @@ -662,26 +655,18 @@ fn prepare_pending_table( } if matches!(table.mode, PendingMode::StrictInsert | PendingMode::Upsert) { - let (rows, bytes) = - table - .batches - .iter() - .try_fold((0_u64, 0_u64), |(rows, bytes), batch| { - let batch_rows = u64::try_from(batch.num_rows()) - .map_err(|_| OmniError::manifest_internal("keyed batch rows exceed u64"))?; - let batch_bytes = - u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("keyed batch bytes exceed u64") - })?; - Ok::<_, OmniError>(( - rows.checked_add(batch_rows).ok_or_else(|| { - OmniError::manifest_internal("keyed row count overflow") - })?, - bytes.checked_add(batch_bytes).ok_or_else(|| { - OmniError::manifest_internal("keyed byte count overflow") - })?, - )) - })?; + let (rows, bytes) = table.batches.iter().try_fold( + (0_u64, KeyedBytes::default()), + |(rows, bytes), batch| { + let batch_rows = u64::try_from(batch.num_rows()) + .map_err(|_| OmniError::manifest_internal("keyed batch rows exceed u64"))?; + Ok::<_, OmniError>(( + rows.checked_add(batch_rows) + .ok_or_else(|| OmniError::manifest_internal("keyed row count overflow"))?, + bytes.checked_add(KeyedBytes::of(batch)?)?, + )) + }, + )?; if rows > KEYED_WRITE_MAX_ROWS as u64 { return Err(OmniError::resource_limit( format!("keyed entities for {table_key}"), @@ -689,13 +674,7 @@ fn prepare_pending_table( rows, )); } - if bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed entity bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - bytes, - )); - } + bytes.ensure_fits(&format!("keyed entity bytes for {table_key}"))?; } let mode = table.mode; diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index 06fcc2ce7..1a6f8734f 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -28,7 +28,10 @@ use crate::error::{OmniError, Result, missing_graph_type_at_snapshot}; use crate::exec::staging::{MutationStaging, PendingMode}; use crate::seams::{catalog, decide_seam, fail}; use crate::session::Session; -use crate::storage_layer::{DeletedIdBudget, KEYED_WRITE_MAX_BYTES, retain_keyed_batch}; +use crate::storage_layer::{ + DeletedIdBudget, KEYED_BLOB_PAYLOAD_MAX_BYTES, KEYED_WRITE_MAX_BYTES, KeyedBytes, + retain_keyed_batch, +}; /// Result of a load operation. #[derive(Debug, Clone, Default)] @@ -771,7 +774,7 @@ async fn load_jsonl_reader_once( // Phase 2a: build and validate every node batch up front. Cheap and // synchronous — surfaces validation errors before any S3 traffic. let mut node_id_remap = TypedNodeIdRemap::default(); - let mut prepared_keyed_bytes = 0; + let mut prepared_keyed_bytes = KeyedBytes::default(); let mut prepared_nodes: Vec<(String, String, Vec, usize)> = Vec::with_capacity(node_rows.len().saturating_add(strict_nodes.len())); let mut __dst_nr: Vec<_> = node_rows.into_iter().collect(); @@ -1433,18 +1436,20 @@ fn take_object_or_empty( #[derive(Default)] struct KeyedInputBudget { tables: HashMap, - bytes: u64, + bytes: KeyedBytes, } #[derive(Default)] struct KeyedTableInput { rows: usize, - bytes: u64, + bytes: KeyedBytes, } /// Charge a keyed JSON record before the parse spool retains it: a lower bound -/// on its Arrow payload per table and across tables, taken before base64 is -/// decoded. Not a JSON DOM bound; `MutationStaging::append_batch` is the authority. +/// on its Arrow bytes per table and across tables, taken before base64 is +/// decoded, split like the later batch check (`KeyedBytes`): a `base64:` +/// value's decoded length as payload, the rest as framing. Not a JSON DOM +/// bound; `MutationStaging::append_batch` is the authority. fn account_keyed_json_row( table_key: &str, data: &JsonValue, @@ -1463,58 +1468,56 @@ fn account_keyed_json_row( entry.rows as u64, )); } - let row_bytes = estimate_json_arrow_bytes(data)? - .checked_add( - u64::try_from(structural_string_bytes) - .map_err(|_| OmniError::manifest_internal("keyed string bytes exceed u64"))?, - ) - .ok_or_else(|| OmniError::manifest_internal("keyed input entity bytes overflow"))?; + let structural = u64::try_from(structural_string_bytes) + .map_err(|_| OmniError::manifest_internal("keyed string bytes exceed u64"))?; + let row_bytes = + estimate_json_keyed_bytes(data)?.checked_add(KeyedBytes::framing(structural))?; entry.bytes = entry .bytes - .checked_add(row_bytes) - .ok_or_else(|| OmniError::manifest_internal("keyed parsed byte count overflow"))?; - if entry.bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed parsed entity bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - entry.bytes, - )); - } - let total = budgets.bytes.checked_add(row_bytes).ok_or_else(|| { - OmniError::manifest_internal("keyed parsed operation byte count overflow") - })?; - if total > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - "keyed parsed entity bytes per operation", - KEYED_WRITE_MAX_BYTES, - total, - )); - } - budgets.bytes = total; + .checked_add(row_bytes)? + .ensure_fits(&format!("keyed parsed entity bytes for {table_key}"))?; + budgets.bytes = budgets + .bytes + .checked_add(row_bytes)? + .ensure_fits("keyed parsed entity bytes per operation")?; Ok(()) } fn estimate_json_arrow_bytes(value: &JsonValue) -> Result { + let bytes = estimate_json_keyed_bytes(value)?; + bytes + .framing + .checked_add(bytes.payload) + .ok_or_else(|| OmniError::manifest_internal("JSON value bytes overflow")) +} + +/// A lower bound on the Arrow bytes a JSON value becomes. A `base64:` string +/// counts its decoded length as payload; everything else is framing. +fn estimate_json_keyed_bytes(value: &JsonValue) -> Result { match value { - JsonValue::Null => Ok(0), - JsonValue::Bool(_) => Ok(1), + JsonValue::Null => Ok(KeyedBytes::default()), + JsonValue::Bool(_) => Ok(KeyedBytes::framing(1)), // Four bytes avoids rejecting valid Float32/Int32 input early. Wider // physical scalars are charged exactly by the later Arrow batch check. - JsonValue::Number(_) => Ok(4), + JsonValue::Number(_) => Ok(KeyedBytes::framing(4)), JsonValue::String(value) => { - let bytes = match value.strip_prefix("base64:") { - Some(encoded) => base64::decoded_len_estimate(encoded.len()).saturating_sub( - encoded - .as_bytes() - .iter() - .rev() - .take_while(|&&byte| byte == b'=') - .count(), - ), - None => value.len(), + let string_bytes = |bytes: usize| { + u64::try_from(bytes) + .map_err(|_| OmniError::manifest_internal("JSON string bytes exceed u64")) }; - u64::try_from(bytes) - .map_err(|_| OmniError::manifest_internal("JSON string bytes exceed u64")) + match value.strip_prefix("base64:") { + Some(encoded) => Ok(KeyedBytes::payload(string_bytes( + base64::decoded_len_estimate(encoded.len()).saturating_sub( + encoded + .as_bytes() + .iter() + .rev() + .take_while(|&&byte| byte == b'=') + .count(), + ), + )?)), + None => Ok(KeyedBytes::framing(string_bytes(value.len())?)), + } } JsonValue::Array(values) => { let offsets = u64::try_from(values.len()) @@ -1522,20 +1525,20 @@ fn estimate_json_arrow_bytes(value: &JsonValue) -> Result { .checked_add(1) .and_then(|count| count.checked_mul(4)) .ok_or_else(|| OmniError::manifest_internal("JSON array offset bytes overflow"))?; - values.iter().try_fold(offsets, |bytes, value| { - bytes - .checked_add(estimate_json_arrow_bytes(value)?) - .ok_or_else(|| OmniError::manifest_internal("JSON array bytes overflow")) - }) + values + .iter() + .try_fold(KeyedBytes::framing(offsets), |bytes, value| { + bytes.checked_add(estimate_json_keyed_bytes(value)?) + }) } // Property names are schema, not per-row Arrow payload. Count values // only so the early lower bound does not reject an otherwise-valid // wide schema; exact field buffers are charged after batch building. - JsonValue::Object(values) => values.values().try_fold(0_u64, |bytes, value| { - bytes - .checked_add(estimate_json_arrow_bytes(value)?) - .ok_or_else(|| OmniError::manifest_internal("JSON object bytes overflow")) - }), + JsonValue::Object(values) => values + .values() + .try_fold(KeyedBytes::default(), |bytes, value| { + bytes.checked_add(estimate_json_keyed_bytes(value)?) + }), } } @@ -2349,10 +2352,10 @@ fn preflight_blob_decode_budget<'a>( decoded_bytes = decoded_bytes.checked_add(estimate).ok_or_else(|| { OmniError::manifest_internal("decoded blob input byte count overflow") })?; - if decoded_bytes > KEYED_WRITE_MAX_BYTES { + if decoded_bytes > KEYED_BLOB_PAYLOAD_MAX_BYTES { return Err(OmniError::resource_limit( format!("decoded blob input bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, decoded_bytes, )); } @@ -2372,10 +2375,10 @@ pub(crate) fn append_blob_value(builder: &mut BlobArrayBuilder, value: &str) -> .take_while(|&&b| b == b'=') .count(), ); - if decoded_estimate as u64 > KEYED_WRITE_MAX_BYTES { + if decoded_estimate as u64 > KEYED_BLOB_PAYLOAD_MAX_BYTES { return Err(OmniError::resource_limit( "decoded blob input bytes", - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, decoded_estimate as u64, )); } @@ -3945,6 +3948,47 @@ edge WorksAt: Person -> Company )); } + /// The pre-decode forecast charges a `base64:` value by its decoded length to + /// the payload ceiling and the rest of the row to the framing ceiling, like + /// the batch check after it, so an exact-limit value fits beside its row and + /// one more decoded byte is refused by the payload ceiling. + #[test] + fn parsed_forecast_charges_base64_payload_apart_from_framing() { + let limit = usize::try_from(KEYED_BLOB_PAYLOAD_MAX_BYTES).unwrap(); + let encode = |bytes: usize| { + format!( + "base64:{}", + base64::engine::general_purpose::STANDARD.encode(vec![7_u8; bytes]) + ) + }; + + let row = serde_json::json!({"title": "exact", "content": encode(limit)}); + let mut budget = KeyedInputBudget::default(); + account_keyed_json_row("node:Document", &row, 0, &mut budget) + .expect("an exact-limit payload fits beside its row"); + assert_eq!( + budget.bytes, + KeyedBytes { + framing: 5, + payload: KEYED_BLOB_PAYLOAD_MAX_BYTES, + } + ); + + let row = serde_json::json!({"title": "over", "content": encode(limit + 1)}); + let error = + account_keyed_json_row("node:Document", &row, 0, &mut KeyedInputBudget::default()) + .expect_err("one decoded byte over must be refused"); + assert!(matches!( + error, + OmniError::ResourceLimitExceeded { + ref resource, + limit: KEYED_BLOB_PAYLOAD_MAX_BYTES, + actual, + } if resource == "keyed parsed entity Blob payload bytes for node:Document" + && actual == KEYED_BLOB_PAYLOAD_MAX_BYTES + 1 + )); + } + #[test] fn operation_byte_allowances_span_graph_types_and_include_their_ceiling() { let row = serde_json::json!({"payload": "x".repeat(17 * 1024 * 1024)}); @@ -3958,11 +4002,22 @@ edge WorksAt: Person -> Company && actual > KEYED_WRITE_MAX_BYTES )); + use crate::storage_layer::{KeyedBytes, retained_keyed_bytes}; assert_eq!( - crate::storage_layer::retained_keyed_bytes(KEYED_WRITE_MAX_BYTES - 1, 1).unwrap(), - KEYED_WRITE_MAX_BYTES + retained_keyed_bytes( + KeyedBytes::framing(KEYED_WRITE_MAX_BYTES - 1), + KeyedBytes::framing(1) + ) + .unwrap(), + KeyedBytes::framing(KEYED_WRITE_MAX_BYTES) + ); + assert!( + retained_keyed_bytes( + KeyedBytes::framing(KEYED_WRITE_MAX_BYTES), + KeyedBytes::framing(1) + ) + .is_err() ); - assert!(crate::storage_layer::retained_keyed_bytes(KEYED_WRITE_MAX_BYTES, 1).is_err()); let id = &row["payload"].as_str().unwrap()[..16 * 1024 * 1024 - std::mem::size_of::()]; let mut removed = DeletedIdBudget::default(); diff --git a/crates/omnigraph/src/storage_layer.rs b/crates/omnigraph/src/storage_layer.rs index 9792d0445..f218fb85e 100644 --- a/crates/omnigraph/src/storage_layer.rs +++ b/crates/omnigraph/src/storage_layer.rs @@ -73,27 +73,114 @@ use crate::table_store::{ pub(crate) const KEYED_WRITE_MAX_ROWS: usize = 8192; pub(crate) const KEYED_WRITE_MAX_BYTES: u64 = 32 * 1024 * 1024; -/// The operation-wide sibling of the per-table keyed ceiling. This uses the +/// The ceiling on managed Blob payload bytes, by logical length, that one +/// keyed write retains: the payload half of [`KeyedBytes`]. A single managed +/// value of exactly this size is admitted. +pub(crate) const KEYED_BLOB_PAYLOAD_MAX_BYTES: u64 = 32 * 1024 * 1024; + +/// What a keyed batch retains, under two ceilings. `payload` is the logical +/// length of every managed Blob value the batch carries; `framing` is the rest +/// of its Arrow memory, the Blob columns' offsets, validity and URIs included, +/// with the existing conservative count of a slice's complete backing buffers. +/// The two always sum to the batch's Arrow memory, so splitting them moves no +/// byte out of the accounting. One combined ceiling could not admit a value of +/// exactly [`KEYED_BLOB_PAYLOAD_MAX_BYTES`]: the batch around it is never empty. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct KeyedBytes { + pub(crate) framing: u64, + pub(crate) payload: u64, +} + +impl KeyedBytes { + pub(crate) fn of(batch: &RecordBatch) -> Result { + let total = u64::try_from(batch.get_array_memory_size()) + .map_err(|_| OmniError::manifest_internal("keyed batch bytes exceed u64"))?; + let payload = crate::table_store::logical_blob_payload_bytes(batch)?; + let framing = total.checked_sub(payload).ok_or_else(|| { + OmniError::manifest_internal("Blob payload bytes exceed their batch's Arrow memory") + })?; + Ok(Self { framing, payload }) + } + + pub(crate) fn framing(framing: u64) -> Self { + Self { + framing, + payload: 0, + } + } + + pub(crate) fn payload(payload: u64) -> Self { + Self { + framing: 0, + payload, + } + } + + pub(crate) fn checked_add(self, other: Self) -> Result { + Ok(Self { + framing: self + .framing + .checked_add(other.framing) + .ok_or_else(|| OmniError::manifest_internal("keyed framing byte count overflow"))?, + payload: self + .payload + .checked_add(other.payload) + .ok_or_else(|| OmniError::manifest_internal("keyed payload byte count overflow"))?, + }) + } + + pub(crate) fn fits(self) -> bool { + self.framing <= KEYED_WRITE_MAX_BYTES && self.payload <= KEYED_BLOB_PAYLOAD_MAX_BYTES + } + + /// Whether either half has reached its ceiling, so nothing more fits. + pub(crate) fn is_full(self) -> bool { + self.framing >= KEYED_WRITE_MAX_BYTES || self.payload >= KEYED_BLOB_PAYLOAD_MAX_BYTES + } + + /// Refuse a half above its ceiling. `resource` names the framing ceiling + /// ("... bytes ..."); the payload ceiling is the same name with "Blob + /// payload bytes" in place of its first "bytes". + pub(crate) fn ensure_fits(self, resource: &str) -> Result { + if self.framing > KEYED_WRITE_MAX_BYTES { + return Err(OmniError::resource_limit( + resource, + KEYED_WRITE_MAX_BYTES, + self.framing, + )); + } + if self.payload > KEYED_BLOB_PAYLOAD_MAX_BYTES { + return Err(OmniError::resource_limit( + blob_payload_resource(resource), + KEYED_BLOB_PAYLOAD_MAX_BYTES, + self.payload, + )); + } + Ok(self) + } +} + +/// The payload ceiling's name for a framing ceiling's name. +pub(crate) fn blob_payload_resource(resource: &str) -> String { + debug_assert!(resource.contains(" bytes"), "{resource}"); + resource.replacen(" bytes", " Blob payload bytes", 1) +} + +/// The operation-wide sibling of the per-table keyed ceilings. This uses the /// existing Arrow accounting (including its conservative shared-buffer count), -/// not a second allocator or a claim about native execution/RSS. -pub(crate) fn retained_keyed_bytes(current: u64, additional: u64) -> Result { - let actual = current - .checked_add(additional) - .ok_or_else(|| OmniError::manifest_internal("retained keyed batch byte count overflow"))?; - if actual > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - "retained keyed batch bytes per operation", - KEYED_WRITE_MAX_BYTES, - actual, - )); - } - Ok(actual) +/// split by [`KeyedBytes`], not a second allocator or a claim about native +/// execution/RSS. +pub(crate) fn retained_keyed_bytes( + current: KeyedBytes, + additional: KeyedBytes, +) -> Result { + current + .checked_add(additional)? + .ensure_fits("retained keyed batch bytes per operation") } -pub(crate) fn retain_keyed_batch(current: u64, batch: &RecordBatch) -> Result { - let bytes = u64::try_from(batch.get_array_memory_size()) - .map_err(|_| OmniError::manifest_internal("retained keyed batch bytes exceed u64"))?; - retained_keyed_bytes(current, bytes) +pub(crate) fn retain_keyed_batch(current: KeyedBytes, batch: &RecordBatch) -> Result { + retained_keyed_bytes(current, KeyedBytes::of(batch)?) } /// One allowance per mutation (all tables and cascades) or load (all replacement @@ -146,11 +233,15 @@ pub(crate) const BLOB_REBUILD_IO_BUFFER_BYTES: u64 = 8 * 1024 * 1024; pub(crate) struct PendingScanBudget { pub(crate) table_key: String, pub(crate) initial_rows: u64, - pub(crate) initial_bytes: u64, + pub(crate) initial_bytes: KeyedBytes, } impl PendingScanBudget { - pub(crate) fn new(table_key: impl Into, initial_rows: u64, initial_bytes: u64) -> Self { + pub(crate) fn new( + table_key: impl Into, + initial_rows: u64, + initial_bytes: KeyedBytes, + ) -> Self { Self { table_key: table_key.into(), initial_rows, diff --git a/crates/omnigraph/src/table_store.rs b/crates/omnigraph/src/table_store.rs index 1c4db6b20..a263f9325 100644 --- a/crates/omnigraph/src/table_store.rs +++ b/crates/omnigraph/src/table_store.rs @@ -74,8 +74,9 @@ use crate::db::{DatasetEntry, Snapshot}; use crate::error::{OmniError, Result}; use crate::seams::{decide_seam, skip}; use crate::storage_layer::{ - BLOB_REBUILD_IO_BUFFER_BYTES, IndexBuildSpec, KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, - KeyedWriteSemantics, PendingScanBudget, ProvenInsertChunk, + BLOB_REBUILD_IO_BUFFER_BYTES, IndexBuildSpec, KEYED_BLOB_PAYLOAD_MAX_BYTES, + KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, KeyedBytes, KeyedWriteSemantics, + PendingScanBudget, ProvenInsertChunk, blob_payload_resource, }; pub(crate) use crate::error::is_scratch_exhaustion; @@ -1931,17 +1932,18 @@ impl TableStore { None } - /// Conservative pre-read size for a persisted descriptor batch. The raw - /// one-row Arrow allocation already accounts every ordinary column and - /// descriptor buffer; adding logical payload bytes may overestimate the - /// smaller logical Blob descriptor, but cannot understate retained memory. + /// Conservative pre-read size for a persisted descriptor batch: its raw + /// Arrow allocation, every ordinary column and descriptor buffer, as + /// framing, and the logical payload its Blob cells will materialize. The + /// descriptor buffers may overestimate the smaller logical Blob framing, + /// but cannot understate retained memory. pub(crate) fn predicted_materialized_blob_batch_bytes( &self, ds: &Dataset, batch: &RecordBatch, external: &ExternalBlobPreflight, max_blob_bytes: u64, - ) -> Result { + ) -> Result { let payload = self.persisted_blob_payload_bytes(ds.schema(), batch, external)?; if payload > max_blob_bytes { return Err(OmniError::resource_limit( @@ -1950,12 +1952,7 @@ impl TableStore { payload, )); } - let retained = u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("persisted Blob batch memory size exceeds u64") - })?; - retained.checked_add(payload).ok_or_else(|| { - OmniError::manifest_internal("materialized Blob batch byte count overflow") - }) + KeyedBytes::of(batch)?.checked_add(KeyedBytes::payload(payload)) } async fn rebuild_blob_column( @@ -2787,8 +2784,6 @@ impl TableStore { "stage_keyed_write called with empty batch", )); } - let batch_bytes = u64::try_from(batch.get_array_memory_size()) - .map_err(|_| OmniError::manifest_internal("keyed write batch bytes exceed u64"))?; if batch.num_rows() > KEYED_WRITE_MAX_ROWS { return Err(OmniError::resource_limit( format!("keyed write entities for {type_key}"), @@ -2796,13 +2791,7 @@ impl TableStore { batch.num_rows() as u64, )); } - if batch_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed write bytes for {type_key}"), - KEYED_WRITE_MAX_BYTES, - batch_bytes, - )); - } + KeyedBytes::of(&batch)?.ensure_fits(&format!("keyed write bytes for {type_key}"))?; let id_field_id = exact_id_primary_key_field_id(&ds, system_columns, "stage_keyed_write")?; let expected_read_version = ds.version().version; let expected_schema_preorder_ids = schema_preorder_field_ids(&ds, "stage_keyed_write")?; @@ -2957,9 +2946,6 @@ impl TableStore { "{context} called with empty batch" ))); } - let batch_bytes = u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal(format!("{context} batch bytes exceed u64")) - })?; if batch.num_rows() > KEYED_WRITE_MAX_ROWS { return Err(OmniError::resource_limit( format!("keyed write entities for {table_key}"), @@ -2967,13 +2953,7 @@ impl TableStore { batch.num_rows() as u64, )); } - if batch_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed write bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - batch_bytes, - )); - } + KeyedBytes::of(&batch)?.ensure_fits(&format!("keyed write bytes for {table_key}"))?; let batch = self .prepare_keyed_write_batch(table_key, batch, system_columns) @@ -3108,38 +3088,21 @@ impl TableStore { self.validate_keyed_write_batch(table_key, &batch, system_columns)?; let external_uris = collect_external_blob_uris(&batch)?; let payload_bytes = preflight.materialized_payload_bytes(&external_uris)?; - if payload_bytes > KEYED_WRITE_MAX_BYTES { + if payload_bytes > KEYED_BLOB_PAYLOAD_MAX_BYTES { return Err(OmniError::resource_limit( "materialized external blob payload bytes", - KEYED_WRITE_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES, payload_bytes, )); } - let retained_bytes = u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("keyed write input batch bytes exceed u64") - })?; - let predicted_bytes = retained_bytes.checked_add(payload_bytes).ok_or_else(|| { - OmniError::manifest_internal("materialized keyed Blob byte count overflow") - })?; - if predicted_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed write bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - predicted_bytes, - )); - } + let resource = format!("keyed write bytes for {table_key}"); + KeyedBytes::of(&batch)? + .checked_add(KeyedBytes::payload(payload_bytes))? + .ensure_fits(&resource)?; let batch = - materialize_external_blob_inputs(batch, KEYED_WRITE_MAX_BYTES, preflight).await?; - let materialized_bytes = u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("materialized keyed write batch bytes exceed u64") - })?; - if materialized_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("keyed write bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - materialized_bytes, - )); - } + materialize_external_blob_inputs(batch, KEYED_BLOB_PAYLOAD_MAX_BYTES, preflight) + .await?; + KeyedBytes::of(&batch)?.ensure_fits(&resource)?; Ok(batch) } @@ -4643,7 +4606,7 @@ impl TableStore { // columns now so an already-overwide match fails before // `take_rows` performs the second full-row read. let non_blob_bytes = non_blob_column_bytes(committed_ds, &batch)?; - let payload_budget = account.remaining_bytes_after(non_blob_bytes)?; + let payload_budget = account.payload_remaining_after(non_blob_bytes)?; if let Some(schema) = &carried_without_blobs { let carried_rows = predicate_rows_as(schema, &batch)?; @@ -4690,16 +4653,10 @@ impl TableStore { .unwrap_or(error)); } Err(OmniError::ResourceLimitExceeded { actual, .. }) => { - let actual = account - .bytes_with(non_blob_bytes)? - .checked_add(actual) - .ok_or_else(|| { - OmniError::manifest_internal("pending scan byte count overflow") - })?; return Err(OmniError::resource_limit( - "retained keyed batch bytes per operation", - KEYED_WRITE_MAX_BYTES, - actual, + blob_payload_resource("retained keyed batch bytes per operation"), + KEYED_BLOB_PAYLOAD_MAX_BYTES, + account.payload_with(actual)?, )); } Err(error) => return Err(error), @@ -4974,7 +4931,7 @@ fn assign_row_id_meta(fragments: &mut [Fragment], start_row_id: u64) -> Result<( struct PendingScanAccount { table_key: String, rows: u64, - bytes: u64, + bytes: KeyedBytes, } impl PendingScanAccount { @@ -4982,7 +4939,7 @@ impl PendingScanAccount { let mut account = Self { table_key: budget.table_key, rows: 0, - bytes: 0, + bytes: KeyedBytes::default(), }; account.add_usage(budget.initial_rows, budget.initial_bytes)?; Ok(account) @@ -5002,12 +4959,10 @@ impl PendingScanAccount { fn add_batch(&mut self, batch: &RecordBatch) -> Result<()> { let rows = u64::try_from(batch.num_rows()) .map_err(|_| OmniError::manifest_internal("pending scan row count exceeds u64"))?; - let bytes = u64::try_from(batch.get_array_memory_size()) - .map_err(|_| OmniError::manifest_internal("pending scan bytes exceed u64"))?; - self.add_usage(rows, bytes) + self.add_usage(rows, KeyedBytes::of(batch)?) } - fn add_usage(&mut self, rows: u64, bytes: u64) -> Result<()> { + fn add_usage(&mut self, rows: u64, bytes: KeyedBytes) -> Result<()> { let next_rows = self .rows .checked_add(rows) @@ -5021,15 +4976,8 @@ impl PendingScanAccount { } let next_bytes = self .bytes - .checked_add(bytes) - .ok_or_else(|| OmniError::manifest_internal("pending scan byte count overflow"))?; - if next_bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - "retained keyed batch bytes per operation", - KEYED_WRITE_MAX_BYTES, - next_bytes, - )); - } + .checked_add(bytes)? + .ensure_fits("retained keyed batch bytes per operation")?; self.rows = next_rows; self.bytes = next_bytes; Ok(()) @@ -5052,22 +5000,21 @@ impl PendingScanAccount { Ok(()) } - fn bytes_with(&self, bytes: u64) -> Result { + fn payload_with(&self, payload: u64) -> Result { self.bytes - .checked_add(bytes) - .ok_or_else(|| OmniError::manifest_internal("pending scan byte count overflow")) + .payload + .checked_add(payload) + .ok_or_else(|| OmniError::manifest_internal("pending scan payload byte count overflow")) } - fn remaining_bytes_after(&self, bytes: u64) -> Result { - let actual = self.bytes_with(bytes)?; - if actual > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - "retained keyed batch bytes per operation", - KEYED_WRITE_MAX_BYTES, - actual, - )); - } - Ok(KEYED_WRITE_MAX_BYTES - actual) + /// Charge a match's non-Blob framing and return the payload its carried + /// Blob cells may still materialize. + fn payload_remaining_after(&self, framing: u64) -> Result { + let bytes = self + .bytes + .checked_add(KeyedBytes::framing(framing))? + .ensure_fits("retained keyed batch bytes per operation")?; + Ok(KEYED_BLOB_PAYLOAD_MAX_BYTES - bytes.payload) } fn next_scan_rows(&self) -> usize { @@ -5078,7 +5025,9 @@ impl PendingScanAccount { } fn next_scan_bytes(&self) -> u64 { - KEYED_WRITE_MAX_BYTES.saturating_sub(self.bytes).max(1) + KEYED_WRITE_MAX_BYTES + .saturating_sub(self.bytes.framing) + .max(1) } } @@ -5257,6 +5206,38 @@ struct LogicalBlobInput<'a> { uris: &'a StringArray, } +/// The logical length of every managed Blob value a batch carries in the +/// logical input shape (`{data, uri}`): the payload half of +/// [`crate::storage_layer::KeyedBytes`]. A Blob column in any other shape, such +/// as a persisted descriptor, holds no payload bytes and counts as framing. +pub(crate) fn logical_blob_payload_bytes(batch: &RecordBatch) -> Result { + let mut total = 0_u64; + for (field, column) in batch.schema().fields().iter().zip(batch.columns()) { + let is_blob = + lance::datatypes::Field::try_from(field.as_ref()).is_ok_and(|field| field.is_blob()); + if !is_blob { + continue; + } + let Some(descriptions) = column.as_any().downcast_ref::() else { + continue; + }; + let Ok(input) = logical_blob_input(descriptions, field.name()) else { + continue; + }; + for row in 0..descriptions.len() { + if descriptions.is_null(row) || input.data.is_null(row) { + continue; + } + let length = u64::try_from(input.data.value_length(row)) + .map_err(|_| OmniError::manifest_internal("negative Blob payload length"))?; + total = total + .checked_add(length) + .ok_or_else(|| OmniError::manifest_internal("Blob payload byte count overflow"))?; + } + } + Ok(total) +} + fn logical_blob_input<'a>( descriptions: &'a StructArray, field_name: &str, @@ -5973,7 +5954,7 @@ fn bounded_proven_insert_stream( None::<(RecordBatch, usize)>, Vec::::new(), 0_usize, - 0_u64, + KeyedBytes::default(), schema, table_key, ), @@ -5992,21 +5973,17 @@ fn bounded_proven_insert_stream( if let Some((batch, consumed_rows)) = pending.take() { let batch_rows = batch.num_rows(); let batch_bytes = - u64::try_from(batch.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal( - "proven insert delta batch bytes exceed u64", - ) - .into_datafusion_external() - })?; + KeyedBytes::of(&batch).map_err(OmniError::into_datafusion_external)?; + let next_bytes = accumulated_bytes + .checked_add(batch_bytes) + .map_err(OmniError::into_datafusion_external)?; let fits = accumulated_rows .checked_add(batch_rows) .is_some_and(|rows| rows <= KEYED_WRITE_MAX_ROWS) - && accumulated_bytes - .checked_add(batch_bytes) - .is_some_and(|bytes| bytes <= KEYED_WRITE_MAX_BYTES); + && next_bytes.fits(); if accumulated.is_empty() || fits { accumulated_rows += batch_rows; - accumulated_bytes += batch_bytes; + accumulated_bytes = next_bytes; accumulated.push(batch); current_offset += consumed_rows; if current @@ -6016,9 +5993,7 @@ fn bounded_proven_insert_stream( current = None; current_offset = 0; } - if accumulated_rows == KEYED_WRITE_MAX_ROWS - || accumulated_bytes == KEYED_WRITE_MAX_BYTES - { + if accumulated_rows == KEYED_WRITE_MAX_ROWS || accumulated_bytes.is_full() { let output = finish_proven_insert_batch( &schema, std::mem::take(&mut accumulated), @@ -6034,7 +6009,7 @@ fn bounded_proven_insert_stream( None, Vec::new(), 0, - 0, + KeyedBytes::default(), schema, table_key, ), @@ -6059,7 +6034,7 @@ fn bounded_proven_insert_stream( pending, Vec::new(), 0, - 0, + KeyedBytes::default(), schema, table_key, ), @@ -6102,7 +6077,7 @@ fn bounded_proven_insert_stream( pending, Vec::new(), 0, - 0, + KeyedBytes::default(), schema, table_key, ), @@ -6149,43 +6124,47 @@ fn next_proven_insert_batch( let (mut rows, mut logical_bytes) = largest_proven_insert_prefix(batch, offset, max_rows)?; loop { let is_whole_batch = offset == 0 && rows == batch.num_rows(); - let retained_bytes = u64::try_from(batch.get_array_memory_size()).unwrap_or(u64::MAX); + let retained = KeyedBytes::of(batch)?; + let retained_bytes = retained.framing.saturating_add(retained.payload); + let logical_total = logical_bytes.framing.saturating_add(logical_bytes.payload); // ArrayData's logical slice measure excludes the backing capacity held // by an Arrow slice. Preserve an already-compact whole scanner batch, // but copy any selected prefix or obvious retained-parent emission. let retained_parent_slop = 64_u64 * 1024; let keeps_only_logical_slice = - retained_bytes <= logical_bytes.saturating_add(retained_parent_slop); - let candidate = if is_whole_batch - && retained_bytes <= KEYED_WRITE_MAX_BYTES - && keeps_only_logical_slice - { + retained_bytes <= logical_total.saturating_add(retained_parent_slop); + let candidate = if is_whole_batch && retained.fits() && keeps_only_logical_slice { batch.clone() } else { copy_proven_insert_batch_range(batch, offset, rows)? }; - let bytes = u64::try_from(candidate.get_array_memory_size()).map_err(|_| { - OmniError::manifest_internal("proven insert delta batch bytes exceed u64") - })?; - if bytes <= KEYED_WRITE_MAX_BYTES { + let bytes = KeyedBytes::of(&candidate)?; + if bytes.fits() { validate_proven_insert_source_batch(&candidate, table_key)?; return Ok((candidate, rows)); } if rows == 1 { - return Err(OmniError::resource_limit( - format!("proven insert delta bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - bytes, - )); + bytes.ensure_fits(&format!("proven insert delta bytes for {table_key}"))?; } drop(candidate); - rows = ((u128::try_from(rows).unwrap() * u128::from(KEYED_WRITE_MAX_BYTES)) - / u128::from(bytes)) + rows = shrunk_row_count(rows, bytes).clamp(1, rows - 1); + logical_bytes = proven_insert_slice_bytes(batch, offset, rows)?; + } +} + +/// Scale a row count down in proportion to the half of `bytes` that most +/// exceeds its ceiling. +fn shrunk_row_count(rows: usize, bytes: KeyedBytes) -> usize { + let scaled = |actual: u64, ceiling: u64| { + if actual <= ceiling { + return u128::MAX; + } + u128::try_from(rows).unwrap_or(u128::MAX) * u128::from(ceiling) / u128::from(actual) + }; + scaled(bytes.framing, KEYED_WRITE_MAX_BYTES) + .min(scaled(bytes.payload, KEYED_BLOB_PAYLOAD_MAX_BYTES)) .try_into() .unwrap_or(1_usize) - .clamp(1, rows - 1); - logical_bytes = proven_insert_slice_memory_size(batch, offset, rows)?; - } } /// Select the largest row prefix whose logical Arrow buffers fit the byte @@ -6196,14 +6175,14 @@ fn largest_proven_insert_prefix( batch: &RecordBatch, offset: usize, max_rows: usize, -) -> Result<(usize, u64)> { - let one_row_bytes = proven_insert_slice_memory_size(batch, offset, 1)?; - if max_rows == 1 || one_row_bytes > KEYED_WRITE_MAX_BYTES { +) -> Result<(usize, KeyedBytes)> { + let one_row_bytes = proven_insert_slice_bytes(batch, offset, 1)?; + if max_rows == 1 || !one_row_bytes.fits() { return Ok((1, one_row_bytes)); } - let max_bytes = proven_insert_slice_memory_size(batch, offset, max_rows)?; - if max_bytes <= KEYED_WRITE_MAX_BYTES { + let max_bytes = proven_insert_slice_bytes(batch, offset, max_rows)?; + if max_bytes.fits() { return Ok((max_rows, max_bytes)); } @@ -6211,13 +6190,27 @@ fn largest_proven_insert_prefix( let mut high = max_rows - 1; while low < high { let middle = low + (high - low).div_ceil(2); - if proven_insert_slice_memory_size(batch, offset, middle)? <= KEYED_WRITE_MAX_BYTES { + if proven_insert_slice_bytes(batch, offset, middle)?.fits() { low = middle; } else { high = middle - 1; } } - Ok((low, proven_insert_slice_memory_size(batch, offset, low)?)) + Ok((low, proven_insert_slice_bytes(batch, offset, low)?)) +} + +/// The selected rows' compact Arrow size, split like [`KeyedBytes::of`]. +fn proven_insert_slice_bytes( + batch: &RecordBatch, + offset: usize, + rows: usize, +) -> Result { + let total = proven_insert_slice_memory_size(batch, offset, rows)?; + let payload = logical_blob_payload_bytes(&batch.slice(offset, rows))?; + let framing = total.checked_sub(payload).ok_or_else(|| { + OmniError::manifest_internal("Blob payload bytes exceed their slice's Arrow memory") + })?; + Ok(KeyedBytes { framing, payload }) } fn proven_insert_slice_memory_size(batch: &RecordBatch, offset: usize, rows: usize) -> Result { @@ -6275,15 +6268,7 @@ fn validate_proven_insert_source_batch(batch: &RecordBatch, table_key: &str) -> batch.num_rows() as u64, )); } - let bytes = u64::try_from(batch.get_array_memory_size()) - .map_err(|_| OmniError::manifest_internal("proven insert delta batch bytes exceed u64"))?; - if bytes > KEYED_WRITE_MAX_BYTES { - return Err(OmniError::resource_limit( - format!("proven insert delta bytes for {table_key}"), - KEYED_WRITE_MAX_BYTES, - bytes, - )); - } + KeyedBytes::of(batch)?.ensure_fits(&format!("proven insert delta bytes for {table_key}"))?; Ok(()) } diff --git a/crates/omnigraph/src/table_store/staged_tests.rs b/crates/omnigraph/src/table_store/staged_tests.rs index b014a169a..5b1272a68 100644 --- a/crates/omnigraph/src/table_store/staged_tests.rs +++ b/crates/omnigraph/src/table_store/staged_tests.rs @@ -23,8 +23,8 @@ use crate::error::{OmniError, StorageFailureKind}; use crate::instrumentation::{MergeWriteProbes, with_merge_write_probes}; use crate::storage_layer::{ - IndexBuildSpec, KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, KeyedWriteSemantics, - PendingScanBudget, ProvenInsertChunk, SnapshotHandle, + IndexBuildSpec, KEYED_BLOB_PAYLOAD_MAX_BYTES, KEYED_WRITE_MAX_BYTES, KEYED_WRITE_MAX_ROWS, + KeyedBytes, KeyedWriteSemantics, PendingScanBudget, ProvenInsertChunk, SnapshotHandle, }; use crate::table_store::{StagedWrite, TableStore}; use arrow_array::{Array, Int32Array, RecordBatch, StringArray, StructArray, UInt64Array}; @@ -253,19 +253,90 @@ fn collect_age_for_id(batches: &[RecordBatch], needle: &str) -> Option { None } +/// A keyed batch charges a managed Blob value's logical length to the payload +/// ceiling and the Arrow bytes around it to the framing ceiling, so a value of +/// exactly the payload ceiling fits, though the batch's whole Arrow size is over +/// it, and one more byte is refused by the payload ceiling. An external +/// reference's URI is framing: it carries no payload. +#[test] +fn keyed_bytes_split_blob_payload_from_framing() { + let limit = usize::try_from(KEYED_BLOB_PAYLOAD_MAX_BYTES).unwrap(); + let exact = blob_person_pk_batch("alice", &vec![7_u8; limit]); + let bytes = KeyedBytes::of(&exact).unwrap(); + assert_eq!(bytes.payload, KEYED_BLOB_PAYLOAD_MAX_BYTES); + assert!(bytes.framing > 0, "the batch around a value is never empty"); + assert_eq!( + bytes.framing + bytes.payload, + u64::try_from(exact.get_array_memory_size()).unwrap() + ); + assert!(bytes.framing + bytes.payload > KEYED_WRITE_MAX_BYTES); + assert_eq!( + bytes + .ensure_fits("keyed write bytes for node:Person") + .unwrap(), + bytes + ); + + let over = blob_person_pk_batch("alice", &vec![7_u8; limit + 1]); + match KeyedBytes::of(&over) + .unwrap() + .ensure_fits("keyed write bytes for node:Person") + { + Err(OmniError::ResourceLimitExceeded { + resource, + limit, + actual, + }) => { + assert_eq!(resource, "keyed write Blob payload bytes for node:Person"); + assert_eq!( + (limit, actual), + ( + KEYED_BLOB_PAYLOAD_MAX_BYTES, + KEYED_BLOB_PAYLOAD_MAX_BYTES + 1 + ) + ); + } + other => panic!("one payload byte over must be refused, got {other:?}"), + } + + let mut external = lance::blob::BlobArrayBuilder::new(1); + external.push_uri("s3://bucket/object.bin").unwrap(); + let schema = Arc::new(Schema::new(vec![ + Field::new("id", DataType::Utf8, false), + lance::blob::blob_field("content", true), + ])); + let reference = RecordBatch::try_new( + schema, + vec![ + Arc::new(StringArray::from(vec!["bob"])) as _, + external.finish().unwrap(), + ], + ) + .unwrap(); + let bytes = KeyedBytes::of(&reference).unwrap(); + assert_eq!(bytes.payload, 0); + assert_eq!( + bytes.framing, + u64::try_from(reference.get_array_memory_size()).unwrap() + ); +} + #[test] fn pending_scan_budget_caps_are_inclusive_and_one_over_is_typed() { PendingScanAccount::new(PendingScanBudget::new( "test:people", KEYED_WRITE_MAX_ROWS as u64, - KEYED_WRITE_MAX_BYTES, + KeyedBytes { + framing: KEYED_WRITE_MAX_BYTES, + payload: KEYED_BLOB_PAYLOAD_MAX_BYTES, + }, )) - .expect("the exact keyed row/byte limits are inclusive"); + .expect("the exact keyed row, framing and payload limits are inclusive"); let row_error = PendingScanAccount::new(PendingScanBudget::new( "test:people", KEYED_WRITE_MAX_ROWS as u64 + 1, - 0, + KeyedBytes::default(), )) .err() .expect("one row over must be rejected"); @@ -281,7 +352,7 @@ fn pending_scan_budget_caps_are_inclusive_and_one_over_is_typed() { let byte_error = PendingScanAccount::new(PendingScanBudget::new( "test:people", 0, - KEYED_WRITE_MAX_BYTES + 1, + KeyedBytes::framing(KEYED_WRITE_MAX_BYTES + 1), )) .err() .expect("one byte over must be rejected"); @@ -294,6 +365,23 @@ fn pending_scan_budget_caps_are_inclusive_and_one_over_is_typed() { } if resource == "retained keyed batch bytes per operation" && actual == KEYED_WRITE_MAX_BYTES + 1 )); + + let payload_error = PendingScanAccount::new(PendingScanBudget::new( + "test:people", + 0, + KeyedBytes::payload(KEYED_BLOB_PAYLOAD_MAX_BYTES + 1), + )) + .err() + .expect("one payload byte over must be rejected"); + assert!(matches!( + payload_error, + OmniError::ResourceLimitExceeded { + ref resource, + limit: KEYED_BLOB_PAYLOAD_MAX_BYTES, + actual, + } if resource == "retained keyed batch Blob payload bytes per operation" + && actual == KEYED_BLOB_PAYLOAD_MAX_BYTES + 1 + )); } #[tokio::test] @@ -1452,7 +1540,7 @@ async fn scan_with_pending_rejects_key_column_missing_from_projection() { Some(&["note"]), None, Some("id"), - PendingScanBudget::new("test:people", 0, 0), + PendingScanBudget::new("test:people", 0, KeyedBytes::default()), ) .await .expect_err("scan_with_pending must reject merge-shadow with missing key in projection"); @@ -1474,7 +1562,7 @@ async fn scan_with_pending_rejects_key_column_missing_from_projection() { Some(&["id", "note"]), None, Some("id"), - PendingScanBudget::new("test:people", 8190, 0), + PendingScanBudget::new("test:people", 8190, KeyedBytes::default()), ) .await .expect("projection containing key_column must succeed"); @@ -1495,7 +1583,7 @@ async fn scan_with_pending_rejects_key_column_missing_from_projection() { Some(&["id", "note"]), None, Some("id"), - PendingScanBudget::new("test:people", 8191, 0), + PendingScanBudget::new("test:people", 8191, KeyedBytes::default()), ) .await .expect_err("pending + unshadowed committed output must share the row budget"); diff --git a/crates/omnigraph/tests/lance_surface_guards.rs b/crates/omnigraph/tests/lance_surface_guards.rs index 0f840b66b..ba54b79a5 100644 --- a/crates/omnigraph/tests/lance_surface_guards.rs +++ b/crates/omnigraph/tests/lance_surface_guards.rs @@ -5054,6 +5054,9 @@ async fn camelcase_index_equality_routes_to_scalar_index() { // a silently-wrong batch) in release. Faithful transcription of lance#7444's // minimal repro: merge-seed → merge-update → delete → filter + with_row_id. // This guard turns red if a future Lance bump regresses the upstream fix. +// It also pins that the merge-update keeps the updated row's stable row id: +// a Blob write reads that id back from its own detached commit to form the +// ETag a later read at the published snapshot returns (RFC 0033 §4.3). #[tokio::test] async fn filtered_scan_tolerates_merge_update_row_id_overlap() { use futures::TryStreamExt; @@ -5108,11 +5111,29 @@ async fn filtered_scan_tolerates_merge_update_row_id_overlap() { (1..=40).map(|i| format!("r{i}")).collect(), ); let ds = merge(ds, seed, schema.clone()).await; + let row_id_of = |ds: Dataset, slug: &'static str| async move { + let mut scan = ds.scan(); + scan.with_row_id(); + scan.filter(&format!("slug = '{slug}'")).unwrap(); + let batch = scan.try_into_batch().await.unwrap(); + assert_eq!(batch.num_rows(), 1, "{slug}"); + batch + .column_by_name(ROW_ID) + .unwrap() + .as_primitive::() + .value(0) + }; + let seeded_row_id = row_id_of(ds.clone(), "t3").await; let updates = mk_batch( (1..=15).map(|i| format!("t{i}")).collect(), (1..=15).map(|i| format!("e{i}")).collect(), ); let ds = Arc::new(merge(ds, updates, schema.clone()).await); + assert_eq!( + row_id_of((*ds).clone(), "t3").await, + seeded_row_id, + "a whole-row merge-update keeps the updated row's stable row id" + ); // The delete's deletion vector makes the overlapping id region sparse. let staged = lance::dataset::DeleteBuilder::new(ds.clone(), "slug = 't20'") @@ -5144,6 +5165,72 @@ async fn filtered_scan_tolerates_merge_update_row_id_overlap() { } } +// --- Guard: merge-insert refuses an external Blob reference outside the bases --- +// +// Lance 11's merge-insert writer stages its files with `WriteParams::default()`, +// whose `allow_external_blob_outside_bases` is false, and a merge-insert takes +// no other write params. A keyed write therefore cannot carry a stored external +// reference as a reference: the engine reads it and stores managed bytes, or +// refuses it under a denying policy (RFC 0033 §4.3). When this goes red, +// merge-insert stores such a reference, and carrying a sibling's reference +// without reading it is open again (RFC 0033, Phase 4). +#[tokio::test] +async fn merge_insert_refuses_an_external_blob_outside_the_dataset_bases() { + let dir = tempfile::tempdir().unwrap(); + let external_path = dir.path().join("outside.bin"); + std::fs::write(&external_path, b"outside the dataset").unwrap(); + let external_uri = url::Url::from_file_path(&external_path) + .unwrap() + .to_string(); + let schema = Arc::new(Schema::new(vec![ + Field::new("id", DataType::Utf8, false), + lance::blob::blob_field("content", true), + ])); + let row = |content: arrow_array::ArrayRef| { + RecordBatch::try_new( + schema.clone(), + vec![Arc::new(StringArray::from(vec!["a"])), content], + ) + .unwrap() + }; + let mut managed = BlobArrayBuilder::new(1); + managed.push_bytes(b"managed").unwrap(); + let ds = Dataset::write( + RecordBatchIterator::new(vec![Ok(row(managed.finish().unwrap()))], schema.clone()), + dir.path().join("table.lance").to_str().unwrap(), + Some(WriteParams { + mode: WriteMode::Create, + enable_stable_row_ids: true, + data_storage_version: Some(LanceFileVersion::V2_2), + ..Default::default() + }), + ) + .await + .unwrap(); + + let mut external = BlobArrayBuilder::new(1); + external.push_uri(external_uri.as_str()).unwrap(); + let job = MergeInsertBuilder::try_new(Arc::new(ds), vec!["id".to_string()]) + .unwrap() + .when_matched(WhenMatched::UpdateAll) + .when_not_matched(WhenNotMatched::InsertAll) + .try_build() + .unwrap(); + let error = job + .execute_reader(RecordBatchIterator::new( + vec![Ok(row(external.finish().unwrap()))], + schema.clone(), + )) + .await + .expect_err("merge-insert must refuse a reference outside the dataset's bases"); + assert!( + error + .to_string() + .contains("is outside registered external bases"), + "{error}" + ); +} + // --- Guard 21: starts_with routes to the BTREE (LikePrefix) and stays literal -- // // The .gq `starts_with` predicate lowers to the DataFusion `starts_with` diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index 515d1c95e..4189a6121 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -672,6 +672,7 @@ async fn mutation_keyed_write_row_cap_accepts_limit_and_rejects_one_over_pre_eff const OPERATION_BYTES: u64 = 32 * 1024 * 1024; const KEYED_BATCH_BYTES: &str = "retained keyed batch bytes per operation"; +const KEYED_BATCH_PAYLOAD_BYTES: &str = "retained keyed batch Blob payload bytes per operation"; const REMOVED_ID_BYTES: &str = "retained removed-id bytes per operation"; fn operation_refusal(error: &OmniError, label: &str) -> bool { @@ -891,10 +892,15 @@ edge LinkZ: Zed -> Zed { n: I32? } ); } -/// 24 MiB of external payload plus 10 MiB of retained rows fit per type, not together. +/// External payload to be copied joins the operation's Blob payload allowance +/// with managed payload, before any payload read: 12 MiB copied plus 21 MiB +/// managed is refused although each fits. Retained rows have their own +/// allowance, so 24 MiB copied plus 10 MiB of rows fits. /// Rust, not GQT: MiB-scale inputs and the payload-read probe are outside the case format. #[tokio::test] -async fn external_blob_bytes_join_the_operation_allowance_before_any_payload_read() { +async fn external_blob_bytes_join_the_operation_payload_allowance_before_any_payload_read() { + use base64::Engine; + const SCHEMA: &str = "\ node Document { title: String @key content: Blob? note: String? } node Image { title: String @key content: Blob? note: String? } @@ -926,7 +932,10 @@ node Image { title: String @key content: Blob? note: String? } let before_manifest = snapshot_main(&db).await.unwrap().graph_manifest_version(); let files = files_under(&graph_path); - let note = "x".repeat(5 * 1024 * 1024); + let managed = format!( + "base64:{}", + base64::engine::general_purpose::STANDARD.encode(vec![7_u8; 21 * 1024 * 1024]) + ); let stage_probes = StageWriteProbes::rendezvous(1); let read_probes = omnigraph::instrumentation::MergeWriteProbes::default(); let error = with_stage_write_probes( @@ -935,19 +944,19 @@ node Image { title: String @key content: Blob? note: String? } read_probes.clone(), db.mutate( "main", - r#"query wide($uri: String, $note: String) { - insert Document { title: "one", content: $uri, note: $note } - insert Image { title: "two", content: $uri, note: $note } + r#"query wide($uri: String, $managed: Blob) { + insert Document { title: "one", content: $uri } + insert Image { title: "two", content: $managed } }"#, "wide", - ¶ms(&[("$uri", &external_uri), ("$note", ¬e)]), + ¶ms(&[("$uri", &external_uri), ("$managed", &managed)]), ), ), ) .await - .expect_err("external payloads and retained rows must share one operation allowance"); + .expect_err("copied and managed payloads must share one operation allowance"); assert!( - operation_refusal(&error, KEYED_BATCH_BYTES), + operation_refusal(&error, KEYED_BATCH_PAYLOAD_BYTES), "unexpected refusal: {error:?}" ); assert_eq!( @@ -962,27 +971,32 @@ node Image { title: String @key content: Blob? note: String? } snapshot_main(&db).await.unwrap().graph_manifest_version(), before_manifest ); + let note = "x".repeat(5 * 1024 * 1024); let accepted = db .mutate( "main", - r#"query small($uri: String) { - insert Document { title: "one", content: $uri } + r#"query wide($uri: String, $note: String) { + insert Document { title: "one", content: $uri, note: $note } + insert Image { title: "two", content: $uri, note: $note } }"#, - "small", - ¶ms(&[("$uri", &external_uri)]), + "wide", + ¶ms(&[("$uri", &external_uri), ("$note", ¬e)]), ) .await - .expect("one 12 MiB external payload fits after the refusal"); - assert_eq!(accepted.affected_nodes, 1); + .expect("24 MiB copied and 10 MiB of rows fit under separate allowances"); + assert_eq!(accepted.affected_nodes, 2); } -/// Three equal payloads leave a LargeBinary buffer at 4/3 of its content: predicted -/// 24.4 MiB, materialized 32.5 MiB. Rust, not GQT: MiB-scale inputs, builder capacity. +/// Three equal payloads leave a LargeBinary buffer at 4/3 of its content, and +/// the unused capacity, about 8.1 MiB over both types, is framing. With 13 MiB +/// of rows per type the predicted framing, 26 MiB, fits; the materialized +/// framing, about 34.1 MiB, does not. Rust, not GQT: MiB-scale inputs, builder +/// capacity. #[tokio::test] async fn materialized_blob_batches_are_rechecked_against_the_operation_allowance_before_staging() { const SCHEMA: &str = "\ -node Document { title: String @key content: Blob? } -node Image { title: String @key content: Blob? } +node Document { title: String @key content: Blob? note: String? } +node Image { title: String @key content: Blob? note: String? } "; let dir = tempfile::tempdir().unwrap(); @@ -1019,16 +1033,19 @@ node Image { title: String @key content: Blob? } read_probes.clone(), db.mutate( "main", - r#"query thrice($uri: String) { - insert Document { title: "a", content: $uri } + r#"query thrice($uri: String, $note: String) { + insert Document { title: "a", content: $uri, note: $note } insert Document { title: "b", content: $uri } insert Document { title: "c", content: $uri } - insert Image { title: "a", content: $uri } + insert Image { title: "a", content: $uri, note: $note } insert Image { title: "b", content: $uri } insert Image { title: "c", content: $uri } }"#, "thrice", - ¶ms(&[("$uri", &external_uri)]), + ¶ms(&[ + ("$uri", &external_uri), + ("$note", &"x".repeat(13 * 1024 * 1024)), + ]), ), ), ) @@ -1223,7 +1240,8 @@ query update_note($note: String) { ref resource, limit: LIMIT, actual, - } if resource == "retained keyed batch bytes per operation" && actual > LIMIT + } if resource == "retained keyed batch Blob payload bytes per operation" + && actual == LIMIT + 1 ), "oversized update blob must be rejected before payload read, got {error:?}" ); @@ -1286,6 +1304,143 @@ query replace_content($c: Blob) { ) .await; assert_eq!(&bytes[..], &[1, 2, 3]); + + // The payload ceiling is inclusive: an external cell of exactly the limit + // is carried beside its row, read once and stored as managed bytes. + let exact_path = dir.path().join("exact.blob"); + let file = std::fs::File::create(&exact_path).unwrap(); + file.set_len(LIMIT).unwrap(); + drop(file); + let exact_row = serde_json::json!({ + "type": "Document", + "data": { + "title": "wide", + "content": format!("file://{}", exact_path.display()), + } + }) + .to_string(); + db.load_jsonl(&exact_row, LoadMode::Overwrite) + .await + .unwrap(); + db.mutate( + "main", + UPDATE, + "update_note", + ¶ms(&[("$note", "carried")]), + ) + .await + .expect("an external cell of exactly the payload ceiling is carried"); + let bytes = read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", "wide", "content"), + ) + .await; + assert_eq!(bytes.len(), usize::try_from(LIMIT).unwrap()); + assert!(bytes.iter().all(|&byte| byte == 0)); +} + +/// A managed Blob value of exactly the payload ceiling fits beside its row: +/// payload and framing have separate ceilings, and the compatibility loader's +/// pre-decode forecast splits like the batch check. One more byte is refused +/// before any graph or table effect. The strict NDJSON loader bounds its +/// encoded line instead, so the exact-limit value's 44.7 MB line is refused. +/// Rust, not GQT: 32 MiB payloads are outside the case format. +#[tokio::test] +async fn exact_limit_blob_payload_fits_beside_its_row_and_one_more_byte_is_refused() { + use base64::Engine; + + const LIMIT: usize = 32 * 1024 * 1024; + const SCHEMA: &str = "node Document { title: String @key content: Blob? }\n"; + const INSERT: &str = r#" +query put_doc($title: String, $c: Blob) { + insert Document { title: $title, content: $c } +} +"#; + let encode = |bytes: usize| { + format!( + "base64:{}", + base64::engine::general_purpose::STANDARD.encode(vec![7_u8; bytes]) + ) + }; + let row = |title: &str, content: &str| { + serde_json::json!({"type": "Document", "data": {"title": title, "content": content}}) + .to_string() + }; + let dir = tempfile::tempdir().unwrap(); + let db = helpers::session( + Omnigraph::init(dir.path().to_str().unwrap(), SCHEMA) + .await + .unwrap(), + ); + + let exact = encode(LIMIT); + db.mutate( + "main", + INSERT, + "put_doc", + ¶ms(&[("$title", "inserted"), ("$c", &exact)]), + ) + .await + .expect("an insert admits an exact-limit value"); + db.load_jsonl(&row("loaded", &exact), LoadMode::Merge) + .await + .expect("the compatibility loader admits an exact-limit value"); + for title in ["inserted", "loaded"] { + let bytes = read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", title, "content"), + ) + .await; + assert_eq!(bytes.len(), LIMIT, "{title}"); + assert!(bytes.iter().all(|&byte| byte == 7), "{title}"); + } + + let before = snapshot_main(&db).await.unwrap().graph_manifest_version(); + let strict = db + .load_graph_batch("main", &row("strict", &exact), LoadMode::Merge) + .await + .expect_err("the strict loader bounds the encoded line"); + assert!( + matches!(&strict, OmniError::ResourceLimitExceeded { resource, .. } + if resource == "graph_batch_line_bytes"), + "{strict:?}" + ); + + let over = encode(LIMIT + 1); + let insert = db + .mutate( + "main", + INSERT, + "put_doc", + ¶ms(&[("$title", "over"), ("$c", &over)]), + ) + .await + .expect_err("one more byte is refused"); + let load = db + .load_jsonl(&row("over", &over), LoadMode::Merge) + .await + .expect_err("one more byte is refused"); + for (error, resource) in [ + (insert, "decoded blob input bytes"), + ( + load, + "keyed parsed entity Blob payload bytes for node:Document", + ), + ] { + assert!( + matches!(&error, OmniError::ResourceLimitExceeded { resource: actual_resource, limit, actual } + if actual_resource == resource + && *limit == LIMIT as u64 + && *actual == LIMIT as u64 + 1), + "{error:?}" + ); + } + assert_eq!( + snapshot_main(&db).await.unwrap().graph_manifest_version(), + before + ); } /// A predicate update carrying many rows' Blob cells reads the managed ones in diff --git a/docs/dev/execution.md b/docs/dev/execution.md index bdf7bb22c..f90fe2d8d 100644 --- a/docs/dev/execution.md +++ b/docs/dev/execution.md @@ -682,8 +682,9 @@ All load modes share the mutation publisher and recovery protocol: Mutation insert/update and keyed Load retain the per-table limits of 8,192 rows and 32 MiB, plus one 32 MiB sum of retained Arrow batches across touched tables. The sum uses `get_array_memory_size`, preserving conservative shared-buffer -counting. Keyed parsing separately caps its decoded-payload estimate across -types at 32 MiB before retaining each row. External Blob copy admission includes +counting, and managed Blob payloads are charged apart from it under their own +32 MiB ceilings (see [writes](writes.md)). Keyed parsing separately caps its +decoded-payload estimate across types at 32 MiB before retaining each row. External Blob copy admission includes the retained keyed batches plus copied payload estimates before payload reads; materialized batches are checked again before fragment staging. diff --git a/docs/dev/writes.md b/docs/dev/writes.md index da820d2ca..7a0ba34fe 100644 --- a/docs/dev/writes.md +++ b/docs/dev/writes.md @@ -342,12 +342,22 @@ Insert/update mutations and keyed Append/Merge loads also cap the sum of retained Arrow batches across tables at 32 MiB (`retained keyed batch bytes per operation`). Admission uses `get_array_memory_size` accounting; shared buffers may be conservatively counted -more than once. An update's pending-aware scan charges the same sum under the -same resource name. The keyed parse spool separately caps its decoded-payload -estimate across tables at 32 MiB (`keyed parsed entity bytes per operation`). -External Blob copy admission adds copied payload estimates, which are not yet -read, to the retained keyed batches under the same resource name before reading -payloads, then checks materialized batches before staging fragments. +more than once. Every keyed byte check splits that count with one function, +`KeyedBytes::of` in `storage_layer.rs`: the logical length of each managed Blob +value in the logical `{data, uri}` shape is payload, under its own 32 MiB +ceiling (`KEYED_BLOB_PAYLOAD_MAX_BYTES`, reported as `… Blob payload bytes …`), +and the rest is framing, under the existing ceilings. The halves sum to the +Arrow count, so the split moves no byte out of the accounting; one combined +ceiling could never admit a value of exactly 32 MiB beside its row. An update's +pending-aware scan charges the same sums under the same resource names, and +the payload remaining is the budget for reading its carried Blob cells. The +keyed parse spool separately caps its pre-decode estimate across tables at +32 MiB (`keyed parsed entity bytes per operation`), split the same way, a +`base64:` value counting its decoded length as payload. External Blob copy +admission adds copied payload estimates, which are not yet read, to the payload +half before reading payloads, then checks materialized batches before staging +fragments. Branch merge's row buffering and proven-insert chunking use the +same split. Delete mutations, cascades and Overwrite's removed-ID detection stream matches instead of collecting the full scan. One 32 MiB diff --git a/docs/user/blobs.md b/docs/user/blobs.md index e58f6ca0c..1f5d0e9fe 100644 --- a/docs/user/blobs.md +++ b/docs/user/blobs.md @@ -152,7 +152,8 @@ and retry. |---|---|---| | 32 MiB of decoded `base64:` bytes | Each node or edge type in one load, in every mode, including `overwrite` | `decoded blob input bytes for ` | | 32 MiB of decoded `base64:` bytes | One `base64:` value, in a load or in an insert or update mutation | `decoded blob input bytes` | -| 32 MiB per touched type, and 32 MiB across all touched types, Blob bytes included | Incremental writes: `append` and `merge` loads, inserts and updates. External bytes copied in and Blob values carried unchanged by an update count | `keyed write bytes for
`, `keyed entity bytes for
`, `retained keyed batch bytes per operation` | +| 32 MiB per touched type, and 32 MiB across all touched types, Blob payloads excluded | Incremental writes: `append` and `merge` loads, inserts, updates and branch merges. Every byte of the rows except managed Blob payloads counts, URIs and Blob framing included | `keyed write bytes for
`, `keyed entity bytes for
`, `retained keyed batch bytes per operation` | +| 32 MiB of managed Blob payload per touched type, and 32 MiB across all touched types, inclusive | The same writes. Each Blob value counts its length; external bytes copied in and Blob values carried unchanged by an update count. A single value of exactly 32 MiB fits beside its row | the same names with `Blob payload bytes` in place of `bytes`, for example `keyed entity Blob payload bytes for
` | | 32 MiB of external payload copied into managed storage | One incremental write operation across all its types, and each type within it: two types copying 20 MiB each exceed it although each fits its per-type limit | `materialized external blob payload bytes` | | 32 MiB of Blob payload | One branch merge that writes rows, across all types, managed and external bytes together | `materialized blob payload bytes` | | 8,192 external references | One write operation or merge | `external Blob reference cells` | @@ -165,7 +166,10 @@ and retry. The HTTP load request body is also capped at 32 MiB. That cap counts the encoded request, so one request carries about 24 MiB of decoded `base64:` -data. Every HTTP request other than a load (`/load` and `/load/ndjson`) is bounded by the default 1 MiB request body limit, so a `base64:` +data. The NDJSON loader that `omnigraph load` and `/load/ndjson` use caps each +encoded line at 32 MiB the same way. The embedded `load` API checks each row's +decoded size instead and has no line cap, so it admits a 32 MiB value; over +HTTP, `/load` keeps its 32 MiB body cap. Every HTTP request other than a load (`/load` and `/load/ndjson`) is bounded by the default 1 MiB request body limit, so a `base64:` literal in an HTTP mutation hits that limit first. Values larger than these limits stay readable. The CLI and the HTTP server diff --git a/docs/user/cli/managed-data.md b/docs/user/cli/managed-data.md index 2a974c333..778d3c0de 100644 --- a/docs/user/cli/managed-data.md +++ b/docs/user/cli/managed-data.md @@ -156,7 +156,8 @@ One managed load accepts at most **32 MiB (33,554,432 bytes)** of input. The CLI checks this before sending the request. Incremental `append` and `merge` also retain the engine's **8,192 rows and 32 MiB per keyed table** bounds, plus **32 MiB across retained keyed batches** and a separate parsed-payload estimate -across types. Strict loads check projected in-memory size too. The NDJSON byte +across types; managed Blob payloads have their own 32 MiB per table and across +tables. Strict loads check projected in-memory size too. The NDJSON byte bound does not prove that a batch fits those [engine limits](../mutations/index.md#limits-and-conflicts). Split prepared `append` and `merge` inputs into suitable batches, preserving diff --git a/docs/user/mutations/index.md b/docs/user/mutations/index.md index ead28817c..65c01b4ef 100644 --- a/docs/user/mutations/index.md +++ b/docs/user/mutations/index.md @@ -149,9 +149,12 @@ Loads preserve supplied embeddings and do not generate them. Results report Insert/update mutations and incremental keyed loads are bounded to 8,192 entities and 32 MiB per touched type, plus 32 MiB of retained Arrow batches -across all touched types in one operation. Keyed loads also have a separate -32 MiB parsed-payload estimate across types. External Blob payloads that require -copying count toward the aggregate allowance. Every strict load retains its +across all touched types in one operation. Managed Blob payloads are not part of +those bytes: they have their own 32 MiB per touched type and across all touched +types, so a single 32 MiB value fits beside its row. Keyed loads also have a +separate 32 MiB parsed-payload estimate across types, split the same way. +External Blob payloads that require copying count toward the payload +allowance. Every strict load retains its projected in-memory size check. Blob values have further limits; see [Blob limits](../blobs.md#limits). From 9425d9f5a66b1b10c581ab82ef99a24a8a2ab98a Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Thu, 8 Oct 2026 22:02:07 -0700 Subject: [PATCH 2/6] feat(blob): put and clear one Blob cell from an embedded Session Session::put_blob_at_as and clear_blob_at_as replace or clear one Blob value of an existing node or edge, addressed by exact id like a Blob read. A cell write is a Mutation-protocol write: it stages one upsert of the carried row and runs the Mutation tail, now split into commit_staged_mutation and publish_committed_mutation, which mutate uses too. The target's old payload is never read; the row's other cells are carried as an update carries them and share the 32 MiB payload allowance with the new value, which is bounded at 32 MiB inclusive before anything is captured. An optional precondition (an ETag set, or any existing value) is evaluated against the attempt's base and re-evaluated when a competing commit forces a re-prepare; a change of branch incarnation or accepted schema between attempts fails closed. The receipt's ETag is read from the detached commit before publication. --- changelog.d/blob-cell-put-clear.added.md | 12 + crates/omnigraph-core/src/error.rs | 14 + crates/omnigraph-server/src/blob_transport.rs | 1 + crates/omnigraph-server/src/lib.rs | 3 + crates/omnigraph/src/blob.rs | 234 +++++--- crates/omnigraph/src/exec/blob_write.rs | 501 ++++++++++++++++++ crates/omnigraph/src/exec/mod.rs | 1 + crates/omnigraph/src/exec/mutation.rs | 133 +++-- crates/omnigraph/src/exec/staging.rs | 8 + crates/omnigraph/src/lib.rs | 7 +- crates/omnigraph/src/loader/mod.rs | 1 + .../omnigraph/tests/detached_commit_matrix.rs | 89 +++- crates/omnigraph/tests/end_to_end.rs | 201 ++++++- crates/omnigraph/tests/failpoints.rs | 456 +++++++++++++++- crates/omnigraph/tests/forbidden_apis.rs | 2 + .../omnigraph/tests/policy_engine_chassis.rs | 43 ++ crates/omnigraph/tests/writes.rs | 316 ++++++++++- docs/dev/blob.md | 13 +- docs/dev/testing.md | 2 +- docs/user/blobs.md | 45 +- 20 files changed, 1943 insertions(+), 139 deletions(-) create mode 100644 changelog.d/blob-cell-put-clear.added.md create mode 100644 crates/omnigraph/src/exec/blob_write.rs diff --git a/changelog.d/blob-cell-put-clear.added.md b/changelog.d/blob-cell-put-clear.added.md new file mode 100644 index 000000000..c47eecd10 --- /dev/null +++ b/changelog.d/blob-cell-put-clear.added.md @@ -0,0 +1,12 @@ +- An embedded `Session` can replace or clear one Blob value of an existing + node or edge by exact id with `put_blob_at_as` and `clear_blob_at_as`. Each + write is one graph commit under the `change` action, never reads the old + value, and carries the row's other cells as an `update` does. An optional + precondition, `BlobPrecondition::Tags` (the current ETag is one of the given + tags) or `BlobPrecondition::AnyExisting` (the cell is not null), fails with + `BlobWritePreconditionFailed`, which names the current ETag, and changes + nothing. A put returns the value's length, its ETag and the commit; the ETag + is the one a read at that commit reports. One put holds at most 32 MiB, + inclusive; a larger one fails before any table is opened with resource + `Blob write payload bytes`. The CLI and HTTP server do not expose these + writes yet. diff --git a/crates/omnigraph-core/src/error.rs b/crates/omnigraph-core/src/error.rs index 0b84f1ecf..7a15a386c 100644 --- a/crates/omnigraph-core/src/error.rs +++ b/crates/omnigraph-core/src/error.rs @@ -312,6 +312,16 @@ pub enum OmniError { expected: String, actual: Option, }, + /// A Blob write's `If-Match` precondition did not hold at the write's + /// pinned base, so the write had no effect. Distinct from the graph-head + /// `PreconditionFailed`: it compares the cell's strong validator, and a + /// null or external cell has none. `current_etag` is the managed cell's + /// validator at that base, or `None` when the cell has none. + #[error( + "Blob write precondition failed; the cell's current validator is {}", + current_etag.as_deref().unwrap_or("") + )] + BlobWritePreconditionFailed { current_etag: Option }, /// Engine-layer policy enforcement (MR-722). Wraps either a policy /// denial ("you can't do that") or a policy-evaluation failure /// ("the policy engine itself blew up"). The HTTP layer maps @@ -863,6 +873,10 @@ impl OmniError { } } + pub fn blob_write_precondition_failed(current_etag: Option) -> Self { + Self::BlobWritePreconditionFailed { current_etag } + } + pub fn recovery_required(operation_id: impl Into, reason: impl Into) -> Self { Self::RecoveryRequired { operation_id: operation_id.into(), diff --git a/crates/omnigraph-server/src/blob_transport.rs b/crates/omnigraph-server/src/blob_transport.rs index 2a041342a..38fa4623f 100644 --- a/crates/omnigraph-server/src/blob_transport.rs +++ b/crates/omnigraph-server/src/blob_transport.rs @@ -80,6 +80,7 @@ impl RedactedCause { OmniError::BlobRangeNotSatisfiable { .. } => "BlobRangeNotSatisfiable", OmniError::RecoveryRequired { .. } => "RecoveryRequired", OmniError::PreconditionFailed { .. } => "PreconditionFailed", + OmniError::BlobWritePreconditionFailed { .. } => "BlobWritePreconditionFailed", OmniError::Policy(_) => "Policy", OmniError::AlreadyInitialized { .. } => "AlreadyInitialized", OmniError::InitializationCommitted { .. } => "InitializationCommitted", diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 76a88b83a..d9c52a7f8 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -1471,6 +1471,9 @@ impl ApiError { ), api::PreconditionFailureOutput { expected, actual }, ), + err @ OmniError::BlobWritePreconditionFailed { .. } => { + Self::blob_precondition_failed(err.to_string()) + } err @ OmniError::ExternalBlobPolicy { .. } => Self::bad_request(err.to_string()), err @ OmniError::StoredExternalBlobDenied { .. } => Self::bad_request(err.to_string()), OmniError::ExternalBlobSource { uri, reason } => { diff --git a/crates/omnigraph/src/blob.rs b/crates/omnigraph/src/blob.rs index 45b481884..2978bb9e7 100644 --- a/crates/omnigraph/src/blob.rs +++ b/crates/omnigraph/src/blob.rs @@ -59,6 +59,13 @@ pub struct BlobCell { pub struct BlobEtag(String); impl BlobEtag { + /// A caller-supplied strong entity tag, for a [`BlobPrecondition::Tags`] + /// comparison. It is compared exactly; a tag the engine never issued + /// matches nothing. + pub fn from_tag(tag: impl Into) -> Self { + Self(tag.into()) + } + pub fn as_str(&self) -> &str { &self.0 } @@ -74,6 +81,34 @@ impl fmt::Display for BlobEtag { } } +/// The condition a Blob write requires of the cell at its pinned write base. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum BlobPrecondition { + /// A non-null value exists, managed or external. A null cell fails it. + AnyExisting, + /// The managed value's validator equals one of these. A null or external + /// cell has no validator and fails it. + Tags(Vec), +} + +/// The result of a Blob write: the cell's new state and the exact commit of +/// the publication that made it visible. +#[derive(Debug, Clone)] +pub enum BlobWriteOutcome { + /// The cell now holds managed bytes. `etag` equals what a read at + /// `commit` returns. + Managed { + length: u64, + etag: BlobEtag, + commit: crate::db::GraphCommit, + }, + /// The cell is now null. `commit` is `None` when it already was, and the + /// clear published nothing. + Null { + commit: Option, + }, +} + /// Descriptor-only external Blob reference. Reading the cell never probes or /// dereferences this URI. #[derive(Debug, Clone, PartialEq, Eq)] @@ -1200,11 +1235,109 @@ impl<'a> BlobDescriptorDecoder<'a> { } } -struct ResolvedBlobCell { - table_key: String, +pub(crate) struct ResolvedBlobCell { + pub(crate) table_key: String, + pub(crate) stable_table_id: u64, + pub(crate) table_incarnation_id: u64, + pub(crate) stable_property_id: u64, +} + +/// Find one entity's row in an opened table version and classify its Blob +/// cell from the descriptor alone, without opening any payload: the stable +/// row id and the descriptor, or `None` when no row has the id. A duplicate id +/// or a malformed descriptor is a Blob integrity failure. +pub(crate) async fn locate_blob_cell( + dataset: &lance::Dataset, + id_column: &str, + cell: &BlobCell, +) -> Result> { + let mut scanner = dataset.scan(); + scanner + .project(&[cell.property.as_str()]) + .map_err(OmniError::storage)?; + scanner.filter_expr(col(id_column).eq(lit(cell.id.clone()))); + scanner.blob_handling(BlobHandling::BlobsDescriptions); + scanner.with_row_id(); + scanner.limit(Some(2), None).map_err(OmniError::storage)?; + let batches = scanner + .try_into_stream() + .await + .map_err(OmniError::storage)? + .try_collect::>() + .await + .map_err(OmniError::storage)?; + + let mut selected = None; + for batch in &batches { + let descriptions = batch + .column_by_name(&cell.property) + .and_then(|column| column.as_any().downcast_ref::()) + .ok_or_else(|| { + OmniError::blob_integrity(format!( + "Blob property '{}.{}' did not scan as a Blob-v2 descriptor", + cell.type_name, cell.property + )) + })?; + let row_ids = batch + .column_by_name("_rowid") + .and_then(|column| column.as_any().downcast_ref::()) + .ok_or_else(|| { + OmniError::blob_integrity(format!( + "Blob lookup for '{}.{}' omitted its stable row id", + cell.type_name, cell.property + )) + })?; + if descriptions.len() != row_ids.len() { + return Err(OmniError::blob_integrity(format!( + "Blob descriptor and stable-row-id cardinalities differ for '{}.{}'", + cell.type_name, cell.property + ))); + } + let decoder = BlobDescriptorDecoder::try_new(descriptions)?; + for row in 0..batch.num_rows() { + if selected.is_some() { + return Err(OmniError::blob_integrity(format!( + "entity id '{}' appears more than once in {} type '{}'", + cell.id, + entity_label(cell.entity), + cell.type_name + ))); + } + if row_ids.is_null(row) { + return Err(OmniError::blob_integrity(format!( + "Blob lookup for '{}.{}' returned a null stable row id", + cell.type_name, cell.property + ))); + } + selected = Some((row_ids.value(row), decoder.classify(row)?)); + } + } + Ok(selected) +} + +/// The strong validator of a managed cell in this exact opened table version: +/// table and property identity, the opened version, the row's stable id and +/// the transaction file named by the version's immutable manifest. A write +/// computes it from its own detached commit, a read from the version it opened, +/// and the two agree for the same version and row. +pub(crate) fn managed_blob_etag( + dataset: &lance::Dataset, stable_table_id: u64, table_incarnation_id: u64, stable_property_id: u64, + stable_row_id: u64, + cell: &BlobCell, +) -> Result { + let transaction_file = + immutable_transaction_witness(dataset.manifest().transaction_file.as_deref(), cell)?; + Ok(blob_etag( + stable_table_id, + table_incarnation_id, + stable_property_id, + dataset.version().version, + stable_row_id, + transaction_file, + )) } decide_seam! { @@ -1442,77 +1575,17 @@ impl Omnigraph { None => {} } - let mut scanner = dataset.scan(); - scanner - .project(&[cell.property.as_str()]) - .map_err(OmniError::storage)?; - scanner.filter_expr(col(catalog.system_columns.id).eq(lit(cell.id.clone()))); - scanner.blob_handling(BlobHandling::BlobsDescriptions); - scanner.with_row_id(); - scanner.limit(Some(2), None).map_err(OmniError::storage)?; - let stream = scanner - .try_into_stream() - .await - .map_err(OmniError::storage)?; - let batches = stream - .try_collect::>() - .await - .map_err(OmniError::storage)?; - - let mut selected = None; - for batch in &batches { - let descriptions = batch - .column_by_name(&cell.property) - .and_then(|column| column.as_any().downcast_ref::()) - .ok_or_else(|| { - OmniError::blob_integrity(format!( - "Blob property '{}.{}' did not scan as a Blob-v2 descriptor", - cell.type_name, cell.property - )) - })?; - let row_ids = batch - .column_by_name("_rowid") - .and_then(|column| column.as_any().downcast_ref::()) + let (stable_row_id, descriptor) = + locate_blob_cell(&dataset, catalog.system_columns.id, &cell) + .await? .ok_or_else(|| { - OmniError::blob_integrity(format!( - "Blob lookup for '{}.{}' omitted its stable row id", - cell.type_name, cell.property + OmniError::manifest_not_found(format!( + "no {} '{}' with id '{}' found", + entity_label(cell.entity), + cell.type_name, + cell.id )) })?; - if descriptions.len() != row_ids.len() { - return Err(OmniError::blob_integrity(format!( - "Blob descriptor and stable-row-id cardinalities differ for '{}.{}'", - cell.type_name, cell.property - ))); - } - let decoder = BlobDescriptorDecoder::try_new(descriptions)?; - for row in 0..batch.num_rows() { - if selected.is_some() { - return Err(OmniError::blob_integrity(format!( - "entity id '{}' appears more than once in {} type '{}'", - cell.id, - entity_label(cell.entity), - cell.type_name - ))); - } - if row_ids.is_null(row) { - return Err(OmniError::blob_integrity(format!( - "Blob lookup for '{}.{}' returned a null stable row id", - cell.type_name, cell.property - ))); - } - selected = Some((row_ids.value(row), decoder.classify(row)?)); - } - } - - let (stable_row_id, descriptor) = selected.ok_or_else(|| { - OmniError::manifest_not_found(format!( - "no {} '{}' with id '{}' found", - entity_label(cell.entity), - cell.type_name, - cell.id - )) - })?; let content = match descriptor { BlobDescriptor::Null => { @@ -1536,8 +1609,12 @@ impl Omnigraph { // after branch delete/recreate. The transaction-file identity // is stored in this exact immutable manifest and carries the // UUID of the transaction that produced this table version. - let transaction_file = immutable_transaction_witness( - dataset.manifest().transaction_file.as_deref(), + let etag = managed_blob_etag( + &dataset, + stable_table_id, + table_incarnation_id, + resolved_cell.stable_property_id, + stable_row_id, &cell, )?; let mut files = dataset @@ -1569,14 +1646,7 @@ impl Omnigraph { } BlobContent::Managed { length, - etag: blob_etag( - stable_table_id, - table_incarnation_id, - resolved_cell.stable_property_id, - actual_table_version, - stable_row_id, - transaction_file, - ), + etag, reader: BlobReader { _dataset: Arc::clone(&dataset), file: Arc::new(file), @@ -1594,7 +1664,7 @@ impl Omnigraph { } } -fn resolve_blob_cell( +pub(crate) fn resolve_blob_cell( catalog: &omnigraph_compiler::catalog::Catalog, cell: &BlobCell, ) -> Result { @@ -1661,7 +1731,7 @@ fn resolve_blob_cell( }) } -fn entity_label(kind: EntityKind) -> &'static str { +pub(crate) fn entity_label(kind: EntityKind) -> &'static str { match kind { EntityKind::Node => "node", EntityKind::Edge => "edge", diff --git a/crates/omnigraph/src/exec/blob_write.rs b/crates/omnigraph/src/exec/blob_write.rs new file mode 100644 index 000000000..2021b73b1 --- /dev/null +++ b/crates/omnigraph/src/exec/blob_write.rs @@ -0,0 +1,501 @@ +//! Blob cell writes (RFC 0033 Phase 3): replace one cell with managed bytes, +//! or clear it, on an existing node or edge addressed by exact id. +//! +//! A cell write is not a new writer kind. It stages the row's replacement +//! through Mutation's staging and validation, commits it detached and publishes +//! it once through the Mutation tail. Lance has no single-cell Blob write, so +//! the whole row is replaced and every other Blob cell of the row is carried by +//! value, under the rule a `.gq` update follows. + +use std::sync::Arc; + +use arrow_array::{ArrayRef, LargeBinaryArray, RecordBatch, StringArray, StructArray}; +use arrow_schema::{DataType, SchemaRef}; +use bytes::Bytes; +use datafusion::arrow::buffer::{Buffer, OffsetBuffer, ScalarBuffer}; +use datafusion::prelude::{col, lit}; + +use super::mutation::{ + build_null_blob_array, concat_match_batches_to_schema, open_table_for_mutation, +}; +use super::staging::{MutationStaging, PendingMode}; +use crate::blob::{ + BlobCell, BlobDescriptor, BlobEtag, BlobPrecondition, BlobWriteOutcome, entity_label, + locate_blob_cell, managed_blob_etag, resolve_blob_cell, +}; +use crate::changes::EntityKind; +use crate::db::Omnigraph; +use crate::db::manifest::HistoryReleaseBytes; +use crate::error::{OmniError, Result}; +use crate::session::Session; +use crate::storage_layer::{KEYED_BLOB_PAYLOAD_MAX_BYTES, PendingScanBudget}; + +/// What one Blob cell write stores. +enum BlobCellValue { + Bytes(Bytes), + Null, +} + +/// The authority a write attempt captured that a re-prepared attempt must +/// still see: the native branch incarnation and the accepted schema. +#[derive(PartialEq, Eq)] +struct AttemptIdentity { + branch_identifier: lance::dataset::refs::BranchIdentifier, + schema_ir_hash: String, + schema_identity_domain: String, + schema_identity_version: u32, +} + +impl AttemptIdentity { + fn of(txn: &crate::db::WriteTxn) -> Self { + Self { + branch_identifier: txn.authority.branch_identifier.clone(), + schema_ir_hash: txn.authority.schema_ir_hash.clone(), + schema_identity_domain: txn.authority.schema_identity_domain.clone(), + schema_identity_version: txn.authority.schema_identity_version, + } + } +} + +impl Session { + /// Replace one Blob cell of an existing node or edge with managed bytes. + /// + /// The entity must exist; the write never inserts a row. `bytes` is at + /// most 32 MiB, inclusive, and is refused before any table is opened when + /// it is larger. The row's other Blob cells are carried by value, so they + /// share the operation's 32 MiB payload allowance, and a stored external + /// reference among them must be admitted by the graph's external Blob + /// policy (`StoredExternalBlobDenied` otherwise). A failed `precondition` + /// returns `BlobWritePreconditionFailed` without effect. The outcome's + /// ETag equals what a read at its commit returns. + pub async fn put_blob_at_as( + &self, + branch: &str, + cell: BlobCell, + bytes: Bytes, + precondition: Option, + actor_id: Option<&str>, + ) -> Result { + let length = u64::try_from(bytes.len()) + .map_err(|_| OmniError::manifest_internal("Blob write payload length exceeds u64"))?; + if length > KEYED_BLOB_PAYLOAD_MAX_BYTES { + return Err(OmniError::resource_limit( + "Blob write payload bytes", + KEYED_BLOB_PAYLOAD_MAX_BYTES, + length, + )); + } + self.write_blob_cell_as( + branch, + cell, + BlobCellValue::Bytes(bytes), + precondition, + actor_id, + ) + .await + } + + /// Set one nullable Blob cell of an existing node or edge to null. + /// + /// Clearing a non-nullable property is refused. A cell that is already + /// null, with no `precondition`, publishes nothing and returns + /// `Null { commit: None }`; with a precondition it fails, since a null + /// cell satisfies neither form. + pub async fn clear_blob_at_as( + &self, + branch: &str, + cell: BlobCell, + precondition: Option, + actor_id: Option<&str>, + ) -> Result { + self.write_blob_cell_as(branch, cell, BlobCellValue::Null, precondition, actor_id) + .await + } + + async fn write_blob_cell_as( + &self, + branch: &str, + cell: BlobCell, + value: BlobCellValue, + precondition: Option, + actor_id: Option<&str>, + ) -> Result { + let requested = Omnigraph::normalize_branch_name(branch)?; + self.enforce( + omnigraph_policy::PolicyAction::Change, + &omnigraph_policy::ResourceScope::Branch( + requested.as_deref().unwrap_or("main").to_string(), + ), + actor_id, + )?; + let settings = self.effective("")?; + self.write_blob_cell( + requested.as_deref(), + &cell, + &value, + precondition.as_ref(), + actor_id, + settings.stage_write_concurrency(), + HistoryReleaseBytes(settings.history_release_bytes()), + ) + .await + } +} + +impl Omnigraph { + /// Re-prepare after a pre-effect read-set change, within the bound an + /// insert-only mutation uses. Unlike a predicate update, whose + /// read-modify-write plan would rebase, the stored value is the caller's, + /// so a fresh attempt is the same write. Each attempt re-reads the cell, + /// re-evaluates the precondition and re-carries the row's other cells. + #[allow(clippy::too_many_arguments)] + async fn write_blob_cell( + &self, + requested: Option<&str>, + cell: &BlobCell, + value: &BlobCellValue, + precondition: Option<&BlobPrecondition>, + actor_id: Option<&str>, + stage_write_concurrency: usize, + history_release_bytes: HistoryReleaseBytes, + ) -> Result { + const MAX_PRE_EFFECT_REPREPARES: usize = 32; + + let mut first = None; + for attempt in 0..=MAX_PRE_EFFECT_REPREPARES { + match self + .write_blob_cell_attempt( + requested, + cell, + value, + precondition, + actor_id, + stage_write_concurrency, + history_release_bytes, + &mut first, + ) + .await + { + Err(error) + if error.is_read_set_changed() && attempt < MAX_PRE_EFFECT_REPREPARES => + { + tracing::debug!( + attempt = attempt + 1, + branch = requested.unwrap_or("main"), + "prepared Blob write authority changed before effects; repreparing" + ); + crate::instrumentation::record_mutation_reprepare(); + self.refresh_coordinator_only().await?; + } + result => return result, + } + } + unreachable!("bounded Blob write retry loop always returns") + } + + #[allow(clippy::too_many_arguments)] + async fn write_blob_cell_attempt( + &self, + requested: Option<&str>, + cell: &BlobCell, + value: &BlobCellValue, + precondition: Option<&BlobPrecondition>, + actor_id: Option<&str>, + stage_write_concurrency: usize, + history_release_bytes: HistoryReleaseBytes, + first: &mut Option, + ) -> Result { + let first_attempt = first.is_none(); + let txn = self.open_write_txn(requested).await.map_err(|error| { + if first_attempt { + error.before_effect() + } else { + error.without_pre_effect_evidence() + } + })?; + let identity = AttemptIdentity::of(&txn); + match first { + None => *first = Some(identity), + Some(first) if *first != identity => { + return Err(OmniError::manifest_conflict(format!( + "branch '{}' changed incarnation or accepted schema while a Blob write \ + re-prepared; the write was not applied", + requested.unwrap_or("main") + ))); + } + Some(_) => {} + } + + let catalog = Arc::clone(&txn.catalog); + let resolved = resolve_blob_cell(&catalog, cell)?; + let schema: SchemaRef = match cell.entity { + EntityKind::Node => Arc::clone(&catalog.node_types[&cell.type_name].arrow_schema), + EntityKind::Edge => Arc::clone(&catalog.edge_types[&cell.type_name].arrow_schema), + }; + let target = schema.index_of(&cell.property).map_err(|_| { + OmniError::manifest_internal(format!( + "Blob property '{}.{}' is missing from its type's schema", + cell.type_name, cell.property + )) + })?; + if matches!(value, BlobCellValue::Null) && !schema.field(target).is_nullable() { + return Err(OmniError::manifest(format!( + "Blob property '{}.{}' is not nullable, so it cannot be cleared", + cell.type_name, cell.property + ))); + } + let entry = txn.base.dataset(&resolved.table_key).ok_or_else(|| { + OmniError::manifest(format!( + "{} type '{}' is unavailable on the write branch", + entity_label(cell.entity), + cell.type_name + )) + })?; + if entry.identity.stable_table_id != resolved.stable_table_id + || entry.identity.table_incarnation_id != resolved.table_incarnation_id + { + return Err(OmniError::blob_integrity(format!( + "write base for {} type '{}' has identity {}, expected {:016x}:{:016x}", + entity_label(cell.entity), + cell.type_name, + entry.identity, + resolved.stable_table_id, + resolved.table_incarnation_id + ))); + } + + let mut staging = MutationStaging::default(); + let (handle, _full_path, _table_branch) = open_table_for_mutation( + self, + &mut staging, + requested, + &resolved.table_key, + crate::db::MutationOpKind::Update, + Some(&txn), + ) + .await?; + let base = handle.ok_or_else(|| { + OmniError::manifest_internal("a strict Blob write opened no table handle") + })?; + let id_column = catalog.system_columns.id; + let (base_row_id, base_cell) = locate_blob_cell(base.dataset(), id_column, cell) + .await? + .ok_or_else(|| { + OmniError::manifest_not_found(format!( + "no {} '{}' with id '{}' found", + entity_label(cell.entity), + cell.type_name, + cell.id + )) + })?; + let current_etag = match base_cell { + BlobDescriptor::Managed { .. } => Some(managed_blob_etag( + base.dataset(), + resolved.stable_table_id, + resolved.table_incarnation_id, + resolved.stable_property_id, + base_row_id, + cell, + )?), + BlobDescriptor::Null | BlobDescriptor::External { .. } => None, + }; + if let Some(precondition) = precondition { + let holds = match precondition { + BlobPrecondition::AnyExisting => !matches!(base_cell, BlobDescriptor::Null), + BlobPrecondition::Tags(tags) => current_etag + .as_ref() + .is_some_and(|current| tags.contains(current)), + }; + if !holds { + return Err(OmniError::blob_write_precondition_failed( + current_etag.map(BlobEtag::into_string), + )); + } + } + if matches!(value, BlobCellValue::Null) && matches!(base_cell, BlobDescriptor::Null) { + return Ok(BlobWriteOutcome::Null { commit: None }); + } + + // The row's other cells, carried by value, with the target column left + // out so its old payload is never read. + let (pending_rows, pending_bytes) = staging.pending_resource_usage(&resolved.table_key)?; + let budget = PendingScanBudget::new(&resolved.table_key, pending_rows, pending_bytes); + let carried = self + .storage() + .scan_with_pending_materialized_blobs( + &base, + &[], + None, + Some(col(id_column).eq(lit(cell.id.clone()))), + Some(id_column), + &[cell.property.as_str()], + budget, + ) + .await?; + let carried_indices = (0..schema.fields().len()) + .filter(|index| *index != target) + .collect::>(); + let carried_schema = Arc::new( + schema + .project(&carried_indices) + .map_err(OmniError::arrow_internal)?, + ); + let carried = concat_match_batches_to_schema(&carried_schema, carried)?; + if carried.num_rows() != 1 { + return Err(OmniError::blob_integrity(format!( + "{} '{}' id '{}' matched {} rows while carrying its row", + entity_label(cell.entity), + cell.type_name, + cell.id, + carried.num_rows() + ))); + } + let target_column = match value { + BlobCellValue::Bytes(bytes) => { + managed_blob_column(schema.field(target).data_type(), bytes)? + } + BlobCellValue::Null => build_null_blob_array(1)?, + }; + let mut columns = carried.columns().to_vec(); + columns.insert(target, target_column); + let row = RecordBatch::try_new(Arc::clone(&schema), columns) + .map_err(OmniError::arrow_internal)?; + staging.append_batch( + &resolved.table_key, + Arc::clone(&schema), + PendingMode::Upsert, + row, + )?; + + let committed = self + .commit_staged_mutation( + staging, + &txn, + actor_id, + stage_write_concurrency, + history_release_bytes, + ) + .await?; + // The validator evidence comes from the exact detached version the pin + // will name, read before publication; nothing after the CAS reads + // storage, so a later write cannot change this outcome. + let managed = match value { + BlobCellValue::Bytes(bytes) => { + let detached = committed + .committed + .detached + .iter() + .find(|(table_key, _)| *table_key == resolved.table_key) + .map(|(_, handle)| handle) + .ok_or_else(|| { + OmniError::manifest_internal(format!( + "Blob write committed no detached version of '{}'", + resolved.table_key + )) + })?; + let (row_id, written) = locate_blob_cell(detached.dataset(), id_column, cell) + .await? + .ok_or_else(|| { + OmniError::blob_integrity(format!( + "the detached commit of '{}' lost id '{}'", + resolved.table_key, cell.id + )) + })?; + let length = u64::try_from(bytes.len()).map_err(|_| { + OmniError::manifest_internal("Blob write payload length exceeds u64") + })?; + if written != (BlobDescriptor::Managed { length }) { + return Err(OmniError::blob_integrity(format!( + "the detached commit of '{}.{}' stores {written:?}, expected a managed value of {length} bytes", + cell.type_name, cell.property + ))); + } + Some(( + length, + managed_blob_etag( + detached.dataset(), + resolved.stable_table_id, + resolved.table_incarnation_id, + resolved.stable_property_id, + row_id, + cell, + )?, + )) + } + BlobCellValue::Null => None, + }; + let commit = self + .publish_committed_mutation(committed, &txn, actor_id) + .await?; + Ok(match managed { + Some((length, etag)) => BlobWriteOutcome::Managed { + length, + etag, + commit, + }, + None => BlobWriteOutcome::Null { + commit: Some(commit), + }, + }) + } +} + +/// One logical Blob value holding `bytes` as managed data, in the column's +/// logical `{data, uri}` shape. Arrow adopts the bytes' buffer, so the payload +/// is not copied here. +fn managed_blob_column(data_type: &DataType, bytes: &Bytes) -> Result { + let DataType::Struct(children) = data_type else { + return Err(OmniError::manifest_internal( + "a Blob column's catalog type is not a struct", + )); + }; + if children.len() != 2 || children[0].name() != "data" || children[1].name() != "uri" { + return Err(OmniError::manifest_internal( + "a Blob column's catalog type is not the logical {data, uri} shape", + )); + } + let length = i64::try_from(bytes.len()) + .map_err(|_| OmniError::manifest_internal("Blob write payload length exceeds i64"))?; + let data = LargeBinaryArray::try_new( + OffsetBuffer::new(ScalarBuffer::from(vec![0_i64, length])), + Buffer::from(bytes.clone()), + None, + ) + .map_err(OmniError::arrow_internal)?; + let uri = StringArray::new_null(1); + let column = StructArray::try_new( + children.clone(), + vec![Arc::new(data) as ArrayRef, Arc::new(uri) as ArrayRef], + None, + ) + .map_err(OmniError::arrow_internal)?; + Ok(Arc::new(column)) +} + +#[cfg(test)] +mod tests { + use arrow_array::Array; + + use super::*; + + #[test] + fn managed_blob_column_adopts_the_payload_buffer() { + let field = lance::blob::blob_field("content", true); + let bytes = Bytes::from(vec![7_u8; 1024]); + let column = managed_blob_column(field.data_type(), &bytes).unwrap(); + let column = column.as_any().downcast_ref::().unwrap(); + let data = column + .column(0) + .as_any() + .downcast_ref::() + .unwrap(); + assert_eq!(data.value(0), &bytes[..]); + assert_eq!( + data.values().as_ptr(), + bytes.as_ptr(), + "the payload is adopted, not copied" + ); + assert!(column.column(1).is_null(0)); + assert!(column.is_valid(0)); + } +} diff --git a/crates/omnigraph/src/exec/mod.rs b/crates/omnigraph/src/exec/mod.rs index 546c6c305..7f2ccc48c 100644 --- a/crates/omnigraph/src/exec/mod.rs +++ b/crates/omnigraph/src/exec/mod.rs @@ -33,6 +33,7 @@ use crate::error::{MergeConflict, MergeConflictKind, OmniError, Result}; use crate::storage_layer::SnapshotHandle; use tempfile::{Builder as TempDirBuilder, TempDir}; +mod blob_write; pub(crate) mod merge; pub(crate) mod mutation; mod query_doors; diff --git a/crates/omnigraph/src/exec/mutation.rs b/crates/omnigraph/src/exec/mutation.rs index 0e0736371..4a8e7ee39 100644 --- a/crates/omnigraph/src/exec/mutation.rs +++ b/crates/omnigraph/src/exec/mutation.rs @@ -349,7 +349,7 @@ fn build_blob_array_from_value(value: &str) -> Result { } /// Build a null blob array with `num_rows` elements. -fn build_null_blob_array(num_rows: usize) -> Result { +pub(super) fn build_null_blob_array(num_rows: usize) -> Result { let mut builder = BlobArrayBuilder::new(num_rows); for _ in 0..num_rows { builder.push_null().map_err(OmniError::lance_internal)?; @@ -503,6 +503,13 @@ fn apply_assignments( use super::staging::{MutationStaging, PendingMode}; +/// Every table effect of a Mutation committed detached, with the gates and the +/// lineage intent its one publication needs. +pub(super) struct CommittedStagedMutation { + pub(super) committed: super::staging::CommittedMutation, + lineage_intent: crate::db::manifest::LineageIntent, +} + /// Open a sub-table dataset for read or staged write within the current /// mutation query, capturing pre-write metadata in `staging` on first touch. /// The captured table version is the physical staging baseline. The publisher's @@ -525,7 +532,7 @@ use super::staging::{MutationStaging, PendingMode}; /// touch records another predicate (`record_delete`), and `stage_all` combines /// them into one staged delete — there is no post-inline-commit reopen to /// special-case anymore. -async fn open_table_for_mutation( +pub(super) async fn open_table_for_mutation( db: &Omnigraph, staging: &mut MutationStaging, branch: Option<&str>, @@ -1007,49 +1014,18 @@ impl Omnigraph { }) } Ok(total) => { - self.validate_staged_mutation(&staging, &txn).await?; - let staged = staging - .stage_all_with_concurrency(self, requested.as_deref(), stage_write_concurrency) - .await?; - fail(&MUTATION_POST_STAGE_PRE_EFFECT_GATE)?; - let lineage_intent = self - .new_lineage_intent_for_branch( - requested.as_deref(), + let committed = self + .commit_staged_mutation( + staging, + &txn, actor_id, + stage_write_concurrency, history_release_bytes, ) .await?; - // `_held_gates` holds the shared schema permit, branch - // effect gate, and sorted table gates acquired by `commit_all`. - // They remain held through manifest publication, covering the - // complete same-process effect lifetime. They are a local - // serialization aid; the exact publisher precondition remains - // the correctness authority. - let super::staging::CommittedMutation { - updates, - expected_versions, - gates: _held_gates, - } = staged.commit_all(self, requested.as_deref(), &txn).await?; - // Failpoint for the detached-effects → publisher boundary: - // every table effect is committed detached but nothing is - // graph-visible. A failure here leaves the graph unchanged and - // the detached versions as reclaimable garbage. See - // `tests/failpoints.rs::finalize_publisher_residual_does_not_drift_untouched_tables`. - fail(&MUTATION_POST_FINALIZE_PRE_PUBLISHER)?; - let publish_result = self - .commit_updates_on_branch_with_expected( - requested.as_deref(), - &updates, - &expected_versions, - actor_id, - &txn, - lineage_intent, - ) - .await; - // RFC 0067: every effect is a detached commit of its pinned base, - // so a publish failure leaves the graph unchanged; the error - // is returned as is (a moved head is `ReadSetChanged`). - let commit = publish_result?; + let commit = self + .publish_committed_mutation(committed, &txn, actor_id) + .await?; Ok(crate::MutationReceipt { result: total, commit: Some(commit), @@ -1058,6 +1034,79 @@ impl Omnigraph { } } + /// The first half of the Mutation protocol's tail, shared by `.gq` + /// mutations and Blob cell writes: validate the staged change set, stage + /// every touched table, and commit each effect detached. Nothing is + /// graph-visible yet; the held gates and the lineage intent travel to + /// [`Self::publish_committed_mutation`]. + pub(super) async fn commit_staged_mutation( + &self, + staging: MutationStaging, + txn: &crate::db::WriteTxn, + actor_id: Option<&str>, + stage_write_concurrency: usize, + history_release_bytes: HistoryReleaseBytes, + ) -> Result { + let requested = txn.branch.as_deref(); + self.validate_staged_mutation(&staging, txn).await?; + let staged = staging + .stage_all_with_concurrency(self, requested, stage_write_concurrency) + .await?; + fail(&MUTATION_POST_STAGE_PRE_EFFECT_GATE)?; + let lineage_intent = self + .new_lineage_intent_for_branch(requested, actor_id, history_release_bytes) + .await?; + let committed = staged.commit_all(self, requested, txn).await?; + Ok(CommittedStagedMutation { + committed, + lineage_intent, + }) + } + + /// The second half of the Mutation protocol's tail: publish every detached + /// effect in one manifest CAS. This is the protocol's one publisher call. + pub(super) async fn publish_committed_mutation( + &self, + committed: CommittedStagedMutation, + txn: &crate::db::WriteTxn, + actor_id: Option<&str>, + ) -> Result { + let CommittedStagedMutation { + committed: + super::staging::CommittedMutation { + updates, + expected_versions, + detached: _, + gates: _held_gates, + }, + lineage_intent, + } = committed; + // `_held_gates` holds the shared schema permit, branch effect gate, + // and sorted table gates acquired by `commit_all`. They remain held + // through manifest publication, covering the complete same-process + // effect lifetime. They are a local serialization aid; the exact + // publisher precondition remains the correctness authority. + // + // Failpoint for the detached-effects → publisher boundary: every + // table effect is committed detached but nothing is graph-visible. A + // failure here leaves the graph unchanged and the detached versions as + // reclaimable garbage. See + // `tests/failpoints.rs::finalize_publisher_residual_does_not_drift_untouched_tables`. + fail(&MUTATION_POST_FINALIZE_PRE_PUBLISHER)?; + // RFC 0067: every effect is a detached commit of its pinned base, so a + // publish failure leaves the graph unchanged; the error is returned as + // is (a moved head is `ReadSetChanged`). + self.commit_updates_on_branch_with_expected( + txn.branch.as_deref(), + &updates, + &expected_versions, + actor_id, + txn, + lineage_intent, + ) + .await + } + /// Lower + validate a named mutation query into its IR. /// /// Hoisted out of [`Self::execute_named_mutation`] so the caller can @@ -1723,7 +1772,7 @@ async fn scan_deleted_ids( /// order; both should share a schema if pending was produced through /// `apply_assignments` with full-schema scan input. If schemas drift, /// surface the internal contract failure at the mutation boundary. -fn concat_match_batches_to_schema( +pub(super) fn concat_match_batches_to_schema( schema: &SchemaRef, batches: Vec, ) -> Result { diff --git a/crates/omnigraph/src/exec/staging.rs b/crates/omnigraph/src/exec/staging.rs index cd1d960e8..8b3663185 100644 --- a/crates/omnigraph/src/exec/staging.rs +++ b/crates/omnigraph/src/exec/staging.rs @@ -823,6 +823,10 @@ pub(crate) struct CommittedMutation { /// publisher checks them together with native branch identity, exact graph /// head, and schema identity as one authority precondition. pub(crate) expected_versions: crate::db::manifest::ExpectedTableVersions, + /// Each table's detached commit, keyed by table: the exact version its + /// pin will name, before publication. A Blob write reads its validator + /// evidence from here, never from a branch head. + pub(crate) detached: Vec<(String, crate::storage_layer::SnapshotHandle)>, /// The write envelope: shared schema permit, coarse branch gate, and /// sorted `(table, branch)` guards. The caller MUST hold the complete /// set across manifest publish (see `commit_all`) so no same-process @@ -931,6 +935,7 @@ impl StagedMutation { return Ok(CommittedMutation { updates: Vec::new(), expected_versions, + detached: Vec::new(), gates, }); } @@ -951,6 +956,7 @@ impl StagedMutation { let witness = txn.authority.staging_witness()?; let mut updates: Vec = Vec::with_capacity(staged.len()); + let mut detached_tables = Vec::with_capacity(staged.len()); for entry in staged { let StagedTableEntry { table_key, @@ -976,6 +982,7 @@ impl StagedMutation { .with_table_fork_owner(table_fork_owner) .with_staged(state.version, identity.uuid.clone()) .with_last_linear_version(path.entry.version_metadata.last_linear_version()); + detached_tables.push((table_key.clone(), detached)); updates.push(DatasetUpdate { identity: path.identity, type_key: table_key, @@ -990,6 +997,7 @@ impl StagedMutation { Ok(CommittedMutation { updates, expected_versions, + detached: detached_tables, gates, }) } diff --git a/crates/omnigraph/src/lib.rs b/crates/omnigraph/src/lib.rs index e8ec4f033..9ed1bb5d9 100644 --- a/crates/omnigraph/src/lib.rs +++ b/crates/omnigraph/src/lib.rs @@ -47,9 +47,10 @@ pub(crate) mod table_store; pub(crate) mod validate; pub use blob::{ - BLOB_READ_RANGE_MAX_BYTES, BlobCell, BlobContent, BlobEtag, BlobRead, BlobReader, - EXTERNAL_BLOB_URI_MAX_BYTES, ExternalBlobBase, ExternalBlobExecutionScope, ExternalBlobPolicy, - ExternalBlobRef, RangedExternalBlob, StorageRootConflict, + BLOB_READ_RANGE_MAX_BYTES, BlobCell, BlobContent, BlobEtag, BlobPrecondition, BlobRead, + BlobReader, BlobWriteOutcome, EXTERNAL_BLOB_URI_MAX_BYTES, ExternalBlobBase, + ExternalBlobExecutionScope, ExternalBlobPolicy, ExternalBlobRef, RangedExternalBlob, + StorageRootConflict, }; pub use changes::EntityKind; pub use omnigraph_compiler::settings; diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index 1a6f8734f..5d93b3eec 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -949,6 +949,7 @@ async fn load_jsonl_reader_once( let crate::exec::staging::CommittedMutation { updates, expected_versions, + detached: _, gates: _held_gates, } = staged.commit_all(db, branch, &txn).await?; // Same detached-effects → publisher boundary as mutations: every table diff --git a/crates/omnigraph/tests/detached_commit_matrix.rs b/crates/omnigraph/tests/detached_commit_matrix.rs index c0dbebaf8..03becaf6d 100644 --- a/crates/omnigraph/tests/detached_commit_matrix.rs +++ b/crates/omnigraph/tests/detached_commit_matrix.rs @@ -25,15 +25,18 @@ mod helpers; use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; +use bytes::Bytes; use helpers::recovery::sidecar_operation_ids; use helpers::{ - MUTATION_QUERIES, Session, collect_column_strings, count_rows, init_and_load, mixed_params, - mutate_main, read_table, + MUTATION_QUERIES, Session, collect_column_strings, count_rows, init_and_load, + init_and_load_with_schema, mixed_params, mutate_main, read_table, }; +use omnigraph::BlobCell; use omnigraph::db::{ CleanupPolicyOptions, Omnigraph, PreparedSchemaApply, ReadTarget, SystemColumnUpgradeOptions, SystemColumnUpgradeOutcome, }; +use omnigraph::error::{ManifestErrorKind, OmniError}; use omnigraph::loader::LoadMode; use omnigraph::seams::FailScenario; use omnigraph::seams::catalog; @@ -51,7 +54,8 @@ const PARK_HIT_ENV: &str = "OMNIGRAPH_RFC0067_PARK_HIT"; const PREPARED_SCHEMA_FILE: &str = "matrix-prepared-schema.json"; /// What the child runs: one of the `Writer::child_op` strings (`insert`, /// `insert_and_friend`, `cleanup`, `ensure_indices`, `merge`, `schema_apply`, -/// `prepared_schema_apply`, `optimize`, `load`, `fts_rebuild`, `system_column_upgrade`). +/// `prepared_schema_apply`, `optimize`, `load`, `fts_rebuild`, `system_column_upgrade`, +/// `blob_put`, `blob_clear`). const OP_ENV: &str = "OMNIGRAPH_RFC0067_OP"; #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -86,6 +90,26 @@ enum Writer { /// spellings: one rename-only detached Project per table, staged and /// published through the schema-apply seams. SystemColumnUpgrade, + /// A Blob put replacing Alice's `photo` by exact id: one detached upsert + /// of the carried row through the mutation seams. + BlobPut, + /// A Blob clear of Alice's seeded `photo`: the same single-row upsert + /// carrying a null value. + BlobClear, +} + +fn photo_schema() -> String { + helpers::TEST_SCHEMA.replace("age: I32?", "age: I32?\n photo: Blob?") +} + +/// The Blob cell the Blob writers replace. +fn alice_photo() -> BlobCell { + helpers::node_blob_cell("Person", "Alice", "photo") +} + +/// The bytes a Blob put by `name` stores. +fn photo_bytes(name: &str) -> Bytes { + Bytes::from(format!("{name}-photo").into_bytes()) } fn city_schema() -> String { @@ -136,6 +160,7 @@ impl Writer { // The upgrade stages one detached commit per table; park after // the first and the second to leave a half-staged tail. Writer::SystemColumnUpgrade => vec![PostDetached(1), PostDetached(2), PrePublish], + Writer::BlobPut | Writer::BlobClear => vec![PostDetached(1), PrePublish], } } @@ -152,6 +177,8 @@ impl Writer { Writer::Load => "load", Writer::FtsRebuild => "fts_rebuild", Writer::SystemColumnUpgrade => "system_column_upgrade", + Writer::BlobPut => "blob_put", + Writer::BlobClear => "blob_clear", } } } @@ -280,6 +307,34 @@ async fn insert_and_friend(db: &Session, name: &str) -> omnigraph::error::Result .map(|_| ()) } +/// The BlobPut writer's op: replace Alice's photo with `name`'s bytes. +async fn put_photo(db: &Session, name: &str) -> omnigraph::error::Result<()> { + db.put_blob_at_as("main", alice_photo(), photo_bytes(name), None, None) + .await + .map(|_| ()) +} + +/// The BlobClear writer's op. +async fn clear_photo(db: &Session) -> omnigraph::error::Result<()> { + db.clear_blob_at_as("main", alice_photo(), None, None) + .await + .map(|_| ()) +} + +/// Alice's photo, `None` when the cell is null. +async fn observe_photo(db: &Omnigraph) -> Option> { + match db + .read_blob_at(ReadTarget::branch("main"), alice_photo()) + .await + { + Ok(_) => Some( + helpers::read_managed_blob_bytes(db, ReadTarget::branch("main"), alice_photo()).await, + ), + Err(OmniError::Manifest(error)) if error.kind == ManifestErrorKind::NotFound => None, + Err(error) => panic!("reading Alice's photo failed: {error}"), + } +} + /// The Load writer's op: two fresh rows through the loader's staging. async fn load_two(db: &Session, name: &str) -> omnigraph::error::Result<()> { let payload = format!( @@ -377,6 +432,8 @@ fn rfc0067_matrix_child_process() { .upgrade_system_columns(upgrade_execute()) .await .map(|_| ()), + "blob_put" => put_photo(&db, &name).await, + "blob_clear" => clear_photo(&db).await, _ => insert(&db, &name).await, }; if let Err(error) = outcome { @@ -574,6 +631,8 @@ async fn run_cell( .await .unwrap(); db + } else if matches!(writer, Writer::BlobPut | Writer::BlobClear) { + init_and_load_with_schema(&dir, &photo_schema()).await } else { init_and_load(&dir).await }; @@ -627,6 +686,11 @@ async fn run_cell( .unwrap(); db.ensure_indices().await.unwrap(); } + // A clear cell clears a photo a put stored first. + if writer == Writer::BlobClear { + put_photo(&db, &format!("m{index}_seed")).await.unwrap(); + } + let photo_before = observe_photo(&db).await; let (mut model, _) = observe_model(&db).await; let head_before = linear_head(&person_uri).await; let knows_head_before = linear_head(&knows_uri).await; @@ -673,6 +737,8 @@ async fn run_cell( .upgrade_system_columns(upgrade_execute()) .await .map(|_| ()), + Writer::BlobPut => put_photo(&db, &write_name).await, + Writer::BlobClear => clear_photo(&db).await, }; acknowledged = outcome.is_ok(); if let Err(error) = outcome { @@ -832,6 +898,21 @@ async fn run_cell( same_handle, model, "{cell}: the writer's handle disagrees with a fresh one" ); + if matches!(writer, Writer::BlobPut | Writer::BlobClear) { + // The cell holds the written value exactly when the write was + // acknowledged, on a fresh handle and on the writer's own. + let expected = match writer { + Writer::BlobPut if visible => Some(photo_bytes(&write_name).to_vec()), + Writer::BlobClear if visible => None, + _ => photo_before.clone(), + }; + assert_eq!(observe_photo(&fresh).await, expected, "{cell}: photo"); + assert_eq!( + observe_photo(&db).await, + expected, + "{cell}: the writer's handle reads a different photo" + ); + } let person_pin = table_pin(&fresh, "node:Person").await; let knows_pin = table_pin(&fresh, "edge:Knows").await; let person_head = linear_head(&person_uri).await; @@ -997,6 +1078,8 @@ async fn run_matrix() { Writer::Load, Writer::FtsRebuild, Writer::SystemColumnUpgrade, + Writer::BlobPut, + Writer::BlobClear, ]; let faults = [Fault::Return, Fault::Kill, Fault::Race]; let only: Option> = std::env::var("OMNIGRAPH_MATRIX_WRITERS").ok().map(|list| { diff --git a/crates/omnigraph/tests/end_to_end.rs b/crates/omnigraph/tests/end_to_end.rs index f53841753..2b254763a 100644 --- a/crates/omnigraph/tests/end_to_end.rs +++ b/crates/omnigraph/tests/end_to_end.rs @@ -4,13 +4,15 @@ use arrow_array::{Array, Int32Array, RecordBatch, StringArray}; use base64::Engine as _; use futures::TryStreamExt; +use bytes::Bytes; +use omnigraph::db::SnapshotId; use omnigraph::db::{Omnigraph, ReadTarget}; use omnigraph::error::{ManifestErrorKind, OmniError}; use omnigraph::instrumentation::{MergeWriteProbes, with_merge_write_probes}; use omnigraph::loader::LoadMode; use omnigraph::{ - BLOB_READ_RANGE_MAX_BYTES, BlobCell, BlobContent, EntityKind, ExternalBlobBase, - ExternalBlobExecutionScope, ExternalBlobPolicy, + BLOB_READ_RANGE_MAX_BYTES, BlobCell, BlobContent, BlobEtag, BlobPrecondition, BlobWriteOutcome, + EntityKind, ExternalBlobBase, ExternalBlobExecutionScope, ExternalBlobPolicy, }; use omnigraph_compiler::ir::ParamMap; @@ -2082,6 +2084,201 @@ async fn blob_update_null_round_trip() { ); } +/// Replacing and clearing one Blob cell by exact id, on a node and on an edge: +/// the write never inserts a row, its receipt is the publication's exact +/// commit, its ETag equals a read at that commit, a stale or `*` precondition +/// is judged at the write base, and clearing an already-null cell publishes +/// nothing. Rust, not GQT: a `.gq` statement cannot carry raw bytes or an +/// `If-Match`. +#[tokio::test] +async fn blob_put_and_clear_replace_one_cell_by_exact_id() { + let dir = tempfile::tempdir().unwrap(); + let db = helpers::session( + Omnigraph::init(dir.path().to_str().unwrap(), BLOB_SCHEMA) + .await + .unwrap(), + ); + let data = r#"{"type":"Document","data":{"title":"readme","content":"base64:SGVsbG8="}} +{"type":"Document","data":{"title":"peer"}} +{"edge":"Attachment","id":"attachment-1","from":"readme","to":"peer","data":{"payload":"base64:RWRnZQ=="}}"#; + db.load_jsonl(data, LoadMode::Overwrite).await.unwrap(); + + let managed_etag = |read: omnigraph::BlobRead| match read.content { + BlobContent::Managed { etag, .. } => etag, + BlobContent::External(external) => panic!("expected managed, got {external:?}"), + }; + for cell in [ + node_blob_cell("Document", "readme", "content"), + edge_blob_cell("Attachment", "attachment-1", "payload"), + ] { + let before = managed_etag( + db.read_blob_at(ReadTarget::branch("main"), cell.clone()) + .await + .unwrap(), + ); + let head = snapshot_main(&db).await.unwrap().graph_manifest_version(); + + let stale = db + .put_blob_at_as( + "main", + cell.clone(), + Bytes::from_static(b"never"), + Some(BlobPrecondition::Tags(vec![BlobEtag::from_tag( + "\"stale\"", + )])), + None, + ) + .await + .unwrap_err(); + assert!( + matches!(&stale, OmniError::BlobWritePreconditionFailed { current_etag: Some(current) } + if current == before.as_str()), + "{stale:?}" + ); + assert_eq!( + snapshot_main(&db).await.unwrap().graph_manifest_version(), + head, + "a failed precondition has no effect" + ); + + let BlobWriteOutcome::Managed { + length, + etag, + commit, + } = db + .put_blob_at_as( + "main", + cell.clone(), + Bytes::from_static(b"Replaced"), + Some(BlobPrecondition::Tags(vec![before.clone()])), + None, + ) + .await + .unwrap() + else { + panic!("a put stores managed bytes"); + }; + assert_eq!(length, 8); + assert_ne!(etag, before); + let at_commit = db + .read_blob_at( + ReadTarget::snapshot(SnapshotId::new(commit.graph_commit_id.clone())), + cell.clone(), + ) + .await + .unwrap(); + assert_eq!( + managed_etag(at_commit), + etag, + "the ETag equals a read at the commit" + ); + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), cell.clone()).await, + b"Replaced" + ); + + let BlobWriteOutcome::Managed { etag: again, .. } = db + .put_blob_at_as( + "main", + cell.clone(), + Bytes::from_static(b""), + Some(BlobPrecondition::AnyExisting), + None, + ) + .await + .unwrap() + else { + panic!("a put stores managed bytes"); + }; + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), cell.clone()).await, + b"", + "a valid empty value is managed, not null" + ); + + let BlobWriteOutcome::Null { commit: Some(_) } = db + .clear_blob_at_as( + "main", + cell.clone(), + Some(BlobPrecondition::Tags(vec![again])), + None, + ) + .await + .unwrap() + else { + panic!("clearing a value publishes a commit"); + }; + let cleared = db + .read_blob_at(ReadTarget::branch("main"), cell.clone()) + .await + .unwrap_err(); + assert!( + matches!(&cleared, OmniError::Manifest(error) if error.kind == ManifestErrorKind::NotFound), + "{cleared:?}" + ); + + let head = snapshot_main(&db).await.unwrap().graph_manifest_version(); + assert!(matches!( + db.clear_blob_at_as("main", cell.clone(), None, None) + .await + .unwrap(), + BlobWriteOutcome::Null { commit: None } + )); + for refused in [ + db.clear_blob_at_as( + "main", + cell.clone(), + Some(BlobPrecondition::AnyExisting), + None, + ) + .await + .unwrap_err(), + db.put_blob_at_as( + "main", + cell.clone(), + Bytes::from_static(b"x"), + Some(BlobPrecondition::AnyExisting), + None, + ) + .await + .unwrap_err(), + ] { + assert!( + matches!( + refused, + OmniError::BlobWritePreconditionFailed { current_etag: None } + ), + "a null cell satisfies neither precondition form" + ); + } + assert_eq!( + snapshot_main(&db).await.unwrap().graph_manifest_version(), + head, + "an already-null clear and failed preconditions publish nothing" + ); + } + + let missing = db + .put_blob_at_as( + "main", + node_blob_cell("Document", "absent", "content"), + Bytes::from_static(b"x"), + None, + None, + ) + .await + .unwrap_err(); + assert!( + matches!(&missing, OmniError::Manifest(error) if error.kind == ManifestErrorKind::NotFound), + "{missing:?}" + ); + assert_eq!( + count_rows(&db, "node:Document").await, + 2, + "a put never inserts a row" + ); +} + /// The control, the same update with a value, opens the table, so the zero /// open count belongs to the refusal and not to a skipped scan. #[tokio::test] diff --git a/crates/omnigraph/tests/failpoints.rs b/crates/omnigraph/tests/failpoints.rs index 55f8dae97..57d932fcc 100644 --- a/crates/omnigraph/tests/failpoints.rs +++ b/crates/omnigraph/tests/failpoints.rs @@ -8,15 +8,18 @@ use std::sync::Arc; use arrow_array::{Int32Array, RecordBatch, StringArray}; use arrow_schema::Schema; +use bytes::Bytes; use lance::Dataset; use omnigraph::db::{ - GraphCreateReconciliation, Omnigraph, PreparedGraphCreate, ReadTarget, StagingVerdict, + GraphCreateReconciliation, Omnigraph, PreparedGraphCreate, ReadTarget, SnapshotId, + StagingVerdict, }; use omnigraph::error::{CompletionEvidence, ManifestErrorKind, OmniError}; use omnigraph::instrumentation::{MergeWriteProbes, with_merge_write_probes}; use omnigraph::loader::LoadMode; use omnigraph::seams::FailScenario; use omnigraph::seams::catalog; +use omnigraph::{BlobCell, BlobContent, BlobEtag, BlobPrecondition, BlobWriteOutcome}; use serial_test::serial; use helpers::collector::{ @@ -46,6 +49,64 @@ const RFC023_EXTERNAL_MODE_ENV: &str = "OMNIGRAPH_RFC023_EXTERNAL_MODE"; const RFC023_EXTERNAL_PAYLOAD_ENV: &str = "OMNIGRAPH_RFC023_EXTERNAL_PAYLOAD"; const RFC023_EXTERNAL_ACTION_ENV: &str = "OMNIGRAPH_RFC023_EXTERNAL_ACTION"; +const BLOB_WRITE_SCHEMA: &str = r#" +node Document { + title: String @key + content: Blob? + preview: Blob? + note: String? +} +"#; + +/// `readme` carries content `Hello`, preview `P0` and note `n0`. +const BLOB_WRITE_SEED: &str = r#"{"type":"Document","data":{"title":"readme","content":"base64:SGVsbG8=","preview":"base64:UDA=","note":"n0"}}"#; + +const BLOB_WRITE_MUTATIONS: &str = r#" +query set_note($title: String, $note: String) { + update Document set { note: $note } where title = $title +} +"#; + +async fn seed_blob_write_graph(uri: &str) -> Session { + let db = helpers::session(Omnigraph::init(uri, BLOB_WRITE_SCHEMA).await.unwrap()); + db.load_jsonl(BLOB_WRITE_SEED, LoadMode::Overwrite) + .await + .unwrap(); + db +} + +fn readme(property: &str) -> BlobCell { + node_blob_cell("Document", "readme", property) +} + +/// The current validator of a managed cell on `branch`. +async fn blob_etag(db: &Omnigraph, branch: &str, cell: BlobCell) -> BlobEtag { + match db + .read_blob_at(ReadTarget::branch(branch), cell) + .await + .unwrap() + .content + { + BlobContent::Managed { etag, .. } => etag, + BlobContent::External(external) => panic!("expected a managed value, got {external:?}"), + } +} + +async fn set_readme_note(db: &Session, note: &str) { + db.mutate( + "main", + BLOB_WRITE_MUTATIONS, + "set_note", + ¶ms(&[("$title", "readme"), ("$note", note)]), + ) + .await + .unwrap(); +} + +async fn main_commit_count(db: &Omnigraph) -> usize { + db.list_commits(Some("main")).await.unwrap().len() +} + const OCC_UNIQUE_SCHEMA: &str = r#" node User { name: String @key @@ -1290,6 +1351,251 @@ async fn mutation_revalidates_unique_after_pre_effect_authority_change() { assert_eq!(collect_column_strings(&users, "name"), vec!["winner"]); } +/// A Blob write re-prepared after a competing write evaluates its +/// precondition against the fresh base, and an unconditional write carries +/// the row's other cells from that fresh base, never from the stale one. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial] +async fn blob_put_reevaluates_its_precondition_and_recarries_the_row_after_a_competing_write() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let db = Arc::new(seed_blob_write_graph(dir.path().to_str().unwrap()).await); + + // The competing write changes only the note, yet it moves the content + // cell's validator: the parked put's If-Match no longer holds. + let stale = blob_etag(&db, "main", readme("content")).await; + let rendezvous = + helpers::failpoint::Rendezvous::park_first(&catalog::MUTATION_POST_STAGE_PRE_EFFECT_GATE); + let writer = { + let db = Arc::clone(&db); + let stale = stale.clone(); + tokio::spawn(async move { + db.put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"stale"), + Some(BlobPrecondition::Tags(vec![stale])), + None, + ) + .await + }) + }; + rendezvous.wait_until_reached().await; + set_readme_note(&db, "n1").await; + let competing_commits = main_commit_count(&db).await; + rendezvous.release(); + let error = writer + .await + .unwrap() + .expect_err("the stale If-Match fails against the fresh base"); + let fresh = blob_etag(&db, "main", readme("content")).await; + assert_ne!(fresh, stale, "the competing write moved the validator"); + assert!( + matches!(&error, OmniError::BlobWritePreconditionFailed { current_etag: Some(current) } + if current == fresh.as_str()), + "the refusal names the fresh validator: {error:?}" + ); + assert_eq!( + main_commit_count(&db).await, + competing_commits, + "the refused put published nothing" + ); + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), readme("content")).await, + b"Hello" + ); + drop(rendezvous); + + // An unconditional put parked while another put replaces the preview and + // an update replaces the note: its retry carries both. + let rendezvous = + helpers::failpoint::Rendezvous::park_first(&catalog::MUTATION_POST_STAGE_PRE_EFFECT_GATE); + let writer = { + let db = Arc::clone(&db); + tokio::spawn(async move { + db.put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"fresh"), + None, + None, + ) + .await + }) + }; + rendezvous.wait_until_reached().await; + db.put_blob_at_as( + "main", + readme("preview"), + Bytes::from_static(b"P1"), + None, + None, + ) + .await + .unwrap(); + set_readme_note(&db, "n2").await; + rendezvous.release(); + let BlobWriteOutcome::Managed { etag, .. } = writer.await.unwrap().unwrap() else { + panic!("a put stores a managed value"); + }; + assert_eq!(etag, blob_etag(&db, "main", readme("content")).await); + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), readme("content")).await, + b"fresh" + ); + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), readme("preview")).await, + b"P1", + "the retry carried the competing preview, not the stale one" + ); + let documents = read_table(&db, "node:Document").await; + assert_eq!(collect_column_strings(&documents, "note"), ["n2"]); +} + +/// A Blob write that re-prepares across an accepted-schema change or a +/// delete-and-recreate of its branch fails closed, without effect. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial] +async fn blob_put_fails_closed_when_schema_or_branch_incarnation_changes_between_attempts() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let db = Arc::new(seed_blob_write_graph(dir.path().to_str().unwrap()).await); + let refused = |error: &OmniError| { + matches!(error, OmniError::Manifest(manifest) + if manifest.kind == ManifestErrorKind::Conflict + && manifest.message.contains("changed incarnation or accepted schema")) + }; + let parked_put = |branch: &'static str| { + let db = Arc::clone(&db); + tokio::spawn(async move { + db.put_blob_at_as( + branch, + readme("content"), + Bytes::from_static(b"lost"), + None, + None, + ) + .await + }) + }; + + let rendezvous = + helpers::failpoint::Rendezvous::park_first(&catalog::MUTATION_POST_STAGE_PRE_EFFECT_GATE); + let writer = parked_put("main"); + rendezvous.wait_until_reached().await; + db.apply_schema(&BLOB_WRITE_SCHEMA.replace("note: String?", "note: String?\n tag: String?")) + .await + .unwrap(); + let schema_commits = main_commit_count(&db).await; + rendezvous.release(); + let error = writer + .await + .unwrap() + .expect_err("a schema change refuses the retry"); + assert!(refused(&error), "{error:?}"); + assert_eq!(main_commit_count(&db).await, schema_commits); + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), readme("content")).await, + b"Hello" + ); + drop(rendezvous); + + db.branch_create("feature").await.unwrap(); + let rendezvous = + helpers::failpoint::Rendezvous::park_first(&catalog::MUTATION_POST_STAGE_PRE_EFFECT_GATE); + let writer = parked_put("feature"); + rendezvous.wait_until_reached().await; + db.branch_delete("feature").await.unwrap(); + db.branch_create("feature").await.unwrap(); + rendezvous.release(); + let error = writer + .await + .unwrap() + .expect_err("a recreated branch refuses the retry"); + assert!(refused(&error), "{error:?}"); + for branch in ["feature", "main"] { + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch(branch), readme("content")).await, + b"Hello" + ); + } +} + +/// The receipt of a Blob put is evidence from its own detached commit: a +/// write by another process that lands between the put's manifest commit and +/// its return does not change the ETag the put reports. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial] +async fn a_write_after_publication_cannot_change_a_blob_put_receipt() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap().to_string(); + let db = Arc::new(seed_blob_write_graph(&uri).await); + + let rendezvous = + helpers::failpoint::Rendezvous::park_first(&catalog::GRAPH_PUBLISH_AFTER_MANIFEST_COMMIT); + let writer = { + let db = Arc::clone(&db); + tokio::spawn(async move { + db.put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"mine"), + None, + None, + ) + .await + }) + }; + rendezvous.wait_until_reached().await; + // "Zm9yZWlnbg==" is "foreign". + let foreign = r#"{"type":"Document","data":{"title":"readme","content":"base64:Zm9yZWlnbg==","preview":"base64:UDA=","note":"n0"}}"#; + let external_uri = uri.clone(); + tokio::task::spawn_blocking(move || { + run_rfc023_external_writer(external_uri, LoadMode::Merge, foreign.to_string()) + }) + .await + .unwrap() + .expect("the foreign write publishes after the parked put's manifest commit"); + rendezvous.release(); + let BlobWriteOutcome::Managed { + length, + etag, + commit, + } = writer.await.unwrap().unwrap() + else { + panic!("a put stores a managed value"); + }; + assert_eq!(length, 4); + + let fresh = helpers::session(Omnigraph::open(&uri).await.unwrap()); + let at_commit = ReadTarget::snapshot(SnapshotId::new(commit.graph_commit_id.clone())); + let BlobContent::Managed { + etag: committed, .. + } = fresh + .read_blob_at(at_commit.clone(), readme("content")) + .await + .unwrap() + .content + else { + panic!("the put's commit holds a managed value"); + }; + assert_eq!(etag, committed, "the receipt names the put's own commit"); + assert_eq!( + read_managed_blob_bytes(&fresh, at_commit, readme("content")).await, + b"mine" + ); + assert_ne!( + etag, + blob_etag(&fresh, "main", readme("content")).await, + "the head now holds the foreign write" + ); + assert_eq!( + read_managed_blob_bytes(&fresh, ReadTarget::branch("main"), readme("content")).await, + b"foreign" + ); +} + /// The coarse token is branch-wide: a commit to a table that the prepared /// mutation does not write can still invalidate schema/cardinality/RI inputs. /// A strict update therefore reports `ReadSetChanged` before moving its Person @@ -6420,6 +6726,154 @@ async fn assert_optimize_detached_pin_survives_next_write(uri: &str) { assert_person_pin_detached(&db, "main", head_before).await; } +/// A Blob put and a Blob clear whose publication acknowledgement is lost are +/// published exactly once: the caller's retry with its old validator is +/// refused, the cell reads the same from the writer's handle and a fresh +/// one, and the same handle writes again once the fault stops. +#[tokio::test] +#[serial] +async fn blob_put_and_clear_publish_once_through_a_lost_acknowledgement() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap().to_string(); + let db = seed_blob_write_graph(&uri).await; + let original = blob_etag(&db, "main", readme("content")).await; + let before = main_commit_count(&db).await; + + { + let _failpoint = catalog::GRAPH_PUBLISH_AFTER_MANIFEST_COMMIT.fire_once_at(1); + let error = db + .put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"Published"), + Some(BlobPrecondition::Tags(vec![original.clone()])), + None, + ) + .await + .expect_err("the acknowledgement is lost after publication"); + assert!( + error + .to_string() + .contains("graph_publish.after_manifest_commit"), + "the publication boundary must be reached: {error}" + ); + } + assert_eq!( + main_commit_count(&db).await, + before + 1, + "published exactly once" + ); + let published = blob_etag(&db, "main", readme("content")).await; + let fresh = helpers::session(Omnigraph::open(&uri).await.unwrap()); + assert_eq!( + blob_etag(&fresh, "main", readme("content")).await, + published + ); + for handle in [&db, &fresh] { + assert_eq!( + read_managed_blob_bytes(handle, ReadTarget::branch("main"), readme("content")).await, + b"Published" + ); + } + let retry = db + .put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"Published"), + Some(BlobPrecondition::Tags(vec![original])), + None, + ) + .await + .expect_err("a retry with the old validator finds the published value"); + assert!( + matches!(&retry, OmniError::BlobWritePreconditionFailed { current_etag: Some(current) } + if current == published.as_str()), + "{retry:?}" + ); + assert_eq!( + main_commit_count(&db).await, + before + 1, + "the retry published nothing" + ); + + { + let _failpoint = catalog::GRAPH_PUBLISH_AFTER_MANIFEST_COMMIT.fire_once_at(1); + db.clear_blob_at_as( + "main", + readme("content"), + Some(BlobPrecondition::Tags(vec![published.clone()])), + None, + ) + .await + .expect_err("the clear's acknowledgement is lost after publication"); + } + assert_eq!( + main_commit_count(&db).await, + before + 2, + "the clear published exactly once" + ); + let fresh = helpers::session(Omnigraph::open(&uri).await.unwrap()); + for handle in [&db, &fresh] { + let error = handle + .read_blob_at(ReadTarget::branch("main"), readme("content")) + .await + .expect_err("a cleared cell has no value and no validator"); + assert!( + matches!(&error, OmniError::Manifest(manifest) if manifest.kind == ManifestErrorKind::NotFound), + "{error:?}" + ); + } + let retry = db + .put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"again"), + Some(BlobPrecondition::Tags(vec![published])), + None, + ) + .await + .expect_err("the cleared cell rejects its old validator"); + assert!( + matches!( + &retry, + OmniError::BlobWritePreconditionFailed { current_etag: None } + ), + "{retry:?}" + ); + assert!(matches!( + db.clear_blob_at_as("main", readme("content"), None, None) + .await + .unwrap(), + BlobWriteOutcome::Null { commit: None } + )); + assert_eq!( + main_commit_count(&db).await, + before + 2, + "no duplicate clear" + ); + + let BlobWriteOutcome::Managed { etag, .. } = db + .put_blob_at_as( + "main", + readme("content"), + Bytes::from_static(b"Next"), + None, + None, + ) + .await + .expect("the same handle writes again once the fault stops") + else { + panic!("a put stores a managed value"); + }; + assert_eq!(etag, blob_etag(&fresh, "main", readme("content")).await); + assert_eq!( + read_managed_blob_bytes(&fresh, ReadTarget::branch("main"), readme("preview")).await, + b"P0", + "every write carried the preview" + ); +} + /// Exercise both shared backend contracts locally even without configured cloud fixtures. #[tokio::test] #[serial] diff --git a/crates/omnigraph/tests/forbidden_apis.rs b/crates/omnigraph/tests/forbidden_apis.rs index ffe188316..be8d5098e 100644 --- a/crates/omnigraph/tests/forbidden_apis.rs +++ b/crates/omnigraph/tests/forbidden_apis.rs @@ -246,6 +246,7 @@ write_surfaces! { "db/omnigraph/prepared_create.rs" => WriteProtocol::Exact("quiesced birth settlement: exact init claim, bounded original empty tables, no published manifest or refs") => ["settle_prepared_graph_create_after_quiescence"], "db/omnigraph.rs" => WriteProtocol::Composed("read-write admission + owned local-root create-if-absent capability probe") => ["open", "open_with_storage"], "exec/mutation.rs" => MUTATION_V9 => ["mutate", "mutate_with_receipt", "mutate_as", "mutate_as_with_receipt", "mutate_as_with_expected_head", "mutate_as_with_expected_head_receipt"], + "exec/blob_write.rs" => MUTATION_V9 => ["put_blob_at_as", "clear_blob_at_as"], "loader/mod.rs" => LOAD_V9 => ["load_jsonl", "load_jsonl_file", "load", "load_with_receipt", "load_file", "load_graph_batch"], "loader/mod.rs" => WriteProtocol::Composed("optional branch create, then Load v9") => ["load_as", "load_as_with_receipt", "load_file_as", "load_file_as_with_receipt", "load_graph_batch_as", "load_graph_batch_as_with_receipt"], "loader/mod.rs" => WriteProtocol::Composed("branch create when absent, then Load v9 alias") => ["ingest", "ingest_as", "ingest_file", "ingest_file_as"], @@ -959,6 +960,7 @@ durable_calls! { // Blob live-branch recheck: lists the table's refs to prove a vanished // fork before the incarnation refusal; read-only access to the handle. ("blob.rs", ".dataset()", 1, WriteProtocol::ReadOnlyAccess), + ("exec/blob_write.rs", ".dataset()", 4, WriteProtocol::ReadOnlyAccess), // Commit-change enumeration: pinned parent/child handles for the ordered // merge's typed row comparison. Read-only by construction — the enumerator // stages no transaction and publishes nothing. diff --git a/crates/omnigraph/tests/policy_engine_chassis.rs b/crates/omnigraph/tests/policy_engine_chassis.rs index 81f529c5f..20d271692 100644 --- a/crates/omnigraph/tests/policy_engine_chassis.rs +++ b/crates/omnigraph/tests/policy_engine_chassis.rs @@ -371,6 +371,49 @@ async fn mutate_as_allows_when_policy_permits_actor() { .expect("act-allowed should be able to Change on main"); } +/// A Blob put or clear enforces `change` on its branch for the supplied actor +/// before it opens anything, like every other Mutation-protocol writer. +#[tokio::test] +async fn blob_put_and_clear_enforce_change_for_the_actor() { + const SCHEMA: &str = "node Document { title: String @key content: Blob? }\n"; + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap(); + { + let db = helpers::session(Omnigraph::init(uri, SCHEMA).await.unwrap()); + db.load_jsonl( + r#"{"type":"Document","data":{"title":"doc"}}"#, + LoadMode::Overwrite, + ) + .await + .unwrap(); + } + let (db, _engine) = install_policy(Omnigraph::open(uri).await.unwrap(), dir.path()); + let cell = || node_blob_cell("Document", "doc", "content"); + let bytes = || bytes::Bytes::from_static(b"payload"); + + assert_denied( + db.put_blob_at_as("main", cell(), bytes(), None, Some("act-denied")) + .await, + "put_blob_at_as", + ); + assert_denied( + db.clear_blob_at_as("main", cell(), None, Some("act-denied")) + .await, + "clear_blob_at_as", + ); + let no_actor = db.put_blob_at_as("main", cell(), bytes(), None, None).await; + assert!( + matches!(no_actor, Err(OmniError::Policy(_))), + "an installed policy refuses a write without an actor: {no_actor:?}" + ); + db.put_blob_at_as("main", cell(), bytes(), None, Some("act-allowed")) + .await + .expect("act-allowed may Change on main"); + db.clear_blob_at_as("main", cell(), None, Some("act-allowed")) + .await + .expect("act-allowed may Change on main"); +} + #[tokio::test] async fn load_as_denies_when_policy_rejects_actor() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index 4189a6121..f789e825e 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -25,7 +25,7 @@ use omnigraph::db::{Omnigraph, ReadTarget}; use omnigraph::error::OmniError; use omnigraph::instrumentation::{StageWriteProbes, with_stage_write_probes}; use omnigraph::loader::LoadMode; -use omnigraph::{ExternalBlobBase, ExternalBlobExecutionScope, ExternalBlobPolicy}; +use omnigraph::{BlobCell, ExternalBlobBase, ExternalBlobExecutionScope, ExternalBlobPolicy}; use helpers::*; @@ -1585,6 +1585,320 @@ query update_shelf($note: String) { ); } +/// A Blob put admits a value of exactly 32 MiB and refuses one more byte +/// before opening a table. The row's other Blob cells are carried by value and +/// share the operation's payload allowance, so a put whose target and carried +/// sibling together exceed it is refused before any table effect. +/// Rust, not GQT: MiB-scale payloads and raw bytes are outside the case format. +#[tokio::test] +async fn blob_put_payload_bound_is_inclusive_and_counts_carried_siblings() { + use base64::Engine; + + const LIMIT: usize = 32 * 1024 * 1024; + const SCHEMA: &str = "node Document { title: String @key content: Blob? preview: Blob? }\n"; + let dir = tempfile::tempdir().unwrap(); + let db = helpers::session( + Omnigraph::init(dir.path().to_str().unwrap(), SCHEMA) + .await + .unwrap(), + ); + let preview = format!( + "base64:{}", + base64::engine::general_purpose::STANDARD.encode(vec![5_u8; 20 * 1024 * 1024]) + ); + let rows = format!( + "{}\n{}", + serde_json::json!({"type": "Document", "data": {"title": "lone"}}), + serde_json::json!({"type": "Document", "data": {"title": "paired", "preview": preview}}), + ); + db.load_jsonl(&rows, LoadMode::Overwrite).await.unwrap(); + let cell = |title: &str, property: &str| node_blob_cell("Document", title, property); + + db.put_blob_at_as( + "main", + cell("lone", "content"), + bytes::Bytes::from(vec![7_u8; LIMIT]), + None, + None, + ) + .await + .expect("a value of exactly the limit is admitted"); + let stored = + read_managed_blob_bytes(&db, ReadTarget::branch("main"), cell("lone", "content")).await; + assert_eq!(stored.len(), LIMIT); + + let head = snapshot_main(&db).await.unwrap().graph_manifest_version(); + let over = db + .put_blob_at_as( + "main", + cell("lone", "content"), + bytes::Bytes::from(vec![7_u8; LIMIT + 1]), + None, + None, + ) + .await + .unwrap_err(); + assert!( + matches!(&over, OmniError::ResourceLimitExceeded { resource, limit, actual } + if resource == "Blob write payload bytes" + && *limit == LIMIT as u64 + && *actual == LIMIT as u64 + 1), + "{over:?}" + ); + let carried = db + .put_blob_at_as( + "main", + cell("paired", "content"), + bytes::Bytes::from(vec![7_u8; 20 * 1024 * 1024]), + None, + None, + ) + .await + .unwrap_err(); + assert!( + matches!(&carried, OmniError::ResourceLimitExceeded { resource, limit, actual } + if resource == "keyed entity Blob payload bytes for node:Document" + && *limit == LIMIT as u64 + && *actual == 40 * 1024 * 1024), + "the carried sibling shares the payload allowance: {carried:?}" + ); + assert_eq!( + snapshot_main(&db).await.unwrap().graph_manifest_version(), + head, + "both refusals precede any graph effect" + ); +} + +/// A put never reads the old value of the cell it replaces, so it succeeds +/// when that value is an external reference whose object is gone. Under an +/// admitting policy it carries the row's other cells by value: a managed +/// sibling stays byte-identical, and an external sibling is read and stored as +/// managed bytes, because Lance's merge-insert cannot carry the reference. +#[tokio::test] +async fn blob_put_carries_siblings_by_value_and_never_reads_its_target() { + const SCHEMA: &str = "node Document { title: String @key content: Blob? preview: Blob? }\n"; + + let root = tempfile::tempdir().unwrap(); + let source_dir = root.path().join("sources"); + std::fs::create_dir(&source_dir).unwrap(); + let gone = source_dir.join("gone.bin"); + std::fs::write(&gone, b"about to vanish").unwrap(); + let sibling = source_dir.join("sibling.bin"); + std::fs::write(&sibling, b"external sibling").unwrap(); + let file_uri = |path: &std::path::Path| url::Url::from_file_path(path).unwrap().to_string(); + let allow = ExternalBlobPolicy::allow(vec![ + ExternalBlobBase::new( + url::Url::from_directory_path(&source_dir).unwrap(), + ExternalBlobExecutionScope::EmbeddedOnly, + ) + .unwrap(), + ]) + .unwrap(); + let graph_path = root.path().join("graph"); + let db = helpers::session( + Omnigraph::init(graph_path.to_str().unwrap(), SCHEMA) + .await + .unwrap() + .with_external_blob_policy(allow) + .unwrap(), + ); + let rows = format!( + "{}\n{}", + serde_json::json!({"type": "Document", "data": { + "title": "stale-target", "content": file_uri(&gone), "preview": "base64:a2VwdA=="}}), + serde_json::json!({"type": "Document", "data": { + "title": "external-sibling", "content": "base64:b2xk", "preview": file_uri(&sibling)}}), + ); + db.load_jsonl(&rows, LoadMode::Overwrite).await.unwrap(); + std::fs::remove_file(&gone).unwrap(); + + let probes = omnigraph::instrumentation::MergeWriteProbes::default(); + omnigraph::instrumentation::with_merge_write_probes( + probes.clone(), + db.put_blob_at_as( + "main", + node_blob_cell("Document", "stale-target", "content"), + bytes::Bytes::from_static(b"replacement"), + None, + None, + ), + ) + .await + .expect("a put never reads the value it replaces"); + assert_eq!(probes.external_blob_probe_calls(), 0); + assert_eq!(probes.external_blob_payload_read_calls(), 0); + for (property, expected) in [("content", &b"replacement"[..]), ("preview", &b"kept"[..])] { + assert_eq!( + read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", "stale-target", property), + ) + .await, + expected, + "{property}" + ); + } + + db.put_blob_at_as( + "main", + node_blob_cell("Document", "external-sibling", "content"), + bytes::Bytes::from_static(b"new"), + None, + None, + ) + .await + .unwrap(); + assert_eq!( + read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", "external-sibling", "preview"), + ) + .await, + b"external sibling", + "the carried external sibling is stored as managed bytes" + ); +} + +/// Under a denying policy a put or clear that must carry a stored external +/// reference fails with `StoredExternalBlobDenied` naming that sibling, on a +/// node and on an edge, without effect. A row holding two such cells is +/// recovered by a `merge` load of the whole row, which never reads the stored +/// cells, for a node and an edge; a node can also be recovered by a `.gq` +/// update assigning both cells. An edge has no `.gq` update. +#[tokio::test] +async fn blob_write_under_deny_names_the_carried_reference_and_whole_row_writes_recover() { + const SCHEMA: &str = r#" +node Document { + title: String @key + content: Blob? + preview: Blob? +} +edge Attachment: Document -> Document { + payload: Blob? + thumb: Blob? +} +"#; + const CLEAR_BOTH: &str = r#" +query clear_both($content: Blob?, $preview: Blob?) { + update Document set { content: $content, preview: $preview } where title = "by-update" +} +"#; + + let root = tempfile::tempdir().unwrap(); + let source_dir = root.path().join("sources"); + std::fs::create_dir(&source_dir).unwrap(); + let source = source_dir.join("stored.bin"); + std::fs::write(&source, b"stored external bytes").unwrap(); + let source_uri = url::Url::from_file_path(&source).unwrap().to_string(); + let allow = ExternalBlobPolicy::allow(vec![ + ExternalBlobBase::new( + url::Url::from_directory_path(&source_dir).unwrap(), + ExternalBlobExecutionScope::EmbeddedOnly, + ) + .unwrap(), + ]) + .unwrap(); + let graph_path = root.path().join("graph"); + let uri = graph_path.to_str().unwrap().to_string(); + { + let seeding = helpers::session( + Omnigraph::init(&uri, SCHEMA) + .await + .unwrap() + .with_external_blob_policy(allow) + .unwrap(), + ); + let mut rows = Vec::new(); + for title in ["by-load", "by-update"] { + rows.push(serde_json::json!({"type": "Document", "data": { + "title": title, "content": source_uri, "preview": source_uri}})); + } + rows.push(serde_json::json!({"edge": "Attachment", "id": "attached", + "from": "by-load", "to": "by-update", + "data": {"payload": source_uri, "thumb": source_uri}})); + let rows = rows + .iter() + .map(serde_json::Value::to_string) + .collect::>() + .join("\n"); + seeding + .load_jsonl(&rows, LoadMode::Overwrite) + .await + .unwrap(); + } + + let db = helpers::session(Omnigraph::open(&uri).await.unwrap()); + let head = snapshot_main(&db).await.unwrap().graph_manifest_version(); + let node = |property: &str| node_blob_cell("Document", "by-load", property); + let edge = |property: &str| BlobCell { + entity: omnigraph::EntityKind::Edge, + type_name: "Attachment".into(), + id: "attached".into(), + property: property.into(), + }; + for (cell, carried, type_key) in [ + (node("content"), "preview", "node:Document"), + (edge("payload"), "thumb", "edge:Attachment"), + ] { + let put = db + .put_blob_at_as( + "main", + cell.clone(), + bytes::Bytes::from_static(b"x"), + None, + None, + ) + .await + .unwrap_err(); + let clear = db + .clear_blob_at_as("main", cell.clone(), None, None) + .await + .unwrap_err(); + for error in [put, clear] { + assert!( + matches!(&error, OmniError::StoredExternalBlobDenied { type_key: key, property, .. } + if key == type_key && property == carried), + "{error:?}" + ); + } + } + assert_eq!( + snapshot_main(&db).await.unwrap().graph_manifest_version(), + head, + "a denied carry has no effect" + ); + + let whole_rows = format!( + "{}\n{}", + serde_json::json!({"type": "Document", "data": { + "title": "by-load", "content": "base64:bmV3", "preview": null}}), + serde_json::json!({"edge": "Attachment", "id": "attached", + "from": "by-load", "to": "by-update", + "data": {"payload": "base64:bmV3", "thumb": null}}), + ); + db.load_jsonl(&whole_rows, LoadMode::Merge) + .await + .expect("a merge load of the whole row never reads the stored cells"); + for cell in [node("content"), edge("payload")] { + assert_eq!( + read_managed_blob_bytes(&db, ReadTarget::branch("main"), cell).await, + b"new" + ); + } + let mut nulls = params(&[]); + for name in ["content", "preview"] { + nulls.insert( + name.to_string(), + omnigraph_compiler::query::ast::Literal::Null, + ); + } + db.mutate("main", CLEAR_BOTH, "clear_both", &nulls) + .await + .expect("an update assigning both cells reads neither"); +} + /// Carrying an unassigned stored external reference needs the graph's policy /// to admit its source; under Deny the refusal names the row and property, and /// assigning the property (a new value or null) replaces it without reading it. diff --git a/docs/dev/blob.md b/docs/dev/blob.md index 4231cc41b..80b62867a 100644 --- a/docs/dev/blob.md +++ b/docs/dev/blob.md @@ -2,7 +2,7 @@ OmniGraph treats a Blob as one property cell on an existing node or edge. Lance owns the Blob-v2 physical placement; OmniGraph owns logical identity, snapshot selection, authorization, external-source admission, bounded delivery, and graph-level publication. -This page describes the implemented read and ingestion behavior. Upload and clear commands proposed by [RFC 0033](../rfcs/0033-blob-management.md) are not implemented yet. +This page describes the implemented read, ingestion and embedded cell-write behavior. The HTTP and CLI put and clear surfaces proposed by [RFC 0033](../rfcs/0033-blob-management.md) are not implemented yet. ## Logical states @@ -68,6 +68,14 @@ A materializing rewrite (a carried update or merge cell, the bounded rewrite str A whole-object surface cannot carry a ranged external descriptor: `ExternalBlobRef::whole_object_uri` returns a `RangedExternalBlob` (whose display never includes the URI) for it, and the HTTP redirect, CLI delivery, schema rewrite and export all refuse it rather than widen it. Surfaces that are not reloaded describe it instead: `export_blob_values` takes `RangedExternalBlobs::{Refuse, Describe}` and returns `LogicalBlobValue::RangedExternal` under `Describe`, which `describe_ranged_blob_cells` writes as `{"uri", "offset", "length"}` with a positive length (the decoder below guarantees one; a missing length there is an internal error) into the row images of `logical_row_image` (change-feed images, entity reads) and the snapshot rows of the change-feed baseline, and change-row comparison compares exactly, so a ranged row never wedges a feed cursor or refuses a baseline. The baseline shares export's table walk and line format and passes `Describe` where export passes `Refuse`: it is the exact state its consumer starts from, and a refusal there would leave the graph with no baseline. The descriptor decoder refuses an external descriptor with an offset but size 0 as a Blob integrity error: Lance reads size 0 as the object's size but keeps the position, which would read past the object's end. +## Engine cell writes + +`Session::put_blob_at_as` and `Session::clear_blob_at_as` (`exec/blob_write.rs`) replace one cell of an existing row. They are Mutation-protocol writes, not a writer kind of their own: an attempt captures a `WriteTxn`, stages one `PendingMode::Upsert` row through `MutationStaging`, and runs the Mutation tail, `commit_staged_mutation` (validation, staging, `commit_all`'s detached commit) then `publish_committed_mutation` (the protocol's one publisher call). `forbidden_apis.rs` registers the file under `MUTATION_V9`. + +Lance has no single-cell Blob write (`UpdateBuilder` and `RewriteColumns` refuse Blob-v2 columns), so the row is rebuilt: the target column is the new value, and every other column comes from one `scan_with_pending_materialized_blobs` of the exact id with the target omitted, so the target's old payload is never read or charged and the other Blob cells are carried under the update rule above. The new value is a logical `{data, uri}` struct whose `LargeBinary` child adopts the caller's `Bytes` buffer without a copy. A put over `KEYED_BLOB_PAYLOAD_MAX_BYTES` is refused before the write captures anything; the carried cells and the new value then share the keyed payload allowance. + +The precondition is evaluated against the attempt's base: the ETag is computed from the base table version the attempt opened, exactly as `read_blob_at` computes it. A pre-effect `ReadSetChanged` re-prepares the whole attempt within the insert-only bound of 32 (re-reading the cell, re-evaluating the precondition, re-carrying the row), because the stored value is the caller's rather than a read-modify-write plan. A re-prepared attempt that sees a different native branch identifier or accepted schema than the first one fails with a conflict. The receipt's ETag is read from the detached version `commit_all` returned, before the manifest CAS, so nothing after publication reads storage and a later write cannot change it. + ## HTTP and CLI delivery `GET` and `HEAD /graphs/{graph_id}/blob` select one logical cell. Managed delivery supports one bounded range, strong conditional requests, and constant-memory backpressure. `HEAD` does not read payload bytes. @@ -76,7 +84,7 @@ An external value produces a `302` redirect with the stored URI; the server neve The CLI exposes `blob get` and `blob stat` for embedded and remote graphs. `get` streams managed bytes to stdout or `--out`; `stat` returns kind, resolved-view metadata, size/ETag for managed data, or the descriptor for external data. The CLI refuses to follow external references. -There is currently no HTTP or CLI Blob put/clear surface. +There is currently no HTTP or CLI Blob put/clear surface; the embedded writes above are the only cell writes. ## Maintenance and export @@ -89,6 +97,7 @@ Export emits managed values as base64 and whole-object external values as URI de ## Test owners - Engine logical reads and ingestion: `crates/omnigraph/tests/end_to_end.rs`, `branching.rs`, and in-source Blob/table-store tests. +- Engine cell writes: `end_to_end.rs::blob_put_and_clear_replace_one_cell_by_exact_id` (node and edge, preconditions, the receipt), `writes.rs` (`blob_put_*` and `blob_write_*`: the inclusive bound, carried siblings, a denied carried reference), `policy_engine_chassis.rs`, the `BlobPut` and `BlobClear` writers of `detached_commit_matrix.rs`, and `failpoints.rs` (a competing write re-evaluates the precondition and re-carries the row, a schema or branch change refuses the retry, a lost acknowledgement publishes once, a write after publication leaves the receipt unchanged). - Base and storage-root disjointness: in-source `blob.rs` tests (root forms and S3/local overlap), `end_to_end.rs::external_blob_policy_refuses_base_overlapping_graph_root`, and the `omnigraph-cluster` tests `external_blob_config_rejects_bases_overlapping_storage_root`, `external_blob_base_overlapping_storage_root_refuses_apply_over_existing_state`, `external_blob_config_reports_uncomparable_storage_root_under_its_own_code`, `serving_quarantines_applied_policies_overlapping_storage_root` (the classifier), `serving_snapshot_quarantines_graph_whose_applied_base_overlaps_storage_root` (the snapshot reader: quarantine with a healthy sibling, refusal when none is left, ledger unchanged), and `serving_snapshot_refuses_overlapping_policy_its_digest_does_not_bind`. - Lance compatibility: `crates/omnigraph/tests/lance_surface_guards.rs`. - Cluster policy persistence and serving projection: `omnigraph-cluster` in-source tests. diff --git a/docs/dev/testing.md b/docs/dev/testing.md index 4867fc8a0..0c8ff18eb 100644 --- a/docs/dev/testing.md +++ b/docs/dev/testing.md @@ -55,7 +55,7 @@ The engine integration suite is grouped by behavior, not implementation module: | Search and physical indexes | `search.rs`, `scalar_indexes.rs`, `lance_surface_guards.rs`, `rrf_prefilter_gate.rs` (the rrf plan gate's differential oracle and fences), `repro_issue_563.rs` (`#[ignore]`d overflow-scale symptom tier) | | Writes, validation, schema, and policy | `writes.rs`, `validators.rs`, `schema_apply.rs`, `policy_engine_chassis.rs` | | Branches, snapshots, diffs, and merges | `branching.rs`, `point_in_time.rs`, `changes.rs`, `merge_truth_table.rs`, `merge_fast_forward.rs` | -| Recovery and crash windows | `recovery.rs`, `failpoints.rs` (including the `live_handle_*` liveness owners: a live handle writes again once faults stop, without reopening), `detached_commit_matrix.rs` (the RFC 0067 writer × window × fault × recovery-actor matrix over the insert, multi-table, load, cleanup, ensure-indices, full-text-rebuild, merge, schema-apply, optimize and system-column-upgrade writers; the same-handle liveness actor runs by default and `OMNIGRAPH_MATRIX=full` adds the other-process and cleanup actors), in-source manifest/recovery tests | +| Recovery and crash windows | `recovery.rs`, `failpoints.rs` (including the `live_handle_*` liveness owners: a live handle writes again once faults stop, without reopening), `detached_commit_matrix.rs` (the RFC 0067 writer × window × fault × recovery-actor matrix over the insert, multi-table, load, cleanup, ensure-indices, full-text-rebuild, merge, schema-apply, optimize, system-column-upgrade and Blob put and clear writers; the same-handle liveness actor runs by default and `OMNIGRAPH_MATRIX=full` adds the other-process and cleanup actors), in-source manifest/recovery tests | | Maintenance and substrate fences | `maintenance.rs`, `lance_surface_guards.rs`, `lance_version_columns.rs`, `forbidden_apis.rs` | | Export and lineage | `export.rs`, `lineage_projection.rs` | | Legacy-vintage graphs (`id`/`src`/`dst` spellings, born at the current stamp) | `legacy_columns.rs` — load, query, export round trip, evolution; needs `--features failpoints` | diff --git a/docs/user/blobs.md b/docs/user/blobs.md index 1f5d0e9fe..bf5c19b91 100644 --- a/docs/user/blobs.md +++ b/docs/user/blobs.md @@ -60,6 +60,44 @@ but no length is refused as a Blob integrity error. OmniGraph never deletes the object named by an external reference. +### Replacing one Blob value + +An embedded `Session` replaces or clears one Blob cell of an existing node or +edge, addressed by exact id like a Blob read: + +- `Session::put_blob_at_as` stores the given bytes as a managed value and + returns its length, its ETag and the commit; +- `Session::clear_blob_at_as` sets a nullable cell to null. Clearing a cell that + is already null publishes nothing and returns no commit. + +Each call that changes the cell is one graph commit and needs the `change` +action on the branch. Neither call inserts a row: a missing entity is +`NotFound`. Clearing a property that is not nullable is refused. The write never +reads the cell's old value. Lance replaces whole rows, so the row's other cells +are carried as an `update` carries them: a stored external reference among +them needs the external Blob policy to admit its source, and their managed +payloads count toward the operation's 32 MiB Blob payload limit together with +the new value. + +A precondition makes the write conditional on the cell's current value. +`BlobPrecondition::Tags` holds when the current ETag is one of the given tags. +`BlobPrecondition::AnyExisting` holds when the cell is not null. When it fails, +the write returns `BlobWritePreconditionFailed`, which carries the cell's +current ETag when it holds a managed value, and changes nothing. A commit that +lands while the write is in flight makes it start again from the new head and +check again, so a stale ETag fails instead of overwriting the newer value; a +write without a precondition replaces whatever is current. A schema apply or a +delete and recreate of the branch landing in that window refuses the write +with a conflict instead. + +The returned ETag is the one a read at the returned commit reports. When a call +fails after its commit became durable, for example because its acknowledgement +was lost, the value is published once; read the cell for its current ETag +before retrying with a precondition. + +The CLI and the HTTP server do not offer these writes yet. Through them, change +a Blob value with a load or a mutation. + ## Query behavior Blob properties are not ordinary `.gq` read values. They cannot be projected, @@ -67,8 +105,10 @@ filtered, ordered, or aggregated. Write them through load or mutation assignment, then read an individual Blob value through the dedicated CLI or HTTP surface. -There are no `blob put` or `blob clear` commands. Use the normal graph write -path so Blob changes remain part of an atomic graph commit. +There are no `blob put` or `blob clear` CLI commands yet. Use the normal graph +write path, or the embedded writes in +[Replacing one Blob value](#replacing-one-blob-value), so Blob changes remain +part of an atomic graph commit. ## CLI reads @@ -154,6 +194,7 @@ and retry. | 32 MiB of decoded `base64:` bytes | One `base64:` value, in a load or in an insert or update mutation | `decoded blob input bytes` | | 32 MiB per touched type, and 32 MiB across all touched types, Blob payloads excluded | Incremental writes: `append` and `merge` loads, inserts, updates and branch merges. Every byte of the rows except managed Blob payloads counts, URIs and Blob framing included | `keyed write bytes for
`, `keyed entity bytes for
`, `retained keyed batch bytes per operation` | | 32 MiB of managed Blob payload per touched type, and 32 MiB across all touched types, inclusive | The same writes. Each Blob value counts its length; external bytes copied in and Blob values carried unchanged by an update count. A single value of exactly 32 MiB fits beside its row | the same names with `Blob payload bytes` in place of `bytes`, for example `keyed entity Blob payload bytes for
` | +| 32 MiB, inclusive | The bytes of one embedded Blob put, checked before the write opens a table | `Blob write payload bytes` | | 32 MiB of external payload copied into managed storage | One incremental write operation across all its types, and each type within it: two types copying 20 MiB each exceed it although each fits its per-type limit | `materialized external blob payload bytes` | | 32 MiB of Blob payload | One branch merge that writes rows, across all types, managed and external bytes together | `materialized blob payload bytes` | | 8,192 external references | One write operation or merge | `external Blob reference cells` | From 13572c18d30fb6d3b8de36d7ab1923c6acb4e881 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Thu, 8 Oct 2026 23:06:17 -0700 Subject: [PATCH 3/6] test(matrix): read the photo baseline only for the Blob writers --- crates/omnigraph/tests/detached_commit_matrix.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/omnigraph/tests/detached_commit_matrix.rs b/crates/omnigraph/tests/detached_commit_matrix.rs index 03becaf6d..d20d8152d 100644 --- a/crates/omnigraph/tests/detached_commit_matrix.rs +++ b/crates/omnigraph/tests/detached_commit_matrix.rs @@ -690,7 +690,12 @@ async fn run_cell( if writer == Writer::BlobClear { put_photo(&db, &format!("m{index}_seed")).await.unwrap(); } - let photo_before = observe_photo(&db).await; + let blob_writer = matches!(writer, Writer::BlobPut | Writer::BlobClear); + let photo_before = if blob_writer { + observe_photo(&db).await + } else { + None + }; let (mut model, _) = observe_model(&db).await; let head_before = linear_head(&person_uri).await; let knows_head_before = linear_head(&knows_uri).await; @@ -898,7 +903,7 @@ async fn run_cell( same_handle, model, "{cell}: the writer's handle disagrees with a fresh one" ); - if matches!(writer, Writer::BlobPut | Writer::BlobClear) { + if blob_writer { // The cell holds the written value exactly when the write was // acknowledged, on a fresh handle and on the writer's own. let expected = match writer { From bbbd4e3c6bb0716828cf76219e0cff44e9927dbe Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Fri, 9 Oct 2026 11:35:34 -0700 Subject: [PATCH 4/6] fix(loader): forecast Blob payload by the declared property type The compatibility loader's pre-decode forecast charged every `base64:` string as Blob payload, so a declared String whose text starts with `base64:` used the payload ceiling: a 24 MiB Blob beside such a 12 MiB String was refused at 33 MiB although the batch check admits it. The forecast now takes the type's Blob properties, as the Arrow check does: only a Blob property's `base64:` value is payload; every other value is framing. --- crates/omnigraph/src/loader/mod.rs | 84 +++++++++++++++++++++++------- crates/omnigraph/tests/writes.rs | 54 +++++++++++++++++-- 2 files changed, 113 insertions(+), 25 deletions(-) diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index 1a6f8734f..9798c1842 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use std::io::{BufRead, BufReader, Cursor}; use std::sync::Arc; @@ -665,6 +665,7 @@ async fn load_jsonl_reader_once( account_keyed_json_row( &format!("node:{type_name}"), &data, + &catalog.node_types[&type_name].blob_properties, 0, &mut keyed_input_budget, )?; @@ -716,6 +717,7 @@ async fn load_jsonl_reader_once( account_keyed_json_row( &format!("edge:{canonical}"), &data, + &edge_type.blob_properties, from.len().saturating_add(to.len()), &mut keyed_input_budget, )?; @@ -1251,7 +1253,13 @@ fn parse_strict_graph_rows( let table_key = format!("node:{type_name}"); let row = JsonValue::Object(data); if bounded_keyed_input { - account_keyed_json_row(&table_key, &row, 0, keyed_input_budget)?; + account_keyed_json_row( + &table_key, + &row, + &catalog.node_types[&type_name].blob_properties, + 0, + keyed_input_budget, + )?; } rows.nodes.entry(type_name).or_default().push(row); } @@ -1291,6 +1299,7 @@ fn parse_strict_graph_rows( account_keyed_json_row( &table_key, &JsonValue::Object(data.clone()), + &edge_type.blob_properties, from.len().saturating_add(to.len()), keyed_input_budget, )?; @@ -1447,12 +1456,15 @@ struct KeyedTableInput { /// Charge a keyed JSON record before the parse spool retains it: a lower bound /// on its Arrow bytes per table and across tables, taken before base64 is -/// decoded, split like the later batch check (`KeyedBytes`): a `base64:` -/// value's decoded length as payload, the rest as framing. Not a JSON DOM -/// bound; `MutationStaging::append_batch` is the authority. +/// decoded, split like the later batch check (`KeyedBytes`) by the type's +/// declared Blob properties: a `base64:` value of a Blob property is payload by its +/// decoded length, and every other value, a `String` that happens to start +/// with `base64:` included, is framing. Not a JSON DOM bound; +/// `MutationStaging::append_batch` is the authority. fn account_keyed_json_row( table_key: &str, data: &JsonValue, + blob_properties: &HashSet, structural_string_bytes: usize, budgets: &mut KeyedInputBudget, ) -> Result<()> { @@ -1470,8 +1482,18 @@ fn account_keyed_json_row( } let structural = u64::try_from(structural_string_bytes) .map_err(|_| OmniError::manifest_internal("keyed string bytes exceed u64"))?; - let row_bytes = - estimate_json_keyed_bytes(data)?.checked_add(KeyedBytes::framing(structural))?; + let fields = match data { + JsonValue::Object(fields) => { + fields + .iter() + .try_fold(KeyedBytes::default(), |bytes, (name, value)| { + let blob = blob_properties.contains(name); + bytes.checked_add(estimate_json_keyed_bytes(value, blob)?) + })? + } + other => estimate_json_keyed_bytes(other, false)?, + }; + let row_bytes = fields.checked_add(KeyedBytes::framing(structural))?; entry.bytes = entry .bytes .checked_add(row_bytes)? @@ -1483,17 +1505,19 @@ fn account_keyed_json_row( Ok(()) } +/// A lower bound on the Arrow bytes of a Blob property's value. fn estimate_json_arrow_bytes(value: &JsonValue) -> Result { - let bytes = estimate_json_keyed_bytes(value)?; + let bytes = estimate_json_keyed_bytes(value, true)?; bytes .framing .checked_add(bytes.payload) .ok_or_else(|| OmniError::manifest_internal("JSON value bytes overflow")) } -/// A lower bound on the Arrow bytes a JSON value becomes. A `base64:` string -/// counts its decoded length as payload; everything else is framing. -fn estimate_json_keyed_bytes(value: &JsonValue) -> Result { +/// A lower bound on the Arrow bytes a JSON value becomes. When `blob` says the +/// value is a Blob property's, a `base64:` string counts its decoded length as +/// payload; everything else, nested values included, is framing. +fn estimate_json_keyed_bytes(value: &JsonValue, blob: bool) -> Result { match value { JsonValue::Null => Ok(KeyedBytes::default()), JsonValue::Bool(_) => Ok(KeyedBytes::framing(1)), @@ -1505,7 +1529,7 @@ fn estimate_json_keyed_bytes(value: &JsonValue) -> Result { u64::try_from(bytes) .map_err(|_| OmniError::manifest_internal("JSON string bytes exceed u64")) }; - match value.strip_prefix("base64:") { + match value.strip_prefix("base64:").filter(|_| blob) { Some(encoded) => Ok(KeyedBytes::payload(string_bytes( base64::decoded_len_estimate(encoded.len()).saturating_sub( encoded @@ -1528,7 +1552,7 @@ fn estimate_json_keyed_bytes(value: &JsonValue) -> Result { values .iter() .try_fold(KeyedBytes::framing(offsets), |bytes, value| { - bytes.checked_add(estimate_json_keyed_bytes(value)?) + bytes.checked_add(estimate_json_keyed_bytes(value, false)?) }) } // Property names are schema, not per-row Arrow payload. Count values @@ -1537,7 +1561,7 @@ fn estimate_json_keyed_bytes(value: &JsonValue) -> Result { JsonValue::Object(values) => values .values() .try_fold(KeyedBytes::default(), |bytes, value| { - bytes.checked_add(estimate_json_keyed_bytes(value)?) + bytes.checked_add(estimate_json_keyed_bytes(value, false)?) }), } } @@ -3962,9 +3986,10 @@ edge WorksAt: Person -> Company ) }; + let blobs = HashSet::from(["content".to_string()]); let row = serde_json::json!({"title": "exact", "content": encode(limit)}); let mut budget = KeyedInputBudget::default(); - account_keyed_json_row("node:Document", &row, 0, &mut budget) + account_keyed_json_row("node:Document", &row, &blobs, 0, &mut budget) .expect("an exact-limit payload fits beside its row"); assert_eq!( budget.bytes, @@ -3974,10 +3999,29 @@ edge WorksAt: Person -> Company } ); + // The declared type decides: the same `base64:` text in a property + // that is not a Blob is framing by its full length. + let note = encode(1024); + let row = serde_json::json!({"title": "typed", "note": note.clone(), "content": encode(8)}); + let mut budget = KeyedInputBudget::default(); + account_keyed_json_row("node:Document", &row, &blobs, 0, &mut budget).unwrap(); + assert_eq!( + budget.bytes, + KeyedBytes { + framing: 5 + note.len() as u64, + payload: 8, + } + ); + let row = serde_json::json!({"title": "over", "content": encode(limit + 1)}); - let error = - account_keyed_json_row("node:Document", &row, 0, &mut KeyedInputBudget::default()) - .expect_err("one decoded byte over must be refused"); + let error = account_keyed_json_row( + "node:Document", + &row, + &blobs, + 0, + &mut KeyedInputBudget::default(), + ) + .expect_err("one decoded byte over must be refused"); assert!(matches!( error, OmniError::ResourceLimitExceeded { @@ -3993,8 +4037,8 @@ edge WorksAt: Person -> Company fn operation_byte_allowances_span_graph_types_and_include_their_ceiling() { let row = serde_json::json!({"payload": "x".repeat(17 * 1024 * 1024)}); let mut budget = KeyedInputBudget::default(); - account_keyed_json_row("node:Person", &row, 0, &mut budget).unwrap(); - let error = account_keyed_json_row("node:Company", &row, 0, &mut budget) + account_keyed_json_row("node:Person", &row, &HashSet::new(), 0, &mut budget).unwrap(); + let error = account_keyed_json_row("node:Company", &row, &HashSet::new(), 0, &mut budget) .expect_err("keyed parse bytes must be aggregated across types"); assert!(matches!(error, OmniError::ResourceLimitExceeded { ref resource, limit: KEYED_WRITE_MAX_BYTES, actual } diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index 4189a6121..973667185 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -1342,20 +1342,28 @@ query replace_content($c: Blob) { /// A managed Blob value of exactly the payload ceiling fits beside its row: /// payload and framing have separate ceilings, and the compatibility loader's -/// pre-decode forecast splits like the batch check. One more byte is refused -/// before any graph or table effect. The strict NDJSON loader bounds its -/// encoded line instead, so the exact-limit value's 44.7 MB line is refused. -/// Rust, not GQT: 32 MiB payloads are outside the case format. +/// pre-decode forecast splits like the batch check, by the declared property +/// types, so a `String` that starts with `base64:` stays framing. One more +/// byte is refused before any graph or table effect. The strict NDJSON loader +/// bounds its encoded line instead, so the exact-limit value's 44.7 MB line +/// is refused. Rust: this owner reads the stored bytes back, asserts nothing +/// moved on refusal and probes the compatibility loader, which GQT reaches +/// only through generated loads capped at 16 MiB per batch; a GQT case would +/// carry the 44.7 MB parameter as literal text. #[tokio::test] async fn exact_limit_blob_payload_fits_beside_its_row_and_one_more_byte_is_refused() { use base64::Engine; const LIMIT: usize = 32 * 1024 * 1024; - const SCHEMA: &str = "node Document { title: String @key content: Blob? }\n"; + const SCHEMA: &str = "node Document { title: String @key note: String? content: Blob? }\n"; const INSERT: &str = r#" query put_doc($title: String, $c: Blob) { insert Document { title: $title, content: $c } } + +query put_noted($title: String, $note: String, $c: Blob) { + insert Document { title: $title, note: $note, content: $c } +} "#; let encode = |bytes: usize| { format!( @@ -1397,6 +1405,42 @@ query put_doc($title: String, $c: Blob) { assert!(bytes.iter().all(|&byte| byte == 7), "{title}"); } + // A 24 MiB Blob beside a String whose text starts with `base64:` and holds + // 12 MiB more: the String is framing to both the mutation and the + // loader's forecast, so each admits the row. Counting the String's + // decoded 9 MiB as payload would refuse it at 33 MiB. + let blob = encode(24 * 1024 * 1024); + let note = format!("base64:{}", "A".repeat(12 * 1024 * 1024)); + db.mutate( + "main", + INSERT, + "put_noted", + ¶ms(&[("$title", "noted"), ("$note", ¬e), ("$c", &blob)]), + ) + .await + .expect("a String that looks like base64 is framing to a mutation"); + db.load_jsonl( + &serde_json::json!({"type": "Document", "data": { + "title": "noted-load", "note": note, "content": blob}}) + .to_string(), + LoadMode::Merge, + ) + .await + .expect("a String that looks like base64 is framing to the loader's forecast"); + for title in ["noted", "noted-load"] { + assert_eq!( + read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", title, "content"), + ) + .await + .len(), + 24 * 1024 * 1024, + "{title}" + ); + } + let before = snapshot_main(&db).await.unwrap().graph_manifest_version(); let strict = db .load_graph_batch("main", &row("strict", &exact), LoadMode::Merge) From 4a8a6972463ae9e55a2800098aa23d7a17972b4f Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sat, 10 Oct 2026 09:51:12 -0700 Subject: [PATCH 5/6] docs(changelog): rewrap the Blob put fragment --- changelog.d/blob-cell-put-clear.added.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog.d/blob-cell-put-clear.added.md b/changelog.d/blob-cell-put-clear.added.md index 31d427194..fd8ec1748 100644 --- a/changelog.d/blob-cell-put-clear.added.md +++ b/changelog.d/blob-cell-put-clear.added.md @@ -8,5 +8,5 @@ nothing. A put returns the value's length, its ETag and the commit; the ETag is the one a read at that commit reports. One put holds at most the session's `write_max_bytes` (32 MiB by default), inclusive; a larger one - fails before any table is opened with resource `Blob write payload bytes`. The CLI and HTTP server do not expose these - writes yet. + fails before any table is opened with resource `Blob write payload bytes`. + The CLI and HTTP server do not expose these writes yet. From c9ed133fccbd493c6ee9eeb2bbdf5e6787f49355 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sat, 10 Oct 2026 21:10:16 -0700 Subject: [PATCH 6/6] docs(rfc-0033): route PUT refuses at the server's write_max_bytes; split owners --- docs/rfcs/0033-blob-management.md | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/docs/rfcs/0033-blob-management.md b/docs/rfcs/0033-blob-management.md index 05d72fbf4..dacce7891 100644 --- a/docs/rfcs/0033-blob-management.md +++ b/docs/rfcs/0033-blob-management.md @@ -1237,7 +1237,7 @@ Physical row addresses never become public stable identity. | URI credential disclosure | Reject user-info/query/fragment credentials at config and input; return URI only to an authorized Blob reader | | URI parser amplification | Reject a raw configured or input URI above 64 KiB before trimming, parsing, decoding, or filesystem resolution | | External SSRF during read | Descriptor-first classification; redirect only; no proxy or validation on GET/HEAD | -| Oversize upload | Route 32 MiB and engine `write_max_bytes` (32 MiB by default) inclusive limits in payload bytes, separate from the row-data budget (§4.3); refusal before effect | +| Oversize upload | Route and engine `write_max_bytes` (32 MiB by default, at most 32 MiB) inclusive limits in payload bytes, separate from the row-data budget (§4.3); refusal before effect, and before reading past the limit on the route | | Rewrite amplification | New logical input and row-writing branch merge pre-size all carried Blob payloads under one 32 MiB operation budget before read; predicate mutation carry applies the same cumulative byte ceiling while materializing bounded scan batches. Schema apply reads no Blob payload: its column changes are metadata-only (§8.4) | | Compaction memory | Optimize sets the compaction scanner batch from the planned fragments' largest row, summing that row's Blob columns: as many rows (1 to 8,192) as fit 32 MiB of managed payload at that row's size, so one batch materializes at most 32 MiB of managed payload. A row whose Blob values together exceed 32 MiB is compacted in a batch of its own and materialized whole. This bounds payload per batch, not heap: Lance's writer copies inline payloads into its prepared arrays while it holds the batch (see the operator guide's optimize section). External descriptors are carried unread | | External-source planning | Row-writing branch merge admits at most 8,192 external-reference cells and 32 MiB of retained URI metadata before HEAD; probes are bounded and normalized aliases deduplicate within the applicable operation or scan-batch envelope | @@ -1430,13 +1430,13 @@ The implementation extends existing owners before creating new fixtures, per reference, proving that target payload is not read while the untouched sibling remains byte-identical. Every refusal proves table HEAD, manifest, lineage, and accepted state unchanged. -- `writes.rs` also owns the payload/framing split (§4.3): an exact 32 MiB PUT is - accepted and one more byte is refused by the payload limit, not by a framing - ceiling; a 32 MiB value carried by an update, one inserted through an - embedded `.gq` parameter and one loaded by the compatibility loader are - accepted where the combined ceiling refused them, while the strict NDJSON - loader still refuses that value's encoded line at its line limit; and a - framing refusal still names its keyed-write ceiling. It owns the sibling carry rule +- The payload/row split (§4.3) is owned by the `write_max_bytes` owners: the + `write_max_bytes*.gqt` and `load_write_max_bytes_*.gqt` cases (an exact + payload beside its row, one more byte refused, a value carried by an update), + `consistency.rs` and `merge_fast_forward.rs`. `writes.rs` owns the put's + bound: an exact-limit put is accepted at the default and under a lowered + `write_max_bytes`, one more byte is refused as `Blob write payload bytes`, and + a carried sibling shares the payload allowance. It owns the sibling carry rule too: an external sibling becomes managed under an admitting policy (visible through `stat`), fails with `StoredExternalBlobDenied` naming the sibling under a denying one, and on a row with two external cells under a denying policy a @@ -1936,9 +1936,13 @@ publisher architecture. most 32 MiB, captured once per operation, for every keyed writer and the compatibility loader's forecast, which classifies a `base64:` value as payload only for a declared Blob property. The Blob put checks its value against the - session's allowance, so a lowered `write_max_bytes` lowers the put's bound too; - the PUT route and the CLI keep their 32 MiB pre-read bounds, the setting's - maximum. The two §12.3 surface guards move to 3A. Superseded: §4.3's "The raw + session's allowance, so a lowered `write_max_bytes` lowers the put's bound too. + A PUT carries no settings, so the route refuses a body over the server's + process `write_max_bytes` before reading past it and reserves only that much + ingress; the CLI keeps its 32 MiB pre-read bound, the setting's maximum, + because a served caller cannot see the server's value. The payload/row split + is owned by the `write_max_bytes` owners, so §12.2's `writes.rs` split bullet + narrows to the put's bound. The two §12.3 surface guards move to 3A. Superseded: §4.3's "The raw managed payload limit is 32 MiB inclusive" and "the same operation-wide 32 MiB pre-effect payload budget"; §10's "Route and engine 32 MiB inclusive limits in payload bytes, separate from the batch framing budget"; and §13's 3-pre