From a0ce4e81d97f80f81deb9311a86c23ec7de61de2 Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Wed, 22 Jul 2026 19:52:26 -0700 Subject: [PATCH 01/13] Unify hosted media omission behavior --- crates/locality-notion/src/lib.rs | 17 +- crates/locality-notion/src/media.rs | 638 +++++++++++++------ crates/locality-notion/src/portable.rs | 82 ++- crates/locality-notion/src/render.rs | 124 +++- crates/locality-notion/tests/fetch_render.rs | 229 +++++-- crates/localityd/src/notion.rs | 13 +- crates/localityd/tests/notion_hydration.rs | 306 ++++++++- docs/notion-canonical-format.md | 4 +- docs/notion-object-support.md | 13 +- 9 files changed, 1110 insertions(+), 316 deletions(-) diff --git a/crates/locality-notion/src/lib.rs b/crates/locality-notion/src/lib.rs index 98e6c1a2..5b560b4e 100644 --- a/crates/locality-notion/src/lib.rs +++ b/crates/locality-notion/src/lib.rs @@ -38,8 +38,8 @@ use crate::apply::{apply_plan, apply_undo, check_concurrency}; use crate::client::{DEFAULT_NOTION_TOKEN_ENV, HttpNotionApi, NotionApi}; use crate::fetch::fetch_page_bundle; use crate::media::{ - MediaDownloadReport, PortableMediaCaptureFetcher, PortableMediaCapturePolicy, - download_media_assets, + MediaDownloadReport, MediaFetchReport, PortableMediaCaptureFetcher, PortableMediaCapturePolicy, + default_portable_media_fetcher, download_media_assets, fetch_media_asset_report_with_fetcher, }; use crate::projection::{ enumerate_explicit_root_trees, enumerate_shared_pages, list_container_children, observe_entity, @@ -248,6 +248,19 @@ impl NotionConnector { download_media_assets(mount_root.as_ref(), &rendered.media_assets) } + /// Fetch the hosted assets selected by the shared renderer. + /// + /// The optional injected fetcher is also used by daemon hydration tests; + /// production connectors use the hardened default transport. + pub fn fetch_rendered_media(&self, rendered: &NotionRenderedEntity) -> MediaFetchReport { + let default_fetcher = default_portable_media_fetcher(); + let fetcher = self + .portable_media_fetcher + .as_deref() + .unwrap_or(default_fetcher.as_ref()); + fetch_media_asset_report_with_fetcher(&rendered.media_assets, fetcher) + } + pub fn database_schema_yaml(&self, database_id: &RemoteId) -> LocalityResult { database::database_schema_yaml(self.api.as_ref(), database_id.as_str()) } diff --git a/crates/locality-notion/src/media.rs b/crates/locality-notion/src/media.rs index f0b3ef55..1b6246fc 100644 --- a/crates/locality-notion/src/media.rs +++ b/crates/locality-notion/src/media.rs @@ -66,8 +66,41 @@ pub struct PortableMediaCapture { pub media_type: String, } +/// Redaction-safe result of attempting to capture one Notion-hosted asset. +/// +/// The variants intentionally carry no provider URL or transport message. This +/// type crosses the public portable boundary, where signed URLs and response +/// details must never be serialized into completeness metadata. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum HostedMediaCaptureOutcome { + Captured(PortableMediaCapture), + Unavailable, + TooLarge, + Unsafe, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum HostedMediaFailureKind { + Unavailable, + TooLarge, + Unsafe, +} + pub trait PortableMediaCaptureFetcher: Send + Sync { fn fetch(&self, hosted_url: &str, max_bytes: usize) -> LocalityResult; + + /// Typed capture API used by portable and desktop projection paths. + /// + /// Existing fetcher implementations remain source-compatible: their + /// errors conservatively become genuine unavailability, while an + /// oversized successful body is still classified as a policy omission. + fn fetch_outcome(&self, hosted_url: &str, max_bytes: usize) -> HostedMediaCaptureOutcome { + match self.fetch(hosted_url, max_bytes) { + Ok(capture) if capture.bytes.len() > max_bytes => HostedMediaCaptureOutcome::TooLarge, + Ok(capture) => HostedMediaCaptureOutcome::Captured(capture), + Err(_) => HostedMediaCaptureOutcome::Unavailable, + } + } } #[derive(Default)] @@ -77,13 +110,31 @@ struct SecurePortableMediaCaptureFetcher { impl PortableMediaCaptureFetcher for SecurePortableMediaCaptureFetcher { fn fetch(&self, hosted_url: &str, max_bytes: usize) -> LocalityResult { - let mut transport = self.transport.lock().map_err(|_| { - LocalityError::InvalidState("portable media HTTP client lock is poisoned".to_string()) - })?; + match self.fetch_outcome(hosted_url, max_bytes) { + HostedMediaCaptureOutcome::Captured(capture) => Ok(capture), + HostedMediaCaptureOutcome::Unavailable => { + Err(LocalityError::Io("hosted media is unavailable".to_string())) + } + HostedMediaCaptureOutcome::TooLarge => Err(LocalityError::InvalidState( + "hosted media exceeds the asset limit".to_string(), + )), + HostedMediaCaptureOutcome::Unsafe => Err(LocalityError::InvalidState( + "hosted media failed safety validation".to_string(), + )), + } + } + + fn fetch_outcome(&self, hosted_url: &str, max_bytes: usize) -> HostedMediaCaptureOutcome { + let Ok(mut transport) = self.transport.lock() else { + return HostedMediaCaptureOutcome::Unavailable; + }; if transport.is_none() { - *transport = Some(ReqwestPortableMediaTransport::new()?); + let Ok(client) = ReqwestPortableMediaTransport::new() else { + return HostedMediaCaptureOutcome::Unavailable; + }; + *transport = Some(client); } - fetch_portable_media_with_transport( + fetch_hosted_media_outcome_with_transport( transport .as_ref() .expect("portable media HTTP client was initialized above"), @@ -179,65 +230,145 @@ impl PortableMediaHttpTransport for ReqwestPortableMediaTransport { } } +#[cfg(test)] fn fetch_portable_media_with_transport( transport: &dyn PortableMediaHttpTransport, hosted_url: &str, max_bytes: usize, ) -> LocalityResult { - let mut current = validate_portable_hosted_media_url(hosted_url)?; - let mut visited = std::collections::BTreeSet::new(); + match fetch_hosted_media_outcome_with_transport(transport, hosted_url, max_bytes) { + HostedMediaCaptureOutcome::Captured(capture) => Ok(capture), + HostedMediaCaptureOutcome::Unavailable => Err(LocalityError::Io( + "portable media is unavailable".to_string(), + )), + HostedMediaCaptureOutcome::TooLarge => Err(LocalityError::InvalidState( + "portable media exceeds the asset limit".to_string(), + )), + HostedMediaCaptureOutcome::Unsafe => Err(LocalityError::InvalidState( + "portable media failed safety validation".to_string(), + )), + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum HostedMediaTransferFailure { + RetryableUnavailable, + Unavailable, + TooLarge, + Unsafe, +} + +fn fetch_hosted_media_outcome_with_transport( + transport: &dyn PortableMediaHttpTransport, + hosted_url: &str, + max_bytes: usize, +) -> HostedMediaCaptureOutcome { + fetch_hosted_media_outcome_with_policy( + transport, + hosted_url, + max_bytes, + PORTABLE_MEDIA_REQUEST_TIMEOUT, + MEDIA_FETCH_RETRY_DELAY, + ) +} + +fn fetch_hosted_media_outcome_with_policy( + transport: &dyn PortableMediaHttpTransport, + hosted_url: &str, + max_bytes: usize, + deadline: Duration, + retry_delay: Duration, +) -> HostedMediaCaptureOutcome { + let initial = match validate_portable_hosted_media_url(hosted_url) { + Ok(url) => url, + Err(_) => return HostedMediaCaptureOutcome::Unsafe, + }; let started = Instant::now(); + for attempt in 1..=MEDIA_FETCH_ATTEMPTS { + match fetch_hosted_media_once(transport, &initial, max_bytes, started, deadline) { + Ok(capture) => return HostedMediaCaptureOutcome::Captured(capture), + Err(HostedMediaTransferFailure::RetryableUnavailable) + if attempt < MEDIA_FETCH_ATTEMPTS && started.elapsed() < deadline => + { + let remaining = deadline.saturating_sub(started.elapsed()); + thread::sleep(retry_delay.min(remaining)); + } + Err(HostedMediaTransferFailure::RetryableUnavailable) + | Err(HostedMediaTransferFailure::Unavailable) => { + return HostedMediaCaptureOutcome::Unavailable; + } + Err(HostedMediaTransferFailure::TooLarge) => { + return HostedMediaCaptureOutcome::TooLarge; + } + Err(HostedMediaTransferFailure::Unsafe) => { + return HostedMediaCaptureOutcome::Unsafe; + } + } + } + HostedMediaCaptureOutcome::Unavailable +} + +fn fetch_hosted_media_once( + transport: &dyn PortableMediaHttpTransport, + initial: &reqwest::Url, + max_bytes: usize, + started: Instant, + deadline: Duration, +) -> Result { + let mut current = initial.clone(); + let mut visited = std::collections::BTreeSet::new(); for redirects in 0..=PORTABLE_MEDIA_MAX_REDIRECTS { if !visited.insert(current.as_str().to_string()) { - return Err(LocalityError::InvalidState( - "portable media redirect loop rejected".to_string(), - )); + return Err(HostedMediaTransferFailure::Unsafe); } - let timeout = PORTABLE_MEDIA_REQUEST_TIMEOUT + let timeout = deadline .checked_sub(started.elapsed()) - .ok_or_else(|| LocalityError::Io("portable media request timed out".to_string()))?; - let mut response = transport.get(current.as_str(), timeout)?; + .ok_or(HostedMediaTransferFailure::RetryableUnavailable)?; + if timeout.is_zero() { + return Err(HostedMediaTransferFailure::RetryableUnavailable); + } + let mut response = transport + .get(current.as_str(), timeout) + .map_err(|_| HostedMediaTransferFailure::RetryableUnavailable)?; if response.status.is_redirection() { if redirects == PORTABLE_MEDIA_MAX_REDIRECTS { - return Err(LocalityError::InvalidState( - "portable media redirect limit exceeded".to_string(), - )); + return Err(HostedMediaTransferFailure::Unsafe); } - let location = response.location.as_deref().ok_or_else(|| { - LocalityError::InvalidState( - "portable media redirect omitted its destination".to_string(), - ) - })?; - let destination = current.join(location).map_err(|_| { - LocalityError::InvalidState( - "portable media redirect destination is invalid".to_string(), - ) - })?; - current = validate_portable_hosted_media_url(destination.as_str())?; + let location = response + .location + .as_deref() + .ok_or(HostedMediaTransferFailure::Unsafe)?; + let destination = current + .join(location) + .map_err(|_| HostedMediaTransferFailure::Unsafe)?; + current = validate_portable_hosted_media_url(destination.as_str()) + .map_err(|_| HostedMediaTransferFailure::Unsafe)?; continue; } if !response.status.is_success() { - return Err(LocalityError::Io(format!( - "portable media request returned HTTP {}", - response.status.as_u16() - ))); + return Err( + if response.status == StatusCode::REQUEST_TIMEOUT + || response.status == StatusCode::TOO_MANY_REQUESTS + || response.status.is_server_error() + { + HostedMediaTransferFailure::RetryableUnavailable + } else { + HostedMediaTransferFailure::Unavailable + }, + ); } if let Some(encoding) = response.content_encoding.as_deref() && !encoding.eq_ignore_ascii_case("identity") { - return Err(LocalityError::InvalidState( - "portable media content encoding is unsupported".to_string(), - )); + return Err(HostedMediaTransferFailure::Unsafe); } if response .content_length .is_some_and(|length| length > max_bytes as u64) { - return Err(LocalityError::InvalidState( - "portable media content length exceeds the asset limit".to_string(), - )); + return Err(HostedMediaTransferFailure::TooLarge); } let mut bytes = Vec::with_capacity( @@ -249,16 +380,15 @@ fn fetch_portable_media_with_transport( ); let mut buffer = [0_u8; PORTABLE_MEDIA_READ_BUFFER_BYTES]; loop { - let read = response.body.read(&mut buffer).map_err(|_| { - LocalityError::Io("portable media response body failed".to_string()) - })?; + let read = response + .body + .read(&mut buffer) + .map_err(|_| HostedMediaTransferFailure::RetryableUnavailable)?; if read == 0 { break; } if bytes.len().saturating_add(read) > max_bytes { - return Err(LocalityError::InvalidState( - "portable media response exceeded the asset limit".to_string(), - )); + return Err(HostedMediaTransferFailure::TooLarge); } bytes.extend_from_slice(&buffer[..read]); } @@ -266,9 +396,7 @@ fn fetch_portable_media_with_transport( .content_length .is_some_and(|length| length != bytes.len() as u64) { - return Err(LocalityError::InvalidState( - "portable media content length did not match the response body".to_string(), - )); + return Err(HostedMediaTransferFailure::RetryableUnavailable); } return Ok(PortableMediaCapture { bytes, @@ -276,7 +404,7 @@ fn fetch_portable_media_with_transport( }); } - unreachable!("redirect loop always returns or continues within its fixed bound") + Err(HostedMediaTransferFailure::Unsafe) } pub(crate) fn validate_portable_hosted_media_url(url: &str) -> LocalityResult { @@ -473,9 +601,20 @@ pub struct MediaDownloadFailure { pub kind: String, pub source_url: String, pub local_path: PathBuf, + /// Redaction-safe failure code retained as a string for API compatibility. pub error: String, } +impl MediaDownloadFailure { + pub fn outcome(&self) -> HostedMediaFailureKind { + match self.error.as_str() { + "hosted_media_too_large" => HostedMediaFailureKind::TooLarge, + "unsafe_hosted_media" => HostedMediaFailureKind::Unsafe, + _ => HostedMediaFailureKind::Unavailable, + } + } +} + #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct MediaManifest { pub version: u32, @@ -533,18 +672,47 @@ pub fn fetch_media_assets(assets: &[MediaAsset]) -> LocalityResult MediaFetchReport { - let client = notion_http_client(); + let fetcher = default_portable_media_fetcher(); + fetch_media_asset_report_with_fetcher(assets, fetcher.as_ref()) +} + +pub fn fetch_media_asset_report_with_fetcher( + assets: &[MediaAsset], + fetcher: &dyn PortableMediaCaptureFetcher, +) -> MediaFetchReport { let mut report = MediaFetchReport::default(); for asset in assets.iter().filter(|asset| should_download(asset)) { - match fetch_media_asset_with_retries(&client, asset) { - Ok(downloaded) => report.downloaded.push(downloaded), - Err(error) => report.failed.push(MediaDownloadFailure { + let outcome = if validate_portable_hosted_media_url(&asset.source_url).is_ok() { + fetcher.fetch_outcome(&asset.source_url, PORTABLE_MEDIA_MAX_ASSET_BYTES) + } else { + // The renderer emits MediaAsset only for structurally hosted + // sources. Validate again at this trust boundary so custom + // fetchers never receive an unsafe provider URL. + HostedMediaCaptureOutcome::Unsafe + }; + match outcome { + HostedMediaCaptureOutcome::Captured(capture) => { + report.downloaded.push(DownloadedMediaAsset { + block_id: asset.block_id.clone(), + kind: asset.kind.clone(), + source_url: asset.source_url.clone(), + local_path: asset.local_path.clone(), + bytes: capture.bytes, + }); + } + outcome => report.failed.push(MediaDownloadFailure { block_id: asset.block_id.clone(), kind: asset.kind.clone(), source_url: asset.source_url.clone(), local_path: asset.local_path.clone(), - error: error.to_string(), + error: match outcome { + HostedMediaCaptureOutcome::TooLarge => "hosted_media_too_large", + HostedMediaCaptureOutcome::Unsafe => "unsafe_hosted_media", + HostedMediaCaptureOutcome::Unavailable + | HostedMediaCaptureOutcome::Captured(_) => "unavailable_hosted_media", + } + .to_string(), }), } } @@ -552,64 +720,14 @@ pub fn fetch_media_asset_report(assets: &[MediaAsset]) -> MediaFetchReport { report } -fn fetch_media_asset_with_retries( - client: &Client, - asset: &MediaAsset, -) -> LocalityResult { - let mut last_error = None; - for attempt in 1..=MEDIA_FETCH_ATTEMPTS { - match fetch_media_asset(client, asset) { - Ok(downloaded) => return Ok(downloaded), - Err(error) => { - last_error = Some(error); - if attempt < MEDIA_FETCH_ATTEMPTS { - thread::sleep(MEDIA_FETCH_RETRY_DELAY); - } - } - } - } - - Err(last_error.unwrap_or_else(|| LocalityError::Io("media download failed".to_string()))) -} - -fn fetch_media_asset(client: &Client, asset: &MediaAsset) -> LocalityResult { - let response = client - .get(&asset.source_url) - .send() - .map_err(|error| LocalityError::Io(format!("media download failed: {error}")))?; - let status = response.status(); - if !status.is_success() { - return Err(LocalityError::Io(format!( - "media download returned HTTP {status} for block `{}`", - asset.block_id - ))); - } - let bytes = response - .bytes() - .map_err(|error| LocalityError::Io(format!("media download body failed: {error}")))? - .to_vec(); - - Ok(DownloadedMediaAsset { - block_id: asset.block_id.clone(), - kind: asset.kind.clone(), - source_url: asset.source_url.clone(), - local_path: asset.local_path.clone(), - bytes, - }) -} - fn should_download(asset: &MediaAsset) -> bool { - is_file_like_media_kind(&asset.kind) && is_downloadable_url(&asset.source_url) + is_file_like_media_kind(&asset.kind) } fn is_file_like_media_kind(kind: &str) -> bool { matches!(kind, "image" | "video" | "file" | "pdf" | "audio") } -pub(crate) fn is_downloadable_url(url: &str) -> bool { - url.starts_with("http://") || url.starts_with("https://") -} - fn media_page_dir(page_path: &Path) -> PathBuf { let mut path = media_root_path(); let mut pushed_component = false; @@ -1043,19 +1161,19 @@ mod tests { #[cfg(windows)] use super::resolve_media_href_with_content_root; use super::{ - MediaAsset, PORTABLE_MEDIA_READ_BUFFER_BYTES, PortableMediaHttpResponse, - PortableMediaHttpTransport, fetch_portable_media_with_transport, local_media_href, - media_local_path, portable_media_expired, replace_media_manifest, resolve_media_href, - sanitize_portable_hosted_media_url, validate_portable_hosted_media_url, + HostedMediaCaptureOutcome, MediaAsset, PORTABLE_MEDIA_READ_BUFFER_BYTES, + PortableMediaCapture, PortableMediaCaptureFetcher, PortableMediaHttpResponse, + PortableMediaHttpTransport, fetch_hosted_media_outcome_with_policy, + fetch_hosted_media_outcome_with_transport, fetch_portable_media_with_transport, + local_media_href, media_local_path, portable_media_expired, replace_media_manifest, + resolve_media_href, sanitize_portable_hosted_media_url, validate_portable_hosted_media_url, }; use reqwest::StatusCode; use std::collections::VecDeque; - use std::io::{Read, Write}; - use std::net::{TcpListener, TcpStream}; + use std::io::Read; use std::path::Path; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; - use std::thread; #[test] fn replacing_absent_manifest_with_no_assets_does_not_create_mount_root() { @@ -1182,26 +1300,72 @@ mod tests { } #[test] - fn downloads_file_like_media_kinds_from_http_urls() { + fn selects_only_file_like_media_for_hosted_validation() { for kind in ["image", "video", "file", "pdf", "audio"] { let asset = MediaAsset { block_id: format!("{kind}-1"), kind: kind.to_string(), - source_url: format!("https://example.com/{kind}.bin"), + source_url: format!("https://secure.notion-static.com/{kind}.bin"), local_path: Path::new(".loc/media/Page/media.bin").to_path_buf(), }; assert!(super::should_download(&asset), "{kind} should download"); } - let relative = MediaAsset { - block_id: "video-1".to_string(), - kind: "video".to_string(), - source_url: "cars.mp4".to_string(), + let unsupported = MediaAsset { + block_id: "unsupported-1".to_string(), + kind: "bookmark".to_string(), + source_url: "https://example.com/cars.mp4".to_string(), local_path: Path::new(".loc/media/Page/video-1.mp4").to_path_buf(), }; - assert!(!super::should_download(&relative)); + assert!(!super::should_download(&unsupported)); + } + + #[test] + fn unsafe_hosted_asset_never_reaches_an_injected_fetcher() { + struct NeverCalledFetcher; + impl PortableMediaCaptureFetcher for NeverCalledFetcher { + fn fetch( + &self, + _hosted_url: &str, + _max_bytes: usize, + ) -> locality_core::LocalityResult { + panic!("unsafe URL reached injected fetcher") + } + } + + let report = super::fetch_media_asset_report_with_fetcher( + &[MediaAsset { + block_id: "unsafe".to_string(), + kind: "image".to_string(), + source_url: "https://example.com/not-a-hosted-origin.png".to_string(), + local_path: Path::new(".loc/media/Page/image.png").to_path_buf(), + }], + &NeverCalledFetcher, + ); + assert!(report.downloaded.is_empty()); + assert_eq!(report.failed.len(), 1); + assert_eq!( + report.failed[0].outcome(), + super::HostedMediaFailureKind::Unsafe + ); + } + + #[test] + fn media_download_failure_preserves_legacy_public_fields() { + let failure = super::MediaDownloadFailure { + block_id: "block".to_string(), + kind: "image".to_string(), + source_url: "https://secure.notion-static.com/image.png".to_string(), + local_path: Path::new(".loc/media/Page/image.png").to_path_buf(), + error: "legacy transport message".to_string(), + }; + assert_eq!(failure.error, "legacy transport message"); + assert_eq!( + failure.outcome(), + super::HostedMediaFailureKind::Unavailable + ); } #[test] @@ -1280,17 +1444,15 @@ mod tests { None, Vec::new(), )]); - let error = fetch_portable_media_with_transport( - &disallowed, - "https://secure.notion-static.com/first.png", - 1024, - ) - .expect_err("disallowed redirect"); assert_eq!( - error.to_string(), - "invalid state: portable media URL host is not allowed" + fetch_hosted_media_outcome_with_transport( + &disallowed, + "https://secure.notion-static.com/first.png", + 1024, + ), + HostedMediaCaptureOutcome::Unsafe ); - assert!(!error.to_string().contains("token=secret")); + assert_eq!(disallowed.requests().len(), 1); let redirect_loop = ScriptedPortableMediaTransport::new([scripted_response( StatusCode::FOUND, @@ -1301,14 +1463,12 @@ mod tests { Vec::new(), )]); assert_eq!( - fetch_portable_media_with_transport( + fetch_hosted_media_outcome_with_transport( &redirect_loop, "https://secure.notion-static.com/first.png", 1024, - ) - .expect_err("redirect loop") - .to_string(), - "invalid state: portable media redirect loop rejected" + ), + HostedMediaCaptureOutcome::Unsafe ); let too_many = ScriptedPortableMediaTransport::new((0..=3).map(|index| { @@ -1325,14 +1485,12 @@ mod tests { ) })); assert_eq!( - fetch_portable_media_with_transport( + fetch_hosted_media_outcome_with_transport( &too_many, "https://secure.notion-static.com/start.png", 1024, - ) - .expect_err("redirect bound") - .to_string(), - "invalid state: portable media redirect limit exceeded" + ), + HostedMediaCaptureOutcome::Unsafe ); } @@ -1347,9 +1505,10 @@ mod tests { b"data".to_vec(), )]); assert_eq!( - portable_transport_error(&encoded, 4), - "invalid state: portable media content encoding is unsupported" + portable_transport_outcome(&encoded, 4), + HostedMediaCaptureOutcome::Unsafe ); + assert_eq!(encoded.requests().len(), 1); let declared_oversize = ScriptedPortableMediaTransport::new([scripted_response( StatusCode::OK, @@ -1360,22 +1519,37 @@ mod tests { b"data".to_vec(), )]); assert_eq!( - portable_transport_error(&declared_oversize, 4), - "invalid state: portable media content length exceeds the asset limit" + portable_transport_outcome(&declared_oversize, 4), + HostedMediaCaptureOutcome::TooLarge ); + assert_eq!(declared_oversize.requests().len(), 1); - let mismatched = ScriptedPortableMediaTransport::new([scripted_response( - StatusCode::OK, - None, - Some("identity"), - Some(3), - Some("image/png"), - b"data".to_vec(), - )]); + let mismatched = ScriptedPortableMediaTransport::new([ + scripted_response( + StatusCode::OK, + None, + Some("identity"), + Some(3), + Some("image/png"), + b"data".to_vec(), + ), + scripted_response( + StatusCode::OK, + None, + None, + Some(4), + Some("image/png"), + b"data".to_vec(), + ), + ]); assert_eq!( - portable_transport_error(&mismatched, 4), - "invalid state: portable media content length did not match the response body" + portable_transport_outcome(&mismatched, 4), + HostedMediaCaptureOutcome::Captured(PortableMediaCapture { + bytes: b"data".to_vec(), + media_type: "image/png".to_string(), + }) ); + assert_eq!(mismatched.requests().len(), 2); let streamed_oversize = ScriptedPortableMediaTransport::new([scripted_response( StatusCode::OK, @@ -1386,9 +1560,10 @@ mod tests { b"12345".to_vec(), )]); assert_eq!( - portable_transport_error(&streamed_oversize, 4), - "invalid state: portable media response exceeded the asset limit" + portable_transport_outcome(&streamed_oversize, 4), + HostedMediaCaptureOutcome::TooLarge ); + assert_eq!(streamed_oversize.requests().len(), 1); } #[test] @@ -1425,6 +1600,7 @@ mod tests { struct ScriptedPortableMediaTransport { responses: Mutex>, requests: Mutex>, + timeouts: Mutex>, max_read_size: Arc, } @@ -1448,6 +1624,7 @@ mod tests { Self { responses: Mutex::new(responses), requests: Mutex::new(Vec::new()), + timeouts: Mutex::new(Vec::new()), max_read_size, } } @@ -1459,18 +1636,23 @@ mod tests { fn max_read_size(&self) -> usize { self.max_read_size.load(Ordering::SeqCst) } + + fn timeouts(&self) -> Vec { + self.timeouts.lock().expect("timeouts").clone() + } } impl PortableMediaHttpTransport for ScriptedPortableMediaTransport { fn get( &self, url: &str, - _timeout: std::time::Duration, + timeout: std::time::Duration, ) -> locality_core::LocalityResult { self.requests .lock() .expect("requests") .push(url.to_string()); + self.timeouts.lock().expect("timeouts").push(timeout); self.responses .lock() .expect("responses") @@ -1522,58 +1704,132 @@ mod tests { } } - fn portable_transport_error( + fn portable_transport_outcome( transport: &ScriptedPortableMediaTransport, max_bytes: usize, - ) -> String { - fetch_portable_media_with_transport( + ) -> HostedMediaCaptureOutcome { + fetch_hosted_media_outcome_with_transport( transport, "https://secure.notion-static.com/image.png", max_bytes, ) - .expect_err("transport must reject") - .to_string() } #[test] - fn media_fetch_retries_transient_connection_failures() { - let bytes = b"retry-media-bytes".to_vec(); - let listener = TcpListener::bind("127.0.0.1:0").expect("bind media test server"); - let url = format!( - "http://{}/retry-image.png", - listener.local_addr().expect("media test server addr") + fn hosted_media_retries_only_transient_statuses_with_one_deadline() { + let transport = ScriptedPortableMediaTransport::new([ + scripted_response( + StatusCode::INTERNAL_SERVER_ERROR, + None, + None, + None, + None, + vec![], + ), + scripted_response( + StatusCode::TOO_MANY_REQUESTS, + None, + None, + None, + None, + vec![], + ), + scripted_response( + StatusCode::OK, + None, + None, + Some(4), + Some("image/png"), + b"data".to_vec(), + ), + ]); + assert_eq!( + fetch_hosted_media_outcome_with_policy( + &transport, + "https://secure.notion-static.com/image.png", + 1024, + std::time::Duration::from_secs(1), + std::time::Duration::from_millis(1), + ), + HostedMediaCaptureOutcome::Captured(super::PortableMediaCapture { + bytes: b"data".to_vec(), + media_type: "image/png".to_string(), + }) + ); + assert_eq!(transport.requests().len(), 3); + let timeouts = transport.timeouts(); + assert_eq!(timeouts.len(), 3); + assert!(timeouts.windows(2).all(|pair| pair[1] <= pair[0])); + assert!( + timeouts + .iter() + .all(|timeout| *timeout <= std::time::Duration::from_secs(1)) ); - let handle = thread::spawn(move || { - let (first, _) = listener.accept().expect("accept failed media request"); - drop(first); - - let (second, _) = listener.accept().expect("accept retried media request"); - serve_test_media_response(second, &bytes); - }); - let report = super::fetch_media_asset_report(&[MediaAsset { - block_id: "image-block".to_string(), - kind: "image".to_string(), - source_url: url, - local_path: Path::new(".loc/media/Page/image.png").to_path_buf(), - }]); + let exhausted = ScriptedPortableMediaTransport::new((0..4).map(|_| { + scripted_response( + StatusCode::SERVICE_UNAVAILABLE, + None, + None, + None, + None, + vec![], + ) + })); + assert_eq!( + fetch_hosted_media_outcome_with_policy( + &exhausted, + "https://secure.notion-static.com/image.png", + 1024, + std::time::Duration::from_secs(1), + std::time::Duration::ZERO, + ), + HostedMediaCaptureOutcome::Unavailable + ); + assert_eq!(exhausted.requests().len(), 3); - handle.join().expect("join media test server"); - assert!(report.failed.is_empty(), "{report:#?}"); - assert_eq!(report.downloaded.len(), 1, "{report:#?}"); - assert_eq!(report.downloaded[0].bytes, b"retry-media-bytes"); + let expired = ScriptedPortableMediaTransport::new([scripted_response( + StatusCode::OK, + None, + None, + Some(4), + Some("image/png"), + b"data".to_vec(), + )]); + assert_eq!( + fetch_hosted_media_outcome_with_policy( + &expired, + "https://secure.notion-static.com/image.png", + 1024, + std::time::Duration::ZERO, + std::time::Duration::ZERO, + ), + HostedMediaCaptureOutcome::Unavailable + ); + assert!(expired.requests().is_empty()); } - fn serve_test_media_response(mut stream: TcpStream, bytes: &[u8]) { - let mut request = [0_u8; 1024]; - let _ = stream.read(&mut request); - let headers = format!( - "HTTP/1.1 200 OK\r\nContent-Type: image/png\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", - bytes.len() + #[test] + fn hosted_media_terminal_4xx_is_not_retried() { + let transport = ScriptedPortableMediaTransport::new([ + scripted_response(StatusCode::NOT_FOUND, None, None, None, None, vec![]), + scripted_response( + StatusCode::OK, + None, + None, + Some(4), + Some("image/png"), + b"data".to_vec(), + ), + ]); + assert_eq!( + fetch_hosted_media_outcome_with_transport( + &transport, + "https://secure.notion-static.com/image.png", + 1024, + ), + HostedMediaCaptureOutcome::Unavailable ); - stream - .write_all(headers.as_bytes()) - .expect("write media response headers"); - stream.write_all(bytes).expect("write media response body"); + assert_eq!(transport.requests().len(), 1); } } diff --git a/crates/locality-notion/src/portable.rs b/crates/locality-notion/src/portable.rs index eacfde6b..cc326619 100644 --- a/crates/locality-notion/src/portable.rs +++ b/crates/locality-notion/src/portable.rs @@ -33,10 +33,10 @@ use crate::dto::{ }; use crate::fetch::fetch_known_page_bundle; use crate::media::{ - PORTABLE_MEDIA_MAX_AGGREGATE_BYTES, PORTABLE_MEDIA_MAX_ASSET_BYTES, PORTABLE_MEDIA_MAX_ASSETS, - PortableMediaCaptureFetcher, PortableMediaCapturePolicy, default_portable_media_fetcher, - portable_media_expired, sanitize_portable_hosted_media_url, sanitize_portable_media_type, - validate_portable_external_media_url, + HostedMediaCaptureOutcome, PORTABLE_MEDIA_MAX_AGGREGATE_BYTES, PORTABLE_MEDIA_MAX_ASSET_BYTES, + PORTABLE_MEDIA_MAX_ASSETS, PortableMediaCaptureFetcher, PortableMediaCapturePolicy, + default_portable_media_fetcher, portable_media_expired, sanitize_portable_hosted_media_url, + sanitize_portable_media_type, validate_portable_external_media_url, }; use crate::projection::enumerate_explicit_root_trees; use crate::render::{RenderOptions, render_native_entity, render_native_entity_with_options}; @@ -409,7 +409,7 @@ impl<'a> PortableMediaCaptureState<'a> { let sanitized_url = match sanitize_portable_hosted_media_url(&original_url) { Ok(url) => url, Err(_) => { - self.record_incomplete(block_id, kind, "unavailable_hosted_media"); + self.record_incomplete(block_id, kind, "unsafe_hosted_media"); return Ok(()); } }; @@ -421,29 +421,37 @@ impl<'a> PortableMediaCaptureState<'a> { return Ok(()); } Err(_) => { - self.record_incomplete(block_id, kind, "unavailable_hosted_media"); + self.record_incomplete(block_id, kind, "unsafe_hosted_media"); return Ok(()); } } } let captured = match self .fetcher - .fetch(&original_url, PORTABLE_MEDIA_MAX_ASSET_BYTES) + .fetch_outcome(&original_url, PORTABLE_MEDIA_MAX_ASSET_BYTES) { - Ok(captured) => captured, - Err(_) => { + HostedMediaCaptureOutcome::Captured(captured) => captured, + HostedMediaCaptureOutcome::Unavailable => { self.record_incomplete(block_id, kind, "unavailable_hosted_media"); return Ok(()); } + HostedMediaCaptureOutcome::TooLarge => { + self.record_incomplete(block_id, kind, "hosted_media_too_large"); + return Ok(()); + } + HostedMediaCaptureOutcome::Unsafe => { + self.record_incomplete(block_id, kind, "unsafe_hosted_media"); + return Ok(()); + } }; if captured.bytes.len() > PORTABLE_MEDIA_MAX_ASSET_BYTES { - self.record_incomplete(block_id, kind, "unavailable_hosted_media"); + self.record_incomplete(block_id, kind, "hosted_media_too_large"); return Ok(()); } let Some(aggregate_bytes) = checked_portable_media_aggregate(self.aggregate_bytes, captured.bytes.len()) else { - self.record_incomplete(block_id, kind, "unavailable_hosted_media"); + self.record_incomplete(block_id, kind, "hosted_media_too_large"); return Ok(()); }; self.aggregate_bytes = aggregate_bytes; @@ -981,7 +989,6 @@ fn render_portable_media_page( .iter() .map(|media| media.block_id.clone()) .collect::>(); - let mut external_block_ids = portable_external_media_block_ids(&bundle.page.blocks); let page_native = NativeEntity { remote_id: request.native.remote_id.clone(), kind: "notion_page".to_string(), @@ -1030,9 +1037,6 @@ fn render_portable_media_page( let mut projected_paths = BTreeSet::new(); for rendered_asset in rendered.media_assets { let Some(captured) = captured_by_block.remove(&rendered_asset.block_id) else { - if external_block_ids.remove(&rendered_asset.block_id) { - continue; - } return Err(LocalityError::InvalidState( "Notion portable media render produced an uncaptured asset".to_string(), )); @@ -1083,12 +1087,6 @@ fn render_portable_media_page( "Notion portable media native payload contains an unrendered asset".to_string(), )); } - if !external_block_ids.is_empty() { - return Err(LocalityError::InvalidState( - "Notion portable external media did not render as a reference".to_string(), - )); - } - Ok(PortableRenderResult { canonical, projections, @@ -1208,7 +1206,9 @@ fn validate_portable_media_bundle(bundle: &NotionPortablePageBundleV1) -> Locali let code = match (payload.external.is_some(), payload.file.is_some()) { (true, true) => "ambiguous_file_source", (true, false) => "invalid_external_media", - (false, true) => "unavailable_hosted_media", + (false, true) => { + actual_hosted_incomplete_code(&bundle.incomplete_media, block_id, kind)? + } (false, false) => "missing_file", }; insert_expected_incomplete(&mut expected_incomplete, block_id, kind, code)?; @@ -1306,27 +1306,25 @@ fn validate_portable_media_bundle(bundle: &NotionPortablePageBundleV1) -> Locali Ok(()) } -fn portable_external_media_block_ids(trees: &[BlockTreeDto]) -> BTreeSet { - fn collect(trees: &[BlockTreeDto], block_ids: &mut BTreeSet) { - for tree in trees { - if is_media_kind(&tree.block.kind) - && media_payload(&tree.block).is_some_and(|payload| { - payload.file.is_none() - && payload.kind == "external" - && payload.external.as_ref().is_some_and(|external| { - validate_portable_external_media_url(&external.url).is_ok() - }) - }) - { - block_ids.insert(tree.block.id.clone()); - } - collect(&tree.children, block_ids); +fn actual_hosted_incomplete_code<'a>( + incomplete: &'a [NotionPortableIncompleteMediaV1], + block_id: &str, + kind: &str, +) -> LocalityResult<&'a str> { + let Some(outcome) = incomplete + .iter() + .find(|outcome| outcome.block_id == block_id && outcome.kind == kind) + else { + return Ok("unavailable_hosted_media"); + }; + match outcome.code.as_str() { + "unavailable_hosted_media" | "hosted_media_too_large" | "unsafe_hosted_media" => { + Ok(&outcome.code) } + _ => Err(LocalityError::InvalidState( + "Notion portable media native payload has invalid incomplete outcomes".to_string(), + )), } - - let mut block_ids = BTreeSet::new(); - collect(trees, &mut block_ids); - block_ids } fn insert_expected_incomplete( @@ -2074,7 +2072,7 @@ mod tests { .expect("aggregate capture"); assert!(state.captured.is_empty()); - assert_eq!(state.incomplete[0].code, "unavailable_hosted_media"); + assert_eq!(state.incomplete[0].code, "hosted_media_too_large"); assert_eq!(payload.file.expect("hosted").url, ""); } } diff --git a/crates/locality-notion/src/render.rs b/crates/locality-notion/src/render.rs index cc0a898f..d9794123 100644 --- a/crates/locality-notion/src/render.rs +++ b/crates/locality-notion/src/render.rs @@ -13,7 +13,10 @@ use crate::dto::{ MeetingNotesBlockDto, NotionPageBundle, PageDto, PagePropertyDto, RichTextBlockDto, RichTextDto, SyncedBlockDto, TableBlockDto, TableRowBlockDto, UrlBlockDto, }; -use crate::media::{MediaAsset, is_downloadable_url, local_media_href, media_local_path}; +use crate::media::{ + MediaAsset, local_media_href, media_local_path, portable_media_expired, + validate_portable_external_media_url, +}; #[derive(Clone, Debug, PartialEq, Eq)] pub struct NotionRenderedEntity { @@ -74,6 +77,9 @@ pub fn render_page_bundle_with_options( bundle: &NotionPageBundle, options: &RenderOptions, ) -> LocalityResult { + if options.page_path.is_some() { + validate_projected_hosted_media_metadata(&bundle.blocks)?; + } let title = page_title(&bundle.page); let frontmatter = page_frontmatter(&bundle.page, &title); let mut rendered_blocks = Vec::new(); @@ -106,6 +112,31 @@ pub fn render_page_bundle_with_options( }) } +fn validate_projected_hosted_media_metadata(trees: &[BlockTreeDto]) -> LocalityResult<()> { + for tree in trees { + let payload = match tree.block.kind.as_str() { + "image" => tree.block.image.as_ref(), + "video" => tree.block.video.as_ref(), + "file" => tree.block.file.as_ref(), + "pdf" => tree.block.pdf.as_ref(), + "audio" => tree.block.audio.as_ref(), + _ => None, + }; + if let Some(expiry) = payload + .filter(|payload| payload.kind == "file" && payload.external.is_none()) + .and_then(|payload| payload.file.as_ref()) + .and_then(|hosted| hosted.expiry_time.as_deref()) + && portable_media_expired(expiry).is_err() + { + return Err(LocalityError::InvalidState( + "Notion hosted media expiry failed safety validation".to_string(), + )); + } + validate_projected_hosted_media_metadata(&tree.children)?; + } + Ok(()) +} + #[derive(Clone, Debug, PartialEq, Eq)] struct RenderedBlock { markdown: String, @@ -477,44 +508,81 @@ fn file_media_block( options: &RenderOptions, ) -> RenderedBlock { let mut attrs = Vec::new(); - let mut media_asset = None; - if let Some(payload) = payload { let title = rich_text_list_title(&payload.caption); if let Some(title) = title.clone() { attrs.push(("title", title)); } - if let Some(url) = file_url(payload) { - let mut markdown_url = url.clone(); - if is_downloadable_url(&url) - && let Some(page_path) = options.page_path.as_deref() - { - let local_path = media_local_path(page_path, &block.id, media_type, &url); - if options.use_local_media_for(&block.id) { - markdown_url = local_media_href(page_path, &local_path); + let label = title.unwrap_or_else(|| media_default_label(media_type).to_string()); + + // Public external references are links, never downloadable projection + // assets. Preserve the exact provider spelling after validating the + // HTTPS-only reference boundary. + if payload.kind == "external" + && payload.file.is_none() + && let Some(url) = payload.external.as_ref().map(|external| &external.url) + && validate_portable_external_media_url(url).is_ok() + { + return rendered_media_link(block, media_type, &label, url, None); + } + + // Hosted files become local only when their bytes were captured. A + // caller-provided local-media set is authoritative: omission renders + // the same URL-free directive used by portable/cloud output. + if payload.kind == "file" + && payload.external.is_none() + && let Some(url) = payload + .file + .as_ref() + .map(|hosted| &hosted.url) + .filter(|url| !url.is_empty()) + { + if let Some(page_path) = options.page_path.as_deref() { + if payload + .file + .as_ref() + .and_then(|hosted| hosted.expiry_time.as_deref()) + .is_some_and(|expiry| portable_media_expired(expiry) != Ok(false)) + { + return directive_block_with_attrs(block, media_type, attrs); } - media_asset = Some(MediaAsset { + let local_path = media_local_path(page_path, &block.id, media_type, url); + if !options.use_local_media_for(&block.id) { + return directive_block_with_attrs(block, media_type, attrs); + } + let href = local_media_href(page_path, &local_path); + let asset = MediaAsset { block_id: block.id.clone(), kind: media_type.to_string(), source_url: url.clone(), local_path, - }); + }; + return rendered_media_link(block, media_type, &label, &href, Some(asset)); } - let label = title.unwrap_or_else(|| media_default_label(media_type).to_string()); - let markdown_url = escape_markdown_link_href(&markdown_url); - let markdown = if media_type == "image" { - format!("![{}]({markdown_url})", escape_markdown_link_label(&label)) - } else { - format!("[{}]({markdown_url})", escape_markdown_link_label(&label)) - }; - let mut rendered = rendered_block(markdown, Some(RemoteId::new(block.id.clone()))); - rendered.media_asset = media_asset; - return rendered; + // Preserve the legacy direct/native V1 render shape when no + // projection path (and therefore no local media path) is supplied. + return rendered_media_link(block, media_type, &label, url, None); } } - let mut rendered = directive_block_with_attrs(block, media_type, attrs); + directive_block_with_attrs(block, media_type, attrs) +} + +fn rendered_media_link( + block: &BlockDto, + media_type: &str, + label: &str, + url: &str, + media_asset: Option, +) -> RenderedBlock { + let markdown_url = escape_markdown_link_href(url); + let markdown = if media_type == "image" { + format!("![{}]({markdown_url})", escape_markdown_link_label(label)) + } else { + format!("[{}]({markdown_url})", escape_markdown_link_label(label)) + }; + let mut rendered = rendered_block(markdown, Some(RemoteId::new(block.id.clone()))); rendered.media_asset = media_asset; rendered } @@ -724,14 +792,6 @@ fn rich_text_list_title(rich_text: &[RichTextDto]) -> Option { } } -fn file_url(file: &FileBlockDto) -> Option { - file.external - .as_ref() - .map(|external| external.url.clone()) - .or_else(|| file.file.as_ref().map(|file| file.url.clone())) - .filter(|url| !url.is_empty()) -} - fn render_table_tree(tree: &BlockTreeDto) -> Option { let table = tree.block.table.as_ref()?; let rows = table_rows(&tree.children)?; diff --git a/crates/locality-notion/tests/fetch_render.rs b/crates/locality-notion/tests/fetch_render.rs index 38a73bc6..866e7201 100644 --- a/crates/locality-notion/tests/fetch_render.rs +++ b/crates/locality-notion/tests/fetch_render.rs @@ -1099,11 +1099,11 @@ fn render_all_known_notion_block_objects_into_markdown_or_directives() { "[Embed](https://example.com/embed)", "[Bookmark](https://example.com/bookmark)", "[Preview](https://example.com/preview)", - "![Image](../.loc/media/Docs/Coverage/image-111111111111aaaa.png)", - "[Video](../.loc/media/Docs/Coverage/video-222222222222bbbb.mp4)", - "[File](../.loc/media/Docs/Coverage/file-333333333333cccc.txt)", - "[PDF](../.loc/media/Docs/Coverage/pdf-444444444444dddd.pdf)", - "[Audio](../.loc/media/Docs/Coverage/audio-555555555555eeee.mp3)", + "![Image](https://example.com/image.png)", + "[Video](https://example.com/video.mp4)", + "[File](https://example.com/file.txt)", + "[PDF](https://example.com/file.pdf)", + "[Audio](https://example.com/audio.mp3)", "::loc{id=synced-original-1 type=synced_block}", "::loc{id=synced-copy-1 type=synced_block source_block_id=\"source-block-1\"}", "[Linked page](https://www.notion.so/target-page-1)", @@ -1130,14 +1130,7 @@ fn render_all_known_notion_block_objects_into_markdown_or_directives() { ); } - assert_eq!( - rendered - .media_assets - .iter() - .map(|asset| asset.kind.as_str()) - .collect::>(), - vec!["image", "video", "file", "pdf", "audio"] - ); + assert!(rendered.media_assets.is_empty()); let link_preview_shadow = rendered .shadow .blocks @@ -1294,10 +1287,10 @@ fn render_media_blocks_as_markdown_links_and_tracks_local_paths() { let bundle = locality_notion::dto::NotionPageBundle { page: page("page-1", "Coverage"), blocks: vec![BlockTreeDto { - block: file_block( + block: hosted_file_block_with_caption( "0123456789abcdef", "image", - "https://example.com/image.PNG?download=1", + "https://secure.notion-static.com/image.PNG?download=1", "Image caption", ), children: Vec::new(), @@ -1327,37 +1320,37 @@ fn render_file_like_media_blocks_as_local_links_when_downloaded() { page: page("page-1", "Coverage"), blocks: vec![ BlockTreeDto { - block: file_block( + block: hosted_file_block_with_caption( "1111111111111111", "video", - "https://example.com/cars.MP4?download=1", + "https://secure.notion-static.com/cars.MP4?download=1", "Cars", ), children: Vec::new(), }, BlockTreeDto { - block: file_block( + block: hosted_file_block_with_caption( "2222222222222222", "pdf", - "https://example.com/brief.PDF?download=1", + "https://secure.notion-static.com/brief.PDF?download=1", "Brief", ), children: Vec::new(), }, BlockTreeDto { - block: file_block( + block: hosted_file_block_with_caption( "3333333333333333", "audio", - "https://example.com/theme.MP3?download=1", + "https://secure.notion-static.com/theme.MP3?download=1", "Theme", ), children: Vec::new(), }, BlockTreeDto { - block: file_block( + block: hosted_file_block_with_caption( "4444444444444444", "file", - "https://example.com/index.HTML?download=1", + "https://secure.notion-static.com/index.HTML?download=1", "Index", ), children: Vec::new(), @@ -1403,14 +1396,14 @@ fn render_file_like_media_blocks_as_local_links_when_downloaded() { } #[test] -fn render_media_blocks_can_keep_failed_downloads_as_remote_urls() { +fn render_failed_hosted_media_as_url_free_directive() { let bundle = locality_notion::dto::NotionPageBundle { page: page("page-1", "Coverage"), blocks: vec![BlockTreeDto { - block: file_block( + block: hosted_file_block_with_caption( "0123456789abcdef", "image", - "https://example.com/image.PNG?download=1", + "https://secure.notion-static.com/image.PNG?X-Amz-Signature=secret", "Image caption", ), children: Vec::new(), @@ -1424,15 +1417,12 @@ fn render_media_blocks_can_keep_failed_downloads_as_remote_urls() { ) .expect("render"); - assert_eq!(rendered.media_assets.len(), 1); - assert_eq!( - rendered.media_assets[0].local_path, - Path::new(".loc/media/Docs/Coverage/image-0123456789abcdef.png") - ); + assert!(rendered.media_assets.is_empty()); assert_eq!( rendered.document.body, - "![Image caption](https://example.com/image.PNG?download=1)\n" + "::loc{id=0123456789abcdef type=image title=\"Image caption\"}\n" ); + assert!(!rendered.document.body.contains("X-Amz")); assert_eq!(rendered.shadow.rendered_body, rendered.document.body); } @@ -1453,12 +1443,12 @@ fn render_relative_media_url_without_local_download_asset() { .expect("render"); assert!(rendered.media_assets.is_empty()); - assert_eq!(rendered.document.body, "![Image](img_2.png)\n"); + assert_eq!(rendered.document.body, "::loc{id=image-1 type=image}\n"); assert_eq!(rendered.shadow.blocks.len(), 1); - assert_eq!( - rendered.shadow.blocks[0].remote_id, - RemoteId::new("image-1") - ); + assert!(matches!( + rendered.shadow.blocks[0].kind, + MarkdownBlockKind::Directive { .. } + )); } #[test] @@ -3341,6 +3331,112 @@ fn portable_external_media_supports_every_file_like_block_without_capture() { assert!(rendered.completeness.is_complete()); } +#[test] +fn mixed_hosted_success_omission_and_external_match_desktop_renderer_exactly() { + let page_id = "mixed-media-page"; + let captured_id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let omitted_id = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + let captured_url = + "https://secure.notion-static.com/captured.png?X-Amz-Signature=captured-secret"; + let omitted_url = "https://secure.notion-static.com/omitted.png?X-Amz-Signature=omitted-secret"; + let external_url = "https://cdn.example.com/public.png?spelling=Exact#fragment"; + let fetcher = Arc::new(FixturePortableMediaFetcher { + outcomes: BTreeMap::from([ + ( + captured_url.to_string(), + FixturePortableMediaOutcome::Success(PortableMediaCapture { + bytes: b"captured-bytes".to_vec(), + media_type: "image/png".to_string(), + }), + ), + ( + omitted_url.to_string(), + FixturePortableMediaOutcome::Failure("temporary transport failure".to_string()), + ), + ]), + calls: Arc::new(Mutex::new(Vec::new())), + }); + let connector = portable_media_connector( + page_id, + vec![ + hosted_file_block(captured_id, "image", captured_url, None), + hosted_file_block(omitted_id, "image", omitted_url, None), + file_block("external-media", "image", external_url, "Public image"), + ], + ) + .with_portable_media_capture_fetcher(PortableMediaCapturePolicy::HostedPilot, fetcher); + + let fetched = connector + .fetch_portable(portable_fetch_request(page_id)) + .expect("mixed portable fetch"); + assert!( + fetched + .completeness + .incomplete_reasons() + .iter() + .any(|reason| { + matches!(reason, PortableIncompleteReason::ConnectorLimitation { code, remote_id } + if code == "notion_media_unavailable_hosted_media" + && remote_id.as_ref() == Some(&RemoteId::new(omitted_id))) + }) + ); + let native: NotionPortablePageBundleV1 = + serde_json::from_slice(&fetched.native.raw).expect("mixed native"); + assert_eq!(native.captured_media.len(), 1); + assert_eq!( + native.incomplete_media, + vec![NotionPortableIncompleteMediaV1 { + block_id: omitted_id.to_string(), + kind: "image".to_string(), + code: "unavailable_hosted_media".to_string(), + }] + ); + + let desktop = locality_notion::render::render_page_bundle_with_options( + &native.page, + &locality_notion::render::RenderOptions::with_page_path("Docs/Mixed/page.md") + .with_local_media_block_ids([captured_id.to_string()]), + ) + .expect("desktop shared render"); + assert_eq!(desktop.media_assets.len(), 1); + assert_eq!( + desktop.media_assets[0].local_path, + Path::new(".loc/media/Docs/Mixed/image-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png") + ); + assert_eq!(desktop.media_assets[0].block_id, captured_id); + + let rendered = connector + .render_portable(&PortableRenderRequest { + source_connection_id: SourceConnectionId::new("source-notion"), + logical_path: LogicalPath::new("Docs/Mixed/page.md").expect("mixed logical path"), + native: fetched.native, + format_version: 1, + }) + .expect("mixed portable render"); + let desktop_bytes = render_canonical_markdown(&desktop.document).into_bytes(); + assert_eq!(rendered.canonical.body, desktop_bytes); + assert_eq!(rendered.projections.len(), 2); + assert_eq!( + rendered.projections[1].logical_path.as_str(), + ".loc/media/Docs/Mixed/image-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png" + ); + assert_eq!(rendered.projections[1].artifact.body, b"captured-bytes"); + let markdown = String::from_utf8(desktop_bytes).expect("UTF-8 markdown"); + assert!(markdown.contains( + "![Image](../../.loc/media/Docs/Mixed/image-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png)" + )); + assert!(markdown.contains("::loc{id=bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb type=image}")); + assert!(markdown.contains(&format!("![Public image]({external_url})"))); + for forbidden in [ + captured_url, + omitted_url, + "captured-secret", + "omitted-secret", + ] { + assert!(!markdown.contains(forbidden)); + } +} + #[test] fn portable_capture_keeps_pages_without_media_byte_exact() { let page_id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; @@ -3404,6 +3500,21 @@ fn portable_media_denials_are_incomplete_and_never_publish_remote_urls() { .fetch_portable(portable_fetch_request(&page_id)) .unwrap_or_else(|error| panic!("{case} fetch failed closed unexpectedly: {error}")); assert!(!fetched.completeness.is_complete(), "{case}"); + assert!(fetched.completeness.incomplete_reasons().iter().any(|reason| { + matches!(reason, PortableIncompleteReason::ConnectorLimitation { code, remote_id } + if code == "notion_media_unsafe_hosted_media" + && remote_id.as_ref() == Some(&RemoteId::new(block_id.clone()))) + })); + let native: NotionPortablePageBundleV1 = + serde_json::from_slice(&fetched.native.raw).expect("denied native"); + assert_eq!( + native.incomplete_media, + vec![NotionPortableIncompleteMediaV1 { + block_id: block_id.clone(), + kind: "image".to_string(), + code: "unsafe_hosted_media".to_string(), + }] + ); let raw = String::from_utf8_lossy(&fetched.native.raw); assert!(!raw.contains(url), "{case}: {raw}"); let rendered = connector @@ -3595,6 +3706,7 @@ fn portable_media_expired_failed_and_oversized_captures_are_redacted() { let cases = [ ( "expired", + "unavailable_hosted_media", Some("2000-01-01T00:00:00.000Z"), FixturePortableMediaOutcome::Success(PortableMediaCapture { bytes: b"unused".to_vec(), @@ -3603,6 +3715,7 @@ fn portable_media_expired_failed_and_oversized_captures_are_redacted() { ), ( "failed", + "unavailable_hosted_media", Some("2099-01-01T00:00:00.000Z"), FixturePortableMediaOutcome::Failure( "X-Amz-Signature=must-not-escape token-secret".to_string(), @@ -3610,14 +3723,24 @@ fn portable_media_expired_failed_and_oversized_captures_are_redacted() { ), ( "oversized", + "hosted_media_too_large", Some("2099-01-01T00:00:00.000Z"), FixturePortableMediaOutcome::Success(PortableMediaCapture { bytes: vec![0; PORTABLE_MEDIA_MAX_ASSET_BYTES + 1], media_type: "image/png".to_string(), }), ), + ( + "malformed-expiry", + "unsafe_hosted_media", + Some("2099-01-01T00:00:00+01:00"), + FixturePortableMediaOutcome::Success(PortableMediaCapture { + bytes: b"unused".to_vec(), + media_type: "image/png".to_string(), + }), + ), ]; - for (case, expiry, outcome) in cases { + for (case, expected_code, expiry, outcome) in cases { let page_id = format!("page-{case}"); let block_id = format!("block-{case}"); let url = @@ -3635,6 +3758,21 @@ fn portable_media_expired_failed_and_oversized_captures_are_redacted() { .fetch_portable(portable_fetch_request(&page_id)) .unwrap_or_else(|error| panic!("{case}: {error}")); assert!(!fetched.completeness.is_complete(), "{case}"); + assert!(fetched.completeness.incomplete_reasons().iter().any(|reason| { + matches!(reason, PortableIncompleteReason::ConnectorLimitation { code, remote_id } + if code == &format!("notion_media_{expected_code}") + && remote_id.as_ref() == Some(&RemoteId::new(block_id.clone()))) + })); + let native: NotionPortablePageBundleV1 = + serde_json::from_slice(&fetched.native.raw).expect("incomplete native"); + assert_eq!( + native.incomplete_media, + vec![NotionPortableIncompleteMediaV1 { + block_id: block_id.clone(), + kind: "image".to_string(), + code: expected_code.to_string(), + }] + ); let raw = String::from_utf8_lossy(&fetched.native.raw); assert!(!raw.contains("X-Amz-Signature"), "{case}"); assert!(!raw.contains("signature-secret"), "{case}"); @@ -5704,6 +5842,21 @@ fn hosted_file_block(id: &str, kind: &str, url: &str, expiry_time: Option<&str>) block } +fn hosted_file_block_with_caption(id: &str, kind: &str, url: &str, caption: &str) -> BlockDto { + let mut block = hosted_file_block(id, kind, url, None); + let payload = match kind { + "image" => block.image.as_mut(), + "video" => block.video.as_mut(), + "file" => block.file.as_mut(), + "pdf" => block.pdf.as_mut(), + "audio" => block.audio.as_mut(), + _ => None, + } + .expect("hosted media payload"); + payload.caption = vec![rich_text(caption)]; + block +} + fn synced_block(id: &str, source_block_id: &str) -> BlockDto { let mut block = block(id, "synced_block"); block.synced_block = Some(SyncedBlockDto { diff --git a/crates/localityd/src/notion.rs b/crates/localityd/src/notion.rs index 26253c66..ac07eb63 100644 --- a/crates/localityd/src/notion.rs +++ b/crates/localityd/src/notion.rs @@ -9,7 +9,7 @@ use locality_core::validation::{ValidationIssue, ValidationReport}; use locality_core::{LocalityError, LocalityResult}; use locality_notion::client::DEFAULT_NOTION_TOKEN_ENV; use locality_notion::dto::NotionPageBundle; -use locality_notion::media::fetch_media_asset_report; +use locality_notion::media::HostedMediaFailureKind; use locality_notion::oauth::{ HttpNotionOAuthBrokerClient, HttpNotionOAuthClient, NotionOAuthBrokerRefresh, NotionOAuthRefresh, StoredNotionCredential, @@ -577,7 +577,16 @@ impl HydrationSource for NotionConnector { let bundle = serde_json::from_slice::(&native.raw).map_err(|error| { locality_core::LocalityError::Io(format!("notion native decode failed: {error}")) })?; - let fetched = fetch_media_asset_report(&rendered.media_assets); + let fetched = self.fetch_rendered_media(&rendered); + if fetched + .failed + .iter() + .any(|failure| failure.outcome() == HostedMediaFailureKind::Unsafe) + { + return Err(LocalityError::InvalidState( + "Notion hosted media failed safety validation".to_string(), + )); + } if !fetched.failed.is_empty() { let local_media_block_ids = fetched .downloaded diff --git a/crates/localityd/tests/notion_hydration.rs b/crates/localityd/tests/notion_hydration.rs index d131d81e..00760ea0 100644 --- a/crates/localityd/tests/notion_hydration.rs +++ b/crates/localityd/tests/notion_hydration.rs @@ -8,8 +8,12 @@ use locality_core::hydration::{HydrationReason, HydrationRequest}; use locality_core::model::{EntityKind, HydrationState, MountId, RemoteId}; use locality_notion::client::NotionApi; use locality_notion::dto::{ - BlockDto, BlockListDto, PageDto, PageListDto, PagePropertyDto, PaginatedListDto, - RichTextBlockDto, RichTextDto, TextRichTextDto, + BlockDto, BlockListDto, ExternalFileDto, FileBlockDto, HostedFileDto, PageDto, PageListDto, + PagePropertyDto, PaginatedListDto, RichTextBlockDto, RichTextDto, TextRichTextDto, +}; +use locality_notion::media::{ + HostedMediaCaptureOutcome, PortableMediaCapture, PortableMediaCaptureFetcher, + PortableMediaCapturePolicy, }; use locality_notion::{NotionConfig, NotionConnector}; use locality_store::{ @@ -65,6 +69,238 @@ fn notion_connector_hydrates_stub_through_daemon_executor() { assert_eq!(entity.content_hash, Some(shadow.body_hash)); } +#[test] +fn notion_hydration_publishes_mixed_hosted_omission_and_external_reference() { + let hosted_ok = "https://secure.notion-static.com/ok.png?X-Amz-Signature=ok-secret"; + let hosted_missing = + "https://secure.notion-static.com/missing.png?X-Amz-Signature=missing-secret"; + let external = "https://cdn.example.com/public.png?version=exact#image"; + let connector = NotionConnector::with_api( + NotionConfig::default(), + Arc::new(FixtureNotionApi::page_with_blocks( + "page-1", + "Mixed Media", + vec![ + hosted_media_block("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", hosted_ok, "Available"), + hosted_media_block( + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + hosted_missing, + "Missing", + ), + external_media_block("cccccccccccccccccccccccccccccccc", external, "Public"), + ], + )), + ) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + Arc::new(FixtureMediaFetcher(BTreeMap::from([ + ( + hosted_ok.to_string(), + HostedMediaCaptureOutcome::Captured(PortableMediaCapture { + bytes: b"png".to_vec(), + media_type: "image/png".to_string(), + }), + ), + ( + hosted_missing.to_string(), + HostedMediaCaptureOutcome::Unavailable, + ), + ]))), + ); + let request = HydrationRequest::new( + MountId::new("notion-main"), + RemoteId::new("page-1"), + "Docs/Coverage/page.md", + HydrationState::Stub, + HydrationReason::StubRead, + ); + + let hydrated = localityd::hydration::HydrationSource::fetch_render(&connector, &request) + .expect("mixed media hydration"); + assert_eq!(hydrated.assets.len(), 1); + assert_eq!(hydrated.assets[0].bytes, b"png"); + assert_eq!( + hydrated.assets[0].path, + PathBuf::from(".loc/media/Docs/Coverage/image-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png") + ); + let markdown = locality_core::canonical::render_canonical_markdown(&hydrated.document); + assert!(markdown.contains( + "![Available](../../.loc/media/Docs/Coverage/image-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png)" + )); + assert!( + markdown + .contains("::loc{id=bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb type=image title=\"Missing\"}") + ); + assert!(markdown.contains(&format!("![Public]({external})"))); + assert!(!markdown.contains(hosted_ok)); + assert!(!markdown.contains(hosted_missing)); + assert!(!markdown.contains("missing-secret")); +} + +#[test] +fn notion_hydration_fails_closed_for_unsafe_hosted_media() { + let hosted = "https://secure.notion-static.com/unsafe.png?X-Amz-Signature=secret"; + let connector = NotionConnector::with_api( + NotionConfig::default(), + Arc::new(FixtureNotionApi::page_with_blocks( + "page-1", + "Unsafe Media", + vec![hosted_media_block("unsafe", hosted, "Unsafe")], + )), + ) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + Arc::new(FixtureMediaFetcher(BTreeMap::from([( + hosted.to_string(), + HostedMediaCaptureOutcome::Unsafe, + )]))), + ); + let request = HydrationRequest::new( + MountId::new("notion-main"), + RemoteId::new("page-1"), + "Unsafe/page.md", + HydrationState::Stub, + HydrationReason::StubRead, + ); + + let error = localityd::hydration::HydrationSource::fetch_render(&connector, &request) + .expect_err("unsafe hosted media must fail closed"); + assert_eq!( + error.to_string(), + "invalid state: Notion hosted media failed safety validation" + ); + assert!(!format!("{error:?}").contains("X-Amz")); + assert!(!format!("{error:?}").contains("secret")); +} + +#[test] +fn notion_hydration_rejects_invalid_hosted_origin_before_injected_fetcher() { + struct NeverCalledFetcher; + impl PortableMediaCaptureFetcher for NeverCalledFetcher { + fn fetch( + &self, + _hosted_url: &str, + _max_bytes: usize, + ) -> locality_core::LocalityResult { + panic!("invalid hosted origin reached injected fetcher") + } + } + + let connector = NotionConnector::with_api( + NotionConfig::default(), + Arc::new(FixtureNotionApi::page_with_blocks( + "page-1", + "Invalid Hosted Origin", + vec![hosted_media_block( + "unsafe-origin", + "https://example.com/masquerading-as-hosted.png", + "Unsafe", + )], + )), + ) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + Arc::new(NeverCalledFetcher), + ); + let request = HydrationRequest::new( + MountId::new("notion-main"), + RemoteId::new("page-1"), + "Unsafe/page.md", + HydrationState::Stub, + HydrationReason::StubRead, + ); + + let error = localityd::hydration::HydrationSource::fetch_render(&connector, &request) + .expect_err("invalid hosted origin must fail closed"); + assert_eq!( + error.to_string(), + "invalid state: Notion hosted media failed safety validation" + ); +} + +#[test] +fn notion_hydration_fails_closed_for_malformed_hosted_expiry() { + let connector = NotionConnector::with_api( + NotionConfig::default(), + Arc::new(FixtureNotionApi::page_with_blocks( + "page-1", + "Malformed Expiry", + vec![hosted_media_block_with_expiry( + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "https://secure.notion-static.com/image.png?X-Amz-Signature=secret", + "Image", + Some("2099-01-01T00:00:00+01:00"), + )], + )), + ); + let request = HydrationRequest::new( + MountId::new("notion-main"), + RemoteId::new("page-1"), + "Unsafe/page.md", + HydrationState::Stub, + HydrationReason::StubRead, + ); + + let error = localityd::hydration::HydrationSource::fetch_render(&connector, &request) + .expect_err("malformed expiry must fail closed"); + assert_eq!( + error.to_string(), + "invalid state: Notion hosted media expiry failed safety validation" + ); + assert!(!format!("{error:?}").contains("X-Amz")); + assert!(!format!("{error:?}").contains("secret")); +} + +#[test] +fn notion_hydration_omits_already_expired_hosted_media_without_fetching() { + struct NeverCalledFetcher; + impl PortableMediaCaptureFetcher for NeverCalledFetcher { + fn fetch( + &self, + _hosted_url: &str, + _max_bytes: usize, + ) -> locality_core::LocalityResult { + panic!("expired hosted media reached fetcher") + } + } + + let connector = NotionConnector::with_api( + NotionConfig::default(), + Arc::new(FixtureNotionApi::page_with_blocks( + "page-1", + "Expired Media", + vec![hosted_media_block_with_expiry( + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "https://secure.notion-static.com/image.png?X-Amz-Signature=secret", + "Expired", + Some("2000-01-01T00:00:00.000Z"), + )], + )), + ) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + Arc::new(NeverCalledFetcher), + ); + let request = HydrationRequest::new( + MountId::new("notion-main"), + RemoteId::new("page-1"), + "Expired/page.md", + HydrationState::Stub, + HydrationReason::StubRead, + ); + + let hydrated = localityd::hydration::HydrationSource::fetch_render(&connector, &request) + .expect("expired hosted media is an omission"); + assert!(hydrated.assets.is_empty()); + let markdown = locality_core::canonical::render_canonical_markdown(&hydrated.document); + assert!( + markdown + .contains("::loc{id=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa type=image title=\"Expired\"}") + ); + assert!(!markdown.contains("https://secure.notion-static.com")); + assert!(!markdown.contains("X-Amz")); +} + #[test] #[ignore = "requires NOTION_TOKEN and access to the target Notion page"] fn live_notion_hydration_source_fetches_codeflash_home_page() { @@ -291,6 +527,72 @@ fn rich_text_block(id: &str, kind: &str, text: &str) -> BlockDto { block } +fn hosted_media_block(id: &str, url: &str, caption: &str) -> BlockDto { + hosted_media_block_with_expiry(id, url, caption, Some("2099-01-01T00:00:00.000Z")) +} + +fn hosted_media_block_with_expiry( + id: &str, + url: &str, + caption: &str, + expiry_time: Option<&str>, +) -> BlockDto { + BlockDto { + id: id.to_string(), + kind: "image".to_string(), + image: Some(FileBlockDto { + kind: "file".to_string(), + external: None, + file: Some(HostedFileDto { + url: url.to_string(), + expiry_time: expiry_time.map(str::to_string), + }), + caption: vec![rich_text(caption)], + }), + ..BlockDto::default() + } +} + +fn external_media_block(id: &str, url: &str, caption: &str) -> BlockDto { + BlockDto { + id: id.to_string(), + kind: "image".to_string(), + image: Some(FileBlockDto { + kind: "external".to_string(), + external: Some(ExternalFileDto { + url: url.to_string(), + }), + file: None, + caption: vec![rich_text(caption)], + }), + ..BlockDto::default() + } +} + +struct FixtureMediaFetcher(BTreeMap); + +impl PortableMediaCaptureFetcher for FixtureMediaFetcher { + fn fetch( + &self, + hosted_url: &str, + _max_bytes: usize, + ) -> locality_core::LocalityResult { + match self.fetch_outcome(hosted_url, usize::MAX) { + HostedMediaCaptureOutcome::Captured(capture) => Ok(capture), + _ => Err(locality_core::LocalityError::Io( + "fixture hosted media unavailable".to_string(), + )), + } + } + + fn fetch_outcome(&self, hosted_url: &str, _max_bytes: usize) -> HostedMediaCaptureOutcome { + self.0 + .get(hosted_url) + .cloned() + .unwrap_or(HostedMediaCaptureOutcome::Unavailable) + } +} + fn rich_text(text: &str) -> RichTextDto { RichTextDto { kind: "text".to_string(), diff --git a/docs/notion-canonical-format.md b/docs/notion-canonical-format.md index 9ecbfc04..bd52d69c 100644 --- a/docs/notion-canonical-format.md +++ b/docs/notion-canonical-format.md @@ -29,7 +29,7 @@ Directive integrity is validated before push. Agents may move directive lines as The first renderer supports common text blocks, richer inline text, display equations, simple tables, bookmark/embed/link-preview URL blocks, child-page links, and file-like media blocks. Inline bold, italic, strikethrough, code, external links, date mentions, page/database mentions, link previews, and equations use ordinary Markdown or small HTML fallbacks when Markdown has no native equivalent. Child pages render as normal Markdown links whose URL contains the stable Notion page ID, for example `[Design Notes](https://www.notion.so/...)`; Locality can use that URL to locate the mounted child page, and the child page itself is edited through its own Markdown file. Child databases, toggles, synced blocks, column layouts, tabs, meeting notes, AI/custom blocks, URL-less media payloads, and unsupported/lossy blocks render as directives. Notion API `unsupported` blocks whose `unsupported.block_type` is only a subtype-only UI artifact, such as `copy_indicator`, `button`, or `alias`, are omitted from Markdown. Other Notion API `unsupported` blocks include a protected directive title, for example `::loc{id=... type=unsupported title="Unsupported Notion block"}`. This keeps the page inspectable while preserving remote block IDs for later safer round-trip support where the API exposes meaningful block content. -Media blocks with a Notion `file.url` or `external.url` render as ordinary Markdown. Images use image syntax, while other file-like blocks use links. When Locality writes a page into a local projection, downloadable file-like media links point at the absolute local media file under the projection output root instead of the remote Notion/S3 URL. For virtual projections this is the daemon content cache: +Media blocks with a valid Notion `file.url` or external-only HTTPS `external.url` render as ordinary Markdown. Images use image syntax, while other file-like blocks use links. External-only HTTPS media remains the exact remote reference and is never downloaded or projected as a binary. When Locality successfully captures Notion-hosted media for a local or portable projection, its link uses the shared mount-relative `.loc/media/` path instead of the signed Notion/S3 URL. For virtual projections this is the daemon content cache: Read-side compatibility also accepts legacy `afs:` identity frontmatter and `::afs{...}` directives from pre-rename projections. Normal Locality rewrites continue to emit `loc:` and `::loc{...}` spellings. @@ -38,7 +38,7 @@ Read-side compatibility also accepts legacy `afs:` identity frontmatter and `::a [Design brief](/home/user/.loc/content/notion-mount-1/files/.loc/media/roadmap/pdf-abcdef1234567890.pdf) ``` -Filesystem-aware pull, hydration, and post-push reconcile paths download image, video, PDF, audio, and generic file blocks into the projection output root's `.loc/media/` directory so agents can open a local copy without cluttering the Markdown page directory or colliding with a projected Notion page named `media`. Durable shadows and `.loc/media/manifest.json` continue to store mount-relative `.loc/media/...` paths; status, diff, inspect, and push treat relative and projection-output-root absolute hrefs for the same media asset as equivalent, including media captions with escaped Markdown label characters and hrefs with balanced parentheses. Locality also writes `.loc/media/manifest.json`, which records the media block ID, kind, source URL, local path, size, and SHA-256 checksum used to detect binary edits. URL-less media payloads still render as directives, for example `::loc{id=image-id type=image title="Architecture diagram"}`. +Filesystem-aware pull, hydration, and post-push reconcile paths download Notion-hosted image, video, PDF, audio, and generic file blocks into the projection output root's `.loc/media/` directory so agents can open a local copy without cluttering the Markdown page directory or colliding with a projected Notion page named `media`. Durable shadows and `.loc/media/manifest.json` continue to store mount-relative `.loc/media/...` paths; status, diff, inspect, and push treat relative and projection-output-root absolute hrefs for the same media asset as equivalent, including media captions with escaped Markdown label characters and hrefs with balanced parentheses. Locality also writes `.loc/media/manifest.json`, which records the media block ID, kind, source URL, local path, size, and SHA-256 checksum used to detect binary edits. A genuinely unavailable or policy-oversized hosted asset does not abort the page revision: Locality omits the binary and renders the same deterministic URL-free directive used by portable projection, for example `::loc{id=image-id type=image title="Architecture diagram"}`. Unsafe hosted origins, redirects, encodings, and malformed expiry metadata remain fail-closed for private desktop hydration. Hosted transfer retries share one overall deadline and apply only to transport/read/timeouts, HTTP 408/429/5xx, and truncated declared lengths; ordinary 4xx responses, unsafe responses, and size-policy failures are terminal. The first writer supports block bodies whose Markdown shape maps to one Notion block or a guarded existing Notion table: paragraphs, headings, single list items, to-dos, quotes, fenced code blocks with variable-length backtick or tilde fences, dividers, display equations, existing stable-width/header-mode tables including row add/delete, existing bookmark/embed URL blocks, and existing URL-backed media blocks. Code-fence closing lines must contain a fence run followed only by optional whitespace, so code lines such as ```` ```not a closer ```` remain inside the code block. Empty code-fence languages and common plain-text aliases such as `text`, `txt`, `plain`, and `plaintext` write Notion's `plain text` code language. Remote URL media edits write external URLs. Local file-like media links backed by `.loc/media/manifest.json` plan as uploads when the Markdown caption, resolved local media asset, or bytes change. Changing only the local media href spelling between equivalent relative and absolute forms is a no-op. Appending a new Markdown image or Markdown link whose href resolves under the projection output root's `.loc/media/` tree uploads that file and creates an image, video, audio, PDF, or generic file block based on the file MIME type. Single-part local media uploads are capped at 20 MB until multipart upload support exists. Existing tables allow cell edits, row appends, and trailing row deletes; detected non-trailing row deletes are blocked because they would shift Notion table-row identities. It also parses the rich inline Markdown emitted by the renderer for bold, italic, strikethrough, underline, code, external links including escaped or balanced parentheses in hrefs, equations, Notion page links, database links whose target ID matches a rendered database mention, explicit page/database mentions written as `@page()` and `@database()`, explicit date mentions written as `@date(2026-06-14)` or `@date(2026-06-14 to 2026-06-21, tz=America/Chicago)`, explicit user mentions written as `@user()`, and legacy `loc://` page links. Rendered link hrefs escape backslashes and parentheses so literal URL characters do not terminate Markdown links early. Rendered `
` is a newline marker, rendered `...` is underline markup, rendered `$...$` is equation markup, and rendered `@date(...)`, `@page(...)`, `@database(...)`, and `@user(...)` are explicit mention markup; literal text containing break tags, underline tags, dollar equation markers, explicit mention markers, Markdown inline markers such as `**`, `_`, `~~`, backticks, and `[`, or paragraph-leading block markers such as `#`, list markers, quote markers, `---`, and `::loc` is escaped with a leading backslash so edits do not turn it into a line break, underline formatting, equation rich text, mention rich text, annotations, links, block type changes, dividers, or directives. Unchanged preimage mentions, such as existing date/user mentions, are preserved during block updates; unsupported inline shapes fail rather than being flattened silently. diff --git a/docs/notion-object-support.md b/docs/notion-object-support.md index 56a8e456..8ee78d8f 100644 --- a/docs/notion-object-support.md +++ b/docs/notion-object-support.md @@ -50,11 +50,12 @@ Sources used for the baseline: | `embed` | Markdown link | Yes for existing blocks | fixture, live read/write | Caption becomes link text; URL edits update the existing embed block. | | `bookmark` | Markdown link | Yes for existing blocks | fixture, live read/write | Caption becomes link text; URL edits update the existing bookmark block. | | `link_preview` | Markdown link | Read only | fixture, local edit/move/delete-blocked | Renders as a normal link when the API returns a URL; the current create-page API rejected it as a child block in live testing, so edits, moves, and deletes are blocked before journaled apply. | -| `image` | Markdown image with local `.loc/media/` href plus local download | Yes for existing URL blocks, local uploads, and appended local image blocks | fixture, live read/write/download/upload | Uses `external.url` or Notion-hosted `file.url` as the source of the downloaded local file. Remote URL Markdown edits write external URLs; local `.loc/media/` href edits upload the local image file back to the existing block. New Markdown images whose href resolves under the projection output root's `.loc/media/` tree upload the local file and create image blocks. URL-less payloads fall back to directives. | -| `video` | Markdown link with local `.loc/media/` href plus local download | Yes for existing URL blocks, local uploads, and appended local video blocks | fixture, live read/write/download/upload | Uses `external.url` or Notion-hosted `file.url` as the source of the downloaded local file. Remote URL Markdown edits write external URLs; local `.loc/media/` href edits upload the local video file back to the existing block. New Markdown links to local video files under the projection output root's `.loc/media/` tree upload the file and create video blocks. | -| `file` | Markdown link with local `.loc/media/` href plus local download | Yes for existing URL blocks, local uploads, and appended local file blocks | fixture, live read/write/download/upload | Uses `external.url` or Notion-hosted `file.url` as the source of the downloaded local file. Remote URL Markdown edits write external URLs; local `.loc/media/` href edits upload the local file back to the existing block. New Markdown links to local files under the projection output root's `.loc/media/` tree upload the file and create generic file blocks. | -| `pdf` | Markdown link with local `.loc/media/` href plus local download | Yes for existing URL blocks, local uploads, and appended local PDF blocks | fixture, live read/write/download/upload | Uses `external.url` or Notion-hosted `file.url` as the source of the downloaded local file. Remote URL Markdown edits write external URLs; local `.loc/media/` href edits upload the local PDF file back to the existing block. New Markdown links to local PDFs under the projection output root's `.loc/media/` tree upload the file and create PDF blocks. | -| `audio` | Markdown link with local `.loc/media/` href plus local download | Yes for existing URL blocks, local uploads, and appended local audio blocks | fixture, live read/write/download/upload | Uses `external.url` or Notion-hosted `file.url` as the source of the downloaded local file. Remote URL Markdown edits write external URLs; local `.loc/media/` href edits upload the local audio file back to the existing block. New Markdown links to local audio files under the projection output root's `.loc/media/` tree upload the file and create audio blocks. | +| `image` | Markdown image; captured hosted media uses local `.loc/media/`, valid external HTTPS stays exact and remote | Yes for existing URL blocks, local uploads, and appended local image blocks | fixture, live read/write/download/upload | Downloads only Notion-hosted `file.url`. External-only `external.url` is never fetched or projected as binary. Genuine hosted omission renders a URL-free directive; local `.loc/media/` edits upload back to the existing block. | +| `video` | Markdown link; captured hosted media uses local `.loc/media/`, valid external HTTPS stays exact and remote | Yes for existing URL blocks, local uploads, and appended local video blocks | fixture, live read/write/download/upload | Same hosted-only capture and external-reference policy as image blocks. | +| `file` | Markdown link; captured hosted media uses local `.loc/media/`, valid external HTTPS stays exact and remote | Yes for existing URL blocks, local uploads, and appended local file blocks | fixture, live read/write/download/upload | Same hosted-only capture and external-reference policy as image blocks. | +| `pdf` | Markdown link; captured hosted media uses local `.loc/media/`, valid external HTTPS stays exact and remote | Yes for existing URL blocks, local uploads, and appended local PDF blocks | fixture, live read/write/download/upload | Same hosted-only capture and external-reference policy as image blocks. | +| `audio` | Markdown link; captured hosted media uses local `.loc/media/`, valid external HTTPS stays exact and remote | Yes for existing URL blocks, local uploads, and appended local audio blocks | fixture, live read/write/download/upload | Same hosted-only capture and external-reference policy as image blocks. | + | `synced_block` | Directive wrapper; source block ID preserved when present | No | fixture | Rewriting synced blocks is lossy without source/copy semantics; live creation of an original synced block was rejected because Notion requires `synced_from`. | | `link_to_page` | Markdown link to Notion URL | Read/delete/move only | fixture, live page/database read/move/retarget-blocked, blocked-write regression | Page/database target ID is preserved in the link target. Moving the rendered link appends a copy of the existing `link_to_page` payload and archives the old block so it does not degrade into a paragraph link. Direct retargeting is blocked before journaled apply because Notion ignores direct target PATCHes and replacement needs undo-aware block identity support. | | `table_of_contents` | Directive | Read/delete/move only | fixture, live read/move | Generated navigation block; no useful Markdown edit surface. Moving the unchanged directive appends a copy at the new position and archives the old block, so reconcile refreshes the block ID. | @@ -71,6 +72,8 @@ Sources used for the baseline: | API `unsupported` subtype-only artifact (`copy_indicator`, legacy `button`, `alias`) | Omitted | No | fixture, live API audit | The API exposes only `unsupported.block_type` and no Markdown content or target. These are treated as non-content UI/artifact blocks so they do not appear as visible `::loc` text. | | Unknown future block / other API `unsupported` block | Directive | No | fixture | Forward compatibility path: preserve block ID and avoid lossy edits. Literal Notion API `unsupported` blocks without an artifact subtype render with a human-readable directive title. | +Portable hosted-media omissions are explicit and redaction-safe. Genuine terminal or exhausted unavailability reports `notion_media_unavailable_hosted_media`, the per-asset or aggregate size policy reports `notion_media_hosted_media_too_large`, and invalid hosted origin/redirect/encoding/expiry reports `notion_media_unsafe_hosted_media`. Existing ambiguous-source, invalid-external, missing-file, page-property, embedded-secret, and asset-count-limit outcomes retain their fail-closed codes. Successful hosted captures use the same `media_local_path` layout and `.loc/media/` logical path as desktop hydration. + ## Rich Text | Rich text object | Read/render | Write | Tests | Notes | From f725417c753d8640239bb2b1984fe4b62c5559c6 Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Wed, 22 Jul 2026 22:31:14 -0700 Subject: [PATCH 02/13] Harden hosted media retry coverage --- crates/loc-cli/tests/e2e_push_workflow.rs | 177 ++++++----------- crates/locality-notion/src/media.rs | 226 +++++++++++++++++++++- crates/localityd/tests/remote_truth.rs | 1 + 3 files changed, 273 insertions(+), 131 deletions(-) diff --git a/crates/loc-cli/tests/e2e_push_workflow.rs b/crates/loc-cli/tests/e2e_push_workflow.rs index fc0469c0..51772da6 100644 --- a/crates/loc-cli/tests/e2e_push_workflow.rs +++ b/crates/loc-cli/tests/e2e_push_workflow.rs @@ -1,13 +1,11 @@ use std::collections::{BTreeMap, hash_map::DefaultHasher}; use std::fs; use std::hash::{Hash, Hasher}; -use std::io::{Read, Write}; -use std::net::{TcpListener, TcpStream}; use std::path::{Path, PathBuf}; use std::process::Command; -use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; -use std::thread::{self, JoinHandle}; +use std::thread; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use loc_cli::connect::{ @@ -64,7 +62,10 @@ use locality_notion::dto::{ PagePropertyDto, PaginatedListDto, ParentDto, RichTextBlockDto, RichTextDto, SelectOptionDto, SyncedBlockDto, SyncedFromDto, TextRichTextDto, TitleBlockDto, }; -use locality_notion::media::resolve_media_href_with_content_root; +use locality_notion::media::{ + PortableMediaCapture, PortableMediaCaptureFetcher, PortableMediaCapturePolicy, + resolve_media_href_with_content_root, +}; use locality_notion::oauth::{ NotionOAuthBrokerCodeExchange, NotionOAuthToken, StoredNotionCredential, }; @@ -630,17 +631,24 @@ fn pull_materializes_and_repairs_downloaded_media_cache() { let fixture = E2eFixture::new(); let mut store = InMemoryStateStore::new(); let image_bytes = b"locality-e2e-image-bytes".to_vec(); - let media_server = LocalMediaServer::new(image_bytes.clone(), 2); + let hosted_url = concat!( + "https://secure.notion-static.com/locality-e2e-image.png?", + "X-Amz-Signature=test-only-signature" + ); + let media_fetcher = Arc::new(CountingHostedMediaFetcher { + expected_url: hosted_url.to_string(), + bytes: image_bytes.clone(), + requests: AtomicUsize::new(0), + }); let api = Arc::new(MutableNotionApi::with_blocks(vec![ paragraph_block("block-1", "Media cache page."), - media_block( - "image-block", - "image", - media_server.url(), - "Local test image", - ), + hosted_media_block("image-block", "image", hosted_url, "Local test image"), ])); - let connector = NotionConnector::with_api(NotionConfig::default(), api.clone()); + let connector = NotionConnector::with_api(NotionConfig::default(), api.clone()) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + media_fetcher.clone(), + ); run_mount( &mut store, @@ -672,7 +680,7 @@ fn pull_materializes_and_repairs_downloaded_media_cache() { let manifest = fs::read_to_string(fixture.root.join(".loc/media/manifest.json")) .expect("read media manifest"); assert!(manifest.contains("image-block"), "{manifest}"); - assert!(manifest.contains(media_server.url()), "{manifest}"); + assert!(manifest.contains(hosted_url), "{manifest}"); fs::remove_file(&local_image).expect("remove materialized image"); let repair = run_pull(&mut store, &connector, &fixture.root).expect("repair media cache page"); @@ -720,11 +728,8 @@ fn pull_materializes_and_repairs_downloaded_media_cache() { !pruned_manifest.contains("image-block"), "{pruned_manifest}" ); - assert!( - !pruned_manifest.contains(&media_server.url()), - "{pruned_manifest}" - ); - media_server.assert_served(); + assert!(!pruned_manifest.contains(hosted_url), "{pruned_manifest}"); + assert_eq!(media_fetcher.requests.load(Ordering::SeqCst), 2); } #[test] @@ -16409,107 +16414,26 @@ fn collect_files_into(path: &Path, files: &mut Vec) { } } -struct LocalMediaServer { - url: String, - expected_requests: usize, - stop: Arc, - handle: Option>, +struct CountingHostedMediaFetcher { + expected_url: String, + bytes: Vec, + requests: AtomicUsize, } -const LOCAL_MEDIA_SERVER_IDLE_TIMEOUT: Duration = Duration::from_secs(30); - -impl LocalMediaServer { - fn new(bytes: Vec, expected_requests: usize) -> Self { - let listener = TcpListener::bind("127.0.0.1:0").expect("bind local media server"); - let url = format!( - "http://{}/locality-e2e-image.png", - listener.local_addr().expect("local media server addr") - ); - let stop = Arc::new(AtomicBool::new(false)); - let server_stop = Arc::clone(&stop); - let handle = thread::spawn(move || { - listener - .set_nonblocking(true) - .expect("nonblocking media listener"); - let mut deadline = Instant::now() + LOCAL_MEDIA_SERVER_IDLE_TIMEOUT; - let mut served = 0; - while !server_stop.load(Ordering::SeqCst) && Instant::now() < deadline { - match listener.accept() { - Ok((stream, _)) => { - if serve_local_media_response(stream, &bytes) { - served += 1; - deadline = Instant::now() + LOCAL_MEDIA_SERVER_IDLE_TIMEOUT; - } - } - Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { - thread::sleep(Duration::from_millis(10)); - } - Err(error) => panic!("accept local media request: {error}"), - } - } - served - }); - - Self { - url, - expected_requests, - stop, - handle: Some(handle), - } - } - - fn url(&self) -> &str { - &self.url - } - - fn assert_served(mut self) { - self.stop.store(true, Ordering::SeqCst); - let served = self - .handle - .take() - .expect("local media server join handle") - .join() - .expect("join local media server"); - assert!( - served >= self.expected_requests, - "local media server should receive at least every expected download request: served {served}, expected {}", - self.expected_requests - ); - } -} - -impl Drop for LocalMediaServer { - fn drop(&mut self) { - self.stop.store(true, Ordering::SeqCst); - if let Some(handle) = self.handle.take() { - let _ = handle.join(); - } - } -} - -fn serve_local_media_response(mut stream: TcpStream, bytes: &[u8]) -> bool { - let _ = stream.set_read_timeout(Some(Duration::from_secs(2))); - let _ = stream.set_write_timeout(Some(Duration::from_secs(2))); - let mut request = [0_u8; 1024]; - let Ok(read) = stream.read(&mut request) else { - return false; - }; - let request = String::from_utf8_lossy(&request[..read]); - if !request.starts_with("GET /locality-e2e-image.png ") { - return false; - } - - let headers = format!( - "HTTP/1.1 200 OK\r\nContent-Type: image/png\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", - bytes.len() - ); - if stream.write_all(headers.as_bytes()).is_err() { - return false; - } - if stream.write_all(bytes).is_err() { - return false; +impl PortableMediaCaptureFetcher for CountingHostedMediaFetcher { + fn fetch( + &self, + hosted_url: &str, + max_bytes: usize, + ) -> locality_core::LocalityResult { + assert_eq!(hosted_url, self.expected_url); + assert!(self.bytes.len() <= max_bytes); + self.requests.fetch_add(1, Ordering::SeqCst); + Ok(PortableMediaCapture { + bytes: self.bytes.clone(), + media_type: "image/png".to_string(), + }) } - stream.flush().is_ok() } #[derive(Debug, Default)] @@ -17592,10 +17516,21 @@ fn synced_block(id: &str, source_block_id: &str) -> BlockDto { block } -fn media_block(id: &str, kind: &str, url: &str, caption: &str) -> BlockDto { - let mut block = media_child(kind, url, caption); - block["id"] = json!(id); - serde_json::from_value(block).expect("media block dto") +fn hosted_media_block(id: &str, kind: &str, url: &str, caption: &str) -> BlockDto { + let mut block = json!({ + "object": "block", + "id": id, + "type": kind + }); + block[kind] = json!({ + "type": "file", + "file": { + "url": url, + "expiry_time": "2099-01-01T00:00:00.000Z" + }, + "caption": rich_text_json(caption) + }); + serde_json::from_value(block).expect("hosted media block dto") } fn ambiguous_tasks_schema() -> &'static str { diff --git a/crates/locality-notion/src/media.rs b/crates/locality-notion/src/media.rs index 1b6246fc..98999f1a 100644 --- a/crates/locality-notion/src/media.rs +++ b/crates/locality-notion/src/media.rs @@ -278,20 +278,59 @@ fn fetch_hosted_media_outcome_with_policy( max_bytes: usize, deadline: Duration, retry_delay: Duration, +) -> HostedMediaCaptureOutcome { + let clock = SystemHostedMediaRetryClock { + started: Instant::now(), + }; + fetch_hosted_media_outcome_with_clock( + transport, + hosted_url, + max_bytes, + deadline, + retry_delay, + &clock, + ) +} + +trait HostedMediaRetryClock { + fn elapsed(&self) -> Duration; + fn sleep(&self, duration: Duration); +} + +struct SystemHostedMediaRetryClock { + started: Instant, +} + +impl HostedMediaRetryClock for SystemHostedMediaRetryClock { + fn elapsed(&self) -> Duration { + self.started.elapsed() + } + + fn sleep(&self, duration: Duration) { + thread::sleep(duration); + } +} + +fn fetch_hosted_media_outcome_with_clock( + transport: &dyn PortableMediaHttpTransport, + hosted_url: &str, + max_bytes: usize, + deadline: Duration, + retry_delay: Duration, + clock: &dyn HostedMediaRetryClock, ) -> HostedMediaCaptureOutcome { let initial = match validate_portable_hosted_media_url(hosted_url) { Ok(url) => url, Err(_) => return HostedMediaCaptureOutcome::Unsafe, }; - let started = Instant::now(); for attempt in 1..=MEDIA_FETCH_ATTEMPTS { - match fetch_hosted_media_once(transport, &initial, max_bytes, started, deadline) { + match fetch_hosted_media_once(transport, &initial, max_bytes, deadline, clock) { Ok(capture) => return HostedMediaCaptureOutcome::Captured(capture), Err(HostedMediaTransferFailure::RetryableUnavailable) - if attempt < MEDIA_FETCH_ATTEMPTS && started.elapsed() < deadline => + if attempt < MEDIA_FETCH_ATTEMPTS && clock.elapsed() < deadline => { - let remaining = deadline.saturating_sub(started.elapsed()); - thread::sleep(retry_delay.min(remaining)); + let remaining = deadline.saturating_sub(clock.elapsed()); + clock.sleep(retry_delay.min(remaining)); } Err(HostedMediaTransferFailure::RetryableUnavailable) | Err(HostedMediaTransferFailure::Unavailable) => { @@ -312,8 +351,8 @@ fn fetch_hosted_media_once( transport: &dyn PortableMediaHttpTransport, initial: &reqwest::Url, max_bytes: usize, - started: Instant, deadline: Duration, + clock: &dyn HostedMediaRetryClock, ) -> Result { let mut current = initial.clone(); let mut visited = std::collections::BTreeSet::new(); @@ -323,7 +362,7 @@ fn fetch_hosted_media_once( return Err(HostedMediaTransferFailure::Unsafe); } let timeout = deadline - .checked_sub(started.elapsed()) + .checked_sub(clock.elapsed()) .ok_or(HostedMediaTransferFailure::RetryableUnavailable)?; if timeout.is_zero() { return Err(HostedMediaTransferFailure::RetryableUnavailable); @@ -1161,9 +1200,10 @@ mod tests { #[cfg(windows)] use super::resolve_media_href_with_content_root; use super::{ - HostedMediaCaptureOutcome, MediaAsset, PORTABLE_MEDIA_READ_BUFFER_BYTES, - PortableMediaCapture, PortableMediaCaptureFetcher, PortableMediaHttpResponse, - PortableMediaHttpTransport, fetch_hosted_media_outcome_with_policy, + HostedMediaCaptureOutcome, HostedMediaRetryClock, MediaAsset, + PORTABLE_MEDIA_READ_BUFFER_BYTES, PortableMediaCapture, PortableMediaCaptureFetcher, + PortableMediaHttpResponse, PortableMediaHttpTransport, + fetch_hosted_media_outcome_with_clock, fetch_hosted_media_outcome_with_policy, fetch_hosted_media_outcome_with_transport, fetch_portable_media_with_transport, local_media_href, media_local_path, portable_media_expired, replace_media_manifest, resolve_media_href, sanitize_portable_hosted_media_url, validate_portable_hosted_media_url, @@ -1599,6 +1639,7 @@ mod tests { struct ScriptedPortableMediaTransport { responses: Mutex>, + transport_failures: Mutex, requests: Mutex>, timeouts: Mutex>, max_read_size: Arc, @@ -1618,17 +1659,31 @@ mod tests { body: Box::new(TrackingReader { bytes: std::io::Cursor::new(response.body), max_read_size: Arc::clone(&max_read_size), + fail_next_read: response.read_error, }), }) .collect(); Self { responses: Mutex::new(responses), + transport_failures: Mutex::new(0), requests: Mutex::new(Vec::new()), timeouts: Mutex::new(Vec::new()), max_read_size, } } + fn with_transport_failures( + failures: usize, + responses: impl IntoIterator, + ) -> Self { + let transport = Self::new(responses); + *transport + .transport_failures + .lock() + .expect("transport failures") = failures; + transport + } + fn requests(&self) -> Vec { self.requests.lock().expect("requests").clone() } @@ -1653,6 +1708,13 @@ mod tests { .expect("requests") .push(url.to_string()); self.timeouts.lock().expect("timeouts").push(timeout); + let mut failures = self.transport_failures.lock().expect("transport failures"); + if *failures > 0 { + *failures -= 1; + return Err(locality_core::LocalityError::Io( + "scripted transport failure".to_string(), + )); + } self.responses .lock() .expect("responses") @@ -1672,6 +1734,7 @@ mod tests { content_length: Option, content_type: Option, body: Vec, + read_error: bool, } fn scripted_response( @@ -1689,17 +1752,31 @@ mod tests { content_length, content_type: content_type.map(str::to_string), body, + read_error: false, } } + fn scripted_read_error_response(mut response: ScriptedResponse) -> ScriptedResponse { + response.read_error = true; + response + } + struct TrackingReader { bytes: std::io::Cursor>, max_read_size: Arc, + fail_next_read: bool, } impl Read for TrackingReader { fn read(&mut self, buffer: &mut [u8]) -> std::io::Result { self.max_read_size.fetch_max(buffer.len(), Ordering::SeqCst); + if self.fail_next_read { + self.fail_next_read = false; + return Err(std::io::Error::new( + std::io::ErrorKind::ConnectionReset, + "scripted response read failure", + )); + } self.bytes.read(buffer) } } @@ -1832,4 +1909,133 @@ mod tests { ); assert_eq!(transport.requests().len(), 1); } + + #[test] + fn hosted_media_retries_transport_and_read_io_failures() { + let success = || { + scripted_response( + StatusCode::OK, + None, + None, + Some(4), + Some("image/png"), + b"data".to_vec(), + ) + }; + let transport_failure = + ScriptedPortableMediaTransport::with_transport_failures(1, [success()]); + assert_eq!( + fetch_hosted_media_outcome_with_policy( + &transport_failure, + "https://secure.notion-static.com/image.png", + 1024, + std::time::Duration::from_secs(1), + std::time::Duration::ZERO, + ), + HostedMediaCaptureOutcome::Captured(PortableMediaCapture { + bytes: b"data".to_vec(), + media_type: "image/png".to_string(), + }) + ); + assert_eq!(transport_failure.requests().len(), 2); + + let read_failure = ScriptedPortableMediaTransport::new([ + scripted_read_error_response(success()), + success(), + ]); + assert_eq!( + fetch_hosted_media_outcome_with_policy( + &read_failure, + "https://secure.notion-static.com/image.png", + 1024, + std::time::Duration::from_secs(1), + std::time::Duration::ZERO, + ), + HostedMediaCaptureOutcome::Captured(PortableMediaCapture { + bytes: b"data".to_vec(), + media_type: "image/png".to_string(), + }) + ); + assert_eq!(read_failure.requests().len(), 2); + } + + #[derive(Default)] + struct ManualHostedMediaRetryClock { + elapsed: Mutex, + } + + impl ManualHostedMediaRetryClock { + fn advance(&self, duration: std::time::Duration) { + let mut elapsed = self.elapsed.lock().expect("manual elapsed"); + *elapsed += duration; + } + } + + impl HostedMediaRetryClock for ManualHostedMediaRetryClock { + fn elapsed(&self) -> std::time::Duration { + *self.elapsed.lock().expect("manual elapsed") + } + + fn sleep(&self, duration: std::time::Duration) { + self.advance(duration); + } + } + + struct DeadlineConsumingTransport { + clock: Arc, + timeouts: Mutex>, + } + + impl PortableMediaHttpTransport for DeadlineConsumingTransport { + fn get( + &self, + _url: &str, + timeout: std::time::Duration, + ) -> locality_core::LocalityResult { + self.timeouts + .lock() + .expect("deadline timeouts") + .push(timeout); + self.clock.advance(std::time::Duration::from_millis(600)); + Err(locality_core::LocalityError::Io( + "scripted transport timeout".to_string(), + )) + } + } + + #[test] + fn hosted_media_retries_share_one_total_elapsed_deadline() { + let clock = Arc::new(ManualHostedMediaRetryClock::default()); + let transport = DeadlineConsumingTransport { + clock: Arc::clone(&clock), + timeouts: Mutex::new(Vec::new()), + }; + + assert_eq!( + fetch_hosted_media_outcome_with_clock( + &transport, + "https://secure.notion-static.com/image.png", + 1024, + std::time::Duration::from_secs(1), + std::time::Duration::from_millis(100), + clock.as_ref(), + ), + HostedMediaCaptureOutcome::Unavailable + ); + assert_eq!( + transport + .timeouts + .lock() + .expect("deadline timeouts") + .as_slice(), + [ + std::time::Duration::from_millis(1000), + std::time::Duration::from_millis(300), + ] + ); + assert_eq!( + HostedMediaRetryClock::elapsed(clock.as_ref()), + std::time::Duration::from_millis(1300) + ); + } } diff --git a/crates/localityd/tests/remote_truth.rs b/crates/localityd/tests/remote_truth.rs index fd432054..72a1cc14 100644 --- a/crates/localityd/tests/remote_truth.rs +++ b/crates/localityd/tests/remote_truth.rs @@ -98,6 +98,7 @@ impl Connector for RecordingDirectConnector { fn apply_undo(&self, _request: ApplyUndoRequest<'_>) -> LocalityResult { Ok(ApplyUndoResult { changed_remote_ids: Vec::new(), + observations: Vec::new(), }) } } From 7e9f14a1e279425c72751ff3ea2de1b949cbf3d0 Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Wed, 22 Jul 2026 22:46:30 -0700 Subject: [PATCH 03/13] Refresh changeset protocol golden --- crates/locality-protocol/fixtures/changeset-envelope.json | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/locality-protocol/fixtures/changeset-envelope.json b/crates/locality-protocol/fixtures/changeset-envelope.json index eae29a98..aaeb0c86 100644 --- a/crates/locality-protocol/fixtures/changeset-envelope.json +++ b/crates/locality-protocol/fixtures/changeset-envelope.json @@ -75,6 +75,7 @@ "blocks_archived": 0, "entities_created": 0, "entities_archived": 0, + "entity_bodies_updated": 0, "entities_moved": 0, "properties_updated": 0 }, From c2743d0b5784a24b49ab69772014b7cabec1a15c Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Wed, 22 Jul 2026 22:56:15 -0700 Subject: [PATCH 04/13] Restore direct mount editing guidance --- templates/mount/AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/mount/AGENTS.md b/templates/mount/AGENTS.md index 7b840c43..bbfa74b9 100644 --- a/templates/mount/AGENTS.md +++ b/templates/mount/AGENTS.md @@ -9,7 +9,7 @@ Common Locality CLI workflow: - Use `loc info .` for context and connector details; if the user asks you to connect a provider before mounting, run `loc connect --no-browser`, share the authorization URL, and ask the user to open it while you wait for verification. - Use `loc search ` for local metadata and indexed content. - Open files directly; Locality hydrates online-only files on open. -- Edit mounted Markdown and keep edits focused. +- Edit mounted Markdown directly and keep edits focused. - Use `loc status ` for pending local changes. - Use `loc inspect ` for read-only remote comparison of a hydrated file. - Use `loc diff ` for planned Notion operations before pushing. From 314921706d3498a8a2effc7e466969a436487c2c Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Wed, 22 Jul 2026 23:01:35 -0700 Subject: [PATCH 05/13] Keep mount guidance within concise limit --- templates/mount/AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/mount/AGENTS.md b/templates/mount/AGENTS.md index bbfa74b9..ed044730 100644 --- a/templates/mount/AGENTS.md +++ b/templates/mount/AGENTS.md @@ -15,7 +15,7 @@ Common Locality CLI workflow: - Use `loc diff ` for planned Notion operations before pushing. - Use `loc mv ` for intentional page/file moves or renames, then review with `loc diff `. - Push intentional changes with `loc push `. Use `loc push ` to make Notion match local edits. -- Use `loc pull ` only to force clean local files to match latest remote now. +- Use `loc pull ` only to force clean local files to match latest remote. - If desktop Live Mode is on, safe edits may sync automatically. Use `loc live-mode status ` to inspect state. Do not run routine `loc pull` or `loc push` after every edit. - For explicit sync/update/publish requests, run `loc diff ` first, then `loc push -y` for safe plans. - If push says the remote changed since last sync, run `loc pull `, resolve conflict markers, rerun `loc diff `, then push. From db2b6a8a3095cc1f4918f51671586eb5828b624f Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 02:15:11 -0700 Subject: [PATCH 06/13] Distinguish external media omission outcomes --- crates/locality-notion/src/portable.rs | 34 +++++- crates/locality-notion/tests/fetch_render.rs | 104 +++++++++++++++++-- docs/notion-connector.md | 13 ++- docs/notion-object-support.md | 2 +- 4 files changed, 141 insertions(+), 12 deletions(-) diff --git a/crates/locality-notion/src/portable.rs b/crates/locality-notion/src/portable.rs index cc326619..66a4a4b4 100644 --- a/crates/locality-notion/src/portable.rs +++ b/crates/locality-notion/src/portable.rs @@ -372,9 +372,14 @@ impl<'a> PortableMediaCaptureState<'a> { if validate_portable_external_media_url(&external.url).is_ok() { return Ok(()); } + let code = if external.url.is_empty() { + "unavailable_external_media" + } else { + "unsafe_external_media" + }; external.url.clear(); if !self.limit_exceeded { - self.record_incomplete(block_id, kind, "invalid_external_media"); + self.record_incomplete(block_id, kind, code); } return Ok(()); } @@ -1205,7 +1210,9 @@ fn validate_portable_media_bundle(bundle: &NotionPortablePageBundleV1) -> Locali if !limit_exceeded { let code = match (payload.external.is_some(), payload.file.is_some()) { (true, true) => "ambiguous_file_source", - (true, false) => "invalid_external_media", + (true, false) => { + actual_external_incomplete_code(&bundle.incomplete_media, block_id, kind)? + } (false, true) => { actual_hosted_incomplete_code(&bundle.incomplete_media, block_id, kind)? } @@ -1306,6 +1313,29 @@ fn validate_portable_media_bundle(bundle: &NotionPortablePageBundleV1) -> Locali Ok(()) } +fn actual_external_incomplete_code<'a>( + incomplete: &'a [NotionPortableIncompleteMediaV1], + block_id: &str, + kind: &str, +) -> LocalityResult<&'a str> { + let Some(outcome) = incomplete + .iter() + .find(|outcome| outcome.block_id == block_id && outcome.kind == kind) + else { + return Err(LocalityError::InvalidState( + "Notion portable media native payload has invalid incomplete outcomes".to_string(), + )); + }; + match outcome.code.as_str() { + "unavailable_external_media" | "unsafe_external_media" | "invalid_external_media" => { + Ok(&outcome.code) + } + _ => Err(LocalityError::InvalidState( + "Notion portable media native payload has invalid incomplete outcomes".to_string(), + )), + } +} + fn actual_hosted_incomplete_code<'a>( incomplete: &'a [NotionPortableIncompleteMediaV1], block_id: &str, diff --git a/crates/locality-notion/tests/fetch_render.rs b/crates/locality-notion/tests/fetch_render.rs index 866e7201..f8e713ad 100644 --- a/crates/locality-notion/tests/fetch_render.rs +++ b/crates/locality-notion/tests/fetch_render.rs @@ -3534,12 +3534,20 @@ fn portable_media_denials_are_incomplete_and_never_publish_remote_urls() { #[test] fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_sources() { let invalid = [ - ("empty", ""), - ("malformed", "not a URL"), - ("http", "http://example.com/image.png"), - ("userinfo", "https://user:pass@example.com/image.png"), + ("empty", "", "unavailable_external_media"), + ("malformed", "not a URL", "unsafe_external_media"), + ( + "http", + "http://example.com/image.png", + "unsafe_external_media", + ), + ( + "userinfo", + "https://user:pass@example.com/image.png", + "unsafe_external_media", + ), ]; - for (index, (case, url)) in invalid.into_iter().enumerate() { + for (index, (case, url, expected_code)) in invalid.into_iter().enumerate() { let page_id = format!("external-invalid-{index}"); let block_id = format!("external-block-{index}"); let calls = Arc::new(Mutex::new(Vec::new())); @@ -3565,7 +3573,15 @@ fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_source vec![NotionPortableIncompleteMediaV1 { block_id: block_id.clone(), kind: "image".to_string(), - code: "invalid_external_media".to_string(), + code: expected_code.to_string(), + }], + "{case}" + ); + assert_eq!( + fetched.completeness.incomplete_reasons(), + &[PortableIncompleteReason::ConnectorLimitation { + code: format!("notion_media_{expected_code}"), + remote_id: Some(RemoteId::new(block_id.clone())), }], "{case}" ); @@ -3626,6 +3642,17 @@ fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_source .expect("raw-unsafe external URL becomes explicitly incomplete"); assert!(calls.lock().expect("calls").is_empty(), "{case}"); assert!(!fetched.completeness.is_complete(), "{case}"); + let native: NotionPortablePageBundleV1 = + serde_json::from_slice(&fetched.native.raw).expect("unsafe external native"); + assert_eq!( + native.incomplete_media, + vec![NotionPortableIncompleteMediaV1 { + block_id: "invalid-raw".to_string(), + kind: "image".to_string(), + code: "unsafe_external_media".to_string(), + }], + "{case}" + ); assert!(!String::from_utf8_lossy(&fetched.native.raw).contains(&url)); } @@ -3701,6 +3728,71 @@ fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_source } } +#[test] +fn portable_external_media_legacy_generic_outcome_remains_readable() { + let page_id = "legacy-invalid-external-page"; + let block_id = "legacy-invalid-external-block"; + let unsafe_url = "http://user:secret@example.com/private.png"; + let calls = Arc::new(Mutex::new(Vec::new())); + let connector = portable_media_connector( + page_id, + vec![file_block(block_id, "image", unsafe_url, "Private image")], + ) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + Arc::new(FixturePortableMediaFetcher { + outcomes: BTreeMap::new(), + calls: Arc::clone(&calls), + }), + ); + let fetched = connector + .fetch_portable(portable_fetch_request(page_id)) + .expect("unsafe external fetch"); + assert!(calls.lock().expect("calls").is_empty()); + let current: NotionPortablePageBundleV1 = + serde_json::from_slice(&fetched.native.raw).expect("portable native"); + let mut unsupported = current.clone(); + unsupported.incomplete_media[0].code = "external_media".to_string(); + assert_eq!( + connector + .render_portable(&portable_render_request( + page_id, + NativeEntity { + remote_id: RemoteId::new(page_id), + kind: "notion_page_portable_media_v1".to_string(), + raw: serde_json::to_vec(&unsupported).expect("unsupported native"), + }, + )) + .expect_err("unsupported generic outcome must fail") + .to_string(), + "invalid state: Notion portable media native payload has invalid incomplete outcomes" + ); + + let mut legacy = current; + legacy.incomplete_media[0].code = "invalid_external_media".to_string(); + let legacy_raw = serde_json::to_vec(&legacy).expect("legacy native"); + assert!(!String::from_utf8_lossy(&legacy_raw).contains(unsafe_url)); + + let rendered = connector + .render_portable(&portable_render_request( + page_id, + NativeEntity { + remote_id: RemoteId::new(page_id), + kind: "notion_page_portable_media_v1".to_string(), + raw: legacy_raw, + }, + )) + .expect("legacy generic outcome remains readable"); + assert_eq!( + rendered.completeness.incomplete_reasons(), + &[PortableIncompleteReason::ConnectorLimitation { + code: "notion_media_invalid_external_media".to_string(), + remote_id: Some(RemoteId::new(block_id)), + }] + ); + assert!(!String::from_utf8_lossy(&rendered.canonical.body).contains(unsafe_url)); +} + #[test] fn portable_media_expired_failed_and_oversized_captures_are_redacted() { let cases = [ diff --git a/docs/notion-connector.md b/docs/notion-connector.md index 009a4ae9..880da5e6 100644 --- a/docs/notion-connector.md +++ b/docs/notion-connector.md @@ -211,9 +211,16 @@ recomputes that outcome ledger and rejects missing, duplicate, spurious, or mismatched entries before producing an artifact. Durable v1 native JSON is canonical: render streams a byte-for-byte reserialization comparison and rejects unknown fields, alternate field order, or noncanonical whitespace. -External media stays unsupported and incomplete in this policy. Production -capture accepts HTTPS on the default port only, rejects user information and IP -literals, and permits exactly these origins: +External media is never fetched. A valid external HTTPS URL remains an exact +remote reference, including its explicit port, path, query, and fragment. An +empty external URL is redacted and reports `unavailable_external_media`; +malformed, non-HTTPS, user-info-bearing, control-containing, oversized, or +otherwise invalid external URLs are redacted and report +`unsafe_external_media`. Older sanitized v1 payloads carrying the legacy +`invalid_external_media` outcome remain readable, but new fetches never emit +that generic code. Production hosted-media capture accepts HTTPS on the default +port only, rejects user information and IP literals, and permits exactly these +origins: - `secure.notion-static.com`; - `prod-files-secure.s3.us-west-2.amazonaws.com`; diff --git a/docs/notion-object-support.md b/docs/notion-object-support.md index 8ee78d8f..b0f58f25 100644 --- a/docs/notion-object-support.md +++ b/docs/notion-object-support.md @@ -72,7 +72,7 @@ Sources used for the baseline: | API `unsupported` subtype-only artifact (`copy_indicator`, legacy `button`, `alias`) | Omitted | No | fixture, live API audit | The API exposes only `unsupported.block_type` and no Markdown content or target. These are treated as non-content UI/artifact blocks so they do not appear as visible `::loc` text. | | Unknown future block / other API `unsupported` block | Directive | No | fixture | Forward compatibility path: preserve block ID and avoid lossy edits. Literal Notion API `unsupported` blocks without an artifact subtype render with a human-readable directive title. | -Portable hosted-media omissions are explicit and redaction-safe. Genuine terminal or exhausted unavailability reports `notion_media_unavailable_hosted_media`, the per-asset or aggregate size policy reports `notion_media_hosted_media_too_large`, and invalid hosted origin/redirect/encoding/expiry reports `notion_media_unsafe_hosted_media`. Existing ambiguous-source, invalid-external, missing-file, page-property, embedded-secret, and asset-count-limit outcomes retain their fail-closed codes. Successful hosted captures use the same `media_local_path` layout and `.loc/media/` logical path as desktop hydration. +Portable media omissions are explicit and redaction-safe. Genuine terminal or exhausted hosted unavailability reports `notion_media_unavailable_hosted_media`, the per-asset or aggregate size policy reports `notion_media_hosted_media_too_large`, and invalid hosted origin/redirect/encoding/expiry reports `notion_media_unsafe_hosted_media`. External media is never fetched: an empty URL reports `notion_media_unavailable_external_media`, while a malformed, non-HTTPS, user-info-bearing, control-containing, oversized, or otherwise invalid URL reports `notion_media_unsafe_external_media`; both are removed from native and rendered content. Valid external HTTPS remains an exact remote link. Older redacted v1 payloads with `notion_media_invalid_external_media` remain readable, but new fetches do not emit that generic code. Ambiguous-source, missing-file, page-property, embedded-secret, and asset-count-limit outcomes retain their fail-closed codes. Successful hosted captures use the same `media_local_path` layout and `.loc/media/` logical path as desktop hydration. ## Rich Text From 1f9f8d04ea011b34393eefb5ed439be64186126f Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 03:10:36 -0700 Subject: [PATCH 07/13] Diagnose unsafe external media exactly --- crates/locality-notion/src/media.rs | 101 ++++++++++-- crates/locality-notion/src/portable.rs | 27 ++-- crates/locality-notion/tests/fetch_render.rs | 159 +++++++++---------- docs/notion-connector.md | 22 ++- docs/notion-object-support.md | 2 +- 5 files changed, 192 insertions(+), 119 deletions(-) diff --git a/crates/locality-notion/src/media.rs b/crates/locality-notion/src/media.rs index 98999f1a..33939120 100644 --- a/crates/locality-notion/src/media.rs +++ b/crates/locality-notion/src/media.rs @@ -486,32 +486,101 @@ pub(crate) fn sanitize_portable_hosted_media_url(url: &str) -> LocalityResult LocalityResult<()> { - if url.is_empty() - || url.len() > PORTABLE_EXTERNAL_MEDIA_MAX_URL_BYTES - || url - .chars() - .any(|character| character.is_ascii_control() || character.is_whitespace()) +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum PortableExternalMediaUrlFailure { + Unavailable, + TooLong, + WhitespaceOrControl, + Malformed, + NonHttps, + MissingHost, + Userinfo, +} + +impl PortableExternalMediaUrlFailure { + pub(crate) const fn omission_code(self) -> &'static str { + match self { + Self::Unavailable => "unavailable_external_media", + Self::TooLong => "unsafe_external_media_too_long", + Self::WhitespaceOrControl => "unsafe_external_media_whitespace_or_control", + Self::Malformed => "unsafe_external_media_malformed", + Self::NonHttps => "unsafe_external_media_non_https", + Self::MissingHost => "unsafe_external_media_missing_host", + Self::Userinfo => "unsafe_external_media_userinfo", + } + } +} + +pub(crate) fn classify_portable_external_media_url( + url: &str, +) -> Result<(), PortableExternalMediaUrlFailure> { + if url.is_empty() { + return Err(PortableExternalMediaUrlFailure::Unavailable); + } + if url.len() > PORTABLE_EXTERNAL_MEDIA_MAX_URL_BYTES { + return Err(PortableExternalMediaUrlFailure::TooLong); + } + if url + .chars() + .any(|character| character.is_ascii_control() || character.is_whitespace()) { - return Err(LocalityError::InvalidState( - "portable external media URL is not raw-safe".to_string(), - )); + return Err(PortableExternalMediaUrlFailure::WhitespaceOrControl); } let parsed = reqwest::Url::parse(url).map_err(|_| { - LocalityError::InvalidState("portable external media URL is invalid".to_string()) + if https_url_has_empty_authority(url) { + PortableExternalMediaUrlFailure::MissingHost + } else { + PortableExternalMediaUrlFailure::Malformed + } })?; - if parsed.scheme() != "https" - || parsed.host_str().is_none() - || !parsed.username().is_empty() + if parsed.scheme() != "https" { + return Err(PortableExternalMediaUrlFailure::NonHttps); + } + if parsed.host_str().is_none() { + return Err(PortableExternalMediaUrlFailure::MissingHost); + } + if !parsed.username().is_empty() || parsed.password().is_some() + || url_authority_has_userinfo(url) { - return Err(LocalityError::InvalidState( - "portable external media URL violates the HTTPS reference policy".to_string(), - )); + return Err(PortableExternalMediaUrlFailure::Userinfo); } Ok(()) } +fn https_url_has_empty_authority(url: &str) -> bool { + let Some(prefix) = url.get(.."https://".len()) else { + return false; + }; + if !prefix.eq_ignore_ascii_case("https://") { + return false; + } + let authority_and_path = &url["https://".len()..]; + let authority = authority_and_path + .split(['/', '?', '#']) + .next() + .unwrap_or_default(); + let host_and_port = authority.rsplit('@').next().unwrap_or_default(); + host_and_port.is_empty() || host_and_port.starts_with(':') +} + +fn url_authority_has_userinfo(url: &str) -> bool { + url.split_once("://") + .map(|(_, authority_and_path)| { + authority_and_path + .split(['/', '?', '#']) + .next() + .is_some_and(|authority| authority.contains('@')) + }) + .unwrap_or(false) +} + +pub(crate) fn validate_portable_external_media_url(url: &str) -> LocalityResult<()> { + classify_portable_external_media_url(url).map_err(|_| { + LocalityError::InvalidState("portable external media URL is not allowed".to_string()) + }) +} + pub(crate) fn portable_media_expired(expiry_time: &str) -> LocalityResult { let expiry = parse_rfc3339_utc_seconds(expiry_time).ok_or_else(|| { LocalityError::InvalidState("portable media expiry is invalid".to_string()) diff --git a/crates/locality-notion/src/portable.rs b/crates/locality-notion/src/portable.rs index 66a4a4b4..0ef74903 100644 --- a/crates/locality-notion/src/portable.rs +++ b/crates/locality-notion/src/portable.rs @@ -35,8 +35,9 @@ use crate::fetch::fetch_known_page_bundle; use crate::media::{ HostedMediaCaptureOutcome, PORTABLE_MEDIA_MAX_AGGREGATE_BYTES, PORTABLE_MEDIA_MAX_ASSET_BYTES, PORTABLE_MEDIA_MAX_ASSETS, PortableMediaCaptureFetcher, PortableMediaCapturePolicy, - default_portable_media_fetcher, portable_media_expired, sanitize_portable_hosted_media_url, - sanitize_portable_media_type, validate_portable_external_media_url, + classify_portable_external_media_url, default_portable_media_fetcher, portable_media_expired, + sanitize_portable_hosted_media_url, sanitize_portable_media_type, + validate_portable_external_media_url, }; use crate::projection::enumerate_explicit_root_trees; use crate::render::{RenderOptions, render_native_entity, render_native_entity_with_options}; @@ -369,13 +370,9 @@ impl<'a> PortableMediaCaptureState<'a> { "Notion portable media payload type does not match its source".to_string(), )); } - if validate_portable_external_media_url(&external.url).is_ok() { - return Ok(()); - } - let code = if external.url.is_empty() { - "unavailable_external_media" - } else { - "unsafe_external_media" + let code = match classify_portable_external_media_url(&external.url) { + Ok(()) => return Ok(()), + Err(failure) => failure.omission_code(), }; external.url.clear(); if !self.limit_exceeded { @@ -1327,9 +1324,15 @@ fn actual_external_incomplete_code<'a>( )); }; match outcome.code.as_str() { - "unavailable_external_media" | "unsafe_external_media" | "invalid_external_media" => { - Ok(&outcome.code) - } + "unavailable_external_media" + | "unsafe_external_media_too_long" + | "unsafe_external_media_whitespace_or_control" + | "unsafe_external_media_malformed" + | "unsafe_external_media_non_https" + | "unsafe_external_media_missing_host" + | "unsafe_external_media_userinfo" + | "unsafe_external_media" + | "invalid_external_media" => Ok(&outcome.code), _ => Err(LocalityError::InvalidState( "Notion portable media native payload has invalid incomplete outcomes".to_string(), )), diff --git a/crates/locality-notion/tests/fetch_render.rs b/crates/locality-notion/tests/fetch_render.rs index f8e713ad..0743bd7e 100644 --- a/crates/locality-notion/tests/fetch_render.rs +++ b/crates/locality-notion/tests/fetch_render.rs @@ -3533,33 +3533,64 @@ fn portable_media_denials_are_incomplete_and_never_publish_remote_urls() { #[test] fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_sources() { - let invalid = [ - ("empty", "", "unavailable_external_media"), - ("malformed", "not a URL", "unsafe_external_media"), + let invalid = vec![ + ("empty", String::new(), "unavailable_external_media"), ( - "http", - "http://example.com/image.png", - "unsafe_external_media", + "too-long", + format!("https://example.com/{}", "a".repeat(8 * 1024)), + "unsafe_external_media_too_long", + ), + ( + "whitespace", + "https://example.com/image name.png".to_string(), + "unsafe_external_media_whitespace_or_control", + ), + ( + "control", + "https://example.com/image.png\u{7f}hidden".to_string(), + "unsafe_external_media_whitespace_or_control", + ), + ( + "malformed", + "not-a-url".to_string(), + "unsafe_external_media_malformed", + ), + ( + "non-https", + "http://example.com/image.png".to_string(), + "unsafe_external_media_non_https", + ), + ( + "missing-host", + "https://:443/image.png".to_string(), + "unsafe_external_media_missing_host", ), ( "userinfo", - "https://user:pass@example.com/image.png", - "unsafe_external_media", + "https://user:pass@example.com/image.png".to_string(), + "unsafe_external_media_userinfo", + ), + ( + "empty-userinfo", + "https://@example.com/image.png".to_string(), + "unsafe_external_media_userinfo", ), ]; for (index, (case, url, expected_code)) in invalid.into_iter().enumerate() { let page_id = format!("external-invalid-{index}"); let block_id = format!("external-block-{index}"); let calls = Arc::new(Mutex::new(Vec::new())); - let connector = - portable_media_connector(&page_id, vec![file_block(&block_id, "image", url, "Image")]) - .with_portable_media_capture_fetcher( - PortableMediaCapturePolicy::HostedPilot, - Arc::new(FixturePortableMediaFetcher { - outcomes: BTreeMap::new(), - calls: Arc::clone(&calls), - }), - ); + let connector = portable_media_connector( + &page_id, + vec![file_block(&block_id, "image", &url, "Image")], + ) + .with_portable_media_capture_fetcher( + PortableMediaCapturePolicy::HostedPilot, + Arc::new(FixturePortableMediaFetcher { + outcomes: BTreeMap::new(), + calls: Arc::clone(&calls), + }), + ); let fetched = connector .fetch_portable(portable_fetch_request(&page_id)) .unwrap_or_else(|error| panic!("{case} external fetch: {error}")); @@ -3608,54 +3639,11 @@ fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_source assert_eq!(rendered.projections.len(), 1, "{case}"); assert!(!rendered.completeness.is_complete(), "{case}"); if !url.is_empty() { - assert!( - !String::from_utf8_lossy(&rendered.canonical.body).contains(url), - "{case}" - ); + assert!(!String::from_utf8_lossy(&fetched.native.raw).contains(&url)); + assert!(!String::from_utf8_lossy(&rendered.canonical.body).contains(&url)); } } - for (case, url) in [ - ( - "control", - "https://example.com/image.png\u{7f}hidden".to_string(), - ), - ( - "oversized", - format!("https://example.com/{}", "a".repeat(8 * 1024)), - ), - ] { - let calls = Arc::new(Mutex::new(Vec::new())); - let connector = portable_media_connector( - case, - vec![file_block("invalid-raw", "image", &url, "Image")], - ) - .with_portable_media_capture_fetcher( - PortableMediaCapturePolicy::HostedPilot, - Arc::new(FixturePortableMediaFetcher { - outcomes: BTreeMap::new(), - calls: Arc::clone(&calls), - }), - ); - let fetched = connector - .fetch_portable(portable_fetch_request(case)) - .expect("raw-unsafe external URL becomes explicitly incomplete"); - assert!(calls.lock().expect("calls").is_empty(), "{case}"); - assert!(!fetched.completeness.is_complete(), "{case}"); - let native: NotionPortablePageBundleV1 = - serde_json::from_slice(&fetched.native.raw).expect("unsafe external native"); - assert_eq!( - native.incomplete_media, - vec![NotionPortableIncompleteMediaV1 { - block_id: "invalid-raw".to_string(), - kind: "image".to_string(), - code: "unsafe_external_media".to_string(), - }], - "{case}" - ); - assert!(!String::from_utf8_lossy(&fetched.native.raw).contains(&url)); - } - let external_url = "https://example.com/public.png"; let hosted_url = "https://secure.notion-static.com/private.png?X-Amz-Signature=must-not-escape"; let mut ambiguous = file_block("ambiguous", "image", external_url, "Image"); @@ -3768,29 +3756,32 @@ fn portable_external_media_legacy_generic_outcome_remains_readable() { "invalid state: Notion portable media native payload has invalid incomplete outcomes" ); - let mut legacy = current; - legacy.incomplete_media[0].code = "invalid_external_media".to_string(); - let legacy_raw = serde_json::to_vec(&legacy).expect("legacy native"); - assert!(!String::from_utf8_lossy(&legacy_raw).contains(unsafe_url)); + for legacy_code in ["unsafe_external_media", "invalid_external_media"] { + let mut legacy = current.clone(); + legacy.incomplete_media[0].code = legacy_code.to_string(); + let legacy_raw = serde_json::to_vec(&legacy).expect("legacy native"); + assert!(!String::from_utf8_lossy(&legacy_raw).contains(unsafe_url)); - let rendered = connector - .render_portable(&portable_render_request( - page_id, - NativeEntity { - remote_id: RemoteId::new(page_id), - kind: "notion_page_portable_media_v1".to_string(), - raw: legacy_raw, - }, - )) - .expect("legacy generic outcome remains readable"); - assert_eq!( - rendered.completeness.incomplete_reasons(), - &[PortableIncompleteReason::ConnectorLimitation { - code: "notion_media_invalid_external_media".to_string(), - remote_id: Some(RemoteId::new(block_id)), - }] - ); - assert!(!String::from_utf8_lossy(&rendered.canonical.body).contains(unsafe_url)); + let rendered = connector + .render_portable(&portable_render_request( + page_id, + NativeEntity { + remote_id: RemoteId::new(page_id), + kind: "notion_page_portable_media_v1".to_string(), + raw: legacy_raw, + }, + )) + .expect("legacy generic outcome remains readable"); + assert_eq!( + rendered.completeness.incomplete_reasons(), + &[PortableIncompleteReason::ConnectorLimitation { + code: format!("notion_media_{legacy_code}"), + remote_id: Some(RemoteId::new(block_id)), + }], + "{legacy_code}" + ); + assert!(!String::from_utf8_lossy(&rendered.canonical.body).contains(unsafe_url)); + } } #[test] diff --git a/docs/notion-connector.md b/docs/notion-connector.md index 880da5e6..3d43f424 100644 --- a/docs/notion-connector.md +++ b/docs/notion-connector.md @@ -214,13 +214,23 @@ rejects unknown fields, alternate field order, or noncanonical whitespace. External media is never fetched. A valid external HTTPS URL remains an exact remote reference, including its explicit port, path, query, and fragment. An empty external URL is redacted and reports `unavailable_external_media`; -malformed, non-HTTPS, user-info-bearing, control-containing, oversized, or -otherwise invalid external URLs are redacted and report -`unsafe_external_media`. Older sanitized v1 payloads carrying the legacy +unsafe URLs are redacted and report one exact, URL-free outcome: + +| Condition | Portable outcome | +|---|---| +| Above the 8 KiB bound | `unsafe_external_media_too_long` | +| Contains whitespace or an ASCII control | `unsafe_external_media_whitespace_or_control` | +| Cannot be parsed as a URL | `unsafe_external_media_malformed` | +| Uses a scheme other than HTTPS | `unsafe_external_media_non_https` | +| Has no host | `unsafe_external_media_missing_host` | +| Contains user information | `unsafe_external_media_userinfo` | + +The outcome retains neither the URL nor parser details. Older sanitized v1 +payloads carrying the generic `unsafe_external_media` or `invalid_external_media` outcome remain readable, but new fetches never emit -that generic code. Production hosted-media capture accepts HTTPS on the default -port only, rejects user information and IP literals, and permits exactly these -origins: +either generic code. Production hosted-media capture accepts HTTPS on the +default port only, rejects user information and IP literals, and permits exactly +these origins: - `secure.notion-static.com`; - `prod-files-secure.s3.us-west-2.amazonaws.com`; diff --git a/docs/notion-object-support.md b/docs/notion-object-support.md index b0f58f25..8fad80c3 100644 --- a/docs/notion-object-support.md +++ b/docs/notion-object-support.md @@ -72,7 +72,7 @@ Sources used for the baseline: | API `unsupported` subtype-only artifact (`copy_indicator`, legacy `button`, `alias`) | Omitted | No | fixture, live API audit | The API exposes only `unsupported.block_type` and no Markdown content or target. These are treated as non-content UI/artifact blocks so they do not appear as visible `::loc` text. | | Unknown future block / other API `unsupported` block | Directive | No | fixture | Forward compatibility path: preserve block ID and avoid lossy edits. Literal Notion API `unsupported` blocks without an artifact subtype render with a human-readable directive title. | -Portable media omissions are explicit and redaction-safe. Genuine terminal or exhausted hosted unavailability reports `notion_media_unavailable_hosted_media`, the per-asset or aggregate size policy reports `notion_media_hosted_media_too_large`, and invalid hosted origin/redirect/encoding/expiry reports `notion_media_unsafe_hosted_media`. External media is never fetched: an empty URL reports `notion_media_unavailable_external_media`, while a malformed, non-HTTPS, user-info-bearing, control-containing, oversized, or otherwise invalid URL reports `notion_media_unsafe_external_media`; both are removed from native and rendered content. Valid external HTTPS remains an exact remote link. Older redacted v1 payloads with `notion_media_invalid_external_media` remain readable, but new fetches do not emit that generic code. Ambiguous-source, missing-file, page-property, embedded-secret, and asset-count-limit outcomes retain their fail-closed codes. Successful hosted captures use the same `media_local_path` layout and `.loc/media/` logical path as desktop hydration. +Portable media omissions are explicit and redaction-safe. Genuine terminal or exhausted hosted unavailability reports `notion_media_unavailable_hosted_media`, the per-asset or aggregate size policy reports `notion_media_hosted_media_too_large`, and invalid hosted origin/redirect/encoding/expiry reports `notion_media_unsafe_hosted_media`. External media is never fetched: an empty URL reports `notion_media_unavailable_external_media`; unsafe inputs report the exact URL-free suffix `too_long`, `whitespace_or_control`, `malformed`, `non_https`, `missing_host`, or `userinfo` under `notion_media_unsafe_external_media_`. Every omitted URL is removed from native and rendered content. Valid external HTTPS remains an exact remote link. Older redacted v1 payloads with generic `notion_media_unsafe_external_media` or `notion_media_invalid_external_media` outcomes remain readable, but new fetches do not emit either generic code. Ambiguous-source, missing-file, page-property, embedded-secret, and asset-count-limit outcomes retain their fail-closed codes. Successful hosted captures use the same `media_local_path` layout and `.loc/media/` logical path as desktop hydration. ## Rich Text From 70a7d2ff33f7ee4697a56c4cb61f499eae308f3c Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 03:09:48 -0700 Subject: [PATCH 08/13] Bound sandbox export HTTP read ahead --- crates/loc-cli/src/sandbox.rs | 272 +++++++++++++++++++++++++++++++- crates/loc-cli/tests/sandbox.rs | 179 ++++++++++++++++++++- 2 files changed, 443 insertions(+), 8 deletions(-) diff --git a/crates/loc-cli/src/sandbox.rs b/crates/loc-cli/src/sandbox.rs index ab290bca..b33fe2b8 100644 --- a/crates/loc-cli/src/sandbox.rs +++ b/crates/loc-cli/src/sandbox.rs @@ -11,6 +11,8 @@ use std::io::{self, Read}; use std::net::IpAddr; use std::path::{Path, PathBuf}; use std::sync::OnceLock; +use std::sync::mpsc::{Receiver, SyncSender, sync_channel}; +use std::thread::{self, JoinHandle}; use std::time::Duration; use locality_protocol::{ @@ -37,6 +39,8 @@ const HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(30 * 60); const BOOTSTRAP_EXCHANGE_ATTEMPTS: usize = 2; const BOOTSTRAP_IDEMPOTENCY_DOMAIN: &[u8] = b"locality.session-exchange-idempotency.v1\0"; const IDEMPOTENCY_KEY_HEADER: &str = "Idempotency-Key"; +const EXPORT_READ_AHEAD_CHUNK_BYTES: usize = 64 * 1024; +const EXPORT_READ_AHEAD_CHUNKS: usize = 8; static REQWEST_CRYPTO_PROVIDER: OnceLock<()> = OnceLock::new(); #[derive(Clone)] @@ -369,14 +373,173 @@ pub fn run_sandbox_init_with_encoding( validate_encoding_preference(offer, content_encoding)?; let limits = limits_for_offer(offer)?; let (encoding, response) = client.open_export(&capability, offer, content_encoding)?; - let archive = ReplicaArchive::new(encoding, response); - let summary = + let (body, mut producer) = + spawn_export_read_ahead(response).map_err(|error| SandboxInitError::Http { + operation: "session export read-ahead setup", + detail: error.to_string(), + })?; + let archive = ReplicaArchive::new(encoding, body); + let materialization = materialize_replica_archive_with_expected_receipt(archive, &root, limits, expected_receipt) - .map_err(|error| SandboxInitError::Materialization(error.to_string()))?; + .map_err(|error| SandboxInitError::Materialization(error.to_string())); + let producer_outcome = producer.join(); + + let summary = match materialization { + Err(error) => return Err(error), + Ok(summary) => { + match producer_outcome { + Ok(ReadAheadProducerOutcome::CleanEof) => {} + Ok( + ReadAheadProducerOutcome::ConsumerClosed + | ReadAheadProducerOutcome::ErrorDelivered, + ) => { + return Err(SandboxInitError::Materialization( + "sandbox export transport ended without a clean EOF".to_string(), + )); + } + Err(()) => { + return Err(SandboxInitError::Materialization( + "sandbox export read-ahead worker panicked".to_string(), + )); + } + } + summary + } + }; Ok(report(&root, &capability, encoding, summary)) } +enum ReadAheadMessage { + Data(Vec), + Error(io::Error), + CleanEof, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum ReadAheadProducerOutcome { + CleanEof, + ConsumerClosed, + ErrorDelivered, +} + +struct ExportReadAhead { + receiver: Receiver, + current: Vec, + offset: usize, + clean_eof: bool, +} + +impl Read for ExportReadAhead { + fn read(&mut self, output: &mut [u8]) -> io::Result { + if output.is_empty() { + return Ok(0); + } + if self.clean_eof { + return Ok(0); + } + + loop { + if self.offset < self.current.len() { + let available = &self.current[self.offset..]; + let copied = available.len().min(output.len()); + output[..copied].copy_from_slice(&available[..copied]); + self.offset += copied; + return Ok(copied); + } + + match self.receiver.recv() { + Ok(ReadAheadMessage::Data(chunk)) => { + self.current = chunk; + self.offset = 0; + } + Ok(ReadAheadMessage::Error(error)) => return Err(error), + Ok(ReadAheadMessage::CleanEof) => { + self.clean_eof = true; + return Ok(0); + } + Err(_) => { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "sandbox export read-ahead producer stopped before EOF", + )); + } + } + } + } +} + +struct ReadAheadProducer { + handle: Option>, +} + +impl ReadAheadProducer { + fn join(&mut self) -> Result { + let handle = self.handle.take().ok_or(())?; + handle.join().map_err(|_| ()) + } +} + +impl Drop for ReadAheadProducer { + fn drop(&mut self) { + if let Some(handle) = self.handle.take() { + let _ = handle.join(); + } + } +} + +fn spawn_export_read_ahead(body: Body) -> io::Result<(ExportReadAhead, ReadAheadProducer)> +where + Body: Read + Send + 'static, +{ + let (sender, receiver) = sync_channel(EXPORT_READ_AHEAD_CHUNKS); + let handle = thread::Builder::new() + .name("locality-export-read-ahead".to_string()) + .spawn(move || produce_export(body, &sender))?; + Ok(( + ExportReadAhead { + receiver, + current: Vec::new(), + offset: 0, + clean_eof: false, + }, + ReadAheadProducer { + handle: Some(handle), + }, + )) +} + +fn produce_export( + mut body: Body, + sender: &SyncSender, +) -> ReadAheadProducerOutcome { + loop { + let mut chunk = vec![0_u8; EXPORT_READ_AHEAD_CHUNK_BYTES]; + match body.read(&mut chunk) { + Ok(0) => { + return if sender.send(ReadAheadMessage::CleanEof).is_ok() { + ReadAheadProducerOutcome::CleanEof + } else { + ReadAheadProducerOutcome::ConsumerClosed + }; + } + Ok(read) => { + chunk.truncate(read); + if sender.send(ReadAheadMessage::Data(chunk)).is_err() { + return ReadAheadProducerOutcome::ConsumerClosed; + } + } + Err(error) => { + return if sender.send(ReadAheadMessage::Error(error)).is_ok() { + ReadAheadProducerOutcome::ErrorDelivered + } else { + ReadAheadProducerOutcome::ConsumerClosed + }; + } + } + } +} + fn absolute_destination(path: &Path) -> Result { if path.is_absolute() { Ok(path.to_path_buf()) @@ -891,6 +1054,8 @@ mod tests { use std::collections::BTreeMap; use std::io::{self, Read, Write}; use std::net::{TcpListener, TcpStream}; + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::mpsc::{self, Receiver}; use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; @@ -899,6 +1064,37 @@ mod tests { use super::*; + struct FixedChunkBody { + reads: Arc, + } + + impl Read for FixedChunkBody { + fn read(&mut self, output: &mut [u8]) -> io::Result { + self.reads.fetch_add(1, Ordering::SeqCst); + output.fill(0x5a); + Ok(output.len()) + } + } + + struct FailingBody { + first_read: bool, + } + + impl Read for FailingBody { + fn read(&mut self, output: &mut [u8]) -> io::Result { + if self.first_read { + self.first_read = false; + output[..3].copy_from_slice(b"abc"); + Ok(3) + } else { + Err(io::Error::new( + io::ErrorKind::ConnectionReset, + "sentinel export transport failure", + )) + } + } + } + #[derive(Debug)] struct CapturedRequest { method: String, @@ -971,6 +1167,76 @@ mod tests { } } + #[test] + fn export_read_ahead_is_byte_bounded_and_consumer_drop_unblocks_producer() { + let reads = Arc::new(AtomicUsize::new(0)); + let (reader, mut producer) = spawn_export_read_ahead(FixedChunkBody { + reads: Arc::clone(&reads), + }) + .expect("start read-ahead producer"); + let expected_reads = EXPORT_READ_AHEAD_CHUNKS + 1; + let deadline = Instant::now() + Duration::from_secs(2); + while reads.load(Ordering::SeqCst) < expected_reads { + assert!( + Instant::now() < deadline, + "producer did not fill bounded queue" + ); + thread::yield_now(); + } + thread::sleep(Duration::from_millis(25)); + assert_eq!( + reads.load(Ordering::SeqCst), + expected_reads, + "the producer may hold only eight queued chunks and one in-flight chunk" + ); + + drop(reader); + assert_eq!( + producer.join(), + Ok(ReadAheadProducerOutcome::ConsumerClosed), + "dropping a rejecting consumer must promptly release a blocked producer" + ); + } + + #[test] + fn export_read_ahead_delivers_the_original_io_error() { + let (mut reader, mut producer) = spawn_export_read_ahead(FailingBody { first_read: true }) + .expect("start read-ahead producer"); + let mut prefix = [0_u8; 3]; + reader.read_exact(&mut prefix).expect("read prefix"); + assert_eq!(&prefix, b"abc"); + + let error = reader.read(&mut [0_u8; 1]).expect_err("transport fails"); + assert_eq!(error.kind(), io::ErrorKind::ConnectionReset); + assert_eq!(error.to_string(), "sentinel export transport failure"); + drop(reader); + assert_eq!( + producer.join(), + Ok(ReadAheadProducerOutcome::ErrorDelivered) + ); + } + + #[test] + fn export_read_ahead_disconnect_is_not_mistaken_for_clean_eof() { + let (sender, receiver) = sync_channel(1); + drop(sender); + let mut reader = ExportReadAhead { + receiver, + current: Vec::new(), + offset: 0, + clean_eof: false, + }; + + let error = reader + .read(&mut [0_u8; 1]) + .expect_err("disconnect without an EOF marker must fail"); + assert_eq!(error.kind(), io::ErrorKind::UnexpectedEof); + assert_eq!( + error.to_string(), + "sandbox export read-ahead producer stopped before EOF" + ); + } + #[test] fn dropped_bootstrap_response_retries_with_identical_key_and_body() { let response = serde_json::to_vec(&capability()).expect("serialize capability"); diff --git a/crates/loc-cli/tests/sandbox.rs b/crates/loc-cli/tests/sandbox.rs index 0ee39eef..2c82e2d2 100644 --- a/crates/loc-cli/tests/sandbox.rs +++ b/crates/loc-cli/tests/sandbox.rs @@ -62,6 +62,9 @@ struct ResponseFixture { status: &'static str, headers: Vec<(&'static str, &'static str)>, body: Vec, + declared_content_length: Option, + split_after: Option<(usize, Duration)>, + staging_gate: Option<(PathBuf, PathBuf, PathBuf)>, } impl ResponseFixture { @@ -70,6 +73,9 @@ impl ResponseFixture { status: "200 OK", headers: vec![("Content-Type", "application/json")], body: serde_json::to_vec(value).expect("serialize response"), + declared_content_length: None, + split_after: None, + staging_gate: None, } } @@ -81,8 +87,40 @@ impl ResponseFixture { ("Content-Encoding", encoding), ], body, + declared_content_length: None, + split_after: None, + staging_gate: None, } } + + fn streaming_export( + encoding: &'static str, + body: Vec, + split_after: usize, + pause: Duration, + ) -> Self { + Self::export(encoding, body).with_split_after(split_after, pause) + } + + fn with_declared_content_length(mut self, length: usize) -> Self { + self.declared_content_length = Some(length); + self + } + + fn with_split_after(mut self, bytes: usize, pause: Duration) -> Self { + self.split_after = Some((bytes, pause)); + self + } + + fn with_staging_gate( + mut self, + parent: PathBuf, + logical_path: PathBuf, + destination: PathBuf, + ) -> Self { + self.staging_gate = Some((parent, logical_path, destination)); + self + } } struct MockServer { @@ -281,6 +319,89 @@ fn zstd_bootstrap_streams_into_the_shared_materializer() { ); } +#[test] +fn export_bytes_are_staged_while_the_http_response_is_still_streaming() { + let directory = TestDirectory::new("streaming-overlap"); + let body = vec![0x5a; 256 * 1024]; + let tar = tar_file(b"large.bin", &body); + let capability = capability(); + let status = ready_status( + capability.session_id.clone(), + COMPONENT_VERSIONS, + &tar, + BTreeSet::from([TarContentEncoding::Identity]), + ); + let destination = directory.root(); + let response = ResponseFixture::streaming_export( + "identity", + tar, + 512 + 64 * 1024, + Duration::from_millis(10), + ) + .with_staging_gate( + destination + .parent() + .expect("destination parent") + .to_path_buf(), + PathBuf::from("large.bin"), + destination.clone(), + ); + let server = MockServer::start(vec![ + ResponseFixture::json(&capability), + ResponseFixture::json(&status), + response, + ]); + + let report = run_sandbox_init( + SandboxInitOptions { + api_url: server.api_url.clone(), + root: destination.clone(), + }, + SandboxBootstrapToken::new("bootstrap-secret").expect("token"), + ) + .expect("stream and publish export"); + + assert_eq!(report.files, 1); + assert_eq!(report.materialized_bytes, body.len() as u64); + assert_eq!( + fs::read(destination.join("large.bin")).expect("read published file"), + body + ); +} + +#[test] +fn truncated_http_body_producer_error_prevents_publication() { + let directory = TestDirectory::new("transport-truncation"); + let tar = tar_file(b"complete-before-http-eof.txt", b"complete\n"); + let capability = capability(); + let status = ready_status( + capability.session_id.clone(), + COMPONENT_VERSIONS, + &tar, + BTreeSet::from([TarContentEncoding::Identity]), + ); + let declared_length = tar.len() + 128; + let server = MockServer::start(vec![ + ResponseFixture::json(&capability), + ResponseFixture::json(&status), + ResponseFixture::export("identity", tar).with_declared_content_length(declared_length), + ]); + + let error = run_sandbox_init( + SandboxInitOptions { + api_url: server.api_url.clone(), + root: directory.root(), + }, + SandboxBootstrapToken::new("bootstrap-secret").expect("token"), + ) + .expect_err("an incomplete HTTP response must not publish a complete-looking tar"); + + assert_eq!(error.code(), "materialization_failed"); + assert!(!directory.root().exists()); + assert!(!error.to_string().contains("bootstrap-secret")); + assert!(!error.to_string().contains("capability-secret")); +} + #[test] fn forced_content_encodings_send_exact_headers_and_match_reports() { let tar = tar_file(b"forced.txt", b"forced\n"); @@ -602,6 +723,9 @@ fn bearer_authenticated_redirect_is_not_followed() { status: "302 Found", headers: vec![("Location", "/redirected")], body: Vec::new(), + declared_content_length: None, + split_after: None, + staging_gate: None, }, ]); @@ -773,6 +897,9 @@ fn version_session_offer_media_and_response_encoding_are_validated() { status: "200 OK", headers: vec![("Content-Type", "application/octet-stream")], body: tar.clone(), + declared_content_length: None, + split_after: None, + staging_gate: None, }), "backend_protocol_invalid", ), @@ -1081,20 +1208,62 @@ fn read_request(stream: &mut TcpStream) -> CapturedRequest { } fn write_response(stream: &mut TcpStream, response: ResponseFixture) { + let content_length = response + .declared_content_length + .unwrap_or(response.body.len()); write!( stream, "HTTP/1.1 {}\r\nContent-Length: {}\r\nConnection: close\r\n", - response.status, - response.body.len() + response.status, content_length ) .expect("write response head"); for (name, value) in response.headers { write!(stream, "{name}: {value}\r\n").expect("write response header"); } write!(stream, "\r\n").expect("finish response headers"); - stream - .write_all(&response.body) - .expect("write response body"); + if let Some((split_after, pause)) = response.split_after { + let split_after = split_after.min(response.body.len()); + stream + .write_all(&response.body[..split_after]) + .expect("write first response body chunk"); + stream.flush().expect("flush first response body chunk"); + if let Some((parent, logical_path, destination)) = &response.staging_gate { + let deadline = std::time::Instant::now() + Duration::from_secs(2); + loop { + let staged = fs::read_dir(parent) + .into_iter() + .flatten() + .flatten() + .any(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with(".locality-stage-") + && entry.path().join(logical_path).is_file() + }); + if staged { + assert!( + !destination.exists(), + "the destination must remain absent while the response is incomplete" + ); + break; + } + assert!( + std::time::Instant::now() < deadline, + "materializer did not stage the first file while the response was paused" + ); + thread::sleep(Duration::from_millis(2)); + } + } + thread::sleep(pause); + stream + .write_all(&response.body[split_after..]) + .expect("write remaining response body"); + } else { + stream + .write_all(&response.body) + .expect("write response body"); + } stream.flush().expect("flush response"); } From 782553c745189bf61c60723c1d8119f586d56ccb Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 04:11:48 -0700 Subject: [PATCH 09/13] Revert "Bound sandbox export HTTP read ahead" This reverts commit 70a7d2ff33f7ee4697a56c4cb61f499eae308f3c. --- crates/loc-cli/src/sandbox.rs | 272 +------------------------------- crates/loc-cli/tests/sandbox.rs | 179 +-------------------- 2 files changed, 8 insertions(+), 443 deletions(-) diff --git a/crates/loc-cli/src/sandbox.rs b/crates/loc-cli/src/sandbox.rs index b33fe2b8..ab290bca 100644 --- a/crates/loc-cli/src/sandbox.rs +++ b/crates/loc-cli/src/sandbox.rs @@ -11,8 +11,6 @@ use std::io::{self, Read}; use std::net::IpAddr; use std::path::{Path, PathBuf}; use std::sync::OnceLock; -use std::sync::mpsc::{Receiver, SyncSender, sync_channel}; -use std::thread::{self, JoinHandle}; use std::time::Duration; use locality_protocol::{ @@ -39,8 +37,6 @@ const HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(30 * 60); const BOOTSTRAP_EXCHANGE_ATTEMPTS: usize = 2; const BOOTSTRAP_IDEMPOTENCY_DOMAIN: &[u8] = b"locality.session-exchange-idempotency.v1\0"; const IDEMPOTENCY_KEY_HEADER: &str = "Idempotency-Key"; -const EXPORT_READ_AHEAD_CHUNK_BYTES: usize = 64 * 1024; -const EXPORT_READ_AHEAD_CHUNKS: usize = 8; static REQWEST_CRYPTO_PROVIDER: OnceLock<()> = OnceLock::new(); #[derive(Clone)] @@ -373,173 +369,14 @@ pub fn run_sandbox_init_with_encoding( validate_encoding_preference(offer, content_encoding)?; let limits = limits_for_offer(offer)?; let (encoding, response) = client.open_export(&capability, offer, content_encoding)?; - let (body, mut producer) = - spawn_export_read_ahead(response).map_err(|error| SandboxInitError::Http { - operation: "session export read-ahead setup", - detail: error.to_string(), - })?; - let archive = ReplicaArchive::new(encoding, body); - let materialization = + let archive = ReplicaArchive::new(encoding, response); + let summary = materialize_replica_archive_with_expected_receipt(archive, &root, limits, expected_receipt) - .map_err(|error| SandboxInitError::Materialization(error.to_string())); - let producer_outcome = producer.join(); - - let summary = match materialization { - Err(error) => return Err(error), - Ok(summary) => { - match producer_outcome { - Ok(ReadAheadProducerOutcome::CleanEof) => {} - Ok( - ReadAheadProducerOutcome::ConsumerClosed - | ReadAheadProducerOutcome::ErrorDelivered, - ) => { - return Err(SandboxInitError::Materialization( - "sandbox export transport ended without a clean EOF".to_string(), - )); - } - Err(()) => { - return Err(SandboxInitError::Materialization( - "sandbox export read-ahead worker panicked".to_string(), - )); - } - } - summary - } - }; + .map_err(|error| SandboxInitError::Materialization(error.to_string()))?; Ok(report(&root, &capability, encoding, summary)) } -enum ReadAheadMessage { - Data(Vec), - Error(io::Error), - CleanEof, -} - -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum ReadAheadProducerOutcome { - CleanEof, - ConsumerClosed, - ErrorDelivered, -} - -struct ExportReadAhead { - receiver: Receiver, - current: Vec, - offset: usize, - clean_eof: bool, -} - -impl Read for ExportReadAhead { - fn read(&mut self, output: &mut [u8]) -> io::Result { - if output.is_empty() { - return Ok(0); - } - if self.clean_eof { - return Ok(0); - } - - loop { - if self.offset < self.current.len() { - let available = &self.current[self.offset..]; - let copied = available.len().min(output.len()); - output[..copied].copy_from_slice(&available[..copied]); - self.offset += copied; - return Ok(copied); - } - - match self.receiver.recv() { - Ok(ReadAheadMessage::Data(chunk)) => { - self.current = chunk; - self.offset = 0; - } - Ok(ReadAheadMessage::Error(error)) => return Err(error), - Ok(ReadAheadMessage::CleanEof) => { - self.clean_eof = true; - return Ok(0); - } - Err(_) => { - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - "sandbox export read-ahead producer stopped before EOF", - )); - } - } - } - } -} - -struct ReadAheadProducer { - handle: Option>, -} - -impl ReadAheadProducer { - fn join(&mut self) -> Result { - let handle = self.handle.take().ok_or(())?; - handle.join().map_err(|_| ()) - } -} - -impl Drop for ReadAheadProducer { - fn drop(&mut self) { - if let Some(handle) = self.handle.take() { - let _ = handle.join(); - } - } -} - -fn spawn_export_read_ahead(body: Body) -> io::Result<(ExportReadAhead, ReadAheadProducer)> -where - Body: Read + Send + 'static, -{ - let (sender, receiver) = sync_channel(EXPORT_READ_AHEAD_CHUNKS); - let handle = thread::Builder::new() - .name("locality-export-read-ahead".to_string()) - .spawn(move || produce_export(body, &sender))?; - Ok(( - ExportReadAhead { - receiver, - current: Vec::new(), - offset: 0, - clean_eof: false, - }, - ReadAheadProducer { - handle: Some(handle), - }, - )) -} - -fn produce_export( - mut body: Body, - sender: &SyncSender, -) -> ReadAheadProducerOutcome { - loop { - let mut chunk = vec![0_u8; EXPORT_READ_AHEAD_CHUNK_BYTES]; - match body.read(&mut chunk) { - Ok(0) => { - return if sender.send(ReadAheadMessage::CleanEof).is_ok() { - ReadAheadProducerOutcome::CleanEof - } else { - ReadAheadProducerOutcome::ConsumerClosed - }; - } - Ok(read) => { - chunk.truncate(read); - if sender.send(ReadAheadMessage::Data(chunk)).is_err() { - return ReadAheadProducerOutcome::ConsumerClosed; - } - } - Err(error) => { - return if sender.send(ReadAheadMessage::Error(error)).is_ok() { - ReadAheadProducerOutcome::ErrorDelivered - } else { - ReadAheadProducerOutcome::ConsumerClosed - }; - } - } - } -} - fn absolute_destination(path: &Path) -> Result { if path.is_absolute() { Ok(path.to_path_buf()) @@ -1054,8 +891,6 @@ mod tests { use std::collections::BTreeMap; use std::io::{self, Read, Write}; use std::net::{TcpListener, TcpStream}; - use std::sync::Arc; - use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::mpsc::{self, Receiver}; use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; @@ -1064,37 +899,6 @@ mod tests { use super::*; - struct FixedChunkBody { - reads: Arc, - } - - impl Read for FixedChunkBody { - fn read(&mut self, output: &mut [u8]) -> io::Result { - self.reads.fetch_add(1, Ordering::SeqCst); - output.fill(0x5a); - Ok(output.len()) - } - } - - struct FailingBody { - first_read: bool, - } - - impl Read for FailingBody { - fn read(&mut self, output: &mut [u8]) -> io::Result { - if self.first_read { - self.first_read = false; - output[..3].copy_from_slice(b"abc"); - Ok(3) - } else { - Err(io::Error::new( - io::ErrorKind::ConnectionReset, - "sentinel export transport failure", - )) - } - } - } - #[derive(Debug)] struct CapturedRequest { method: String, @@ -1167,76 +971,6 @@ mod tests { } } - #[test] - fn export_read_ahead_is_byte_bounded_and_consumer_drop_unblocks_producer() { - let reads = Arc::new(AtomicUsize::new(0)); - let (reader, mut producer) = spawn_export_read_ahead(FixedChunkBody { - reads: Arc::clone(&reads), - }) - .expect("start read-ahead producer"); - let expected_reads = EXPORT_READ_AHEAD_CHUNKS + 1; - let deadline = Instant::now() + Duration::from_secs(2); - while reads.load(Ordering::SeqCst) < expected_reads { - assert!( - Instant::now() < deadline, - "producer did not fill bounded queue" - ); - thread::yield_now(); - } - thread::sleep(Duration::from_millis(25)); - assert_eq!( - reads.load(Ordering::SeqCst), - expected_reads, - "the producer may hold only eight queued chunks and one in-flight chunk" - ); - - drop(reader); - assert_eq!( - producer.join(), - Ok(ReadAheadProducerOutcome::ConsumerClosed), - "dropping a rejecting consumer must promptly release a blocked producer" - ); - } - - #[test] - fn export_read_ahead_delivers_the_original_io_error() { - let (mut reader, mut producer) = spawn_export_read_ahead(FailingBody { first_read: true }) - .expect("start read-ahead producer"); - let mut prefix = [0_u8; 3]; - reader.read_exact(&mut prefix).expect("read prefix"); - assert_eq!(&prefix, b"abc"); - - let error = reader.read(&mut [0_u8; 1]).expect_err("transport fails"); - assert_eq!(error.kind(), io::ErrorKind::ConnectionReset); - assert_eq!(error.to_string(), "sentinel export transport failure"); - drop(reader); - assert_eq!( - producer.join(), - Ok(ReadAheadProducerOutcome::ErrorDelivered) - ); - } - - #[test] - fn export_read_ahead_disconnect_is_not_mistaken_for_clean_eof() { - let (sender, receiver) = sync_channel(1); - drop(sender); - let mut reader = ExportReadAhead { - receiver, - current: Vec::new(), - offset: 0, - clean_eof: false, - }; - - let error = reader - .read(&mut [0_u8; 1]) - .expect_err("disconnect without an EOF marker must fail"); - assert_eq!(error.kind(), io::ErrorKind::UnexpectedEof); - assert_eq!( - error.to_string(), - "sandbox export read-ahead producer stopped before EOF" - ); - } - #[test] fn dropped_bootstrap_response_retries_with_identical_key_and_body() { let response = serde_json::to_vec(&capability()).expect("serialize capability"); diff --git a/crates/loc-cli/tests/sandbox.rs b/crates/loc-cli/tests/sandbox.rs index 2c82e2d2..0ee39eef 100644 --- a/crates/loc-cli/tests/sandbox.rs +++ b/crates/loc-cli/tests/sandbox.rs @@ -62,9 +62,6 @@ struct ResponseFixture { status: &'static str, headers: Vec<(&'static str, &'static str)>, body: Vec, - declared_content_length: Option, - split_after: Option<(usize, Duration)>, - staging_gate: Option<(PathBuf, PathBuf, PathBuf)>, } impl ResponseFixture { @@ -73,9 +70,6 @@ impl ResponseFixture { status: "200 OK", headers: vec![("Content-Type", "application/json")], body: serde_json::to_vec(value).expect("serialize response"), - declared_content_length: None, - split_after: None, - staging_gate: None, } } @@ -87,40 +81,8 @@ impl ResponseFixture { ("Content-Encoding", encoding), ], body, - declared_content_length: None, - split_after: None, - staging_gate: None, } } - - fn streaming_export( - encoding: &'static str, - body: Vec, - split_after: usize, - pause: Duration, - ) -> Self { - Self::export(encoding, body).with_split_after(split_after, pause) - } - - fn with_declared_content_length(mut self, length: usize) -> Self { - self.declared_content_length = Some(length); - self - } - - fn with_split_after(mut self, bytes: usize, pause: Duration) -> Self { - self.split_after = Some((bytes, pause)); - self - } - - fn with_staging_gate( - mut self, - parent: PathBuf, - logical_path: PathBuf, - destination: PathBuf, - ) -> Self { - self.staging_gate = Some((parent, logical_path, destination)); - self - } } struct MockServer { @@ -319,89 +281,6 @@ fn zstd_bootstrap_streams_into_the_shared_materializer() { ); } -#[test] -fn export_bytes_are_staged_while_the_http_response_is_still_streaming() { - let directory = TestDirectory::new("streaming-overlap"); - let body = vec![0x5a; 256 * 1024]; - let tar = tar_file(b"large.bin", &body); - let capability = capability(); - let status = ready_status( - capability.session_id.clone(), - COMPONENT_VERSIONS, - &tar, - BTreeSet::from([TarContentEncoding::Identity]), - ); - let destination = directory.root(); - let response = ResponseFixture::streaming_export( - "identity", - tar, - 512 + 64 * 1024, - Duration::from_millis(10), - ) - .with_staging_gate( - destination - .parent() - .expect("destination parent") - .to_path_buf(), - PathBuf::from("large.bin"), - destination.clone(), - ); - let server = MockServer::start(vec![ - ResponseFixture::json(&capability), - ResponseFixture::json(&status), - response, - ]); - - let report = run_sandbox_init( - SandboxInitOptions { - api_url: server.api_url.clone(), - root: destination.clone(), - }, - SandboxBootstrapToken::new("bootstrap-secret").expect("token"), - ) - .expect("stream and publish export"); - - assert_eq!(report.files, 1); - assert_eq!(report.materialized_bytes, body.len() as u64); - assert_eq!( - fs::read(destination.join("large.bin")).expect("read published file"), - body - ); -} - -#[test] -fn truncated_http_body_producer_error_prevents_publication() { - let directory = TestDirectory::new("transport-truncation"); - let tar = tar_file(b"complete-before-http-eof.txt", b"complete\n"); - let capability = capability(); - let status = ready_status( - capability.session_id.clone(), - COMPONENT_VERSIONS, - &tar, - BTreeSet::from([TarContentEncoding::Identity]), - ); - let declared_length = tar.len() + 128; - let server = MockServer::start(vec![ - ResponseFixture::json(&capability), - ResponseFixture::json(&status), - ResponseFixture::export("identity", tar).with_declared_content_length(declared_length), - ]); - - let error = run_sandbox_init( - SandboxInitOptions { - api_url: server.api_url.clone(), - root: directory.root(), - }, - SandboxBootstrapToken::new("bootstrap-secret").expect("token"), - ) - .expect_err("an incomplete HTTP response must not publish a complete-looking tar"); - - assert_eq!(error.code(), "materialization_failed"); - assert!(!directory.root().exists()); - assert!(!error.to_string().contains("bootstrap-secret")); - assert!(!error.to_string().contains("capability-secret")); -} - #[test] fn forced_content_encodings_send_exact_headers_and_match_reports() { let tar = tar_file(b"forced.txt", b"forced\n"); @@ -723,9 +602,6 @@ fn bearer_authenticated_redirect_is_not_followed() { status: "302 Found", headers: vec![("Location", "/redirected")], body: Vec::new(), - declared_content_length: None, - split_after: None, - staging_gate: None, }, ]); @@ -897,9 +773,6 @@ fn version_session_offer_media_and_response_encoding_are_validated() { status: "200 OK", headers: vec![("Content-Type", "application/octet-stream")], body: tar.clone(), - declared_content_length: None, - split_after: None, - staging_gate: None, }), "backend_protocol_invalid", ), @@ -1208,62 +1081,20 @@ fn read_request(stream: &mut TcpStream) -> CapturedRequest { } fn write_response(stream: &mut TcpStream, response: ResponseFixture) { - let content_length = response - .declared_content_length - .unwrap_or(response.body.len()); write!( stream, "HTTP/1.1 {}\r\nContent-Length: {}\r\nConnection: close\r\n", - response.status, content_length + response.status, + response.body.len() ) .expect("write response head"); for (name, value) in response.headers { write!(stream, "{name}: {value}\r\n").expect("write response header"); } write!(stream, "\r\n").expect("finish response headers"); - if let Some((split_after, pause)) = response.split_after { - let split_after = split_after.min(response.body.len()); - stream - .write_all(&response.body[..split_after]) - .expect("write first response body chunk"); - stream.flush().expect("flush first response body chunk"); - if let Some((parent, logical_path, destination)) = &response.staging_gate { - let deadline = std::time::Instant::now() + Duration::from_secs(2); - loop { - let staged = fs::read_dir(parent) - .into_iter() - .flatten() - .flatten() - .any(|entry| { - entry - .file_name() - .to_string_lossy() - .starts_with(".locality-stage-") - && entry.path().join(logical_path).is_file() - }); - if staged { - assert!( - !destination.exists(), - "the destination must remain absent while the response is incomplete" - ); - break; - } - assert!( - std::time::Instant::now() < deadline, - "materializer did not stage the first file while the response was paused" - ); - thread::sleep(Duration::from_millis(2)); - } - } - thread::sleep(pause); - stream - .write_all(&response.body[split_after..]) - .expect("write remaining response body"); - } else { - stream - .write_all(&response.body) - .expect("write response body"); - } + stream + .write_all(&response.body) + .expect("write response body"); stream.flush().expect("flush response"); } From db07d04dd425dfaaf0dc854f956274e22be409fb Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 04:24:20 -0700 Subject: [PATCH 10/13] Classify malformed external media exactly --- crates/locality-notion/src/media.rs | 2 +- crates/locality-notion/src/portable.rs | 1 + crates/locality-notion/tests/fetch_render.rs | 8 ++++++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/locality-notion/src/media.rs b/crates/locality-notion/src/media.rs index 33939120..0507bf56 100644 --- a/crates/locality-notion/src/media.rs +++ b/crates/locality-notion/src/media.rs @@ -503,7 +503,7 @@ impl PortableExternalMediaUrlFailure { Self::Unavailable => "unavailable_external_media", Self::TooLong => "unsafe_external_media_too_long", Self::WhitespaceOrControl => "unsafe_external_media_whitespace_or_control", - Self::Malformed => "unsafe_external_media_malformed", + Self::Malformed => "external_media_malformed", Self::NonHttps => "unsafe_external_media_non_https", Self::MissingHost => "unsafe_external_media_missing_host", Self::Userinfo => "unsafe_external_media_userinfo", diff --git a/crates/locality-notion/src/portable.rs b/crates/locality-notion/src/portable.rs index 0ef74903..ad356c8f 100644 --- a/crates/locality-notion/src/portable.rs +++ b/crates/locality-notion/src/portable.rs @@ -1325,6 +1325,7 @@ fn actual_external_incomplete_code<'a>( }; match outcome.code.as_str() { "unavailable_external_media" + | "external_media_malformed" | "unsafe_external_media_too_long" | "unsafe_external_media_whitespace_or_control" | "unsafe_external_media_malformed" diff --git a/crates/locality-notion/tests/fetch_render.rs b/crates/locality-notion/tests/fetch_render.rs index 0743bd7e..85c2afcf 100644 --- a/crates/locality-notion/tests/fetch_render.rs +++ b/crates/locality-notion/tests/fetch_render.rs @@ -3553,7 +3553,7 @@ fn portable_external_media_fails_closed_without_fetching_bad_or_ambiguous_source ( "malformed", "not-a-url".to_string(), - "unsafe_external_media_malformed", + "external_media_malformed", ), ( "non-https", @@ -3756,7 +3756,11 @@ fn portable_external_media_legacy_generic_outcome_remains_readable() { "invalid state: Notion portable media native payload has invalid incomplete outcomes" ); - for legacy_code in ["unsafe_external_media", "invalid_external_media"] { + for legacy_code in [ + "unsafe_external_media", + "unsafe_external_media_malformed", + "invalid_external_media", + ] { let mut legacy = current.clone(); legacy.incomplete_media[0].code = legacy_code.to_string(); let legacy_raw = serde_json::to_vec(&legacy).expect("legacy native"); From 1a38aa2d314383e35f4aaa5f74b64f5f6b070954 Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 04:42:49 -0700 Subject: [PATCH 11/13] Pipeline sandbox export materialization --- crates/loc-cli/src/sandbox.rs | 454 +++++++++++++++++++++++++++++++- crates/loc-cli/tests/sandbox.rs | 179 ++++++++++++- 2 files changed, 622 insertions(+), 11 deletions(-) diff --git a/crates/loc-cli/src/sandbox.rs b/crates/loc-cli/src/sandbox.rs index ab290bca..e92326c9 100644 --- a/crates/loc-cli/src/sandbox.rs +++ b/crates/loc-cli/src/sandbox.rs @@ -11,6 +11,8 @@ use std::io::{self, Read}; use std::net::IpAddr; use std::path::{Path, PathBuf}; use std::sync::OnceLock; +use std::sync::mpsc::{Receiver, SyncSender, sync_channel}; +use std::thread::{self, JoinHandle}; use std::time::Duration; use locality_protocol::{ @@ -34,9 +36,15 @@ const TAR_MEDIA_TYPE: &str = "application/x-tar"; const MAX_JSON_RESPONSE_BYTES: u64 = 1024 * 1024; const HTTP_CONNECT_TIMEOUT: Duration = Duration::from_secs(10); const HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(30 * 60); +/// Reqwest's blocking response reapplies the client's operation timeout to +/// each `Read`. A dedicated export client therefore bounds idle body reads +/// without imposing a 60-second total limit on a progressing export. +const HTTP_READ_TIMEOUT: Duration = Duration::from_secs(60); const BOOTSTRAP_EXCHANGE_ATTEMPTS: usize = 2; const BOOTSTRAP_IDEMPOTENCY_DOMAIN: &[u8] = b"locality.session-exchange-idempotency.v1\0"; const IDEMPOTENCY_KEY_HEADER: &str = "Idempotency-Key"; +const EXPORT_READ_AHEAD_CHUNK_BYTES: usize = 64 * 1024; +const EXPORT_READ_AHEAD_CHUNKS: usize = 8; static REQWEST_CRYPTO_PROVIDER: OnceLock<()> = OnceLock::new(); #[derive(Clone)] @@ -369,14 +377,194 @@ pub fn run_sandbox_init_with_encoding( validate_encoding_preference(offer, content_encoding)?; let limits = limits_for_offer(offer)?; let (encoding, response) = client.open_export(&capability, offer, content_encoding)?; - let archive = ReplicaArchive::new(encoding, response); - let summary = + let (body, mut producer) = + spawn_export_read_ahead(response).map_err(|error| SandboxInitError::Http { + operation: "session export read-ahead setup", + detail: error.to_string(), + })?; + let archive = ReplicaArchive::new(encoding, body); + let materialization = materialize_replica_archive_with_expected_receipt(archive, &root, limits, expected_receipt) - .map_err(|error| SandboxInitError::Materialization(error.to_string()))?; + .map_err(|error| SandboxInitError::Materialization(error.to_string())); + let producer_outcome = producer.join(); + + let summary = match materialization { + Err(error) => return Err(error), + Ok(summary) => { + match producer_outcome { + Ok(ReadAheadProducerOutcome::CleanEof) => {} + Ok( + ReadAheadProducerOutcome::ConsumerClosed + | ReadAheadProducerOutcome::ErrorDelivered, + ) => { + return Err(SandboxInitError::Materialization( + "sandbox export transport ended without a clean EOF".to_string(), + )); + } + Err(()) => { + return Err(SandboxInitError::Materialization( + "sandbox export read-ahead worker panicked".to_string(), + )); + } + } + summary + } + }; Ok(report(&root, &capability, encoding, summary)) } +enum ReadAheadMessage { + Data(Vec), + Error(io::Error), + CleanEof, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum ReadAheadProducerOutcome { + CleanEof, + ConsumerClosed, + ErrorDelivered, +} + +struct ExportReadAhead { + receiver: Receiver, + recycle: SyncSender>, + current: Option>, + offset: usize, + clean_eof: bool, +} + +impl Read for ExportReadAhead { + fn read(&mut self, output: &mut [u8]) -> io::Result { + if output.is_empty() { + return Ok(0); + } + if self.clean_eof { + return Ok(0); + } + + loop { + if self + .current + .as_ref() + .is_some_and(|current| self.offset < current.len()) + { + let available = &self.current.as_ref().expect("current chunk")[self.offset..]; + let copied = available.len().min(output.len()); + output[..copied].copy_from_slice(&available[..copied]); + self.offset += copied; + return Ok(copied); + } + if let Some(mut exhausted) = self.current.take() { + exhausted.clear(); + let _ = self.recycle.send(exhausted); + } + + match self.receiver.recv() { + Ok(ReadAheadMessage::Data(chunk)) => { + self.current = Some(chunk); + self.offset = 0; + } + Ok(ReadAheadMessage::Error(error)) => return Err(error), + Ok(ReadAheadMessage::CleanEof) => { + self.clean_eof = true; + return Ok(0); + } + Err(_) => { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "sandbox export read-ahead producer stopped before EOF", + )); + } + } + } + } +} + +struct ReadAheadProducer { + handle: Option>, +} + +impl ReadAheadProducer { + fn join(&mut self) -> Result { + let handle = self.handle.take().ok_or(())?; + handle.join().map_err(|_| ()) + } +} + +impl Drop for ReadAheadProducer { + fn drop(&mut self) { + if let Some(handle) = self.handle.take() { + let _ = handle.join(); + } + } +} + +fn spawn_export_read_ahead(body: Body) -> io::Result<(ExportReadAhead, ReadAheadProducer)> +where + Body: Read + Send + 'static, +{ + let (sender, receiver) = sync_channel(EXPORT_READ_AHEAD_CHUNKS); + let (recycle, buffers) = sync_channel(EXPORT_READ_AHEAD_CHUNKS); + for _ in 0..EXPORT_READ_AHEAD_CHUNKS { + recycle + .send(Vec::with_capacity(EXPORT_READ_AHEAD_CHUNK_BYTES)) + .expect("new buffer pool accepts its fixed capacity"); + } + let handle = thread::Builder::new() + .name("locality-export-read-ahead".to_string()) + .spawn(move || produce_export(body, &sender, &buffers))?; + Ok(( + ExportReadAhead { + receiver, + recycle, + current: None, + offset: 0, + clean_eof: false, + }, + ReadAheadProducer { + handle: Some(handle), + }, + )) +} + +fn produce_export( + mut body: Body, + sender: &SyncSender, + buffers: &Receiver>, +) -> ReadAheadProducerOutcome { + loop { + let Ok(mut chunk) = buffers.recv() else { + return ReadAheadProducerOutcome::ConsumerClosed; + }; + chunk.resize(EXPORT_READ_AHEAD_CHUNK_BYTES, 0); + match body.read(&mut chunk) { + Ok(0) => { + return if sender.send(ReadAheadMessage::CleanEof).is_ok() { + ReadAheadProducerOutcome::CleanEof + } else { + ReadAheadProducerOutcome::ConsumerClosed + }; + } + Ok(read) => { + chunk.truncate(read); + if sender.send(ReadAheadMessage::Data(chunk)).is_err() { + return ReadAheadProducerOutcome::ConsumerClosed; + } + } + Err(error) => { + let redacted = io::Error::new(error.kind(), "sandbox export transport read failed"); + return if sender.send(ReadAheadMessage::Error(redacted)).is_ok() { + ReadAheadProducerOutcome::ErrorDelivered + } else { + ReadAheadProducerOutcome::ConsumerClosed + }; + } + } + } +} + fn absolute_destination(path: &Path) -> Result { if path.is_absolute() { Ok(path.to_path_buf()) @@ -595,11 +783,19 @@ fn report( struct SandboxHttpClient { client: Client, + export_client: Client, api_url: reqwest::Url, } impl SandboxHttpClient { fn new(api_url: &str) -> Result { + Self::new_with_read_timeout(api_url, HTTP_READ_TIMEOUT) + } + + fn new_with_read_timeout( + api_url: &str, + read_timeout: Duration, + ) -> Result { let api_url = reqwest::Url::parse(api_url) .map_err(|_| SandboxInitError::InvalidApiUrl("URL cannot be parsed"))?; if !matches!(api_url.scheme(), "http" | "https") { @@ -635,7 +831,20 @@ impl SandboxHttpClient { operation: "HTTP client setup", detail: error.without_url().to_string(), })?; - Ok(Self { client, api_url }) + let export_client = Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .connect_timeout(HTTP_CONNECT_TIMEOUT) + .timeout(read_timeout) + .build() + .map_err(|error| SandboxInitError::Http { + operation: "HTTP client setup", + detail: error.without_url().to_string(), + })?; + Ok(Self { + client, + export_client, + api_url, + }) } fn exchange_bootstrap( @@ -710,7 +919,7 @@ impl SandboxHttpClient { preference: SandboxContentEncodingPreference, ) -> Result<(ReplicaArchiveEncoding, Response), SandboxInitError> { let response = self - .client + .export_client .get(self.export_url(capability.session_id.as_str())) .header(ACCEPT, TAR_MEDIA_TYPE) .header(ACCEPT_ENCODING, preference.accept_encoding()) @@ -891,6 +1100,8 @@ mod tests { use std::collections::BTreeMap; use std::io::{self, Read, Write}; use std::net::{TcpListener, TcpStream}; + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::mpsc::{self, Receiver}; use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; @@ -899,6 +1110,37 @@ mod tests { use super::*; + struct FixedChunkBody { + reads: Arc, + } + + impl Read for FixedChunkBody { + fn read(&mut self, output: &mut [u8]) -> io::Result { + self.reads.fetch_add(1, Ordering::SeqCst); + output.fill(0x5a); + Ok(output.len()) + } + } + + struct FailingBody { + first_read: bool, + } + + impl Read for FailingBody { + fn read(&mut self, output: &mut [u8]) -> io::Result { + if self.first_read { + self.first_read = false; + output[..3].copy_from_slice(b"abc"); + Ok(3) + } else { + Err(io::Error::new( + io::ErrorKind::ConnectionReset, + "sentinel export transport failure", + )) + } + } + } + #[derive(Debug)] struct CapturedRequest { method: String, @@ -909,7 +1151,18 @@ mod tests { enum TestResponse { DropConnection, - Json { status: &'static str, body: Vec }, + Json { + status: &'static str, + body: Vec, + }, + StalledExport { + prefix: Vec, + stall: Duration, + }, + ProgressingExport { + chunks: Vec>, + pause: Duration, + }, } struct TestServer { @@ -937,6 +1190,36 @@ mod tests { TestResponse::Json { status, body } => { write_json_response(&mut stream, status, &body); } + TestResponse::StalledExport { prefix, stall } => { + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nContent-Type: application/x-tar\r\nContent-Encoding: identity\r\nConnection: close\r\n\r\n", + prefix.len() + 512 + ) + .expect("write stalled response head"); + stream + .write_all(&prefix) + .expect("write stalled response prefix"); + stream.flush().expect("flush stalled response prefix"); + thread::sleep(stall); + } + TestResponse::ProgressingExport { chunks, pause } => { + let content_length = chunks.iter().map(Vec::len).sum::(); + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {content_length}\r\nContent-Type: application/x-tar\r\nContent-Encoding: identity\r\nConnection: close\r\n\r\n" + ) + .expect("write progressing response head"); + for (index, chunk) in chunks.into_iter().enumerate() { + if index != 0 { + thread::sleep(pause); + } + stream + .write_all(&chunk) + .expect("write progressing response chunk"); + stream.flush().expect("flush progressing response chunk"); + } + } } } if reject_extra_request { @@ -971,6 +1254,165 @@ mod tests { } } + #[test] + fn export_read_ahead_is_byte_bounded_and_consumer_drop_unblocks_producer() { + let reads = Arc::new(AtomicUsize::new(0)); + let (reader, mut producer) = spawn_export_read_ahead(FixedChunkBody { + reads: Arc::clone(&reads), + }) + .expect("start read-ahead producer"); + let expected_reads = EXPORT_READ_AHEAD_CHUNKS; + let deadline = Instant::now() + Duration::from_secs(2); + while reads.load(Ordering::SeqCst) < expected_reads { + assert!( + Instant::now() < deadline, + "producer did not fill bounded queue" + ); + thread::yield_now(); + } + thread::sleep(Duration::from_millis(25)); + assert_eq!( + reads.load(Ordering::SeqCst), + expected_reads, + "the producer is bounded by exactly eight reusable 64 KiB buffers" + ); + + drop(reader); + assert_eq!( + producer.join(), + Ok(ReadAheadProducerOutcome::ConsumerClosed), + "dropping a rejecting consumer must promptly release a blocked producer" + ); + } + + #[test] + fn export_read_ahead_redacts_the_original_io_error() { + let (mut reader, mut producer) = spawn_export_read_ahead(FailingBody { first_read: true }) + .expect("start read-ahead producer"); + let mut prefix = [0_u8; 3]; + reader.read_exact(&mut prefix).expect("read prefix"); + assert_eq!(&prefix, b"abc"); + + let error = reader.read(&mut [0_u8; 1]).expect_err("transport fails"); + assert_eq!(error.kind(), io::ErrorKind::ConnectionReset); + assert_eq!(error.to_string(), "sandbox export transport read failed"); + assert!(!error.to_string().contains("sentinel")); + drop(reader); + assert_eq!( + producer.join(), + Ok(ReadAheadProducerOutcome::ErrorDelivered) + ); + } + + #[test] + fn export_read_ahead_disconnect_is_not_mistaken_for_clean_eof() { + let (sender, receiver) = sync_channel(1); + let (recycle, _) = sync_channel(1); + drop(sender); + let mut reader = ExportReadAhead { + receiver, + recycle, + current: None, + offset: 0, + clean_eof: false, + }; + + let error = reader + .read(&mut [0_u8; 1]) + .expect_err("disconnect without an EOF marker must fail"); + assert_eq!(error.kind(), io::ErrorKind::UnexpectedEof); + assert_eq!( + error.to_string(), + "sandbox export read-ahead producer stopped before EOF" + ); + } + + #[test] + fn early_materializer_rejection_joins_a_producer_blocked_on_http_read() { + static DIRECTORY_SEQUENCE: AtomicUsize = AtomicUsize::new(0); + + let server = TestServer::start( + vec![TestResponse::StalledExport { + prefix: vec![0xff; 512], + stall: Duration::from_millis(500), + }], + false, + ); + let client = + SandboxHttpClient::new_with_read_timeout(&server.api_url, Duration::from_millis(100)) + .expect("HTTP client"); + let response = client + .export_client + .get(endpoint_url(&client.api_url, &["stalled-export"])) + .send() + .expect("open stalled response"); + let (body, mut producer) = spawn_export_read_ahead(response).expect("start producer"); + let parent = std::env::temp_dir().join(format!( + "locality-stalled-export-{}-{}", + std::process::id(), + DIRECTORY_SEQUENCE.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir(&parent).expect("create test parent"); + let destination = parent.join("tree"); + + let archive = ReplicaArchive::new(ReplicaArchiveEncoding::Identity, body); + localityd::replica_materializer::materialize_replica_archive( + archive, + &destination, + ReplicaMaterializationLimits::default(), + ) + .expect_err("invalid first header rejects before HTTP EOF"); + let join_started = Instant::now(); + assert_eq!( + producer.join(), + Ok(ReadAheadProducerOutcome::ConsumerClosed) + ); + assert!( + join_started.elapsed() >= Duration::from_millis(20), + "producer was not blocked in the stalled response read" + ); + assert!( + join_started.elapsed() < Duration::from_secs(1), + "blocked response read exceeded its configured deadline" + ); + assert!(!destination.exists()); + fs::remove_dir_all(&parent).expect("remove test parent"); + server.finish(); + } + + #[test] + fn export_read_deadline_resets_for_a_progressing_multi_read_response() { + let chunks = vec![vec![1; 17], vec![2; 19], vec![3; 23]]; + let expected = chunks.iter().flatten().copied().collect::>(); + let server = TestServer::start( + vec![TestResponse::ProgressingExport { + chunks, + pause: Duration::from_millis(120), + }], + false, + ); + let client = + SandboxHttpClient::new_with_read_timeout(&server.api_url, Duration::from_millis(200)) + .expect("HTTP client"); + let response = client + .export_client + .get(endpoint_url(&client.api_url, &["progressing-export"])) + .send() + .expect("open progressing response"); + let started = Instant::now(); + let (mut body, mut producer) = spawn_export_read_ahead(response).expect("start producer"); + let mut actual = Vec::new(); + body.read_to_end(&mut actual) + .expect("read progressing body"); + assert_eq!(producer.join(), Ok(ReadAheadProducerOutcome::CleanEof)); + assert_eq!(actual, expected); + assert!( + started.elapsed() > Duration::from_millis(200), + "fixture must exceed one read deadline in total" + ); + server.finish(); + } + #[test] fn dropped_bootstrap_response_retries_with_identical_key_and_body() { let response = serde_json::to_vec(&capability()).expect("serialize capability"); diff --git a/crates/loc-cli/tests/sandbox.rs b/crates/loc-cli/tests/sandbox.rs index 0ee39eef..2c82e2d2 100644 --- a/crates/loc-cli/tests/sandbox.rs +++ b/crates/loc-cli/tests/sandbox.rs @@ -62,6 +62,9 @@ struct ResponseFixture { status: &'static str, headers: Vec<(&'static str, &'static str)>, body: Vec, + declared_content_length: Option, + split_after: Option<(usize, Duration)>, + staging_gate: Option<(PathBuf, PathBuf, PathBuf)>, } impl ResponseFixture { @@ -70,6 +73,9 @@ impl ResponseFixture { status: "200 OK", headers: vec![("Content-Type", "application/json")], body: serde_json::to_vec(value).expect("serialize response"), + declared_content_length: None, + split_after: None, + staging_gate: None, } } @@ -81,8 +87,40 @@ impl ResponseFixture { ("Content-Encoding", encoding), ], body, + declared_content_length: None, + split_after: None, + staging_gate: None, } } + + fn streaming_export( + encoding: &'static str, + body: Vec, + split_after: usize, + pause: Duration, + ) -> Self { + Self::export(encoding, body).with_split_after(split_after, pause) + } + + fn with_declared_content_length(mut self, length: usize) -> Self { + self.declared_content_length = Some(length); + self + } + + fn with_split_after(mut self, bytes: usize, pause: Duration) -> Self { + self.split_after = Some((bytes, pause)); + self + } + + fn with_staging_gate( + mut self, + parent: PathBuf, + logical_path: PathBuf, + destination: PathBuf, + ) -> Self { + self.staging_gate = Some((parent, logical_path, destination)); + self + } } struct MockServer { @@ -281,6 +319,89 @@ fn zstd_bootstrap_streams_into_the_shared_materializer() { ); } +#[test] +fn export_bytes_are_staged_while_the_http_response_is_still_streaming() { + let directory = TestDirectory::new("streaming-overlap"); + let body = vec![0x5a; 256 * 1024]; + let tar = tar_file(b"large.bin", &body); + let capability = capability(); + let status = ready_status( + capability.session_id.clone(), + COMPONENT_VERSIONS, + &tar, + BTreeSet::from([TarContentEncoding::Identity]), + ); + let destination = directory.root(); + let response = ResponseFixture::streaming_export( + "identity", + tar, + 512 + 64 * 1024, + Duration::from_millis(10), + ) + .with_staging_gate( + destination + .parent() + .expect("destination parent") + .to_path_buf(), + PathBuf::from("large.bin"), + destination.clone(), + ); + let server = MockServer::start(vec![ + ResponseFixture::json(&capability), + ResponseFixture::json(&status), + response, + ]); + + let report = run_sandbox_init( + SandboxInitOptions { + api_url: server.api_url.clone(), + root: destination.clone(), + }, + SandboxBootstrapToken::new("bootstrap-secret").expect("token"), + ) + .expect("stream and publish export"); + + assert_eq!(report.files, 1); + assert_eq!(report.materialized_bytes, body.len() as u64); + assert_eq!( + fs::read(destination.join("large.bin")).expect("read published file"), + body + ); +} + +#[test] +fn truncated_http_body_producer_error_prevents_publication() { + let directory = TestDirectory::new("transport-truncation"); + let tar = tar_file(b"complete-before-http-eof.txt", b"complete\n"); + let capability = capability(); + let status = ready_status( + capability.session_id.clone(), + COMPONENT_VERSIONS, + &tar, + BTreeSet::from([TarContentEncoding::Identity]), + ); + let declared_length = tar.len() + 128; + let server = MockServer::start(vec![ + ResponseFixture::json(&capability), + ResponseFixture::json(&status), + ResponseFixture::export("identity", tar).with_declared_content_length(declared_length), + ]); + + let error = run_sandbox_init( + SandboxInitOptions { + api_url: server.api_url.clone(), + root: directory.root(), + }, + SandboxBootstrapToken::new("bootstrap-secret").expect("token"), + ) + .expect_err("an incomplete HTTP response must not publish a complete-looking tar"); + + assert_eq!(error.code(), "materialization_failed"); + assert!(!directory.root().exists()); + assert!(!error.to_string().contains("bootstrap-secret")); + assert!(!error.to_string().contains("capability-secret")); +} + #[test] fn forced_content_encodings_send_exact_headers_and_match_reports() { let tar = tar_file(b"forced.txt", b"forced\n"); @@ -602,6 +723,9 @@ fn bearer_authenticated_redirect_is_not_followed() { status: "302 Found", headers: vec![("Location", "/redirected")], body: Vec::new(), + declared_content_length: None, + split_after: None, + staging_gate: None, }, ]); @@ -773,6 +897,9 @@ fn version_session_offer_media_and_response_encoding_are_validated() { status: "200 OK", headers: vec![("Content-Type", "application/octet-stream")], body: tar.clone(), + declared_content_length: None, + split_after: None, + staging_gate: None, }), "backend_protocol_invalid", ), @@ -1081,20 +1208,62 @@ fn read_request(stream: &mut TcpStream) -> CapturedRequest { } fn write_response(stream: &mut TcpStream, response: ResponseFixture) { + let content_length = response + .declared_content_length + .unwrap_or(response.body.len()); write!( stream, "HTTP/1.1 {}\r\nContent-Length: {}\r\nConnection: close\r\n", - response.status, - response.body.len() + response.status, content_length ) .expect("write response head"); for (name, value) in response.headers { write!(stream, "{name}: {value}\r\n").expect("write response header"); } write!(stream, "\r\n").expect("finish response headers"); - stream - .write_all(&response.body) - .expect("write response body"); + if let Some((split_after, pause)) = response.split_after { + let split_after = split_after.min(response.body.len()); + stream + .write_all(&response.body[..split_after]) + .expect("write first response body chunk"); + stream.flush().expect("flush first response body chunk"); + if let Some((parent, logical_path, destination)) = &response.staging_gate { + let deadline = std::time::Instant::now() + Duration::from_secs(2); + loop { + let staged = fs::read_dir(parent) + .into_iter() + .flatten() + .flatten() + .any(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with(".locality-stage-") + && entry.path().join(logical_path).is_file() + }); + if staged { + assert!( + !destination.exists(), + "the destination must remain absent while the response is incomplete" + ); + break; + } + assert!( + std::time::Instant::now() < deadline, + "materializer did not stage the first file while the response was paused" + ); + thread::sleep(Duration::from_millis(2)); + } + } + thread::sleep(pause); + stream + .write_all(&response.body[split_after..]) + .expect("write remaining response body"); + } else { + stream + .write_all(&response.body) + .expect("write response body"); + } stream.flush().expect("flush response"); } From 17856f8631751448ed6692ffb07e8ef73529efaa Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 11:03:37 -0700 Subject: [PATCH 12/13] Profile sandbox client hydration --- crates/loc-cli/src/commands.rs | 82 +++++++++++++---- crates/loc-cli/src/sandbox.rs | 107 +++++++++++++++++++++- crates/loc-cli/tests/sandbox.rs | 157 ++++++++++++++++++++++++++++++++ 3 files changed, 328 insertions(+), 18 deletions(-) diff --git a/crates/loc-cli/src/commands.rs b/crates/loc-cli/src/commands.rs index 1a78b23e..ff88403e 100644 --- a/crates/loc-cli/src/commands.rs +++ b/crates/loc-cli/src/commands.rs @@ -110,8 +110,9 @@ use crate::push::{ }; use crate::restore::{RestoreError, RestoreOptions, RestoreReport, run_restore}; use crate::sandbox::{ - SandboxContentEncodingPreference, SandboxInitOptions, SandboxInitReport, - resolve_bootstrap_token, run_sandbox_init_with_encoding, + PROFILE_BOOTSTRAP_TOKEN_INPUT, PROFILE_TOTAL, SandboxContentEncodingPreference, + SandboxInitOptions, SandboxInitProfile, SandboxInitReport, resolve_bootstrap_token, + run_sandbox_init_with_encoding, run_sandbox_init_with_encoding_and_profile, }; use crate::search::{ SearchError, SearchOptions, SearchReport, SearchResult, is_notion_url_host, notion_id_from_url, @@ -276,6 +277,8 @@ struct SandboxInitArgs { encoding: Option, #[arg(long, help = "Read the one-time bootstrap token from standard input")] bootstrap_token_stdin: bool, + #[arg(long, help = "Print redaction-safe phase timings to standard error")] + profile: bool, } #[derive(Clone, Copy, Debug, PartialEq, Eq, ValueEnum)] @@ -1513,6 +1516,7 @@ fn legacy_args_for_command(command: &LocalityCommand) -> Vec { "--bootstrap-token-stdin", options.bootstrap_token_stdin, ); + push_flag(&mut args, "--profile", options.profile); } } } @@ -1672,25 +1676,37 @@ fn sandbox_init(options: SandboxInitArgs, json: bool) -> i32 { let content_encoding = options .encoding .map_or(SandboxContentEncodingPreference::Automatic, Into::into); - let token = { + let mut profile = options.profile.then(SandboxInitProfile::start); + let token_result = { let mut stdin = io::stdin().lock(); - match resolve_bootstrap_token( + resolve_bootstrap_token( options.bootstrap_token_stdin, std::env::var_os("LOCALITY_BOOTSTRAP_TOKEN"), &mut stdin, - ) { - Ok(token) => token, - Err(error) => return sandbox_init_command_error(json, error), + ) + }; + if let Some(profile) = profile.as_mut() { + profile.mark(PROFILE_BOOTSTRAP_TOKEN_INPUT); + } + let token = match token_result { + Ok(token) => token, + Err(error) => { + finish_sandbox_profile(profile.as_mut()); + return sandbox_init_command_error(json, error); } }; - match run_sandbox_init_with_encoding( - SandboxInitOptions { - api_url: options.api_url, - root: PathBuf::from(options.root), - }, - token, - content_encoding, - ) { + let init_options = SandboxInitOptions { + api_url: options.api_url, + root: PathBuf::from(options.root), + }; + let outcome = if let Some(profile) = profile.as_mut() { + run_sandbox_init_with_encoding_and_profile(init_options, token, content_encoding, profile) + } else { + run_sandbox_init_with_encoding(init_options, token, content_encoding) + }; + finish_sandbox_profile(profile.as_mut()); + + match outcome { Ok(report) => { if json { print_json(&report); @@ -1703,6 +1719,21 @@ fn sandbox_init(options: SandboxInitArgs, json: bool) -> i32 { } } +fn finish_sandbox_profile(profile: Option<&mut SandboxInitProfile>) { + let Some(profile) = profile else { + return; + }; + profile.mark(PROFILE_TOTAL); + let mut stderr = io::stderr().lock(); + for timing in profile.timings() { + let _ = writeln!( + stderr, + "locality sandbox profile phase={} phase_ms={} total_ms={}", + timing.phase, timing.phase_ms, timing.total_ms + ); + } +} + fn sandbox_init_command_error(json: bool, error: crate::sandbox::SandboxInitError) -> i32 { let exit_code = if error.is_usage_error() { EXIT_USAGE @@ -9903,6 +9934,7 @@ mod tests { "--root ", "--encoding ", "--bootstrap-token-stdin", + "--profile", "--json", ], ), @@ -10546,7 +10578,7 @@ mod tests { #[test] fn sandbox_scope_and_bootstrap_token_are_not_accepted_from_argv() { for args in [ - vec!["sandbox", "init", "--profile", "pilot"], + vec!["sandbox", "init", "--tenant", "pilot"], vec!["sandbox", "init", "--bootstrap-token", "argv-secret"], vec!["sandbox", "init", "--bootstrap-token=argv-secret"], ] { @@ -10603,6 +10635,24 @@ mod tests { panic!("sandbox init command expected"); }; assert_eq!(options.encoding, None); + assert!(!options.profile); + + let profiled = parse_cli([ + "sandbox", + "init", + "--api-url", + "https://api.locality.test", + "--root", + "/mnt/locality", + "--profile", + ]); + let Some(LocalityCommand::Sandbox { + command: SandboxCommand::Init(options), + }) = profiled.command + else { + panic!("sandbox init command expected"); + }; + assert!(options.profile); let error = Cli::try_parse_from(argv([ "sandbox", diff --git a/crates/loc-cli/src/sandbox.rs b/crates/loc-cli/src/sandbox.rs index e92326c9..71160dd1 100644 --- a/crates/loc-cli/src/sandbox.rs +++ b/crates/loc-cli/src/sandbox.rs @@ -13,7 +13,7 @@ use std::path::{Path, PathBuf}; use std::sync::OnceLock; use std::sync::mpsc::{Receiver, SyncSender, sync_channel}; use std::thread::{self, JoinHandle}; -use std::time::Duration; +use std::time::{Duration, Instant}; use locality_protocol::{ OpaqueBootstrapExchangeRequest, SandboxSessionState, SandboxSessionStatus, SessionCapability, @@ -47,6 +47,51 @@ const EXPORT_READ_AHEAD_CHUNK_BYTES: usize = 64 * 1024; const EXPORT_READ_AHEAD_CHUNKS: usize = 8; static REQWEST_CRYPTO_PROVIDER: OnceLock<()> = OnceLock::new(); +pub(crate) const PROFILE_BOOTSTRAP_TOKEN_INPUT: &str = "bootstrap_token_input"; +const PROFILE_BOOTSTRAP_EXCHANGE: &str = "bootstrap_exchange"; +const PROFILE_SESSION_STATUS: &str = "session_status"; +const PROFILE_EXPORT_OPEN_HEADERS: &str = "export_open_headers"; +const PROFILE_FIRST_BODY_BYTE: &str = "first_body_byte"; +const PROFILE_STREAM_DECODE_MATERIALIZE: &str = "stream_decode_materialize"; +pub(crate) const PROFILE_TOTAL: &str = "total"; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct SandboxProfileTiming { + pub phase: &'static str, + pub phase_ms: u128, + pub total_ms: u128, +} + +pub(crate) struct SandboxInitProfile { + started: Instant, + last_total_ms: u128, + timings: Vec, +} + +impl SandboxInitProfile { + pub(crate) fn start() -> Self { + Self { + started: Instant::now(), + last_total_ms: 0, + timings: Vec::new(), + } + } + + pub(crate) fn mark(&mut self, phase: &'static str) { + let total_ms = self.started.elapsed().as_millis(); + self.timings.push(SandboxProfileTiming { + phase, + phase_ms: total_ms.saturating_sub(self.last_total_ms), + total_ms, + }); + self.last_total_ms = total_ms; + } + + pub(crate) fn timings(&self) -> &[SandboxProfileTiming] { + &self.timings + } +} + #[derive(Clone)] pub struct SandboxBootstrapToken(String); @@ -365,28 +410,51 @@ pub fn run_sandbox_init_with_encoding( options: SandboxInitOptions, bootstrap_token: SandboxBootstrapToken, content_encoding: SandboxContentEncodingPreference, +) -> Result { + run_sandbox_init_internal(options, bootstrap_token, content_encoding, None) +} + +pub(crate) fn run_sandbox_init_with_encoding_and_profile( + options: SandboxInitOptions, + bootstrap_token: SandboxBootstrapToken, + content_encoding: SandboxContentEncodingPreference, + profile: &mut SandboxInitProfile, +) -> Result { + run_sandbox_init_internal(options, bootstrap_token, content_encoding, Some(profile)) +} + +fn run_sandbox_init_internal( + options: SandboxInitOptions, + bootstrap_token: SandboxBootstrapToken, + content_encoding: SandboxContentEncodingPreference, + mut profile: Option<&mut SandboxInitProfile>, ) -> Result { let root = absolute_destination(&options.root)?; validate_destination(&root)?; let client = SandboxHttpClient::new(&options.api_url)?; let capability = client.exchange_bootstrap(&bootstrap_token)?; + mark_profile(&mut profile, PROFILE_BOOTSTRAP_EXCHANGE); validate_capability(&capability)?; let status = client.session_status(&capability)?; + mark_profile(&mut profile, PROFILE_SESSION_STATUS); let (offer, expected_receipt) = validate_status(&capability, &status)?; validate_encoding_preference(offer, content_encoding)?; let limits = limits_for_offer(offer)?; let (encoding, response) = client.open_export(&capability, offer, content_encoding)?; + mark_profile(&mut profile, PROFILE_EXPORT_OPEN_HEADERS); let (body, mut producer) = spawn_export_read_ahead(response).map_err(|error| SandboxInitError::Http { operation: "session export read-ahead setup", detail: error.to_string(), })?; - let archive = ReplicaArchive::new(encoding, body); + let profiled_body = ProfiledExportBody::new(body, profile.as_deref_mut()); + let archive = ReplicaArchive::new(encoding, profiled_body); let materialization = materialize_replica_archive_with_expected_receipt(archive, &root, limits, expected_receipt) .map_err(|error| SandboxInitError::Materialization(error.to_string())); let producer_outcome = producer.join(); + mark_profile(&mut profile, PROFILE_STREAM_DECODE_MATERIALIZE); let summary = match materialization { Err(error) => return Err(error), @@ -414,6 +482,41 @@ pub fn run_sandbox_init_with_encoding( Ok(report(&root, &capability, encoding, summary)) } +fn mark_profile(profile: &mut Option<&mut SandboxInitProfile>, phase: &'static str) { + if let Some(profile) = profile.as_deref_mut() { + profile.mark(phase); + } +} + +struct ProfiledExportBody<'a, Body> { + body: Body, + profile: Option<&'a mut SandboxInitProfile>, + observed_first_byte: bool, +} + +impl<'a, Body> ProfiledExportBody<'a, Body> { + fn new(body: Body, profile: Option<&'a mut SandboxInitProfile>) -> Self { + Self { + body, + profile, + observed_first_byte: false, + } + } +} + +impl Read for ProfiledExportBody<'_, Body> { + fn read(&mut self, output: &mut [u8]) -> io::Result { + let read = self.body.read(output)?; + if read != 0 && !self.observed_first_byte { + self.observed_first_byte = true; + if let Some(profile) = self.profile.as_deref_mut() { + profile.mark(PROFILE_FIRST_BODY_BYTE); + } + } + Ok(read) + } +} + enum ReadAheadMessage { Data(Vec), Error(io::Error), diff --git a/crates/loc-cli/tests/sandbox.rs b/crates/loc-cli/tests/sandbox.rs index 2c82e2d2..17f33a0a 100644 --- a/crates/loc-cli/tests/sandbox.rs +++ b/crates/loc-cli/tests/sandbox.rs @@ -1020,6 +1020,7 @@ fn cli_forced_identity_reports_encoding_without_leaking_environment_token() { assert!(!stdout.contains("capability-secret")); assert!(!stderr.contains("cli-bootstrap-secret")); assert!(!stderr.contains("capability-secret")); + assert!(stderr.is_empty(), "profiling is opt-in: {stderr}"); let report: serde_json::Value = serde_json::from_str(&stdout).expect("JSON report"); assert_eq!(report["command"], "sandbox_init"); assert_eq!(report["content_encoding"], "identity"); @@ -1038,6 +1039,162 @@ fn cli_forced_identity_reports_encoding_without_leaking_environment_token() { assert_eq!(export.headers.get("accept-encoding").unwrap(), "identity"); } +#[test] +fn cli_profile_has_stable_monotonic_phases_and_no_request_details() { + let directory = TestDirectory::new("cli-profile"); + let tar = tar_file(b"profiled.txt", b"profile-content-secret\n"); + let capability = capability(); + let status = ready_status( + capability.session_id.clone(), + COMPONENT_VERSIONS, + &tar, + BTreeSet::from([TarContentEncoding::Identity]), + ); + let server = MockServer::start(vec![ + ResponseFixture::json(&capability), + ResponseFixture::json(&status), + ResponseFixture::export("identity", tar), + ]); + let root = directory.root().to_string_lossy().into_owned(); + + let output = Command::new(env!("CARGO_BIN_EXE_loc")) + .args([ + "sandbox", + "init", + "--api-url", + &server.api_url, + "--root", + &root, + "--encoding", + "identity", + "--profile", + "--json", + ]) + .env("LOCALITY_BOOTSTRAP_TOKEN", "profile-bootstrap-secret") + .output() + .expect("run profiled loc sandbox init"); + + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8(output.stderr).expect("stderr UTF-8"); + let expected_phases = [ + "bootstrap_token_input", + "bootstrap_exchange", + "session_status", + "export_open_headers", + "first_body_byte", + "stream_decode_materialize", + "total", + ]; + let lines = stderr.lines().collect::>(); + assert_eq!(lines.len(), expected_phases.len(), "{stderr}"); + let mut previous_total_ms = 0_u128; + for (line, expected_phase) in lines.iter().zip(expected_phases) { + let prefix = format!("locality sandbox profile phase={expected_phase} phase_ms="); + let timing = line + .strip_prefix(&prefix) + .unwrap_or_else(|| panic!("unexpected profile line: {line}")); + let (phase_ms, total_ms) = timing + .split_once(" total_ms=") + .unwrap_or_else(|| panic!("profile line lacks exact timing fields: {line}")); + assert!( + !phase_ms.is_empty() + && phase_ms.bytes().all(|byte| byte.is_ascii_digit()) + && !total_ms.is_empty() + && total_ms.bytes().all(|byte| byte.is_ascii_digit()), + "profile timing fields must contain only decimal milliseconds: {line}" + ); + let phase_ms = phase_ms.parse::().expect("phase milliseconds"); + let total_ms = total_ms.parse::().expect("total milliseconds"); + assert!( + total_ms >= previous_total_ms, + "profile timings must be monotonic: {stderr}" + ); + assert_eq!( + phase_ms, + total_ms - previous_total_ms, + "phase timing must be the delta since the prior mark: {stderr}" + ); + previous_total_ms = total_ms; + } + + for secret_or_detail in [ + "profile-bootstrap-secret", + "capability-secret", + "session-7", + server.api_url.as_str(), + root.as_str(), + "application/x-tar", + "profile-content-secret", + "authorization", + ] { + assert!( + !stderr.contains(secret_or_detail), + "profile leaked forbidden detail `{secret_or_detail}`: {stderr}" + ); + } +} + +#[test] +fn cli_profile_failure_prints_completed_phases_and_total() { + let directory = TestDirectory::new("cli-profile-failure"); + let tar = tar_file(b"never-exported.txt", b"never-exported-content\n"); + let capability = capability(); + let status = ready_status( + capability.session_id.clone(), + COMPONENT_VERSIONS, + &tar, + BTreeSet::from([TarContentEncoding::Identity]), + ); + let server = MockServer::start(vec![ + ResponseFixture::json(&capability), + ResponseFixture::json(&status), + ]); + let root = directory.root().to_string_lossy().into_owned(); + + let output = Command::new(env!("CARGO_BIN_EXE_loc")) + .args([ + "sandbox", + "init", + "--api-url", + &server.api_url, + "--root", + &root, + "--encoding", + "zstd", + "--profile", + ]) + .env("LOCALITY_BOOTSTRAP_TOKEN", "failed-profile-secret") + .output() + .expect("run failing profiled loc sandbox init"); + + assert!(!output.status.success()); + let stderr = String::from_utf8(output.stderr).expect("stderr UTF-8"); + let phases = stderr + .lines() + .filter_map(|line| line.strip_prefix("locality sandbox profile phase=")) + .map(|timing| timing.split_once(' ').expect("phase timing fields").0) + .collect::>(); + assert_eq!( + phases, + [ + "bootstrap_token_input", + "bootstrap_exchange", + "session_status", + "total" + ], + "{stderr}" + ); + assert!(!stderr.contains("failed-profile-secret")); + assert!(!stderr.contains("capability-secret")); + assert!(!stderr.contains("session-7")); + assert!(!stderr.contains(&server.api_url)); + assert!(!stderr.contains(&root)); +} + trait StatusFixtureExt { fn with_selected_entries(self, selected_entries: u64) -> Self; fn with_decoded_bytes(self, decoded_bytes: u64) -> Self; From 7e43e76391aa9997c97194e8941a01897c7b7857 Mon Sep 17 00:00:00 2001 From: misrasaurabh1 Date: Thu, 23 Jul 2026 11:24:23 -0700 Subject: [PATCH 13/13] Clarify sandbox profile boundaries --- crates/loc-cli/src/sandbox.rs | 6 ++++-- crates/loc-cli/tests/sandbox.rs | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/loc-cli/src/sandbox.rs b/crates/loc-cli/src/sandbox.rs index 71160dd1..3cc87b5b 100644 --- a/crates/loc-cli/src/sandbox.rs +++ b/crates/loc-cli/src/sandbox.rs @@ -48,10 +48,11 @@ const EXPORT_READ_AHEAD_CHUNKS: usize = 8; static REQWEST_CRYPTO_PROVIDER: OnceLock<()> = OnceLock::new(); pub(crate) const PROFILE_BOOTSTRAP_TOKEN_INPUT: &str = "bootstrap_token_input"; +const PROFILE_CLIENT_SETUP: &str = "client_setup"; const PROFILE_BOOTSTRAP_EXCHANGE: &str = "bootstrap_exchange"; const PROFILE_SESSION_STATUS: &str = "session_status"; const PROFILE_EXPORT_OPEN_HEADERS: &str = "export_open_headers"; -const PROFILE_FIRST_BODY_BYTE: &str = "first_body_byte"; +const PROFILE_FIRST_CONSUMER_BODY_BYTE: &str = "first_consumer_body_byte"; const PROFILE_STREAM_DECODE_MATERIALIZE: &str = "stream_decode_materialize"; pub(crate) const PROFILE_TOTAL: &str = "total"; @@ -432,6 +433,7 @@ fn run_sandbox_init_internal( let root = absolute_destination(&options.root)?; validate_destination(&root)?; let client = SandboxHttpClient::new(&options.api_url)?; + mark_profile(&mut profile, PROFILE_CLIENT_SETUP); let capability = client.exchange_bootstrap(&bootstrap_token)?; mark_profile(&mut profile, PROFILE_BOOTSTRAP_EXCHANGE); @@ -510,7 +512,7 @@ impl Read for ProfiledExportBody<'_, Body> { if read != 0 && !self.observed_first_byte { self.observed_first_byte = true; if let Some(profile) = self.profile.as_deref_mut() { - profile.mark(PROFILE_FIRST_BODY_BYTE); + profile.mark(PROFILE_FIRST_CONSUMER_BODY_BYTE); } } Ok(read) diff --git a/crates/loc-cli/tests/sandbox.rs b/crates/loc-cli/tests/sandbox.rs index 17f33a0a..eeb6bc4e 100644 --- a/crates/loc-cli/tests/sandbox.rs +++ b/crates/loc-cli/tests/sandbox.rs @@ -1082,10 +1082,11 @@ fn cli_profile_has_stable_monotonic_phases_and_no_request_details() { let stderr = String::from_utf8(output.stderr).expect("stderr UTF-8"); let expected_phases = [ "bootstrap_token_input", + "client_setup", "bootstrap_exchange", "session_status", "export_open_headers", - "first_body_byte", + "first_consumer_body_byte", "stream_decode_materialize", "total", ]; @@ -1182,6 +1183,7 @@ fn cli_profile_failure_prints_completed_phases_and_total() { phases, [ "bootstrap_token_input", + "client_setup", "bootstrap_exchange", "session_status", "total"