diff --git a/opm/simulators/flow/Main.cpp b/opm/simulators/flow/Main.cpp index ca9c6aab42..668803e8b5 100644 --- a/opm/simulators/flow/Main.cpp +++ b/opm/simulators/flow/Main.cpp @@ -285,6 +285,13 @@ void Main::notifyMasterSlaveInitFailed_() if (Parameters::Get()) { MPI_Comm parent_comm = MPI_COMM_NULL; MPI_Comm_get_parent(&parent_comm); + // MPI_Comm_get_parent() returns MPI_COMM_NULL when this process was not + // spawned by a master. --slave is a private flag the master sets when + // spawning, so this normally cannot happen. If it does (e.g. a deck run + // standalone with --slave), there is simply no master to notify. We do + // NOT MPI_Abort() here: this is the cooperative-shutdown path (the parse + // error is already reported and we return EXIT_FAILURE), and with no + // spawned master there is no peer that could hang. if (parent_comm != MPI_COMM_NULL) { int rank; MPI_Comm_rank(MPI_COMM_WORLD, &rank); @@ -298,6 +305,11 @@ void Main::notifyMasterSlaveInitFailed_() } MPI_Comm_disconnect(&parent_comm); } + else if (outputCout_) { + std::cerr << "Warning: --slave was set but this process has no parent " + "communicator; running standalone, no master to notify." + << std::endl; + } } #endif } diff --git a/opm/simulators/flow/rescoup/ReservoirCoupling.hpp b/opm/simulators/flow/rescoup/ReservoirCoupling.hpp index 49175ec14f..21e5fef455 100644 --- a/opm/simulators/flow/rescoup/ReservoirCoupling.hpp +++ b/opm/simulators/flow/rescoup/ReservoirCoupling.hpp @@ -141,6 +141,7 @@ enum class MessageTag : int { MasterGroupNames, MasterGroupNamesSize, MasterGroupNodePressures, + MasterInitStatus, MasterStartOfReportStep, NumMasterGroupNodePressures, NumSlaveGroupConstraints, diff --git a/opm/simulators/flow/rescoup/ReservoirCouplingSlave.cpp b/opm/simulators/flow/rescoup/ReservoirCouplingSlave.cpp index ed396c5482..7a827f4700 100644 --- a/opm/simulators/flow/rescoup/ReservoirCouplingSlave.cpp +++ b/opm/simulators/flow/rescoup/ReservoirCouplingSlave.cpp @@ -370,6 +370,11 @@ sendAndReceiveInitialData() { // Communication order must match master's spawn() method. // First, send status OK to let master know we initialized successfully. this->sendStatusToMasterProcess_(/*ok=*/true); + // Then wait for the master's go-ahead. If another slave failed to + // initialize, the master replies "abort" here so that we disconnect our + // communicator collectively (the master cannot disconnect it on its own + // while we are still mid-handshake). On "proceed" we continue normally. + this->receiveInitStatusFromMasterProcess_(); // We receive master group names before sending activation date so that // we can detect history matching mode (no master groups) and use start_date as activation. this->sendSimulationStartDateToMasterProcess_(); @@ -713,6 +718,37 @@ sendStatusToMasterProcess_(bool ok) { } } +template +void +ReservoirCouplingSlave:: +receiveInitStatusFromMasterProcess_() { + // Receive the master's go-ahead after we reported our OK status: 0 = proceed, + // non-zero = abort (another slave failed). On rank 0 we receive from the + // master and broadcast to all slave ranks so that the collective + // MPI_Comm_disconnect below is matched on every rank. + int proceed = 1; // default to abort if nothing is received + if (this->comm_.rank() == 0) { + MPI_Recv( + &proceed, + /*count=*/1, + /*datatype=*/MPI_INT, + /*source_rank=*/0, + /*tag=*/static_cast(MessageTag::MasterInitStatus), + this->slave_master_comm_, + MPI_STATUS_IGNORE + ); + } + this->comm_.broadcast(&proceed, /*count=*/1, /*emitter_rank=*/0); + if (proceed != 0) { + this->logger_.info( + "Master signaled initialization abort (another slave failed to initialize)"); + MPI_Comm_disconnect(&this->slave_master_comm_); + this->slave_master_comm_ = MPI_COMM_NULL; + RCOUP_LOG_THROW(std::runtime_error, + "Reservoir coupling initialization aborted by master"); + } +} + // Explicit template instantiations template class ReservoirCouplingSlave; #if FLOW_INSTANTIATE_FLOAT diff --git a/opm/simulators/flow/rescoup/ReservoirCouplingSlave.hpp b/opm/simulators/flow/rescoup/ReservoirCouplingSlave.hpp index 38b69778e8..970d9c728a 100644 --- a/opm/simulators/flow/rescoup/ReservoirCouplingSlave.hpp +++ b/opm/simulators/flow/rescoup/ReservoirCouplingSlave.hpp @@ -192,6 +192,13 @@ private: std::pair getGrupSlavActivationDateAndCheckHistoryMatchingMode_() const; bool historyMatchingMode_() const { return this->history_matching_mode_; } std::size_t numMasterGroups_() const { return this->slave_to_master_group_map_.size(); } + //! \brief Receive the master's go-ahead after reporting our OK status. + //! + //! Blocks until the master replies "proceed" (continue the handshake) or + //! "abort" (another slave failed to initialize). On abort, all slave ranks + //! disconnect the master communicator collectively and throw, so the master + //! can tear down cleanly instead of deadlocking on the disconnect. + void receiveInitStatusFromMasterProcess_(); void receiveMasterGroupNamesFromMasterProcess_(); void receiveSlaveNameFromMasterProcess_(); void saveMasterGroupNamesAsMapAndEstablishOrder_(const std::vector& group_names); diff --git a/opm/simulators/flow/rescoup/ReservoirCouplingSpawnSlaves.cpp b/opm/simulators/flow/rescoup/ReservoirCouplingSpawnSlaves.cpp index 89a84c6255..eb94e68040 100644 --- a/opm/simulators/flow/rescoup/ReservoirCouplingSpawnSlaves.cpp +++ b/opm/simulators/flow/rescoup/ReservoirCouplingSpawnSlaves.cpp @@ -272,10 +272,17 @@ receiveSlaveStatus_() { auto num_slaves = this->master_.numSlavesStarted(); if (this->comm_.rank() == 0) { + // Step 1: receive the init status from every slave that finished spawning. + // We must receive from all slaves before disconnecting any communicator: + // a slave that reported OK proceeds with the rest of the handshake and is + // not sitting at MPI_Comm_disconnect, so disconnecting its (collective) + // communicator out from under it would deadlock. See step 2. + std::vector statuses(num_slaves, 0); + bool any_failed = false; + unsigned int first_failed = 0; for (unsigned int i = 0; i < num_slaves; i++) { - int status = 0; MPI_Recv( - &status, + &statuses[i], /*count=*/1, /*datatype=*/MPI_INT, /*source_rank=*/0, @@ -283,27 +290,60 @@ receiveSlaveStatus_() this->master_.getSlaveComm(i), MPI_STATUS_IGNORE ); - if (status != 0) { + if (statuses[i] != 0) { this->logger_.error(fmt::format( - "Slave '{}' failed to initialize (parse error or other failure). " - "The master cannot continue without all slaves.", + "Received FAILED status from slave '{}': it failed to initialize " + "(parse error or other failure). The master cannot continue " + "without all slaves.", this->master_.getSlaveName(i) )); - // Disconnect from all slave communicators to avoid hanging - for (unsigned int j = 0; j < num_slaves; j++) { - auto comm = this->master_.getSlaveComm(j); - if (comm != MPI_COMM_NULL) { - MPI_Comm_disconnect(&comm); - } + if (!any_failed) { + any_failed = true; + first_failed = i; } - RCOUP_LOG_THROW(std::runtime_error, - "Reservoir coupling slave '" + this->master_.getSlaveName(i) + - "' failed to initialize" + } + else { + this->logger_.debug(fmt::format( + "Received OK status from slave '{}'", this->master_.getSlaveName(i) + )); + } + } + // Step 2: tell every slave that reported OK whether to proceed (0) or + // abort (1). A slave that reported FAILED has already disconnected its + // parent communicator (see Main::notifyMasterSlaveInitFailed_()) and is + // not waiting for a reply, so we do not send it one. An OK slave blocks + // on this reply right after sending its status, so on abort it will reach + // MPI_Comm_disconnect and the disconnect below is collectively matched. + int reply = any_failed ? 1 : 0; + for (unsigned int i = 0; i < num_slaves; i++) { + if (statuses[i] == 0) { + MPI_Send( + &reply, + /*count=*/1, + /*datatype=*/MPI_INT, + /*dest_rank=*/0, + /*tag=*/static_cast(MessageTag::MasterInitStatus), + this->master_.getSlaveComm(i) ); } - this->logger_.debug(fmt::format( - "Received OK status from slave '{}'", this->master_.getSlaveName(i) - )); + } + if (any_failed) { + // Disconnect from all slave communicators. Failed slaves are already in MPI_Comm_disconnect; + // OK slaves received the abort reply and disconnect in response. + for (unsigned int i = 0; i < num_slaves; i++) { + // MPI_Comm is a copyable handle, so disconnecting a copy releases the underlying communicator. + auto comm = this->master_.getSlaveComm(i); + if (comm != MPI_COMM_NULL) { + MPI_Comm_disconnect(&comm); + this->logger_.debug(fmt::format( + "Disconnected from slave '{}'", this->master_.getSlaveName(i) + )); + } + } + RCOUP_LOG_THROW(std::runtime_error, + "Reservoir coupling slave '" + this->master_.getSlaveName(first_failed) + + "' failed to initialize" + ); } } } diff --git a/rescoupTests.cmake b/rescoupTests.cmake index cb0744f972..3abc62e893 100644 --- a/rescoupTests.cmake +++ b/rescoupTests.cmake @@ -76,3 +76,27 @@ add_test(NAME rc_slave_parsing_err CONFIGURATIONS Integration ) set_tests_properties(rc_slave_parsing_err PROPERTIES TIMEOUT 30) + +# Multi-slave parse error test: one slave fails to parse while another is +# healthy. Verifies the master aborts the healthy slave cleanly (exit 1) instead +# of deadlocking on a collective MPI_Comm_disconnect. +add_custom_target(test_rc_slave_parsing_err_2slaves + COMMAND ${CMAKE_COMMAND} -E copy_directory + ${CMAKE_CURRENT_SOURCE_DIR}/tests/rescoup/slave_parse_error_2slaves + ${CMAKE_CURRENT_BINARY_DIR}/tests/rescoup/slave_parse_error_2slaves + COMMAND ${CMAKE_COMMAND} -E copy_directory + ${CMAKE_CURRENT_SOURCE_DIR}/tests/include + ${CMAKE_CURRENT_BINARY_DIR}/tests/include + COMMENT "Copying slave_parse_error_2slaves test data to build tree" + DEPENDS ${_rescoup_simulator} +) +add_test(NAME rc_slave_parsing_err_2slaves + COMMAND + ${CMAKE_CURRENT_SOURCE_DIR}/tests/rescoup/slave_parse_error_2slaves/run_ctest.sh + $ + ${_rescoup_mpiexec} + WORKING_DIRECTORY + ${CMAKE_CURRENT_BINARY_DIR}/tests/rescoup/slave_parse_error_2slaves + CONFIGURATIONS Integration +) +set_tests_properties(rc_slave_parsing_err_2slaves PROPERTIES TIMEOUT 40) diff --git a/tests/rescoup/slave_parse_error_2slaves/RC_MASTER.DATA b/tests/rescoup/slave_parse_error_2slaves/RC_MASTER.DATA new file mode 100644 index 0000000000..a8f254bb0a --- /dev/null +++ b/tests/rescoup/slave_parse_error_2slaves/RC_MASTER.DATA @@ -0,0 +1,163 @@ +-- This reservoir simulation deck is made available under the Open Database +-- License: http://opendatacommons.org/licenses/odbl/1.0/. Any rights in +-- individual contents of the database are licensed under the Database Contents +-- License: http://opendatacommons.org/licenses/dbcl/1.0/ + +-- Copyright (C) 2026 Equinor + +-- Reservoir coupling master deck for testing slave parse error behavior with +-- MULTIPLE slaves. The master has a single 1x1x1 grid cell, no wells, and +-- spawns two slaves: +-- RES-1 (slave1): DATA file contains a deliberate parse error -> fails +-- RES-2 (slave2): a valid deck -> initializes and reports OK +-- +-- This exercises the mixed failed+healthy abort path: the master must detect +-- RES-1's failure and cleanly abort the healthy RES-2 (which is mid-handshake) +-- without deadlocking on a collective MPI_Comm_disconnect. The master should +-- exit cleanly with code 1 on all MPI implementations. +-- +-- Usage: mpirun -np 1 flow RC_MASTER.DATA --parsing-strictness=low +-- (--parsing-strictness=low is needed because RC keywords are not yet +-- activated in opm-common, pending PR #5643) + +------------------------------------------------------------------------------------------------ +RUNSPEC +------------------------------------------------------------------------------------------------ + +DIMENS + 1 1 1 / + +OIL +WATER +GAS +DISGAS +VAPOIL + +METRIC + +START + 1 'OCT' 2018 / + +EQLDIMS + 1 1* 25 / + +WELLDIMS +--max.well max.con/well max.grup max.w/grup + 5 10 10 10 / + +TABDIMS +--ntsfun ntpvt max.nssfun max.nppvt max.ntfip max.nrpvt + 1 1 50 60 72 60 / + +UNIFIN +UNIFOUT + +------------------------------------------------------------------------------------------------ +GRID +------------------------------------------------------------------------------------------------ + +NEWTRAN + +GRIDFILE + 0 1 / + +GRIDUNIT +METRES / + +INIT + +SPECGRID + 1 1 1 1 F / + +COORD + 2000.0000 2000.0000 2000.0000 2000.0000 2000.0000 2002.5000 + 2100.0000 2000.0000 2000.0000 2100.0000 2000.0000 2002.5000 + 2000.0000 2100.0000 2000.0000 2000.0000 2100.0000 2002.5000 + 2100.0000 2100.0000 2000.0000 2100.0000 2100.0000 2002.5000 +/ + +ZCORN + 2000.0000 2000.0000 2000.0000 2000.0000 2002.5000 2002.5000 + 2002.5000 2002.5000 / + +PORO + 0.25 / + +PERMX + 2100 / + +COPY + PERMX PERMY / + PERMX PERMZ / +/ + +MULTIPLY + 'PERMZ' 0.1 / +/ + +------------------------------------------------------------------------------------------------ +PROPS +------------------------------------------------------------------------------------------------ + +NOECHO + +INCLUDE + '../../include/PVT-WET-GAS.INC' / + +INCLUDE + '../../include/scal_mod2.inc' / + +FILLEPS + +------------------------------------------------------------------------------------------------ +REGIONS +------------------------------------------------------------------------------------------------ + +------------------------------------------------------------------------------------------------ +SOLUTION +------------------------------------------------------------------------------------------------ + +RPTRST + BASIC=2 / + +EQUIL +-- Datum P woc Pc goc Pc Rsvd Rvvd N +2300.00 300.0 2300.0 0.0 1800.00 0.0 1 1 0 / + +PBVD + 2000.0 100.0 + 2300.0 100.0 / + +PDVD + 2000.0 100.0 + 2300.0 100.0 / +/ + +------------------------------------------------------------------------------------------------ +SUMMARY +------------------------------------------------------------------------------------------------ + +------------------------------------------------------------------------------------------------ +SCHEDULE +------------------------------------------------------------------------------------------------ + +TUNING + 1.0 5.0 / + / + / + +GRUPTREE + 'MOD2' 'FIELD' / + 'E1_M' 'MOD2' / +/ + +SLAVES + 'RES-1' 'RC_SLAVE' '*' 'slave1' 1 / + 'RES-2' 'RC_SLAVE' '*' 'slave2' 1 / +/ + +DATES + 1 NOV 2018 / +/ + +END diff --git a/tests/rescoup/slave_parse_error_2slaves/run_ctest.sh b/tests/rescoup/slave_parse_error_2slaves/run_ctest.sh new file mode 100755 index 0000000000..9c8f70e82b --- /dev/null +++ b/tests/rescoup/slave_parse_error_2slaves/run_ctest.sh @@ -0,0 +1,69 @@ +#!/bin/bash +# Copyright 2026 Equinor +# +# This file is part of the Open Porous Media project (OPM). +# +# OPM is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# OPM is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with OPM. If not, see . + +# CTest wrapper for the multi-slave reservoir coupling parse error test. +# +# The master spawns two slaves: RES-1 (slave1) has a deliberate parse error and +# fails; RES-2 (slave2) is valid and reports OK, then waits for the master's +# go-ahead. The master must detect RES-1's failure and cleanly abort the healthy +# RES-2 (which is mid-handshake) without deadlocking on a collective +# MPI_Comm_disconnect, then exit with code 1. +# +# Expected outcome (with the master-hang fix): exit code 1 on all MPI +# implementations. +# - exit 1: master detected the slave failure and aborted cleanly -> PASS +# - exit 0: master completed despite a slave parse error -> FAIL +# - exit 124: master hung (e.g. the old multi-slave disconnect bug) -> FAIL +# - other: unexpected -> FAIL +# +# Usage: run_ctest.sh + +set -u + +FLOW_BINARY="${1:?Usage: $0 }" +MPI_LAUNCHER="${2:?Usage: $0 }" + +echo "Flow binary: $FLOW_BINARY" +echo "MPI launcher: $MPI_LAUNCHER" +echo "Working dir: $(pwd)" +echo "" + +# Run with a timeout as a safety net. With the fix the master exits quickly with +# code 1; a timeout (124) would indicate the master hung (regression). +timeout 20 \ + "$MPI_LAUNCHER" -np 1 "$FLOW_BINARY" \ + RC_MASTER.DATA \ + --parsing-strictness=low \ + --output-dir=. \ + 2>&1 + +EXIT_CODE=$? + +if [ $EXIT_CODE -eq 1 ]; then + echo "PASS: Master detected slave failure and aborted cleanly (exit code 1)" + exit 0 +elif [ $EXIT_CODE -eq 0 ]; then + echo "FAIL: Master completed successfully — slave parse error was ignored" + exit 1 +elif [ $EXIT_CODE -eq 124 ]; then + echo "FAIL: Master hung until timeout (possible multi-slave disconnect deadlock)" + exit 1 +else + echo "FAIL: Master exited with unexpected code $EXIT_CODE" + exit 1 +fi diff --git a/tests/rescoup/slave_parse_error_2slaves/slave1/RC_SLAVE.DATA b/tests/rescoup/slave_parse_error_2slaves/slave1/RC_SLAVE.DATA new file mode 100644 index 0000000000..c2f49efa0c --- /dev/null +++ b/tests/rescoup/slave_parse_error_2slaves/slave1/RC_SLAVE.DATA @@ -0,0 +1,44 @@ +-- This reservoir simulation deck is made available under the Open Database +-- License: http://opendatacommons.org/licenses/odbl/1.0/. Any rights in +-- individual contents of the database are licensed under the Database Contents +-- License: http://opendatacommons.org/licenses/dbcl/1.0/ + +-- Copyright (C) 2026 Equinor + +-- Deliberately broken slave deck (RES-1) for the multi-slave reservoir coupling +-- parse error test. The INCLUDE in the GRID section references a non-existent +-- file, which triggers a parse error regardless of --parsing-strictness setting. + +------------------------------------------------------------------------------------------------ +RUNSPEC +------------------------------------------------------------------------------------------------ + +DIMENS + 2 2 2 / + +OIL +WATER + +METRIC + +START + 1 'JAN' 2019 / + +EQLDIMS + 1 / + +TABDIMS + 1 1 / + +UNIFIN +UNIFOUT + +------------------------------------------------------------------------------------------------ +GRID +------------------------------------------------------------------------------------------------ + +-- Intentional error: this file does not exist +INCLUDE + 'nonexistent_grid_file.inc' / + +END diff --git a/tests/rescoup/slave_parse_error_2slaves/slave2/RC_SLAVE.DATA b/tests/rescoup/slave_parse_error_2slaves/slave2/RC_SLAVE.DATA new file mode 100644 index 0000000000..1776b81e1e --- /dev/null +++ b/tests/rescoup/slave_parse_error_2slaves/slave2/RC_SLAVE.DATA @@ -0,0 +1,144 @@ +-- This reservoir simulation deck is made available under the Open Database +-- License: http://opendatacommons.org/licenses/odbl/1.0/. Any rights in +-- individual contents of the database are licensed under the Database Contents +-- License: http://opendatacommons.org/licenses/dbcl/1.0/ + +-- Copyright (C) 2026 Equinor + +-- Valid slave deck (RES-2) for the multi-slave reservoir coupling parse error +-- test. This slave initializes successfully and reports OK status, then waits +-- for the master's go-ahead. When the other slave (RES-1) fails to parse, the +-- master signals abort here and this slave disconnects cleanly. A minimal +-- 1x1x1 single-cell deck (history matching mode, no master groups). + +------------------------------------------------------------------------------------------------ +RUNSPEC +------------------------------------------------------------------------------------------------ + +DIMENS + 1 1 1 / + +OIL +WATER +GAS +DISGAS +VAPOIL + +METRIC + +START + 1 'OCT' 2018 / + +EQLDIMS + 1 1* 25 / + +WELLDIMS +--max.well max.con/well max.grup max.w/grup + 5 10 10 10 / + +TABDIMS +--ntsfun ntpvt max.nssfun max.nppvt max.ntfip max.nrpvt + 1 1 50 60 72 60 / + +UNIFIN +UNIFOUT + +------------------------------------------------------------------------------------------------ +GRID +------------------------------------------------------------------------------------------------ + +NEWTRAN + +GRIDFILE + 0 1 / + +GRIDUNIT +METRES / + +INIT + +SPECGRID + 1 1 1 1 F / + +COORD + 2000.0000 2000.0000 2000.0000 2000.0000 2000.0000 2002.5000 + 2100.0000 2000.0000 2000.0000 2100.0000 2000.0000 2002.5000 + 2000.0000 2100.0000 2000.0000 2000.0000 2100.0000 2002.5000 + 2100.0000 2100.0000 2000.0000 2100.0000 2100.0000 2002.5000 +/ + +ZCORN + 2000.0000 2000.0000 2000.0000 2000.0000 2002.5000 2002.5000 + 2002.5000 2002.5000 / + +PORO + 0.25 / + +PERMX + 2100 / + +COPY + PERMX PERMY / + PERMX PERMZ / +/ + +MULTIPLY + 'PERMZ' 0.1 / +/ + +------------------------------------------------------------------------------------------------ +PROPS +------------------------------------------------------------------------------------------------ + +NOECHO + +INCLUDE + '../../../include/PVT-WET-GAS.INC' / + +INCLUDE + '../../../include/scal_mod2.inc' / + +FILLEPS + +------------------------------------------------------------------------------------------------ +REGIONS +------------------------------------------------------------------------------------------------ + +------------------------------------------------------------------------------------------------ +SOLUTION +------------------------------------------------------------------------------------------------ + +RPTRST + BASIC=2 / + +EQUIL +-- Datum P woc Pc goc Pc Rsvd Rvvd N +2300.00 300.0 2300.0 0.0 1800.00 0.0 1 1 0 / + +PBVD + 2000.0 100.0 + 2300.0 100.0 / + +PDVD + 2000.0 100.0 + 2300.0 100.0 / +/ + +------------------------------------------------------------------------------------------------ +SUMMARY +------------------------------------------------------------------------------------------------ + +------------------------------------------------------------------------------------------------ +SCHEDULE +------------------------------------------------------------------------------------------------ + +TUNING + 1.0 5.0 / + / + / + +DATES + 1 NOV 2018 / +/ + +END