logos-co/logos-protocol
1
0
Fork 0
mirror of https://github.com/logos-co/logos-protocol.git synced 2026-08-27 20:11:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(protocol): a burst that goes quiet must not wait for a call that never comes

Browse Source 
378d889 retired finished waiters, but from ONE site: the async-call spawn path.
So whatever finishes after the LAST spawn is never reaped, and a module that
bursts and then goes idle parks it all until the handle dies. Measured on
378d889, one handle, 2000 concurrent calls, every one delivered:

    after 2000 completed calls, IDLE:  m_waiters=1428   rss=+24.17 MiB
    after ONE further call:            m_waiters=1      rss=+ 1.92 MiB

The unbounded-per-call class was gone; this is what it left behind, and the
second line is the whole diagnosis — the corpses go the instant anything calls
again, so the reaper works and simply never runs. LogosAPIConsumer caches one
handle per module and never releases it between calls, so "bursts, then quiet"
is not a corner case: it is a UI that fans out on a refresh and then waits for
the user.

A finishing waiter now reaps the OTHER finished waiters before publishing
itself, so a burst drains as it completes. Same probe, same workload:

    after 2000 completed calls, IDLE:  m_waiters=1      rss=+ 1.88 MiB

THE BOUND IS ONE, NOT ZERO, and by construction rather than by luck: a waiter
can only reap OTHERS (a thread cannot join itself), so the last one to finish
has nobody behind it to collect it. Anything that publishes after the final
reap survives too, which is why 12 runs of the probe gave 1 eleven times and 2
once. Those go on the next call, or in teardown. Retention now tracks neither
call count nor peak concurrency — the sequential and in-flight-16 numbers move
from "15 -> 16 waiters" to "1 -> 1" — and the memory figures are unchanged
against 378d889 where they were already flat: 10k sequential +0.00 MiB, 10k at
16 in flight +0.06 MiB, 30k +0.09 MiB, and 10k through the production C ABI
(one lp_client, N lp_invoke_async) +0.09 MiB / 10 B per call, the same as
378d889 reported.

THE ORDER IS THE SAFETY ARGUMENT. Reap first, publish last, never the reverse:

  * Publishing is what makes a waiter joinable BY ANOTHER WAITER. Reaping first
    keeps that relation one-way — unpublished threads join published ones,
    published ones join nobody — so it has no cycles. Inverted, two waiters
    publishing in the same instant can each take the other's thread out of
    m_waiters and then join it; both are already out of the registry, so
    teardown does not even wait for them. Built that variant: pthread_join
    detects the cycle and throws, the half-drained thread vector then destroys
    a still-joinable thread, and the process aborts — the EXISTING hammer
    (ReapingRacesPublishingWithoutDeadlocking) catches it 5 runs out of 5, with
    the stack showing two waiters inside FinishOnExit joining each other.
  * While a waiter is unpublished it is still in m_waiters, so a concurrent
    teardown joins it and the object cannot be destroyed under the reap. Once
    published, a reaper may take its thread out of the map and release() may
    `delete this` — and a reaper on the CALLER's thread (the spawn path) is one
    teardown neither knows about nor waits for, so a post-publish touch of
    m_waiterMu is a use-after-free on a member mutex. That path needs a caller
    still issuing calls while another thread releases, which this class already
    treats as caller-side UB, so it is stated as an argument; the cycle above is
    what the tests actually demonstrate.

