Files
logos-protocol/cpp/implementations/plain/plain_logos_object.h
Dario LipicarandClaude Opus 5 dda5dae1bf test(plain): bound the burst-drain assertion against the burst, not a constant (#56)
BurstThatGoesIdleDrainsWithoutAnotherCall failed on ubuntu-latest at 21, then at
10 on a re-run, against EXPECT_LE(idle, 8u) — having scored 7 against that same
8 the run before. The change under review is not involved: the same source
compiles to a byte-identical object file with and without it.

WHAT THE RESIDUE IS. A waiter reaps only OTHERS, never itself, so what survives
an idle burst is whatever published after the FINAL reap: the last waiter to
finish has nobody behind it, and a waiter sitting in the join loop of its own
reap has not published yet while the batch it did not collect already has. That
is the size of the last exit batch, which is the scheduler's business.

AND NOTHING TAKES IT LATER, so this is not a window that was too short. Sampled
from 100ms to 25.6s after the burst went quiet the count does not move: 5->5 and
17->17 idle, 2->2 under 4x CPU oversubscription, and 24->24, 33->33, 49->49,
82->82, 123->123, 138->138, 168->168 under 32x — 12 runs, every one flat. It is
a residue, not a drain in progress.

MEASURED, 20 runs per cell, this 800-call burst, m_waiters when it goes idle:

                    this code             reaping only on the spawn path
  macOS idle        1 every run           572-723
  Linux idle        1-80  (median 9)      4-168   (median 80)
  Linux 2x CPU      1-145 (median 14)     16-527  (median 275)
  Linux 4x CPU      1-104 (median 28)     10-504  (median 271)

Two things fall out of that, and the second is why this commit says more than
"the number was too small".

  1. 8 WAS READ OFF THE macOS COLUMN. On Linux it sits under the MEDIAN of a
     correct build — 35 of 60 unloaded runs of correct code exceed it — so the
     test was failing correct code in most Linux runs. CI's 7 was luck.

  2. THE ~600 THIS TEST IS DOCUMENTED AGAINST IS macOS-ONLY. On Linux the burst
     is not concurrent: spawning 800 std::threads costs more than a loopback
     ping, so most of it has already been collected by the SPAWN-path reaper
     before the last call is issued, and the defect's own residue collapses
     into the same range as a correct build's (4-168 idle, min 4). The two arms
     overlap there at any bound, 8 included. This assertion is a coarse
     retention check on Linux, not the detector for that defect.

THE NEW BOUND is kBurst/2 — the majority of the burst must have retired itself
with no further call — because the residue has no ceiling for a tighter
fraction to sit under. Worst per load level, 620 runs on a 6-core Linux box:

  idle 152 (n=60)  2x 145 (n=20)  4x 172 (n=80)  8x 175 (n=160)
  16x 278 (n=120)  32x 402 (n=60)  64x 317 (n=40)

Flat out to 8x, climbing after. A quarter of the burst (200) would have been
the original mistake in a new unit: it clears the worst by 1.14x, the same
ratio as 7-against-8. Half clears everything up to 16x by 1.44x and the worst
CI has ever produced (21) by 19x. The single run in 620 that scored 402, at 32x
oversubscription, is recorded in the comment rather than rounded away.

Both assertions in the test take the same expression, the second included: a
residue the follow-up call did not collect is the same retention bug, and a
tighter hard-coded number there would only move the magic constant somewhere
quieter.

Also corrects the retention note in plain_logos_object.h, which quoted "1-2
after a 2000-call burst" as though it were platform-independent.

THE DETECTOR, rebuilt with the defect this test exists to catch — the reap
dropped from the waiter's exit guard, leaving only the spawn path:

  this assertion, macOS          RED 10/10, 550-614 against 400
  this assertion, Linux 16x      RED 4/10, up to 645
  this assertion, Linux idle     GREEN 0/15, 25-208 — see below
  publish-is-last, macOS         RED 5/5
  publish-is-last, Linux         RED 8/8   (green 3/3 with the reap in place)
  nix build '.#tests'            fails its own checkPhase with the defect in

The third line is a real loss of Linux coverage in THIS assertion and it is
stated in the comment rather than glossed: on an unloaded Linux box no bound
that a correct build survives will catch it, because the burst is not
concurrent there. It costs the SUITE nothing — with the exit-guard reap gone,
PublishedWaiterDoesNotTouchTheRegistryAgain is RED deterministically on both
platforms, and it is that test, not this one, that pins the reap. If this one
ever has to be the detector again, the answer is to pace the provider so the
burst is concurrent on every platform, not to tighten the number.

No behaviour change: the only non-comment edit is the bound.

VERIFIED: nix build '.#tests' green on macOS (289/289, 69.6s) and Linux
(289/289, 78.3s).


(cherry picked from commit f147aed2c6)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-08-10 10:55:37 -03:00

177 lines
9.0 KiB
C++

#ifndef LOGOS_PLAIN_LOGOS_OBJECT_H
#define LOGOS_PLAIN_LOGOS_OBJECT_H
#include "logos_object.h"
#include "rpc_connection.h"
#include <atomic>
#include <condition_variable>
#include <cstdint>
#include <map>
#include <memory>
#include <mutex>
#include <string>
#include <thread>
#include <utility>
#include <vector>
namespace logos::plain {
// -----------------------------------------------------------------------------
// PlainLogosObject — consumer-side LogosObject backed by the plain-C++
// RPC runtime. Identical public shape to LocalLogosObject / RemoteLogosObject
// so LogosAPIConsumer doesn't care which backend it's talking to.
//
// Owns a shared_ptr<RpcConnectionBase>; the transport layer hands the
// connection over after opening the socket. release() stops the connection.
// -----------------------------------------------------------------------------
class PlainLogosObject : public LogosObject, public LogosObjectErrorChannel {
public:
PlainLogosObject(std::string objectName,
std::shared_ptr<RpcConnectionBase> conn);
~PlainLogosObject() override;
QVariant callMethod(const QString& authToken,
const QString& methodName,
const QVariantList& args,
int timeoutMs) override;
void callMethodAsync(const QString& authToken,
const QString& methodName,
const QVariantList& args,
int timeoutMs,
AsyncResultCallback callback) override;
// LogosObjectErrorChannel — the real implementations. The two LogosObject
// entry points above are thin adapters that discard the error, so there is
// exactly ONE call path per direction and the two front doors cannot drift.
QVariant callMethodWithError(const QString& authToken,
const QString& methodName,
const QVariantList& args,
int timeoutMs,
logos::CallError* err) override;
void callMethodAsyncWithError(const QString& authToken,
const QString& methodName,
const QVariantList& args,
int timeoutMs,
AsyncResultErrorCallback callback) override;
bool informModuleToken(const QString& authToken,
const QString& moduleName,
const QString& token,
int timeoutMs) override;
void onEvent(const QString& eventName, EventCallback callback) override;
void disconnectEvents() override;
void emitEvent(const QString& eventName, const QVariantList& data) override;
QJsonArray getMethods() override;
void release() override;
quintptr id() const override;
private:
// Deferred ("multi") completion rendezvous. A multi provider returns a
// pending sentinel (logos::pendingCallKey) from callMethod and later pushes
// the real result as a logos::callCompleteEvent event keyed by callId. We
// subscribe to that event EAGERLY (before any call can defer) so a completion
// racing ahead of the waiter is buffered, then block the caller until the
// matching callId lands. The completion arrives on the connection's IO
// thread; the caller waits on another thread — m_completionMu/Cv bridge them.
void ensureCompletionSub();
// `err` (optional) receives the reason when no completion lands: the
// timeout when the deadline elapses (a deferred call that gives up is a
// timeout like any other, and used to be reported as a null result), or a
// transport error when the object is released out from under the wait.
QVariant awaitCompletion(const QString& callId, int timeoutMs,
const QString& methodName = QString(),
logos::CallError* err = nullptr);
// Ask every in-flight waiter to give up, then join them, then return.
//
// The JOIN is what makes the waiters safe at all: they capture `this` (they
// read m_stopping and call awaitCompletion), and callMethodAsync used to
// DETACH them, so release()/delete racing an in-flight wait was a
// use-after-free. But joining alone means teardown blocks for whatever is
// left of the call's timeout — up to 20s on the protocol default — because
// a waiter has no reason to return early. Hence the stop first: it costs
// one wait slice instead, and a cancelled call still delivers its callback
// exactly once (with an error), because dropping it would turn the stall
// into a permanent hang in the caller awaiting it.
void stopAndJoinWaiters();
// Raise the stop flag and wake anything parked on m_completionCv. Split out
// because the flag has to be published under m_completionMu (see the .cpp).
void stopWaiters();
// Join and drop the waiters that have already FINISHED, so a handle that
// 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. Nothing the
// waiter does may follow it, its own reap least of all.
void publishFinishedWaiter(std::uint64_t id);
std::string m_objectName;
std::shared_ptr<RpcConnectionBase> m_conn;
std::mutex m_mu;
std::vector<std::pair<QString, EventCallback>> m_subs;
std::mutex m_completionMu;
std::condition_variable m_completionCv;
std::map<QString, QVariant> m_completions;
bool m_completionSubscribed = false;
// The waiter registry. KEYED, not a plain vector, because a thread cannot
// join itself: a waiter can therefore never retire its own entry, and a
// vector left only one moment to clear it — teardown — so every completed
// call parked a finished-but-unjoined thread (~one page of resident memory
// 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 both the next spawn and every
// OTHER waiter on its way out join and erase it: see reapFinishedWaiters().
//
// 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. The next call, or teardown, takes those.
//
// That remainder is the size of the last exit batch, NOT a small constant,
// and nothing collects it while the handle stays idle: sampled out to 25.6s
// it does not move. 1-2 after a 2000-call burst on macOS, but up to 402 of
// an 800-call burst on Linux under CPU oversubscription, where a waiter's
// reap can sit in its join loop while the batch behind it publishes. It does
// not scale with the call count, which is the claim; "1-2" was a
// one-platform reading of it.
//
// 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
// at all. This makes the interim honest, it does not replace that.
std::mutex m_waiterMu;
std::map<std::uint64_t, std::thread> m_waiters;
std::vector<std::uint64_t> m_finishedWaiters;
std::uint64_t m_nextWaiterId = 0;
// Read lock-free by the sliced future wait and under m_completionMu by
// awaitCompletion's predicate; written under m_completionMu so the
// condition-variable side cannot miss it. Never cleared — an object that
// has begun tearing down does not come back.
std::atomic<bool> m_stopping{false};
};
} // namespace logos::plain
#endif // LOGOS_PLAIN_LOGOS_OBJECT_H