chore: Revert graceful peer disconnection and follow-up fix

[Tapanito]

Reverts the following commits:

• 17a2606 "Bugfix: Adds graceful peer disconnection (Bugfix: Adds graceful peer disconnection #5669)"
• e80642f "fix: Fix regression in ConnectAttempt (Fix regression in ConnectAttempt #5900)"

Performed as a manual revert against current HEAD so that identifier renames landed by subsequent refactors (snake_case to camelCase methods, fee* to kFee* constants, scoped enums, etc.) are preserved.

High Level Overview of Change

Context of Change

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[xrplf-ai-reviewer]

No issues.

Review by Claude Opus 4.6 · Prompt: V15

[xrplf-ai-reviewer]

Ship it

Review by Claude Opus 4.6 · Prompt: V15

[xrplf-ai-reviewer]

No issues.

Review by Claude Opus 4.6 · Prompt: V15

[Copilot]

Pull request overview

This PR manually reverts prior changes that introduced a more “graceful” peer disconnect path (SSL async_shutdown + shutdown state machine) and a follow-up ConnectAttempt regression fix, while keeping later refactor renames intact.

Changes:

• Reverts PeerImp shutdown/state-machine logic back toward a simpler close path, reintroducing a gracefulClose_-based EOF handling flow.
• Reverts ConnectAttempt’s multi-step timeout/shutdown tracking to a simplified single-timer model and simpler close behavior.
• Replaces std::set::count(...) > 0 checks with C++20 contains(...) in Door.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/xrpld/overlay/detail/PeerImp.h	Removes extensive shutdown documentation/state, adds new flags and reshapes close/fail/shutdown-related method surface.
src/xrpld/overlay/detail/PeerImp.cpp	Reworks stop/send/close + EOF/SSL shutdown handling and timer helpers.
src/xrpld/overlay/detail/ConnectAttempt.h	Simplifies ConnectAttempt interface/state and removes step-timeout machinery.
src/xrpld/overlay/detail/ConnectAttempt.cpp	Reverts connection attempt lifecycle toward simpler close + single timer; adjusts response processing and logging.
include/xrpl/server/detail/Door.h	Uses protocol.contains(...) for protocol detection.

Comments suppressed due to low confidence (6)

src/xrpld/overlay/detail/PeerImp.cpp:813

• PeerImp::onShutdown treats a successful SSL shutdown (ec == 0) as an error ("expected error condition") and only accepts boost::asio::error::eof as the success case. This is inconsistent with other shutdown handlers in the repo (e.g. SSLHTTPPeer::onShutdown) and can produce false error logs / incorrect failure handling when async_shutdown completes successfully. Consider treating both !ec and ec == boost::asio::error::eof as normal completion, and only logging/failing for other errors (with operation_aborted handled separately if needed).