Two corrections to 378d889, which this change makes load-bearing rather than
cosmetic. NOT amended into it — it is pushed, and a commit that misstates its
own reasoning is better read alongside the correction than rewritten.

  * plain_logos_object.h:107-109 said reapFinishedWaiters() is "called on every
    async spawn ... and from stopAndJoinWaiters()". It is not, and never was,
    called from stopAndJoinWaiters(): teardown does its own id-independent
    brute-force join, which is precisely why it needs no cooperation from the
    reaper. Harmless behaviourally, wrong in a mechanism whose entire argument
    is who joins what and when. The comment now names the two real callers —
    the spawn path and, as of this commit, every waiter on its way out.

  * 378d889's message presented "the join is outside the lock" as THE property
    that prevents the reaper deadlock, "proven by construction" by its hammer.
    That is overstated, in a way that would let the guarantee be refactored
    away with the suite still green. TWO independent properties each suffice:
    joining only PUBLISHED ids (a published waiter never needs m_waiterMu
    again, so it cannot be the thread being shut out), and joining outside the
    lock. The hammer only wedges when BOTH are gone. Measured, on top of this
    change: the variant that joins under the lock but KEEPS the published-only
    filter passes ReapingRacesPublishingWithoutDeadlocking in 293/297/290ms
    across three runs and the whole reaping suite besides, while the variant
    that joins everything under the lock trips the watchdog at 60s. So a later
    "simplification" that moves the join inside the lock would ship green. Both
    properties are kept, and the comment now says which one the test is
    actually testing.

The TODO above the waiter still stands: the real fix is to fold the wait into
the shared Asio io_context and have no thread per pending RPC at all. This
makes the interim honest; it does not replace it.

Verified by running, each check first shown to FAIL on unfixed code:

  * Retention: the burst probe above, plus a committed regression test that
    reads m_waiters out of the live object through the explicit-instantiation
    access hole. 800 concurrent completed calls, then IDLE with NO further
    call: 1 waiter left, 20 runs out of 20. On 378d889 the same test leaves
    610 of 800 and fails. The pre-existing sequential and in-flight tests are
    unchanged and still pass.
  * The UAF stays closed — the check that matters most here, because this adds
    an object access late in the waiter's life. 9 reaping/teardown-race tests
    plus the 7-test teardown suite clean under macOS Guard Malloc (ASan is
    unusable on this box: it hangs in its own initializer). DETECTOR VALIDATED
    both ways: turning teardown's join back into a detach SIGSEGVs under Guard
    Malloc on the release-during-call hammer (exit 139), and the specific
    inversion this change risks — reaping AFTER publishing — aborts as
    described above.
  * No deadlock: reap-vs-publish hammered 20x (1600 calls in 40 overlapping
    bursts each), plus 60 rounds of teardown landing from another thread while
    the tail of a burst retires itself, plus 6x600-call bursts checking that
    LIVE OS threads (task_threads, which counts wedges and not corpses) come
    back to baseline every round. Clean; the watchdog names the cause if it
    ever is not.
  * Exactly-once on all four paths — normal, timeout, cancellation, deferred
    sentinel — counted per call. Each detector validated with a broken build:
    dropping the cancelled callback fails 4 tests, dropping the timeout one
    fails its test, and double-delivering the normal/deferred arm fails those.
  * Teardown latency unchanged from 378d889: 1-25ms with an in-flight 8000ms
    call and 0ms mid-defer across 5 runs, against 2-21ms / 0ms on that commit —
    the same one-wait-slice (25ms) bound, since a waiter's extra work happens
    after it has stopped waiting.
  * Full suite 282/282 three times, `nix build .#tests` green,
    CallErrorAfterAcquireTest hammered 40x clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Dario Gabriel Lipicar
