Repository navigation
libsql-server: drop a namespace cache entry that was left empty - #59
Closed
tszymczyszyn-shopify wants to merge 1 commit into
Closed
tszymczyszyn-shopify wants to merge 1 commit into
tszymczyszyn-shopify wants to merge 1 commit into
Conversation
…reloads it After the `checkpoint` fix, one thing still leaves an entry cached but empty: a fork that fails before it stores its record. `with` then finds the same empty entry on every attempt and gives up with `NamespaceUnavailable`, and a `create` of that name gets the empty entry back from the cache and returns without loading anything, so its restore option (a dump, say) is silently dropped. Either way the name answers 503 until the entry is evicted, which never happens while it is being asked for, or the server restarts. `evict_if_still_empty` removes such an entry under moka's key lock, only if it is still the cached entry, still empty and nobody holds its lock. It never fills an entry itself: the next lookup loads the namespace into a fresh one. `with` calls it after it found an emptied entry (for an evicted or destroyed entry, which is no longer cached, it does nothing), and `create` before it loads. Two windows stay open and are documented on the function: a `fork` or `reset` that holds a fresh entry but has not locked it yet can have that entry dropped under it and then fill one the cache no longer holds, and `Op::Remove` removes whatever the cache holds when it runs. Closing them needs the entry state to live in the cache value rather than behind the lock, which is the cache entry redesign this change is meant to be reconciled with. The `libsql_server_namespace_left_empty_entry_dropped` counter records each drop; a rising rate points at whatever leaves entries empty.
tszymczyszyn-shopify
added this pull request to stack #60
October 16, 2026 08:44
tszymczyszyn-shopify
added a commit
that referenced
this pull request
Oct 16, 2026
`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`.
Author
|
Closing: folded into #53 (
This PR's tests moved to #53 as well. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #56 at review, so that the hotfix carries only what the incident needs. Stacked on #56's branch; retarget to
v0.9.30-shopify-patchesonce #56 merges.Problem
After #56, one thing still leaves a namespace cache entry cached but empty: a
forkthat fails before it stores its record. Then:withfinds the same empty entry on every attempt and gives up withNamespaceUnavailable(503).createof that name gets the empty entry back from the cache and returnsOkwithout loading anything, so its restore option (a dump, say) is silently dropped.The name answers 503 until the entry is evicted, which never happens while it is being asked for (every access resets the idle timer), or until the server restarts. On the base branch the same sequence answers 404 instead.
Change
evict_if_still_emptyremoves such an entry under moka's key lock, only if it is still the cached entry, still empty and nobody holds its lock. It never fills an entry itself: the next lookup loads the namespace into a fresh one.withcalls it after it found an emptied entry. For an evicted or destroyed entry, which is no longer cached, it does nothing.createcalls it before it loads, so its restore option is applied.libsql_server_namespace_left_empty_entry_dropped, one per drop, and awarnlog.Known windows (documented on the function)
try_readcannot see aforkorresetthat holds a fresh entry but has not locked it yet. That window is not a few instructions: moka'sget_withmakes the entry visible and may then run housekeeping, awaiting eviction listeners, before it returns. Aforkorresetwhose entry is dropped in that window fills an entry the cache no longer holds, which nothing shuts down. Acreateracing aforkof the same name is the realistic way in.Op::Removeremoves whatever the cache holds when it runs; moka does not tie it to the entry the closure saw. Detected and logged at error level.Closing both needs the entry state (
Filling | Loaded | Gone) to live in the cache value rather than behind the lock, which #53 explores. This change should be reconciled with that redesign rather than merged as is; it is kept here so that the code and its tests are not lost.Tests
Six tests in
namespace::store::test: a request that finds a left-empty entry loads the namespace into a fresh entry and does not fill the old one; eight requests waiting on one left-empty entry share one reloaded namespace;evict_if_still_emptyleaves a loaded entry, a different entry and a locked entry alone and drops an unlocked empty one;createover a left-empty entry loads the namespace;forkfrom a left-empty source reloads it. The store tests passed 10 consecutive runs.