mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-09-01 14:31:08 +00:00
69e08ab76795a5c89295ba91c5d3bb85686dba76
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dario Gabriel LipicarandClaude Opus 5
|
69e08ab767 |
Merge base branch: drop the compiled-in detector inversion
Picks up #45's removal of LOGOS_PLAIN_DETECTOR_BREAK_SUB_ORDER and the CMake option that reached it, plus the validation numbers re-measured against master. The three inversions this branch adds on top are removed in the next commit. Conflicts, both in text only: * plain_logos_object.h — kept this branch's CallState-based wording for the completion subscription and took the base's re-measured figures (18-28 of 250 rounds on the wire, 6-10 of 500 through the host). Also dropped a duplicated copy of the subscribeToCompletions comment that this branch had been carrying since the fold. * tests/protocol/CMakeLists.txt — merged the two validation notes, keeping this branch's list of which tests are real detectors and its record that the per-path exactly-once tests are PINS, not detectors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario Gabriel LipicarandClaude Opus 5
|
3ee0842e42 |
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:
|
||
|
Dario Gabriel LipicarandClaude Opus 5
|
d3558276af |
Merge base branch: pick up the completion-subscription ordering fix
fix/plain-completion-sub-lifetime (#45) gained a second commit — ensureCompletionSub() now serializes concurrent first callers instead of letting them past a raised flag. This branch is stacked on it and rewrites the same function, so it takes the fix in rather than silently reverting it when #46 merges. Merged rather than rebased: #46 is already pushed and its review comments are anchored to those commits. Conflicts, all three in the code #45 touched: * ensureCompletionSub() — kept #46's CallState (m_state) as the block the handler holds, with #45's call_once + release/acquire fast path around it, and #45's subscribeToCompletions() split; * the member block — m_state (#46) plus m_completionSubscribed as an atomic and m_completionSubOnce (#45), replacing the plain mutex-guarded bool; * tests/protocol/CMakeLists.txt — one LOGOS_PROTOCOL_DETECTOR_INVERSIONS option carrying all three inversions (BREAK_ONCE, BREAK_DEADLINE_ISOLATION, BREAK_SUB_ORDER) instead of one from each side. Resulting tree is identical to the rebase of this branch onto #45, which is what was actually built and tested: 302/302, and six tests red under the inverted build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario Gabriel LipicarandClaude Opus 5
|
1c6f94a538 |
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> |
||
|
Dario Gabriel LipicarandClaude Opus 5
|
993eb49271 |
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
|
||
|
Dario Gabriel LipicarandClaude Opus 5
|
02383f7375 |
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> |
||
|
|
0f26ffdeef |
fix(protocol): report the failures that happen AFTER acquire — on both twins, without moving the ABI (#41)
* 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
|
||
|
Dario LipicarandClaude Opus 5
|
c0df466172 |
fix: integer signedness in the codec, and a shape check on the pending-call sentinel (#31)
* 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> |
||
|
Dario LipicarandClaude Opus 4.8
|
4ea32a314a |
Per-module concurrent dispatch: async provider seam + transports (#5)
* 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>
|
||
|
Dario Lipicar
|
29afbac532 |
Extract the Logos protocol layer from logos-cpp-sdk (lp_* C ABI + protocol semver) (#2)
* 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.
|