Skip to content

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

Open
timon-schelling wants to merge 6 commits into
masterfrom
improve-resource-fixes
Open

Improve fix for corrupted font resource loading in older documents#4482
timon-schelling wants to merge 6 commits into
masterfrom
improve-resource-fixes

Conversation

@timon-schelling

Copy link
Copy Markdown
Member

improvements of 404d9f3

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 5 files

Confidence score: 2/5

  • node-graph/nodes/gstd/src/platform_application_io.rs now panics when platform application IO or resource data is missing, removing the prior empty-Resource fallback for an expected missing-resource case; restore graceful handling to avoid runtime failures.
  • editor/src/messages/portfolio/document/resource/resource_message_handler.rs can treat a cached font hash as resolved even when its bytes are absent from storage, allowing downstream operations to proceed without usable font data; verify the hash still has retrievable bytes before accepting it.
  • node-graph/graph-craft/src/application_io/resource/opfs.rs retains the on_disk marker after a failed background OPFS write, so later stores skip retrying and the resource may disappear after reload; clear the marker or retry the write.
  • editor/src/messages/portfolio/document/resource/resource_message_handler.rs can silently omit missing embedded bytes during flatten() and repeatedly attempt unrecoverable Embedded-only resources in ResolveAll, risking incomplete saved documents and unresolved state; preserve missing-resource diagnostics and avoid re-resolving resources with no fetchable source.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="editor/src/messages/portfolio/document/resource/resource_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/document/resource/resource_message_handler.rs:53">
P3: `ResolveAll` now re-resolves every resolved resource whose bytes are missing, including Embedded-only resources that have no fetchable source. For those, `Resolve`'s `DataSource::Embedded => continue` can never recover the data, so each document load produces a wasted resolve that always exhausts sources and logs "all sources exhausted". The previous code deliberately filtered to sources that can be refetched (URL/Font); restore that so unrecoverable embedded resources aren't re-resolved.</violation>

<violation number="2" location="editor/src/messages/portfolio/document/resource/resource_message_handler.rs:83">
P1: When the registry hash is missing from storage but `FontsMessageHandler` still remembers that font, this branch treats the cached hash as a successful resolution without restoring any bytes. Check the cached hash with resource storage before returning it; otherwise let the font source fall through to download.</violation>

<violation number="3" location="editor/src/messages/portfolio/document/resource/resource_message_handler.rs:228">
P2: When an embedded resource is missing from storage, `flatten()` silently drops its bytes while the registry still claims they are embedded, so saving produces an incomplete document. Keep the missing-resource diagnostic or abort the save instead of silently dropping the entry.</violation>
</file>

<file name="node-graph/graph-craft/src/application_io/resource/opfs.rs">

<violation number="1" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:158">
P2: When a newly stored hash's background OPFS write fails, retaining `on_disk` prevents later `store` calls from retrying it, leaving the resource only in `cache` and risking its loss after reload. Clear the marker or schedule a retry when the write fails.</violation>
</file>

<file name="node-graph/nodes/gstd/src/platform_application_io.rs">

<violation number="1" location="node-graph/nodes/gstd/src/platform_application_io.rs:273">
P1: The `resource` node now panics when the platform application IO is absent or a resource's data is missing, instead of returning the previous graceful empty-`Resource` fallback. A missing resource is exactly the case this PR targets (older documents with corrupted/evicted font data), and on the WASM target a `panic!` aborts the whole application rather than degrading the single resource. Restore graceful handling: log the error and return `Resource::empty()` as before.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment on lines +83 to 86
DataSource::Font { family, style } => {
let font = match style {
Some(style) => Font::new(family.clone(), style.clone()),
None => Font::new_with_default_style(family.clone()),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When the registry hash is missing from storage but FontsMessageHandler still remembers that font, this branch treats the cached hash as a successful resolution without restoring any bytes. Check the cached hash with resource storage before returning it; otherwise let the font source fall through to download.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/resource/resource_message_handler.rs, line 83:

<comment>When the registry hash is missing from storage but `FontsMessageHandler` still remembers that font, this branch treats the cached hash as a successful resolution without restoring any bytes. Check the cached hash with resource storage before returning it; otherwise let the font source fall through to download.</comment>

<file context>
@@ -89,7 +80,7 @@ impl MessageHandler<ResourceMessage, ResourceMessageContext<'_>> for ResourceMes
 					.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()),
</file context>

};

