From d0523c14862febacaa660aeff43cd4b84f0364a5 Mon Sep 17 00:00:00 2001 From: Dario Lipicar Date: Mon, 3 Aug 2026 17:51:44 -0300 Subject: [PATCH] fix(lp): lp_invoke_async can finally report a failure (#40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cpp/logos_protocol.cpp | 22 +- cpp/logos_protocol.h | 10 + tests/protocol/CMakeLists.txt | 8 + tests/protocol/test_lp_invoke_async_error.cpp | 210 ++++++++++++++++++ 4 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 tests/protocol/test_lp_invoke_async_error.cpp diff --git a/cpp/logos_protocol.cpp b/cpp/logos_protocol.cpp index c5c55d5..8ea6ff1 100644 --- a/cpp/logos_protocol.cpp +++ b/cpp/logos_protocol.cpp @@ -1,6 +1,7 @@ #include "logos_protocol.h" #include "logos_api_client.h" +#include "logos_call_error.h" #include "logos_json_convert.h" #include "logos_mode.h" #include "logos_object.h" @@ -338,11 +339,30 @@ int lp_invoke_async(lp_client* client, return LP_ERR_INVALID_ARG; std::shared_ptr guard = client->guard; + // A TWO-argument lambda: invocable only as LogosAPIClient's + // AsyncResultErrorCallback, so it binds to the CallError-aware overload and + // never to the value-only one sitting next to it. That overload is what + // makes `ok == 0` reachable at all — this used to subscribe with the + // value-only one and hard-code cb(1, ...), so a call to a module that is + // not loaded reached the callback as a SUCCESS carrying a default value, + // contradicting both lp_result_cb's documented contract and the sync twin + // lp_invoke (which returns LP_ERR_UNAVAILABLE + out_error_json). + // + // The failure shape is deliberately identical to lp_invoke's + // out_error_json — the same makeErrorJson({code, message, origin}) — so the + // two entry points report the same event the same way, and a caller can + // parse one decoder for both. client->client->invokeRemoteMethodAsync( client->target, QString::fromUtf8(method), args, - [guard, cb, user_data](QVariant result) { + [guard, cb, user_data](QVariant result, const logos::CallError& err) { std::lock_guard lock(guard->mutex); if (!guard->alive) return; // client destroyed: drop the result + if (!err.ok()) { + const std::string json = makeErrorJson(err.code.c_str(), + err.message, err.origin); + cb(0, json.c_str(), user_data); + return; + } const std::string json = logos::qvariantToNlohmann(result).dump(); cb(1, json.c_str(), user_data); }, diff --git a/cpp/logos_protocol.h b/cpp/logos_protocol.h index d89625d..761410c 100644 --- a/cpp/logos_protocol.h +++ b/cpp/logos_protocol.h @@ -189,6 +189,16 @@ int lp_invoke(lp_client* client, * Asynchronous variant of lp_invoke. Returns LP_OK if the call was * dispatched; `cb` then fires exactly once with the result (from the * client's owner thread). Safe to call from any thread. + * + * `cb` carries the same outcome the sync twin splits across its return code + * and out-params: ok != 0 → `json` is the result JSON value; ok == 0 → `json` + * is the canonical error object lp_invoke would have written to + * out_error_json (e.g. code "object_unavailable" when the target module is + * not loaded). A LP_OK return therefore means "dispatched", never "succeeded" + * — the outcome is only known in the callback. + * + * Argument/handle validation still fails synchronously with + * LP_ERR_INVALID_ARG and `cb` is NOT called in that case. */ int lp_invoke_async(lp_client* client, const char* method, diff --git a/tests/protocol/CMakeLists.txt b/tests/protocol/CMakeLists.txt index 30faa70..587b59d 100644 --- a/tests/protocol/CMakeLists.txt +++ b/tests/protocol/CMakeLists.txt @@ -28,6 +28,14 @@ add_executable(protocol_tests # falling out on replica-acquire timeouts. test_lp_client_owner_thread.cpp test_call_error.cpp + # The ASYNC half of the same channel. lp_invoke_async subscribed with the + # value-only transport overload and hard-coded cb(1, ...), so a failed call + # reached lp_result_cb as a success carrying a default value — the exact + # thing lp_result_cb's own doc comment (and its sync twin lp_invoke) says + # cannot happen. Matched pair: a failing call must report ok=0 with the + # canonical error object, a succeeding one must still report ok=1 with its + # value. Both over a real (plain TCP) transport. + test_lp_invoke_async_error.cpp # Component tests that moved here with their code (from logos-cpp-sdk) test_token_manager.cpp test_mock_store.cpp diff --git a/tests/protocol/test_lp_invoke_async_error.cpp b/tests/protocol/test_lp_invoke_async_error.cpp new file mode 100644 index 0000000..2afdd3d --- /dev/null +++ b/tests/protocol/test_lp_invoke_async_error.cpp @@ -0,0 +1,210 @@ +// The ASYNC half of the call-error channel. +// +// 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 lp_invoke, its synchronous twin, has always honoured that shape via +// 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, so a call to a module that +// cannot be acquired reached the callback as a SUCCESS carrying a +// default-constructed value. An error channel that always says "fine" is worse +// than none — it is why logos-cpp-sdk#132 shipped `fooAsyncResult` on the Qt +// surface but deliberately withheld it on the Qt-free (Lp) one. +// +// These two tests are a matched pair and only mean something together: +// +// 1. a genuinely failing call must reach the callback with ok == 0 and the +// canonical {code, message, origin} object — the case that used to lie; +// 2. a genuinely succeeding call must still reach it with ok == 1 and its +// value — the control that an over-eager "report failure everywhere" fix +// would break. +// +// Both run against a REAL transport (plain TCP), not the mock: (1) dials a +// port nothing listens on, (2) dials a live in-process PlainTransportHost +// publishing a real provider through ModuleProxy. + +#include + +#include "logos_protocol.h" + +#include "logos_provider_interface.h" +#include "logos_transport_config.h" +#include "module_proxy.h" + +#include "plain_transport_host.h" + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include +#include + +using namespace logos::plain; + +namespace { + +// Minimal live provider: compute() -> 7. +class EchoProvider : public LogosProviderObject { +public: + QVariant callMethod(const QString& method, const QVariantList& args) override + { + if (method == QLatin1String("compute")) return QVariant(7); + if (method == QLatin1String("echo")) return args.value(0); + return QVariant(); + } + QJsonArray getMethods() override { return QJsonArray{}; } + bool informModuleToken(const QString&, const QString&) override { return true; } + void setEventListener(EventCallback) override {} + void init(void*) override {} + QString providerName() const override { return QStringLiteral("echo"); } + QString providerVersion() const override { return QStringLiteral("1.0.0"); } +}; + +QCoreApplication* ensureApp() +{ + static int argc = 0; + static char* argv[] = { nullptr }; + if (!QCoreApplication::instance()) + new QCoreApplication(argc, argv); + return QCoreApplication::instance(); +} + +// What lp_result_cb handed us. +struct Capture { + std::atomic fired{false}; + int ok = -1; + std::string json; +}; + +void captureCb(int ok, const char* json, void* user_data) +{ + auto* c = static_cast(user_data); + c->ok = ok; + c->json = json ? json : ""; + c->fired = true; +} + +// Pump the owner thread's loop until the callback lands (or we give up). +bool pumpUntilFired(Capture& c, int budgetMs) +{ + QElapsedTimer timer; + timer.start(); + while (!c.fired && timer.elapsed() < budgetMs) + QCoreApplication::processEvents(QEventLoop::AllEvents, 20); + return c.fired; +} + +} // namespace + +class LpInvokeAsyncErrorTest : public ::testing::Test { +protected: + void SetUp() override { ensureApp(); } +}; + +// ── 1. The case that used to lie ──────────────────────────────────────────── +// +// Plain TCP to a port nothing listens on: the object cannot be acquired, which +// the protocol reports as "object_unavailable" — exactly what lp_invoke puts in +// out_error_json for the same call (test_call_error.cpp pins that half). +// Pre-fix this delivered ok=1 with json "null". +TEST_F(LpInvokeAsyncErrorTest, UnreachableTargetDeliversTheErrorToTheCallback) +{ + const char* deadTarget = + "{\"protocol\":\"tcp\",\"host\":\"127.0.0.1\",\"port\":9}"; + + // Pre-save a token so the capability requestModule handshake is skipped — + // this exercises transport acquisition only. + ASSERT_EQ(lp_token_save("async_missing_module", "test-token"), LP_OK); + + lp_client* client = lp_client_create("async_missing_module", "origin", + deadTarget, deadTarget); + ASSERT_NE(client, nullptr); + + Capture c; + ASSERT_EQ(lp_invoke_async(client, "anyMethod", "[1,2]", 1500, + &captureCb, &c), + LP_OK); + ASSERT_TRUE(pumpUntilFired(c, 10000)) << "async callback never fired"; + + std::cout << " FAILING async call -> ok=" << c.ok + << " json=" << c.json << std::endl; + + // The point of the whole fix. + EXPECT_EQ(c.ok, 0) << "a failed async call reported success"; + + nlohmann::json e = nlohmann::json::parse(c.json, nullptr, false); + ASSERT_TRUE(e.is_object()) << "ok==0 must carry the canonical error object"; + EXPECT_EQ(e.value("code", std::string{}), "object_unavailable"); + EXPECT_EQ(e.value("origin", std::string{}), "async_missing_module"); + EXPECT_FALSE(e.value("message", std::string{}).empty()); + + lp_client_destroy(client); +} + +// ── 2. The control ───────────────────────────────────────────────────────── +// +// A live provider over the same real transport must still report ok=1 with its +// value. Without this, "always report failure" would pass test 1. +TEST_F(LpInvokeAsyncErrorTest, LiveTargetStillReportsOkWithItsValue) +{ + LogosTransportConfig cfg; + cfg.protocol = LogosProtocol::Tcp; + cfg.host = "127.0.0.1"; + cfg.port = 0; // ephemeral + + auto host = std::make_unique(cfg); + ASSERT_TRUE(host->start()); + + EchoProvider provider; + ModuleProxy proxy(&provider); + // The token the consumer will present (pre-saved below), so ModuleProxy + // authorizes the call without a capability_module handshake. + ASSERT_TRUE(proxy.saveToken(QStringLiteral("origin"), + QStringLiteral("live-token"))); + ASSERT_TRUE(host->publishObject("async_live_module", &proxy)); + + const QString endpoint = host->endpoint(); + ASSERT_TRUE(endpoint.startsWith("tcp://")); + const uint16_t port = + endpoint.mid(endpoint.lastIndexOf(':') + 1).toUShort(); + ASSERT_NE(port, 0); + + const std::string liveTarget = + "{\"protocol\":\"tcp\",\"host\":\"127.0.0.1\",\"port\":" + + std::to_string(port) + "}"; + + ASSERT_EQ(lp_token_save("async_live_module", "live-token"), LP_OK); + + lp_client* client = lp_client_create("async_live_module", "origin", + liveTarget.c_str(), + liveTarget.c_str()); + ASSERT_NE(client, nullptr); + + Capture c; + ASSERT_EQ(lp_invoke_async(client, "compute", "[]", 5000, &captureCb, &c), + LP_OK); + ASSERT_TRUE(pumpUntilFired(c, 10000)) << "async callback never fired"; + + std::cout << " SUCCEEDING async call -> ok=" << c.ok + << " json=" << c.json << std::endl; + + EXPECT_EQ(c.ok, 1) << "a successful async call reported failure"; + nlohmann::json v = nlohmann::json::parse(c.json, nullptr, false); + ASSERT_TRUE(v.is_number()); + EXPECT_DOUBLE_EQ(v.get(), 7.0); + + lp_client_destroy(client); + // Let the deferred client teardown run before the host goes away. + QCoreApplication::processEvents(QEventLoop::AllEvents, 50); + host.reset(); +}