Skip to content

Commit eb29429

Browse files
DeusDataDeusData
committed
fix(daemon): settings startup survives busy and interrupted writes
Nondeterministic Windows reds came from races on _config.db: test-windows-guards section_start_status_port / daemon_lifecycle ("the daemon did not accept the UI configuration"), section_churn_stability ("could not load saved runtime settings"), the windows-2025 release smoke whose `--ui=true --port` server exited about 5 s after start (Phase 15, read as "no embedded UI assets"), and section_crash_recovery ("the stale daemon state never cleared"). - Every _config.db connection now waits out a committing writer (cbm_runtime_settings_wait_for_writers: one busy handler, logs config_db.busy_wait once per busy wait, a 3 s bound for a wedged holder). Startup's 50 ms window let a daemon's routine commit end unrelated CLI starts; cbm_config_open and the UI load had no wait at all, so a concurrent commit failed the daemon's UI save and made the load keep defaults. - A process killed mid-write (crash, taskkill /F, power loss) left a hot rollback journal that the read-only settings reads cannot roll back (SQLITE_READONLY_ROLLBACK). Every later command then failed with "could not load saved runtime settings", permanently. Settings startup and the UI load now recover once through a read-write open that rolls the interrupted write back (config_db.hot_journal_recovered); a failed recovery is reported with the journal path and stays fatal. - write_value no longer rewrites an unchanged value. The upsert fired the revision trigger, so every `daemon start` on a UI build committed and locked readers out although nothing changed. - `daemon start` and the stdio server wait for the set_context and set_ui_config replies within their startup budget instead of the 1 s probe budget (MAIN_CONNECT_TIMEOUT_MS) that decided the command whenever the daemon's save ran long. Tests, red on 8df0639 and green here: - runtime_settings suite: three single-threaded lock tests (a log sink commits the held lock when the waiter logs config_db.busy_wait, so no test depends on a duration) and two hot-journal tests (a portable fixture snapshots the database and its journal mid-transaction and checks the journal is hot before the code under test runs). - tests/windows/test_daemon_stability.py: new section start_port_waits_for_settings_writer holds the lock against a real `daemon start --port` and releases it on events; the crash section now prints the status output it saw. Signed-off-by: Martin Vogel <[email protected]>
1 parent 8df0639 commit eb29429

6 files changed

Lines changed: 469 additions & 36 deletions

File tree

‎src/cli/cli.c‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "cli/config_toml_edit.h"
1515
#include "ui/config.h"
1616
#include "cli/config_yaml_edit.h"
17+
#include "cli/runtime_settings.h"
1718
#include "daemon/bootstrap.h"
1819
#include "daemon/ipc.h"
1920
#include "daemon/runtime.h"
@@ -7409,6 +7410,9 @@ cbm_config_t *cbm_config_open(const char *cache_dir) {
74097410
}
74107411
return NULL;
74117412
}
7413+
/* Another process mid-commit is waited out; failing here failed every
7414+
* settings write behind it, the daemon's UI save included. */
7415+
cbm_runtime_settings_wait_for_writers(db);
74127416

74137417
/* Create table if not exists */
74147418
const char *sql = "CREATE TABLE IF NOT EXISTS config (key TEXT PRIMARY KEY, value TEXT)";

‎src/cli/runtime_settings.c‎

Lines changed: 101 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,32 @@ void cbm_runtime_settings_note_http_port(int port) {
203203
atomic_store_explicit(&active_http_port, port, memory_order_relaxed);
204204
}
205205

