Every LiveHost fixture in tests/protocol (five of them: test_iofold,
test_plain_object_teardown, test_plain_waiter_reaping,
test_plain_completion_sub_lifetime, test_call_error_after_acquire) puts a
ModuleProxy on a worker QThread and publishes it through a PlainTransportHost,
and every one of them freed that host from the TEST thread:
m_host.reset(); // test thread
m_thread->quit();
m_thread->wait();
That is a use-after-free with a millisecond-wide window, in code five test
files share.
WHY. publishObject() connects a lambda to the proxy's eventResponse signal with
NO context object, so it is a direct connection and runs on whichever thread
emits. ModuleProxy always QUEUES that emission to its own thread — it must, or
QtRO source serialization races the reply socket — so the emitting thread is
always the worker. The lambda converts the payload and then calls fanOutEvent,
which locks the host's m_mu. Free the host on the test thread and that lock is
on a destroyed mutex. ~PlainTransportHost does disconnect the connection, which
covers an emission that has not started; a worker already inside the lambda is
not called back, it is simply running, and qvariantListToRpcList on a real
payload sits in front of the lock.
MOVING reset() AFTER quit()/wait() IS NOT THE FIX, and the obvious reason for
saying so is wrong, so here is the measured one. That order does stop the fault:
wait() joins the worker, so an in-flight lambda has finished, and it is clean
under Guard Malloc. It is clean because it throws the queue away —
QThread::quit() reaches QEventLoop::exit(), which sets the exit flag
SYNCHRONOUSLY from the calling thread instead of posting an event, so the
worker's loop stops at its next iteration and discards every emission still
queued behind it. Same specimen, same load: 273-383 of 960 events delivered,
silently. In a suite whose tests count deliveries that is the worse failure,
because nothing reports it. It also shuts the host down AFTER the proxy's
thread, which test_call_error_after_acquire needs the other way round.
THE FIX, in one shared place (live_host_teardown.h) rather than five copies,
because a copied pattern is what this was: destroy the host ON the proxy's
thread, via the SDK's own logos::runOnOwnerThread marshal. A QMetaCallEvent is
dispatched by the worker's event loop, so while it runs the worker is by
definition not inside any other slot. Qt dispatches equal-priority events FIFO,
so every emission queued before it runs first, against a live host; everything
after finds the connection already severed by ~PlainTransportHost. The io
thread, the third thread that reaches the host, is still covered by the drain
barrier ~PlainTransportHost already carries — the proxy thread is not the io
thread, so that barrier's running_in_this_thread() check still takes the
blocking path. The added blocking wait introduces no hang that was not already
there: the next two statements are quit()/wait() on the same thread, with no
timeout.
EVIDENCE. test_plain_host_event_teardown.cpp is the specimen, and it is a
detector: 24 wide events queued per round, teardown aimed at the trailing edge
of the first so the worker is inside the host's lambda. Validated the way this
directory validates detectors — against a real checkout of the code it replaces
(cf1b9b0), with the fixture's teardown as it was:
pre-fix, no detector: SIGABRT 3/3 runs ("mutex lock failed: Invalid
argument" out of a Qt event handler)
pre-fix, Guard Malloc: SIGSEGV 3/3 runs at plain_transport_host.cpp:354
in fanOutEvent, on the thread named "QThread", under
ModuleProxy::eventResponse
naive (quit/wait/reset): NO fault, either detector — and 273-383 of 960
emissions delivered; the rest silently dropped
fixed, no detector: 960/960 emitted, 0 after the free, 40/40 rounds
still draining when teardown began
fixed, Guard Malloc: same counts, clean
WHAT THIS IS NOT. The five fixtures do not fault on their own today: pre-fix,
their suites are clean under Guard Malloc (2/2 runs, 33 tests) because their
test bodies drain the burst before teardown. So this closes a live trap in
shared fixture code — one that two separate probes fell into by copying the
pattern — rather than a reproduced CI failure. It remains a candidate for the
suite's unexplained SIGSEGVs, not a proven cause, and it is stated that way in
the test file.
Full suite: 304/304 (302 before, +2 new). `nix build .#tests`: 304/304 via
ctest, 95s. The five affected suites are clean under Guard Malloc on the fixed
tree.
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>
* 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>
