From d7d5c6069c8f763b8ea3bf1290440bf76a4eed3a Mon Sep 17 00:00:00 2001 From: Reynold Hu Date: Fri, 9 Oct 2026 23:51:51 +0800 Subject: [PATCH] Test the dispatch panic guard end to end; mark REL-3 done (#1803) A cfg(test)-only test.panic command drives Session::execute into the catch_unwind guard in jobs::dispatch; the test asserts an internal-error Err, an untouched document and history, and a working session afterwards. REL-3 flips to done. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands.rs | 14 ++++++++++++++ crates/engine/src/jobs_tests.rs | 17 +++++++++++++++++ docs/scorecard.md | 4 ++-- scorecard/reliability.toml | 4 ++-- 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index aa31010bee..85c82e82c0 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -1170,9 +1170,23 @@ fn build() -> Vec { v.extend(crate::fx_visibility_cmds::specs()); v.extend(crate::mask_view_cmds::specs()); v.extend(crate::actions_cmds::specs()); + // The dispatch guard's unit test needs a command that panics on purpose (REL-3). + #[cfg(test)] + v.push(test_panic_command()); v } +/// A command whose `run` panics on purpose, so the last-resort guard in `jobs::dispatch` +/// can be tested (REL-3). Test builds only: `commands::build` pushes it under `cfg(test)`. +#[cfg(test)] +fn test_panic_command() -> CommandSpec { + #[allow(clippy::panic)] + fn run(_: &mut Session, _: &Value) -> Result { + panic!("test: this command panicked on purpose") + } + CommandSpec { id: "test.panic", label: "Test Panic", menu: &[], shortcut: None, params: r##"{}"##, enabled: |_: &Session| Ok(()), run, journal: false } +} + /// Commands generated from a table share one `run` fn; the kind is recovered from the id. pub(crate) fn inject_kind(id: &str, params: Value) -> Value { let kind = id.strip_prefix("layer.newAdjustmentLayer.").or_else(|| id.strip_prefix("image.adjustments.")); diff --git a/crates/engine/src/jobs_tests.rs b/crates/engine/src/jobs_tests.rs index e9a4934682..548fff28a0 100644 --- a/crates/engine/src/jobs_tests.rs +++ b/crates/engine/src/jobs_tests.rs @@ -126,6 +126,23 @@ fn a_panic_in_the_worker_becomes_an_error_and_the_document_is_unchanged() { s.execute("filter.blur.gaussianBlur", json!({"radius": 1})).unwrap(); } +#[test] +fn a_panic_in_a_command_becomes_an_error_and_the_document_is_unchanged() { + // REL-3: the last-resort guard in `jobs::dispatch` catches a panic that escapes `run`, + // reports it as an error and leaves the document (and its history) untouched. + let mut s = session(64, 64); + let doc_before = s.active().unwrap().doc.clone(); + let (rev, steps) = (s.active().unwrap().revision, s.active().unwrap().history.past_len()); + let msg = s.execute("test.panic", json!({})).unwrap_err().to_string(); + assert!(msg.contains("internal error"), "{msg}"); + assert!(msg.contains("test.panic"), "{msg}"); + let st = s.active().unwrap(); + assert!(Arc::ptr_eq(&st.doc, &doc_before), "the document snapshot was not replaced"); + assert_eq!((st.revision, st.history.past_len()), (rev, steps)); + // The session still works. + s.execute("filter.blur.gaussianBlur", json!({"radius": 1})).unwrap(); +} + #[test] fn progress_is_monotonic_and_reaches_one() { let ctx = JobCtx::new(); diff --git a/docs/scorecard.md b/docs/scorecard.md index 021ae00cf1..50c95c23b8 100644 --- a/docs/scorecard.md +++ b/docs/scorecard.md @@ -12,7 +12,7 @@ Generated by `cargo xtask scorecard` from `perf/budgets.toml`, `perf/baseline.js | [Workspace and UI](#workspace-and-ui) | 1 | 7 | 15 | settings that do nothing: 55 of 141 (target 0) | | [Type](#type) | 0 | 3 | 6 | | | [Automation](#automation) | 3 | 2 | 6 | 627 of 627 menu items live (100.0%) | -| [Reliability](#reliability) | 4 | 1 | 2 | never-crash lints on 28 of 28 crates; panic_hunt present | +| [Reliability](#reliability) | 5 | 0 | 2 | never-crash lints on 28 of 28 crates; panic_hunt present | | [Distribution](#distribution) | 4 | 4 | 3 | | | [Open bugs](#open-bugs) | 22 | 2 | 8 | | @@ -228,7 +228,7 @@ Never-crash attribute on 28 of 28 crate roots. |---|---|---|---|---| | REL-1 | panic_hunt fuzzes every command with adversarial params | done | | engine/tests/panic_hunt.rs: every command, own thread, 4 s timeout; #[ignore], run in the gate | | REL-2 | Every workspace crate carries the never-crash deny(clippy::unwrap_used, ...) attribute | done | | Counted automatically above (crate roots under crates/ and apps/); testkit gained it in #221 | -| REL-3 | The app catches escaped panics around command dispatch and import/export | partial | | engine lib.rs:336 dispatch; crash_guard.rs:21 import/export; the dispatch guard is untested | +| REL-3 | The app catches escaped panics around command dispatch and import/export | done | | a_panic_in_a_command_... runs the test-only `test.panic` spec through Session::execute and asserts Err plus an unchanged document (engine jobs_tests.rs); the worker guard is covered by a_panic_in_the_worker_...; crash_guard.rs guard_turns_a_panic_into_an_error covers import/export (apps/photocraft) | | REL-4 | Atomic save (temp, fsync, rename) for every document write | done | [#203](https://github.com/storytold/photocraft/issues/203) | photocraft_format::atomic_write (format/src/atomic.rs, #230): same-dir temp, sync_all, rename with Windows PermissionDenied retry, Unix dir fsync; every document write site routes through it and format/src/atomic/tests.rs failure-injects write, rename and read-only-dir errors | | REL-212-1 | Fallible allocation for paste/erase/fill/transform/save/import, clear out-of-memory error | missing | [#212](https://github.com/storytold/photocraft/issues/212) | try_reserve only in io/flat.rs:221; codec max_alloc limits cover decode only | | REL-212-2 | Export switches to PSB when the estimated file size exceeds 2 GB | done | [#212](https://github.com/storytold/photocraft/issues/212) | PSD export uses checked size estimates for raster, masks, channels, and retained metadata | diff --git a/scorecard/reliability.toml b/scorecard/reliability.toml index f0ff269dd5..a777e62378 100644 --- a/scorecard/reliability.toml +++ b/scorecard/reliability.toml @@ -20,8 +20,8 @@ note = "Counted automatically above (crate roots under crates/ and apps/); testk [[item]] id = "REL-3" target = "The app catches escaped panics around command dispatch and import/export" -status = "partial" -note = "engine lib.rs:336 dispatch; crash_guard.rs:21 import/export; the dispatch guard is untested" +status = "done" +note = "a_panic_in_a_command_... runs the test-only `test.panic` spec through Session::execute and asserts Err plus an unchanged document (engine jobs_tests.rs); the worker guard is covered by a_panic_in_the_worker_...; crash_guard.rs guard_turns_a_panic_into_an_error covers import/export (apps/photocraft)" [[item]] id = "REL-4"