Skip to content

Commit 900e2ea

Browse files
libsql-server: make namespace creation all-or-nothing
`NamespaceStore::create` persisted the namespace config before setting the namespace up, and nothing undid it: a dump that failed to import left a "ghost" namespace (config without data) whose name could not be reused and which the next user request lazily turned into an empty database. A request cancelled mid-import ran no cleanup at all, since the error branch of the future is never reached when it is dropped, and the schema configurator had no cleanup even for errors. The metastore write is now the commit point and everything before it is reversible. Contract for callers: 2xx means complete; anything else, including a lost connection, means retry; `already exists` on the retry means the earlier attempt did complete. Reservation (store). `create` and `fork` write-lock the name's cache entry for the duration and keep the config in memory only. `publish` installs the finished namespace; `abort` (awaited on error paths) and `Drop` (the creating future was dropped, i.e. the request was cancelled) undo the attempt — config, then directory, then unpin — and only then release the lock, so waiters observe a namespace that does not exist, never a half-built one, and a retry issued the moment the error arrives neither races the cleanup nor finds leftovers. Replaces fork's ad-hoc guard, which blocked in place (panics on a current-thread runtime) and published before flushing. Marker and tombstones (namespace/incomplete.rs). A new namespace directory carries `.incomplete` (fsynced, so it is durable before anything else in the directory can be) until the config is persisted; the store removes it after publishing. A creation that finds a marked directory for a name the metastore does not know discards it, so a crash mid-import never blocks a retry; a startup sweep does the same and deletes leftover tombstones; the metastore's filesystem recovery skips marked directories. Directories are never deleted in place: `discard_dir` renames them into `<base>/.discarded/` (instant; frees the path; a still-running import executor writes into the tombstone) and deletes in the background. `FreshDir` (primary and schema configurators) creates the marked directory synchronously, guard first, and discards it on drop. Fork marks its temporary directory, and only removes its destination on failure if it carries the marker. Metastore. `discard` (via the change queue, so it is ordered after a config store still queued when the request was cancelled) forgets a namespace unconditionally — `remove`'s linked-DB guards would otherwise leave a ghost when a linked-DB creation's flush fails. `existing_handle` looks a namespace up without registering it; `with()` and the fork source use it, and the lazy init re-checks existence, so a lookup racing the end of a failed creation cannot resurrect the name. `process`'s flush branch no longer holds `configs` across `try_process` (lock-order inversion against `remove`). Cache eviction. The reservation is the entry's write lock, which moka knows nothing about. If the cache evicted that entry during a long operation (capacity pressure): a concurrent lookup lazily opened the directory being written; the eviction listener awaited the lock and moka awaits async listeners inline from whichever cache op runs maintenance, so that op hung for the whole import and, once the write-op channel filled, so did every namespace load; an operation touching the cache while holding its own entry (fork) could deadlock. Now the locked entry is pinned in `in_flight`; `load_namespace` and `destroy` consult it first and wait on it; the listener spawns its shutdown only for pinned entries; `install` publishes into a fresh entry when the locked one was evicted, or hands the namespace to the old entry when the name was destroyed meanwhile (destroy is waiting on it). `shutdown` skips pinned entries; `create`/`fork` re-check `has_shutdown` before the commit. Empty cache entries. A rejected `acquire` (`already exists` for a namespace that is not loaded, e.g. a retry after a restart) left behind the empty entry its `get_with` inserted, and `try_get_with` returned that entry forever instead of loading the namespace. `acquire` removes the entry it created; `load_namespace` treats an empty, unlocked entry as a miss; `checkpoint` no longer inserts one for an unloaded namespace (the pre-existing source). Single-namespace mode: `create(default, Latest)` stays a config upsert; creating the default namespace from a dump while it exists is rejected instead of silently skipping the import. Needs async-lock 3 for `RwLock::write_arc` (already in the lockfile; the bump removes the duplicate 2.x). Dump tests set `tcp_capacity(4096)`: the fsync makes setup's following blocking fs ops slower in real time, turmoil advances simulated time while a host waits on them, and the dump store writes 1-byte chunks past turmoil's default 64-segment socket buffer. Tests. Store unit tests build a real `NamespaceStore` with a channel-fed streaming import: lookup waits across an eviction and sees the data, or "doesn't exist" when the import fails; destroy during creation; retry the instant the error returns; abort mid-import; discard vs remove's guards. Unit tests for the marker protocol, tombstones, sweep and metastore recovery. Integration (turmoil): failed creation leaves no trace (both importers, schema configurator), cancelled creation (cancel fires once the marker exists), concurrent requests wait, crash remnants discarded, unmarked directories untouched, marker on a published namespace ignored (create before any lookup after a restart), single-namespace dump create rejected. Rebased onto the false-404 fix (#56), whose semantics `with` and `load_namespace` keep: `with` checks for shutdown on every attempt, looks the namespace up again (up to three times) when the entry it got was emptied, and answers `NamespaceDoesntExist` only when the metastore has no record (otherwise `NamespaceUnavailable` or `NamespaceStoreShutdown`, both 503); `load_namespace` refuses to open a namespace once the store has shut down. `with` keeps `existing_handle`, so the retry cannot resurrect a destroyed record. An empty entry left in the cache is dropped with `evict_if_still_empty` (from #59): under moka's key lock, only if it is still the cached entry, still empty and unlocked. That replaces the `get`-then-`invalidate` pairs in `load_namespace` and `Reservation::acquire`, which could evict a fresh entry loaded in between. Dropped entries are counted in `libsql_server_namespace_left_empty_entry_dropped`.
1 parent 5381d4d commit 900e2ea

