mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-30 21:41:10 +00:00
Dario Gabriel LipicarandClaude Opus 5
8f0c60fe17
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>