Resolve a write when it commits, not when the request succeeds (#250) - #253
Merged
Conversation
…ucceeds `run()` resolved on `request.onsuccess`. In IndexedDB a request succeeding and its transaction committing are separate events, in that order, so a transaction that aborts at commit time -- quota exhaustion being the realistic cause -- fired `onabort` after the promise had already settled, where `reject()` is a no-op. The comment above it claimed to handle exactly the case it did not. Every write to this database goes through `run()`: `putDocument`, `putFile`, `putBlob`, `deleteDocument`, `deleteFile`, `deleteBlob`. So a quota-aborted save of a preset reported success and lost the data, and because `readJson` in `app-storage.ts` swallows read errors into a fallback, the loss surfaced later as a missing preset rather than as an error at save time. Resolving on `transaction.oncomplete` is the durable signal, which is what `updateDocument` already does. The result now has to be captured in `onsuccess` and handed over at commit, since `request.result` is only valid inside its own handler. Reads share `run()`, so they settle a tick later too; every action it issues is a single request, so no caller is left holding a transaction that has since committed. #249 makes this worth fixing now rather than later: it added a RAW conversion cache of up to 2 GB to the same origin, and it writes through `putBlob`, so it both raises quota pressure and is subject to the same false success. Tests cover the abort paths, which had none. Closes #250 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Closes #250.
The defect
src/lib/storage/kv.ts'srun()resolved onrequest.onsuccess. In IndexedDB a request succeeding and its transaction committing are separate events, in that order, so a transaction that aborts at commit time -- quota exhaustion being the realistic cause -- firedonabortafter the promise had already settled, wherereject()is a no-op.The comment above it claimed to handle exactly the case it did not.
Why it matters
Every write to this database goes through
run():putDocument,putFile,putBlob,deleteDocument,deleteFile,deleteBlob. A quota-aborted save of a preset reported success and lost the data, and becausereadJsoninapp-storage.tsswallows read errors into a fallback, the loss surfaced later as a missing preset rather than as an error at save time.#249 raised the odds of hitting it: it added a RAW conversion cache of up to 2 GB to the same origin, and it writes through
putBlob.The fix
Resolve on
transaction.oncomplete, which is the durable signal, capturingrequest.resultinonsuccesssince it is only valid inside its own handler. This is whatupdateDocumentalready did.Verification
The RED run is the whole argument. Quota cannot be exhausted in
fake-indexeddb, so the tests reproduce the ordering that matters -- the request succeeds, the transaction dies afterwards -- by aborting from a listener on the successful request:...while the companion test asserting the data is gone passed. That pairing is the bug in one screen: the write was rolled back, and the promise resolved with the key anyway.
Four tests in
kv-durable-writes.test.ts. The two that guard already-correct behaviour were validated by deliberately breaking the production code:updateDocumentguard fails as expected whenupdateDocumentresolves at put timerequest.onerrorremoved, so it was renamed to what it actually pins ("rejects rather than hanging") and the finding recorded in a comment rather than left behind a misleading nameReviewed for, and cleared
Moving resolution to
oncompletemeans the transaction is dead when the caller sees the value, so anyrun()action issuing a second request would now getTransactionInactiveError. All ten call sites audited: every action is exactly one request, andrunis module-private and never leaks the store or transaction past its synchronous callback. The one genuinely multi-request function,updateDocument, does not userun().Reads share
run()and so settle a tick later too. Harmless -- a readonly transaction commits right after its last request -- and noted in the code rather than left for a future reader to wonder about.npx jest: 58 suites, 388 tests, all pass.ultracite checkandtsc --noEmitclean.🤖 Generated with Claude Code