mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-30 21:41:10 +00:00
378d889retired 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 on378d889, 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 against378d889where 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 as378d889reported. 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 to378d889, 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. On378d889the 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 from378d889: 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>
170 lines
8.5 KiB
C++
170 lines
8.5 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 (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
|
|
// 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
|