sql: avoid trying to do parallel refreshes.
What the developer wrote
Authored by Rusty Russell
sql: avoid trying to do parallel refreshes.
Simply wait if there's one going already. This is a minor
optimization, but critical for the case where we do partial refreshes
asynchonously (rather than deleting everything and reloading). This
is currently only coinmoves and chainmoves, but the duplicated effort
is a waste everywhere.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This change fixes a bug in Core Lightning's SQL plugin where running the same table refresh twice at the same time could cause database errors (duplicate rows) or wasted work. The fix makes later refresh requests wait until the current one finishes, then continue. The commit message calls it a 'minor optimization' but the test shows it previously caused real SQL errors. It is not obviously exploitable by an external attacker, but it could affect reliability or data consistency under concurrent use.
Treat as a reliability/integrity fix worth including in a maintenance release. Review whether the parallel-refresh race could be triggered by untrusted RPC callers or plugin users, and consider whether command_still_pending waiters are correctly cleaned up if the waiting command is cancelled. No immediate emergency response is indicated.
Security signals we found
Race condition in asynchronous table refresh logic
SQL UNIQUE constraint failure under parallel refreshes
Potential duplicate/ inconsistent mirrored SQL data
Fix removes xfail from parallel-refresh test
No input validation or authentication changes
Evidence from the diff
The SQL plugin maintains mirrored SQL tables populated by refreshing data from RPC sources. When multiple commands requested the same table refresh concurrently, parallel refreshes could race: both would try to INSERT the same rows, hitting UNIQUE constraint failures on created_index (observed for chainmoves and coinmoves). The patch serializes refreshes per table by adding a ‘refreshing’ flag and a waiters list to struct table_desc. If a refresh is already in progress for a table, the new command is parked on refresh_waiters and returns command_still_pending. When the active refresh completes, one_refresh_done() dequeues waiters, removes the now-current table from their dbq->tables list, and resumes refresh_tables() for the remaining tables. A previously xfail test, test_sql_parallel, is re-enabled.
Changed components
plugins/sql.ctests/test_plugin.pySQL plugin table refresh machinerychainmoves and coinmoves table mirrorsInspect captured patch +49 / −3
diff --git a/plugins/sql.c b/plugins/sql.c
index 971bbb55..7b7e21bf 100644
--- a/plugins/sql.c
+++ b/plugins/sql.c
@@ -99,6 +99,13 @@ struct db_query {
u64 *last_created_index;
};
+/* Waiting for another command to refresh table */
+struct refresh_waiter {
+ struct list_node list;
+ struct command *cmd;
+ struct db_query *dbq;
+};
+
struct table_desc {
/* e.g. listpeers. For sub-tables, the raw name without
* parent prepended */
@@ -121,6 +128,10 @@ struct table_desc {
struct db_query *dbq);
/* some refresh functions maintain changed and created indexes */
u64 last_created_index;
+ /* Are we refreshing now? */
+ bool refreshing;
+ /* Any other commands waiting for the refresh completion */
+ struct list_head refresh_waiters;
};
static STRMAP(struct table_desc *) tablemap;
static size_t max_dbmem = 500000000;
@@ -482,6 +493,29 @@ static struct command_result *refresh_tables(struct command *cmd,
static struct command_result *one_refresh_done(struct command *cmd,
struct db_query *dbq)
{
+ struct table_desc *td = dbq->tables[0];
+ struct list_head waiters;
+ struct refresh_waiter *rw;
+
+ /* We are no longer refreshing */
+ assert(td->refreshing);
+ td->refreshing = false;
+
+ /* Transfer refresh waiters onto local list */
+ list_head_init(&waiters);
+ list_append_list(&waiters, &td->refresh_waiters);
+
+ while ((rw = list_pop(&waiters, struct refresh_waiter, list)) != NULL) {
+ struct command *rwcmd = rw->cmd;
+ struct db_query *rwdbq = rw->dbq;
+ tal_free(rw);
+
+ /* Remove that one, and refresh the rest */
+ assert(rwdbq->tables[0] == td);
+ tal_arr_remove(&rwdbq->tables, 0);
+ refresh_tables(rwcmd, rwdbq);
+ }
+
/* Remove that, iterate */
tal_arr_remove(&dbq->tables, 0);
return refresh_tables(cmd, dbq);
@@ -1069,9 +1103,9 @@ static struct command_result *nodes_refresh(struct command *cmd,
}
static struct command_result *refresh_tables(struct command *cmd,
- struct db_query *dbq)
+ struct db_query *dbq)
{
- const struct table_desc *td;
+ struct table_desc *td;
if (tal_count(dbq->tables) == 0)
return refresh_complete(cmd, dbq);
@@ -1079,7 +1113,18 @@ static struct command_result *refresh_tables(struct command *cmd,
/* td is const, but last_created_index needs updating, so we hand
* pointer in dbq. */
td = dbq->tables[0];
+
+ /* If it's currently being refreshed, wait */
+ if (td->refreshing) {
+ struct refresh_waiter *rw = tal(cmd, struct refresh_waiter);
+ rw->cmd = cmd;
+ rw->dbq = dbq;
+ list_add(&td->refresh_waiters, &rw->list);
+ return command_still_pending(cmd);
+ }
+
dbq->last_created_index = &dbq->tables[0]->last_created_index;
+ td->refreshing = true;
return td->refresh(cmd, dbq->tables[0], dbq);
}
@@ -1477,6 +1522,8 @@ static struct table_desc *new_table_desc(const tal_t *ctx,
td->columns = tal_arr(td, struct column *, 0);
td->last_created_index = 0;
td->has_created_index = false;
+ td->refreshing = false;
+ list_head_init(&td->refresh_waiters);
/* Only top-levels have refresh functions */
if (!parent) {
diff --git a/tests/test_plugin.py b/tests/test_plugin.py
index d84c645c..4913d350 100644
--- a/tests/test_plugin.py
+++ b/tests/test_plugin.py
@@ -4258,7 +4258,6 @@ def test_sql_crash(node_factory, bitcoind):
l1.rpc.sql(f"SELECT * FROM peerchannels;")
-@pytest.mark.xfail(strict=True)
def test_sql_parallel(node_factory, executor):
"""Parallel refreshes of tables causes SQL errors:
Error executing INSERT INTO chainmoves VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?); on row 0: UNIQUE constraint failed: chainmoves.created_index
Why this scored 43/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.