mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-27 12:01:15 +00:00
Dario LipicarandClaude Opus 5
7be3a6b856
* perf(plain): fold the per-call waiter thread into a call object with its own clock An async call on the plain transport used to be an OS thread whose entire job was to be blockable: std::future cannot be waited on with a deadline AND a cancel, so the waiter polled it in 25ms slices, parked on a condition variable for the deferred half, and delivered. Three costs came with that — one thread per pending RPC, a 25ms floor on teardown, and a registry-plus-reaping protocol to stop finished threads accumulating, because a thread cannot join itself. The TODO in callMethodAsyncWithError has said to fold it away since it was written. A call is now a shared_ptr<AsyncCall>: state that the reply (delivered as a handler rather than parked in a promise), a deadline, and cancellation race to finish. Nothing captures `this`. Handlers hold a shared_ptr to their AsyncCall and a weak_ptr to CallState, so "no handler touches a destroyed object" is true by construction rather than by a barrier, and the join is replaced by ownership. postToQtEventLoop is kept verbatim as the re-entrancy firebreak: all four completion sites route through it, so no user callback ever runs on an Asio stack. Measured against pristine0f26ffd, same probe compiled into both: * 32 calls parked in a provider that will not answer: threads 5 -> 37 on master, 6 -> 6 here (the 6th is the deadline clock below, not per-call); * release() with those 32 in flight: 26-29ms -> 0ms; * 24,000 calls resolved by their deadline and never answered: 8.5MB of resident memory on master, 353 bytes per call growing strictly linearly, against 0.95MB here that has stopped growing by 3,000 calls. THE DEADLINE GETS ITS OWN THREAD, and that is the one decision worth arguing with. Hanging the per-call timer off the connection's strand is the obvious move and it has a real regression: IoContextPool runs ONE thread for the whole process and this transport delivers user onEvent callbacks INLINE on it, so an event handler that calls another module — ordinary module code — holds every deadline in the process. Measured on that design: an onEvent handler making a 2000ms call delayed a 200ms deadline on a DIFFERENT connection to 2003ms, and a handler that never returns meant the deadline never fired at all. A second io thread does not fix it (user handlers are unbounded, so N blocked handlers need N+1 threads); moving inline event delivery off the strand is a much larger change to event ordering for every consumer; a Qt timer is strictly worse, because a synchronous call from the Qt thread blocks that loop too. So: DeadlineService, one thread process-wide, doing nothing but arming, cancelling and firing timers. Both shapes are back to 200ms and 250ms, and are pinned. Two things closed on the way, neither of them inherited: * RpcConnection::cancelPending(). m_pendingCalls was emptied only by a decoded reply and by fail()'s sweep, so a call resolved by its DEADLINE left its registration there for the life of the connection — which outlives every handle it hands out. That was true of the promise before this change too; the fold would have made the orphan bigger, so it is closed rather than passed on. The sync call path and getMethods withdraw theirs as well. * A sentinel that arrives after its own deadline used to be filed under CallState::deferred by a reply handler that had not noticed the call was already resolved. deliver() therefore leaves the registries BEFORE the exactly-once gate, not after it. TESTS. tests/protocol/test_iofold.cpp is the evidence, and its detectors are validated by an explicit inverted build rather than asserted: cmake -S tests -B build-broken -DLOGOS_PROTOCOL_DETECTOR_INVERSIONS=ON which removes the exactly-once CAS and puts the deadline back on the shared io_context. In that build the deadline tests fail at 2002ms and never-fires respectively, and the release-vs-replies race reports 6-16 double deliveries per 10,000 calls. Finding a race wide enough to be a RELIABLE exactly-once detector took three attempts and the two rejected candidates are documented in the file: the plain outcomes are weak detectors (one resolver, one deliver()), and release-against-a-single-completion caught one double in seven runs. Teardown against a burst of arriving replies is the one that works, because teardown snapshots the whole in-flight map and then delivers with the lock released. test_plain_waiter_publish_is_last.cpp is DELETED. It pinned exactly one rule — publishFinishedWaiter() is a waiter thread's last access to the object, which was the only reason stopAndJoinWaiters() could return while a reaper was still mid-join. There are no waiter threads, no reaper and no publish list, so there is no ordering left to pin; the property it protected is now structural. test_plain_waiter_reaping.cpp is retargeted at CallState::inflight, keeping its claims and losing its mechanism. The four guarantees from #41 all re-measured on this branch: no thread growth with in-flight calls, teardown 0ms with 32 outstanding, exactly one callback on normal/deferred/timeout/cancellation counted per call over 10,000 calls including a release race, and retention final 0 on both registries. Guard Malloc clean over the lifetime suites (27 tests), with the completion-subscription detector still faulting 3/3 on pristine master. ctest 296/296 (287 before, minus 3 deleted, plus 12); nix build .#tests 296/296. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(plain): say what cancelPending() actually guarantees, and pin it rpc_connection.h claimed that "a caller that gives up before the reply arrives calls cancelPending() and is never called back at all". It is not true. dispatchIncoming copies the handler out of m_pendingCalls under m_mu and invokes it with the mutex RELEASED, so a cancelPending() landing in that gap erases an entry that is already gone and returns having stopped nothing — the handler then runs to completion, after cancelPending() has returned. THE CODE IS FINE; THE COMMENT WAS NOT, and the distinction it was blurring is the load-bearing one: * at-most-once INVOCATION of a registered handler is this layer's, and comes from the extract-and-erase under m_mu — three contenders (dispatchIncoming, fail()'s sweep, cancelPending) and only one can have it; * exactly-once DELIVERY to the user is NOT this layer's. It is AsyncCall::deliver()'s CAS, and nothing else. Both callers of sendCallAsync() were checked, because a non-idempotent one would have made this a bug rather than a comment. PlainLogosObject funnels every outcome into deliver(). RpcConnection::sendCall()'s promise handler has no CAS and needs none: only one contender ever reaches it, and fulfilling a future its caller has already walked away from is a no-op. Same for the two cancelPending() callers that are not deliver() — the sync callMethodWithError timeout and getMethods(). Three comment sites corrected (ResultHandler, dispatchIncoming's Result arm, cancelPending's own contract) and the claims moved out of prose into tests/protocol/test_plain_cancel_pending_race.cpp, which BUILDS the interleaving instead of racing for it: a stub connection reproduces dispatchIncoming's extract-then-invoke and lets the test stand between the halves. Four tests — the callback that fires after cancelPending() returns, the extracted reply racing teardown (exactly one delivery), the promise-shaped handler in the same gap, and a real RpcConnection pair proving the half of the old comment that IS true. The exactly-once one goes RED under -DLOGOS_PROTOCOL_DETECTOR_INVERSIONS=ON: 2 deliveries for 1 call, deterministically rather than probabilistically. Six tests now go red in that build, listed in tests/protocol/CMakeLists.txt. Full suite 302/302. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(plain): take the fold's detector inversions out of the transport too Three more compiled-in alternative implementations, same anti-pattern as the one the base branch just lost: production source holding a second, deliberately wrong version of its own contract so the suite could be built once with the mechanism removed. * deadlineContext() had LOGOS_PLAIN_DETECTOR_BREAK_DEADLINE_ISOLATION returning IoContextPool::shared().ioContext() — the rejected design DeadlineService exists to avoid. Kept DeadlineService::shared() .context(); the io_context_pool.h include the fold added for that branch alone goes with it. * AsyncCall::claim() had LOGOS_PLAIN_DETECTOR_BREAK_ONCE storing `delivered` and returning true unconditionally. Kept the CAS. * AsyncCall::takeCallback() had the same macro returning a COPY of the callback. Kept the swap. And the CMake option that defined all three, whose surviving content — which six tests are real detectors, and that the per-path exactly-once tests are PINS rather than detectors because a call resolved once calls deliver() once whatever guards it — moved into the note that replaces it. The comments now describe the validation that actually happened: a local edit in a throwaway checkout, with the numbers each run produced. The sub-order detector needs no edit at all, since it goes red on pristine master. 302 tests pass, unchanged in count: no test deleted or weakened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(iofold): count the provider's completion workers so none outlives the proxy Fixes the Linux SIGSEGV (exit 139) in the "Run protocol tests" step. `OmniProvider::defer` is a "multi" provider: it answers with a pending sentinel and pushes the real completion later from a worker of its own. That worker calls the EventCallback ModuleProxy handed the provider, and that listener captures `this` RAW (module_proxy.cpp) — its first act is `QMetaObject::invokeMethod(this, …, Qt::QueuedConnection)`, which dereferences the QObject. The worker was spawned DETACHED, so nothing proved it had finished, and `~LiveHost` runs `delete m_proxy` a couple of milliseconds after the last spawn. Nothing keeps the two apart. A round of ReleaseRacingAnInFlightCompletionIsSafe ends when the CALLER is answered, and a release()d call is answered by teardown — so the round can be over before the provider has run at all, and the proxy's thread is still working through a backlog of `defer` calls while ~LiveHost is already tearing down. Two to five workers were standing in the same frame at the moment of the fault: Thread "QThread" received signal SIGSEGV QObject::thread() const QMetaObject::invokeMethodImpl(QObject*, …) ModuleProxy::ModuleProxy(...)::<lambda(const QString&, const QVariantList&)> module_proxy.cpp:37 std::function<void(const QString&, const QList<QVariant>&)>::operator() OmniProvider::callMethod(...)::<lambda()> test_iofold.cpp, in defer std::thread::_State_impl<…>::_M_run() WHY ONLY THE WHOLE-BINARY RUN. The corpse is left by the test that spawned the worker and lands in whichever test runs NEXT — in CI, always ReleaseFromInsideAnIoThreadEventCallbackDoesNotWedge, which follows the 300-round ReleaseRacingAnInFlightCompletionIsSafe and its 300 `defer` calls. Under ctest every test is its own process, so the worker dies with the process that owned the proxy and there is nothing left to fault: `nix build '.#tests'` is green on the unfixed tree, which is exactly how this hid. Measured on Linux (aarch64, Qt 6.9.2, gcc 14.3, `nix build '.#tests'` artifact, QT_QPA_PLATFORM=offscreen, five concurrent copies): tree IoFoldTest.* whole binary master (03842db) n/a (no test) 0/10 feat/plain-async-io-fold 6/50 (12%) 2/20 (10%) + harness-host-thread-affinity 10/50 (20%) 5/20 (25%) this commit, on either tree 0/50 0/20 So it is latent HERE and merely widened by destroying the fixtures' host on the proxy's thread: that order lets the queued `defer` backlog RUN instead of discarding it with QThread::quit(), which is why the rate roughly doubles. The defect and the fix both belong to this commit's tree. macOS never opens the window — 3/3 clean whole-binary runs on the unfixed tree, and 4/4 clean under Guard Malloc (which unmaps the freed page, so a late worker would fault every time), which is why that half of the matrix stayed green. A COUNT, NOT A JOIN. Joinable workers would hold their 8MB stacks until reaped — 400 outstanding in NormalAndDeferredCompletionsDeliverExactlyOnceAtVolume — and reaping from the dispatch thread would block the very thread `defer` exists to free. So they stay detached and the provider counts them, with the decrement and its notify under one mutex so a drain() woken by it cannot return before the worker has released that mutex. ~LiveHost drains AFTER `m_thread->wait()`: the proxy's event loop has stopped, so no queued call can reach the provider any more and the worker set is FINAL — before that point a drain could pass and the backlog spawn more. ~OmniProvider drains too, because a worker's last act touches one of its members. Verification, all on the fixed tree: * `nix build '.#tests'` — 402/402, Linux and macOS (173s on macOS). * 20 whole-binary Linux runs: 402/402 every time. * Exactly-once, on the release-race shape rather than the per-path pins: 200,000 calls released mid-burst across those 20 runs (20 rounds x 500 each) reported DOUBLE deliveries=0 dropped=0, and all 20 runs of ReleaseRacingAnInFlightCompletionIsSafe were 300/300 with 0 doubles. * Teardown stays fast: `release()` 0-2ms with 32 calls in flight, and the added wait is on the FIXTURE, not on release() — it delays `delete m_proxy` by however long a completion worker still had to sleep (<=2.4ms here). * No production code touched, so "no user callback inline on an io thread" and the deadline guarantees are unchanged. Not touched: test_plain_completion_sub_order.cpp (InstantMultiModule) and test_concurrent_dispatch.cpp spawn the same detached completion worker, and read the callback member through a captured `this` on top of it. Neither has been observed to fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>