mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-31 05:51:08 +00:00
Dario Gabriel Lipicar
378d889a1d
The per-call waiters are joinable rather than detached, which is what closed the use-after-free where release() deleted the object under a still-running waiter (4f9d824), and they are interruptible, so teardown no longer waits out the call's timeout (731e579). Both stay. What neither did was retire a waiter that had FINISHED: m_waiters was only ever swap()ped, in stopAndJoinWaiters(), so an exited-but-unjoined std::thread — whose stack and pthread struct are not reclaimed until somebody joins it — stayed parked for the lifetime of the handle. Measured against a live PlainTransportHost over TCP, every call completing normally, one handle held throughout, before: 10000 calls m_waiters 300 -> 10300 rss +156.56 MiB 16417 B/call 30000 calls m_waiters 300 -> 30300 rss +469.28 MiB 16403 B/call and the same through the production C ABI — one lp_client, N lp_invoke_async — at +156.53 MiB. That path is why this matters: LogosAPIConsumer caches ONE handle per module and reuses it for every async call, releasing it only on eviction or teardown (cpp/logos_api_consumer.cpp:129 and :207), so a long-lived module leaks per lp_invoke_async. The ~16KB constant is one page on this 16KiB-page arm64 and will be smaller elsewhere; the UNBOUNDEDNESS is the platform-independent part, and follows from m_waiters.size() rising 1:1 with completed calls and only ever falling in teardown. Attribution: the retention arrived with the join in4f9d824, not with731e579— but731e579is what makes the join permanent. The registry is now KEYED, because a thread cannot join itself and so a waiter can never retire its own entry. Each waiter publishes its id as its FINAL act (a scope guard declared first, so it destructs last, covering all four exit paths), and the next spawn — plus teardown — joins those ids and erases them. Joining a thread that has already returned is a couple of syscalls. Same probe, same workload, after: 10000 calls m_waiters 15 -> 16 rss +0.08 MiB 8 B/call 30000 calls m_waiters 16 -> 16 rss +0.06 MiB 2 B/call 10000 calls via lp_invoke_async rss +0.09 MiB 10 B/call Retention is now bounded by the waiters that finish after the LAST spawn, i.e. by peak in-flight concurrency — 16 at the in-flight window above, and exactly 1 when calls are issued sequentially — instead of by call count. THE DEADLOCK THIS SHAPE INVITES is a reaper that joins while holding m_waiterMu, against a waiter blocked on m_waiterMu trying to publish. It is avoided by construction rather than by argument: nothing is joined with a lock held, in the reaper or in teardown, whatever a waiter does on its way out. Proven by building the naive variant that does join under the lock — the new hammer wedges it, with the main thread in reapFinishedWaiters -> pthread_join and a waiter in publishFinishedWaiter -> mutex wait, and the test's watchdog names the cause instead of letting CI hang. Teardown's guarantee is restated rather than weakened. It is not "every waiter has been joined by the time stopAndJoinWaiters() returns" — a waiter a concurrent reaper is mid-join on is no longer in the map — but the thing that guarantee was ever for: NO WAITER TOUCHES THE OBJECT AFTER IT RETURNS. An entry leaves m_waiters only once its thread has published, and publishing is that thread's last access. 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 that. Two more things review turned up, folded in here: * The two wait sites resolved stop-vs-result in OPPOSITE directions. waitForResult tested the stop flag BEFORE polling, so an already-ready future was still reported as transport_error, while awaitCompletion deliberately preferred a completion that had landed — and both were commented as intentional. One rule now, applied to both: AN ANSWER ALREADY IN HAND BEATS A CONCURRENT STOP, and the stop only decides what happens when there is nothing to hand over. The callback fires either way (postToQtEventLoop copies everything it delivers), so the only thing a stop can change is what the callback SAYS — and manufacturing transport_error while the true answer sits in the future reports a failure that did not happen, to callers that re-acquire, retry and log on that code. Preferring the answer costs nothing, since it is already there: the flag is still checked before every sleep, so the teardown-latency bound is unchanged. * CORRECTION to 731e579's message, which claimed it "closes the registration window" where a call arriving after the stop would never be joined. That branch is unreachable in defined behaviour: m_stopping is raised only by teardown, so any thread that can read it inside callMethodAsyncWithError is already calling a method on an object whose destructor is running — the load is itself the use-after-free, reproduced as a SIGSEGV on that commit and on its parent alike, and nothing inside that function can repair it. The guard is harmless and stays (one predictable branch, and it fails safe with one callback), but its comment now says what it is instead of claiming a fix it does not make. Verified by running, with every check first shown to FAIL on unfixed code: * Retention: the probe above, plus a committed regression test that reads m_waiters out of the live object through the explicit-instantiation access hole ([temp.spec] does not check access on an explicit instantiation's template arguments) — so the code under test keeps its private state, with no friend, no test-only accessor and no `#define private public`. 200 sequential completed calls keep 1 waiter; without pruning they keep 200. * Exactly-once on all four paths — normal completion, timeout, cancellation and the deferred-completion (pending-sentinel) arm — counted PER CALL so a dropped one and a doubled one cannot cancel out, plus the 60-round release-during-call race. Shown to catch a cancelled path that returns silently (3 failures) rather than delivering. * Teardown latency unchanged from731e579: 10-17ms with an in-flight 8000ms call and 0-1ms mid-defer, against 15ms / 1ms on that commit. * The UAF stays closed: 11 teardown + reaping tests clean under macOS Guard Malloc (ASan/TSan remain unusable on this toolchain). * Full suite 281/281 twice, `nix build .#tests` green (281/281 in the sandbox), CallErrorAfterAcquireTest hammered 40x clean.