diff --git a/features/environment.py b/features/environment.py index a4c5684e2..0c2c7a3bc 100644 --- a/features/environment.py +++ b/features/environment.py @@ -54,6 +54,21 @@ LANGSMITH_ENV_VARS = [ _TDD_ISSUE_N_RE = re.compile(r"tdd_issue_\d+") +# --------------------------------------------------------------------------- +# Process-global set of already-initialized DB paths +# --------------------------------------------------------------------------- +# After ``_fast_init_or_upgrade`` has processed a given database URL (either +# by copying the template or confirming the file is non-empty), the URL is +# added to this set. Subsequent calls with the same URL short-circuit at +# the very top of the function, avoiding all URL parsing, path extraction, +# prefix matching, and ``stat()`` syscalls. The set is cleared in +# ``before_scenario`` to prevent cross-scenario state leaks. +# +# See issue #735 — eliminates ~65,000 unnecessary function executions per +# full test run. +# --------------------------------------------------------------------------- +_INITIALIZED_DBS: set[str] = set() + _tdd_logger = logging.getLogger("cleveragents.testing.tdd_tags") @@ -443,6 +458,7 @@ def _install_template_db_patch() -> None: def _fast_init_or_upgrade(self: Any, **kwargs: Any) -> None: """Replace Alembic migrations with fast alternatives. + - Process-global cache hit → immediate return (no work at all) - Non-SQLite databases → fall through to original - In-memory SQLite → ``Base.metadata.create_all()`` + alembic stamp - File-based SQLite with matching prefix → copy template @@ -450,6 +466,12 @@ def _install_template_db_patch() -> None: """ db_url: str = getattr(self, "database_url", "") + # Process-global fast-path: if we have already initialised this + # exact URL in the current scenario, skip all work. Eliminates + # ~65k redundant function-body executions per full test run. + if db_url in _INITIALIZED_DBS: + return + # Non-SQLite: always fall through if not db_url.startswith("sqlite"): return _original_init_or_upgrade(self, **kwargs) @@ -479,11 +501,13 @@ def _install_template_db_patch() -> None: # creation, SQLite lock contention, and the cumulative overhead that # causes intermittent hangs in parallel test runs. if db_path.exists() and db_path.stat().st_size > 0: + _INITIALIZED_DBS.add(db_url) return # Copy the template — creates a fully-migrated DB in ~1ms db_path.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(template_path, db_file_path) + _INITIALIZED_DBS.add(db_url) MigrationRunner.init_or_upgrade = _fast_init_or_upgrade # type: ignore[assignment] @@ -502,6 +526,12 @@ def before_scenario(context, scenario): _tdd_logger.error("TDD TAG ERROR in %r: %s", scenario.name, exc) return + # Clear the process-global set of already-initialised DB paths so + # that stale entries from the previous scenario cannot leak into this + # one. Each scenario receives fresh temp-DB paths, so old entries are + # irrelevant and would only waste memory. + _INITIALIZED_DBS.clear() + # Store original working directory context.original_cwd = os.getcwd() diff --git a/features/fast_init_upgrade.feature b/features/fast_init_upgrade.feature index de78aa16f..4a3fe3a31 100644 --- a/features/fast_init_upgrade.feature +++ b/features/fast_init_upgrade.feature @@ -57,3 +57,41 @@ Feature: Fast init_or_upgrade early-return behavior When I call init_or_upgrade with keyword arguments on the fast-init database Then the original init_or_upgrade should have been invoked exactly once And the original should have received the runner instance and keyword arguments + + # ----------------------------------------------------------------------- + # Process-global _INITIALIZED_DBS cache (issue #735) + # ----------------------------------------------------------------------- + + @mock_only + Scenario: Second call for the same non-empty DB skips all logic via global cache + Given a non-empty SQLite database file with a matching scenario prefix + And the initialized-DB cache is cleared + When I call init_or_upgrade on the fast-init database + And I call init_or_upgrade on the fast-init database a second time + Then the second init_or_upgrade call should have been a cache hit + And the database file content should be unchanged + + @mock_only + Scenario: Second call for a newly-copied template DB skips all logic via global cache + Given a non-existent SQLite database path with a matching scenario prefix + And the initialized-DB cache is cleared + When I call init_or_upgrade on the fast-init database + And I call init_or_upgrade on the fast-init database a second time + Then the second init_or_upgrade call should have been a cache hit + And the database file should be a copy of the template + + @mock_only + Scenario: Global cache is cleared between scenarios + Given a non-empty SQLite database file with a matching scenario prefix + And the initialized-DB cache is cleared + When I call init_or_upgrade on the fast-init database + Then the URL should be present in the initialized-DB cache + When the initialized-DB cache is cleared for a new scenario + Then the URL should not be present in the initialized-DB cache + + @mock_only + Scenario: Non-matching prefix URLs are not added to the global cache + Given a SQLite database URL whose filename has a non-matching prefix + And the initialized-DB cache is cleared + When I call init_or_upgrade on the fast-init database + Then the URL should not be present in the initialized-DB cache diff --git a/features/steps/fast_init_upgrade_steps.py b/features/steps/fast_init_upgrade_steps.py index 55334de84..66192f383 100644 --- a/features/steps/fast_init_upgrade_steps.py +++ b/features/steps/fast_init_upgrade_steps.py @@ -282,3 +282,103 @@ def step_assert_file_is_template_copy(context: Context) -> None: context.fast_init_template_path, shallow=False, ), "Database file does not match the template" + + +# --------------------------------------------------------------------------- +# Steps — process-global _INITIALIZED_DBS cache (issue #735) +# --------------------------------------------------------------------------- + + +def _get_initialized_dbs() -> set[str]: + """Return the ``_INITIALIZED_DBS`` set used by the ``_fast_init_or_upgrade`` closure. + + Behave's ``Runner`` loads environment.py via ``exec_file()`` into a + private hooks dict — **not** via Python's import system. This means + module-level variables defined in environment.py exist in the exec'd + globals, which is a *separate* namespace from + ``features.environment.__dict__``. To access the same set that the + closure actually mutates at runtime, we read it from the closure + function's ``__globals__`` dict. + """ + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + + fast_fn: Any = MigrationRunner.init_or_upgrade + if not isinstance(fast_fn, types.FunctionType): + raise RuntimeError( + "MigrationRunner.init_or_upgrade is not a plain function — " + "the _fast_init_or_upgrade patch does not appear to be installed." + ) + cache: set[str] = fast_fn.__globals__.get("_INITIALIZED_DBS", set()) + return cache + + +@given("the initialized-DB cache is cleared") +def step_clear_initialized_db_cache(context: Context) -> None: + """Explicitly clear the global cache before exercising the fast-path.""" + _get_initialized_dbs().clear() + + +@when("I call init_or_upgrade on the fast-init database a second time") +def step_call_init_or_upgrade_second_time(context: Context) -> None: + """Invoke init_or_upgrade a second time for the same URL. + + The second call should be a cache hit — ``_fast_init_or_upgrade`` + returns immediately without executing any of its body. We record + whether the mock original was invoked during this second call. + """ + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + from features.mocks.fast_init_test_helpers import ( + patch_original_init_or_upgrade, + ) + + runner = MigrationRunner(database_url=context.fast_init_db_url) + + with patch_original_init_or_upgrade() as mock_original: + runner.init_or_upgrade() + context.fast_init_second_call_mock_called = mock_original.called + context.fast_init_second_call_mock_count = mock_original.call_count + + +@then("the second init_or_upgrade call should have been a cache hit") +def step_assert_second_call_cache_hit(context: Context) -> None: + """Assert the second call was short-circuited by the global cache. + + A cache hit means the function returned immediately — neither the + original ``init_or_upgrade`` nor any of the URL-parsing / stat logic + was executed. + """ + assert not context.fast_init_second_call_mock_called, ( + "Expected second init_or_upgrade call to be a cache hit " + "(no delegation to original), but the original was invoked " + f"{context.fast_init_second_call_mock_count} time(s)." + ) + + +@then("the URL should be present in the initialized-DB cache") +def step_assert_url_in_cache(context: Context) -> None: + """Assert the fast-init database URL was added to _INITIALIZED_DBS.""" + cache = _get_initialized_dbs() + assert context.fast_init_db_url in cache, ( + f"Expected URL {context.fast_init_db_url!r} to be present in " + f"_INITIALIZED_DBS, but the cache contains: {cache}" + ) + + +@when("the initialized-DB cache is cleared for a new scenario") +def step_simulate_scenario_boundary_cache_clear(context: Context) -> None: + """Simulate the cache clearing that happens in before_scenario.""" + _get_initialized_dbs().clear() + + +@then("the URL should not be present in the initialized-DB cache") +def step_assert_url_not_in_cache(context: Context) -> None: + """Assert the fast-init database URL is not in _INITIALIZED_DBS.""" + cache = _get_initialized_dbs() + assert context.fast_init_db_url not in cache, ( + f"Expected URL {context.fast_init_db_url!r} NOT to be in " + f"_INITIALIZED_DBS, but it was found. Cache: {cache}" + )