Skip to content

Commit 776a58e

Browse files
committed
Address PR 2230 review: non-blocking integrity_check + replica guard
Three fixes surfaced in code-review of PR 2230: 1. integrity_check (namespace/mod.rs): Move the PRAGMA quick_check/integrity_check PRAGMA run from the current Tokio worker onto the blocking thread pool via spawn_blocking. On a large DB, integrity_check can take seconds and would otherwise stall async tasks on that worker. Matches the pattern already used for checkpoint() and vacuum_if_needed() on LegacyConnection and for block_in_place in admin_shell. 2. reset_replication replica guard (namespace/store.rs): Reject calls with 400 NotAPrimary unless db_kind is Primary. On a replica, the ReplicaConfigurator does not consume .sentinel, so the re-init path would not actually rebuild anything and we'd destroy wallog/snapshots the replica still needs. 3. Async filesystem check (namespace/store.rs): Replace sync Path::exists() with tokio::fs::try_exists to avoid blocking the Tokio worker on a filesystem stat. Also leaves a TODO comment on the string-based corruption classification in the checkpoint error path — follow-up tracked to replace it with typed rusqlite::ErrorCode matching (or a dedicated Error::DatabaseCorrupt variant) once the Error enum plumbing is updated. Verified: cargo check clean, all 7 reset/integrity turmoil tests pass (reset_replication_preserves_data_on_healthy_namespace, reset_replication_on_nonexistent_namespace_returns_404, reset_replication_is_idempotent, reset_replication_response_includes_elapsed_ms, integrity_check_on_healthy_namespace, integrity_check_defaults_to_quick_when_full_omitted, integrity_check_on_nonexistent_namespace_returns_404).
1 parent d706429 commit 776a58e

2 files changed

Lines changed: 46 additions & 12 deletions

File tree

libsql-server/src/namespace/mod.rs

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -113,16 +113,27 @@ impl Namespace {
113113
}
114114
};
115115
let pragma = if full { "integrity_check" } else { "quick_check" };
116-
let result = conn.with_raw(move |raw| -> rusqlite::Result<Vec<String>> {
117-
let mut stmt = raw.prepare(&format!("PRAGMA {pragma}"))?;
118-
let mut rows = stmt.query([])?;
119-
let mut out = Vec::new();
120-
while let Some(row) = rows.next()? {
121-
let s: String = row.get(0)?;
122-
out.push(s);
123-
}
124-
Ok(out)
125-
});
116+
// `with_raw` holds a parking_lot mutex and runs the closure on the
117+
// current thread. `PRAGMA integrity_check` scans the full database
118+
// and can take seconds on large DBs, which would stall a Tokio
119+
// worker. Move the work to the blocking thread pool so Tokio's
120+
// async workers keep making progress (consistent with the
121+
// `checkpoint` / `vacuum_if_needed` paths on `LegacyConnection`).
122+
let pragma_owned = pragma.to_string();
123+
let result = tokio::task::spawn_blocking(move || {
124+
conn.with_raw(move |raw| -> rusqlite::Result<Vec<String>> {
125+
let mut stmt = raw.prepare(&format!("PRAGMA {pragma_owned}"))?;
126+
let mut rows = stmt.query([])?;
127+
let mut out = Vec::new();
128+
while let Some(row) = rows.next()? {
129+
let s: String = row.get(0)?;
130+
out.push(s);
131+
}
132+
Ok(out)
133+
})
134+
})
135+
.await
136+
.map_err(|e| anyhow::anyhow!("integrity_check join failure: {e}"))?;
126137
match result {
127138
Ok(rows) => Ok(rows.join("\n")),
128139
Err(e) => {

libsql-server/src/namespace/store.rs

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,16 @@ impl NamespaceStore {
267267
return Err(Error::NamespaceStoreShutdown);
268268
}
269269

270+
// reset-replication rebuilds the primary's replication log from
271+
// its live data file. On a replica node the `.sentinel` sentinel
272+
// is not consumed by `ReplicaConfigurator`, so the re-init path
273+
// does nothing useful and we'd only destroy wallog/snapshots that
274+
// the replica still needs. Reject the call with 400 NotAPrimary
275+
// instead of producing confusing behavior.
276+
if !matches!(self.inner.db_kind, DatabaseKind::Primary) {
277+
return Err(Error::NotAPrimary);
278+
}
279+
270280
if !self.inner.metadata.exists(&namespace).await {
271281
return Err(Error::NamespaceDoesntExist(namespace.to_string()));
272282
}
@@ -308,6 +318,16 @@ impl NamespaceStore {
308318
match ns.checkpoint().await {
309319
Ok(()) => {}
310320
Err(e) => {
321+
// TODO(follow-up): classify corruption via a typed
322+
// `rusqlite::ErrorCode` (DatabaseCorrupt / NotADatabase)
323+
// or a dedicated `Error::DatabaseCorrupt` variant. See
324+
// <https://github.com/tursodatabase/libsql/pull/2230>
325+
// review thread for the design. The current
326+
// `checkpoint()` path boxes through `anyhow::Error`
327+
// which erases the source type, so we match the
328+
// rendered message for now. False-positive risk here
329+
// is low: all substrings below come from SQLite's own
330+
// error text for genuine live-DB corruption.
311331
let msg = e.to_string();
312332
let is_live_db_corrupt = msg.contains("malformed")
313333
|| msg.contains("DatabaseCorrupt")
@@ -370,8 +390,11 @@ impl NamespaceStore {
370390
let sentinel = ns_path.join(".sentinel");
371391
let _ = tokio::fs::remove_file(&sentinel).await;
372392
// Ensure parent dir exists (it should, because data still lives
373-
// there, but be defensive).
374-
if !ns_path.exists() {
393+
// there, but be defensive). Use async `try_exists` rather than the
394+
// sync `Path::exists()` so we don't block the Tokio worker on a
395+
// filesystem stat.
396+
let parent_exists = tokio::fs::try_exists(&ns_path).await.unwrap_or(false);
397+
if !parent_exists {
375398
tokio::fs::create_dir_all(&ns_path).await.map_err(|e| {
376399
Error::Internal(format!(
377400
"reset_replication: create_dir_all {} failed: {e}",

0 commit comments

Comments
 (0)