206+
/* A commit holds _config.db's lock for milliseconds, so its release is an event
207+
* that always comes; waiting for it is not a race. Failing on the lock instead
208+
* (SQLite's default, or startup's former 50 ms window) let one process's routine
209+
* commit fail another: a daemon saving its UI configuration made unrelated CLI
210+
* starts exit with "could not load saved runtime settings", and a concurrent
211+
* commit made the daemon's own UI save fail. The bound only catches a wedged
212+
* holder. Each busy wait is logged once (SQLite restarts the attempt count for
213+
* every lock attempt), so a test that holds the lock can observe the waiter
214+
* instead of guessing a duration. */
215+
enum { SETTINGS_BUSY_TIMEOUT_MS = 3000, SETTINGS_BUSY_STEP_MS = 10 };
216+
217+
static int settings_busy_wait(void *context, int attempts) {
218+
(void)context;
219+
if (attempts == 0)
220+
cbm_log_warn("config_db.busy_wait", "action", "waiting for a settings writer");
221+
if ((long long)attempts * SETTINGS_BUSY_STEP_MS >= SETTINGS_BUSY_TIMEOUT_MS)
222+
return 0;
223+
sqlite3_sleep(SETTINGS_BUSY_STEP_MS);
224+
return 1;
225+
}
226+
227+
void cbm_runtime_settings_wait_for_writers(sqlite3 *db) {
228+
if (db)
229+
sqlite3_busy_handler(db, settings_busy_wait, NULL);
230+
}
231+
206232
static bool canonical_cache_path(const char *cache, char out[SETTINGS_PATH_CAP], bool create) {
207233
if (!cache || !cache[0])
208234
return false;
@@ -234,7 +260,7 @@ static sqlite3 *settings_open(const char *cache) {
234260
sqlite3_close(db);
235261
return NULL;
236262
}
237-
sqlite3_busy_timeout(db, 3000);
263+
cbm_runtime_settings_wait_for_writers(db);
238264
const char *schema =
239265
"BEGIN IMMEDIATE;"
240266
"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) {
442468
}
443469
#endif
444470

