Fix IDBMirrorVFS keeping blocks past the end of a shrunken database - #353
Open
lalexdotcom wants to merge 2 commits into
Open
lalexdotcom wants to merge 2 commits into
lalexdotcom wants to merge 2 commits into
Conversation
Two places keep them. #acceptTx truncates the in-memory view from tx.fileSize + blockSize, but the first block past the end of the file starts at tx.fileSize, so one block survives every shrink; its loop also stops at the first absent block, so scattered blocks past the end survive as well. And #commitTx writes the transaction's blocks to IndexedDB without deleting any, so everything a transaction wrote before shrinking stays in the store. A database grown to 531 pages and then emptied and VACUUMed keeps 531 blocks for the 2 it uses. The database itself stays correct -- reopening reads the right rows and integrity_check passes -- so this costs storage quota rather than data: a database that repeatedly grows and shrinks holds the high-water mark for ever. Deleting from the store is safe here because blocks are not versioned: the key is [path, offset], so a block has one value, and connections learn about changes through the BroadcastChannel, which carries the block data itself. IndexedDB is read on open and written on commit. #getOldestTxInUse governs the tx store, whose records are what a connection needs to catch up, and it is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The database is grown well past the page cache, then emptied and VACUUMed, and the number of blocks IndexedDB holds is compared with PRAGMA page_count. It shrinks the database without rolling anything back, so it reaches the truncation on its own: nothing in it depends on a transaction being undone, and it runs on master as it stands. Against the previous behaviour it fails with 531 blocks kept for a 2-page database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
A database that grows and then shrinks keeps the blocks it no longer uses. Grown to 531 pages, then emptied and
VACUUMed back to 2, it leaves 531 blocks in IndexedDB for the 2 it uses.Two places keep them.
#acceptTxtruncates the in-memory view fromtx.fileSize + blockSize:The first block past the end of the file starts at
tx.fileSize, not one block beyond it, so one block survives every shrink. And becauseMap.deletereturnsfalseon a missing key, the loop stops at the first absent block, so blocks past the end that are not contiguous survive too.#commitTxwrites the transaction's blocks to IndexedDB and deletes none, so everything a transaction wrote before shrinking stays in the store.The database stays correct: reopening reads the right rows,
PRAGMA integrity_checkpasses,page_countis right. What it costs is storage — a database that repeatedly grows and shrinks holds its high-water mark for ever.Why deleting from the store is safe
Blocks are not versioned. The key is
[path, offset], so a block has exactly one value, and a connection learns about changes through theBroadcastChannel, which carries the block data itself rather than a pointer into IndexedDB. The store is read on open and written on commit.#getOldestTxInUseguards thetxstore, whose records are what lets a connection catch up, and it is untouched here.The change
#acceptTxtruncates from the first block that is actually past the end, and walks the keys instead of stopping at the first gap.#commitTxdeletes the range past the end of the file, in the transaction it already has open.Test
test/vfs_leak.js, wired intotest/IDBMirrorVFS.test.js: grow the database well past the page cache, empty it,VACUUM, and compare the number of blocks IndexedDB holds withPRAGMA page_count.It shrinks the database without rolling anything back, which is what lets it run on
masterunchanged — a rollback reaches the same truncation, but through a path where the database does not survive (that is #352, and this PR does not depend on it).Against
masterit fails with 531 blocks kept for a 2-page database. With the change, the file's tests pass and the full suite is 13 files, 0 failures.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.