Skip to content

libsql-server: make namespace creation all-or-nothing - #53

Draft
tszymczyszyn-shopify wants to merge 6 commits into
v0.9.30-shopify-patchesfrom
tszymczyszyn/atomic-namespace-create
Draft

tszymczyszyn-shopify wants to merge 6 commits into
v0.9.30-shopify-patchesfrom
tszymczyszyn/atomic-namespace-create

Conversation

@tszymczyszyn-shopify

@tszymczyszyn-shopify tszymczyszyn-shopify commented Oct 9, 2026 •

Copy link
Copy Markdown

Summary

Targets v0.9.30-shopify-patches directly, on top of the merged false-404 fix (#56). It was stacked on the streaming dump importer (#51) until that PR was closed on 2026-10-21; the restack is described under "Rebased onto #56" below. Six commits: db3afbf398 code and tests, 5b77c0e4b2 design doc and admin API, 8d24f2fc84 diagrams, a2f828a3b1 one lookup for every reader, c426293973 more tests, 44afb04010 fixes from the adversarial review (below).

Makes POST /v1/namespaces/:ns/create all-or-nothing. Today NamespaceStore::create persists the namespace config before setting the namespace up and nothing undoes it: a dump that fails to import leaves a ghost namespace (config without data, name unusable, lazily turned into an empty database on first access); a request cancelled mid-import runs no cleanup at all; the schema configurator has no cleanup even on error.

Design: docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md.

The walkthrough is pinned to bb446259f6, from before the rebase onto #56. The flows it shows still hold. with and load_namespace changed as described under "Rebased onto #56" below.

📖 Reviewer walkthrough: before/after and component diagrams, sequence diagrams for the happy path, failure/cancellation and eviction, the lifecycle state machine, a suggested reading order with pinned links, and where reviewer judgment is most valuable.

Contract (new, documented in ADMIN_API.md)

After a non-2xx or a lost connection the namespace is either absent (no config, no directory, name reusable) or complete, never partial. For callers: 2xx ⇒ complete; anything else ⇒ retry; already exists on the retry ⇒ the earlier attempt did complete.

How

The metastore write becomes the commit point; everything before it is reversible.

create(name, restore, config)
├─ Reservation::acquire(name)        lock the cache entry, pin it; reject if loaded or config exists
├─ handle(); store(cfg, flush=false) in memory only; setup can read it, nothing on disk
├─ configurator.discard_incomplete   rename dbs/<name> away iff it carries `.incomplete`
├─ make_namespace(...)               setup: FreshDir::begin → mkdir + marker (fsynced) … import … keep()
├─ handle.flush()                    ◄── COMMIT POINT
├─ reservation.publish(ns)           entry := Some(ns); unpin; lock released
└─ remove .incomplete                best effort; a stale marker is removed on the next load
     Err  ⇒ Reservation::abort (awaited): discard config → discard marked dir → unpin → unlock
     drop ⇒ the same, on the blocking pool (the creating request was cancelled)
  • Reservation generalises the pattern fork already used (write-lock the cache entry, config in memory only, flush on success). The name stays locked until the attempt is published or fully undone, so waiters see the namespace or "doesn't exist", never a half-built one, and a retry issued the moment the error arrives neither races the cleanup nor finds leftovers. fork uses it too; its old guard used block_in_place (panics on a current-thread runtime) and published before flushing.
  • Marker and tombstones (namespace/incomplete.rs): a new directory carries .incomplete until the config is persisted. Directories are never deleted in place; discard_dir renames them into <base>/.discarded/ (instant, frees the path) and deletes in the background; a startup sweep removes leftover tombstones and marked orphans; the metastore's filesystem recovery skips marked directories. Directories without the marker keep today's semantics.
  • MetaStore::discard forgets a namespace unconditionally through the metastore's own change queue (so it is ordered after a config store that was still queued when the request was cancelled); remove's linked-DB guards would otherwise leave a ghost. existing_handle looks a namespace up without registering it; with() and the fork source use it.
  • Cache eviction during an operation: the reservation is the entry's write lock, which moka knows nothing about. The locked entry is pinned in in_flight; load_namespace and destroy consult it first and wait on it; the eviction listener spawns its shutdown only for pinned entries (moka awaits async listeners inline from whichever cache operation runs maintenance); install publishes into a fresh entry when the locked one was evicted, or hands the namespace to destroy when that is why it is gone.
  • Requests addressed to a namespace under creation wait (as during fork/reset). shutdown skips pinned entries; create/fork re-check has_shutdown before the commit point.
  • Needs async-lock = "3" for RwLock::write_arc (already in the lockfile; the bump removes the duplicate 2.x).

Behaviour changes visible to clients

  • Failed create ⇒ GET …/config is 404 and user requests get "namespace doesn't exist" (was: 200 / "no such table").
  • Single-namespace mode: create default from a dump while it exists ⇒ already exists (was: 200 with the dump silently skipped). The config-upsert behaviour of a plain create default is unchanged.
  • A <db-path>/.discarded/ directory appears; it holds renamed directories until the background delete or the startup sweep removes them.

Rebased onto #56 (2026-10-16), restacked on the release branch (2026-10-22)

The false-404 fix (#56) and this PR both rewrote NamespaceStore. This PR now sits on top of #56 and keeps both PRs' guarantees. After the rebase the two retry loops (one in #56's with, one in this PR's load_namespace) were merged into one (f1dc57c91a):

Restack without #51. #51 is closed, so this PR no longer carries or depends on the streaming importer. What changed in the restack: the store tests feed their dumps through a channel as before, but as the base DumpStream into the base importer (which reads the whole stream before executing, so a creation still pauses while the sender is open); the lifecycle integration tests carry their own small helpers (slow dump store, row count, sim() with a larger turmoil socket buffer) instead of importing #51's, and the _streaming variants are gone; the test HTTP client gained post_raw (returns 5xx responses instead of failing), which was #51's; the admin API and design doc no longer describe importer selection. Nothing in the production code changed in the restack. async-lock goes from 2.6 to 3 for read_arc/write_arc, as before.

  • One lookup for every reader. load_namespace(name) is the only way to get a namespace. It returns the namespace read-locked (LoadedNamespace), so nothing can take it out of its entry while the caller uses it. with, fork (for its source) and lazy create make one call each. Each attempt checks for shutdown, looks the name up in the metastore with the non-inserting existing_handle (so a retry can never bring a destroyed name back: Never report a namespace whose entry was emptied as missing #56's follow-up 2), waits on a pinned entry if an operation holds one, otherwise reads the cache or loads. An entry found empty is dropped from the cache if it is still there, and the lookup starts again, up to five times, then gives up with NamespaceUnavailable (503). Only the metastore produces NamespaceDoesntExist.
  • Why five attempts. That is the budget the two nested loops had (3 × 2). In the live eviction check, which runs the cache at a third of the hot set so that entries live for milliseconds, three attempts gave up on about 1 request in 20,000 (19 runs, 490k requests); five gave up on none in 340,000. In a cache that holds its working set, eviction happens at shutdown only and the second attempt succeeds.
  • Empty leftover entries are dropped atomically, with evict_if_still_empty folded in from the closed libsql-server: drop a namespace cache entry that was left empty #59: under moka's key lock, only if the entry is still cached, still empty and unlocked. It replaces the get-then-invalidate pairs in load_namespace and Reservation::acquire, which could evict a fresh entry loaded in between. Drops are counted in libsql_server_namespace_left_empty_entry_dropped. The drop is gated on contains_key, which records no access: after a real eviction the key is already gone, and the compute call behind the drop would only run moka's maintenance in the retrying task (measured: it tripled the give-up rate in the eviction check).
  • Every shutdown check is refuse_if_shut_down, at each entry point and at each commit point. reset gained the commit-point check create and fork had: shutdown does not wait for a reset in progress, so a namespace installed after it would never be shut down.
  • Code from Never report a namespace whose entry was emptied as missing #56 that this design makes redundant was dropped. create's record cleanup on shutdown (the reservation already undoes an interrupted creation) and fork loading its source through with (the lookup reloads an evicted source).
  • Never report a namespace whose entry was emptied as missing #56's follow-ups 2, 3 and 4 and the load-latency metric in 8 are fixed here, and the lock-held-across-a-cache-call part of 5.

Adversarial review (2026-10-22)

An independent correctness review of the whole diff against the release branch (one reviewer, two passes: findings, then the fixes). Six findings, all in code this PR introduced, all fixed in 44afb04010; three residuals from the second pass fixed in the same commit; one accepted. The design doc (§9, second table) has the full list. In short:

  • Cancellation inside publish (moka's insert awaits after putting the entry in the map) could leave the namespace installed while the reservation's cleanup discarded its row and directory: create 400, everything else 404, until eviction. publish now runs in its own task, so past the commit point the creation completes without the request. The contract already said a 400 on retry means the earlier attempt completed; the admin API doc now says it explicitly.
  • The pin was claimed after the lock. In that window the eviction listener waited on the lock inline under moka's key lock, which install's insert needs: a deadlock for that name. After an abort a third create could also reserve a fresh entry for the same name. claim pins before locking, waits for a holder, and releases the pin if the attempt is dropped (PinClaim); reset uses it too.
  • create of an existing name write-locked its entry before "already exists"; async-lock prefers writers, so new readers waited (for a whole fork when the namespace was a fork source). The metastore is checked first.
  • Shutdown re-check after publish and after reset's install; destroy takes both the pinned and the cached entry, and install drops a fresh entry whose record a destroy removed meanwhile; sweep skips files in dbs/; a panic in the publish task is returned, not reported as success.
  • Accepted: the eviction listener reads the pin once when moka creates it, so a claim that starts and finishes within nanoseconds of that read still waits inline.
  • Noted, out of scope (pre-existing): connection/program.rs calls MetaStore::handle for ATTACH, which registers a config for a nonexistent name.

Tests

  • 9 lifecycle integration tests: failed creation leaves no trace (primary and schema configurators), cancelled creation (cancel fires once the marker exists), concurrent requests wait, a create after a failed one succeeds, crash remnant discarded at startup, unmarked directory left alone, marker on a published namespace ignored, single-namespace mode rejects a dump into the existing namespace.

  • 36 store unit tests. They build a real NamespaceStore and cover: a lookup waiting across an eviction (one waiter, and eight sharing one reload); a namespace destroyed while waiting (404, from the metastore); an entry left empty (one waiter, eight waiters, as a fork source, under reset, under destroy); evict_if_still_empty leaving loaded, foreign and locked entries alone; create over a left-empty entry applying its dump; create of an existing unloaded namespace leaving no empty entry; a create and a fork racing for one destination; lookups racing a destroy (20 rounds of 8) leaving no record, directory or filled entry; shutdown: with, the load itself, reset, create refuse, shutdown does not wait for a creation in progress, and a creation or reset interrupted at its commit point leaves nothing; lazy creation (what a replica runs with) reloading an evicted namespace and never answering "doesn't exist"; checkpoint of an unloaded namespace caching nothing; the 503 and 404 mapping and messages; from the review: 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, and (in incomplete.rs) a stray file does not stop the sweep.

  • A 10 s randomized run (#[ignore], cargo nextest run -p libsql-server --run-ignored only -E 'test(random_operations)'): 12 tasks create, fork, look up, checkpoint and evict up to 60 namespaces (about 75,000 operations). No lookup of a created namespace may fail; afterwards every namespace is servable, no unlocked empty entry is cached, nothing is pinned, no marker remains. Six consecutive runs pass. It leaves destroy out and keeps evictions rare; see the findings below for why.

  • Mutation checks (each change applied alone, store tests run):

    removed guard killed by
    shutdown check at the top of each attempt with_after_shutdown_is_unavailable_not_missing
    shutdown check inside the load load_namespace_refuses_to_open_a_namespace_once_the_store_has_shut_down
    existing_handle (inserting handle instead) 4 tests, incl. lookups_racing_a_destroy_do_not_resurrect_the_namespace
    an emptied entry answered as "doesn't exist" (the base's behaviour) 8 tests
    the drop of a left-empty entry, or its contains_key gate inverted 3 tests
    one attempt instead of five 7 tests
    acquire not dropping the entry it created create_of_an_existing_unloaded_namespace_leaves_no_empty_entry
    evict_if_still_empty without the identity check, or without the lock check evict_if_still_empty_drops_only_the_cached_unlocked_empty_entry
    install not checking the metastore destroy_during_creation_destroys_the_result
    shutdown waiting on pinned entries shutdown_does_not_wait_for_a_creation_in_progress
    create's commit-point check create_interrupted_by_shutdown_leaves_no_record
    reset's commit-point check reset_interrupted_by_shutdown_installs_nothing
    lookups ignoring pinned entries 2 tests
    PinClaim not unpinning on drop, or the pin claimed after the lock a_reservation_attempt_dropped_while_waiting_for_the_lock_unpins (+ 2 reset tests)
    create's metastore check before claiming create_of_a_loaded_namespace_does_not_wait_for_its_readers
    sweep not skipping files sweep_cleans_leftovers_of_a_crash

    Six mutations survive, knowingly. From the review fixes: the spawned publish (no pause point inside moka's insert), the shutdown re-check (needs shutdown to finish between two adjacent awaits), and destroy taking both entries (the eviction listener reaches the same end state). From before: fork's commit-point check (a fork cannot be paused deterministically; it is the same two lines as create's), the gave_up counter counting every give-up (no metrics recorder in the tests), and the metastore re-check inside the load (every attempt now re-validates existence before it, so this check only narrows the destroy-vs-load window described in the findings; a test would need a pause inside the load).

  • 30 consecutive clean runs of the store and rpc modules (49 tests after the review fixes), on the restacked tree too. Full cargo nextest run -p libsql-server after the review fixes: 262 tests, 261 passed; the three others (embedded_replica::local::local_sync_with_writes, embedded_replica::replica_no_resync_on_restart, connection_core::test::test_many_concurrent) fail or flake the same way on the base branch and pass alone (nextest: run test_many_concurrent alone #65 isolates the last one).

  • Live checks (libsql-server: live checks for the namespace store #58 harness) after the review fixes: eviction churn, 3 runs: 114,654 × 200, no other answer, 6,357 emptied entries retried, no give-up; SIGTERM under load, 2 runs: no 404; the 60-check matrix (S1 to S9) unchanged. Before the fixes, on the restacked tree: eviction 126,339 × 200, no give-up; SIGTERM no 404. Before the restack, on the same code over libsql-server: memory-bounded streaming dump importer (opt-in, benchmarkable against the buffered one) #51: eviction churn, 12 runs: 342,653 × 200, no other answer, 18,484 emptied entries retried, no give-up (Never report a namespace whose entry was emptied as missing #56 on the same scenario: 4 give-ups in 312,403). SIGTERM under load, two runs: no 404, only 503 Namespace store has shutdown then connection errors. Embedded replicas across a SIGTERM: 9,231 clean syncs, no "doesn't exist". The 70-check matrix against Never report a namespace whose entry was emptied as missing #56 differs only where intended: fork onto an existing unloaded namespace is 400 already exists with the destination intact (on Never report a namespace whose entry was emptied as missing #56, 500 and the destination deleted); every other difference is a counter or an id.

Found while testing, not in scope

These predate this PR, or are design decisions it does not change. Each was reproduced; none is fixed here.

  1. destroy does not hold the name while it removes the directory. A lookup whose entry was evicted may be loading (creating files) while destroy removes the directory: the destroy fails with Directory not empty, and the load can leave a namespace cached for a name the metastore no longer knows, until it is evicted. A create of the name during the cleanup finds the directory still there (File exists; the base would have opened files in a directory being deleted). Reproduced with the randomized run with destroy in the mix. Fix: destroy should take moka's key lock for the name (so it waits for a load in progress) and pin a locked entry while it cleans up, as create does.
  2. A create racing a destroy of the same name can fail with MetaStoreUpdateFailure. destroy drops the config's watch channel, which the creating request is waiting on; the creation is undone. Same cause as 1.
  3. install re-inserts with a plain insert, which moka does not serialize with the compute lock evict_if_still_empty runs under, so a drop decided on an empty entry can remove a fresh one inserted in between. evict_if_still_empty detects it and logs an error; the requests holding the fresh entry see it emptied and look again. No interleaving that reaches it on a primary was found. Fix: use the compute API in install too.
  4. fork holds its destination reserved while it loads the source. A slow source load holds the destination name. No cycle is possible (reservations are only on names that do not exist, loads only on names that do).
  5. A cache too small for its working set answers 503 after five loads. moka's admission policy can evict a freshly loaded entry at once when the cache is full. libsql_server_namespace_emptied_entry_gave_up is the signal.

Pre-production soak

A pre-release build of this PR's code, from before the rebase onto #56, ran on a pre-production deployment through the streaming-importer validation's creates and destroys, with live traffic. It recorded no libsql_server_namespace_create_aborted increments and left no .discarded/ directories. The rebased code has not been deployed yet.

Sequencing and follow-ups

@tszymczyszyn-shopify

tszymczyszyn-shopify commented Oct 13, 2026 •

Copy link
Copy Markdown
Author

Reviewer walkthrough

Pinned to bb446259f6 , from before the stack was rebased onto #56. The flows it shows still hold. with and load_namespace changed as described in the PR body under "Rebased onto #56". Stacked on #51; this PR is the last three commits (code and tests, design doc and admin API, diagrams). Suggested reading order below; ~45 minutes, a little more with the primer.

TL;DR

POST /v1/namespaces/:ns/create used to persist the namespace's config first and set the namespace up afterwards; nothing undid the config on failure and nothing at all ran on cancellation, so a failed import left a ghost (name taken, next request lazily opened an empty database). This PR makes the metastore flush the commit point and makes everything before it reversible — with two drop guards (one per layer), an on-disk marker for crashes, and a pin that keeps the operation safe from cache eviction.

Contract for callers: 2xx ⇒ complete. Anything else, including a lost connection ⇒ retry; already exists on the retry ⇒ the earlier attempt did complete.

Primer: the pieces this PR touches (if you don't know libsql-server's namespace code)

A namespace is one SQLite database served under a name (http://<name>.<host>). Four things cooperate to make one exist:

Piece File What it is
NamespaceStore namespace/store.rs The registry of loaded namespaces: a moka cache Name → Arc<RwLock<Option<Namespace>>>, bounded by --max-active-namespaces (default 100). Evicted namespaces are shut down and lazily reloaded from disk on the next request. with(name, f) is how every user request reaches a namespace; create, fork, reset, destroy are the admin operations.
MetaStore namespace/meta_store.rs Which namespaces exist and their DatabaseConfig: an in-memory map (configs, what exists() reads) backed by a SQLite table (namespace_configs). Writes go through a FIFO change queue processed on the blocking pool. Gotcha this PR works around: handle(name) inserts the name into the in-memory map as a side effect.
Configurators namespace/configurator/{primary,schema,replica,fork}.rs setup(name, RestoreOption) builds a Namespace from dbs/<name>/: directory, .sentinel (dirty-shutdown flag), replication logger (wallog), SQLite connection maker, and — for RestoreOption::Dump — the dump import from #51. cleanup removes a namespace's files.
Admin API http/admin/mod.rs POST /v1/namespaces/:ns/create resolves dump_url into a stream and calls NamespaceStore::create.

Two moka facts matter: (1) try_get_with(key, init) runs init at most once per key and makes concurrent callers wait for it — the base code relies on this to avoid loading a namespace twice; (2) moka 0.12 has no background thread: cache maintenance (including awaiting async eviction listeners) runs inline inside whatever cache operation trips it.

Before this PR, create did: metastore.store(config) (durable) → load_namespace → setup. The order is what this PR inverts.

Before / after

Before and after

Who owns what

Components and ownership

The happy path, with a concurrent lookup

sequenceDiagram
    autonumber
    participant A as Admin API
    participant S as NamespaceStore
    participant M as MetaStore
    participant C as Configurator
    participant FS as Filesystem
    participant U as User request (with foo)

    A->>S: create(foo, Dump, cfg)
    S->>S: Reservation::acquire — get_with entry, write_arc(), reject if Some or exists(foo), pin in in_flight
    S->>M: handle(foo); store(cfg, flush=false)
    Note over M: in memory only — exists(foo) is now true, nothing on disk
    S->>C: discard_incomplete(foo)
    Note over C,FS: removes dbs/foo only if it still carries .incomplete (a crashed earlier attempt)
    S->>C: setup(foo)
    C->>FS: FreshDir::begin — mkdir dbs/foo + .incomplete (sync)
    C->>C: logger, WAL, connections, dump import…
    U-->>S: with(foo): existing_handle → load_namespace
    Note over S,U: in_flight has foo → wait on the pinned entry's read lock (never a lazy load)
    C-->>S: Ok(Namespace)  (FreshDir.keep())
    S->>M: handle.flush()
    Note over M: COMMIT POINT — row written; a row now implies a complete namespace
    S->>S: publish → install(ns) into the locked entry, unpin, unlock
    S->>FS: remove dbs/foo/.incomplete (best effort)
    S-->>A: 2xx
    S-->>U: read lock acquired → Some(ns) → serve
Loading

Failure or cancellation

sequenceDiagram
    autonumber
    participant A as Admin API
    participant S as NamespaceStore
    participant M as MetaStore
    participant C as Configurator
    participant FS as Filesystem
    participant U as User request (with foo)

    A->>S: create(foo, Dump, cfg)
    S->>S: Reservation::acquire (lock + pin)
    S->>M: store(cfg, flush=false)
    S->>C: setup(foo)
    C->>FS: mkdir dbs/foo + .incomplete
    U-->>S: with(foo) → waits on the pinned entry
    alt import fails
        C->>FS: FreshDir::drop — rename dbs/foo → .discarded/foo-<uuid> (sync), delete in background
        C-->>S: Err
        S->>S: Reservation::abort() — awaited
    else client disconnects (future dropped)
        Note over C,S: FreshDir and Reservation are dropped during teardown
        C->>FS: FreshDir::drop — rename, same as above
        S->>S: Reservation::drop — cleanup handed to the blocking pool
    end
    S->>M: Discard(foo) via the change queue (unconditional, ordered after any queued store)
    S->>C: discard_incomplete(foo) — no-op if FreshDir already renamed it
    S->>S: unpin, then unlock
    S-->>A: error (only after the cleanup, on the Err path)
    S-->>U: read lock acquired → None; exists(foo) false → NamespaceDoesntExist
    Note over A: a retry now finds a free name and a free path
Loading

Lifecycle of a name, including crashes

stateDiagram-v2
    [*] --> Absent
    Absent --> Reserved: acquire (entry locked + pinned, config in memory)
    Reserved --> SettingUp: FreshDir::begin — dbs/foo + .incomplete
    SettingUp --> Committed: flush — row written
    Committed --> Published: install, unpin, unlock
    Published --> Complete: .incomplete removed
    Complete --> [*]

    Reserved --> Discarding: Err / drop
    SettingUp --> Discarding: Err / drop — dir renamed to .discarded/
    Committed --> Discarding: drop before publish — Discard deletes the row again
    Discarding --> Absent: unpin, unlock

    SettingUp --> MarkedOrphan: process dies
    Committed --> Published: process dies here — row + marker; serves after restart, marker dropped on load
    MarkedOrphan --> Absent: sweep at startup, or next create of the name (discard_incomplete)
    Published --> Complete: process dies before unmark — stale marker removed on next load

    note right of Discarding
        Order matters: config → directory → unpin → unlock.
        The name is unobservable until it is clean.
    end note
    note right of MarkedOrphan
        No row, so exists(foo) is false: invisible to clients.
        The marker is what tells it apart from a real namespace.
    end note
Loading

Eviction during an operation (commit 3)

sequenceDiagram
    autonumber
    participant S as NamespaceStore (create foo)
    participant K as moka cache
    participant L as eviction listener
    participant U as with(foo)

    S->>K: get_with(foo) → entry E; write_arc(E); pin E in in_flight
    Note over S: import running for minutes…
    K->>K: capacity pressure — evicts E
    K->>L: notify(foo, E)
    alt foo is pinned
        L->>L: spawn the shutdown (don't await E's lock inline — moka awaits listeners from arbitrary cache ops)
    else not pinned (a normal eviction)
        L->>L: await E.write() and shut down inline (serialises with a reload of the same name)
    end
    U->>S: load_namespace(foo): in_flight has foo → wait on E (not on the cache)
    S->>K: install: store.get(foo) ≠ E and exists(foo) → insert fresh entry F with Some(ns)
    Note over S: E stays empty — the spawned listener finds nothing to shut down
    S->>S: unpin, unlock
    U->>S: E is None → exists(foo) true → try_get_with → F → serve
Loading

If foo was destroyed meanwhile instead of evicted, install sees exists(foo) == false and hands the namespace to E, where destroy is waiting for it ("created, then destroyed").

Suggested reading order

1. The on-disk protocol (small, self-contained)

  • namespace/incomplete.rs: INCOMPLETE_MARKER, create_marked_dir/mark_incomplete (fsynced), discard_dir (rename into .discarded/, background delete), discard_if_incomplete, sweep — with unit tests at the bottom (tombstone with an open file, sweep, FS recovery skipping marked dirs).
  • FreshDir: the configurator's guard; synchronous begin, Drop ⇒ discard_dir. Used by primary and schema setup; fork marks its temp dir.
  • Startup sweep call in lib.rs.

2. The reservation (the heart of the change)

  • Reservation: acquire (prefers a pinned entry; invalidates the empty entry it created when rejecting), publish, abort (awaited on error paths), Drop (cancellation only), take_cleanup with the config → directory → unpin → unlock order.
  • create / create_reserved and fork / fork_reserved: same shape; has_shutdown re-checked before the commit; publish before unmark.
  • MetaStore: ChangeMsg::{Update, Discard}; discard is unconditional (unlike remove, whose linked-DB guards would leave a ghost); existing_handle is the non-registering lookup; the flush branch of process no longer holds configs across try_process (lock order vs remove).

3. Eviction safety

4. Tests

  • Store unit tests (store.rs::test): a real NamespaceStore with a channel-fed streaming import — lookup waits across an eviction; lookup sees failure across an eviction; destroy during creation; retry the instant the error returns (no polling); abort mid-import (JoinHandle::abort); discard vs remove's guards.
  • Integration (tests/namespaces/lifecycle.rs, dumps.rs cancel tests): failed create leaves no trace (both importers + schema configurator), cancelled create (cancel fires only once dbs/foo/.incomplete exists), concurrent requests wait, crash remnant discarded, unmarked dir untouched, marker on a published namespace ignored (creates before any lookup after a restart), single-namespace dump create rejected.
  • 9 of the original 11 integration tests fail on the unfixed code; the two that pass pin behaviour that must hold before and after.

Behaviour changes visible to clients

  • Failed create ⇒ GET …/config is 404 and users get "doesn't exist" (was: 200 / "no such table").
  • Single-namespace mode: create default from a dump while it exists ⇒ already exists (was: 200, dump silently skipped). Plain create default stays a config upsert.
  • <db-path>/.discarded/ appears; it holds renamed directories until the background delete or the startup sweep removes them.

Where reviewer judgment is most valuable

  1. The cleanup order in Reservation::take_cleanup — is there any interleaving where the name becomes observable before it is clean?
  2. install's three-way decision (same entry / destroyed / evicted) and load_namespace's empty-entry healing: do they cover every way an entry can go stale?
  3. MetaStore::discard bypassing remove's linked-DB guards — correct for an unpublished namespace; is there any path where it runs for a published one? (It only runs from a Reservation, whose acquire rejects existing names.)
  4. The pin-aware eviction listener: inline for normal evictions, spawned for pinned — any entry that is long-locked but not pinned?
  5. Whether the tombstone directory and startup sweep are acceptable operationally (disk use is bounded by the background delete; sweep logs each leftover).

Explicitly out of scope

  • Bottomless: not used in our deployment; this change is untested with it.
  • Two accepted residues (design doc §9): an empty cache entry after an abort (never served; healed on the next lookup or evicted), and a metastore rebuilt from the filesystem in the milliseconds between a namespace's flush and its unmark.
  • An import timeout; idempotency keys / async create jobs.

Full rationale, failure matrix and the review table: docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md.

@tszymczyszyn-shopify

Copy link
Copy Markdown
Author

Closing for now. Database moves are run by an operator, who deletes a namespace left behind by a failed import, so all-or-nothing creation is not needed yet. The branch is kept at bb446259f6. Reopen if partial namespaces from failed imports become a recurring problem. #51 continues on its own.

tszymczyszyn-shopify added a commit that referenced this pull request Oct 14, 2026
The design doc pointed to internal tracking for the pre-existing
namespace lifecycle issues. Say instead that they are not fixed here
and that #53, which proposed all-or-nothing creation, is closed for now.
@tszymczyszyn-shopify

Copy link
Copy Markdown
Author

Reopening. Beyond database moves, this fixes ghost namespaces after failed or cancelled creates, fork publishing before its config is saved, and the cache-eviction window during long operations. It is not planned for the next patch release; it will be reviewed on its own after that.

tszymczyszyn-shopify added a commit that referenced this pull request Oct 15, 2026
Four real catalog databases (0.45–7.27 GB) imported on the staging fleet
with a release build: peak RSS +11…+38 MB across the range, every target's
dump byte-identical to its input, integrity_check ok, 2.7x faster than the
buffered importer on the same file. All four real dumps contain the word
"attach" in product text and are rejected by the buffered importer; its
control run needed a copy with the substring neutralised.

Also: #53 was reopened, so section 13 no longer calls it closed.
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/atomic-namespace-create branch from bb44625 to 28defca Compare October 15, 2026 13:50
tszymczyszyn-shopify added a commit that referenced this pull request Oct 16, 2026
The design doc pointed to internal tracking for the pre-existing
namespace lifecycle issues. Say instead that they are not fixed here
and that #53, which proposed all-or-nothing creation, is closed for now.
tszymczyszyn-shopify added a commit that referenced this pull request Oct 16, 2026
Four real catalog databases (0.45–7.27 GB) imported on the staging fleet
with a release build: peak RSS +11…+38 MB across the range, every target's
dump byte-identical to its input, integrity_check ok, 2.7x faster than the
buffered importer on the same file. All four real dumps contain the word
"attach" in product text and are rejected by the buffered importer; its
control run needed a copy with the substring neutralised.

Also: #53 was reopened, so section 13 no longer calls it closed.
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/streaming-dump-importer branch from b61f5d2 to 0a4acf9 Compare October 16, 2026 11:33
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/atomic-namespace-create branch from 28defca to 1369b8c Compare October 16, 2026 11:33
@tszymczyszyn-shopify
tszymczyszyn-shopify removed this pull request from stack #54 October 16, 2026 11:35
@tszymczyszyn-shopify
tszymczyszyn-shopify added this pull request to stack #61 October 16, 2026 11:35
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/atomic-namespace-create branch from 1369b8c to f4772df Compare October 16, 2026 12:23
tszymczyszyn-shopify added a commit that referenced this pull request Oct 16, 2026
The design doc described how the reader hands statements to the executor,
but not why the queue is bounded or why it has both a message limit and a
byte limit. Add that to the architecture section, and replace the
"< 32 MiB" memory estimate with "tens of MiB". Also note that #53 is
stacked on this change instead of closed.
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/streaming-dump-importer branch from 95d2b70 to 820aa86 Compare October 16, 2026 12:30
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/atomic-namespace-create branch 4 times, most recently from 3415426 to 59f81ca Compare October 20, 2026 09:03
`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`.
Adds docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md (design, failure matrix,
concurrency analysis, the eviction mechanism, and what the adversarial
review changed), states the all-or-nothing creation contract and the
single-namespace-mode behaviour in ADMIN_API.md, and marks the lifecycle
items of the streaming dump import design (section 13) as fixed.
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/atomic-namespace-create branch from 59f81ca to 385093c Compare October 22, 2026 14:53
@tszymczyszyn-shopify
tszymczyszyn-shopify removed this pull request from stack #61 October 22, 2026 14:53
@tszymczyszyn-shopify tszymczyszyn-shopify changed the title libsql-server: make namespace creation all-or-nothing (stacked on #51) libsql-server: make namespace creation all-or-nothing Oct 22, 2026
@tszymczyszyn-shopify
tszymczyszyn-shopify changed the base branch from tszymczyszyn/streaming-dump-importer to v0.9.30-shopify-patches October 22, 2026 14:53
`load_namespace` now returns the namespace read-locked (`LoadedNamespace`)
instead of its cache entry, and owns the whole lookup: the metastore check
(`existing_handle`, so that no lookup can bring a destroyed name back), the
cache lookup or load, and the retry. When the entry it found is empty it
drops it from the cache (if it is still there) and looks again, up to five
times, then gives up with the 503 or the metastore-backed 404. `with` and
`fork` make one call each; the second retry loop in `with`, the two rounds
in `load_namespace` and `fork`'s own "source emptied" arm are gone, and an
emptied entry is counted once.

Five lookups keep the budget the two nested loops had. In the live
eviction check (cache at a third of the hot set) three lookups gave up on
about one request in 20,000; five on none in 340,000. The drop of an
emptied entry is gated on `contains_key`, which records no access: after a
real eviction the key is gone, and the compute call behind the drop would
only run moka's maintenance in the retrying task.

This also makes the one mutation the unit tests could not see before
visible: an emptied entry answered as "doesn't exist" now fails seven
tests, because the read happens inside the retry.

`reset` refuses to install its result once shutdown has begun, like
`create` and `fork` refuse to commit: `shutdown` does not wait for a reset
in progress, so a namespace installed then would never be shut down. The
ten shutdown checks share `refuse_if_shut_down`.

New tests: `reset_interrupted_by_shutdown_installs_nothing`,
`shutdown_does_not_wait_for_a_creation_in_progress`.
- lookups racing a destroy do not resurrect the namespace (no record,
  directory or filled entry afterwards; 20 rounds of 8 lookups);
- reset and destroy of a namespace whose entry was left empty;
- a create and a fork racing for one destination: the fork waits for
  the create's reservation, then is rejected, and leaves nothing behind;
- lazy creation (what a replica runs with): an evicted namespace is
  reloaded and giving up is "unavailable", never "doesn't exist";
- a 10 s randomized run (`#[ignore]`): 12 tasks create, fork, look up,
  checkpoint and evict up to 60 namespaces; no lookup of a created
  namespace may fail, and afterwards every namespace is servable, no
  unlocked empty entry is cached, nothing is pinned, no marker remains.

The randomized run leaves `destroy` out: a lookup whose entry was evicted
can be loading while `destroy` removes the directory, which fails the
destroy with `Directory not empty` and can leave a loaded namespace cached
for a name the metastore no longer knows. That predates this change. It
also keeps evictions rare: a lookup whose entry is emptied again after
each of its three loads gives up with `NamespaceUnavailable` by design.

`make_store` takes the lazy-creation flag and the cache capacity.
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.
@tszymczyszyn-shopify
tszymczyszyn-shopify force-pushed the tszymczyszyn/atomic-namespace-create branch from 385093c to 44afb04 Compare October 22, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant