mirror of
https://github.com/logos-co/logos-logoscore-cli.git
synced 2026-08-31 04:41:06 +00:00
Against a session whose daemon is no longer there, `logosctl module ls` waited 22 seconds and then printed `[]` and exited 0. Not "failed slowly" -- reported success, with an empty module list, about a daemon that did not exist. `stats` did the same. `call`, `package`, `catalog` and `key` waited the same 20 seconds before reporting RPC_FAILED. Only `stop` and `status` were quick, because #100 gave them a guard the other fourteen commands never got. The mechanism is the one #100 diagnosed. A LocalSocket client "connects" to a socket path with no listener without complaint, QtRO surfaces no transport error for an absent peer, and the request is therefore neither answered nor refused -- so it waits out Timeout(20000) (logos-protocol, cpp/logos_mode.h) and a dead daemon is indistinguishable from a slow one until the deadline fires. Connecting is not the check it looks like. A session outlives its daemon in two shapes, and they need different evidence. CRASHED SESSION. daemon/state.json is still on disk naming a pid that is gone. This is #100's check, and it was copied into stop_command and status_command. It now lives in one place -- detectStaleSession(), called from Command::ensureConnected() -- which is the single door every RPC-opening command goes through, so all of them inherit it instead of the two that had it hand-written. StatusCommand still calls the helper itself, one step earlier, because its answer to "no daemon" is a status report rather than an error. #100's instance_id gate is preserved exactly: the guard fires only when the state file describes the daemon THIS client dials. A remote client can have a co-resident daemon's leftovers sitting in its own session directory, and its dial spec carries no instance_id at all, so an empty one never matches. The liveness syscall now runs before the client-config read, so the common path (daemon running) does not parse client/config.yaml twice per command. STOPPED SESSION. The tidier way to get here, and the one the pid guard cannot see: a clean `daemon stop` REMOVES daemon/state.json, leaving client/config.yaml and the token behind with no pid left to find dead. Every command still waited the full 20s. RpcClient::connect() now asks the socket instead, before it builds a LogosAPIClient (localEndpointProvablyAbsent, src/local_endpoint.h): the dial resolves to QDir::tempPath()/logos_core_service_<instance_id>, because the SDK asks for the bare name (LogosInstance::id) and Qt resolves a bare QLocalSocket/QLocalServer name against the temp dir. Deriving it the same way is what makes the answer sound rather than a guess. A stat alone is NOT enough, which cost this patch a wrong first draft. The socket file outlives the daemon: a hard kill leaves it, and a clean stop leaves it for the window between the shutdown reply and QLocalServer's destructor -- which is exactly when the next command gets typed. Measured through the new CLI sweep, stat-only vs stat-plus-connect over the same abandoned socket: 85.3s (every command timed out) vs 0.8s. So presence settles nothing and being REFUSED does; ECONNREFUSED is the same signal logos::isSocketDead uses to decide a socket is safe for the daemon's boot reaper to unlink. That function is not reused directly only because it sits behind the logos-protocol link, which logosctl_testlib deliberately does without. The check fails closed on everything short of proof: a socket that accepts us, any other connect() error, a path too long for sun_path, a non-socket inode, a tcp/tcp_ssl dial, an empty instance_id, Windows (named pipes, no inode). Refusing a reachable daemon would be far worse than the wait being removed. AN UNANSWERED QUERY IS NOT AN EMPTY ONE. The exit-0 half is a separate defect and survives independently of the timing: listModules() and getModuleStats() answered a failed RPC with LogosList::array(), the only two calls in the client that reported failure as data. Both now return optional<LogosList>, and the commands report DAEMON_UNREACHABLE with exit 2. `status` had the same shape by a different route -- RpcClient::getStatus synthesises a not_running report and marks it `rpc_error`, and that report has a "daemon" key, so it reached the success branch and exited 0 while printing "not running". It exits 1 now, as docs/project.md always said it did. `status` also connects directly rather than through ensureConnected(): that helper PRINTS a NO_DAEMON envelope, and letting it do so put two JSON documents on stdout for one command, which no `jq` invocation survives. Nothing opts out of the guard. `watch` is the one command with a case for waiting -- a daemon that has not started yet is a reasonable thing to watch for -- but it does no waiting today: it connects once and gives up, so failing in milliseconds is what it already meant to do. The four commands the issue listed that are NOT covered (`token issue|revoke|list`, `daemon|client config`) never call ensureConnected at all: they read and write the session's own files and have no daemon to be absent. TESTS. * CLITest.{Crashed,CleanlyStopped}Session_EveryRpcCommandFailsAtOnce and SocketLeftOverWithNoListener_EveryRpcCommandFailsAtOnce: all 17 commands against all three shapes, end-to-end, killed at 5s so exit 124 means the command was still waiting. Driven against the pre-fix binary via $LOGOSCTL_BINARY these fail with 124 on 15 of 17 commands, 80.3s. * CLITest.*_StatusReportsNotRunningAtOnce: exit 1, names the pid where there is one, and exactly one JSON document. * CommandTest.EveryRpcCommand_*: the 17 commands x 4 session shapes, against a mock, asserting on connectAttempts/rpcCalls -- a guard that fired is visible as the ABSENCE of contact. Three of the four shapes are the controls: live pid, foreign instance_id, and no state file at all must still dial. * LocalEndpointTest.*: the path derivation against QDir::tempPath(), plus a verdict for each shape the path can be in -- missing, socket with no listener, LIVE listener, and a regular file wearing the name. * CommandTest.{ListModules,Stats}_{UnansweredRpc,AnsweredWithNothing}_* and Status_{UnansweredRpc,LiveDaemon}_*: both sides of the empty-vs-unanswered line. CommandTest had no Status_ coverage at all, which is how exit 0 survived. Before/after over the shipped binaries, same stale session, macOS: module ls exit 0 after 22s printing [] -> exit 2 in <1s, names the pid stats exit 0 after 20s printing [] -> exit 2 in <1s status exit 0 after 20s -> exit 1 in <1s call/package/catalog/key 20s, RPC_FAILED -> exit 2 in <1s and against a cleanly stopped session, where nothing was fast before, all of the above are now under a second too. Live-daemon behaviour is unchanged and checked: 249 unit + 30 CLI + 25 integration tests pass for logosctl and 20 CLI + 24 integration for logoscore via `nix build .#checks.<sys>.tests-logosctl` / `-logoscore`. The 25 integration tests drive real daemons through logosctl, so a wrong socket path would fail them loudly rather than silently refusing live sessions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
149 lines
4.8 KiB
C++
149 lines
4.8 KiB
C++
#include <gtest/gtest.h>
|
|
|
|
#include "local_endpoint.h"
|
|
|
|
#include <QDir>
|
|
|
|
#include <cstring>
|
|
#include <fstream>
|
|
#include <string>
|
|
|
|
#ifndef _WIN32
|
|
#include <sys/socket.h>
|
|
#include <sys/un.h>
|
|
#include <unistd.h>
|
|
#endif
|
|
|
|
// localEndpointProvablyAbsent() is what turns "a daemon that stopped" from a
|
|
// twenty-second wait into an immediate answer, and it is the one piece of this
|
|
// that could refuse a LIVE daemon if it got either half wrong. So these pin
|
|
// both halves: the path derivation (against QDir::tempPath(), which is what Qt
|
|
// resolves a bare QLocalSocket/QLocalServer name against) and the liveness
|
|
// verdict for each shape the path can be in.
|
|
|
|
namespace {
|
|
|
|
std::string uniqueId(const char* suffix)
|
|
{
|
|
return "ut" + std::to_string(::getpid()) + suffix;
|
|
}
|
|
|
|
QString endpointPath(const std::string& instanceId)
|
|
{
|
|
return QDir::tempPath()
|
|
+ QStringLiteral("/logos_core_service_")
|
|
+ QString::fromStdString(instanceId);
|
|
}
|
|
|
|
#ifndef _WIN32
|
|
// Bind and listen at `path`. Returns the fd, or -1. Closing the fd without
|
|
// unlinking leaves exactly what a hard-killed daemon leaves: a socket inode
|
|
// with nobody behind it.
|
|
int bindListen(const QString& path)
|
|
{
|
|
const std::string p = path.toStdString();
|
|
sockaddr_un addr{};
|
|
if (p.size() >= sizeof(addr.sun_path)) return -1;
|
|
addr.sun_family = AF_UNIX;
|
|
std::memcpy(addr.sun_path, p.c_str(), p.size());
|
|
|
|
const int fd = ::socket(AF_UNIX, SOCK_STREAM, 0);
|
|
if (fd < 0) return -1;
|
|
::unlink(p.c_str());
|
|
if (::bind(fd, reinterpret_cast<sockaddr*>(&addr), sizeof(addr)) != 0
|
|
|| ::listen(fd, 4) != 0) {
|
|
::close(fd);
|
|
return -1;
|
|
}
|
|
return fd;
|
|
}
|
|
#endif
|
|
|
|
} // namespace
|
|
|
|
TEST(LocalEndpointTest, ReportsTheDerivedPathItChecked)
|
|
{
|
|
#ifdef _WIN32
|
|
GTEST_SKIP() << "named pipes: no path is derived";
|
|
#else
|
|
std::string path;
|
|
logosctl::localEndpointProvablyAbsent("core_service", "abc123", &path);
|
|
EXPECT_EQ(path, endpointPath("abc123").toStdString())
|
|
<< "the path must be the one a bare QLocalSocket name resolves to, or "
|
|
"the check is answering a question about the wrong file";
|
|
#endif
|
|
}
|
|
|
|
TEST(LocalEndpointTest, NoSocketFileAtAll_IsProvablyAbsent)
|
|
{
|
|
// What a clean `daemon stop` leaves: QLocalServer's destructor unlinks it.
|
|
const std::string id = uniqueId("_gone");
|
|
QDir().remove(endpointPath(id));
|
|
|
|
#ifdef _WIN32
|
|
EXPECT_FALSE(logosctl::localEndpointProvablyAbsent("core_service", id));
|
|
#else
|
|
EXPECT_TRUE(logosctl::localEndpointProvablyAbsent("core_service", id));
|
|
#endif
|
|
}
|
|
|
|
#ifndef _WIN32
|
|
TEST(LocalEndpointTest, SocketFileWithNoListener_IsProvablyAbsent)
|
|
{
|
|
// The case a stat cannot answer, and the reason this does a connect at
|
|
// all. A hard-killed daemon leaves the inode behind, and even a clean stop
|
|
// leaves a window between the shutdown reply and the destructor running --
|
|
// which is exactly when someone types the next command.
|
|
const std::string id = uniqueId("_dead");
|
|
const QString path = endpointPath(id);
|
|
|
|
const int fd = bindListen(path);
|
|
ASSERT_GE(fd, 0) << "could not bind " << path.toStdString();
|
|
::close(fd); // listener gone, inode stays
|
|
ASSERT_TRUE(QDir().exists(path)) << "the socket file should have survived";
|
|
|
|
EXPECT_TRUE(logosctl::localEndpointProvablyAbsent("core_service", id))
|
|
<< "a socket file nobody is listening on is not a reachable daemon";
|
|
|
|
::unlink(path.toStdString().c_str());
|
|
}
|
|
|
|
TEST(LocalEndpointTest, LiveListener_IsNeverCalledAbsent)
|
|
{
|
|
// The control, and the one that matters most: refusing a reachable daemon
|
|
// is far worse than the wait this avoids.
|
|
const std::string id = uniqueId("_live");
|
|
const QString path = endpointPath(id);
|
|
|
|
const int fd = bindListen(path);
|
|
ASSERT_GE(fd, 0) << "could not bind " << path.toStdString();
|
|
|
|
EXPECT_FALSE(logosctl::localEndpointProvablyAbsent("core_service", id));
|
|
|
|
::close(fd);
|
|
::unlink(path.toStdString().c_str());
|
|
}
|
|
#endif
|
|
|
|
TEST(LocalEndpointTest, SomeOtherFileWearingTheName_IsNotEvidence)
|
|
{
|
|
// Only S_ISSOCK inodes get an opinion. A regular file that happens to
|
|
// match the name says nothing about any daemon.
|
|
const std::string id = uniqueId("_plain");
|
|
const QString path = endpointPath(id);
|
|
{ std::ofstream ofs(path.toStdString(), std::ios::trunc); ofs << "x"; }
|
|
ASSERT_TRUE(QDir().exists(path));
|
|
|
|
EXPECT_FALSE(logosctl::localEndpointProvablyAbsent("core_service", id));
|
|
|
|
QDir().remove(path);
|
|
}
|
|
|
|
TEST(LocalEndpointTest, NothingToDeriveANameFromIsNotEvidence)
|
|
{
|
|
// A remote dial spec carries no instance_id. That says nothing about any
|
|
// local socket, so it must not be read as "the endpoint is missing".
|
|
EXPECT_FALSE(logosctl::localEndpointProvablyAbsent("core_service", ""));
|
|
EXPECT_FALSE(logosctl::localEndpointProvablyAbsent("", "abc123"));
|
|
}
|