Refuse to update a spectral library when none was given

[acesnik]

🤖 Fixes #2291.

Asking MetaMorpheus to update a spectral library without giving it one has always ended badly, because UpdateSpectralLibrary dereferences the library without checking whether it exists. What makes this worth more than a one-line null check is where in the run it happens: the search completes in full, every result is written, and only then does the code reach for a library that was never there.

Thanks @nbollis for the report, and particularly for the detail in it. The stack trace, the version, and the observation that the task stalled specifically during "Writing PSM results" between them identified the failing line and the point in the run it was reached from, which is most of the diagnosis.

The behaviour described in the report no longer reproduces

When #2291 was filed against version 1.0.2, the failure announced itself: the task appeared to hang, and the NullReferenceException surfaced in results.txt only after force-quitting. Somewhere since then UpdateSpectralLibrary acquired a try/catch, which means it no longer hangs, and also that it no longer visibly fails. The exception goes to EngineCrashed, which writes a crash file and lets the task return as though all were well, so a test asserting a throw against current master finds nothing to catch:

Expected: <EngineLayer.MetaMorpheusException>
But was:  null

The live defect is therefore a run that reports success while quietly producing no updated library, with the reason sitting in an UpdateSpectralLibrary_crash.txt that nobody has reason to open. In one respect that is worse than the original hang, which at least demanded attention.

Refusing early, where the answer is already known

The information needed to prevent this has been available all along. LoadSpectralLibraries returns null when the database list contains no library, and the code immediately below the search loop already accounts for that possibility:

if (spectralLibrary != null && SearchParameters.UpdateSpectralLibrary == false)

Nothing, however, consulted it before committing to a search. SearchTask now checks the combination as soon as the libraries are loaded and refuses it there, so the cost of the mistake is a database load rather than an entire search, and the message explains the two ways out: supply a library to update, or ask for a new one to be written instead.

UpdateSpectralLibrary retains a null guard that warns and returns, which matters for any caller assembling PostSearchAnalysisParameters on its own. That guard deliberately does not throw. By the time it runs the search has finished and its results are on disk, and discarding all of that over a request that cannot be honoured would be a poor trade.

Test

A single test in Test/SpectralLibraryUpdateTests.cs asserts the refusal and checks that the message names what is missing rather than merely reporting that something went wrong. It finishes in roughly half a second, which is as much the substance of the fix as the assertion is: reaching the check no longer requires searching anything. It fails against unfixed code, where, as above, nothing is thrown at all.

Test targets net10.0-windows and cannot run on macOS, so I exercised it through a throwaway net10.0 harness over TaskLayer. CI remains the real check.

One thing deliberately left alone

A tempting alternative is to fall back to generating a library from the search results, which SpectralLibraryGeneration does a few lines above and which is plausibly what someone ticking this box actually wants. I have not done that, because it turns a bug fix into a behaviour change: it would emit a library through a path the user did not select, quietly reinterpreting one request as another. Refusing clearly and letting the user choose seemed the more honest option, though I am happy to take the other road if you would prefer it.

[codecov]

Codecov Report

❌ Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.36%. Comparing base (5e5a865) to head (6fe5e32).
⚠️ Report is 1 commits behind head on master.

⚠️ Current head 6fe5e32 differs from pull request most recent head a46a840

Please upload reports for the commit a46a840 to get more accurate results.

Files with missing lines	Patch %	Lines
...eus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs	0.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2721      +/-   ##
==========================================
+ Coverage   93.32%   93.36%   +0.03%     
==========================================
  Files         214      214              
  Lines       21801    21808       +7     
  Branches     4080     4082       +2     
==========================================
+ Hits        20346    20361      +15     
+ Misses        901      888      -13     
- Partials      554      559       +5     

