Skip to content

Commit 44afb04

Browse files
libsql-server: address the adversarial review of the namespace store
Six findings from an independent correctness review of the all-or-nothing creation, all in code this change introduced, plus three residuals from a second pass on the fixes. - A request dropped inside `publish` could leave the namespace installed in the cache while the reservation's cleanup discarded its row and directory; the name then answered "already exists" to `create` and "doesn't exist" to everything else until the entry was evicted. `publish` now runs in its own task: past the commit point the creation completes even if the request that started it is gone. A panic in that task is returned as an error, not reported as success. - The pin was claimed after the entry was locked. In that window the eviction listener took the inline path and waited on the lock under moka's key lock, which the operation's own `install` needs: a deadlock for that name. After an abort, a third `create` could also take a second reservation on a fresh entry for the same name. `claim` now pins the name under the `in_flight` mutex before the lock is taken, waits for a holder instead of overwriting its pin, and releases the pin if the attempt is dropped before it got the lock (`PinClaim`). `reset` uses the same step. - `create` of an existing namespace write-locked its entry before answering "already exists"; the lock prefers writers, so every new reader waited until the lock was granted. The metastore is consulted first. - A creation or reset that passed its last shutdown check could install after `shutdown` had collected the entries; `shut_down_if_stopped` runs after publish and after `reset`'s install. - `destroy` took either the pinned or the cached entry; both are taken, so a namespace an operation just installed into a fresh entry is destroyed by `destroy` rather than shut down by the eviction listener. `install` drops a fresh entry whose record a `destroy` removed between its check and its insert. - `sweep` ended at the first regular file in `dbs/` (`<file>/.incomplete` fails with ENOTDIR); files are skipped. Tests: a loaded namespace's readers do not delay `create`'s rejection; a reservation attempt dropped while waiting for the lock leaves no pin; `destroy` takes both entries; a stray file does not stop the sweep.
1 parent c426293 commit 44afb04

4 files changed

Lines changed: 288 additions & 81 deletions

File tree

‎docs/ADMIN_API.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ deleted). For callers:
4040

4141
- `2xx`: the namespace is complete;
4242
- any other outcome, including a lost connection: retry the same request; a
43-
`400 Namespace already exists` on the retry means the earlier attempt did complete.
43+
`400 Namespace already exists` on the retry means the earlier attempt did complete. (Once
44+
the configuration is persisted, the creation finishes even if the client has gone away.)
4445

4546
In single-namespace mode (`--disable-namespaces`) creating the default namespace is a
4647
configuration upsert and always succeeds, except with a `dump_url`: a dump can only be imported

‎docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ The reservation is the cache entry's lock, but moka knows nothing about that loc
263263

264264
Fix, in three parts:
265265

266-
- **Pin.** `NamespaceStoreInner::in_flight: Mutex<HashMap<NamespaceName, NamespaceEntry>>`. `Reservation::acquire` (create, fork) and `reset` pin the locked entry; `load_namespace` consults the map *before* `try_get_with` and waits on the pinned entry instead of loading. If the entry ends up empty — the operation failed, or published elsewhere — the lookup falls through: `NamespaceDoesntExist` if the metastore no longer knows the name, otherwise the regular cache path. Unpin happens after the config removal (abort) or after the namespace is visible (publish), so no lookup can slip into a lazy load in between. `destroy` consults the map too.
266+
- **Pin.** `NamespaceStoreInner::in_flight: Mutex<HashMap<NamespaceName, NamespaceEntry>>`. `Reservation::acquire` (create, fork) and `reset` pin the locked entry; `load_namespace` consults the map *before* `try_get_with` and waits on the pinned entry instead of loading. If the entry ends up empty — the operation failed, or published elsewhere — the lookup falls through: `NamespaceDoesntExist` if the metastore no longer knows the name, otherwise the regular cache path. Unpin happens after the config removal (abort) or after the namespace is visible (publish), so no lookup can slip into a lazy load in between. `destroy` consults the map too. The pin is claimed **before** the entry is locked (`NamespaceStore::claim`, under the `in_flight` mutex), so the name is pinned at every moment its entry is locked: the listener never waits on the lock inline, and a second operation finds the pin and waits for the holder instead of reserving a fresh entry for the same name. An attempt dropped before it got the lock releases its claim (`PinClaim`). `create` consults the metastore before claiming anything, so an existing namespace is rejected without its readers ever seeing a pending writer.
267267
- **Non-blocking eviction listener.** The listener spawns the shutdown on its own task and returns immediately, so maintenance never waits for an operation in progress.
268268
- **Publish into a fresh entry when evicted.** `NamespaceStore::install` checks (by `Arc::ptr_eq`) that the locked entry is still the cache's entry for the name; if not, the namespace goes into a fresh entry and the orphaned one stays empty, so the spawned listener finds nothing to shut down and waiters fall through to the cache. Used by `create`, `fork` and `reset`.
269269

