From 6fe5e323ccadba807f4237de651bbbb0b064faf1 Mon Sep 17 00:00:00 2001 From: Anthony Cesnik Date: Fri, 14 Aug 2026 00:36:35 -0500 Subject: [PATCH 1/5] Refuse to update a spectral library when none was given (#2291) Checking "update spectral library" without supplying one ran the entire search and then dereferenced a null library in UpdateSpectralLibrary. The reporter saw the task appear to hang on "Writing PSM results", with a NullReferenceException only visible in results.txt after force-quitting. The dereference is still there on master, but the symptom has changed rather than gone: UpdateSpectralLibrary is now wrapped in try/catch, so the exception is written to a crash file and the task returns normally. A test asserting a throw against current code gets no exception at all - the run looks successful and the error is buried in the output folder. SearchTask now refuses the combination immediately after loading spectral libraries, where the null is already known: LoadSpectralLibraries returns null when the database list has no library, and the line just below the search loop is already written to handle that. Reporting it there costs a database load rather than a whole search. UpdateSpectralLibrary keeps a null guard that warns and returns, for a caller that builds PostSearchAnalysisParameters itself. It does not throw, because by then the search has finished and its results are already written. Test/SpectralLibraryUpdateTests.cs asserts the refusal, and that the message names what is missing. It passes in about half a second, which is the point - reaching the check no longer requires searching. Verified failing against unfixed code, where no exception is raised at all. Run locally through a net10.0 TaskLayer harness, since the Test project targets net10.0-windows and cannot run on macOS. The redundant positive control I first wrote - update with a library present, assert the guard stays quiet - was dropped: TestLibraryUpdate in MatchIonsOfAllCharges already exercises that path end to end, and duplicating it cost a full search. Co-Authored-By: Claude Opus 5 --- .../SearchTask/PostSearchAnalysisTask.cs | 9 ++++ .../TaskLayer/SearchTask/SearchTask.cs | 9 ++++ .../Test/SpectralLibraryUpdateTests.cs | 49 +++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 MetaMorpheus/Test/SpectralLibraryUpdateTests.cs diff --git a/MetaMorpheus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs b/MetaMorpheus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs index bdd692ac5..91e14a573 100644 --- a/MetaMorpheus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs +++ b/MetaMorpheus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs @@ -817,6 +817,15 @@ private void WriteIndividualPeptideResults() } private void UpdateSpectralLibrary() { + // SearchTask refuses this combination before searching, so reaching here without a library means + // some other caller built the parameters. Skip rather than throw: the search itself is finished + // and its results are already written. + if (Parameters.SpectralLibrary is null) + { + Warn("No spectral library was given, so there was nothing to update."); + return; + } + try { var peptidesForSpectralLibrary = FilteredPsms.Filter(Parameters.AllSpectralMatches, diff --git a/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs b/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs index bdfa5e98c..2eea1e44a 100644 --- a/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs +++ b/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs @@ -174,6 +174,15 @@ protected override MyTaskResults RunSpecific(string OutputFolder, List + /// Issue #2291. Asking to update a spectral library without giving one used to run the whole search + /// and then throw NullReferenceException out of UpdateSpectralLibrary, which surfaced as the task + /// hanging on "Writing PSM results" with the exception only visible in results.txt afterwards. + /// + /// The combination is refused before searching now, so the wasted run is what this pins: reaching + /// the check costs a database load, not a search. + /// + [Test] + public static void UpdatingASpectralLibraryWithoutOneIsRefusedBeforeSearching() + { + string outputFolder = Path.Combine(TestContext.CurrentContext.TestDirectory, + "SpectralLibraryUpdateTests", "NoLibrary"); + Directory.CreateDirectory(outputFolder); + + var task = new SearchTask(); + task.SearchParameters.UpdateSpectralLibrary = true; + + string database = Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "hela_snip_for_unitTest.fasta"); + string spectra = Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "TaGe_SA_A549_3_snip.mzML"); + + var thrown = Assert.Throws(() => task.RunTask( + outputFolder, + new List { new DbForTask(database, false) }, + new List { spectra }, + "TestUpdateWithoutLibrary")); + + Assert.That(thrown.Message, Does.Contain("spectral library")); + Assert.That(thrown.Message, Does.Contain("no spectral library was given").IgnoreCase, + "say what is missing, not just that something went wrong"); + + Directory.Delete(outputFolder, recursive: true); + } + } +} From ab0b03301c84235bad83ced980316a8675a26e01 Mon Sep 17 00:00:00 2001 From: trishorts Date: Fri, 14 Aug 2026 09:41:24 -0500 Subject: [PATCH 2/5] test: cover the spectral library update path directly PR #2721 is at 57% patch coverage. Every uncovered line is the null guard in PostSearchAnalysisTask.UpdateSpectralLibrary, which SearchTask now makes unreachable by refusing the combination before searching -- so the only way to reach it is the case the guard was written for: a caller assembling PostSearchAnalysisParameters itself. These tests do exactly that. Parameters has a public setter and SearchTask.cs:531 already constructs the task that way in production, so no search is needed; the whole fixture runs in under a second where the one existing update test (MatchIonsOfAllCharges.TestLibraryUpdate) takes seven and needs two mzML files. Beyond the guard, they pin the update logic itself, which until now was observable only through that full search: originals survive when there are no PSMs, the better of the stored spectrum and the search result wins, an unseen peptide is appended, and decoys and low-confidence matches stay out of a library the next search will trust. Removing the guard fails UpdateSpectralLibraryWithoutALibraryWarnsAnd- LeavesNothingBehind, so the coverage is real rather than incidental. Also adds a OneTimeTearDown for the "Task Settings" folder RunTask writes beside the output folder, which the existing test leaves behind. Co-Authored-By: Claude Opus 5 (1M context) --- .../Test/SpectralLibraryUpdateTests.cs | 392 +++++++++++++++++- 1 file changed, 391 insertions(+), 1 deletion(-) diff --git a/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs b/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs index a4844c6ab..53f9bd5e7 100644 --- a/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs +++ b/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs @@ -1,9 +1,21 @@ +using Chemistry; using EngineLayer; +using EngineLayer.DatabaseLoading; +using MassSpectrometry; using NUnit.Framework; +using Omics.Digestion; +using Omics.Fragmentation; +using Omics.Modifications; +using Omics.SpectrumMatch; +using Proteomics; +using Proteomics.ProteolyticDigestion; +using Readers.SpectralLibrary; +using System; using System.Collections.Generic; using System.IO; +using System.Linq; +using System.Reflection; using TaskLayer; -using EngineLayer.DatabaseLoading; namespace Test { @@ -45,5 +57,383 @@ public static void UpdatingASpectralLibraryWithoutOneIsRefusedBeforeSearching() Directory.Delete(outputFolder, recursive: true); } + + /// + /// The guard SearchTask now applies is only reachable when a spectral library is actually in the + /// database list, so this pins the precondition it reads: LoadSpectralLibraries returns null when + /// nothing in the list is a library, and a real library when something is. If that ever changed to + /// return an empty SpectralLibrary instead of null, the refusal above would silently stop firing. + /// + [Test] + public static void LoadSpectralLibrariesReturnsNullOnlyWhenNoLibraryIsInTheDatabaseList() + { + var task = new SearchTask(); + var load = typeof(MetaMorpheusTask).GetMethod("LoadSpectralLibraries", + BindingFlags.NonPublic | BindingFlags.Instance); + Assert.That(load, Is.Not.Null, "LoadSpectralLibraries is the seam SearchTask's guard depends on"); + + var fastaOnly = new List { new DbForTask(HelaSnipFasta, false) }; + Assert.That(load.Invoke(task, new object[] { "id", fastaOnly }), Is.Null); + + Assert.That(load.Invoke(task, new object[] { "id", new List() }), Is.Null, + "an empty database list has no library either"); + + var withLibrary = new List + { + new DbForTask(HelaSnipFasta, false), + new DbForTask(SmallLibrary, false) + }; + var loaded = load.Invoke(task, new object[] { "id", withLibrary }); + Assert.That(loaded, Is.Not.Null); + ((SpectralLibrary)loaded).CloseConnections(); + } + + /// + /// The null guard PR #2721 adds to PostSearchAnalysisTask.UpdateSpectralLibrary. SearchTask refuses + /// the combination before searching, so this branch is only reachable by a caller that assembles + /// PostSearchAnalysisParameters itself — which is exactly what this test does. + /// + /// Without the guard the method dereferences the missing library, the surrounding catch turns that + /// into an UpdateSpectralLibrary_crash.txt nobody reads, and the task reports success. Asserting the + /// crash file is absent is what makes this a regression test rather than a restatement of the code. + /// + [Test] + public static void UpdateSpectralLibraryWithoutALibraryWarnsAndLeavesNothingBehind() + { + string outputFolder = MakeOutputFolder("NoLibraryPostSearch"); + + var task = new PostSearchAnalysisTask { CommonParameters = new CommonParameters() }; + task.Parameters = new PostSearchAnalysisParameters + { + SearchParameters = new SearchParameters { UpdateSpectralLibrary = true }, + OutputFolder = outputFolder, + SearchTaskId = "NoLibraryPostSearch", + AllSpectralMatches = new List(), + DatabaseFilenameList = new List(), + SpectralLibrary = null, + SearchTaskResults = NewTaskResults(task) + }; + + var warnings = CaptureWarnings(() => InvokePrivate(task, "UpdateSpectralLibrary")); + + Assert.That(warnings.Any(w => w.Contains("spectral library", StringComparison.OrdinalIgnoreCase)), + Is.True, "the user has to be told why no library came out: " + string.Join(" | ", warnings)); + Assert.That(File.Exists(Path.Combine(outputFolder, "UpdateSpectralLibrary_crash.txt")), Is.False, + "a missing library is a configuration problem, not an engine crash"); + Assert.That(Directory.GetFiles(outputFolder, "updateSpectralLibrary*.msp"), Is.Empty, + "nothing to update means nothing written"); + Assert.That(task.Parameters.SearchTaskResults.NewDatabases, Is.Null, + "no library was produced, so none may be advertised to the next task in the run"); + + Directory.Delete(outputFolder, recursive: true); + } + + /// + /// The other side of the same guard: given a library and no search results, every original spectrum + /// has to survive into the updated library, and the updated library plus the original protein + /// database have to be handed to the next task. This is the path the guard exists to protect, and + /// until now it was covered only by a full two-file search that takes about seven seconds. + /// + [Test] + public static void UpdateSpectralLibraryWithNoSearchResultsCarriesEveryOriginalSpectrumThrough() + { + string outputFolder = MakeOutputFolder("LibraryNoPsms"); + var library = new SpectralLibrary(new List { SmallLibrary }); + + var task = new PostSearchAnalysisTask { CommonParameters = new CommonParameters() }; + task.Parameters = new PostSearchAnalysisParameters + { + SearchParameters = new SearchParameters { UpdateSpectralLibrary = true }, + OutputFolder = outputFolder, + SearchTaskId = "LibraryNoPsms", + AllSpectralMatches = new List(), + DatabaseFilenameList = new List + { + new DbForTask(SmallLibrary, false), + new DbForTask(HelaSnipFasta, false) + }, + SpectralLibrary = library, + SearchTaskResults = NewTaskResults(task) + }; + + var originalSequences = library.GetAllLibrarySpectra().Select(s => s.Sequence).OrderBy(s => s).ToList(); + Assert.That(originalSequences.Count, Is.EqualTo(3), "fixture changed; the assertions below assume three spectra"); + + var warnings = CaptureWarnings(() => InvokePrivate(task, "UpdateSpectralLibrary")); + library.CloseConnections(); + + Assert.That(warnings, Is.Empty, "nothing went wrong: " + string.Join(" | ", warnings)); + + var written = Directory.GetFiles(outputFolder, "updateSpectralLibrary*.msp"); + Assert.That(written.Length, Is.EqualTo(1), "exactly one updated library should be written"); + + var updated = new SpectralLibrary(new List { written[0] }); + var updatedSequences = updated.GetAllLibrarySpectra().Select(s => s.Sequence).OrderBy(s => s).ToList(); + updated.CloseConnections(); + Assert.That(updatedSequences, Is.EqualTo(originalSequences), + "with no PSMs to compare against, an update must not lose or invent a spectrum"); + + var newDatabases = task.Parameters.SearchTaskResults.NewDatabases; + Assert.That(newDatabases.Count, Is.EqualTo(2)); + Assert.That(newDatabases[0].FilePath, Is.EqualTo(written[0])); + Assert.That(newDatabases[0].IsSpectralLibrary, Is.True); + Assert.That(newDatabases[1].FilePath, Is.EqualTo(HelaSnipFasta), + "the protein database has to travel with the library or the next task has nothing to search"); + + Directory.Delete(outputFolder, recursive: true); + } + + /// + /// The selection rule at the heart of the update: for a peptide/charge already in the library, the + /// search result replaces the stored spectrum only when the stored one has fewer matched ions than + /// the PSM's score. Both outcomes are exercised here from the same fixture, so a change to the + /// comparison shows up as a failure rather than as a quietly different library. + /// + [Test] + [TestCase(30d, true, TestName = "UpdateSpectralLibraryReplacesTheStoredSpectrumWhenThePsmScoresHigher")] + [TestCase(2d, false, TestName = "UpdateSpectralLibraryKeepsTheStoredSpectrumWhenThePsmScoresLower")] + public static void UpdateSpectralLibraryPrefersTheBetterOfTheStoredSpectrumAndTheSearchResult( + double psmScore, bool expectReplacement) + { + string outputFolder = MakeOutputFolder("LibraryVsPsm_" + psmScore); + var library = new SpectralLibrary(new List { SmallLibrary }); + + var stored = library.GetAllLibrarySpectra().Single(s => s.Sequence == RepeatedSequence); + Assert.That(stored.ChargeState, Is.EqualTo(2), "fixture changed; the PSM below is built at charge 2"); + + var psm = MakePsm(RepeatedSequence, charge: 2, score: psmScore); + Assert.That(psm.MatchedFragmentIons.Count, Is.Not.EqualTo(stored.MatchedFragmentIons.Count), + "the two spectra must be distinguishable for this test to mean anything"); + + var task = new PostSearchAnalysisTask { CommonParameters = new CommonParameters() }; + task.Parameters = new PostSearchAnalysisParameters + { + SearchParameters = new SearchParameters { UpdateSpectralLibrary = true }, + OutputFolder = outputFolder, + SearchTaskId = "LibraryVsPsm", + AllSpectralMatches = new List { psm }, + DatabaseFilenameList = new List + { + new DbForTask(SmallLibrary, false), + new DbForTask(HelaSnipFasta, false) + }, + SpectralLibrary = library, + SearchTaskResults = NewTaskResults(task) + }; + + InvokePrivate(task, "UpdateSpectralLibrary"); + library.CloseConnections(); + + var written = Directory.GetFiles(outputFolder, "updateSpectralLibrary*.msp").Single(); + var updated = new SpectralLibrary(new List { written }); + var all = updated.GetAllLibrarySpectra().ToList(); + var result = all.Single(s => s.Sequence == RepeatedSequence); + int resultIonCount = result.MatchedFragmentIons.Count; + updated.CloseConnections(); + + Assert.That(all.Count, Is.EqualTo(3), + "the peptide was already in the library, so it must be replaced rather than appended"); + Assert.That(resultIonCount, Is.EqualTo(expectReplacement + ? psm.MatchedFragmentIons.Count + : stored.MatchedFragmentIons.Count), + expectReplacement + ? "a PSM scoring above the stored ion count should win" + : "a PSM scoring below the stored ion count should not displace it"); + + Directory.Delete(outputFolder, recursive: true); + } + + /// + /// A confidently identified peptide that the library has never seen has to be added, not dropped — + /// growing the library is the point of asking for an update at all. + /// + [Test] + public static void UpdateSpectralLibraryAppendsAPeptideTheLibraryHasNotSeen() + { + string outputFolder = MakeOutputFolder("LibraryNewPeptide"); + var library = new SpectralLibrary(new List { SmallLibrary }); + + const string newSequence = "PEPTIDEK"; + Assert.That(library.GetAllLibrarySpectra().Any(s => s.Sequence == newSequence), Is.False, + "fixture changed; this peptide is supposed to be absent from the library"); + + var task = new PostSearchAnalysisTask { CommonParameters = new CommonParameters() }; + task.Parameters = new PostSearchAnalysisParameters + { + SearchParameters = new SearchParameters { UpdateSpectralLibrary = true }, + OutputFolder = outputFolder, + SearchTaskId = "LibraryNewPeptide", + AllSpectralMatches = new List { MakePsm(newSequence, charge: 2, score: 12) }, + DatabaseFilenameList = new List + { + new DbForTask(SmallLibrary, false), + new DbForTask(HelaSnipFasta, false) + }, + SpectralLibrary = library, + SearchTaskResults = NewTaskResults(task) + }; + + InvokePrivate(task, "UpdateSpectralLibrary"); + library.CloseConnections(); + + var written = Directory.GetFiles(outputFolder, "updateSpectralLibrary*.msp").Single(); + var updated = new SpectralLibrary(new List { written }); + var sequences = updated.GetAllLibrarySpectra().Select(s => s.Sequence).ToList(); + updated.CloseConnections(); + + Assert.That(sequences.Count, Is.EqualTo(4), "three originals plus the newly identified peptide"); + Assert.That(sequences, Does.Contain(newSequence)); + + Directory.Delete(outputFolder, recursive: true); + } + + /// + /// A decoy or a low-confidence hit must never reach the library — an updated library is used as + /// ground truth by the next search, so anything that leaks in is amplified. + /// + [Test] + public static void UpdateSpectralLibraryIgnoresDecoyAndLowConfidenceMatches() + { + string outputFolder = MakeOutputFolder("LibraryFiltering"); + var library = new SpectralLibrary(new List { SmallLibrary }); + + var decoy = MakePsm("DECOYPEPTIDEK", charge: 2, score: 40, isDecoy: true); + var lowConfidence = MakePsm("BADQVALUEPEPTIDEK", charge: 2, score: 40, qValue: 0.5); + + var task = new PostSearchAnalysisTask { CommonParameters = new CommonParameters() }; + task.Parameters = new PostSearchAnalysisParameters + { + SearchParameters = new SearchParameters { UpdateSpectralLibrary = true }, + OutputFolder = outputFolder, + SearchTaskId = "LibraryFiltering", + AllSpectralMatches = new List { decoy, lowConfidence }, + DatabaseFilenameList = new List + { + new DbForTask(SmallLibrary, false), + new DbForTask(HelaSnipFasta, false) + }, + SpectralLibrary = library, + SearchTaskResults = NewTaskResults(task) + }; + + InvokePrivate(task, "UpdateSpectralLibrary"); + library.CloseConnections(); + + var written = Directory.GetFiles(outputFolder, "updateSpectralLibrary*.msp").Single(); + var updated = new SpectralLibrary(new List { written }); + var sequences = updated.GetAllLibrarySpectra().Select(s => s.Sequence).ToList(); + updated.CloseConnections(); + + Assert.That(sequences.Count, Is.EqualTo(3), "neither match qualifies, so the library is unchanged"); + Assert.That(sequences, Does.Not.Contain("DECOYPEPTIDEK")); + Assert.That(sequences, Does.Not.Contain("BADQVALUEPEPTIDEK")); + + Directory.Delete(outputFolder, recursive: true); + } + + #region helpers + + /// KAPAGGAADAAAK is one of the three spectra in spectralLibraryForTestingLibraryUpdate.msp. + private const string RepeatedSequence = "KAPAGGAADAAAK"; + + private static string SmallLibrary => Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "SpectralLibrarySearch", "spectralLibraryForTestingLibraryUpdate.msp"); + + private static string HelaSnipFasta => Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "hela_snip_for_unitTest.fasta"); + + private static string MakeOutputFolder(string name) + { + string folder = Path.Combine(TestContext.CurrentContext.TestDirectory, + "SpectralLibraryUpdateTests", name); + if (Directory.Exists(folder)) + { + Directory.Delete(folder, true); + } + Directory.CreateDirectory(folder); + return folder; + } + + /// + /// A scored, FDR-resolved PSM for the given sequence, built from a synthetic scan of that peptide's + /// own fragments so the matched ion list is real rather than empty. Score is set independently of + /// the ion count because the update rule compares the two against each other. + /// + private static SpectralMatch MakePsm(string sequence, int charge, double score, + bool isDecoy = false, double qValue = 0) + { + var commonParameters = new CommonParameters(); + var protein = new Protein(sequence, isDecoy ? "DECOY_acc" : "acc", isDecoy: isDecoy); + var peptide = new PeptideWithSetModifications(protein, commonParameters.DigestionParams, + 1, sequence.Length, CleavageSpecificity.Full, "", 0, new Dictionary(), 0); + + MsDataFile dataFile = new TestDataFile(peptide, "quadratic"); + var scan = new Ms2ScanWithSpecificMass(dataFile.GetOneBasedScan(2), + peptide.MonoisotopicMass.ToMz(charge), charge, null, commonParameters); + + var theoretical = new List(); + peptide.Fragment(DissociationType.HCD, FragmentationTerminus.Both, theoretical); + var matched = MetaMorpheusEngine.MatchFragmentIons(scan, theoretical, commonParameters); + + SpectralMatch psm = new PeptideSpectralMatch(peptide, 0, score, 1, scan, commonParameters, matched); + psm.ResolveAllAmbiguities(); + psm.SetFdrValues(0, 0, qValue, 0, 0, 0, 0, 0); + return psm; + } + + /// MyTaskResults has an internal constructor and TaskLayer does not expose internals to Test. + private static MyTaskResults NewTaskResults(MetaMorpheusTask task) => + (MyTaskResults)Activator.CreateInstance(typeof(MyTaskResults), + BindingFlags.Instance | BindingFlags.NonPublic, null, new object[] { task }, null); + + private static void InvokePrivate(PostSearchAnalysisTask task, string methodName) + { + // The parameterless overload has to be named explicitly: MetaMorpheusTask carries a + // protected UpdateSpectralLibrary(List, string) that would otherwise tie. + var method = typeof(PostSearchAnalysisTask).GetMethod(methodName, + BindingFlags.NonPublic | BindingFlags.Instance, null, Type.EmptyTypes, null); + Assert.That(method, Is.Not.Null, methodName + " was renamed or removed"); + try + { + method.Invoke(task, null); + } + catch (TargetInvocationException e) + { + throw e.InnerException ?? e; + } + } + + private static List CaptureWarnings(Action action) + { + var warnings = new List(); + void Handler(object sender, StringEventArgs e) => warnings.Add(e.S); + + MetaMorpheusTask.WarnHandler += Handler; + try + { + action(); + } + finally + { + MetaMorpheusTask.WarnHandler -= Handler; + } + return warnings; + } + + /// + /// RunTask writes its settings toml to a "Task Settings" folder beside the output folder, so the + /// fixture's own directory outlives the tests that created it. + /// + [OneTimeTearDown] + public static void TearDown() + { + string root = Path.Combine(TestContext.CurrentContext.TestDirectory, "SpectralLibraryUpdateTests"); + if (Directory.Exists(root)) + { + Directory.Delete(root, true); + } + } + + #endregion } } From d4cf63d27e404446aeb6c0d61319c4c4d8078f43 Mon Sep 17 00:00:00 2001 From: Anthony Cesnik Date: Sat, 15 Aug 2026 22:40:21 -0500 Subject: [PATCH 3/5] Check for the spectral library before starting any work DatabaseLoadingEngine.RunAsync is Task.Run with no cancellation token, so throwing after it left the FASTA load, decoy generation and ScrambleHomologousDecoys running on a thread-pool thread after RunTask had unhooked its handlers and rethrown. The check does not need any of that. LoadSpectralLibraries is a filter over dbFilenameList that returns null when nothing in it is a library, so the precondition is available before the load starts. Extracted as AnySpectralLibrary so the guard and the loader cannot drift apart, and hoisted above RunAsync. Reaching the refusal now costs no I/O. The test asserts this by watching StartingSingleEngineHander rather than by looking for output files: every result file is written by PostSearchAnalysisTask, so a check placed anywhere ahead of that leaves the output folder empty whether or not the search ran. With the guard moved back down the assertion fails naming DatabaseLoadingEngine and ClassicSearchEngine. Also pins the remedy half of the message, which nothing covered. Co-Authored-By: Claude Opus 5 (1M context) --- MetaMorpheus/TaskLayer/MetaMorpheusTask.cs | 10 +++++ .../TaskLayer/SearchTask/SearchTask.cs | 31 ++++++++++---- .../Test/SpectralLibraryUpdateTests.cs | 41 +++++++++++++++---- 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs b/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs index 97f039bd6..00de83cf5 100644 --- a/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs +++ b/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs @@ -734,6 +734,16 @@ public MyTaskResults RunTask(string output_folder, List currentProtei #region Database Loading + /// + /// The precondition reads before it opens anything, so a caller + /// can test for a library without paying for the byte-offset index the SpectralLibrary constructor + /// builds. Kept beside it so the two cannot drift apart. + /// + protected static bool AnySpectralLibrary(List dbFilenameList) + { + return dbFilenameList.Any(p => p.IsSpectralLibrary); + } + protected SpectralLibrary LoadSpectralLibraries(string taskId, List dbFilenameList) { Status("Loading spectral libraries...", new List { taskId }); diff --git a/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs b/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs index 2eea1e44a..c2cb92c8a 100644 --- a/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs +++ b/MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs @@ -166,6 +166,28 @@ protected override MyTaskResults RunSpecific(string OutputFolder, List [Test] public static void UpdatingASpectralLibraryWithoutOneIsRefusedBeforeSearching() @@ -45,17 +45,40 @@ public static void UpdatingASpectralLibraryWithoutOneIsRefusedBeforeSearching() string spectra = Path.Combine(TestContext.CurrentContext.TestDirectory, "TestData", "TaGe_SA_A549_3_snip.mzML"); - var thrown = Assert.Throws(() => task.RunTask( - outputFolder, - new List { new DbForTask(database, false) }, - new List { spectra }, - "TestUpdateWithoutLibrary")); + // "Before searching" is half the fix, so observe it rather than trusting the method name. + // Absence of output files does not show it: every result file is written by + // PostSearchAnalysisTask, so a check placed anywhere ahead of that leaves the folder empty even + // though the search ran. Engine start events are the signal that discriminates -- the guard + // precedes the database load, so no engine may start at all. + var enginesStarted = new List(); + void OnEngineStarting(object sender, SingleEngineEventArgs e) + => enginesStarted.Add(e.MyEngine.GetType().Name); + + MetaMorpheusEngine.StartingSingleEngineHander += OnEngineStarting; + MetaMorpheusException thrown; + try + { + thrown = Assert.Throws(() => task.RunTask( + outputFolder, + new List { new DbForTask(database, false) }, + new List { spectra }, + "TestUpdateWithoutLibrary")); + } + finally + { + MetaMorpheusEngine.StartingSingleEngineHander -= OnEngineStarting; + } - Assert.That(thrown.Message, Does.Contain("spectral library")); Assert.That(thrown.Message, Does.Contain("no spectral library was given").IgnoreCase, "say what is missing, not just that something went wrong"); + Assert.That(thrown.Message, Does.Contain("list of databases").IgnoreCase, + "and say what to do about it, so the message cannot be trimmed to the diagnosis alone"); - Directory.Delete(outputFolder, recursive: true); + Assert.That(enginesStarted, Is.Empty, + "the refusal has to come before any work starts, but these ran: " + string.Join(", ", enginesStarted)); + + // No delete here: RunTask writes its toml one level above this folder, and a failing assertion + // would skip it anyway. OneTimeTearDown removes the whole root. } /// From a46a8405308ee790878ddd7685c2a467b8f0e0bb Mon Sep 17 00:00:00 2001 From: Anthony Cesnik Date: Sat, 15 Aug 2026 22:41:04 -0500 Subject: [PATCH 4/5] Build the .msp path with Path.Combine Both spectral library writers concatenated a literal backslash onto the output folder. On Windows that is the separator, so nothing changes there. On macOS and Linux a backslash is an ordinary filename character, so the library was written into the *parent* directory under a single name containing a backslash, and the path handed to NewDatabases pointed at something the next task could not resolve. Found because five of the eight tests in the previous commit fail on macOS and pass on Windows; with this they pass on both. CI runs the test project on windows-latest only, so it could not have surfaced there. Pre-existing and separate from what this PR is about -- drop this commit if you would rather keep the PR to the guard. The same pattern is in ten more places in PostGlycoSearchAnalysisTask, deliberately left alone. Co-Authored-By: Claude Opus 5 (1M context) --- MetaMorpheus/TaskLayer/MetaMorpheusTask.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs b/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs index 00de83cf5..51c7bccb4 100644 --- a/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs +++ b/MetaMorpheus/TaskLayer/MetaMorpheusTask.cs @@ -1120,7 +1120,7 @@ protected static void WritePsmsToTsv(IEnumerable psms, string fil protected static void WriteSpectrumLibrary(List spectrumLibrary, string outputFolder) { var startTimeForAllFilenames = DateTime.Now.ToString("yyyy-MM-dd-HH-mm-ss", CultureInfo.InvariantCulture); - string spectrumFilePath = outputFolder + "\\SpectralLibrary" + "_" + startTimeForAllFilenames + ".msp"; + string spectrumFilePath = Path.Combine(outputFolder, "SpectralLibrary_" + startTimeForAllFilenames + ".msp"); using (StreamWriter output = new StreamWriter(spectrumFilePath)) { foreach (var x in spectrumLibrary) @@ -1134,7 +1134,7 @@ protected static void WriteSpectrumLibrary(List spectrumLibrary protected string UpdateSpectralLibrary(List spectrumLibrary, string outputFolder) { var startTimeForAllFilenames = DateTime.Now.ToString("yyyy-MM-dd-HH-mm-ss", CultureInfo.InvariantCulture); - string spectrumFilePath = outputFolder + "\\updateSpectralLibrary" +"_" + startTimeForAllFilenames + ".msp"; + string spectrumFilePath = Path.Combine(outputFolder, "updateSpectralLibrary_" + startTimeForAllFilenames + ".msp"); using (StreamWriter output = new StreamWriter(spectrumFilePath)) { From c551b1658840e79bda5e1317979a3e8507c4b42f Mon Sep 17 00:00:00 2001 From: Anthony Cesnik Date: Sat, 15 Aug 2026 23:13:24 -0500 Subject: [PATCH 5/5] Refuse the update in the runner, so it reads as a notification Throwing MetaMorpheusException out of a task reaches the GUI through EverythingRunnerExceptionHandler, which does not special-case it: the user gets "Would you like to report this crash?" and a mailto to support for what is a configuration mistake. EverythingRunnerEngine.Run is where the repo already refuses bad database/task combinations, with Warn + FinishedAllTasks + return, including one gate that inspects IsSpectralLibrary. This adds a fourth alongside them. Checked per task rather than up front because CurrentXmlDbFilenameList grows as tasks chain, so a search that writes a library followed by one that updates it stays legal. The task-level throw stays as a backstop for callers that invoke RunTask directly rather than through the runner. It is now unreachable through the runner, which is the same relationship this PR already sets up between SearchTask and the guard in PostSearchAnalysisTask. Presenting MetaMorpheusException as a message rather than a crash would fix this for all 32 throw sites at once, but that is a GUI change and its own PR. Co-Authored-By: Claude Opus 5 (1M context) --- .../TaskLayer/EverythingRunnerEngine.cs | 13 +++++ .../Test/SpectralLibraryUpdateTests.cs | 57 ++++++++++++++++++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/MetaMorpheus/TaskLayer/EverythingRunnerEngine.cs b/MetaMorpheus/TaskLayer/EverythingRunnerEngine.cs index 3d1e1323d..d53aad22c 100644 --- a/MetaMorpheus/TaskLayer/EverythingRunnerEngine.cs +++ b/MetaMorpheus/TaskLayer/EverythingRunnerEngine.cs @@ -87,6 +87,19 @@ public void Run() var ok = TaskList[i]; + // Checked per task rather than up front: an earlier task can add a library to the running + // database list, so a search that writes one followed by a search that updates it is legal. + if (ok.Item2 is SearchTask searchTask + && searchTask.SearchParameters.UpdateSpectralLibrary + && !CurrentXmlDbFilenameList.Any(p => p.IsSpectralLibrary)) + { + Warn("Cannot proceed. Updating a spectral library was requested, but no spectral library " + + "was given. Add one to the list of databases, or select writing a new spectral " + + "library instead of updating one."); + FinishedAllTasks(OutputFolder); + return; + } + // reset product types for custom fragmentation ok.Item2.CommonParameters.SetCustomProductTypes(); diff --git a/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs b/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs index ea569b4bb..19da4f635 100644 --- a/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs +++ b/MetaMorpheus/Test/SpectralLibraryUpdateTests.cs @@ -22,13 +22,66 @@ namespace Test [TestFixture] public static class SpectralLibraryUpdateTests { + /// + /// Issue #2291, as the user meets it. Every entry point runs tasks through EverythingRunnerEngine, + /// which refuses the combination alongside its other database checks: a warning and a clean stop, + /// not an exception, so the GUI shows a notification rather than a crash report. + /// + /// Checked per task, so a search that writes a library followed by one that updates it stays legal. + /// + [Test] + public static void TheRunnerRefusesAnUpdateWithNoLibraryAndRunsNothing() + { + string outputFolder = MakeOutputFolder("RunnerNoLibrary"); + + var task = new SearchTask(); + task.SearchParameters.UpdateSpectralLibrary = true; + + string database = Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "hela_snip_for_unitTest.fasta"); + string spectra = Path.Combine(TestContext.CurrentContext.TestDirectory, + "TestData", "TaGe_SA_A549_3_snip.mzML"); + + var runner = new EverythingRunnerEngine( + new List<(string, MetaMorpheusTask)> { ("Task1Search", task) }, + new List { spectra }, + new List { new DbForTask(database, false) }, + outputFolder); + + var enginesStarted = new List(); + void OnEngineStarting(object sender, SingleEngineEventArgs e) + => enginesStarted.Add(e.MyEngine.GetType().Name); + + MetaMorpheusEngine.StartingSingleEngineHander += OnEngineStarting; + try + { + Assert.That(() => runner.Run(), Throws.Nothing, + "the runner reports this, it does not throw -- a throw reaches the GUI as a crash report"); + } + finally + { + MetaMorpheusEngine.StartingSingleEngineHander -= OnEngineStarting; + } + + // EverythingRunnerEngine has its own WarnHandler, separate from MetaMorpheusTask's, and + // collects into Warnings; MyTaskTest.MissingDbInSpectralLibrarySearch reads it the same way. + List warnings = runner.Warnings; + Assert.That(warnings.Any(w => w.Contains("no spectral library was given", StringComparison.OrdinalIgnoreCase)), + Is.True, "the refusal has to reach the user as a warning: " + string.Join(" | ", warnings)); + Assert.That(warnings.Any(w => w.Contains("list of databases", StringComparison.OrdinalIgnoreCase)), + Is.True, "and has to say what to do about it"); + Assert.That(enginesStarted, Is.Empty, + "nothing may run before the refusal, but these did: " + string.Join(", ", enginesStarted)); + } + /// /// Issue #2291. Asking to update a spectral library without giving one used to run the whole search /// and then throw NullReferenceException out of UpdateSpectralLibrary, which surfaced as the task /// hanging on "Writing PSM results" with the exception only visible in results.txt afterwards. /// - /// The combination is refused before searching now, so the wasted run is what this pins. The check - /// also precedes the background protein load, so reaching it costs no I/O at all. + /// The runner gate above is what users hit. This pins the backstop for a caller invoking RunTask + /// directly: it still refuses, before searching and before the background protein load, so reaching + /// the check costs no I/O at all. /// [Test] public static void UpdatingASpectralLibraryWithoutOneIsRefusedBeforeSearching()