mirror of
https://github.com/langflow-ai/langflow.git
synced 2026-07-23 13:55:17 +08:00
fix: fail-fast on unresolvable SQLite path; stop temp_dirs masking startup errors Addresses #13634 (Bug 2 in full; Bug 1 diagnostics + docs — relative-path normalization deferred to a separate design decision per maintainer triage). - main.py: bind `temp_dirs = []` before the lifespan `try` so the shutdown `finally` cleanup never raises UnboundLocalError and masks the real startup error when failure occurs before bundle loading. - database/service.py: add get_sqlite_database_file_path() and check_sqlite_database_path(), surfacing an actionable error (resolved path, CWD anchoring, absolute-path guidance) when a SQLite DB's parent directory is missing — instead of the opaque "Error creating DB and tables". Diagnostics only: no path normalization, no directory creation, no rejection of currently-working relative paths. - database/utils.py: run the check in initialize_database() before creation. - docs: note that SQLite LANGFLOW_DATABASE_URL paths should be absolute. - tests: regression coverage for both bugs. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@ -7,6 +7,12 @@ Langflow's default database is [SQLite](https://www.sqlite.org/docs.html), but y
|
||||
|
||||
This guide walks you through setting up an external database for Langflow by replacing the default SQLite connection string `sqlite:///./langflow.db` with PostgreSQL, both in local and containerized environments.
|
||||
|
||||
:::note SQLite paths should be absolute
|
||||
If you set `LANGFLOW_DATABASE_URL` to a SQLite connection string, use an **absolute** path, such as `sqlite:////absolute/path/to/langflow.db` (note the four leading slashes) or, on Windows, `sqlite:///C:/path/to/langflow.db`.
|
||||
|
||||
A relative path such as `sqlite:///./langflow.db` is resolved against the directory you start Langflow from, so launching from different directories points at different database files (your flows can appear to "disappear"), and a relative path that includes a subdirectory which doesn't yet exist fails at startup because SQLite does not create intermediate directories.
|
||||
:::
|
||||
|
||||
In this configuration, all structured application data from Langflow, including flows, message history, and logs, is instead managed by PostgreSQL.
|
||||
PostgreSQL is better suited for production environments due to its robust support for concurrent users, advanced data integrity features, and scalability.
|
||||
Langflow can more efficiently handle multiple users and larger workloads by using PostgreSQL as the database.
|
||||
|
||||
@ -179,6 +179,12 @@ def get_lifespan(*, fix_migration=False, version=None):
|
||||
sync_flows_from_fs_task = None
|
||||
mcp_init_task = None
|
||||
models_dev_refresh_task = None
|
||||
# Bind ``temp_dirs`` before the ``try`` so the shutdown cleanup in the
|
||||
# ``finally`` block (which iterates it) never raises ``UnboundLocalError``
|
||||
# when startup fails before bundle loading assigns it below. Otherwise an
|
||||
# early failure (e.g. an unresolvable LANGFLOW_DATABASE_URL) is masked by a
|
||||
# secondary error during cleanup. See issue #13634.
|
||||
temp_dirs: list = []
|
||||
|
||||
try:
|
||||
start_time = asyncio.get_event_loop().time()
|
||||
|
||||
@ -19,7 +19,7 @@ from lfx.log.logger import logger
|
||||
from lfx.services.deps import session_scope
|
||||
from sqlalchemy import event, inspect
|
||||
from sqlalchemy.dialects import sqlite as dialect_sqlite
|
||||
from sqlalchemy.engine import Engine
|
||||
from sqlalchemy.engine import Engine, make_url
|
||||
from sqlalchemy.exc import OperationalError
|
||||
from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine
|
||||
from sqlmodel import SQLModel, select, text
|
||||
@ -193,6 +193,65 @@ def check_postgresql_version_sync(database_url: str) -> None:
|
||||
engine.dispose()
|
||||
|
||||
|
||||
def get_sqlite_database_file_path(database_url: str) -> Path | None:
|
||||
"""Return the on-disk file path for a SQLite URL, or ``None`` when there is none.
|
||||
|
||||
Returns ``None`` for non-SQLite URLs and for in-memory SQLite databases
|
||||
(``sqlite://`` and ``sqlite:///:memory:``), which have no file on disk. The
|
||||
returned path is kept exactly as written in the URL (relative paths are *not*
|
||||
resolved) so callers can report it back to the user verbatim.
|
||||
"""
|
||||
if not database_url.startswith("sqlite"):
|
||||
return None
|
||||
try:
|
||||
database = make_url(database_url).database
|
||||
except Exception: # noqa: BLE001 - defensive: malformed URLs are handled elsewhere
|
||||
return None
|
||||
if not database or database == ":memory:":
|
||||
return None
|
||||
return Path(database)
|
||||
|
||||
|
||||
def check_sqlite_database_path(database_url: str) -> None:
|
||||
"""Fail fast with an actionable message when a SQLite database cannot be opened.
|
||||
|
||||
SQLite does not create intermediate directories, and relative paths in
|
||||
``LANGFLOW_DATABASE_URL`` are resolved by SQLAlchemy against the current
|
||||
working directory at connect time. When the resolved parent directory is
|
||||
missing the raw ``sqlite3.OperationalError`` ("unable to open database file")
|
||||
is opaque, so surface where Langflow actually tried to open the database and
|
||||
how a relative path was resolved. No-op for non-SQLite and in-memory URLs.
|
||||
|
||||
Note: this only improves diagnostics; it does not change which URLs are
|
||||
accepted nor create any directories. See issue #13634.
|
||||
"""
|
||||
db_path = get_sqlite_database_file_path(database_url)
|
||||
if db_path is None:
|
||||
return
|
||||
|
||||
resolved = db_path.resolve()
|
||||
logger.debug(f"Using SQLite database at {resolved}")
|
||||
|
||||
parent = resolved.parent
|
||||
if parent.exists():
|
||||
return
|
||||
|
||||
msg = (
|
||||
f"Cannot open the SQLite database at '{resolved}': the parent directory "
|
||||
f"'{parent}' does not exist, and SQLite does not create intermediate "
|
||||
f"directories. "
|
||||
)
|
||||
if db_path.is_absolute():
|
||||
msg += "Create the directory before starting Langflow, or point LANGFLOW_DATABASE_URL at an existing path."
|
||||
else:
|
||||
msg += (
|
||||
f"The relative path '{db_path}' from LANGFLOW_DATABASE_URL was resolved against the current working "
|
||||
f"directory ('{Path.cwd()}'). Set LANGFLOW_DATABASE_URL to an absolute path "
|
||||
f"(e.g. 'sqlite:///{resolved}'), or create the directory before starting Langflow."
|
||||
)
|
||||
raise ValueError(msg)
|
||||
|
||||
|
||||
class DatabaseService(Service):
|
||||
name = "database_service"
|
||||
|
||||
|
||||
@ -20,6 +20,13 @@ async def initialize_database(*, fix_migration: bool = False) -> None:
|
||||
|
||||
database_service: DatabaseService = get_db_service()
|
||||
await database_service.ensure_postgresql_version()
|
||||
# Fail fast with a clear, actionable message when a SQLite path cannot be
|
||||
# opened (e.g. a relative LANGFLOW_DATABASE_URL whose parent directory does
|
||||
# not exist), instead of the opaque "Error creating DB and tables" below.
|
||||
# See issue #13634.
|
||||
from langflow.services.database.service import check_sqlite_database_path
|
||||
|
||||
check_sqlite_database_path(database_service.database_url)
|
||||
try:
|
||||
if database_service.settings_service.settings.database_connection_retry:
|
||||
await database_service.create_db_and_tables_with_retry()
|
||||
|
||||
@ -0,0 +1,140 @@
|
||||
"""Tests for the SQLite database-path diagnostics.
|
||||
|
||||
Regression coverage for issue #13634 (Bug 1, diagnostics half): a relative
|
||||
``LANGFLOW_DATABASE_URL`` pointing at a missing subdirectory used to crash with
|
||||
an opaque ``RuntimeError: Error creating DB and tables``. ``check_sqlite_database_path``
|
||||
fails fast with an actionable message instead. These helpers only improve
|
||||
diagnostics -- they never create directories nor change which URLs are accepted.
|
||||
|
||||
Issue: https://github.com/langflow-ai/langflow/issues/13634
|
||||
"""
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
from langflow.services.database.service import (
|
||||
check_sqlite_database_path,
|
||||
get_sqlite_database_file_path,
|
||||
)
|
||||
|
||||
|
||||
class TestGetSqliteDatabaseFilePath:
|
||||
"""``get_sqlite_database_file_path`` extracts the on-disk path, or ``None``."""
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"url",
|
||||
[
|
||||
"postgresql+psycopg://localhost:5432/langflow",
|
||||
"postgresql://localhost/langflow",
|
||||
"mysql://localhost/langflow",
|
||||
],
|
||||
)
|
||||
def test_non_sqlite_urls_return_none(self, url):
|
||||
assert get_sqlite_database_file_path(url) is None
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"url",
|
||||
[
|
||||
"sqlite://", # default in-memory
|
||||
"sqlite+aiosqlite://",
|
||||
"sqlite:///:memory:",
|
||||
"sqlite+aiosqlite:///:memory:",
|
||||
],
|
||||
)
|
||||
def test_in_memory_urls_return_none(self, url):
|
||||
assert get_sqlite_database_file_path(url) is None
|
||||
|
||||
def test_relative_path_is_returned_verbatim(self):
|
||||
# The path is intentionally NOT resolved here so callers can echo it back.
|
||||
assert get_sqlite_database_file_path("sqlite:///db/langflow.db") == Path("db/langflow.db")
|
||||
|
||||
def test_relative_dot_path_is_returned_verbatim(self):
|
||||
assert get_sqlite_database_file_path("sqlite:///./langflow.db") == Path("./langflow.db")
|
||||
|
||||
def test_absolute_path_is_returned(self):
|
||||
assert get_sqlite_database_file_path("sqlite:////var/data/langflow.db") == Path("/var/data/langflow.db")
|
||||
|
||||
def test_sanitized_async_driver_is_recognized(self):
|
||||
# ``_sanitize_database_url`` rewrites ``sqlite`` -> ``sqlite+aiosqlite``.
|
||||
assert get_sqlite_database_file_path("sqlite+aiosqlite:///db/langflow.db") == Path("db/langflow.db")
|
||||
|
||||
|
||||
class TestCheckSqliteDatabasePath:
|
||||
"""``check_sqlite_database_path`` is a no-op unless a SQLite parent dir is missing."""
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"url",
|
||||
[
|
||||
"postgresql+psycopg://localhost:5432/langflow",
|
||||
"sqlite://",
|
||||
"sqlite:///:memory:",
|
||||
],
|
||||
)
|
||||
def test_no_raise_for_non_file_urls(self, url):
|
||||
# Should not raise even though e.g. the postgres "database" name has no
|
||||
# parent directory on disk.
|
||||
check_sqlite_database_path(url)
|
||||
|
||||
def test_no_raise_when_absolute_parent_exists(self, tmp_path):
|
||||
url = f"sqlite:///{tmp_path / 'langflow.db'}"
|
||||
check_sqlite_database_path(url)
|
||||
|
||||
def test_no_raise_for_relative_existing_parent(self, tmp_path, monkeypatch):
|
||||
# The documented default ``sqlite:///./langflow.db`` resolves its parent
|
||||
# to the CWD, which always exists -- it must keep working.
|
||||
monkeypatch.chdir(tmp_path)
|
||||
check_sqlite_database_path("sqlite:///./langflow.db")
|
||||
|
||||
def test_raises_for_absolute_missing_parent(self, tmp_path):
|
||||
missing = tmp_path / "does_not_exist" / "langflow.db"
|
||||
url = f"sqlite:///{missing}"
|
||||
with pytest.raises(ValueError, match="parent directory") as exc_info:
|
||||
check_sqlite_database_path(url)
|
||||
message = str(exc_info.value)
|
||||
assert str(missing) in message
|
||||
assert "Create the directory" in message
|
||||
# The absolute branch must not talk about the working directory.
|
||||
assert "working directory" not in message
|
||||
|
||||
def test_raises_for_relative_missing_subdirectory(self, tmp_path, monkeypatch):
|
||||
monkeypatch.chdir(tmp_path)
|
||||
with pytest.raises(ValueError, match="parent directory") as exc_info:
|
||||
check_sqlite_database_path("sqlite:///db/langflow.db")
|
||||
message = str(exc_info.value)
|
||||
# The message must point at the resolved location and explain CWD anchoring.
|
||||
assert str(tmp_path / "db") in message
|
||||
assert "working directory" in message
|
||||
assert str(tmp_path) in message
|
||||
assert "absolute" in message
|
||||
|
||||
def test_does_not_create_directories(self, tmp_path, monkeypatch):
|
||||
monkeypatch.chdir(tmp_path)
|
||||
with pytest.raises(ValueError): # noqa: PT011 - message asserted elsewhere
|
||||
check_sqlite_database_path("sqlite:///db/langflow.db")
|
||||
# The diagnostic must be side-effect free.
|
||||
assert not (tmp_path / "db").exists()
|
||||
|
||||
|
||||
class TestInitializeDatabaseWiring:
|
||||
"""``initialize_database`` runs the diagnostic before attempting creation."""
|
||||
|
||||
async def test_relative_missing_dir_fails_fast_instead_of_opaque_error(self, tmp_path, monkeypatch):
|
||||
from langflow.services.database import utils as db_utils
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
class _StubDatabaseService:
|
||||
database_url = "sqlite+aiosqlite:///db/langflow.db"
|
||||
|
||||
async def ensure_postgresql_version(self):
|
||||
return None
|
||||
|
||||
async def create_db_and_tables(self): # pragma: no cover - must not be reached
|
||||
pytest.fail("create_db_and_tables should not run when the path is invalid")
|
||||
|
||||
monkeypatch.setattr("langflow.services.deps.get_db_service", lambda: _StubDatabaseService())
|
||||
|
||||
# Fails fast with the clear diagnostic, not the opaque "Error creating DB and tables".
|
||||
with pytest.raises(ValueError, match="parent directory") as exc_info:
|
||||
await db_utils.initialize_database()
|
||||
assert "Error creating DB and tables" not in str(exc_info.value)
|
||||
57
src/backend/tests/unit/test_lifespan_startup_cleanup.py
Normal file
57
src/backend/tests/unit/test_lifespan_startup_cleanup.py
Normal file
@ -0,0 +1,57 @@
|
||||
"""Regression test for issue #13634 (Bug 2): the masking ``UnboundLocalError``.
|
||||
|
||||
When startup fails before bundle loading assigns ``temp_dirs``, the shutdown
|
||||
``finally`` block in ``lifespan`` iterates ``temp_dirs`` during temp-file cleanup.
|
||||
Previously ``temp_dirs`` was only bound inside the ``try``, so an early failure
|
||||
(such as an unresolvable ``LANGFLOW_DATABASE_URL``) caused the cleanup to raise
|
||||
``UnboundLocalError: cannot access local variable 'temp_dirs'`` -- a second,
|
||||
confusing error logged on top of the real one.
|
||||
|
||||
Binding ``temp_dirs = []`` before the ``try`` fixes this. This test drives the
|
||||
real ``lifespan`` context manager with a failing ``initialize_services`` and
|
||||
asserts the cleanup path no longer raises.
|
||||
|
||||
Issue: https://github.com/langflow-ai/langflow/issues/13634
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import langflow.main as main_module
|
||||
import pytest
|
||||
|
||||
|
||||
async def test_startup_failure_does_not_mask_error_with_unbound_temp_dirs(monkeypatch):
|
||||
lifespan = main_module.get_lifespan()
|
||||
|
||||
sentinel = RuntimeError("Error creating DB and tables")
|
||||
|
||||
async def _failing_initialize_services(*_args, **_kwargs):
|
||||
raise sentinel
|
||||
|
||||
telemetry_calls: list[tuple[str, str]] = []
|
||||
|
||||
async def _record_telemetry(exc, context):
|
||||
telemetry_calls.append((context, type(exc).__name__))
|
||||
|
||||
# Force an early startup failure (before bundle loading binds temp_dirs).
|
||||
monkeypatch.setattr(main_module, "initialize_services", _failing_initialize_services)
|
||||
# Capture both the primary failure and any secondary cleanup failure.
|
||||
monkeypatch.setattr(main_module, "log_exception_to_telemetry", _record_telemetry)
|
||||
# Replace destructive/heavy shutdown calls so the finally block runs in
|
||||
# isolation without tearing down services shared by the wider test session.
|
||||
monkeypatch.setattr(main_module, "teardown_services", AsyncMock())
|
||||
monkeypatch.setattr(main_module, "cleanup_mcp_sessions", AsyncMock())
|
||||
|
||||
with pytest.raises(RuntimeError, match="Error creating DB and tables") as exc_info:
|
||||
async with lifespan(object()):
|
||||
pass
|
||||
|
||||
# The real startup error propagates unchanged...
|
||||
assert exc_info.value is sentinel
|
||||
|
||||
# ...and the shutdown cleanup itself did not raise. A crash inside the
|
||||
# ``finally`` block is reported via ``log_exception_to_telemetry`` under the
|
||||
# "lifespan_cleanup" context, so its absence proves temp_dirs was bound.
|
||||
cleanup_failures = [context for context, _ in telemetry_calls if context == "lifespan_cleanup"]
|
||||
assert cleanup_failures == [], f"shutdown cleanup raised during startup failure: {telemetry_calls}"
|
||||
assert ("lifespan_cleanup", "UnboundLocalError") not in telemetry_calls
|
||||
Reference in New Issue
Block a user