Improve fix for corrupted font resource loading in older documents (#4482)

* Revert "Fix corrupted font resource loading in older documents"

This reverts commit 404d9f3047.

* Fix migrations for the Read Vector, Dot Product, Query JSON, and Sample Polyline nodes

* Fix text node migrations

* Provide resource storage to the resource message handler

* Refetch resources whose stored bytes are missing

* Add test for refetching resources with missing bytes

editor/src/dispatcher.rs

@@ -96,8 +96,9 @@ impl Dispatcher {
 		s
 	}
 
+	#[cfg(test)]
 	pub fn with_executor(executor: crate::node_graph_executor::NodeGraphExecutor) -> Self {
-		let mut s = Self::default();
+		let mut s = Self::new(Arc::new(graph_craft::application_io::resource::HashMapResourceStorage::new()), None);
 		s.message_handlers.portfolio_message_handler = PortfolioMessageHandler::with_executor(executor);
 		s
 	}

editor/src/messages/portfolio/document/resource/resource_message_handler.rs

@@ -3,7 +3,7 @@ use crate::messages::portfolio::{document::resource::utility_types::EmbeddedReso
 use crate::messages::prelude::*;
 use base64::Engine;
 use base64::engine::general_purpose::STANDARD as BASE64;
-use graph_craft::application_io::resource::{DataSource, LoadResource, Resource, ResourceHash, ResourceId, ResourceRegistry};
+use graph_craft::application_io::resource::{DataSource, LoadResource, Resource, ResourceHash, ResourceId, ResourceRegistry, ResourceStorage};
 use graphene_std::text::Font;
 use std::sync::Arc;
 use url::Url;
@@ -49,15 +49,8 @@ impl MessageHandler<ResourceMessage, ResourceMessageContext<'_>> for ResourceMes
 				responses.add(ResourceMessage::Resolve { resource_id });
 			}
 			ResourceMessage::ResolveAll => {
-				// A resource keeps its hash when storage evicts its data, so only a fetchable source can repair it
+				let storage = resource_storage.resources_mut();
-				let refetchable = self
+				let ids: Vec<ResourceId> = self.registry.ids().filter(|id| !self.registry.hash(id).is_some_and(|hash| storage.contains(&hash))).collect();
-					.registry
-					.resolved()
-					.filter(|info| info.hash.is_some_and(|hash| !resource_storage.contains(hash)))
-					.filter(|info| info.sources.iter().any(|source| matches!(source, DataSource::Url(_) | DataSource::Font { .. })))
-					.map(|info| info.id);
-				let ids: Vec<ResourceId> = self.registry.unresolved().map(|info| info.id).chain(refetchable).collect();
-
 				for id in ids {
 					if self.pending_resolves.contains(&id) {
 						continue;
@@ -74,9 +67,7 @@ impl MessageHandler<ResourceMessage, ResourceMessageContext<'_>> for ResourceMes
 					log::error!("Resolve for {resource_id}: no registry entry");
 					return;
 				};
-				// This hash names the very data that is missing, so it cannot stand in for fetching that data
+				if info.hash.is_some_and(|hash| resource_storage.resources_mut().contains(hash)) {
-				let data_missing = info.hash.is_some_and(|hash| !resource_storage.contains(hash));
-				if info.hash.is_some() && !data_missing {
 					log::warn!("Resource {resource_id} already resolved");
 					return;
 				}
@@ -89,7 +80,7 @@ impl MessageHandler<ResourceMessage, ResourceMessageContext<'_>> for ResourceMes
 					.sources
 					.iter()
 					.map(|source| match source {
-						DataSource::Font { family, style } if !data_missing => {
+						DataSource::Font { family, style } => {
 							let font = match style {
 								Some(style) => Font::new(family.clone(), style.clone()),
 								None => Font::new_with_default_style(family.clone()),
@@ -225,20 +216,16 @@ impl ResourceMessageHandler {
 			.resolved()
 			.filter(|info| info.sources.contains(&DataSource::Embedded))
 			.filter_map(|info| {
-				let (id, hash) = (info.id, *info.hash?);
+				if let Some(hash) = info.hash {
-				let resource = resources_load_handle.load(hash);
+					let resource = resources_load_handle.load(*hash);
-				Some(async move { (id, hash, resource.await) })
+					Some(async move { resource.await.map(|resource| (*hash, resource)) })
+				} else {
+					None
+				}
 			})
 			.collect::<Vec<_>>();
 
-		let loaded = futures::future::join_all(embedded).await;
+		self.embedded = EmbeddedResources::from_iter(futures::future::join_all(embedded).await.into_iter().flatten());
-
-		// Saving without these bytes writes a document whose registry claims to carry them
-		for (id, hash, _) in loaded.iter().filter(|(_, _, resource)| resource.is_none()) {
-			log::error!("Resource {id} ({hash}) is marked as embedded but its data is missing from storage, so the saved document will not contain it");
-		}
-
-		self.embedded = EmbeddedResources::from_iter(loaded.into_iter().filter_map(|(_, hash, resource)| resource.map(|resource| (hash, resource))));
 	}
 
 	pub fn collect_garbage(&mut self, used: &[ResourceId]) {
@@ -318,45 +305,28 @@ mod tests {
 	use super::*;
 	use graph_craft::application_io::resource::ResourceStorage;
 
-	/// Storage can lose a resource's data while the document keeps the hash naming it, which leaves the graph
-	/// pointing at bytes that are gone. Only sources that can be fetched again are worth re-resolving.
 	#[test]
-	fn resolve_all_refetches_resources_whose_data_is_missing() {
+	fn resolve_all_refetches_resources_whose_bytes_are_missing() {
 		let mut handler = ResourceMessageHandler::default();
 		let storage = ResourceStorageMessageHandler::default();
+		let fonts = FontsMessageHandler::default();
 
-		// Present: its bytes are in storage, so it is already usable
+		let font = |style: &str| DataSource::Font {
-		let present = ResourceId::new();
+			family: "Lato".into(),
-		let present_hash = storage.resources_mut().store(b"stored font bytes");
+			style: Some(style.into()),
-		handler.registry.resolve(&present, present_hash);
+		};
-		handler.registry.push_source_back(&present, DataSource::Embedded);
-		handler.registry.push_source_back(
-			&present,
-			DataSource::Font {
-				family: "Lato".into(),
-				style: Some("Regular (400)".into()),
-			},
-		);
 
-		// Recoverable: its bytes are gone, but the font it came from can be downloaded again
+		let cached = ResourceId::from(1);
-		let recoverable = ResourceId::new();
+		let cached_hash = storage.resources_mut().store(b"stored font bytes");
-		handler.registry.resolve(&recoverable, ResourceHash::from(b"evicted font bytes".as_slice()));
+		handler.registry.resolve(&cached, cached_hash);
-		handler.registry.push_source_back(&recoverable, DataSource::Embedded);
+		handler.registry.push_source_back(&cached, font("Regular (400)"));
-		handler.registry.push_source_back(
-			&recoverable,
-			DataSource::Font {
-				family: "Lato".into(),
-				style: Some("Black (900)".into()),
-			},
-		);
 
-		// Unrecoverable: its bytes are gone and nothing records where to fetch them from
+		let evicted = ResourceId::from(2);
-		let unrecoverable = ResourceId::new();
+		let evicted_hash = ResourceHash::from(b"evicted font bytes".as_slice());
-		handler.registry.resolve(&unrecoverable, ResourceHash::from(b"evicted image bytes".as_slice()));
+		handler.registry.resolve(&evicted, evicted_hash);
-		handler.registry.push_source_back(&unrecoverable, DataSource::Embedded);
+		handler.registry.push_source_back(&evicted, font("Black (900)"));
 
 		let mut responses = VecDeque::new();
-		let fonts = FontsMessageHandler::default();
 		handler.process_message(
 			ResourceMessage::ResolveAll,
 			&mut responses,
@@ -368,9 +338,8 @@ mod tests {
 		);
 
 		let resolve_requested = |id: ResourceId| responses.contains(&Message::from(ResourceMessage::Resolve { resource_id: id }));
-
+		assert!(resolve_requested(evicted), "a resource whose bytes are gone should be resolved again");
-		assert!(resolve_requested(recoverable), "a missing resource with a font source should be fetched again");
+		assert!(!resolve_requested(cached), "a resource whose bytes are in storage should be left alone");
-		assert!(!resolve_requested(present), "a resource whose data is in storage should be left alone");
+		assert_eq!(handler.registry.hash(&evicted), Some(evicted_hash), "the hash keeps naming the content");
-		assert!(!resolve_requested(unrecoverable), "a missing resource with no fetchable source has nowhere to fetch from");
 	}
 }

editor/src/messages/resource_storage/resource_storage_message_handler.rs

@@ -48,11 +48,6 @@ impl ResourceStorageMessageHandler {
 			inner: self.storage.clone().expect("Resource storage not initialized"),
 		}
 	}
-
-	/// Whether the resource's data is held in storage, assuming it is until storage is initialized.
-	pub fn contains(&self, hash: &ResourceHash) -> bool {
-		self.storage.as_ref().is_none_or(|storage| storage.contains(hash))
-	}
 }
 
 impl std::fmt::Debug for ResourceStorageMessageHandler {

node-graph/graph-craft/src/application_io/resource/opfs.rs

@@ -153,9 +153,6 @@ async fn drain_queue(inner: Arc<Mutex<Inner>>) {
 			Mutation::Write { hash, bytes } => {
 				if let Err(error) = write_file(&directory, &hash, &bytes).await {
 					log::error!("OPFS write for {hash} failed: {error:?}");
-
-					// Nothing reached disk, so leaving the hash listed would claim a file that later sessions cannot read
-					inner.lock().unwrap().on_disk.remove(&hash);
 				}
 			}
 			Mutation::Delete { hash } => {

node-graph/nodes/gstd/src/platform_application_io.rs

@@ -276,7 +276,6 @@ pub async fn resource<'a: 'n>(
 		return placeholder();
 	};
 
-	// Stored bytes go missing when the browser evicts its storage or a write is interrupted
 	let Some(resource) = application_io.load_resource(hash).await else {
 		log::error!("Resource {hash} was not found in storage");
 		return placeholder();