Flag	Coverage Δ
unittests	93.36% <57.14%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs	95.84% <100.00%> (+0.04%)	⬆️
...eus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs	89.79% <0.00%> (-0.30%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[trishorts]

Nice diagnosis, and I agree with both the framing and the decision not to silently fall back to generating a library. Three things below, plus a note on coverage.

I verified each of these against the branch rather than reasoning from the diff — file:line references are to this PR's head.

Patch coverage. Codecov is red at 57.14% (7 patch lines, 4 hit / 2 miss / 1 partial). All three uncovered lines are the new guard in PostSearchAnalysisTask.cs:823-826 — unreachable through SearchTask by construction, which is exactly the point of it. I've written a set of fast direct-construction tests that cover it (and the update logic generally, which today is only exercised by a 7-second full search) and will open them as a PR against this branch so you can take or leave them.

[trishorts]

The check lands nine lines too late to deliver what the PR describes.

var proteinLoadingTask = dbLoader.RunAsync(); already fired at line 171, and RunAsync is Task.Run(Run) (EngineLayer/MetaMorpheusEngine.cs:318) with no cancellation token — there is no CancellationToken overload anywhere in TaskLayer/EngineLayer, and DatabaseLoadingEngine.RunSpecific never checks GlobalVariables.StopLoops. Throwing here abandons that task: it runs the FASTA load, decoy generation and ScrambleHomologousDecoys to completion on a thread-pool thread after RunTask has unhooked FinishedSingleEngineHandler (MetaMorpheusTask.cs:685) and rethrown, and any exception it raises is never observed.

The check doesn't need any of that work. LoadSpectralLibraries is a pure filter over dbFilenameList — dbFilenameList.Where(p => p.IsSpectralLibrary), returning null when the list is empty (MetaMorpheusTask.cs:741-746), no I/O. So the whole test can move above line 170 and the cost of the mistake becomes nothing at all, rather than "a database load".

In practice the leak is small (thread-pool work, and WriteTargetDecoyFasta defaults to false so nothing is written), and SearchTask already abandons this task on other throw paths — :301 for an unknown isobaric mass tag. So this isn't a new class of problem. But it is the one line of the PR whose placement is the whole argument, and hoisting it is free.

[trishorts]

Worth knowing before you settle on throw: in the GUI this surfaces as a crash report prompt, not as a configuration message.

I traced it end to end. RunTask catches, writes a full dump into results.txt — system info, stack trace, TargetSite — and rethrows (MetaMorpheusTask.cs:683-700). EverythingRunnerEngine.Run has no try/catch around RunTask. MainWindow wires the runner with t.ContinueWith(EverythingRunnerExceptionHandler, TaskContinuationOptions.OnlyOnFaulted) (GUI/MainWindow.xaml.cs:1020), and that handler (:431-490) does not special-case MetaMorpheusException:

MessageBox.Show(message + "\n\nWould you like to report this crash?", "Runtime Error", MessageBoxButton.YesNo);

Answering Yes composes a mailto to mm_support@chem.wisc.edu with the stack trace and the toml. So a user who ticked one checkbox too many is told MetaMorpheus crashed and invited to file a bug about it. (Since you exercised this through a net10 harness on macOS, you'd have had no way to see that.)

The house channel for refusing a bad database/task combination is EverythingRunnerEngine.Run, which already does three of these with Warn(...) + FinishedAllTasks(...) + return — including one that inspects IsSpectralLibrary (EverythingRunnerEngine.cs:78-86, "Cannot proceed. No protein database files selected."). That gate runs before RunTask, has access to both the task list and the database list, and already has a test asserting on its warning string (Test/MyTaskTest.cs:839 MissingDbInSpectralLibrarySearch). Putting the refusal there would be earlier still, and would reach the user as a notification rather than a crash dialog.

Counter-argument, which is real: CalibrationTask.GenerateIndexes (CalibrationTask.cs:349-361) throws MetaMorpheusException for exactly this shape of impossible combination and is the newest precedent in the repo, and SearchTask.cs:301 does the same. So the inline throw isn't out of keeping. I'd still rather this one didn't read as a crash — either move it to the runner, or keep it here and teach EverythingRunnerExceptionHandler to present MetaMorpheusException as a message rather than a crash report (that second one is its own PR).

Either way, not something to fix silently — your call, and worth saying out loud in the PR description if you keep the throw.

[trishorts]

The test name and doc comment promise "before searching"; the assertions only prove "at all".

The three assertions are Assert.Throws<MetaMorpheusException> plus two substring checks on the message. Nothing observes that the search loop was skipped. The check could migrate anywhere upstream of PostSearchAnalysisTask — after the per-file search loop, say — and this test stays green, which is the half of the fix you describe as "as much the substance of the fix as the assertion is".

(To be precise about the one place it would catch: moving the throw into UpdateSpectralLibrary() itself would be swallowed by the try/catch at PostSearchAnalysisTask.cs:820/892-895 and Assert.Throws would fail on "But was: null" — the same failure you quote in the description. So the test does pin some earliness. It just doesn't pin "before the search".)

Cheapest fix that pins it, after the expected exception:

Assert.That(Directory.GetFiles(outputFolder, "*.psmtsv", SearchOption.AllDirectories), Is.Empty,
    "the refusal has to come before the search, not after it");
Assert.That(Directory.Exists(Path.Combine(outputFolder, "Individual File Results")), Is.False);

Two smaller things while you're in here:

• The message assertions cover the diagnosis but not the remedy. Does.Contain("no spectral library was given") subsumes Does.Contain("spectral library"), and neither pins the half of the message that tells the user what to do about it — the string could be trimmed to the diagnosis and this stays green. One more substring on the "add one to the list of databases / write a new one instead" clause would cover it.
• RunTask writes its toml to Directory.GetParent(output_folder)/"Task Settings" (MetaMorpheusTask.cs:627), i.e. SpectralLibraryUpdateTests/Task Settings/, one level above the folder you delete — so it survives every run. And the delete is the last statement rather than a finally, so a failing assertion leaves NoLibrary/ behind too. CalibrationTests.cs:273 and BinGenerationTest.cs:80 are the sibling pattern. (The test PR I'm sending you handles this with a [OneTimeTearDown] on the whole SpectralLibraryUpdateTests root.)