review E: follow-up review docs/30 (R1-R9)

CHANGELOG.md

@@ -49,6 +49,12 @@ than a number. Both are recorded in every run's `manifest.json`.
 
 ### Changed
 
+- Model identities carry the installed predictor versions: `deeplc-4.1.1-base`,
+  `deeplc-4.1.1-finetuned`, `ms2pip-4.2.0-HCDch2` in the library report and the manifests,
+  in place of the family labels `deeplc-4.0-mt` and `ms2pip-<model>` (docs/30).
+- With nothing to transfer, the MBR worker writes a transfer table with its ten columns and
+  zero rows, and the requested augmented scored table with every row unflagged, instead of
+  a one-column placeholder and no scored table (docs/30).
 - The candidate-audit rejection code `NO_PEAK_GROUP` is `DID_NOT_SURVIVE_EXTRACTION`
   (`RejectionReason::DidNotSurviveExtraction`). The audit assigns it to every candidate
   with no extracted row, and `extract` does not write the per-candidate table that would
@@ -171,6 +177,41 @@ than a number. Both are recorded in every run's `manifest.json`.
     cancellation flag was written and never read. The waiter is now the only writer: it
     reads the intent after reaping the engine and publishes `cancelled`, `done` when the
     engine had already finished, or `failed`; until then the run shows "Stopping" (#14).
+- Code review E, follow-up (`docs/30_code_review_2026-09-08.md`, R1 to R9):
+  - Enabling DeepLC fine-tuning with its own defaults was rejected at load, because the
+    documented automatic batch size is `finetune_batch = 0` and the new validation demanded
+    a positive batch. Only the epoch count has a lower bound now (R1; a regression from
+    review A).
+  - `run-experiment --run-names` accepted `a` and `a.`, one directory on Windows, and the
+    second run overwrote the first with exit 0. Names ending in a dot or a space, containing
+    `<>:"|?*` or a control character, or naming a Windows reserved device are rejected on
+    every platform before anything is written (R2).
+  - A desktop stop could sweep temporary files that belonged to the next run in the same
+    folder: cancellation swept after the reservation had been released, and a stop on a
+    finished run swept as well. Cancellation is now intent and kill only and inert once the
+    run is terminal; the sweep happens in the waiter, after the reap and before the release,
+    and a stop still killing finishes before the folder changes hands (R3).
+  - Two searches converting the same vendor file concurrently shared one temporary output
+    and one could publish the other's bytes. Each conversion writes a unique partial file
+    under a lock beside the destination; a concurrent converter waits and reuses the
+    result (R4).
+  - Domain checks for the numeric settings review A left unchecked: `mbr.q_anchor`,
+    `min_anchor_runs`, `extract.min_matched_fraction`, `features.bound_peak_fraction`,
+    `quant.reliable_q` and the remaining fractions, correlations, tolerances and counts (R5).
+  - The DeepLC fine-tune and re-prediction worker zipped predictions with peptidoforms
+    without checking the count and silently kept the imported iRT for anything missing. A
+    count mismatch is an error; rows that keep their imported value are counted in
+    `<lib_out>.summary.json` and the engine warns when there are any (R6).
+  - The audit's `reported` flag repeated the precursor gate, so it could read `true` next to
+    `FAILED_PEPTIDE_FDR`, and a decoy could be `REPORTED`; the flag now follows the reason,
+    a decoy past both gates is `REMOVED_DURING_REPORTING`, and a present `precursor_q` of the
+    wrong type is an error rather than a fallback (R7).
+  - The desktop results-folder reservation compared exact folders only, so a search into a
+    child of an active experiment's folder was allowed; ancestors and descendants are
+    refused, siblings are not (R8).
+  - The Windows debug binary overflowed its 1 MiB main-thread stack on `--version`; the CLI
+    runs on a thread with a 256 MiB reservation and an integration test runs the built
+    binary (R9).
 - Code review D, calibration, provenance, reporting (`docs/29`, findings 10, 15, 16, 19):
   - LOESS retention-time calibration switched to the global least-squares line the
     moment a query left the anchor range, while the grid just inside used the local fit,

desktop/src-tauri/src/run.rs

@@ -79,18 +79,38 @@ fn ownership_key(dir: &Path) -> String {
 pub fn reserve_out_dir(dir: &Path, id: &str) -> Result<String, String> {
     let key = ownership_key(dir);
     let mut active = ACTIVE_OUT_DIRS.lock().unwrap_or_else(|e| e.into_inner());
-    if let Some(owner) = active.get(&key) {
+    // Equal keys, and also one folder inside the other (docs/30 R8): an experiment writes
+    // into its per-run subfolders and cleanup walks its whole folder, so a search into a
+    // child of an active experiment, or an experiment over the parent of an active search,
+    // is an overlapping writer. Component-wise, so `out` and `out2` stay independent.
+    if let Some((held, owner)) = active
+        .iter()
+        .find(|(held, _)| **held == key || paths_nest(held, &key))
+    {
+        let relation = if *held == key {
+            "is in use".to_string()
+        } else if Path::new(&key).starts_with(Path::new(held)) {
+            format!("is inside the results folder {held}, which is in use")
+        } else {
+            format!("contains the results folder {held}, which is in use")
+        };
         return Err(format!(
-            "the results folder {} is in use by a search that is still running ({owner}). \
+            "the results folder {} {relation} by a search that is still running ({owner}). \
              Wait for it to finish or stop it, or choose another folder: two searches \
-             writing one folder overwrite each other's results.",
+             writing one folder tree overwrite each other's results.",
             dir.display()
         ));
     }
     active.insert(key.clone(), id.to_string());
     Ok(key)
 }
 
+/// True when one path is an ancestor of the other, by path components.
+fn paths_nest(a: &str, b: &str) -> bool {
+    let (pa, pb) = (Path::new(a), Path::new(b));
+    pa.starts_with(pb) || pb.starts_with(pa)
+}
+
 /// Give a reserved results folder back.
 pub fn release_out_dir(key: &str) {
     let mut active = ACTIVE_OUT_DIRS.lock().unwrap_or_else(|e| e.into_inner());
@@ -231,16 +251,39 @@ impl Run {
     /// publish `failed`, with the last log line as the "error", and this method then
     /// declined to replace a terminal status (docs/29 #14).
     pub fn cancel(&self) {
+        // Inert once terminal: there is no process to kill, and the folder may already
+        // belong to a later run (docs/30 R3). Cleanup is not done here at all any more:
+        // it belongs to `publish_exit`, which runs after the reap and before the
+        // reservation is released, so it can only ever touch this run's own files.
+        if !self.is_active() {
+            return;
+        }
         self.cancelled.store(true, Ordering::SeqCst);
         self.set(|s| s.cancel_requested = true);
-        let pid = self.pid.lock().ok().and_then(|p| *p);
-        if let Some(pid) = pid {
+        // The pid lock is held across the kill, and `publish_exit` retires the pid under
+        // the same lock before it sweeps and releases. A stop still in flight when the
+        // engine is reaped therefore finishes before the folder changes hands, and a stop
+        // that arrives after the reap finds no pid.
+        let guard = self.pid.lock().unwrap_or_else(|e| e.into_inner());
+        if let Some(pid) = *guard {
             kill_tree(pid);
         }
-        // A first sweep once the kill has returned. `publish_exit` sweeps again after
-        // the process is reaped, the only moment nothing can still be writing.
-        let out_dir = self.snapshot().out_dir;
-        sweep_temp_files(Path::new(&out_dir));
+        drop(guard);
+    }
+
+    fn is_active(&self) -> bool {
+        matches!(self.snapshot().status.as_str(), "running" | "starting")
+    }
+
+    /// Apply `f` only while the run is still active; returns whether it was.
+    fn set_if_active<F: FnOnce(&mut Snapshot)>(&self, f: F) -> bool {
+        if let Ok(mut s) = self.snapshot.lock() {
+            if matches!(s.status.as_str(), "running" | "starting") {
+                f(&mut s);
+                return true;
+            }
+        }
+        false
     }
 
     /// Publish the terminal state of the run from how its process ended.
@@ -255,8 +298,16 @@ impl Run {
     /// engine finished before the kill landed and its outputs are complete, and
     /// calling them cancelled would hide a finished result.
     fn publish_exit(&self, outcome: std::io::Result<std::process::ExitStatus>, out_dir: &Path) {
+        // Retire the pid first. This waits for a stop that is still killing (it holds the
+        // same lock), so nothing below overlaps a kill, and a later stop finds nothing to
+        // signal (docs/30 R3).
+        if let Ok(mut p) = self.pid.lock() {
+            *p = None;
+        }
         let cancelled = self.cancelled.load(Ordering::SeqCst);
         if cancelled {
+            // The only sweep: after the reap, before the release, inside this run's
+            // ownership of the folder.
             sweep_temp_files(out_dir);
         }
         let stages = scan_stages(out_dir);
@@ -671,15 +722,15 @@ pub fn start(id: String, req: Request) -> Result<Arc<Run>, String> {
             let started = Instant::now();
             loop {
                 let stages = scan_stages(&out_dir);
-                let running = {
-                    let s = run.snapshot();
-                    s.status == "running" || s.status == "starting"
-                };
-                run.set(|s| {
+                // Written only while the run is still active, under the snapshot lock:
+                // once `publish_exit` has published, a scan that was in flight must not
+                // replace the finished snapshot's stages with whatever the folder holds
+                // now, which may already be a later run's contents (docs/30).
+                let still_active = run.set_if_active(|s| {
                     s.stages = stages;
                     s.elapsed_ms = started.elapsed().as_millis() as u64;
                 });
-                if !running {
+                if !still_active {
                     // The final scan belongs to the waiter, not here: it has to happen
                     // BEFORE the status becomes terminal, or a caller that polls until
                     // the run is finished can read a snapshot whose stages and results
@@ -903,7 +954,8 @@ mod tests {
         let s = run.snapshot();
         assert_eq!(s.status, "failed");
         assert_eq!(s.error.as_deref(), Some("Error: no such file"));
-        assert!(s.cancel_requested);
+        // A stop that arrives after the end is inert and records nothing (docs/30 R3).
+        assert!(!s.cancel_requested);
         let _ = std::fs::remove_dir_all(&dir);
     }
 
@@ -948,6 +1000,84 @@ mod tests {
         let _ = std::fs::remove_dir_all(&dir);
     }
 
+    #[test]
+    fn a_late_stop_after_the_run_ended_leaves_the_folder_alone() {
+        // docs/30 R3: A finished and released its folder; B took it and is writing. A
+        // stop delivered to A must neither sweep B's temporary file nor change A's state.
+        let (run, dir) = running("late_stop");
+        let key = reserve_out_dir(&dir, "run-A").unwrap();
+        *run.reservation.lock().unwrap() = Some(key);
+        run.publish_exit(Ok(exit_status(1)), &dir);
+        assert_eq!(run.snapshot().status, "failed");
+        let key_b = reserve_out_dir(&dir, "run-B").expect("A released its folder");
+        let b_file = dir.join("new.parquet.tmp-999-1");
+        std::fs::write(&b_file, b"B's partial write").unwrap();
+        run.cancel();
+        assert!(
+            b_file.is_file(),
+            "a late stop must not sweep another run's files"
+        );
+        let s = run.snapshot();
+        assert_eq!(s.status, "failed");
+        assert!(!s.cancel_requested, "a terminal run records no stop");
+        release_out_dir(&key_b);
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn a_stop_still_in_flight_finishes_before_the_folder_is_released() {
+        // docs/30 R3, the concurrent route: the engine is reaped while the stop thread is
+        // still inside the kill. Publication must wait for the kill to finish, so the
+        // reservation cannot be released, and taken by a new run, while the stop is
+        // still active in that folder. The kill is simulated by holding the pid lock.
+        let (run, dir) = running("inflight_stop");
+        let key = reserve_out_dir(&dir, "run-A").unwrap();
+        *run.reservation.lock().unwrap() = Some(key);
+        run.cancelled.store(true, Ordering::SeqCst);
+        let killing = run.pid.lock().unwrap();
+        let (r2, d2) = (Arc::clone(&run), dir.clone());
+        let waiter = std::thread::spawn(move || r2.publish_exit(Ok(exit_status(1)), &d2));
+        std::thread::sleep(Duration::from_millis(300));
+        assert!(
+            reserve_out_dir(&dir, "run-B").is_err(),
+            "the folder must stay reserved while the stop is in flight"
+        );
+        assert_eq!(
+            run.snapshot().status,
+            "running",
+            "nothing is published mid-kill"
+        );
+        drop(killing);
+        waiter.join().unwrap();
+        assert_eq!(run.snapshot().status, "cancelled");
+        let k = reserve_out_dir(&dir, "run-B").expect("released once the stop completed");
+        release_out_dir(&k);
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn overlapping_result_folders_are_refused_in_both_orders_but_siblings_are_not() {
+        // docs/30 R8: an experiment owns its per-run subfolders and its cleanup walks the
+        // whole tree, so a parent and a child are one writer.
+        let parent = scratch("nest");
+        let child = parent.join("run1");
+        let sibling = std::env::temp_dir().join(format!("mumdia_run_{}_nest2", std::process::id()));
+        std::fs::create_dir_all(&child).unwrap();
+        std::fs::create_dir_all(&sibling).unwrap();
+        let k = reserve_out_dir(&parent, "run-1").unwrap();
+        let e = reserve_out_dir(&child, "run-2").unwrap_err();
+        assert!(e.contains("is inside") && e.contains("run-1"), "{e}");
+        let ks = reserve_out_dir(&sibling, "run-3").expect("a sibling is independent");
+        release_out_dir(&k);
+        release_out_dir(&ks);
+        let kc = reserve_out_dir(&child, "run-2").unwrap();
+        let e = reserve_out_dir(&parent, "run-1").unwrap_err();
+        assert!(e.contains("contains") && e.contains("run-2"), "{e}");
+        release_out_dir(&kc);
+        let _ = std::fs::remove_dir_all(&parent);
+        let _ = std::fs::remove_dir_all(&sibling);
+    }
+
     #[test]
     fn a_results_folder_owned_by_an_active_run_is_refused_to_a_second() {
         let dir = scratch("owned");

docs/04_convert.md

@@ -540,7 +540,13 @@ than its input is either from a different acquisition of the same name or from
 before the input was re-acquired, and searching it would search the wrong data. An
 unreadable timestamp counts as not reusable.
 
-The converter writes to `<name>.partial.mzML` and the engine renames it to
+The converter writes to `<name>.partial-<pid>-<n>.mzML`, a name unique to this
+conversion, under a `<name>.mzML.converting` lock beside the destination; a second
+process converting the same input waits for the lock and reuses the finished mzML
+instead of converting into the same destination (docs/30 R4: two concurrent
+conversions used to share one partial file, and one of them published the other's
+bytes). A lock whose holder stopped writing for fifteen minutes is broken. The engine
+then renames the partial file to
 `<name>.mzML` only after a zero exit and a file at that path, so a killed run or a
 converter crash leaves nothing the reuse rule can mistake for a finished conversion;
 a stale `.partial.mzML` is removed at the next attempt. The marker sits in the stem

docs/06_predict_frag_index_matchers.md

@@ -156,9 +156,10 @@ paired decoy leave together; the counts land in the library report as
 used to be anchored at `irt = 0.0` with a warning, which collapsed the RT window onto
 the gradient origin for those candidates (docs/29 #17). `run_deeplc` also rejects a
 returned id that was not requested or that appears twice. The DeepLC branch requires `deeplc_python`
-and errors otherwise (`predict_frag.rs:318-321`); its returned model id is the
-hardcoded string `"deeplc-4.0-mt"` (`predict_frag.rs:353`), not a trait
-`identity()` (the sidecar path has no `RtPredictor` impl to query).
+and errors otherwise (`predict_frag.rs:318-321`); its returned model id is
+`deeplc-<installed version>-base`, the version read from the interpreter, not a trait
+`identity()` (the sidecar path has no `RtPredictor` impl to query). It was the literal
+`"deeplc-4.0-mt"` until docs/30, which named a family, not the release that predicted.
 
 **intensity assignment** (`assign_intensities`, `predict_frag.rs:359`). Native
 path calls `NativeFrag::predict_intensities`. MS2PIP path runs the sidecar over
@@ -422,8 +423,8 @@ m/z (`Library::local_frag_index`, `index.rs:325`).
 | `PredictFragParams` | `predict_frag.rs:24` | Stage C entry args: in/out paths, `cfg`, `work_dir`, `config_hash` |
 | `predict_frag::run` | `predict_frag.rs:50` | Stage C entry: parse, fragment, assign intensity/iRT, top-N, sort, write; returns `(n_prec, n_frag)` |
 | `Raw` | `predict_frag.rs:34` | one candidate pre-assignment; caches the `ParsedPeptidoform` so RT/intensity reuse the parse |
-| `assign_rt` | `predict_frag.rs:308` | native or DeepLC iRT; emits the DeepLC-miss warning; DeepLC id `"deeplc-4.0-mt"` |
-| `assign_intensities` | `predict_frag.rs:359` | native or MS2PIP intensity with per-charge-group normalization + native charge-2 fallback; MS2PIP id `"ms2pip-{model}"` |
+| `assign_rt` | `predict_frag.rs:308` | native or DeepLC iRT; emits the DeepLC-miss warning; DeepLC id `deeplc-<version>-base` |
+| `assign_intensities` | `predict_frag.rs:359` | native or MS2PIP intensity with per-charge-group normalization + native charge-2 fallback; MS2PIP id `ms2pip-<version>-{model}` |
 | `fragment_cardinality` | `predict_frag.rs:457` | distinct precursors per 0.01 Da fragment-m/z bin, per fragment row; diagnostic column, no consumer yet |
 | `RtPredictor` / `FragmentPredictor` | `predict.rs:13` / `predict.rs:19` | predictor traits (predict + `identity`); implemented only by the native structs |
 | `NativeRt` | `predict.rs:25` | additive retention-coefficient model + `sqrt(len)` + `0.01*mod` term, `identity` `native-rt-v1` |