PeerImp::onShutdown(error_code ec)
{
    cancelTimer();
    // If we don't get eof then something went wrong
    if (!ec)
    {
        JLOG(journal_.error()) << "onShutdown: expected error condition";
        close();
        return;
    }
    if (ec != boost::asio::error::eof)
    {
        fail("onShutdown", ec);
        return;
    }
    close();

src/xrpld/overlay/detail/PeerImp.cpp:631

• PeerImp::close only cancels timer_ when socket_.is_open(). If close() (or stop()/fail()) is invoked after the socket has already been closed, the pending timer_.async_wait will remain scheduled and keep the PeerImp alive until it fires (up to kPeerTimerInterval). Consider cancelling the timer unconditionally (via cancelTimer()) even when the socket is already closed.

PeerImp::close()
{
    XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::close : strand in this thread");
    if (socket_.is_open())
    {
        detaching_ = true;  // DEPRECATED
        try
        {
            timer_.cancel();
            socket_.close();

src/xrpld/overlay/detail/ConnectAttempt.cpp:108

• ConnectAttempt::run no longer posts to the strand and no longer arms a timeout before starting async_connect. This makes run() not thread-safe (callers can invoke it off-strand) and can allow the TCP connect to hang indefinitely (no setTimer() until after onConnect). Consider restoring the running_in_this_thread() gate and starting the timer before async_connect.

This issue also appears in the following locations of the same file:

• line 280
• line 329

void
ConnectAttempt::run()
{
    stream_.next_layer().async_connect(
        remoteEndpoint_,
        boost::asio::bind_executor(
            strand_,
            std::bind(&ConnectAttempt::onConnect, shared_from_this(), std::placeholders::_1)));
}

src/xrpld/overlay/detail/ConnectAttempt.cpp:300

• After a successful HTTP write, ConnectAttempt::onWrite starts http::async_read without arming a timer. This means the HTTP read step can block indefinitely if the peer stops responding. Consider calling setTimer() before starting the async_read (and cancelling it in onRead).

void
ConnectAttempt::onWrite(error_code ec)
{
    cancelTimer();
    if (!socket_.is_open())
        return;
    if (ec == boost::asio::error::operation_aborted)
        return;
    if (ec)
    {
        fail("onWrite", ec);
        return;
    }
    boost::beast::http::async_read(
        stream_,
        readBuf_,
        response_,
        boost::asio::bind_executor(
            strand_,
            std::bind(&ConnectAttempt::onRead, shared_from_this(), std::placeholders::_1)));
}

src/xrpld/overlay/detail/ConnectAttempt.cpp:345

• ConnectAttempt::onShutdown treats a successful SSL shutdown (ec == 0) as an error and only accepts boost::asio::error::eof as success. That can lead to false error logs and unnecessary failures when the TLS shutdown handshake completes normally. Consider treating !ec and ec == boost::asio::error::eof as normal completion, and failing/logging only other errors (with operation_aborted handled separately if applicable).

void
ConnectAttempt::onShutdown(error_code ec)
{
    cancelTimer();
    if (!ec)
    {
        JLOG(journal_.error()) << "onShutdown: expected error condition";
        close();
        return;
    }
    if (ec != boost::asio::error::eof)
    {
        fail("onShutdown", ec);
        return;
    }
    close();
}

src/xrpld/overlay/detail/ConnectAttempt.h:101

• parseEndpoint uses std::istringstream, but this header doesn't include <sstream> directly, relying on transitive includes (currently via Application.h -> PropertyStream.h). Adding the direct include would avoid fragile header-order dependencies.

    template <class = void>
    static boost::asio::ip::tcp::endpoint
    parseEndpoint(std::string const& s, boost::system::error_code& ec)
    {
        beast::IP::Endpoint bep;
        std::istringstream is(s);
        is >> bep;
        if (is.fail())
        {
            ec = boost::system::errc::make_error_code(boost::system::errc::invalid_argument);
            return boost::asio::ip::tcp::endpoint{};
        }

        return beast::IPAddressConversion::toAsioEndpoint(bep);
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 145 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.3%. Comparing base (afcf6fb) to head (454a5ef).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/overlay/detail/ConnectAttempt.cpp	0.0%	82 Missing ⚠️
src/xrpld/overlay/detail/PeerImp.cpp	0.0%	63 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #7296     +/-   ##
=========================================
+ Coverage     82.1%   82.3%   +0.1%     
=========================================
  Files         1011    1011             
  Lines        76343   76194    -149     
  Branches      7417    7341     -76     
=========================================
- Hits         62691   62681     -10     
+ Misses       13652   13513    -139     

Files with missing lines	Coverage Δ
include/xrpl/server/detail/Door.h	76.0% <ø> (ø)
src/xrpld/overlay/detail/ConnectAttempt.h	0.0% <ø> (ø)
src/xrpld/overlay/detail/PeerImp.h	19.6% <ø> (ø)
src/xrpld/overlay/detail/PeerImp.cpp	5.6% <0.0%> (+0.1%)	⬆️
src/xrpld/overlay/detail/ConnectAttempt.cpp	0.0% <0.0%> (ø)

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[mvadari]

Can you also call out the clang-tidy changes in the second commit in the PR description, since it's not just the revert?

[bthomee]

I didn't get to reviewing it all now, but at least can give you some feedback.

[xrplf-ai-reviewer]

No issues.

Review by Claude Opus 4.6 · Prompt: V15

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

src/xrpld/overlay/detail/PeerImp.cpp:1053

• When gracefulClose_ is set and the sendQueue_ becomes empty, onWriteMessage starts stream_.async_shutdown() directly but does not arm any shutdown timeout. If the peer never completes the TLS close_notify handshake, this async_shutdown can hang indefinitely. Prefer routing through gracefulClose() (or arming a shutdown timer here) so shutdown has a bounded duration.

    if (gracefulClose_)
    {
        stream_.async_shutdown(bind_executor(
            strand_, std::bind(&PeerImp::onShutdown, shared_from_this(), std::placeholders::_1)));
        return;

[xrplf-ai-reviewer]

No issues.

Review by Claude Opus 4.6 · Prompt: V15