daemon: fix startMode 'later' -32603 and isolate the single-instance lock by runtime dir

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-<euid>" — 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01GRDjHGgpYmMoPE2UFbe7pP
2026-09-11 17:04:02 +04:00
parent a967eca669
commit de748cc2fc
10 changed files with 270 additions and 35 deletions
@@ -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
@@ -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<socklen_t>(offsetof(sockaddr_un, sun_path) + 1 + name.size());
-
-    if (::bind(fd, reinterpret_cast<sockaddr*>(&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;

@@ -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;
@@ -0,0 +1,38 @@
+#include "rpc/single_instance.hpp"
+
+#include <cstddef>
+#include <cstring>
+
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <unistd.h>
+
+#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<socklen_t>(offsetof(sockaddr_un, sun_path) + 1 + name.size());
+
+    if (::bind(fd, reinterpret_cast<sockaddr*>(&addr), len) != 0) {
+        ::close(fd);
+        return -1;
+    }
+    return fd;
+}
+
+}  // namespace velox::daemon::rpc
@@ -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/<uid>/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-<euid>" 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 <string>
+
+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
@@ -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);
@@ -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<std::string> effective_url;
@@ -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)
@@ -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 <unistd.h>
+
+#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()
@@ -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:");