Files
vdm/gui/src/rpc/RpcClient.cpp
T
samiandClaude Sonnet 5 c6f864ea30 gui: fix RpcClient double-free on stop() and the 1000-row list cap
Both found live while building gui/tests/dod's DoD harness, not from reading
the code — see that commit for how.

RpcClient::stop() left conn_ dangling after joining the worker thread: the
thread's own finish() flushes the DeferredDelete stop()'s
connect(&thread_, &QThread::finished, conn_, &QObject::deleteLater) already
posted, so conn_ is gone by the time stop() returns, but nothing cleared the
pointer. Any caller that calls stop() and later lets the client destruct
(the harness's own client.stop() at shutdown; also plain, correct API usage)
hit a double-free in the destructor's leftover `delete conn_`. Caught by
ASan on the very first run that actually exercised the stop-then-destroy
path.

requestInitialList() also called download.list with a hardcoded
`{"limit": 1000}`, silently capping the table at 1000 rows no matter how
many the daemon actually has — download.list.schema.json's own description
says "the GUI pages", not "the GUI takes it all in one call". The
scroll-60fps DoD gate refused to run against mockd --tasks 10000 rather
than "pass" against a 1000-row table, which is what surfaced it.
requestInitialList() now pages (5000 per call, the schema's own max) until
`total` is satisfied, then resets the model once with everything.

Separately: RpcConnection's session.subscribe list never included
event.settings.changed or event.grabber.progress, even though RpcClient has
carried signals for both since the Options/Grabber work — session.subscribe
"replaces the previous selection" and "nothing is delivered until this is
called", so both events were being silently dropped by any real daemon that
enforces the subscription (mockd does; verified live with a second
subscribed client actually receiving event.settings.changed after this
fix, round-tripped through a real veloxd's settings.set). GrabberWizard's
5 s poll fallback is exactly why this went unnoticed until now — it covered
for the missing push the whole time.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01NSCdCWFXBTSBBK3MzWtJiC
2026-09-12 21:27:28 +04:00

166 lines
6.9 KiB
C++

#include "rpc/RpcClient.hpp"
#include <utility>
#include <QLoggingCategory>
#include "rpc/RpcConnection.hpp"
namespace velox::gui::rpc {
// Off by default; enable with QT_LOGGING_RULES="velox.rpc.info=true".
Q_LOGGING_CATEGORY(lcRpc, "velox.rpc")
RpcClient::RpcClient(QString socketPath, QObject *parent) : QObject(parent) {
conn_ = new RpcConnection(std::move(socketPath));
conn_->moveToThread(&thread_);
// QThread flushes DeferredDelete for its objects after the loop exits, so this is the
// safe way to destroy an object that lives on the worker thread.
connect(&thread_, &QThread::finished, conn_, &QObject::deleteLater);
connect(conn_, &RpcConnection::stateChanged, this, &RpcClient::onConnectionState);
connect(conn_, &RpcConnection::notificationReceived, this, &RpcClient::onNotification);
connect(conn_, &RpcConnection::callCompleted, this, &RpcClient::onCallCompleted);
}
RpcClient::~RpcClient() {
if (thread_.isRunning()) {
stop(); // joins thread_; the finished -> deleteLater above disposes of conn_
} else {
delete conn_; // start() was never called: no loop will service deleteLater
}
conn_ = nullptr;
// Nothing will ever complete the in-flight callbacks now.
const auto pending = std::move(callbacks_);
callbacks_.clear();
for (const auto &cb : pending) {
if (cb) {
RpcReply reply;
reply.error = {-1, QStringLiteral("client shutting down"), {}};
cb(reply);
}
}
}
void RpcClient::start() {
if (!thread_.isRunning()) {
thread_.start();
}
QMetaObject::invokeMethod(conn_, "start", Qt::QueuedConnection);
}
void RpcClient::stop() {
if (!thread_.isRunning()) {
return;
}
QMetaObject::invokeMethod(conn_, "stop", Qt::QueuedConnection);
thread_.quit();
thread_.wait();
// thread_.wait() does not return until thread_'s own finish() has already flushed the
// DeferredDelete this class's own connect(&thread_, &QThread::finished, conn_,
// &QObject::deleteLater) posted — conn_ is gone by now. Null it out so a later call
// (stop() is a public slot; a caller stopping and then destroying the client is normal
// use, and the destructor's own `delete conn_` for the never-started case must not
// run a second time against memory this path already freed).
conn_ = nullptr;
}
void RpcClient::call(const QString &methodName, const QJsonObject &params,
std::function<void(const RpcReply &)> cb) {
const quint64 tag = nextTag_++;
callbacks_.insert(tag, std::move(cb));
QMetaObject::invokeMethod(conn_, "sendCall", Qt::QueuedConnection, Q_ARG(quint64, tag),
Q_ARG(QString, methodName), Q_ARG(QJsonObject, params));
}
void RpcClient::onConnectionState(int state) {
state_ = static_cast<ConnectionState>(state);
qCInfo(lcRpc, "connection state: %s", toString(state_));
emit stateChanged(state_);
if (state_ == ConnectionState::Connected) {
requestInitialList();
}
}
void RpcClient::requestInitialList() {
fetchListPage(0, {});
}
// download.list.schema.json: "Filtering, sorting and paging all happen in the daemon so
// the GUI never materializes 100k rows to show 40" — limit maxes out at 5000, so one call
// cannot ever return everything for a table the DoD's own gate says can hold 10 000 rows.
// A single fixed-limit call here silently truncated the table below that (caught by
// gui/tests/dod's scroll-60fps gate refusing to run against a 1000-row table when mockd
// seeded 10000). Page until `total` is satisfied, then reset the model exactly once.
void RpcClient::fetchListPage(int offset, QJsonArray accumulated) {
constexpr int kPageSize = 5000; // download.list's own maximum
constexpr int kMaxPages = 100; // 500 000 rows — a safety cap, not an expected ceiling
call(QString::fromLatin1(method::kDownloadList),
QJsonObject{{"offset", offset}, {"limit", kPageSize}},
[this, offset, accumulated](const RpcReply &reply) mutable {
if (!reply.ok()) {
qCWarning(lcRpc, "download.list failed: %d %s", reply.error.code,
qUtf8Printable(reply.error.message));
if (!accumulated.isEmpty()) {
emit taskListReset(accumulated); // show what we got rather than nothing
}
return;
}
const QJsonObject result = reply.result.toObject();
const QJsonArray page = result.value("items").toArray();
const qint64 total = static_cast<qint64>(result.value("total").toDouble());
for (const QJsonValue &item : page) {
accumulated.append(item);
}
const bool morePages =
!page.isEmpty() && accumulated.size() < total && (offset / kPageSize) < kMaxPages;
if (morePages) {
fetchListPage(offset + static_cast<int>(page.size()), accumulated);
return;
}
qCInfo(lcRpc, "initial download.list: %lld of %lld row(s)",
static_cast<long long>(accumulated.size()), static_cast<long long>(total));
emit taskListReset(accumulated);
});
}
void RpcClient::onNotification(const QString &methodName, const QJsonObject &params) {
qCDebug(lcRpc, "notification: %s", qUtf8Printable(methodName));
if (methodName == QLatin1String(event::kTaskProgress)) {
const QJsonArray tasks = params.value("tasks").toArray();
qCDebug(lcRpc, "progress batch: %lld task(s)", static_cast<long long>(tasks.size()));
emit taskProgress(tasks);
} else if (methodName == QLatin1String(event::kTaskAdded)) {
emit taskAdded(params.value("summary").toObject());
} else if (methodName == QLatin1String(event::kTaskState)) {
emit taskStateChanged(params);
} else if (methodName == QLatin1String(event::kTaskRemoved)) {
emit taskRemoved(params.value("taskId").toString());
} else if (methodName == QLatin1String(event::kSpeedGlobal)) {
emit speedGlobal(params);
} else if (methodName == QLatin1String(event::kNotify)) {
emit notify(params);
} else if (methodName == QLatin1String(event::kSettingsChanged)) {
emit settingsChanged(params);
} else if (methodName == QLatin1String(event::kGrabberProgress)) {
emit grabberProgress(params);
}
}
void RpcClient::onCallCompleted(quint64 tag, const QJsonObject &envelope) {
const auto cb = callbacks_.take(tag);
if (!cb) {
return;
}
RpcReply reply;
if (envelope.contains("error")) {
const QJsonObject e = envelope.value("error").toObject();
reply.error = {e.value("code").toInt(), e.value("message").toString(), e.value("data")};
} else {
reply.result = envelope.value("result");
}
cb(reply);
}
} // namespace velox::gui::rpc