Skip to content

Commit 4cc2df7

Browse files
libsql-server: close out the low-severity review items
- `Reservation::publish` keeps the guard inside the reservation across `install`'s awaits, so a cancellation there undoes the creation instead of leaving the name pinned with the lock released. - `remove_incomplete_marker` stats before taking the blocking-pool hop; every cold load passes through it and the marker is almost always absent. - Failing to remove the marker after publish is logged at `error` (it only matters if the metastore is rebuilt from the filesystem meanwhile); `FreshDir`'s log is `info` (client errors pass through it too); `acquire` logs when it has to wait for an operation in progress. - Restore the `Namespace` doc comment that had ended up on a helper. - Tests: store-level `aborted_creation_is_undone` (`JoinHandle::abort` mid-import; the waiting lookup sees "doesn't exist", the name is free); the restart test drops the first server before starting the second; the unmarked-directory test asserts the outcome instead of discarding it. - Docs: 400 not "409-class"; the marker is acted upon in two places (create and the startup sweep), both only for names the metastore doesn't know; upgrade/downgrade notes; single-namespace-mode behaviour in ADMIN_API.md.
1 parent 53298a9 commit 4cc2df7

6 files changed

Lines changed: 109 additions & 16 deletions

File tree

‎docs/ADMIN_API.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ deleted). For callers:
6666
- any other outcome, including a lost connection: retry the same request; a
6767
`400 Namespace already exists` on the retry means the earlier attempt did complete.
6868

69+
In single-namespace mode (`--disable-namespaces`) creating the default namespace is a
70+
configuration upsert and always succeeds, except with a `dump_url`: a dump can only be imported
71+
into a namespace that does not exist yet, so that request is rejected like any other.
72+
6973
See `docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md`.
7074

