From 2b4e720e36a3b6e3bdcee41cfa57f35305b0c107 Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Fri, 19 Jun 2026 15:29:51 -0700 Subject: [PATCH] fix: stop temp_dirs masking startup errors; fail-fast on unresolvable SQLite path (#13634) (#13729) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Develop/configuration-custom-database.mdx | 6 + src/backend/base/langflow/main.py | 6 + .../langflow/services/database/service.py | 61 +++++++- .../base/langflow/services/database/utils.py | 7 + .../database/test_sqlite_path_validation.py | 140 ++++++++++++++++++ .../unit/test_lifespan_startup_cleanup.py | 57 +++++++ 6 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 src/backend/tests/unit/services/database/test_sqlite_path_validation.py create mode 100644 src/backend/tests/unit/test_lifespan_startup_cleanup.py diff --git a/docs/docs/Develop/configuration-custom-database.mdx b/docs/docs/Develop/configuration-custom-database.mdx index 2b00e19933..d03f8833df 100644 --- a/docs/docs/Develop/configuration-custom-database.mdx +++ b/docs/docs/Develop/configuration-custom-database.mdx @@ -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. diff --git a/src/backend/base/langflow/main.py b/src/backend/base/langflow/main.py index 2672e57cc8..5da90b35e4 100644 --- a/src/backend/base/langflow/main.py +++ b/src/backend/base/langflow/main.py @@ -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() diff --git a/src/backend/base/langflow/services/database/service.py b/src/backend/base/langflow/services/database/service.py index 685f27e9db..dc2a961cb5 100644 --- a/src/backend/base/langflow/services/database/service.py +++ b/src/backend/base/langflow/services/database/service.py @@ -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" diff --git a/src/backend/base/langflow/services/database/utils.py b/src/backend/base/langflow/services/database/utils.py index 9fce80c016..7f4b2435d0 100644 --- a/src/backend/base/langflow/services/database/utils.py +++ b/src/backend/base/langflow/services/database/utils.py @@ -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() diff --git a/src/backend/tests/unit/services/database/test_sqlite_path_validation.py b/src/backend/tests/unit/services/database/test_sqlite_path_validation.py new file mode 100644 index 0000000000..4ab52a51ff --- /dev/null +++ b/src/backend/tests/unit/services/database/test_sqlite_path_validation.py @@ -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) diff --git a/src/backend/tests/unit/test_lifespan_startup_cleanup.py b/src/backend/tests/unit/test_lifespan_startup_cleanup.py new file mode 100644 index 0000000000..823e2747a3 --- /dev/null +++ b/src/backend/tests/unit/test_lifespan_startup_cleanup.py @@ -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