* fix(plain): give every handle its own event subscription on the shared connection
RpcConnection::m_eventCallbacks was keyed by (object, eventName) and ASSIGNED.
One RpcConnection is shared by every PlainLogosObject a PlainTransportConnection
hands out, and requestObject() mints a fresh handle per acquire, so the second
handle to subscribe to the same event on the same module silently took the first
one's channel — including the deferred ("multi") completion channel every handle
subscribes to on its first call.
No concurrency is needed to reach it. Measured on cf1b9b0: handle A's deferred
call answers 8/8 in 0 ms alone, and 0/8 in 1505 ms (timeout, INVALID) once a
second handle merely exists. Identical on master.
THE WIRE DOES NOT MOVE. Subscribe/Unsubscribe still carry (object, event) and
nothing else. The host keeps ONE sink per (object, event, connection) — the right
model, since every sink for a connection is the same "write this frame back down
that socket" — and the CONSUMER, the only end that knows how many of its own
handles want an event, does the demultiplexing: a list of registrations per key,
a subscription id to withdraw one of them, and an Unsubscribe frame only when the
last local registration for the pair is gone, because that frame is
connection-wide.
Also here, because the consumer-side fan-out makes it observable as a doubled
delivery: PlainTransportHost::fanOutEvent sent a connection subscribed both by
name and by wildcard two copies of the same event. One copy per connection now.
And the parked-completion staging area is gated. It exists for one ordering — a
"multi" worker that finishes before the sentinel it answers has been written, so
the completion event overtakes its own Result — and a completion arriving that
early is not attributable to a handle at all. Every handle now sees every
completion on the object, so parking is allowed only while this handle has a call
outstanding, the map is emptied the instant none is, and it is capped at 512 with
oldest-first eviction. Pre-fix the same leak existed with one victim instead of N:
the handle that had stolen the channel parked every other handle's completions and
never claimed one.
Detectors: nine tests in test_plain_event_sub_sharing.cpp, all validated RED on
cf1b9b0 and on master and green here, including both mixed-version directions (a
new consumer against a verbatim pre-fix host, and an old consumer's frame sequence
against the new host over a raw socket).
── REBASED ONTO #50, THE FIRST REPLAY IN THIS STACK WITH A REAL PRODUCTION
CONFLICT (and then onto #51, which lands cleanly) ──────────────────────────
the object (a live-reference count with deferred destruction) and to give async
callbacks somewhere to land in a Qt-free process (a never-destroyed
DeliveryService). Both of those and both of this commit's changes are kept; the
resolution picks no side anywhere.
* plain_logos_object.cpp, hunk 1 — #50 replaced postToQtEventLoop's comment
header with DeliveryService; this commit inserted SyncCallScope immediately
above it. The insert point survives, the replaced header does not:
SyncCallScope now sits above #50's DeliveryService block.
* plain_logos_object.cpp, hunk 2 — #50 split disconnectEvents() into a guarded
entry point plus an unguarded disconnectEventsImpl() (release() and the
destructor must not take a reference to what they are destroying); this
commit changed that body's local from (name, callback) pairs to subscription
ids. Kept as #50's split with this commit's body. onEvent() auto-merged the
same way: #50's EntryGuard declared FIRST, then the m_mu-across-the-subscribe
body from here.
* test_sync_call_release_race.cpp — not a text conflict and not visible to git:
#50's StalledConnection double implements RpcConnectionBase, and this commit
changes that interface, so it became abstract and the file stopped compiling.
Its stub now returns a DISTINCT id per subscribe, so it cannot hide a bug
that withdraws the wrong registration.
* tests/protocol/CMakeLists.txt — both detector lists are additive and both are
in. The prose is NOT concatenated: an earlier replay on this stack had
grafted a duplicated fragment ("per-path test as evidence the gate is there.
Nor is") into the middle of a paragraph, and the sentence it belonged to is
now where it was meant to go, at the end of the "what is NOT on that list"
paragraph. #50's "the CAS is TWO gates" correction is untouched. #51's own
additions to the same file merged without a conflict on the second replay.
gives the five LiveHost fixtures a teardown that destroys the host on the thread
that emits into it, and the host in test_plain_event_sub_sharing.cpp is not that
shape — its ModuleProxy stays on the test thread, so there is no worker to race.
RE-VERIFIED BY RUNNING, on macOS arm64, Debug, all after the rebase:
* THIS FIX still fixes the bug on the NEW master. The ten
PlainEventSubSharingTest cases compile unmodified on 5be3a84 and nine go RED
there: handle A alone 8/8 answered at 0 ms avg, handle A once B exists 0/8 at
1503 ms avg with 8 timeouts, and 0/4 with 4 timeouts through the shipping
host. On this commit: 8/8 and 4/4, 0 ms avg, 0 timeouts. So neither #50 nor
#51 caused or masked this defect, and this branch is not a no-op.
* #50's FIX still works through this change. SyncCallReleaseRaceTest 8/8 green
(release() returns in 0 ms with a call parked, destroyed=1 only after the
caller leaves; 400 release-wake races, one destruction each), the whole no-Qt
binary 7/7 plus the after-main() probe (delivered=1, off the issuing thread,
exit 0), and its mechanisms are still in the source: m_liveRefs with the
EntryGuard reference-first/reference-last ordering, release() dropping the
owner's reference instead of `delete this`, DeliveryService `new`-ed with
`~DeliveryService() = delete` and a detached thread, and NO m_conn.reset() in
release().
* EXACTLY-ONCE, in the release-race shape rather than the per-path pins. Qt
vehicle: 20 rounds x 500 calls released mid-burst, answered-by-reply=1034,
cancelled-by-teardown=8966, 0 doubles, 0 dropped. No-Qt twin: 1130 / 8870, 0
and 0. Both resolvers live in both. Re-validated as a DETECTOR on this merged
tree by removing BOTH gates (claim()'s CAS and takeCallback()'s swap) in a
throwaway build: 19 doubles Qt, 13 doubles no-Qt, both FAIL, while the
per-path exactly-once tests stay green — which is the difference the
CMakeLists note describes.
* NO USE-AFTER-FREE. Guard Malloc clean over SyncCallReleaseRace, IoFold,
PlainObjectTeardown, PlainCompletionSubLifetime, PlainCancelPendingRace,
PlainWaiterReaping, PlainHostEventTeardown, PlainEventSubSharing and
PlainParkedCompletionGate (52/52), and over the whole no-Qt binary (7/7,
exit 0).
* TEARDOWN AND DEADLINES. release() with 32 calls in flight: 0 ms. release()
after 100 reaped calls: 0 ms. A 200 ms deadline fires at 200 ms while an
onEvent handler holds the io thread for 2001 ms; a 250 ms deadline fires at
250 ms with the io thread blocked forever; 12 idle 200 ms deadlines
min=200 median=200 max=202 ms.
* NO USER CALLBACK INLINE ON AN IO THREAD: no-Qt replies 300/300 with
on-caller-thread=0 and delivery-thread!=io-thread=1; cancellations 20/20 with
inside-release=0 and on-releasing-thread=0.
* Full suite 433/433 (was 421 on master; this adds 12), and `nix build .#tests`
433/433.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): make the subscription detectors barrier-driven instead of timed
Every "wait a bit then count" in the new tests is now an in-band round trip: a
Methods request travels the same socket and is dispatched on the same strand as
everything written before it, so its reply proves the earlier Subscribe /
Unsubscribe frames have been applied AND that the Event frames the peer wrote
before answering have been dispatched. The audits are exact counts rather than
polls with a timeout, which is what they have to be on a loaded CI runner — a
short sleep there fails the test rather than skipping it.
Still 9 of 10 RED on cf1b9b0 and on master, with the teardown pin green; the set
now runs in 9s instead of 110s.
* test(plain): drop the poll helper the barrier replaced
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): deliver async callbacks in a Qt-free host, and make release()-racing-a-call diagnosable
Two pre-existing defects in the plain transport's async surface. Both are
older than #45/#46 and neither is caused by the io_context fold; the fold is
just what this is stacked on.
DEFECT 5 — the async surface promised exactly-once and delivered ZERO in a
Qt-free host. Every completion went through one hop, and the hop was:
QCoreApplication* app = QCoreApplication::instance();
if (!app) return; // <- the callback, dropped
In a Qt host that branch only fires at shutdown, which is why it read as a
reasonable guard. In a process that never had a QCoreApplication — the
deployment the plain transport exists for — it fires for EVERY call, forever,
on all four resolvers (reply, deferred completion, deadline, cancellation).
Not an error, not a timeout: silence, which turns a bounded call into an
unbounded wait in every caller that awaits it, including lp_invoke_async and
every generated async wrapper.
Fixed with a dedicated DELIVERY THREAD, used only when the process has no Qt
loop. NOT inline on the completing stack: inline delivery on an Asio read
handler is the re-entrancy class that already cost this codebase a SIGSEGV
(deferred-multi completion on the QtRO read stack), so a fix that delivers by
removing the hop is not a fix. NOT the deadline thread either — user callbacks
there would make every deadline in the process hostage to user code, which is
exactly the coupling DeadlineService was extracted to prevent.
The Qt-loop check LATCHES, so Qt hosts see no behavioural difference at all:
instance() also goes null inside ~QCoreApplication, and module teardown after
the application is gone is what static-destruction ordering produces — with
stopAndCancelCalls() handing every in-flight call a cancellation callback at
exactly that moment. Running user code on a side thread into half-destroyed
module state would be a NEW failure mode introduced by a bug-fix change, so a
process that has ever been seen with an event loop keeps the old shutdown
behaviour. logos_object.h now states that residue instead of glossing it.
DEFECT 3 — release() racing a call on another thread. NOT FIXED, because it
cannot be, and the honest answer is a contract plus a detector.
release() ends in `delete this`, so a synchronous call parked in its future
wait dereferences freed memory when it comes back. Reproduced deterministically
on master (exit 139 under Guard Malloc, 3/3) and on cf1b9b0 (exit 139 with AND
without Guard Malloc, 3/3), faulting in callMethodWithError one line after the
wait.
It is not fixable from inside the object: every mechanism that could make the
racing call safe — a refcount, a flag, a lock, an epoch — is a MEMBER, so the
racing thread's first act would be to read it out of storage that has just been
freed. There is no synchronising with a destruction you can only learn about by
reading the destroyed object. Three alternatives were considered and rejected,
each for a stated reason (an atomic alive-flag is check-then-use on freed
memory; a blocking release() breaks the fast-teardown guarantee and deadlocks
in the shipped reentrant shape; an immortal forwarding handle works but trades
the crash for permanent retention proportional to requestObject count, in a
transport whose two preceding changes were spent proving retention does not
grow with call count — and would fix one of four transports). The reasoning is
in the note over PlainLogosObject::release().
So: the contract is stated (logos_object.h, plain_logos_object.h), and the
object counts entries into its public methods and REPORTS when release() or
the destructor finds the count non-zero — aborting in debug builds. The misuse
becomes a named diagnostic at the line that committed it instead of a SIGSEGV
somewhere else. It is a diagnostic, not a rescue, and it is deliberately biased
to under-report rather than ever accuse a correct program.
EVIDENCE, all by running:
* Defect 5: six detectors in a NEW binary (protocol_noqt_tests) that never
constructs a QCoreApplication — the state protocol_tests can never reach,
since its main() constructs one first. All six red on cf1b9b0 (0/300
replies, 0/40 deferred, 0/20 deadlines, 0/20 cancellations delivered),
all six green after, including under Guard Malloc.
* Defect 3: a death test red on BOTH pre-fix trees, 3/3 each, with and
without Guard Malloc ("died but not with expected error"), green after.
Its three companion tests prove the detector never fires on a correct
program, and were themselves validated by deleting the decrement from
EntryGuard's destructor in a throwaway build: all three then abort.
* Exactly-once still holds via the release-race shape — the only one that
detects a broken gate — on both delivery vehicles: 20 rounds x 500 calls
released mid-burst, 0 double deliveries, 0 dropped, with both resolvers
live, under Guard Malloc too.
* nix build '.#tests': 312/312 ctest cases pass. Both installed binaries run
clean through the exact CI commands.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): bound the concurrent-callers wait, and fail the harness loudly
Two ways this file could have reported something other than what it measures.
An unbounded `while (ok < N) processEvents()` does not fail when it goes
wrong — it hangs the CI job until the job timeout, and a hang says nothing
about what broke. Bounded at 60s; the assertion below it then reports the
actual count.
And the death test's harness setup checked the host and the connection but
not the handle, so a failed acquire would have crashed on a null pointer and
been reported as "died but not with expected error" — indistinguishable from
the defect the test is looking for. It now exits 9 with a message, like the
other two harness paths.
Re-validated after the change: still red on cf1b9b0 (3/3), green here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): the detector must not touch the object after dropping its count
CI caught this, on Linux, in the shape this whole change is about — the
detector inventing the use-after-free it exists to report.
EntryGuard's destructor restored m_lastEntryPoint AFTER decrementing
m_callsInFlight. That opens a window exactly one store wide: the count reaches
zero, a release() racing on another thread reads zero, concludes nothing is in
flight and runs `delete this`, and the store lands in freed memory. Exit 139
in IoFoldTest.ReleaseFromInsideAnIoThreadEventCallbackDoesNotWedge on
ubuntu-latest; macOS was green in the same run, and the retry was green too,
which is exactly how a one-store window behaves.
The count is now the FIRST and LAST thing either the constructor or the
destructor touches. Between them the object is covered — a concurrent release()
sees a non-zero count and reports. Outside them the guard touches nothing. The
cost is a vaguer message across threads (the restore now happens before the
decrement, so a reader can see the outer frame's name); a diagnostic string is
worth less than not storing into freed memory.
AND THE REASON IT WAS REACHABLE AT ALL: that test really does violate the
contract this PR documents. It issued its triggering `fire` call on the same
handle its io-thread event callback releases, so release() ran while the main
thread was still inside that handle's callMethodAsyncWithError. The violation
was always UB and always silent — the pre-existing code touches no member after
sendCallAsync() returns, so losing the race cost nothing observable — which is
why it stayed green for seven runs on #46. Adding bookkeeping to the epilogue
made it visible.
Both tests with that shape now fire the event through a SECOND handle, which
changes nothing about what they pin: the event still arrives on the io thread,
the handler still releases the handle it was delivered through, and that handle
still has an outstanding call for teardown to cancel.
Verified by running:
* With a 300ms sleep injected into callMethodAsyncWithError's epilogue — a
window the old code lost every time — both tests reported
"LOGOS FATAL: ... callMethodAsyncWithError()" before the fix and are clean
after it. That is the violation demonstrated and then removed, not narrowed.
* The same injection at 5ms across the WHOLE suite produces zero LOGOS FATAL
reports: no other test has this shape. (The one failure it causes,
IoFoldTest.ReleaseRacingRepliesInFlightDeliversEachCallOnce, is that test's
own "the race did not run" guard firing because a 5ms-per-call sleep lets
every reply land before the release — 10000 answered-by-reply, 0
by-teardown, 0 doubles, 0 drops. Correct behaviour from the test.)
* Full suite green again: 306/306 Qt, 6/6 no-Qt, and the UAF-sensitive subset
green under Guard Malloc.
* Detectors re-validated on cf1b9b0 after the edits: death test still red 3/3.
Also fixes a fragility this found in the new no-Qt race test. In that binary
the provider shares the process's single io thread with the consumer, so under
the nix sandbox the issuing thread enqueued all 500 calls and released before
one reply came back: answered-by-reply=0, cancelled-by-teardown=10000. Zero
doubles and zero drops — but only ONE resolver ran, so the exactly-once
assertion was proving nothing, which is precisely why the "both resolvers were
live" guards are in the test. It now waits for the first reply before
releasing; both resolvers are live every run (byReply 496-744, byTeardown
9256-9504 over six runs).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): make release() safe against a call already inside the object
The defect this PR reported as unfixable is fixable, and the argument that
said otherwise conflated two different races.
That argument ran: every mechanism that could save the racing call is a
member of the object, so the racing thread's first act would be to read
freed storage. That is true of a call that ENTERS after destruction. It is
false of a call ALREADY INSIDE the object, which is the defect actually
reproduced — a synchronous callMethod parked in its future wait, released
from a second thread, faulting on the next line it executes. That call took
its bookkeeping on the way in, while the object was provably alive, so
release() cannot fail to see it.
So PlainLogosObject carries a live-reference count next to the counter the
detector already added: 1 for the owner plus one per caller inside a public
entry point. release() tears down and then drops THE OWNER'S reference
instead of `delete this`; whoever drops the count to zero destroys the
object, which for a racing call is that call's own thread on its way out.
EntryGuard takes the reference before it touches anything else and drops it
after everything else, because the drop may BE the delete.
SAFE now: release() concurrent with any call that entered first, sync or
async, any number of threads; and release() re-entered from inside a call
or an event callback on the same thread (shipped behaviour, io thread).
STILL a caller error, and still diagnosed: STARTING a call at or after
release() — its first act is to increment a counter that may already be
freed, so nothing in the object can save it — and `delete obj` in place of
release() with a call in flight, where there is no destruction left to
defer. Both report and abort in debug builds whenever the object still
exists to notice; when the storage is already freed there is nothing left
to look at, and that residue is the documented contract.
Two consequences worth naming. m_conn is no longer reset by release(): the
parked caller's next act is `m_conn->cancelPending(...)`, and resetting a
shared_ptr while another thread reads it is a data race on the shared_ptr
itself. And the object — with its share of the connection — now outlives
release() by however long the slowest call still inside it takes, which is
bounded by that call's own timeout. release() itself still blocks on
nothing: 0ms with an 8000ms call in flight, unchanged.
release() and the destructor call an unguarded disconnectEventsImpl(),
because taking a reference during destruction would drop it again and
recurse into the delete.
VERIFIED by running, on macOS arm64, debug:
* The reproduction now exits 0 through the real host stack; on cf1b9b0 the
child dies by signal, 3 runs of 3.
* The deterministic twin (a connection double that never answers, so the
park needs no timing assumption): release() returns in 0ms with the call
parked, destroyed=0 at that moment, destroyed=1 after the caller leaves,
and the caller reaches its post-wait cancelPending. On cf1b9b0: exit 139,
with and without Guard Malloc.
* The tight version — the double answers with a pending sentinel so
release()'s notify wakes the parked caller inside the window — 400 rounds,
one destruction each, 0 double deletes. On cf1b9b0 that one is SILENT
without Guard Malloc and 139 with it, which is noted in the test.
* Both remaining misuses die with their named diagnostic; both fail on
cf1b9b0, where no diagnostic exists to match.
* No false alarms: 310/310 protocol_tests, and with the detector's
decrement removed by hand all four "not accused" tests abort on a
correct program (rc=134), which is what makes them detectors.
* Guard Malloc clean over SyncCallReleaseRace, IoFold, PlainObjectTeardown,
PlainCompletionSubLifetime, PlainCancelPendingRace, PlainWaiterReaping.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): keep the Qt-free delivery vehicle alive as long as its callers
The delivery thread this PR added fixed the drop and introduced a new
use-after-free one moment later in the process's life.
DeliveryService was an ordinary function-local static, so it was constructed
on the FIRST async delivery — which means every object with static storage
constructed before that (i.e. everything constructed during dynamic
initialisation) is destroyed AFTER it. A delivery issued from such a
destructor posted into an io_context that had already run its own destructor,
on a thread that had already been joined. Reproduced with nothing but the
null-connection early-return path: SIGSEGV under Guard Malloc inside
scheduler::post_immediate_completion, reached from __cxa_finalize, 3 runs of
3; and without Guard Malloc, silently, as delivered=0 — the exact drop this
class exists to prevent, moved to a later moment. So "exactly once holds for
the whole life of a Qt-free process" was still untrue.
FIX: the service is never destroyed and registers no destructor — a
`new`-ed pointer behind the function-local static, with the destructor
DELETED so no future edit can reintroduce one — and its thread is detached.
There is now no state in which the vehicle is gone but callers remain. The
old destructor's own comment worried about a user callback blocking the join
at static-destruction time; with no join there is no such hang, and exit()
does not wait for a detached thread. Costs: one io_context and one thread in
a process that is ending, and a callback that is RUNNING at process exit can
be cut off — the same exposure a Qt slot has when the loop's thread goes.
The alternative (detect the destroyed service and deliver inline) was
rejected: inline delivery is the re-entrancy class this hop exists to
prevent, and "we are at static destruction, so no io thread is running" is
not knowable from inside postDelivery — the completing thread there can be
IoContextPool's.
ALSO IN THIS COMMIT, because it is the same file and the same claim:
* THE INLINE CHECK NOW MEASURES NESTING. Test 1 read d.total after
callMethodAsyncWithError returned and asserted it was zero, which is a
race against the delivery thread and not an inline check: 7 failures in
200 runs here (the review reported 3/200 plain, 2/40 under Guard Malloc),
every one of them with on-caller-thread=0 — i.e. nothing had actually run
inline. This file already says as much about its own tests 2 and 5. The
replacement is a thread-local depth marker raised around the issuing call
and read BY THE DELIVERING THREAD at delivery time: a callback that runs
inline is nested on the issuing thread and says so from inside itself,
with no shared state and no timing. Applied to tests 1, 2 and 5, where it
also strengthens 5 — "did a cancellation run from inside release()" is now
nesting rather than a thread comparison.
* A HARNESS LIFETIME BUG in the same file: QtFreeHost held its
IncomingCallHandler as a member, RpcServer keeps a raw pointer to it and
nothing joins the io thread, so a frame already read from the socket could
be dispatched into freed storage. SIGBUS on the io thread inside
dispatchIncoming, 1 run in 25 (1 in 5 under Guard Malloc) once the run got
long enough for the io thread to reach the queued frames. The handler is
now deliberately leaked, which is the shape that cannot lose that race.
CONTRACT WORDING. logos_object.h promised exactly-once unconditionally. It
now promises AT MOST once always, EXACTLY once whenever the callback has
somewhere to run, and enumerates the three process-level cases where it does
not: after ~QCoreApplication in a Qt process; in a process that constructs a
QCoreApplication and never RUNS its loop (queued onto a loop that never
turns — unfixable here, and it was covered by the old unconditional promise);
and in a process whose QCoreApplication was TRANSIENT, where the latch keeps
dropping for the rest of that process's life. That last one is the price of
the first: from inside postDelivery "the app is gone because we are shutting
down" and "a helper's app object went out of scope" are the same observation,
and guessing the other way would run user callbacks on a side thread during
every Qt host's teardown. A process with no QCoreApplication in its life is
NOT on the list — there delivery now holds through static destruction, with
the only residue being the process exiting before the delivery thread runs.
VERIFIED by running, on macOS arm64, debug:
* The after-main window is now a TEST: a static destructor issues a delivery
and reports through the process exit code, because no test case runs
there. On the pre-fix delivery service it fails 3/3 (exit 70,
delivered=0) and 3/3 under Guard Malloc (139). On this commit:
delivered=1, off the issuing thread, exit 0.
* The de-flaked test: 0 failures in 250 runs plain, 0 in 60 under Guard
Malloc (was 7/200 before).
* Whole no-Qt binary: 40/40 clean plain, 10/10 clean under Guard Malloc
(was 1/25 and 1/5 with the SIGBUS above). Process exit adds ~50ms and does
not hang.
* All 7 no-Qt tests still fail on cf1b9b0 (0 deliveries), so defect 5 is
still what it was.
* 310/310 protocol_tests, 3 runs of 3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(noqt): record that the exactly-once gate is TWO gates, not one
Re-validating the release-race exactly-once test against the no-Qt delivery
vehicle turned up a correction to what this suite says about its own
mechanism. AsyncCall guards a duplicate delivery twice — claim()'s
compare-exchange, and the swap in takeCallback() that leaves a second caller
holding an empty std::function — and the note in tests/protocol/CMakeLists.txt
describes only the first.
Measured, on the no-Qt twin (20 rounds x 500 calls released mid-burst):
* CAS removed, swap intact: 0 doubled deliveries. This test, its Qt twin
and PlainCancelPendingRaceTest all stay GREEN. So a validation that
removes only the CAS proves nothing about the gate.
* both removed: 22 doubled deliveries, this test FAILS — while
the three per-path exactly-once tests stay green, which is the difference
between a detector and a pin.
Neither half is redundant: the CAS is what stops a second caller from also
erasing registries and cancelling timers, and the swap is what protects the
callback itself. Comment-only; no behaviour change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): answer a call that registers as the connection fails, instead of leaving it to its deadline
sendCallAsync() reads m_stopped and THEN registers its handler under m_mu.
fail() writes m_stopped and THEN sweeps the pending map under the same mutex.
The two are ordered opposite ways round, so a fail() that completes in between
sweeps a map the caller has not written to yet:
caller fail()
------------------------------ -------------------------------
m_stopped.load() -> false
CAS m_stopped -> true
lock(m_mu); swap(m_pendingCalls)
unlock(m_mu) ... the map was EMPTY
lock(m_mu); m_pendingCalls[id] = h
writeFrame() ... drops: stopped
The handler is now parked in the pending map of a connection nobody will sweep
again — fail() runs once and has been, no reply can arrive on a closed socket,
and the frame was never written. THE CALL IS ANSWERED BY NOTHING, and what
answers instead is the caller's own deadline: callMethodAsyncWithError reports
"timeout" after the full timeoutMs, callMethodWithError blocks its thread for
the same span and reports the same wrong code, and getMethods() waits out a
hard-coded five seconds that no caller can shorten. A connection already known
to be gone is reported as a peer that was merely slow — which is also the code
callers retry and re-acquire on.
This predates the io_context fold: master has the identical shape on the
promise-based path. #46's cancelPending() only made the orphaned entry
self-cleaning rather than permanent.
THE FIX: register first, then re-read m_stopped, and reclaim our own entry if
the connection died in between. It closes the hole by an ordering argument
rather than by a smaller window:
* if fail()'s sweep ran BEFORE the registration then its CAS ran before that,
so the re-read cannot see false, and the reclaim answers the call;
* if the re-read DOES see false then, in the total order over m_stopped, it
precedes fail()'s store; the registration is sequenced-before the re-read,
so it precedes fail()'s lock, and the sweep is guaranteed to find the entry.
There is no third case, and exactly one of the reclaim and the sweep can extract
the handler because both extract-and-erase under m_mu — the same single-winner
rule dispatchIncoming and cancelPending already play by, with one more
contender. sendMethods() gets the same treatment for the same reason.
REJECTED, since the tempting fixes deadlock: holding m_mu across the check AND
the delivery self-deadlocks on the first inline delivery, because a handler here
is AsyncCall's, which calls deliver(), which calls cancelPending(), which takes
m_mu — and m_mu is not recursive (the symmetric version, fail() invoking swept
handlers under the lock, dies the same way). Moving fail()'s once-only CAS under
m_mu is correct and deadlock-free, but makes teardown's flag wait on a mutex
every in-flight send and every decoded reply also take, so m_stopped stops being
the instantly-visible "stop writing" signal that writeFrame(), doWrite() and
doRead() read lock-free — a race traded for a teardown-latency regression.
sendSubscribe() has the same shape and is deliberately left alone, with a note
saying why: nobody waits on a subscription, so there is no deadline to blow and
no caller to strand.
tests/protocol/test_plain_send_after_fail.cpp builds the interleaving instead of
waiting for it — it takes the connection's own mutex, which parks the caller
between its m_stopped check and its registration, then drops the mutex and calls
stop() from the hot thread. Validated against cf1b9b0 (the head of #46), where
all four tests are red: 39 of 40 calls never answered and their registrations
left behind; 39 of 40 reported as "timeout" at 833ms against an 800ms deadline;
a 5053ms getMethods(); 5 of 10,000 calls dropped in the unaided race. The suite
spends 154s there and 9s here, almost all of it callers sitting out deadlines
that had already been decided.
Exactly-once is re-proved rather than assumed, because the fix adds a third
contender for a registered handler. The new 10,000-call burst that stops the
CONNECTION mid-burst reports 0 doubles and 0 drops over 12 runs (120,000 calls),
and it is a validated double-detector: against the obvious spelling of this fix
(copy the handler, deliver it without the extract-and-erase) it reports 5-10
doubles per 10,000. The suite's existing 10,000-call release-race is BLIND to
that — 0 doubles against the same broken code — because it races teardown of the
HANDLE with the connection still up, so sendCallAsync's stopped branch is never
taken.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): arrange the volume test's race instead of timing it
The first CI run on the parent commit was green on ubuntu-latest (306/306)
and failed ONE assertion on macos-latest:
10000 calls racing stop() -> answered=10000 (by a reply=1512,
by fail()'s sweep=8488, stopped-connection=0)
DROPPED=0 DOUBLED=0 left registered on a dead connection=0
Expected: (byClosed) > (0)
Every substantive claim held — nothing dropped, nothing doubled, nothing
leaked. What failed was the test's own "this run actually raced something"
gate: on a 3-core runner the four caller threads finished issuing all 1,000
registrations before the teardown landed, so no call took the stopped path
and the run could not speak to it. That is exactly what those gates exist to
make loud, and it did its job — but a gate that goes vacuous on a slow box is
not one, so the race is now ARRANGED rather than timed.
Each of the three resolvers is made live by construction:
* a REPLY — phase one answers normally and is waited for, and asserted;
* fail()'s SWEEP — phase two puts the provider in a hold-every-reply mode,
so every call registered between there and the stop is still pending when
the sweep runs;
* the RECLAIM — the stop is triggered by a COUNT of registrations rather
than a clock, so hundreds of calls are always still to be issued when it
lands, however slow the box.
Both earlier cuts failed the opposite way round on the same runner: a fixed
delay put the stop ahead of the whole burst, and a first-reply trigger put it
behind all of it.
This also strengthens the detector, re-validated against real pre-fix code
rather than assumed to carry over. On cf1b9b0 the four tests now report 40/40
calls never answered (was 39/40), 40/40 reported as a timeout at 828ms, a
5049ms getMethods, and 14 dropped per 10,000 (was 5). Against the broken
"deliver a copy without the extract-and-erase" reclaim it still reports 4-8
doubles per 10,000 over four runs, while the suite's existing 10,000-call
release-race still reports 0 — which is the point of having this one.
The other fix here is a SIGPIPE the harness was provoking (one run in six died
with signal 13): asio sets SO_NOSIGPIPE in socket_ops::socket() and accept(),
which is how every socket in the shipped transports is made, but connect_pair
descriptors go through assign(), which does not — so the test sets the option
asio would have, rather than touching the process-wide signal disposition.
Nothing to do with the product.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* 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 pristine 0f26ffd, 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>
* fix(plain): close the RPC socket on the connection's strand
RpcConnection<Stream>::fail() closed the socket on whatever thread called
it. Every other access to m_stream is serialized on m_strand — start()
and writeFrame() post onto it, doRead()/doWrite() complete through
bind_executor(m_strand, ...) — but a strand serializes handlers, not a
raw call made from outside it, and asio sockets are documented as unsafe
for concurrent use.
Consumer teardown (~RpcClient -> ~PlainTransportConnection -> stop() ->
fail()) therefore ran close() -> cleanup_descriptor_data(), nulling
impl.reactor_data_, while the io worker thread was inside
reactive_socket_service_base::start_op() for a doWrite() that had just
been posted. start_op()'s 'descriptor_data' is a reference to that member:
the null check passes before the store lands, then the shutdown_ read
after it dereferences null. SIGSEGV at +0x98 on the IoContextPool thread.
fail() now hands the close to the strand via boost::asio::dispatch, which
runs it inline when fail() is already on the strand (the io-thread error
path, unchanged behaviour) and queues it otherwise. dispatch never
blocks, so teardown cannot deadlock or hang; the lambda holds a
shared_ptr so a close queued from a destructor still finds a live object.
writeFrame()'s m_stopped check is also repeated inside the posted lambda
and in doWrite(): the outer load is only a hint, and fail() can land
between it and the handler.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): a teardown-race regression that also guards against leaks and hangs
Hammers the shape that crashed: a consumer connection with frames still
queued is destroyed from its own thread, 400 times over, while the io
worker is initiating the async_write for a just-posted frame. Pre-fix
this takes the whole test binary down inside asio's reactor; post-fix the
close runs on the strand and can never overlap a write initiation.
The same loop is the guard for the two things the fix could plausibly
break: the descriptor count must come back (an async close that never
runs would strand fds) and the loop must finish promptly (a close that
blocked on the io thread would show up as a stall).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): stop dispatching inbound frames once the connection failed
Moving the close onto the strand left the socket open between stop()
returning and the strand getting to it. A frame that arrived in that gap
still ran through handleFrame -> dispatchIncoming and into the
IncomingCallHandler — which, on the host side, the caller may already be
in the middle of destroying (RpcServer::stop() runs from
~PlainTransportHost). Before the close moved, the immediate close aborted
the read and that frame never landed.
The connection is torn down either way: every pending promise has already
been failed and every event callback cleared, so there is nothing a late
frame could usefully resolve. Drop it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Extract the Logos protocol layer from logos-cpp-sdk
Transports (plain TCP/TLS, qt_local, qt_remote/QRO, mock), token manager,
consumer core (LogosAPIClient/LogosAPIConsumer incl. the capability
auto-requestModule flow), ModuleProxy, the abstract LogosProviderObject
interface, and the canonical QVariant<->JSON conversion — now behind the
language-neutral lp_* C ABI (logos_protocol.h) carrying the protocol
semver (LOGOS_PROTOCOL_VERSION_*, lp_protocol_version()).
Bytes crossing the ABI use the lossless {"_bytes": base64url} tagging
(NUL-safe), matching the plain wire encoding.
Provider lp_* surface is compiled groundwork; serving lands with module
authoring.
* consumer: typed requestModule for the capability flow
Port of logos-cpp-sdk master f5a127dd ('use updated capability module',
cpp-sdk#85, Iuri Matias) — the touched files (logos_api_client.cpp,
logos_api_consumer.{h,cpp}) moved into this repo in the P1 extraction.
The capability auto-requestModule path now calls a typed std::string
helper on the consumer (which acquires the capability object directly)
instead of a stringly invokeRemoteMethod round-trip. 111/111 tests.
