Skip to content

Commit afa6361

Browse files
libsql-server: durable creation marker; fix two pre-existing empty-entry sources
- The `.incomplete` marker and its directory entry are fsynced when written (`incomplete::mark_incomplete`, also used for fork's temp dir), so the marker is durable before anything else in the directory can be. Without this a power loss could in principle leave a directory with content but no marker, which nothing would discard. - `checkpoint` of a namespace that is not loaded no longer inserts an empty cache entry (pre-existing: lookups then found the empty entry instead of loading the namespace, until eviction). - A failed fork only removes its destination directory if it carries the marker (pre-existing: it removed whatever was there). - Dump tests: `tcp_capacity(4096)`. The fsync makes the following blocking fs ops in setup slower in real time; turmoil advances simulated time while the server waits on them and the dump store keeps writing 1-byte chunks into a socket nobody reads yet, past turmoil's default 64-segment buffer. Real TCP would apply back-pressure; turmoil panics. - Bottomless is not used in our deployment: the design doc now says "untested with this change" instead of reasoning about remote cleanup.
1 parent 4cc2df7 commit afa6361

5 files changed

Lines changed: 33 additions & 20 deletions

File tree

‎docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ Both dump importers run inside this sequence; the problem is independent of PR #
2828
- No lock-step changes to clients, replication, the metastore schema or the `DatabaseConfig` protobuf.
2929
- Small: reuse the pattern `fork` already uses; one mechanism for `create` and `fork`.
3030

31-
Non-goals: a server-side import timeout; idempotency keys / async create jobs; bottomless remote cleanup for aborted imports (remote objects under a never-published db_id are inert junk, as today); the moka eviction hazard shared with `reset`/`fork` (§8).
31+
Non-goals: a server-side import timeout; idempotency keys / async create jobs; bottomless (not used in our deployment; untested with this change). The moka eviction hazard shared with `reset`/`fork` turned out to need fixing (§8).
3232

3333
## 3. Design in one paragraph
3434

@@ -291,7 +291,9 @@ Reviewers (correctness, architecture, testing, operations) plus a manual pass fo
291291

292292
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.
293293

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.
294+
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.
295+
296+
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.
295297

296298
**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).
297299

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

Lines changed: 8 additions & 9 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),
@@ -147,12 +151,7 @@ impl ForkTask {
147151
.await??;
148152
// Marked incomplete until the store has persisted the namespace's config, so that a
149153
// fork that does not complete is discarded like any other unfinished creation.
150-
tokio::fs::File::create(
151-
temp_dir
152-
.path()
153-
.join(crate::namespace::incomplete::INCOMPLETE_MARKER),
154-
)
155-
.await?;
154+
crate::namespace::incomplete::mark_incomplete(temp_dir.path())?;
156155
let db_path = temp_dir.path().join("data");
157156

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

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,19 @@ pub(crate) fn namespace_dir(base: &Path, name: &NamespaceName) -> PathBuf {
3838

3939
/// Create the directory of a namespace that does not exist yet, marked incomplete.
4040
///
41-
/// Synchronous (two small filesystem operations) so that there is no await point between
42-
/// creating the directory and marking it.
41+
/// Synchronous (a few small filesystem operations) so that there is no await point between
42+
/// creating the directory and marking it. The marker and the directory entry are fsynced: the
43+
/// marker must be durable before anything else in the directory can be, or a power loss could
44+
/// leave a directory with content but no marker, which nothing would dare discard.
4345
pub(crate) fn create_marked_dir(db_path: &Path) -> std::io::Result<()> {
4446
std::fs::create_dir_all(db_path)?;
45-
std::fs::File::create(db_path.join(INCOMPLETE_MARKER))?;
46-
Ok(())
47+
mark_incomplete(db_path)
48+
}
49+
50+
/// Write the marker into an existing directory and make it durable.
51+
pub(crate) fn mark_incomplete(db_path: &Path) -> std::io::Result<()> {
52+
std::fs::File::create(db_path.join(INCOMPLETE_MARKER))?.sync_all()?;
53+
std::fs::File::open(db_path)?.sync_all()
4754
}
4855

4956
/// Remove `db_path` (a namespace directory under `base`): rename it into the discarded area

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -369,11 +369,11 @@ impl NamespaceStore {
369369
}
370370

371371
pub async fn checkpoint(&self, namespace: NamespaceName) -> crate::Result<()> {
372-
let entry = self
373-
.inner
374-
.store
375-
.get_with(namespace.clone(), async { Default::default() })
376-
.await;
372+
// A namespace that is not loaded has nothing to checkpoint. (`get_with(Default)` here
373+
// used to leave an empty entry behind that lookups then mistook for the namespace.)
374+
let Some(entry) = self.inner.store.get(&namespace).await else {
375+
return Ok(());
376+
};
377377
let lock = entry.read().await;
378378
if let Some(ns) = &*lock {
379379
ns.checkpoint().await?;

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@ pub(super) fn file_url(path: &Path) -> String {
4646
pub(super) fn sim() -> Sim<'static> {
4747
Builder::new()
4848
.simulation_duration(Duration::from_secs(1000))
49+
// Dumps are served in many small chunks that the server only starts reading once the
50+
// namespace is set up (fsyncs, file creation — real time, during which turmoil keeps
51+
// advancing simulated time and the dump store keeps writing). Real TCP would apply
52+
// back-pressure; turmoil panics once its socket buffer (64 segments by default) is full.
53+
.tcp_capacity(4096)
4954
.build()
5055
}
5156

0 commit comments

Comments
 (0)