Open documents in bounded reads, with a clear out-of-memory error (#375) (#401)

Every open path read the whole file with std::fs::read, which asks
Windows for up to 4 GiB in a single ReadFile. Some file systems (exFAT,
network redirectors) reject a read that large with
ERROR_INVALID_PARAMETER, "The parameter is incorrect" (os error 87),
as reported for a 4.75 GB PSB in #375.

Add photocraft_format::read_file and read_all beside atomic_write:
64 MiB reads into a buffer reserved up front, and an OutOfMemory error
that says how much the file needed. Route every document read through
them:
- the background open job (engine jobs.rs read_path);
- the engine's read_file;
- Open Recent and OS open events;
- desktop drag and drop, which now goes through open_path (and so
  through the background job when jobs are on);
- the File > Open dialog;
- .pcraft loading;
- the automation workspace read and automation::files::open;
- smart object Replace Contents and linked files.

The dialog no longer fails silently: PickOpenFn returns the picked name
with the read result, and a failure is reported through open_failed and
the notices like any other open failure.

Speed is unchanged (1.11 s vs 1.13 s for a warm 4.75 GB file).

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
ljkx
2026-10-07 07:40:35 -04:00
committed by GitHub
co-authored by Claude Opus 5.5
parent fbe5f80ba4
commit a8afec4455
16 changed files with 199 additions and 27 deletions
+2 -1
View File
@@ -156,7 +156,8 @@ pub fn native(automation: Option<photocraft_automation::AuthorizedWorkspace>) ->
})),
pick_open: Some(Box::new(|| {
let path = rfd::FileDialog::new().add_filter("All Formats", OPEN_EXTS).add_filter("PhotoCraft", &["pcraft"]).pick_file()?;
let bytes = std::fs::read(&path).ok()?;
// A read failure goes back to the app, which reports it like any other open failure.
let bytes = photocraft_format::read_file(&path).map_err(|e| e.to_string());
Some((path.to_string_lossy().to_string(), bytes))
})),
pick_save: Some(Box::new(|suggested: &str| {
+1 -1
View File
@@ -30,7 +30,7 @@ pub fn open(path: &Path) -> Result<Opened, AutomationError> {
if path.is_dir() {
return Ok(Opened { document: photocraft_format::load_path(path)?, warnings: Vec::new() });
}
let bytes = std::fs::read(path).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?;
let bytes = photocraft_format::read_file(path).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?;
let name = path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default();
open_bytes(&name, &bytes)
}
+3 -4
View File
@@ -1,6 +1,6 @@
//! Capability-based filesystem policy for untrusted automation paths.
use std::io::{Read, Write};
use std::io::Write;
use std::path::{Path, PathBuf};
use std::sync::Arc;
@@ -46,9 +46,8 @@ impl AuthorizedWorkspace {
if !metadata.is_file() {
return Err(AutomationError::BadRequest(format!("automation read path is not a regular file: `{path}`")));
}
let mut bytes = Vec::new();
file.read_to_end(&mut bytes).map_err(|e| file_error("read", path, e))?;
Ok(bytes)
// Bounded reads, and a clear error for a file larger than memory (#375).
photocraft_format::read::read_all(&mut file, metadata.len()).map_err(|e| file_error("read", path, e))
}
/// Create or replace one file below the configured write root, crash-safely: the bytes go to
+3 -1
View File
@@ -75,8 +75,10 @@ fn is_file(_path: &str) -> bool {
}
#[cfg(not(target_arch = "wasm32"))]
/// Every file the engine opens is read here: in bounded reads, with a clear error when it does
/// not fit in memory (see [`photocraft_format::read`]).
pub(crate) fn read_file(path: &str) -> Result<Vec<u8>> {
std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))
photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| EngineError::Other(format!("{path}: {e}")))
}
#[cfg(target_arch = "wasm32")]
pub(crate) fn read_file(path: &str) -> Result<Vec<u8>> {
+2 -1
View File
@@ -258,7 +258,8 @@ fn independent_of_document(id: &str) -> bool {
#[cfg(not(target_arch = "wasm32"))]
fn read_path(path: &str) -> Result<Vec<u8>> {
std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))
// Bounded reads, and a clear error for a file larger than memory (#375).
photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| EngineError::Other(format!("{path}: {e}")))
}
#[cfg(target_arch = "wasm32")]
+2 -2
View File
@@ -72,7 +72,7 @@ fn base_name(path: &str) -> String {
}
fn read_file(path: &str) -> Option<Vec<u8>> {
if path.is_empty() { None } else { std::fs::read(path).ok() }
if path.is_empty() { None } else { photocraft_format::read_file(std::path::Path::new(path)).ok() }
}
/// The source file of a smart object: embedded bytes, the PSD's embedded linked-layer data (PSD
@@ -614,7 +614,7 @@ fn set_source(s: &mut Session, p: &Value, label: &str, keep_psd: bool, make: imp
fn replace_contents(s: &mut Session, p: &Value) -> Result<Value> {
let path = path_param("layer.smartObjects.replaceContents", p)?.to_string();
let bytes = std::fs::read(&path).map_err(|e| other(format!("can't read {path}: {e}")))?;
let bytes = photocraft_format::read_file(std::path::Path::new(&path)).map_err(|e| other(format!("can't read {path}: {e}")))?;
let name = base_name(&path);
decode_source(&name, &bytes)?; // fail before touching the document
set_source(s, p, "Replace Contents", false, |_, _| Ok(SmartSource::Embedded { file_name: name, bytes: Arc::new(bytes) }))
+3 -1
View File
@@ -25,6 +25,7 @@ pub mod autosave;
mod convert;
pub mod manifest;
mod migrate;
pub mod read;
mod store;
pub mod zip;
@@ -39,6 +40,7 @@ pub use autosave::RecoveryStore;
pub use autosave::{Autosaver, RecoveryEntry, discard_recovery, list_recovery, recover};
pub use convert::MAX_GROUP_DEPTH;
pub use manifest::{FORMAT_VERSION, Manifest};
pub use read::read_file;
pub use store::{PcraftWriter, SaveStats};
/// File extension of the native format.
@@ -137,7 +139,7 @@ pub fn load_path_with(path: &Path, opts: &LoadOptions) -> Result<Document> {
if path.is_dir() {
store::load(&store::DirSource { root: path.to_path_buf() }, opts)
} else {
let bytes = std::fs::read(path)?;
let bytes = read::read_file(path)?;
load_from_bytes_with(&bytes, opts)
}
}
+122
View File
@@ -0,0 +1,122 @@
//! Reading a whole file to open it: the read side of [`crate::atomic`].
//!
//! What it adds over `std::fs::read` for multi-gigabyte PSB files:
//!
//! - **Bounded reads.** Each read asks for at most [`READ_CHUNK`] bytes. `std` asks Windows for up
//! to 4 GiB in one `ReadFile`, and some file systems (exFAT, network redirectors) reject a read
//! that large with `ERROR_INVALID_PARAMETER` ("The parameter is incorrect", #375).
//! - **A clear out-of-memory error.** The buffer is reserved up front with `try_reserve_exact`,
//! and a file larger than the memory available says how much it needed.
//!
//! On `wasm32` there is no file system and every call returns the `std` "unsupported" error.
use std::io::{self, Read};
use std::path::Path;
/// Largest single read: big enough that a 5 GB file takes under a hundred reads.
pub const READ_CHUNK: usize = 64 << 20;
/// Read all of `path`, in reads of at most [`READ_CHUNK`] bytes.
pub fn read_file(path: &Path) -> io::Result<Vec<u8>> {
read_file_with(path, READ_CHUNK)
}
/// [`read_file`] with a chosen read size (tests use small ones to exercise the loop).
pub fn read_file_with(path: &Path, chunk: usize) -> io::Result<Vec<u8>> {
let mut f = std::fs::File::open(path)?;
let len = f.metadata().map(|m| m.len()).unwrap_or(0);
read_all_with(&mut f, len, chunk)
}
/// Read `reader` to its end in reads of at most [`READ_CHUNK`] bytes, for an already open file
/// (a capability-scoped handle, say). `size_hint` (the file's length) sizes the buffer up front.
pub fn read_all(reader: &mut impl Read, size_hint: u64) -> io::Result<Vec<u8>> {
read_all_with(reader, size_hint, READ_CHUNK)
}
fn read_all_with(reader: &mut impl Read, size_hint: u64, chunk: usize) -> io::Result<Vec<u8>> {
// The hint only sizes the buffer: a file that grows or shrinks while being read is still read
// to its end.
let mut buf = buffer_for(size_hint)?;
let chunk = u64::try_from(chunk.max(1)).unwrap_or(u64::MAX);
while reader.take(chunk).read_to_end(&mut buf)? > 0 {}
Ok(buf)
}
/// An empty buffer with room for `len` bytes, or an `OutOfMemory` error saying how much was needed.
pub fn buffer_for(len: u64) -> io::Result<Vec<u8>> {
let mut buf = Vec::new();
let reserved = usize::try_from(len).ok().and_then(|n| buf.try_reserve_exact(n).ok());
match reserved {
Some(()) => Ok(buf),
None => Err(io::Error::new(io::ErrorKind::OutOfMemory, format!("not enough memory to read the whole file ({})", size_text(len)))),
}
}
/// `len` bytes for people: "4.75 GB", "759 MB", "12 KB".
fn size_text(len: u64) -> String {
const UNITS: [&str; 5] = ["bytes", "KB", "MB", "GB", "TB"];
let mut v = len as f64;
let mut unit = 0;
while v >= 1000.0 && unit + 1 < UNITS.len() {
v /= 1000.0;
unit += 1;
}
match UNITS.get(unit) {
Some(name) if unit > 0 => format!("{v:.2} {name}"),
_ => format!("{len} bytes"),
}
}
#[cfg(test)]
mod tests {
use super::*;
fn temp(name: &str, bytes: &[u8]) -> std::path::PathBuf {
let dir = std::env::temp_dir().join(format!("photocraft-read-{}-{name}", std::process::id()));
std::fs::create_dir_all(&dir).unwrap();
let p = dir.join("f.bin");
std::fs::write(&p, bytes).unwrap();
p
}
#[test]
fn reads_whole_file_across_many_chunks() {
let data: Vec<u8> = (0..100_003u32).map(|i| (i * 7 % 251) as u8).collect();
let p = temp("chunks", &data);
for chunk in [1, 7, 4096, 100_003, 1 << 20] {
assert_eq!(read_file_with(&p, chunk).unwrap(), data, "chunk {chunk}");
}
assert_eq!(read_file(&p).unwrap(), data);
assert_eq!(read_file_with(&p, 0).unwrap(), data, "a zero chunk still makes progress");
// From an open reader, whatever the hint says.
for hint in [0, 10, data.len() as u64, 1 << 20] {
assert_eq!(read_all(&mut std::io::Cursor::new(&data), hint).unwrap(), data, "hint {hint}");
}
}
#[test]
fn empty_and_missing_files() {
assert!(read_file(&temp("empty", b"")).unwrap().is_empty());
let missing = std::env::temp_dir().join("photocraft-read-does-not-exist.psb");
assert_eq!(read_file(&missing).unwrap_err().kind(), io::ErrorKind::NotFound);
}
#[test]
fn a_file_too_large_for_memory_is_a_clear_error() {
// More than any address space can hold.
let e = buffer_for(u64::MAX).unwrap_err();
assert_eq!(e.kind(), io::ErrorKind::OutOfMemory);
assert!(e.to_string().contains("18446744.07 TB"), "{e}");
let e = buffer_for(1 << 62).unwrap_err();
assert_eq!(e.kind(), io::ErrorKind::OutOfMemory);
assert_eq!(buffer_for(1024).unwrap().capacity(), 1024);
}
#[test]
fn sizes_read_like_the_os_shows_them() {
assert_eq!(size_text(512), "512 bytes");
assert_eq!(size_text(4_750_000_000), "4.75 GB");
assert_eq!(size_text(759_000_000), "759.00 MB");
}
}
+2 -1
View File
@@ -19,6 +19,8 @@ photocraft-plugins = { workspace = true }
photocraft-algo = { workspace = true }
photocraft-text = { workspace = true }
photocraft-vector = { workspace = true }
# Reading files to open them (bounded reads, a clear error for files larger than memory).
photocraft-format = { workspace = true }
egui = { workspace = true }
egui_extras = { version = "0.36", default-features = false, features = ["svg"] }
eframe = { workspace = true, features = ["default_fonts", "wgpu", "accesskit"] }
@@ -36,7 +38,6 @@ egui_kittest = { workspace = true, features = ["wgpu", "snapshot", "eframe"] }
photocraft-io = { workspace = true }
# Only for examples/interactive_bench.rs (JPEG encode, .pcraft saves).
photocraft-codecs = { workspace = true }
photocraft-format = { workspace = true }
# Benchmark helpers for the `--json` benches `cargo xtask perf` runs (in-process RSS, percentiles).
photocraft-testkit = { workspace = true }
# Only for examples/perf_scenarios.rs (a 500-brush .abr for the #210 import row).
+4 -1
View File
@@ -159,7 +159,10 @@ pub fn menu(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option<Result<
r
}
"file.import.notes" => {
let (name, bytes) = app.services.pick_open.as_mut().and_then(|f| f())?;
let (name, bytes) = match app.pick_file_bytes()? {
Ok(picked) => picked,
Err(e) => return Some(Err(e)),
};
let r = photocraft_engine::notes_cmds::import_notes_from(&mut app.session, &name, &bytes).map_err(|e| e.to_string());
if r.is_ok() {
app.ui.analysis.notes = true;
+8 -3
View File
@@ -54,7 +54,7 @@ impl PhotocraftApp {
crate::jobs_ui::start_open(self, &display_name(path), Some(path.to_string()), photocraft_engine::jobs::OpenSource::Path(path.to_string()))?;
return Ok(Vec::new());
}
let bytes = std::fs::read(path).map_err(|e| format!("{path}: {e}"))?;
let bytes = photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| format!("{path}: {e}"))?;
self.open_file(path, &bytes)
}
@@ -91,8 +91,13 @@ impl PhotocraftApp {
for f in files {
let path = f.path().to_string_lossy().to_string();
let name = f.path().file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "dropped".into());
let opened =
crate::read_dropped(&*f).and_then(|bytes| if f.path().is_absolute() { self.open_file(&path, &bytes) } else { self.open_bytes(&name, &bytes) });
// A desktop drop opens like File › Open: on the background worker when jobs are on, and
// read in bounded reads either way (#375) rather than with egui's whole-file read.
let opened = if f.path().is_absolute() {
self.open_path(&path).map(|_| ())
} else {
crate::read_dropped(&*f).and_then(|bytes| self.open_bytes(&name, &bytes)).map(|_| ())
};
if let Err(e) = opened {
self.open_failed(&name, &e);
}
+26 -3
View File
@@ -29,7 +29,7 @@ fn app_with(pick_open: Option<(String, Vec<u8>)>, pick_save: Option<String>) ->
let warnings = if path.ends_with(".png") { vec!["Layers were flattened".to_string()] } else { Vec::new() };
Ok((b"out".to_vec(), warnings))
})),
pick_open: Some(Box::new(move || pick_open.take())),
pick_open: Some(Box::new(move || pick_open.take().map(|(name, bytes)| (name, Ok(bytes))))),
pick_save: Some(Box::new(move |_s: &str| pick_save.clone())),
write: Some(Box::new(move |p: &str, b: &[u8]| {
w.borrow_mut().push((p.to_string(), b.to_vec()));
@@ -140,6 +140,23 @@ fn open_failures_are_errors_and_leave_no_document() {
assert!(app.ui.notices.last().is_some_and(|n| n.error));
}
#[test]
fn a_picked_file_that_cannot_be_read_is_reported_like_any_open_failure() {
let (mut app, _) = app_with(None, None);
let mut picked = Some(("C:/photos/big.psb".to_string(), Err("The parameter is incorrect. (os error 87)".to_string())));
app.services.pick_open = Some(Box::new(move || picked.take()));
app.open_dialog_file();
assert!(app.session.documents().is_empty());
assert!(app.ui.status_error);
let notice = app.ui.notices.last().unwrap();
assert!(notice.error && notice.title.contains("Couldn't open big.psb: The parameter is incorrect"), "{}", notice.title);
// Commands that read a picked file (scripts, notes, Place) get the same message as an error.
app.services.pick_open = Some(Box::new(|| Some(("C:/photos/big.psb".to_string(), Err("denied".to_string())))));
assert_eq!(app.pick_file_bytes(), Some(Err("big.psb: denied".to_string())));
app.services.pick_open = Some(Box::new(|| None));
assert_eq!(app.pick_file_bytes(), None, "cancelled");
}
#[test]
fn open_paths_reports_each_failure_without_panicking() {
let (mut app, _) = app_with(None, None);
@@ -202,10 +219,15 @@ impl egui::DroppedFile for FakeDrop {
#[test]
fn dropped_files_open_with_path_and_recent() {
let (mut app, _) = app_with(None, None);
let abs = std::env::temp_dir().join("dropped.psd");
let dir = std::env::temp_dir().join(format!("photocraft-drop-{}", std::process::id()));
std::fs::create_dir_all(&dir).unwrap();
let abs = dir.join("dropped.psd");
std::fs::write(&abs, b"x").unwrap();
let abs_s = abs.to_string_lossy().to_string();
app.open_dropped(vec![
std::sync::Arc::new(FakeDrop(abs, Ok(b"x".to_vec()))),
// A desktop drop (absolute path) is read from disk like File › Open (#375), never through
// egui's whole-file `std::fs::read`.
std::sync::Arc::new(FakeDrop(abs, Err("egui's reader must not be used".into()))),
// Unreadable and undecodable drops are errors; no path means no recent entry.
std::sync::Arc::new(FakeDrop("gone.psd".into(), Err("permission denied".into()))),
std::sync::Arc::new(FakeDrop("rel.psd".into(), Ok(b"x".to_vec()))),
@@ -218,6 +240,7 @@ fn dropped_files_open_with_path_and_recent() {
assert_eq!(app.ui.recent_files, vec![abs_s]);
assert!(app.ui.status_error);
assert_eq!(app.ui.notices.iter().filter(|n| n.error).count(), 2);
let _ = std::fs::remove_dir_all(&dir);
}
#[test]
+4 -1
View File
@@ -88,7 +88,10 @@ pub fn invoke(app: &mut PhotocraftApp, ctx: &egui::Context, id: &str, params: &V
form(app, id, "Image Statistics", json!({"mode": "median", "input": dir, "align": false}), json!({"mode": modes}))
}
"file.scripts.browse" => {
let (name, bytes) = app.services.pick_open.as_mut().and_then(|f| f())?;
let (name, bytes) = match app.pick_file_bytes()? {
Ok(picked) => picked,
Err(e) => return Some(Err(e)),
};
let r = app.run(id, json!({"script": String::from_utf8_lossy(&bytes)}));
if r.is_ok() {
app.ui.status = format!("Ran script {name}");
+12 -5
View File
@@ -145,7 +145,9 @@ pub struct ExportSettings {
/// Encode a document: (file bytes, warnings about anything approximated or dropped).
pub type ExportFn = Box<dyn Fn(&Document, &str, &ExportSettings) -> Result<(Vec<u8>, Vec<String>), String>>;
pub type PickOpenFn = Box<dyn FnMut() -> Option<(String, Vec<u8>)>>;
/// The picked file's name and its bytes, or why it could not be read (shown like any other open
/// failure); `None` when the dialog was cancelled.
pub type PickOpenFn = Box<dyn FnMut() -> Option<(String, Result<Vec<u8>, String>)>>;
pub type PickSaveFn = Box<dyn FnMut(&str) -> Option<String>>;
pub type WriteFn = Box<dyn FnMut(&str, &[u8]) -> Result<(), String>>;
/// Read bytes through the desktop control session's authorized read root.
@@ -689,14 +691,19 @@ impl PhotocraftApp {
/// File › Open: the platform dialog returns the chosen file's path (native; the web delivers
/// picks through the inbox instead).
pub fn open_dialog_file(&mut self) {
let picked = self.services.pick_open.as_mut().and_then(|f| f());
if let Some((path, bytes)) = picked
&& let Err(e) = self.open_file(&path, &bytes)
{
let Some((path, bytes)) = self.services.pick_open.as_mut().and_then(|f| f()) else { return };
if let Err(e) = bytes.and_then(|bytes| self.open_file(&path, &bytes)) {
self.open_failed(&file_open::display_name(&path), &e);
}
}
/// Show the open dialog for a file a command reads (a script, notes, a placed image, presets):
/// `None` when cancelled, else its name and bytes or the read error.
pub(crate) fn pick_file_bytes(&mut self) -> Option<Result<(String, Vec<u8>), String>> {
let (name, bytes) = self.services.pick_open.as_mut().and_then(|f| f())?;
Some(bytes.map(|b| (name.clone(), b)).map_err(|e| format!("{}: {e}", file_open::display_name(&name))))
}
/// Save the active document to `path` (or a path chosen in the save dialog); returns the path
/// and the export warnings (also shown to the user).
pub fn save_as(&mut self, path: Option<String>) -> Result<(String, Vec<String>), String> {
+1 -1
View File
@@ -1338,7 +1338,7 @@ pub fn confirm(app: &mut PhotocraftApp, f: &Map<String, Value>) -> Result<Value,
"presetsIO" => {
let kinds: Vec<&str> = ["brushes", "customShapes"].into_iter().filter(|k| f.get(*k).and_then(Value::as_bool).unwrap_or(true)).collect();
if f.get("action").and_then(Value::as_str) == Some("import") {
let (name, bytes) = app.services.pick_open.as_mut().and_then(|p| p()).ok_or("cancelled")?;
let (name, bytes) = app.pick_file_bytes().ok_or_else(|| "cancelled".to_string())??;
let text = String::from_utf8(bytes).map_err(|_| format!("{name} is not a preset file"))?;
app.run("edit.presets.exportImportPresets", json!({"action": "import", "kinds": kinds, "data": text}))
} else {
+4 -1
View File
@@ -800,7 +800,10 @@ fn front(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option<Result<Val
}
"file.saveACopy" => Some(save_a_copy(app)),
"file.placeEmbedded" | "file.placeLinked" => {
let (name, bytes) = app.services.pick_open.as_mut().and_then(|f| f())?;
let (name, bytes) = match app.pick_file_bytes()? {
Ok(picked) => picked,
Err(e) => return Some(Err(e)),
};
let linked = (id == "file.placeLinked").then(|| name.clone());
let r = photocraft_engine::file_cmds::place_bytes(&mut app.session, &name, bytes, linked, &json!({})).map_err(|e| e.to_string());
app.sync_views();