let application_io = editor_api.into_element().application_io.as_ref().expect("ApplicationIo must be available when using resources");
let resource = application_io.load_resource(hash).await.unwrap_or_else(|| panic!("Resource {hash} not found"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The resource node now panics when the platform application IO is absent or a resource's data is missing, instead of returning the previous graceful empty-Resource fallback. A missing resource is exactly the case this PR targets (older documents with corrupted/evicted font data), and on the WASM target a panic! aborts the whole application rather than degrading the single resource. Restore graceful handling: log the error and return Resource::empty() as before.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/platform_application_io.rs, line 273:

<comment>The `resource` node now panics when the platform application IO is absent or a resource's data is missing, instead of returning the previous graceful empty-`Resource` fallback. A missing resource is exactly the case this PR targets (older documents with corrupted/evicted font data), and on the WASM target a `panic!` aborts the whole application rather than degrading the single resource. Restore graceful handling: log the error and return `Resource::empty()` as before.</comment>

<file context>
@@ -269,19 +269,8 @@ pub async fn resource<'a: 'n>(
-	};
-
+	let application_io = editor_api.into_element().application_io.as_ref().expect("ApplicationIo must be available when using resources");
+	let resource = application_io.load_resource(hash).await.unwrap_or_else(|| panic!("Resource {hash} not found"));
 	Item::new_from_element(resource)
 }
</file context>

}

self.embedded = EmbeddedResources::from_iter(loaded.into_iter().filter_map(|(_, hash, resource)| resource.map(|resource| (hash, resource))));
self.embedded = EmbeddedResources::from_iter(futures::future::join_all(embedded).await.into_iter().flatten());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When an embedded resource is missing from storage, flatten() silently drops its bytes while the registry still claims they are embedded, so saving produces an incomplete document. Keep the missing-resource diagnostic or abort the save instead of silently dropping the entry.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/resource/resource_message_handler.rs, line 228:

<comment>When an embedded resource is missing from storage, `flatten()` silently drops its bytes while the registry still claims they are embedded, so saving produces an incomplete document. Keep the missing-resource diagnostic or abort the save instead of silently dropping the entry.</comment>

<file context>
@@ -225,20 +216,16 @@ impl ResourceMessageHandler {
-		}
-
-		self.embedded = EmbeddedResources::from_iter(loaded.into_iter().filter_map(|(_, hash, resource)| resource.map(|resource| (hash, resource))));
+		self.embedded = EmbeddedResources::from_iter(futures::future::join_all(embedded).await.into_iter().flatten());
 	}
 
</file context>

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When a newly stored hash's background OPFS write fails, retaining on_disk prevents later store calls from retrying it, leaving the resource only in cache and risking its loss after reload. Clear the marker or schedule a retry when the write fails.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graph-craft/src/application_io/resource/opfs.rs, line 158:

<comment>When a newly stored hash's background OPFS write fails, retaining `on_disk` prevents later `store` calls from retrying it, leaving the resource only in `cache` and risking its loss after reload. Clear the marker or schedule a retry when the write fails.</comment>

<file context>
@@ -153,9 +153,6 @@ async fn drain_queue(inner: Arc<Mutex<Inner>>) {
-					inner.lock().unwrap().on_disk.remove(&hash);
 				}
 			}
 			Mutation::Delete { hash } => {
</file context>

let ids: Vec<ResourceId> = self.registry.unresolved().map(|info| info.id).chain(refetchable).collect();

let storage = resource_storage.resources_mut();
let ids: Vec<ResourceId> = self.registry.ids().filter(|id| !self.registry.hash(id).is_some_and(|hash| storage.contains(&hash))).collect();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: ResolveAll now re-resolves every resolved resource whose bytes are missing, including Embedded-only resources that have no fetchable source. For those, Resolve's DataSource::Embedded => continue can never recover the data, so each document load produces a wasted resolve that always exhausts sources and logs "all sources exhausted". The previous code deliberately filtered to sources that can be refetched (URL/Font); restore that so unrecoverable embedded resources aren't re-resolved.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/resource/resource_message_handler.rs, line 53:

<comment>`ResolveAll` now re-resolves every resolved resource whose bytes are missing, including Embedded-only resources that have no fetchable source. For those, `Resolve`'s `DataSource::Embedded => continue` can never recover the data, so each document load produces a wasted resolve that always exhausts sources and logs "all sources exhausted". The previous code deliberately filtered to sources that can be refetched (URL/Font); restore that so unrecoverable embedded resources aren't re-resolved.</comment>

<file context>
@@ -49,15 +49,8 @@ impl MessageHandler<ResourceMessage, ResourceMessageContext<'_>> for ResourceMes
-				let ids: Vec<ResourceId> = self.registry.unresolved().map(|info| info.id).chain(refetchable).collect();
-
+				let storage = resource_storage.resources_mut();
+				let ids: Vec<ResourceId> = self.registry.ids().filter(|id| !self.registry.hash(id).is_some_and(|hash| storage.contains(&hash))).collect();
 				for id in ids {
 					if self.pending_resolves.contains(&id) {
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant