feat: add visibilitychange probe for proactive IDB health check#788
Open
leshniak wants to merge 12 commits into
Open
feat: add visibilitychange probe for proactive IDB health check#788leshniak wants to merge 12 commits into
leshniak wants to merge 12 commits into
Conversation
Adds a Dexie-style heal pattern to createStore for Chromium's Internal error opening backing store error (884K errors/month). - isBackingStoreError() detects the Chromium-specific corruption - Shared healAttemptsRemaining counter (3, reset on success) - On backing store error: clear cached connection, retry once - Clear dbp on rejection so retries get fresh indexedDB.open() - 5 new tests: mid-session heal, init heal, budget exhaustion, budget reset, error classification No deleteDatabase(), no provider swap, no UI changes. Scoped to IDBKeyValProvider only -- SQLite provider untouched. Ref: Expensify/App#90636 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Capture dbp reference before attaching reject handler; only clear if dbp hasn't been replaced by a concurrent heal/retry (prevents stale rejection handler from clearing a newer promise) - Add comment documenting concurrent store() budget drain behavior - Fix test formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ose) The heal path clears the cached dbp and reopens via indexedDB.open(), but does not call db.close() on the old IDBDatabase. Updated comments and log messages from 'close + reopen' to 'drop cached connection and reopen' to match what the code actually does. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- isBackingStoreError: use Error instead of DOMException, drop .name check - InvalidStateError catch: same simplification - Remove issue link from JSDoc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add isConnectionLostError() to detect 'Connection to Indexed Database server lost' and 'Connection is closing' — Safari/WebKit errors that fire when the browser terminates IDB connections for backgrounded tabs. Uses the same heal-and-retry mechanism as backing store corruption: drop cached dbp, retry once with fresh indexedDB.open(), shared budget. Addresses Expensify/App#87864. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract module-level helpers: isInvalidStateError, isBudgetedHealError, getBudgetedHealErrorLabel. Extract cacheOpenPromise to deduplicate rejected-promise cleanup in getDB and verifyStoreExists. Pure refactor — no behavior change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Heal attempts: logAlert with error type, action taken, remaining budget - Heal success: logInfo confirming recovery after each error type - Budget exhaustion: explicit logAlert when heal budget drains - Non-recoverable errors: logAlert with error details - Updated test assertions to match new log messages and levels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only budget exhaustion and non-recoverable errors should trigger alerts. Heal attempts are handled gracefully and only need informational logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Register a visibilitychange listener inside createStore() that runs a lightweight readonly probe when the tab returns to foreground. If the probe detects a dead IDB connection (connection lost, backing store error, or InvalidStateError), it drops the cached dbp so the next real operation opens a fresh connection instead of failing. This prevents the ReconnectApp write storm from hitting a dead IDB connection after Safari backgrounds a tab. Addresses Expensify/App#87864. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Probe start: logInfo when tab becomes visible and probe begins - Probe healthy: logInfo confirming connection is healthy - Probe stale: logAlert with error details when stale connection detected - Heal attempts/success/exhaustion/non-recoverable: same as Expensify#780 - Updated test assertions to match new log messages and levels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The "connection is healthy" log was emitted synchronously after count(), before the IDB request completed. If the request later failed via onerror, both healthy and stale logs would fire for the same visibility event. Now only logs on actual success. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
leshniak
force-pushed
the
fix/idb-visibilitychange-probe
branch
from
bcd1f6e to
b63de8b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b63de8b399
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| throw error; | ||
| }; | ||
|
|
||
| probePromise.then((db) => { |
There was a problem hiding this comment.
When the tab becomes visible while dbp is still a pending indexedDB.open() and that open later rejects (for example with the same backing-store or connection-lost errors handled below), this separate probePromise.then(...) chain rejects without any rejection handler. The original store operation may catch its own chain, but the promise returned by this then is ignored, so browsers can emit an unhandled rejection during foregrounding; add a rejection handler/catch for the probe path.
Useful? React with 👍 / 👎.
Contributor
|
hold for my review 🙏 |
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
2 participants
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.
Details
Adds a proactive
visibilitychangeprobe to the IDB connection manager, building on the reactive heal mechanism from #780.Problem: Safari kills IDB connections for backgrounded tabs (WebKit #197050, #201483). When the user returns to the Expensify tab, the app fires a ReconnectApp write storm that hits the dead cached
dbp— every write fails before the reactive heal can kick in.Solution: Register a
visibilitychangelistener insidecreateStore()that runs a lightweight readonlycount()probe when the tab becomes visible. If the probe detects a dead IDB connection, it drops the staledbpbefore the write storm arrives, so the first real operation opens a fresh connection.What it does:
isStaleConnectionError()— union detector for all three stale connection error types (InvalidStateError, backing store corruption, connection lost)visibilitychangelistener withprobePromiseguard — prevents stale probe from clearing adbpthat was already replaced by a concurrent heal/retryreq.onerror— only dropsdbpfor actual stale connection errors, not unrelated IDB errorslogInfoon probe start ("tab became visible, checking connection health") and healthy result ("connection is healthy");logAlerton stale detection ("stale connection detected, dropping cached connection") with error messagetypeof document !== 'undefined'for SSR/Node safetyDepends on: #780 (reactive heal mechanism)
Related Issues
Expensify/App#87864
Automated Tests
4 new tests in
tests/unit/storage/providers/createStoreTest.ts:Visibilitychange probe (4): probe detects dead connection + drops dbp, skipped when no dbp, healthy connection preserved, InvalidStateError sync throw handled
All 456 tests pass.
Manual Test Steps
Simulating Safari connection lost:
IDB visibilitychange probe: stale connection detected, dropping cached connectionappears (if Safari killed the connection)Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)Screenshots/Videos
Android: Native
N/A — library-level change, IDB is web-only. No UI, no native code touched.
Android: mWeb Chrome
N/A — library-level change, IDB is web-only. No UI, no native code touched.
iOS: Native
N/A — library-level change, IDB is web-only. No UI, no native code touched.
iOS: mWeb Safari
N/A — library-level change, IDB is web-only. No UI, no native code touched.
MacOS: Chrome / Safari
Healed (simulated error):
healed.mov
Killed connection (simulated error):
killed.mp4
Exhausted (simulated error):
exhausted.mov
Healed (simulated on Safari):
safari_error.mp4
Killed connection (simulated on Safari):
safari_killed.mp4
Exhausted (simulated on Safari):
safari_exhausted.mp4
Healed offline (simulated on Chrome):
offline.mp4