mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-27 12:01:15 +00:00
Dario LipicarandClaude Opus 5
e6d5b575c2
* 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>