mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-27 12:01:15 +00:00
Dario LipicarandClaude Opus 5
4a20e99260
* fix(plain): the completion subscription must not outlive the object it points at
PlainLogosObject::ensureCompletionSub() registered the deferred-completion
handler with raw `this` captured. That handler is stored in the RpcConnection,
which is SHARED by every handle the connection hands out and outlives all of
them — release() says so itself, and ends in `delete this`. So a completion
event arriving across a release() ran a handler holding a dangling pointer, on
the io thread, on a path nothing joins: #41's waiter JOIN covers the per-call
waiter threads and nothing else.
The unsubscribe release() sends is real — RpcConnection::sendUnsubscribe erases
the entry under the connection's mutex — but it cannot close this, because
dispatchIncoming copies the handler out under that mutex and then invokes it
with the mutex dropped. An erase racing an already-copied handler changes
nothing about the invocation in flight.
Reproduced, not assumed. tests/protocol/test_plain_completion_sub_lifetime.cpp
widens the window with a large completion payload (the conversion between the
copy and the handler's first touch of the object) and aims release() into it
using a wildcard subscriber as a clock. On master:
* SIGSEGV under macOS Guard Malloc, 3/3 runs, faulting in
pthread_mutex_lock <- std::mutex::lock <- ensureCompletionSub()::$_0 <-
onEvent()::$_0 <- dispatchIncoming <- doRead <- IoContextPool's thread;
* without a detector, 4/5 runs die differently and just as fatally: the freed
mutex makes pthread_mutex_lock return EINVAL, std::mutex::lock() throws, and
the exception unwinds into doRead()'s catch, which fail()s the whole
connection. That is the per-round isConnected() assertion in the test.
The fix moves the rendezvous (mutex, condvar, completions map) into a
shared_ptr-held block and hands the handler a weak_ptr, so "no handler touches
a destroyed object" holds by construction: a handler that locks it keeps it
alive for one callback, one that cannot lock it does nothing. Nothing else in
the object was reachable from that handler, which is what keeps this to two
files; rpc_connection.h is untouched.
Verified after the fix: repro clean 10/10 plain and 3/3 under Guard Malloc,
with the same cadence and 24/24 releases still landing inside a dispatch — the
window is still exercised, it is just no longer a use-after-free. The control
(same storm, nothing released) is clean under the same detector on both sides,
so the detector is not objecting to the load. All four #41 guarantees re-measured
and unchanged: waiters joined (60/60 rounds), teardown 10-22ms against master's
6-27ms, exactly one callback on all four outcomes, registry final=1 after 200 /
600 / 800 / 1600 calls. Full suite 287/287, including nix build .#tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): a concurrent first caller must WAIT for the completion subscription, not just see the flag
ensureCompletionSub() raised m_completionSubscribed under the rendezvous mutex
and then RELEASED that mutex before subscribing. Two threads entering
callMethod() on the same fresh object is enough: the second reads "subscribed",
builds its Call and puts it on the wire while the first has not enqueued the
Subscribe frame yet. A "multi" provider that answers such a call quickly emits
its completion into a subscription the host has not registered —
PlainTransportHost::fanOutEvent finds no sink for that connection and DROPS it —
and the caller waits out its whole timeout for a result that was computed and
thrown away.
A LOST COMPLETION, NOT A CRASH, which is why it survived: the failure looks like
a slow provider, arrives seconds after the code that caused it, and leaves
nothing behind.
PRE-EXISTING, not introduced by this branch: pristine master has the identical
flag-then-subscribe shape and reproduces at 42/400 two-thread first-call rounds
(this branch before the fix: 28/400; through the real host stack: 10/600 calls).
It ships here, as its own commit, because it is four lines in the very function
this PR rewrites and in the same subscription this PR is about.
The fix is std::call_once plus a release/acquire fast path. Serializing is the
whole of it: a second caller blocks until the first has both registered the
client-side callback and enqueued the Subscribe frame, and asio then keeps the
two posts in that order because the mutex supplies the happens-before edge its
strand guarantee is conditioned on.
Rejected: holding the rendezvous mutex across the subscribe (works, but makes
the io thread's completion handler wait on the connection's write path — that
mutex exists to hand a completion over, not to gate I/O); subscribing eagerly in
the constructor (kills the race outright but costs a Subscribe frame and a host
sink per handle, deferred call or not).
tests/protocol/test_plain_completion_sub_order.cpp pins both halves — the wire
order, observed at a provider that stamps every frame it receives, and the
consequence through PlainTransportHost with nothing instrumented at all. Both go
RED under -DLOGOS_PROTOCOL_DETECTOR_INVERSIONS=ON, which restores the pre-fix
shape: 5 of 5 broken runs failed (25-37 dropped completions per 250 rounds), 8
of 8 fixed runs were clean. Full suite 289/289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): validate the detectors against master, not a compiled-in inversion
The transport carried a second ensureCompletionSub() behind
LOGOS_PLAIN_DETECTOR_BREAK_SUB_ORDER: the pre-fix racy shape, reachable
from a -DLOGOS_PROTOCOL_DETECTOR_INVERSIONS=ON configure of the tests
tree, so the ordering tests could be shown to fail. That is the same
anti-pattern as the getenv() probes an earlier draft carried, wearing a
build flag instead — production source keeping a deliberately wrong
implementation of its own contract — and it does not belong in the PR.
Both detectors are validated by the stronger check anyway: this file
compiles unmodified on master, which still raises the flag under the
rendezvous mutex and drops it before subscribing, and still captures raw
`this` in the completion handler. Numbers now in the comments are from
that run, not from the synthetic build:
sub-order raw wire 18/26/27/28 of 250 rounds inverted, dropped and
timed out, four runs
sub-order real stack 6 to 10 of 500 calls timed out
sub-lifetime RED in 11 of 12 solo runs, connection dying at
round 6 in 9 of them
The 400/600-round figures the comments quoted were also stale: 66e0153
moved both tests to 250 rounds.
No test is deleted or weakened. The std::call_once fix and the weak_ptr
handler capture are untouched; only the alternative implementation and
the CMake option that reached it are gone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): make the burst-drain experiment a burst on both platforms
BurstThatGoesIdleDrainsWithoutAnotherCall claimed to detect "reaping only
happens on the spawn path". It did not, on Linux, at any bound — because the
experiment was not a concurrent burst there.
"Issue 800 calls in a loop and hope they overlap" is a race between two rates:
spawning a std::thread, and a loopback RPC coming back. On macOS the first is
much cheaper and the burst is real (544-612 of 800 still outstanding when the
last call goes out). On Linux they are comparable, so the burst completes WHILE
IT IS BEING ISSUED and the spawn-path reaper — the very reaper this test is
supposed to be doing without — collects it: 0-237 outstanding, median 85, and
one run in 25 with the entire burst answered before the last call was issued.
Both arms then drew from overlapping distributions and no number could separate
them. 400 scored 25/25 on macOS and 0/40 on an idle Linux box; the 8 before it
failed correct code 96 runs in 160.
So gate the provider instead of arbitrating the race. Every call in the burst
invokes `gate`, which parks; dispatch is single-threaded, so the first arrival
holds up all 800 and not one reply exists until the test releases them. The
burst's own precondition is now asserted rather than assumed, off the provider's
reply counter and the registry size: 800/800 in flight, 0 replies, in all 760
runs of both arms at every load level.
The drain is then PACED, 16 at a time, which is the other half. Releasing all
800 at once replaces one scheduling artefact with another: a reaper that
collects a large batch sits in its join loop while everyone behind it publishes,
so on 6-core Linux correct code leaves 2-389 (macOS: 1). That is a real property
of the exit guard, not a defect, and it now says so in plain_logos_object.h —
but it is useless as a detector, since the defect is only ~2x it. Paced, the
residue is the last step's exit batch: measured over 380 runs, worst 1 on macOS
and 7 on Linux, and the unloaded Linux cell is the worst one.
The bound is therefore a constant again, and stated against the release step
rather than the burst, because that is what bounds the quantity: 4 * kRelease =
64. 9.1x above the worst correct-code value seen, 12.5x below the defect. With
the exit-guard reap removed the residue is 800 — kBurst exactly, not "several
hundred" — in every one of 380 runs on both platforms at every load level,
because nothing is left that can remove an entry. THE DEFECT IS NOW CAUGHT ON
LINUX: 200/200, where the previous bound caught 0/40.
Cannot hang, constructed three ways. The gate is bounded and opens itself on the
way out, so a release that never happens costs one budget and fails naming the
gate (20.2s, measured) instead of wedging the host 800 times over; a drain step
that stalls breaks the loop instead of spending fifty budgets; and a bail-out
with the burst still in flight now releases and pumps through a scope guard
before Deliveries dies — without it that path is a SIGABRT on a destroyed mutex
(3/3), which is a crash where a verdict is wanted.
800 live waiter threads is the new peak, up from ~150 on Linux. Lazily faulted:
peak RSS 29MiB Linux / 32MiB macOS.
nix build .#tests: 289/289 on both platforms.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>