Fix misleading "All convergence criteria satisfied." message on interrupted runs - #2881
Open
bellonarts wants to merge 1 commit into
Open
Fix misleading "All convergence criteria satisfied." message on interrupted runs#2881bellonarts wants to merge 1 commit into
bellonarts wants to merge 1 commit into
Conversation
…rupted runs When a SIGTERM/interrupt signal is received, ConvergenceMonitoring forces convergence = true so the solver stops and saves the solution. The steady exit path in CSinglezoneDriver and CMultizoneDriver then printed 'All convergence criteria satisfied.' directly above a convergence summary table marking every field 'No'. Track the signal-forced stop in a separate flag, propagated across ranks in the same Allreduce, and print an explicit interrupt message instead. Genuine convergence and maximum-iteration exits are unchanged, as is the stop-and-save behavior on interrupt. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
pcarruscag
approved these changes
Sep 5, 2026
6 tasks
bellonarts
added a commit
to babybluechips/SU2
that referenced
this pull request
Sep 6, 2026
Reseal PR su2code#2881 at 2b0b332 after replacing the flagged shell invocation with posix_spawnp/waitpid. All other exact PR and stage identities remain unchanged.
Member
|
Revert the changes after I approved please. |
bellonarts
force-pushed
the
fix_interrupt_exit_banner
branch
from
September 6, 2026 02:14
2b0b332 to
07801f0
Compare
Author
|
You’re right. I accidentally kept working on the branch after you approved it because I was trying to add regression coverage. That test created a CodeFactor warning, so I pushed another change to address it. I have now removed both changes and restored the branch exactly to |
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
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.
Proposed Changes
Interrupt-driven termination is not limited to a user's Ctrl-C. SIGTERM is the normal way batch schedulers terminate a job at its wall-clock limit: SLURM sends SIGTERM when a job reaches its
TimeLimitand onscancel(waitingKillWaitbefore SIGKILL), and PBS/Torque and LSF behave equivalently. Today every SU2 job that exhausts its allocation on a cluster ends with a log stating "All convergence criteria satisfied." directly above a Solver Exit table whose every criterion reads "No". Any parameter sweep or optimization loop that classifies runs by scraping that banner silently admits timed-out, unconverged runs as converged, and a human skimming the log is misled the same way. That population is far larger than the manual-interrupt case.Mechanism: on SIGTERM the signal handler sets
STOP, andCOutput::ConvergenceMonitoringforcesconvergence = trueso the run stops and saves. BothCSinglezoneDriver::MonitorandCMultizoneDriver::Monitorthen read that flag asInnerConvergenceand print the satisfied banner.Fix: a new
convergenceInterruptedmember ofCOutputrecords that the stop came from the interrupt path rather than from the criteria. It is propagated across ranks in the existing convergence Allreduce as a count-2 element-wiseMPI_MAX(SU2's serial MPI stub does not defineMPI_BOR, and the AD layer maps only SUM/MIN/MAX/PROD), exposed throughGetConvergenceInterrupted(), and both drivers printon that path. Stop-and-save behavior, exit code 0, and the genuine-convergence and maximum-iteration messages are unchanged.
Files:
SU2_CFD/include/output/COutput.hpp,SU2_CFD/src/output/COutput.cpp,SU2_CFD/src/drivers/CSinglezoneDriver.cpp,SU2_CFD/src/drivers/CMultizoneDriver.cpp(+29/-8).Verification (QuickStart
inv_NACA0012.cfg, serial release build of develop07aa46b1):SU2_CFDprocess about 8 s into the run: the new interrupt message is printed, the satisfied banner does not appear, restart and solution files are written, exit code 0.81ce6a68f9).-Wall -Wextra: no new warnings.Related Work
The defect is present at v8.5.0 and at the current develop tip. I found no existing upstream issue or PR that addresses it. The change is independent of the other NEMO-related PRs from the same qualification campaign; it touches only the output and driver exit path.
PR Checklist
Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.
pre-commit run --allto format old commits.