Codex review flagged the drop test as timing-dependent: a very fast
endpoint could let sendThread drain/restart between send() calls, so the
deque might not stay full and no drop would occur. Add a per-reply delay
to the mock endpoint so delivery is reliably slower than the
microsecond-fast enqueue loop — the deque stays at the cap and drops are
deterministic (received is now consistently 2/50).
Add a mock mode that sends a header promising more body than it
delivers then holds the socket, so the client blocks in handleData and
hits its deadline — driving the read-error branch (mShutdown != eof)
that no existing test exercised.
With per-batch re-queue, sendThread is only ever started with a
non-empty deque (send() enqueues before starting a job; the re-queue
fires only when the deque is non-empty), so the dispatched==0 early
return was unreachable. Guard the dispatch JLOG/run() with dispatched>0
instead, which falls through to clear mSending if a batch is ever empty.
Removes an uncovered branch.
RPCSub_test.cpp includes xrpld/core/Job.h and JobQueue.h (for
jtCLIENT_SUBSCRIBE and getJobCountTotal), adding a test.net > xrpld.core
edge that wasn't regenerated when those includes were added.
Minor: the MockHTTPServer destructor closed heldSockets_ from the main
thread while the server io_service thread could still be running. The
held sockets have no pending async op so it was benign, but move the
close to after the thread is joined to avoid the cross-thread access
entirely (no lock needed once single-threaded).
The test never set noContentLength, so the mock sent Content-Length and
completion happened in handleHeader, not the handleData EOF branch the
test targets — it passed regardless of the fix. Force the no-Content-
Length path so it covers what it claims.
RPCCall::fromNetwork hardcoded a 256MB read budget for responses with no
Content-Length. Webhook deliveries (method "event") ignore the response
body, but each in-flight delivery still pre-allocated a ~256MB streambuf
on the EOF path — up to 32 (maxInFlight) x 256MB = 8GB transient, on
exactly the no-Content-Length error shape this branch targets. Cap the
"event" method at 1MB; genuine RPC replies (CLI) keep the 256MB budget.
Two issues with sendThread, both surfaced by an independent review:
UAF: the sending job captured a raw `this`. An admin Unsubscribe runs
NetworkOPsImp::tryRemoveRpcSub, which erases the sub from mRpcSubMap
under mSubLock and destroys it inline — while sendThread may be running
on a job-queue worker under the *unrelated* InfoSub::mLock. The blocking
io_service.run() (up to the 30s request deadline for a hung endpoint)
makes the window large, and a hung endpoint is exactly this fix's
scenario. RPCSubImp now derives enable_shared_from_this and the job
captures weak_from_this(), re-locking it on entry so the sub stays alive
for the batch; once it's gone the job is a no-op.
Starvation: sendThread drained the entire backlog (up to 16384 events,
512 batches x up to 30s) in one job, blocking a worker thread the whole
time. With a small pool shared with consensus/ledger/RPC, a few hung
subscribers could occupy most workers. sendThread now processes ONE
batch per job and re-queues if more remain, so the worker is released
between batches and other jobs interleave.
The RPCSub tests crashed/timed out under the instrumented Debug coverage
build (where the full HTTP stack is ~50x slower than Release):
- Deterministic teardown: wait on JobQueue::getJobCountTotal(
jtCLIENT_SUBSCRIBE) == 0 before destroying the sub. sendThread captures
a raw `this`; a fixed-sleep grace raced it under slow builds → UAF.
- Lower per-test event counts (50 -> 10) so a handful of real HTTP
deliveries finish well within the timeout under instrumentation.
Queue-cap drop coverage: make_RPCSub gains a defaulted maxQueueSize
parameter (production default unchanged at 16384) so a test can set a
tiny cap and exercise the drop path by out-pacing delivery — enqueue is
microsecond-fast while each delivery is a full round-trip — instead of
queueing 16384 events. New testQueueCapDrops confirms some events are
delivered and some dropped.
Raises RPCSub.cpp patch coverage 62% -> 87% (overall diff 65% -> 87%);
the only remaining uncovered lines are the defensive sendThread
exception catches.
RPCSub_test (new): the webhook delivery flow control + EOF handling had
zero automated coverage. Three cases drive events through a real local
HTTP endpoint: delivery, delivery continues despite HTTP 500 responses
(the #6341 stall, exercised via EOF-delimited error replies), and
sending restarts after the queue drains (mSending reset).
HTTPClient_test mock fixes:
- heldSockets_: 'sendResponse=false' now genuinely holds the connection
open instead of dropping the only socket shared_ptr (which closed it).
testCleanupAfterTimeout now actually exercises the deadline path.
- await-client-EOF on Content-Length responses: the mock decrements its
connection count only when the CLIENT closes its socket, so
activeConnectionCount() reflects client-side FD release rather than the
server's own lifecycle — giving testGetSelfReferenceCleanup real teeth.
- poll helpers replace fixed sleeps for the cross-thread count checks.
get() bound shared_from_this() into mBuild (a member), forming a
this -> mBuild -> shared_ptr<this> cycle that never broke after the
request completed — leaking the HTTPClientImp and its socket FD. mBuild
is only ever invoked from handleRequest(), which always runs inside an
async handler already holding a shared_from_this(), so a non-owning
this is safe and lets the object be destroyed once the request finishes.
Pre-existing (also present upstream in rippled); only the fetch path
uses get(), not the webhook delivery path.
Revert the eof->success translation from the previous commit. The
upstream rippled #6344 deliberately keeps forwarding ecResult as-is;
translating eof to success would report a truncated Content-Length
response (server closes before sending the promised N body bytes) as a
successful read and suppress HTTPClient::get() multi-site fallback.
Keeps the structural simplification (collapsed branches, early return).
Backports the robustness changes that landed during review of the
upstream rippled PR (XRPLF/rippled#6344) but never made it into this
branch, plus the applicable Copilot suggestions.
RPCSub::sendThread:
- Wrap the whole dispatch loop in try/catch so mSending is reset under
the lock on EVERY exit path (drain, dispatch throw, run() throw).
Previously a throw from the lock-guarded dispatch block escaped
sendThread() without clearing mSending, re-introducing the original
stall (issue #6341) where send() never starts another job.
- Bail out (reset mSending + return) on an io_service.run() exception
instead of silently looping.
RPCSub::send:
- Rate-limit the queue-full drop warning (once per dropLogInterval) so
a persistently behind endpoint can't flood the log; still advance
mSeq on every drop so consumers detect the gap.
HTTPClient::handleData:
- Collapse the identical EOF/non-EOF completion branches into one and
early-return on read error (per maintainer review).
- Report success (not eof) to invokeComplete on the EOF-delimited path
so callers don't treat a complete response as a failure.
Resolves conflicts from the src/ripple -> src/xrpld + cmake restructure:
- src/xrpld/net/RPCSub.h: take dev's xrpld/ include paths, keep our
removal of the now-unused boost/asio/io_service.hpp include
- Builds/CMake/RippledCore.cmake: accept dev's deletion; tests are now
auto-discovered via GLOB_RECURSE over src/test, so the explicit
HTTPClient_test.cpp listing is obsolete
- Builds/levelization/results/ordering.txt: regenerated via
levelization.py (adds test.net > {xrpl.basics, xrpld.net, test.toplevel})
- src/test/net/HTTPClient_test.cpp: update ripple/ includes to xrpl//xrpld/
