diff --git a/src/cli/cli.c b/src/cli/cli.c index 0fec3ca95d..1ebe1687e8 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -14,6 +14,7 @@ #include "cli/config_toml_edit.h" #include "ui/config.h" #include "cli/config_yaml_edit.h" +#include "cli/runtime_settings.h" #include "daemon/bootstrap.h" #include "daemon/ipc.h" #include "daemon/runtime.h" @@ -7409,6 +7410,9 @@ cbm_config_t *cbm_config_open(const char *cache_dir) { } return NULL; } + /* Another process mid-commit is waited out; failing here failed every + * settings write behind it, the daemon's UI save included. */ + cbm_runtime_settings_wait_for_writers(db); /* Create table if not exists */ const char *sql = "CREATE TABLE IF NOT EXISTS config (key TEXT PRIMARY KEY, value TEXT)"; diff --git a/src/cli/runtime_settings.c b/src/cli/runtime_settings.c index 1e3b1e6fe0..6b11101c6e 100644 --- a/src/cli/runtime_settings.c +++ b/src/cli/runtime_settings.c @@ -203,6 +203,32 @@ void cbm_runtime_settings_note_http_port(int port) { atomic_store_explicit(&active_http_port, port, memory_order_relaxed); } +/* A commit holds _config.db's lock for milliseconds, so its release is an event + * that always comes; waiting for it is not a race. Failing on the lock instead + * (SQLite's default, or startup's former 50 ms window) let one process's routine + * commit fail another: a daemon saving its UI configuration made unrelated CLI + * starts exit with "could not load saved runtime settings", and a concurrent + * commit made the daemon's own UI save fail. The bound only catches a wedged + * holder. Each busy wait is logged once (SQLite restarts the attempt count for + * every lock attempt), so a test that holds the lock can observe the waiter + * instead of guessing a duration. */ +enum { SETTINGS_BUSY_TIMEOUT_MS = 3000, SETTINGS_BUSY_STEP_MS = 10 }; + +static int settings_busy_wait(void *context, int attempts) { + (void)context; + if (attempts == 0) + cbm_log_warn("config_db.busy_wait", "action", "waiting for a settings writer"); + if ((long long)attempts * SETTINGS_BUSY_STEP_MS >= SETTINGS_BUSY_TIMEOUT_MS) + return 0; + sqlite3_sleep(SETTINGS_BUSY_STEP_MS); + return 1; +} + +void cbm_runtime_settings_wait_for_writers(sqlite3 *db) { + if (db) + sqlite3_busy_handler(db, settings_busy_wait, NULL); +} + static bool canonical_cache_path(const char *cache, char out[SETTINGS_PATH_CAP], bool create) { if (!cache || !cache[0]) return false; @@ -234,7 +260,7 @@ static sqlite3 *settings_open(const char *cache) { sqlite3_close(db); return NULL; } - sqlite3_busy_timeout(db, 3000); + cbm_runtime_settings_wait_for_writers(db); const char *schema = "BEGIN IMMEDIATE;" "CREATE TABLE IF NOT EXISTS runtime_settings(key TEXT PRIMARY KEY,value TEXT NOT NULL);" @@ -442,6 +468,65 @@ void cbm_runtime_settings_reset_for_tests(void) { } #endif +/* A process killed mid-write leaves a hot rollback journal, and a read-only + * connection cannot roll it back (SQLITE_READONLY_ROLLBACK): every later start + * failed with "could not load saved runtime settings" until something opened + * the database read-write, and nothing did. One read-write open that reads the + * schema makes SQLite roll the interrupted write back. A journal is hot only + * while no live writer holds its lock, so this never undoes a write in + * progress; a live writer takes the busy path instead. */ +static bool settings_recover_hot_journal(const char *path) { + sqlite3 *db = NULL; + bool recovered = sqlite3_open_v2(path, &db, SQLITE_OPEN_READWRITE, NULL) == SQLITE_OK; + if (recovered) { + cbm_runtime_settings_wait_for_writers(db); + recovered = + sqlite3_exec(db, "SELECT count(*) FROM sqlite_master", NULL, NULL, NULL) == SQLITE_OK; + } + if (db) + sqlite3_close(db); + char journal[SETTINGS_PATH_CAP + 16]; + (void)snprintf(journal, sizeof(journal), "%s-journal", path); + if (recovered) + cbm_log_warn("config_db.hot_journal_recovered", "journal", journal, "action", + "rolled back an interrupted settings write"); + else + /* Still fatal for the caller: without the rollback the saved settings + * cannot be read consistently, and guessing them is worse. */ + cbm_log_error("config_db.hot_journal_unrecovered", "journal", journal, "hint", + "an interrupted settings write could not be rolled back; make the " + "settings directory writable and retry"); + return recovered; +} + +/* Read-only connection holding one read snapshot (BEGIN + a schema read), or + * NULL. A writer mid-commit is waited out; a database still locked after the + * bound is an error, not an absent override. */ +static sqlite3 *settings_open_snapshot(const char *path, bool *hot_journal) { + *hot_journal = false; + sqlite3 *db = NULL; + if (sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, NULL) != SQLITE_OK) { + if (db) + sqlite3_close(db); + return NULL; + } + cbm_runtime_settings_wait_for_writers(db); + if (sqlite3_exec(db, "BEGIN; SELECT count(*) FROM sqlite_master", NULL, NULL, NULL) == + SQLITE_OK) + return db; + *hot_journal = sqlite3_extended_errcode(db) == SQLITE_READONLY_ROLLBACK; + sqlite3_close(db); + return NULL; +} + +static sqlite3 *settings_read_snapshot(const char *path) { + bool hot_journal = false; + sqlite3 *db = settings_open_snapshot(path, &hot_journal); + if (!db && hot_journal && settings_recover_hot_journal(path)) + db = settings_open_snapshot(path, &hot_journal); + return db; +} + bool cbm_runtime_settings_init(const char *cache) { char canonical[SETTINGS_PATH_CAP]; if (!canonical_cache_path(cache, canonical, true)) @@ -453,20 +538,12 @@ bool cbm_runtime_settings_init(const char *cache) { return false; sqlite3 *db = NULL; if (cbm_file_exists(path)) { - if (sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, NULL) != SQLITE_OK) { - if (db) - sqlite3_close(db); + /* The snapshot read happens before individual optional tables are + * queried; failing here exits the whole process. Hook clients skip + * this init. */ + db = settings_read_snapshot(path); + if (!db) return false; - } - /* Startup, especially hook clients, must never wait on a schema writer. - * This read forces the shared snapshot before individual optional tables - * are queried; a busy database is an error, not an absent override. */ - sqlite3_busy_timeout(db, 50); - if (sqlite3_exec(db, "BEGIN; SELECT count(*) FROM sqlite_master", NULL, NULL, NULL) != - SQLITE_OK) { - sqlite3_close(db); - return false; - } } for (size_t i = 0; i < SETTINGS_COUNT; i++) { bool saved = false; @@ -640,15 +717,18 @@ static char *error_json(const char *message, int code, int *status) { return json; } +/* An unchanged value is not rewritten: the UPDATE would still fire the revision + * trigger, so every `daemon start` on a UI build committed (journal + fsync) and + * locked out concurrent readers although nothing changed. */ static bool write_value(sqlite3 *db, const setting_t *setting, const char *value) { const char *sql; if (setting->storage == SET_CONFIG) sql = value ? "INSERT INTO config(key,value) VALUES(?,?) ON CONFLICT(key) DO UPDATE SET " - "value=excluded.value" + "value=excluded.value WHERE value IS NOT excluded.value" : "DELETE FROM config WHERE key=?"; else sql = value ? "INSERT INTO runtime_settings(key,value) VALUES(?,?) ON CONFLICT(key) DO " - "UPDATE SET value=excluded.value" + "UPDATE SET value=excluded.value WHERE value IS NOT excluded.value" : "DELETE FROM runtime_settings WHERE key=?"; sqlite3_stmt *statement = NULL; if (sqlite3_prepare_v2(db, sql, -1, &statement, NULL) != SQLITE_OK) @@ -759,13 +839,12 @@ void cbm_runtime_settings_load_ui(bool *enabled, int *port) { char path[SETTINGS_PATH_CAP]; if (snprintf(path, sizeof(path), "%s/_config.db", canonical) >= (int)sizeof(path)) return; - sqlite3 *db = NULL; - if (sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, NULL) != SQLITE_OK) { - if (db) - sqlite3_close(db); + /* Without the wait and the hot-journal recovery, a concurrent commit or an + * interrupted write made every read fail and the caller silently kept its + * defaults. */ + sqlite3 *db = settings_read_snapshot(path); + if (!db) return; - } - sqlite3_exec(db, "BEGIN", NULL, NULL, NULL); for (size_t i = 0; i < SETTINGS_COUNT; i++) if (settings[i].storage == SET_UI) { char value[SETTINGS_VALUE_CAP]; diff --git a/src/cli/runtime_settings.h b/src/cli/runtime_settings.h index 3a9eb5e34a..099a201e5a 100644 --- a/src/cli/runtime_settings.h +++ b/src/cli/runtime_settings.h @@ -25,6 +25,11 @@ bool cbm_runtime_settings_save_ui(bool enabled, int port); /* The HTTP handler supplies its actual bound port for truthful listener state. */ void cbm_runtime_settings_note_http_port(int port); +/* Every connection to _config.db waits for a committing writer instead of + * failing on its lock. Logs config_db.busy_wait once per busy wait. */ +struct sqlite3; +void cbm_runtime_settings_wait_for_writers(struct sqlite3 *db); + #ifdef CBM_ENABLE_TEST_SEAMS /* Single-threaded test teardown only; never exposed in production builds. */ void cbm_runtime_settings_reset_for_tests(void); diff --git a/src/main.c b/src/main.c index 5c2dfe82c9..54bad875eb 100644 --- a/src/main.c +++ b/src/main.c @@ -2483,7 +2483,7 @@ static void main_daemon_ctl_open_browser(int port) { * not a state a test can ask for; it is COMPILED OUT of ordinary builds (see * TEST_SEAMS in Makefile.cbm). */ static bool main_daemon_ctl_apply_ui_config(cbm_daemon_runtime_client_t *client, - uint8_t update_mask, int port) { + uint8_t update_mask, int port, uint32_t timeout_ms) { #ifdef CBM_ENABLE_TEST_SEAMS char forced[8]; if (cbm_safe_getenv("CBM_TEST_DAEMON_UI_CONFIG_REFUSED", forced, sizeof(forced), NULL) && @@ -2491,9 +2491,8 @@ static bool main_daemon_ctl_apply_ui_config(cbm_daemon_runtime_client_t *client, return false; } #endif - return cbm_daemon_application_client_set_ui_config(client, update_mask, true, port, - MAIN_CONNECT_TIMEOUT_MS) == - CBM_DAEMON_RUNTIME_APPLICATION_OK; + return cbm_daemon_application_client_set_ui_config( + client, update_mask, true, port, timeout_ms) == CBM_DAEMON_RUNTIME_APPLICATION_OK; } static int main_daemon_ctl_finish_ui_open(cbm_daemon_runtime_client_t **client_io, int port, @@ -2712,7 +2711,13 @@ static int main_run_daemon_ctl(int argc, char **argv, const cbm_daemon_ipc_endpo } /* The committed control connection satisfied the daemon's no-client - * startup window; configure the UI before departing. */ + * startup window; configure the UI before departing. + * + * Both requests wait for the daemon's reply within the startup budget. The + * connection is up, so the reply is an event that comes unless the daemon + * dies; the old 1 s bound (MAIN_CONNECT_TIMEOUT_MS, a probe budget) decided + * the command whenever saving the UI configuration (journaled commits to + * _config.db) ran long on a loaded runner. */ int ui_port = 0; bool ui_configured = false; if ((CBM_EMBEDDED_FILE_COUNT > 0)) { @@ -2722,13 +2727,14 @@ static int main_run_daemon_ctl(int argc, char **argv, const cbm_daemon_ipc_endpo uint8_t update_mask = 0x03U; /* enabled + port */ bool context_set = main_set_client_context(start_result.client, ".", CBM_MCP_TOOL_PROFILE_ALL, NULL, NULL, - MAIN_CONNECT_TIMEOUT_MS); + MAIN_DAEMON_CTL_START_TIMEOUT_MS); ui_configured = context_set && - main_daemon_ctl_apply_ui_config(start_result.client, update_mask, ui_port); + main_daemon_ctl_apply_ui_config(start_result.client, update_mask, ui_port, + MAIN_DAEMON_CTL_START_TIMEOUT_MS); if (!ui_configured) { /* Reaching here means the daemon is up: the control connection above * already satisfied its startup window. Only the UI handshake — two - * requests bounded by MAIN_CONNECT_TIMEOUT_MS — came back short. + * requests bounded by the startup budget — came back short. * * Whether that is fatal depends on what was asked for. `--port`/`--open` * make the UI the point of the command, so failing to configure it is a @@ -3371,9 +3377,13 @@ int main(int argc, char **argv) { g_daemon_client = bootstrap_result.client; + /* Startup requests on the connected client wait within the startup budget, + * like the bootstrap before them: a 1 s bound on the reply (a probe budget) + * ended the server whenever the daemon's UI save ran long, which the smoke's + * `--ui=true --port` run saw as a binary without UI assets. */ if (role == CBM_DAEMON_PROCESS_MCP_CLIENT && !main_set_client_context(g_daemon_client, NULL, tool_profile, NULL, NULL, - MAIN_CONNECT_TIMEOUT_MS)) { + bootstrap_config.startup_timeout_ms)) { (void)fprintf(stderr, "codebase-memory-mcp: daemon session context was rejected\n"); (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); g_daemon_client = NULL; @@ -3391,9 +3401,10 @@ int main(int argc, char **argv) { bool explicitly_enabled = false; uint8_t update_mask = parse_ui_flags(argc, argv, &ui_enabled, &ui_port, &explicitly_enabled); - if (update_mask != 0 && cbm_daemon_application_client_set_ui_config( - g_daemon_client, update_mask, ui_enabled, ui_port, - MAIN_CONNECT_TIMEOUT_MS) != CBM_DAEMON_RUNTIME_APPLICATION_OK) { + if (update_mask != 0 && + cbm_daemon_application_client_set_ui_config( + g_daemon_client, update_mask, ui_enabled, ui_port, + bootstrap_config.startup_timeout_ms) != CBM_DAEMON_RUNTIME_APPLICATION_OK) { (void)fprintf(stderr, "codebase-memory-mcp: daemon UI configuration update failed\n"); (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); g_daemon_client = NULL; diff --git a/tests/test_runtime_settings.c b/tests/test_runtime_settings.c index 08f477b607..28e8651d33 100644 --- a/tests/test_runtime_settings.c +++ b/tests/test_runtime_settings.c @@ -4,6 +4,7 @@ #include "foundation/compat_fs.h" #include "foundation/platform.h" #include "foundation/limits.h" +#include "foundation/log.h" #include "cli/cli.h" #include "cli/runtime_settings.h" #include "ui/config.h" @@ -397,7 +398,240 @@ TEST(runtime_settings_response_memory_returns_to_caller_baseline) { PASS(); } +/* A second connection holds _config.db's lock, as another process mid-commit + * would. The sink commits the holder the moment the waiter logs + * config_db.busy_wait, so the wait is observed as an event, single-threaded and + * without a duration. A call that fails on the lock instead never logs, so the + * holder stays held through it and the call reports the failure. Results are + * captured before any ASSERT so a failure never leaks the lock or the sink. */ +static sqlite3 *g_lock_holder; +static int g_lock_waits; +static CBMLogLevel g_lock_saved_level; + +static void release_holder_on_wait(const char *line) { + if (!g_lock_holder || !strstr(line, "config_db.busy_wait")) + return; + g_lock_waits++; + (void)sqlite3_exec(g_lock_holder, "COMMIT", NULL, NULL, NULL); +} + +static bool hold_config_lock(const char *cache, const char *lock_sql) { + char path[512]; + snprintf(path, sizeof(path), "%s/_config.db", cache); + g_lock_waits = 0; + if (sqlite3_open(path, &g_lock_holder) != SQLITE_OK || + sqlite3_exec(g_lock_holder, lock_sql, NULL, NULL, NULL) != SQLITE_OK) { + sqlite3_close(g_lock_holder); + g_lock_holder = NULL; + return false; + } + g_lock_saved_level = cbm_log_get_level(); + if (g_lock_saved_level > CBM_LOG_WARN) + cbm_log_set_level(CBM_LOG_WARN); + cbm_log_set_sink(release_holder_on_wait); + return true; +} + +static void release_config_lock(void) { + cbm_log_set_sink(NULL); + cbm_log_set_level(g_lock_saved_level); + if (g_lock_holder) { + if (!sqlite3_get_autocommit(g_lock_holder)) + (void)sqlite3_exec(g_lock_holder, "COMMIT", NULL, NULL, NULL); + sqlite3_close(g_lock_holder); + g_lock_holder = NULL; + } +} + +static long long settings_revision(const char *cache) { + yyjson_doc *snapshot = read_snapshot(cache); + const char *text = + snapshot ? yyjson_get_str(yyjson_obj_get(yyjson_doc_get_root(snapshot), "revision")) : NULL; + long long revision = text ? strtoll(text, NULL, 10) : -1; + yyjson_doc_free(snapshot); + return revision; +} + +/* A test that fails between init and fixture_close leaves the settings bound + * to its own cache; the lock tests reset first so each one fails on its own + * defect, never on a predecessor's leftover state. */ +static bool fresh_fixture_open(settings_fixture_t *fixture) { + cbm_runtime_settings_reset_for_tests(); + return fixture_open(fixture); +} + +/* CI churn: a daemon committing _config.db made an unrelated CLI start exit + * with "could not load saved runtime settings" (50 ms window). */ +TEST(runtime_settings_startup_waits_out_a_committing_writer) { + settings_fixture_t fixture; + ASSERT_TRUE(fresh_fixture_open(&fixture)); + int status = 0; + char *result = apply(fixture.path, "{\"CBM_WORKERS\":\"4\"}", &status); + ASSERT_EQ(status, 200); + cbm_free(CBM_MEM_CLASS_OTHER, result); + ASSERT_TRUE(hold_config_lock(fixture.path, "BEGIN EXCLUSIVE")); + bool initialized = cbm_runtime_settings_init(fixture.path); + int waits = g_lock_waits; + release_config_lock(); + ASSERT_TRUE(initialized); + ASSERT_EQ(waits, 1); + ASSERT_STR_EQ(cbm_runtime_getenv("CBM_WORKERS"), "4"); + fixture_close(&fixture); + PASS(); +} + +/* The daemon's set_ui_config handler: a concurrent commit failed the save at + * once (cbm_config_open had no wait) and made the load silently return the + * legacy defaults instead of the saved port. */ +TEST(runtime_settings_ui_save_and_load_wait_out_a_committing_writer) { + settings_fixture_t fixture; + ASSERT_TRUE(fresh_fixture_open(&fixture)); + ASSERT_TRUE(cbm_runtime_settings_init(fixture.path)); + cbm_ui_config_t config = {.ui_enabled = true, .ui_port = 8222}; + ASSERT_TRUE(cbm_ui_config_save(&config)); + + ASSERT_TRUE(hold_config_lock(fixture.path, "BEGIN EXCLUSIVE")); + config.ui_port = 8333; + bool saved = cbm_ui_config_save(&config); + int save_waits = g_lock_waits; + release_config_lock(); + ASSERT_TRUE(saved); + ASSERT_TRUE(save_waits >= 1); + + ASSERT_TRUE(hold_config_lock(fixture.path, "BEGIN EXCLUSIVE")); + cbm_ui_config_t loaded = {0}; + cbm_ui_config_load(&loaded); + int load_waits = g_lock_waits; + release_config_lock(); + ASSERT_EQ(load_waits, 1); + ASSERT_TRUE(loaded.ui_enabled); + ASSERT_EQ(loaded.ui_port, 8333); + fixture_close(&fixture); + PASS(); +} + +/* Every `daemon start` on a UI build saved the same two values; the upsert + * still updated the rows, fired the revision trigger and committed. */ +TEST(runtime_settings_unchanged_ui_save_does_not_rewrite) { + settings_fixture_t fixture; + ASSERT_TRUE(fresh_fixture_open(&fixture)); + ASSERT_TRUE(cbm_runtime_settings_init(fixture.path)); + cbm_ui_config_t config = {.ui_enabled = true, .ui_port = 8222}; + ASSERT_TRUE(cbm_ui_config_save(&config)); + long long first = settings_revision(fixture.path); + ASSERT_TRUE(first > 0); + ASSERT_TRUE(cbm_ui_config_save(&config)); + ASSERT_TRUE(cbm_ui_config_save(&config)); + ASSERT_EQ(settings_revision(fixture.path), first); + config.ui_port = 8223; + ASSERT_TRUE(cbm_ui_config_save(&config)); + ASSERT_EQ(settings_revision(fixture.path), first + 1); + fixture_close(&fixture); + PASS(); +} + +/* A process killed mid-write (crash, taskkill /F, power loss) leaves _config.db + * with a hot rollback journal. Made portably and deterministically: snapshot the + * database and its journal while a spilled write transaction is open, roll the + * transaction back, then put the snapshot back. The journal is checked to be + * hot (SQLite's magic and a nonzero record count) before the code under test + * runs, so these tests cannot pass on a journal that was never hot. */ +static bool copy_file(const char *from, const char *to) { + FILE *in = cbm_fopen(from, "rb"); + FILE *out = in ? cbm_fopen(to, "wb") : NULL; + bool ok = in && out; + char buffer[8192]; + size_t got = 0; + while (ok && (got = fread(buffer, 1, sizeof(buffer), in)) > 0) + ok = fwrite(buffer, 1, got, out) == got; + if (in) + fclose(in); + if (out && fclose(out) != 0) + ok = false; + return ok; +} + +static bool journal_is_hot(const char *path) { + static const unsigned char magic[8] = {0xd9, 0xd5, 0x05, 0xf9, 0x20, 0xa1, 0x63, 0xd7}; + unsigned char header[12] = {0}; + FILE *file = cbm_fopen(path, "rb"); + bool read = file && fread(header, 1, sizeof(header), file) == sizeof(header); + if (file) + fclose(file); + unsigned records = ((unsigned)header[8] << 24) | ((unsigned)header[9] << 16) | + ((unsigned)header[10] << 8) | (unsigned)header[11]; + return read && memcmp(header, magic, sizeof(magic)) == 0 && records > 0; +} + +static bool leave_hot_journal(const char *cache, const char *update_sql) { + char db_path[512], journal_path[512], db_copy[512], journal_copy[512]; + snprintf(db_path, sizeof(db_path), "%s/_config.db", cache); + snprintf(journal_path, sizeof(journal_path), "%s/_config.db-journal", cache); + snprintf(db_copy, sizeof(db_copy), "%s/snapshot.db", cache); + snprintf(journal_copy, sizeof(journal_copy), "%s/snapshot.journal", cache); + sqlite3 *writer = NULL; + bool ok = sqlite3_open(db_path, &writer) == SQLITE_OK && + sqlite3_exec(writer, "PRAGMA cache_size=1; BEGIN IMMEDIATE; CREATE TABLE spill(x BLOB)", + NULL, NULL, NULL) == SQLITE_OK && + sqlite3_exec(writer, update_sql, NULL, NULL, NULL) == SQLITE_OK; + for (int i = 0; ok && i < 64; i++) + ok = sqlite3_exec(writer, "INSERT INTO spill VALUES(randomblob(8192))", NULL, NULL, NULL) == + SQLITE_OK; + ok = ok && copy_file(db_path, db_copy) && copy_file(journal_path, journal_copy); + if (writer) { + (void)sqlite3_exec(writer, "ROLLBACK", NULL, NULL, NULL); + sqlite3_close(writer); + } + ok = ok && journal_is_hot(journal_copy) && copy_file(db_copy, db_path) && + copy_file(journal_copy, journal_path); + remove(db_copy); + remove(journal_copy); + return ok; +} + +/* The hot journal made every later start fail with "could not load saved + * runtime settings", permanently: a read-only connection cannot roll it back. */ +TEST(runtime_settings_startup_recovers_a_hot_journal) { + settings_fixture_t fixture; + ASSERT_TRUE(fresh_fixture_open(&fixture)); + int status = 0; + char *result = apply(fixture.path, "{\"CBM_WORKERS\":\"4\"}", &status); + ASSERT_EQ(status, 200); + cbm_free(CBM_MEM_CLASS_OTHER, result); + ASSERT_TRUE(leave_hot_journal(fixture.path, + "UPDATE runtime_settings SET value='9' WHERE key='CBM_WORKERS'")); + ASSERT_TRUE(cbm_runtime_settings_init(fixture.path)); + ASSERT_STR_EQ(cbm_runtime_getenv("CBM_WORKERS"), "4"); + char journal[512]; + snprintf(journal, sizeof(journal), "%s/_config.db-journal", fixture.path); + ASSERT_FALSE(cbm_file_exists(journal)); + fixture_close(&fixture); + PASS(); +} + +/* The same journal made the UI load silently fall back to the legacy defaults. */ +TEST(runtime_settings_ui_load_recovers_a_hot_journal) { + settings_fixture_t fixture; + ASSERT_TRUE(fresh_fixture_open(&fixture)); + ASSERT_TRUE(cbm_runtime_settings_init(fixture.path)); + cbm_ui_config_t config = {.ui_enabled = true, .ui_port = 8222}; + ASSERT_TRUE(cbm_ui_config_save(&config)); + ASSERT_TRUE(leave_hot_journal(fixture.path, + "UPDATE runtime_settings SET value='9999' WHERE key='ui_port'")); + cbm_ui_config_t loaded = {0}; + cbm_ui_config_load(&loaded); + ASSERT_TRUE(loaded.ui_enabled); + ASSERT_EQ(loaded.ui_port, 8222); + fixture_close(&fixture); + PASS(); +} + SUITE(runtime_settings) { + RUN_TEST(runtime_settings_startup_recovers_a_hot_journal); + RUN_TEST(runtime_settings_ui_load_recovers_a_hot_journal); + RUN_TEST(runtime_settings_startup_waits_out_a_committing_writer); + RUN_TEST(runtime_settings_ui_save_and_load_wait_out_a_committing_writer); + RUN_TEST(runtime_settings_unchanged_ui_save_does_not_rewrite); RUN_TEST(runtime_settings_response_memory_returns_to_caller_baseline); RUN_TEST(runtime_settings_preserves_unknown_inherited_values_and_reset); RUN_TEST(runtime_settings_canonical_alias_and_actual_listener); diff --git a/tests/windows/test_daemon_stability.py b/tests/windows/test_daemon_stability.py index 7bc8381821..94fa61a455 100644 --- a/tests/windows/test_daemon_stability.py +++ b/tests/windows/test_daemon_stability.py @@ -12,6 +12,8 @@ with the SAME pid (no second daemon, no restart); ``daemon status`` reports the active pid; a ``--port=N`` pointing at an occupied port must not block the start (the UI bind is retried in the background by design). +* ``daemon start --port=N`` while another process holds ``_config.db``'s + write lock waits for the daemon's UI save instead of failing the handshake. * ``daemon stop`` REFUSES while an MCP session is attached, lists the blocking client, and succeeds once the session closes. * Crash recovery: after a kill -9 the stale daemon state must clear, a cold @@ -49,6 +51,7 @@ import re import shutil import socket +import sqlite3 import subprocess import sys import tempfile @@ -247,6 +250,97 @@ def section_start_status_port(binary, work): kill_pid(daemon_pid) +def daemon_log_contains(cache, needle): + try: + with open(os.path.join(cache, "logs", "cbm-daemon.log"), "rb") as handle: + return needle in handle.read() + except OSError: + return False + + +def section_start_port_waits_for_settings_writer(binary, work): + """`daemon start --port` while another process holds _config.db's write lock. + + section_start_status_port went red on CI runners: the client gave the UI + handshake a 1 s reply budget while the daemon was still committing the UI + configuration. Holding the lock makes that slow save a state the test can + ask for. The holder is released on EVENTS: when the client exits (it gave + up under the lock), or WRITER_HOLD_S after the daemon LOGGED that it waits + for the writer. WRITER_HOLD_S outlasts the old 1 s budget and stays inside + the daemon's 3 s bound for a wedged holder. + """ + writer_hold_s = 1.5 + cache = section_dirs(work, "writer") + daemon_pid = 0 + holder = None + client = None + try: + warm = run_cli(binary, cache, ["daemon", "start"], timeout=60) + if warm.returncode != 0 or not pid_from(out_text(warm)): + raise SetupFailure("permanent daemon did not start to create _config.db " + "(rc=%s):\n%s" % (warm.returncode, out_text(warm))) + stop = run_cli(binary, cache, ["daemon", "stop"]) + if stop.returncode != 0 or not wait_status_not_running(binary, cache, 45): + print("RED: the warm-up daemon did not stop cleanly") + return False + holder = sqlite3.connect(os.path.join(cache, "_config.db"), isolation_level=None, + timeout=0) + holder.execute("BEGIN IMMEDIATE") + probe = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + probe.bind(("127.0.0.1", 0)) + port = probe.getsockname()[1] + probe.close() + client = subprocess.Popen([binary, "daemon", "start", "--port=%d" % port], + env=cli_env(cache), stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + waited_at = None + deadline = time.monotonic() + 90 + while client.poll() is None and time.monotonic() < deadline: + now = time.monotonic() + if waited_at is None and daemon_log_contains(cache, b"config_db.busy_wait"): + waited_at = now + if waited_at is not None and now - waited_at >= writer_hold_s: + break + time.sleep(0.05) + exited_under_lock = client.poll() is not None + # Release by CLOSING the holder: the OS drops a handle's locks when it + # closes, so the release cannot be refused. On Windows a COMMIT of this + # change-free transaction returned plain SQLITE_BUSY (code 5, no journal, + # client still waiting) while the daemon retried the lock: 3 of 5 full + # guard runs, on the test's own connection, never on the product's side. + holder.close() + holder = None + output, _ = client.communicate(timeout=90) + text = output.decode("utf-8", "replace") + daemon_pid = pid_from(text) + if exited_under_lock or client.returncode != 0 or "permanent" not in text: + print("RED: `daemon start --port` must wait for a settings writer, not give up " + "on the UI handshake (exited_under_lock=%s rc=%s daemon_waited=%s):\n%s" + % (exited_under_lock, client.returncode, waited_at is not None, + excerpt(text))) + return False + stop = run_cli(binary, cache, ["daemon", "stop"]) + if stop.returncode != 0 or not wait_status_not_running(binary, cache, 45): + print("RED: the daemon did not stop cleanly after the held-writer start") + return False + daemon_pid = 0 + client = None + print("PASS: `daemon start --port` waits out a held settings writer and starts") + return True + finally: + if holder is not None: + holder.close() + if client is not None: + if client.poll() is None: + client.kill() + client.wait() + # The client may have started a daemon whose pid was never read (a + # failure before its output); a survivor locked the guard root's + # binary and failed the next run's residue cleanup. + run_cli(binary, cache, ["daemon", "stop"], timeout=60) + kill_pid(daemon_pid) + + def section_stop_refuses_busy(binary, work): cache = section_dirs(work, "busy") daemon_pid = 0 @@ -328,8 +422,13 @@ def section_crash_recovery(binary, work): "(rc=%s):\n%s" % (start.returncode, out_text(start))) kill_pid(daemon_pid) if not wait_status_not_running(binary, cache, 60): - print("RED: after kill -9 of pid %d the stale daemon state never cleared " - "(`daemon status` kept reporting it)" % daemon_pid) + # Print what status said: a hot settings journal left by the kill + # made every command fail with "could not load saved runtime + # settings", which a bare "kept reporting it" hid. + last = run_cli(binary, cache, ["daemon", "status"], timeout=30) + print("RED: after kill -9 of pid %d the stale daemon state never cleared; " + "last `daemon status` rc=%s:\n%s" + % (daemon_pid, last.returncode, excerpt(out_text(last)))) return False cold = run_cli(binary, cache, ["cli", "list_projects", "{}"], timeout=90) if cold.returncode != 0 or "daemon start" in out_text(cold): @@ -455,6 +554,7 @@ def main(): section_params, section_hook_fail_open, section_start_status_port, + section_start_port_waits_for_settings_writer, section_stop_refuses_busy, section_crash_recovery, section_churn_stability,