Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(app): recursive cursor errors #7727

Merged
merged 7 commits into from
Mar 3, 2025

Conversation

psychedelicious
Copy link
Collaborator

Summary

In #7724, we enabled WAL mode (which is the suggested way to use SQLite since 2010) and removed the SQLite global mutex. This inadvertently exposed some tech debt/bad practices with SQLite.

Throughout the app, each service would create a single SQLite "cursor" at init and re-use it through the app's lifetime.

The global mutex accidentally prevented an issue where a cursor was used multiple times at once. With it gone, it's possible for a DB operation to fail with an error like this:

[2025-03-03 17:21:14,534]::[InvokeAI]::ERROR --> Non-fatal error in session processor ProgrammingError: Recursive use of cursors not allowed.
[2025-03-03 17:21:14,535]::[InvokeAI]::ERROR --> Traceback (most recent call last):
  File "/opt/invokeai/invokeai/app/services/session_processor/session_processor_default.py", line 448, in _process
    self.session_runner.run(queue_item=self._queue_item)
  File "/opt/invokeai/invokeai/app/services/session_processor/session_processor_default.py", line 109, in run
    self._on_after_run_session(queue_item=queue_item)
  File "/opt/invokeai/invokeai/app/services/session_processor/session_processor_default.py", line 202, in _on_after_run_session
    queue_item = self._services.session_queue.set_queue_item_session(queue_item.item_id, queue_item.session)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/invokeai/invokeai/app/services/session_queue/session_queue_sqlite.py", line 509, in set_queue_item_session
    self.__cursor.execute(
sqlite3.ProgrammingError: Recursive use of cursors not allowed.

Cursors are meant to be lightweight and exist only for short periods of time. This issue is one reason why cursors are not meant to be long-lived entities, shared across a broad scope.

This PR pays off the tech debt by creating a new cursor in all service methods that need a cursor.

It also removes more extraneous class attribute type annotations, which could obscure a runtime issue by telling the type checker that a class attribute exists even if it was never set.

This fixes errors like this:

Related Issues / Discussions

Initially reported by @mickr777 on discord: https://discord.com/channels/1020123559063990373/1346012301056151663

QA Instructions

@mickr777 already confirmed this fixes their issue. I did some smoke tests and do not get any errors, but also I cannot repro it despite following the same steps as outlined in the discord thread.

Merge Plan

n/a

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

SQLite cursors are meant to be lightweight and not reused. For whatever reason, we reuse one per service for the entire app lifecycle.

This can cause issues where a cursor is used twice at the same time in different transactions.

This experiment makes the session queue use a fresh cursor for each method, hopefully fixing the issue.
@github-actions github-actions bot added python PRs that change python files services PRs that change app services labels Mar 3, 2025
@psychedelicious psychedelicious merged commit c2a3c66 into main Mar 3, 2025
15 checks passed
@psychedelicious psychedelicious deleted the psyche/experiment/app/avoid-nested-cursors branch March 3, 2025 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs that change python files services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants