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>

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<CbGuard> 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<std::recursive_mutex> 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);
         },

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,

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

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 <gtest/gtest.h>
+
+#include "logos_protocol.h"
+
+#include "logos_provider_interface.h"
+#include "logos_transport_config.h"
+#include "module_proxy.h"
+
+#include "plain_transport_host.h"
+
+#include <QCoreApplication>
+#include <QElapsedTimer>
+#include <QJsonArray>
+#include <QString>
+#include <QVariant>
+#include <QVariantList>
+
+#include <nlohmann/json.hpp>
+
+#include <atomic>
+#include <cstdint>
+#include <iostream>
+#include <memory>
+#include <string>
+
+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<bool> fired{false};
+    int ok = -1;
+    std::string json;
+};
+
+void captureCb(int ok, const char* json, void* user_data)
+{
+    auto* c = static_cast<Capture*>(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<PlainTransportHost>(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<double>(), 7.0);
+
+    lp_client_destroy(client);
+    // Let the deferred client teardown run before the host goes away.
+    QCoreApplication::processEvents(QEventLoop::AllEvents, 50);
+    host.reset();
+}