Repository navigation
Never report a namespace whose entry was emptied as missing - #56
Open
tszymczyszyn-shopify wants to merge 17 commits into
Open
tszymczyszyn-shopify wants to merge 17 commits into
tszymczyszyn-shopify wants to merge 17 commits into
Conversation
NamespaceStore::with mapped an emptied cache entry to NamespaceDoesntExist. Entries are emptied by shutdown (which runs while HTTP and gRPC services still accept requests), by the cache's eviction listener, and by destroy. A request that got the entry before it was emptied answered 404 "namespace doesn't exist" for a namespace the metastore still has. Clients cannot tell that from a namespace that is really gone, and a 404 is the one answer a client may act on as authoritative (for example by creating the namespace again or discarding local state). with now: - answers NamespaceStoreShutdown (503) once the store is shut down, on every path, so a request after shutdown no longer reloads the namespace; - looks the namespace up again when the entry it got was emptied, so an evicted namespace is reloaded instead of reported missing; - drops an emptied entry that is still the cached one, under moka's key lock and only if it is still cached, empty and unlocked, so the next lookup loads the namespace; it never fills an entry itself; - answers NamespaceDoesntExist only when the metastore no longer has the namespace, and the new NamespaceUnavailable (503) otherwise. Also: - fork loads its source through with. create refuses once the store is shut down, and drops a left-empty entry before loading, so its restore option is applied. checkpoint no longer caches an empty entry for a namespace that is not loaded. - gRPC: rpc::namespace_error_status maps both 503 errors to UNAVAILABLE in replication auth, ReplicationLogService and ProxyService, instead of INTERNAL, and neither auth path falls back to another strategy for them. - libsql_server_namespace_emptied_entry counts emptied entries a request meets; looking again, dropping a left-empty entry and giving up are logged at warn. Related upstream issue: tursodatabase#1772.
tszymczyszyn-shopify
force-pushed
the
tszymczyszyn/no-false-namespace-not-found
branch
from
October 14, 2026 20:06
4adb7a4 to
2d1c5aa
Compare
No test reached the final `Err` of `with`, or the `else` in `fork` that answers for a source emptied between its two loads. Every test exited the loop earlier: from the shutdown check, from the metastore check, or with `Ok`. Putting the original `NamespaceDoesntExist` back on either path passed every test. Split the loop into `with_attempts(namespace, max_attempts, f)`; `with` passes `MAX_EMPTIED_ENTRY_ATTEMPTS`. With one attempt, the emptied entry a request finds is the last one it looks at, so the tail is the only exit. Two tests pin it: `NamespaceUnavailable` while the metastore still has the namespace, `NamespaceDoesntExist` once it does not. For `fork`, holding the destination entry's write lock stops it between loading the source through `with` and reading it from its own lookup. Emptying the source there reaches the `else`, which must answer `NamespaceUnavailable`. `start_waiting_request` is now a thin wrapper over `start_waiting`, which takes the future to run, so these tests reuse the same rendezvous.
For an entry that was left empty, `with` used all three attempts: one to find it empty, one to recognise it as still cached and drop it, one to load the namespace. There was no attempt left for the fresh entry, so if that was emptied as well (the cache rejecting a new entry at capacity, for example) the request got a 503 that one more lookup would have avoided. Count only lookups that found an emptied entry. The drop branch clears `emptied`, so it cannot run twice without such a lookup in between, and the loop still ends after at most twice the budget. The new test gives `with_attempts` a budget of two over a left-empty entry. With the old counting the budget was gone after the drop.
`with`, `create` and `fork` check `has_shutdown` before they load, but the load itself awaits the metastore and then opens the namespace. A request that passed the check just before shutdown started could open a namespace after `shutdown()` had taken out every cached entry. Nothing would ever shut that namespace down. `reset`, which only replicas call, had no check at all and fills an entry of its own. Check the flag again inside `load_namespace`'s init, which only runs on a cache miss, so one check covers every caller. The error comes back as `Error::Ref`, which the HTTP and gRPC mappings and both auth paths unwrap with `as_ref`. Add the same check at the top of `reset`. A load that was already opening the namespace when shutdown started can still finish. Closing that needs the shutdown-ordering follow-up: stop accepting requests, drain, then take the namespaces out.
`ReplicaProxyService::do_auth` sent `NamespaceStoreShutdown` and `NamespaceUnavailable` to its generic arm, which answers `INTERNAL`. Before, a replica shutting down produced a false `NamespaceDoesntExist` there, and the request fell back to the server-wide auth strategy. So this change had altered what clients of a restarting replica see, from a wrong fallback to an error they do not retry, while primaries answer `UNAVAILABLE` for the same condition. Map the two errors through `namespace_error_status`, as the primary's proxy and replication services do. Neither falls back to the user strategy.
`libsql_server_namespace_emptied_entry` rises on every deploy, because every request that was waiting while shutdown took its namespace out looks again and succeeds or gets a 503 from the shutdown check. The two outcomes an operator would alert on had no metric: a request answered with an error because every lookup found an emptied entry, and a cache entry dropped because something left it empty. Both were log lines only. Add `libsql_server_namespace_emptied_entry_gave_up`, incremented where `with` gives up, and `libsql_server_namespace_left_empty_entry_dropped`, incremented when `evict_if_still_empty` removed the entry (a `Nop` is not counted). Neither has a label.
The test for a namespace destroyed while a request waited removed the metastore record and emptied the entry by hand. The contract it checks, that the request answers "missing" because the record is gone, depends on `destroy` removing the record before it takes the namespace out. Swapping those two steps in `destroy` failed no test. Run the real `destroy` in a task. It removes the record, then waits for the entry's write lock the test holds. The test waits for the record to go, then takes the namespace out itself and drops the lock, so the request finds the entry empty whichever of the two the lock wakes first. Letting `destroy` take the namespace instead would make the outcome depend on that order. With the steps swapped in `destroy`, the wait for the record times out.
The eviction test and both fork tests only compared the namespace's name, which any instance has, including one opened outside the cache or one filled into the emptied entry. Only the left-empty test checked the cache afterwards. After each reload, assert that the cache holds a different entry, that it holds a namespace, and that the emptied entry is still empty.
Every test had one request on the emptied entry, so `evict_if_still_empty` ran alone and in a fixed order. A rolling restart or an eviction empties an entry under all the requests waiting on it at once, and the identity and lock checks exist for that case. Eight requests wait on one entry that is then left empty. Each must succeed, and all of them must be served by the one namespace the cache holds afterwards, compared by the pointer of its `Stats`. The test runs on the multi-thread runtime so the requests really interleave. 50 consecutive runs passed.
- The doc comment on `evict_if_still_empty` promised that it "cannot open a namespace that the cache no longer holds". It cannot see a `fork` or `reset` that holds the entry but has not locked it yet, and moka's `Op::Remove` removes whatever the cache holds when it runs; its compute lock does not exclude `try_get_with` or `invalidate`. Say so, and log an error when the removed entry is not the one the closure saw. - The retry log fires once per request that was waiting when shutdown or eviction took the namespace. That is expected and the counter carries the signal; log it at debug. The drop and give-up logs stay at warn. - The eviction listener logged "deallocated" before checking whether the entry held anything, so dropping a left-empty entry logged a deallocation that did not happen. - Note the known hazard in `fork` and `reset`: an entry write lock held across a cache call, which moka's inline maintenance can wait for. The fix (reserving the name outside the cache) is a follow-up.
`ProxyService::extract_context` authenticates a request for a namespace reported as missing from `x-proxy-authorization`, a header the client writes. Before this change, a false "missing" during shutdown or eviction let a forged `FullAccess` through for a namespace that has its own keys. The test for the 503 answer sent no header, so a change that brought the fallback back for the two 503 errors would have passed it as long as the request then failed for some other reason. Send the header: with the fallback, `extract_context` returns `Ok`, and the test fails.
- `settle`, `wait_until` and `wait_until_async` spun on `yield_now` for up to ten seconds. That burns a core while the awaited event runs on a blocking thread, as `destroy`'s metastore removal does. Sleep a millisecond between checks instead. - The fork tests ran on the current-thread runtime. `fork`'s cleanup on failure calls `block_in_place`, which panics there, so a regression in `fork` would have surfaced as that panic rather than the real error. Run them on the multi-thread runtime. - Note that the reference counting in `settle` and `start_waiting` is what moka 0.12.8 does, and that an upgrade fails these tests with a timeout rather than hanging them.
The loop in `with` tracked the emptied entry it had found so that, when the next lookup returned the same entry, it could drop that entry from the cache (`evict_if_still_empty`) and reload the namespace. That handled an entry that something left cached but empty, which after the `checkpoint` fix only a failed fork does, and it was the most delicate code in this change: a compute under moka's key lock, with windows it could not close on its own. It is removed here and kept for a separate change, to be reconciled with the cache entry redesign. What remains is a plain loop: look the namespace up, serve it if its entry holds it, otherwise count the emptied entry and look again, up to three times. An entry that stays empty is found on every attempt, so the request gives up with `NamespaceUnavailable` and the namespace answers 503 until the entry is evicted or the server restarts, as `with_gives_up_with_unavailable_when_the_entry_stays_empty` pins. The give-up counter now records only that outcome: giving up because the store shut down, or because the namespace is really gone, is not a failure of the store. Since the give-up path is reachable through `with` itself now, the `with_attempts` test seam and the `left_empty_entry_dropped` counter go too. The fork test that said "emptied while waiting" left the entry cached, which is the stays-empty case; it now evicts the entry as the `with` test does. Also corrects the comment in `reset`: `make_namespace` does reach the cache on a replica, through `with` for a shared schema during the replication handshake. That hazard predates this change.
`create` stores the namespace's record, then loads the namespace. Since `load_namespace` refuses to open a namespace once the store has shut down, a shutdown that starts between the two left the record behind with nothing loaded. Every retry then answered "already exists", and the restore option (a dump, say) was never applied. Before this change the same window loaded the namespace, into an entry nothing would shut down. The refusal happens before anything is opened or written, so `create` now removes the record again when its load is refused for that reason. Other load failures are left as they were.
The three gRPC authentication paths each matched the store's error to decide between "missing namespace" (fall back to the user strategy, or skip authentication) and "exists but cannot be served" (UNAVAILABLE, no fallback). Each unwrapped `Error::Ref` on its own, and the replica proxy did not, so a wrapped `NamespaceDoesntExist` would have been INTERNAL there. They now decide on the code `namespace_error_status` returns, which is the one place that classifies these errors and the one that is tested with wrapped errors. `Error::as_ref` unwraps nested `Ref`s, as `into_response` already did. The Hello test for a shut-down store now configures Basic auth and sends no credentials, so that it fails if the UNAVAILABLE case ever falls back to the user strategy.
tszymczyszyn-shopify
added this pull request to stack #60
October 16, 2026 08:44
`with_gives_up_with_unavailable_when_the_entry_stays_empty` becomes `with_reports_unavailable_not_missing_for_an_entry_left_empty`: the same form as its neighbours, the PR's term for the case, and the contract it pins (a namespace that exists must not be reported missing).
…ever read like 404s Eight requests waiting on one entry that is then evicted all look the namespace up again at once; `try_get_with` coalesces them into one load and one fresh entry. Before, nothing asserted that on the eviction path. Clients tell a missing namespace from a temporarily unavailable one by the status, and some also by the message. The two 503 messages must not contain "doesn't exist" or "not found".
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`.
tszymczyszyn-shopify
removed this pull request from stack #60
October 16, 2026 11:35
tszymczyszyn-shopify
added this pull request to stack #61
October 16, 2026 11:35
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. Replace the "< 32 MiB" estimate in the memory model with the values measured on staging, and name the build the staging run used now that the stack sits on #56.
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`.
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`.
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
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.
Problem
When libsql-server shuts down, or drops a namespace from its in-memory cache, a request that is already waiting for that namespace can be told that the namespace doesn't exist: HTTP 404, or gRPC
FAILED_PRECONDITION"namespace doesn't exist". The namespace is fine. It is still on disk and in the metastore. The server only stopped holding it in memory while the request was waiting.Why it happens. Each loaded namespace sits in a cache entry. Three things take the namespace out of its entry: shutdown, cache eviction and
destroy. A request that picked up the entry just before that finds it empty, andNamespaceStore::withreports an empty entry as "doesn't exist". Only afterdestroyis that true.When it happens. On every rolling restart, because the server tears the namespace store down while it is still accepting requests. Under cache pressure it can also happen at any time. In our production deployment it showed up as several hundred
Namespace `<name>` doesn't existerrors in one week of rolling restarts and node drains, all for namespaces that existed. Against a local server, the harness in #58 reproduces it at will: with the cache capacity below the number of hot namespaces, 5.9% of answers were false 404s; a SIGTERM under load produced 13 to 41 of them each time.Why it matters. A 404 tells the client the namespace is gone, and clients act on that, for example by creating it again or discarding local state. A temporary error would have been retried. Embedded replicas built on the
libsqlcrate are an example: their replicator retries connection andUNAVAILABLEerrors on its own, but passes "namespace doesn't exist" straight up to the application as async()failure.Two smaller problems in the same code:
Upstream tracks the shutdown case as tursodatabase#1772.
Change
withNamespaceStoreShutdown(503) once the store has shut down, on every path.NamespaceDoesntExistonly if the metastore no longer has the namespace. Otherwise it answersNamespaceStoreShutdown, or the newNamespaceUnavailable(503).load_namespacecheckshas_shutdownagain inside its init, which runs on a cache miss only, so one check coverswith,createandfork. The error comes back wrapped asError::Ref.resetchecks the flag at its top. A load that was already opening the namespace when shutdown started can still finish; closing that is the shutdown-order follow-up.forkloads its source throughwithand uses the same error rule.createdestroyandfork.createremoves the record again. Left in place, every retry answered "already exists" for a namespace that was never loaded, so a dump was never applied.checkpointno longer inserts an empty cache entry for a namespace that isn't loaded. Before, a checkpoint of an unloaded namespace made that namespace answer 404 until the entry was evicted.rpc::namespace_error_statusmapsNamespaceDoesntExistto the existingFAILED_PRECONDITION, andNamespaceStoreShutdownandNamespaceUnavailabletoUNAVAILABLE. Anything else staysINTERNAL.FAILED_PRECONDITIONis only ever a missing namespace.replication::auth::authenticate,ProxyService::extract_contextandReplicaProxyService::do_authdecide on the code it returns: only a missing namespace falls back to the user strategy (or skips authentication, in the proxy). So the error is classified, andError::Refunwrapped, in one place, and that place is tested with wrapped errors. Before, aHelloduring shutdown gotINTERNAL "Error fetching jwt key for a namespace: Namespace store has shutdown", and a replica shutting down answeredINTERNALfrom its proxy.Error::as_refunwraps nestedRefs, as the HTTP mapping already did.libsql_server_namespace_emptied_entry: each time a request finds its entry emptied and looks again. Rises on every deploy; informational.libsql_server_namespace_emptied_entry_gave_up: requests answeredNamespaceUnavailablebecause every lookup found an emptied entry while the namespace still exists. Giving up because the store shut down, or because the namespace is really gone, is not counted. This is the only new user-visible failure; alert on it.debug, giving up atwarn.What embedded replicas see
Measured with eight embedded replicas (the
libsqlcrate) syncing in a tight loop against a primary that is stopped and started again (replica-sigterm.shin #58):sync()fails withRequested namespace doesn't exist(Error::NamespaceDoesntExist). The replicator does not retry it; the application gets the error.error connecting to primary. retrying. error: status: Unavailable, message: "Namespace store has shutdown"and retries the handshake itself, with 1 s sleeps, up to its retry budget.Timeout performing handshake with primaryonce the retries run out, same as before.So for a client built on the crate, a primary restart that is shorter than the replicator's handshake retries is now absorbed inside
sync(). Before, every restart surfaced as a "namespace doesn't exist" failure to the application, which is the one error it might reasonably act on by discarding its local replica.How to review
About 240 lines are production code, comments included; the rest is tests.
Reading order:
NamespaceStore::withandemptied_entry_errorinnamespace/store.rs. This is the fix.load_namespace's init, the ones at the top ofcreateandreset, andload_created.forkandcheckpoint.rpc::namespace_error_statusand its call sites inproxy.rs,replica_proxy.rs,replication_log.rsandreplication/auth.rs.start_waitingandsettle.Invariants worth checking:
has_shutdownis set beforeshutdown()empties any entry.withchecks it at the start of every attempt, andload_namespacechecks it before opening a namespace. A request that wakes on an entry emptied by shutdown gets a 503. The ordering between the flag and the entries comes from the entry'sRwLock: the flag store is sequenced before shutdown's write-unlock, and the request's read-lock acquires after it, so theRelaxedloads and stores are enough. The one remaining gap is a load that was already insidemake_namespacewhen the flag was set.withreturnsNamespaceDoesntExistonly when the metastore has no record. The remaining exception is theMetaStore::handleresurrection listed under follow-ups.with.forkandresethold an entry write lock across a cache call; that predates this change and is commented in the code, with the fix left to the follow-ups.FAILED_PRECONDITIONfromnamespace_error_statusand on nothing else.Tests
namespace::store::test,rpc::test,rpc::proxy::test,rpc::replica_proxy::testandrpc::replication::replication_log::testadd 21 tests. They drive each case deterministically: the test holds the entry's write lock, so the request waits on the same entry, then empties it the way each caller does. Covered:withafter shutdown, and shutdown starting while a request waits:NamespaceStoreShutdown. Before, the first reloaded the namespace.NamespaceDoesntExist.NamespaceUnavailableafter the three attempts, through the realwith, and the entry is still the cached one.destroy: stillNamespaceDoesntExist.load_namespacerefuses to open a namespace once the store has shut down;resetandcreateafter shutdown are refused.createwhose load shutdown refused removes its record; a retry against a fresh store on the same files creates the namespace.forkreloads a source evicted while it waited; a source emptied between its two loads answersNamespaceUnavailable.checkpointof an unloaded namespace caches nothing.emptied_entry_erroroutcomes.Hello(external and internal service),ProxyService::extract_contextandReplicaProxyService::do_authafter shutdown:UNAVAILABLE. The Hello test configures Basic auth and sends no credentials, and the proxy test sends anx-proxy-authorizationheader, so each fails if the fallback for the two 503 errors ever returns.NAMESPACE_DOESNT_EXISTmessage.Mutation checks: putting the original
NamespaceDoesntExistback on the give-up path, removing either shutdown check, dropping the record removal increate, letting theUNAVAILABLEcase fall back to the user strategy, unwrapping only oneRef, swapping the two steps insidedestroy, and restoring the proxy fallback each fail the test added for it. The concurrency tests passed 300 consecutive runs before the last restructuring and 30 after it.cargo nextest run -p libsql-server: 225 tests, 224 passed, with the same results as the base branch.embedded_replica::local::local_sync_with_writesandembedded_replica::replica_no_resync_on_restartfail on both the base and this branch under parallel load.replica_no_resync_on_restartpasses when run alone.cluster::replication::apply_partial_snapshotandconnection_core::test::test_many_concurrentare flaky under parallel load on both.Verified against running servers
Debug
sqldbuilt from this branch and from the base, run locally with--enable-namespacesand driven over Hrana HTTP by the scripts in #58.--max-active-namespaces 2, 6 hot namespaces, 24 clients, 20 sNamespace store has shutdown, then connection errors; first 503 at +1 ms| Embedded replica (the
libsqlcrate): 8 replicas on 8 namespaces syncing in a tight loop, 20 namespaces loaded, SIGTERM at 4 s, restart at 7 s |sync()failed at the signal withRequested namespace doesn't exist(Error::NamespaceDoesntExist, not retried) | never "doesn't exist".sync()either surfacedstatus: Unavailable, message: "Namespace store has shutdown"or, because the client retries aClienterror's handshake with 1 s sleeps, ended inTimeout performing handshake with primaryonce the server had been down for the retry budget; all replicas resumed after the restart || 503 window versus loaded namespaces: SIGTERM under load with every namespace holding a WAL | | 2,000 namespaces: first 503 at +4 ms, port closed at +0.41 s (server log 0.48 s). 5,000 namespaces: +1.52 s (server log 1.70 s). About 0.3 ms per loaded namespace in a debug build; the work per namespace is task shutdown and sentinel removal, not data size, since
--snapshot-at-shutdownis off |The 65-check regression matrix (
regress.shin #58) ran against both binaries. Only the intended lines differ:destroysometimes leavesdata-walbehind (DirectoryNotEmpty, two 500disk I/O erroranswers) and, on the base, requests sometimes still got 200 after the delete. In a reviewer's 10 runs per binary the directory stayed in 6 of 10 on the base and 2 of 10 here, where requests always got 404 after the delete; in the run above the base left it and this branch did not. The race is theMetaStore::handleresurrection under follow-ups; this change does not add to it and shows it less often.Identical on both: namespace lifecycle (create, duplicate 400, query, config, stats, delete, real 404s, directory removed); fork with hot and cold source, unknown source 404, loaded destination 400; create from a
file://dump; shared-schema create with hot and cold schema source, linked-db delete guard; data intact after SIGTERM restarts with and without--snapshot-at-shutdown; replication catch-up, proxied writes through the replica, replication and proxied writes resuming after a primary restart; a replica request for an unknown namespace answering 404 through theNAMESPACE_DOESNT_EXISTstring match; mixed versions in both directions catching up and recovering after a primary restart. The replica log showed the new mapping live:error connecting to primary. retrying. error: status: Unavailable, message: "Namespace store has shutdown", followed by recovery.Not exercised live: the proxy-side
UNAVAILABLE(the shutdown window is about 10 ms); it is covered by the unit test.Rollout
UNAVAILABLEinstead ofFAILED_PRECONDITIONorINTERNAL.libsqlcrate):sync()fails with a transport error carryingUNAVAILABLEinstead ofError::NamespaceDoesntExist. An application that matched on "namespace doesn't exist" to reset its local replica no longer sees that during a primary restart, which is the point; one that only retried sees a different error text.sqldreplicas:snapshotmapsUNAVAILABLEtoSnapshotPending, so a replica waiting for a snapshot loops on 1 s sleeps logging "snapshot not ready yet" until the primary is back.RemoteConnection::connectin the write proxy retriesUNAVAILABLEwith uncapped exponential backoff, where it failed immediately onFAILED_PRECONDITIONbefore.http/user/trace.rscounts any HTTP 5xx inlibsql_server_user_http_fault{protcol="http"}and treats gRPCFAILED_PRECONDITIONas success. Restart-time errors were therefore invisible there. After this change they appear as 503 andUNAVAILABLEon both protocols, andlibsql_server_user_http_response{status="503"}rises during restarts. The first rollout of this build looks like today, because the stopping pods run the old binary; the next rolling restart is the first to show the burst. Check dashboards, alerts and deploy gates that read those series before then. Replicas log a non-fatal replication error instead ofnamespace ... doesn't exist, trying to replicate againwhile their primary restarts./healthkeeps answering 200, so from SIGTERM until every namespace is checkpointed the pod stays in rotation and answers 503 to every request, with noRetry-After. Clients must retry 503 with backoff. Before this change the same window produced the false 404s. Locally it was 0.5 s for 2,000 loaded namespaces and 1.7 s for 5,000 (debug build); our production pods hold about a quarter ofmax_active_namespaces, so expect well under a second there, inside the sync service's per-call retry budget. Measure it in staging all the same.FAILED_PRECONDITIONfrom the primary.max_active_namespaces, so capacity eviction does not occur; only the 24 h idle eviction does, which races a request only when one arrives as the idle timer fires. Expectlibsql_server_namespace_emptied_entryto rise during restarts and stay flat otherwise.Namespace store has shutdown). Tooling that opens issues per distinct error will open one for it once; it is the same restart-time condition, now with the right status.Not in scope (follow-ups)
Ordered by payoff. #53, stacked on this PR through #51, fixes items 2, 3 and 4, part of 5, and the metric in 8.
has_shutdownis set (a separate endpoint, since failing/healthwould end the pod mid-flush), stop accepting connections, drain in-flight requests with a timeout, then take the namespaces out. This turns the 503 window into zero errors and closes the last shutdown gap, a load already insidemake_namespacewhen the flag is set.MetaStore::handleresurrects records.handleinserts a default in-memory record when the namespace is missing. A request that passesexistsjust beforedestroyremoves the record re-creates it, and can then open an empty database over the directory being deleted; that is the delete-under-load race above. This predates this change; the retry loop repeats theexists/handlepair per attempt, so a request that retried has up to three such windows instead of one. A non-insertinghandle_if_existscloses it. It also removes the last reasonProxyService::extract_contextskips authentication for a missing namespace. Fixed in libsql-server: make namespace creation all-or-nothing (stacked on #51) #53:withlooks a namespace up with a non-insertingexisting_handle.withthen gives up withNamespaceUnavailableon every request for that name, andcreateof that name returnsOkwithout loading (so a dump is dropped), until the entry is evicted or the server restarts. On the base the same sequence answers 404. Until the fix ships, a sustainedgave_uprate for one namespace means a restart. Fixed in libsql-server: make namespace creation all-or-nothing (stacked on #51) #53:load_namespacedrops a leftover empty entry and reloads, under moka's key lock (evict_if_still_empty, folded in from the closed libsql-server: drop a namespace cache entry that was left empty #59), andcreate's reservation fills such an entry under its lock, so a dump is applied.forkteststo_lock.is_some()instead of the metastore, so an unloaded destination passes. The rename fails withDirectory not empty,ForkTask::forkrunsremove_dir_allon the destination directory on any error, and the store's cleanup removes the record. Observed: 500, then the destination answers 404 and its directory is gone. Admin API only, but data loss. Fix: checkmetadata.exists(&to)beforeget_with(to), and makeForkTask::forkremove only a directory it created. Deserves its own PR soon. Fixed in libsql-server: make namespace creation all-or-nothing (stacked on #51) #53:forkreserves its destination, and the reservation rejects a name the metastore knows. The matrix shows 400already existswith the destination intact. TheForkTask::forkcleanup that removes any directory on error remains.Arc<RwLock<Option<Namespace>>>lets "empty" be a normal state that every reader must handle;with,fork,createandreseteach handle it differently. libsql-server: make namespace creation all-or-nothing (stacked on #51) #53 makes the operations that hold an entry locked safe (it pins them inin_flight, its eviction listener does not wait on a pinned entry, andinstallre-inserts a namespace whose entry was evicted). It still uses the same entry type, though, and defers theFilling | Loaded | Gonestate with the reservation held outside the cache.NamespaceDoesntExistin a tight loop: about 5,500 log lines in 2 s per namespace (the existing TODO inconfigurator/replica.rs).withon the hot path.existsandhandleboth lock the global metastore mutex, andhandlebuilds aMetaStoreHandlethat a cache hit discards. Check the cache first and callhandleonly on a miss. Benchmark before and after.libsql_server_namespace_load_latencyis recorded on every lookup, cache hits included, so it counts requests, not loads (libsql-server: make namespace creation all-or-nothing (stacked on #51) #53 records it only for real loads).checkpointof an unknown or unloaded namespace returnsOk(pre-existing). Under eviction churn,Namespace::shutdownlogsunable to remove .sentinel filewhen a namespace was reopened while the evicted instance was still shutting down (pre-existing, both binaries). A request that holds a connection maker whendestroyremoves the directory fails with 500unable to open database file. A proxied write in flight when the primary exits answers 500PrimaryStreamDisconnect, and new ones wait in the write proxy's unboundedUnavailableretry loop until the primary is back.format_errlogs every 503 at error level, as it did the 404s.withskips the metastore check and never answersDoesntExist, and a request waiting while its namespace is destroyed reopens a fresh replica namespace for the name; the reload, give-up and destroy tests should run with it. Counters asserted through aDebuggingRecorder. A positive control forcheckpoint.start_waiting/settledepend on moka's reference counts; a#[cfg(test)]hook inwithwould decouple them. One sharedmake_storefor the store and scheduler tests.