@@ -297,6 +297,20 @@ Reviewers (correctness, architecture, testing, operations) plus a manual pass fo
297297

298298
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.
299299

300+
### Second review (2026-10-22, after the rebase onto #56 and the restack on the release branch)
301+
302+
| Severity | Finding | Fix |
303+
|---|---|---|
304+
| MED | A request dropped inside `publish` (moka's `insert` awaits after putting the entry in the map) left the namespace installed while `Reservation::drop` discarded its row and directory: `create` ⇒ 400, everything else ⇒ 404, until eviction. | `publish` runs in its own task: past the commit point the creation completes without the request. |
305+
| MED | The pin was claimed after the lock. In the locked-but-unpinned window the eviction listener waited on the lock inline, under moka's key lock, which `install`'s `insert` needs ⇒ deadlock for that name. After an abort, a third `create` could reserve a fresh entry for the same name ⇒ two creations on one directory. | `claim` pins before locking, waits for a holder, releases the claim on cancellation; `reset` uses it too. |
306+
| LOW–MED | `create` of an existing namespace write-locked its entry before "already exists"; async-lock prefers writers, so new readers waited (for the whole fork when the namespace was a fork source). | Metastore check before touching the entry. |
307+
| LOW | A creation (or reset) past its last shutdown check could install after `shutdown` had collected the entries. | `shut_down_if_stopped` after publish and after `reset`'s install. |
308+
| LOW | `destroy` took the pinned *or* the cached entry; a namespace just installed into a fresh entry was left to the eviction listener. | Both entries are taken; `install` drops a fresh entry whose record a `destroy` removed between its check and its insert. |
309+
| LOW | A panic in the spawned publish task was only logged; the caller returned 200 for a namespace the task's `Drop` had undone. | `publish` returns the error. |
310+
| LOW | `sweep` stopped at a regular file in `dbs/` (`<file>/.incomplete` ⇒ ENOTDIR). | Files skipped. |
311+
312+
Accepted: the eviction listener reads the pin once, when moka creates it; a claim that starts and finishes between that read and the listener's first poll (nanoseconds) still waits inline. Noted, out of scope: `connection/program.rs` calls `MetaStore::handle` for `ATTACH`, which registers a config for a nonexistent name (pre-existing).
313+
300314
Also closed: the marker and its directory entry are fsynced on creation (`mark_incomplete`), so the marker is durable before anything else in the directory can be; `checkpoint` no longer mints an empty cache entry for an unloaded namespace (the pre-existing source of the same wedge); `ForkTask::fork` only removes a failed fork's destination if it carries the marker.
301315

302316
Not changed: 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. Bottomless is not used in our deployment and this change is untested with it.

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,11 @@ pub(crate) async fn sweep(base: &Path, meta_store: &MetaStore) -> std::io::Resul
133133
return Ok(());
134134
};
135135
for entry in entries.flatten() {
136+
// Only directories can be namespaces; `<file>/.incomplete` would fail with ENOTDIR
137+
// and end the sweep.
138+
if !entry.file_type()?.is_dir() {
139+
continue;
140+
}
136141
if !entry.path().join(INCOMPLETE_MARKER).try_exists()? {
137142
continue;
138143
}
@@ -223,9 +228,15 @@ mod test {
223228
create_marked_dir(&known).unwrap();
224229
let unmarked = base.join("dbs").join("unmarked");
225230
std::fs::create_dir_all(&unmarked).unwrap();
231+
// A stray file is skipped, not an error that ends the sweep (`<file>/.incomplete`
232+
// would fail with ENOTDIR). Named to sort before the directories.
233+
let stray = base.join("dbs").join("a-stray-file");
234+
std::fs::write(&stray, b"junk").unwrap();
226235

227236
sweep(base, &store).await.unwrap();
228237

238+
assert!(stray.exists());
239+
229240
assert!(!crashed.exists());
230241
assert!(
231242
known.join(INCOMPLETE_MARKER).exists(),

0 commit comments

Comments
 (0)