Fix OPFSCoopSyncVFS access handle leak on a failed acquisition - #350
Open
lalexdotcom wants to merge 2 commits into
Open
lalexdotcom wants to merge 2 commits into
lalexdotcom wants to merge 2 commits into
Conversation
#requestAccessHandle acquires the database file and its sidecars in parallel with Promise.all, which rejects as soon as one acquisition does, while the others are still in flight. The catch then calls #releaseAccessHandle, which finds those persistent files carrying no access handle yet and closes nothing; the in-flight acquisitions complete afterwards and assign handles that nothing will ever close. So one access handle leaks per failed attempt, and it leaks on the sidecars rather than on the file that failed — after which the VFS blocks itself: every later open of the same database fails on -journal, held by the very instance asking for it. Observed on Chromium with a dead worker briefly holding the main file. The file was free again 78 ms in, yet 25 successive opens over 2.5 s all failed, every one of them on -journal. Promise.allSettled lets every acquisition finish before the failure is reported, so the cleanup in the catch below sees the handles that succeeded and closes them — which is what its comment already says it does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A worker holds an exclusive access handle on the database file, so the open fails - which is expected, and is what a connection elsewhere or a worker whose handles the engine has not reclaimed yet would cause. The holder then closes it and the same VFS instance opens the database again, which has to succeed. Against the previous behaviour the second open failed too, on a sidecar file the instance was holding itself, and went on failing for the life of the instance. The holder runs in a worker of its own because createSyncAccessHandle is not available on the main thread, and the test is skipped where the engine grants a second handle on the same file, since nothing can be held from another context there. 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
#requestAccessHandleacquires the access handles for the database file and its sidecars ('',-journal,-wal) in parallel withPromise.all. When one acquisition fails,Promise.allrejects immediately — while the others are still in flight.The
catchthen calls#releaseAccessHandle, whose job is stated by its own comment: "Close any of the potentially opened access handles". But at that moment the in-flight acquisitions have not assigned anything yet, so it findsaccessHandlestill null on those persistent files and closes nothing. They complete a moment later and assign handles that nothing holds a reference to any more: the next attempt calls#createPersistentFile, which replaces the entries inpersistentFiles.So one access handle leaks per failed attempt — and it leaks on the sidecars, not on the file that actually failed. Once a single acquisition has failed, the VFS instance blocks itself: every later
sqlite3_open_v2on that database fails on-journal, held by the very instance asking for it. The failure outlives whatever caused the first one, for the life of the instance.The first failure needs no bug to happen — another connection holding the file, or a worker terminated a moment ago whose handles the engine has not reclaimed yet. Failing then is correct. Never recovering is not.
Reproducing
A self-contained reproduction — Playwright, run from the root of a wa-sqlite checkout (
npm i playwright,node coopsync-access-handle-leak.mjs 3), no build needed sincedist/is committed — does this:/demo(createSyncAccessHandle()with nomode).OPFSCoopSyncVFSand opensdemo. It fails — expected, and not the subject.demoagain.demo,demo-journal,demo-walcan be acquired right now.On
master, 3 runs of 3:demois free at step 5 — the holder released it — whiledemo-journalanddemo-walare not, and the only thing alive that can be holding them is the VFS of step 2. With this change, 3 runs of 3:The change
Promise.allSettledinstead ofPromise.all, then rethrow the first rejection. Every acquisition has finished by the time the failure is reported, so the cleanup in thecatchsees the handles that succeeded and closes them.Nothing else moves: same behaviour on success, the original error still propagates (
failure.reason), samefinally. Only the moment of the rejection changes.Test
test/vfs_handle_recovery.js, wired intotest/OPFSCoopSyncVFS.test.js. A worker holds an exclusive handle on the database file, the open fails, the holder releases, and the same VFS instance must then open it. It is skipped where the engine grants a second handle on the same file, since nothing can be held from another context there.Against
masterit fails on thedefaultandasyncifybuilds withError: sqlite3_open_v2; with this change the file's 70 tests pass.Scope
This does not make a blocked acquisition succeed — a file held elsewhere is a legitimate failure, and the caller is expected to retry. It makes that failure recoverable, which today it is not.
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.