review F: whole-repository review docs/31 (F1-F10)

CHANGELOG.md

@@ -177,6 +177,63 @@ 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 F, whole-repository review (`docs/31_code_review_2026-09-08_full.md`,
+  F1 to F10):
+  - `prescan` read the infinite-bounds sentinel that `rt-im-train` writes for "calibration
+    unavailable, search the whole gradient" as "cannot be screened" and dropped the
+    candidate, so a run with no confident seeds discarded the entire library and exited 0
+    with a zero-row survivors table. An unbounded window now screens over the whole
+    gradient, a candidate with no window row is treated the same, both are counted, and
+    screening every candidate away is an error (F1).
+  - A present-but-wrong-typed `is_transferred` was swallowed as "no transfers", silently
+    removing every match-between-runs identification from `peptides.tsv` and
+    `proteins.tsv` while the parquet still carried them. Present columns are read in their
+    declared type and a mismatch is an error; only an absent column falls back (F2).
+  - `sidecar::resolve_script` tried the working directory before the directory beside the
+    binary, the ordering `python::resolve_script_dir` was hardened against, so a `scripts/`
+    directory inside an untrusted dataset could have its worker executed. An absolute
+    directory is taken as given, then the executable's directory, then `<exe>/scripts`, and
+    the working directory last (F3).
+  - `Loess::predict` indexed before the start of its grid for a non-finite query, so one
+    library row with a null `predicted_irt` could abort or misread memory at rt-im-train.
+    It returns NaN, and rt-im-train treats a non-finite library iRT as "no calibrated RT"
+    and counts those rows (F4).
+  - The rescorer's in-memory TSV backend standardised with median/IQR while the parquet and
+    streaming backends used mean/std, so the same pool scored differently depending on
+    `rescore.handoff` and on the 4 GB streaming threshold. All three use mean/std, which
+    leaves the shipped parquet default and every published benchmark unchanged. The
+    `MUMDIA_NN_FOLD_KEYS` companion is length-checked instead of being sliced short, which
+    used to leave the tail rows unscored at a fabricated mid-rank score, and the estimate
+    that picks the backend counts feature columns by name (F5).
+  - `refuse_output_over_input` was wired into two of eighteen stages, so
+    `compete --features f.parquet --out f.parquet` replaced the widest artifact of the run
+    with the competed subset at exit 0. It now guards every output of `search-seed`,
+    `rt-im-train`, `extract`, `features`, `compete`, `rescore`, `quant` and `audit` (F6).
+  - The LOESS boundary extrapolation slope introduced in the previous package was the
+    pointwise local slope at the sparsest, most one-sided point of the fit: unbounded, free
+    to be negative, and multiplying an unbounded distance. It is the secant of the fitted
+    curve over its end decile, clamped non-negative and to at most four times the global
+    slope, and the test uses noisy anchors rather than a noiseless quadratic (F7).
+    Measured on HYE B01 against the previous behaviour, same library and settings: 48,533
+    stripped peptides at 1%, 53,127 PSM-q 1% targets, 6,519 protein groups and 1,961,800
+    extracted rows in both arms, identical to the row. The two extrapolations agree
+    wherever the anchors are dense and differ only outside the anchor range.
+  - A desktop stop arriving between the reap and the end of `publish_exit` could pass a
+    recycled process id to the tree kill. The waiter retires the id the instant `wait`
+    returns, before it reads the output directory (F8).
+  - The conversion lock added in the previous package spun without pause on an undeletable
+    stale lock, mistook clock skew and a peer's partial file for evidence about its own
+    holder, could be held by two processes at once, and left every interrupted conversion's
+    partial mzML behind for ever. Take-overs are bounded and paced, the holder is
+    identified by a token it reads back, a future modification time counts as fresh, the
+    partial-file probe matches this destination only, and abandoned partials are swept
+    under the lock (F9).
+  - Dropping an unpredicted candidate with everything sharing its pair key also removed
+    positional isomers that predicted correctly, bounded only by the library being emptied.
+    The direct misses and the collateral are counted separately and exceeding 2% of the
+    library is an error naming the sidecar. The key stays position-free deliberately: a
+    positional key would stop matching a reverse decoy to its target, trading a sensitivity
+    defect for an FDR one (F10).
 - 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

desktop/src-tauri/src/run.rs

@@ -275,6 +275,22 @@ impl Run {
         matches!(self.snapshot().status.as_str(), "running" | "starting")
     }
 
