fix sqlite locking during index clearing

[uinstinct]

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

[netlify]

✅ Deploy Preview for continuedev ready!

Name	Link
🔨 Latest commit	a4f05d1
🔍 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.

[uinstinct]

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

[Patrick-Erichsen]

Let's make sure to remove this before merging 👍

[Patrick-Erichsen]

Thanks for fixing this and adding test coverage!

FWIW, the tests are failing locally for me too:

👾 ❯ npm run test -- CodebaseIndexer.test.ts


> @continuedev/[email protected] test
> cross-env NODE_OPTIONS=--experimental-vm-modules jest -- CodebaseIndexer.test.ts CodebaseIndexer.test.ts

  console.log
    NODE_ENV test

      at util/logger.ts:3:9

{"level":"info","message":"Loading config.yaml from {\"uriType\":\"file\",\"filePath\":\"/Users/patrickerichsen/Git/continue-dev/continue/core/test/.continue-test/config.yaml\"} with root path Users/patrickerichsen/Git/continue-dev/continue/core/test/.continue-test"}
(node:49171) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
  console.log
    Error closing SqliteDb database connection... Retrying (1 / 3)

      at Function.close (indexing/refreshIndex.ts:120:17)

  console.log
    Error closing SqliteDb database connection... Retrying (2 / 3)

      at Function.close (indexing/refreshIndex.ts:120:17)

  console.log
    Error closing SqliteDb database connection... Retrying (3 / 3)

      at Function.close (indexing/refreshIndex.ts:120:17)

  console.error
    Error closing Error: SQLITE_BUSY: unable to close due to unfinalized statements or unfinished backups
        at formatError (/Users/patrickerichsen/Git/continue-dev/continue/core/node_modules/sqlite/src/utils/format-error.ts:7:22)
        at Database.<anonymous> (/Users/patrickerichsen/Git/continue-dev/continue/core/node_modules/sqlite/src/Database.ts:84:36)
        at Database.replacement (/Users/patrickerichsen/Git/continue-dev/continue/core/node_modules/sqlite3/lib/trace.js:25:27) {
      errno: 5,
      code: 'SQLITE_BUSY',
      __augmented: true
    }

      124 |         await SqliteDb.close();
      125 |       } else {
    > 126 |         console.error("Error closing", error);
          |                 ^
      127 |         throw error;
      128 |       }
      129 |     }

      at Function.close (indexing/refreshIndex.ts:126:17)
      at Function.close (indexing/refreshIndex.ts:124:9)
      at Function.close (indexing/refreshIndex.ts:124:9)
      at Function.close (indexing/refreshIndex.ts:124:9)
      at TestCodebaseIndexer.clearIndexes (indexing/CodebaseIndexer.ts:66:7)
      at Object.<anonymous> (indexing/CodebaseIndexer.test.ts:216:5)

  console.error
    Error deleting /Users/patrickerichsen/Git/continue-dev/continue/core/test/.continue-test/index/index.sqlite folder: Error: SQLITE_BUSY: unable to close due to unfinalized statements or unfinished backups

      69 |       await fs.unlink(sqliteFilepath);
      70 |     } catch (error) {
    > 71 |       console.error(`Error deleting ${sqliteFilepath} folder: ${error}`);
         |               ^
      72 |     }
      73 |
      74 |     try {

      at TestCodebaseIndexer.clearIndexes (indexing/CodebaseIndexer.ts:71:15)
      at Object.<anonymous> (indexing/CodebaseIndexer.test.ts:216:5)

Switched to a new branch 'main'
 FAIL  indexing/CodebaseIndexer.test.ts (15.238 s)
  CodebaseIndexer
    ✓ should index test folder without problem (24 ms)
    ✓ should have created index folder with all necessary files
    ✓ should have indexed all of the files (7 ms)
    ✓ should successfuly re-index specific files (1 ms)
    ✓ should successfully re-index after adding a file (2 ms)
    ✓ should successfully re-index after deleting a file (2 ms)
    ✕ should successfully re-index after clearing index (4032 ms)
    ✓ should successfully re-index even if the database was blocked (2007 ms)
    ✓ shouldn't index any files when nothing changed (2 ms)
    ✓ should create git repo for testing (84 ms)
    ○ skipped should only re-index the changed files when changing branches
    ○ skipped shouldn't re-index anything when changing back to original branch

  ● CodebaseIndexer › should successfully re-index after clearing index

    expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: 2

      217 |
      218 |     const afterClearingIndexes = await getAllIndexedFiles();
    > 219 |     expect(afterClearingIndexes.length).toBe(0);
          |                                         ^
      220 |
      221 |     const updates = await refreshIndex();
      222 |

      at Object.<anonymous> (indexing/CodebaseIndexer.test.ts:219:41)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 2 skipped, 9 passed, 12 total
Snapshots:   0 total
Time:        15.255 s

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

[Patrick-Erichsen]

@uinstinct still hoping to get this one merged?

[uinstinct]

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.

[Patrick-Erichsen]

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?

[github-actions]

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

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

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]

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