fix: Address review feedback in graceful peer disconnect

Three concurrency / correctness fixes:

- Arm the kShutdownTimerInterval (5s) safety timer in PeerImp::shutdown()
  rather than in tryAsyncShutdown(). Otherwise, when shutdown is requested
  while I/O is pending, the existing 60s peer-health timer remains armed
  and onTimer() treats its expiry as a shutdown timeout, delaying forced
  close far beyond the intended 5s budget.

- Track the inbound HTTP-handshake-response write in doAccept() via
  writePending_, matching the pattern used for send() and onWriteMessage().
  Without this, tryAsyncShutdown() could launch async_shutdown() while the
  response write was still in flight on the SSL stream.

- Replace the string-match for "application data after close notify" in
  onShutdown() (both ConnectAttempt and PeerImp) with a category +
  ERR_GET_REASON check against SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY.
  The previous text comparison was fragile across OpenSSL versions.

Plus cleanup: Doxygen fixes (20s -> 25s timeout, broken @brief markup),
"handshare" -> "handshake" typo, removed extraneous semicolon after
stepToString(), made stepToString return string_view, and simplified the
stop() log-severity comment so it matches the actual code.

.github/workflows/reusable-package.yml

@@ -58,7 +58,6 @@ jobs:
 
   package:
     needs: [generate-matrix, generate-version]
-    if: ${{ github.event.repository.visibility == 'public' }}
     strategy:
       fail-fast: false
       matrix: ${{ fromJson(needs.generate-matrix.outputs.matrix) }}
@@ -89,7 +88,8 @@ jobs:
         run: ./package/build_pkg.sh
 
       - name: Upload package artifact
-        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0
+        if: ${{ github.event.repository.visibility == 'public' }}
         with:
           name: ${{ matrix.artifact_name }}-pkg-${{ needs.generate-version.outputs.version }}
           path: |

cfg/xrpld-example.cfg

@@ -1466,7 +1466,10 @@ admin = 127.0.0.1
 protocol = http
 
 [port_peer]
-port = 2459
+# Many servers still use the legacy port of 51235, so for backward-compatibility
+# we maintain that port number here. However, for new servers we recommend
+# changing this to the default port of 2459.
+port = 51235
 ip = 0.0.0.0
 # alternatively, to accept connections on IPv4 + IPv6, use:
 #ip = ::

src/libxrpl/ledger/helpers/LendingHelpers.cpp

@@ -1126,34 +1126,18 @@ computePaymentComponents(
             "xrpl::detail::computePaymentComponents",
             "excess non-negative");
     };
-    auto giveTo = [](Number& component, Number& shortage, Number const& maximum) {
-        if (shortage > beast::kZero)
-        {
-            // Put as much of the shortage as we can into the provided part
-            // and the total
-            auto part = std::min(maximum - component, shortage);
-            component += part;
-            shortage -= part;
-        }
-        // If the shortage goes negative, we put too much, which should be
-        // impossible
-        XRPL_ASSERT_PARTS(
-            shortage >= beast::kZero,
-            "ripple::detail::computePaymentComponents",
-            "excess non-negative");
-    };
+    // Helper to reduce deltas when they collectively exceed a limit.
+    // Order matters: we prefer to reduce interest first (most flexible),
+    // then management fee, then principal (least flexible).
     auto addressExcess = [&takeFrom](LoanStateDeltas& deltas, Number& excess) {
         // This order is based on where errors are the least problematic
         takeFrom(deltas.interest, excess);
         takeFrom(deltas.managementFee, excess);
         takeFrom(deltas.principal, excess);
     };
