Fix resetting isHandleRequested in OPFSCoopSyncVFS

[simolus3]

isHandleRequested is a property on PersistedFile, not the VFS itself.

We've seen this cause an infinite loop when a statement is backed by a SQLite extension / application-defined function using their own statements internally (because a single step call may then cause multiple lock / unlock operations internally). So the unlock call should not give up the access handle right away, because a subsequent lock then returns SQLITE_BUSY, causing us to retry this call (which again unlocks + locks later, repeating this indefinitely).

[gemini-code-assist]

Hello @simolus3, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request addresses an issue in the OPFSCoopSyncVFS where isHandleRequested was incorrectly being reset on the VFS itself instead of the PersistedFile object. This was causing an infinite loop scenario when SQLite extensions or application-defined functions were used, due to multiple lock/unlock operations during a single step call. By correcting the property being reset, the unlock call will not prematurely release the access handle, preventing the subsequent SQLITE_BUSY error and retry loop.

Highlights

• Bug Fix: Corrects the scope of isHandleRequested reset to file.persistentFile.isHandleRequested instead of this.isHandleRequested.
• Performance Improvement: Prevents an infinite loop caused by incorrect handle management during SQLite extension usage.

Changelog

• src/examples/OPFSCoopSyncVFS.js
  • Corrected the scope of isHandleRequested reset from the VFS to the PersistedFile object.
  • Addressed an infinite loop scenario when SQLite extensions or application-defined functions were used.

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

A handle's request,
Misplaced, caused endless test,
Now fixed, loops cease.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request addresses a critical issue in the OPFSCoopSyncVFS where isHandleRequested was incorrectly being reset on the VFS itself instead of the PersistedFile. This could lead to infinite loops in certain scenarios. The fix appears to be correct and well-justified.

Merge Readiness

The code change appears to correctly address the issue described in the pull request description. Given the potential for infinite loops, this fix is important. I recommend merging this pull request after addressing the review comments. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.

[gemini-code-assist]

Good catch! Moving the isHandleRequested reset to the file.persistentFile scope is the correct solution to prevent the infinite loop. This ensures that the state is correctly managed per file and not globally on the VFS.

No code suggestion needed.

[rhashimoto]

Thanks for the PR! I really appreciate the effort it must have taken to track that down.

Can you give me more details about the problem case that can lock, unlock, and lock again in a single call to step()?

[simolus3]

I'm actually not 100% sure what caused that to happen in the end. But we link a SQLite extension into the wasm files, and mostly interact with that using user-defined functions we call with SQL. These functions, e.g. this one set up their own statements to run on the database. So an API-level step call could actually run multiple statements to completion internally in our case.

If I remember correctly, the flow was:

1. We have an internal SQL function that will make multiple writes to the database. On the first one, jLock returns SQLITE_BUSY, our error handling in Rust aborts the whole function and returns that error too.
2. Acquiring the lock is awaited in JS.
3. We call the internal function again, the first write completes because jLock works this time. isLockBusy is set to false.
4. The internal function is done with the first write, so SQLite calls jUnlock which releases the lock immediately because isHandleRequested is true.
5. But then the function continues for a second write, so we run into a SQLITE_BUSY error that resets the whole function, so we'll go into an infinite loop starting at step 2.