Improve fix for corrupted font resource loading in older documents - #4482
Improve fix for corrupted font resource loading in older documents#4482timon-schelling wants to merge 6 commits into
Conversation
This reverts commit 404d9f3.
…le Polyline nodes
There was a problem hiding this comment.
5 issues found across 5 files
Confidence score: 2/5
node-graph/nodes/gstd/src/platform_application_io.rsnow panics when platform application IO or resource data is missing, removing the prior empty-Resourcefallback for an expected missing-resource case; restore graceful handling to avoid runtime failures.editor/src/messages/portfolio/document/resource/resource_message_handler.rscan 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.rsretains theon_diskmarker 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.rscan silently omit missing embedded bytes duringflatten()and repeatedly attempt unrecoverable Embedded-only resources inResolveAll, 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
| DataSource::Font { family, style } => { | ||
| let font = match style { | ||
| Some(style) => Font::new(family.clone(), style.clone()), | ||
| None => Font::new_with_default_style(family.clone()), |
There was a problem hiding this comment.
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")); |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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>
improvements of 404d9f3