Skip to content

libsql-server: make namespace creation all-or-nothing (stacked on #51) - #53

Draft
tszymczyszyn-shopify wants to merge 2 commits into
tszymczyszyn/streaming-dump-importerfrom
tszymczyszyn/atomic-namespace-create
Draft

tszymczyszyn-shopify wants to merge 2 commits into
tszymczyszyn/streaming-dump-importerfrom
tszymczyszyn/atomic-namespace-create

Conversation

@tszymczyszyn-shopify

Copy link
Copy Markdown

Summary

Stacked on #51 (targets tszymczyszyn/streaming-dump-importer; will be retargeted to v0.9.30-shopify-patches once #51 merges — only the last two commits are this PR).

Makes POST /v1/namespaces/:ns/create all-or-nothing. Fixes the pre-existing lifecycle gap called out in #51's design doc §13: a dump that failed to import left a ghost namespace (config without data, name unusable, lazily turned into an empty DB on first access); a request cancelled mid-import ran no cleanup at all; the schema configurator had no cleanup even on error.

Design: docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md.

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 cache entry; reject if loaded or config exists
├─ handle(); store(cfg, flush=false) in-memory only — setup can read it, nothing on disk
├─ configurator.discard_incomplete   remove dbs/<name> iff it carries `.incomplete`
├─ make_namespace(...)               setup: FreshDir::begin → mkdir + marker … import … keep()
├─ handle.flush()                    ◄── COMMIT POINT
├─ ns.mark_complete()                remove marker (best effort)
└─ reservation.publish(ns)           entry := Some(ns); lock released
     Err/drop above   ⇒ Reservation::drop ⇒ remove in-memory config, THEN release lock
     Err/drop in setup ⇒ FreshDir::drop   ⇒ remove_dir_all
  • Reservation generalises the pattern fork already used (write-lock the cache entry, config in memory only, flush on success). Dropping it unpublished — error or cancellation — removes the config on the blocking pool and releases the lock only afterwards, so waiters see "doesn't exist", never a half-built namespace. fork now uses it too; its old guard used block_in_place (panics on a current-thread runtime) and published before flushing.
  • FreshDir removes a brand-new directory on drop unless keep()-ed; primary and schema configurators.
  • .incomplete marker for crashes: written with the directory, removed after the flush; consulted in exactly one place (create, for a name with no config and nothing loaded) so it can never wipe a published namespace; maybe_recover_from_fs skips marked dirs. Marker-less directories keep today's semantics.
  • Requests addressed to a namespace under creation wait (as during fork/reset).
  • 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.

Tests

  • 9 new lifecycle tests + 2 tightened cancellation tests (both importers); 9 of the 11 fail on the unfixed code, the other two pin behaviour that must hold before and after (requests wait; unmarked dirs untouched).
  • Unit tests for FreshDir (incl. drop of the enclosing future) and discard_incomplete.
  • Full libsql-server suite under nextest: identical results to the base branch (local_sync_with_writes and the wall-clock-ratio replica_no_resync_on_restart fail under parallel load on both; both pass alone).

Known limitation (inherited, documented)

If moka evicts the locked cache entry during a multi-minute import, a concurrent request can lazily open the directory being written. reset/fork have the same exposure today; the fix is an in-flight-operations map outside the cache — separate change.

`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:

- `Reservation` (store) write-locks the namespace's cache entry for the
  duration of the creation and keeps the config in memory only. `publish`
  installs the finished namespace; dropping the reservation unpublished
  (error or cancellation) removes the in-memory config and only then releases
  the lock, so waiters observe a namespace that does not exist, never a
  partial one. `fork` uses it too, replacing its ad-hoc guard, which blocked
  in place (panics on a current-thread runtime) and published before flushing.
- `FreshDir` (configurators) removes a brand-new namespace directory when
  setup fails or its future is dropped; used by the primary and schema
  configurators.
- An `.incomplete` marker is written into a new directory and removed once
  the config is persisted. A creation that finds a marked directory for a
  name the metastore doesn't know discards it, so a process crash during an
  import never blocks a retry; the metastore's filesystem recovery skips
  marked directories. Directories without the marker are left alone.

Requests for a namespace that is being created wait for the outcome, as
during `fork` and `reset`. In single-namespace mode `create` of the default
namespace remains a config upsert, but creating it *from a dump* while it
exists is now rejected instead of silently skipping the import.

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.

Requires async-lock 3 for `RwLock::write_arc` (already in the lockfile; the
bump removes the duplicate 2.x version).

Tests: 9 lifecycle tests (failed/cancelled creation leaves no trace for both
importers and for shared-schema namespaces, concurrent requests wait, crash
remnants are discarded, unmarked directories and published namespaces are
untouched, single-namespace dump create is rejected) plus unit tests for the
guards. Nine of the eleven integration tests fail on the previous code.
Adds docs/ATOMIC_NAMESPACE_CREATE_DESIGN.md, states the all-or-nothing
creation contract in ADMIN_API.md, and marks the lifecycle items of the
streaming dump import design (section 13) as fixed.
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