From a02b236e97f3b1d8242625cc3330a3f0132e8ccb Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Tue, 7 Jul 2026 00:05:35 +0900 Subject: [PATCH] fix: more production crashes (View Transition noise, book-dir race, stats transaction) (#4962) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sentry): drop more benign View Transition rejections Broaden is_ignored_browser_error to also drop "Transition was skipped" (navigation superseded, READEST-F) and "aborted because of invalid state" (READEST-G), matched case-insensitively alongside the existing hidden-tab case. These are expected browser behavior — the navigation completes, only the animation is skipped/aborted. A transition timeout stays visible (real perf signal, handled separately). Fixes READEST-F Fixes READEST-G Co-Authored-By: Claude Opus 4.8 (1M context) * fix(library): create the book directory idempotently Importing a book did a check-then-create with a non-recursive createDir. Two concurrent imports of the same book both pass the exists check, then the second create fails — on Windows with "Cannot create a file when that file already exists" (Sentry READEST-H). Use a recursive create (create_dir_all), a no-op when the directory already exists. Fixes READEST-H Co-Authored-By: Claude Opus 4.8 (1M context) * fix(stats): serialize applyRemoteEvents to avoid nested transactions The statistics connection is shared across ReadingStatsTracker instances (split view). applyRemoteEvents runs a manual BEGIN/COMMIT that the per-op native connection lock does not make atomic, so two concurrent pulls opened a BEGIN inside a BEGIN ("cannot start a transaction within a transaction", Sentry READEST-N). Serialize applyRemoteEvents against itself with a small promise mutex. Adds a regression test that fails without the guard. Fixes READEST-N Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../src-tauri/src/sentry_config.rs | 28 +++++++--- .../__tests__/statistics/statisticsDb.test.ts | 16 ++++++ apps/readest-app/src/services/bookService.ts | 9 ++- .../src/services/statistics/statisticsDb.ts | 56 ++++++++++++------- 4 files changed, 79 insertions(+), 30 deletions(-) diff --git a/apps/readest-app/src-tauri/src/sentry_config.rs b/apps/readest-app/src-tauri/src/sentry_config.rs index ff2b2802..9bd897e2 100644 --- a/apps/readest-app/src-tauri/src/sentry_config.rs +++ b/apps/readest-app/src-tauri/src/sentry_config.rs @@ -76,13 +76,18 @@ pub fn android_version_from_uname(release: &str) -> Option { } } -/// A known-benign browser error that is expected behavior, not an app bug, and -/// only adds noise to crash reporting: the View Transition API skips a -/// transition when the tab is hidden. Matched on the exception value so it is -/// dropped in `before_send`. NOTE: a transition *timeout* abort is deliberately -/// NOT ignored — a slow DOM update can signal a real performance problem. +/// Known-benign browser errors that are expected behavior, not app bugs, and +/// only add noise to crash reporting: the View Transition API skips a transition +/// when the tab is hidden or the navigation is superseded, and aborts it when the +/// document is in an invalid state. These arrive as unhandled rejections while +/// the navigation itself still completes. Matched (case-insensitively) on the +/// exception value so they are dropped in `before_send`. NOTE: a transition +/// *timeout* abort is deliberately NOT ignored — a slow DOM update can signal a +/// real performance problem. pub fn is_ignored_browser_error(value: &str) -> bool { - value.contains("View transition was skipped because document visibility state is hidden") + let value = value.to_lowercase(); + value.contains("transition was skipped") + || value.contains("transition was aborted because of invalid state") } /// The WebView (engine, major-version), set once at startup when the app reports @@ -216,10 +221,19 @@ mod tests { } #[test] - fn ignores_benign_hidden_view_transition_error() { + fn ignores_benign_view_transition_errors() { + // Skipped because the tab is hidden (READEST-7). assert!(is_ignored_browser_error( "InvalidStateError: View transition was skipped because document visibility state is hidden." )); + // Skipped because the navigation was superseded (READEST-F). + assert!(is_ignored_browser_error( + "AbortError: Transition was skipped" + )); + // Aborted because the document was in an invalid state (READEST-G). + assert!(is_ignored_browser_error( + "InvalidStateError: Transition was aborted because of invalid state" + )); } #[test] diff --git a/apps/readest-app/src/__tests__/statistics/statisticsDb.test.ts b/apps/readest-app/src/__tests__/statistics/statisticsDb.test.ts index 1259b9f9..87802aff 100644 --- a/apps/readest-app/src/__tests__/statistics/statisticsDb.test.ts +++ b/apps/readest-app/src/__tests__/statistics/statisticsDb.test.ts @@ -103,6 +103,22 @@ describe('StatisticsDb', () => { expect(book!.total_read_time).toBe(8); }); + it('serializes concurrent applyRemoteEvents without nesting transactions (READEST-N)', async () => { + // Two pulls racing on the shared connection (split-view trackers) must not + // open a BEGIN inside a BEGIN ("cannot start a transaction within a transaction"). + const a = stats.applyRemoteEvents( + [{ bookMd5: 'ra', title: 'RA', authors: '' }], + [{ bookMd5: 'ra', page: 1, startTime: 400, duration: 3, totalPages: 10 }], + ); + const b = stats.applyRemoteEvents( + [{ bookMd5: 'rb', title: 'RB', authors: '' }], + [{ bookMd5: 'rb', page: 1, startTime: 401, duration: 4, totalPages: 10 }], + ); + await expect(Promise.all([a, b])).resolves.toBeDefined(); + expect((await stats.getBookByMd5('ra'))!.total_read_time).toBe(3); + expect((await stats.getBookByMd5('rb'))!.total_read_time).toBe(4); + }); + it('reads and writes sync cursors', async () => { expect(await stats.getCursor('push')).toBe(0); await stats.setCursor('push', 1234); diff --git a/apps/readest-app/src/services/bookService.ts b/apps/readest-app/src/services/bookService.ts index dda15606..9c2a0e04 100644 --- a/apps/readest-app/src/services/bookService.ts +++ b/apps/readest-app/src/services/bookService.ts @@ -537,9 +537,12 @@ export async function importBook( existingBook.downloadedAt = Date.now(); } - if (!(await fs.exists(getDir(book), 'Books'))) { - await fs.createDir(getDir(book), 'Books'); - } + // Idempotent create (recursive): a plain createDir defaults to + // non-recursive and throws if the dir already exists, and the check-then- + // create above races two concurrent imports of the same book — on Windows + // the loser fails with "Cannot create a file when that file already exists" + // (Sentry READEST-H). create_dir_all is a no-op when the dir exists. + await fs.createDir(getDir(book), 'Books', true); const bookFilename = getLocalBookFilename(book); const willWriteBookFile = saveBook && diff --git a/apps/readest-app/src/services/statistics/statisticsDb.ts b/apps/readest-app/src/services/statistics/statisticsDb.ts index 8455e50a..2c34bec1 100644 --- a/apps/readest-app/src/services/statistics/statisticsDb.ts +++ b/apps/readest-app/src/services/statistics/statisticsDb.ts @@ -43,6 +43,9 @@ function bindLifecycle(): void { * leaves this class. */ export class StatisticsDb { + // Serializes applyRemoteEvents so two concurrent pulls can't nest BEGINs. + private applyRemoteLock: Promise = Promise.resolve(); + private constructor(private readonly db: DatabaseService) {} /** Production entry point — opens + migrates statistics.db (per-tab singleton). */ @@ -187,29 +190,42 @@ export class StatisticsDb { async applyRemoteEvents(books: StatBook[], events: PageStatEvent[]): Promise { if (books.length === 0 && events.length === 0) return; - // One transaction for the whole pulled batch: a single commit instead of - // O(rows) fsyncs, and the apply is atomic (a failed pull leaves no partial - // state). Critical when a fresh device backfills tens of thousands of rows. - await this.db.execute('BEGIN'); + // Serialize against other pulls: the statistics connection is shared across + // ReadingStatsTracker instances (split view), and a second concurrent pull + // would open a BEGIN inside this one's still-open BEGIN ("cannot start a + // transaction within a transaction", Sentry READEST-N). The per-op native + // lock can't make this multi-statement transaction atomic on its own. + const prev = this.applyRemoteLock; + let release!: () => void; + this.applyRemoteLock = new Promise((resolve) => (release = resolve)); + await prev; try { - const idByMd5 = new Map(); - for (const b of books) idByMd5.set(b.bookMd5, await this.upsertBook(b)); - // Books referenced only by events (no metadata record) get a placeholder row. - const touched = new Set(); - for (const e of events) { - let id = idByMd5.get(e.bookMd5); - if (id === undefined) { - id = await this.ensureBookId(e.bookMd5); - idByMd5.set(e.bookMd5, id); + // One transaction for the whole pulled batch: a single commit instead of + // O(rows) fsyncs, and the apply is atomic (a failed pull leaves no partial + // state). Critical when a fresh device backfills tens of thousands of rows. + await this.db.execute('BEGIN'); + try { + const idByMd5 = new Map(); + for (const b of books) idByMd5.set(b.bookMd5, await this.upsertBook(b)); + // Books referenced only by events (no metadata record) get a placeholder row. + const touched = new Set(); + for (const e of events) { + let id = idByMd5.get(e.bookMd5); + if (id === undefined) { + id = await this.ensureBookId(e.bookMd5); + idByMd5.set(e.bookMd5, id); + } + await this.insertPageEvent(id, e); + touched.add(id); } - await this.insertPageEvent(id, e); - touched.add(id); + for (const id of touched) await this.recomputeBookTotals(id); + await this.db.execute('COMMIT'); + } catch (err) { + await this.db.execute('ROLLBACK'); + throw err; } - for (const id of touched) await this.recomputeBookTotals(id); - await this.db.execute('COMMIT'); - } catch (err) { - await this.db.execute('ROLLBACK'); - throw err; + } finally { + release(); } }