review C: desktop output ownership, converter preflight, cancel race (docs/29 findings 5, 13, 14) - #64
Merged
Conversation
… per request, publish one terminal state Code review C (docs/29 findings 5, 13, 14). Output ownership (#5). A repeated Start in the desktop application launched a second engine into the same results folder: the start flow had several awaits and no in-progress guard, and the backend launched every request. The frontend now refuses a Start while one is in progress or while the run it follows is still running. The backend reserves a run's results folder by canonical path before spawning the engine and releases it when the run's end is published, so a request for an active folder is refused with the owning run named. The CLI's --run-names check compares names without regard to case on every platform, because RunA and runa are one directory on Windows, macOS and most network shares. Converter preflight (#13). The desktop asked `doctor --json` without the request's configuration, so a converter named in convert.thermo_raw_parser or convert.msconvert was reported missing and the search refused; it also required ThermoRawFileParser for Thermo .raw even when msconvert, the engine's fallback for a parser left at auto, was present. The probe now carries the configuration and the verdict is the engine's rule: only msconvert present runs with a note, an explicitly configured parser that is missing blocks. Cancel race (#14). `cancel` and the process waiter both wrote the terminal status, whichever ran second won, and the cancellation flag was never read, so a stopped run could be shown as failed with the last log line as its error. The waiter is the only writer now: it reads the intent after reaping the engine and publishes cancelled, done (the engine had already finished) or failed; until then the snapshot carries cancel_requested and the interface shows Stopping. Also: Dependabot for /desktop and a cargo audit of desktop/Cargo.lock in CI (one documented ignore, RUSTSEC-2024-0429 via Tauri 2's gtk 0.18). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 8, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of the four stacked code-review PRs (
docs/29_code_review_2026-09-07.md, work package C). Base isreview/b-workers(#63); merge A, B, then this.What changes
#5, output ownership. The desktop frontend refuses a Start while one is in progress (guard before the first await) or while the run it follows is still running. The backend reserves a run's results folder by canonical path before spawning the engine and releases it when the run's end is published; a request for an active folder is refused with the owning run named.
run-experiment --run-namesrejects names that differ only in case on every platform.#13, converter preflight.
thermo::converters(config)probesdoctor --json --config <request config>, so converters named inconvert.thermo_raw_parser/convert.msconvertcount.thermo::converter_verdictrestates the engine's rule (raw::ensure_mzml): Thermo with the parser atautoand only msconvert present runs with a note; an explicitly configured parser that is missing blocks; other vendors need msconvert. A probe that cannot run is a warning, not a block.#14, cancel race.
Run::publish_exitis the single writer of the terminal status.cancelrecords intent (cancel_requestedin the snapshot, atomic flag read by the waiter) and kills; the waiter publishescancelled,done(successful exit before the kill landed) orfailed, releases the folder reservation first, and sweeps temp files after the reap.Coverage. Dependabot entry for
/desktop; CI auditsdesktop/Cargo.lock(--ignore RUSTSEC-2024-0429, glib 0.18 pinned by Tauri 2's gtk 0.18, documented in the workflow).Tests
run.rs: controlled interleavings of Stop and the engine's own exit (cancel before the waiter wakes, failure before Stop, success under Stop, wait error), intent-only cancel, reservation refusal, spelling alias, case alias (asserts per what the filesystem reports), release on publish.thermo.rs: Thermo with only msconvert (note, no block); explicit missing parser blocks despite msconvert; off-PATH configured converters pass; nothing installed blocks Thermo and Bruker; probe argv carries--config; report parsing.run_experiment.rs: case-only and repeated run names rejected, distinct pass.Local: desktop
cargo fmt --check,clippy -D warnings,cargo test --lib(78), engineclippy+cargo test --workspace,ci/check_desktop_ui.py,ci/check_workflows.py,cargo audit --file desktop/Cargo.lock ... --ignore RUSTSEC-2024-0429.🤖 Generated with Claude Code