Simplify db swap and remove transactions - #44
Open
kamatsuoka wants to merge 21 commits into
Open
Conversation
The search DB is read-only and refreshed daily; there's no need to swap a newer copy into a running app. Drop the DbWrapper concurrency machinery (transaction refcounting, spin-waits, replace-in-place) and open one immutable connection per launch. The background update now just validates the download and stages it on disk for the next launch to open, without touching the live connection. This removes the entire class of connection-swap bugs we'd been patching one at a time (unawaited move, close-before-open) by making them structurally impossible: there is no swap, no second connection, and nothing to close out from under an in-flight query. searchutil now calls the connection's withTransactionAsync directly.
With one immutable read-only connection per session, wrapping the four search reads in a transaction bought nothing -- nothing can write between them -- while the explicit BEGIN/COMMIT risked "cannot start a transaction within a transaction" when fetches overlapped on the shared connection (favoritesSlice had already worked around this with useTransaction: false). Drop the transaction, the useTransaction flag, and withTransactionAsync from the InnerDb surface.
Adds a search-database refresh row to the Data screen that checks for a newer DB and adopts it into the running session immediately, with snackbar feedback (updated / already up to date / couldn't reach server). - backgroundCheckForRemoteUpdates now returns a DbUpdateResult instead of void, making the outcome observable (the startup path just logs it). - refreshDbNow() runs the check and, on success, reopens the connection by reassigning the singleton -- without closing the old one, so an in-flight query finishes safely and the next query uses the new file. - A shared in-flight guard dedupes the manual refresh against the startup check and against a double-tap, so two downloads can't race the same tmp file. This is the only coordination retained -- one promise, not the old txnCount machinery.
"Refresh" was a check-if-newer, so it almost always reported "already up to date" -- useless for its main purpose, recovering a stale or corrupted local DB. Make the button always force: re-download, re-validate, and re-adopt the remote DB regardless of the on-disk manifest. - backgroundCheckForRemoteUpdates takes a `force` flag that skips the not-newer early-out. - refreshDbNow(force) threads it through; the button passes force=true. - Replaced the dedupe guard with a serialization chain so a forced refresh can't be satisfied by an in-flight non-force check, and the two still can't run overlapping downloads into the same tmp file. Also simplified getDbConnection's memoization to a single `??=` (the local + array dance was only there for TS non-null narrowing; the synchronous set-before-await race guard is preserved).
The backup fragment already enters the Data screen, so add a tap on the new "refresh" row there: force-refresh, wait for the result snackbar, screenshot it, and dismiss. Robust to no network -- the snackbar (and its close button) appears whether the refresh succeeds or reports unavailable.
A failed initializeDbConnection() left its rejected promise cached on the singleton forever, so every later caller re-threw the same error: search, popular, random and friends all showed "error fetching tags: ..." until the user force-quit the app. A transient failure (Metro not up yet, a momentary FS error) was as fatal as a permanent one. - getDbConnection clears the singleton when an attempt fails, so the next caller retries. Callers arriving together still share one attempt, so the delete-and-move seeding can't run concurrently. - That clear is guarded on still owning the cell: refreshDbNow can install a healthy connection while a doomed attempt is in flight, and clearing unconditionally would discard it and force a needless re-init. - backgroundCheckForRemoteUpdates now reads the local manifest tolerantly. A missing or truncated manifest -- what a half-finished first-launch seed leaves behind -- threw before the check ever reached the network, so refreshDbNow reported Unavailable and the user could never download the DB that would fix them. Absent now reads as 0, i.e. older than anything. - Reworded that Unavailable message: it blamed the update server, but its remaining causes include a download that arrived fine and failed validation. Tests: getDbConnection had no coverage at all, because initializeDbConnection require()s the bundled .sqlite asset and Jest parses it as JS (dying on the file's own "SQLite format 3" header). Stubbing the two asset getters makes the init path testable; new cases cover memoization, retry-after-failure, the ownership guard, and manifest recovery. Also swapped the dbConnectionPromise box for a plain `let` -- it's module-private, so nothing needed the array indirection.
Add docs/search-database.md covering the tag-search SQLite DB end to end: server-side generation and schema versioning, on-device seeding and file layout, the single-immutable-connection model, the stage-then-adopt and manual force-refresh update flows, the hard-won invariants (each fixing a recurring bug), known tradeoffs, and an fts4->fts5 migration scoping note. Add docs/CLAUDE.md indexing the docs directory and pointing future work at search-database.md before touching the DB code. Add a scope cross-reference to data-migration-solution.md so the search DB and user-data backup aren't conflated.
sqlUtil.ts comments had drifted from the code: - the FTS table is fts4, not fts5 (the linked expo issue is fts5-titled, but the double-finalize mechanism is the same); - the connection is no longer "opened once / never swapped / immutable" -- a force refresh and a post-failure re-init both replace it wholesale; - a failed init now clears the singleton to retry, so errors aren't "cached on the singleton promise"; - backgroundCheckForRemoteUpdates only stages files; the background caller adopts on next launch while refreshDbNow adopts immediately. Also refresh the generator's fts5 placeholder comment and align docs/search-database.md (connection model wording, drop the obsolete "generator comment is stale" note).
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.
No description provided.