7175
```HTTP

‎docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ Ok(ns)
201201
- `pub(crate) const INCOMPLETE_MARKER: &str = ".incomplete";` in `namespace/mod.rs`.
202202
- Created by `FreshDir::begin` for every brand-new directory.
203203
- Removed by `Namespace::mark_complete(&self)` (`remove_file(self.path.join(MARKER))`, `NotFound` ok), called (a) by `create` after `flush`, (b) by `load_namespace`'s init after `make_namespace` — a namespace opened from the metastore is complete by definition, so lazily loaded namespaces (and single-namespace-mode auto-creation) never keep a marker.
204-
- Consulted in exactly one place: `ConfigureNamespace::discard_incomplete(name)` (new trait method, default no-op; primary and schema share `helpers::discard_incomplete(base, name)` = `if dbs/<name>/.incomplete exists → remove_dir_all`). `create` calls it after `acquire`, i.e. only for a name with **no row and no loaded namespace** — the only situation in which a marked directory is unambiguously garbage. Lazy loads never see it, so a stale marker on a published namespace (crash between `flush` and `mark_complete`) is removed, never acted upon.
204+
- Acted upon only for a name the metastore does **not** know: by `ConfigureNamespace::discard_incomplete(name)` (new trait method, default no-op; primary and schema delegate to `incomplete::discard_if_incomplete`), which `create` calls after `acquire` — i.e. with no row and nothing loaded — and by the startup `sweep` under the same condition. That is the only situation in which a marked directory is unambiguously garbage. Lazy loads never act on it, so a stale marker on a published namespace (crash between `flush` and unmark) is removed, never acted upon. The metastore's filesystem recovery (`maybe_recover_from_fs`, empty metastore only) skips marked directories rather than adopting them.
205205
- `MetaStoreInner::maybe_recover_from_fs` (metastore empty ⇒ adopt `dbs/*`) skips directories carrying the marker, so a crash during the very first creation on a fresh server does not get adopted as a namespace at the next start.
206206

207207
A directory **without** a marker and without a row keeps today's semantics (`Dump` ⇒ `LoadDumpExistingDb`; `Latest` ⇒ adopted). Such directories can only come from pre-marker servers or manual intervention; being destructive there is not worth it.
@@ -224,7 +224,7 @@ Replace the `Bomb` + manual `to_lock` with `Reservation::acquire(to)`; order bec
224224
## 5. Flow for a dump import (streaming or buffered)
225225

226226
1. Admin handler resolves `dump_url` (errors here touch nothing).
227-
2. `create` → `Reservation::acquire` (409-class error if the name is loaded or has a row).
227+
2. `create` → `Reservation::acquire` (`NamespaceAlreadyExist`, HTTP 400, if the name is loaded or has a row).
228228
3. In-memory config; `discard_incomplete` (removes a crashed previous attempt, if any).
229229
4. `setup`: `FreshDir::begin` (mkdir + marker) → logger, WAL, connection maker → `load_dump` → `keep()`.
230230
5. `flush` — the namespace now exists durably.
@@ -243,7 +243,7 @@ Replace the `Bomb` + manual `to_lock` with `Reservation::acquire(to)`; order bec
243243
| between `publish` and marker removal | — (published; a stale marker is harmless) | same | row + marker + complete data ⇒ serves; marker removed on first lazy load |
244244
| after `publish` | — | response may be lost ⇒ client retries ⇒ `NamespaceAlreadyExist` ⇒ by the contract, complete | same |
245245

246-
Client rule: **2xx ⇒ complete; anything else ⇒ retry; `NamespaceAlreadyExist` on retry ⇒ complete.** No inspection of tables needed.
246+
Client rule: **2xx ⇒ complete; anything else ⇒ retry; `NamespaceAlreadyExist` on retry ⇒ complete.** No inspection of tables needed. (For namespaces *created* through this path. The single-namespace-mode config upsert of the default namespace, and ghost rows left by older binaries, are outside it.)
247247

248248
## 7. Concurrency
249249

@@ -289,7 +289,11 @@ Reviewers (correctness, architecture, testing, operations) plus a manual pass fo
289289
| MED | Cancel tests could fire before the creation started (turmoil latency) and so pass vacuously; the restart test never reached the `\|\| exists` guard. | Cancel once `dbs/foo/.incomplete` exists; create before any lookup after restart; "usable afterwards" assertion restored. |
290290
| LOW | Marked orphans were only reclaimed by a `create` of the same name; `maybe_recover_from_fs` skip untested; success not logged. | Startup `sweep`; `recovery_from_fs_skips_marked_directories`, `sweep_cleans_leftovers_of_a_crash`; `info!("namespace created")` with elapsed, `warn!` with the error on failure, distinct message on cancellation. |
291291

292-
Not changed: bottomless objects uploaded under a never-published db_id are inert junk (as today); `fsync` of the marker (power loss between creating and syncing it could leave an unmarked orphan — accepted); the moka `get_with(default)` leftover in `checkpoint` is healed lazily rather than fixed at the source.
292+
LOW items closed in the follow-up commit: `publish` keeps the guard inside the reservation across `install`'s awaits, so a cancellation there undoes the creation instead of leaking the pin; `remove_incomplete_marker` stats before taking the blocking-pool hop (every cold load passes through it); a failure to remove the marker after publish is logged at `error`; `FreshDir`'s log is `info` (every failed create, including client errors, passes through it); `acquire` logs when it has to wait; store-level `aborted_creation_is_undone` test (`JoinHandle::abort` mid-import); the restart test drops the first server before starting the second; the unmarked-directory test asserts the outcome.
293+
294+
Not changed: bottomless objects uploaded under a never-published db_id are inert junk (as today); `fsync` of the marker (power loss between creating and syncing it could leave an unmarked orphan — accepted); the moka `get_with(default)` leftover in `checkpoint` is healed lazily rather than fixed at the source; after an *abort* the (empty) cache entry stays until a lookup heals it or the cache evicts it — it counts toward capacity but is never served; a metastore rebuilt from the filesystem (`destroy_on_error` + `allow_recover_from_fs`) in the milliseconds between a namespace's flush and its unmark would skip that namespace.
295+
296+
**Upgrade / downgrade.** Ghost rows and unmarked orphan directories left by failed creates on *older* binaries are untouched (everything here is marker-gated); for them the old remedy stays: `DELETE /v1/namespaces/:ns`, then create again. Downgrading the binary is safe: the metastore schema and the config protobuf are unchanged; an old binary ignores `.incomplete` and `.discarded/` (it may adopt a marked orphan via `maybe_recover_from_fs` on an empty metastore, as it always could with any leftover directory).
293297

294298
## 10. Interaction with the streaming importer
295299

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ impl Drop for FreshDir {
6767
let Some(path) = self.path.take() else {
6868
return;
6969
};
70-
tracing::warn!(
70+
tracing::info!(
7171
path = %path.display(),
7272
"namespace creation did not complete; discarding its directory"
7373
);

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,21 @@ pub enum NamespaceBottomlessDbIdInit {
5757
FetchFromConfig,
5858
}
5959

60-
/// A namespace isolates the resources pertaining to a database of type T
6160
/// Remove the incomplete marker of the namespace directory `path`, if present.
6261
pub(crate) async fn remove_incomplete_marker(path: &Path) -> std::io::Result<()> {
63-
match tokio::fs::remove_file(path.join(incomplete::INCOMPLETE_MARKER)).await {
62+
let marker = path.join(incomplete::INCOMPLETE_MARKER);
63+
// Almost always absent (every cold load passes through here): a stat is cheaper than a
64+
// trip to the blocking pool.
65+
if !marker.exists() {
66+
return Ok(());
67+
}
68+
match tokio::fs::remove_file(marker).await {
6469
Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(e),
6570
_ => Ok(()),
6671
}
6772
}
6873

74+
/// A namespace isolates the resources pertaining to a database of type T
6975
#[derive(Debug)]
7076
pub struct Namespace {
7177
pub db: Database,

‎libsql-server/src/namespace/store.rs‎

Lines changed: 81 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,9 @@ impl Reservation {
9393
.await
9494
}
9595
};
96+
if entry.try_write().is_none() {
97+
tracing::debug!(namespace = %name, "waiting for an operation in progress on the namespace");
98+
}
9699
let guard = entry.write_arc().await;
97100
if guard.is_some() || store.inner.metadata.exists(&name).await {
98101
if guard.is_none() {
@@ -120,13 +123,17 @@ impl Reservation {
120123

121124
/// Install the finished namespace and release the lock.
122125
async fn publish(mut self, ns: Namespace) {
123-
let mut guard = self
124-
.guard
125-
.take()
126-
.expect("a reservation is published at most once");
127-
self.store
128-
.install(&self.name, &self.entry, &mut guard, ns)
129-
.await;
126+
// `install` awaits (cache lookups). Keep the guard inside `self` across them: if this
127+
// future is dropped there, `Drop` still sees an unpublished reservation and undoes the
128+
// creation — rather than leaving the name pinned and the lock released.
129+
{
130+
let guard = self
131+
.guard
132+
.as_deref_mut()
133+
.expect("a reservation is published at most once");
134+
self.store.install(&self.name, &self.entry, guard, ns).await;
135+
}
136+
let guard = self.guard.take().expect("checked above");
130137
// Unpin before unlocking: a lookup racing with us either still finds the pinned entry
131138
// (and waits for the lock, then sees the namespace) or goes through the cache, where
132139
// the namespace now is.
@@ -513,7 +520,9 @@ impl NamespaceStore {
513520
let marker_dir = ns.path.clone();
514521
reservation.publish(ns).await;
515522
if let Err(e) = super::remove_incomplete_marker(&marker_dir).await {
516-
tracing::warn!(namespace = %name, "could not remove creation marker: {e}");
523+
// The namespace is published and serves; the marker is removed at its next load. It
524+
// matters only if the metastore is lost and rebuilt from the filesystem meanwhile.
525+
tracing::error!(namespace = %name, "could not remove creation marker: {e}");
517526
}
518527
}
519528

@@ -1260,4 +1269,68 @@ mod test {
12601269
.unwrap();
12611270
assert!(!store.exists(&linked).await);
12621271
}
1272+
1273+
/// The creating task is aborted mid-import (the request was cancelled). The reservation is
1274+
/// dropped unpublished: config discarded, directory discarded, name released — and a
1275+
/// lookup that was waiting sees "doesn't exist".
1276+
#[tokio::test]
1277+
async fn aborted_creation_is_undone() {
1278+
let tmp = tempfile::tempdir().unwrap();
1279+
let store = make_store(tmp.path()).await;
1280+
let foo = NamespaceName::from_string("foo".into()).unwrap();
1281+
let dir = tmp.path().join("dbs").join("foo");
1282+
1283+
let (tx, dump) = slow_dump();
1284+
let create = tokio::spawn({
1285+
let (store, foo) = (store.clone(), foo.clone());
1286+
async move {
1287+
store
1288+
.create(foo, RestoreOption::Dump(dump), Default::default())
1289+
.await
1290+
}
1291+
});
1292+
tx.send(Ok(Bytes::from_static(
1293+
b"BEGIN TRANSACTION; CREATE TABLE test (x); INSERT INTO test VALUES (1);",
1294+
)))
1295+
.await
1296+
.unwrap();
1297+
wait_until_in_flight(&store, &foo).await;
1298+
assert!(dir
1299+
.join(crate::namespace::incomplete::INCOMPLETE_MARKER)
1300+
.exists());
1301+
1302+
let lookup = tokio::spawn({
1303+
let store = store.clone();
1304+
async move { count_test_rows(&store, "foo").await }
1305+
});
1306+
tokio::time::sleep(Duration::from_millis(100)).await;
1307+
assert!(!lookup.is_finished());
1308+
1309+
create.abort();
1310+
assert!(create.await.unwrap_err().is_cancelled());
1311+
1312+
assert!(matches!(
1313+
lookup.await.unwrap(),
1314+
Err(Error::NamespaceDoesntExist(_))
1315+
));
1316+
// the cleanup ran on the blocking pool; the lookup's completion proves the lock was
1317+
// released after it, so by now everything is undone
1318+
assert!(!store.exists(&foo).await);
1319+
assert!(!dir.exists());
1320+
assert!(store.inner.in_flight.lock().is_empty());
1321+
1322+
// the importer's executor thread is still unwinding its channel; the name is free anyway
1323+
let (tx, dump) = slow_dump();
1324+
tx.send(Ok(Bytes::from_static(
1325+
b"BEGIN TRANSACTION; CREATE TABLE test (x); INSERT INTO test VALUES (1); COMMIT;",
1326+
)))
1327+
.await
1328+
.unwrap();
1329+
drop(tx);
1330+
store
1331+
.create(foo.clone(), RestoreOption::Dump(dump), Default::default())
1332+
.await
1333+
.unwrap();
1334+
assert_eq!(count_test_rows(&store, "foo").await.unwrap(), 1);
1335+
}
12631336
}

‎libsql-server/tests/namespaces/lifecycle.rs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,14 +263,19 @@ fn unmarked_directory_is_left_alone() {
263263

264264
sim.client("client", async move {
265265
let client = Client::new();
266-
let _ = create_from_dump(
266+
// No database in there, so the import proceeds into the existing directory ...
267+
let resp = create_from_dump(
267268
&client,
268269
"foo",
269270
&file_url(&tmp_path.join("good.sql")),
270271
STREAMING,
271272
)
272273
.await?;
274+
assert_eq!(resp.status(), StatusCode::OK);
275+
assert_eq!(count_rows("foo", "test").await?, 1);
276+
// ... and whatever was in it is left alone, before and after.
273277
assert_eq!(std::fs::read(dir.join("keep.txt"))?, b"precious");
278+
assert!(!dir.join(INCOMPLETE_MARKER).exists());
274279
Ok(())
275280
});
276281

@@ -321,6 +326,7 @@ fn marker_on_published_namespace_is_ignored() {
321326
}
322327
});
323328
first.run().unwrap();
329+
drop(first); // the first server must be gone before the second opens the same files
324330

325331
// restart: nothing is loaded yet, the marker is still there and the metastore knows the
326332
// name. A `create` must be rejected on the strength of the metastore alone (not wipe the

0 commit comments

Comments
 (0)