471+
/* A process killed mid-write leaves a hot rollback journal, and a read-only
472+
* connection cannot roll it back (SQLITE_READONLY_ROLLBACK): every later start
473+
* failed with "could not load saved runtime settings" until something opened
474+
* the database read-write, and nothing did. One read-write open that reads the
475+
* schema makes SQLite roll the interrupted write back. A journal is hot only
476+
* while no live writer holds its lock, so this never undoes a write in
477+
* progress; a live writer takes the busy path instead. */
478+
static bool settings_recover_hot_journal(const char *path) {
479+
sqlite3 *db = NULL;
480+
bool recovered = sqlite3_open_v2(path, &db, SQLITE_OPEN_READWRITE, NULL) == SQLITE_OK;
481+
if (recovered) {
482+
cbm_runtime_settings_wait_for_writers(db);
483+
recovered =
484+
sqlite3_exec(db, "SELECT count(*) FROM sqlite_master", NULL, NULL, NULL) == SQLITE_OK;
485+
}
486+
if (db)
487+
sqlite3_close(db);
488+
char journal[SETTINGS_PATH_CAP + 16];
489+
(void)snprintf(journal, sizeof(journal), "%s-journal", path);
490+
if (recovered)
491+
cbm_log_warn("config_db.hot_journal_recovered", "journal", journal, "action",
492+
"rolled back an interrupted settings write");
493+
else
494+
/* Still fatal for the caller: without the rollback the saved settings
495+
* cannot be read consistently, and guessing them is worse. */
496+
cbm_log_error("config_db.hot_journal_unrecovered", "journal", journal, "hint",
497+
"an interrupted settings write could not be rolled back; make the "
498+
"settings directory writable and retry");
499+
return recovered;
500+
}
501+
502+
/* Read-only connection holding one read snapshot (BEGIN + a schema read), or
503+
* NULL. A writer mid-commit is waited out; a database still locked after the
504+
* bound is an error, not an absent override. */
505+
static sqlite3 *settings_open_snapshot(const char *path, bool *hot_journal) {
506+
*hot_journal = false;
507+
sqlite3 *db = NULL;
508+
if (sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, NULL) != SQLITE_OK) {
509+
if (db)
510+
sqlite3_close(db);
511+
return NULL;
512+
}
513+
cbm_runtime_settings_wait_for_writers(db);
514+
if (sqlite3_exec(db, "BEGIN; SELECT count(*) FROM sqlite_master", NULL, NULL, NULL) ==
515+
SQLITE_OK)
516+
return db;
517+
*hot_journal = sqlite3_extended_errcode(db) == SQLITE_READONLY_ROLLBACK;
518+
sqlite3_close(db);
519+
return NULL;
520+
}
521+
522+
static sqlite3 *settings_read_snapshot(const char *path) {
523+
bool hot_journal = false;
524+
sqlite3 *db = settings_open_snapshot(path, &hot_journal);
525+
if (!db && hot_journal && settings_recover_hot_journal(path))
526+
db = settings_open_snapshot(path, &hot_journal);
527+
return db;
528+
}
529+
445530
bool cbm_runtime_settings_init(const char *cache) {
446531
char canonical[SETTINGS_PATH_CAP];
447532
if (!canonical_cache_path(cache, canonical, true))
@@ -453,20 +538,12 @@ bool cbm_runtime_settings_init(const char *cache) {
453538
return false;
454539
sqlite3 *db = NULL;
455540
if (cbm_file_exists(path)) {
456-
if (sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, NULL) != SQLITE_OK) {
457-
if (db)
458-
sqlite3_close(db);
541+
/* The snapshot read happens before individual optional tables are
542+
* queried; failing here exits the whole process. Hook clients skip
543+
* this init. */
544+
db = settings_read_snapshot(path);
545+
if (!db)
459546
return false;
460-
}
461-
/* Startup, especially hook clients, must never wait on a schema writer.
462-
* This read forces the shared snapshot before individual optional tables
463-
* are queried; a busy database is an error, not an absent override. */
464-
sqlite3_busy_timeout(db, 50);
465-
if (sqlite3_exec(db, "BEGIN; SELECT count(*) FROM sqlite_master", NULL, NULL, NULL) !=
466-
SQLITE_OK) {
467-
sqlite3_close(db);
468-
return false;
469-
}
470547
}
471548
for (size_t i = 0; i < SETTINGS_COUNT; i++) {
472549
bool saved = false;
@@ -640,15 +717,18 @@ static char *error_json(const char *message, int code, int *status) {
640717
return json;
641718
}
642719

720+
/* An unchanged value is not rewritten: the UPDATE would still fire the revision
721+
* trigger, so every `daemon start` on a UI build committed (journal + fsync) and
722+
* locked out concurrent readers although nothing changed. */
643723
static bool write_value(sqlite3 *db, const setting_t *setting, const char *value) {
644724
const char *sql;
645725
if (setting->storage == SET_CONFIG)
646726
sql = value ? "INSERT INTO config(key,value) VALUES(?,?) ON CONFLICT(key) DO UPDATE SET "
647-
"value=excluded.value"
727+
"value=excluded.value WHERE value IS NOT excluded.value"
648728
: "DELETE FROM config WHERE key=?";
649729
else
650730
sql = value ? "INSERT INTO runtime_settings(key,value) VALUES(?,?) ON CONFLICT(key) DO "
651-
"UPDATE SET value=excluded.value"
731+
"UPDATE SET value=excluded.value WHERE value IS NOT excluded.value"
652732
: "DELETE FROM runtime_settings WHERE key=?";
653733
sqlite3_stmt *statement = NULL;
654734
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) {
759839
char path[SETTINGS_PATH_CAP];
760840
if (snprintf(path, sizeof(path), "%s/_config.db", canonical) >= (int)sizeof(path))
761841
return;
762-
sqlite3 *db = NULL;
763-
if (sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, NULL) != SQLITE_OK) {
764-
if (db)
765-
sqlite3_close(db);
842+
/* Without the wait and the hot-journal recovery, a concurrent commit or an
843+
* interrupted write made every read fail and the caller silently kept its
844+
* defaults. */
845+
sqlite3 *db = settings_read_snapshot(path);
846+
if (!db)
766847
return;
767-
}
768-
sqlite3_exec(db, "BEGIN", NULL, NULL, NULL);
769848
for (size_t i = 0; i < SETTINGS_COUNT; i++)
770849
if (settings[i].storage == SET_UI) {
771850
char value[SETTINGS_VALUE_CAP];

‎src/cli/runtime_settings.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ bool cbm_runtime_settings_save_ui(bool enabled, int port);
2525
/* The HTTP handler supplies its actual bound port for truthful listener state. */
2626
void cbm_runtime_settings_note_http_port(int port);
2727

28+
/* Every connection to _config.db waits for a committing writer instead of
29+
* failing on its lock. Logs config_db.busy_wait once per busy wait. */
30+
struct sqlite3;
31+
void cbm_runtime_settings_wait_for_writers(struct sqlite3 *db);
32+
2833
#ifdef CBM_ENABLE_TEST_SEAMS
2934
/* Single-threaded test teardown only; never exposed in production builds. */
3035
void cbm_runtime_settings_reset_for_tests(void);

‎src/main.c‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2483,17 +2483,16 @@ static void main_daemon_ctl_open_browser(int port) {
24832483
* not a state a test can ask for; it is COMPILED OUT of ordinary builds (see
24842484
* TEST_SEAMS in Makefile.cbm). */
24852485
static bool main_daemon_ctl_apply_ui_config(cbm_daemon_runtime_client_t *client,
2486-
uint8_t update_mask, int port) {
2486+
uint8_t update_mask, int port, uint32_t timeout_ms) {
24872487
#ifdef CBM_ENABLE_TEST_SEAMS
24882488
char forced[8];
24892489
if (cbm_safe_getenv("CBM_TEST_DAEMON_UI_CONFIG_REFUSED", forced, sizeof(forced), NULL) &&
24902490
forced[0] == '1') {
24912491
return false;
24922492
}
24932493
#endif
2494-
return cbm_daemon_application_client_set_ui_config(client, update_mask, true, port,
2495-
MAIN_CONNECT_TIMEOUT_MS) ==
2496-
CBM_DAEMON_RUNTIME_APPLICATION_OK;
2494+
return cbm_daemon_application_client_set_ui_config(
2495+
client, update_mask, true, port, timeout_ms) == CBM_DAEMON_RUNTIME_APPLICATION_OK;
24972496
}
24982497

24992498
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
27122711
}
27132712

27142713
/* The committed control connection satisfied the daemon's no-client
2715-
* startup window; configure the UI before departing. */
2714+
* startup window; configure the UI before departing.
2715+
*
2716+
* Both requests wait for the daemon's reply within the startup budget. The
2717+
* connection is up, so the reply is an event that comes unless the daemon
2718+
* dies; the old 1 s bound (MAIN_CONNECT_TIMEOUT_MS, a probe budget) decided
2719+
* the command whenever saving the UI configuration (journaled commits to
2720+
* _config.db) ran long on a loaded runner. */
27162721
int ui_port = 0;
27172722
bool ui_configured = false;
27182723
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
27222727
uint8_t update_mask = 0x03U; /* enabled + port */
27232728
bool context_set =
27242729
main_set_client_context(start_result.client, ".", CBM_MCP_TOOL_PROFILE_ALL, NULL, NULL,
2725-
MAIN_CONNECT_TIMEOUT_MS);
2730+
MAIN_DAEMON_CTL_START_TIMEOUT_MS);
27262731
ui_configured = context_set &&
2727-
main_daemon_ctl_apply_ui_config(start_result.client, update_mask, ui_port);
2732+
main_daemon_ctl_apply_ui_config(start_result.client, update_mask, ui_port,
2733+
MAIN_DAEMON_CTL_START_TIMEOUT_MS);
27282734
if (!ui_configured) {
27292735
/* Reaching here means the daemon is up: the control connection above
27302736
* already satisfied its startup window. Only the UI handshake — two
2731-
* requests bounded by MAIN_CONNECT_TIMEOUT_MS — came back short.
2737+
* requests bounded by the startup budget — came back short.
27322738
*
27332739
* Whether that is fatal depends on what was asked for. `--port`/`--open`
27342740
* 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) {
33713377

33723378
g_daemon_client = bootstrap_result.client;
33733379

3380+
/* Startup requests on the connected client wait within the startup budget,
3381+
* like the bootstrap before them: a 1 s bound on the reply (a probe budget)
3382+
* ended the server whenever the daemon's UI save ran long, which the smoke's
3383+
* `--ui=true --port` run saw as a binary without UI assets. */
33743384
if (role == CBM_DAEMON_PROCESS_MCP_CLIENT &&
33753385
!main_set_client_context(g_daemon_client, NULL, tool_profile, NULL, NULL,
3376-
MAIN_CONNECT_TIMEOUT_MS)) {
3386+
bootstrap_config.startup_timeout_ms)) {
33773387
(void)fprintf(stderr, "codebase-memory-mcp: daemon session context was rejected\n");
33783388
(void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS);
33793389
g_daemon_client = NULL;
@@ -3391,9 +3401,10 @@ int main(int argc, char **argv) {
33913401
bool explicitly_enabled = false;
33923402
uint8_t update_mask =
33933403
parse_ui_flags(argc, argv, &ui_enabled, &ui_port, &explicitly_enabled);
3394-
if (update_mask != 0 && cbm_daemon_application_client_set_ui_config(
3395-
g_daemon_client, update_mask, ui_enabled, ui_port,
3396-
MAIN_CONNECT_TIMEOUT_MS) != CBM_DAEMON_RUNTIME_APPLICATION_OK) {
3404+
if (update_mask != 0 &&
3405+
cbm_daemon_application_client_set_ui_config(
3406+
g_daemon_client, update_mask, ui_enabled, ui_port,
3407+
bootstrap_config.startup_timeout_ms) != CBM_DAEMON_RUNTIME_APPLICATION_OK) {
33973408
(void)fprintf(stderr, "codebase-memory-mcp: daemon UI configuration update failed\n");
33983409
(void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS);
33993410
g_daemon_client = NULL;

0 commit comments

Comments
 (0)