mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-27 12:01:15 +00:00
master
62
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dario Gabriel LipicarandClaude Opus 5
|
303ab08d4c |
fix(deferred): let onEventWhenAvailable take the wildcard, like onEvent
An EMPTY event name means "every event on this object". LogosObject::onEvent
has always honoured it -- RemoteEventHelper appends the callbacks registered
under QString() to every dispatch, and PlainEventSubSharingTest.
ANamedAndAWildcardSubscriberEachGetOneCopy already pins that on the plain
transport. onEventWhenAvailable refused it.
There was no reason for the refusal, and I looked for one before removing it:
* the guard is a single `objectName.isEmpty() || eventName.isEmpty() ||
!callback` line from the original commit (#47), whose message never
mentions wildcards;
* nothing anywhere asserted the refusal;
* the registry already carries empty event names -- whenObjectAvailable()
adds its readiness entries with exactly that, so add(), takeMatching(),
pending() and reviveArmed() have always handled them;
* the arm path is `handle->onEvent(e.eventName, e.callback)`, which passes
the name straight through, so the wildcard needs no code of its own.
It was a category error: an empty objectName and a null callback are unusable,
while an empty eventName is meaningful. Lumping the three together silently
denied the deferred path to every hand-rolled wildcard subscriber, leaving them
on exactly the one-shot requestObject() + onEvent() this class exists to
replace. logoscore's `watch <module>` with no --event is one such caller, and
had to route around it through whenObjectAvailable().
pendingEventSubscriptions() now renders a wildcard as `<module>::(any)` rather
than a truncated `<module>::`.
Three tests, in the style of the file: subscribe-before-publish (firing two
DIFFERENT event names, because one would pass for a subscription that merely
matched the empty string against nothing), its publish-first control, and the
refusals that REMAIN -- pinned so widening the guard cannot quietly widen it
further, including that a refusal still ANSWERS via onArmed(false) rather than
going quiet.
Negative control: with the tests present and the guard restored, both wildcard
tests fail and the refusal test still passes. With the change, the full suite
is green (540 tests).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario Gabriel LipicarandClaude Opus 5
|
42460e5b2a |
fix(tokens): separate INBOUND from OUTBOUND, without moving a single byte
A grant one way was a grant both ways. TokenManager was ONE flat map with no
direction tag, written from both sides of every relationship: LogosAPIClient
stored the token it will PRESENT to a callee under the CALLEE's name, and a
token RECEIVED from a caller was stored under the CALLER's name. Same key
namespace, last write wins.
Measured on the shipped fleet with two ordinary modules doing nothing unusual:
one grant A -> B leaves the SAME token value under both opposite-meaning keys,
and the never-granted B -> A call then succeeds. Silently.
A.callOther(B, ping) CALL_OK
T1 A holds token for B? val=7685c776-...
T1 B holds token for A? val=7685c776-... <-- one value, two meanings
B.callOther(A, ping) CALL_OK <-- never granted
WHY THE LAYOUT COULD NOT CHANGE. TokenManager's layout is a cross-package ABI:
the host ALLOCATES the object and module/UI-plugin images MUTATE it through
their own statically-linked accessors — and host and modules ship as separate
.lgx that mix versions at runtime by design. The header's ABI-safety note is
about ALLOCATION ("no consumer allocates one, none needs sizeof"); the hazard
is MUTATION.
Splitting into three members took sizeof 32 -> 64 and moved m_mutex 24 -> 56.
QMutex::fastTryLock() compare-exchanges at this+24, which in that layout is
m_inbound's QHash d-pointer. Empty, the old code silently borrows the hash's
pointer slot as a mutex and puts it back, so it LOOKS fine; non-empty, the
exchange fails and lockInternal() interprets the QHash Data* as a
QMutexPrivate* and futex-waits on it — hung forever, inside a token-store
write, on the module host's Qt main thread. No crash, no log line, no timeout
that recovers. Reproduced by calling the shipped 0.6 plugin's own saveToken on
a 0.7 object: exit=124.
So direction lives in the KEY NAMESPACE instead. Outbound is the bare peer name
(byte-identical to master); inbound is "\x01in\x01" + caller. m_tokens@16,
m_mutex@24, sizeof 32 — measured identical to master in every shipped image,
pinned by a static_assert against a reference struct that fires if a member is
added.
Two things a key namespace forces that separate members did not: every door
REFUSES a key carrying the namespace character, or a wire-supplied caller name
could forge across the direction boundary; and credential() is DERIVED from
bootstrapKeys() rather than cached, because a cached field reads empty on a
store another image wrote and then refuses every push.
AN ANCHOR KEY IS NO LONGER SPELLED AS A MODULE NAME. scanIssuedTokens' m_tokens
loop offered every matched key unconditionally while the m_store loop
deliberately never offers, so "an anchor must never name a caller" was enforced
on one side only. A module announcing itself as "core" — which logos-rust-sdk
did unprompted — therefore authorized as kind:module name:core. The rule
generalises: a store may only name a caller with a key it alone can write.
Implemented as a masked operand, so the comparison count is unchanged;
RefusingToNameAnAnchorKeyCostsNoComparison pins that via
logos::tokenComparisonCount().
lp_token_save / lp_token_save_for now return LP_ERR_INVALID_ARG on a reserved
key instead of LP_OK. Only the return code was wrong; saveToken already refused.
PROTOCOL 0.8: logos_module_accept_inbound_token joins the module-impl C ABI
(12 exports). onInit keeps logos_module_accept_token for the module's own
anchor — that one IS outbound, and merging the two paths is what reintroduces
the bug.
Supersedes the field-split approach; the semantics are unchanged from it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario Gabriel LipicarandClaude Opus 5
|
b37a2e9f1c |
feat(tokens): a private store is created EMPTY, not seeded with the host anchor
TokenManager::forIdentity seeded every new private store by COPYING the
host's tokens for bootstrapKeys() = {core, capability_module}. Those values
are the HOST's, so an isolated in-process consumer presented basecamp's
anchor and ModuleProxy::resolveCaller answered HostAnchor: a sandboxed view
wearing the host's authority.
Half of that was LIVE, not latent. informModuleToken's trusted-channel gate
compares against the SAME two keys, so anything holding an isolated
LogosAPI* could read getToken("capability_module") and call
informModuleToken on capability_module — three public calls, no glue — and
write into the map that is both its known-caller gate and its moduleToken
source. Reading a caller needs generated glue; writing one did not.
The copy could not simply be deleted. Measured: removing it alone turns 5
of 495 protocol tests red, and two are behavioural — an isolated identity
cannot reach capability_module.requestModule (it dies at ModuleProxy's
`authToken.isEmpty()`), and an isolated PROVIDER can never be told about a
caller. Isolation without a credential is a lockout.
The credential already existed and was being thrown away. All five host
registration sites minted a per-spawn UUID, registered it with
capability_module, and then dropped it: the identity was registered under a
token nobody held, and it worked only because the store presented the
copied anchor. The anchor copy was masking that at every site, which is why
neither could be fixed alone.
So: a private store starts empty, and an identity's store carries THAT
IDENTITY's own host-issued credential under the bootstrap keys —
adoptCredentialFor, which refuses the host anchor by construction. This is
not a new rule. ui-host already does exactly it for the out-of-process half
(saveToken(core/capability_module, its own authToken)), and
LogosAPIProvider::seedHandshakeTrustAnchor does it for a module image. The
in-process private store was the only store in the system seeded with
somebody else's credential.
`core` is not part of it for a CONSUMER: every reader of a store's "core"
entry is provider-side, and in a real host instance() has no "core" key at
all — the host ring is written only under module names, and no module is
named core.
Closing the elevation also makes the consumer NAMEABLE in the same change:
it now resolves as {"kind":"module","name":<identity>} at capability_module
and at ordinary modules, instead of {"kind":"host"}.
NOTE FOR CONSUMERS OF THE C ABI: lp_token_reset_identity changed meaning on
an existing exported symbol — it no longer re-seeds, so an out-of-tree
caller that reset and kept going is now locked out. No in-workspace caller
exists; carried by the MINOR bump to 0.7.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario Gabriel LipicarandClaude Opus 5
|
6c24fcb132 |
feat(caller): resolve who is calling, and declare the export that carries it
A module can now learn which module is calling it. Not via the LIDL — this
is not part of any module's interface, and the callee already has the
identity from the token the call carried; the only question was surfacing
it. So it is ambient: logos::currentCaller(), no declared parameter, no
contract change, no per-method opt-in.
WHAT THIS PR CONTAINS
* LogosCaller — Unknown | HostAnchor | Module{name, instance?} |
Derived{parent, leaf} | Operator{name} — std-typed and Qt-free.
* CallerScope, an RAII save/restore around a thread-local STACK. Not a
slot: A calling B calling back into A on one thread must nest, and an
exception thrown from a handler must still pop.
* resolveCaller, replacing the bool fold in ModuleProxy. It reads the
INBOUND store #69 made direction-pure — the only store that may
legitimately name a caller.
* logos_module_set_call_caller DECLARED, and MINOR 5 -> 6.
WHY AMBIENT, AND WHY IT MUST CROSS AN IMAGE BOUNDARY
LogosProviderObject::callMethod is a vtable slot, and this codebase avoids
vtable changes on purpose. But the deeper reason is measured, not stylistic:
nm on real binaries shows the host and the module plugin EACH define
ModuleProxy::callRemoteMethod and TokenManager::instance, each with its own
function-local static at a distinct address, and neither with a single
undefined reference to the other's. Mach-O is TWOLEVEL; PE has no
interposition. A thread_local opened host-side is NOT the one a handler
reads. Since --backend qt is now refused outright, every module is a cdylib
and the C ABI push is the only path, not a fallback.
The pull is only safe through QMetaObject::invokeMethod on the host's
LogosAPI, because metaObject()/qt_metacall are virtual and the vptr was
written by the host's constructor — LogosAPI is duplicated across images
too, meta-object included, so a direct call would bind to the plugin's copy
and read the plugin's TLS, silently empty forever. A dynamic property
cannot carry it either: one process-global slot, so two overlapping
concurrency:"multi" calls from different callers would clobber each other.
Nothing here is spelled "verified". capability_module checks only that an
asserted name EXISTS as a key, so the strongest honest word is token-bound.
HostAnchor carries no name because core and capability_module hold one
token VALUE under two keys by construction. Unknown is the fail-closed
value and is always in-band, never spelled by absence.
The constant-time fold survives: the matched key is accumulated into a
fixed-width buffer with no data-dependent branch, verified at the
instruction level (csel, not a branch) with the comparison count invariant.
THE BUMP IS SAFE BECAUSE THE BACKENDS WENT FIRST
logos-protocol only DECLARES this ABI; every backend owes the definition,
and that gap shipped twice. logos-cpp-sdk#147 and logos-rust-sdk#47 already
define logos_module_set_call_caller, gated on >= 0.6 and therefore inert
until this lands. Verified on x86_64-linux: with this tree as the protocol,
BOTH backends at master pass their ABI checks and define the export;
manifest reports 0.6.0 with 11 exports. No repo is red at any point.
Rule 6 is now normative on a point the two backends had silently diverged
on — a present-but-unreadable "instance" is dropped and the module still
identified — each having pinned its own answer with a passing test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario Gabriel LipicarandClaude Opus 5
|
d872024847 |
fix(proxy): authorize against a store that can only hold INBOUND tokens
TokenManager is one flat QHash<QString,QString> with no direction tag, and both directions write into it under a bare module name: the client saves the OUTBOUND token under the CALLEE's name, the provider saves the INBOUND token under the CALLER's name. Last write wins. So a reverse lookup there can name a module we CALL as the module CALLING us — affirmatively wrong, and worse than answering "unknown". The per-identity work did not close this. `forIdentity` splits by CALLING IDENTITY, a different axis, and it returns `&instance()` for every identity until `isolateIdentity` runs — whose only production caller is LogosQmlBridge, on the consumer side. For providers the mechanism is inert. What has kept it from being an auth hole is TOPOLOGY, by accident rather than design: with a module as a cdylib in its own host process, the glue splits the directions across two IMAGES — informModuleToken reaches the host's store, logos_module_accept_token the cdylib's. Any single-image configuration puts them back in one map: the in-process plugin host that Basecamp already uses, local/mobile mode, the shared-runtime migration, and this repo's own test suite. So ModuleProxy now takes an optional token store and authorizes against THAT, defaulting to &instance() — every existing two-argument construction is byte-identical. Its own m_tokens becomes the inbound record and is documented as the only store that may name a caller, which is the property caller-identity recovery will need. Two things found on the way, both worth their own attention: * m_tokens had ZERO production writers. Its only feeder is LogosAPIProvider::saveToken, which nothing in 43 repos calls, so the store was permanently empty in production. * The isAuthorized/getTokenManager split breaks BOTH ways under isolation: privately seeded tokens are invisible AND every ambient token is still accepted, re-opening the escalation isolation exists to close. The glue comment asserts the opposite. A test changed the design. The first draft recorded the token BEFORE forwarding to the provider, justified by a re-entrancy window. That test went red, and lp_module_accept_token turned out to reach module code only as far as a store write — it calls nothing back. No window, so the record moved after the provider's verdict. Proven red-then-green in three builds: with neither mechanism 3 of 7 fail; with the record but the scan still on instance() exactly one survives, and that survivor is what makes it a detector for the SCAN rather than the record; with the anchor read reverted, the anchor test alone fails. Three tests are pins that hold on both sides and are labelled as such. The constant-time fold is preserved: only the receiver object changed. 483/483 protocol tests, the module-impl ABI check, and the mingw cross all pass; downstream logos-qt-sdk is 239/239 with this tree overridden in (confirmed reaching by the differing store path). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario Gabriel LipicarandClaude Opus 5
|
79894727e5 |
docs(version): the obvious way to guard a conditional surface is wrong
Two corrections, both to claims this header makes about its own versioning. 1. It PRESCRIBED the buggy guard. The 0.5 note told codegen to write `LOGOS_PROTOCOL_VERSION_MINOR >= 5`, and two emitters duly do (logos-cpp-sdk's lidl_gen_cdylib.cpp and logos-plugin-qt's glue). At 1.0.0 the MINOR resets to 0 and every such guard silently goes false. Nothing fails to build and nothing fails to load — the definitions and the calls disappear together — so the symptom is modules quietly losing teardown and grantability, with no diagnostic anywhere. The rule now sits above the version macros, with the expanded arithmetic spelled out and a note on why it must NOT hide behind a function-like macro: the generated sources are resolved by unifdef in the backends' ABI checks, and unifdef silently no-ops on what it cannot evaluate. logos-rust-sdk already compares the (major, minor) tuple. 2. It repeated the compatibility claim already corrected in logos_module_impl.h — that the teardown pair is safe because the glue is generated alongside the module. That does not follow, and the ABI has been broken twice on the strength of it. Being generated in the same build makes the two agree on the VERSION; it says nothing about which SYMBOLS a backend's emitter writes for that version. The wrong version of this reasoning living in two headers is how it survived the first correction. Documentation only; no macro or value changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario Gabriel LipicarandClaude Opus 5
|
4d9e3e740a |
docs(shared-api): describe the scheme that actually shipped
PR-7 of the shared-runtime migration. Comment-only: no non-comment line changes,
and checks.tests passes.
This header is the canonical explanation of the one-runtime invariant, and after
the migration it described a scheme that no longer exists. It still said the
resolution was ONE PROVIDER with liblogos_core exporting types it does not own,
and still pointed at logos-basecamp/cmake/LogosSharedFromDll.cmake -- a file
deleted in logos-basecamp#348 and logos-logoscore-cli#98.
Now states where definitions actually live:
liblogos_protocol TokenManager, LogosAPIClient, the StoreRegistry
liblogos_qt_host LogosAPI
and that liblogos_core defines ZERO runtime symbols, importing them like every
other consumer.
RECORDS THE FIX THAT DID NOT WORK, because it looks obvious and someone will try
it again: hand-marking a class list with __declspec(dllexport) is not a smaller
version of the right answer. It exported 116 symbols and the Qt host runtime
still failed to link with ELEVEN undefined references across five classes, plus
free functions nobody had marked. A curated list is correct only until the next
consumer touches a symbol nobody thought of, and the failure lands in a
downstream repo far from the cause. Hence a .def GENERATED from the objects.
STATES THE IN-PROCESS / OUT-OF-PROCESS SPLIT, which is the distinction that
decides who links what and the one most likely to be "simplified" away by
someone tidying up. In-process images link the SHARED libraries; logos_host,
ui-host, module plugins and ui_qml backends keep linking the STATIC archive, and
that is CORRECT rather than a leftover: each runs in its own process, so its own
copy IS the right per-process singleton -- measured, logos_host and ui-host
define ~100 runtime symbols each and are deliberately exempt from the symbol
gates. Staying static also keeps a .lgx self-contained, since a .lgx records an
EMPTY nix closure and a shared library would not travel with it.
Says plainly not to unify the two, and why: it would couple every .lgx to the
exact runtime build it was packaged against, to fix a duplication that is not a
bug in a separate process.
Ends with what asserts any of this, since nothing did for a long time and that
is how nine images came to define the same singleton: symbol gates in
logos-basecamp, logos-logoscore-cli and logos-standalone-app, each shipped with
a negative control that plants a real duplicate and requires rejection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
480f40ff63 |
feat(abi): publish the module-impl export list as data, for backends to check against (#66)
* feat(abi): publish the module-impl export list as data, for backends to check against
logos-protocol DECLARES the module-impl C ABI; every language backend
(logos-cpp-sdk, logos-rust-sdk, and the Nim path now in flight) must
DEFINE every entry. Those are independent facts, and the gap between them
has shipped twice — grant_host_services at 0.3, the teardown pair at 0.5.
Each time it surfaced three repos downstream as an "undefined symbol" at
dlopen, on Linux only, and each time the runtime still reported the module
as LOADED, so what anyone actually saw was other modules timing out on a
replica that never appeared.
Both breakages happened at PERFECT version agreement between the caller
and the module. Version agreement is necessary and not sufficient: it says
nothing about which symbols a given backend's emitter happens to write.
So derive the list once, here, in the repo that owns the ABI, and ship it
as a build output:
packages.<sys>.module-impl-abi
exports.txt — the declared names
version — the protocol version that header belongs to
bin/logos-module-impl-diff — the assertion, and the explanation
Two properties follow from putting it here rather than in each backend.
There is ONE parser to keep working, rather than one regex per language
that can each silently stop matching. And the list is version-correct with
no version arithmetic anywhere: the header is itself versioned — at 0.4 it
declared eight exports, at 0.5 it declares ten — so "what this protocol
requires" is just "what this header declares". A backend pinning 0.4 reads
eight and is right to define eight. No @since tags, no MINOR comparisons,
nothing for a backend to get wrong.
The extractor parses LOGICAL declarations rather than lines (a reflowed
header must not silently drop one) and refuses to emit a list it is unsure
of: under-reporting is the dangerous direction, because a short list makes
every consumer's diff pass over an ABI nobody checked. The floor it checks
against is asserted rather than derived, so a broken parse cannot satisfy
it. logos-protocol failing to build is the right consequence of
logos-protocol being unable to state its own ABI.
checks.<sys>.module-impl-abi-tests proves all of that can still fail: empty
header, renamed macro, a founding export removed, an empty defined-set, and
a reflowed declaration — eleven cases, each a way this could have decayed
into a green check over nothing.
Also corrects the compatibility note above logos_module_about_to_unload.
It argued the pair was safe because "the glue is generated alongside the
module". That does not follow, and is the reasoning the 0.5 break rested on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* ci: run the ABI manifest check — `nix build .#tests` never reaches `checks`
The step added here is not incidental. `nix build '.#tests'` builds the
PACKAGE; nothing in this workflow evaluated the `checks` attrset at all, so
the manifest self-test added in the previous commit would have sat there
green-by-absence — which is precisely the failure mode it exists to catch.
`ws test` is not a substitute either: it evaluates exactly one check per
repo (scripts/ws truncates the checks JSON at the first comma), so a green
`ws test logos-protocol` says nothing about whether this ran.
builtins.currentSystem rather than a literal, so one line is correct on both
matrix runners.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(abi): the helper must not depend on the consumer's PATH — and the suite must notice
Two bugs, and the second is the interesting one.
1. The installed helper carried `#!/usr/bin/env bash`. Consumers execute it
from inside their own nix builds, whose PATH is whatever THEIR
nativeBuildInputs provide. It resolved on macOS and not in the Linux
sandbox, so the helper simply did not run there. patchShebangs pins an
absolute interpreter.
2. The self-test did not notice, and the reason is worth keeping. expect_fail
accepted ANY non-zero exit as a correct refusal — but a script that cannot
be executed exits 126/127, so all five refusal cases reported PASS while
proving nothing at all. Only the two POSITIVE cases failed, which is the
only reason this surfaced.
That is precisely the failure this whole change exists to prevent, one
level up: a check that reports green over something it never examined. So
expect_fail now asserts a deliberate refusal and rejects 126/127 by name.
Caught by CI on ubuntu-latest while macOS was green — the same
platform-asymmetry that let the original ABI break through.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
2e3344accc |
feat(shared-api): make liblogos_protocol.dll a usable single provider (#65)
PR-1+PR-2 of the shared-runtime migration, squashed: they were raised
separately and the second replaced the first's mechanism, so the split was
history rather than review value.
WHY. The runtime types that must exist EXACTLY ONCE per process (TokenManager,
LogosAPIClient, the per-identity StoreRegistry) are moving from "absorbed into
liblogos_core by whole-archive and re-exported through a generated .def" to
"owned by the shared library that defines them". Every image that links a static
archive gets its own copy of every function-local static inside it, so the host
writes a capability token into one store and another in-process image reads an
empty one -- with no build diagnostic.
Three things, and the order they were discovered in is the order they matter:
1. THE CMAKE PACKAGE. logos_protocol_shared was built and installed but
deliberately kept OUT of the export set: it existed only for FFI callers that
dlopen the lp_* C ABI, and those never link it. In-process C++ consumers do,
and a consumer cannot link what find_package() does not hand it. Now exported
as logos-protocol::logos_protocol_shared, with the INSTALL_INTERFACE include
dirs the static target already had, and with ARCHIVE DESTINATION -- on Windows
a shared library's import library (.dll.a) is the ARCHIVE artifact, so
omitting it installs no import library at all and the failure is invisible on
ELF and Mach-O, which have none.
2. THE EXPORT TABLE IS GENERATED, NOT HAND-MARKED. The first attempt annotated
the classes with __declspec(dllexport). That exported 116 symbols and the Qt
host runtime STILL failed to link against it, with ELEVEN undefined
references across five classes -- LogosProviderObject and its vtable,
ModuleProxy, ModuleHandshakeProxy, LogosTransportFactory -- plus free
functions such as logos::qvariantToNlohmann. A curated list is correct only
until the next consumer touches a symbol nobody marked, and the failure lands
in a downstream repo far from the cause.
cmake/gen-shared-exports.sh is adapted from logos-liblogos, which generated
the same table one layer up. The mechanism is unchanged because the reasons
for it are unchanged; this moves it down to the library that owns the
symbols. It cannot be shared as a file: logos-liblogos depends on
logos-protocol, not the other way round.
logos_shared_api.h therefore resolves its "building the shared library"
branch to NOTHING on Windows, so the .def and the annotations never compete.
The macro keeps its import half, which is what stops a consumer pulling the
archive member that would redefine the symbol.
30 exports on master -> 116 hand-marked -> 360 generated.
3. VTABLES AND TYPEINFO ARE CARVED OUT OF THE COMDAT FILTER. The last undefined
symbol was the vtable for LogosProviderObject. PE HAS NO WEAK SYMBOLS --
COMDAT is the mechanism for weak and inline linkage -- so GCC emits a vtable
into .rdata$_ZTV... even when the class has a key function and the vtable is a
single strong definition. The section name cannot tell "one definition nobody
duplicates" from "every TU emits its own", so the filter dropped it. The
filter's reasoning does not apply to vtables: a consumer of a class WITH a key
function emits a .refptr and needs ours; a class WITHOUT one emits its own
copy and never references ours, so exporting is inert. This never mattered
while liblogos_core absorbed both archives -- definition and consumer landed
in one image and the reference never crossed a boundary.
WHY PROTOCOL NEEDS A .def WHEN THE QT HOST DOES NOT. The shared qt-host DLL
exports 2799 symbols with no .def at all, because it carries no dllexport marks
and GNU ld auto-exports everything. Protocol cannot rely on that: LP_API's
dllexport on the lp_* C ABI disables auto-export for the whole target. ANY single
dllexport turns the automatic path off -- which is also why CMake's
WINDOWS_EXPORT_ALL_SYMBOLS was measured as completely inert here.
TWO MACROS, NOT ONE. LOGOS_QT_HOST_API is added for logos-plugin-qt's LogosAPI,
which lives in a different library. While building the Qt host shared library
LogosAPI must NOT be dllimport while TokenManager must be, and one macro cannot
say both in the same translation unit. Off Windows the distinction is moot --
both resolve to default visibility -- which is exactly why getting it wrong would
go unnoticed until a Windows build.
Also corrects this file's own premise, the origin of the false claim that "ELF
and Mach-O give this for free. Both formats interpose symbols across the whole
process image set." True of ELF, false of Mach-O, whose two-level namespace gives
no interposition -- measured in logos-basecamp, where one reference to
LogosAPI::forIdentity dragged logos_api.cpp.o into the executable and produced 31
refused calls against a baseline of 0.
VERIFIED.
aarch64-darwin static archive symbol tables IDENTICAL (14257 lines), exactly
ONE byte differing in 5.4MB -- 351 -> 352 in __.SYMDEF's ar
header, build metadata, not content
logos-protocolTargets.cmake names both targets
checks.tests PASS, .#default PASS
x86_64-linux checks.tests PASS
x86_64-mingw export table 30 -> 360, all 30 lp_* preserved, 0 removed
import library liblogos_protocol.dll.a now installed
logos-plugin-qt#22 links against it, and its PE layering is
correct: defines LogosAPI 25, defines TokenManager 0 and
LogosAPIClient 0, imports from liblogos_protocol.dll
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
0d2a3c06bf |
feat(abi): 0.5 — advertise the module teardown surface (#63)
#62 added logos_module_about_to_unload() and logos_module_set_unload_done_callback() to the module ABI and left the MINOR at 4. That was my oversight, and it is not cosmetic: this header's whole versioning convention is that each additive surface bumps the MINOR so a CONSUMER can detect it, and 0.3 exists for exactly this shape -- the logos_module_grant_host_services export, guarded downstream on LOGOS_PROTOCOL_VERSION_MINOR >= 3. Without the bump a code generator emitting calls to the new pair has nothing to guard on, so its output requires protocol >= #62 unconditionally and fails to compile against any older header with "logos_module_unload_done_cb was not declared". That is what logos-cpp-sdk#143 hit: new generator, older protocol pin, and no way to tell them apart. With 0.5 the emitters guard the same way 0.3's grant surface is guarded, and new codegen compiles against an older protocol header -- emitting no teardown calls, which is exactly right for a module whose ABI cannot carry them. nix/default.nix tracks the header string by its own comment, so it moves too. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario LipicarandClaude Opus 5
|
9664ae219d |
feat(abi): an optional teardown pair on the module C ABI (#62)
A module has no way to finish work before it is torn down. The host stops it,
its destructors run, and anything mid-flight is simply gone. This adds the two
symbols that let a module say "not yet" and then "done":
int logos_module_about_to_unload(void);
void logos_module_set_unload_done_callback(cb, user_data);
Returning 1 buys a BOUNDED grace period, not a veto. A module that never
signals delays every teardown by that period and is torn down anyway, so the
deadline is real rather than a courtesy -- said in the header, because the
alternative is authors discovering it from a shutdown that got slower.
OPTIONAL, and that is load-bearing rather than politeness. The glue that calls
these is generated alongside the module, so a cdylib built before they existed
exports neither and its glue emits no calls: an older module keeps exactly the
teardown it always had, with no version negotiation and no new failure mode.
Header-only; no implementation and no behaviour change in this repo. The
emitters live in logos-cpp-sdk (C++ exports) and logos-plugin-qt (the Qt glue
that reaches them), and the host-side wait in logos-module-loader-qt.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
43cd059608 |
feat(proxy): answer name()/version() for a provider that does not (#61)
Module identity should be total: every module answers name() and version(),
whatever built it. A module generated through the LIDL frontend now has both in
its own dispatch, but that leaves the rest -- already-built .lgx packages, ui /
ui_qml plugins, any provider whose dispatch does not answer -- reporting
nothing.
Every provider already knows both, through the providerName() /
providerVersion() vtable slots LogosProviderObject has always had. ModuleProxy
answers from those, so those modules gain identity with no edit to any of them.
Two placement decisions do the work:
* the dispatch fallback runs AFTER m_provider->callMethod. An invalid
QVariant is that slot's "unknown method" answer, so a provider that DOES
implement name() keeps its own result -- nothing existing changes
behaviour. It is also gated on an empty argument list, so a module with its
own name(which) reaches its dispatch exactly as before.
* getPluginInterface() advertises the same two methods when the provider does
not list them. Without this a module would ANSWER a method it claimed not
to have: present to whoever already knew to ask, invisible to `lm` and to
every untyped caller. Additive only -- an entry the provider already lists
wins, keeping its description and parameters.
Identity is a method, not introspection, so it stays behind the auth gate. The
three getPlugin* calls are ungated on purpose (they precede the token
exchange); these are not.
This is the one place both transports converge -- the plain transport publishes
a ModuleProxy and reaches it through QMetaObject::invokeMethod -- so one change
covers qt_remote and plain alike.
475/475 tests pass, 6 new.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
f4407ff485 |
ci: use logos-co/setup-nix-cache-action for Nix setup and caching (#60)
Replaces the per-repo installer + cachix pair with the shared action, which
installs Nix with the Logos Attic cache (cache.nix.logos.co) preconfigured and
publishes what the job builds — master to the public cache, every other ref to
ci.
Each converted job also gains
environment: ${{ github.ref == 'refs/heads/master' && 'public-cache' || '' }}
because ATTIC_TOKEN_PUBLIC only exists inside that environment. Without it the
secret resolves empty on master and publishing is silently skipped — the job
still passes, so the omission would not show up as a failure.
The action installs Nix itself on every runner, macOS included. That is a
deliberate reversal of the workaround these files carried: the comments here
said cachix/install-nix-action collides with the runner's pre-existing _nixbld
users (eDSRecordAlreadyExists), so DeterminateSystems' installer was used
instead. It no longer reproduces — logos-delivery-module has already been
converted the plain way and its `build-and-test (macos-latest)` leg passes.
Keeping the workaround would have meant a second installer plus a duplicated
substituter/key block in ten files, guarding against something two green runs
say does not happen. If it ever recurs it fails loudly at install, which is
recoverable; the silent-skip above is the failure mode worth engineering
against.
One property is deliberately NOT carried over: the old cachix step ran with
`continue-on-error: true` so a failed cache push could not fail a job whose
tests passed. The action exposes no equivalent, and adding one here would also
swallow genuine setup failures now that the same step installs Nix rather than
only publishing at the end.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
988e0ba906 |
feat: per-client token store, the host-services C ABI, and a container shape-check (#59)
* feat: the host-services C ABI a trust-root module needs
capability_module is the last legacy Qt Q_INVOKABLE provider, and it cannot
become an ordinary `interface: universal` module while the two things it does
have no C entry point: reading the token store, and pushing a token to an
ARBITRARY target. This adds both, plus the grant that gates them. Purely
additive — no existing symbol changes behaviour.
lp_token_keys() the module names THIS image's TokenManager
holds. NULL means REFUSED, never "empty" — a
granted call with no tokens answers "[]", and a
known-caller gate needs to tell those apart.
lp_inform_module_token_to() routes to LogosAPIClient::informModuleToken_module,
the 5-arg form. Note the existing
lp_inform_module_token is the WRONG DIRECTION
for this: it reaches a consumer path that
hardcodes requestObject("capability_module"),
i.e. core -> capability, not capability ->
target. That 5-arg method had no C entry point.
lp_grant_host_services() sets the in-image grant over the closed set
{token_registry, token_delivery}. Replaces
rather than merges; NULL/""/"[]" clears. An
unknown name is rejected wholesale and leaves
the existing grant untouched, so a typo can
never silently drop a service.
Why the gate is per-IMAGE, which looks like an odd choice until it doesn't:
the host binary and a module's cdylib each link their own copy of this library,
so they have separate process-global state. A gate "simplified" into the host
would be checked against state the calling image can never set, and would read
as ungranted forever. The grant therefore crosses the module-impl C ABI the
same way the auth token already does — hence the logos_module_grant_host_services
declaration added to logos_module_impl.h, whose generated body and host-side
call land in logos-cpp-sdk and logos-module-loader-qt respectively.
MINOR 2 -> 3; MAJOR unchanged, so the equal-MAJOR compatibility rule is
unaffected. 387/387 tests pass, including 6 new ones covering both gates
closed, both opened, clearing re-closing them, and the unknown-name rejection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(tokens): a per-CLIENT token store, selected by origin
TokenManager::instance() is the IMAGE's store, and in a host that loads
plugins in-process it is also an ambient ring: the host writes
`name -> that module's root auth token` for EVERY module it loads. On the
hot path a client asserts no identity at all — invokeRemoteMethod reads
the store first and only mints on a miss — so a plugin asking for target X
finds X's own root token sitting there and presents it. The provider
accepts any token in its image's store, so the call authorizes and no
requestModule is ever logged. Every plugin in that image holds every other
module's authority, and giving a plugin its own ORIGIN STRING changes none
of it, because origin was never consulted on the path taken.
This makes origin SELECT THE STORE rather than merely label the caller.
TokenManager::forIdentity(x) the store to present tokens from when I am x
TokenManager::isolateIdentity(x) give x a private store (host opt-in)
isIsolated / isolatedIdentities / bootstrapKeys / seedBootstrapTokens /
resetIdentity
ADDITIVE BY CONSTRUCTION, not by promise: forIdentity() returns the SAME
OBJECT instance() returns — pointer-identical — for every name until
someone isolates that exact name, so a host that knows nothing about this
is byte-for-byte unchanged. All seven are static member FUNCTIONS: no data
member, no virtual, nothing moc sees. Measured, not asserted: the exported
symbol table of liblogos_protocol.dylib gains exactly 12 names (7 statics +
5 lp_*) and LOSES NONE (736 -> 748). Neither ABI-sensitive private layout
(LogosAPIClient, LogosAPIConsumer) was touched at all.
Construction paths in this repo:
* LogosAPIClient / LogosAPIConsumer: an explicit store still wins; a NULL
store now resolves to forIdentity(origin) instead of being a guaranteed
crash on the first getToken().
* lp_client_create: &TokenManager::forIdentity(origin), not instance().
This is the whole answer to that function's frozen signature — the store
cannot be handed to it, so the origin it already takes must select it.
Bootstrap (constraint 4) survives because a private store is created seeded
with "core" and "capability_module" copied from instance(), and with
NOTHING else — the two keys the first requestModule authenticates with, not
a copy of the ring. resetIdentity() is the unload hook: it clears the
contents and re-seeds, while the store OBJECT stays immortal because a
client holds it by raw pointer from continuations that outlive their caller.
The trust root (constraint 3) is unaffected, and it is checked rather than
argued: lp_token_keys() still reads instance(), isolation only ADDS stores,
and the one thing that moves — an isolated identity's consumer-side CACHE
write — is keyed by TARGET while the known-caller gate consults ORIGIN
names, which the HOST writes and this change never touches.
C ABI grows five additive symbols, each carrying LP_API:
lp_token_isolate_identity, lp_token_identity_is_isolated, lp_token_get_for,
lp_token_save_for, lp_token_reset_identity. Protocol version 0.3.0 -> 0.4.0
(MINOR: additive).
Tests: 439/439 before, 469/469 after. The 30 new cases were validated as
DETECTORS the way this suite requires — against a throwaway build with
forIdentity()'s isolation branch neutered to `if (true)`, i.e. origin as a
label again. 15 go RED there (the walled identity holds the target's root
token; the handshake count is 0 instead of 2; lp_token_keys() lists the
identity's private mint), and the other 15 are pins of behaviour that must
be identical either way. Every escalation case carries an ambient CONTROL
asserting the token IS reachable without isolation.
Hosts are deliberately NOT changed here.
* feat(codec): shape-check the untyped containers
`[any]` and `{tstr:any}` both spell `nlohmann::json` in C++ — LogosList and
LogosMap are aliases of it — so no Codec<T> specialization can tell them apart
and fromJson<T> has nothing to dispatch on. Their SHAPE is still declared,
though, and array-ness / object-ness is the whole of the declared type at that
layer.
jsonRequireArray / jsonRequireObject check exactly that and hand the value on
UNCHANGED, throwing through the codec's own detail::typeError so the message is
the one every other surface already produces ("expected array at arg0, got
string"). The value is not rebuilt from JSON: that would retype nested elements
for no validation gain, which is the same reasoning logos_qt_arg_decode.h gives
for the Qt surface.
This is what logos_codec.h:36 already promised and these two types quietly did
not honour — "shape mismatches throw CodecError … rather than silently
substituting a default, silent defaults are how a mangled value reaches business
logic."
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
e6d5b575c2 |
fix(plain): a second handle must not steal the first's event channel (#52)
* fix(plain): give every handle its own event subscription on the shared connection
RpcConnection::m_eventCallbacks was keyed by (object, eventName) and ASSIGNED.
One RpcConnection is shared by every PlainLogosObject a PlainTransportConnection
hands out, and requestObject() mints a fresh handle per acquire, so the second
handle to subscribe to the same event on the same module silently took the first
one's channel — including the deferred ("multi") completion channel every handle
subscribes to on its first call.
No concurrency is needed to reach it. Measured on
|
||
|
Dario LipicarandClaude Opus 5
|
5be3a84989 |
test(plain): destroy the fixtures' host on the thread that emits into it (#51)
Every LiveHost fixture in tests/protocol (five of them: test_iofold,
test_plain_object_teardown, test_plain_waiter_reaping,
test_plain_completion_sub_lifetime, test_call_error_after_acquire) puts a
ModuleProxy on a worker QThread and publishes it through a PlainTransportHost,
and every one of them freed that host from the TEST thread:
m_host.reset(); // test thread
m_thread->quit();
m_thread->wait();
That is a use-after-free with a millisecond-wide window, in code five test
files share.
WHY. publishObject() connects a lambda to the proxy's eventResponse signal with
NO context object, so it is a direct connection and runs on whichever thread
emits. ModuleProxy always QUEUES that emission to its own thread — it must, or
QtRO source serialization races the reply socket — so the emitting thread is
always the worker. The lambda converts the payload and then calls fanOutEvent,
which locks the host's m_mu. Free the host on the test thread and that lock is
on a destroyed mutex. ~PlainTransportHost does disconnect the connection, which
covers an emission that has not started; a worker already inside the lambda is
not called back, it is simply running, and qvariantListToRpcList on a real
payload sits in front of the lock.
MOVING reset() AFTER quit()/wait() IS NOT THE FIX, and the obvious reason for
saying so is wrong, so here is the measured one. That order does stop the fault:
wait() joins the worker, so an in-flight lambda has finished, and it is clean
under Guard Malloc. It is clean because it throws the queue away —
QThread::quit() reaches QEventLoop::exit(), which sets the exit flag
SYNCHRONOUSLY from the calling thread instead of posting an event, so the
worker's loop stops at its next iteration and discards every emission still
queued behind it. Same specimen, same load: 273-383 of 960 events delivered,
silently. In a suite whose tests count deliveries that is the worse failure,
because nothing reports it. It also shuts the host down AFTER the proxy's
thread, which test_call_error_after_acquire needs the other way round.
THE FIX, in one shared place (live_host_teardown.h) rather than five copies,
because a copied pattern is what this was: destroy the host ON the proxy's
thread, via the SDK's own logos::runOnOwnerThread marshal. A QMetaCallEvent is
dispatched by the worker's event loop, so while it runs the worker is by
definition not inside any other slot. Qt dispatches equal-priority events FIFO,
so every emission queued before it runs first, against a live host; everything
after finds the connection already severed by ~PlainTransportHost. The io
thread, the third thread that reaches the host, is still covered by the drain
barrier ~PlainTransportHost already carries — the proxy thread is not the io
thread, so that barrier's running_in_this_thread() check still takes the
blocking path. The added blocking wait introduces no hang that was not already
there: the next two statements are quit()/wait() on the same thread, with no
timeout.
EVIDENCE. test_plain_host_event_teardown.cpp is the specimen, and it is a
detector: 24 wide events queued per round, teardown aimed at the trailing edge
of the first so the worker is inside the host's lambda. Validated the way this
directory validates detectors — against a real checkout of the code it replaces
(
|
||
|
Dario LipicarandClaude Opus 5
|
0fef299362 |
fix(plain): deliver async callbacks in a Qt-free host, and make release()-racing-a-call diagnosable (#50)
* fix(plain): deliver async callbacks in a Qt-free host, and make release()-racing-a-call diagnosable
Two pre-existing defects in the plain transport's async surface. Both are
older than #45/#46 and neither is caused by the io_context fold; the fold is
just what this is stacked on.
DEFECT 5 — the async surface promised exactly-once and delivered ZERO in a
Qt-free host. Every completion went through one hop, and the hop was:
QCoreApplication* app = QCoreApplication::instance();
if (!app) return; // <- the callback, dropped
In a Qt host that branch only fires at shutdown, which is why it read as a
reasonable guard. In a process that never had a QCoreApplication — the
deployment the plain transport exists for — it fires for EVERY call, forever,
on all four resolvers (reply, deferred completion, deadline, cancellation).
Not an error, not a timeout: silence, which turns a bounded call into an
unbounded wait in every caller that awaits it, including lp_invoke_async and
every generated async wrapper.
Fixed with a dedicated DELIVERY THREAD, used only when the process has no Qt
loop. NOT inline on the completing stack: inline delivery on an Asio read
handler is the re-entrancy class that already cost this codebase a SIGSEGV
(deferred-multi completion on the QtRO read stack), so a fix that delivers by
removing the hop is not a fix. NOT the deadline thread either — user callbacks
there would make every deadline in the process hostage to user code, which is
exactly the coupling DeadlineService was extracted to prevent.
The Qt-loop check LATCHES, so Qt hosts see no behavioural difference at all:
instance() also goes null inside ~QCoreApplication, and module teardown after
the application is gone is what static-destruction ordering produces — with
stopAndCancelCalls() handing every in-flight call a cancellation callback at
exactly that moment. Running user code on a side thread into half-destroyed
module state would be a NEW failure mode introduced by a bug-fix change, so a
process that has ever been seen with an event loop keeps the old shutdown
behaviour. logos_object.h now states that residue instead of glossing it.
DEFECT 3 — release() racing a call on another thread. NOT FIXED, because it
cannot be, and the honest answer is a contract plus a detector.
release() ends in `delete this`, so a synchronous call parked in its future
wait dereferences freed memory when it comes back. Reproduced deterministically
on master (exit 139 under Guard Malloc, 3/3) and on
|
||
|
Dario LipicarandClaude Opus 5
|
01ebf33c8a |
fix(plain): answer a call that registers as the connection fails, instead of leaving it to its deadline (#49)
* fix(plain): answer a call that registers as the connection fails, instead of leaving it to its deadline
sendCallAsync() reads m_stopped and THEN registers its handler under m_mu.
fail() writes m_stopped and THEN sweeps the pending map under the same mutex.
The two are ordered opposite ways round, so a fail() that completes in between
sweeps a map the caller has not written to yet:
caller fail()
------------------------------ -------------------------------
m_stopped.load() -> false
CAS m_stopped -> true
lock(m_mu); swap(m_pendingCalls)
unlock(m_mu) ... the map was EMPTY
lock(m_mu); m_pendingCalls[id] = h
writeFrame() ... drops: stopped
The handler is now parked in the pending map of a connection nobody will sweep
again — fail() runs once and has been, no reply can arrive on a closed socket,
and the frame was never written. THE CALL IS ANSWERED BY NOTHING, and what
answers instead is the caller's own deadline: callMethodAsyncWithError reports
"timeout" after the full timeoutMs, callMethodWithError blocks its thread for
the same span and reports the same wrong code, and getMethods() waits out a
hard-coded five seconds that no caller can shorten. A connection already known
to be gone is reported as a peer that was merely slow — which is also the code
callers retry and re-acquire on.
This predates the io_context fold: master has the identical shape on the
promise-based path. #46's cancelPending() only made the orphaned entry
self-cleaning rather than permanent.
THE FIX: register first, then re-read m_stopped, and reclaim our own entry if
the connection died in between. It closes the hole by an ordering argument
rather than by a smaller window:
* if fail()'s sweep ran BEFORE the registration then its CAS ran before that,
so the re-read cannot see false, and the reclaim answers the call;
* if the re-read DOES see false then, in the total order over m_stopped, it
precedes fail()'s store; the registration is sequenced-before the re-read,
so it precedes fail()'s lock, and the sweep is guaranteed to find the entry.
There is no third case, and exactly one of the reclaim and the sweep can extract
the handler because both extract-and-erase under m_mu — the same single-winner
rule dispatchIncoming and cancelPending already play by, with one more
contender. sendMethods() gets the same treatment for the same reason.
REJECTED, since the tempting fixes deadlock: holding m_mu across the check AND
the delivery self-deadlocks on the first inline delivery, because a handler here
is AsyncCall's, which calls deliver(), which calls cancelPending(), which takes
m_mu — and m_mu is not recursive (the symmetric version, fail() invoking swept
handlers under the lock, dies the same way). Moving fail()'s once-only CAS under
m_mu is correct and deadlock-free, but makes teardown's flag wait on a mutex
every in-flight send and every decoded reply also take, so m_stopped stops being
the instantly-visible "stop writing" signal that writeFrame(), doWrite() and
doRead() read lock-free — a race traded for a teardown-latency regression.
sendSubscribe() has the same shape and is deliberately left alone, with a note
saying why: nobody waits on a subscription, so there is no deadline to blow and
no caller to strand.
tests/protocol/test_plain_send_after_fail.cpp builds the interleaving instead of
waiting for it — it takes the connection's own mutex, which parks the caller
between its m_stopped check and its registration, then drops the mutex and calls
stop() from the hot thread. Validated against
|
||
|
Dario LipicarandClaude Opus 5
|
7be3a6b856 |
perf(plain): fold the per-call waiter thread into a call object with its own clock (#46)
* 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 LipicarandClaude Opus 5
|
4a20e99260 |
fix(plain): the completion subscription must not outlive the object it points at (#45)
* fix(plain): the completion subscription must not outlive the object it points at
PlainLogosObject::ensureCompletionSub() registered the deferred-completion
handler with raw `this` captured. That handler is stored in the RpcConnection,
which is SHARED by every handle the connection hands out and outlives all of
them — release() says so itself, and ends in `delete this`. So a completion
event arriving across a release() ran a handler holding a dangling pointer, on
the io thread, on a path nothing joins: #41's waiter JOIN covers the per-call
waiter threads and nothing else.
The unsubscribe release() sends is real — RpcConnection::sendUnsubscribe erases
the entry under the connection's mutex — but it cannot close this, because
dispatchIncoming copies the handler out under that mutex and then invokes it
with the mutex dropped. An erase racing an already-copied handler changes
nothing about the invocation in flight.
Reproduced, not assumed. tests/protocol/test_plain_completion_sub_lifetime.cpp
widens the window with a large completion payload (the conversion between the
copy and the handler's first touch of the object) and aims release() into it
using a wildcard subscriber as a clock. On master:
* SIGSEGV under macOS Guard Malloc, 3/3 runs, faulting in
pthread_mutex_lock <- std::mutex::lock <- ensureCompletionSub()::$_0 <-
onEvent()::$_0 <- dispatchIncoming <- doRead <- IoContextPool's thread;
* without a detector, 4/5 runs die differently and just as fatally: the freed
mutex makes pthread_mutex_lock return EINVAL, std::mutex::lock() throws, and
the exception unwinds into doRead()'s catch, which fail()s the whole
connection. That is the per-round isConnected() assertion in the test.
The fix moves the rendezvous (mutex, condvar, completions map) into a
shared_ptr-held block and hands the handler a weak_ptr, so "no handler touches
a destroyed object" holds by construction: a handler that locks it keeps it
alive for one callback, one that cannot lock it does nothing. Nothing else in
the object was reachable from that handler, which is what keeps this to two
files; rpc_connection.h is untouched.
Verified after the fix: repro clean 10/10 plain and 3/3 under Guard Malloc,
with the same cadence and 24/24 releases still landing inside a dispatch — the
window is still exercised, it is just no longer a use-after-free. The control
(same storm, nothing released) is clean under the same detector on both sides,
so the detector is not objecting to the load. All four #41 guarantees re-measured
and unchanged: waiters joined (60/60 rounds), teardown 10-22ms against master's
6-27ms, exactly one callback on all four outcomes, registry final=1 after 200 /
600 / 800 / 1600 calls. Full suite 287/287, including nix build .#tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plain): a concurrent first caller must WAIT for the completion subscription, not just see the flag
ensureCompletionSub() raised m_completionSubscribed under the rendezvous mutex
and then RELEASED that mutex before subscribing. Two threads entering
callMethod() on the same fresh object is enough: the second reads "subscribed",
builds its Call and puts it on the wire while the first has not enqueued the
Subscribe frame yet. A "multi" provider that answers such a call quickly emits
its completion into a subscription the host has not registered —
PlainTransportHost::fanOutEvent finds no sink for that connection and DROPS it —
and the caller waits out its whole timeout for a result that was computed and
thrown away.
A LOST COMPLETION, NOT A CRASH, which is why it survived: the failure looks like
a slow provider, arrives seconds after the code that caused it, and leaves
nothing behind.
PRE-EXISTING, not introduced by this branch: pristine master has the identical
flag-then-subscribe shape and reproduces at 42/400 two-thread first-call rounds
(this branch before the fix: 28/400; through the real host stack: 10/600 calls).
It ships here, as its own commit, because it is four lines in the very function
this PR rewrites and in the same subscription this PR is about.
The fix is std::call_once plus a release/acquire fast path. Serializing is the
whole of it: a second caller blocks until the first has both registered the
client-side callback and enqueued the Subscribe frame, and asio then keeps the
two posts in that order because the mutex supplies the happens-before edge its
strand guarantee is conditioned on.
Rejected: holding the rendezvous mutex across the subscribe (works, but makes
the io thread's completion handler wait on the connection's write path — that
mutex exists to hand a completion over, not to gate I/O); subscribing eagerly in
the constructor (kills the race outright but costs a Subscribe frame and a host
sink per handle, deferred call or not).
tests/protocol/test_plain_completion_sub_order.cpp pins both halves — the wire
order, observed at a provider that stamps every frame it receives, and the
consequence through PlainTransportHost with nothing instrumented at all. Both go
RED under -DLOGOS_PROTOCOL_DETECTOR_INVERSIONS=ON, which restores the pre-fix
shape: 5 of 5 broken runs failed (25-37 dropped completions per 250 rounds), 8
of 8 fixed runs were clean. Full suite 289/289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): validate the detectors against master, not a compiled-in inversion
The transport carried a second ensureCompletionSub() behind
LOGOS_PLAIN_DETECTOR_BREAK_SUB_ORDER: the pre-fix racy shape, reachable
from a -DLOGOS_PROTOCOL_DETECTOR_INVERSIONS=ON configure of the tests
tree, so the ordering tests could be shown to fail. That is the same
anti-pattern as the getenv() probes an earlier draft carried, wearing a
build flag instead — production source keeping a deliberately wrong
implementation of its own contract — and it does not belong in the PR.
Both detectors are validated by the stronger check anyway: this file
compiles unmodified on master, which still raises the flag under the
rendezvous mutex and drops it before subscribing, and still captures raw
`this` in the completion handler. Numbers now in the comments are from
that run, not from the synthetic build:
sub-order raw wire 18/26/27/28 of 250 rounds inverted, dropped and
timed out, four runs
sub-order real stack 6 to 10 of 500 calls timed out
sub-lifetime RED in 11 of 12 solo runs, connection dying at
round 6 in 9 of them
The 400/600-round figures the comments quoted were also stale:
|
||
|
Dario LipicarandClaude Opus 5
|
03842db5c1 |
feat(windows): named pipes, an explicit lp_* ABI, and a cross target (#58)
* feat(windows): port logos_socket_paths and add a cross target logos_socket_paths.cpp is the only POSIX-bound file in logos-protocol. All of it is unix-domain-socket machinery, and on Windows the local transport is named pipes (QLocalServer maps a name to \\.\pipe\<name>), where none of the assumptions hold: a pipe has no inode to lstat/chown/chmod -- access comes from a security descriptor set at CreateNamedPipe time -- and a pipe cannot outlive its last handle, so a hard-killed process leaves nothing behind. isSocketDead and reapStaleSockets are therefore not merely unimplemented on Windows, they are vacuous: the state they detect cannot arise. Both return the fail-closed answer (false / 0), matching the documented contract that an endpoint is never reported dead unless certain. applySocketPerms deliberately does NOT no-op. With no policy requested it returns true, as on POSIX. But when LOGOS_SOCKET_GROUP or LOGOS_SOCKET_MODE *are* set it fails with an explanatory error, because silently returning true would leave the endpoint more permissive than the operator asked for -- the one direction this file is careful never to go (cf. the chgrp-then-chmod ordering in the POSIX branch). Granting a pipe to a group needs a DACL plus a group->SID resolver; until that exists, refuse loudly. Also gates qt6.wrapQtAppsNoGuiHook behind !isWindows and sets dontWrapQtApps. Both halves are required: the hook does not even evaluate for a mingw host, it would be inert anyway (wrap-qt-apps-hook.sh skips anything that is not ELF or Mach-O), and qtbase's setup hook hard-errors in qtPreHook unless dontWrapQtApps is set. Header contract updated per function. POSIX branch unchanged and still compiles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: make the Boost.System component optional, not required find_package(Boost REQUIRED COMPONENTS system) hard-fails on Boost 1.89: Could not find a package configuration file provided by "boost_system" Boost.System has been header-only for years, and 1.89 finally dropped the compiled boost_system library, so no boost_systemConfig.cmake is installed at all. The COMPONENTS request was not gratuitous though -- on 1.87 the Boost::system imported target is only exported when the component is asked for, which is what the previous comment recorded. So ask optionally and fall back to Boost::headers, which supplies the same header-only error_code either way. The choice is by BOOST VERSION, not by platform: this is not a Windows quirk, it simply surfaced first there because the Windows target pins a newer nixpkgs (Boost 1.89) than the native one (Boost 1.87). Verified both ways -- native aarch64-darwin still selects Boost::system: -- Boost.System target: Boost::system (Boost 1.87.0) and the build completes unchanged. Also adds QT_HOST_PATH / QT_ADDITIONAL_HOST_PACKAGES_PREFIX_PATH for the Windows target. Qt6RemoteObjectsDependencies.cmake declares set(__qt_RemoteObjects_tool_deps "Qt6RemoteObjectsTools;6.11.1") and Qt6RemoteObjectsTools holds repc, which must RUN on the build machine -- so under cross it lives in the build-platform Qt, not the mingw one. Without these, find_package reports the thoroughly misleading "Expected Config file at <qtbase>/lib/cmake/Qt6RemoteObjects ... does NOT exist": the TARGET config is found fine; it is the HOST tool package that is missing. Every Qt-consuming repo will need this, so it should be hoisted into logos-nix rather than repeated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor: declare the lp_* C ABI explicitly instead of relying on auto-export Adds LP_API (__declspec(dllexport) when building the shared library, default visibility elsewhere) to the 21 lp_* entry points, and defines LOGOS_PROTOCOL_BUILDING_SHARED for the shared target only, so the static archive leaves LP_API empty and its consumers need no import library. This is NOT a bug fix, contrary to what the concern in the Windows plan suggested. Measured on the cross-built DLL, before and after: before: export table 0x2ece (11982 symbols), lp_* present: 21 after: export table 0x15 ( 21 symbols), lp_* present: 21 GNU ld's PE auto-export was already exporting lp_* -- along with roughly twelve thousand other symbols. The worry was that logos_module_impl.h's __declspec(dllexport) would disable auto-export image-wide and silently drop lp_*; it does not, because no translation unit in logos_protocol includes that header (it is listed in PROTOCOL_SOURCES for IDE visibility only). What this does buy is worth having anyway: the exported surface is now the ABI we actually declare rather than whatever happens to have external linkage, it stops being contingent on auto-export staying enabled -- which the very next TU to gain a dllexport would silently end -- and it drops ~12k incidental symbols from the export table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: relax the Boost.System requirement in the EXPORTED cmake config too The previous commit fixed cpp/CMakeLists.txt but left logos-protocolConfig.cmake.in still doing find_dependency(Boost REQUIRED COMPONENTS system) so logos-protocol itself built fine on Boost 1.89 while every CONSUMER of its installed CMake package failed at configure time -- caught by logos-qt-sdk, which is the first downstream repo to be cross-built. Worth noting as a general trap: a package can be internally consistent and still ship a broken contract, because the exported config is a separate artifact from the build. Anything changed in one has to be checked in the other. Verified both directions: the Windows cross builds of logos-cpp-sdk and logos-qt-sdk now succeed, and a native aarch64-darwin logos-qt-sdk build -- which consumes this same config against Boost 1.87 -- still succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(windows): mark the types that must exist once per process PE has no symbol interposition. ELF and Mach-O interpose across the whole image set, so when liblogos_core exports TokenManager::instance() every other image binds to that one definition and the function-local `static TokenManager instance;` is genuinely a singleton. On Windows every image that links liblogos_protocol.a / liblogos_qt_sdk.a statically gets its own copy of the code and therefore its own statics -- measured: NINE images in the Basecamp payload each defined TokenManager::instance()::instance. The host saved a capability token into its copy, the UI plugin read its own empty copy, and every cross-module call was refused (29 "ModuleProxy: rejecting unauthorized call"). LOGOS_SHARED_API marks the affected types. It expands to __declspec(dllimport) only for a consumer that opts in with LOGOS_SHARED_USE_DLL, and to nothing everywhere else -- off Windows, and inside logos-protocol/logos-qt-sdk/liblogos_core themselves, so the static archives compile byte-identically to before. The dllimport is the load-bearing half, not the export: it rewrites the reference to go through __imp_, so the plain symbol is never undefined and GNU ld never pulls the archive member that would redefine it. Without it the link still succeeds, binds to the archive, and gives no diagnostic at all. logos_shared_api.h records both wrong answers -- export everything (collides with the static archive over LogosAPI) and export nothing (today's silent per-image statics) -- so neither gets reinvented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(windows): let checks and devShells take the arg forAllSystems now passes The cross-target commit added `inherit system;` to forAllSystems so the Windows arm could tell which target it was building, but left `checks` and `devShells` on the strict `({ pkgs }: ...)` pattern. A Nix attrset pattern without `...` is exact, so both stopped evaluating: error: function 'anonymous lambda' called with unexpected argument 'system' on EVERY platform, not just Windows -- `nix flake check` and `ws develop logos-protocol` are dead on this branch while they work on master. `packages` was unaffected because it goes through forAllTargets, which is why nothing caught it. Measured, same worktree, before and after: before: checks.aarch64-darwin -> the error above at flake.nix:52 after: checks.aarch64-darwin -> [ "tests" ] devShells.aarch64-darwin.default.name -> "nix-shell" packages -> [ aarch64-darwin aarch64-linux x86_64-darwin x86_64-linux x86_64-windows ] * chore(deps): re-pin logos-nix to the merged Windows overlay The cross overlay landed in logos-nix#2. This branch was locked to a pre-merge rev, which has no `lib.forAllTargets` and no `lib.mkWindowsPkgs`, so it could not evaluate standalone -- only against the unmerged branch. Level 2 of the Windows chain; L1 (logos-nix) is merged. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario LipicarandClaude Opus 5
|
07745712e8 |
test(protocol): pin that the arm-now probe cannot free a replica QtRO still holds (#57)
The use-after-free fixed in #47 (
|
||
|
Dario LipicarandClaude Opus 5
|
0183e8c26a |
fix: make the caller's timeout bound the token handshake, not just the call (#55)
* 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> * fix: make the caller's timeout bound the token handshake, not just the call requestModule gains a timeoutMs parameter (defaulted to today's 20000, so every existing caller is source-compatible and unchanged), and LogosAPIClient threads its caller's Timeout through mintAndCacheToken into it. The budget bounds the WHOLE handshake -- the capability_module acquire plus the requestModule call on it share one deadline, rather than each getting a fresh copy. Halving it would be arbitrary; giving each the full amount would make the worst case twice what the caller asked for. What is left after the acquire is never allowed to reach 0, because some transports read 0 as "no timeout" and an exhausted budget must not silently become an unbounded wait. Also corrects a comment that argued the handshake-refusal fallthrough was safe because "capability_module passes 3000 ms". It does not: capability_module reaches informModuleToken_module through its FOUR-argument overload (capability_module_plugin.cpp:112), so timeoutMs takes the header's 20 s default. The bound is real, it is just not short -- and the code should say the true thing about why it is safe. Not covered here: the ASYNC first-call path still acquires capability_module through invokeRemoteMethodAsync without threading a budget. It does not block the caller, so it is a different defect with a different fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Dario LipicarandClaude Opus 5
|
dcc0f73d1c |
feat: hold a CALL until its module is reachable (whenObjectAvailable) (#53)
* feat: add whenObjectAvailable(), the call-path counterpart of onEventWhenAvailable() Event subscribers got a way to ask "tell me when this module is reachable" without blocking and without giving up on the first no. Callers had no such thing, so a caller facing the same startup race had only two bad options: fail fast, which strands a UI that will never retry, or call straight through and sit in the transport's acquire timeout on whatever thread it was called from — the GUI thread, in practice. whenObjectAvailable() reuses the pending-subscription registry that already exists, as a readiness-only entry: it attaches no subscription, fires its callback exactly once, and is then forgotten rather than being re-armed on reconnect, because a one-shot readiness answer that arrives twice is not an answer. It shares the registry's timer, backoff and diagnostics, so it costs nothing new on qt_remote and shows up in pendingSubscriptions() as "<object>::(readiness)" while it waits. Its first consumer is LogosQmlBridge::callModuleAsync, which can now hold a call issued before its module exists and dispatch it when the module appears. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(events): pin whenObjectAvailable on every transport It shares the pending registry with event subscriptions but answers a different question, and it had coverage only end-to-end through the QML bridge. Three cases x 6 params: fires true for a module that is already up; holds rather than answering "not reachable" for one that is merely not up YET, then fires exactly once when it appears; and is NOT resurrected by a reconnect. That last one is the asymmetry worth pinning. Event subscriptions ARE re-armed across a reconnect on purpose; a readiness answer already delivered is spent, and re-firing it would re-dispatch whatever call it was gating. Suite 356 -> 374. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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
|
43595575a3 |
feat(codec): thread the path through the bstr decoder (#33)
Prerequisite for deleting the codec copy that the cdylib generator emits.
That copy's Codec<std::vector<uint8_t>>::from reported a path ("[0].payload");
the canonical one discarded it and said "at value". Swapping one for the other
without this would have lost the diagnostic exactly where it matters most — a
bad bstr buried in a container.
bytesFromJsonLenient takes the path as a defaulted argument, so every existing
caller and every existing test compiles unchanged.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Dario LipicarandClaude Opus 5
|
3da8de93df |
fix(codec): a whole-valued float still decodes as an integer (#32)
The signedness/range check in #31 went one step too far: it rejected 3.0 for an `int`, not just 3.7. That broke four long-standing test_basic_module_cpp cases (`addInts(3.0, 4.0)`, `echoInt(42.0)`, `isPositive(5.0)`, `twoArgs(hi, 3.0)`) which pass a whole-valued double where the contract declares an integer. They are right and the check was wrong. JSON does not distinguish 3 from 3.0, and this codec already says so in the other direction — Codec<double> accepts an integral number because "2 and 2.0 are the same value to JSON, and every encoder that sees a whole double may emit either". The two directions have to agree. It also matters in practice rather than in principle: logoscore's CLI types its arguments by parsing, so `logoscore call m addInts 3.0 4.0` produces JSON floats. Refusing them rejects a caller over a spelling of the same number. So a float decodes as an integer when it has no fractional part and fits; 3.7 is still refused, which is what the original change was actually for. Bounds are strict on the upper end for the same reason as the QJsonValue guard: double(int64max) rounds UP to 2^63, so `<=` would admit a value the cast cannot represent. verified: test-modules 176/176 with the four cases green again, and the conformance matrix unchanged at 170 pass / 2 xfail — hostile/int/fractional still expects dispatch_failed and gets it. 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>
|
||
|
Khushboo-dev-cpp
|
1e96004711 |
Merge pull request #19 from logos-co/fix/asyncCallErrorChannel
Fix/async call error channel |
||
|
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
|
ee1c5192da | feat: plumb CallError through invokeRemoteMethodAsync | ||
|
Dario LipicarandClaude Opus 4.8
|
d7ad26d369 |
feat: ship liblogos_protocol shared library exporting lp_* (#4)
Add a `logos_protocol_shared` target that builds the same sources as a
shared library (liblogos_protocol.{so,dylib}), exporting the
language-neutral lp_* C ABI for out-of-plugin callers that bind it at
runtime via dlopen/FFI (logos-js-sdk's koffi.load, logos-rust-sdk's
callerBuildSupport) — the role liblogos_module_client previously filled.
The static `logos_protocol` archive and its EXPORT set are untouched, so
in-plugin code and find_package(logos-protocol) are unaffected; the shared
target is deliberately not exported into the CMake package.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|