-    auto addressShortage =
-        [&giveTo](LoanStateDeltas& deltas, Number& shortage, LoanState const& current) {
-            giveTo(deltas.interest, shortage, current.interestDue);
-            giveTo(deltas.managementFee, shortage, current.managementFeeDue);
-            giveTo(deltas.principal, shortage, current.principalOutstanding);
-        };
+
+    // Check if deltas exceed the total outstanding value. This should never
+    // happen due to earlier caps, but handle it defensively.
     Number totalOverpayment = deltas.total() - currentLedgerState.valueOutstanding;
 
     if (totalOverpayment > beast::kZero)
@@ -1181,33 +1165,14 @@ computePaymentComponents(
         addressExcess(deltas, excess);
         shortage = -excess;
     }
-    else if (shortage > beast::kZero && totalOverpayment < beast::kZero)
-    {
-        // If there's a shortage, and there's room in the loan itself, we can
-        // top up the parts to make the payment correct.
-        shortage = std::min(-totalOverpayment, shortage);
-        addressShortage(deltas, shortage, currentLedgerState);
-    }
 
-    // The shortage should never be negative, which indicates that the parts are
-    // trying to take more than the whole payment. The shortage should not be
-    // positive, either, which indicates that we're not going to take the whole
-    // payment amount. Only the last payment should be allowed to have a
-    // shortage, and that's handled in a special case above.
+    // At this point, shortage >= 0 means we're paying less than the full
+    // periodic payment (due to rounding or component caps).
+    // shortage < 0 would mean we're trying to pay more than allowed (bug).
     XRPL_ASSERT_PARTS(
-        shortage == beast::kZero,
-        "ripple::detail::computePaymentComponents",
+        shortage >= beast::kZero,
+        "xrpl::detail::computePaymentComponents",
         "no shortage or excess");
-#if LOANCOMPLETE
-    /*
-    // This used to be part of the above assert. It will eventually be removed
-    // if proved accurate
-    ||
-        (shortage > beast::kZero &&
-         ((asset.integral() && shortage < 3) ||
-          (scale - shortage.exponent() > 14)))
-          */
-#endif
 
     // Final validation that all components are valid
     XRPL_ASSERT_PARTS(

src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp

@@ -184,7 +184,7 @@ EscrowCancel::doApply()
                     return escrowUnlockApplyHelper<T>(
                         ctx_.view(),
                         kParityRate,
-                        ctx_.view().rules().enabled(fixCleanup3_2_0) ? sle : slep,
+                        slep,
                         preFeeBalance_,
                         amount,
                         issuer,

src/libxrpl/tx/transactors/vault/VaultDeposit.cpp

@@ -7,7 +7,6 @@
 #include <xrpl/ledger/helpers/MPTokenHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
 #include <xrpl/ledger/helpers/VaultHelpers.h>
-#include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/LedgerFormats.h>
@@ -253,26 +252,19 @@ VaultDeposit::doApply()
         !isTesSuccess(ter))
         return ter;
 
-    // This check is wrong. Disable it with fixCleanup3_2_0.
-    // For XRP and MPT the predicate is structurally unsatisfiable: xrpLiquid clamps at zero, and
-    // MPT balances are unsigned. For IOUs it only fires when the deposit drove the depositor's
-    // trust line into debt the exact case preclaim authorizes via SpendableHandling::FullBalance.
-    // The check thus converts a preclaim- authorized deposit into tefINTERNAL after the asset
-    // transfer.
-    if (!view().rules().enabled(fixCleanup3_2_0))
+    // Sanity check
+    if (accountHolds(
+            view(),
+            accountID_,
+            assetsDeposited.asset(),
+            FreezeHandling::IgnoreFreeze,
+            AuthHandling::IgnoreAuth,
+            j_) < beast::kZero)
     {
-        // Sanity check
-        if (accountHolds(
-                view(),
-                accountID_,
-                assetsDeposited.asset(),
-                FreezeHandling::IgnoreFreeze,
-                AuthHandling::IgnoreAuth,
-                j_) < beast::kZero)
-        {
-            JLOG(j_.error()) << "VaultDeposit: negative balance of account assets.";
-            return tefINTERNAL;
-        }
+        // LCOV_EXCL_START
+        JLOG(j_.error()) << "VaultDeposit: negative balance of account assets.";
+        return tefINTERNAL;
+        // LCOV_EXCL_STOP
     }
 
     // Transfer shares from vault to depositor.

src/test/app/EscrowToken_test.cpp

@@ -886,70 +886,6 @@ struct EscrowToken_test : public beast::unit_test::Suite
         }
     }
 
