mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-31 14:01:14 +00:00
fix/bound-request-module-timeout
28
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dario Gabriel LipicarandClaude Opus 5
|
5bff8f8f80 |
test(token): pin that the caller's budget bounds the handshake, not just the call
Written first and on its own commit so the red is on the record: against this parent it fails at ~20s, because the capability handshake ignores the caller's budget entirely. LogosAPIClient::invokeRemoteMethod takes a Timeout, but on an un-tokened target the handshake runs FIRST and LogosAPIConsumer::requestModule hardcodes 20000 twice -- once for the capability_module acquire, once for the requestModule call on it. A caller asking for 1500ms could therefore block on the order of 40s before the part it had actually bounded began. logos-view-module-runtime's callModule advertises a 1500ms bound on precisely this path. capability_module is deliberately NOT published, so the acquire runs its budget out rather than succeeding. Every other test in this file publishes it, which is how a hardcoded 20s survived alongside them: none of them ever entered the wait. The assertion is two-sided on purpose. An upper bound alone would pass if something made the acquire return instantly -- leaving the hardcoded 20s in place and the test green for the wrong reason, which is the exact shape of two earlier tests in this change set that passed in both directions. So: >= budget-200ms proves the timeout path actually ran; < 4x budget proves it was the CALLER's budget and not the 20s default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario LipicarandClaude Opus 5
|
07b0fb1c64 |
fix: make event subscriptions survive a module that is not reachable yet (#47)
* 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". |
||
|
Dario LipicarandClaude Opus 5
|
dda5dae1bf |
test(plain): bound the burst-drain assertion against the burst, not a constant (#56)
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
|
||
|
Dario LipicarandClaude Opus 5
|
9aa16aeb14 |
test(plain): stop the registry probe from starving the waiter it waits for (#54)
PublishedWaiterDoesNotTouchTheRegistryAgain failed on ubuntu-latest at exactly
10000ms with "the waiter's exit guard never reaped the planted entry", and had
failed once before on macOS at 13.6s with "the waiter never published". Both are
the same defect, and it is in the test rather than in the code under test.
tryWithRegistry() declared its try-lock in the same scope as the 200us sleep at
the bottom of the loop, so the probe HELD m_waiterMu across that sleep and gave
it up only for the handful of nanoseconds between the unlock and the next
try_to_lock. Every condition the probe waits for is produced by the waiter under
that same mutex — reapFinishedWaiters() erases the bait under it,
publishFinishedWaiter() appends the id under it — so the loop was not polling the
waiter, it was BLOCKING it, and std::mutex hands off by barging rather than FIFO.
The waiter got in only when it happened to be running on another core in that
nanosecond-wide window, which is a lottery with no bound on it.
MEASURED, with iteration counters added to the probe: it acquired the mutex on
essentially every iteration (455/455, 567/567, ~0 try-lock misses) while the
waiter needed between 1 and 700+ attempts to land a single acquisition. The wall
clock is that count times the cost of one iteration, and the second factor is
what CI supplies: on an idle box an iteration costs ~290us, under CPU
oversubscription ~20ms. A few hundred attempts is then the whole 10s budget.
That is also why it never reproduced locally — with cores to spare the woken
waiter is dispatched fast enough to win within a few dozen attempts — and why
the sibling half of this suite never flaked: waitForPublish() takes and releases
the same mutex inside publishedCount() and sleeps OUTSIDE it.
REPRODUCED before changing anything, on Linux in the nix sandbox under 600x CPU
oversubscription: 3 failures in 25 runs, both CI messages verbatim — tookBait1
false at 10001ms after 455 probe iterations, published false at 10002ms after
311 and after 567.
THE FIX IS ONE SCOPE: release the try-lock before the sleep. The waiter then
blocks on the mutex and takes it the moment the probe lets go, so each phase
completes in one or two iterations instead of hundreds (measured: 25-232
iterations and 6-60ms become 2 iterations and 0ms). Nothing about what is being
tested moves — the window this test needs is held open by gate1, not by timing —
and the 10s budget goes back to being a backstop instead of the mechanism.
The budget is deliberately NOT raised: that would have hidden the cause.
STILL A DETECTOR, checked by rebuilding with the defect this half exists to
catch (a second reapFinishedWaiters() below publishFinishedWaiter()): 10/10
caught on Linux, 10/10 on macOS, and 10/10 on Linux under the same 600x load, so
the reliability fix did not narrow the window. Every one of them fired on
bait2Gone — the registry-half invariant — not on a precondition.
Verified: nix build .#tests 289/289 on aarch64-linux and on aarch64-darwin; the
full gtest binary 3 times, 289/289 each; this test 50/50 on Linux (slowest
268ms), 50/50 on macOS (slowest 261ms), and 50/50 under the 600x load that
produced 3 failures in 25 before. Against CI's 10737ms pass and 10000ms failure,
the test now runs in ~260ms.
(cherry picked from commit
|
||
|
Dario LipicarandClaude Opus 5
|
c1b0a0f554 |
fix(startup): publish a token-only handshake surface before a module initializes (#42)
* fix(startup): publish a token-only handshake surface before a module initializes A module's initializer is synchronous and routinely calls out — a Qt module's initLogos, a cdylib's context-ready hook — including capability_module's requestModule, which capability answers by pushing a token back to that same module. The module's business object is published only once the initializer returns, so that push had nothing to reach: capability waited for a source that could not appear until the initializer returned, and the initializer could not return until capability answered. On Linux this wedged UI startup until the standalone app's 10s ui-host deadline expired and the view never rendered. Adds a second, deliberately tiny surface — ModuleHandshakeProxy, published under logos::handshakeObjectName(name) — carrying informModuleToken and nothing else. It forwards to the ModuleProxy that owns the token store, so a grant delivered early is the one the business object honours later, with the same authorization. The business object's publish timing is UNCHANGED, which is the point: a caller of a real method still blocks at acquire until the module is genuinely ready, exactly as it always has. An earlier attempt published the business object early and refused calls during init; that quietly turned a call that used to wait and succeed into one that returned empty, which old consumers cannot even detect. informModuleToken_module now tries the handshake surface first (short probe) and falls back to the business object, so modules built before this surface existed are reached exactly as they are today. It also reuses the cached handle instead of acquiring a fresh replica per grant, and takes a timeout (default unchanged). No wire change, no ABI change, no reply-shape change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(startup): do not treat a handshake refusal as the final answer The handshake surface is published before the target's initializer runs, so a target whose token store is seeded BY that initializer refuses a push that arrives first. Returning that refusal to the caller handed it an empty grant it could not distinguish from a real denial: measured on Linux, the first requestModule for wallet_backend_module came back empty in 29 of 34 runs, and never once in the pre-surface baseline. Fall through to the business object instead, which is what the caller got before this surface existed. The business object is published only once the initializer has returned, by which point the store is populated. The wait is bounded by the caller's own budget -- capability_module passes 3000 ms, not the 20 s default that made the original deadlock fatal -- so this cannot reintroduce the wedge. The companion change in logos-qt-sdk seeds the trust anchor before publishing, which removes the refusal at its source; this is the safety net for hosts and modules that do not. Also adds the regression test that would have caught this: the existing case seeds "core" before pushing, which is exactly the state that does NOT hold in the window the surface covers, so it asserted the surface works under a precondition production never met. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(startup): marshal the token push, and stop re-probing a missing handshake Two review findings from Copilot, both verified against the code before acting. 1. Thread affinity. informModuleToken_module was one of only two entry points in LogosAPIClient that did not wrap in logos::runOnOwnerThread -- requestObject, both invokeRemoteMethod forms and onEvent all do. The missing marshal is inherited, but THIS change is what made it reachable: the method used to take an uncached requestObject() + release() and touch no shared state, and routing it through acquireCachedObject put it on m_objectCache, which is declared single-threaded and holds thread-affine QtRO handles. Now marshalled, matching its four siblings. The 3-arg informModuleToken has the same gap but still uses an uncached handle and predates this work, so it is deliberately left alone rather than widened into this fix; noted at the call site. 2. No negative cache on the handshake probe. acquireCachedObject caches successes only, so a module built before the handshake surface existed failed the probe on EVERY grant -- and on QtRO that failure is a blocking waitForSource, i.e. 250 ms of dead time per token, forever. Remember the absence and go straight to the business object; cleared by clearObjectCache() so a reconnect, or a module reloaded from a build that has the surface, is re-probed rather than written off permanently. (The review attributed this cost to the Local/Plain adapters rejecting a non-ModuleProxy object. Checked per transport: plain is unaffected -- its token push is nameless fire-and-forget and it never had the acquire deadlock -- and on qt_local requestObject ignores timeoutMs entirely, so the cost there is a spurious warning, not 250 ms. The real cost is the missing negative cache, on QtRO.) The same review's ABI-break and name-collision findings were measured and do not apply: logos_protocol is a static archive with zero undefined imports of these symbols anywhere in the built stack, and object names are scoped to a per-module socket rather than a global registry. Both answered in-thread. 290/290 protocol tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(startup): exercise the handshake surface over a real transport The existing handshake cases call ModuleHandshakeProxy directly, with no transport underneath. That is what let a whole class of defect through: the surface is only useful if a transport will PUBLISH a token-only QObject and a consumer can ACQUIRE it by the derived name, and a direct-call test can see neither half. The adapter survey prompted by review found qt_local silently rejects a non-ModuleProxy on acquire while still reporting a successful publish -- invisible to every test in the suite. These run on the transport the production stack actually uses (QtRO, the LogosTransportConfig default), and model the startup window honestly: the handshake object is published and the business object deliberately is NOT, because it does not exist until the initializer returns. That window is the entire reason the surface exists and is the one state the direct-call tests could never represent. TokenReachesAModuleWhoseBusinessObjectIsNotPublishedYet the pre-init window end to end: publish -> probe by derived name -> acquire -> push lands on the provider. AnUnseededAnchorRefusesEvenThoughTheSurfaceIsReachable the transport-level twin of the gate test: proves the refusal measured in production (29 of 34 app runs) is the gate rejecting the push, not the transport failing to deliver it -- the provider is never reached. ALegacyModuleFallsBackAndIsNotReProbed a module with no handshake surface still gets its token, and the missing surface is probed ONCE. Timed rather than functional, so it was falsified before being trusted: with the negative cache removed the suite fails on exactly this case and no other. 293/293 protocol tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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
|
d0523c1486 |
fix(lp): lp_invoke_async can finally report a failure (#40)
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>
|
||
|
Dario LipicarandClaude Opus 5
|
3a31c91d13 |
fix(plain): close the RPC acceptor on the server's strand, not the caller's thread (#39)
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> |
||
|
Dario LipicarandClaude Opus 5
|
4db061aba3 |
fix(plain): close the RPC socket on the connection's strand, not the caller's thread (#38)
* 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> |
||
|
Dario LipicarandClaude Opus 5
|
72754ab9b2 |
feat(codec): Codec<std::optional<T>> — the ?T slot, two-state and canonicalising (#37)
`?T` had no C++ codec, so an optional slot could not cross the canonical JSON
wire at all: every spelling of "empty" landed on Codec<T>, which correctly
refuses null, and the value became a type error instead of an absence.
The contract this implements:
* TWO-state, never three. Every target has exactly ONE empty inhabitant (Rust
None, std::nullopt, an invalid QVariant, JS undefined), so "one LIDL type <->
one type per language" leaves nowhere to put a third state. std::nullopt is
that inhabitant.
* DECODE IS LIBERAL. Absent and explicit null are the SAME state coming in.
They cannot be told apart even in principle here — the record decoder
materialises a missing field as a null json (`j.contains(f) ? j.at(f) :
nlohmann::json()`) before a Codec ever sees it.
* ENCODE IS CANONICAL. Empty has one spelling out: null. A round trip
therefore CANONICALISES rather than reproducing its input.
* A PRESENT VALUE IS STILL TYPE-CHECKED. Optional widens the domain by exactly
one inhabitant; it does not switch checking off. Anything non-null goes
through Codec<T> unchanged and throws with the same path it would have in a
required slot. A required slot is untouched — null there still means "wrong
type", which is the only reason absent-means-empty is safe to allow here.
KEY OMISSION IS NOT IN THIS LAYER, and the comment says so at the definition.
Empty is spelled by omitting the key where the slot is NAMED (a record field)
and by null where it is POSITIONAL (argument, return, event parameter — no key
to omit, and arity must never change). A Codec is handed a VALUE and cannot see
the slot it sits in, so it emits the positional spelling; skipping the key for a
nullopt field belongs to the record emitter in logos-cpp-sdk, the only code that
knows there IS a key. It is also unimplementable one level down: an optional
inside a [T] must still occupy its array position.
Ten tests: absent, explicit null, present, present-but-wrong-typed (including
the path inside a container), null still rejected in a required slot, ?bstr
(tagged at depth, and present-but-EMPTY bytes staying present), ?[T] / ?{tstr:T}
separating `[]` from missing, [?T] / {tstr:?T} keeping position and key, and ??T
collapsing.
The tenth pins a trap rather than a feature: JsonArg cannot deliver an optional.
std::optional's converting constructor optional(U&&) binds an rvalue reference
to the proxy prvalue, which out-ranks JsonArg's const-qualified conversion
function before partial ordering is consulted, so the compiler decodes X instead
of std::optional<X> and null throws. Both alternatives were tried and measured:
an rvalue-qualified conversion operator ties with the constructor (ambiguity
error), and one written specifically for std::optional still loses. There is no
signature that wins, so optional parameters must NAME the type —
fromJson<std::optional<X>>(j, path), which is what the cdylib backend already
emits. A present value survives the proxy by accident, which is exactly why the
empty case is pinned.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
ec43a0b441 |
feat(json-convert): jsonToLogosResult — the missing inverse of a converter we already had (#35)
qvariantToNlohmann has always owned LogosResult -> {success,value,error}. The
way back did not exist: nlohmannToQVariant turns that object into a plain
QVariantMap, and a qvariant_cast<LogosResult> of a QVariantMap yields a
default-constructed, silently-failed result. So every consumer that received a
`result` over the canonical JSON wire either re-derived the decode or lost it.
The pair is now symmetric, and both fields recurse through the canonical
decoder — so a `value` carrying bytes / 64-bit integers / containers comes back
with the shape the encoder sent, and a null `error` stays an INVALID QVariant
rather than becoming an empty QString. That last state is the point: it is what
the Qt transport delivers for "no error", and no std::string-typed intermediate
can carry it.
Tests pin the round trip, the absent-error state, bytes + uint64 inside `value`,
and the non-object input.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
4ee85b26a6 |
test(codec): adopt the tagged-bytes coverage from logos-cpp-sdk (#34)
logos-cpp-sdk's tests/sdk/test_logos_json_bytes.cpp tested b64UrlEncode / bytesToJson back when logos_json.h carried its own copies. Those copies are gone (logos-cpp-sdk#117), so the coverage belongs with the canonical definitions rather than in a repo that has to reach across for the header — reaching across is what broke that test target after the dedupe. Six cases: all 256 byte values, the URL-safe alphabet and no padding, every tail length 0-5, an embedded NUL, the canonical tag shape, and padded input decoding. That last one is the one with history. The cdylib backend used to carry a SECOND decoder that bailed on any non-alphabet character, so padded input silently produced an EMPTY vector while this test pinned the opposite for the shared helper — two copies contradicting a committed test in the same repo. There is one decoder now, and this is what it must satisfy. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
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 5
|
8b8a358c8b |
fix: uint64 survives the event path and the plain wire (#30)
* 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>
|
||
|
Dario LipicarandClaude Opus 5
|
362b03fb1e |
feat(codec): one canonical LIDL ↔ JSON codec, generic over composition (#29)
* 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>
|
||
|
Dario LipicarandClaude Opus 5
|
ae2f7e1b58 |
fix(lp): create Qt-affine clients on the Qt main thread (#28)
lp_client_create() made the CALLING thread the client's owner thread. Callers reach it through a lazily-created wrapper (the generated bind_<iface>() -> LpClient::ensure()), so the first thread to make an outbound call captured the whole transport for the life of the process. For the qt_remote transport that thread also ends up owning the QRemoteObjectNode and its QLocalSocket, which are only serviced by a thread running a Qt event loop. A module whose first call came from a worker — an HTTP handler, a timer thread — bound its transport to a thread that only pumps events while it is already blocked inside a call. Replica acquisition then never completed: every requestObject() burned its full 20s timeout and returned nullptr, and since a failed acquire yields an empty result the data loss was silent. openmetrics-module hit exactly this: one GET /metrics took 40s (2 x 20s) and came back missing a module, /health went unanswered behind the wedged libmicrohttpd thread, and the follow-up stop RPC failed. Construct the client on the Qt main thread when the transport needs a Qt event loop, so the per-call marshal that already exists (logos::runOnOwnerThread) lands somewhere that can actually service it. This is the anchor the Qt path always had — LogosAPI::getClient marshals construction to the LogosAPI's thread — given to the lp path. Plain (tcp/tcp_ssl) and mock transports are Qt-free and thread-agnostic, so they keep the calling thread: a worker-thread consumer stays off the main thread's back. LogosTransportFactory::needsQtEventLoop() carries that rule next to the createConnection resolution it mirrors. When there is nothing to anchor to (a Qt-affine transport with no QCoreApplication) we now warn instead of letting it surface as a mute timeout. Tests: a worker thread creates an lp client over qt_remote and calls a provider published on the main thread; passes in ~0.15s, and with the construction hop reverted fails after 24.8s / 49.9s — the acquire timeouts themselves. Plus a truth table for needsQtEventLoop. 183/183 protocol tests pass. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario LipicarandClaude Opus 4.8
|
8ede8ece08 |
fix(lp): destroy clients on their owner thread (#27)
lp_client_destroy() called `delete` on the LogosAPIClient directly, on
whatever thread happened to release the last handle share. That thread is
not always the owner: any binding that parks a client share in a worker —
a Rust EventSubscription moved into a bridge thread, for one — runs the
destroy there when the worker exits.
Deleting the client there destroys its consumers' transport objects off
their owner thread. With Qt Remote Objects that tears down the node's
QLocalSocket and its socket notifiers cross-thread; Qt warns ("socket
notifiers cannot be enabled or disabled from another thread"), the fd
closes under the owner's event dispatcher ("Invalid socket N with type
Read, disabling..."), and the process takes SIGSEGV. Observed as
chat_module crashing on shutdown, when joining its bridge worker dropped
the last delivery_module share on that worker.
Defer the teardown to the owner thread via deleteLater() when the caller
is elsewhere, mirroring the marshaling every call path already does with
logos::runOnOwnerThread. A blocking marshal is not usable here: the owner
is typically the dispatch thread and may be blocked joining the very
worker running the destroy. Deferring is invisible to callers because the
callback guard, not the delete, enforces the ABI's "no callbacks after
this returns" contract.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 4.8
|
6401e30ae1 |
feat: group-shareable local sockets, stale-socket reaper, bind-failure detection (#20)
* 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>
|
||
|
Dario LipicarandClaude Opus 4.8
|
ef24bd70d9 |
fix(protocol): re-exchange token on provider rejection (#26)
When a provider rejects a call for a stale/unrecognized token it now returns a structured "unauthorized" sentinel (logos_rpc_status.h) instead of a bare QVariant(). LogosAPIClient detects it below the typed wrapper, drops the cached token, re-runs capability_module.requestModule and retries the call once — closing the gap where a stale token was reused forever (the consumer-latched-dead failure mode) and lazily recovering the common provider-reload case. The return VALUE is the only provider->consumer channel available on every transport (qt_local/qt_remote/plain) without an ABI break, since the QtRO dispatch slot returns a single QVariant — hence a value sentinel. Backward compatible: - OLD consumers convert the sentinel identically to QVariant() for every scalar/string/LogosResult return, so they keep seeing today's empty/failed result. - OLD providers return bare QVariant(); a NEW consumer never matches the sentinel and so never re-exchanges against them. The retry is bounded to one attempt and fires ONLY on the explicit sentinel (never a legitimately-empty result), so no loops and no misfire. Downstream note: logos-qt-sdk's test_auth_token_enforcement.cpp asserts !isValid() on unauthorized calls; those become isUnauthorizedSentinel() when it re-pins (the security property — no provider dispatch — is unchanged). Tests: tests/protocol/test_token_reexchange.cpp covers provider-side emission, sync/async re-exchange+retry, bounded retry (no loop), the false-positive guard (a legit empty return must not re-exchange), and old-consumer decode. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Dario Lipicar
|
664b43f18a |
perf(qt_remote): cache the remote-object handle per name in LogosAPIConsumer (#24)
Acquiring a QtRO replica per call (acquireDynamic + waitForSource) is expensive:
under a tight loop — e.g. a proxy forwarding every method to its target, or a UI
backend driving a whole surface — it dominates and can even starve the nested
synchronous calls. Cache the LogosObject handle per object name in m_objectCache
and reuse it across calls (both the sync invokeRemoteMethod and the async
invokeRemoteMethodAsync paths); no per-call release(). A stale handle (source
went away — module unloaded / transport dropped) is detected via a new
LogosObject::isValid() (QtRO replica state == Valid) and transparently
re-acquired. The cache is released in clearObjectCache() from the destructor and
before reconnect().
- logos_object.h: add virtual bool isValid() (default true).
- qt_remote/remote_transport.{h,cpp}: RemoteLogosObject::isValid() (replica
Valid state) + a process-wide acquireCount() test hook.
- logos_api_consumer.{h,cpp}: m_objectCache + acquireCachedObject()/
clearObjectCache(); sync + async reuse the cached handle; async keeps the
QPointer guard and never releases the shared handle from its callback.
Test: RemoteEventTest.ConsumerReusesCachedHandleAcrossSyncAndAsyncCalls publishes
a provider over the qt_remote host, does 12 sync + 12 async echo calls, and
asserts every result is correct AND acquireCount() == 1 (one replica for all 24
calls). 164/164 green.
|
||
|
Dario LipicarandClaude Opus 4.8
|
d5ba950313 |
fix(json): keep nested bytes/ints tagged in qvariantToNlohmann containers (#23)
qvariantToNlohmann ran its canConvert<QJsonObject>/<QJsonArray> fallbacks BEFORE
the type-preserving QVariantList/QVariantMap recursion. A QVariantList/QVariantMap
also reports canConvert<QJson*>()==true, so a container was routed through QJson —
which has no byte type and degrades numerics to double. A nested QByteArray was
therefore flattened to a plain string, losing the canonical {"_bytes":...} tag.
Concretely this broke bstr method ARGUMENTS to cdylib (Rust) modules:
LogosProviderObject::callMethodStdBridge feeds each call arg through
qvariantToNlohmann, and a bstr arg arrives (over QtRO) as a QByteArray nested in
the QVariantList of call args. It was flattened to "hello", so the cdylib's
{"_bytes":...} decoder produced an empty Vec (e.g. echoBytes returned null). The
QtRO C++ path was unaffected (native QByteArray marshaling) and the plain-lp path
was already correct; only the container-through-QVariant leg dropped the tag.
Fix: move the container recursion (QStringList/QVariantList/QVariantMap) ahead of
the QJson fallbacks so nested elements recurse element-by-element (bytes stay
tagged, integers stay integers); only genuine QJson-typed variants reach the
fallbacks. Adds nested-bytes-in-list/map + bridge-shape regression tests.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 4.8
|
4775e635ff |
fix(json-convert): preserve integer types inside QVariant containers (#21)
* fix(json-convert): preserve integer types inside containers qvariantToNlohmann() kept integer QVariant types only for a top-level scalar; a QVariantList/QVariantMap fell through to QJsonValue::fromVariant, which degrades every numeric to double at every depth. So a `[int]`/`[uint]`/ `[float64]`/`[bool]` method arg (a QVariantList of ints) arrived as a float array, and the generated cdylib dispatch's strict .get<std::vector<int64_t>>() threw -> the param decoded as an EMPTY vector. Surfaced by a UI plugin driving [int] method args over QtRO. Recurse into QVariantList/QStringList/QVariantMap element-by-element so nested integers keep their type (and bytes/maps/lists keep their shape); also route the LogosResult value through the same recursion. Adds JsonConvertInts tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: pin LongLong container test with values > 2^53 Copilot review: (10, 20) survive an accidental IEEE-754 double detour, so they did not actually pin the integer-preservation regression. Use 2^53+1 and INT64_MAX, which lose precision / serialize in scientific notation if degraded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Khushboo Mehta
|
84be236552 |
fix(logos_api_client): cache minted capability token on the client side
Without a client-side cache, every sync invokeRemoteMethod re-mints a fresh capability token. On Linux, QtRO's waitForFinished() spins a nested QEventLoop that dispatches queued slots mid-wait, so back-to-back calls reenter the function and each fires its own requestModule. The target stores ONE token per caller (TokenManager::saveToken replaces) — last inform wins and earlier in-flight calls arrive with a superseded token, rejected by ModuleProxy::isAuthorized as "auth token not recognized". Fix: save the minted token into the client's TokenManager after a successful requestModule on both the sync path and the async drain callback. Subsequent calls short-circuit the handshake — one mint per (client, target), no rotation. |
||
|
Dario LipicarandClaude Opus 4.8
|
b0c6f75498 |
fix(qt_remote): defer async completion delivery off the QtRO read stack (#7)
A `concurrency:"multi"` call's result comes back as a deferred completion event (`__logos_call_complete__`), delivered by RemoteEventHelper::onEventResponse — a slot fired by the replica's eventResponse signal. Cross-process, that slot runs on QtRO's read stack (QRemoteObjectNodePrivate::onClientRead). Until now the async user callback was invoked *inline* there, and that callback routinely (a) emits a module event — which the host-side ModuleProxy serializes onto the QtRO source — and (b) release()s the client object. Doing either while onClientRead is still unwinding re-enters QtRO and corrupts the node: a SIGSEGV in onClientRead (EXC_BAD_ACCESS, KERN_INVALID_ADDRESS at 0x80). This is the crash the EVM wallet backend hit from refresh_balances, which fans balance reads out to eth_rpc via call_async and then emits `balances_updated` from the gather completion. Primary fix (remote_transport.cpp): deliver the async completion callback on the next event-loop turn via QTimer::singleShot(0, m_helper, …) instead of inline, so all user code (event emits, release(), further calls) runs after onClientRead has fully unwound. m_helper is the context so the callback is dropped if the object is torn down first. Defense-in-depth for the same re-entrancy class: - remote_transport.cpp release()/disconnectEvents()/dtor: deleteLater() the helper (signal receiver) and replica (signal sender) and disconnect first, instead of deleting them inline — deleting a QObject mid-emission corrupts the connection list Qt is iterating. - module_proxy.cpp: always queue the source eventResponse emit to the owning thread (Qt::QueuedConnection), never emit inline, so a module that emits from inside a same-thread dispatch can't re-enter QtRO's source serialization. Tests (tests/protocol/test_remote_transport_events.cpp, newly wired): qt_remote LocalSocket event delivery (direct + full provider chain) and a reentrant-release regression that drives release() from inside a deferred-completion callback. The hard crash only reproduces cross-process (in-process QtRO posts the event, so the read stack has already unwound) — the cross-process guard is the wallet Anvil integration doctest, where this fix is A/B-proven: the published backend crashes on refresh_balances, the patched backend returns balances cleanly. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Dario LipicarandClaude Opus 4.8
|
315a3a2e0a |
fix(qt_remote): defer async completion delivery off the QtRO read stack (#7)
A `concurrency:"multi"` call's result comes back as a deferred completion event (`__logos_call_complete__`), delivered by RemoteEventHelper::onEventResponse — a slot fired by the replica's eventResponse signal. Cross-process, that slot runs on QtRO's read stack (QRemoteObjectNodePrivate::onClientRead). Until now the async user callback was invoked *inline* there, and that callback routinely (a) emits a module event — which the host-side ModuleProxy serializes onto the QtRO source — and (b) release()s the client object. Doing either while onClientRead is still unwinding re-enters QtRO and corrupts the node: a SIGSEGV in onClientRead (EXC_BAD_ACCESS, KERN_INVALID_ADDRESS at 0x80). This is the crash the EVM wallet backend hit from refresh_balances, which fans balance reads out to eth_rpc via call_async and then emits `balances_updated` from the gather completion. Primary fix (remote_transport.cpp): deliver the async completion callback on the next event-loop turn via QTimer::singleShot(0, m_helper, …) instead of inline, so all user code (event emits, release(), further calls) runs after onClientRead has fully unwound. m_helper is the context so the callback is dropped if the object is torn down first. Defense-in-depth for the same re-entrancy class: - remote_transport.cpp release()/disconnectEvents()/dtor: deleteLater() the helper (signal receiver) and replica (signal sender) and disconnect first, instead of deleting them inline — deleting a QObject mid-emission corrupts the connection list Qt is iterating. - module_proxy.cpp: always queue the source eventResponse emit to the owning thread (Qt::QueuedConnection), never emit inline, so a module that emits from inside a same-thread dispatch can't re-enter QtRO's source serialization. Tests (tests/protocol/test_remote_transport_events.cpp, newly wired): qt_remote LocalSocket event delivery (direct + full provider chain) and a reentrant-release regression that drives release() from inside a deferred-completion callback. The hard crash only reproduces cross-process (in-process QtRO posts the event, so the read stack has already unwound) — the cross-process guard is the wallet Anvil integration doctest, where this fix is A/B-proven: the published backend crashes on refresh_balances, the patched backend returns balances cleanly. Co-authored-by: Claude Opus 4.8 (1M context) <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
|
9de4165ab6 |
Qt split + module authoring groundwork: LogosProviderPlugin + the common module-impl C ABI (#3)
* 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.
* Move LogosProviderPlugin into logos_provider_interface.h
Plugin-loading tools (logos-cpp-generator's introspection mode, lm, the
hosts) need only qobject_cast<LogosProviderPlugin*>() + the abstract
LogosProviderObject — both framework-internal. Hosting the detection
interface here keeps those tools off the developer-facing logos-qt-sdk
layer. Same iid (org.logos.LogosProviderPlugin); header-only, ABI-neutral.
* Define the common module-impl C ABI (logos_module_impl.h)
ONE cdylib contract for module implementations in every language:
dispatch / get_methods / set_context / set_emit_callback / accept_token
/ get_protocol_version / string_free. The C++ and Rust SDKs emit these
exports around their respective impls; the uniform generated Qt glue
(and later a no-Qt host) talks to the cdylib only through this ABI.
JSON data model and tagged bytes form match the lp_* consumer ABI; the
protocol-version handshake complements the build-time metadata stamp.
* json convert: integers stay integers across the C ABI
QJsonValue::fromVariant degrades every numeric to double, so Int/UInt/
LongLong/ULongLong QVariants serialized as 5.0 — and a strict consumer on
the other side of the C ABI (a generated dispatch reading an int param)
rejects or zeroes them. Surfaced by the first cdylib-authored module
whose inbound args cross qvariantToNlohmann; the dlopen smoke harness
fed hand-written int JSON and never exercised this edge.
* call-error channel: surface {code,message,origin} for unacquirable targets
invokeRemoteMethod could not distinguish a failed call from a void/null
result — lp_invoke returned LP_OK with a null JSON result even when the
target module was never reached, and generated typed wrappers silently
defaulted (0 / empty string). Additive err-out overloads on
LogosAPIConsumer/LogosAPIClient fill a std-only logos::CallError
(logos_call_error.h, new LogosCallError exception for the generated
wrappers to throw); lp_invoke now honors its documented contract for
this class of failure: LP_ERR_UNAVAILABLE + canonical error JSON.
First detectable code: object_unavailable (requestObject failure) —
the struct is the extension point for transport-level statuses.
* call-error: drop the exception type — the error channel is the out-param
Per review, generated wrappers expose CallError as an optional trailing
out-parameter instead of throwing; the struct is the whole contract.
* ci: build + run the protocol test suite
On every pull request (unfiltered — stacked PRs included), master pushes,
and manual dispatch. The repo shipped without CI; its 111-test suite only
ran locally and through the workspace gate.
* 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.
* ci: DeterminateSystems nix installer (macOS runners)
cachix/install-nix-action fails on the macOS runners with
eDSRecordAlreadyExists (pre-existing nix build users); the org's
macOS-bearing workflows use the DeterminateSystems installer.
|
||
|
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.
|