Skip to content

Revert "fix SQLAlchemy compatibility issues on macOS"#8638

Merged
Soulter merged 3 commits into
masterfrom
revert-7724-hibiki233i/Fix-SQLAlchemy-compatibility-on-macOS
Jun 7, 2026
Merged

Revert "fix SQLAlchemy compatibility issues on macOS"#8638
Soulter merged 3 commits into
masterfrom
revert-7724-hibiki233i/Fix-SQLAlchemy-compatibility-on-macOS

Conversation

@Soulter
Copy link
Copy Markdown
Member

@Soulter Soulter commented Jun 7, 2026

Reverts #7724

Summary by Sourcery

Revert recent SQLite/SQLAlchemy compatibility changes and restore previous database configuration and behavior.

Enhancements:

  • Restore SQLModel-based table creation for document storage initialization.
  • Revert document ID uniqueness constraints on the documents table to previous behavior.
  • Simplify async engine creation by removing SQLite-specific NullPool and connection PRAGMA configuration.
  • Adjust knowledge base SQLite engine configuration to use pooled connections with pre-ping and recycle settings.
  • Restore previous site-packages path handling in main startup logic.

Tests:

  • Remove regression test that enforced unique doc_id index creation on existing document tables.

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend feature:knowledge-base The bug / feature is about knowledge base labels Jun 7, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request simplifies database initialization and configuration by removing custom SQLite connection listeners, WAL mode configurations, and unique constraints on document IDs. However, several critical issues were identified in the review: removing the unique=True constraint on doc_id will cause application crashes due to MultipleResultsFound exceptions in queries using scalar_one_or_none(); disabling Write-Ahead Logging (WAL) mode for SQLite is highly likely to cause database is locked errors in this asynchronous environment; and removing the connection event listener prevents performance-optimizing PRAGMAs from being applied to subsequent connections in the pool.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

sa_column_kwargs={"autoincrement": True},
)
doc_id: str = Field(nullable=False, unique=True)
doc_id: str = Field(nullable=False)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Removing unique=True from doc_id allows duplicate doc_id values to be inserted into the database. However, get_document_by_doc_id, update_document_by_doc_id, and delete_document_by_doc_id all use result.scalar_one_or_none(). If duplicate doc_ids exist, these queries will raise sqlalchemy.exc.MultipleResultsFound and crash the application. If doc_id is not unique, these queries must be updated to use .first() or handle multiple results. Otherwise, unique=True should be retained.

Suggested change
doc_id: str = Field(nullable=False)
doc_id: str = Field(nullable=False, unique=True)

Comment thread astrbot/core/db/__init__.py
Comment thread astrbot/core/knowledge_base/kb_db_sqlite.py
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • The change from make_url(...).get_backend_name() to checking 'sqlite' in self.DATABASE_URL risks misdetecting non-SQLite URLs; consider keeping URL parsing or a more robust backend check when setting connect_args.
  • Removing the doc_id unique constraint and the index migration helpers will allow duplicate doc_id values again; if higher-level logic assumes doc_id uniqueness, consider preserving the unique index while still reverting the macOS-specific changes.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The change from `make_url(...).get_backend_name()` to checking `'sqlite' in self.DATABASE_URL` risks misdetecting non-SQLite URLs; consider keeping URL parsing or a more robust backend check when setting `connect_args`.
- Removing the `doc_id` unique constraint and the index migration helpers will allow duplicate `doc_id` values again; if higher-level logic assumes `doc_id` uniqueness, consider preserving the unique index while still reverting the macOS-specific changes.

## Individual Comments

### Comment 1
<location path="main.py" line_range="56-55" />
<code_context>
         sys.path.insert(0, astrbot_root)

     site_packages_path = get_astrbot_site_packages_path()
-    if not is_packaged_desktop_runtime() and site_packages_path not in sys.path:
-        # Packaged desktop runtime keeps shared plugin dependencies out of the
-        # global import path so bundled core libraries don't mix with user-
-        # installed wheels from ~/.astrbot/data/site-packages.
+    if site_packages_path not in sys.path:
         sys.path.append(site_packages_path)

</code_context>
<issue_to_address>
**issue (bug_risk):** Always adding `site_packages_path` may break packaged desktop runtime isolation.

The previous `is_packaged_desktop_runtime()` guard intentionally kept `site_packages_path` off the global import path in packaged desktop builds to prevent bundled core libs from mixing with user-installed wheels. Dropping this guard changes that contract and may reintroduce import/version conflicts in packaged environments. If this behavior change is desired, please confirm its impact on desktop packaging; otherwise, consider restoring a runtime-specific condition.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread main.py
@Soulter Soulter merged commit c70a192 into master Jun 7, 2026
19 of 20 checks passed
@Soulter Soulter deleted the revert-7724-hibiki233i/Fix-SQLAlchemy-compatibility-on-macOS branch June 7, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend feature:knowledge-base The bug / feature is about knowledge base size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant