Repository navigation
Rebase Shopify patches onto upstream libsql 0.10.0 - #63
Closed
mostafa-yassine wants to merge 17 commits into
Closed
mostafa-yassine wants to merge 17 commits into
mostafa-yassine wants to merge 17 commits into
Conversation
(cherry picked from commit 30f4d57)
The admin listener serves both the admin API and Prometheus metrics. Annotation-based scraping cannot attach credentials, so guarding /metrics behind LIBSQL_ADMIN_AUTH_KEY would blind the fleet. Keep GET /metrics open; all other admin routes remain guarded. (cherry picked from commit 96b0b7f)
The Extensions Tests job fresh-resolves rust_suite deps on every run; icu 2.3.0 (via idna_adapter 1.2) now requires rustc 1.88 while the repo pins 1.85.0, failing CI on any branch of this tag. Pinning idna_adapter to 1.1.0 (unicode-rs backend) drops icu from the graph. (cherry picked from commit e6a394a)
Intercepts raw HTTP responses before tonic-web gRPC-web framing to capture the actual response body when the server returns a non-gRPC response. This helps diagnose 'Invalid header bit N expected 0 or 1' errors by logging the HTTP status, content-type, and response body preview. (cherry picked from commit 11e180e) (cherry picked from commit 1691763)
vectorParseSqliteText stores each vector element in a 1025-byte stack buffer whose final byte must remain NUL. The length guard used `>`, so a 1025-character element overwrote that terminator before the guard fired; the following error path then formatted the buffer with `%s`, reading past the end of the stack buffer. Reject the element once it reaches MAX_FLOAT_CHAR_SZ characters so the terminator is preserved, regenerate the bundled amalgamations, and add boundary regression tests for every vector function that parses TEXT. (cherry picked from commit 553cb83) (cherry picked from commit bf91f74)
libsql-sqlite3/test/rust_suite has no committed Cargo.lock, so CI resolves its transitive dependencies fresh on every run. Several of those now require a newer compiler than the pinned 1.85.0 (icu_* and wasm-encoder/wast declare rust-version 1.88; yoke-derive 0.8.3 declares none but uses str::from_utf8 as an inherent method, stabilized in 1.87). This breaks the Extensions Tests job and the rusttestwasm step of make-sqlite3 on main. Pin current stable (1.98.1) rather than the minimum that compiles today (1.88.0, verified green in CI), so crate MSRV bumps do not break CI again in the near term. Document the constraint next to the unlocked test crate. (cherry picked from commit e48d754) (cherry picked from commit a78b64c)
CI compiles with RUSTFLAGS="-D warnings", so lints added since 1.85.0 fail the build: - mismatched_lifetime_syntaxes (new in 1.89): eleven signatures elide a lifetime on the input side (&self / &str) but hide it on the output type (Vec<Column>, PageHdrIter, CursorStep<S>, Cow<str>). Spell the output lifetime as '_ as the compiler suggests. No semantic change; this is the lifetime rustc already inferred. - unused_assignments: `frameno` in bottomless-cli's restore loop was only ever copied into BatchReader::new and then incremented, never read. BatchReader tracks its own next_frame_no and the function returns the separate last_received_frame_no, so the local was dead since it was introduced in 4a71b20. Remove it and pass first_frame_no directly. Verified locally on 1.98.1 with the same flags as CI: cargo check --all-targets --all-features, cargo fmt --check, and cargo check -p libsql --no-default-features for core/replication/remote. (cherry picked from commit c22cc62) (cherry picked from commit f35e15e)
Port gwenn/lemon-rs@14f422a to replace the unchecked UTF-8 conversion with a lossy conversion. Bump the vendored parser to 0.13.1 and add regression coverage for invalid input.\n\nAddresses CVE-2025-47736 / GHSA-8m95-fffc-h4c5. (cherry picked from commit 4feb2b2)
(cherry picked from commit 595c9ea)
(cherry picked from commit aee7757)
(cherry picked from commit 5f4fa6d)
(cherry picked from commit 61e0984)
(cherry picked from commit 9196686)
(cherry picked from commit 422db4a)
* Never report a namespace whose entry was emptied as missing 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. * Test the give-up path of NamespaceStore::with 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. * Do not spend an attempt on dropping a left-empty entry 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. * Refuse to open a namespace once the store has shut down `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. * Answer UNAVAILABLE from the replica proxy while the store is shut down `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. * Count give-ups and dropped left-empty entries `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. * Test the destroy race against the real destroy 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. * Assert that reloads go into a fresh cache entry 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. * Test several requests waiting on one left-empty entry 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. * Make comments and logs match what the code guarantees - 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. * Send a forged proxy auth header in the shutdown test `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. * Test hygiene for the namespace store tests - `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. * Leave entries that stay empty to a follow-up and simplify the retry loop 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. * Remove the record of a create that shutdown interrupted before its load `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. * Classify namespace errors for authentication in one place 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. * Name the left-empty test after what it guards `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). * Test that waiters on an evicted entry share one load, and that 503s never 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". (cherry picked from commit 851ddea)
(cherry picked from commit eaabc92)
(cherry picked from commit 5863c87)
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.
Context
This fork tracks upstream
tursodatabase/libsqland carries a small set of Shopify patches on top. Today the default branchv0.9.30-shopify-patchesis upstream0.9.30(0653c5788d, 2026-03-19) plus 15 patches.Upstream
mainis now0.10.0(d8bc862d20, 2026-10-13). It includes the fix for the double close inlocal::Connection::disconnect(tursodatabase#2261) that #62 cherry-picks onto the 0.9.30 line.Problem
The fork is 7 months behind upstream. Each upstream fix we need has to be cherry-picked by hand, and the gap grows. Rebasing on
0.10.0lets consumers pick up the double-close fix and everything else upstream shipped, and resets the carry to the patches that are ours.Solution
New branch
v0.10.0-shopify-patches: the fork'smain(which mirrors upstreammainatd8bc862d20,0.10.0) plus the fork patches, cherry-picked with-xin their original order. Every one applied without conflicts. The PR targetsmain, so the diff is only our patches.The 17 commits on this branch, oldest first:
30f4d57767SQLITE_OPEN_FULLMUTEXwhen opening a connection96b0b7fcacGET /metricsfrom admin API authe6a394a75f1691763b33bf91f74a79a78b64c5caf35e15e233-D warnings4feb2b2e46595c9ea980aee7757d7c5f4fa6d1ad61e0984cd69196686203422db4a2d7851ddea771eaabc92847Connection::disconnect5863c87421The fix commit from #62 (
760d4e45f7) is not cherry-picked. Upstream0.10.0already contains it as4e06af572d. Only the two test commits come over.Merge commits from the 0.9.30 line are not replayed.
git cherry upstream/main v0.9.30-shopify-patchesreports none of the 15 patches as already upstream.What upstream
0.9.30to0.10.0brings1174 upstream commits. By area:
3.45.1to3.47.0(libsql-ffi/bundled/src/sqlite3.c, +10,486 / -5,659 lines), SQLite3MultipleCiphers 1.8 to 1.9.0, and a WAL savepoint-forget hook inlibsql-ffi. This is almost all of the commit count (ext/,src/,test/underlibsql-sqlite3).libsqlclient crate: 2 commits.4e06af572ddouble-close fix and291d1011d5ANALYZEsupport.libsql-replication,libsql-rusqlite: 0 commits.libsql-server: 1 commit.For embedded consumers the material change is the SQLite engine bump. That is why this is a separate PR from #62: #62 is the one-line crash fix on the current line, this PR is the engine upgrade.
Suggested follow-up for maintainers
main.mainshould stay a clean mirror of upstream. Instead, setv0.10.0-shopify-patchesas the default branch once reviewed.libsql-rs-v0.10.0+shopify.1.v0.9.30-shopify-patchesfor consumers that need a hotfix on the old engine.Testing
Local, on this branch:
cargo check -p libsql -p libsql_replication -p libsql-rusqlite --features libsql/replication: pass.cargo fmt -p libsql -- --check: pass.cargo test -p libsql --features replication --lib: 25 passed, includingdisconnect_is_idempotentanddropping_public_connection_closes_once.cargo test -p libsql --features replication --test integration_tests: 27 passed, 1 ignored.cargo test -p libsql_replication -p libsql-rusqlite: pass exceptbootstrap, which needsprotocand is not installed on my machine. CI has it.libsql-servertests: not run locally. CI runs them.Not tested here: embedded replica sync against a
0.9.30libsql-server over the wire, and a consumer app build on Android/iOS against this branch. Both should happen before a consumer bumps to the new tag.Related