From 318e54168ae322538f50938e007a68b474b8dfbd Mon Sep 17 00:00:00 2001 From: Andrea Donetti Date: Fri, 24 Apr 2026 12:11:41 -0600 Subject: [PATCH 1/2] fix: actionable error when API called before cloudsync_init Six functions produced misleading errors when called before any cloudsync_init(''): cloudsync_changes -> "out of memory (7)" cloudsync_db_version -> "Unable to retrieve db_version (not an error)" cloudsync_db_version_next -> same pattern (SQLite) / silent -1 (PG) cloudsync_set_filter -> 10+ "no such table" NOTICEs + generic trigger error cloudsync_clear_filter -> same as set_filter cloudsync_payload_apply -> 3x "no such table: cloudsync_settings" debug lines followed by "Runtime error: not an error" Add cloudsync_context_is_initialized() helper and guard each function on its error branch. When the root cause is missing init, raise a single message pointing at SELECT cloudsync_init(''). The guard is a NULL-pointer check on the error branch only, so the sync hot path (cloudsync_db_version_next stepped by merge triggers on every received row) is unaffected. cloudsync_changes on PostgreSQL was already graceful (empty result set), so no fix was needed there. Bumps to 1.0.17. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 6 +++ src/cloudsync.c | 23 +++++++++++ src/cloudsync.h | 3 +- src/postgresql/cloudsync_postgresql.c | 39 +++++++++++++++++++ src/sqlite/cloudsync_changes_sqlite.c | 18 ++++++++- src/sqlite/cloudsync_sqlite.c | 55 +++++++++++++++++++++++---- 6 files changed, 133 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b96bc16..b9fb7d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). +## [1.0.17] - 2026-04-24 + +### Fixed + +- **Confusing errors when `cloudsync_init` was never called**: `cloudsync_changes` (SQLite), `cloudsync_db_version`, `cloudsync_db_version_next`, `cloudsync_set_filter`, `cloudsync_clear_filter`, and `cloudsync_payload_apply` now raise a single actionable message pointing at `SELECT cloudsync_init('')` instead of leaking low-level symptoms (`out of memory`, `not an error`, silent `-1`, multi-line "no such table" dumps). The guard runs only on the error branch, so the sync hot path is unaffected. + ## [1.0.16] - 2026-04-16 ### Fixed diff --git a/src/cloudsync.c b/src/cloudsync.c index d23c29d..908e9c1 100644 --- a/src/cloudsync.c +++ b/src/cloudsync.c @@ -2341,6 +2341,16 @@ bool cloudsync_config_exists (cloudsync_context *data) { return database_internal_table_exists(data, CLOUDSYNC_SITEID_NAME) == true; } +bool cloudsync_context_is_initialized (cloudsync_context *data) { + // A fully initialized context has its persistent "is the DB stale" probe + // prepared. cloudsync_context_init prepares data_version_stmt (via + // cloudsync_add_dbvms) only after the cloudsync_site_id table exists, so + // a non-NULL pointer means cloudsync_init has been called at least once + // on this connection. Used to produce actionable error messages when + // callers hit a function before calling cloudsync_init. + return data != NULL && data->data_version_stmt != NULL; +} + cloudsync_context *cloudsync_context_create (void *db) { cloudsync_context *data = (cloudsync_context *)cloudsync_memory_zeroalloc((uint64_t)(sizeof(cloudsync_context))); if (!data) return NULL; @@ -3201,6 +3211,19 @@ static int cloudsync_payload_decode_callback (void *xdata, int index, int type, // #ifndef CLOUDSYNC_OMIT_RLS_VALIDATION int cloudsync_payload_apply (cloudsync_context *data, const char *payload, int blen, int *pnrows) { + // Guard against calling payload_apply before cloudsync_init: without this, + // the settings lookups at the top of this function would each emit a + // "no such table: cloudsync_settings" debug line, control would fall + // through to the meta-table insert, and the function would ultimately + // return an error with an empty errmsg — SQLite then surfaces that as + // the confusing "Runtime error: not an error". + if (!cloudsync_context_is_initialized(data)) { + return cloudsync_set_error(data, + "cloudsync is not initialized: call SELECT cloudsync_init('') " + "to enable sync on a table before calling cloudsync_payload_apply().", + DBRES_MISUSE); + } + // sanity check if (blen < (int)sizeof(cloudsync_payload_header)) return cloudsync_set_error(data, "Error on cloudsync_payload_apply: invalid payload length", DBRES_MISUSE); diff --git a/src/cloudsync.h b/src/cloudsync.h index 1bf0b53..d1641ed 100644 --- a/src/cloudsync.h +++ b/src/cloudsync.h @@ -18,7 +18,7 @@ extern "C" { #endif -#define CLOUDSYNC_VERSION "1.0.16" +#define CLOUDSYNC_VERSION "1.0.17" #define CLOUDSYNC_MAX_TABLENAME_LEN 512 #define CLOUDSYNC_VALUE_NOTSET -1 @@ -64,6 +64,7 @@ int64_t cloudsync_dbversion (cloudsync_context *data); void cloudsync_update_schema_hash (cloudsync_context *data); int cloudsync_dbversion_check_uptodate (cloudsync_context *data); bool cloudsync_config_exists (cloudsync_context *data); +bool cloudsync_context_is_initialized (cloudsync_context *data); dbvm_t *cloudsync_colvalue_stmt (cloudsync_context *data, const char *tbl_name, bool *persistent); // CloudSync alter table diff --git a/src/postgresql/cloudsync_postgresql.c b/src/postgresql/cloudsync_postgresql.c index 1939a09..4d0ed6a 100644 --- a/src/postgresql/cloudsync_postgresql.c +++ b/src/postgresql/cloudsync_postgresql.c @@ -218,6 +218,15 @@ Datum cloudsync_db_version (PG_FUNCTION_ARGS) { { int rc = cloudsync_dbversion_check_uptodate(data); if (rc != DBRES_OK) { + // When cloudsync_init was never called, data_version_stmt is NULL + // and database_errmsg() is empty, producing the unhelpful "Unable + // to retrieve db_version ()". Detect the uninitialized state and + // return an actionable message instead. The extra check only runs + // on the error branch, so it costs nothing on the sync hot path. + if (!cloudsync_context_is_initialized(data)) { + ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cloudsync is not initialized: call SELECT cloudsync_init('') to enable sync on a table before calling cloudsync_db_version()."))); + } ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("Unable to retrieve db_version (%s)", database_errmsg(data)))); } @@ -256,6 +265,18 @@ Datum cloudsync_db_version_next (PG_FUNCTION_ARGS) { PG_TRY(); { next_version = cloudsync_dbversion_next(data, merging_version); + if (next_version == -1) { + // Previously this path silently returned -1, which is worse than + // an error because callers cannot distinguish a bogus version + // number from a real one. Emit an actionable message when the + // root cause is that cloudsync_init has never been called. + if (!cloudsync_context_is_initialized(data)) { + ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cloudsync is not initialized: call SELECT cloudsync_init('') to enable sync on a table before calling cloudsync_db_version_next()."))); + } + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), + errmsg("Unable to retrieve next_db_version (%s)", database_errmsg(data)))); + } } PG_CATCH(); { @@ -670,6 +691,16 @@ Datum cloudsync_set_filter (PG_FUNCTION_ARGS) { PG_TRY(); { + // Guard against calling set_filter before the target table has been + // set up for sync: without this, we'd drop and fail to recreate + // triggers, emitting ten+ noisy "does not exist, skipping" NOTICEs + // followed by a generic "error recreating triggers" message that + // does not point at the real cause. + if (!cloudsync_context_is_initialized(data) || !table_lookup(data, tbl)) { + ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cloudsync_set_filter: table '%s' is not configured for sync. Call SELECT cloudsync_init('%s') first.", tbl, tbl))); + } + // Store filter in table settings dbutils_table_settings_set_key_value(data, tbl, "*", "filter", filter_expr); @@ -735,6 +766,14 @@ Datum cloudsync_clear_filter (PG_FUNCTION_ARGS) { PG_TRY(); { + // Guard against calling clear_filter before the target table has + // been set up for sync — see cloudsync_set_filter for the same + // rationale. + if (!cloudsync_context_is_initialized(data) || !table_lookup(data, tbl)) { + ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cloudsync_clear_filter: table '%s' is not configured for sync. Call SELECT cloudsync_init('%s') first.", tbl, tbl))); + } + // Remove filter from settings dbutils_table_settings_set_key_value(data, tbl, "*", "filter", NULL); diff --git a/src/sqlite/cloudsync_changes_sqlite.c b/src/sqlite/cloudsync_changes_sqlite.c index b6679d6..58a9b88 100644 --- a/src/sqlite/cloudsync_changes_sqlite.c +++ b/src/sqlite/cloudsync_changes_sqlite.c @@ -395,12 +395,26 @@ int cloudsync_changesvtab_best_index (sqlite3_vtab *vtab, sqlite3_index_info *id int cloudsync_changesvtab_filter (sqlite3_vtab_cursor *cursor, int idxn, const char *idxs, int argc, sqlite3_value **argv) { DEBUG_VTAB("cloudsync_changesvtab_filter"); - + cloudsync_changes_cursor *c = (cloudsync_changes_cursor *)cursor; cloudsync_context *data = c->vtab->data; sqlite3 *db = c->vtab->db; char *sql = vtab_build_changes_sql(data, idxs); - if (sql == NULL) return SQLITE_NOMEM; + if (sql == NULL) { + // vtab_build_changes_sql returns NULL when no *_cloudsync meta-tables + // exist (cloudsync_init was never called, or the last configured table + // was cleaned up): the inner GROUP_CONCAT produces a NULL row and the + // outer SELECT yields a NULL final string. Distinguish this from a + // genuine OOM by checking whether cloudsync is configured, so the user + // gets an actionable message instead of "out of memory". + if (!cloudsync_config_exists(data) || dbutils_table_settings_count_tables(data) == 0) { + return vtab_set_error((sqlite3_vtab *)c->vtab, + "cloudsync has no tables configured for sync. Call " + "SELECT cloudsync_init('') to enable sync on a " + "table before querying cloudsync_changes."); + } + return SQLITE_NOMEM; + } // the xFilter method may be called multiple times on the same sqlite3_vtab_cursor* if (c->vm) sqlite3_finalize(c->vm); diff --git a/src/sqlite/cloudsync_sqlite.c b/src/sqlite/cloudsync_sqlite.c index 86c1e83..bdff56b 100644 --- a/src/sqlite/cloudsync_sqlite.c +++ b/src/sqlite/cloudsync_sqlite.c @@ -77,32 +77,51 @@ void dbsync_db_version (sqlite3_context *context, int argc, sqlite3_value **argv DEBUG_FUNCTION("cloudsync_db_version"); UNUSED_PARAMETER(argc); UNUSED_PARAMETER(argv); - + // retrieve context cloudsync_context *data = (cloudsync_context *)sqlite3_user_data(context); - + int rc = cloudsync_dbversion_check_uptodate(data); if (rc != SQLITE_OK) { - dbsync_set_error(context, "Unable to retrieve db_version (%s).", database_errmsg(data)); + // When cloudsync_init was never called, data_version_stmt is NULL and + // database_errmsg() falls back to "not an error", producing the + // confusing "Unable to retrieve db_version (not an error)". Detect the + // uninitialized state and return an actionable message instead. The + // extra check only runs on the error branch, so it costs nothing on + // the sync hot path (merge operations keep going through the normal + // path where rc == SQLITE_OK). + if (!cloudsync_context_is_initialized(data)) { + dbsync_set_error(context, + "cloudsync is not initialized: call SELECT cloudsync_init('') " + "to enable sync on a table before calling cloudsync_db_version()."); + } else { + dbsync_set_error(context, "Unable to retrieve db_version (%s).", database_errmsg(data)); + } return; } - + sqlite3_result_int64(context, cloudsync_dbversion(data)); } void dbsync_db_version_next (sqlite3_context *context, int argc, sqlite3_value **argv) { DEBUG_FUNCTION("cloudsync_db_version_next"); - + // retrieve context cloudsync_context *data = (cloudsync_context *)sqlite3_user_data(context); - + sqlite3_int64 merging_version = (argc == 1) ? database_value_int(argv[0]) : CLOUDSYNC_VALUE_NOTSET; sqlite3_int64 value = cloudsync_dbversion_next(data, merging_version); if (value == -1) { - dbsync_set_error(context, "Unable to retrieve next_db_version (%s).", database_errmsg(data)); + if (!cloudsync_context_is_initialized(data)) { + dbsync_set_error(context, + "cloudsync is not initialized: call SELECT cloudsync_init('') " + "to enable sync on a table before calling cloudsync_db_version_next()."); + } else { + dbsync_set_error(context, "Unable to retrieve next_db_version (%s).", database_errmsg(data)); + } return; } - + sqlite3_result_int64(context, value); } @@ -1243,6 +1262,17 @@ void dbsync_set_filter (sqlite3_context *context, int argc, sqlite3_value **argv cloudsync_context *data = (cloudsync_context *)sqlite3_user_data(context); + // Guard against calling set_filter before the target table has been set + // up for sync: without this, we'd hit "no such table: + // cloudsync_table_settings" or "no such table: main." deep inside + // the trigger recreation path, which is not actionable. + if (!cloudsync_context_is_initialized(data) || !table_lookup(data, tbl)) { + dbsync_set_error(context, + "cloudsync_set_filter: table '%s' is not configured for sync. " + "Call SELECT cloudsync_init('%s') first.", tbl, tbl); + return; + } + // Store filter in table settings dbutils_table_settings_set_key_value(data, tbl, "*", "filter", filter_expr); @@ -1281,6 +1311,15 @@ void dbsync_clear_filter (sqlite3_context *context, int argc, sqlite3_value **ar cloudsync_context *data = (cloudsync_context *)sqlite3_user_data(context); + // Guard against calling clear_filter before the target table has been set + // up for sync — see dbsync_set_filter for the same rationale. + if (!cloudsync_context_is_initialized(data) || !table_lookup(data, tbl)) { + dbsync_set_error(context, + "cloudsync_clear_filter: table '%s' is not configured for sync. " + "Call SELECT cloudsync_init('%s') first.", tbl, tbl); + return; + } + // Remove filter from table settings (set to NULL/empty) dbutils_table_settings_set_key_value(data, tbl, "*", "filter", NULL); From ffa3c95fb9a6ee7c321ed4997847047508eb9701 Mon Sep 17 00:00:00 2001 From: Andrea Donetti Date: Fri, 24 Apr 2026 13:34:26 -0600 Subject: [PATCH 2/2] test: regression test for uninit error messages Asserts that every function guarded by the previous commit points the caller at cloudsync_init when called before any table has been set up for sync. Matches on the stable substring "cloudsync_init" rather than the full sentence, so future rewordings of the user-facing message do not break the test. Covers cloudsync_changes, cloudsync_db_version, cloudsync_db_version_next, cloudsync_set_filter, cloudsync_clear_filter, and cloudsync_payload_apply. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/unit.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/test/unit.c b/test/unit.c index 30c4f69..05e9c95 100644 --- a/test/unit.c +++ b/test/unit.c @@ -2493,6 +2493,76 @@ static int deny_sqlite_master_authorizer(void *pUserData, int action, const char return SQLITE_OK; } +// Run a SQL statement expected to fail, and verify that its error message +// contains the stable substring "cloudsync_init" — which every uninitialized +// guard added by the fix points the caller at. Matching a single stable token +// rather than full text tolerates future rewordings of the user-facing string. +static bool expect_uninit_error (sqlite3 *db, const char *sql) { + sqlite3_stmt *stmt = NULL; + int rc = sqlite3_prepare_v2(db, sql, -1, &stmt, NULL); + if (rc != SQLITE_OK) { + // Prepare-time failures also land in sqlite3_errmsg — accept those. + const char *m = sqlite3_errmsg(db); + bool ok = (m != NULL && strstr(m, "cloudsync_init") != NULL); + if (stmt) sqlite3_finalize(stmt); + return ok; + } + rc = sqlite3_step(stmt); + bool failed = (rc != SQLITE_ROW && rc != SQLITE_DONE); + const char *msg = sqlite3_errmsg(db); + bool has_hint = (msg != NULL && strstr(msg, "cloudsync_init") != NULL); + sqlite3_finalize(stmt); + return failed && has_hint; +} + +// Regression test for the "uninit error messages" fix. Every function that +// previously leaked a misleading low-level symptom (out of memory, "not an +// error", silent -1, multi-line "no such table" dumps) must now point the +// caller at SELECT cloudsync_init(...). Match on a stable substring rather +// than full text so the test tolerates future rewordings. +bool do_test_uninit_error_messages (void) { + sqlite3 *db = NULL; + bool result = false; + + if (sqlite3_open(":memory:", &db) != SQLITE_OK) return false; + if (sqlite3_cloudsync_init(db, NULL, NULL) != SQLITE_OK) goto cleanup; + + // 96 bytes of zero padding — bigger than cloudsync_payload_header, so the + // size sanity check in dbsync_payload_decode passes and control reaches + // our guard inside cloudsync_payload_apply. + const char *payload_sql = + "SELECT cloudsync_payload_apply(zeroblob(96));"; + + if (!expect_uninit_error(db, "SELECT * FROM cloudsync_changes;")) goto cleanup; + if (!expect_uninit_error(db, "SELECT cloudsync_db_version();")) goto cleanup; + if (!expect_uninit_error(db, "SELECT cloudsync_db_version_next();")) goto cleanup; + if (!expect_uninit_error(db, "SELECT cloudsync_set_filter('foo','1=1');")) goto cleanup; + if (!expect_uninit_error(db, "SELECT cloudsync_clear_filter('foo');")) goto cleanup; + if (!expect_uninit_error(db, payload_sql)) goto cleanup; + + // Happy path: after cloudsync_init the same functions no longer fail with + // the uninitialized hint. cloudsync_db_version must now return a value. + if (sqlite3_exec(db, + "CREATE TABLE t (id TEXT PRIMARY KEY NOT NULL, v TEXT);" + "SELECT cloudsync_init('t');", + NULL, NULL, NULL) != SQLITE_OK) goto cleanup; + + sqlite3_stmt *stmt = NULL; + if (sqlite3_prepare_v2(db, "SELECT cloudsync_db_version();", -1, &stmt, NULL) != SQLITE_OK) goto cleanup; + int step_rc = sqlite3_step(stmt); + sqlite3_finalize(stmt); + if (step_rc != SQLITE_ROW) goto cleanup; + + result = true; + +cleanup: + if (db) { + sqlite3_exec(db, "SELECT cloudsync_terminate();", NULL, NULL, NULL); + sqlite3_close(db); + } + return result; +} + // Verify that cloudsync_dbversion_rebuild surfaces a real failure from // database_select_text(SQL_DBVERSION_BUILD_QUERY, ...) instead of silently // treating it as "no *_cloudsync meta-tables present" — which would leave @@ -12425,6 +12495,7 @@ int main (int argc, const char * argv[]) { result += test_report("Stale Table Settings:", do_test_stale_table_settings(cleanup_databases)); result += test_report("Stale Table Settings Dropped Meta:", do_test_stale_table_settings_dropped_meta(cleanup_databases)); result += test_report("DBVersion Rebuild Error:", do_test_dbversion_rebuild_error()); + result += test_report("Uninit Error Messages:", do_test_uninit_error_messages()); result += test_report("Block LWW Existing Data:", do_test_block_lww_existing_data(cleanup_databases)); result += test_report("Block Column Reload:", do_test_block_column_reload(cleanup_databases)); result += test_report("CB Error Cleanup:", do_test_context_cb_error_cleanup());