Reservoir coupling: Continue the master run when a slave run ends before it

opm/simulators/flow/SimulatorFullyImplicit_impl.hpp

@@ -130,10 +130,16 @@ run(SimulatorTimer& timer)
         this->reservoirCouplingMaster_->sendTerminateAndDisconnect();
     }
     else if (this->reservoirCouplingSlave_ && !this->reservoirCouplingSlave_->terminated()) {
-        // TODO: Implement GECON item 8: stop master process when a slave finishes
+        // We got here by running out of report steps of our own. If the master is still
+        // running, notifyEndOfRunAndDisconnect() tells it so, and the master then continues
+        // with no flow from this slave.
+        //
+        // TODO: Implement GECON item 8, which lets a master deck ask for the opposite: stop
+        //   the master run when one of its slaves finishes, rather than continuing without it.
+        //
         // Only call if not already terminated via maybeReceiveTerminateSignalFromMaster()
         // (which happens when master finishes before slave reaches end of its loop)
-        this->reservoirCouplingSlave_->receiveTerminateAndDisconnect();
+        this->reservoirCouplingSlave_->notifyEndOfRunAndDisconnect();
     }
 #endif
 

opm/simulators/flow/rescoup/ReservoirCoupling.hpp

@@ -160,6 +160,24 @@ enum class MessageTag : int {
     SlaveStatus,
 };
 
+/// @brief Sentinel sent by a slave on the MessageTag::SlaveNextReportDate channel to tell the
+///   master that the slave has reached the end of its own schedule and will not take any
+///   further report step.
+///
+/// @details A genuine next-report-date offset is measured from the slave's own simulation
+/// start and is therefore never negative, so a negative value is unambiguous.  Reusing the
+/// next-report-date channel rather than adding a new message tag means the master learns
+/// about the ended slave at the point where it is already blocked waiting for that slave,
+/// which is what avoids a deadlock.  See ReservoirCouplingSlave::notifyEndOfRunAndDisconnect()
+/// and ReservoirCouplingTimeStepper::receiveNextReportDateFromSlaves().
+inline constexpr double slave_end_of_run_sentinel = -1.0;
+
+/// @brief Whether a next-report-date offset received from a slave is the end-of-run sentinel.
+inline bool isSlaveEndOfRunSentinel(double next_report_time_offset)
+{
+    return next_report_time_offset < 0.0;
+}
+
 /// @brief Phase indices for reservoir coupling, we currently only support black-oil phases
 /// (oil, gas, and water).
 enum class Phase : std::size_t {

opm/simulators/flow/rescoup/ReservoirCouplingMaster.cpp

@@ -210,6 +210,45 @@ isMasterGroup(const std::string &group_name) const
            this->master_group_slave_names_.end();
 }
 
+template <class Scalar>
+void
+ReservoirCouplingMaster<Scalar>::
+markSlaveEndedAndDisconnect(int index)
+{
+    if (this->slave_ended_.size() < this->numSlavesStarted()) {
+        this->slave_ended_.resize(this->numSlavesStarted(), 0);
+    }
+    if (this->slave_ended_[index] != 0) {
+        return;  // Already recorded and disconnected
+    }
+    this->slave_ended_[index] = 1;
+    // Acknowledge the slave's end-of-run notice. The slave is blocked waiting for this
+    // signal before it joins the disconnect below, see
+    // ReservoirCouplingSlave::notifyEndOfRunAndDisconnect().
+    if (this->comm_.rank() == 0) {
+        int terminate_signal = 1;
+        // NOTE: See comment about error handling at the top of this file.
+        MPI_Send(
+            &terminate_signal,
+            /*count=*/1,
+            /*datatype=*/MPI_INT,
+            /*dest_rank=*/0,
+            /*tag=*/static_cast<int>(MessageTag::SlaveProcessTermination),
+            this->master_slave_comm_[index]
+        );
+    }
+    // MPI_Comm_disconnect() is collective over the intercommunicator: every master rank must
+    // call it, and it only completes once the slave has called it too. Afterwards the handle
+    // is MPI_COMM_NULL, so every later loop over slaves must skip this one - which it does,
+    // because slaveIsCoupled() is now false for it.
+    MPI_Comm_disconnect(&this->master_slave_comm_[index]);
+    this->logger_.info(fmt::format(
+        "Slave run {} has ended.\n"
+        "Master run will continue with no flow from this slave.",
+        this->slave_names_[index]
+    ));
+}
+
 template <class Scalar>
 void
 ReservoirCouplingMaster<Scalar>::