2026-08-05 22:17:27 -03:00
co-authored by Claude Opus 5
parent 378d889a1d
commit 8f0c60fe17
3 changed files with 181 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cpp/implementations/plain/plain_logos_object.cpp
+72 -18
View File
@@ -164,11 +164,14 @@ void PlainLogosObject::reapFinishedWaiters()
if (it == m_waiters.end())
continue; // teardown already took this one
if (it->second.get_id() == std::this_thread::get_id()) {
// Unreachable today — callbacks are delivered on the Qt event
// loop, so a waiter thread never re-enters this class — but a
// thread that joined itself would terminate the process, and
// this is one comparison. Leave it registered; teardown, which
// runs on somebody else's thread, will collect it.
// A waiter DOES run this now, on its way out — but always
// BEFORE it publishes, so its own id cannot be in the list it
// is walking, and this branch stays unreachable. It costs one
// comparison, and it turns the ordering slip that would make it
// reachable (publishing before reaping) into a leaked entry
// rather than a self-join, which throws out of the noexcept
// destructor doing the reaping and takes the process with it.
// Leave it registered; the next reaper, or teardown, collects it.
keep.push_back(id);
continue;
}
@@ -177,12 +180,24 @@ void PlainLogosObject::reapFinishedWaiters()
}
m_finishedWaiters.swap(keep);
}
// Joined with NO lock held. Not just hygiene — this is THE deadlock this
// whole mechanism can introduce: a reaper holding m_waiterMu while it joins
// a waiter that is itself blocked on m_waiterMu trying to publish would
// wedge the process. Holding no lock across a join makes that impossible by
// construction rather than by argument, whatever a waiter does on its way
// out. stopAndJoinWaiters() keeps the same discipline for the same reason.
// Joined with NO lock held. The deadlock this whole mechanism can
// introduce is a reaper that holds m_waiterMu while it joins a waiter which
// is itself blocked on m_waiterMu trying to publish. TWO INDEPENDENT
// PROPERTIES each prevent it, and either one alone would be enough:
//
// * only PUBLISHED ids are joined, and publishing is a waiter's last
// access — so a thread this function joins can never be a thread that
// still wants m_waiterMu;
// * no join happens with a lock held, so even joining a thread that DID
// still want the mutex could not shut it out.
//
// Both are kept on purpose: the filter is a property of the logic here,
// which a refactor can lose without looking wrong, while "no join under a
// lock" is structural and tends to survive one. Be precise about what that
// costs in testing, though — the hammer in the regression suite only wedges
// when BOTH are gone. A variant that joins under the lock but keeps the
// published-only filter passes it, measured, in the usual few hundred ms.
// stopAndJoinWaiters() keeps the same discipline.
for (auto& t : done) {
if (t.joinable())
t.join();
@@ -447,10 +462,13 @@ void PlainLogosObject::callMethodAsyncWithError(const QString& authToken,
// `delete this` while a waiter could still be mid-flight.
const std::string objectName = m_objectName;
const std::string method = methodName.toStdString();
// Retire the previous calls' waiters before adding one. Done HERE rather
// than by the waiters themselves because a thread cannot join itself; done
// BEFORE taking m_waiterMu because it joins, and joining under that lock is
// the deadlock described in reapFinishedWaiters().
// Retire the previous calls' waiters before adding one. The waiters reap
// each other too, on their way out (see the guard below) — that is what
// drains a burst which then goes quiet, and it is why this site is no
// longer the only reaper. It still earns its keep: a waiter can only reap
// OTHERS, so the last one to finish has nobody behind it to collect it.
// Done BEFORE taking m_waiterMu because it joins, and joining under that
// lock is the shape described in reapFinishedWaiters().
reapFinishedWaiters();
// Register under the lock BEFORE the thread can outrun release(): a
// detach-then-push left a window where delete this raced the waiter.
@@ -494,11 +512,47 @@ void PlainLogosObject::callMethodAsyncWithError(const QString& authToken,
// it is the unwinding of the captures above, none of which belongs
// to the object: a string, a shared_ptr to the call's future, and a
// callback that has already been moved out.
struct PublishOnExit {
//
// It REAPS BEFORE IT PUBLISHES, and that order is the safety
// argument rather than a stylistic choice. Two reasons, one of
// which the test suite demonstrates:
//
// * Publishing is what makes a waiter joinable BY ANOTHER WAITER.
// Reaping first keeps that relation one-way — unpublished
// threads join published ones, published ones join nobody — so
// it cannot contain a cycle. Inverted, two waiters that publish
// in the same instant can each take the other's thread out of
// m_waiters and then join it. Both are already out of the
// registry, so teardown does not even wait for them; here
// pthread_join detects the cycle and throws out of
// reapFinishedWaiters, whose half-drained vector then destroys
// a still-joinable thread — std::terminate. Measured: with the
// two lines below swapped, ReapingRacesPublishingWithoutDead-
// locking aborts the process, 5 runs out of 5.
// * Until it publishes, this waiter is still in m_waiters, so a
// concurrent teardown joins it and the object cannot be
// destroyed under the reap. After publishing, a reaper can take
// its thread out of the map and release() can `delete this`,
// and a reaper running on the CALLER's thread (the spawn path
// above) is one teardown neither knows about nor waits for — so
// the touch of m_waiterMu would land on freed memory. That one
// needs a caller still issuing calls while another thread
// releases, which this class already treats as caller-side UB,
// so it is an argument and not a demonstration; the cycle above
// is the demonstration.
//
// Reaping here at all is what makes the retention bound hold for a
// module that bursts and then goes quiet: the spawn-path reaper
// only runs if another call ever comes.
struct FinishOnExit {
PlainLogosObject* self;
std::uint64_t id;
~PublishOnExit() { self->publishFinishedWaiter(id); }
} publishOnExit{this, waiterId};
~FinishOnExit()
{
self->reapFinishedWaiters(); // others, never itself
self->publishFinishedWaiter(id); // strictly last
}
} finishOnExit{this, waiterId};
const WaitOutcome outcome = waitForResult(*fut, timeoutMs, m_stopping);
if (outcome == WaitOutcome::Cancelled) {
cpp/implementations/plain/plain_logos_object.h
+22 -9
View File
@@ -104,14 +104,23 @@ private:
void stopWaiters();
// Join and drop the waiters that have already FINISHED, so a handle that
// outlives its calls does not accumulate them. Called on every async spawn
// (a call pays for the corpse of an earlier one) and from
// stopAndJoinWaiters(). Cheap: a join on an already-returned thread is a
// couple of syscalls, and only ids a waiter itself published are touched.
// outlives its calls does not accumulate them. TWO call sites, which
// between them cover both shapes of traffic:
//
// * every async spawn — a call pays for the corpses of earlier ones;
// * every waiter as it finishes, BEFORE it publishes its own id — so a
// burst drains itself instead of parking until the next call, which for
// a module that bursts and goes quiet may never come.
//
// NOT called from stopAndJoinWaiters(): teardown joins by id-independent
// brute force and needs no published list. (It used to say otherwise here;
// it never did.) Cheap either way: a join on an already-returned thread is
// a couple of syscalls, and only ids a waiter itself published are touched.
void reapFinishedWaiters();
// A waiter's FINAL act — see the scope guard in callMethodAsyncWithError.
// After this returns, that thread never touches the object again, which is
// what makes it safe for someone else to join and drop it.
// what makes it safe for someone else to join and drop it. Nothing the
// waiter does may follow it, its own reap least of all.
void publishFinishedWaiter(std::uint64_t id);
std::string m_objectName;
@@ -131,11 +140,15 @@ private:
// each) for the whole life of the handle. The production shape is one
// cached handle per module reused for every call (logos_api_consumer.cpp),
// so that grew without bound. Now a waiter publishes its id into
// m_finishedWaiters as its last act and the next spawn (or teardown) joins
// and erases it: see reapFinishedWaiters().
// m_finishedWaiters as its last act, and both the next spawn and every
// OTHER waiter on its way out join and erase it: see reapFinishedWaiters().
//
// Retention is bounded by the waiters that finish after the LAST spawn,
// i.e. by peak in-flight concurrency, not by call count.
// Retention tracks neither call count nor peak concurrency. A burst drains
// as it completes, because each waiter reaps the ones that finished before
// it. What survives an idle handle is only what published after the last
// reap — at minimum the last waiter to finish, which by construction has
// nobody behind it to collect it (measured: 1-2 after a 2000-call burst).
// The next call, or teardown, takes those.
//
// The real fix is still the TODO in callMethodAsyncWithError — fold the
// wait into the shared Asio io_context and have no thread per pending RPC
tests/protocol/test_plain_waiter_reaping.cpp
+87 -3
View File
@@ -24,9 +24,15 @@
// 1. THE REGISTRY DOES NOT GROW WITH CALL COUNT. Counting threads, not bytes:
// RSS is a noisy proxy and its per-call constant is platform-specific,
// whereas "m_waiters.size() rises 1:1 with completed calls and only ever
// falls in teardown" is the defect itself, exactly and portably. The bound
// is peak in-flight concurrency, because the reaper runs on the next
// spawn — so a sequential caller keeps ~1 and NOT ~N.
// falls in teardown" is the defect itself, exactly and portably. So a
// sequential caller keeps ~1 and NOT ~N.
//
// 1b. AND IT DRAINS WITHOUT ANOTHER CALL. Reaping on the spawn path alone
// leaves the tail of a burst parked until the next call, which for a
// module that bursts and then goes quiet may never come: 2000 completed
// calls kept 1428 waiters and 24MiB once the handle went idle, and one
// further call dropped that to 1. Waiters therefore reap each other on
// their way out, and this pins the IDLE bound with no further spawn.
//
// 2. THE DEADLOCK THE FIX COULD INTRODUCE. Reaping means joining, and a
// reaper that joined while holding the lock a waiter needs in order to
@@ -445,6 +451,84 @@ TEST_F(PlainWaiterReapingTest, ConcurrentCompletedCallsStayBoundedByInFlight)
pump(50);
}
// ── 1b. a burst that goes idle drains itself ────────────────────────────────
//
// The test above always has another call coming, which hides the case that
// actually shows up in production: a module bursts, every call completes, and
// then the handle goes quiet. If reaping only ever happened on the spawn path,
// everything that finished after the LAST spawn would stay parked for the life
// of the handle — measured at 1428 waiters and +24MiB after 2000 completed
// calls, collapsing to 1 the moment one further call was issued.
//
// So the bound is read here with NO further call: the burst has to have drained
// itself. What remains is what published after the last reap — at minimum the
// last waiter to finish, which has nobody behind it to collect it (1 in almost
// every run, 2 when a waiter's publish slips past the final reap). The bound
// below is generous against that and still ~100x under the pre-fix number.
TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall)
{
LiveHost host;
ASSERT_TRUE(host.ok());
auto conn = connectTo(host.port());
ASSERT_NE(conn, nullptr);
LogosObject* obj = conn->requestObject(QStringLiteral("echo_module"), 5000);
ASSERT_NE(obj, nullptr);
auto* ch = channelFor(obj);
ASSERT_NE(ch, nullptr);
auto* plain = dynamic_cast<PlainLogosObject*>(obj);
ASSERT_NE(plain, nullptr);
// Issued in one go, with no pumping in between, so they really are
// concurrent and the tail of the burst is large.
constexpr int kBurst = 800;
Deliveries d(kBurst);
for (int i = 0; i < kBurst; ++i) {
ch->callMethodAsyncWithError(kToken, QStringLiteral("ping"),
QVariantList{ QVariant(i) }, 9000,
[&d, i](QVariant, const logos::CallError& e) {
d.record(i, e);
});
}
pumpUntilTotal(d, kBurst, 60000);
ASSERT_EQ(d.total.load(), kBurst) << "the burst did not all complete";
// Every callback has landed; now let the waiters that delivered them finish
// and retire each other. No call is issued in this window — that is the
// whole point — so anything still registered is retained, not in flight.
for (int i = 0; i < 40; ++i) {
QCoreApplication::processEvents(QEventLoop::AllEvents, 5);
QThread::msleep(10);
}
const size_t idle = waiterCount(plain);
std::cout << " " << kBurst << " completed calls then IDLE -> m_waiters="
<< idle << std::endl;
EXPECT_EQ(d.worst(), 1);
EXPECT_EQ(d.missing(), 0);
EXPECT_EQ(d.errors.load(), 0);
EXPECT_LE(idle, 8u)
<< "a burst that went idle left " << idle << " of " << kBurst
<< " waiters parked: they are only being reaped on the spawn path";
// And the handle still works afterwards — draining from inside the waiters
// must not have disturbed the object they are draining.
Deliveries after(1);
ch->callMethodAsyncWithError(kToken, QStringLiteral("ping"),
QVariantList{ QVariant(7) }, 5000,
[&after](QVariant, const logos::CallError& e) {
after.record(0, e);
});
pumpUntilTotal(after, 1, 10000);
EXPECT_EQ(after.total.load(), 1);
EXPECT_EQ(after.errors.load(), 0);
EXPECT_LE(waiterCount(plain), 8u);
obj->release();
pump(50);
}
// ── 2. the deadlock the fix could introduce ─────────────────────────────────
//
// A waiter announces itself as finished under m_waiterMu; the reaper takes that