continue
continue copied to clipboard
fix sqlite locking during index clearing
Description
When doing a force reindex, sqlite gets locked and the associated sqlite file does not get deleted as expected.
Added corresponding fix and tests to ensure this case is handled.
reference: https://discord.com/channels/1108621136150929458/1364504237677088809/1364504237677088809
Checklist
- [] I've read the contributing guide
- [] The relevant docs, if any, have been updated or created
- [] The relevant tests, if any, have been updated or created
Screenshots
[ For visual changes, include screenshots. Screen recordings are particularly helpful, and appreciated! ]
Testing instructions
[ For new or modified features, provide step-by-step testing instructions to validate the intended behavior of the change, including any relevant tests to run. ]
Deploy Preview for continuedev ready!
| Name | Link |
|---|---|
| Latest commit | a4f05d13d6438a50467b1c050f60c2c655b06fa1 |
| Latest deploy log | https://app.netlify.com/projects/continuedev/deploys/682571ecb0a7bf0008a47630 |
| Deploy Preview | https://deploy-preview-5403--continuedev.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify project configuration.
Hmmm. Tests are running fine locally... Let me try some changes.
hmmm, tricky part with SQLite3
it does not close the intended transaction even on explicit commit/rollback. need to dive deeper. best guess is that tests are making another different sqlite connection to the same db file.
@uinstinct still hoping to get this one merged?
Hey @Patrick-Erichsen
I dove deeper. I think we will need to replace sqlite3 package with better-sqlite3 which would be able to accomplish closing connections due to its synchronous nature (and it is also faster than the sqlite3 package).
We have a few instances of sqlite so I don't anticipate any breaking changes. If the team wants to replace with this new package, I can start drafting a PR.
Hmm, were on a bit of a stability push right now so I'd be weary to shake things up too much by swapping out our sqlite lib.
Mind sharing how you came to the conclusion that we need to do that?
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
Hmm, were on a bit of a stability push right now so I'd be weary to shake things up too much by swapping out our sqlite lib.
Agree. Right now stability is important.
Mind sharing how you came to the conclusion that we need to do that?
Sure. The main problem right now is on closing connections. Sqlite is performing another transaction and hence is unable to close the connection. Had this been synchronous, we would be sure that the previous sqlite transaction was complete. Due to node-sqlite's asynchronous nature the connection seems to perform operations while we are closing it.
small history how can i came to this conclusion
The main problem that we are getting during testing (in both local and actions) is that connections are not closing after tests. This is could be due to: a) unfinalized statements b) memory leak issue I inspected the first case multiple times to check for such statements and we are finalizing all prepare statements. We are even committing/rolling back unfinished transactions. The second case is some operation is underway while we are closing it. This could be due to multiple reasons like jest not resetting modules properly between tests or incomplete test isolation or statements that are persisting across tests. Upon searching ways to avoid this, I came across better-sqlite3.
I can give better-sqlite3 a try in a Proof-of-Concept.
I have read the CLA Document and I hereby sign the CLA
✨ No issues found! Your code is sparkling clean! ✨