* 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): 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>
* fix: make isConnected() mean connected, and stop the log claiming it
QRemoteObjectNode::connectToNode() returns false only when the URL
SCHEME is unregistered -- it never contacts the peer. Our registry URLs
are COMPUTED rather than discovered (logos_instance.h:
local:logos_<module>_<instanceId>), so they are identical whether or not
the module exists. Latching m_connected from that return therefore made
isConnected() answer "yes" for modules that were never loaded, which made
every `if (!client->isConnected()) return;` guard in the codebase DEAD
CODE.
Callers then paid a 20 s waitForSource per call, twice over, because the
token handshake tries capability_module first. Measured in Basecamp with
package_manager absent: ~417 s of blocked GUI thread on macOS and 361 s
on Linux before the window appeared, and over 900 s under load. Not a
Windows bug -- the Windows port merely exposed it.
isConnected() now also requires a listener at the endpoint. For `local:`
that is a direct socket / named-pipe probe, which costs microseconds
precisely in the case that used to cost 20 seconds; any other scheme
keeps its previous behaviour.
Two logging changes, because the diagnostics cost more than the defect:
"Successfully connected to registry" asserted a connection that often did
not exist and sent three separate investigations to the wrong place -- it
now says a connect attempt started and makes no claim about the peer.
And requestObject warns BEFORE a doomed wait instead of going silent for
20 s and then reporting failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: let event subscriptions survive a module that is not reachable yet
requestObject() answers "is the module there RIGHT NOW", and every
subscriber in this codebase asks at the one moment the answer is no: a
module's init(), a UI backend's onContextReady(), a QML view's
Component.onCompleted. All of those run while the dependency's host
process has been spawned but has not called listen() yet. The subscriber
then gave up permanently -- lp_subscribe returned nullptr with no log at
all, and callers turned that into a `false` the documented example
discards. Method calls kept working through the same window because
acquireCachedObject() reaches the replica by a path that never asks, so
the symptom was "events are broken", not "the subscription never
happened".
1238316 (isConnected() means connected) is what made this deterministic
rather than lucky, and it must not be reverted -- it removed ~417 s
(macOS) / 361 s (Linux) of blocked GUI thread at Basecamp startup. So
the subscription becomes deferrable instead.
- LogosTransportAsyncAcquire: a sibling interface (dynamic_cast, like
LogosObjectErrorChannel) so LogosTransportConnection's installed
vtable is unchanged. requestObjectWhenAvailable() registers interest
and returns; it never blocks and never spins a nested event loop.
- qt_remote implements it by acquiring a dynamic replica before the
peer exists -- legal, free, and armed by the node's existing 250 ms
reconnect loop, so it adds no polling. Delivery is deferred one
event-loop turn because stateChanged fires from inside onClientRead
(the refresh_balances re-entrancy SIGSEGV).
- LogosAPIConsumer::onEventWhenAvailable() holds the pending
subscriptions, arms them when the object appears, shares ONE handle
per object (separate from the call cache, so a call re-acquiring a
stale handle cannot silently kill a live subscription), and re-arms
them after reconnect(). Unbounded in time on purpose -- a module can
be installed mid-session -- but bounded in noise: one warning at 3 s,
one at 60 s, a log line when it arms, and a loud abandon when the
transport proves it impossible.
- lp_subscribe routes through it, which fixes the same defect for
every C++/Nim/Rust module and UI backend without touching qt-sdk or
any generated code.
tests/protocol/test_deferred_subscription.cpp pins all three layers,
each with a published-first control so a red case cannot be a mis-wired
fixture.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: close the remaining silent-failure holes in deferred event subscriptions
The deferred-subscription registry from the previous commit fixed the reported
defect, but review found six ways it could still lose a subscription without
saying so — five in the registry itself, one in the plain transport's host — and
every one of them lived in a cell with no test. All of its tests ran in Remote
mode; three of the four transports had none at all.
Registry (cpp/logos_api_consumer.cpp):
* An already-present module was deferred to the first 250 ms tick on every
transport without a deferred acquire, and every event emitted in that window
was dropped. lp_subscribe used to attach synchronously and deliver them, so
this relocated the silent event loss rather than removing it. startAcquire()
now reports which of three answers the transport gave, and only an
Unsupported answer takes the one synchronous requestObject() — which is also
what keeps that call structurally away from qt_remote, whose requestObject()
enters waitForSource()'s nested event loop even at timeout 0. Previously that
invariant lived in a comment, and tick() could reach it whenever
acquireDynamic() returned null.
* reconnected() put every armed subscription back in the pending set but never
restarted the timer, which takeMatching() had stopped when they armed. Since
tick() is the sole driver of both the retry and the watchdog, a reconnect left
the subscription dead AND silent — quieter than the "not connected" warning it
replaced.
* armAgainst() released a stale handle while entries were still attached to its
event helper. Those entries stayed in m_armed, never fired again, and reported
as healthy. They are now revived and re-armed against the new handle.
* The retry timer ran forever at the 5 s cap with nothing to do. It now stops
once every pending entry has an acquire in flight and has said everything it
will say, and restarts when that changes.
* A cancelled subscription had no way to leave the registry, so lp_unsubscribe
left it holding the timer up and warning about a subscription nobody wanted.
onEventWhenAvailable() now returns an id; cancelEventSubscription() and
eventSubscriptionState() are its counterparts, and lp_unsubscribe uses them.
Plain transport (cpp/implementations/plain/plain_transport_host.cpp):
* onSubscribe() dropped a Subscribe for an object that was not published YET —
which is exactly when consumers subscribe — and the consumer could not know,
because requestObject() had already succeeded. Publishing also overwrote the
sink table wholesale, so a republish took every subscriber down with it. The
sinks now live in a table keyed independently of publication.
Also adds lp_pending_subscriptions() to the C ABI. The Qt consumer has had this
visibility all along and the C ABI had none, which is why a subscription that
silently never armed was undetectable from Rust, Nim or a universal C++ module.
tests/protocol/test_event_delivery_matrix.cpp pins the product rather than a
sample of it: 3 transports x 2 provider kinds (Qt-native and universal/std, which
reach the wire by different conversions) x 2 consumer paths (onEventWhenAvailable
and lp_subscribe) x 6 timings, plus mock and the non-blocking guard. Every
delivery case has a control that is green independently of these fixes.
One thing that is NOT fixed and is now stated in the contract: arming is not
retroactive and no transport buffers, so a module that emits a one-shot "ready"
event synchronously inside its own init() can still be missed. That window is
inherent to the transport — the blocking requestObject() this replaced had it
too — but "subscriptions survive a late module" is not "no event can be missed".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: name the QtRO invariant the stale-handle revive rests on
* test(events): state what the non-blocking guard can and cannot catch
The acquireCount assertion catches a retry that polls qt_remote's blocking
requestObject() in the ordinary case. It cannot reach the narrow one -- the
poll is only reachable when the transport declines a deferred acquire while
still reporting connected, which needs acquireDynamic() to return null and is
not forcible from outside. That case is held shut by control flow instead, and
saying so is better than leaving a reader to assume the test covers it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: make the async-acquire contract and lp_subscribe's return honest
Both from review on #47, both real.
The LogosTransportAsyncAcquire contract promised that a true return means
onReady "WILL be invoked exactly once". It will not: RemoteTransportConnection
parents every in-flight PendingAcquire to m_pendingAcquires, which is reset at
the top of the destructor and rebuilt on reconnect, so an accepted request is
cancelled silently with no callback whenever the connection it belongs to goes
away. The contract now says AT MOST once, names both cancellation triggers, and
states what a caller has to do about them — re-issue after a reconnect, or carry
its own deadline. It also records that the layer above already does the first,
which is why a subscription made through onEventWhenAvailable() survives
something the raw transport call does not. That asymmetry is the reason to
prefer the consumer API, and it was previously implicit.
lp_subscribe returned a non-null lp_subscription even when onEventWhenAvailable
refused and returned 0, leaving the caller with a handle that can never fire
while the ABI documents NULL as the one signal that the arguments were refused.
It now checks sub->id and returns nullptr.
That second one is defensive rather than a live bug, and the code says so: the
guard at the top of lp_subscribe already rejects an empty event name and a null
callback, and lp_client_create rejects an empty target, so the three inputs that
make onEventWhenAvailable() return 0 cannot all arrive there today. No test
drives it. The two contracts simply have to agree, and one of them changing is
how they would stop agreeing.
374/374 green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: stop lp_unsubscribe deadlocking, without dereferencing a freed client
lp_unsubscribe took ownerGuard->mutex and, while holding it, called
cancelEventSubscription(), which marshals to the owner thread with a BLOCKING
queued connection. The delivery callback lp_subscribe installs runs ON that
thread and takes subGuard->mutex then clientGuard->mutex — and clientGuard IS
ownerGuard, both assigned from client->guard. Lock-order inversion. It also hung
outright once the owner's event loop had stopped, which is exactly when a
language binding drops its subscription handle.
The first attempt at this dropped the guard entirely and checked `alive` inside
the posted lambda. That was a use-after-free: QMetaObject::invokeMethod
dereferences the target (it reads object->thread()) before the lambda can run,
and lp_client_destroy sets alive=false and deletes the client synchronously —
so the check was unreachable on the exact ordering lp_subscription's own comment
documents as supported. Proven rather than argued: with MallocScribble=1, a test
that destroys the client before unsubscribing segfaulted 6/6 with the guard
removed and passed 6/6 with it restored.
So the guard is held across the POST and not across the cancel. Both halves are
load-bearing, and the distinction is the whole fix: posting never waits on the
owner thread, so holding the mutex across it cannot invert; only the blocking
marshal ever had to move.
Consequence, now stated in the ABI header: un-registration is EVENTUAL. The
callback-will-not-fire guarantee stays synchronous and unconditional, but
lp_pending_subscriptions() may still list a just-cancelled subscription until the
owner thread runs, and if the client is destroyed first the cancellation never
runs at all — correct, since the registry died with it. The matrix test now
pumps for the drain instead of asserting it happened synchronously.
374/374 green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: arm a subscription immediately when the module is already reachable
Deferral introduced a narrower version of the loss it removed. The common
consumer shape is a call followed by a subscription in the same function --
wallet-ui's backend calls get_chains() and subscribes on the next line, the
tutorial's C++ UI backend does the same. Before deferral the generated Qt
wrapper acquired synchronously, so the subscription was live before on()
returned and an event emitted straight after was delivered. Holding it until the
next event-loop turn silently drops that event.
Measured on the generated-wrapper harness: 1/1 delivered pre-migration, 0/1
after, over 3 runs.
LogosTransportAsyncAcquire gains tryAcquireNow(): hand back a handle ONLY if
that costs nothing -- for qt_remote, a replica that is already Valid, which is
exactly the state a prior call leaves behind since QtRO shares one replica
implementation per object name on a node. It must never block, never spin a
nested event loop and never wait on a peer; "not immediately available" is an
answer and the caller falls back to the deferred path. Default returns nullptr,
so a transport that cannot answer cheaply simply does not.
Delivering inline here is safe for the reason the never-synchronous rule exists:
that rule protects against re-entering the transport's READ stack from a
stateChanged callback. tryAcquireNow runs on the subscriber's own stack.
The new matrix case fires ONCE, synchronously, with no pumping in between --
re-firing would hide the exact gap under test -- and states the transport
difference rather than papering over it. Subscription registration is local on
qt_remote (attach to a held replica) and qt_local (connect an in-process
signal), so delivery there must be instant. On plain it is a wire frame to the
host, so instant delivery was never on offer and never was before this change
either; that leg asserts it still arms and delivers.
Also de-flaked EventDeliveryNonBlocking: its heartbeat COUNT over a fixed
wall-clock window measures the machine, not the code. The gap assertion is the
one that means something; the count is now only a floor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: stop tryAcquireNow leaving a dangling facade in QtRO's connect list
e9f82ac introduced a use-after-free. tryAcquireNow() acquired a dynamic replica
and, when it was not already Valid, deleted it. That is not safe: QtRO shares one
replica IMPLEMENTATION per object name per node, and while that implementation is
still waiting for the source's metaobject it records every facade built on it as a
RAW pointer in QConnectedReplicaImplementation::m_parentsNeedingConnect.
~QRemoteObjectReplica is an empty body, so destroying a facade never deregisters
it, and the implementation dereferences the whole list when the class definition
arrives.
So each probe of an unreachable module left one dangling pointer behind.
WHY IT HID. The first probe owns the only implementation and takes it down with
itself, so a single subscription is harmless. It needs a second subscription whose
implementation is pinned by an in-flight PendingAcquire before a freed facade can
outlive its implementation. A consumer subscribing once sees nothing; the QML
plugin shape -- a view registering every event it cares about up front -- dies.
REPRODUCED, 4 runs of 4, serially as well as in parallel, in
logos-view-module-runtime's existing suite (unchanged from master, and green there
against this same protocol checkout):
LogosQmlBridge: subscription accepted for "echo_module" :: "ev13"
Received signal 10 (SIGBUS), code 1, for address 0x5a
SIGBUS code 1 is BUS_ADRALN -- a misaligned atomic access on a garbage base read
out of a recycled heap block, in the event loop rather than at the call site,
which is why it reads as a mystery crash rather than as a subscription bug.
PROVEN, before writing this fix, by commenting out that single `delete replica`:
the same suite went 4 failures -> 6/6 with no other change. With this fix: 6/6.
THE FIX IS TO PARK, NOT TO FREE. One probe per object name, parented to
m_pendingAcquires -- which both the destructor and reconnect() already destroy
BEFORE the node, so the implementations die in the same breath and freeing them
there is safe. Ownership transfers out only when the replica reaches Valid, by
which point the implementation is configured and is no longer holding the facade.
It costs one idle replica per name until it goes Valid or the connection dies.
AND REMOVE THE MULTIPLIER: beginAcquire() probed on EVERY add(), ahead of
startAcquire() and therefore ahead of the m_acquiring one-acquire-per-object
guard. tick() already applies that filter; beginAcquire() was the one caller that
did not, which is what turned one probe per module into one per subscription.
While an acquire is in flight its PendingAcquire already holds a replica and will
arm every waiting entry at once, so the probe buys nothing there.
Not QML-specific: lp_subscribe reaches the same entry point.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
BurstThatGoesIdleDrainsWithoutAnotherCall failed on ubuntu-latest at 21, then at
10 on a re-run, against EXPECT_LE(idle, 8u) — having scored 7 against that same
8 the run before. The change under review is not involved: the same source
compiles to a byte-identical object file with and without it.
WHAT THE RESIDUE IS. A waiter reaps only OTHERS, never itself, so what survives
an idle burst is whatever published after the FINAL reap: the last waiter to
finish has nobody behind it, and a waiter sitting in the join loop of its own
reap has not published yet while the batch it did not collect already has. That
is the size of the last exit batch, which is the scheduler's business.
AND NOTHING TAKES IT LATER, so this is not a window that was too short. Sampled
from 100ms to 25.6s after the burst went quiet the count does not move: 5->5 and
17->17 idle, 2->2 under 4x CPU oversubscription, and 24->24, 33->33, 49->49,
82->82, 123->123, 138->138, 168->168 under 32x — 12 runs, every one flat. It is
a residue, not a drain in progress.
MEASURED, 20 runs per cell, this 800-call burst, m_waiters when it goes idle:
this code reaping only on the spawn path
macOS idle 1 every run 572-723
Linux idle 1-80 (median 9) 4-168 (median 80)
Linux 2x CPU 1-145 (median 14) 16-527 (median 275)
Linux 4x CPU 1-104 (median 28) 10-504 (median 271)
Two things fall out of that, and the second is why this commit says more than
"the number was too small".
1. 8 WAS READ OFF THE macOS COLUMN. On Linux it sits under the MEDIAN of a
correct build — 35 of 60 unloaded runs of correct code exceed it — so the
test was failing correct code in most Linux runs. CI's 7 was luck.
2. THE ~600 THIS TEST IS DOCUMENTED AGAINST IS macOS-ONLY. On Linux the burst
is not concurrent: spawning 800 std::threads costs more than a loopback
ping, so most of it has already been collected by the SPAWN-path reaper
before the last call is issued, and the defect's own residue collapses
into the same range as a correct build's (4-168 idle, min 4). The two arms
overlap there at any bound, 8 included. This assertion is a coarse
retention check on Linux, not the detector for that defect.
THE NEW BOUND is kBurst/2 — the majority of the burst must have retired itself
with no further call — because the residue has no ceiling for a tighter
fraction to sit under. Worst per load level, 620 runs on a 6-core Linux box:
idle 152 (n=60) 2x 145 (n=20) 4x 172 (n=80) 8x 175 (n=160)
16x 278 (n=120) 32x 402 (n=60) 64x 317 (n=40)
Flat out to 8x, climbing after. A quarter of the burst (200) would have been
the original mistake in a new unit: it clears the worst by 1.14x, the same
ratio as 7-against-8. Half clears everything up to 16x by 1.44x and the worst
CI has ever produced (21) by 19x. The single run in 620 that scored 402, at 32x
oversubscription, is recorded in the comment rather than rounded away.
Both assertions in the test take the same expression, the second included: a
residue the follow-up call did not collect is the same retention bug, and a
tighter hard-coded number there would only move the magic constant somewhere
quieter.
Also corrects the retention note in plain_logos_object.h, which quoted "1-2
after a 2000-call burst" as though it were platform-independent.
THE DETECTOR, rebuilt with the defect this test exists to catch — the reap
dropped from the waiter's exit guard, leaving only the spawn path:
this assertion, macOS RED 10/10, 550-614 against 400
this assertion, Linux 16x RED 4/10, up to 645
this assertion, Linux idle GREEN 0/15, 25-208 — see below
publish-is-last, macOS RED 5/5
publish-is-last, Linux RED 8/8 (green 3/3 with the reap in place)
nix build '.#tests' fails its own checkPhase with the defect in
The third line is a real loss of Linux coverage in THIS assertion and it is
stated in the comment rather than glossed: on an unloaded Linux box no bound
that a correct build survives will catch it, because the burst is not
concurrent there. It costs the SUITE nothing — with the exit-guard reap gone,
PublishedWaiterDoesNotTouchTheRegistryAgain is RED deterministically on both
platforms, and it is that test, not this one, that pins the reap. If this one
ever has to be the detector again, the answer is to pace the provider so the
burst is concurrent on every platform, not to tighten the number.
No behaviour change: the only non-comment edit is the bound.
VERIFIED: nix build '.#tests' green on macOS (289/289, 69.6s) and Linux
(289/289, 78.3s).
(cherry picked from commit f147aed2c6)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(lp): lp_invoke_async can finally report a failure
lp_result_cb has always been documented as carrying an outcome —
"ok != 0 -> `json` is the result JSON value; ok == 0 -> `json` is the
canonical error object" — and the synchronous twin lp_invoke has always
honoured it (LP_ERR_UNAVAILABLE + out_error_json). lp_invoke_async did
not: it subscribed with the VALUE-ONLY invokeRemoteMethodAsync overload
and called back `cb(1, json, user_data)` with ok hard-coded to 1, so a
call to a module that cannot be acquired reached the callback as a
SUCCESS carrying a default-constructed value.
The fix is to pass a TWO-argument lambda, which is invocable only as
LogosAPIClient::AsyncResultErrorCallback and so binds to the
CallError-aware overload that already exists next to the value-only one.
The failure is then rendered with the same makeErrorJson the sync path
uses, so both entry points report the same event in the same shape.
The ABI is unchanged. lp_result_cb's (ok, json, user_data) signature
already reserves ok == 0 for exactly this; nothing had to grow a new
entry point, and every in-tree consumer already branches on `ok`
(logos-rust-sdk's async_call_trampoline even parses `message` out of the
canonical error object — code written against a contract the
implementation never kept).
Regression test: a matched pair over a REAL transport (plain TCP), not
the mock.
FAILING async call -> ok=0 {"code":"object_unavailable", ...}
SUCCEEDING async call -> ok=1 7
The first fails on the unfixed tree (ok=1, json "null"); the second
passes on both, so an over-eager "report failure everywhere" fix cannot
sneak through.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(protocol): report the failures that happen AFTER acquire, on both twins
#40 made lp_invoke_async able to report a failure, but only for the two
conditions produced ABOVE the transport: acquire failure and the unauthorized
sentinel. Everything the transport learns while the call is in flight was still
discarded — PlainLogosObject answered a bare QVariant() for a timeout and for
`ResultMessage.ok == false` alike, and LogosAPIConsumer hard-coded an empty
CallError next to it.
Two ordinary failures therefore still reported success on both entry points:
a TIMEOUT, and MODULE NOT LOADED against a host that is up (which is not an
acquire failure on the plain wire — requestObject hands back a handle for any
name over an open connection).
The information already exists: ResultMessage carries err/errCode, the futures
know they expired, QtRO knows its pending call never finished. It had nowhere to
go because LogosObject's callMethod returns a lone QVariant and its
callMethodAsync callback takes a lone QVariant.
Widening those virtuals would append a vtable slot to an installed, subclassed
interface, so instead this adds LogosObjectErrorChannel — a SIBLING interface
reached by dynamic_cast. LogosObject's size, layout and vtable are unchanged
(verified: a subclass compiled against the old and new headers emits the same
14-entry vtable with identical slot indices), and a transport that does not
implement it keeps today's behaviour.
logos_protocol.cpp needs no change: lp_invoke and lp_invoke_async already render
this CallError, so both twins gain the coverage together.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(protocol): stop the macOS flake that was sinking #41
Three real races the new CallErrorAfterAcquire suite exposed (and that
Copilot flagged on the QtRO half):
1. ~PlainTransportHost stopped the acceptor but did not quiesce the shared
Asio I/O thread. Server-side RpcConnections hold a raw IncomingCallHandler*
back to the host; a fail()/onConnectionClosed racing teardown freed the
handler mid-call. That is the macOS CI SIGSEGV in
AsyncSuccessStillReportsTheValue — it fires with no output of its own
because the previous live-host test's destructor left the heap corrupted.
Restore the I/O barrier that landed on the qtfree branches but never on
master (proven: 80/80 clean on the CI crash sequence that was ~2/50 before).
2. PlainLogosObject::callMethodAsync detached its per-call waiter while
capturing `this`. release()/delete this could then race the waiter.
Join waiters in the destructor/release, and register the thread under the
lock before it can outrun teardown.
3. QtRO async could deliver the user callback twice when the timeout timer
and the pending-call watcher finished around the same moment, violating
the exactly-once contract. Gate both paths (and the deferred-completion
arm) on one atomic.
Also drain queued onCall invokes after host.reset() in the #40 live-target
control, matching LiveHost's teardown discipline.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(protocol): the drain barrier must not dangle on its own timeout
Two defects in the barrier added by 1e9c934, both on the path it takes when
it fails:
std::promise<void> drained; // stack local
boost::asio::post(ioc, [&drained] { drained.set_value(); }); // by REFERENCE
fut.wait_for(std::chrono::seconds(5)); // result dropped
1. The wait is bounded, so on timeout this frame returns while the posted task
is still queued -- and the task holds a pointer to a destroyed stack object.
set_value() then writes to freed stack memory. The bound that stops a wedged
I/O thread hanging teardown introduced the exact class of use-after-free the
barrier exists to prevent. The promise is now a shared_ptr captured BY VALUE,
so the task keeps it alive whether or not anyone is still waiting.
2. The wait_for result was discarded. A timeout means the barrier did NOT hold
and we are about to free an IncomingCallHandler that a live connection may
still call back into -- the original crash, minus any way to know it
happened. It now warns, naming the consequence.
Neither is reachable while the I/O thread drains promptly, which is why the
suite is green either way; both matter precisely when it does not, which is
the only situation the barrier is for.
Tests: 270/270.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(protocol): teardown must not wait out the call it is abandoning
Joining the per-call waiters (rather than detaching them) closed a real
use-after-free: the waiter captures `this`, and release() used to `delete this`
underneath it. But joinWaiters() could only join. It had no way to ASK a waiter
to stop, so destroying a PlainLogosObject with a call in flight blocked for the
remainder of that call's timeout — up to 20s on the protocol default. A module
unloading mid-call stalled the unload for that long, on the releasing thread.
Measured, 8s call timeout, provider parked:
release() before after
future wait (site 1) 7804 ms 11 ms
deferred completion (site 2) 7703 ms 0 ms
Both blocking sites are now interruptible, and they need different treatment:
* the std::future wait cannot be interrupted at all, so it is SLICED: one
deadline computed up front, waited in 25ms increments, stop flag checked
between them. Teardown latency is one slice; the timeout the caller asked
for is unchanged, because the last slice ends exactly on the deadline. 25ms
is under two frames (so a module unload stays imperceptible) and costs 40
wakeups/second per in-flight call, which is nothing beside the Qt event loop
these threads already sit next to.
* awaitCompletion's condition_variable is interruptible by construction:
widen the predicate, notify_all. No latency floor at all — hence 0 ms. The
flag is published under m_completionMu so a waiter cannot evaluate the
predicate, decide to sleep, and then miss the notify.
A CANCELLED CALL STILL DELIVERS, EXACTLY ONCE. This is the part a naive fix
breaks: callMethodAsyncWithError and lp_invoke_async promise the callback fires
exactly once, so a waiter that simply returns on stop trades a bounded stall for
an unbounded hang in every caller awaiting it. Proven by building that naive
variant: it passes the latency test and fails three exactly-once tests with the
callback never arriving.
The code is "transport_error", from the existing vocabulary rather than a new
one, since these codes are the wire contract. logos_call_error.h defines it as
"the connection failed or was torn down mid-call", which is precisely what
happened — the consumer tore its own end down. The alternatives all misattribute
it: "object_unavailable" says the module is absent (it is not, and callers
re-acquire on that code), "call_failed" blames the peer for a dispatch it
performed fine, and "timeout" — what this used to report, after waiting the
deadline out — claims a deadline elapsed that did not. It is also already what
the wire produces for the same event seen from the other side (callErrorFromWire
maps TRANSPORT_CLOSED to transport_error).
Delivering during teardown is safe because postToQtEventLoop touches nothing
owned by the object: it is a free function taking the callback, value and error
BY VALUE, and the waiter copies objectName/method up front. That was already
true and is now load-bearing, so it is documented at the function. The queued
lambda runs after the object may be gone; everything the waiter reaches through
`this` runs before the join returns, which is why the join must stay.
Also closes the registration window it opens: a call arriving after the stop
would push a thread onto an m_waiters that teardown has already swapped out, so
it would never be joined. It is answered as cancelled instead.
The UAF is verified still closed under macOS Guard Malloc rather than ASan —
libclang_rt livelocks in its own initializer before main on this toolchain, for
both ASan and TSan, on a hello-world. Under Guard Malloc the race test is clean
across 5 runs and SIGSEGVs immediately when the join is turned back into a
detach, so the check is a real detector and not a vacuous pass.
Tests: 277/277 (was 270; 7 new). CallErrorAfterAcquireTest hammered 40x, 0
failures — it was ~2/50 flaky before this branch's earlier fixes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(protocol): a call that FINISHED must not park its thread for the object's life
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 in 4f9d824, not with 731e579 — but 731e579 is 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 from 731e579: 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.
* fix(protocol): a burst that goes quiet must not wait for a call that never comes
378d889 retired 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 on
378d889, 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
against 378d889 where 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 as
378d889 reported.
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 to 378d889, 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. On 378d889 the 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 from 378d889: 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>
* test(protocol): pin publishing as a waiter's LAST access to the object
PlainLogosObject's waiters are joinable, interruptible and reaped, and all
three rest on one ordering rule that nothing in the suite could see:
~FinishOnExit() {
self->reapFinishedWaiters(); // others, never itself
self->publishFinishedWaiter(id); // strictly last
}
reapFinishedWaiters() erases published entries from m_waiters under m_waiterMu
and joins those threads OUTSIDE it. stopAndJoinWaiters() swaps m_waiters under
the same lock and brute-force joins whatever it got. So a waiter that a
concurrent reaper is mid-join on is NOT in teardown's map, and teardown can
return — with release() going straight on to `delete this` — while that waiter
is still unwinding. stopAndJoinWaiters() already says this in as many words:
the guarantee is not "everything is joined when this returns" but "no waiter
touches this object after this returns". Publishing being last is the entire
reason the second sentence is true, so one member access below it is a
use-after-free, and moving the publish above the reap is a join cycle.
THE DEFECT SHIPS GREEN. Rebuild plain_logos_object.cpp with a single object
read after the publish and the whole of PlainObjectTeardownTest and
PlainWaiterReapingTest passes, 10 runs out of 10, cleanly under Guard Malloc.
That is not a hole in those suites. No SUPPORTED caller can provoke it: under
calls-in-flight-plus-release, every waiter is still joined transitively,
because a waiter leaves m_waiters only via teardown (which joins it) or a
reaper, and a reaper is either another waiter — itself in m_waiters until after
its join returns — or the async-spawn path, whose join completes before the
call returns. The one uncovered reaper is the spawn path racing a concurrent
release(), and calling a method on an object another thread is releasing is
caller-side UB that faults on correct code too. A test built on that race would
be red on green code, so it is not a usable detector.
SO STOP RACING AND OBSERVE. tests/protocol/test_plain_waiter_publish_is_last.cpp
drives a real PlainLogosObject through a scripted RpcConnectionBase — no socket,
no host, no event-loop timing, and the test decides exactly when the call's
future is satisfied — and watches the accesses in two halves.
* THE STATE. The object is placement-newed into an mmap'd two-page arena, put
down so a page boundary lands at m_waiterMu: the members teardown
coordinates on go on the second page, everything else on the first. The
first page is mprotect(PROT_NONE)'d for exactly as long as a waiter runs,
and a SIGSEGV/SIGBUS handler RECORDS each access — address, thread, and how
many ids were published at that instant — then unprotects so the access
proceeds. Nothing crashes; the access is evidence. A correct waiter touches
that page zero times: objectName and method are copied into the closure
precisely so it needs nothing from the object. Four rounds, one per exit
path out of the lambda (answered, rejected, timed out, cancelled), since all
four end in the same guard.
* THE REGISTRY, which that page cannot cover because publishing has to reach
it. Caught with bait, using the reaper's own shape: reapFinishedWaiters()
joins outside m_waiterMu, so a waiter that has picked up somebody else's
finished thread sits in that join holding nothing — a window the test holds
open as long as it likes, because the thread being joined is one the test
planted and keeps parked. Plant bait 1; let the call finish; the exit guard
reaps, takes it, parks. Plant bait 2 at leisure. Release bait 1; the waiter
finishes its reap and publishes. Bait 2 must still be registered. Bait 1
doubles as a check that the reap really does join with the lock free.
Neither half is probabilistic. A third test proves the detector can fire at all,
so the two "this counter stayed at zero" assertions are not vacuous.
MEASURED, rebuilding the file under test with each defect (caught/runs):
defect below publishFinishedWaiter() new teardown+reaping
------------------------------------ --- ----------------
read m_objectName 40/40 0/10
read m_conn 10/10 0/10
read m_completions 10/10 0/10
read m_completionSubscribed 10/10 0/10
lock m_mu 10/10 0/10
call reapFinishedWaiters() again 20/20 2/2
read m_stopping 0/10 0/10
(publish moved ABOVE the reap) 0/5 12/15
no defect — 8f0c60f 0/40 0/10
The one gap is m_stopping, the single member sharing the registry's page, which
cannot be guarded without guarding the publish. The inverted order is left to
the reaping suite's hammer, which has it covered. Runtime 0.9-1.0s for all
three tests; clean 40/40 on 8f0c60f, and clean 3/3 under Guard Malloc
(MALLOC_PROTECT_BEFORE=1, banner confirmed) — the test never touches freed
memory itself, which is the other half of not being built on UB. No Guard
Malloc needed to detect anything: mprotect and the bait are the detectors.
Also: nix build .#tests 100% (285/285), the full binary 285/285, and
CallErrorAfterAcquireTest 40/40.
CORRECTIONS to measurements claimed earlier on this branch. All three were
overstated in the same direction — a single sample read as a constant:
* "ReapingRacesPublishingWithoutDeadlocking aborts the process, 5 runs out of
5" (plain_logos_object.cpp, and 378d889's message) is 12 runs in 15, ~80%.
It is a race detector, so one green run of it proves nothing — which is
exactly the argument for the deterministic suite added here. Corrected in
the comment.
* C-ABI retention was reported as "+0.09 MiB / 10 B per call" for 10k
lp_invoke_async on one client (8f0c60f's message). ~6 B/call. Same
conclusion — flat — different arithmetic.
* The burst retention figures 1428 (2000 calls, idle) and 610 of 800 came
back as 1421 and 599 on re-measure of the same build. Race-dependent, same
magnitude, which is why the tests assert a bound and not a value. Noted in
test_plain_waiter_reaping.cpp so the next reader does not treat them as
reproducible constants.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(protocol): make the publish-is-last probe fail loudly, and state the rule it actually checks
Three defects in cb015f5's regression test, plus the corrections that commit's
own CORRECTIONS section still owed. No behaviour change: the diff against
8f0c60f under cpp/ is comment lines only, verified by filtering the diff.
(a) THE BAIL-OUT PATH HUNG INSTEAD OF FAILING, which is the one that can stall
CI. PublishedWaiterDoesNotTouchTheRegistryAgain plants parked "bait" threads
behind gates and registers them in m_waiters. An ASSERT that fires before
gate1.open() — ASSERT_TRUE(tookBait1) is the obvious one — returns from the
function, and then ~PlainLogosObject blocks forever joining a thread nobody
will release. Reproduced by removing the reap from the exit guard: the
assertion PRINTS and the run still ends as a timeout kill, exit 124, with no
test result at all.
The gates are now opened by a scope guard on every exit path, and declared
BEFORE the GuardedObject so they outlive the teardown that joins the threads
parked on them. Same break, after: the same assertion, exit 1, 10.0s — which
is the probe's own tryWithRegistry budget and not a hang.
This is the shape a future refactor trips, not a hypothetical: the TODO
above the waiter (fold the wait into the shared Asio io_context) moves where
reaping happens, which is exactly the edit that makes tookBait1 false.
Both tests also stopped capturing their delivery counter by reference. On a
bail-out the cancelled call's callback is delivered on a later event-loop
iteration, i.e. after the frame is gone — a real use-after-free on the way
out of a failing test in a file about use-after-free. Owned by the callback
now.
(b) THE STATE ASSERTION WAS STRICTER THAN THE INVARIANT. It asserted
accessCount() == 0; the rule is only "no access AFTER the publish", and the
fault handler already stamps each access with how many ids were published at
that instant, so it can tell them apart.
Not hypothetical either. On the deferred/"multi" path a CORRECT waiter calls
awaitCompletion() (plain_logos_object.cpp:338), which locks m_completionMu
and reads m_completions and m_objectName — all on the guarded page, all
before it publishes. cb015f5 was green only because none of its four rounds
returned a pending sentinel, and the header's claim that "a correct waiter
touches that page ZERO times, before the publish or after" was true only of
the non-deferred rounds.
So: a fifth round drives the pending-sentinel path (ScriptedConn now answers
with the sentinel; nothing pushes the completion, so awaitCompletion runs out
its deadline), and the assertion narrowed to accesses stamped published >= 1.
PROVEN BOTH WAYS on this tree — with the old accessCount() == 0 predicate the
new round fails on correct code, naming offset 144 with "0 waiter id(s)
already published"; with the narrowed one the suite is 30/30 clean.
The round cannot pass vacuously: it REQUIRES at least one recorded access, so
a machine slow enough to turn it into a plain timeout fails it instead of
quietly proving nothing. Each round also now asserts m_finishedWaiters is
empty before arming, which is what makes "published >= 1" mean "after THIS
waiter's publish".
Two things guard against the narrowing being a quiet disarm:
* the handler now re-arms. It could not before (the faulting instruction
re-runs immediately), so the observing thread does it — it polls the
registry anyway and never touches the guarded page. Without it the first
legitimate access disarms the detector for the whole round.
* the catch rates were re-measured, not assumed. They are unchanged.
(c) TWO OVERSTATED NUMBERS, in the section whose whole point was to stop
overstating. Fixed where they live; cb015f5 is pushed and is not rewritten.
* "C-ABI retention ~6 B/call, not 10" replaced one sample with another.
10k lp_invoke_async on one lp_client, run ten times: 0, 5, 5, 5, 7, 7, 8,
10, 13, 10 bytes/call (mean 7.0). Ten more, run here: 3, 11, 8, 5, 8, 10,
13, 8, 3, 10 (mean 7.9). One distribution, range 0-13; both 6 and 10 sit
inside it and 8f0c60f's arithmetic (0.09 MiB / 10k) was not wrong.
THE HONEST STATEMENT IS THAT IT IS FLAT: indistinguishable from zero, RSS
noise and not a per-call rate. Recorded in test_plain_waiter_reaping.cpp
beside the other retention figures, where the next person to quote one
will see it.
* the table cell "reapFinishedWaiters() again ... 2/2" for the older
teardown+reaping suites was a two-run sample printed beside 10-40 run
samples. Re-measured over 30 runs: 9/30 here, 12/30 on another 30-run
sample — roughly one run in three, matching what reapFinishedWaiters'
own comment already said ("about one run in four"). The cell now reads
9/30, and the table says to read that column as rates and the left-hand
one as deterministic.
ALSO STATED PLAINLY, because it was overstated in review: the window where
"teardown returns while a reaped waiter is still unwinding" is NOT reachable by
a supported caller. A waiter leaves m_waiters only via teardown (which joins it)
or via a reaper, and that reaper is either another waiter — still registered
itself, since it reaps before it publishes, so teardown joins it and therefore
waits out the join it is in — or the async-spawn path, whose join completes
before the call returns. The only uncovered reaper is the spawn path racing a
concurrent release(), which is caller-side UB on any version of this class.
So publish-is-last is an invariant the design rests on and documents, not a
lurking use-after-free. This suite pins it against future edits; it does not
close an open hole. The file header, both failure messages and the comment in
plain_logos_object.cpp now say that instead of implying otherwise.
MEASURED AFTER THE CHANGE, rebuilding plain_logos_object.cpp with each defect
below publishFinishedWaiter() and running the suite (caught/runs), beside the
numbers from before it:
defect before after
------ ------ -----
read m_objectName 25/25 25/25
lock m_mu 25/25 25/25
write m_completions under m_completionMu 25/25 25/25
call reapFinishedWaiters() again (bait) 20/20 20/20
read m_conn - 10/10
read m_completions - 10/10
read m_completionSubscribed - 10/10
read m_stopping (declared blind spot) 0/10 0/10
publish moved ABOVE the reap (delegated) 0/5 0/5
no defect 0/30 0/30
Nothing moved, including the two declared blind spots — a narrowing that had
started catching or stopped catching something would show here. cb015f5 reported
40/40 for m_objectName from a longer run; 25/25 is this run, not a regression.
WHERE THE DEFERRED ROUND IS WEAKER, said here rather than left to be found: on
that one round the post-publish half is best-effort. A legitimate access opens
the page, the re-arm is a syscall behind, and a defect firing a microsecond
later slips through — measured with every round forced to run, the other four
catch a post-publish m_objectName read 5/5 and the deferred round 0/5, and a
variant that spins on the re-arm instead of polling records 4-24 accesses per
round and still catches it 0/5. It costs nothing: FinishOnExit is ONE piece of
code shared by all five exit paths, so the same defect is the same defect on
every round and the other four catch it deterministically. The deferred round is
there to keep the assertion honest about correct code, not to add a fifth copy
of the same detection.
Verified: PlainWaiterPublishIsLastTest 30/30 clean, the three waiter suites
15/15, the full binary 285/285, `nix build .#tests` 100% (285/285),
CallErrorAfterAcquireTest 40/40. Suite runtime 1.3-1.5s for the three tests
(0.9-1.0s before — the deferred round waits out a 400ms completion deadline).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
RpcServerTcp::stop() and RpcServerSsl::stop() closed m_acceptor on whatever
thread called them — in practice the host thread, via ~PlainTransportHost —
while doAccept() re-armed async_accept from inside its own completion handler,
on the io worker. Nothing serialized the two.
This is the acceptor half of the race PR #38 fixed for RpcConnection, and it
fails identically: asio acceptors are "Shared objects: Unsafe", and close()
runs cleanup_descriptor_data(), which nulls the reactor's per-descriptor state
while reactive_socket_service_base::start_op() holds it by reference. It was
left out of #38 because every backtrace captured in the wild was a write
initiation, never an accept — but it reproduces on demand:
EXC_BAD_ACCESS KERN_INVALID_ADDRESS at 0x98
logos::plain::RpcServerTcp::doAccept()
...reactive_socket_move_accept_op<...>::do_complete(...)
logos::plain::IoContextPool::IoContextPool()::$_0 <- io worker thread
Both servers now own a strand. doAccept()'s completion handler is
bind_executor'd onto it (so the re-arm runs there) and stop() hands the close
to it with dispatch() — inline when already on the strand, queued and
non-blocking from anywhere else, exactly as RpcConnection::closeStreamOnStrand
does.
start() still runs open/bind/listen inline: callers read boundPort() the moment
it returns. That is safe because no async op on the acceptor exists yet, and
PlainTransportHost serializes start()/stop() under its own mutex. Only the
accept loop moves onto the strand, which is invisible to clients — listen() has
already run, so an early connect waits in the backlog.
Deferring the close leaves the listener open for the microseconds between
stop() returning and the strand running it, so a connection can still be
accepted in that gap. The accept path therefore tests m_stopped and publishes
the connection under one lock, and drops a late socket instead of wrapping it
in a connection and stop()ing it — conn->stop() would call
onConnectionClosed() on the IncomingCallHandler whose destructor started this
teardown. The TLS server gets the same guard, where it was already latent: an
async_handshake in flight was never aborted by closing the acceptor.
Adds RpcServerTeardownTest: a start/connect/stop stress loop shaped like
test_rpc_connection_teardown.cpp, plus a round-trip check that a client
connecting the instant start() returns is still served.
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>
* fix(codec): signedness and range are part of the integer type
Codec<T>::from accepted any integral JSON number and handed it to .get<T>().
That is silent in both directions:
.get<uint64_t>() on -1 -> 18446744073709551615 (a sign flip)
.get<int32_t>() on 2^40 -> truncated
Both now reject with the usual path-carrying CodecError instead. Rejecting is the
codec's existing contract — a value the declared type cannot represent must not
reach business logic wearing a different one — this just extends it to the half
of the integer domain it was skipping.
Note the check is on the JSON category, not the value: a negative literal parses
as number_integer and never as number_unsigned, so `is_number_unsigned()` is the
reliable discriminator rather than a comparison after conversion.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(async): the pending-call sentinel is matched by shape, not by key presence
All four detection sites tested `m.contains(pendingCallKey())` and nothing else,
so ANY user map carrying that key was taken for a deferred call: the consumer
extracted a call id, found no completion, and waited out a nested event loop.
The measured outcome is a ~20s HANG, not a fast failure. An `any` slot is enough
to reach it — anything a user can put in a map.
logos::isPendingCallSentinel now requires the canonical shape: exactly one entry,
under the sentinel key, holding a non-empty string. Shape and signature are
mirrored from isUnauthorizedSentinel (logos_rpc_status.h), QJsonObject arm
included — the two are the same kind of in-band marker and there was no reason
for them to be guarded differently. That guard, and isTaggedBytes's, both already
existed in this repo; the difference was chronology, not principle.
Behaviour-preserving: the generated glue builds this map with exactly one entry
whose value is a QString call id, so no real sender changes. The concurrent
dispatch tests pass unchanged.
NARROWS, DOES NOT CLOSE — and the tests say so out loud. A one-key, string-valued
forgery IS the sentinel; no predicate can separate them. It still hangs, and
because call ids are a per-object counter from 0, a forged "lc-0" can collide
with a genuine in-flight completion and steal its result. Closing that needs an
out-of-band channel for "deferred", which the single-QVariant dispatch slot
cannot express without an ABI break — the constraint is stated at
logos_rpc_status.h:24-27 and is real.
tests: 10 new, including one asserting the forgery still matches, so a future
reader cannot mistake the green cells for "the sentinel is safe". 236/236.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(events): the event bridge converts through the canonical helper
setEventListenerStdBridge adapts the universal event callback (name + JSON
string) to the Qt EventCallback (name + QVariantList). It is the event-path
counterpart of callMethodStdBridge, but it did the conversion itself:
callMethodStdBridge -> logos::nlohmannToQVariant (canonical)
setEventListenerStdBridge -> QJsonDocument::fromJson
+ QJsonValue::toVariant (Qt's parser)
Two consequences, both measured by the LIDL conformance matrix as M6:
* a uint64 above int64max degraded to a double. Qt 6 backs QJsonValue with
QCborValue, so integers up to int64 DID survive — only values with no
integral representation there fell back to double. echoUint(2^64-1) was
exact while uintEvent(2^64-1) arrived as 1.8446744073709552e+19: same
value, same process, one hop later.
* canonical tagged bytes {"_bytes": ...} were not decoded, arriving as a
QVariantMap where the method path yields a QByteArray. This never showed up
end-to-end because the undecoded map round-trips to JSON and the python
client decodes the tag itself — but a C++ or QML event subscriber got a map.
Both now go through logos::nlohmannArgsToQVariantList, which the generated
cdylib emitTrampoline already used. Numbers and bytes no longer depend on
whether a value left the module as a return or as an event.
Not the residue of the codec convergence, despite how M6 was originally
registered. #29 converged six copies of the VALUE codec; this was a seventh
conversion inside an ADAPTER, which that scope never touched. It is also not on
the providers' own path — a Qt provider stores its callback verbatim and a
cdylib provider already converted correctly. The one live caller is the
logoscore daemon's CoreServiceImpl, which forwards every watched module event;
that is why C++ and Rust providers measured identically.
Why it survived: the bridge appeared in the test suite once, in
test_universal_provider_dispatch.cpp, purely to satisfy the pure virtual. No
test asserted anything about an event payload. The method path got 15 contract
tests in #29; the event path got none.
tests: 11 new cells pin the bridge directly — uint64 past int64max, 2^53+1,
int64::min, large integers nested in containers, tagged bytes at top level and
at depth, plus the shapes that already worked (multi-param order, double staying
double, null elements, empty payload, the non-array raw-string fallback) so a
future rewrite cannot quietly drop them. 210/210.
verified: logos-cpp-sdk, logos-qt-sdk, logos-liblogos and logos-logoscore-cli
all green against this build; the conformance matrix goes 156 -> 158 pass with
M6's two cells retired, and the ext table stays 40/40.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(events): pin the signedness rule the convergence brings with it
nlohmannArgsToQVariantList classifies every non-negative integer as unsigned, so
a LIDL `int` event argument now arrives as ULongLong where it used to be
LongLong. That matches what nlohmannToQVariant (the method path) and the cdylib
emitTrampoline already did — the surfaces now agree — but it is an observable
metatype change that nothing asserted.
Pinned in both directions (non-negative -> ULongLong, negative -> LongLong) so
it stays a decision rather than a side effect. Value-level reads are unaffected.
212/212.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): RpcValue can represent a uint64 above int64max
The plain (tcp/tcp_ssl) wire squeezed every unsigned value through int64_t, so a
LIDL `uint` above int64max wrapped — independently in each direction:
outbound qvariant_rpc_value.cpp QMetaType::ULongLong -> int64_t(...)
inbound json_mapping.cpp is_number_unsigned -> get<int64_t>()
Neither wraps loudly: .get<int64_t>() past int64max returns -1 with no
exception. Two peers both running this code agreed on -1, so nothing looked
broken from inside — and no plain-tier test used an integer outside int32 range.
Measured over real tcp before the fix:
echoUint(2^63) -> -9223372036854775808
echoUint(2^64-1) -> -1
This was never a wire-format constraint. Both codecs carry uint64 natively (CBOR
emits major type 0, `1b ff..ff`) and the envelope's own `id` field already
crossed this wire as uint64_t. Only RpcValue *payloads* could not represent it.
RpcValue gains a uint64_t alternative, used through `makeInteger()` and ONLY for
values above int64max — the sole case where int64_t loses information. Anything
broader would change the representation of every non-negative integer already on
this wire, and since std::variant equality compares the alternative index it
would break comparisons against int64-built values, to fix nothing. Small
unsigned values keep crossing as signed, pinned by a test so the rule stays
visible.
Also fixes an off-by-one in the QJsonValue::Double -> int64 guard while here:
double(int64max) rounds UP to exactly 2^63, so `d <= double(int64max)` admitted
2^63 and then ran int64_t(d) out of range — undefined behaviour, saturating on
arm64 and INT64_MIN on x86-64. Now a strict `<` against 2^63.
tests: 14 new. Both codecs round-trip 2^64-1 flat and nested; negatives stay
signed; the Qt boundary is exact in both directions; the narrow representation
rule and the 2^63 guard are pinned. 226/226.
verified end-to-end, cross-process, with a negative control: the new 64-bit
boundary cases in logos-logoscore-py fail on the pinned protocol over tcp with
exactly the values above, and all 68 pass with this build — on local, tcp and
tcp_ssl alike.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* feat(codec): one canonical LIDL <-> JSON codec, generic over composition
The tagged-bytes encoding {"_bytes": "<base64url, unpadded>"} was implemented
SIX times — the Qt conversion here, the plain wire's json_mapping, the lp helper
in logos-cpp-sdk, a copy emitted into every generated cdylib module, the Rust
SDK and the Python client — and they disagreed on which inputs they accept:
- {"_bytes":"AA","x":1} decoded as BYTES on the lp path (no size()==1 check)
but as a MAP on the plain wire and in the glue.
- Padded "AH-A_w==" gave correct bytes in one copy, empty in another, None in
Rust.
- A plain string / number / number-array argument was accepted by C++
providers (Qt and CLI parity) and rejected by Rust ones.
logos_codec.h is the single implementation. Leaves: tstr, bstr, every signed and
unsigned integral spelling, every floating spelling, bool, any (recursion stops).
Composition is GENERIC — std::vector<T> and std::map/unordered_map<std::string,T>
for any supported T, at any depth — so [bstr], [[bstr]], {tstr: [bstr]} and bytes
nested in a map all encode canonically without anything enumerating combinations.
Codec<T> is a trait, so an unsupported T is an incomplete type: a compile error
naming the type, never a silent fallback. Decode throws CodecError carrying the
path ("[0][1]", ".k") instead of substituting a default — a mangled value must
not reach business logic. bstr keeps a documented lenient form for provider-side
arguments, because the Qt consumer path and the logoscore CLI both produce plain
strings and number arrays for byte parameters.
JsonArg exists for generated dispatch: it converts itself into whatever the
callee's parameter type is. Naming the type instead is a trap — spelling [uint]
as std::vector<uint64_t> (the LIDL mapping) does not bind to an author's
std::vector<uint32_t>, since distinct vector instantiations do not convert.
logos_codec.h joins the installed header set; nix/include.nix already globs
cpp/*.h.
Tests: 198/198. 15 new ones pin the contract rather than the happy path —
[[bstr]] tagged at depth, map-of-bytes, empty elements surviving as elements,
uint64 past 2^63, an integral JSON number decoding as float64, padded base64,
the multi-key {"_bytes":...} case being a map, and path-carrying failures.
Not yet converged onto this header (follow-ups): the Qt conversion in
logos_json_convert.cpp, and the plain wire's copy in json_mapping.cpp — the
latter needs a strict variant first, because it THROWS on malformed base64
(via its own logos::plain::CodecError) where every other copy is tolerant.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(codec): fold the Qt and plain-wire copies into the shared codec
The two remaining in-repo implementations now delegate:
- logos_json_convert.cpp (the Qt CONSUMER path — argument encoding and return
decoding) dropped Qt's toBase64/fromBase64 and its own tagged-bytes
predicate. Only the QByteArray <-> std::vector<uint8_t> hop stays local, so
the Qt path cannot drift from the wire or from providers: same alphabet, same
padding rule, same single-key shape.
- implementations/plain/json_mapping.cpp dropped its anonymous-namespace
b64url_encode/decode.
The wire needed something the tolerant decode does not give it: it REJECTS a
corrupt frame rather than silently decoding fewer bytes. Hence
b64UrlDecodeChecked — strict about the alphabet and the length, tolerant of '='
padding — which json_mapping uses to keep throwing its own
logos::plain::CodecError. Consumer-facing decodes stay tolerant. Both behaviours
now come from one implementation instead of four that disagreed.
Also removed the local isTaggedBytes wrapper, which shadowed the shared one and
made unqualified calls ambiguous.
Tests: 199/199, with the strict decode's accept/reject set pinned (padding
tolerated, stray character rejected, impossible length rejected).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* feat: group-shareable local sockets, stale-socket reaper, bind-failure detection
The QtRO local transport binds each module's unix socket at 0777 & ~umask
(0755) with no way for a second OS user to reach it, discards the listen
result so a failed bind surfaces only as clients hanging, and never cleans up
the socket file — a hard-killed logos_host leaks it forever.
Add a Qt-free helper (logos_socket_paths.{h,cpp}) usable from both the qt_remote
and plain transport paths:
- applySocketPerms(path): chgrp + chmod a bound socket per LOGOS_SOCKET_GROUP /
LOGOS_SOCKET_MODE (chgrp-then-chmod so a half-applied policy is only ever
too strict). No-op when unset, so default behaviour is unchanged. Connecting
to an AF_UNIX socket needs write permission, so 0660 is what lets a group
member in.
- isSocketDead(path): S_ISSOCK && owned-by-us && non-blocking connect returns
ECONNREFUSED/ENOENT. Fails closed on any other outcome, so it never reports
a live socket or a regular file dead.
- reapStaleSockets(dir, prefix): unlink only the dead sockets, never a regular
file that shares the prefix (e.g. a *.lgx build artefact).
Wire it into RemoteTransportHost::publishObject and QtRemoteRegistry:
- construct QRemoteObjectRegistryHost empty and listen via setRegistryUrl() so
a bind failure is observed and logged (with lastError() + the socket path)
instead of leaving a silently-broken host;
- apply the socket-access policy to the freshly-bound local: socket.
The env-driven policy means every process in a node's tree (daemon, logos_host
subprocesses, their children) applies the same rule to every socket it binds
without threading config through each layer — the daemon exports the vars once.
Adds test_socket_paths.cpp (8 gtests): mode/group application, no-op default,
bad-mode rejection, live/dead/regular-file classification, and the reaper
keeping live sockets and regular files while removing only dead ones.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* review: harden socket helpers (gid overflow, socket-owner check, empty-prefix guard, dedup path)
Addressing automated review feedback on the socket helpers:
- resolveGid(): validate strtoul() errno/range so an out-of-range numeric
LOGOS_SOCKET_GROUP is rejected instead of silently truncating to a wrong gid.
- applySocketPerms(): when a policy is requested, stat the path first and refuse
unless it's a socket we own (S_ISSOCK + st_uid == geteuid()), so a malformed
URL can never chmod/chown a stray file. No-op fast path when the env is unset.
- reapStaleSockets(): refuse an empty prefix (would make every dead socket the
process owns a deletion candidate).
- Extract the duplicated `localSocketFilePath()` (Qt QLocalServer name->path
rule) into a shared qt_remote/qt_socket_path.h so RemoteTransportHost and
QtRemoteRegistry can't drift.
Adds tests: non-socket path refused (mode unchanged), empty-prefix reaper no-op.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat: transport-aware token validator hook on ModuleProxy (#22)
* feat: transport-aware token validator hook on ModuleProxy
Adds an injectable authorizer so a host (the logoscore daemon) can accept tokens
the built-in issued-token scan doesn't know — specifically operator-issued named
tokens validated against a persistent store — with per-token expiry and
local_only enforced against the transport the call arrived on.
- ModuleProxy::setTokenValidator(std::function<bool(token, transportProtocol)>).
isAuthorized() consults it ONLY after the existing m_tokens + TokenManager
scan fails, so installing a validator is purely additive: it can grant, never
revoke, access the built-in path already allows. Empty (default) = today's
behaviour exactly.
- callRemoteMethod() gains a defaulted `transportProtocol` ("local"). The QtRO
local path (RemoteTransportHost) uses the default; PlainTransportHost::onCall
passes the real wire ("tcp" | "tcp_ssl", fail-closed to non-local on an
unexpected protocol) so a local_only token presented over the network is
rejected. One ModuleProxy is shared across a provider's transports, so the
transport can't be inferred — it must be threaded per call, which the defaulted
arg does without changing the QtRO replica's 3-arg call.
The daemon backs the validator with TokenStore::lookupByToken; other modules
keep the default (no validator) and are unaffected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* review: split callRemoteMethod into explicit 3-arg + 4-arg overloads; include <utility>
Addressing review feedback:
- Replace the defaulted transportProtocol argument with two explicit Q_INVOKABLE
overloads. The Qt meta-object system matches methods by their full parameter
list and doesn't apply C++ default arguments, so the QtRO/local 3-arg call
must remain a real 3-arg method rather than relying on moc's reduced-arity
generation. The 3-arg form forwards to the transport-aware 4-arg form with
"local"; PlainTransportHost keeps calling the 4-arg form with the real wire.
- Include <utility> explicitly in module_proxy.h for std::move rather than
relying on an indirect include.
Full protocol suite green (160/160).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* feat: per-module concurrent dispatch (concurrency:"multi") — zero ABI change
A "multi" module serves calls concurrently behind the ORDINARY callMethod — no
new provider/host vtable method, so LogosProviderObject's ABI is byte-identical
to before and an old host/daemon loads and forwards a multi module unmodified.
Mechanism: a multi module's generated glue returns a pending sentinel
({"__logos_pending_call__": callId}) from callMethod and pushes the real result
back later as a __logos_call_complete__ event keyed by callId, over the existing
event channel. The consumer transport detects the sentinel and awaits the
completion transparently, so generated clients are unchanged.
- logos_async_dispatch.h: shared wire constants + the contract.
- remote_transport.cpp (QtRO) / plain_logos_object.{h,cpp} (plain): consumer
sentinel detection + await keyed by callId. The host is a pure forwarder.
- logos_protocol.h + nix/default.nix: protocol 0.2.0 (additive minor; same MAJOR
stays compatible, so an old host accepts a 0.2 "multi" module).
- rpc_server.cpp: fix a teardown self-deadlock (stop() held m_mu while invoking a
per-connection error handler that re-locks m_mu) that the new in-process
subscription path exposed.
- tests/protocol/test_concurrent_dispatch.cpp: proves a multi provider overlaps
two concurrent calls (peak 2) while single serializes (peak 1), over the plain
transport, with the host unchanged from master.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: coalesce concurrent async requestModule handshakes (+ async fan-out test)
A driver that fans out N async calls to an un-tokened target before any
completes used to fire N separate requestModule handshakes. Each mints a
distinct capability token and informs the target, and the later inform
OVERWRITES the earlier token there (the target stores one token per caller),
so the already-dispatched calls carried a superseded token and the target
rejected them as unauthorized ("auth token not recognized"). The sync path
never hit this — it blocks per call, so handshakes never overlap.
Coalesce in LogosAPIClient::invokeRemoteMethodAsync: the first async call to
an un-tokened target starts ONE handshake; concurrent calls to the same
target queue behind it and all drain with the single minted token when it
resolves. m_pendingHandshakes is touched only on the owner thread, so no lock
(appended last per the class's ABI note). This is what lets a concurrency:
"multi" worker actually run a single-threaded driver's fan-out concurrently —
otherwise the fanned-out calls are rejected before reaching dispatch.
Also add MultiProviderOverlapsAsync / SingleProviderSerializesAsync to the
concurrent-dispatch gtest: they fire N concurrent callMethodAsync() calls (the
fan-out pattern over the async consumer path, which the sync tests don't
exercise) and assert peak overlap 4 for "multi", 1 for "single".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <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.