+    /// Forget the process id, under the lock a stop takes before it kills.
+    ///
+    /// Called the instant `wait` returns, before anything else. The pid is free for the
+    /// operating system to reuse from that moment, and a stop landing later would
+    /// otherwise pass it to `kill_tree`, which on Windows terminates whatever now owns it
+    /// and its whole tree (docs/31 F8). Retiring it inside `publish_exit` was too late:
+    /// that function scans the output directory and reads the result reports first, so
+    /// the window was as long as that disk work. It is now a few instructions, and a stop
+    /// that reaches the lock inside it still finds the pid this run really owns, because
+    /// the reap has only just returned.
+    fn retire_pid(&self) {
+        if let Ok(mut p) = self.pid.lock() {
+            *p = None;
+        }
+    }
+
     /// 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() {
@@ -298,12 +314,11 @@ 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;
-        }
+        // Idempotent: the waiter retires the pid the moment `wait` returns (docs/31 F8),
+        // and this call is what makes `publish_exit` safe to reach from a test or any
+        // other path. Taking the lock here also waits for a stop that is still killing,
+        // so nothing below overlaps a kill.
+        self.retire_pid();
         let cancelled = self.cancelled.load(Ordering::SeqCst);
         if cancelled {
             // The only sweep: after the reap, before the release, inside this run's
@@ -748,6 +763,8 @@ pub fn start(id: String, req: Request) -> Result<Arc<Run>, String> {
         let out_dir = PathBuf::from(&req.out_dir);
         std::thread::spawn(move || {
             let outcome = child.wait();
+            // Before anything else: the pid is reusable from here (docs/31 F8).
+            run.retire_pid();
             run.publish_exit(outcome, &out_dir);
         });
     }
@@ -988,6 +1005,22 @@ mod tests {
         let _ = std::fs::remove_dir_all(&dir);
     }
 
+    #[test]
+    fn a_stop_after_the_reap_has_no_pid_to_kill() {
+        // docs/31 F8: the pid is reusable the moment `wait` returns, so the waiter retires
+        // it there rather than after reading the output directory. A stop arriving in
+        // between must find nothing, not a recycled pid.
+        let (run, dir) = running("reaped");
+        *run.pid.lock().unwrap() = Some(4242);
+        run.retire_pid();
+        assert_eq!(*run.pid.lock().unwrap(), None);
+        // The run is still active, so cancel proceeds and simply has nothing to signal.
+        run.cancel();
+        assert!(run.snapshot().cancel_requested);
+        assert_eq!(*run.pid.lock().unwrap(), None);
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
     #[test]
     fn cancel_records_the_intent_without_publishing_a_terminal_status() {
         // Until the engine is reaped the run is still running, whatever the button

docs/06_predict_frag_index_matchers.md

@@ -429,7 +429,7 @@ m/z (`Library::local_frag_index`, `index.rs:325`).
 | `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` |
 | `NativeFrag` | `predict.rs:73` | heuristic b/y intensity model (y=1.0, b=0.75, mid-seq positional, charge-2 x0.5), max-normalized, `identity` `native-frag-v1` |
-| `resolve_script` | `sidecar.rs:20` | locate a worker script (CWD, exe dir/dir, exe dir/scripts, else CWD-relative) |
+| `resolve_script` | `sidecar.rs:20` | locate a worker script (an absolute directory as given, else exe dir/dir, exe dir/scripts, and the working directory LAST; docs/31 F3) |
 | `run_ms2pip` | `sidecar.rs:42` | MS2PIP client; in `id`/`peptidoform`/`charge`, out `id`/`ion_type`/`ordinal`/`intensity`; returns `cid -> (ion_byte, ordinal) -> intensity` |
 | `run_deeplc` | `sidecar.rs:81` | DeepLC client; in `id`/`peptidoform`, out `id`/`predicted_rt`; returns `id -> predicted_rt` |
 | `run_deeplc_finetune` | `sidecar.rs:111` | DeepLC multitask fine-tune; `deeplc_finetune.py <lib_in> <seed> <lib_out>` + epoch/patience/q-train/batch flags (called by `run`, not predict-frag) |

docs/08_rt_im_train.md

@@ -208,8 +208,10 @@ The math:
   contribute. If the weighted system is degenerate, meaning `sw < 1e-12` (all
   weights vanished) or `sw*swxx - swx^2` near zero (calibrate.rs:146-149), it
   falls back to the global line.
-- **`Loess::predict`** (calibrate.rs:87-102) uses the linear fallback for `x` at or
-  outside the grid ends, and also when the grid has fewer than 2 nodes
+- **`Loess::predict`** (calibrate.rs:87-102) returns NaN for a non-finite `x` (a null
+  library iRT reads as NaN, and interpolating it used to index before the start of the
+  grid; docs/31 F4), extrapolates from the nearer grid end for `x` outside the range,
+  uses the linear fallback when the grid has fewer than 2 nodes
   (calibrate.rs:89-94), and otherwise linearly interpolates between the two
   bracketing grid nodes found by `partition_point` (calibrate.rs:95-101). When the
   two bracketing nodes are within `1e-12` in `x` it returns the lower node's `y`
@@ -514,9 +516,14 @@ cheaper option with equal RT residuals on the one run measured here.
 | `candidate_window` | rt_im_train.rs:65-70 | Builds `(cal, lo, hi)`; returns `(NaN, -inf, +inf)` when calibrated RT or width is absent. |
 | `rt_im_train::run` | rt_im_train.rs:72-354 | The stage: join iRT, select anchors, fit, window, apply, write. |
 | `linear_fit` | calibrate.rs:6-28 | OLS `y = slope*x + intercept` with degenerate-case guards. |
-| `Loess` | calibrate.rs:31-37 | Grid-based local-linear smoother; carries the boundary local slopes for extrapolation and the global line as the degenerate fallback. |
+| `Loess` | calibrate.rs:31-37 | Grid-based local-linear smoother; carries the two boundary extrapolation slopes and the global line as the degenerate fallback. |
 | `Loess::fit` | calibrate.rs:42-83 | Sorts anchors, builds a `grid_n`-point local-linear grid, `k = clamp(ceil(span*n),3,n)`. |
-| `Loess::predict` | calibrate.rs:87-102 | Grid interpolation inside range; outside it, the boundary grid value continued with the boundary local slope, so the map is continuous at both ends. Until docs/29 #10 it switched to the global line there, which on `y = 200 + 10x^2` (span 0.3) jumped from 193.4 to 38.3 at `x = 0` and from 1173.5 to 1018.4 at the top: about 155 s discontinuities for gradient-edge peptides. Measured on HYE B01 with the imported iRT and `native_tda`: 45,946 stripped peptides at 1% before, 45,957 after, decoy fraction unchanged; 1.9% of candidates got a different window, almost all with iRT above the anchor range, which the global line had placed past the end of the run. With the DeepLC 4.1.1 re-predicted precursors (`w_rt` 414 s): 48,533 in both arms, 0.2% of windows moved. |
+| `Loess::predict` | calibrate.rs:87-102 | NaN for a non-finite query (docs/31 F4). Grid interpolation inside range; outside it, the boundary grid value continued with the boundary extrapolation slope, so the map is continuous at both ends. Until docs/29 #10 it switched to the global line there, which on `y = 200 + 10x^2` (span 0.3) jumped from 193.4 to 38.3 at `x = 0` and from 1173.5 to 1018.4 at the top: about 155 s discontinuities for gradient-edge peptides. The extrapolation slope is the
+secant of the fitted curve over its end decile, clamped non-negative and to at most four
+times the global slope: the pointwise local slope it first used comes from the sparsest,
+most one-sided window in the fit, and on noisy anchors it was free to be negative (which
+inverts the iRT-to-RT map) or several times the global slope, multiplying a distance that
+is unbounded by construction (docs/31 F7). Measured on HYE B01 with the imported iRT and `native_tda`: 45,946 stripped peptides at 1% before, 45,957 after, decoy fraction unchanged; 1.9% of candidates got a different window, almost all with iRT above the anchor range, which the global line had placed past the end of the run. With the DeepLC 4.1.1 re-predicted precursors (`w_rt` 414 s): 48,533 in both arms, 0.2% of windows moved. |
 | `local_linear` | calibrate.rs:107-153 | Tricubic-weighted local least squares at one point. |
 | `percentile` | calibrate.rs:156-164 | Nearest-rank percentile: sorts a copy, `rank = round(p.clamp(0,1)*(len-1))`. Not interpolated. Empty input returns 0.0. |
 | `CalibrationMethod` | config.rs:56-61 | Enum `{ Loess, Linear, None }`; default `Loess`. `None` is rejected at load. |
@@ -621,9 +628,9 @@ though the enum variant still exists.
 - **`slope`/`intercept` are emitted in `cal.json` whenever calibration is available,
   including under LOESS** (rt_im_train.rs:128, 286-287, 313-314). Under LOESS they
   are the degenerate fallback (fewer than four anchors, or a local window without
-  spread), not the extrapolation model: since docs/29 #10 the map continues the
-  boundary local fit outside the anchor range. Do not read them as the calibration
-  when `method == "loess"`. They are serialized as `null` only when calibration is
+  spread), not the extrapolation model: since docs/29 #10 the map continues the fitted
+  curve outside the anchor range. Do not read them as the calibration when
+  `method == "loess"`. They are serialized as `null` only when calibration is
   unavailable (`n_train < 2`), since the fit is not computed in that case.
 - **`CalibrationMethod::None` still exists but is rejected** at config load
   (config.rs:1336-1342). The stage would otherwise fall through to the linear path