From de748cc2fc714bbff3a03b518fc37225228b5887 Mon Sep 17 00:00:00 2001 From: sami Date: Fri, 11 Sep 2026 17:04:02 +0400 Subject: [PATCH] daemon: fix startMode 'later' -32603 and isolate the single-instance lock by runtime dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs blocking PROTO's live-veloxd conformance check. 1. tasks.start_mode's CHECK was ('auto','now','queue','manual') — not the contract's StartMode enum (['now','later','queue']) at all. 'later', a real documented value (the File Info dialog's Download Later button), hit the CHECK on every insert and surfaced as an unhandled -32603; 'auto'/'manual' were never contract values to begin with. Migration 0003 rebuilds tasks (SQLite can't ALTER a CHECK) with the contract's values, remapping existing rows by what they actually meant: 'auto' -> 'now' (eligible for the scheduler immediately), 'manual' -> 'later' (parked, matching StartMode's own "lands the task in paused" description). store_migrations_test covers the remap and that 'later' inserts clean while the retired spellings are rejected. dispatcher.cpp's on_download_add matched: default (absent startMode) is now 'now' instead of the invented 'auto'; 'later' actually lands the task in `paused` (pause_reason 'user') instead of a dead 'manual' -> `new` branch that spec.startMode (typed as the 3-value enum) could never even reach. TaskRow::start_mode's in-memory default followed suit ('now'). 2. main.cpp's single-instance guard bound an abstract socket named "velox-daemon-" — one name per user, system-wide. XDG_RUNTIME_DIR isolation never reached it: a leaked test veloxd held the lock for 4h40m and locked out every other isolated instance with the same euid (PROTO, EXT, the orchestrator), real daemon included. Extracted rpc/single_instance.{hpp,cpp} (was a static in main.cpp, untestable) and derived the abstract-socket name from a hash of the resolved runtime dir path instead of euid alone. The real per-user daemon is still unique (its runtime dir is unique to it); isolated instances pointed at their own runtime dirs now coexist. main() resolves the runtime dir before acquiring the lock (was the other way around). New single_instance_test covers same-dir refusal, different-dir coexistence, and release-on-close. Verified against real veloxd binaries, not just unit tests: startMode: "later" via a live download.add lands in `paused`; two veloxd with different runtime dirs run concurrently, two with the same one and the second refuses with the runtime dir named in the error. Full ctest: 39/39. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GRDjHGgpYmMoPE2UFbe7pP --- daemon/CMakeLists.txt | 1 + daemon/src/main.cpp | 38 ++------ daemon/src/rpc/dispatcher.cpp | 17 +++- daemon/src/rpc/single_instance.cpp | 38 ++++++++ daemon/src/rpc/single_instance.hpp | 27 ++++++ .../0003_start_mode_contract_values.sql | 91 +++++++++++++++++++ daemon/src/store/tasks.hpp | 2 +- daemon/tests/CMakeLists.txt | 1 + daemon/tests/single_instance_test.cpp | 40 ++++++++ daemon/tests/store_migrations_test.cpp | 50 ++++++++++ 10 files changed, 270 insertions(+), 35 deletions(-) create mode 100644 daemon/src/rpc/single_instance.cpp create mode 100644 daemon/src/rpc/single_instance.hpp create mode 100644 daemon/src/store/migrations/0003_start_mode_contract_values.sql create mode 100644 daemon/tests/single_instance_test.cpp diff --git a/daemon/CMakeLists.txt b/daemon/CMakeLists.txt index 6d896ea..cccadd2 100644 --- a/daemon/CMakeLists.txt +++ b/daemon/CMakeLists.txt @@ -73,6 +73,7 @@ target_link_libraries(veloxd_sched PUBLIC velox::proto velox::core veloxd_store # --- veloxd_rpc — the RPC transports + dispatcher ------------------------------------ add_library(veloxd_rpc STATIC src/rpc/runtime_dir.cpp + src/rpc/single_instance.cpp src/rpc/event_loop.cpp src/rpc/event_hub.cpp src/rpc/uds_server.cpp diff --git a/daemon/src/main.cpp b/daemon/src/main.cpp index 14a1567..a70a9bf 100644 --- a/daemon/src/main.cpp +++ b/daemon/src/main.cpp @@ -24,6 +24,7 @@ #include "rpc/event_loop.hpp" #include "rpc/pairing.hpp" #include "rpc/runtime_dir.hpp" +#include "rpc/single_instance.hpp" #include "rpc/uds_server.hpp" #include "rpc/ws_server.hpp" #include "sched/engine_port_core.hpp" @@ -43,48 +44,25 @@ void on_signal(int) { if (g_loop != nullptr) g_loop->stop(); // stop() is async-signal-safe (writes an eventfd) } -// Single-instance guard: bind an abstract-namespace Unix socket whose name is unique to -// this user. A second daemon gets EADDRINUSE and exits. The kernel reclaims an -// abstract-namespace address when the holding process dies, so a crash never wedges it -// (docs/01 §2). Returns the held fd (kept open for the process lifetime) or -1. -int acquire_single_instance_lock() { - const int fd = ::socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); - if (fd < 0) return -1; - - const std::string name = std::string("velox-daemon-") + std::to_string(::geteuid()); - sockaddr_un addr{}; - addr.sun_family = AF_UNIX; - // Leading NUL selects the abstract namespace; the name follows, not NUL-terminated. - addr.sun_path[0] = '\0'; - std::memcpy(addr.sun_path + 1, name.c_str(), name.size()); - const socklen_t len = - static_cast(offsetof(sockaddr_un, sun_path) + 1 + name.size()); - - if (::bind(fd, reinterpret_cast(&addr), len) != 0) { - ::close(fd); - return -1; - } - return fd; -} - } // namespace int main() { std::cout << "veloxd " << velox::daemon::kDaemonVersion << " (protocol " << velox::proto::kProtocolVersion << ")\n"; - const int lock_fd = acquire_single_instance_lock(); - if (lock_fd < 0) { - std::cerr << "veloxd: another instance is already running for this user\n"; - return 1; - } - velox::daemon::rpc::RuntimeDir rt; if (const auto ec = velox::daemon::rpc::resolve_runtime_dir(rt)) { std::cerr << "veloxd: cannot prepare runtime directory: " << ec.message() << "\n"; return 1; } + const int lock_fd = velox::daemon::rpc::acquire_single_instance_lock(rt.path); + if (lock_fd < 0) { + std::cerr << "veloxd: another instance is already running for this runtime " + "directory (" << rt.path << ")\n"; + return 1; + } + velox::daemon::rpc::EventLoop loop; g_loop = &loop; diff --git a/daemon/src/rpc/dispatcher.cpp b/daemon/src/rpc/dispatcher.cpp index 524ad37..fafec40 100644 --- a/daemon/src/rpc/dispatcher.cpp +++ b/daemon/src/rpc/dispatcher.cpp @@ -152,10 +152,19 @@ VeloxDispatcher::on_download_add(const proto::DownloadSpec& spec) { row.save_dir = target->dir; row.filename = target->leaf; row.created_at = velox::daemon::now_iso(); - row.start_mode = spec.startMode ? std::string(proto::to_string(*spec.startMode)) : "auto"; - // startMode 'manual' parks the task in `new`; anything else makes it eligible for the - // scheduler (`queued`); on_mutation_ nudges it. - row.state = row.start_mode == "manual" ? "new" : "queued"; + // Contract values only (StartMode.schema.json: 'now'|'later'|'queue'); absent means + // "start it", same as an explicit 'now'. + row.start_mode = + spec.startMode ? std::string(proto::to_string(*spec.startMode)) : "now"; + // 'later' — the File Info dialog's Download Later button — lands the task in `paused` + // per StartMode's own description; anything else ('now' or 'queue') is eligible for + // the scheduler immediately (`queued`); on_mutation_ nudges it. + if (row.start_mode == "later") { + row.state = "paused"; + row.pause_reason = "user"; + } else { + row.state = "queued"; + } row.category_id = spec.categoryId; row.queue_id = spec.queueId; row.description = spec.description; diff --git a/daemon/src/rpc/single_instance.cpp b/daemon/src/rpc/single_instance.cpp new file mode 100644 index 0000000..5385c96 --- /dev/null +++ b/daemon/src/rpc/single_instance.cpp @@ -0,0 +1,38 @@ +#include "rpc/single_instance.hpp" + +#include +#include + +#include +#include +#include + +#include "util/crypto.hpp" + +namespace velox::daemon::rpc { + +int acquire_single_instance_lock(const std::string& runtime_dir) { + const int fd = ::socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); + if (fd < 0) return -1; + + // Truncated to 16 hex chars (64 bits): a collision would need two distinct runtime + // dirs to hash together, which is not a security boundary here — the socket itself is + // still 0600 same-UID-checked; this is only "don't let two daemons stomp each other". + const std::string name = + "velox-daemon-" + velox::daemon::crypto::sha256_hex(runtime_dir).substr(0, 16); + sockaddr_un addr{}; + addr.sun_family = AF_UNIX; + // Leading NUL selects the abstract namespace; the name follows, not NUL-terminated. + addr.sun_path[0] = '\0'; + std::memcpy(addr.sun_path + 1, name.c_str(), name.size()); + const socklen_t len = + static_cast(offsetof(sockaddr_un, sun_path) + 1 + name.size()); + + if (::bind(fd, reinterpret_cast(&addr), len) != 0) { + ::close(fd); + return -1; + } + return fd; +} + +} // namespace velox::daemon::rpc diff --git a/daemon/src/rpc/single_instance.hpp b/daemon/src/rpc/single_instance.hpp new file mode 100644 index 0000000..c2b87a4 --- /dev/null +++ b/daemon/src/rpc/single_instance.hpp @@ -0,0 +1,27 @@ +#pragma once + +// Single-instance guard: bind an abstract-namespace Unix socket whose name is derived +// from the canonical runtime directory (resolve_runtime_dir's result — already the +// per-user default, /run/user//velox, unless XDG_RUNTIME_DIR says otherwise). A +// second daemon pointed at the same runtime dir gets EADDRINUSE and exits; one pointed at +// a different (isolated / test) runtime dir gets its own lock and starts fine. The kernel +// reclaims an abstract-namespace address when the holding process dies, so a crash never +// wedges it (docs/01 §2). +// +// Naming this "velox-daemon-" alone (the old scheme) meant exactly one name per +// user system-wide, so XDG_RUNTIME_DIR isolation never reached it: a leaked test veloxd +// with the same euid held the lock for every isolated instance too, real or test, until +// it was killed. Hashing the resolved runtime dir path instead keeps the real per-user +// daemon unique (its runtime dir is unique to it) while letting isolated instances that +// each point at their own runtime dir coexist. + +#include + +namespace velox::daemon::rpc { + +// Returns the held fd (kept open for the process lifetime; closing it releases the lock) +// or -1 if another process already holds the lock for this exact `runtime_dir`, or on any +// other socket error. +int acquire_single_instance_lock(const std::string& runtime_dir); + +} // namespace velox::daemon::rpc diff --git a/daemon/src/store/migrations/0003_start_mode_contract_values.sql b/daemon/src/store/migrations/0003_start_mode_contract_values.sql new file mode 100644 index 0000000..32cb6ef --- /dev/null +++ b/daemon/src/store/migrations/0003_start_mode_contract_values.sql @@ -0,0 +1,91 @@ +-- Migration 0003 — tasks.start_mode is rebuilt to the contract's StartMode values. +-- +-- 0001's CHECK read `start_mode IN ('auto','now','queue','manual')`. That is not +-- StartMode.schema.json's enum at all: the contract is ['now','later','queue']. The +-- practical effect: `download.add` with `startMode: "later"` — a real, documented value +-- (the File Info dialog's Download Later button) — hit the CHECK constraint on insert +-- and surfaced as an unhandled -32603, every time. 'auto' and 'manual' were never +-- contract values; they were this table's own invention and nothing on the wire ever +-- sends them. +-- +-- SQLite cannot ALTER a CHECK constraint in place, so this rebuilds the table (same +-- pattern as 0002: create the new shape, copy with the value mapped, drop, rename). +-- Existing rows are remapped by what they actually meant: 'auto' was "eligible for the +-- scheduler the moment it's added", i.e. 'now'; 'manual' was "parked, wait for the user", +-- which is what 'later' means on the wire (StartMode's own description: "lands the task +-- in paused"). Any row already spelled 'now' or 'queue' passes through unchanged. +-- +-- This does NOT touch `state` or `pause_reason` — a row that was start_mode='manual' and +-- (per the dispatcher's now-dead branch) state='new' keeps state='new'; the daemon-side +-- fix to actually land a 'later' task in 'paused' going forward lives in dispatcher.cpp, +-- not in this migration. Historical rows are not replayed through the scheduler. + +CREATE TABLE tasks_new ( + task_id TEXT PRIMARY KEY, + url TEXT NOT NULL, + effective_url TEXT, + filename TEXT NOT NULL DEFAULT '', + save_dir TEXT NOT NULL, + category_id TEXT REFERENCES categories(category_id) ON DELETE SET NULL, + queue_id TEXT REFERENCES queues(queue_id) ON DELETE SET NULL, + queue_position INTEGER, + + state TEXT NOT NULL DEFAULT 'new' + CHECK (state IN ('new','probing','queued','connecting','downloading','paused', + 'retry_wait','assembling','verifying','complete','failed','cancelled')), + pause_reason TEXT CHECK (pause_reason IN + ('user','schedule','queue_stopped','admission_reconcile','auto')), + + size_bytes INTEGER, + downloaded_bytes INTEGER NOT NULL DEFAULT 0, + resumable INTEGER NOT NULL DEFAULT 0, + + req_segments INTEGER, + eff_segments INTEGER NOT NULL DEFAULT 0, + req_buffer_bytes INTEGER, + eff_buffer_bytes INTEGER, + + -- The contract's StartMode (StartMode.schema.json): 'now' | 'later' | 'queue'. + start_mode TEXT NOT NULL DEFAULT 'now' + CHECK (start_mode IN ('now','later','queue')), + description TEXT, + + etag TEXT, + last_modified TEXT, + content_type TEXT, + + checksum_algo TEXT CHECK (checksum_algo IN ('md5','sha1','sha256','sha512')), + checksum_value TEXT, + + error_code TEXT, + error_message TEXT, + error_http_status INTEGER, + error_retryable INTEGER, + error_attempt INTEGER, + error_next_retry_at TEXT, + + speed_bps INTEGER NOT NULL DEFAULT 0, + + created_at TEXT NOT NULL, + last_try_at TEXT, + completed_at TEXT +) STRICT; + +INSERT INTO tasks_new + SELECT task_id, url, effective_url, filename, save_dir, category_id, queue_id, + queue_position, state, pause_reason, size_bytes, downloaded_bytes, resumable, + req_segments, eff_segments, req_buffer_bytes, eff_buffer_bytes, + CASE start_mode WHEN 'auto' THEN 'now' WHEN 'manual' THEN 'later' ELSE start_mode END, + description, etag, last_modified, content_type, checksum_algo, checksum_value, + error_code, error_message, error_http_status, error_retryable, error_attempt, + error_next_retry_at, speed_bps, created_at, last_try_at, completed_at + FROM tasks; + +DROP TABLE tasks; +ALTER TABLE tasks_new RENAME TO tasks; + +CREATE INDEX idx_tasks_state ON tasks(state); +CREATE INDEX idx_tasks_category ON tasks(category_id); +CREATE INDEX idx_tasks_queue_order ON tasks(queue_id, queue_position); +CREATE INDEX idx_tasks_created ON tasks(created_at); +CREATE INDEX idx_tasks_completed ON tasks(completed_at); diff --git a/daemon/src/store/tasks.hpp b/daemon/src/store/tasks.hpp index 71b33db..4012a7b 100644 --- a/daemon/src/store/tasks.hpp +++ b/daemon/src/store/tasks.hpp @@ -21,7 +21,7 @@ struct TaskRow { std::string save_dir; std::string filename; std::string state = "new"; - std::string start_mode = "auto"; + std::string start_mode = "now"; // contract StartMode: 'now'|'later'|'queue' std::string created_at; std::optional effective_url; diff --git a/daemon/tests/CMakeLists.txt b/daemon/tests/CMakeLists.txt index 9c84ca4..91803c6 100644 --- a/daemon/tests/CMakeLists.txt +++ b/daemon/tests/CMakeLists.txt @@ -23,3 +23,4 @@ veloxd_test(store_tasks LIBS veloxd_store) veloxd_test(sched_scheduler LIBS veloxd_sched veloxd_rpc) veloxd_test(event_hub LIBS veloxd_rpc) veloxd_test(store_categories_queues LIBS veloxd_store) +veloxd_test(single_instance LIBS veloxd_rpc) diff --git a/daemon/tests/single_instance_test.cpp b/daemon/tests/single_instance_test.cpp new file mode 100644 index 0000000..60e5e2d --- /dev/null +++ b/daemon/tests/single_instance_test.cpp @@ -0,0 +1,40 @@ +// The single-instance lock: isolated by runtime dir, not just by euid. This is the bug a +// leaked test veloxd exploited — one abstract-socket name per user meant every isolated +// instance (real daemon, tests, other lanes) fought over the same lock. + +#include + +#include "check.hpp" +#include "rpc/single_instance.hpp" + +using namespace velox::daemon::rpc; + +void run() { + // Two different runtime dirs: both acquire the lock independently. + { + const int a = acquire_single_instance_lock("/run/user/1000/velox-test-a"); + const int b = acquire_single_instance_lock("/run/user/1000/velox-test-b"); + CHECK(a >= 0); + CHECK(b >= 0); + if (a >= 0) ::close(a); + if (b >= 0) ::close(b); + } + + // Same runtime dir: the second attempt is refused while the first still holds it. + { + const int first = acquire_single_instance_lock("/run/user/1000/velox-test-shared"); + CHECK(first >= 0); + const int second = acquire_single_instance_lock("/run/user/1000/velox-test-shared"); + CHECK(second < 0); + if (first >= 0) ::close(first); + if (second >= 0) ::close(second); + + // Releasing (closing) the fd frees the abstract-namespace name immediately — a + // third attempt at the same dir succeeds once the first is gone. + const int third = acquire_single_instance_lock("/run/user/1000/velox-test-shared"); + CHECK(third >= 0); + if (third >= 0) ::close(third); + } +} + +TEST_MAIN() diff --git a/daemon/tests/store_migrations_test.cpp b/daemon/tests/store_migrations_test.cpp index 76e97d9..9b41389 100644 --- a/daemon/tests/store_migrations_test.cpp +++ b/daemon/tests/store_migrations_test.cpp @@ -86,6 +86,56 @@ void run() { .has_value()); } + // --- 0003: start_mode is rebuilt to the contract's values, existing rows mapped - + { + auto db = Db::open(":memory:"); + CHECK(db.has_value()); + if (!db) return; + // Build a real pre-0003 db (schema 1..2) with rows in the old, non-contract + // start_mode spelling, the way an actually-released daemon would have them. + for (const auto& m : embedded_migrations()) { + if (m.version > 2) break; + CHECK(db->exec(m.sql).has_value()); + CHECK(db->set_user_version(m.version).has_value()); + } + CHECK(db->exec("INSERT INTO tasks(task_id,url,save_dir,created_at,start_mode) " + "VALUES('auto1','http://x','/tmp','2026-09-10T00:00:00Z','auto')") + .has_value()); + CHECK(db->exec("INSERT INTO tasks(task_id,url,save_dir,created_at,start_mode) " + "VALUES('man1','http://x','/tmp','2026-09-10T00:00:00Z','manual')") + .has_value()); + CHECK(db->exec("INSERT INTO tasks(task_id,url,save_dir,created_at,start_mode) " + "VALUES('q1','http://x','/tmp','2026-09-10T00:00:00Z','queue')") + .has_value()); + + CHECK(migrate_to_head(*db).has_value()); + CHECK_EQ(db->user_version(), head); + + auto start_mode_of = [&](const char* id) -> std::string { + auto st = db->prepare("SELECT start_mode FROM tasks WHERE task_id=?1"); + if (!st || !st->bind(1, std::string_view(id))) return ""; + auto row = st->step(); + if (!row || !*row) return ""; + return std::string(st->column_text(0)); + }; + CHECK_EQ(start_mode_of("auto1"), std::string("now")); + CHECK_EQ(start_mode_of("man1"), std::string("later")); + CHECK_EQ(start_mode_of("q1"), std::string("queue")); // passes through unchanged + + // The bug this migration closes: 'later' — a real, documented StartMode value — + // used to hit the old CHECK and fail every insert. It's accepted now, and the two + // retired spellings are gone for good. + CHECK(db->exec("INSERT INTO tasks(task_id,url,save_dir,created_at,start_mode) " + "VALUES('later1','http://x','/tmp','2026-09-10T00:00:00Z','later')") + .has_value()); + CHECK(db->exec("INSERT INTO tasks(task_id,url,save_dir,created_at,start_mode) " + "VALUES('bad1','http://x','/tmp','2026-09-10T00:00:00Z','auto')") + .has_value() == false); + CHECK(db->exec("INSERT INTO tasks(task_id,url,save_dir,created_at,start_mode) " + "VALUES('bad2','http://x','/tmp','2026-09-10T00:00:00Z','manual')") + .has_value() == false); + } + // --- re-running the migrator on an at-head DB is a no-op ------------------------ { auto db = Db::open(":memory:");