16 files changed

Lines changed: 2246 additions & 372 deletions

File tree

‎Cargo.lock‎

Lines changed: 5 additions & 20 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎libsql-server/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ path = "src/main.rs"
1111

1212
[dependencies]
1313
anyhow = "1.0.66"
14-
async-lock = "2.6.0"
14+
async-lock = "3"
1515
async-stream = "0.3.5"
1616
async-tempfile = "0.4.0"
1717
async-trait = "0.1.58"

‎libsql-server/src/lib.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,13 @@ where
638638
)
639639
.await?;
640640

641+
// Namespace creations the previous process died in the middle of leave marked
642+
// directories (and tombstones of discarded ones) behind; clean them up before serving.
643+
if let Err(e) = namespace::incomplete::sweep(&self.path, namespace_store.meta_store()).await
644+
{
645+
tracing::error!("failed to sweep unfinished namespace directories: {e}");
646+
}
647+
641648
self.spawn_monitoring_tasks(&mut task_manager, stats_receiver)?;
642649

643650
// if namespaces are enabled, then bottomless must have set DB ID

‎libsql-server/src/metrics.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,14 @@ pub static NAMESPACE_EMPTIED_ENTRY_GAVE_UP: Lazy<Counter> = Lazy::new(|| {
5353
);
5454
register_counter!(NAME)
5555
});
56+
pub static NAMESPACE_LEFT_EMPTY_ENTRY_DROPPED: Lazy<Counter> = Lazy::new(|| {
57+
const NAME: &str = "libsql_server_namespace_left_empty_entry_dropped";
58+
describe_counter!(
59+
NAME,
60+
"number of empty namespace cache entries dropped so that the next lookup loads the namespace (left by a rejected creation, a failed reset or fork)"
61+
);
62+
register_counter!(NAME)
63+
});
5664
pub static NAMESPACE_LOAD_LATENCY: Lazy<Histogram> = Lazy::new(|| {
5765
const NAME: &str = "libsql_server_namespace_load_latency";
5866
describe_histogram!(NAME, "latency is us when loading a namespace");

‎libsql-server/src/namespace/configurator/fork.rs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,13 @@ impl ForkTask {
130130
let dest_namespace = self.to_namespace.clone();
131131
match self.try_fork().await {
132132
Err(e) => {
133-
let _ =
134-
tokio::fs::remove_dir_all(base_path.join("dbs").join(dest_namespace.as_str()))
135-
.await;
133+
// Only what this fork put there (the renamed, marked temp dir); a directory that
134+
// was already at the destination is not ours to remove.
135+
if let Err(e) =
136+
crate::namespace::incomplete::discard_if_incomplete(&base_path, &dest_namespace)
137+
{
138+
tracing::error!(namespace = %dest_namespace, "failed to discard fork directory: {e}");
139+
}
136140
Err(e)
137141
}
138142
Ok(ns) => Ok(ns),
@@ -145,6 +149,9 @@ impl ForkTask {
145149
let temp_dir = BLOCKING_RT
146150
.spawn_blocking(move || tempfile::tempdir_in(base_path))
147151
.await??;
152+
// Marked incomplete until the store has persisted the namespace's config, so that a
153+
// fork that does not complete is discarded like any other unfinished creation.
154+
crate::namespace::incomplete::mark_incomplete(temp_dir.path())?;
148155
let db_path = temp_dir.path().join("data");
149156

150157
if let Some(restore) = self.restore_to {

‎libsql-server/src/namespace/configurator/helpers.rs‎

Lines changed: 142 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,68 @@ use crate::namespace::broadcasters::BroadcasterHandle;
1919
use crate::namespace::meta_store::MetaStoreHandle;
2020
use crate::namespace::replication_wal::{make_replication_wal_wrapper, ReplicationWalWrapper};
2121
use crate::namespace::{
22-
NamespaceBottomlessDbId, NamespaceBottomlessDbIdInit, NamespaceName, ResolveNamespacePathFn,
23-
RestoreOption,
22+
incomplete, NamespaceBottomlessDbId, NamespaceBottomlessDbIdInit, NamespaceName,
23+
ResolveNamespacePathFn, RestoreOption,
2424
};
2525
use crate::replication::ReplicationLogger;
2626
use crate::stats::Stats;
2727
use crate::{StatsSender, BLOCKING_RT, DB_CREATE_TIMEOUT, DEFAULT_AUTO_CHECKPOINT};
2828

2929
use super::{BaseNamespaceConfig, PrimaryConfig};
3030

31+
/// The directory of a namespace that does not exist yet, created marked incomplete.
32+
///
33+
/// If the guard is dropped before [`keep`](Self::keep) — setup failed, or the future creating
34+
/// the namespace was dropped — the directory is discarded (atomically renamed away and deleted
35+
/// in the background, see [`incomplete::discard_dir`]). The marker itself is removed by the
36+
/// namespace store once the config is persisted.
37+
pub(super) struct FreshDir {
38+
base: Arc<Path>,
39+
path: Option<Arc<Path>>,
40+
}
41+
42+
impl FreshDir {
43+
/// Returns `None` when a namespace directory already exists at `db_path`.
44+
///
45+
/// Synchronous on purpose: the guard exists before anything is created, and there is no
46+
/// await point between creating the directory and marking it.
47+
pub(super) fn begin(base: &Arc<Path>, db_path: &Arc<Path>) -> crate::Result<Option<Self>> {
48+
if db_path.try_exists()? {
49+
return Ok(None);
50+
}
51+
let guard = Self {
52+
base: base.clone(),
53+
path: Some(db_path.clone()),
54+
};
55+
incomplete::create_marked_dir(db_path)?;
56+
Ok(Some(guard))
57+
}
58+
59+
/// The namespace was set up; the directory stays.
60+
pub(super) fn keep(mut self) {
61+
self.path.take();
62+
}
63+
}
64+
65+
impl Drop for FreshDir {
66+
fn drop(&mut self) {
67+
let Some(path) = self.path.take() else {
68+
return;
69+
};
70+
tracing::info!(
71+
path = %path.display(),
72+
"namespace creation did not complete; discarding its directory"
73+
);
74+
if let Err(e) = incomplete::discard_dir(&self.base, &path) {
75+
// still marked: discarded by the next creation of the name, or by the startup sweep
76+
tracing::error!(
77+
path = %path.display(),
78+
"failed to discard unfinished namespace directory: {e}"
79+
);
80+
}
81+
}
82+
}
83+
3184
#[tracing::instrument(skip_all)]
3285
pub(super) async fn make_primary_connection_maker(
3386
primary_config: &PrimaryConfig,
@@ -405,3 +458,90 @@ pub(super) async fn cleanup_primary(
405458

406459
Ok(())
407460
}
461+
462+
#[cfg(test)]
463+
mod test {
464+
use super::*;
465+
use crate::namespace::incomplete::INCOMPLETE_MARKER;
466+
467+
async fn settle_background_deletes() {
468+
for _ in 0..50 {
469+
tokio::task::yield_now().await;
470+
tokio::time::sleep(Duration::from_millis(2)).await;
471+
}
472+
}
473+
474+
#[tokio::test]
475+
async fn fresh_dir_is_discarded_unless_kept() {
476+
let tmp = tempfile::tempdir().unwrap();
477+
let base: Arc<Path> = tmp.path().into();
478+
let path: Arc<Path> = tmp.path().join("dbs").join("ns").into();
479+
480+
let fresh = FreshDir::begin(&base, &path).unwrap().expect("fresh");
481+
assert!(path.join(INCOMPLETE_MARKER).exists());
482+
drop(fresh);
483+
// the path is free synchronously, the tombstone goes away in the background
484+
assert!(!path.exists(), "dropped before keep: discarded");
485+
settle_background_deletes().await;
486+
assert!(std::fs::read_dir(tmp.path().join(".discarded"))
487+
.unwrap()
488+
.next()
489+
.is_none());
490+
491+
let fresh = FreshDir::begin(&base, &path).unwrap().expect("fresh again");
492+
fresh.keep();
493+
assert!(
494+
path.join(INCOMPLETE_MARKER).exists(),
495+
"kept: stays, marker included"
496+
);
497+
498+
assert!(
499+
FreshDir::begin(&base, &path).unwrap().is_none(),
500+
"an existing directory is not fresh"
501+
);
502+
}
503+
504+
#[tokio::test]
505+
async fn fresh_dir_is_discarded_when_its_future_is_dropped() {
506+
let tmp = tempfile::tempdir().unwrap();
507+
let base: Arc<Path> = tmp.path().into();
508+
let path: Arc<Path> = tmp.path().join("dbs").join("ns").into();
509+
let (ready, is_ready) = tokio::sync::oneshot::channel();
510+
511+
let setup = {
512+
let (base, path) = (base.clone(), path.clone());
513+
async move {
514+
let fresh = FreshDir::begin(&base, &path).unwrap();
515+
ready.send(()).unwrap();
516+
std::future::pending::<()>().await; // "the import"
517+
fresh.map(FreshDir::keep);
518+
}
519+
};
520+
let setup = tokio::spawn(setup);
521+
is_ready.await.unwrap();
522+
assert!(path.join(INCOMPLETE_MARKER).exists());
523+
setup.abort();
524+
let _ = setup.await;
525+
assert!(!path.exists());
526+
}
527+
528+
#[tokio::test]
529+
async fn discard_if_incomplete_only_removes_marked_directories() {
530+
let tmp = tempfile::tempdir().unwrap();
531+
let marked = tmp.path().join("dbs").join("marked");
532+
let unmarked = tmp.path().join("dbs").join("unmarked");
533+
for dir in [&marked, &unmarked] {
534+
std::fs::create_dir_all(dir).unwrap();
535+
std::fs::write(dir.join("data"), b"junk").unwrap();
536+
}
537+
std::fs::write(marked.join(INCOMPLETE_MARKER), b"").unwrap();
538+
539+
let ns = |s: &str| NamespaceName::from_string(s.into()).unwrap();
540+
assert!(incomplete::discard_if_incomplete(tmp.path(), &ns("marked")).unwrap());
541+
assert!(!incomplete::discard_if_incomplete(tmp.path(), &ns("unmarked")).unwrap());
542+
assert!(!incomplete::discard_if_incomplete(tmp.path(), &ns("absent")).unwrap());
543+
544+
assert!(!marked.exists());
545+
assert!(unmarked.join("data").exists());
546+
}
547+
}

‎libsql-server/src/namespace/configurator/mod.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,16 @@ pub trait ConfigureNamespace {
131131
bottomless_db_id_init: NamespaceBottomlessDbIdInit,
132132
) -> Pin<Box<dyn Future<Output = crate::Result<()>> + Send + 'a>>;
133133

134+
/// Discard whatever a creation of `namespace` that did not complete left on disk, so that
135+
/// the name starts from nothing. Only called for a name that is neither loaded nor known to
136+
/// the metastore; must leave anything that does not carry the
137+
/// [`INCOMPLETE_MARKER`](super::incomplete::INCOMPLETE_MARKER) alone. Synchronous so that
138+
/// it can run while a creating future is being dropped (see `NamespaceStore`).
139+
fn discard_incomplete(&self, namespace: &NamespaceName) -> crate::Result<()> {
140+
let _ = namespace;
141+
Ok(())
142+
}
143+
134144
fn fork<'a>(
135145
&'a self,
136146
from_ns: &'a Namespace,

‎libsql-server/src/namespace/configurator/primary.rs‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use crate::namespace::{
2121
use crate::run_periodic_checkpoint;
2222
use crate::schema::{has_pending_migration_task, setup_migration_table};
2323

24-
use super::helpers::cleanup_primary;
24+
use super::helpers::{cleanup_primary, FreshDir};
2525
use super::{BaseNamespaceConfig, ConfigureNamespace, PrimaryConfig};
2626

2727
pub struct PrimaryConfigurator {
@@ -129,35 +129,31 @@ impl ConfigureNamespace for PrimaryConfigurator {
129129
) -> Pin<Box<dyn Future<Output = crate::Result<Namespace>> + Send + 'a>> {
130130
Box::pin(async move {
131131
let db_path: Arc<Path> = self.base.base_path.join("dbs").join(name.as_str()).into();
132-
let fresh_namespace = !db_path.try_exists()?;
133-
// FIXME: make that truly atomic. explore the idea of using temp directories, and it's implications
134-
match self
132+
// A brand-new directory is discarded again if setup fails or this future is dropped.
133+
let fresh = FreshDir::begin(&self.base.base_path, &db_path)?;
134+
let ns = self
135135
.try_new_primary(
136136
name.clone(),
137137
meta_store_handle,
138138
restore_option,
139139
resolve_attach_path,
140-
db_path.clone(),
140+
db_path,
141141
broadcaster,
142142
self.base.encryption_config.clone(),
143143
)
144-
.await
145-
{
146-
Ok(this) => Ok(this),
147-
Err(e) if fresh_namespace => {
148-
tracing::error!(
149-
"an error occured while deleting creating namespace, cleaning..."
150-
);
151-
if let Err(e) = tokio::fs::remove_dir_all(&db_path).await {
152-
tracing::error!("failed to remove dirty namespace directory: {e}")
153-
}
154-
Err(e)
155-
}
156-
Err(e) => Err(e),
144+
.await?;
145+
if let Some(fresh) = fresh {
146+
fresh.keep();
157147
}
148+
Ok(ns)
158149
})
159150
}
160151

152+
fn discard_incomplete(&self, namespace: &NamespaceName) -> crate::Result<()> {
153+
crate::namespace::incomplete::discard_if_incomplete(&self.base.base_path, namespace)?;
154+
Ok(())
155+
}
156+
161157
fn cleanup<'a>(
162158
&'a self,
163159
namespace: &'a NamespaceName,

0 commit comments

Comments
 (0)