Repository navigation
Firestore: preserve batch IDs after queue restart - #16751
dajiaohuang wants to merge 7 commits into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
|
The reviewing agent claims this is also an issue on Android |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that pending document overlays for indexed queries are preserved when the mutation queue is recreated. It achieves this by updating LevelDbMutationQueue::Start() to restore the next batch ID using the global high-water mark of indexed batch IDs across all users, retrieved via a new GetHighestBatchIdAcrossUsers() method in LevelDbIndexManager. Additionally, assertions were added to prevent batch ID overflow, and a unit test was introduced to verify the new behavior. Feedback is provided to check the LevelDB iterator's status after the loop in GetHighestBatchIdAcrossUsers() to prevent potential silent data corruption from unhandled read errors.
|
Thanks for the contribution! A few housekeeping items:
|
paulb777
left a comment
paulb777
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. The analysis matches what I see in the code: index offsets are persisted per user and outlive the mutation queue, so restarting with an empty queue can hand out batch IDs at or below an existing largest_batch_id, and those pending writes are then skipped by index-backed cache queries (#16742). Taking the max across users in Start() looks like the right fix. This will also need a review from the Firestore team.
A couple of requests inline.
|
Addressed the requested review changes: moved the CHANGELOG entry to Unreleased with #16751, removed the two unrelated mutation-table/write-time overflow assertions, and replaced the new persisted-index assertion with a clamp before incrementing. Renamed the queue test to StartIncludesLargestIndexedBatchIdAcrossUsers, retaining the existing two-user fixture with global index configuration and separate user offsets. Added a LocalStore-level regression using a real LevelDB close/reopen and indexed cache query after acknowledging all writes. It verifies that the new pending document is returned without a collection scan. Actual Linux CMake Debug validation compiled firestore_local_test from production sources: all 790 tests passed. With only the global index high-water restoration disabled, both StartIncludesLargestIndexedBatchIdAcrossUsers and IndexedCacheQueryIncludesNewPendingWrite failed; restoring the patch made both pass in the complete local-test target. Run: https://github.com/dajiaohuang/firebase-ios-sdk/actions/runs/37100308605/job/111138367633 . The validated Firestore source/test tree is identical to c87ebae; the validation workflow stays on a separate fork branch. This is Linux local-store validation, not an iOS SDK or backend integration run. |
This branch has not been deployed
2 participants
Summary
Restore Firestore's global mutation batch ID high-water mark from persisted index offsets for every user when a LevelDB mutation queue starts. Acknowledging all mutations and reopening an indexed store must not cause the next pending write to reuse a batch ID that the persisted index offset already excludes.
The change keeps the queue fix focused: the additional mutation-table and write-time overflow assertions are removed, and an unexpectedly large persisted index offset is clamped before the new increment rather than asserting during startup. The CHANGELOG entry is under Unreleased and references #16751.
Fixes #16742
Validation
StartIncludesLargestIndexedBatchIdAcrossUsersverifies the greatest persisted offset across two users is included when restarting the queue. Index configuration is global; each user has its own persisted index state.LevelDbLocalStoreRestartTest.IndexedCacheQueryIncludesNewPendingWriteuses actual LevelDB persistence and LocalStore: configure an index, write/backfill, acknowledge, finish backfilling the acknowledged document, close/reopen the same database, write another matching document, and verify both documents through an indexed cache query without a collection scan.Current-main integration
Merged current main c276eca normally at 5aba540 to resolve the Firestore CHANGELOG conflict. Both Unreleased entries are retained. All six affected Firestore production/test files are byte-identical to the previously validated c87ebae tree; git diff --check passes. The earlier 790-test Linux result remains component evidence, not a new post-merge run. No Apple tests were run on this Windows host; updated-head CI is separate.