@@ -260,6 +299,9 @@ maybeReceiveActivationHandshakeFromSlaves(double current_time)
     if (this->slave_activation_status_.empty()) {
         this->slave_activation_status_.resize(this->numSlavesStarted(), false);
     }
+    if (this->slave_ended_.empty()) {
+        this->slave_ended_.resize(this->numSlavesStarted(), 0);
+    }
 
     if (this->comm_.rank() == 0) {
         auto current_date = this->schedule_.getStartTime() + current_time;
@@ -268,6 +310,12 @@ maybeReceiveActivationHandshakeFromSlaves(double current_time)
             if (this->slaveIsActivated(i)) {
                 continue;
             }
+            // A slave that ended without ever activating has no intercommunicator left to
+            // probe, and its activation date lies in the past, which would trip the assert
+            // below.
+            if (this->slaveHasEnded(i)) {
+                continue;
+            }
             // Check if slave should activate during this timestep
             double slave_activation_date = this->slave_activation_dates_[i];
             // NOTE: The master will adjust its time stepping to ensure that its step will always
@@ -323,11 +371,11 @@ numSlavesStarted() const
 template <class Scalar>
 std::size_t
 ReservoirCouplingMaster<Scalar>::
-numActivatedSlaves() const
+numCoupledSlaves() const
 {
     std::size_t count = 0;
     for (std::size_t i = 0; i < this->slave_activation_status_.size(); ++i) {
-        if (this->slave_activation_status_[i] != 0) {
+        if (this->slaveIsCoupled(static_cast<int>(i))) {
             ++count;
         }
     }
@@ -404,23 +452,27 @@ void
 ReservoirCouplingMaster<Scalar>::
 sendDontTerminateSignalToSlaves()
 {
-    // Send "don't terminate" signal (value=0) to the activated slaves.
+    // Send "don't terminate" signal (value=0) to the coupled slaves.
     // This is called at the start of each iteration in the master's substep loop.
-    // We send only to activated slaves: only an activated slave runs the coupled substep
+    // We send only to coupled slaves: only an activated slave runs the coupled substep
     // loop and is blocked at the terminate-signal receive point. A slave that has not yet
     // activated does not consume this channel, so sending to it would queue stale signals
     // that the slave later reads out of step when it does activate (e.g. a slave whose
     // activation date coincides with the master's last report step when the master schedule
     // is truncated by an END keyword). The final terminate signal is still sent to all
     // started slaves in sendTerminateAndDisconnect().
+    // NOTE: A slave that is about to end is still coupled at this point and does receive
+    // this signal - that is deliberate. It consumes the signal as the first step of
+    // ReservoirCouplingSlave::notifyEndOfRunAndDisconnect(), which is how the two sides stay
+    // in step while the slave reports that its run has ended.
     if (this->comm_.rank() == 0) {
         int terminate_signal = 0;
         // maybeReceiveActivationHandshakeFromSlaves() has resized the activation-status
         // vector before the substep loop that calls us, and the slaves are spawned only
         // once, so the vector covers all started slaves here.
         assert(this->slave_activation_status_.size() == this->numSlavesStarted());
         for (std::size_t i = 0; i < this->numSlavesStarted(); i++) {
-            if (!this->slaveIsActivated(i)) {
+            if (!this->slaveIsCoupled(i)) {
                 continue;
             }
             // NOTE: See comment about error handling at the top of this file.
@@ -506,9 +558,15 @@ sendTerminateAndDisconnect()
     // Step 1: Send terminate signal (value=1) to all spawned slaves (only from rank 0)
     // We send to all spawned slaves, not just activated ones, because even non-activated
     // slaves are running and waiting at the terminate signal receive point.
+    // Slaves that already ended are the exception: they have been acknowledged and
+    // disconnected in markSlaveEndedAndDisconnect(), and their MPI_Comm handle is now
+    // MPI_COMM_NULL.
     if (this->comm_.rank() == 0) {
         int terminate_signal = 1;
         for (std::size_t i = 0; i < this->numSlavesStarted(); i++) {
+            if (this->slaveHasEnded(i)) {
+                continue;
+            }
             this->logger_.info(fmt::format(
                 "Sending terminate signal to slave process: {}", this->slave_names_[i]));
             // NOTE: See comment about error handling at the top of this file.
@@ -524,6 +582,9 @@ sendTerminateAndDisconnect()
     }
     // Step 2: Disconnect intercommunicators (collective operation - all ranks must participate)
     for (std::size_t i = 0; i < this->numSlavesStarted(); i++) {
+        if (this->slaveHasEnded(i)) {
+            continue;  // Already disconnected in markSlaveEndedAndDisconnect()
+        }
         MPI_Comm_disconnect(&this->master_slave_comm_[i]);
         if (this->comm_.rank() == 0) {
             this->logger_.info(fmt::format(

opm/simulators/flow/rescoup/ReservoirCouplingMaster.hpp

@@ -157,7 +157,11 @@ class ReservoirCouplingMaster {
     std::size_t numSlaveGroups(unsigned int index);
     std::size_t numSlaves() const { return this->numSlavesStarted(); }
     std::size_t numSlavesStarted() const;
-    std::size_t numActivatedSlaves() const;
+
+    /// @brief Number of slaves that currently take part in the coupling.
+    /// @details Counts the slaves for which slaveIsCoupled() holds, i.e. activated and not
+    ///   yet ended.
+    std::size_t numCoupledSlaves() const;
     void rebuildSlaveIdxToMasterGroupsVector();
     void receiveNextReportDateFromSlaves();
     void receiveProductionDataFromSlaves();
@@ -216,8 +220,44 @@ class ReservoirCouplingMaster {
     void setSlaveActivationDate(int index, double date) { this->slave_activation_dates_[index] = date; }
     void setSlaveNextReportTimeOffset(int index, double offset);
     void setSlaveStartDate(int index, std::time_t date) { this->slave_start_dates_[index] = date; }
+    /// @brief Whether the slave has completed its activation handshake with the master.
+    /// @details Monotonic: once true it never becomes false again, not even when the slave
+    ///   later reaches the end of its own schedule.  Callers that ask "does this slave take
+    ///   part in the coupling right now?" want slaveIsCoupled() instead.
     bool slaveIsActivated(int index) const { return this->slave_activation_status_[index] != 0; }
 
+    /// @brief Whether the slave has reached the end of its own schedule and left the coupling.
+    /// @details A slave whose deck runs out of report steps before the master's does notifies
+    ///   the master, and the two disconnect their intercommunicator.  From that point on the
+    ///   master must not communicate with the slave, and must treat the slave's master groups
+    ///   as producing and injecting nothing.
+    /// @param index Index of the slave process.
+    /// @return true once the slave has notified the master that its run has ended.
+    bool slaveHasEnded(int index) const
+    {
+        return (static_cast<std::size_t>(index) < this->slave_ended_.size())
+            && (this->slave_ended_[index] != 0);
+    }
+
+    /// @brief Whether the slave currently takes part in the coupling.
+    /// @details True only for a slave that has activated and has not ended.  This is the
+    ///   predicate to use before any master-slave message exchange, and before letting a
+    ///   master group contribute rates, potentials or guide rates: a slave that is not
+    ///   coupled contributes zeros.
+    bool slaveIsCoupled(int index) const
+    {
+        return this->slaveIsActivated(index) && !this->slaveHasEnded(index);
+    }
+
+    /// @brief Record that a slave has ended, and close its intercommunicator.
+    /// @details Must be called on **every** master rank, because MPI_Comm_disconnect() is
+    ///   collective over the whole intercommunicator.  Rank 0 additionally sends the
+    ///   acknowledging terminate signal the slave is waiting for.  After this call
+    ///   slaveIsCoupled() returns false for the slave and no further message may be sent to
+    ///   it or received from it.
+    /// @param index Index of the slave process that has ended.
+    void markSlaveEndedAndDisconnect(int index);
+
     /// @brief Whether the master syncs with slaves at slave report-step boundaries
     ///        (true) or at every master actual time step (false, default CLI flag value).
     /// @details Set from the --rescoup-sync-at-report-steps CLI flag in the
@@ -287,6 +327,12 @@ class ReservoirCouplingMaster {
     // with MPI broadcast().
     std::vector<std::uint8_t> slave_activation_status_;
 
+    // Whether the slave has reached the end of its own schedule and disconnected. Same
+    // std::uint8_t-instead-of-bool reasoning as slave_activation_status_ above: the value is
+    // derived on every rank from the broadcast next-report-date vector, so it must be a plain
+    // array of bytes.
+    std::vector<std::uint8_t> slave_ended_;
+
     // A mapping from master group names to slave names
     std::map<std::string, std::string> master_group_slave_names_;
 

opm/simulators/flow/rescoup/ReservoirCouplingMasterReportStep.cpp

@@ -140,7 +140,7 @@ receiveInjectionDataFromSlaves()
         }
         std::vector<SlaveGroupInjectionData> injection_data(num_slave_groups);
         if (this->comm().rank() == 0) {
-            if (this->slaveIsActivated(i)) {
+            if (this->slaveIsCoupled(i)) {
                 // NOTE: See comment about error handling at the top of this file.
                 auto MPI_INJECTION_DATA_TYPE = Dune::MPITraits<SlaveGroupInjectionData>::getType();
                 MPI_Recv(
@@ -158,8 +158,11 @@ receiveInjectionDataFromSlaves()
                 ));
             }
             else {
+                // Either the slave has not activated yet, or it has reached the end of its own
+                // schedule and left the coupling. In both cases the master continues with no
+                // injection from this slave.
                 this->logger().debug(fmt::format(
-                    "Slave {} has not activated yet, skipping injection data",
+                    "Slave {} is not coupled, skipping injection data",
                     this->slaveName(i)
                 ));
                 injection_data.assign(num_slave_groups, SlaveGroupInjectionData{}); // Set to zero injection data
@@ -193,7 +196,7 @@ receiveProductionDataFromSlaves()
         }
         std::vector<SlaveGroupProductionData> production_data(num_slave_groups);
         if (this->comm().rank() == 0) {
-            if (this->slaveIsActivated(i)) {
+            if (this->slaveIsCoupled(i)) {
                 // NOTE: See comment about error handling at the top of this file.
                 auto MPI_PRODUCTION_DATA_TYPE = Dune::MPITraits<SlaveGroupProductionData>::getType();
                 MPI_Recv(
@@ -211,8 +214,11 @@ receiveProductionDataFromSlaves()
                 ));
             }
             else {
+                // Either the slave has not activated yet, or it has reached the end of its own
+                // schedule and left the coupling. In both cases the master continues with no
+                // production from this slave.
                 this->logger().debug(fmt::format(
-                    "Slave {} has not activated yet, skipping production data",
+                    "Slave {} is not coupled, skipping production data",
                     this->slaveName(i)
                 ));
                 production_data.assign(num_slave_groups, SlaveGroupProductionData{}); // Set to zero production data

opm/simulators/flow/rescoup/ReservoirCouplingMasterReportStep.hpp

@@ -260,10 +260,12 @@ class ReservoirCouplingMasterReportStep {
     /// @return ReservoirCouplingGroupRates struct with per-group rates.
     data::ReservoirCouplingGroupRates collectGroupRatesForSummary() const;
 
-    /// @brief Check if a specific slave process has been activated
+    /// @brief Check if a specific slave process currently takes part in the coupling
+    /// @details True only for a slave that has activated and has not ended. A slave that is
+    ///   not coupled sends no data, and its groups contribute zero rates.
     /// @param index Index of the slave process
-    /// @return true if the slave is activated, false otherwise
-    bool slaveIsActivated(int index) const { return this->master_.slaveIsActivated(index); }
+    /// @return true if the slave is coupled, false otherwise
+    bool slaveIsCoupled(int index) const { return this->master_.slaveIsCoupled(index); }
 
     /// @brief Get the name of a specific slave process
     /// @param index Index of the slave process