-    void
-    testIOUCancelDoApply(FeatureBitset features)
-    {
-        testcase("IOU Cancel DoApply");
-        using namespace jtx;
-        using namespace std::literals;
-
-        {
-            Env env{*this, features};
-            auto const baseFee = env.current()->fees().base;
-            auto const alice = Account("alice");
-            auto const bob = Account("bob");
-            auto const gw = Account("gw");
-            auto const usd = gw["USD"];
-
-            env.fund(XRP(10'000), alice, bob, gw);
-            env.close();
-
-            env(fset(gw, asfAllowTrustLineLocking));
-            env.close();
-
-            env.trust(usd(100'000), alice);
-            env.trust(usd(100'000), bob);
-            env.close();
-
-            env(pay(gw, alice, usd(10'000)));
-            env.close();
-
-            auto const seq = env.seq(alice);
-            env(escrow::create(alice, bob, usd(1'000)),
-                escrow::kFinishTime(env.now() + 1s),
-                escrow::kCancelTime(env.now() + 2s),
-                Fee(baseFee));
-            env.close();
-
-            BEAST_EXPECT(env.balance(alice, usd) == usd(9'000));
-
-            env(pay(alice, gw, usd(9'000)));
-            env.close();
-
-            env(trust(alice, usd(0)));
-            env.close();
-
-            auto const trustLineKey = keylet::line(alice.id(), gw.id(), usd.currency);
-            BEAST_EXPECT(!env.current()->exists(trustLineKey));
-
-            env.close();
-            env.close();
-
-            auto const expectedResult = env.current()->rules().enabled(fixCleanup3_2_0)
-                ? Ter(tesSUCCESS)
-                : Ter(tefEXCEPTION);
-            env(escrow::cancel(alice, alice, seq), Fee(baseFee), expectedResult);
-            env.close();
-
-            if (env.current()->rules().enabled(fixCleanup3_2_0))
-            {
-                BEAST_EXPECT(!env.le(keylet::escrow(alice.id(), seq)));
-                BEAST_EXPECT(env.current()->exists(trustLineKey));
-                BEAST_EXPECT(env.balance(alice, usd) == usd(1'000));
-            }
-        }
-    }
-
     void
     testIOUBalances(FeatureBitset features)
     {
@@ -3951,7 +3887,6 @@ struct EscrowToken_test : public beast::unit_test::Suite
         testIOUFinishPreclaim(features);
         testIOUFinishDoApply(features);
         testIOUCancelPreclaim(features);
-        testIOUCancelDoApply(features);
         testIOUBalances(features);
         testIOUMetaAndOwnership(features);
         testIOURippleState(features);
@@ -3993,7 +3928,6 @@ public:
              {all - featureSingleAssetVault - featureLendingProtocol, all})
         {
             testIOUWithFeats(feats);
-            testIOUWithFeats(feats - fixCleanup3_2_0);
             testMPTWithFeats(feats);
             testMPTWithFeats(feats - fixTokenEscrowV1);
         }

src/test/app/Loan_test.cpp

@@ -2602,10 +2602,8 @@ protected:
                         broker.params.managementFeeRate);
 
                     BEAST_EXPECTS(
-                        paymentComponents.trackedValueDelta == roundedPeriodicPayment ||
-                            (paymentComponents.specialCase ==
-                                 xrpl::detail::PaymentSpecialCase::Final &&
-                             paymentComponents.trackedValueDelta < roundedPeriodicPayment),
+                        paymentComponents.specialCase == xrpl::detail::PaymentSpecialCase::Final ||
+                            paymentComponents.trackedValueDelta <= roundedPeriodicPayment,
                         "Delta: " + to_string(paymentComponents.trackedValueDelta) +
                             ", periodic payment: " + to_string(roundedPeriodicPayment));
 

src/test/app/Vault_test.cpp

@@ -6140,90 +6140,10 @@ class Vault_test : public beast::unit_test::Suite
         runTest(amendments);
     }
 
-    // VaultDeposit::preclaim uses accountHolds(..., SpendableHandling::
-    // shFULL_BALANCE), which for an IOU asset adds the counterparty's
-    // LowLimit/HighLimit to the depositor's raw balance (TokenHelpers.cpp:
-    // getTrustLineBalance with includeOppositeLimit=true). When the
-    // depositor's raw balance < deposit amount but raw + opposite limit >=
-    // amount, preclaim is satisfied. doApply then calls
-    // directSendNoFeeIOU, which unconditionally subtracts saAmount from
-    // saBalance — driving the trust line negative — and returns tesSUCCESS.
-    // The post-send sanity check uses the default shSIMPLE_BALANCE (no
-    // opposite-limit add), sees a negative balance, and returns tefINTERNAL.
-    void
-    testVaultDepositNegativeBalanceFromOppositeLimit()
-    {
-        auto runTest = [&](FeatureBitset f, TER expected) {
-            using namespace test::jtx;
-            using namespace std::literals;
-
-            Env env{*this, f};
-            Account const gw{"gateway"};
-            Account const owner{"owner"};
-            Account const depositor{"depositor"};
-
-            env.fund(XRP(10000), gw, owner, depositor);
-            env.close();
-
-            // Gateway with DefaultRipple so vault creation on its IOU works.
-            env(fset(gw, asfDefaultRipple));
-            env.close();
-
-            // Depositor opens a trust line to gateway and receives a small
-            // balance.
-            PrettyAsset const usd = gw["USD"];
-            env.trust(usd(1000), depositor);
-            env(pay(gw, depositor, usd(100)));  // raw trust-line balance: 100
-            env.close();
-
-            // Key precondition: gateway sets a non-zero limit on the same
-            // RippleState — the "opposite field" from depositor's perspective.
-            // This is what inflates shFULL_BALANCE in preclaim above the raw
-            // balance.
-            env(trust(gw, depositor["USD"](1000)));
-            env.close();
-
-            // Create the IOU vault.
-            Vault const vault{env};
-            auto [vaultTx, keylet] = vault.create({.owner = owner, .asset = usd});
-            env(vaultTx);
-            env.close();
-
-            // Submit a deposit of 500 USD:
-            //   - raw balance:                100 USD
-            //   - opposite limit (gw's side): 1000 USD
-            //   - preclaim sees 100 + 1000 = 1100, passes (>= 500)
-            //   - doApply transfers 500, depositor's trust-line balance
-            //     becomes -400
-            //   - sanity check at VaultDeposit.cpp:256 fires
-            //   - tx returns tefINTERNAL (BUG — should be tesSUCCESS.
-            auto depositTx =
-                vault.deposit({.depositor = depositor, .id = keylet.key, .amount = usd(500)});
-            env(depositTx, Ter(expected));
-            env.close();
-        };
-
-        {
-            testcase(
-                "IOU vault deposit exceeding depositor's balance but "
-                "within counterparty's trust limit, pre-fixCleanup3_2_0 "
-                "(tefINTERNAL)");
-            runTest(test::jtx::testableAmendments() - fixCleanup3_2_0, tefINTERNAL);
-        }
-        {
-            testcase(
-                "IOU vault deposit exceeding depositor's balance but "
-                "within counterparty's trust limit, post-fixCleanup3_2_0 "
-                "(tesSUCCESS)");
-            runTest(test::jtx::testableAmendments(), tesSUCCESS);
-        }
-    }
-
 public:
     void
     run() override
     {
-        testVaultDepositNegativeBalanceFromOppositeLimit();
         testSequences();
         testPreflight();
         testCreateFailXRP();

src/xrpld/overlay/detail/ConnectAttempt.cpp

@@ -25,6 +25,7 @@
 #include <boost/asio/io_context.hpp>
 #include <boost/asio/ip/tcp.hpp>
 #include <boost/asio/post.hpp>
+#include <boost/asio/ssl/error.hpp>
 #include <boost/asio/ssl/stream_base.hpp>
 #include <boost/asio/ssl/verify_mode.hpp>
 #include <boost/asio/strand.hpp>
@@ -34,6 +35,9 @@
 #include <boost/beast/http/status.hpp>
 #include <boost/system/system_error.hpp>
 
+#include <openssl/err.h>
+#include <openssl/sslerr.h>
+
 #include <chrono>
 #include <cstdint>
 #include <exception>
@@ -179,10 +183,14 @@ ConnectAttempt::onShutdown(error_code ec)
         // - stream_truncated: the tcp connection closed (no handshake) it could
         // occur if a peer does not perform a graceful disconnect
         // - broken_pipe: the peer is gone
-        // - application data after close notify: benign SSL shutdown condition
+        // - SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY: benign SSL shutdown
+        //   condition where the peer sent data after we sent close_notify
+        bool const isAppDataAfterCloseNotify =
+            ec.category() == boost::asio::error::get_ssl_category() &&
+            ERR_GET_REASON(ec.value()) == SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY;
         bool const shouldLog =
             (ec != boost::asio::error::eof && ec != boost::asio::error::operation_aborted &&
-             ec.message().find("application data after close notify") == std::string::npos);
+             !isAppDataAfterCloseNotify);
 
         if (shouldLog)
         {

src/xrpld/overlay/detail/ConnectAttempt.h

@@ -3,6 +3,7 @@
 #include <xrpld/overlay/detail/OverlayImpl.h>
 
 #include <chrono>
+#include <string_view>
 
 namespace xrpl {
 
@@ -24,7 +25,7 @@ namespace xrpl {
  * 5. **Complete**: Connection successfully established
  *
  * Uses a hybrid timeout approach:
- * - **Global Timer**: Hard limit (20s) for entire connection process
+ * - **Global Timer**: Hard limit (25s) for entire connection process
  * - **Step Timers**: Individual timeouts for each connection phase
  *
  * - All errors result in connection termination
@@ -229,7 +230,7 @@ private:
     void
     processResponse();
 
-    static std::string
+    static std::string_view
     stepToString(ConnectionStep step)
     {
         switch (step)
@@ -250,7 +251,7 @@ private:
                 return "ShutdownStarted";
         }
         return "Unknown";
-    };
+    }
 
     template <class = void>
     static boost::asio::ip::tcp::endpoint

src/xrpld/overlay/detail/PeerImp.cpp

@@ -70,6 +70,7 @@
 #include <boost/asio/completion_condition.hpp>
 #include <boost/asio/error.hpp>
 #include <boost/asio/post.hpp>
+#include <boost/asio/ssl/error.hpp>
 #include <boost/asio/strand.hpp>
 #include <boost/asio/write.hpp>
 #include <boost/beast/core/multi_buffer.hpp>
@@ -77,6 +78,8 @@
 #include <boost/beast/core/stream_traits.hpp>
 
 #include <google/protobuf/message.h>
+#include <openssl/err.h>
+#include <openssl/sslerr.h>
 
 #include <xrpl.pb.h>
 
@@ -272,10 +275,7 @@ PeerImp::stop()
     if (!socket_.is_open())
         return;
 
-    // The rationale for using different severity levels is that
-    // outbound connections are under our control and may be logged
-    // at a higher level, but inbound connections are more numerous and
-    // uncontrolled so to prevent log flooding the severity is reduced.
+    // Log peer stop at debug severity to avoid excessive log noise.
     JLOG(journal_.debug()) << "stop: Stop";
 
     shutdown();
@@ -675,9 +675,9 @@ PeerImp::tryAsyncShutdown()
 
     shutdownStarted_ = true;
 
-    setTimer(kShutdownTimerInterval);
-
-    // gracefully shutdown the SSL socket, performing a shutdown handshake
+    // gracefully shutdown the SSL socket, performing a shutdown handshake.
+    // The safety timer was armed in shutdown(); it bounds the whole shutdown
+    // lifecycle (I/O drain + SSL handshake), not just this async_shutdown.
     stream_.async_shutdown(bind_executor(
         strand_, std::bind(&PeerImp::onShutdown, shared_from_this(), std::placeholders::_1)));
 }
@@ -694,6 +694,14 @@ PeerImp::shutdown()
 
     boost::beast::get_lowest_layer(stream_).cancel();
 
+    // Arm the shutdown safety timer at request time, replacing any pending
+    // peer-health timer. Otherwise, if I/O is still pending, the existing
+    // kPeerTimerInterval (60s) timer could be the one to fire and onTimer()
+    // would treat its expiry as a shutdown timeout — delaying forced close
+    // far beyond kShutdownTimerInterval.
+    cancelTimer();
+    setTimer(kShutdownTimerInterval);
+
     tryAsyncShutdown();
 }
 
@@ -708,9 +716,14 @@ PeerImp::onShutdown(error_code ec)
         // - stream_truncated: the tcp connection closed (no handshake) it could
         // occur if a peer does not perform a graceful disconnect
         // - broken_pipe: the peer is gone
+        // - SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY: benign SSL shutdown
+        //   condition where the peer sent data after we sent close_notify
+        bool const isAppDataAfterCloseNotify =
+            ec.category() == boost::asio::error::get_ssl_category() &&
+            ERR_GET_REASON(ec.value()) == SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY;
         bool const shouldLog =
             (ec != boost::asio::error::eof && ec != boost::asio::error::operation_aborted &&
-             ec.message().find("application data after close notify") == std::string::npos);
+             !isAppDataAfterCloseNotify);
 
         if (shouldLog)
         {
@@ -913,6 +926,9 @@ PeerImp::doAccept()
         app_);
 
     // Write the whole buffer and only start protocol when that's done.
+    // Track the write via writePending_ so a concurrent shutdown() doesn't
+    // launch async_shutdown while this write is still in flight.
+    writePending_ = true;
     boost::asio::async_write(
         stream_,
         writeBuffer->data(),
@@ -921,6 +937,7 @@ PeerImp::doAccept()
             strand_,
             [this, writeBuffer, self = shared_from_this()](
                 error_code ec, std::size_t bytesTransferred) {
+                writePending_ = false;
                 if (!socket_.is_open())
                     return;
                 if (ec == boost::asio::error::operation_aborted)
@@ -963,7 +980,7 @@ PeerImp::domain() const
 void
 PeerImp::doProtocolStart()
 {
-    // a shutdown was initiated before the handshare, there is nothing to do
+    // a shutdown was initiated before the handshake, there is nothing to do
     if (shutdown_)
     {
         tryAsyncShutdown();

src/xrpld/overlay/detail/PeerImp.h

@@ -33,9 +33,8 @@ class SHAMap;
 /**
  * @class PeerImp
  * @brief This class manages established peer-to-peer connections, handles
- message exchange, monitors connection health, and graceful shutdown.
+ * message exchange, monitors connection health, and graceful shutdown.
  *
-
  * The PeerImp shutdown mechanism is a multi-stage process
  * designed to ensure graceful connection termination while handling ongoing
  * I/O operations safely. The shutdown can be initiated from multiple points