Fix IDBMirrorVFS corrupting the database on rollback - #352
Open
lalexdotcom wants to merge 2 commits into
Open
lalexdotcom wants to merge 2 commits into
lalexdotcom wants to merge 2 commits into
Conversation
…ase. jWrite stores files other than the main database in one buffer and copies into it with block.set(pData, iOffset). pData is a Uint8ArrayProxy, not a Uint8Array: it carries the methods but has no indexed access, so TypedArray.set treats it as an array-like, reads undefined at every index and stores zeroes. The comment on the class says as much, and MemoryVFS already calls subarray() for this reason. What it costs is the rollback journal. SQLite reserves its header, journals the pages it is about to write, and only then stamps the magic and the record count at offset 0 -- a 12-byte write, which lands as zeroes. On rollback SQLite reads that header back, finds a journal that looks empty, concludes there is nothing to undo, and deletes it. The pages it had already written to the database stay, and the database header no longer matches the file: "database disk image is malformed". Larger writes are unaffected, which is why this went unseen: they extend the buffer, and the copy inside the growth path uses newBlock.set(block) between two real arrays. Only a write that fits the existing buffer takes the faulty path, and the journal header is exactly that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A transaction big enough that SQLite must write pages before the commit, then rolled back. That is what makes SQLite journal the pages and stamp the journal header, and the rollback is what reads the header back. The test asserts through SQL alone -- PRAGMA integrity_check and the table contents -- so it does not depend on how the VFS stores anything. It has to go through SQLite: the defect is in how a Uint8ArrayProxy reaches the VFS, and a test calling jWrite directly receives a real Uint8Array from Comlink and cannot see it. Against the previous behaviour it fails with "database disk image is malformed". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 tasks
This branch has not been deployed
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.
What happens
jWritestores files other than the main database in a single buffer, and copies into it with:pDatais aUint8ArrayProxy, not aUint8Array. It carries the methods but has no indexed access, soTypedArray.prototype.settreats it as an array-like: it readspData[0],pData[1], … which are allundefined, and stores zeroes. The class comment inFacadeVFS.jssays exactly this — "it is not a real Uint8Array and passing it to functions that expect a Uint8Array may not work. Use subarray()" — andMemoryVFSalready callssubarray()for this reason.What it costs is the rollback journal. SQLite reserves its header, journals the pages it is about to write, and only then stamps the magic and the record count at offset 0. That stamp is a 12-byte write, and it lands as zeroes:
On rollback SQLite reads the header back, finds a journal that looks empty, concludes there is nothing to undo, and deletes it. The pages already written to the database stay, and the database header no longer describes the file — one measurement had the header claiming 3 pages for a file of 2070, another 2694 pages for a file of 3. SQLite reports
database disk image is malformed.Larger writes are unaffected, which is why this went unseen. A write that does not fit the current buffer takes the growth path, where the copy is
newBlock.set(block)between two real arrays; only the faultyblock.set(pData, …)line writes the payload. A write that fits the existing buffer takes it directly — and the journal header, written after the journal has grown, is exactly that.The change
subarray()returns a realUint8Arrayover the same bytes. Nothing else moves.Test
test/vfs_rollback.js, wired intotest/IDBMirrorVFS.test.js: a transaction large enough that SQLite must write pages before the commit — which is what makes it journal them — then rolled back. It asserts through SQL alone,PRAGMA integrity_checkand the table contents, so it does not depend on how the VFS stores anything.It has to go through SQLite. A test calling
jWritedirectly receives a realUint8Arrayfrom Comlink and cannot see the defect; I wrote that test first and it passed against the bug.Against
masterit fails withdatabase disk image is malformed. With the change, the file's tests pass and the full suite is 13 files, 0 failures.Scope
The defect needs no interruption, no concurrency and no savepoint: an ordinary
BEGIN/ largeINSERT/ROLLBACKreaches it. Any transaction big enough to spill the page cache and then rolled back corrupts the database.Checklist
non-exclusive, royalty-free, irrevocable copyright license to reproduce, prepare
derivative works of, publicly display, sublicense, and distribute this
Contribution and such derivative works.
Contribution contains no content requiring a license from any third party.