continue icon indicating copy to clipboard operation
continue copied to clipboard

fix sqlite locking during index clearing

Open uinstinct opened this issue 6 months ago • 10 comments
trafficstars

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. ]

uinstinct avatar Apr 28 '25 16:04 uinstinct

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

netlify[bot] avatar Apr 28 '25 16:04 netlify[bot]

Hmmm. Tests are running fine locally... Let me try some changes.

uinstinct avatar Apr 28 '25 16:04 uinstinct

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 avatar Apr 29 '25 06:04 uinstinct

@uinstinct still hoping to get this one merged?

Patrick-Erichsen avatar May 13 '25 18:05 Patrick-Erichsen

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.

uinstinct avatar May 14 '25 04:05 uinstinct

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?

Patrick-Erichsen avatar May 14 '25 18:05 Patrick-Erichsen

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

github-actions[bot] avatar May 15 '25 04:05 github-actions[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.

uinstinct avatar May 15 '25 05:05 uinstinct

I have read the CLA Document and I hereby sign the CLA

uinstinct avatar May 15 '25 05:05 uinstinct

✨ No issues found! Your code is sparkling clean! ✨

recurseml[bot] avatar Jun 13 '25 21:06 recurseml[bot]